Consolidate the supervisor's timeout settings into a single struct

- This struct represents the configuration of all timeouts. These
  timeouts are all interrelated to declare them all in one place.
  This should also make it easier to allow the user to override
  our defaults if we would like to implement such a feature in the
  future.

Signed-off-by: Margo Crawford <margaretc@vmware.com>

internal/oidc/auth/auth_handler_test.go

@@ -127,7 +127,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 	hmacSecret := []byte("some secret - must have at least 32 bytes")
 	require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes")
 	jwksProviderIsUnused := jwks.NewDynamicJWKSProvider()
-	oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused)
+	oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration())
 
 	happyCSRF := "test-csrf"
 	happyPKCE := "test-pkce"

internal/oidc/callback/callback_handler_test.go

@@ -61,6 +61,7 @@ const (
 	downstreamPKCEChallenge       = "some-challenge"
 	downstreamPKCEChallengeMethod = "S256"
 
+	authCodeExpirationSeconds = 10 * 60 // Current, we set our auth code expiration to 10 minutes
 	timeComparisonFudgeFactor = time.Second * 15
 )
 
@@ -460,7 +461,7 @@ func TestCallbackEndpoint(t *testing.T) {
 			hmacSecret := []byte("some secret - must have at least 32 bytes")
 			require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes")
 			jwksProviderIsUnused := jwks.NewDynamicJWKSProvider()
-			oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused)
+			oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration())
 
 			idpListGetter := oidctestutil.NewIDPListGetter(&test.idp)
 			subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI)
@@ -768,7 +769,7 @@ func validateAuthcodeStorage(
 	require.Empty(t, storedSessionFromAuthcode.Headers)
 
 	// The authcode that we are issuing should be good for the length of time that we declare in the fosite config.
-	testutil.RequireTimeInDelta(t, time.Now().Add(time.Minute*3), storedSessionFromAuthcode.ExpiresAt[fosite.AuthorizeCode], timeComparisonFudgeFactor)
+	testutil.RequireTimeInDelta(t, time.Now().Add(authCodeExpirationSeconds*time.Second), storedSessionFromAuthcode.ExpiresAt[fosite.AuthorizeCode], timeComparisonFudgeFactor)
 	require.Len(t, storedSessionFromAuthcode.ExpiresAt, 1)
 
 	// Now confirm the ID token claims.

internal/oidc/oidc.go

@@ -91,30 +91,120 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient {
 	}
 }
 
