diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index ea0d9290..1211f7f4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -666,6 +666,27 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantStatus: http.StatusBadRequest, wantResponseBodyContains: "missing audience parameter", }, + { + name: "bad requested audience when it looks like the name of an OIDCClient CR", + authcodeExchange: doValidAuthCodeExchange, + requestedAudience: "client.oauth.pinniped.dev-some-client-abc123", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: "requested audience cannot contain '.pinniped.dev'", + }, + { + name: "bad requested audience when it contains the substring .pinniped.dev because it is reserved for potential future usage", + authcodeExchange: doValidAuthCodeExchange, + requestedAudience: "something.pinniped.dev/some_aud", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: "requested audience cannot contain '.pinniped.dev'", + }, + { + name: "bad requested audience when it is the same name as the static public client pinniped-cli", + authcodeExchange: doValidAuthCodeExchange, + requestedAudience: "pinniped-cli", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: "requested audience cannot equal 'pinniped-cli'", + }, { name: "missing subject_token", authcodeExchange: doValidAuthCodeExchange, diff --git a/internal/oidc/token_exchange.go b/internal/oidc/token_exchange.go index d6dc2d29..a7a7812b 100644 --- a/internal/oidc/token_exchange.go +++ b/internal/oidc/token_exchange.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidc @@ -6,6 +6,7 @@ package oidc import ( "context" "net/url" + "strings" "github.com/coreos/go-oidc/v3/oidc" "github.com/ory/fosite" @@ -127,6 +128,24 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er } } + // Validate that the requested audience is not one of the reserved strings. All possible requested audience strings + // are subdivided into these classifications: + // 1. pinniped-cli is reserved for the statically defined OAuth client, which is disallowed for this token exchange. + // 2. client.oauth.pinniped.dev-* is reserved to be the names of user-defined dynamic OAuth clients, which is also + // disallowed for this token exchange. + // 3. Anything else matching *.pinniped.dev* is reserved for future use, in case we want to create more + // buckets of names some day, e.g. something.pinniped.dev/*. These names are also disallowed for this + // token exchange. + // 4. Any other string is reserved to conceptually mean the name of a workload cluster (technically, it's the + // configured audience of its Concierge JWTAuthenticator or other OIDC JWT validator). These are the only + // allowed values for this token exchange. + if strings.Contains(result.requestedAudience, ".pinniped.dev") { + return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot contain '.pinniped.dev'") + } + if result.requestedAudience == "pinniped-cli" { + return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot equal 'pinniped-cli'") + } + return &result, nil } diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 47587db7..b376e9e4 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -162,12 +162,14 @@ func TestSupervisorLogin_Browser(t *testing.T) { deleteTestUser func(t *testing.T, username string) requestAuthorization func(t *testing.T, downstreamIssuer, downstreamAuthorizeURL, downstreamCallbackURL, username, password string, httpClient *http.Client) createIDP func(t *testing.T) string + requestTokenExchangeAud string wantLocalhostCallbackToNeverHappen bool wantDownstreamIDTokenSubjectToMatch string wantDownstreamIDTokenUsernameToMatch func(username string) string wantDownstreamIDTokenGroups []string wantErrorDescription string wantErrorType string + wantTokenExchangeResponse func(t *testing.T, status int, body string) // Either revoke the user's session on the upstream provider, or manipulate the user's session // data in such a way that it should cause the next upstream refresh attempt to fail. @@ -1110,6 +1112,69 @@ func TestSupervisorLogin_Browser(t *testing.T) { }, wantDownstreamIDTokenGroups: env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs, }, + { + name: "disallowed requested audience using reserved substring on token exchange results in token exchange error", + maybeSkip: skipNever, + createIDP: func(t *testing.T) string { + return testlib.CreateTestOIDCIdentityProvider(t, basicOIDCIdentityProviderSpec(), idpv1alpha1.PhaseReady).Name + }, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, + requestTokenExchangeAud: "contains-disallowed-substring.pinniped.dev-something", // .pinniped.dev substring is not allowed + // the ID token Subject should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", + // the ID token Username should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+" }, + wantTokenExchangeResponse: func(t *testing.T, status int, body string) { + require.Equal(t, http.StatusBadRequest, status) + require.Equal(t, + `{"error":"invalid_request","error_description":"The request is missing a required parameter, `+ + `includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. `+ + `requested audience cannot contain '.pinniped.dev'"}`, + body) + }, + }, + { + name: "disallowed requested audience using specific reserved name of a dynamic client on token exchange results in token exchange error", + maybeSkip: skipNever, + createIDP: func(t *testing.T) string { + return testlib.CreateTestOIDCIdentityProvider(t, basicOIDCIdentityProviderSpec(), idpv1alpha1.PhaseReady).Name + }, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, + requestTokenExchangeAud: "client.oauth.pinniped.dev-client-name", // OIDC dynamic client name is not allowed + // the ID token Subject should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", + // the ID token Username should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+" }, + wantTokenExchangeResponse: func(t *testing.T, status int, body string) { + require.Equal(t, http.StatusBadRequest, status) + require.Equal(t, + `{"error":"invalid_request","error_description":"The request is missing a required parameter, `+ + `includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. `+ + `requested audience cannot contain '.pinniped.dev'"}`, + body) + }, + }, + { + name: "disallowed requested audience pinniped-cli on token exchange results in token exchange error", + maybeSkip: skipNever, + createIDP: func(t *testing.T) string { + return testlib.CreateTestOIDCIdentityProvider(t, basicOIDCIdentityProviderSpec(), idpv1alpha1.PhaseReady).Name + }, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, + requestTokenExchangeAud: "pinniped-cli", // pinniped-cli is not allowed + // the ID token Subject should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", + // the ID token Username should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+" }, + wantTokenExchangeResponse: func(t *testing.T, status int, body string) { + require.Equal(t, http.StatusBadRequest, status) + require.Equal(t, + `{"error":"invalid_request","error_description":"The request is missing a required parameter, `+ + `includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. `+ + `requested audience cannot equal 'pinniped-cli'"}`, + body) + }, + }, } for _, test := range tests { tt := test @@ -1123,12 +1188,14 @@ func TestSupervisorLogin_Browser(t *testing.T) { tt.breakRefreshSessionData, tt.createTestUser, tt.deleteTestUser, + tt.requestTokenExchangeAud, tt.wantLocalhostCallbackToNeverHappen, tt.wantDownstreamIDTokenSubjectToMatch, tt.wantDownstreamIDTokenUsernameToMatch, tt.wantDownstreamIDTokenGroups, tt.wantErrorDescription, tt.wantErrorType, + tt.wantTokenExchangeResponse, ) }) } @@ -1260,12 +1327,14 @@ func testSupervisorLogin( breakRefreshSessionData func(t *testing.T, pinnipedSession *psession.PinnipedSession, idpName, username string), createTestUser func(t *testing.T) (string, string), deleteTestUser func(t *testing.T, username string), + requestTokenExchangeAud string, wantLocalhostCallbackToNeverHappen bool, wantDownstreamIDTokenSubjectToMatch string, wantDownstreamIDTokenUsernameToMatch func(username string) string, wantDownstreamIDTokenGroups []string, wantErrorDescription string, wantErrorType string, + wantTokenExchangeResponse func(t *testing.T, status int, body string), ) { env := testlib.IntegrationEnv(t) @@ -1433,7 +1502,10 @@ func testSupervisorLogin( expectedIDTokenClaims, wantDownstreamIDTokenSubjectToMatch, wantDownstreamIDTokenUsernameToMatch(username), wantDownstreamIDTokenGroups) // token exchange on the original token - doTokenExchange(t, &downstreamOAuth2Config, tokenResponse, httpClient, discovery) + if requestTokenExchangeAud == "" { + requestTokenExchangeAud = "some-cluster-123" // use a default test value + } + doTokenExchange(t, requestTokenExchangeAud, &downstreamOAuth2Config, tokenResponse, httpClient, discovery, wantTokenExchangeResponse) refreshedGroups := wantDownstreamIDTokenGroups if editRefreshSessionDataWithoutBreaking != nil { @@ -1474,7 +1546,7 @@ func testSupervisorLogin( require.NotEqual(t, tokenResponse.Extra("id_token"), refreshedTokenResponse.Extra("id_token")) // token exchange on the refreshed token - doTokenExchange(t, &downstreamOAuth2Config, refreshedTokenResponse, httpClient, discovery) + doTokenExchange(t, requestTokenExchangeAud, &downstreamOAuth2Config, refreshedTokenResponse, httpClient, discovery, wantTokenExchangeResponse) // Now that we have successfully performed a refresh, let's test what happens when an // upstream refresh fails during the next downstream refresh. @@ -1763,14 +1835,22 @@ func (s *localCallbackServer) waitForCallback(timeout time.Duration) (*http.Requ } } -func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2.Token, httpClient *http.Client, provider *coreosoidc.Provider) { +func doTokenExchange( + t *testing.T, + requestTokenExchangeAud string, + config *oauth2.Config, + tokenResponse *oauth2.Token, + httpClient *http.Client, + provider *coreosoidc.Provider, + wantTokenExchangeResponse func(t *testing.T, status int, body string), +) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() // Form the HTTP POST request with the parameters specified by RFC8693. reqBody := strings.NewReader(url.Values{ "grant_type": []string{"urn:ietf:params:oauth:grant-type:token-exchange"}, - "audience": []string{"cluster-1234"}, + "audience": []string{requestTokenExchangeAud}, "client_id": []string{config.ClientID}, "subject_token": []string{tokenResponse.AccessToken}, "subject_token_type": []string{"urn:ietf:params:oauth:token-type:access_token"}, @@ -1782,8 +1862,19 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2. resp, err := httpClient.Do(req) require.NoError(t, err) - require.Equal(t, resp.StatusCode, http.StatusOK) defer func() { _ = resp.Body.Close() }() + + // If a function was passed, call it, so it can make the desired assertions. + if wantTokenExchangeResponse != nil { + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + wantTokenExchangeResponse(t, resp.StatusCode, string(body)) + return // the above call should have made all desired assertions about the response, so return + } + + // Else, want a successful response. + require.Equal(t, resp.StatusCode, http.StatusOK) + var respBody struct { AccessToken string `json:"access_token"` IssuedTokenType string `json:"issued_token_type"` @@ -1791,7 +1882,7 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2. } require.NoError(t, json.NewDecoder(resp.Body).Decode(&respBody)) - var clusterVerifier = provider.Verifier(&coreosoidc.Config{ClientID: "cluster-1234"}) + var clusterVerifier = provider.Verifier(&coreosoidc.Config{ClientID: requestTokenExchangeAud}) exchangedToken, err := clusterVerifier.Verify(ctx, respBody.AccessToken) require.NoError(t, err)