Merge pull request #1866 from vmware-tanzu/upgrade_fosite_feb_2024

Upgrade fosite to latest version

Dockerfile

@@ -3,8 +3,8 @@
 # Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 # SPDX-License-Identifier: Apache-2.0
 
-ARG BUILD_IMAGE=golang:1.22.0@sha256:ef61a20960397f4d44b0e729298bf02327ca94f1519239ddc6d91689615b1367
-ARG BASE_IMAGE=gcr.io/distroless/static:nonroot@sha256:112a87f19e83c83711cc81ce8ed0b4d79acd65789682a6a272df57c4a0858534
+ARG BUILD_IMAGE=golang:1.22.0@sha256:4a3e85e88ca4edb571679a3e8b86aaef16ad65134d3aba68760741a850d69f41
+ARG BASE_IMAGE=gcr.io/distroless/static:nonroot@sha256:6a3500b086c2856fbc189f5d11351bdbcf7c4dc5673c2b6070aac9d607da90d7
 
 # Prepare to cross-compile by always running the build stage in the build platform, not the target platform.
 FROM --platform=$BUILDPLATFORM $BUILD_IMAGE as build-env

go.mod

@@ -46,7 +46,7 @@ require (
 	github.com/gorilla/websocket v1.5.1
 	github.com/joshlf/go-acl v0.0.0-20200411065538-eae00ae38531
 	github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826
-	github.com/ory/fosite v0.45.1-0.20240103162202-f4114878826c
+	github.com/ory/fosite v0.46.1-0.20240213131620-c16a2d159d6d
 	github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c
 	github.com/pkg/errors v0.9.1
 	github.com/sclevine/spec v1.4.0

go.sum

@@ -458,8 +458,8 @@ github.com/openzipkin/zipkin-go v0.4.1 h1:kNd/ST2yLLWhaWrkgchya40TJabe8Hioj9udfP
 github.com/openzipkin/zipkin-go v0.4.1/go.mod h1:qY0VqDSN1pOBN94dBc6w2GJlWLiovAyg7Qt6/I9HecM=
 github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde h1:x0TT0RDC7UhAVbbWWBzr41ElhJx5tXPWkIHA2HWPRuw=
 github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde/go.mod h1:nZgzbfBr3hhjoZnS66nKrHmduYNpc34ny7RK4z5/HM0=
-github.com/ory/fosite v0.45.1-0.20240103162202-f4114878826c h1:l2O0QFfznFYWpCSc8rsNosv9qb6eYBCdK5uFLQIDmQc=
-github.com/ory/fosite v0.45.1-0.20240103162202-f4114878826c/go.mod h1:fkMPsnm/UjiefE9dE9CdZQGOH48TWJLIzUcdGIXg8Kk=
+github.com/ory/fosite v0.46.1-0.20240213131620-c16a2d159d6d h1:Kw5dYrxKmY28QOds1q9gJdPEdMOcQQQt/RG/4Yb6vXA=
+github.com/ory/fosite v0.46.1-0.20240213131620-c16a2d159d6d/go.mod h1:fkMPsnm/UjiefE9dE9CdZQGOH48TWJLIzUcdGIXg8Kk=
 github.com/ory/go-acc v0.2.9-0.20230103102148-6b1c9a70dbbe h1:rvu4obdvqR0fkSIJ8IfgzKOWwZ5kOT2UNfLq81Qk7rc=
 github.com/ory/go-acc v0.2.9-0.20230103102148-6b1c9a70dbbe/go.mod h1:z4n3u6as84LbV4YmgjHhnwtccQqzf4cZlSk9f1FhygI=
 github.com/ory/go-convenience v0.1.0 h1:zouLKfF2GoSGnJwGq+PE/nJAE6dj2Zj5QlTgmMTsTS8=

hack/Dockerfile_fips

@@ -16,8 +16,8 @@
 # See https://go.googlesource.com/go/+/dev.boringcrypto/README.boringcrypto.md
 # and https://kupczynski.info/posts/fips-golang/ for details.
 
-ARG BUILD_IMAGE=golang:1.22.0@sha256:ef61a20960397f4d44b0e729298bf02327ca94f1519239ddc6d91689615b1367
-ARG BASE_IMAGE=gcr.io/distroless/static:nonroot@sha256:112a87f19e83c83711cc81ce8ed0b4d79acd65789682a6a272df57c4a0858534
+ARG BUILD_IMAGE=golang:1.22.0@sha256:4a3e85e88ca4edb571679a3e8b86aaef16ad65134d3aba68760741a850d69f41
+ARG BASE_IMAGE=gcr.io/distroless/static:nonroot@sha256:6a3500b086c2856fbc189f5d11351bdbcf7c4dc5673c2b6070aac9d607da90d7
 
 # This is not currently using --platform to prepare to cross-compile because we use gcc below to build
 # platform-specific GCO code. This makes multi-arch builds slow due to target platform emulation.

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
 

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(

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 {

internal/federationdomain/oidc/oidc.go

@@ -1,16 +1,14 @@
-// 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
 // downstream OIDC functionality.
 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(),

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)
 }
 
 //

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

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