+type TimeoutsConfiguration struct {
+	// The length of time that our state param that we encrypt and pass to the upstream OIDC IDP should be considered
+	// valid. If a state param generated by the authorize endpoint is sent to the callback endpoint after this much
+	// time has passed, then the callback endpoint should reject it. This allows us to set a limit on how long
+	// the end user has to finish their login with the upstream IDP, including the time that it takes to fumble
+	// with password manager and two-factor authenticator apps, and also accounting for taking a coffee break while
+	// the browser is sitting at the upstream IDP's login page.
+	UpstreamStateParamLifespan time.Duration
+
+	// How long an authcode issued by the callback endpoint is valid. This determines how much time the end user
+	// has to come back to exchange the authcode for tokens at the token endpoint.
+	AuthorizeCodeLifespan time.Duration
+
+	// The lifetime of an downstream access token issued by the token endpoint. Access tokens should generally
+	// be fairly short-lived.
+	AccessTokenLifespan time.Duration
+
+	// The lifetime of an downstream ID token issued by the token endpoint. This should generally be the same
+	// as the AccessTokenLifespan, or longer if it would be useful for the user's proof of identity to be valid
+	// for longer than their proof of authorization.
+	IDTokenLifespan time.Duration
+
+	// The lifetime of an downstream refresh token issued by the token endpoint. This should generally be
+	// significantly longer than the access token lifetime, so it can be used to refresh the access token
+	// multiple times. Once the refresh token expires, the user's session is over and they will need
+	// to start a new authorization request, which will require them to log in again with the upstream IDP
+	// in their web browser.
+	RefreshTokenLifespan time.Duration
+
+	// AuthorizationCodeSessionStorageLifetime is the length of time after which an authcode is allowed to be garbage
+	// collected from storage. Authcodes are kept in storage after they are redeemed to allow the system to mark the
+	// authcode as already used, so it can reject any future uses of the same authcode with special case handling which
+	// include revoking the access and refresh tokens associated with the session. Therefore, this should be
+	// significantly longer than the AuthorizeCodeLifespan, and there is probably no reason to make it longer than
+	// the sum of the AuthorizeCodeLifespan and the RefreshTokenLifespan.
+	AuthorizationCodeSessionStorageLifetime time.Duration
+
+	// PKCESessionStorageLifetime is the length of time after which PKCE data is allowed to be garbage collected from
+	// storage. PKCE sessions are closely related to authorization code sessions. After the authcode is successfully
+	// redeemed, the PKCE session is explicitly deleted. After the authcode expires, the PKCE session is no longer needed,
+	// but it is not explicitly deleted. 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.
+	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.
+	// 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
+
+	// AccessTokenSessionStorageLifetime is the length of time after which an access token's session data is allowed
+	// to be garbage collected from storage.  These must exist in storage for as long as the refresh token is valid.
+	// Therefore, this can be just slightly longer than the AccessTokenLifespan. Access tokens are handed back to
+	// the token endpoint for the token exchange use case. During a token exchange, if the access token is expired
+	// and still exists in storage, then the endpoint will be able to give a slightly more specific error message,
+	// rather than a more generic error that is returned when the token does not exist. If this is desirable, then
+	// the AccessTokenSessionStorageLifetime can be made to be significantly larger than AccessTokenLifespan, at the
+	// cost of slower cleanup.
+	AccessTokenSessionStorageLifetime time.Duration
+
+	// RefreshTokenSessionStorageLifetime is the length of time after which a refresh token's session data is allowed
+	// to be garbage collected from storage. These must exist in storage for as long as the refresh token is valid.
+	// Therefore, this can be just slightly longer than the RefreshTokenLifespan. We'll avoid making it exactly the same
+	// as RefreshTokenLifespan to avoid any chance of the garbage collector deleting it while it is being used.
+	// If an expired token is still stored when the user tries to refresh it, then they will get a more specific
+	// error message telling them that the token is expired, rather than a more generic error that is returned
+	// when the token does not exist. If this is desirable, then the RefreshTokenSessionStorageLifetime can be made
+	// to be significantly larger than RefreshTokenLifespan, at the cost of slower cleanup.
+	RefreshTokenSessionStorageLifetime time.Duration
+}
+
+// Get the defaults for the Supervisor server.
+func DefaultOIDCTimeoutsConfiguration() TimeoutsConfiguration {
+	accessTokenLifespan := 15 * time.Minute
+	authorizationCodeLifespan := 10 * time.Minute
+	refreshTokenLifespan := 9 * time.Hour
+
+	return TimeoutsConfiguration{
+		UpstreamStateParamLifespan:              90 * time.Minute,
+		AuthorizeCodeLifespan:                   authorizationCodeLifespan,
+		AccessTokenLifespan:                     accessTokenLifespan,
+		IDTokenLifespan:                         accessTokenLifespan,
+		RefreshTokenLifespan:                    refreshTokenLifespan,
+		AuthorizationCodeSessionStorageLifetime: authorizationCodeLifespan + refreshTokenLifespan,
+		PKCESessionStorageLifetime:              authorizationCodeLifespan + (1 * time.Minute),
+		OIDCSessionStorageLifetime:              authorizationCodeLifespan + (1 * time.Minute),
+		AccessTokenSessionStorageLifetime:       accessTokenLifespan + (1 * time.Minute),
+		RefreshTokenSessionStorageLifetime:      refreshTokenLifespan + (5 * time.Minute),
+	}
+}
+
 func FositeOauth2Helper(
 	oauthStore interface{},
 	issuer string,
 	hmacSecretOfLengthAtLeast32 []byte,
 	jwksProvider jwks.DynamicJWKSProvider,
+	timeoutsConfiguration TimeoutsConfiguration,
 ) fosite.OAuth2Provider {
 	oauthConfig := &compose.Config{
-		AuthorizeCodeLifespan: 3 * time.Minute, // seems more than long enough to exchange a code
-
-		IDTokenLifespan:     5 * time.Minute, // match clientCertificateTTL since it has similar properties to this token
-		AccessTokenLifespan: 5 * time.Minute, // match clientCertificateTTL since it has similar properties to this token
-
-		RefreshTokenLifespan: 16 * time.Hour, // long enough for a single workday
-
 		IDTokenIssuer: issuer,
 
-		ScopeStrategy:            fosite.ExactScopeStrategy, // be careful and only support exact string matching for scopes
+		AuthorizeCodeLifespan: timeoutsConfiguration.AuthorizeCodeLifespan,
-		AudienceMatchingStrategy: nil,                       // I believe the default is fine
+		IDTokenLifespan:       timeoutsConfiguration.IDTokenLifespan,
-		EnforcePKCE:              true,                      // follow current set of best practices and always require PKCE
+		AccessTokenLifespan:   timeoutsConfiguration.AccessTokenLifespan,
-		AllowedPromptValues:      []string{"none"},          // TODO unclear what we should set here
+		RefreshTokenLifespan:  timeoutsConfiguration.RefreshTokenLifespan,
 
-		RefreshTokenScopes: []string{coreosoidc.ScopeOfflineAccess}, // as per https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess
+		ScopeStrategy:            fosite.ExactScopeStrategy,
-
+		AudienceMatchingStrategy: nil,
-		MinParameterEntropy: 32, // 256 bits seems about right
+		EnforcePKCE:              true,
+		AllowedPromptValues:      []string{"none"},                        // TODO unclear what we should set here
+		RefreshTokenScopes:       []string{coreosoidc.ScopeOfflineAccess}, // as per https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess
+		MinParameterEntropy:      32,                                      // TODO is 256 bits too high?
 	}
 
 	return compose.Compose(

internal/oidc/provider/manager/manager.go

@@ -79,10 +79,10 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) {
 
 		// Use NullStorage for the authorize endpoint because we do not actually want to store anything until
 		// the upstream callback endpoint is called later.
-		oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil)
+		oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil, oidc.DefaultOIDCTimeoutsConfiguration())
 
 		// For all the other endpoints, make another oauth helper with exactly the same settings except use real storage.
