Skip to content

refactor: refresh token rotation #838

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.

Sign up for GitHub

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

Merged
aeneasr merged 3 commits into from
Dec 12, 2024
Merged

refactor: refresh token rotation #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4fe21c
refactor: refresh token rotation interfaces
aeneasr Nov 28, 2024
b57570a
feat: better refresh token debug-ability
aeneasr Dec 4, 2024
57cf545
ci: pin hydra version
aeneasr Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/oidc-conformity.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
with:
fetch-depth: 2
repository: ory/hydra
ref: 2866a0499d02341ed0603601cfe4e63b24506fcb
ref: a35e78e364a26c4f87f37d9f545ef10b3ffa468a
- uses: actions/setup-go@v2
with:
go-version: "1.21"
Expand Down
8 changes: 8 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
export PATH := .bin:${PATH}

format: .bin/goimports .bin/ory node_modules # formats the source code
.bin/ory dev headers copyright --type=open-source
.bin/goimports -w .
Expand All @@ -18,6 +20,9 @@ test: # runs all tests
.bin/licenses: Makefile
curl https://raw.githubusercontent.com/ory/ci/master/licenses/install | sh

.bin/mockgen:
go build -o .bin/mockgen github.com/golang/mock/mockgen

.bin/ory: Makefile
curl https://raw.githubusercontent.com/ory/meta/master/install.sh | bash -s -- -b .bin ory v0.1.48
touch .bin/ory
Expand All @@ -26,4 +31,7 @@ node_modules: package-lock.json
npm ci
touch node_modules

gen: .bin/goimports .bin/mockgen # generates mocks
./generate-mocks.sh

.DEFAULT_GOAL := help
2 changes: 1 addition & 1 deletion handler/oauth2/flow_authorize_code_token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func (c *AuthorizeExplicitGrantHandler) PopulateTokenEndpointResponse(ctx contex
} else if err = c.CoreStorage.CreateAccessTokenSession(ctx, accessSignature, requester.Sanitize([]string{})); err != nil {
return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
} else if refreshSignature != "" {
if err = c.CoreStorage.CreateRefreshTokenSession(ctx, refreshSignature, requester.Sanitize([]string{})); err != nil {
if err = c.CoreStorage.CreateRefreshTokenSession(ctx, refreshSignature, accessSignature, requester.Sanitize([]string{})); err != nil {
return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
}
}
Expand Down
4 changes: 2 additions & 2 deletions handler/oauth2/flow_authorize_code_token_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ func TestAuthorizeCodeTransactional_HandleTokenEndpointRequest(t *testing.T) {
Times(1)
mockCoreStore.
EXPECT().
CreateRefreshTokenSession(propagatedContext, gomock.Any(), gomock.Any()).
CreateRefreshTokenSession(propagatedContext, gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil).
Times(1)
mockTransactional.
Expand Down Expand Up @@ -627,7 +627,7 @@ func TestAuthorizeCodeTransactional_HandleTokenEndpointRequest(t *testing.T) {
Times(1)
mockCoreStore.
EXPECT().
CreateRefreshTokenSession(propagatedContext, gomock.Any(), gomock.Any()).
CreateRefreshTokenSession(propagatedContext, gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil).
Times(1)
mockTransactional.
Expand Down
3 changes: 2 additions & 1 deletion handler/oauth2/flow_client_credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ func (c *ClientCredentialsGrantHandler) PopulateTokenEndpointResponse(ctx contex
}

atLifespan := fosite.GetEffectiveLifespan(request.GetClient(), fosite.GrantTypeClientCredentials, fosite.AccessToken, c.Config.GetAccessTokenLifespan(ctx))
return c.IssueAccessToken(ctx, atLifespan, request, response)
_, err := c.IssueAccessToken(ctx, atLifespan, request, response)
return err
}

func (c *ClientCredentialsGrantHandler) CanSkipClientAuth(ctx context.Context, requester fosite.AccessRequester) bool {
Expand Down
50 changes: 23 additions & 27 deletions handler/oauth2/flow_refresh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,25 +51,31 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
return errorsx.WithStack(rErr)
}

return errorsx.WithStack(fosite.ErrInactiveToken.WithWrap(err).WithDebug(err.Error()))
return fosite.ErrInvalidGrant.WithWrap(err).
Comment thread
aeneasr marked this conversation as resolved.
WithHint("The refresh token was already used.").
WithDebugf("Refresh token re-use was detected. All related tokens have been revoked.")
} else if errors.Is(err, fosite.ErrNotFound) {
return errorsx.WithStack(fosite.ErrInvalidGrant.WithWrap(err).WithDebugf("The refresh token has not been found: %s", err.Error()))
return fosite.ErrInvalidGrant.WithWrap(err).
WithHint("The refresh token is malformed or not valid.").
WithDebug("The refresh token can not be found.")
} else if err != nil {
return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
} else if err := c.RefreshTokenStrategy.ValidateRefreshToken(ctx, originalRequest, refresh); err != nil {
return fosite.ErrServerError.WithWrap(err).WithDebug(err.Error())
}

if err := c.RefreshTokenStrategy.ValidateRefreshToken(ctx, originalRequest, refresh); err != nil {
// The authorization server MUST ... validate the refresh token.
// This needs to happen after store retrieval for the session to be hydrated properly
if errors.Is(err, fosite.ErrTokenExpired) {
return errorsx.WithStack(fosite.ErrInvalidGrant.WithWrap(err).WithDebug(err.Error()))
return fosite.ErrInvalidGrant.WithWrap(err).
WithHint("The refresh token expired.")
}
return errorsx.WithStack(fosite.ErrInvalidRequest.WithWrap(err).WithDebug(err.Error()))
return fosite.ErrInvalidRequest.WithWrap(err).WithDebug(err.Error())
}

if !(len(c.Config.GetRefreshTokenScopes(ctx)) == 0 || originalRequest.GetGrantedScopes().HasOneOf(c.Config.GetRefreshTokenScopes(ctx)...)) {
scopeNames := strings.Join(c.Config.GetRefreshTokenScopes(ctx), " or ")
hint := fmt.Sprintf("The OAuth 2.0 Client was not granted scope %s and may thus not perform the 'refresh_token' authorization grant.", scopeNames)
return errorsx.WithStack(fosite.ErrScopeNotGranted.WithHint(hint))

}

// The authorization server MUST ... and ensure that the refresh token was issued to the authenticated client
Expand Down Expand Up @@ -130,30 +136,20 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con
if err != nil {
return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
}
defer func() {
err = c.handleRefreshTokenEndpointStorageError(ctx, err)
}()

ts, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil)
if err != nil {
return err
} else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, ts.GetID()); err != nil {
return err
}
storeReq := requester.Sanitize([]string{})
storeReq.SetID(requester.GetID())

