-
Notifications
You must be signed in to change notification settings - Fork 360
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
fix: Don't send scope with OAuth2 introspection request #379
Conversation
@@ -85,7 +85,7 @@ func (a *AuthenticatorOAuth2Introspection) Authenticate(r *http.Request, session | |||
return errors.WithStack(ErrAuthenticatorNotResponsible) | |||
} | |||
|
|||
body := url.Values{"token": {token}, "scope": {strings.Join(cf.Scopes, " ")}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclude this only when the scope strategy is set to none
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we exclude it in all cases.
For example, if the scope strategy of the given Oathkeeper rule is wildcard
, Hydra won't know that and therefore won't be able to properly validate the scope which is why we opt out of Hydra's scope validation all together and rely only on Oathkeeper's validation which happens after the token introspection.
That logic applies no matter what the scope strategy for a given Oathkeeper rule is.
When the scope strategy is none
, that means we still want to opt out of Hydra's validation but we also want to opt out of Oathkeeper's validation in favor of the application's scope validation. This is useful for GraphQL APIs where it's not possible to know what scopes are required based solely on the path name.
The basic problem is Hydra can't adapt its behavior bases on the scope strategy. For that to be possible, Hydra would need to accept the strategy as another parameter to the introspection endpoint. Also, Oathkeepr doesn't know the scope strategy that Hydra is configured to use so it can't conditionally send the scope to Hydra based on the scope strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with your sentiment. The scope is enforced by the Authorization Server and only in cases where we explicitly want to override this behavior we use a specific strategy for Oathkeeper.
Making this change now will leave unintended breaking changes for all people that rely on this feature for authorization. It is thus not a good idea to completely remove it.
When the scope strategy is none, that means we still want to opt out of Hydra's validation but we also want to opt out of Oathkeeper's validation in favor of the application's scope validation. This is useful for GraphQL APIs where it's not possible to know what scopes are required based solely on the path name.
If you want to opt out of any validation just don't include any scopes in the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes sense. So we only send the scope to Hydra when the scope strategy is none
. Let me make that change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some tests for the different strategies. Before, all of the tests where running with the exact
strategy. Now there are at least two tests for each. One that passes and one that fails. And the tests default to a strategy of none
which is in line with the default behavior of Oathkeeper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Related issue
Fixes #377
Proposed changes
Oathkeeper shouldn't send scope with introspection requests and instead should rely on its own scope validation. See linked issue for more details.
Checklist
vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.