From 90c95866d12a184b0e641f4169907bc71f4663f5 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 13 Dec 2024 10:17:42 -0800 Subject: [PATCH] upgrade fosite to v0.49.0 and handle its API changes --- go.mod | 4 +-- go.sum | 8 +++--- .../endpoints/token/token_handler_test.go | 28 ++++++++++++++----- .../federationdomain/storage/kube_storage.go | 16 +++++++---- .../federationdomain/storage/null_storage.go | 6 ++-- .../refreshtoken/refreshtoken.go | 8 +++--- .../refreshtoken/refreshtoken_test.go | 18 ++++++------ test/integration/supervisor_login_test.go | 4 +-- test/integration/supervisor_warnings_test.go | 4 +-- 9 files changed, 58 insertions(+), 38 deletions(-) diff --git a/go.mod b/go.mod index c8c473a37..caa5e4960 100644 --- a/go.mod +++ b/go.mod @@ -33,7 +33,7 @@ require ( github.com/joshlf/go-acl v0.0.0-20200411065538-eae00ae38531 github.com/migueleliasweb/go-github-mock v1.1.0 github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 - github.com/ory/fosite v0.48.1-0.20241204153806-6c26dc54eb64 + github.com/ory/fosite v0.49.0 github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/pkg/errors v0.9.1 github.com/sclevine/spec v1.4.0 @@ -43,7 +43,7 @@ require ( github.com/tdewolff/minify/v2 v2.21.2 go.uber.org/mock v0.5.0 go.uber.org/zap v1.27.0 - golang.org/x/crypto v0.30.0 + golang.org/x/crypto v0.31.0 golang.org/x/net v0.32.0 golang.org/x/oauth2 v0.24.0 golang.org/x/sync v0.10.0 diff --git a/go.sum b/go.sum index 2ad010402..7c481ad71 100644 --- a/go.sum +++ b/go.sum @@ -464,8 +464,8 @@ github.com/openzipkin/zipkin-go v0.4.2 h1:zjqfqHjUpPmB3c1GlCvvgsM1G4LkvqQbBDueDO github.com/openzipkin/zipkin-go v0.4.2/go.mod h1:ZeVkFjuuBiSy13y8vpSDCjMi9GoI3hPpCJSBx/EYFhY= github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde h1:x0TT0RDC7UhAVbbWWBzr41ElhJx5tXPWkIHA2HWPRuw= github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde/go.mod h1:nZgzbfBr3hhjoZnS66nKrHmduYNpc34ny7RK4z5/HM0= -github.com/ory/fosite v0.48.1-0.20241204153806-6c26dc54eb64 h1:EFceZAYrvvkh/ODW4EpNWLbVoRbNv2tDmxKDGqhrpS8= -github.com/ory/fosite v0.48.1-0.20241204153806-6c26dc54eb64/go.mod h1:M+C+Ng1UDNgwX4SaErnuZwEw26uDN7I3kNUt0WyValI= +github.com/ory/fosite v0.49.0 h1:KNqO7RVt/1X8F08/UI0Y+GRvcpscCWgjqvpLBQPRovo= +github.com/ory/fosite v0.49.0/go.mod h1:FAn7IY+I6DjT1r29wMouPeRYq63DWUuBj++96uOS4mE= github.com/ory/go-acc v0.2.9-0.20230103102148-6b1c9a70dbbe h1:rvu4obdvqR0fkSIJ8IfgzKOWwZ5kOT2UNfLq81Qk7rc= github.com/ory/go-acc v0.2.9-0.20230103102148-6b1c9a70dbbe/go.mod h1:z4n3u6as84LbV4YmgjHhnwtccQqzf4cZlSk9f1FhygI= github.com/ory/go-convenience v0.1.0 h1:zouLKfF2GoSGnJwGq+PE/nJAE6dj2Zj5QlTgmMTsTS8= @@ -680,8 +680,8 @@ golang.org/x/crypto v0.0.0-20220722155217-630584e8d5aa/go.mod h1:IxCIyHEi3zRg3s0 golang.org/x/crypto v0.6.0/go.mod h1:OFC/31mSvZgRz0V1QTNCzfAI1aIRzbiufJtkMIlEp58= golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.21.0/go.mod h1:0BP7YvVV9gBbVKyeTG0Gyn+gZm94bibOW5BjDEYAOMs= -golang.org/x/crypto v0.30.0 h1:RwoQn3GkWiMkzlX562cLB7OxWvjH1L8xutO2WoJcRoY= -golang.org/x/crypto v0.30.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk= +golang.org/x/crypto v0.31.0 h1:ihbySMvVjLAeSH1IbfcRTkD/iNscyz8rGzjF/E5hV6U= +golang.org/x/crypto v0.31.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= diff --git a/internal/federationdomain/endpoints/token/token_handler_test.go b/internal/federationdomain/endpoints/token/token_handler_test.go index b6bc43217..b387fca5d 100644 --- a/internal/federationdomain/endpoints/token/token_handler_test.go +++ b/internal/federationdomain/endpoints/token/token_handler_test.go @@ -168,6 +168,20 @@ var ( } `) + fositeInvalidRefreshTokenErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The refresh token is malformed or not valid." + } + `) + + fositeExpiredRefreshTokenErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The refresh token expired." + } + `) + fositeReusedAuthCodeErrorBody = here.Doc(` { "error": "invalid_grant", @@ -3766,7 +3780,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusBadRequest, - wantErrorResponseBody: fositeInvalidAuthCodeErrorBody, + wantErrorResponseBody: fositeExpiredRefreshTokenErrorBody, }, }, }, @@ -3793,7 +3807,7 @@ func TestRefreshGrant(t *testing.T) { }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusBadRequest, - wantErrorResponseBody: fositeInvalidAuthCodeErrorBody, + wantErrorResponseBody: fositeInvalidRefreshTokenErrorBody, }, }, }, @@ -3820,7 +3834,7 @@ func TestRefreshGrant(t *testing.T) { }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusBadRequest, - wantErrorResponseBody: fositeInvalidAuthCodeErrorBody, + wantErrorResponseBody: fositeInvalidRefreshTokenErrorBody, }, }, }, @@ -4831,7 +4845,7 @@ func TestRefreshGrant(t *testing.T) { session.Fosite = &openid.DefaultSession{} err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) require.NoError(t, err) - err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, "ignored", firstRequester) require.NoError(t, err) }, refreshRequest: refreshRequestInputs{ @@ -4869,7 +4883,7 @@ func TestRefreshGrant(t *testing.T) { delete(session.Fosite.Claims.Extra, "groups") err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) require.NoError(t, err) - err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, "ignored", firstRequester) require.NoError(t, err) }, refreshRequest: refreshRequestInputs{ @@ -4907,7 +4921,7 @@ func TestRefreshGrant(t *testing.T) { session.Custom.Username = "" err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) require.NoError(t, err) - err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, "ignored", firstRequester) require.NoError(t, err) }, refreshRequest: refreshRequestInputs{ @@ -4989,7 +5003,7 @@ func TestRefreshGrant(t *testing.T) { session.Fosite.Claims = fositeSessionClaims err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) require.NoError(t, err) - err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, "ignored", firstRequester) require.NoError(t, err) }, refreshRequest: refreshRequestInputs{ diff --git a/internal/federationdomain/storage/kube_storage.go b/internal/federationdomain/storage/kube_storage.go index c75053ccb..e1c484bfe 100644 --- a/internal/federationdomain/storage/kube_storage.go +++ b/internal/federationdomain/storage/kube_storage.go @@ -158,7 +158,7 @@ func (k KubeStorage) RevokeAccessToken(ctx context.Context, requestID string) er // // These are keyed by the signature of the refresh token. // -// Fosite will create these in the token endpoint whenever it wants to hand out an refresh token, including the original +// Fosite will create these in the token endpoint whenever it wants to hand out a refresh token, including the original // authcode redemption and also during refresh. Refresh tokens are only handed out when the user requested the // offline_access scope on the original authorization request. // @@ -169,8 +169,8 @@ func (k KubeStorage) RevokeAccessToken(ctx context.Context, requestID string) er // refresh token will never be deleted. // -func (k KubeStorage) CreateRefreshTokenSession(ctx context.Context, signatureOfRefreshToken string, request fosite.Requester) (err error) { - return k.refreshTokenStorage.CreateRefreshTokenSession(ctx, signatureOfRefreshToken, request) +func (k KubeStorage) CreateRefreshTokenSession(ctx context.Context, signatureOfRefreshToken string, accessTokenSignature string, request fosite.Requester) (err error) { + return k.refreshTokenStorage.CreateRefreshTokenSession(ctx, signatureOfRefreshToken, accessTokenSignature, request) } func (k KubeStorage) GetRefreshTokenSession(ctx context.Context, signatureOfRefreshToken string, session fosite.Session) (request fosite.Requester, err error) { @@ -185,8 +185,14 @@ func (k KubeStorage) RevokeRefreshToken(ctx context.Context, requestID string) e return k.refreshTokenStorage.RevokeRefreshToken(ctx, requestID) } -func (k KubeStorage) RevokeRefreshTokenMaybeGracePeriod(ctx context.Context, requestID string, signature string) error { - return k.refreshTokenStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, requestID, signature) +func (k KubeStorage) RotateRefreshToken(ctx context.Context, requestID string, _refreshTokenSignature string) error { + // RotateRefreshToken was added in fosite v0.49.0, replacing RevokeRefreshTokenMaybeGracePeriod. + // Confusingly, its job is to both revoke the old refresh token and also revoke the old access token. + // See their sample storage implementation here: https://github.com/ory/fosite/blob/v0.49.0/storage/memory.go#L497-L504 + if err := k.refreshTokenStorage.RevokeRefreshToken(ctx, requestID); err != nil { + return err + } + return k.accessTokenStorage.RevokeAccessToken(ctx, requestID) } // diff --git a/internal/federationdomain/storage/null_storage.go b/internal/federationdomain/storage/null_storage.go index ae5b0ecc8..8c46be755 100644 --- a/internal/federationdomain/storage/null_storage.go +++ b/internal/federationdomain/storage/null_storage.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package storage @@ -39,7 +39,7 @@ func (NullStorage) RevokeRefreshToken(_ context.Context, _ string) error { return errNullStorageNotImplemented } -func (NullStorage) RevokeRefreshTokenMaybeGracePeriod(_ context.Context, _ string, _ string) error { +func (NullStorage) RotateRefreshToken(_ context.Context, _ string, _ string) error { return errNullStorageNotImplemented } @@ -47,7 +47,7 @@ func (NullStorage) RevokeAccessToken(_ context.Context, _ string) error { return errNullStorageNotImplemented } -func (NullStorage) CreateRefreshTokenSession(_ context.Context, _ string, _ fosite.Requester) (err error) { +func (NullStorage) CreateRefreshTokenSession(_ context.Context, _ string, _ string, _ fosite.Requester) (err error) { return nil } diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index 78986a2c0..a3eb93400 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -42,7 +42,7 @@ const ( type RevocationStorage interface { fositeoauth2.RefreshTokenStorage RevokeRefreshToken(ctx context.Context, requestID string) error - RevokeRefreshTokenMaybeGracePeriod(ctx context.Context, requestID string, signature string) error + RotateRefreshToken(ctx context.Context, requestID string, refreshTokenSignature string) error } var _ RevocationStorage = &refreshTokenStorage{} @@ -82,12 +82,12 @@ func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) } -func (a *refreshTokenStorage) RevokeRefreshTokenMaybeGracePeriod(ctx context.Context, requestID string, _signature string) error { - // We don't support a grace period, so always call the regular RevokeRefreshToken(). +func (a *refreshTokenStorage) RotateRefreshToken(ctx context.Context, requestID string, _refreshTokenSignature string) error { + // Rotation is called to revoke an old token during a refresh, so we can always call RevokeRefreshToken(). return a.RevokeRefreshToken(ctx, requestID) } -func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, signature string, requester fosite.Requester) error { +func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, signature string, _accessTokenSignature string, requester fosite.Requester) error { request, err := fositestorage.ValidateAndExtractAuthorizeRequest(requester) if err != nil { return err diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index 1ac19e836..2aa9102fe 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -109,7 +109,7 @@ func TestRefreshTokenStorage(t *testing.T) { RequestedAudience: nil, GrantedAudience: nil, } - err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", request) + err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", "ignored", request) require.NoError(t, err) require.Equal(t, 1, storageLifetimeFuncCallCount) require.Equal(t, request, storageLifetimeFuncCallRequesterArg) @@ -172,7 +172,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { Form: url.Values{"key": []string{"val"}}, Session: testutil.NewFakePinnipedSession(), } - err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", request) + err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", "ignored", request) require.NoError(t, err) // Revoke the request ID of the session that we just created @@ -227,12 +227,12 @@ func TestRefreshTokenStorageRevokeRefreshTokenMaybeGracePeriod(t *testing.T) { Form: url.Values{"key": []string{"val"}}, Session: testutil.NewFakePinnipedSession(), } - err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", request) + err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", "ignored", request) require.NoError(t, err) - // Revoke the request ID of the session that we just created. We don't support grace periods, so this + // Revoke the request ID of the session that we just created. This // should work exactly like the regular RevokeRefreshToken() function. - err = storage.RevokeRefreshTokenMaybeGracePeriod(ctx, "abcd-1", "fancy-signature") + err = storage.RotateRefreshToken(ctx, "abcd-1", "fancy-signature") require.NoError(t, err) testutil.LogActualJSONFromCreateAction(t, client, 0) // makes it easier to update expected values when needed @@ -306,7 +306,7 @@ func TestNilSessionRequest(t *testing.T) { func TestCreateWithNilRequester(t *testing.T) { ctx, _, _, storage := makeTestSubject(lifetimeFunc) - err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", nil) + err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", "ignored", nil) require.EqualError(t, err, "requester must be of type fosite.Request") } @@ -317,14 +317,14 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { Session: nil, Client: &clientregistry.Client{}, } - err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", request) + err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", "ignored", request) require.EqualError(t, err, "requester's session must be of type PinnipedSession") request = &fosite.Request{ Session: &psession.PinnipedSession{}, Client: nil, } - err = storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", request) + err = storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", "ignored", request) require.EqualError(t, err, "requester's client must be of type clientregistry.Client") } @@ -336,7 +336,7 @@ func TestCreateWithoutRequesterID(t *testing.T) { Session: &psession.PinnipedSession{}, Client: &clientregistry.Client{}, } - err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", request) + err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", "ignored", request) require.NoError(t, err) // the blank ID was filled in with an auto-generated ID diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 582aeea40..a4f5b1713 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -3201,7 +3201,7 @@ func testSupervisorLogin( // Then save the mutated Secret back to Kubernetes. // There is no update function, so delete and create again at the same name. require.NoError(t, oauthStore.DeleteRefreshTokenSession(ctx, signatureOfLatestRefreshToken)) - require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, signatureOfLatestRefreshToken, storedRefreshSession)) + require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, signatureOfLatestRefreshToken, "ignored", storedRefreshSession)) } // Use the refresh token to get new tokens by calling the token endpoint again. @@ -3275,7 +3275,7 @@ func testSupervisorLogin( // Then save the mutated Secret back to Kubernetes. // There is no update function, so delete and create again at the same name. require.NoError(t, oauthStore.DeleteRefreshTokenSession(ctx, signatureOfLatestRefreshToken)) - require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, signatureOfLatestRefreshToken, storedRefreshSession)) + require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, signatureOfLatestRefreshToken, "ignored", storedRefreshSession)) // Now try to perform a downstream refresh again, knowing that the corresponding upstream refresh should fail. _, err = downstreamOAuth2Config.TokenSource(oidcHTTPClientContext, &oauth2.Token{RefreshToken: latestRefreshToken}).Token() diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go index d06ef31e4..2c91487f9 100644 --- a/test/integration/supervisor_warnings_test.go +++ b/test/integration/supervisor_warnings_test.go @@ -193,7 +193,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { pinnipedSession.Fosite.Claims.Extra["groups"] = []string{"some-wrong-group", "some-other-group"} // update downstream groups require.NoError(t, oauthStore.DeleteRefreshTokenSession(ctx, refreshTokenSignature)) - require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, refreshTokenSignature, storedRefreshSession)) + require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, refreshTokenSignature, "ignored", storedRefreshSession)) // remove the credential cache, which includes the cached cert, so it won't be reused and the refresh flow will be triggered. err = os.Remove(credentialCachePath) @@ -529,7 +529,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { pinnipedSession.Fosite.Claims.Extra["groups"] = []string{"some-wrong-group", "some-other-group"} require.NoError(t, oauthStore.DeleteRefreshTokenSession(ctx, refreshTokenSignature)) - require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, refreshTokenSignature, storedRefreshSession)) + require.NoError(t, oauthStore.CreateRefreshTokenSession(ctx, refreshTokenSignature, "ignored", storedRefreshSession)) // remove the credential cache, which includes the cached cert, so it won't be reused and the refresh flow will be triggered. err = os.Remove(credentialCachePath)