diff --git a/Dockerfile b/Dockerfile index 922a9ae34..3d7f3f239 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/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl index a53d6f535..798275a9f 100644 --- a/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl @@ -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 v1alpha1 diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index 0333be23e..2a9dad36d 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 aeee21ea3..4a384c760 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 12cd07253..5fd04e0d6 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 cc4634b49..509e5a0b4 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/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index a53d6f535..798275a9f 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.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 v1alpha1 diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index a53d6f535..798275a9f 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.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 v1alpha1 diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index a53d6f535..798275a9f 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.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 v1alpha1 diff --git a/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index a53d6f535..798275a9f 100644 --- a/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.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 v1alpha1 diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index a53d6f535..798275a9f 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.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 v1alpha1 diff --git a/hack/header.txt b/hack/header.txt index f6970c2b3..8ebfbf38f 100644 --- a/hack/header.txt +++ b/hack/header.txt @@ -1,2 +1,2 @@ -Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. SPDX-License-Identifier: Apache-2.0 diff --git a/internal/authenticators/authenticators.go b/internal/authenticators/authenticators.go index 60151c18f..e343ecd13 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 811cbb055..828d94e9b 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 e5acd5f99..6eedd9505 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 c4b888fe1..1b7247003 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 cfc8b6ee8..9741de5d7 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 aee116cca..f287e9152 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 db7e9bcb0..c482c6d65 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 6f4b4e029..240cd0aa6 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 11ccde32d..4cd7b0b02 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 af6560967..d11cca03e 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 9ac15b402..8408005bf 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 adf38ae2d..ee3824588 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 922a748c8..9f4fc54eb 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/kubecertagent/mocks/mockdynamiccert.go b/internal/controller/kubecertagent/mocks/mockdynamiccert.go index 030d3e07c..fda36b657 100644 --- a/internal/controller/kubecertagent/mocks/mockdynamiccert.go +++ b/internal/controller/kubecertagent/mocks/mockdynamiccert.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 // diff --git a/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go b/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go index 637f0927a..f9aef2d05 100644 --- a/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go +++ b/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.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 // diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 4303051bd..08492e17e 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 aa8aa0b20..bf19af469 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 3caee10a2..1c38a1d06 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 08a3159a5..5ddc69539 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 795f86190..6683c2a97 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 1467cb9e2..61720a0f8 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/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index 5b5033719..ab6f7ee65 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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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 e7c6655b6..f2504d904 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -371,32 +371,34 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ "providerType": "ɥ闣ʬ橳(ý綃ʃʚƟ覣k眐4", "oidc": { "upstreamRefreshToken": "tC嵽痊w", - "upstreamSubject": "a紽ǒ|鰽ŋ猊I", - "upstreamIssuer": "妬\u003e6鉢緋uƴŤȱʀ" + "upstreamAccessToken": "a紽ǒ|鰽ŋ猊I", + "upstreamSubject": "妬\u003e6鉢緋uƴŤȱʀ", + "upstreamIssuer": ":設虝27就伒犘c" }, "ldap": { - "userDN": "Â?墖\u003cƬb獭潜Ʃ饾k|鬌R蜚蠣", + "userDN": "ɏȫ齁š%Op", "extraRefreshAttributes": { - "ȱ藚ɏ¬Ê蒭堜]ȗ韚ʫ": "鷞aŚB碠k9帴ʘ赱" + "T妼É4İ\u003e×1": "ʥ笿0D", + "÷驣7Ʀ澉1æɽ誮": "ʫ繕ȫ", + "ŚB碠k9": "i磊ůď逳鞪?3)藵睋邔\u0026Ű" } }, "activedirectory": { - "userDN": "瑹xȢ~1Įx欼笝?úT妼", + "userDN": "s", "extraRefreshAttributes": { - "iYn": "麹Œ颛", - "İ\u003e×1飞O+î艔垎0OƉǢIȽ齤士": "ȐĨf跞@)¿,ɭS隑ip偶" + "ƉǢIȽ齤士bEǎ儯惝IozŁ5rƖ螼": "偶宾儮猷V麹Œ颛Ė應,Ɣ鬅X¤" } } } }, "requestedAudience": [ - "應,Ɣ鬅X¤", - "¤.岵骘胲ƤkǦ" + "tO灞浛a齙\\蹼偦歛ơ", + "皦pSǬŝ社Vƅȭǝ*" ], "grantedAudience": [ - "鸖I¶媁y衑拁Ȃ", - "社Vƅȭǝ*擦28Dž 甍 ć", - "bņ抰蛖a³2ʫ承dʬ)ġ,TÀqy_" + "ĝ\"zvưã置bņ抰蛖a³2ʫ", + "Ŷɽ蔒PR}Ųʓl{鼐jÃ轘屔挝", + "Œų崓ļ憽-蹐È_¸]fś" ] }, "version": "2" diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 72c4cfba6..182aa2cf7 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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -389,11 +389,12 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { validSessionJSONBytes, err := json.MarshalIndent(validSession, "", "\t") require.NoError(t, err) authorizeCodeSessionJSONFromFuzzing := string(validSessionJSONBytes) - t.Log(authorizeCodeSessionJSONFromFuzzing) // 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 ac680a606..03ae3626d 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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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 b84798ce9..589217256 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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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 ce74793a7..36af0559c 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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"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":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", diff --git a/internal/kubeclient/kubeclient.go b/internal/kubeclient/kubeclient.go index 710dbb54a..98d0b7f6f 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 1fe6db383..e398d7e2e 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/credentialrequestmocks/credentialrequestmocks.go b/internal/mocks/credentialrequestmocks/credentialrequestmocks.go index 68ff7f2ed..db5c5cced 100644 --- a/internal/mocks/credentialrequestmocks/credentialrequestmocks.go +++ b/internal/mocks/credentialrequestmocks/credentialrequestmocks.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 // diff --git a/internal/mocks/issuermocks/issuermocks.go b/internal/mocks/issuermocks/issuermocks.go index 045fbed34..737b9ab1b 100644 --- a/internal/mocks/issuermocks/issuermocks.go +++ b/internal/mocks/issuermocks/issuermocks.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 // diff --git a/internal/mocks/mockkeyset/mockkeyset.go b/internal/mocks/mockkeyset/mockkeyset.go index a2cb28e6b..81f4b012e 100644 --- a/internal/mocks/mockkeyset/mockkeyset.go +++ b/internal/mocks/mockkeyset/mockkeyset.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 // diff --git a/internal/mocks/mockldapconn/mockldapconn.go b/internal/mocks/mockldapconn/mockldapconn.go index 24b0b1aa9..b8839a04e 100644 --- a/internal/mocks/mockldapconn/mockldapconn.go +++ b/internal/mocks/mockldapconn/mockldapconn.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 // diff --git a/internal/mocks/mocksecrethelper/mocksecrethelper.go b/internal/mocks/mocksecrethelper/mocksecrethelper.go index 051c75482..d7520b204 100644 --- a/internal/mocks/mocksecrethelper/mocksecrethelper.go +++ b/internal/mocks/mocksecrethelper/mocksecrethelper.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 // diff --git a/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go b/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go index 31349d33a..6946e9670 100644 --- a/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go +++ b/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.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 // diff --git a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go index b8c7c28ca..3f651eb1f 100644 --- a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go +++ b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.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 // diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 2a2a689ed..87d5b75bb 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.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 // @@ -14,12 +14,12 @@ import ( reflect "reflect" gomock "github.com/golang/mock/gomock" - oauth2 "golang.org/x/oauth2" - types "k8s.io/apimachinery/pkg/types" - + 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" + oauth2 "golang.org/x/oauth2" + types "k8s.io/apimachinery/pkg/types" ) // MockUpstreamOIDCIdentityProviderI is a mock of UpstreamOIDCIdentityProviderI interface. @@ -186,6 +186,20 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) GetUsernameClaim() *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUsernameClaim", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).GetUsernameClaim)) } +// HasUserInfoURL mocks base method. +func (m *MockUpstreamOIDCIdentityProviderI) HasUserInfoURL() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasUserInfoURL") + ret0, _ := ret[0].(bool) + return ret0 +} + +// HasUserInfoURL indicates an expected call of HasUserInfoURL. +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) HasUserInfoURL() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasUserInfoURL", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).HasUserInfoURL)) +} + // PasswordCredentialsGrantAndValidateTokens mocks base method. func (m *MockUpstreamOIDCIdentityProviderI) PasswordCredentialsGrantAndValidateTokens(arg0 context.Context, arg1, arg2 string) (*oidctypes.Token, error) { m.ctrl.T.Helper() @@ -216,31 +230,31 @@ 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. -func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { +// ValidateTokenAndMergeWithUserInfo mocks base method. +func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3, arg4 bool) (*oidctypes.Token, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3, arg4) ret0, _ := ret[0].(*oidctypes.Token) ret1, _ := ret[1].(error) return ret0, ret1 } -// ValidateToken indicates an expected call of ValidateToken. -func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +// ValidateTokenAndMergeWithUserInfo indicates an expected call of ValidateTokenAndMergeWithUserInfo. +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateTokenAndMergeWithUserInfo(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3, arg4) } diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index b08d39bc1..4f281ad8c 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -174,16 +174,6 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( fosite.ErrAccessDenied.WithDebug(err.Error()), true) // WithDebug hides the error from the client } - if token.RefreshToken == nil || token.RefreshToken.Token == "" { - plog.Warning("refresh token not returned by upstream provider during password grant, "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", - "upstreamName", oidcUpstream.GetName(), - "scopes", oidcUpstream.GetScopes()) - return writeAuthorizeError(w, oauthHelper, authorizeRequester, - fosite.ErrAccessDenied.WithHint( - "Refresh token not returned by upstream provider during password grant."), true) - } - subject, username, groups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(oidcUpstream, token.IDToken.Claims) if err != nil { // Return a user-friendly error for this case which is entirely within our control. @@ -191,31 +181,14 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, ) } - upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims) + + customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(oidcUpstream, token) if err != nil { - // Return a user-friendly error for this case which is entirely within our control. - return writeAuthorizeError(w, oauthHelper, authorizeRequester, - fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, - ) - } - upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims) - if err != nil { - // Return a user-friendly error for this case which is entirely within our control. return writeAuthorizeError(w, oauthHelper, authorizeRequester, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, ) } - customSessionData := &psession.CustomSessionData{ - ProviderUID: oidcUpstream.GetResourceUID(), - ProviderName: oidcUpstream.GetName(), - ProviderType: psession.ProviderTypeOIDC, - OIDC: &psession.OIDCSessionData{ - UpstreamRefreshToken: token.RefreshToken.Token, - UpstreamIssuer: upstreamIssuer, - UpstreamSubject: upstreamSubject, - }, - } return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData) } diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 845e39b4b..0198c83a2 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -56,6 +56,7 @@ func TestAuthorizationEndpoint(t *testing.T) { oidcUpstreamUsernameClaim = "the-user-claim" oidcUpstreamGroupsClaim = "the-groups-claim" oidcPasswordGrantUpstreamRefreshToken = "some-opaque-token" //nolint: gosec + oidcUpstreamAccessToken = "some-access-token" downstreamIssuer = "https://my-downstream-issuer.com/some-path" downstreamRedirectURI = "http://127.0.0.1/callback" @@ -154,9 +155,15 @@ func TestAuthorizationEndpoint(t *testing.T) { "state": happyState, } - fositeAccessDeniedWithMissingRefreshTokenErrorQuery = map[string]string{ + fositeAccessDeniedWithMissingAccessTokenErrorQuery = map[string]string{ "error": "access_denied", - "error_description": "The resource owner or authorization server denied the request. Refresh token not returned by upstream provider during password grant.", + "error_description": "The resource owner or authorization server denied the request. Reason: neither access token nor refresh token returned by upstream provider.", + "state": happyState, + } + + fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery = map[string]string{ + "error": "access_denied", + "error_description": "The resource owner or authorization server denied the request. Reason: access token was returned by upstream provider but there was no userinfo endpoint.", "state": happyState, } @@ -475,6 +482,17 @@ func TestAuthorizationEndpoint(t *testing.T) { }, } + expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken := &psession.CustomSessionData{ + ProviderUID: oidcPasswordGrantUpstreamResourceUID, + ProviderName: oidcPasswordGrantUpstreamName, + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: oidcUpstreamAccessToken, + UpstreamSubject: oidcUpstreamSubject, + UpstreamIssuer: oidcUpstreamIssuer, + }, + } + // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyState @@ -873,6 +891,50 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "OIDC password grant happy path when upstream IDP returned empty refresh token but it did return an access token and has a userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: htmlContentType, + wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken, + }, + { + name: "OIDC password grant happy path when upstream IDP did not return a refresh token but it did return an access token and has a userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: htmlContentType, + wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken, + }, { name: "error during upstream LDAP authentication", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&erroringUpstreamLDAPIdentityProvider), @@ -1015,8 +1077,8 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().Build()), + name: "password grant returns an error when upstream IDP returns no refresh token with an access token but has no userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), @@ -1024,12 +1086,12 @@ func TestAuthorizationEndpoint(t *testing.T) { wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", - wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingRefreshTokenErrorQuery), + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery), wantBodyString: "", }, { - name: "return an error when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().Build()), + name: "password grant returns an error when upstream IDP returns empty refresh token with an access token but has no userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), @@ -1037,7 +1099,59 @@ func TestAuthorizationEndpoint(t *testing.T) { wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", - wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingRefreshTokenErrorQuery), + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery), + wantBodyString: "", + }, + { + name: "password grant returns an error when upstream IDP returns empty refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithEmptyAccessToken().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery), + wantBodyString: "", + }, + { + name: "password grant returns an error when upstream IDP returns no refresh and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithoutAccessToken().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery), + wantBodyString: "", + }, + { + name: "password grant returns an error when upstream IDP returns no refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithEmptyAccessToken().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery), + wantBodyString: "", + }, + { + name: "password grant returns an error when upstream IDP returns empty refresh token and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithoutAccessToken().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery), wantBodyString: "", }, { diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 1a5a3aee0..fbf13728d 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -19,7 +19,6 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/provider/formposthtml" "go.pinniped.dev/internal/plog" - "go.pinniped.dev/internal/psession" ) func NewHandler( @@ -69,39 +68,17 @@ func NewHandler( return httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens") } - if token.RefreshToken == nil || token.RefreshToken.Token == "" { - plog.Warning("refresh token not returned by upstream provider during authcode exchange, "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", - "upstreamName", upstreamIDPConfig.GetName(), - "scopes", upstreamIDPConfig.GetScopes(), - "additionalParams", upstreamIDPConfig.GetAdditionalAuthcodeParams()) - return httperr.New(http.StatusUnprocessableEntity, "refresh token not returned by upstream provider during authcode exchange") - } - subject, username, groups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(upstreamIDPConfig, token.IDToken.Claims) if err != nil { return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } - upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) - if err != nil { - return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) - } - upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) + customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(upstreamIDPConfig, token) if err != nil { return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, &psession.CustomSessionData{ - ProviderUID: upstreamIDPConfig.GetResourceUID(), - ProviderName: upstreamIDPConfig.GetName(), - ProviderType: psession.ProviderTypeOIDC, - OIDC: &psession.OIDCSessionData{ - UpstreamRefreshToken: token.RefreshToken.Token, - UpstreamSubject: upstreamSubject, - UpstreamIssuer: upstreamIssuer, - }, - }) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData) authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index d14c159f9..a43b2926a 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -31,6 +31,7 @@ const ( oidcUpstreamIssuer = "https://my-upstream-issuer.com" oidcUpstreamRefreshToken = "test-refresh-token" + oidcUpstreamAccessToken = "test-access-token" oidcUpstreamSubject = "abc123-some guid" // has a space character which should get escaped in URL oidcUpstreamSubjectQueryEscaped = "abc123-some+guid" oidcUpstreamUsername = "test-pinniped-username" @@ -83,6 +84,16 @@ var ( UpstreamSubject: oidcUpstreamSubject, }, } + happyDownstreamAccessTokenCustomSessionData = &psession.CustomSessionData{ + ProviderUID: happyUpstreamIDPResourceUID, + ProviderName: happyUpstreamIDPName, + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: oidcUpstreamAccessToken, + UpstreamIssuer: oidcUpstreamIssuer, + UpstreamSubject: oidcUpstreamSubject, + }, + } ) func TestCallbackEndpoint(t *testing.T) { @@ -200,6 +211,29 @@ func TestCallbackEndpoint(t *testing.T) { args: happyExchangeAndValidateTokensArgs, }, }, + { + name: "GET with authcode exchange that returns an access token but no refresh token when there is a userinfo endpoint returns 303 to downstream client callback with its state and code", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusSeeOther, + wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, + wantBody: "", + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: happyDownstreamAccessTokenCustomSessionData, + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, { name: "upstream IDP provides no username or group claim configuration, so we use default username claim and skip groups", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( @@ -323,28 +357,70 @@ func TestCallbackEndpoint(t *testing.T) { }, }, { - name: "return an error when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().Build()), + name: "return an error when upstream IDP returned no refresh token with an access token when there is no userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyState).String(), csrfCookie: happyCSRFCookie, wantStatus: http.StatusUnprocessableEntity, wantContentType: htmlContentType, - wantBody: "Unprocessable Entity: refresh token not returned by upstream provider during authcode exchange\n", + wantBody: "Unprocessable Entity: access token was returned by upstream provider but there was no userinfo endpoint\n", wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ performedByUpstreamName: happyUpstreamIDPName, args: happyExchangeAndValidateTokensArgs, }, }, { - name: "return an error when upstream IDP returned an empty refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().Build()), + name: "return an error when upstream IDP returned no refresh token and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithoutAccessToken().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyState).String(), csrfCookie: happyCSRFCookie, wantStatus: http.StatusUnprocessableEntity, wantContentType: htmlContentType, - wantBody: "Unprocessable Entity: refresh token not returned by upstream provider during authcode exchange\n", + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "return an error when upstream IDP returned an empty refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithEmptyAccessToken().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: htmlContentType, + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "return an error when upstream IDP returned no refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithEmptyAccessToken().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: htmlContentType, + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "return an error when upstream IDP returned an empty refresh token and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithoutAccessToken().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: htmlContentType, + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ performedByUpstreamName: happyUpstreamIDPName, args: happyExchangeAndValidateTokensArgs, diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 265ed5c1e..64ac14189 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -5,6 +5,7 @@ package downstreamsession import ( + "errors" "fmt" "net/url" "time" @@ -19,6 +20,7 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" + "go.pinniped.dev/pkg/oidcclient/oidctypes" ) const ( @@ -58,6 +60,55 @@ func MakeDownstreamSession(subject string, username string, groups []string, cus return openIDSession } +func MakeDownstreamOIDCCustomSessionData(oidcUpstream provider.UpstreamOIDCIdentityProviderI, token *oidctypes.Token) (*psession.CustomSessionData, error) { + upstreamSubject, err := ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + return nil, err + } + upstreamIssuer, err := ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + return nil, err + } + + customSessionData := &psession.CustomSessionData{ + ProviderUID: oidcUpstream.GetResourceUID(), + ProviderName: oidcUpstream.GetName(), + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamIssuer: upstreamIssuer, + UpstreamSubject: upstreamSubject, + }, + } + + const pleaseCheck = "please check configuration of OIDCIdentityProvider and the client in the " + + "upstream provider's API/UI and try to get a refresh token if possible" + logKV := []interface{}{ + "upstreamName", oidcUpstream.GetName(), + "scopes", oidcUpstream.GetScopes(), + "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams(), + } + + hasRefreshToken := token.RefreshToken != nil && token.RefreshToken.Token != "" + hasAccessToken := token.AccessToken != nil && token.AccessToken.Token != "" + switch { + case hasRefreshToken: // we prefer refresh tokens, so check for this first + customSessionData.OIDC.UpstreamRefreshToken = token.RefreshToken.Token + case hasAccessToken: // as a fallback, we can use the access token as long as there is a userinfo endpoint + if !oidcUpstream.HasUserInfoURL() { + plog.Warning("access token was returned by upstream provider during login without a refresh token "+ + "and there was no userinfo endpoint available on the provider. "+pleaseCheck, logKV...) + return nil, errors.New("access token was returned by upstream provider but there was no userinfo endpoint") + } + plog.Info("refresh token not returned by upstream provider during login, using access token instead. "+pleaseCheck, logKV...) + customSessionData.OIDC.UpstreamAccessToken = token.AccessToken.Token + default: + plog.Warning("refresh token and access token not returned by upstream provider during login. "+pleaseCheck, logKV...) + return nil, errors.New("neither access token nor refresh token returned by upstream provider") + } + + return customSessionData, nil +} + // GrantScopesIfRequested auto-grants the scopes for which we do not require end-user approval, if they were requested. func GrantScopesIfRequested(authorizeRequester fosite.AuthorizeRequester) { oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index 794731a9d..8480c5949 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 24b821e7b..5f9e41041 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. @@ -31,6 +40,9 @@ type UpstreamOIDCIdentityProviderI interface { // GetAuthorizationURL returns the Authorization Endpoint fetched from discovery. GetAuthorizationURL() *url.URL + // HasUserInfoURL returns whether there is a non-empty value for userinfo_endpoint fetched from discovery. + HasUserInfoURL() bool + // GetScopes returns the scopes to request in authorization (authcode or password grant) flow. GetScopes() []string @@ -68,13 +80,16 @@ 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 // tokens, or an error. - ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) + ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) } type UpstreamLDAPIdentityProviderI interface { @@ -162,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 5dc706687..1f18dcf7d 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/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index e0fc44080..56200aad3 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -11,12 +11,14 @@ import ( "github.com/ory/fosite" "github.com/ory/x/errorsx" + "golang.org/x/oauth2" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" + "go.pinniped.dev/pkg/oidcclient/oidctypes" ) var ( @@ -101,7 +103,15 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister) error { s := session.Custom - if s.OIDC == nil || s.OIDC.UpstreamRefreshToken == "" { + if s.OIDC == nil { + return errorsx.WithStack(errMissingUpstreamSessionInternalError) + } + + accessTokenStored := s.OIDC.UpstreamAccessToken != "" + refreshTokenStored := s.OIDC.UpstreamRefreshToken != "" + + exactlyOneTokenStored := (accessTokenStored || refreshTokenStored) && !(accessTokenStored && refreshTokenStored) + if !exactlyOneTokenStored { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -113,63 +123,88 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, plog.Debug("attempting upstream refresh request", "providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID) - refreshedTokens, err := p.PerformRefresh(ctx, s.OIDC.UpstreamRefreshToken) - if err != nil { - return errorsx.WithStack(errUpstreamRefreshError.WithHint( - "Upstream refresh failed.", - ).WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + var tokens *oauth2.Token + if refreshTokenStored { + tokens, err = p.PerformRefresh(ctx, s.OIDC.UpstreamRefreshToken) + if err != nil { + return errorsx.WithStack(errUpstreamRefreshError.WithHint( + "Upstream refresh failed.", + ).WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + } else { + tokens = &oauth2.Token{AccessToken: s.OIDC.UpstreamAccessToken} } // Upstream refresh may or may not return a new ID token. From the spec: // "the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token." // https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse - _, hasIDTok := refreshedTokens.Extra("id_token").(string) + _, hasIDTok := tokens.Extra("id_token").(string) // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at // least some providers do not include one, so we skip the nonce validation here (but not other validations). - validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(ctx, refreshedTokens, "", hasIDTok) + validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(ctx, tokens, "", hasIDTok, accessTokenStored) if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( "Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } - claims := validatedTokens.IDToken.Claims - // if we have any claims at all, we better have a subject, and it better match the previous value. - // but it's possible that we don't because both returning a new refresh token on refresh and having a userinfo - // endpoint are optional. - if len(validatedTokens.IDToken.Claims) != 0 { //nolint:nestif - newSub, hasSub := getString(claims, oidc.IDTokenSubjectClaim) - if !hasSub { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh not found")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) - } - if s.OIDC.UpstreamSubject != newSub { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) - } - usernameClaim := p.GetUsernameClaim() - newUsername, hasUsername := getString(claims, usernameClaim) - oldUsername := session.Fosite.Claims.Extra[oidc.DownstreamUsernameClaim] - // its possible this won't be returned. - // but if it is, verify that it hasn't changed. - if hasUsername && oldUsername != newUsername { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) - } - newIssuer, hasIssuer := getString(claims, oidc.IDTokenIssuerClaim) - if hasIssuer && s.OIDC.UpstreamIssuer != newIssuer { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) - } + err = validateIdentityUnchangedSinceInitialLogin(validatedTokens, session, p.GetUsernameClaim()) + if err != nil { + return err } // Upstream refresh may or may not return a new refresh token. If we got a new refresh token, then update it in // the user's session. If we did not get a new refresh token, then keep the old one in the session by avoiding // overwriting the old one. - if refreshedTokens.RefreshToken != "" { - plog.Debug("upstream refresh request did not return a new refresh token", + if tokens.RefreshToken != "" { + plog.Debug("upstream refresh request returned a new refresh token", "providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID) - s.OIDC.UpstreamRefreshToken = refreshedTokens.RefreshToken + s.OIDC.UpstreamRefreshToken = tokens.RefreshToken + } + + return nil +} + +func validateIdentityUnchangedSinceInitialLogin(validatedTokens *oidctypes.Token, session *psession.PinnipedSession, usernameClaimName string) error { + s := session.Custom + mergedClaims := validatedTokens.IDToken.Claims + + // If we have any claims at all, we better have a subject, and it better match the previous value. + // but it's possible that we don't because both returning a new id token on refresh and having a userinfo + // endpoint are optional. + if len(mergedClaims) == 0 { + return nil + } + + newSub, hasSub := getString(mergedClaims, oidc.IDTokenSubjectClaim) + if !hasSub { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh not found")). + WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + if s.OIDC.UpstreamSubject != newSub { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")). + WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + + newUsername, hasUsername := getString(mergedClaims, usernameClaimName) + oldUsername := session.Fosite.Claims.Extra[oidc.DownstreamUsernameClaim] + // It's possible that a username wasn't returned by the upstream provider during refresh, + // but if it is, verify that it hasn't changed. + if hasUsername && oldUsername != newUsername { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")). + WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + + newIssuer, hasIssuer := getString(mergedClaims, oidc.IDTokenIssuerClaim) + // It's possible that an issuer wasn't returned by the upstream provider during refresh, + // but if it is, verify that it hasn't changed. + if hasIssuer && s.OIDC.UpstreamIssuer != newIssuer { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")). + WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } return nil diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 2a8f039ac..11405ed40 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -225,7 +225,7 @@ type expectedUpstreamRefresh struct { type expectedUpstreamValidateTokens struct { performedByUpstreamName string - args *oidctestutil.ValidateTokenArgs + args *oidctestutil.ValidateTokenAndMergeWithUserInfoArgs } type tokenEndpointResponseExpectedValues struct { @@ -881,6 +881,7 @@ func TestRefreshGrant(t *testing.T) { oidcUpstreamInitialRefreshToken = "initial-upstream-refresh-token" oidcUpstreamRefreshedIDToken = "fake-refreshed-id-token" oidcUpstreamRefreshedRefreshToken = "fake-refreshed-refresh-token" + oidcUpstreamAccessToken = "fake-upstream-access-token" //nolint:gosec ldapUpstreamName = "some-ldap-idp" ldapUpstreamResourceUID = "ldap-resource-uid" @@ -904,7 +905,7 @@ func TestRefreshGrant(t *testing.T) { WithResourceUID(oidcUpstreamResourceUID) } - initialUpstreamOIDCCustomSessionData := func() *psession.CustomSessionData { + initialUpstreamOIDCRefreshTokenCustomSessionData := func() *psession.CustomSessionData { return &psession.CustomSessionData{ ProviderName: oidcUpstreamName, ProviderUID: oidcUpstreamResourceUID, @@ -917,8 +918,21 @@ func TestRefreshGrant(t *testing.T) { } } + initialUpstreamOIDCAccessTokenCustomSessionData := func() *psession.CustomSessionData { + return &psession.CustomSessionData{ + ProviderName: oidcUpstreamName, + ProviderUID: oidcUpstreamResourceUID, + ProviderType: oidcUpstreamType, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: oidcUpstreamAccessToken, + UpstreamSubject: goodUpstreamSubject, + UpstreamIssuer: goodIssuer, + }, + } + } + upstreamOIDCCustomSessionDataWithNewRefreshToken := func(newRefreshToken string) *psession.CustomSessionData { - sessionData := initialUpstreamOIDCCustomSessionData() + sessionData := initialUpstreamOIDCRefreshTokenCustomSessionData() sessionData.OIDC.UpstreamRefreshToken = newRefreshToken return sessionData } @@ -957,13 +971,15 @@ func TestRefreshGrant(t *testing.T) { } } - happyUpstreamValidateTokenCall := func(expectedTokens *oauth2.Token) *expectedUpstreamValidateTokens { + happyUpstreamValidateTokenCall := func(expectedTokens *oauth2.Token, requireIDToken bool) *expectedUpstreamValidateTokens { return &expectedUpstreamValidateTokens{ performedByUpstreamName: oidcUpstreamName, - args: &oidctestutil.ValidateTokenArgs{ + args: &oidctestutil.ValidateTokenAndMergeWithUserInfoArgs{ Ctx: nil, // this will be filled in with the actual request context by the test below Tok: expectedTokens, ExpectedIDTokenNonce: "", // always expect empty string + RequireUserInfo: false, + RequireIDToken: requireIDToken, }, } } @@ -986,7 +1002,7 @@ func TestRefreshGrant(t *testing.T) { // Should always try to perform an upstream refresh. want.wantUpstreamRefreshCall = happyOIDCUpstreamRefreshCall() if expectToValidateToken != nil { - want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken) + want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken, true) } return want } @@ -1049,7 +1065,7 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant with openid scope granted (id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "sub": goodUpstreamSubject, @@ -1057,9 +1073,9 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( @@ -1071,7 +1087,7 @@ func TestRefreshGrant(t *testing.T) { { name: "refresh grant with unchanged username claim", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", @@ -1081,9 +1097,9 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( @@ -1092,23 +1108,64 @@ func TestRefreshGrant(t *testing.T) { ), }, }, + { + name: "refresh grant when the customsessiondata has a stored access token and no stored refresh token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim"). + WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": goodUpstreamSubject, + "username-claim": goodUsername, + }, + }, + AccessToken: &oidctypes.AccessToken{ + Token: oidcUpstreamAccessToken, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCAccessTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCAccessTokenCustomSessionData()), + }, // do not want upstreamRefreshRequest??? + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"refresh_token", "id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantUpstreamOIDCValidateTokenCall: &expectedUpstreamValidateTokens{ + oidcUpstreamName, + &oidctestutil.ValidateTokenAndMergeWithUserInfoArgs{ + Ctx: nil, // this will be filled in with the actual request context by the test below + Tok: &oauth2.Token{AccessToken: oidcUpstreamAccessToken}, // only the old access token + ExpectedIDTokenNonce: "", // always expect empty string + RequireIDToken: false, + RequireUserInfo: true, + }, + }, + wantCustomSessionDataStored: initialUpstreamOIDCAccessTokenCustomSessionData(), // doesn't change when we refresh + }, + }, + }, { name: "happy path refresh grant without openid scope granted (no id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{}, }, - }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + }).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, - wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(), + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), }, }, refreshRequest: refreshRequestInputs{ @@ -1118,7 +1175,7 @@ func TestRefreshGrant(t *testing.T) { wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), false), wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), }, }, @@ -1126,27 +1183,32 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant when the upstream refresh does not return a new ID token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{}, }, }).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ - want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( - upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), - refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateTokenAndMergeWithUserInfo is called - ), + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), false), + wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + }, }, }, { name: "happy path refresh grant when the upstream refresh does not return a new refresh token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "sub": goodUpstreamSubject, @@ -1154,13 +1216,13 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( - initialUpstreamOIDCCustomSessionData(), // still has the initial refresh token stored + initialUpstreamOIDCRefreshTokenCustomSessionData(), // still has the initial refresh token stored refreshedUpstreamTokensWithIDTokenWithoutRefreshToken(), ), }, @@ -1168,7 +1230,7 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request adds a new scope to the list of requested scopes then it is ignored", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "sub": goodUpstreamSubject, @@ -1176,9 +1238,9 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { @@ -1193,7 +1255,7 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request removes a scope which was originally granted from the list of requested scopes then it is granted anyway", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "sub": goodUpstreamSubject, @@ -1201,14 +1263,14 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, - wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(), + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), }, }, refreshRequest: refreshRequestInputs{ @@ -1221,7 +1283,7 @@ func TestRefreshGrant(t *testing.T) { wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), }, }, @@ -1229,7 +1291,7 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request does not include a scope param then it gets all the same scopes as the original authorization request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "sub": goodUpstreamSubject, @@ -1237,9 +1299,9 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { @@ -1255,14 +1317,14 @@ func TestRefreshGrant(t *testing.T) { name: "when a bad refresh token is sent in the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, - wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(), + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), }, }, refreshRequest: refreshRequestInputs{ @@ -1279,14 +1341,14 @@ func TestRefreshGrant(t *testing.T) { name: "when the access token is sent as if it were a refresh token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, - wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(), + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), }, }, refreshRequest: refreshRequestInputs{ @@ -1303,14 +1365,14 @@ func TestRefreshGrant(t *testing.T) { name: "when the wrong client ID is included in the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, - wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(), + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), }, }, refreshRequest: refreshRequestInputs{ @@ -1474,7 +1536,7 @@ func TestRefreshGrant(t *testing.T) { }, }, { - name: "when there is no OIDC refresh token in custom session data found in the session storage during the refresh request", + name: "when there is no OIDC refresh token nor access token in custom session data found in the session storage during the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: &psession.CustomSessionData{ @@ -1482,7 +1544,8 @@ func TestRefreshGrant(t *testing.T) { ProviderUID: oidcUpstreamResourceUID, ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{ - UpstreamRefreshToken: "", // this should not happen in practice + UpstreamRefreshToken: "", // this should not happen in practice. we should always have exactly one of these. + UpstreamAccessToken: "", }, }, modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1493,6 +1556,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: "", // this should not happen in practice + UpstreamAccessToken: "", }, }, ), @@ -1573,9 +1637,9 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). WithPerformRefreshError(errors.New("some upstream refresh error")).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ @@ -1595,17 +1659,17 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go - WithValidateTokenError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))). + WithValidateTokenAndMergeWithUserInfoError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))). Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { @@ -1621,7 +1685,7 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go - WithValidatedTokens(&oidctypes.Token{ + WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "sub": "something-different", @@ -1630,14 +1694,14 @@ func TestRefreshGrant(t *testing.T) { }). Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { @@ -1651,7 +1715,7 @@ func TestRefreshGrant(t *testing.T) { { name: "refresh grant with claims but not the subject claim", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", @@ -1659,14 +1723,14 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { @@ -1680,7 +1744,7 @@ func TestRefreshGrant(t *testing.T) { { name: "refresh grant with changed username claim", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", @@ -1690,14 +1754,14 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { @@ -1711,7 +1775,7 @@ func TestRefreshGrant(t *testing.T) { { name: "refresh grant with changed issuer claim", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", @@ -1721,14 +1785,14 @@ func TestRefreshGrant(t *testing.T) { }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ - customSessionData: initialUpstreamOIDCCustomSessionData(), + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), - wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { diff --git a/internal/plog/klog.go b/internal/plog/klog.go index f5ad9cdda..cd62b6c66 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/psession/pinniped_session.go b/internal/psession/pinniped_session.go index bbcc43174..3ed39ee0c 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -61,9 +61,27 @@ 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"` - UpstreamSubject string `json:"upstreamSubject"` - UpstreamIssuer string `json:"upstreamIssuer"` + + // 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"` + + // UpstreamSubject is the "sub" claim from the upstream identity provider from the user's initial login. We store this + // so that we can validate that it does not change upon refresh. + UpstreamSubject string `json:"upstreamSubject"` + + // UpstreamIssuer is the "iss" claim from the upstream identity provider from the user's initial login. We store this + // so that we can validate that it does not change upon refresh. + UpstreamIssuer string `json:"upstreamIssuer"` } // LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider. diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index d79a13c87..f5b6f8cd8 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 d5e3a7be3..486348798 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 543b87271..0e1d63c0a 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 aa6820816..92cc582c3 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.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 oidctestutil @@ -68,19 +68,22 @@ 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 -// TestUpstreamOIDCIdentityProvider.ValidateTokenFunc(). -type ValidateTokenArgs struct { +// ValidateTokenAndMergeWithUserInfoArgs is used to spy on calls to +// TestUpstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfoFunc(). +type ValidateTokenAndMergeWithUserInfoArgs struct { Ctx context.Context Tok *oauth2.Token ExpectedIDTokenNonce nonce.Nonce + RequireIDToken bool + RequireUserInfo bool } type ValidateRefreshArgs struct { @@ -150,6 +153,7 @@ type TestUpstreamOIDCIdentityProvider struct { ClientID string ResourceUID types.UID AuthorizationURL url.URL + UserInfoURL bool RevocationURL *url.URL UsernameClaim string GroupsClaim string @@ -172,9 +176,9 @@ 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) + ValidateTokenAndMergeWithUserInfoFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) exchangeAuthcodeAndValidateTokensCallCount int exchangeAuthcodeAndValidateTokensArgs []*ExchangeAuthcodeAndValidateTokenArgs @@ -182,10 +186,10 @@ type TestUpstreamOIDCIdentityProvider struct { passwordCredentialsGrantAndValidateTokensArgs []*PasswordCredentialsGrantAndValidateTokensArgs performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs - revokeRefreshTokenCallCount int - revokeRefreshTokenArgs []*RevokeRefreshTokenArgs - validateTokenCallCount int - validateTokenArgs []*ValidateTokenArgs + revokeTokenCallCount int + revokeTokenArgs []*RevokeTokenArgs + validateTokenAndMergeWithUserInfoCallCount int + validateTokenAndMergeWithUserInfoArgs []*ValidateTokenAndMergeWithUserInfoArgs } var _ provider.UpstreamOIDCIdentityProviderI = &TestUpstreamOIDCIdentityProvider{} @@ -210,6 +214,10 @@ func (u *TestUpstreamOIDCIdentityProvider) GetAuthorizationURL() *url.URL { return &u.AuthorizationURL } +func (u *TestUpstreamOIDCIdentityProvider) HasUserInfoURL() bool { + return u.UserInfoURL +} + func (u *TestUpstreamOIDCIdentityProvider) GetRevocationURL() *url.URL { return u.RevocationURL } @@ -284,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 { @@ -307,39 +316,41 @@ 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) (*oidctypes.Token, error) { - if u.validateTokenArgs == nil { - u.validateTokenArgs = make([]*ValidateTokenArgs, 0) +func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) { + if u.validateTokenAndMergeWithUserInfoArgs == nil { + u.validateTokenAndMergeWithUserInfoArgs = make([]*ValidateTokenAndMergeWithUserInfoArgs, 0) } - u.validateTokenCallCount++ - u.validateTokenArgs = append(u.validateTokenArgs, &ValidateTokenArgs{ + u.validateTokenAndMergeWithUserInfoCallCount++ + u.validateTokenAndMergeWithUserInfoArgs = append(u.validateTokenAndMergeWithUserInfoArgs, &ValidateTokenAndMergeWithUserInfoArgs{ Ctx: ctx, Tok: tok, ExpectedIDTokenNonce: expectedIDTokenNonce, + RequireIDToken: requireIDToken, + RequireUserInfo: requireUserInfo, }) - return u.ValidateTokenFunc(ctx, tok, expectedIDTokenNonce) + return u.ValidateTokenAndMergeWithUserInfoFunc(ctx, tok, expectedIDTokenNonce) } -func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenCallCount() int { - return u.validateTokenCallCount +func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfoCallCount() int { + return u.validateTokenAndMergeWithUserInfoCallCount } -func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenArgs(call int) *ValidateTokenArgs { - if u.validateTokenArgs == nil { - u.validateTokenArgs = make([]*ValidateTokenArgs, 0) +func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfoArgs(call int) *ValidateTokenAndMergeWithUserInfoArgs { + if u.validateTokenAndMergeWithUserInfoArgs == nil { + u.validateTokenAndMergeWithUserInfoArgs = make([]*ValidateTokenAndMergeWithUserInfoArgs, 0) } - return u.validateTokenArgs[call] + return u.validateTokenAndMergeWithUserInfoArgs[call] } type UpstreamIDPListerBuilder struct { @@ -524,18 +535,18 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPerformRefresh(t *te func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken( t *testing.T, expectedPerformedByUpstreamName string, - expectedArgs *ValidateTokenArgs, + expectedArgs *ValidateTokenAndMergeWithUserInfoArgs, ) { t.Helper() - var actualArgs *ValidateTokenArgs + var actualArgs *ValidateTokenAndMergeWithUserInfoArgs var actualNameOfUpstreamWhichMadeCall string actualCallCountAcrossAllOIDCUpstreams := 0 for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { - callCountOnThisUpstream := upstreamOIDC.validateTokenCallCount + callCountOnThisUpstream := upstreamOIDC.validateTokenAndMergeWithUserInfoCallCount actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream if callCountOnThisUpstream == 1 { actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name - actualArgs = upstreamOIDC.validateTokenArgs[0] + actualArgs = upstreamOIDC.validateTokenAndMergeWithUserInfoArgs[0] } } require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, @@ -551,47 +562,47 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes t.Helper() actualCallCountAcrossAllOIDCUpstreams := 0 for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { - actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenCallCount + actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenAndMergeWithUserInfoCallCount } require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()", ) } -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()", ) } @@ -600,24 +611,26 @@ func NewUpstreamIDPListerBuilder() *UpstreamIDPListerBuilder { } type TestUpstreamOIDCIdentityProviderBuilder struct { - name string - resourceUID types.UID - clientID string - scopes []string - idToken map[string]interface{} - refreshToken *oidctypes.RefreshToken - usernameClaim string - groupsClaim string - refreshedTokens *oauth2.Token - validatedTokens *oidctypes.Token - authorizationURL url.URL - additionalAuthcodeParams map[string]string - allowPasswordGrant bool - authcodeExchangeErr error - passwordGrantErr error - performRefreshErr error - revokeRefreshTokenErr error - validateTokenErr error + name string + resourceUID types.UID + clientID string + scopes []string + idToken map[string]interface{} + refreshToken *oidctypes.RefreshToken + accessToken *oidctypes.AccessToken + usernameClaim string + groupsClaim string + refreshedTokens *oauth2.Token + validatedAndMergedWithUserInfoTokens *oidctypes.Token + authorizationURL url.URL + hasUserInfoURL bool + additionalAuthcodeParams map[string]string + allowPasswordGrant bool + authcodeExchangeErr error + passwordGrantErr error + performRefreshErr error + revokeTokenErr error + validateTokenAndMergeWithUserInfoErr error } func (u *TestUpstreamOIDCIdentityProviderBuilder) WithName(value string) *TestUpstreamOIDCIdentityProviderBuilder { @@ -640,6 +653,16 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAuthorizationURL(value url return u } +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithUserInfoURL() *TestUpstreamOIDCIdentityProviderBuilder { + u.hasUserInfoURL = true + return u +} + +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutUserInfoURL() *TestUpstreamOIDCIdentityProviderBuilder { + u.hasUserInfoURL = false + return u +} + func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAllowPasswordGrant(value bool) *TestUpstreamOIDCIdentityProviderBuilder { u.allowPasswordGrant = value return u @@ -703,6 +726,20 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutRefreshToken() *TestUps return u } +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAccessToken(token string) *TestUpstreamOIDCIdentityProviderBuilder { + u.accessToken = &oidctypes.AccessToken{Token: token} + return u +} + +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithEmptyAccessToken() *TestUpstreamOIDCIdentityProviderBuilder { + u.accessToken = &oidctypes.AccessToken{Token: ""} + return u +} +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutAccessToken() *TestUpstreamOIDCIdentityProviderBuilder { + u.accessToken = nil + return u +} + func (u *TestUpstreamOIDCIdentityProviderBuilder) WithUpstreamAuthcodeExchangeError(err error) *TestUpstreamOIDCIdentityProviderBuilder { u.authcodeExchangeErr = err return u @@ -723,18 +760,18 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithPerformRefreshError(err er return u } -func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidatedTokens(tokens *oidctypes.Token) *TestUpstreamOIDCIdentityProviderBuilder { - u.validatedTokens = tokens +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidatedAndMergedWithUserInfoTokens(tokens *oidctypes.Token) *TestUpstreamOIDCIdentityProviderBuilder { + u.validatedAndMergedWithUserInfoTokens = tokens return u } -func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { - u.validateTokenErr = err +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenAndMergeWithUserInfoError(err error) *TestUpstreamOIDCIdentityProviderBuilder { + u.validateTokenAndMergeWithUserInfoErr = 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 } @@ -748,18 +785,19 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdent Scopes: u.scopes, AllowPasswordGrant: u.allowPasswordGrant, AuthorizationURL: u.authorizationURL, + UserInfoURL: u.hasUserInfoURL, AdditionalAuthcodeParams: u.additionalAuthcodeParams, ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.authcodeExchangeErr != nil { return nil, u.authcodeExchangeErr } - return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken}, nil + return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken, AccessToken: u.accessToken}, nil }, PasswordCredentialsGrantAndValidateTokensFunc: func(ctx context.Context, username, password string) (*oidctypes.Token, error) { if u.passwordGrantErr != nil { return nil, u.passwordGrantErr } - return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken}, nil + return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken, AccessToken: u.accessToken}, nil }, PerformRefreshFunc: func(ctx context.Context, refreshToken string) (*oauth2.Token, error) { if u.performRefreshErr != nil { @@ -767,14 +805,14 @@ 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 { - return nil, u.validateTokenErr + ValidateTokenAndMergeWithUserInfoFunc: func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { + if u.validateTokenAndMergeWithUserInfoErr != nil { + return nil, u.validateTokenAndMergeWithUserInfoErr } - return u.validatedTokens, nil + return u.validatedAndMergedWithUserInfoTokens, nil }, } } diff --git a/internal/testutil/testlogger/stdr_copied.go b/internal/testutil/testlogger/stdr_copied.go index d35bd86a2..69f3ab292 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 1e0a17a12..907d37223 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 7f14d6199..add67486a 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 0a7e37a26..1985823b8 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 65a7629f5..5618f7124 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 b9d371edf..6c71b83ab 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -61,6 +61,19 @@ func (p *ProviderConfig) GetRevocationURL() *url.URL { return p.RevocationURL } +func (p *ProviderConfig) HasUserInfoURL() bool { + providerJSON := &struct { + UserInfoURL string `json:"userinfo_endpoint"` + }{} + if err := p.Provider.Claims(providerJSON); err != nil { + // This should never happen in practice because we should have already successfully + // parsed these claims when p.Provider was created. + return false + } + + return len(providerJSON.UserInfoURL) > 0 +} + func (p *ProviderConfig) GetAdditionalAuthcodeParams() map[string]string { return p.AdditionalAuthcodeParams } @@ -113,7 +126,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C // There is no nonce to validate for a resource owner password credentials grant because it skips using // the authorize endpoint and goes straight to the token endpoint. const skipNonceValidation nonce.Nonce = "" - return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true, false) } func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { @@ -127,7 +140,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return nil, err } - return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true, false) } func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { @@ -138,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 @@ -172,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} @@ -194,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"` @@ -219,27 +242,40 @@ 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) } } // ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. -func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { +func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) { var validatedClaims = make(map[string]interface{}) var idTokenExpiry time.Time @@ -255,7 +291,7 @@ func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, if len(idTokenSubject) > 0 || !requireIDToken { // only fetch userinfo if the ID token has a subject or if we are ignoring the id token completely. // otherwise, defer to existing ID token validation - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken); err != nil { + if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken, requireUserInfo); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) } } @@ -309,10 +345,10 @@ func (p *ProviderConfig) validateIDToken(ctx context.Context, tok *oauth2.Token, return idTokenExpiry, idTok, nil } -func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) error { +func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool, requireUserInfo bool) error { idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) - userInfo, err := p.maybeFetchUserInfo(ctx, tok) + userInfo, err := p.maybeFetchUserInfo(ctx, tok, requireUserInfo) if err != nil { return err } @@ -355,17 +391,13 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t return nil } -func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) { - providerJSON := &struct { - UserInfoURL string `json:"userinfo_endpoint"` - }{} - if err := p.Provider.Claims(providerJSON); err != nil { - // this should never happen because we should have already parsed these claims at an earlier stage - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err) - } - - // implementing the user info endpoint is not required, skip this logic when it is absent - if len(providerJSON.UserInfoURL) == 0 { +func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token, requireUserInfo bool) (*coreosoidc.UserInfo, error) { + // implementing the user info endpoint is not required by the OIDC spec, but we may require it in certain situations. + if !p.HasUserInfoURL() { + if requireUserInfo { + // TODO should these all be http errors? + return nil, httperr.New(http.StatusInternalServerError, "userinfo endpoint not found, but is required") + } return nil, nil } diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 90dc6f1f9..f8f15ce10 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" @@ -40,6 +41,9 @@ func TestProviderConfig(t *testing.T) { Endpoint: oauth2.Endpoint{AuthURL: "https://example.com"}, Scopes: []string{"scope1", "scope2"}, }, + Provider: &mockProvider{ + rawClaims: []byte(`{"userinfo_endpoint": "https://example.com/userinfo"}`), + }, } require.Equal(t, "test-name", p.GetName()) require.Equal(t, "test-client-id", p.GetClientID()) @@ -54,6 +58,16 @@ func TestProviderConfig(t *testing.T) { require.True(t, p.AllowsPasswordGrant()) p.AllowPasswordGrant = false require.False(t, p.AllowsPasswordGrant()) + + require.True(t, p.HasUserInfoURL()) + p.Provider = &mockProvider{ + rawClaims: []byte(`{"some_other_endpoint": "https://example.com/blah"}`), + } + require.False(t, p.HasUserInfoURL()) + p.Provider = &mockProvider{ + rawClaims: []byte(`{`), + } + require.False(t, p.HasUserInfoURL()) }) const ( @@ -455,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 { @@ -536,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. })) @@ -574,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) } }) } @@ -609,6 +740,7 @@ func TestProviderConfig(t *testing.T) { tok *oauth2.Token nonce nonce.Nonce requireIDToken bool + requireUserInfo bool userInfo *oidc.UserInfo rawClaims []byte userInfoErr error @@ -694,6 +826,34 @@ func TestProviderConfig(t *testing.T) { }, }, }, + { + name: "userinfo is required, token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + requireUserInfo: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, + }, { name: "claims from userinfo override id token claims", tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}), @@ -748,6 +908,32 @@ func TestProviderConfig(t *testing.T) { }, }, }, + { + name: "token with id, access and refresh tokens and valid nonce, but no userinfo endpoint from discovery and it's not required", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + requireUserInfo: false, + rawClaims: []byte(`{"not_the_userinfo_endpoint": "some-other-endpoint"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + }, + }, + }, + }, { name: "token with no id token but valid userinfo", tok: testTokenWithoutIDToken, @@ -838,6 +1024,23 @@ func TestProviderConfig(t *testing.T) { userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantErr: "received response missing ID token", }, + { + name: "expected to have userinfo, but doesn't", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireUserInfo: true, + rawClaims: []byte(`{}`), + wantErr: "could not fetch user info claims: userinfo endpoint not found, but is required", + }, + { + name: "expected to have id token and userinfo, but doesn't have either", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireUserInfo: true, + requireIDToken: true, + rawClaims: []byte(`{}`), + wantErr: "received response missing ID token", + }, { name: "mismatched access token hash", tok: testTokenWithoutIDToken, @@ -898,7 +1101,7 @@ func TestProviderConfig(t *testing.T) { userInfoErr: tt.userInfoErr, }, } - gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) + gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken, tt.requireUserInfo) if tt.wantErr != "" { require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) @@ -982,6 +1185,36 @@ func TestProviderConfig(t *testing.T) { rawClaims: []byte(`{}`), // user info not supported wantUserInfoCalled: false, }, + { + name: "valid but userinfo endpoint could not be found due to parse error", + authCode: "valid", + returnIDTok: validIDToken, + wantToken: oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Expiry: metav1.Time{}, + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: validIDToken, + Expiry: metav1.Time{}, + Claims: map[string]interface{}{ + "foo": "bar", + "bat": "baz", + "aud": "test-client-id", + "iat": 1.606768593e+09, + "jti": "test-jti", + "nbf": 1.606768593e+09, + "sub": "test-user", + }, + }, + }, + // cannot be parsed as json, but note that in this case constructing a real provider would have failed + rawClaims: []byte(`{`), + wantUserInfoCalled: false, + }, { name: "valid", authCode: "valid", @@ -1011,13 +1244,6 @@ func TestProviderConfig(t *testing.T) { rawClaims: []byte(`{}`), // user info not supported wantUserInfoCalled: false, }, - { - name: "user info discovery parse error", - authCode: "valid", - returnIDTok: validIDToken, - rawClaims: []byte(`junk`), // user info discovery fails - wantErr: "could not fetch user info claims: could not unmarshal discovery JSON: invalid character 'j' looking for beginning of value", - }, { name: "user info fetch error", authCode: "valid", diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 325ec4a4c..d9364689e 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.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 oidcclient implements a CLI OIDC login flow. @@ -822,7 +822,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least // some providers do not include one, so we skip the nonce validation here (but not other validations). - return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true) + return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true, false) } func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index e55f8e03a..8ee920d72 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_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 oidcclient @@ -406,7 +406,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). @@ -453,7 +453,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). Return(nil, fmt.Errorf("some validation error")) mock.EXPECT(). PerformRefresh(gomock.Any(), "test-refresh-token-returning-invalid-id-token"). @@ -1648,7 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 47fa66845..eac464688 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/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 29a46e799..5fdd60604 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -129,6 +129,40 @@ func TestSupervisorLogin(t *testing.T) { wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" }, wantDownstreamIDTokenGroups: env.SupervisorUpstreamOIDC.ExpectedGroups, }, + { + name: "oidc without refresh token", + maybeSkip: func(t *testing.T) { + // never need to skip this test + }, + createIDP: func(t *testing.T) string { + t.Helper() + oidcIDP := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ + Issuer: env.SupervisorUpstreamOIDC.Issuer, + TLS: &idpv1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), + }, + Client: idpv1alpha1.OIDCClient{ + SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name, + }, + Claims: idpv1alpha1.OIDCClaims{ + Username: env.SupervisorUpstreamOIDC.UsernameClaim, + Groups: env.SupervisorUpstreamOIDC.GroupsClaim, + }, + AuthorizationConfig: idpv1alpha1.OIDCAuthorizationConfig{ + AdditionalScopes: []string{"email"}, // does not ask for offline_access. + }, + }, idpv1alpha1.PhaseReady) + return oidcIDP.Name + }, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Extra["username"] = "some-incorrect-username" + }, + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" }, + wantDownstreamIDTokenGroups: env.SupervisorUpstreamOIDC.ExpectedGroups, + }, { name: "oidc with CLI password flow", maybeSkip: func(t *testing.T) { diff --git a/test/testlib/env.go b/test/testlib/env.go index 7ce6d0bec..999df7a14 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