diff --git a/Dockerfile b/Dockerfile index 922a9ae3..3d7f3f23 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,6 @@ # syntax = docker/dockerfile:1.0-experimental -# Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +# Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 FROM golang:1.17.6 as build-env diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index 0333be23..2a9dad36 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package cmd diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index aeee21ea..4a384c76 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package cmd diff --git a/cmd/pinniped/cmd/login_static_test.go b/cmd/pinniped/cmd/login_static_test.go index 12cd0725..5fd04e0d 100644 --- a/cmd/pinniped/cmd/login_static_test.go +++ b/cmd/pinniped/cmd/login_static_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package cmd diff --git a/cmd/pinniped/cmd/root.go b/cmd/pinniped/cmd/root.go index cc4634b4..509e5a0b 100644 --- a/cmd/pinniped/cmd/root.go +++ b/cmd/pinniped/cmd/root.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package cmd diff --git a/internal/authenticators/authenticators.go b/internal/authenticators/authenticators.go index 60151c18..e343ecd1 100644 --- a/internal/authenticators/authenticators.go +++ b/internal/authenticators/authenticators.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package authenticators contains authenticator interfaces. diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index 811cbb05..828d94e9 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package impersonator diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index e5acd5f9..6eedd950 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package impersonator diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index c4b888fe..1b724700 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package server is the command line entry point for pinniped-concierge. diff --git a/internal/controller/apicerts/certs_expirer_test.go b/internal/controller/apicerts/certs_expirer_test.go index cfc8b6ee..9741de5d 100644 --- a/internal/controller/apicerts/certs_expirer_test.go +++ b/internal/controller/apicerts/certs_expirer_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package apicerts diff --git a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go index aee116cc..f287e915 100644 --- a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go +++ b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package cachecleaner diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index db7e9bcb..c482c6d6 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package jwtcachefiller diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 6f4b4e02..240cd0aa 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package webhookcachefiller diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 11ccde32..4cd7b0b0 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package impersonatorconfig diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index af656096..d11cca03 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package impersonatorconfig diff --git a/internal/controller/kubecertagent/kubecertagent.go b/internal/controller/kubecertagent/kubecertagent.go index 9ac15b40..8408005b 100644 --- a/internal/controller/kubecertagent/kubecertagent.go +++ b/internal/controller/kubecertagent/kubecertagent.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package kubecertagent provides controllers that ensure a pod (the kube-cert-agent), is diff --git a/internal/controller/kubecertagent/kubecertagent_test.go b/internal/controller/kubecertagent/kubecertagent_test.go index adf38ae2..ee382458 100644 --- a/internal/controller/kubecertagent/kubecertagent_test.go +++ b/internal/controller/kubecertagent/kubecertagent_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubecertagent diff --git a/internal/controller/kubecertagent/legacypodcleaner_test.go b/internal/controller/kubecertagent/legacypodcleaner_test.go index 922a748c..9f4fc54e 100644 --- a/internal/controller/kubecertagent/legacypodcleaner_test.go +++ b/internal/controller/kubecertagent/legacypodcleaner_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubecertagent diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index c96d20a4..8cc13256 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package activedirectoryupstreamwatcher implements a controller which watches ActiveDirectoryIdentityProviders. diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go index 76a35ca2..713e32f7 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package activedirectoryupstreamwatcher diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 4303051b..08492e17 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package supervisorconfig diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index aa8aa0b2..bf19af46 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package supervisorconfig diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index 3caee10a..1c38a1d0 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package supervisorstorage @@ -113,25 +113,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 @@ -139,13 +144,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, @@ -162,30 +169,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 @@ -194,29 +207,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: @@ -225,8 +238,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 } @@ -243,32 +256,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.RevokeRefreshToken(ctx, customSessionData.OIDC.UpstreamRefreshToken) - 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 08a3159a..5ddc6953 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package supervisorstorage @@ -260,7 +260,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", @@ -355,18 +355,141 @@ 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, + }, + ) + + // Both authcode session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession", testutil.NewPreconditions("uid-123", "rv-123")), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession", testutil.NewPreconditions("uid-456", "rv-456")), + }, + kubeClient.Actions(), + ) + }) + }) + + 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, }, ) @@ -430,14 +553,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( @@ -500,14 +623,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( @@ -570,14 +693,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( @@ -637,28 +760,60 @@ 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"). - WithRevokeRefreshTokenError(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()) 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, }, ) // 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.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession", testutil.NewPreconditions("uid-123", "rv-123")), + }, + kubeClient.Actions(), + ) + }) }) when("there is a valid, long-since expired authcode secret but the upstream revocation fails", func() { @@ -713,18 +868,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, }, ) @@ -738,7 +894,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", @@ -833,18 +989,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, }, ) @@ -859,7 +1016,129 @@ 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.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession", testutil.NewPreconditions("uid-123", "rv-123")), + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession", testutil.NewPreconditions("uid-456", "rv-456")), + }, + kubeClient.Actions(), + ) + }) + }) + + when("there are valid, expired refresh secrets which contain upstream refresh tokens", func() { it.Before(func() { oidcRefreshSession := &refreshtoken.Session{ Version: "2", @@ -909,18 +1188,95 @@ 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, + }, + ) + + // The secret is deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "oidcRefreshSession", testutil.NewPreconditions("uid-123", "rv-123")), + }, + kubeClient.Actions(), + ) + }) + }) + + 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, }, ) @@ -962,7 +1318,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/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 795f8619..6683c2a9 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package controllermanager provides an entrypoint into running all of the controllers that run as diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 1467cb9e..61720a0f 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package crud diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index fe30e4d9..182aa2cf 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -393,7 +393,7 @@ 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 + // 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 diff --git a/internal/kubeclient/kubeclient.go b/internal/kubeclient/kubeclient.go index 710dbb54..98d0b7f6 100644 --- a/internal/kubeclient/kubeclient.go +++ b/internal/kubeclient/kubeclient.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubeclient diff --git a/internal/kubeclient/kubeclient_test.go b/internal/kubeclient/kubeclient_test.go index 1fe6db38..e398d7e2 100644 --- a/internal/kubeclient/kubeclient_test.go +++ b/internal/kubeclient/kubeclient_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubeclient diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index c27c0184..87d5b75b 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" @@ -229,18 +230,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) } // ValidateTokenAndMergeWithUserInfo mocks base method. diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index 794731a9..8480c594 100644 --- a/internal/oidc/kube_storage.go +++ b/internal/oidc/kube_storage.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidc diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index c471955f..5f9e4104 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" @@ -17,6 +18,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. @@ -71,8 +80,11 @@ 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. + // 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 // ValidateTokenAndMergeWithUserInfo 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 @@ -165,3 +177,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/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 5dc70668..1f18dcf7 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package manager diff --git a/internal/plog/klog.go b/internal/plog/klog.go index f5ad9cdd..cd62b6c6 100644 --- a/internal/plog/klog.go +++ b/internal/plog/klog.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package plog diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index d79a13c8..f5b6f8cd 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package credentialrequest diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index d5e3a7be..48634879 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package server defines the entrypoint for the Pinniped Supervisor server. diff --git a/internal/testutil/delete.go b/internal/testutil/delete.go index 543b8727..0e1d63c0 100644 --- a/internal/testutil/delete.go +++ b/internal/testutil/delete.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testutil diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index bed4685b..92cc582c 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 } // ValidateTokenAndMergeWithUserInfoArgs is used to spy on calls to @@ -175,7 +176,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 ValidateTokenAndMergeWithUserInfoFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) @@ -185,8 +186,8 @@ type TestUpstreamOIDCIdentityProvider struct { passwordCredentialsGrantAndValidateTokensArgs []*PasswordCredentialsGrantAndValidateTokensArgs performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs - revokeRefreshTokenCallCount int - revokeRefreshTokenArgs []*RevokeRefreshTokenArgs + revokeTokenCallCount int + revokeTokenArgs []*RevokeTokenArgs validateTokenAndMergeWithUserInfoCallCount int validateTokenAndMergeWithUserInfoArgs []*ValidateTokenAndMergeWithUserInfoArgs } @@ -291,16 +292,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 { @@ -314,15 +316,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) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) { @@ -567,40 +569,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()", ) } @@ -627,7 +629,7 @@ type TestUpstreamOIDCIdentityProviderBuilder struct { authcodeExchangeErr error passwordGrantErr error performRefreshErr error - revokeRefreshTokenErr error + revokeTokenErr error validateTokenAndMergeWithUserInfoErr error } @@ -768,8 +770,8 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenAndMergeWithU return u } -func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { - u.revokeRefreshTokenErr = err +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { + u.revokeTokenErr = err return u } @@ -803,8 +805,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 }, ValidateTokenAndMergeWithUserInfoFunc: func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.validateTokenAndMergeWithUserInfoErr != nil { diff --git a/internal/testutil/testlogger/stdr_copied.go b/internal/testutil/testlogger/stdr_copied.go index d35bd86a..69f3ab29 100644 --- a/internal/testutil/testlogger/stdr_copied.go +++ b/internal/testutil/testlogger/stdr_copied.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testlogger diff --git a/internal/testutil/testlogger/testlogger.go b/internal/testutil/testlogger/testlogger.go index 1e0a17a1..907d3722 100644 --- a/internal/testutil/testlogger/testlogger.go +++ b/internal/testutil/testlogger/testlogger.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package testlogger wraps logr.Logger to allow for writing test assertions. diff --git a/internal/testutil/transcript_logger.go b/internal/testutil/transcript_logger.go index 7f14d619..add67486 100644 --- a/internal/testutil/transcript_logger.go +++ b/internal/testutil/transcript_logger.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testutil diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 0a7e37a2..1985823b 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamldap implements an abstraction of upstream LDAP IDP interactions. diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 65a7629f..5618f712 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamldap diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 94ce7502..6c71b83a 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -151,32 +151,39 @@ 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. +// 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("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 @@ -185,8 +192,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} @@ -207,22 +214,25 @@ func (p *ProviderConfig) tryRevokeRefreshToken( 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("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: + case status == 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, - 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"` @@ -232,21 +242,34 @@ func (p *ProviderConfig) tryRevokeRefreshToken( 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("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 + 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("RevokeRefreshToken() 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 811b2300..f8f15ce1 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" @@ -468,73 +469,171 @@ 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 + 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", + 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", - statusCodes: []int{http.StatusBadRequest, http.StatusOK}, + name: "success when the server returns 200 OK on the first call for refresh token", + tokenType: provider.RefreshTokenType, + 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, + 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, + 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, + 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, + 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 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, + 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 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, + 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 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, + 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 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, + 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 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, + 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, + 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 { @@ -549,23 +648,23 @@ 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) 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. })) @@ -587,16 +686,35 @@ func TestProviderConfig(t *testing.T) { p.RevocationURL = nil } - err = p.RevokeRefreshToken( + if tt.unreachableServer { + tokenServer.Close() // make the sever unreachable by closing it before making any requests + } + + err = p.RevokeToken( context.Background(), - "test-initial-refresh-token", + "test-upstream-token", + tt.tokenType, ) 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) } }) } diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 47fa6684..eac46468 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration diff --git a/test/testlib/env.go b/test/testlib/env.go index 7ce6d0be..999df7a1 100644 --- a/test/testlib/env.go +++ b/test/testlib/env.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testlib