-		oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider)
+		oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider, oidc.DefaultOIDCTimeoutsConfiguration())
 
 		// TODO use different codecs for the state and the cookie, because:
 		//  1. we would like to state to have an embedded expiration date while the cookie does not need that

internal/oidc/token/token_handler_test.go

@@ -58,9 +58,9 @@ const (
 
 	hmacSecret = "this needs to be at least 32 characters to meet entropy requirements"
 
-	authCodeExpirationSeconds    = 3 * 60 // Current, we set our auth code expiration to 3 minutes
+	authCodeExpirationSeconds    = 10 * 60 // Current, we set our auth code expiration to 10 minutes
-	accessTokenExpirationSeconds = 5 * 60 // Currently, we set our access token expiration to 5 minutes
+	accessTokenExpirationSeconds = 15 * 60 // Currently, we set our access token expiration to 15 minutes
-	idTokenExpirationSeconds     = 5 * 60 // Currently, we set our ID token expiration to 5 minutes
+	idTokenExpirationSeconds     = 15 * 60 // Currently, we set our ID token expiration to 15 minutes
 
 	timeComparisonFudgeSeconds = 15
 )
@@ -1346,7 +1346,7 @@ func makeHappyOauthHelper(
 	t.Helper()
 
 	jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer)
-	oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider)
+	oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration())
 	authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper)
 	return oauthHelper, authResponder.GetCode(), jwtSigningKey
 }
@@ -1378,7 +1378,7 @@ func makeOauthHelperWithJWTKeyThatWorksOnlyOnce(
 	t.Helper()
 
 	jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer)
-	oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider})
+	oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}, oidc.DefaultOIDCTimeoutsConfiguration())
 	authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper)
 	return oauthHelper, authResponder.GetCode(), jwtSigningKey
 }
@@ -1397,7 +1397,7 @@ func makeOauthHelperWithNilPrivateJWTSigningKey(
 	t.Helper()
 
 	jwkProvider := jwks.NewDynamicJWKSProvider() // empty provider which contains no signing key for this issuer
-	oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider)
+	oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration())
 	authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper)
 	return oauthHelper, authResponder.GetCode(), nil
 }

test/integration/supervisor_login_test.go

@@ -17,7 +17,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/coreos/go-oidc"
+	coreosoidc "github.com/coreos/go-oidc"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
@@ -25,6 +25,7 @@ import (
 	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
 	idpv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/idp/v1alpha1"
 	"go.pinniped.dev/internal/certauthority"
+	"go.pinniped.dev/internal/oidc"
 	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/pkg/oidcclient/nonce"
 	"go.pinniped.dev/pkg/oidcclient/pkce"
@@ -69,7 +70,7 @@ func TestSupervisorLogin(t *testing.T) {
 			return proxyURL, nil
 		},
 	}}
