login oidc cmd checks access token expiry before doing token exchange

In the RFC8693 token exchange, the CLI sends your access token and
receives in exchange a new cluster-scoped ID token.

Fix a bug in the CLI. Whenever the "pinniped login oidc" command was
planning to perform the RFC8693 token exchange, it failed to check if
the cached access token was still valid before performing the exchange,
which sends the access token. It instead checked if the cached ID token
was still valid, but that it not relevant in this situation because the
ID token is not going to be used for anything (instead the new ID token
returned by the RFC8693 token exchange will be used for auth).

This bug doesn't actually matter today, because the Supervisor-issued
access and ID tokens always both have the same 2-minute lifetimes.
However, future enhancements may cause them to have different lifetimes
in certain circumstances. Fixing this CLI bug now to prepare for those
potential future enhancements.

cmd/pinniped/cmd/login_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 cmd
@@ -241,12 +241,12 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
 	}
 
 	pLogger.Debug("Performing OIDC login", "issuer", flags.issuer, "client id", flags.clientID)
-	// Do the basic login to get an OIDC token.
+	// Do the basic login to get an OIDC token. Although this can return several tokens, we only need the ID token here.
 	token, err := deps.login(flags.issuer, flags.clientID, opts...)
 	if err != nil {
 		return fmt.Errorf("could not complete Pinniped login: %w", err)
 	}
