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

add cookie as an option for oauth2_introspection authenticator #301

Merged
merged 2 commits into from
Nov 25, 2019

Conversation

paulbdavis
Copy link
Contributor

@paulbdavis paulbdavis commented Nov 16, 2019

Proposed changes

Adds the ability to get the OAuth2 bearer token from a cookie in addition to header or query param. This allows an http-only, secure cookie to be used for storing oauth credentials.

Checklist

  • I have read the contributing 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 necessary documentation within the code base (if appropriate)
  • I have documented my changes in the
    developer guide (if appropriate)

Further comments

I'm not quite sure how to properly handle the schema files, I'm guessing there is some compilation going on there that is not documented (vs just editing the config.schema.json file as I have for now).

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.

The schema process is a bit messy at the moment (for bc reasons). The way you solved it is correct!

I'm not sure if you are thinking about this use case, but just some points of warning:

  1. We do not check if the cookie is httpOnly, thus, any XSS could set this cookie
  2. There is no source authenticity (e.g. via signature verification), so anyone being able to set the cookie can set the token

The same of course applies to the other methods (e.g. me linking you to foo?access_token=.... or me doing an XSS that intercepts and/or injects the access token to API requests).

I'm saying that because many people think that cookies are "secure" and we should make sure to document this appropriately to avoid any misconceptions.

.schemas/config.schema.json Outdated Show resolved Hide resolved
@paulbdavis
Copy link
Contributor Author

I'm saying that because many people think that cookies are "secure" and we should make sure to document this appropriately to avoid any misconceptions.

@aeneasr I meant the "secure" flag is set on the cookie, not that cookies are inherently secure.

As with the cookie_session handler, the onus to protect against CSRF and XSS is on the application developer, not on oathkeeper, correct?

@paulbdavis
Copy link
Contributor Author

paulbdavis commented Nov 18, 2019

For this and #302, I can also go ahead and add the same options to other oauth2 authenticator before it gets merged, just so those two are at feature parity

If that's the case, I'd probably combine this and #302 for both authenticators in another new pull request

@aeneasr
Copy link
Member

aeneasr commented Nov 19, 2019

Makes sense!

@paulbdavis
Copy link
Contributor Author

Actually, after review, it looks like these options are not really appropriate for the client_credentials authenticator. I think this and #302 are ready to be merged as is

@aeneasr aeneasr merged commit e3fa55a into ory:master Nov 25, 2019
@paulbdavis paulbdavis deleted the feature/oath2-introspect-from-cookie branch November 25, 2019 18:22
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.

2 participants