From b981055d31746aa3fc5948d1f65663a773e27a43 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 3 Dec 2021 13:44:24 -0800 Subject: [PATCH 1/4] Support revocation of access tokens in UpstreamOIDCIdentityProviderI - Rename the RevokeRefreshToken() function to RevokeToken() and make it take the token type (refresh or access) as a new parameter. - This is a prefactor getting ready to support revocation of upstream access tokens in the garbage collection handler. --- .../supervisorstorage/garbage_collector.go | 2 +- .../garbage_collector_test.go | 69 +++++---- .../mockupstreamoidcidentityprovider.go | 13 +- .../provider/dynamic_upstream_idp_provider.go | 12 +- .../testutil/oidctestutil/oidctestutil.go | 74 ++++----- internal/upstreamoidc/upstreamoidc.go | 32 ++-- internal/upstreamoidc/upstreamoidc_test.go | 143 ++++++++++++------ 7 files changed, 204 insertions(+), 141 deletions(-) diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index 2ce06533..5f388b80 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -243,7 +243,7 @@ func (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context. } // Revoke the upstream refresh token. This is a noop if the upstream provider does not offer a revocation endpoint. - err := foundOIDCIdentityProviderI.RevokeRefreshToken(ctx, customSessionData.OIDC.UpstreamRefreshToken) + err := foundOIDCIdentityProviderI.RevokeToken(ctx, customSessionData.OIDC.UpstreamRefreshToken, provider.RefreshTokenType) if err != nil { // This could be a network failure, a 503 result which we should retry // (see https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1), diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index 042c4d38..d442c474 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -366,18 +366,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(nil) + WithRevokeTokenError(nil) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // The upstream refresh token is only revoked for the active authcode session. - idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, "upstream-oidc-provider-name", - &oidctestutil.RevokeRefreshTokenArgs{ - Ctx: syncContext.Context, - RefreshToken: "fake-upstream-refresh-token", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-refresh-token", + TokenType: provider.RefreshTokenType, }, ) @@ -448,14 +449,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(nil) + WithRevokeTokenError(nil) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // Nothing to revoke since we couldn't read the invalid secret. - idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t) + idpListerBuilder.RequireExactlyZeroCallsToRevokeToken(t) // The invalid authcode session secrets is still deleted because it is expired. r.ElementsMatch( @@ -524,14 +525,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(nil) + WithRevokeTokenError(nil) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // Nothing to revoke since we couldn't find the upstream in the cache. - idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t) + idpListerBuilder.RequireExactlyZeroCallsToRevokeToken(t) // The authcode session secrets is still deleted because it is expired. r.ElementsMatch( @@ -600,14 +601,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(nil) + WithRevokeTokenError(nil) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // Nothing to revoke since we couldn't find the upstream in the cache. - idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t) + idpListerBuilder.RequireExactlyZeroCallsToRevokeToken(t) // The authcode session secrets is still deleted because it is expired. r.ElementsMatch( @@ -677,18 +678,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail + WithRevokeTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // Tried to revoke it, although this revocation will fail. - idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, "upstream-oidc-provider-name", - &oidctestutil.RevokeRefreshTokenArgs{ - Ctx: syncContext.Context, - RefreshToken: "fake-upstream-refresh-token", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-refresh-token", + TokenType: provider.RefreshTokenType, }, ) @@ -749,18 +751,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail + WithRevokeTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // Tried to revoke it, although this revocation will fail. - idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, "upstream-oidc-provider-name", - &oidctestutil.RevokeRefreshTokenArgs{ - Ctx: syncContext.Context, - RefreshToken: "fake-upstream-refresh-token", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-refresh-token", + TokenType: provider.RefreshTokenType, }, ) @@ -875,18 +878,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(nil) + WithRevokeTokenError(nil) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // The upstream refresh token is only revoked for the downstream session which had offline_access granted. - idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, "upstream-oidc-provider-name", - &oidctestutil.RevokeRefreshTokenArgs{ - Ctx: syncContext.Context, - RefreshToken: "fake-upstream-refresh-token", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-refresh-token", + TokenType: provider.RefreshTokenType, }, ) @@ -958,18 +962,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeRefreshTokenError(nil) + WithRevokeTokenError(nil) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) // The upstream refresh token is revoked. - idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, "upstream-oidc-provider-name", - &oidctestutil.RevokeRefreshTokenArgs{ - Ctx: syncContext.Context, - RefreshToken: "fake-upstream-refresh-token", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-refresh-token", + TokenType: provider.RefreshTokenType, }, ) @@ -1015,7 +1020,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { r.False(syncContext.Queue.(*testQueue).called) // Run sync again when not enough time has passed since the most recent run, so no delete - // operations should happen even though there is a expired secret now. + // operations should happen even though there is an expired secret now. fakeClock.Step(29 * time.Second) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) require.Empty(t, kubeClient.Actions()) diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 046f1849..2d0dbed1 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -14,6 +14,7 @@ import ( reflect "reflect" gomock "github.com/golang/mock/gomock" + provider "go.pinniped.dev/internal/oidc/provider" nonce "go.pinniped.dev/pkg/oidcclient/nonce" oidctypes "go.pinniped.dev/pkg/oidcclient/oidctypes" pkce "go.pinniped.dev/pkg/oidcclient/pkce" @@ -215,18 +216,18 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) PerformRefresh(arg0, ar return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PerformRefresh", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).PerformRefresh), arg0, arg1) } -// RevokeRefreshToken mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) RevokeRefreshToken(arg0 context.Context, arg1 string) error { +// RevokeToken mocks base method. +func (m *MockUpstreamOIDCIdentityProviderI) RevokeToken(arg0 context.Context, arg1 string, arg2 provider.RevocableTokenType) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RevokeRefreshToken", arg0, arg1) + ret := m.ctrl.Call(m, "RevokeToken", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } -// RevokeRefreshToken indicates an expected call of RevokeRefreshToken. -func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0, arg1 interface{}) *gomock.Call { +// RevokeToken indicates an expected call of RevokeToken. +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeToken(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeRefreshToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).RevokeRefreshToken), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).RevokeToken), arg0, arg1, arg2) } // ValidateToken mocks base method. diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index a054a4c8..e5be51ff 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -17,6 +17,14 @@ import ( "go.pinniped.dev/pkg/oidcclient/pkce" ) +type RevocableTokenType string + +// These strings correspond to the token types defined by https://datatracker.ietf.org/doc/html/rfc7009#section-2.1 +const ( + RefreshTokenType RevocableTokenType = "refresh_token" + AccessTokenType RevocableTokenType = "access_token" +) + type UpstreamOIDCIdentityProviderI interface { // GetName returns a name for this upstream provider, which will be used as a component of the path for the // callback endpoint hosted by the Supervisor. @@ -68,8 +76,8 @@ type UpstreamOIDCIdentityProviderI interface { // validate the ID token. PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) - // RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint. - RevokeRefreshToken(ctx context.Context, refreshToken string) error + // RevokeToken will attempt to revoke the given token, if the provider has a revocation endpoint. + RevokeToken(ctx context.Context, token string, tokenType RevocableTokenType) error // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 03603bdb..6d0cabca 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -68,11 +68,12 @@ type PerformRefreshArgs struct { ExpectedSubject string } -// RevokeRefreshTokenArgs is used to spy on calls to -// TestUpstreamOIDCIdentityProvider.RevokeRefreshTokenArgsFunc(). -type RevokeRefreshTokenArgs struct { - Ctx context.Context - RefreshToken string +// RevokeTokenArgs is used to spy on calls to +// TestUpstreamOIDCIdentityProvider.RevokeTokenArgsFunc(). +type RevokeTokenArgs struct { + Ctx context.Context + Token string + TokenType provider.RevocableTokenType } // ValidateTokenArgs is used to spy on calls to @@ -166,7 +167,7 @@ type TestUpstreamOIDCIdentityProvider struct { PerformRefreshFunc func(ctx context.Context, refreshToken string) (*oauth2.Token, error) - RevokeRefreshTokenFunc func(ctx context.Context, refreshToken string) error + RevokeTokenFunc func(ctx context.Context, refreshToken string, tokenType provider.RevocableTokenType) error ValidateTokenFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) @@ -176,8 +177,8 @@ type TestUpstreamOIDCIdentityProvider struct { passwordCredentialsGrantAndValidateTokensArgs []*PasswordCredentialsGrantAndValidateTokensArgs performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs - revokeRefreshTokenCallCount int - revokeRefreshTokenArgs []*RevokeRefreshTokenArgs + revokeTokenCallCount int + revokeTokenArgs []*RevokeTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs } @@ -278,16 +279,17 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefresh(ctx context.Context, r return u.PerformRefreshFunc(ctx, refreshToken) } -func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshToken(ctx context.Context, refreshToken string) error { - if u.revokeRefreshTokenArgs == nil { - u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) +func (u *TestUpstreamOIDCIdentityProvider) RevokeToken(ctx context.Context, token string, tokenType provider.RevocableTokenType) error { + if u.revokeTokenArgs == nil { + u.revokeTokenArgs = make([]*RevokeTokenArgs, 0) } - u.revokeRefreshTokenCallCount++ - u.revokeRefreshTokenArgs = append(u.revokeRefreshTokenArgs, &RevokeRefreshTokenArgs{ - Ctx: ctx, - RefreshToken: refreshToken, + u.revokeTokenCallCount++ + u.revokeTokenArgs = append(u.revokeTokenArgs, &RevokeTokenArgs{ + Ctx: ctx, + Token: token, + TokenType: tokenType, }) - return u.RevokeRefreshTokenFunc(ctx, refreshToken) + return u.RevokeTokenFunc(ctx, token, tokenType) } func (u *TestUpstreamOIDCIdentityProvider) PerformRefreshCallCount() int { @@ -301,15 +303,15 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefreshArgs(call int) *Perform return u.performRefreshArgs[call] } -func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenCallCount() int { +func (u *TestUpstreamOIDCIdentityProvider) RevokeTokenCallCount() int { return u.performRefreshCallCount } -func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *RevokeRefreshTokenArgs { - if u.revokeRefreshTokenArgs == nil { - u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) +func (u *TestUpstreamOIDCIdentityProvider) RevokeTokenArgs(call int) *RevokeTokenArgs { + if u.revokeTokenArgs == nil { + u.revokeTokenArgs = make([]*RevokeTokenArgs, 0) } - return u.revokeRefreshTokenArgs[call] + return u.revokeTokenArgs[call] } func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { @@ -552,40 +554,40 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes ) } -func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeRefreshToken( +func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeToken( t *testing.T, expectedPerformedByUpstreamName string, - expectedArgs *RevokeRefreshTokenArgs, + expectedArgs *RevokeTokenArgs, ) { t.Helper() - var actualArgs *RevokeRefreshTokenArgs + var actualArgs *RevokeTokenArgs var actualNameOfUpstreamWhichMadeCall string actualCallCountAcrossAllOIDCUpstreams := 0 for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { - callCountOnThisUpstream := upstreamOIDC.revokeRefreshTokenCallCount + callCountOnThisUpstream := upstreamOIDC.revokeTokenCallCount actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream if callCountOnThisUpstream == 1 { actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name - actualArgs = upstreamOIDC.revokeRefreshTokenArgs[0] + actualArgs = upstreamOIDC.revokeTokenArgs[0] } } require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, - "should have been exactly one call to RevokeRefreshToken() by all OIDC upstreams", + "should have been exactly one call to RevokeToken() by all OIDC upstreams", ) require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, - "RevokeRefreshToken() was called on the wrong OIDC upstream", + "RevokeToken() was called on the wrong OIDC upstream", ) require.Equal(t, expectedArgs, actualArgs) } -func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeRefreshToken(t *testing.T) { +func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeToken(t *testing.T) { t.Helper() actualCallCountAcrossAllOIDCUpstreams := 0 for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { - actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeRefreshTokenCallCount + actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeTokenCallCount } require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, - "expected exactly zero calls to RevokeRefreshToken()", + "expected exactly zero calls to RevokeToken()", ) } @@ -610,7 +612,7 @@ type TestUpstreamOIDCIdentityProviderBuilder struct { authcodeExchangeErr error passwordGrantErr error performRefreshErr error - revokeRefreshTokenErr error + revokeTokenErr error validateTokenErr error } @@ -727,8 +729,8 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenError(err err return u } -func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { - u.revokeRefreshTokenErr = err +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { + u.revokeTokenErr = err return u } @@ -761,8 +763,8 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdent } return u.refreshedTokens, nil }, - RevokeRefreshTokenFunc: func(ctx context.Context, refreshToken string) error { - return u.revokeRefreshTokenErr + RevokeTokenFunc: func(ctx context.Context, refreshToken string, tokenType provider.RevocableTokenType) error { + return u.revokeTokenErr }, ValidateTokenFunc: func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.validateTokenErr != nil { diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 437eb6ef..310a6df4 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -137,32 +137,36 @@ func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string return p.Config.TokenSource(httpClientContext, &oauth2.Token{RefreshToken: refreshToken}).Token() } -// RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint. -func (p *ProviderConfig) RevokeRefreshToken(ctx context.Context, refreshToken string) error { +// RevokeToken will attempt to revoke the given token, if the provider has a revocation endpoint. +func (p *ProviderConfig) RevokeToken(ctx context.Context, token string, tokenType provider.RevocableTokenType) error { if p.RevocationURL == nil { - plog.Trace("RevokeRefreshToken() was called but upstream provider has no available revocation endpoint", "providerName", p.Name) + plog.Trace("RevokeToken() was called but upstream provider has no available revocation endpoint", + "providerName", p.Name, + "tokenType", tokenType, + ) return nil } // First try using client auth in the request params. - tryAnotherClientAuthMethod, err := p.tryRevokeRefreshToken(ctx, refreshToken, false) + tryAnotherClientAuthMethod, err := p.tryRevokeToken(ctx, token, tokenType, false) if tryAnotherClientAuthMethod { // Try again using basic auth this time. Overwrite the first client auth error, // which isn't useful anymore when retrying. - _, err = p.tryRevokeRefreshToken(ctx, refreshToken, true) + _, err = p.tryRevokeToken(ctx, token, tokenType, true) } return err } -// tryRevokeRefreshToken will call the revocation endpoint using either basic auth or by including +// tryRevokeToken will call the revocation endpoint using either basic auth or by including // client auth in the request params. It will return an error when the request failed. If the // request failed for a reason that might be due to bad client auth, then it will return true // for the tryAnotherClientAuthMethod return value, indicating that it might be worth trying // again using the other client auth method. // RFC 7009 defines how to make a revocation request and how to interpret the response. // See https://datatracker.ietf.org/doc/html/rfc7009#section-2.1 for details. -func (p *ProviderConfig) tryRevokeRefreshToken( +func (p *ProviderConfig) tryRevokeToken( ctx context.Context, - refreshToken string, + token string, + tokenType provider.RevocableTokenType, useBasicAuth bool, ) (tryAnotherClientAuthMethod bool, err error) { clientID := p.Config.ClientID @@ -171,8 +175,8 @@ func (p *ProviderConfig) tryRevokeRefreshToken( httpClient := p.Client params := url.Values{ - "token": []string{refreshToken}, - "token_type_hint": []string{"refresh_token"}, + "token": []string{token}, + "token_type_hint": []string{string(tokenType)}, } if !useBasicAuth { params["client_id"] = []string{clientID} @@ -200,11 +204,11 @@ func (p *ProviderConfig) tryRevokeRefreshToken( switch resp.StatusCode { case http.StatusOK: // Success! - plog.Trace("RevokeRefreshToken() got 200 OK response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) + plog.Trace("RevokeToken() got 200 OK response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) return false, nil case http.StatusBadRequest: // Bad request might be due to bad client auth method. Try to detect that. - plog.Trace("RevokeRefreshToken() got 400 Bad Request response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) + plog.Trace("RevokeToken() got 400 Bad Request response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) body, err := io.ReadAll(resp.Body) if err != nil { return false, @@ -227,11 +231,11 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } // Got an "invalid_client" response, which might mean client auth failed, so it may be worth trying again // using another client auth method. See https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 - plog.Trace("RevokeRefreshToken()'s 400 Bad Request response from provider's revocation endpoint was type 'invalid_client'", "providerName", p.Name, "usedBasicAuth", useBasicAuth) + plog.Trace("RevokeToken()'s 400 Bad Request response from provider's revocation endpoint was type 'invalid_client'", "providerName", p.Name, "usedBasicAuth", useBasicAuth) return true, err default: // Any other error is probably not due to failed client auth. - plog.Trace("RevokeRefreshToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", resp.StatusCode) + plog.Trace("RevokeToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", resp.StatusCode) return false, fmt.Errorf("server responded with status %d", resp.StatusCode) } } diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index f8906562..ecef00a5 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -24,6 +24,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.pinniped.dev/internal/mocks/mockkeyset" + "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" @@ -455,73 +456,114 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("RevokeRefreshToken", func(t *testing.T) { + t.Run("RevokeToken", func(t *testing.T) { tests := []struct { - name string - nilRevocationURL bool - statusCodes []int - returnErrBodies []string - wantErr string - wantNumRequests int + name string + tokenType provider.RevocableTokenType + nilRevocationURL bool + statusCodes []int + returnErrBodies []string + wantErr string + wantNumRequests int + wantTokenTypeHint string }{ { - name: "success without calling the server when there is no revocation URL set", + name: "success without calling the server when there is no revocation URL set for refresh token", + tokenType: provider.RefreshTokenType, nilRevocationURL: true, wantNumRequests: 0, }, { - name: "success when the server returns 200 OK on the first call", - statusCodes: []int{http.StatusOK}, - wantNumRequests: 1, + name: "success without calling the server when there is no revocation URL set for access token", + tokenType: provider.AccessTokenType, + nilRevocationURL: true, + wantNumRequests: 0, }, { - name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call", + name: "success when the server returns 200 OK on the first call for refresh token", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusOK}, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", + }, + { + name: "success when the server returns 200 OK on the first call for access token", + tokenType: provider.AccessTokenType, + statusCodes: []int{http.StatusOK}, + wantNumRequests: 1, + wantTokenTypeHint: "access_token", + }, + { + name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for refresh token", + tokenType: provider.RefreshTokenType, statusCodes: []int{http.StatusBadRequest, http.StatusOK}, // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure - returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`}, - wantNumRequests: 2, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`}, + wantNumRequests: 2, + wantTokenTypeHint: "refresh_token", }, { - name: "error when the server returns 400 Bad Request on the first call due to client auth, then any 400 error on second call", - statusCodes: []int{http.StatusBadRequest, http.StatusBadRequest}, - returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, `{ "error":"anything", "error_description":"unhappy" }`}, - wantErr: `server responded with status 400 with body: { "error":"anything", "error_description":"unhappy" }`, - wantNumRequests: 2, + name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for access token", + tokenType: provider.AccessTokenType, + statusCodes: []int{http.StatusBadRequest, http.StatusOK}, + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`}, + wantNumRequests: 2, + wantTokenTypeHint: "access_token", }, { - name: "error when the server returns 400 Bad Request with bad JSON body on the first call", - statusCodes: []int{http.StatusBadRequest}, - returnErrBodies: []string{`invalid JSON body`}, - wantErr: `error parsing response body "invalid JSON body" on response with status code 400: invalid character 'i' looking for beginning of value`, - wantNumRequests: 1, + name: "error when the server returns 400 Bad Request on the first call due to client auth, then any 400 error on second call", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusBadRequest, http.StatusBadRequest}, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, `{ "error":"anything", "error_description":"unhappy" }`}, + wantErr: `server responded with status 400 with body: { "error":"anything", "error_description":"unhappy" }`, + wantNumRequests: 2, + wantTokenTypeHint: "refresh_token", }, { - name: "error when the server returns 400 Bad Request with empty body", - statusCodes: []int{http.StatusBadRequest}, - returnErrBodies: []string{``}, - wantErr: `error parsing response body "" on response with status code 400: unexpected end of JSON input`, - wantNumRequests: 1, + name: "error when the server returns 400 Bad Request with bad JSON body on the first call", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{`invalid JSON body`}, + wantErr: `error parsing response body "invalid JSON body" on response with status code 400: invalid character 'i' looking for beginning of value`, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", }, { - name: "error when the server returns 400 Bad Request on the first call due to client auth, then any other error on second call", - statusCodes: []int{http.StatusBadRequest, http.StatusForbidden}, - returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""}, - wantErr: "server responded with status 403", - wantNumRequests: 2, + name: "error when the server returns 400 Bad Request with empty body", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{``}, + wantErr: `error parsing response body "" on response with status code 400: unexpected end of JSON input`, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", }, { - name: "error when server returns any other 400 error on first call", - statusCodes: []int{http.StatusBadRequest}, - returnErrBodies: []string{`{ "error":"anything_else", "error_description":"unhappy" }`}, - wantErr: `server responded with status 400 with body: { "error":"anything_else", "error_description":"unhappy" }`, - wantNumRequests: 1, + name: "error when the server returns 400 Bad Request on the first call due to client auth, then any other error on second call", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusBadRequest, http.StatusForbidden}, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""}, + wantErr: "server responded with status 403", + wantNumRequests: 2, + wantTokenTypeHint: "refresh_token", }, { - name: "error when server returns any other error aside from 400 on first call", - statusCodes: []int{http.StatusForbidden}, - returnErrBodies: []string{""}, - wantErr: "server responded with status 403", - wantNumRequests: 1, + name: "error when server returns any other 400 error on first call", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{`{ "error":"anything_else", "error_description":"unhappy" }`}, + wantErr: `server responded with status 400 with body: { "error":"anything_else", "error_description":"unhappy" }`, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", + }, + { + name: "error when server returns any other error aside from 400 on first call", + tokenType: provider.RefreshTokenType, + statusCodes: []int{http.StatusForbidden}, + returnErrBodies: []string{""}, + wantErr: "server responded with status 403", + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", }, } for _, tt := range tests { @@ -536,15 +578,15 @@ func TestProviderConfig(t *testing.T) { if numRequests == 1 { // First request should use client_id/client_secret params. require.Equal(t, 4, len(r.Form)) + require.Equal(t, "test-upstream-token", r.Form.Get("token")) + require.Equal(t, tt.wantTokenTypeHint, r.Form.Get("token_type_hint")) require.Equal(t, "test-client-id", r.Form.Get("client_id")) require.Equal(t, "test-client-secret", r.Form.Get("client_secret")) - require.Equal(t, "refresh_token", r.Form.Get("token_type_hint")) - require.Equal(t, "test-initial-refresh-token", r.Form.Get("token")) } else { // Second request, if there is one, should use basic auth. require.Equal(t, 2, len(r.Form)) - require.Equal(t, "refresh_token", r.Form.Get("token_type_hint")) - require.Equal(t, "test-initial-refresh-token", r.Form.Get("token")) + require.Equal(t, "test-upstream-token", r.Form.Get("token")) + require.Equal(t, tt.wantTokenTypeHint, r.Form.Get("token_type_hint")) username, password, hasBasicAuth := r.BasicAuth() require.True(t, hasBasicAuth, "request should have had basic auth but did not") require.Equal(t, "test-client-id", username) @@ -574,9 +616,10 @@ func TestProviderConfig(t *testing.T) { p.RevocationURL = nil } - err = p.RevokeRefreshToken( + err = p.RevokeToken( context.Background(), - "test-initial-refresh-token", + "test-upstream-token", + tt.tokenType, ) require.Equal(t, tt.wantNumRequests, numRequests, From 46008a72355c4c0df05768eb4be78e46a10f772a Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 6 Dec 2021 14:43:39 -0800 Subject: [PATCH 2/4] Add struct field for storing upstream access token in downstream session --- .../accesstoken/accesstoken_test.go | 4 ++-- .../authorizationcode/authorizationcode.go | 17 ++++++++++------- .../authorizationcode/authorizationcode_test.go | 6 ++++-- .../openidconnect/openidconnect_test.go | 2 +- internal/fositestorage/pkce/pkce_test.go | 2 +- .../refreshtoken/refreshtoken_test.go | 4 ++-- internal/psession/pinniped_session.go | 12 ++++++++++++ 7 files changed, 32 insertions(+), 15 deletions(-) diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index 5f3933df..de475a6a 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -53,7 +53,7 @@ func TestAccessTokenStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/access-token", @@ -122,7 +122,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/access-token", diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 50c35788..93db45ad 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -345,23 +345,26 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ "providerName": "nŐǛ3", "providerType": "闣ʬ橳(ý綃ʃʚƟ覣k眐4Ĉt", "oidc": { - "upstreamRefreshToken": "嵽痊w©Ź榨Q|ôɵt毇妬" + "upstreamRefreshToken": "嵽痊w©Ź榨Q|ôɵt毇妬", + "upstreamAccessToken": "巈環_ɑ" }, "ldap": { - "userDN": "6鉢緋uƴŤȱʀļÂ?墖\u003cƬb獭潜Ʃ饾" + "userDN": "ƍ蛊ʚ£:設虝27" }, "activedirectory": { - "userDN": "|鬌R蜚蠣麹概÷驣7Ʀ澉1æɽ誮rʨ鷞" + "userDN": "伒犘c钡ɏȫ" } } }, "requestedAudience": [ - "ŚB碠k9" + "š%OpKȱ藚ɏ¬Ê蒭堜", + "ɽ誮rʨ鷞aŚB碠k", + "Ċi磊ůď逳鞪?3)藵睋" ], "grantedAudience": [ - "ʘ赱", - "ď逳鞪?3)藵睋邔\u0026Ű惫蜀Ģ¡圔", - "墀jMʥ" + "\u0026Ű惫蜀Ģ¡圔", + "墀jMʥ", + "+î艔垎0" ] }, "version": "2" diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index fdb9653e..d5502015 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -65,7 +65,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -84,7 +84,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -393,6 +393,8 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { // the fuzzed session and storage session should have identical JSON require.JSONEq(t, authorizeCodeSessionJSONFromFuzzing, authorizeCodeSessionJSONFromStorage) + // t.Log("actual value from fuzzing", authorizeCodeSessionJSONFromFuzzing) // can be useful when updating expected value + // while the fuzzer will panic if AuthorizeRequest changes in a way that cannot be fuzzed, // if it adds a new field that can be fuzzed, this check will fail // thus if AuthorizeRequest changes, we will detect it here (though we could possibly miss an omitempty field) diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go index ba76fe8e..4970ce9c 100644 --- a/internal/fositestorage/openidconnect/openidconnect_test.go +++ b/internal/fositestorage/openidconnect/openidconnect_test.go @@ -52,7 +52,7 @@ func TestOpenIdConnectStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/oidc", diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go index d57649be..8a2f4e50 100644 --- a/internal/fositestorage/pkce/pkce_test.go +++ b/internal/fositestorage/pkce/pkce_test.go @@ -52,7 +52,7 @@ func TestPKCEStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/pkce", diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index 92c728e9..1a0d854b 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -52,7 +52,7 @@ func TestRefreshTokenStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", @@ -122,7 +122,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamAccessToken":""}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index 8009f91d..970a4e64 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -61,7 +61,19 @@ const ( // OIDCSessionData is the additional data needed by Pinniped when the upstream IDP is an OIDC provider. type OIDCSessionData struct { + // UpstreamRefreshToken will contain the refresh token from the upstream OIDC provider, if the upstream provider + // returned a refresh token during initial authorization. Otherwise, this field should be empty + // and the UpstreamAccessToken should be non-empty. We may not get a refresh token from the upstream provider, + // but we should always get an access token. However, when we do get a refresh token there is no need to + // also store the access token, since storing unnecessary tokens makes it possible for them to be leaked and + // creates more upstream revocation HTTP requests when it comes time to revoke the stored tokens. UpstreamRefreshToken string `json:"upstreamRefreshToken"` + + // UpstreamAccessToken will contain the access token returned by the upstream OIDC provider during initial + // authorization, but only when the provider did not also return a refresh token. When UpstreamRefreshToken is + // non-empty, then this field should be empty, indicating that we should use the upstream refresh token during + // downstream refresh. + UpstreamAccessToken string `json:"upstreamAccessToken"` } // LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider. From f410d2bd00802cd9ab1f3775d59511b3bf108bc5 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 8 Dec 2021 14:29:25 -0800 Subject: [PATCH 3/4] Add revocation of upstream access tokens to garbage collector Also refactor the code that decides which types of revocation failures are worth retrying. Be more selective by only retrying those types of errors that are likely to be worth retrying. --- .../supervisorstorage/garbage_collector.go | 106 ++--- .../garbage_collector_test.go | 387 +++++++++++++++++- .../provider/dynamic_upstream_idp_provider.go | 20 + internal/upstreamoidc/upstreamoidc.go | 41 +- internal/upstreamoidc/upstreamoidc_test.go | 197 ++++++--- 5 files changed, 625 insertions(+), 126 deletions(-) diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index 5f388b80..fb48ac4e 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -112,25 +112,30 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { timeString, ok := secret.Annotations[crud.SecretLifetimeAnnotationKey] if !ok { + // Secret did not request garbage collection via annotations, so skip deletion. continue } garbageCollectAfterTime, err := time.Parse(crud.SecretLifetimeAnnotationDateFormat, timeString) if err != nil { plog.WarningErr("could not parse resource timestamp for garbage collection", err, logKV(secret)...) + // Can't tell if the Secret has expired or not, so skip deletion. continue } if !garbageCollectAfterTime.Before(frozenClock.Now()) { - // not old enough yet + // Secret is not old enough yet, so skip deletion. continue } + // The Secret has expired. Check if it is a downstream session storage Secret, which may require extra processing. storageType, isSessionStorage := secret.Labels[crud.SecretLabelKey] if isSessionStorage { - err := c.maybeRevokeUpstreamOIDCRefreshToken(ctx.Context, storageType, secret) - if err != nil { - plog.WarningErr("garbage collector could not revoke upstream refresh token", err, logKV(secret)...) + revokeErr := c.maybeRevokeUpstreamOIDCToken(ctx.Context, storageType, secret) + if revokeErr != nil { + plog.WarningErr("garbage collector could not revoke upstream OIDC token", revokeErr, logKV(secret)...) + // Note that RevokeToken (called by the private helper) might have returned an error of type + // provider.RetryableRevocationError, in which case we would like to retry the revocation later. // If the error is of a type that is worth retrying, then do not delete the Secret right away. // A future call to Sync will try revocation again for that secret. However, if the Secret is // getting too old, then just delete it anyway. We don't want to extend the lifetime of these @@ -138,13 +143,15 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { // cleaning them out of etcd storage. fourHoursAgo := frozenClock.Now().Add(-4 * time.Hour) nowIsLessThanFourHoursBeyondSecretGCTime := garbageCollectAfterTime.After(fourHoursAgo) - if errors.As(err, &retryableRevocationError{}) && nowIsLessThanFourHoursBeyondSecretGCTime { + if errors.As(revokeErr, &provider.RetryableRevocationError{}) && nowIsLessThanFourHoursBeyondSecretGCTime { // Hasn't been very long since secret expired, so skip deletion to try revocation again later. + plog.Trace("garbage collector keeping Secret to retry upstream OIDC token revocation later", logKV(secret)...) continue } } } + // Garbage collect the Secret. err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{ Preconditions: &metav1.Preconditions{ UID: &secret.UID, @@ -161,30 +168,36 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { return nil } -func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx context.Context, storageType string, secret *v1.Secret) error { - // All session storage types hold upstream refresh tokens when the upstream IDP is an OIDC provider. +func (c *garbageCollectorController) maybeRevokeUpstreamOIDCToken(ctx context.Context, storageType string, secret *v1.Secret) error { + // All downstream session storage types hold upstream tokens when the upstream IDP is an OIDC provider. // However, some of them will be outdated because they are not updated by fosite after creation. // Our goal below is to always revoke the latest upstream refresh token that we are holding for the - // session, and only the latest. + // session, and only the latest, or to revoke the original upstream access token. Note that we don't + // bother to store new upstream access tokens seen during upstream refresh because we only need to store + // the upstream access token when we intend to use it *instead* of an upstream refresh token. + // This implies that all the storage types will contain a copy of the original upstream access token, + // since it is never updated in the session. Thus, we can use the same logic to decide which upstream + // access token to revoke as we use for upstream refresh tokens, which allows us to avoid revoking an + // upstream access token more than once. switch storageType { case authorizationcode.TypeLabelValue: authorizeCodeSession, err := authorizationcode.ReadFromSecret(secret) if err != nil { return err } - // Check if this downstream authcode was already used. If it was already used (i.e. not active anymore), then - // the latest upstream refresh token can be found in one of the other storage types handled below instead. + // Check if this downstream authcode was already used. If it was already used (i.e. not active anymore), + // then the latest upstream token can be found in one of the other storage types handled below instead. if !authorizeCodeSession.Active { return nil } - // When the downstream authcode was never used, then its storage must contain the latest upstream refresh token. - return c.revokeUpstreamOIDCRefreshToken(ctx, authorizeCodeSession.Request.Session.(*psession.PinnipedSession).Custom, secret) + // When the downstream authcode was never used, then its storage must contain the latest upstream token. + return c.tryRevokeUpstreamOIDCToken(ctx, authorizeCodeSession.Request.Session.(*psession.PinnipedSession).Custom, secret) case accesstoken.TypeLabelValue: // For access token storage, check if the "offline_access" scope was granted on the downstream session. - // If it was granted, then the latest upstream refresh token should be found in the refresh token storage instead. + // If it was granted, then the latest upstream token should be found in the refresh token storage instead. // If it was not granted, then the user could not possibly have performed a downstream refresh, so the - // access token storage has the latest version of the upstream refresh token. + // access token storage has the latest version of the upstream token. accessTokenSession, err := accesstoken.ReadFromSecret(secret) if err != nil { return err @@ -193,29 +206,29 @@ func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx con if slices.Contains(accessTokenSession.Request.GetGrantedScopes(), coreosoidc.ScopeOfflineAccess) { return nil } - return c.revokeUpstreamOIDCRefreshToken(ctx, pinnipedSession.Custom, secret) + return c.tryRevokeUpstreamOIDCToken(ctx, pinnipedSession.Custom, secret) case refreshtoken.TypeLabelValue: - // For refresh token storage, always revoke its upstream refresh token. This refresh token storage could - // be the result of the initial downstream authcode exchange, or it could be the result of a downstream - // refresh. Either way, it always contains the latest upstream refresh token when it exists. + // For refresh token storage, always revoke its upstream token. This refresh token storage could be + // the result of the initial downstream authcode exchange, or it could be the result of a downstream + // refresh. Either way, it always contains the latest upstream token when it exists. refreshTokenSession, err := refreshtoken.ReadFromSecret(secret) if err != nil { return err } - return c.revokeUpstreamOIDCRefreshToken(ctx, refreshTokenSession.Request.Session.(*psession.PinnipedSession).Custom, secret) + return c.tryRevokeUpstreamOIDCToken(ctx, refreshTokenSession.Request.Session.(*psession.PinnipedSession).Custom, secret) case pkce.TypeLabelValue: - // For PKCE storage, its very existence means that the authcode was never exchanged, because these - // are deleted during authcode exchange. No need to do anything, since the upstream refresh token - // revocation is handled by authcode storage case above. + // For PKCE storage, its very existence means that the downstream authcode was never exchanged, because + // these are deleted during downstream authcode exchange. No need to do anything, since the upstream + // token revocation is handled by authcode storage case above. return nil case openidconnect.TypeLabelValue: // For OIDC storage, there is no need to do anything for reasons similar to the PKCE storage. - // These are not deleted during authcode exchange, probably due to a bug in fosite, even though it - // will never be read or updated again. However, the refresh token contained inside will be revoked - // by one of the other cases above. + // These are not deleted during downstream authcode exchange, probably due to a bug in fosite, even + // though it will never be read or updated again. However, the upstream token contained inside will + // be revoked by one of the other cases above. return nil default: @@ -224,8 +237,8 @@ func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx con } } -func (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context.Context, customSessionData *psession.CustomSessionData, secret *v1.Secret) error { - // When session was for another upstream IDP type, e.g. LDAP, there is no upstream OIDC refresh token involved. +func (c *garbageCollectorController) tryRevokeUpstreamOIDCToken(ctx context.Context, customSessionData *psession.CustomSessionData, secret *v1.Secret) error { + // When session was for another upstream IDP type, e.g. LDAP, there is no upstream OIDC token involved. if customSessionData.ProviderType != psession.ProviderTypeOIDC { return nil } @@ -242,32 +255,29 @@ func (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context. return fmt.Errorf("could not find upstream OIDC provider named %q with resource UID %q", customSessionData.ProviderName, customSessionData.ProviderUID) } - // Revoke the upstream refresh token. This is a noop if the upstream provider does not offer a revocation endpoint. - err := foundOIDCIdentityProviderI.RevokeToken(ctx, customSessionData.OIDC.UpstreamRefreshToken, provider.RefreshTokenType) - if err != nil { - // This could be a network failure, a 503 result which we should retry - // (see https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1), - // or any other non-200 response from the revocation endpoint. - // Regardless of which, it is probably worth retrying. - return retryableRevocationError{wrapped: err} + // In practice, there should only be one of these tokens saved in the session. + upstreamRefreshToken := customSessionData.OIDC.UpstreamRefreshToken + upstreamAccessToken := customSessionData.OIDC.UpstreamAccessToken + + if upstreamRefreshToken != "" { + err := foundOIDCIdentityProviderI.RevokeToken(ctx, upstreamRefreshToken, provider.RefreshTokenType) + if err != nil { + return err + } + plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret)...) + } + + if upstreamAccessToken != "" { + err := foundOIDCIdentityProviderI.RevokeToken(ctx, upstreamAccessToken, provider.AccessTokenType) + if err != nil { + return err + } + plog.Trace("garbage collector successfully revoked upstream OIDC access token (or provider has no revocation endpoint)", logKV(secret)...) } - plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret)...) return nil } -type retryableRevocationError struct { - wrapped error -} - -func (e retryableRevocationError) Error() string { - return fmt.Sprintf("retryable revocation error: %v", e.wrapped) -} - -func (e retryableRevocationError) Unwrap() error { - return e.wrapped -} - func logKV(secret *v1.Secret) []interface{} { return []interface{}{ "secretName", secret.Name, diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index d442c474..84f293c6 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -271,7 +271,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) - when("there are valid, expired authcode secrets", func() { + when("there are valid, expired authcode secrets which contain upstream refresh tokens", func() { it.Before(func() { activeOIDCAuthcodeSession := &authorizationcode.Session{ Version: "2", @@ -400,6 +400,135 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) + when("there are valid, expired authcode secrets which contain upstream access tokens", func() { + it.Before(func() { + activeOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: "fake-upstream-access-token", + }, + }, + }, + }, + } + activeOIDCAuthcodeSessionJSON, err := json.Marshal(activeOIDCAuthcodeSession) + r.NoError(err) + activeOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "activeOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": activeOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(activeOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + + inactiveOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: false, + Request: &fosite.Request{ + ID: "request-id-2", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: "other-fake-upstream-access-token", + }, + }, + }, + }, + } + inactiveOIDCAuthcodeSessionJSON, err := json.Marshal(inactiveOIDCAuthcodeSession) + r.NoError(err) + inactiveOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "inactiveOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-456", + ResourceVersion: "rv-456", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": inactiveOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(inactiveOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(inactiveOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(inactiveOIDCAuthcodeSessionSecret)) + }) + + it("should revoke upstream tokens only from the active authcode secrets and delete them all", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // The upstream refresh token is only revoked for the active authcode session. + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-access-token", + TokenType: provider.AccessTokenType, + }, + ) + + // Both authcode session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + testutil.NewPreconditions("uid-456", "rv-456"), + }, + *deleteOptions, + ) + }) + }) + when("there is an invalid, expired authcode secret", func() { it.Before(func() { invalidOIDCAuthcodeSession := &authorizationcode.Session{ @@ -674,11 +803,12 @@ func TestGarbageCollectorControllerSync(t *testing.T) { r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) }) - it("keeps the secret for a while longer so the revocation can be retried on a future sync", func() { + it("keeps the secret for a while longer so the revocation can be retried on a future sync for retryable errors", func() { happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName("upstream-oidc-provider-name"). WithResourceUID("upstream-oidc-provider-uid"). - WithRevokeTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail + // make the upstream revocation fail in a retryable way + WithRevokeTokenError(provider.NewRetryableRevocationError(errors.New("some retryable upstream revocation error"))) idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) startInformersAndController(idpListerBuilder.Build()) @@ -697,6 +827,42 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // The authcode session secrets is not deleted. r.Empty(kubeClient.Actions()) }) + + it("deletes the secret for non-retryable errors", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + // make the upstream revocation fail in a non-retryable way + WithRevokeTokenError(errors.New("some upstream revocation error not worth retrying")) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // Tried to revoke it, although this revocation will fail. + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-refresh-token", + TokenType: provider.RefreshTokenType, + }, + ) + + // The authcode session secrets is still deleted because it is expired and the revocation error is not retryable. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) }) when("there is a valid, long-since expired authcode secret but the upstream revocation fails", func() { @@ -783,7 +949,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) - when("there are valid, expired access token secrets", func() { + when("there are valid, expired access token secrets which contain upstream refresh tokens", func() { it.Before(func() { offlineAccessGrantedOIDCAccessTokenSession := &accesstoken.Session{ Version: "2", @@ -912,7 +1078,136 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) - when("there are valid, expired refresh secrets", func() { + when("there are valid, expired access token secrets which contain upstream access tokens", func() { + it.Before(func() { + offlineAccessGrantedOIDCAccessTokenSession := &accesstoken.Session{ + Version: "2", + Request: &fosite.Request{ + GrantedScope: fosite.Arguments{"scope1", "scope2", "offline_access"}, + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: "offline-access-granted-fake-upstream-access-token", + }, + }, + }, + }, + } + offlineAccessGrantedOIDCAccessTokenSessionJSON, err := json.Marshal(offlineAccessGrantedOIDCAccessTokenSession) + r.NoError(err) + offlineAccessGrantedOIDCAccessTokenSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "offlineAccessGrantedOIDCAccessTokenSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": accesstoken.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": offlineAccessGrantedOIDCAccessTokenSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + accesstoken.TypeLabelValue, + } + _, err = accesstoken.ReadFromSecret(offlineAccessGrantedOIDCAccessTokenSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid accesstoken secret") + r.NoError(kubeInformerClient.Tracker().Add(offlineAccessGrantedOIDCAccessTokenSessionSecret)) + r.NoError(kubeClient.Tracker().Add(offlineAccessGrantedOIDCAccessTokenSessionSecret)) + + offlineAccessNotGrantedOIDCAccessTokenSession := &accesstoken.Session{ + Version: "2", + Request: &fosite.Request{ + GrantedScope: fosite.Arguments{"scope1", "scope2"}, + ID: "request-id-2", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: "fake-upstream-access-token", + }, + }, + }, + }, + } + offlineAccessNotGrantedOIDCAccessTokenSessionJSON, err := json.Marshal(offlineAccessNotGrantedOIDCAccessTokenSession) + r.NoError(err) + offlineAccessNotGrantedOIDCAccessTokenSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "offlineAccessNotGrantedOIDCAccessTokenSession", + Namespace: installedInNamespace, + UID: "uid-456", + ResourceVersion: "rv-456", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": accesstoken.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": offlineAccessNotGrantedOIDCAccessTokenSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + accesstoken.TypeLabelValue, + } + _, err = accesstoken.ReadFromSecret(offlineAccessNotGrantedOIDCAccessTokenSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid accesstoken secret") + r.NoError(kubeInformerClient.Tracker().Add(offlineAccessNotGrantedOIDCAccessTokenSessionSecret)) + r.NoError(kubeClient.Tracker().Add(offlineAccessNotGrantedOIDCAccessTokenSessionSecret)) + }) + + it("should revoke upstream tokens only from the active authcode secrets and delete them all", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // The upstream refresh token is only revoked for the downstream session which had offline_access granted. + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-access-token", + TokenType: provider.AccessTokenType, + }, + ) + + // Both session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + testutil.NewPreconditions("uid-456", "rv-456"), + }, + *deleteOptions, + ) + }) + }) + + when("there are valid, expired refresh secrets which contain upstream refresh tokens", func() { it.Before(func() { oidcRefreshSession := &refreshtoken.Session{ Version: "2", @@ -994,6 +1289,88 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) + when("there are valid, expired refresh secrets which contain upstream access tokens", func() { + it.Before(func() { + oidcRefreshSession := &refreshtoken.Session{ + Version: "2", + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: "fake-upstream-access-token", + }, + }, + }, + }, + } + oidcRefreshSessionJSON, err := json.Marshal(oidcRefreshSession) + r.NoError(err) + oidcRefreshSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oidcRefreshSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": refreshtoken.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": oidcRefreshSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + refreshtoken.TypeLabelValue, + } + _, err = refreshtoken.ReadFromSecret(oidcRefreshSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid refresh token secret") + r.NoError(kubeInformerClient.Tracker().Add(oidcRefreshSessionSecret)) + r.NoError(kubeClient.Tracker().Add(oidcRefreshSessionSecret)) + }) + + it("should revoke upstream tokens from the secrets and delete them all", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // The upstream refresh token is revoked. + idpListerBuilder.RequireExactlyOneCallToRevokeToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeTokenArgs{ + Ctx: syncContext.Context, + Token: "fake-upstream-access-token", + TokenType: provider.AccessTokenType, + }, + ) + + // The secret is deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "oidcRefreshSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + when("very little time has passed since the previous sync call", func() { it.Before(func() { // Add a secret that will expire in 20 seconds. diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index e5be51ff..78ed7998 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -5,6 +5,7 @@ package provider import ( "context" + "fmt" "net/url" "sync" @@ -77,6 +78,9 @@ type UpstreamOIDCIdentityProviderI interface { PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) // RevokeToken will attempt to revoke the given token, if the provider has a revocation endpoint. + // It may return an error wrapped by a RetryableRevocationError, which is an error indicating that it may + // be worth trying to revoke the same token again later. Any other error returned should be assumed to + // represent an error such that it is not worth retrying revocation later, even though revocation failed. RevokeToken(ctx context.Context, token string, tokenType RevocableTokenType) error // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response @@ -163,3 +167,19 @@ func (p *dynamicUpstreamIDPProvider) GetActiveDirectoryIdentityProviders() []Ups defer p.mutex.RUnlock() return p.activeDirectoryUpstreams } + +type RetryableRevocationError struct { + wrapped error +} + +func NewRetryableRevocationError(wrapped error) RetryableRevocationError { + return RetryableRevocationError{wrapped: wrapped} +} + +func (e RetryableRevocationError) Error() string { + return fmt.Sprintf("retryable revocation error: %v", e.wrapped) +} + +func (e RetryableRevocationError) Unwrap() error { + return e.wrapped +} diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 310a6df4..d3f24b9f 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -138,6 +138,9 @@ func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string } // RevokeToken will attempt to revoke the given token, if the provider has a revocation endpoint. +// It may return an error wrapped by a RetryableRevocationError, which is an error indicating that it may +// be worth trying to revoke the same token again later. Any other error returned should be assumed to +// represent an error such that it is not worth retrying revocation later, even though revocation failed. func (p *ProviderConfig) RevokeToken(ctx context.Context, token string, tokenType provider.RevocableTokenType) error { if p.RevocationURL == nil { plog.Trace("RevokeToken() was called but upstream provider has no available revocation endpoint", @@ -197,22 +200,25 @@ func (p *ProviderConfig) tryRevokeToken( resp, err := httpClient.Do(req) if err != nil { // Couldn't connect to the server or some similar error. - return false, err + // Could be a temporary network problem, so it might be worth retrying. + return false, provider.NewRetryableRevocationError(err) } defer resp.Body.Close() - switch resp.StatusCode { - case http.StatusOK: + status := resp.StatusCode + + switch { + case status == http.StatusOK: // Success! plog.Trace("RevokeToken() got 200 OK response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) return false, nil - case http.StatusBadRequest: + case status == http.StatusBadRequest: // Bad request might be due to bad client auth method. Try to detect that. plog.Trace("RevokeToken() got 400 Bad Request response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) body, err := io.ReadAll(resp.Body) if err != nil { return false, - fmt.Errorf("error reading response body on response with status code %d: %w", resp.StatusCode, err) + fmt.Errorf("error reading response body on response with status code %d: %w", status, err) } var parsedResp struct { ErrorType string `json:"error"` @@ -222,21 +228,34 @@ func (p *ProviderConfig) tryRevokeToken( err = json.Unmarshal(body, &parsedResp) if err != nil { return false, - fmt.Errorf("error parsing response body %q on response with status code %d: %w", bodyStr, resp.StatusCode, err) + fmt.Errorf("error parsing response body %q on response with status code %d: %w", bodyStr, status, err) } - err = fmt.Errorf("server responded with status %d with body: %s", resp.StatusCode, bodyStr) + err = fmt.Errorf("server responded with status %d with body: %s", status, bodyStr) if parsedResp.ErrorType != "invalid_client" { - // Got an error unrelated to client auth, so not worth trying again. + // Got an error unrelated to client auth, so not worth trying client auth again. Also, these are errors + // of the type where the server is pretty conclusively rejecting our request, so they are generally + // not worth trying again later either. + // These errors could be any of the other errors from https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + // or "unsupported_token_type" from https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1 + // or could be some unspecified custom error added by the OIDC provider. return false, err } // Got an "invalid_client" response, which might mean client auth failed, so it may be worth trying again // using another client auth method. See https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 plog.Trace("RevokeToken()'s 400 Bad Request response from provider's revocation endpoint was type 'invalid_client'", "providerName", p.Name, "usedBasicAuth", useBasicAuth) return true, err + case status >= 500 && status <= 599: + // The spec says 503 Service Unavailable should be retried by the client later. + // See https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1. + // Other forms of 5xx server errors are not particularly conclusive failures. For example, gateway errors could + // be caused by an underlying problem which could potentially become resolved in the near future. We'll be + // optimistic and call all 5xx errors retryable. + plog.Trace("RevokeToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", status) + return false, provider.NewRetryableRevocationError(fmt.Errorf("server responded with status %d", status)) default: - // Any other error is probably not due to failed client auth. - plog.Trace("RevokeToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", resp.StatusCode) - return false, fmt.Errorf("server responded with status %d", resp.StatusCode) + // Any other error is probably not due to failed client auth, and is probably not worth retrying later. + plog.Trace("RevokeToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", status) + return false, fmt.Errorf("server responded with status %d", status) } } diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index ecef00a5..00e6c6e7 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -458,14 +458,17 @@ func TestProviderConfig(t *testing.T) { t.Run("RevokeToken", func(t *testing.T) { tests := []struct { - name string - tokenType provider.RevocableTokenType - nilRevocationURL bool - statusCodes []int - returnErrBodies []string - wantErr string - wantNumRequests int - wantTokenTypeHint string + name string + tokenType provider.RevocableTokenType + nilRevocationURL bool + unreachableServer bool + returnStatusCodes []int + returnErrBodies []string + wantErr string + wantErrRegexp string // use either wantErr or wantErrRegexp + wantRetryableErrType bool // additionally assert error type when wantErr is non-empty + wantNumRequests int + wantTokenTypeHint string }{ { name: "success without calling the server when there is no revocation URL set for refresh token", @@ -482,88 +485,142 @@ func TestProviderConfig(t *testing.T) { { name: "success when the server returns 200 OK on the first call for refresh token", tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusOK}, + returnStatusCodes: []int{http.StatusOK}, wantNumRequests: 1, wantTokenTypeHint: "refresh_token", }, { name: "success when the server returns 200 OK on the first call for access token", tokenType: provider.AccessTokenType, - statusCodes: []int{http.StatusOK}, + returnStatusCodes: []int{http.StatusOK}, wantNumRequests: 1, wantTokenTypeHint: "access_token", }, { - name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for refresh token", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusBadRequest, http.StatusOK}, + name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for refresh token", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusBadRequest, http.StatusOK}, // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`}, wantNumRequests: 2, wantTokenTypeHint: "refresh_token", }, { - name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for access token", - tokenType: provider.AccessTokenType, - statusCodes: []int{http.StatusBadRequest, http.StatusOK}, + name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for access token", + tokenType: provider.AccessTokenType, + returnStatusCodes: []int{http.StatusBadRequest, http.StatusOK}, // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`}, wantNumRequests: 2, wantTokenTypeHint: "access_token", }, { - name: "error when the server returns 400 Bad Request on the first call due to client auth, then any 400 error on second call", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusBadRequest, http.StatusBadRequest}, - returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, `{ "error":"anything", "error_description":"unhappy" }`}, - wantErr: `server responded with status 400 with body: { "error":"anything", "error_description":"unhappy" }`, - wantNumRequests: 2, - wantTokenTypeHint: "refresh_token", + name: "error when the server returns 400 Bad Request on the first call due to client auth, then any 400 error on second call", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusBadRequest, http.StatusBadRequest}, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, `{ "error":"anything", "error_description":"unhappy" }`}, + wantErr: `server responded with status 400 with body: { "error":"anything", "error_description":"unhappy" }`, + wantRetryableErrType: false, + wantNumRequests: 2, + wantTokenTypeHint: "refresh_token", }, { - name: "error when the server returns 400 Bad Request with bad JSON body on the first call", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusBadRequest}, - returnErrBodies: []string{`invalid JSON body`}, - wantErr: `error parsing response body "invalid JSON body" on response with status code 400: invalid character 'i' looking for beginning of value`, - wantNumRequests: 1, - wantTokenTypeHint: "refresh_token", + name: "error when the server returns 400 Bad Request with bad JSON body on the first call", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{`invalid JSON body`}, + wantErr: `error parsing response body "invalid JSON body" on response with status code 400: invalid character 'i' looking for beginning of value`, + wantRetryableErrType: false, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", }, { - name: "error when the server returns 400 Bad Request with empty body", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusBadRequest}, - returnErrBodies: []string{``}, - wantErr: `error parsing response body "" on response with status code 400: unexpected end of JSON input`, - wantNumRequests: 1, - wantTokenTypeHint: "refresh_token", + name: "error when the server returns 400 Bad Request with empty body", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{``}, + wantErr: `error parsing response body "" on response with status code 400: unexpected end of JSON input`, + wantRetryableErrType: false, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", }, { - name: "error when the server returns 400 Bad Request on the first call due to client auth, then any other error on second call", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusBadRequest, http.StatusForbidden}, - returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""}, - wantErr: "server responded with status 403", - wantNumRequests: 2, - wantTokenTypeHint: "refresh_token", + name: "error when the server returns 400 Bad Request on the first call due to client auth, then any other error on second call", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusBadRequest, http.StatusForbidden}, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""}, + wantErr: "server responded with status 403", + wantRetryableErrType: false, + wantNumRequests: 2, + wantTokenTypeHint: "refresh_token", }, { - name: "error when server returns any other 400 error on first call", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusBadRequest}, - returnErrBodies: []string{`{ "error":"anything_else", "error_description":"unhappy" }`}, - wantErr: `server responded with status 400 with body: { "error":"anything_else", "error_description":"unhappy" }`, - wantNumRequests: 1, - wantTokenTypeHint: "refresh_token", + name: "error when server returns any other 400 error on first call", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{`{ "error":"anything_else", "error_description":"unhappy" }`}, + wantErr: `server responded with status 400 with body: { "error":"anything_else", "error_description":"unhappy" }`, + wantRetryableErrType: false, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", }, { - name: "error when server returns any other error aside from 400 on first call", - tokenType: provider.RefreshTokenType, - statusCodes: []int{http.StatusForbidden}, - returnErrBodies: []string{""}, - wantErr: "server responded with status 403", - wantNumRequests: 1, - wantTokenTypeHint: "refresh_token", + name: "error when server returns any other error aside from 400 on first call", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusForbidden}, + returnErrBodies: []string{""}, + wantErr: "server responded with status 403", + wantRetryableErrType: false, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", + }, + { + name: "retryable error when server returns 503 on first call", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusServiceUnavailable}, // 503 + returnErrBodies: []string{""}, + wantErr: "retryable revocation error: server responded with status 503", + wantRetryableErrType: true, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", + }, + { + name: "retryable error when the server returns 400 Bad Request on the first call due to client auth, then 503 on second call", + tokenType: provider.AccessTokenType, + returnStatusCodes: []int{http.StatusBadRequest, http.StatusServiceUnavailable}, // 400, 503 + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""}, + wantErr: "retryable revocation error: server responded with status 503", + wantRetryableErrType: true, + wantNumRequests: 2, + wantTokenTypeHint: "access_token", + }, + { + name: "retryable error when server returns any 5xx status on first call, testing lower bound of 5xx range", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{http.StatusInternalServerError}, // 500 + returnErrBodies: []string{""}, + wantErr: "retryable revocation error: server responded with status 500", + wantRetryableErrType: true, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", + }, + { + name: "retryable error when server returns any 5xx status on first call, testing upper bound of 5xx range", + tokenType: provider.RefreshTokenType, + returnStatusCodes: []int{599}, // not defined by an RFC, but sometimes considered Network Connect Timeout Error + returnErrBodies: []string{""}, + wantErr: "retryable revocation error: server responded with status 599", + wantRetryableErrType: true, + wantNumRequests: 1, + wantTokenTypeHint: "refresh_token", + }, + { + name: "retryable error when the server cannot be reached", + tokenType: provider.AccessTokenType, + unreachableServer: true, + wantErrRegexp: "^retryable revocation error: Post .*: dial tcp .*: connect: connection refused$", + wantRetryableErrType: true, + wantNumRequests: 0, }, } for _, tt := range tests { @@ -592,9 +649,9 @@ func TestProviderConfig(t *testing.T) { require.Equal(t, "test-client-id", username) require.Equal(t, "test-client-secret", password) } - if tt.statusCodes[numRequests-1] != http.StatusOK { + if tt.returnStatusCodes[numRequests-1] != http.StatusOK { w.Header().Set("content-type", "application/json") - http.Error(w, tt.returnErrBodies[numRequests-1], tt.statusCodes[numRequests-1]) + http.Error(w, tt.returnErrBodies[numRequests-1], tt.returnStatusCodes[numRequests-1]) } // Otherwise, responds with 200 OK and empty body by default. })) @@ -616,6 +673,10 @@ func TestProviderConfig(t *testing.T) { p.RevocationURL = nil } + if tt.unreachableServer { + tokenServer.Close() // make the sever unreachable by closing it before making any requests + } + err = p.RevokeToken( context.Background(), "test-upstream-token", @@ -625,8 +686,20 @@ func TestProviderConfig(t *testing.T) { require.Equal(t, tt.wantNumRequests, numRequests, "did not make expected number of requests to revocation endpoint") - if tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) + if tt.wantErr != "" || tt.wantErrRegexp != "" { // nolint:nestif + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + } else { + require.Error(t, err) + require.Regexp(t, tt.wantErrRegexp, err.Error()) + } + + if tt.wantRetryableErrType { + require.ErrorAs(t, err, &provider.RetryableRevocationError{}) + } else if errors.As(err, &provider.RetryableRevocationError{}) { + // There is no NotErrorAs() assertion available in the current version of testify, so do the equivalent. + require.Fail(t, "error should not be As RetryableRevocationError") + } } else { require.NoError(t, err) } From 7551af3eb81f445cec4d1a0f44987b5b1532a091 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 14 Jan 2022 10:59:39 -0800 Subject: [PATCH 4/4] Fix code that did not auto-merge correctly in previous merge from main --- .../garbage_collector_test.go | 38 +++---------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index 1ee56d6b..5ddc6953 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -374,18 +374,11 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // Both authcode session secrets are deleted. r.ElementsMatch( []kubetesting.Action{ - kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), - kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession"), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession", testutil.NewPreconditions("uid-123", "rv-123")), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession", testutil.NewPreconditions("uid-456", "rv-456")), }, kubeClient.Actions(), ) - r.ElementsMatch( - []metav1.DeleteOptions{ - testutil.NewPreconditions("uid-123", "rv-123"), - testutil.NewPreconditions("uid-456", "rv-456"), - }, - *deleteOptions, - ) }) }) @@ -816,16 +809,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // The authcode session secrets is still deleted because it is expired and the revocation error is not retryable. r.ElementsMatch( []kubetesting.Action{ - kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession", testutil.NewPreconditions("uid-123", "rv-123")), }, kubeClient.Actions(), ) - r.ElementsMatch( - []metav1.DeleteOptions{ - testutil.NewPreconditions("uid-123", "rv-123"), - }, - *deleteOptions, - ) }) }) @@ -1143,18 +1130,11 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // Both session secrets are deleted. r.ElementsMatch( []kubetesting.Action{ - kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession"), - kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession"), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession", testutil.NewPreconditions("uid-123", "rv-123")), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession", testutil.NewPreconditions("uid-456", "rv-456")), }, kubeClient.Actions(), ) - r.ElementsMatch( - []metav1.DeleteOptions{ - testutil.NewPreconditions("uid-123", "rv-123"), - testutil.NewPreconditions("uid-456", "rv-456"), - }, - *deleteOptions, - ) }) }) @@ -1227,16 +1207,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // The secret is deleted. r.ElementsMatch( []kubetesting.Action{ - kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "oidcRefreshSession"), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "oidcRefreshSession", testutil.NewPreconditions("uid-123", "rv-123")), }, kubeClient.Actions(), ) - r.ElementsMatch( - []metav1.DeleteOptions{ - testutil.NewPreconditions("uid-123", "rv-123"), - }, - *deleteOptions, - ) }) })