-	cred := tokenCredential(token)
+	cred := tokenCredential(token.IDToken)
 
 	// If the concierge was configured, exchange the credential for a separate short-lived, cluster-specific credential.
 	if concierge != nil {
@@ -344,18 +344,18 @@ func makeClient(caBundlePaths []string, caBundleData []string) (*http.Client, er
 	return phttp.Default(pool), nil
 }
 
-func tokenCredential(token *oidctypes.Token) *clientauthv1beta1.ExecCredential {
+func tokenCredential(idToken *oidctypes.IDToken) *clientauthv1beta1.ExecCredential {
 	cred := clientauthv1beta1.ExecCredential{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "ExecCredential",
 			APIVersion: "client.authentication.k8s.io/v1beta1",
 		},
 		Status: &clientauthv1beta1.ExecCredentialStatus{
-			Token: token.IDToken.Token,
+			Token: idToken.Token,
 		},
 	}
-	if !token.IDToken.Expiry.IsZero() {
-		cred.Status.ExpirationTimestamp = &token.IDToken.Expiry
+	if !idToken.Expiry.IsZero() {
+		cred.Status.ExpirationTimestamp = &idToken.Expiry
 	}
 	return &cred
 }

cmd/pinniped/cmd/login_static.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 cmd
@@ -133,7 +133,7 @@ func runStaticLogin(cmd *cobra.Command, deps staticLoginDeps, flags staticLoginP
 			return fmt.Errorf("--token-env variable %q is empty", flags.staticTokenEnvName)
 		}
 	}
-	cred := tokenCredential(&oidctypes.Token{IDToken: &oidctypes.IDToken{Token: token}})
+	cred := tokenCredential(&oidctypes.IDToken{Token: token})
 
 	// Look up cached credentials based on a hash of all the CLI arguments, the current token value, and the cluster info.
 	cacheKey := struct {

internal/federationdomain/endpoints/auth/auth_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 auth
@@ -366,10 +366,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 	sadCSRFGenerator := func() (csrftoken.CSRFToken, error) { return "", fmt.Errorf("some csrf generator error") }
 	sadPKCEGenerator := func() (pkce.Code, error) { return "", fmt.Errorf("some PKCE generator error") }
 	sadNonceGenerator := func() (nonce.Nonce, error) { return "", fmt.Errorf("some nonce generator error") }
-
-	// This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example:
-	// $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1
-	expectedUpstreamCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"
+	expectedUpstreamCodeChallenge := testutil.SHA256("test-pkce")
 
 	var stateEncoderHashKey = []byte("fake-hash-secret")
 	var stateEncoderBlockKey = []byte("0123456789ABCDEF") // block encryption requires 16/24/32 bytes for AES

pkg/oidcclient/login.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 oidcclient implements a CLI OIDC login flow.
@@ -44,10 +44,16 @@ import (
 
 const (
 	// minIDTokenValidity is the minimum amount of time that a cached ID token must be still be valid to be considered.
-	// This is non-zero to ensure that most of the time, your ID token won't expire in the middle of a multi-step k8s
+	// This is non-zero to ensure that most of the time, your ID token won't expire in the middle of a multistep k8s
 	// API operation.
 	minIDTokenValidity = 10 * time.Minute
 
+	// minAccessTokenValidity is the minimum amount of time that a cached access token must be still be valid
+	// to be considered.
+	// This is non-zero to ensure that most of the time, your access token won't expire before we submit it for
+	// RFC8693 token exchange.
+	minAccessTokenValidity = 10 * time.Second
+
 	// httpRequestTimeout is the timeout for operations that involve one (or a few) non-interactive HTTPS requests.
 	// Since these don't involve any user interaction, they should always be roughly as fast as network latency.
 	httpRequestTimeout = 60 * time.Second
@@ -328,22 +334,53 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er
 	}
 
 	// Do the basic login to get an access and ID token issued to our main client ID.
-	baseToken, err := h.baseLogin()
+	token, err := h.baseLogin()
 	if err != nil {
 		return nil, err
 	}
 
-	// If there is no requested audience, or the requested audience matches the one we got, we're done.
-	if h.requestedAudience == "" || (baseToken.IDToken != nil && h.requestedAudience == baseToken.IDToken.Claims["aud"]) {
-		return baseToken, err
+	// Perform the RFC8693 token exchange, if needed. Note that the new ID token returned by this exchange
+	// does not need to be cached because the new ID token is intended to be a very short-lived token.
+	if h.needRFC8693TokenExchange(token) {
+		token, err = h.tokenExchangeRFC8693(token)
+		if err != nil {
+			return nil, fmt.Errorf("failed to exchange token: %w", err)
+		}
 	}
 
-	// Perform the RFC8693 token exchange.
-	exchangedToken, err := h.tokenExchangeRFC8693(baseToken)
-	if err != nil {
-		return nil, fmt.Errorf("failed to exchange token: %w", err)
+	return token, nil
+}
+
+func (h *handlerState) needRFC8693TokenExchange(token *oidctypes.Token) bool {
+	// Need a new ID token if there is a requested audience value and any of the following are true...
+	return h.requestedAudience != "" &&
+		// we don't have an ID token
+		(token.IDToken == nil ||
+			// or, our current ID token has expired or is close to expiring
+			idTokenExpiredOrCloseToExpiring(token.IDToken) ||
+			// or, our current ID token has a different audience
+			(h.requestedAudience != token.IDToken.Claims["aud"]))
+}
+
+func (h *handlerState) tokenValidForNearFuture(token *oidctypes.Token) (bool, string) {
+	if token == nil {
+		return false, ""
 	}
-	return exchangedToken, nil
+	// If we plan to do an RFC8693 token exchange, then we need an access token that will still be valid when we do the
+	// exchange (which will happen momentarily). Otherwise, we need an ID token that will be valid for a little while
+	// (long enough for multistep k8s API operations).
+	if h.needRFC8693TokenExchange(token) {
+		return !accessTokenExpiredOrCloseToExpiring(token.AccessToken), "access_token"
+	}
+	return !idTokenExpiredOrCloseToExpiring(token.IDToken), "id_token"
+}
+
+func accessTokenExpiredOrCloseToExpiring(accessToken *oidctypes.AccessToken) bool {
+	return accessToken == nil || time.Until(accessToken.Expiry.Time) <= minAccessTokenValidity
+}
+
+func idTokenExpiredOrCloseToExpiring(idToken *oidctypes.IDToken) bool {
+	return idToken == nil || time.Until(idToken.Expiry.Time) <= minIDTokenValidity
 }
 
 func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
@@ -360,10 +397,11 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		UpstreamProviderName: h.upstreamIdentityProviderName,
 	}
 
-	// If the ID token is still valid for a bit, return it immediately and skip the rest of the flow.
+	// If the cached tokens include the token type that we need, and that token is still valid for a bit,
+	// return the cached tokens immediately and skip the rest of the flow.
 	cached := h.cache.GetToken(cacheKey)
-	if cached != nil && cached.IDToken != nil && time.Until(cached.IDToken.Expiry.Time) > minIDTokenValidity {
-		h.logger.V(plog.KlogLevelDebug).Info("Pinniped: Found unexpired cached token.")
+	if valid, whichTokenWasValid := h.tokenValidForNearFuture(cached); valid {
+		h.logger.V(plog.KlogLevelDebug).Info("Pinniped: Found unexpired cached token.", "type", whichTokenWasValid)
 		return cached, nil
 	}
 
@@ -378,13 +416,14 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		if err != nil {
 			return nil, err
 		}
-		// If we got a fresh token, we can update the cache and return it. Otherwise we fall through to the full refresh flow.
+		// If we got a fresh token, update the cache and return it. Otherwise, fall through to the full login flow.
 		if freshToken != nil {
 			h.cache.PutToken(cacheKey, freshToken)
 			return freshToken, nil
 		}
 	}
 
+	// We couldn't refresh, so now we need to perform a fresh login attempt.
 	// Prepare the common options for the authorization URL. We don't have the redirect URL yet though.
 	authorizeOptions := []oauth2.AuthCodeOption{
 		oauth2.AccessTypeOffline,
@@ -833,7 +872,7 @@ func (h *handlerState) tokenExchangeRFC8693(baseToken *oidctypes.Token) (*oidcty
 }
 
 func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctypes.RefreshToken) (*oidctypes.Token, error) {
-	h.logger.V(plog.KlogLevelDebug).Info("Pinniped: Refreshing cached token.")
+	h.logger.V(plog.KlogLevelDebug).Info("Pinniped: Refreshing cached tokens.")
 	upstreamOIDCIdentityProvider := h.getProvider(h.oauth2Config, h.provider, h.httpClient)
 
 	refreshed, err := upstreamOIDCIdentityProvider.PerformRefresh(ctx, refreshToken.Token)