if err := c.TokenRevocationStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, ts.GetID(), signature); err != nil {
return err
if err = c.TokenRevocationStorage.RotateRefreshToken(ctx, requester.GetID(), signature); err != nil {

@mitar mitar Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aeneasr I think this introduced a bug. After we upgraded our systems to the version with this change, no refresh tokens in our tests (where we use memory storage) got invalidated. The reason I think is that requester here has a completely new and random ID, which of course has nothing with the existing refresh token.

Previous code had it right, it retrieved refresh token requester with GetRefreshTokenSession and used ts.GetID() here. I think this should be fixed and correct requester ID should be passed here.

@mitar mitar Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My current workaround is to fix this at store level:

func (s *MemoryStore) RotateRefreshToken(ctx context.Context, _ string, refreshTokenSignature string) error {
	ts, err := s.GetRefreshTokenSession(ctx, refreshTokenSignature, nil)
	if err != nil {
		return err
	}
	if err := s.RevokeRefreshToken(ctx, ts.GetID()); err != nil {
		return err
	}
	return s.RevokeAccessToken(ctx, ts.GetID())
}

But this is just a workaround, I think, it is completely useless to pass new requester ID into this function. Or is this something you use and need in Hydra?

@aeneasr aeneasr Aug 19, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The request ID is supposed to remain stable for the lifetime duration of a token chain, so this is correct! You can check the implementation in Hydra for reference

@mitar mitar Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are right. It works. I had in our code a part where I was unconditionally setting request ID to a new value. The reason is that I want a bit shorter ID representation than what default uuid.New().String() provides. I will have to find another way to do that. I could not find an easy way to patch GetID() to use a different ID constructor. :-(

Thanks.

@aeneasr aeneasr Aug 19, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fosite’s not the easiest library to work with

@mitar mitar Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, it is great!

return c.handleRefreshTokenEndpointStorageError(ctx, err)
}

storeReq := requester.Sanitize([]string{})
storeReq.SetID(ts.GetID())

if err = c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, storeReq); err != nil {
return err
return c.handleRefreshTokenEndpointStorageError(ctx, err)
}

if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
return err
if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, accessSignature, storeReq); err != nil {
return c.handleRefreshTokenEndpointStorageError(ctx, err)
}

responder.SetAccessToken(accessToken)
Expand All @@ -164,7 +160,7 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con
responder.SetExtra("refresh_token", refreshToken)

if err = storage.MaybeCommitTx(ctx, c.TokenRevocationStorage); err != nil {
return err
return c.handleRefreshTokenEndpointStorageError(ctx, err)
}

return nil
Expand Down Expand Up @@ -222,14 +218,14 @@ func (c *RefreshTokenGrantHandler) handleRefreshTokenEndpointStorageError(ctx co
return errorsx.WithStack(fosite.ErrInvalidRequest.
WithDebugf(storageErr.Error()).
WithWrap(storageErr).
WithHint("Failed to refresh token because of multiple concurrent requests using the same token which is not allowed."))
WithHint("Failed to refresh token because of multiple concurrent requests using the same token. Please retry the request."))
}

if errors.Is(storageErr, fosite.ErrNotFound) || errors.Is(storageErr, fosite.ErrInactiveToken) {
return errorsx.WithStack(fosite.ErrInvalidRequest.
WithDebugf(storageErr.Error()).
WithWrap(storageErr).
WithHint("Failed to refresh token because of multiple concurrent requests using the same token which is not allowed."))
WithHint("Failed to refresh token. Please retry the request."))
}

return errorsx.WithStack(fosite.ErrServerError.WithWrap(storageErr).WithDebug(storageErr.Error()))
Expand Down
Loading