diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index eeec6ad40..2bab1bcd2 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package supervisorstorage @@ -227,8 +227,7 @@ func (c *garbageCollectorController) maybeRevokeUpstreamOIDCToken(ctx context.Co 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 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 + // These are deleted during downstream authcode exchange. The upstream token contained inside will // be revoked by one of the other cases above. return nil diff --git a/internal/federationdomain/endpoints/token/token_handler_test.go b/internal/federationdomain/endpoints/token/token_handler_test.go index 260d16b9e..2bd515762 100644 --- a/internal/federationdomain/endpoints/token/token_handler_test.go +++ b/internal/federationdomain/endpoints/token/token_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package token @@ -944,23 +944,17 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { requireInvalidAccessTokenStorage(t, parsedResponseBody, oauthStore) // This was previously invalidated by the first request, so it remains invalidated requireInvalidPKCEStorage(t, authCode, oauthStore) - // Fosite never cleans up OpenID Connect session storage, so it is still there. - // Note that customSessionData is only relevant to refresh grant, so we leave it as nil for this - // authcode exchange test, even though in practice it would actually be in the session. - requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, - test.authcodeExchange.want.wantClientID, - test.authcodeExchange.want.wantRequestedScopes, test.authcodeExchange.want.wantGrantedScopes, - test.authcodeExchange.want.wantUsername, test.authcodeExchange.want.wantGroups, - nil, test.authcodeExchange.want.wantAdditionalClaims, approxRequestTime) + // OpenID Connect session storage is deleted during a successful authcode exchange. + requireDeletedOIDCStorage(t, authCode, oauthStore) // Check that the access token and refresh token storage were both deleted, and the number of other storage objects did not change. testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: pkce.TypeLabelValue}, 0) // Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage. - testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2) + testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 1) }) } } @@ -4425,11 +4419,9 @@ func TestRefreshGrant(t *testing.T) { // Refreshed ID tokens do not include the nonce from the original auth request wantNonceValueInIDToken := false - requireTokenEndpointBehavior(t, + requireTokenEndpointBehavior( + t, test.refreshRequest.want, - test.authcodeExchange.want.wantUsername, // the old username from the initial login - test.authcodeExchange.want.wantGroups, // the old groups from the initial login - test.authcodeExchange.customSessionData, // the old custom session data from the initial login wantNonceValueInIDToken, refreshResponse, authCode, @@ -4564,11 +4556,9 @@ func exchangeAuthcodeForTokens( wantNonceValueInIDToken := true // ID tokens returned by the authcode exchange must include the nonce from the auth request (unlike refreshed ID tokens) - requireTokenEndpointBehavior(t, + requireTokenEndpointBehavior( + t, test.want, - test.want.wantUsername, // the old username from the initial login - test.want.wantGroups, // the old groups from the initial login - test.customSessionData, // the old custom session data from the initial login wantNonceValueInIDToken, rsp, authCode, @@ -4584,9 +4574,6 @@ func exchangeAuthcodeForTokens( func requireTokenEndpointBehavior( t *testing.T, test tokenEndpointResponseExpectedValues, - oldUsername string, - oldGroups []string, - oldCustomSessionData *psession.CustomSessionData, wantNonceValueInIDToken bool, tokenEndpointResponse *httptest.ResponseRecorder, authCode string, @@ -4611,16 +4598,13 @@ func requireTokenEndpointBehavior( requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets, requestTime) requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, test.wantUsername, test.wantGroups, test.wantCustomSessionDataStored, test.wantAdditionalClaims, secrets, requestTime) requireInvalidPKCEStorage(t, authCode, oauthStore) - // Performing a refresh does not update the OIDC storage, so after a refresh it should still have the old custom session data and old username and groups from the initial login. - requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, oldUsername, oldGroups, oldCustomSessionData, test.wantAdditionalClaims, requestTime) + requireDeletedOIDCStorage(t, authCode, oauthStore) // The OIDC storage was deleted during the authcode exchange. expectedNumberOfRefreshTokenSessionsStored := 0 if wantRefreshToken { expectedNumberOfRefreshTokenSessionsStored = 1 } - expectedNumberOfIDSessionsStored := 0 if wantIDToken { - expectedNumberOfIDSessionsStored = 1 requireValidIDToken(t, parsedResponseBody, jwtSigningKey, test.wantClientID, wantNonceValueInIDToken, test.wantUsername, test.wantGroups, test.wantAdditionalClaims, parsedResponseBody["access_token"].(string), requestTime) } if wantRefreshToken { @@ -4631,9 +4615,9 @@ func requireTokenEndpointBehavior( testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: pkce.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, expectedNumberOfRefreshTokenSessionsStored) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, expectedNumberOfIDSessionsStored) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 0) // Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage. - testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2+expectedNumberOfRefreshTokenSessionsStored+expectedNumberOfIDSessionsStored) + testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2+expectedNumberOfRefreshTokenSessionsStored) } else { require.NotNil(t, test.wantErrorResponseBody, "problem with test table setup: wanted failure but did not specify failure response body") @@ -5001,52 +4985,11 @@ func requireInvalidPKCEStorage( require.True(t, errors.Is(err, fosite.ErrNotFound)) } -func requireValidOIDCStorage( - t *testing.T, - body map[string]interface{}, - code string, - storage openid.OpenIDConnectRequestStorage, - wantClientID string, - wantRequestedScopes []string, - wantGrantedScopes []string, - wantUsername string, - wantGroups []string, - wantCustomSessionData *psession.CustomSessionData, - wantAdditionalClaims map[string]interface{}, - requestTime time.Time, -) { +func requireDeletedOIDCStorage(t *testing.T, code string, storage openid.OpenIDConnectRequestStorage) { t.Helper() - if slices.Contains(wantGrantedScopes, "openid") { - // Make sure the OIDC session is still there. Note that Fosite stores OIDC sessions using the full auth code as a key. - storedRequest, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) - require.NoError(t, err) - - // Fosite stores OIDC sessions with only the nonce in the original request form. - accessToken, ok := body["access_token"] - require.True(t, ok) - accessTokenString, ok := accessToken.(string) - require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) - require.NotEmpty(t, accessTokenString) - - requireValidStoredRequest( - t, - storedRequest, - storedRequest.Sanitize([]string{"nonce"}).GetRequestForm(), - wantClientID, - wantRequestedScopes, - wantGrantedScopes, - false, - wantUsername, - wantGroups, - wantCustomSessionData, - wantAdditionalClaims, - requestTime, - ) - } else { - _, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) - require.True(t, errors.Is(err, fosite.ErrNotFound)) - } + _, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) + require.True(t, errors.Is(err, fosite.ErrNotFound)) } func requireValidStoredRequest( diff --git a/internal/federationdomain/endpointsmanager/manager_test.go b/internal/federationdomain/endpointsmanager/manager_test.go index 639160205..e857e18bd 100644 --- a/internal/federationdomain/endpointsmanager/manager_test.go +++ b/internal/federationdomain/endpointsmanager/manager_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package endpointsmanager @@ -211,8 +211,8 @@ func TestManager(t *testing.T) { r.Equal("some-state", actualLocationQueryParams.Get("state")) // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. - r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+3, - "did not perform any kube actions during the callback request, but should have") + r.Equal(numberOfKubeActionsBeforeThisRequest+3, len(kubeClient.Actions()), + "did not perform expected number of kube actions during the callback request") // Return the important parts of the response so we can use them in our next request to the token endpoint. return actualLocationQueryParams.Get("code") @@ -253,8 +253,8 @@ func TestManager(t *testing.T) { oidctestutil.VerifyECDSAIDToken(t, jwkIssuer, downstreamClientID, privateKey, idToken) // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. - r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+9, - "did not perform any kube actions during the callback request, but should have") + r.Equal(numberOfKubeActionsBeforeThisRequest+10, len(kubeClient.Actions()), + "did not perform expected number of kube actions during the callback request") } requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) *jose.JSONWebKeySet { diff --git a/internal/federationdomain/oidc/oidc.go b/internal/federationdomain/oidc/oidc.go index 2f5c89418..dddee951e 100644 --- a/internal/federationdomain/oidc/oidc.go +++ b/internal/federationdomain/oidc/oidc.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package oidc contains common OIDC functionality needed by FederationDomains to implement @@ -6,11 +6,9 @@ package oidc import ( - "context" "crypto/subtle" "fmt" "net/http" - "net/url" "time" "github.com/felixge/httpsnoop" @@ -131,10 +129,6 @@ func FositeOauth2Helper( jwksProvider jwks.DynamicJWKSProvider, timeoutsConfiguration timeouts.Configuration, ) fosite.OAuth2Provider { - isRedirectURISecureStrict := func(_ context.Context, uri *url.URL) bool { - return fosite.IsRedirectURISecureStrict(uri) - } - oauthConfig := &fosite.Config{ IDTokenIssuer: issuer, @@ -157,7 +151,7 @@ func FositeOauth2Helper( MinParameterEntropy: fosite.MinParameterEntropy, // do not allow custom scheme redirects, only https and http (on loopback) - RedirectSecureChecker: isRedirectURISecureStrict, + RedirectSecureChecker: fosite.IsRedirectURISecureStrict, // html template for rendering the authorization response when the request has response_mode=form_post FormPostHTMLTemplate: formposthtml.Template(), diff --git a/internal/federationdomain/storage/kube_storage.go b/internal/federationdomain/storage/kube_storage.go index e10ed60e3..3ffe1b0df 100644 --- a/internal/federationdomain/storage/kube_storage.go +++ b/internal/federationdomain/storage/kube_storage.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package storage @@ -107,10 +107,7 @@ func (k KubeStorage) DeletePKCERequestSession(ctx context.Context, signatureOfAu // Fosite will create these in the authorize endpoint when it creates an authcode, but only if the user // requested the openid scope. // -// Fosite will never delete these, which is likely a bug in fosite. Although there is a delete method below, fosite -// never calls it. Used during authcode redemption, they will never be accessed again after a successful authcode -// redemption. Although that implies that they should probably follow a lifecycle similar the the PKCE storage, they -// are, in fact, not deleted. +// Used during authcode redemption, and will be deleted during a successful authcode redemption. // func (k KubeStorage) CreateOpenIDConnectSession(ctx context.Context, fullAuthcode string, requester fosite.Requester) error { @@ -122,7 +119,7 @@ func (k KubeStorage) GetOpenIDConnectSession(ctx context.Context, fullAuthcode s } func (k KubeStorage) DeleteOpenIDConnectSession(ctx context.Context, fullAuthcode string) error { - return k.oidcStorage.DeleteOpenIDConnectSession(ctx, fullAuthcode) //nolint:staticcheck // we know this is deprecated and never called. our GC controller cleans these up. + return k.oidcStorage.DeleteOpenIDConnectSession(ctx, fullAuthcode) } // diff --git a/internal/federationdomain/timeouts/timeouts_configuration.go b/internal/federationdomain/timeouts/timeouts_configuration.go index 9657eaa49..cd55cfde6 100644 --- a/internal/federationdomain/timeouts/timeouts_configuration.go +++ b/internal/federationdomain/timeouts/timeouts_configuration.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package timeouts @@ -51,8 +51,8 @@ type Configuration struct { PKCESessionStorageLifetime time.Duration // OIDCSessionStorageLifetime is the length of time after which the OIDC session data related to an authcode - // is allowed to be garbage collected from storage. Due to a bug in an underlying library, these are not explicitly - // deleted. Similar to the PKCE session, they are not needed anymore after the corresponding authcode has expired. + // is allowed to be garbage collected from storage. After the authcode is successfully redeemed, the OIDC session is + // explicitly deleted. Similar to the PKCE session, they are not needed anymore after the corresponding authcode has expired. // Therefore, this can be just slightly longer than the AuthorizeCodeLifespan. We'll avoid making it exactly the same // as AuthorizeCodeLifespan to avoid any chance of the garbage collector deleting it while it is being used. OIDCSessionStorageLifetime time.Duration diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go index 0d4a59be6..663feef9d 100644 --- a/internal/fositestorage/openidconnect/openidconnect_test.go +++ b/internal/fositestorage/openidconnect/openidconnect_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package openidconnect @@ -100,7 +100,7 @@ func TestOpenIdConnectStorage(t *testing.T) { require.NoError(t, err) require.Equal(t, request, newRequest) - err = storage.DeleteOpenIDConnectSession(ctx, "fancy-code.fancy-signature") //nolint:staticcheck // we know this is deprecated and never called. our GC controller cleans these up. + err = storage.DeleteOpenIDConnectSession(ctx, "fancy-code.fancy-signature") require.NoError(t, err) testutil.LogActualJSONFromCreateAction(t, client, 0) // makes it easier to update expected values when needed diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index a9c09f12d..ef0e14550 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidctestutil @@ -1302,13 +1302,28 @@ func validateAuthcodeStorage( // Check which scopes were granted. require.ElementsMatch(t, wantDownstreamGrantedScopes, storedRequestFromAuthcode.GetGrantedScopes()) + // Don't care about the order of requested scopes, as long as they match the expected list. + storedRequestedScopes := storedRequestFromAuthcode.Form["scope"] + require.Len(t, storedRequestedScopes, 1) + require.NotEmpty(t, storedRequestedScopes[0]) + storedRequestedScopesSlice := strings.Split(storedRequestedScopes[0], " ") + require.ElementsMatch(t, storedRequestedScopesSlice, wantDownstreamRequestedScopes) + // Check all the other fields of the stored request. require.NotEmpty(t, storedRequestFromAuthcode.ID) require.Equal(t, wantDownstreamClientID, storedRequestFromAuthcode.Client.GetID()) require.ElementsMatch(t, wantDownstreamRequestedScopes, storedRequestFromAuthcode.RequestedScope) require.Nil(t, storedRequestFromAuthcode.RequestedAudience) require.Empty(t, storedRequestFromAuthcode.GrantedAudience) - require.Equal(t, url.Values{"redirect_uri": []string{wantDownstreamRedirectURI}}, storedRequestFromAuthcode.Form) + require.Equal(t, + url.Values{ + "client_id": []string{wantDownstreamClientID}, + "redirect_uri": []string{wantDownstreamRedirectURI}, + "response_type": []string{"code"}, + "scope": storedRequestedScopes, // already asserted about this actual value above + }, + storedRequestFromAuthcode.Form, + ) testutil.RequireTimeInDelta(t, time.Now(), storedRequestFromAuthcode.RequestedAt, timeComparisonFudgeFactor) // We're not using these fields yet, so confirm that we did not set them (for now).