From dce9409ccccda0fa2c7e8a79c3299d10fceef33b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 9 Feb 2024 11:35:02 -0800 Subject: [PATCH] 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 | 14 +- cmd/pinniped/cmd/login_static.go | 4 +- .../endpoints/auth/auth_handler_test.go | 7 +- pkg/oidcclient/login.go | 71 ++++- pkg/oidcclient/login_test.go | 289 +++++++++++++----- test/integration/supervisor_warnings_test.go | 51 +++- 6 files changed, 325 insertions(+), 111 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index ac76cb4f8..de46034c5 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/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 } diff --git a/cmd/pinniped/cmd/login_static.go b/cmd/pinniped/cmd/login_static.go index 670502ddf..283b4a0fa 100644 --- a/cmd/pinniped/cmd/login_static.go +++ b/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 { diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go index ffa11934d..dc847169d 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler_test.go +++ b/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 diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 03fbe5d82..6aae7cc9c 100644 --- a/pkg/oidcclient/login.go +++ b/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) diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index f49d444fb..f08298f03 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -75,23 +75,18 @@ func newClientForServer(server *httptest.Server) *http.Client { } func TestLogin(t *testing.T) { //nolint:gocyclo - time1 := time.Date(2035, 10, 12, 13, 14, 15, 16, time.UTC) - time1Unix := int64(2075807775) - require.Equal(t, time1Unix, time1.Add(2*time.Minute).Unix()) + distantFutureTime := time.Date(2065, 10, 12, 13, 14, 15, 16, time.UTC) - // 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 - // VVaezYqum7reIhoavCHD1n2d+piN3r/mywoYj7fCR7g - const testCodeChallenge = "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" + testCodeChallenge := testutil.SHA256("test-pkce") testToken := oidctypes.Token{ - AccessToken: &oidctypes.AccessToken{Token: "test-access-token", Expiry: metav1.NewTime(time1.Add(1 * time.Minute))}, + AccessToken: &oidctypes.AccessToken{Token: "test-access-token", Expiry: metav1.NewTime(distantFutureTime.Add(1 * time.Minute))}, RefreshToken: &oidctypes.RefreshToken{Token: "test-refresh-token"}, - IDToken: &oidctypes.IDToken{Token: "test-id-token", Expiry: metav1.NewTime(time1.Add(2 * time.Minute))}, + IDToken: &oidctypes.IDToken{Token: "test-id-token", Expiry: metav1.NewTime(distantFutureTime.Add(2 * time.Minute))}, } testExchangedToken := oidctypes.Token{ - IDToken: &oidctypes.IDToken{Token: "test-id-token-with-requested-audience", Expiry: metav1.NewTime(time1.Add(3 * time.Minute))}, + IDToken: &oidctypes.IDToken{Token: "test-id-token-with-requested-audience", Expiry: metav1.NewTime(distantFutureTime.Add(3 * time.Minute))}, } // Start a test server that returns 500 errors. @@ -216,6 +211,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }) } } + tokenHandler := func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { http.Error(w, "unexpected method", http.StatusMethodNotAllowed) @@ -434,7 +430,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantErr: `issuer is not a valid URL: parse "%": invalid URL escape "%"`, }, { - name: "session cache hit but token expired", + name: "without request audience, session cache hit but ID token expired", issuer: errorServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -443,7 +439,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ IDToken: &oidctypes.IDToken{ Token: "test-id-token", - Expiry: metav1.NewTime(time.Now()), // less than Now() + minIDTokenValidity + Expiry: metav1.NewTime(time.Now().Add(9 * time.Minute)), // less than Now() + minIDTokenValidity }, }} t.Cleanup(func() { @@ -459,11 +455,10 @@ func TestLogin(t *testing.T) { //nolint:gocyclo } }, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + errorServer.URL + `"`}, - wantErr: `could not perform OIDC discovery for "` + errorServer.URL + `": 500 Internal Server Error: some discovery error -`, + wantErr: `could not perform OIDC discovery for "` + errorServer.URL + `": 500 Internal Server Error: some discovery error` + "\n", }, { - name: "session cache hit with valid token", + name: "without request audience, session cache hit with valid ID token", issuer: "test-issuer", clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -478,10 +473,11 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }}, cache.sawGetKeys) require.Empty(t, cache.sawPutTokens) }) - return WithSessionCache(cache)(h) + require.NoError(t, WithSessionCache(cache)(h)) + return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`}, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="id_token"`}, wantToken: &testToken, }, { @@ -509,7 +505,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantErr: fmt.Sprintf("could not decode response_modes_supported in OIDC discovery from %q: json: cannot unmarshal string into Go struct field .response_modes_supported of type []string", brokenResponseModeServer.URL), }, { - name: "session cache hit with refreshable token", + name: "without request audience, session cache hit with expired ID token which is refreshable", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -533,7 +529,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ IDToken: &oidctypes.IDToken{ Token: "expired-test-id-token", - Expiry: metav1.Now(), // less than Now() + minIDTokenValidity + Expiry: metav1.NewTime(time.Now().Add(9 * time.Minute)), // less than Now() + minIDTokenValidity }, RefreshToken: &oidctypes.RefreshToken{Token: "test-refresh-token"}, }} @@ -553,8 +549,10 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, - `"level"=4 "msg"="Pinniped: Refreshing cached token."`}, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + `"level"=4 "msg"="Pinniped: Refreshing cached tokens."`, + }, wantToken: &testToken, }, { @@ -582,7 +580,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ IDToken: &oidctypes.IDToken{ Token: "expired-test-id-token", - Expiry: metav1.Now(), // less than Now() + minIDTokenValidity + Expiry: metav1.NewTime(time.Now().Add(9 * time.Minute)), // less than Now() + minIDTokenValidity }, RefreshToken: &oidctypes.RefreshToken{Token: "test-refresh-token-returning-invalid-id-token"}, }} @@ -591,12 +589,13 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.Empty(t, cache.sawPutTokens) }) h.cache = cache - return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, - `"level"=4 "msg"="Pinniped: Refreshing cached token."`}, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + `"level"=4 "msg"="Pinniped: Refreshing cached tokens."`, + }, wantErr: "some validation error", }, { @@ -610,7 +609,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ IDToken: &oidctypes.IDToken{ Token: "expired-test-id-token", - Expiry: metav1.Now(), // less than Now() + minIDTokenValidity + Expiry: metav1.NewTime(time.Now().Add(9 * time.Minute)), // less than Now() + minIDTokenValidity }, RefreshToken: &oidctypes.RefreshToken{Token: "test-refresh-token"}, }} @@ -627,7 +626,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, wantLogs: []string{ `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, - `"level"=4 "msg"="Pinniped: Refreshing cached token."`, + `"level"=4 "msg"="Pinniped: Refreshing cached tokens."`, `"level"=4 "msg"="Pinniped: Refresh failed." "error"="oauth2: cannot fetch token: 400 Bad Request\nResponse: expected client_id 'test-client-id'\n"`, `"msg"="could not open callback listener" "error"="some listen error"`, }, @@ -1136,7 +1135,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo } }) require.NoError(t, WithClient(client)(h)) - return nil } }, @@ -1587,7 +1585,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantToken: &testToken, }, { - name: "with requested audience, session cache hit with valid token, but discovery fails", + name: "with requested audience, session cache hit with valid access token, but discovery fails", clientID: "test-client-id", issuer: errorServer.URL, opt: func(t *testing.T) Option { @@ -1608,13 +1606,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="cluster-1234"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + errorServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + errorServer.URL + `"`, + }, wantErr: fmt.Sprintf("failed to exchange token: could not perform OIDC discovery for %q: 500 Internal Server Error: some discovery error\n", errorServer.URL), }, { - name: "with requested audience, session cache hit with valid token, but token URL is insecure", + name: "with requested audience, session cache hit with valid access token, but token URL is insecure", issuer: insecureTokenURLServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1635,13 +1635,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="cluster-1234"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + insecureTokenURLServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + insecureTokenURLServer.URL + `"`, + }, wantErr: `failed to exchange token: discovered token URL from issuer must be an https URL, but had scheme "http" instead`, }, { - name: "with requested audience, session cache hit with valid token, but token URL is invalid", + name: "with requested audience, session cache hit with valid access token, but token URL is invalid", issuer: brokenTokenURLServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1662,13 +1664,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="cluster-1234"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + brokenTokenURLServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + brokenTokenURLServer.URL + `"`, + }, wantErr: `failed to exchange token: discovered token URL from issuer is not a valid URL: parse "%": invalid URL escape "%"`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request fails", + name: "with requested audience, session cache hit with valid access token, but token exchange request fails", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1689,13 +1693,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-invalid-http-response"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: fmt.Sprintf(`failed to exchange token: Post "%s/token": failed to parse Location header "%%": parse "%%": invalid URL escape "%%"`, successServer.URL), }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns non-200", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns non-200", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1716,13 +1722,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-http-400"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: unexpected HTTP response status 400`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns invalid content-type header", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns invalid content-type header", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1743,13 +1751,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-invalid-content-type"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: failed to decode content-type header: mime: invalid media parameter`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns wrong content-type", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns wrong content-type", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1770,13 +1780,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-wrong-content-type"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: unexpected HTTP response content type "invalid"`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns invalid JSON", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns invalid JSON", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1797,13 +1809,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-invalid-json"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: failed to decode response: unexpected EOF`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns invalid token_type", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns invalid token_type", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1824,13 +1838,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-invalid-tokentype"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: got unexpected token_type "invalid"`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns invalid issued_token_type", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns invalid issued_token_type", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1851,13 +1867,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-invalid-issuedtokentype"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: got unexpected issued_token_type "invalid"`, }, { - name: "with requested audience, session cache hit with valid token, but token exchange request returns invalid JWT", + name: "with requested audience, session cache hit with valid access token, but token exchange request returns invalid JWT", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1878,13 +1896,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience-produce-invalid-jwt"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantErr: `failed to exchange token: received invalid JWT: oidc: malformed jwt: oidc: malformed jwt, expected 3 parts got 1`, }, { - name: "with requested audience, session cache hit with valid token, and token exchange request succeeds", + name: "with requested audience, session cache hit with valid access token, ID token has wrong audience, and token exchange request succeeds", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1911,23 +1931,156 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil } }, - wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token."`, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience"`, - `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, wantToken: &testExchangedToken, }, { - name: "with requested audience, session cache hit with valid refresh token, and token exchange request succeeds", + name: "with requested audience, session cache hit with valid access token, and valid ID token already has the requested audience, returns cached tokens without any exchange or refresh", issuer: successServer.URL, clientID: "test-client-id", opt: func(t *testing.T) Option { return func(h *handlerState) error { + cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ + AccessToken: testToken.AccessToken, + IDToken: &oidctypes.IDToken{ + Token: testToken.IDToken.Token, + Expiry: testToken.IDToken.Expiry, + Claims: map[string]interface{}{"aud": "request-this-test-audience"}, + }, + RefreshToken: testToken.RefreshToken, + }} + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + }}, cache.sawGetKeys) + require.Empty(t, cache.sawPutTokens) + }) require.NoError(t, WithClient(newClientForServer(successServer))(h)) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithRequestAudience("request-this-test-audience")(h)) + h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + require.FailNow(t, "should not have performed a token exchange because the cached ID token already had the requested audience") + return nil, nil + } + return nil + } + }, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="id_token"`}, + wantToken: &oidctypes.Token{ // the same tokens that were pulled from from the cache + AccessToken: testToken.AccessToken, + IDToken: &oidctypes.IDToken{ + Token: testToken.IDToken.Token, + Expiry: testToken.IDToken.Expiry, + Claims: map[string]interface{}{"aud": "request-this-test-audience"}, + }, + RefreshToken: testToken.RefreshToken, + }, + }, + { + name: "with requested audience, session cache hit with valid access token, ID token already has the requested audience, but ID token is expired", + issuer: successServer.URL, + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ + AccessToken: testToken.AccessToken, IDToken: &oidctypes.IDToken{ - Token: "expired-test-id-token", - Expiry: metav1.Now(), // less than Now() + minIDTokenValidity + Token: testToken.IDToken.Token, + Expiry: metav1.NewTime(time.Now().Add(9 * time.Minute)), // less than Now() + minIDTokenValidity + Claims: map[string]interface{}{"aud": "request-this-test-audience"}, + }, + RefreshToken: testToken.RefreshToken, + }} + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + }}, cache.sawGetKeys) + require.Empty(t, cache.sawPutTokens) + }) + require.NoError(t, WithClient(newClientForServer(successServer))(h)) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithRequestAudience("request-this-test-audience")(h)) + + h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + require.Equal(t, "request-this-test-audience", audience) + require.Equal(t, "test-id-token-with-requested-audience", token) + return &oidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil + } + return nil + } + }, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, + `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="request-this-test-audience"`, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, + wantToken: &testExchangedToken, + }, + { + name: "with requested audience, session cache hit with valid access token, but no ID token", + issuer: successServer.URL, + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ + AccessToken: testToken.AccessToken, + RefreshToken: testToken.RefreshToken, + }} + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + }}, cache.sawGetKeys) + require.Empty(t, cache.sawPutTokens) + }) + require.NoError(t, WithClient(newClientForServer(successServer))(h)) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithRequestAudience("request-this-test-audience")(h)) + + h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + require.Equal(t, "request-this-test-audience", audience) + require.Equal(t, "test-id-token-with-requested-audience", token) + return &oidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil + } + return nil + } + }, + wantLogs: []string{ + `"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="access_token"`, + `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="request-this-test-audience"`, + `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, + }, + wantToken: &testExchangedToken, + }, + { + name: "with requested audience, session cache hit with expired access token and valid refresh token, and token exchange request succeeds", + issuer: successServer.URL, + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + require.NoError(t, WithClient(newClientForServer(successServer))(h)) + + cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Token: "not-yet-expired-test-id-token", + Expiry: metav1.NewTime(distantFutureTime), + }, + AccessToken: &oidctypes.AccessToken{ + Token: "expired-test-access-token", + Expiry: metav1.NewTime(time.Now().Add(9 * time.Second)), // less than Now() + minAccessTokenValidity }, RefreshToken: &oidctypes.RefreshToken{Token: "test-refresh-token"}, }} @@ -1971,7 +2124,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, wantLogs: []string{ `"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`, - `"level"=4 "msg"="Pinniped: Refreshing cached token."`, + `"level"=4 "msg"="Pinniped: Refreshing cached tokens."`, `"level"=4 "msg"="Pinniped: Performing RFC8693 token exchange" "requestedAudience"="test-audience"`, }, wantToken: &testExchangedToken, diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go index 3327bd4ad..bf133668b 100644 --- a/test/integration/supervisor_warnings_test.go +++ b/test/integration/supervisor_warnings_test.go @@ -1,4 +1,4 @@ -// Copyright 2022-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -209,12 +209,14 @@ func TestSupervisorWarnings_Browser(t *testing.T) { err = os.Remove(credentialCachePath) require.NoError(t, err) - // wait for the existing tokens to expire, triggering the refresh flow. - ctx2, cancel2 := context.WithTimeout(ctx, 1*time.Minute) - defer cancel2() + // The cached access token is valid for 2 minutes, and can be used to perform the token exchange again + // during that time. Remove it to force the refresh flow to happen on the next kubectl invocation, + // without needing to wait for the access token to expire first. + token.AccessToken = nil + cache.PutToken(sessionCacheKey, token) // Run kubectl, which should work without any prompting for authentication. - kubectlCmd2 := exec.CommandContext(ctx2, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) + kubectlCmd2 := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd2.Env = append(os.Environ(), env.ProxyEnv()...) startTime2 := time.Now() var kubectlStdoutPipe2 io.ReadCloser @@ -253,7 +255,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { expectedUsername, password := testlib.CreateFreshADTestUser(t, env) sAMAccountName := expectedUsername + "@" + env.SupervisorUpstreamActiveDirectory.Domain - setupClusterForEndToEndActiveDirectoryTest(t, sAMAccountName, env) + createdProvider := setupClusterForEndToEndActiveDirectoryTest(t, sAMAccountName, env) testlib.WaitForFederationDomainStatusPhase(ctx, t, downstream.Name, configv1alpha1.FederationDomainPhaseReady) // Use a specific session cache for this test. @@ -309,6 +311,24 @@ func TestSupervisorWarnings_Browser(t *testing.T) { t.Logf("first kubectl command took %s", time.Since(start).String()) + cache := filesession.New(sessionCachePath, filesession.WithErrorReporter(func(err error) { + require.NoError(t, err) + })) + + // construct the cache key + downstreamScopes := []string{"offline_access", "openid", "pinniped:request-audience", "groups"} + sort.Strings(downstreamScopes) + sessionCacheKey := oidcclient.SessionCacheKey{ + Issuer: downstream.Spec.Issuer, + ClientID: "pinniped-cli", + Scopes: downstreamScopes, + RedirectURI: "http://localhost:0/callback", + UpstreamProviderName: createdProvider.Name, + } + // use it to get the cache entry + token := cache.GetToken(sessionCacheKey) + require.NotNil(t, token) + // create an active directory group, and add our user to it. groupName := testlib.CreateFreshADTestGroup(t, env) testlib.AddTestUserToGroup(t, env, groupName, expectedUsername) @@ -317,11 +337,14 @@ func TestSupervisorWarnings_Browser(t *testing.T) { err = os.Remove(credentialCachePath) require.NoError(t, err) - ctx2, cancel2 := context.WithTimeout(ctx, 1*time.Minute) - defer cancel2() + // The cached access token is valid for 2 minutes, and can be used to perform the token exchange again + // during that time. Remove it to force the refresh flow to happen on the next kubectl invocation, + // without needing to wait for the access token to expire first. + token.AccessToken = nil + cache.PutToken(sessionCacheKey, token) // Run kubectl, which should work without any prompting for authentication. - kubectlCmd2 := exec.CommandContext(ctx2, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) + kubectlCmd2 := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd2.Env = append(os.Environ(), env.ProxyEnv()...) startTime2 := time.Now() var kubectlStdoutPipe2 io.ReadCloser @@ -520,12 +543,14 @@ func TestSupervisorWarnings_Browser(t *testing.T) { err = os.Remove(credentialCachePath) require.NoError(t, err) - // wait for the existing tokens to expire, triggering the refresh flow. - ctx2, cancel2 := context.WithTimeout(ctx, 1*time.Minute) - defer cancel2() + // The cached access token is valid for 2 minutes, and can be used to perform the token exchange again + // during that time. Remove it to force the refresh flow to happen on the next kubectl invocation, + // without needing to wait for the access token to expire first. + token.AccessToken = nil + cache.PutToken(sessionCacheKey, token) // Run kubectl, which should work without any prompting for authentication. - kubectlCmd2 := exec.CommandContext(ctx2, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) + kubectlCmd2 := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd2.Env = append(os.Environ(), env.ProxyEnv()...) startTime2 := time.Now() var kubectlStdoutPipe2 io.ReadCloser