create custom forward request authenticator #949

vladr11 · 2022-04-04T08:48:52Z

Adds a new authenticator which forwards the requests to an upstream service to perform the authentication. This adds the possibility to authenticate a request where the request body is needed.

Related issue(s)

Related to issue #841

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

CLAassistant · 2022-04-04T08:52:05Z

All committers have signed the CLA.

codecov · 2022-04-04T13:53:04Z

Codecov Report

Attention: Patch coverage is 60.82474% with 38 lines in your changes missing coverage. Please review.

Project coverage is 77.57%. Comparing base (6e3844e) to head (53ead8f).
Report is 156 commits behind head on master.

Files with missing lines	Patch %	Lines
pipeline/authn/authenticator_remote_json.go	60.41%	29 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
- Coverage   78.05%   77.57%   -0.49%     
==========================================
  Files          83       84       +1     
  Lines        3992     4089      +97     
==========================================
+ Hits         3116     3172      +56     
- Misses        597      628      +31     
- Partials      279      289      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aeneasr

Not sure if you have done that already, but could you also please create a PR against ory/docs to document this new functionality? :)

aeneasr · 2022-04-11T22:37:31Z

.schema/config.schema.json

@@ -1459,6 +1504,38 @@
            }
          ]
        },
+        "forward": {


I would prefer calling this remote_json similar to the authorizer, as we expect a JSON response from the upstream! :)

Renamed the authenticator into remote_json

vladr11 · 2022-05-24T15:39:06Z

Not sure if you have done that already, but could you also please create a PR against ory/docs to document this new functionality? :)

Created a new PR into ory/docs: ory/docs#827

vladr11 · 2022-06-30T11:03:26Z

A polite reminder here, as the change requests have been addressed 😄

Fixed linting issues, as well.

jendrikjoe · 2022-09-01T15:31:58Z

A polite ping here again 😉

aeneasr · 2022-09-10T08:55:10Z

@hperl could you please help review this one? :)

hperl

Thanks for the PR. It looks very good already, I left just a few comments :)

hperl · 2022-09-14T12:14:24Z

pipeline/authn/authenticator_remote_json.go

+	)
+
+	if err = json.Unmarshal(subjectRaw, &subject); err != nil {
+		return helper.ErrForbidden.WithReasonf("The configured subject_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.SubjectFrom, body, subjectRaw).WithTrace(err)


Minor: Please break long lines into multiple lines, e.g., after the dots.

hperl · 2022-09-14T12:18:22Z

pipeline/authn/authenticator_remote_json.go

+		// Unfortunately the body reader needs to be read once to forward the request,
+		// thus the upstream request will fail miserably without recreating a fresh ReaderCloser
+		forwardRequestBody = ioutil.NopCloser(bytes.NewReader(body))
+		r.Body = ioutil.NopCloser(bytes.NewReader(body))


I think by reassigning r.Body you will leak the original body, which will not be closed. Usually, the HTTP server takes care of closing the body after the handler is done, but in this case we are replacing the body with a NopCloser and are leaving the original r.Body unclosed. I think it should be fine to manually Close() the original body here, but I'd feel more comfortable having a test for this.

Added a manual close to the original body stream. Not sure how to add a specific test for it.

hperl · 2022-09-14T12:27:31Z

pipeline/authn/authenticator_remote_json.go

+	if len(c.ExtraFrom) == 0 {
+		c.ExtraFrom = "extra"
+	}
+
+	if len(c.SubjectFrom) == 0 {
+		c.SubjectFrom = "subject"
+	}
+


Are these checks really necessary, given that you already set the default values in the JSON schema?

Removed unnecessary checks

hperl · 2022-09-14T12:31:47Z

pipeline/authn/authenticator_remote_json.go

+
+	var forwardRequestBody io.ReadCloser = nil
+	if r.Body != nil {
+		body, err := ioutil.ReadAll(r.Body)


ioutil is deprecated. Please replace with calls from the io package. (Here and elsewhere). Thanks :)

Replace ioutil with io

hperl · 2022-09-14T13:06:54Z

pipeline/authn/authenticator_remote_json_test.go

+			err := pipelineAuthenticator.Authenticate(
+				makeRemoteJSONRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""),
+				session,
+				json.RawMessage(fmt.Sprintf(`{"check_session_url": "%s"}`, testServer.URL)),


Suggested change

json.RawMessage(fmt.Sprintf(`{"check_session_url": "%s"}`, testServer.URL)),

json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)),

