Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create custom forward request authenticator #949

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vladr11
Copy link

@vladr11 vladr11 commented Apr 4, 2022

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.

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

CLAassistant commented Apr 4, 2022

CLA assistant check
All committers have signed the CLA.

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

codecov bot commented Apr 4, 2022

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 aeneasr mentioned this pull request Apr 11, 2022
6 tasks
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the authenticator into remote_json

@aeneasr aeneasr force-pushed the forward-request-authenticator branch from c9310a7 to 598f646 Compare May 23, 2022 20:30
@vladr11 vladr11 force-pushed the forward-request-authenticator branch from 598f646 to 2b78ace Compare May 24, 2022 12:59
@vladr11
Copy link
Author

vladr11 commented May 24, 2022

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 vladr11 force-pushed the forward-request-authenticator branch from 2b78ace to 96fa276 Compare June 14, 2022 13:37
@vladr11 vladr11 force-pushed the forward-request-authenticator branch from 96fa276 to 73f7fb1 Compare June 30, 2022 11:00
@vladr11
Copy link
Author

vladr11 commented Jun 30, 2022

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

Fixed linting issues, as well.

@jendrikjoe
Copy link

A polite ping here again 😉

@aeneasr
Copy link
Member

aeneasr commented Sep 10, 2022

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

Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved

// 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 70 to 77
if len(c.ExtraFrom) == 0 {
c.ExtraFrom = "extra"
}

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed unnecessary checks


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace ioutil with io

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

;)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :) Thanks

- 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
Copy link

jaspeen commented May 26, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants