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

fix: requested scope ignored in refresh flow #718

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

Conversation

james-d-elliott
Copy link
Contributor

This addresses an issue where the clients requested scope is ignored during the refresh flow preventing a refresh token from granting an access token with a more narrow scope. It is critical to note that it is completely ignored, and previous behaviour only issued the same scope as originally granted. An access request which requests a broader scope is equally ignored and it has been thoroughly tested to ensure this cannot occur in HEAD. See https://www.rfc-editor.org/rfc/rfc6749#section-6 for more information.

Related Issue or Design Document

Fixes #696

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • 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 approval (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 the necessary documentation within the code base (if appropriate).

Further comments

Copy link
Contributor Author

@james-d-elliott james-d-elliott left a comment

Choose a reason for hiding this comment

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

I've done a self review to highlight sections that I am unsure on and to give some context to the changes.

handler/oauth2/flow_refresh.go Outdated Show resolved Hide resolved
handler/oauth2/flow_refresh.go Outdated Show resolved Hide resolved
@james-d-elliott james-d-elliott changed the title fix(oauth2): requested scope ignored in refresh flow fix: requested scope ignored in refresh flow Nov 2, 2022
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.

This looks very good! Just one minor question

handler/oauth2/flow_refresh.go Outdated Show resolved Hide resolved
@james-d-elliott
Copy link
Contributor Author

Regaring the following sequence;

  1. Request token with scope a,b,c
  2. Do refresh token with scope a,b
  3. Do refresh token with scope a,b,c

I can confirm with my high level testing that it fails at step 3.

@james-d-elliott
Copy link
Contributor Author

james-d-elliott commented Nov 3, 2022

So looking here:

originalRequest, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, request.GetSession())

The original request we're looking up is the RT Session. Thus I think IF we want to preserve the original scope we just have to ensure the new RT session has the same scope as the original (granted), and the AT session has the narrowed scope. We just have to decide if that's desirable. With the current code including that addition would result in a second refresh without the scope value to restore the initially granted scope, and with the value the RP would be able to obtain the original scopes again or any subset of them even if they are not the same subset as the first refresh. See the flow sequences below.

The issue is I'm not entirely sure how we'd accomplish that, looking at the code it looks like the RT session and AT sessions share their scope. Maybe we have to have a new value to store the scope specific to the RT session, which defaults to the scope of the AT session so that it's appropriately handled during the PopulateTokenEndpointResponse.

If we did add this functionality the following flow sequence would be accurate (with a client which is authorized to grant scope openid foo bar baz fizz offline_access):

  1. Relying party requests access to a completely new token with scope openid foo bar baz offline_access
    1. Relying party is granted scope openid foo bar baz
  2. Relying party requests access to a refresh without a scope
    1. Relying party is granted scope openid foo bar baz
  3. Relying party requests access to a refresh with a scope of openid foo bar
    1. Relying party is granted scope openid foo bar
  4. Relying party requests access to a refresh with a scope of openid bar baz
    1. Relying party is granted scope openid bar baz
  5. Relying party requests access to a refresh without a scope
    1. Relying party is granted scope openid foo bar baz
  6. Relying party requests access to a refresh with a scope of openid foo bar baz fizz
    1. Relying party is rejected invalid_scope because they were not granted this scope in step 1
  7. Relying party requests access to a completely new token with scope openid fizz buzz offline_access
    1. Relying party is rejected invalid_scope because the client is not permitted to request a scope with buzz

@aeneasr
Copy link
Member

aeneasr commented Nov 4, 2022

The original request we're looking up is the RT Session. Thus I think IF we want to preserve the original scope we just have to ensure the new RT session has the same scope as the original (granted), and the AT session has the narrowed scope.

To me that sounds very, very reasonable!

@james-d-elliott
Copy link
Contributor Author

james-d-elliott commented Nov 4, 2022

@aeneasr I'm fairly sure of the most effective way to achieve this, however you probably know this code base better than anyone so maybe I've missed something or you have an alternative way that is preferable.

This is the critical path in making this functional I believe:

storeReq := requester.Sanitize([]string{})
storeReq.SetID(ts.GetID())
if err = c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, storeReq); err != nil {
return err
}
if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
return err
}

We need to ensure the storeReq for the AT contains the narrowed scope and that the storeReq for the RT contains the original scope I believe.

I think the best way to achieve this is to add an interface which extends AccessRequester with the following signature:

// RefreshTokenAccessRequester is an extended AccessRequester implementation that allows preserving
// the original scopes between refresh access requests allowing an relying-party to narrow the scope of
// the access token and then broaden them again to the resource owners original grant.
type RefreshTokenAccessRequester interface {
	// GetRefreshTokenRequestedScopes returns the request's scopes specifically for the refresh token.
	GetRefreshTokenRequestedScopes() (scopes Arguments)
	// SetRefreshTokenRequestedScopes sets the request's scopes specifically for the refresh token.
	SetRefreshTokenRequestedScopes(scopes Arguments)
	// GetRefreshTokenGrantedScopes returns all granted scopes specifically for the refresh token.
	GetRefreshTokenGrantedScopes() (grantedScopes Arguments)
	// SetRefreshTokenRequestedScopes sets the request's scopes specifically for the refresh token.
	SetRefreshTokenRequestedScopes(scopes Arguments)
	
	AccessRequester
}

The key thing being that if that interface is not implemented that people would not have this functionality.

@aeneasr
Copy link
Member

aeneasr commented Nov 4, 2022

I think that could work! We probably need a few more tests to ensure that this works without getting confused in the process in longer refresh token chains :)

@james-d-elliott james-d-elliott force-pushed the fix-refresh-scope-narrowing branch 2 times, most recently from 08cde0e to 008a527 Compare November 15, 2022 23:31
access_request.go Outdated Show resolved Hide resolved
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.

Awesome, getting much closer! :)

handler/oauth2/flow_refresh.go Outdated Show resolved Hide resolved
access_request.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vivshankar vivshankar left a comment

Choose a reason for hiding this comment

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

@james-d-elliott Thanks for this PR! I had a couple of observations that you might consider.

if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
return err
if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok {
rtStoreReq := requester.Sanitize([]string{}).(*fosite.Request)
Copy link
Contributor

Choose a reason for hiding this comment

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

@james-d-elliott Wondering if this negates the idea of having a Requester interface at all. I understand why you do it though.

A better approach might be to introduce an interface that allows you to invoke SetGrantedScopes and SetRequestedScopes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the only intent behind this was to avoid breaking any other implementation. I'm completely fine with which ever approach most fits with how ory is using this however.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the spirit of preserving the interfaces, I suggest a new interface be added that is able to mutate scopes in the requester.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been addressed in a use specific way I believe.

handler/oauth2/flow_refresh.go Outdated Show resolved Hide resolved
This addresses an issue where the clients requested scope is ignored during the refresh flow preventing a refresh token from granting an access token with a more narrow scope. It is critical to note that it is completely ignored, and previous behaviour only issued the same scope as originally granted. An access request which requests a broader scope is equally ignored and it has been thoroughly tested to ensure this cannot occur in HEAD. See https://www.rfc-editor.org/rfc/rfc6749#section-6 for more information.
@james-d-elliott james-d-elliott force-pushed the fix-refresh-scope-narrowing branch 2 times, most recently from 9c10f44 to 33e6bfc Compare July 21, 2023 00:08
Copy link
Contributor

@vivshankar vivshankar left a comment

Choose a reason for hiding this comment

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

LGTM. Approving FWIW :-)

Copy link
Contributor Author

@james-d-elliott james-d-elliott left a comment

Choose a reason for hiding this comment

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

RFC. Also the intention behind them is bad clients which this change may cause breakage for as highlighted by Vivek.

Comment on lines +33 to +36
// IgnoreRequestedScopeNotInOriginalGrant determines the action to take when the requested scopes in the refresh
// flow were not originally granted. If false which is the default the handler will automatically return an error.
// If true the handler will filter out / ignore the scopes which were not originally granted.
IgnoreRequestedScopeNotInOriginalGrant bool
Copy link
Contributor Author

