diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index ae6a3a1f7..187f913d1 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -4,15 +4,19 @@ package supervisorstorage import ( + "context" "errors" + "fmt" "time" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" + "k8s.io/utils/strings/slices" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" @@ -24,6 +28,7 @@ import ( "go.pinniped.dev/internal/fositestorage/refreshtoken" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/psession" ) const minimumRepeatInterval = 30 * time.Second @@ -119,10 +124,10 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { if garbageCollectAfterTime.Before(frozenClock.Now()) { storageType, isSessionStorage := secret.Labels[crud.SecretLabelKey] if isSessionStorage { - err := c.maybeRevokeUpstreamOIDCRefreshToken(storageType, secret) + err := c.maybeRevokeUpstreamOIDCRefreshToken(ctx.Context, storageType, secret) if err != nil { // Log the error for debugging purposes, but still carry on to delete the Secret despite the error. - plog.DebugErr("garbage collector could not revoke upstream refresh token", err, logKV(secret)) + plog.WarningErr("garbage collector could not revoke upstream refresh token", err, logKV(secret)) } } @@ -143,70 +148,96 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { return nil } -//nolint:godot // do not complain about the following to-do comment -// TODO write unit tests for all of the following cases. Note that there is already a test -// double implemented for the RevokeRefreshToken() method on the objects in the idpCache -// to help with the test mocking. See RevokeRefreshTokenCallCount() and RevokeRefreshTokenArgs(int) -// in TestUpstreamOIDCIdentityProvider (in file oidctestutil.go). This will allowing following -// the pattern used in other unit tests that fill the idpCache with mock providers using the builders -// from oidctestutil.go. -func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(storageType string, secret *v1.Secret) error { +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. // 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. switch storageType { case authorizationcode.TypeLabelValue: - // For authcode storage, check if the authcode was used. If the authcode was never used, then its - // storage must contain the latest upstream refresh token, so revoke it. - // TODO Use ReadFromSecret from the authorizationcode package under fositestorage to validate/parse the Secret, return any errors - // TODO return nil if the upstream type is not OIDC - // TODO return nil if the authcode is *NOT* active (meaning that it was already used) - // TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found - // TODO use the cached interface to revoke the refresh token, return any error - plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret)) - return nil + 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. + 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) + case accesstoken.TypeLabelValue: - // For access token storage, check if the "offline_access" scope was granted on the downstream - // session. If it was not, then the user could not possibly have performed a downstream refresh. - // In this case, the access token storage has the latest version of the upstream refresh token, - // so call the upstream issuer to revoke it. - // TODO Implement ReadFromSecret in the accesstoken package, similar to how it was done in the authorizationcode package - // TODO Use that the new ReadFromSecret func to validate/parse the Secret, return any errors - // TODO return nil if the upstream type is not OIDC - // TODO return nil if the "offline_access" scope was *NOT* granted on the downstream session - // TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found - // TODO use the cached interface to revoke the refresh token, return any error - plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret)) - return nil + // 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 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. + accessTokenSession, err := accesstoken.ReadFromSecret(secret) + if err != nil { + return err + } + pinnipedSession := accessTokenSession.Request.Session.(*psession.PinnipedSession) + if slices.Contains(accessTokenSession.Request.GetGrantedScopes(), coreosoidc.ScopeOfflineAccess) { + return nil + } + return c.revokeUpstreamOIDCRefreshToken(ctx, pinnipedSession.Custom, secret) + case refreshtoken.TypeLabelValue: - // For refresh token storage, revoke its upstream refresh token. This refresh token storage could - // be the result of the authcode token exchange, or it could be the result of a downstream refresh. - // Either way, it always contains the latest upstream refresh token when it exists. - // TODO Implement ReadFromSecret in the refreshtoken package, similar to how it was done in the authorizationcode package - // TODO Use that new ReadFromSecret func to validate/parse the Secret, return any errors - // TODO return nil if the upstream type is not OIDC - // TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found - // TODO use the cached interface to always revoke the refresh token, return any error - plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret)) - return nil + // 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. + refreshTokenSession, err := refreshtoken.ReadFromSecret(secret) + if err != nil { + return err + } + return c.revokeUpstreamOIDCRefreshToken(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. 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. return nil + default: // There are no other storage types, so this should never happen in practice. return errors.New("garbage collector saw invalid label on Secret when trying to determine if upstream revocation was needed") } } +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. + if customSessionData.ProviderType != psession.ProviderTypeOIDC { + return nil + } + + // Try to find the provider that was originally used to create the stored session. + var foundOIDCIdentityProviderI provider.UpstreamOIDCIdentityProviderI + for _, p := range c.idpCache.GetOIDCIdentityProviders() { + if p.GetName() == customSessionData.ProviderName && p.GetResourceUID() == customSessionData.ProviderUID { + foundOIDCIdentityProviderI = p + } + } + if foundOIDCIdentityProviderI == nil { + 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 { + return err + } + + plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret)) + return nil +} + 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 5451a9189..7e4469348 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -5,10 +5,12 @@ package supervisorstorage import ( "context" + "encoding/json" "errors" "testing" "time" + "github.com/ory/fosite" "github.com/sclevine/spec" "github.com/sclevine/spec/report" "github.com/stretchr/testify/require" @@ -23,7 +25,14 @@ import ( kubetesting "k8s.io/client-go/testing" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/fositestorage/accesstoken" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/refreshtoken" + "go.pinniped.dev/internal/oidc/clientregistry" + "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" + "go.pinniped.dev/internal/testutil/oidctestutil" ) func TestGarbageCollectorControllerInformerFilters(t *testing.T) { @@ -130,10 +139,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. - var startInformersAndController = func() { + var startInformersAndController = func(idpCache provider.DynamicUpstreamIDPProvider) { // Set this at the last second to allow for injection of server override. subject = GarbageCollectorController( - nil, // TODO put an IDP cache here for these tests (use the builder like other controller tests) + idpCache, fakeClock, deleteOptionsRecorder, kubeInformers.Core().V1().Secrets(), @@ -185,7 +194,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { when("there are secrets without the garbage-collect-after annotation", func() { it("does not delete those secrets", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) require.Empty(t, kubeClient.Actions()) @@ -196,7 +205,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) - when("there are secrets with the garbage-collect-after annotation", func() { + when("there are any secrets with the garbage-collect-after annotation", func() { it.Before(func() { firstExpiredSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -238,7 +247,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("should delete any that are past their expiration", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.ElementsMatch( @@ -262,6 +271,651 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) + when("there are valid, expired authcode secrets", 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{ + UpstreamRefreshToken: "fake-upstream-refresh-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{ + UpstreamRefreshToken: "other-fake-upstream-refresh-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"). + WithRevokeRefreshTokenError(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, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // Both authcode session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + testutil.NewPreconditions("uid-456", "rv-456"), + }, + *deleteOptions, + ) + }) + }) + + when("there is an invalid, expired authcode secret", func() { + it.Before(func() { + invalidOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "", // it is invalid for there to be a missing request ID + 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{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + invalidOIDCAuthcodeSessionJSON, err := json.Marshal(invalidOIDCAuthcodeSession) + r.NoError(err) + invalidOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalidOIDCAuthcodeSession", + 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": invalidOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + r.NoError(kubeInformerClient.Tracker().Add(invalidOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(invalidOIDCAuthcodeSessionSecret)) + }) + + it("should remove the secret without revoking any upstream tokens", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(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) + + // The invalid authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "invalidOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there is a valid, expired authcode secret but its upstream name does not match any existing upstream", func() { + it.Before(func() { + wrongProviderNameOIDCAuthcodeSession := &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-will-not-match", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + wrongProviderNameOIDCAuthcodeSessionJSON, err := json.Marshal(wrongProviderNameOIDCAuthcodeSession) + r.NoError(err) + wrongProviderNameOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wrongProviderNameOIDCAuthcodeSession", + 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": wrongProviderNameOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(wrongProviderNameOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + }) + + it("should remove the secret without revoking any upstream tokens", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(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) + + // The authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "wrongProviderNameOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there is a valid, expired authcode secret but its upstream UID does not match any existing upstream", func() { + it.Before(func() { + wrongProviderNameOIDCAuthcodeSession := &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-will-not-match", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + wrongProviderNameOIDCAuthcodeSessionJSON, err := json.Marshal(wrongProviderNameOIDCAuthcodeSession) + r.NoError(err) + wrongProviderNameOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wrongProviderNameOIDCAuthcodeSession", + 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": wrongProviderNameOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(wrongProviderNameOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + }) + + it("should remove the secret without revoking any upstream tokens", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(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) + + // The authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "wrongProviderNameOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there is a valid, expired authcode secret but the upstream revocation fails", 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{ + UpstreamRefreshToken: "fake-upstream-refresh-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)) + }) + + it("should remove the secret anyway because it has expired", 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 + 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, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // The authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there are valid, expired access token secrets", 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{ + UpstreamRefreshToken: "offline-access-granted-fake-upstream-refresh-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{ + UpstreamRefreshToken: "fake-upstream-refresh-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"). + WithRevokeRefreshTokenError(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, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // Both session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + testutil.NewPreconditions("uid-456", "rv-456"), + }, + *deleteOptions, + ) + }) + }) + + when("there are valid, expired refresh secrets", 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{ + UpstreamRefreshToken: "fake-upstream-refresh-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"). + WithRevokeRefreshTokenError(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, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // The secret is deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "oidcRefreshSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + when("very little time has passed since the previous sync call", func() { it.Before(func() { // Add a secret that will expire in 20 seconds. @@ -279,7 +933,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("should do nothing to avoid being super chatty since it is called for every change to any Secret, until more time has passed", func() { - startInformersAndController() + startInformersAndController(nil) require.Empty(t, kubeClient.Actions()) // Run sync once with the current time set to frozenTime. @@ -344,7 +998,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("does not delete that secret", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.ElementsMatch( @@ -393,7 +1047,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("ignores the error and continues on to delete the next expired Secret", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.ElementsMatch( diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go index dbd57fb35..792b76e71 100644 --- a/internal/fositestorage/accesstoken/accesstoken.go +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -10,6 +10,7 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -42,7 +43,7 @@ type accessTokenStorage struct { storage crud.Storage } -type session struct { +type Session struct { Request *fosite.Request `json:"request"` Version string `json:"version"` } @@ -51,6 +52,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } +// ReadFromSecret reads the contents of a Secret as a Session. +func ReadFromSecret(secret *v1.Secret) (*Session, error) { + session := newValidEmptyAccessTokenSession() + err := crud.FromSecret(TypeLabelValue, secret, session) + if err != nil { + return nil, err + } + if session.Version != accessTokenStorageVersion { + return nil, fmt.Errorf("%w: access token session has version %s instead of %s", + ErrInvalidAccessTokenRequestVersion, session.Version, accessTokenStorageVersion) + } + if session.Request.ID == "" { + return nil, fmt.Errorf("malformed access token session: %w", ErrInvalidAccessTokenRequestData) + } + return session, nil +} + func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error { return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) } @@ -64,7 +82,7 @@ func (a *accessTokenStorage) CreateAccessTokenSession(ctx context.Context, signa _, err = a.storage.Create( ctx, signature, - &session{Request: request, Version: accessTokenStorageVersion}, + &Session{Request: request, Version: accessTokenStorageVersion}, map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, ) return err @@ -84,7 +102,7 @@ func (a *accessTokenStorage) DeleteAccessTokenSession(ctx context.Context, signa return a.storage.Delete(ctx, signature) } -func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) { +func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*Session, string, error) { session := newValidEmptyAccessTokenSession() rv, err := a.storage.Get(ctx, signature, session) @@ -108,8 +126,8 @@ func (a *accessTokenStorage) getSession(ctx context.Context, signature string) ( return session, rv, nil } -func newValidEmptyAccessTokenSession() *session { - return &session{ +func newValidEmptyAccessTokenSession() *Session { + return &Session{ Request: &fosite.Request{ Client: &clientregistry.Client{}, Session: &psession.PinnipedSession{}, diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index dc4687427..5f3933df9 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -277,3 +278,119 @@ func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInt secrets := client.CoreV1().Secrets(namespace) return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) } + +func TestReadFromSecret(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + wantSession *Session + wantErr string + }{ + { + name: "happy path", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","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"}}}},"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + }, + wantSession: &Session{ + Version: "2", + Request: &fosite.Request{ + ID: "abcd-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Fosite: &openid.DefaultSession{ + Username: "snorlax", + Subject: "panda", + }, + Custom: &psession.CustomSessionData{ + ProviderUID: "fake-provider-uid", + ProviderName: "fake-provider-name", + ProviderType: "fake-provider-type", + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + }, + }, + { + name: "wrong secret type", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1"},"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/not-access-token", + }, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-access-token must equal storage.pinniped.dev/access-token", + }, + { + name: "wrong session version", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1"},"version":"wrong-version-here","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + }, + wantErr: "access token request data has wrong version: access token session has version wrong-version-here instead of 2", + }, + { + name: "missing request", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + }, + wantErr: "malformed access token session: access token request data must be present", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + session, err := ReadFromSecret(tt.secret) + if tt.wantErr == "" { + require.NoError(t, err) + require.Equal(t, tt.wantSession, session) + } else { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, session) + } + }) + } +} diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 04ebe12dd..1d379a66c 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -39,7 +39,7 @@ type authorizeCodeStorage struct { storage crud.Storage } -type AuthorizeCodeSession struct { +type Session struct { Active bool `json:"active"` Request *fosite.Request `json:"request"` Version string `json:"version"` @@ -49,8 +49,8 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } -// ReadFromSecret reads the contents of a Secret as an AuthorizeCodeSession. -func ReadFromSecret(secret *v1.Secret) (*AuthorizeCodeSession, error) { +// ReadFromSecret reads the contents of a Secret as a Session. +func ReadFromSecret(secret *v1.Secret) (*Session, error) { session := NewValidEmptyAuthorizeCodeSession() err := crud.FromSecret(TypeLabelValue, secret, session) if err != nil { @@ -88,7 +88,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s // of the consent authorization request. It is used to identify the session. // signature for lookup in the DB - _, err = a.storage.Create(ctx, signature, &AuthorizeCodeSession{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil) + _, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil) return err } @@ -126,7 +126,7 @@ func (a *authorizeCodeStorage) InvalidateAuthorizeCodeSession(ctx context.Contex return nil } -func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) (*AuthorizeCodeSession, string, error) { +func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) (*Session, string, error) { session := NewValidEmptyAuthorizeCodeSession() rv, err := a.storage.Get(ctx, signature, session) @@ -155,8 +155,8 @@ func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) return session, rv, nil } -func NewValidEmptyAuthorizeCodeSession() *AuthorizeCodeSession { - return &AuthorizeCodeSession{ +func NewValidEmptyAuthorizeCodeSession() *Session { + return &Session{ Request: &fosite.Request{ Client: &clientregistry.Client{}, Session: &psession.PinnipedSession{}, diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index eadfd0a42..fdb9653ed 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -403,7 +403,7 @@ func TestReadFromSecret(t *testing.T) { tests := []struct { name string secret *corev1.Secret - wantSession *AuthorizeCodeSession + wantSession *Session wantErr string }{ { @@ -422,7 +422,7 @@ func TestReadFromSecret(t *testing.T) { }, Type: "storage.pinniped.dev/authcode", }, - wantSession: &AuthorizeCodeSession{ + wantSession: &Session{ Version: "2", Active: true, Request: &fosite.Request{ diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index c2d4336ce..8faddd5bc 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -10,6 +10,7 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -42,7 +43,7 @@ type refreshTokenStorage struct { storage crud.Storage } -type session struct { +type Session struct { Request *fosite.Request `json:"request"` Version string `json:"version"` } @@ -51,6 +52,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } +// ReadFromSecret reads the contents of a Secret as a Session. +func ReadFromSecret(secret *v1.Secret) (*Session, error) { + session := newValidEmptyRefreshTokenSession() + err := crud.FromSecret(TypeLabelValue, secret, session) + if err != nil { + return nil, err + } + if session.Version != refreshTokenStorageVersion { + return nil, fmt.Errorf("%w: refresh token session has version %s instead of %s", + ErrInvalidRefreshTokenRequestVersion, session.Version, refreshTokenStorageVersion) + } + if session.Request.ID == "" { + return nil, fmt.Errorf("malformed refresh token session: %w", ErrInvalidRefreshTokenRequestData) + } + return session, nil +} + func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) } @@ -64,7 +82,7 @@ func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, sig _, err = a.storage.Create( ctx, signature, - &session{Request: request, Version: refreshTokenStorageVersion}, + &Session{Request: request, Version: refreshTokenStorageVersion}, map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, ) return err @@ -84,7 +102,7 @@ func (a *refreshTokenStorage) DeleteRefreshTokenSession(ctx context.Context, sig return a.storage.Delete(ctx, signature) } -func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) { +func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*Session, string, error) { session := newValidEmptyRefreshTokenSession() rv, err := a.storage.Get(ctx, signature, session) @@ -108,8 +126,8 @@ func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) return session, rv, nil } -func newValidEmptyRefreshTokenSession() *session { - return &session{ +func newValidEmptyRefreshTokenSession() *Session { + return &Session{ Request: &fosite.Request{ Client: &clientregistry.Client{}, Session: &psession.PinnipedSession{}, diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index e8a155c9f..92c728e94 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -277,3 +278,119 @@ func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInt secrets := client.CoreV1().Secrets(namespace) return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) } + +func TestReadFromSecret(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + wantSession *Session + wantErr string + }{ + { + name: "happy path", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","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"}}}},"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + }, + wantSession: &Session{ + Version: "2", + Request: &fosite.Request{ + ID: "abcd-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Fosite: &openid.DefaultSession{ + Username: "snorlax", + Subject: "panda", + }, + Custom: &psession.CustomSessionData{ + ProviderUID: "fake-provider-uid", + ProviderName: "fake-provider-name", + ProviderType: "fake-provider-type", + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + }, + }, + { + name: "wrong secret type", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1"},"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/not-refresh-token", + }, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-refresh-token must equal storage.pinniped.dev/refresh-token", + }, + { + name: "wrong session version", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1"},"version":"wrong-version-here","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + }, + wantErr: "refresh token request data has wrong version: refresh token session has version wrong-version-here instead of 2", + }, + { + name: "missing request", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + }, + wantErr: "malformed refresh token session: refresh token request data must be present", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + session, err := ReadFromSecret(tt.secret) + if tt.wantErr == "" { + require.NoError(t, err) + require.Equal(t, tt.wantSession, session) + } else { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, session) + } + }) + } +} diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 07986df1d..118e31c50 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -495,6 +495,43 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes ) } +func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeRefreshToken( + t *testing.T, + expectedPerformedByUpstreamName string, + expectedArgs *RevokeRefreshTokenArgs, +) { + t.Helper() + var actualArgs *RevokeRefreshTokenArgs + var actualNameOfUpstreamWhichMadeCall string + actualCallCountAcrossAllOIDCUpstreams := 0 + for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { + callCountOnThisUpstream := upstreamOIDC.revokeRefreshTokenCallCount + actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream + if callCountOnThisUpstream == 1 { + actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name + actualArgs = upstreamOIDC.revokeRefreshTokenArgs[0] + } + } + require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, + "should have been exactly one call to RevokeRefreshToken() by all OIDC upstreams", + ) + require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, + "RevokeRefreshToken() was called on the wrong OIDC upstream", + ) + require.Equal(t, expectedArgs, actualArgs) +} + +func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeRefreshToken(t *testing.T) { + t.Helper() + actualCallCountAcrossAllOIDCUpstreams := 0 + for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { + actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeRefreshTokenCallCount + } + require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, + "expected exactly zero calls to RevokeRefreshToken()", + ) +} + func NewUpstreamIDPListerBuilder() *UpstreamIDPListerBuilder { return &UpstreamIDPListerBuilder{} } @@ -633,6 +670,11 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenError(err err return u } +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { + u.revokeRefreshTokenErr = err + return u +} + func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdentityProvider { return &TestUpstreamOIDCIdentityProvider{ Name: u.name,