-	oidcHTTPClientContext := oidc.ClientContext(ctx, httpClient)
+	oidcHTTPClientContext := coreosoidc.ClientContext(ctx, httpClient)
 
 	// Use the CA to issue a TLS server cert.
 	t.Logf("issuing test certificate")
@@ -110,9 +111,9 @@ func TestSupervisorLogin(t *testing.T) {
 	}, idpv1alpha1.PhaseReady)
 
 	// Perform OIDC discovery for our downstream.
-	var discovery *oidc.Provider
+	var discovery *coreosoidc.Provider
 	assert.Eventually(t, func() bool {
-		discovery, err = oidc.NewProvider(oidcHTTPClientContext, downstream.Spec.Issuer)
+		discovery, err = coreosoidc.NewProvider(oidcHTTPClientContext, downstream.Spec.Issuer)
 		return err == nil
 	}, 30*time.Second, 200*time.Millisecond)
 	require.NoError(t, err)
@@ -193,7 +194,7 @@ func TestSupervisorLogin(t *testing.T) {
 func verifyTokenResponse(
 	t *testing.T,
 	tokenResponse *oauth2.Token,
-	discovery *oidc.Provider,
+	discovery *coreosoidc.Provider,
 	downstreamOAuth2Config oauth2.Config,
 	upstreamIssuerName string,
 	nonceParam nonce.Nonce,
@@ -205,7 +206,7 @@ func verifyTokenResponse(
 	// Verify the ID Token.
 	rawIDToken, ok := tokenResponse.Extra("id_token").(string)
 	require.True(t, ok, "expected to get an ID token but did not")
-	var verifier = discovery.Verifier(&oidc.Config{ClientID: downstreamOAuth2Config.ClientID})
+	var verifier = discovery.Verifier(&coreosoidc.Config{ClientID: downstreamOAuth2Config.ClientID})
 	idToken, err := verifier.Verify(ctx, rawIDToken)
 	require.NoError(t, err)
 
@@ -215,7 +216,8 @@ func verifyTokenResponse(
 	require.Greater(t, len(idToken.Subject), len(expectedSubjectPrefix),
 		"the ID token Subject should include the upstream user ID after the upstream issuer name")
 	require.NoError(t, nonceParam.Validate(idToken))
-	testutil.RequireTimeInDelta(t, time.Now().UTC().Add(time.Minute*5), idToken.Expiry, time.Second*30)
+	expectedIDTokenLifetime := oidc.DefaultOIDCTimeoutsConfiguration().IDTokenLifespan
+	testutil.RequireTimeInDelta(t, time.Now().UTC().Add(expectedIDTokenLifetime), idToken.Expiry, time.Second*30)
 	idTokenClaims := map[string]interface{}{}
 	err = idToken.Claims(&idTokenClaims)
 	require.NoError(t, err)
@@ -229,7 +231,8 @@ func verifyTokenResponse(
 	require.NotEmpty(t, tokenResponse.AccessToken)
 	require.Equal(t, "bearer", tokenResponse.TokenType)
 	require.NotZero(t, tokenResponse.Expiry)
-	testutil.RequireTimeInDelta(t, time.Now().UTC().Add(time.Minute*5), tokenResponse.Expiry, time.Second*30)
+	expectedAccessTokenLifetime := oidc.DefaultOIDCTimeoutsConfiguration().AccessTokenLifespan
+	testutil.RequireTimeInDelta(t, time.Now().UTC().Add(expectedAccessTokenLifetime), tokenResponse.Expiry, time.Second*30)
 
 	require.NotEmpty(t, tokenResponse.RefreshToken)
 }
@@ -262,7 +265,7 @@ func (s *localCallbackServer) waitForCallback(timeout time.Duration) *http.Reque
 	}
 }
 
-func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2.Token, httpClient *http.Client, provider *oidc.Provider) {
+func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2.Token, httpClient *http.Client, provider *coreosoidc.Provider) {
 	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
 	defer cancel()
 
@@ -289,7 +292,7 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2.
 	}
 	require.NoError(t, json.NewDecoder(resp.Body).Decode(&respBody))
 
-	var clusterVerifier = provider.Verifier(&oidc.Config{ClientID: "cluster-1234"})
+	var clusterVerifier = provider.Verifier(&coreosoidc.Config{ClientID: "cluster-1234"})
 	exchangedToken, err := clusterVerifier.Verify(ctx, respBody.AccessToken)
 	require.NoError(t, err)