;)

Fixed :) Thanks

…ault to the auth service

- Split long lines on multiple lines - Remove unnecessary checks - Move schema to new location in `spec` directory - Replace ioutil with io packages - Add manual close to the original request body stream.

jaspeen · 2024-05-26T20:03:13Z

Hey guys, @hperl, @aeneasr. What else do we need there except all these checks to be pushed to main?

vladr11 requested a review from aeneasr as a code owner April 4, 2022 08:48

vladr11 force-pushed the forward-request-authenticator branch from 2d4b289 to c9310a7 Compare April 4, 2022 13:47

aeneasr mentioned this pull request Apr 11, 2022

HMAC Authenticator #936

Closed

6 tasks

aeneasr requested changes Apr 11, 2022

View reviewed changes

aeneasr force-pushed the forward-request-authenticator branch from c9310a7 to 598f646 Compare May 23, 2022 20:30

vladr11 force-pushed the forward-request-authenticator branch from 598f646 to 2b78ace Compare May 24, 2022 12:59

vladr11 force-pushed the forward-request-authenticator branch from 2b78ace to 96fa276 Compare June 14, 2022 13:37

vladr11 force-pushed the forward-request-authenticator branch from 96fa276 to 73f7fb1 Compare June 30, 2022 11:00

aeneasr requested a review from hperl September 10, 2022 08:55

aeneasr mentioned this pull request Sep 10, 2022

feat: introduce auth scheme and jumping to next authentication #982

Open

7 tasks

aeneasr force-pushed the forward-request-authenticator branch from 73f7fb1 to 54d6d85 Compare September 10, 2022 09:00

hperl requested changes Sep 14, 2022

View reviewed changes

hperl reviewed Sep 14, 2022

View reviewed changes

vladr11 added 4 commits September 28, 2022 18:12

create custom forward request authenticator

c8772a6

refactor: rename forward authz int remote JSON

1ccc89d

fix: change behaviour of auth remote_json to send POST request by def…

854e659

…ault to the auth service

Prepare for merge

53ead8f

- Split long lines on multiple lines - Remove unnecessary checks - Move schema to new location in `spec` directory - Replace ioutil with io packages - Add manual close to the original request body stream.

vladr11 force-pushed the forward-request-authenticator branch from 54d6d85 to 53ead8f Compare September 29, 2022 13:49

jendrikjoe mentioned this pull request Apr 9, 2024

Remote Authenticator #841

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create custom forward request authenticator #949

create custom forward request authenticator #949

vladr11 commented Apr 4, 2022 •

edited

Loading

CLAassistant commented Apr 4, 2022 •

edited

Loading

codecov bot commented Apr 4, 2022 •

edited

Loading

aeneasr left a comment

aeneasr Apr 11, 2022

vladr11 May 24, 2022

vladr11 commented May 24, 2022

vladr11 commented Jun 30, 2022

jendrikjoe commented Sep 1, 2022

aeneasr commented Sep 10, 2022

hperl left a comment

hperl Sep 14, 2022

vladr11 Sep 29, 2022

hperl Sep 14, 2022

vladr11 Sep 29, 2022

hperl Sep 14, 2022

vladr11 Sep 29, 2022

hperl Sep 14, 2022

vladr11 Sep 29, 2022

hperl Sep 14, 2022

vladr11 Sep 29, 2022

jaspeen commented May 26, 2024

	json.RawMessage(fmt.Sprintf(`{"check_session_url": "%s"}`, testServer.URL)),
	json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)),

create custom forward request authenticator #949

Are you sure you want to change the base?

create custom forward request authenticator #949

Conversation

vladr11 commented Apr 4, 2022 • edited Loading

Related issue(s)

Checklist

CLAassistant commented Apr 4, 2022 • edited Loading

codecov bot commented Apr 4, 2022 • edited Loading

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladr11 commented May 24, 2022

vladr11 commented Jun 30, 2022

jendrikjoe commented Sep 1, 2022

aeneasr commented Sep 10, 2022

hperl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaspeen commented May 26, 2024

vladr11 commented Apr 4, 2022 •

edited

Loading

CLAassistant commented Apr 4, 2022 •

edited

Loading

codecov bot commented Apr 4, 2022 •

edited

Loading