@james-d-elliott james-d-elliott Jul 21, 2023

Choose a reason for hiding this comment

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

Side note when looking at this. I have a per-client implementation of this utilizing an interface which passes the same tests. Maybe this is more desirable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not made through a new config interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it looks like currently all config is done through config interface, even if it is not meant for hot reloading?

Comment on lines +110 to +117
if c.IgnoreRequestedScopeNotInOriginalGrant {
// Skips addressing point 2 of the text in RFC6749 Section 6 and instead just prevents the scope
// requested from being granted.
continue
} else {
// Addresses point 2 of the text in RFC6749 Section 6.
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' was not originally granted by the resource owner.", scope))
}
Copy link
Contributor Author

@james-d-elliott james-d-elliott Jul 21, 2023

Choose a reason for hiding this comment

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

Side note when looking at this. I have a per-client implementation of this utilizing an interface which passes the same tests. Maybe this is more desirable?

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the use case of having this per-client?

Copy link
Contributor Author

@james-d-elliott james-d-elliott Feb 15, 2024

Choose a reason for hiding this comment

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

The only way I'll personally use it is per-client where the default is to enforce the spec and you make an exception on a per-client basis. I'm happy enough to maintain my own entire implementation however.

if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
return err
if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok {
rtStoreReq := rtRequest.SanitizeRestoreRefreshTokenOriginalRequester(originalRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change about? Why it is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the scopes of the current request are for the access token, not the refresh token. The scopes of the original granted refresh token must be restored to make it possible to meet the strict requirements of the spec:

If a new refresh token is issued, the refresh token scope MUST be identical to that of the refresh token included by the client in the request.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I mean, my question is why you have two cases here. It looks like you added RefreshTokenAccessRequester for backwards compatibility? But it feels to me that if you do not implement RefreshTokenAccessRequester then you get wrong behavior? Not sure if we should keep the old behavior at all?

Also, I am not sure that an extra method is already needed here? Sanitize returns a clone of requester to begin with? And you then call just:

ar.SetID(requester.GetID())
ar.SetRequestedScopes(requester.GetRequestedScopes())
ar.SetGrantedScopes(requester.GetGrantedScopes())

So why not just call originalRequest.Sanitize(nil) and then those three calls above, here, at this point? Without a method?

Probably I am missing something, I just checked very quickly this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An AccessRequester doesn't have SetGranted* receivers as part of the implementation. So a new receiver will be needed regardless. The question is just about what solution is going to lead to the least harmful outcome.

Adding those receivers to the AccessRequester interface seems like a poor choice as it isn't used now but could potentially lead to implementers making harmful decisions in this stage of the relevant flows in my opinion.

As far as the other I brought it up with aeneasr I believe so really up to an ory preference (he may have actually asked for it but I don't recall specifics). I personally don't have one myself in either direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

It has GrantScope? Isn't this enough? You loop over all granted scopes and call GrantScope?

Copy link
Contributor

@mitar mitar Feb 15, 2024

Choose a reason for hiding this comment

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

I see the same being done currently in the code:

	request.SetID(originalRequest.GetID())
	request.SetSession(originalRequest.GetSession().Clone())
	request.SetRequestedScopes(originalRequest.GetRequestedScopes())
	request.SetRequestedAudience(originalRequest.GetRequestedAudience())

	for _, scope := range originalRequest.GetGrantedScopes() {
		if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {
			return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope))
		}
		request.GrantScope(scope)
	}

	...

	for _, audience := range originalRequest.GetGrantedAudience() {
		request.GrantAudience(audience)
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this was a previous comment here: #718 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

So to seems there is already some code to clone a requester. Maybe that should be just moved out into utility function and reused here for refresh token as well?

(And again sorry if I am missing something.)

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.

Unable to request narrower scopes in OAuth2 Refresh Token Exchange
4 participants