From ea45e5dfef0f3c2224be67c3a9740d745690f96d Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 7 Jun 2022 16:32:19 -0700 Subject: [PATCH 1/2] Disallow certain requested audience strings in token exchange --- internal/oidc/token/token_handler_test.go | 21 ++++++ internal/oidc/token_exchange.go | 21 +++++- test/integration/supervisor_login_test.go | 80 +++++++++++++++++++++-- 3 files changed, 116 insertions(+), 6 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index ea0d9290..e3d70952 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 '.oauth.pinniped.dev'", + }, + { + name: "bad requested audience when it contains the substring .oauth.pinniped.dev because it is reserved for potential future usage", + authcodeExchange: doValidAuthCodeExchange, + requestedAudience: "something.oauth.pinniped.dev/some_aud", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: "requested audience cannot contain '.oauth.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..94c37c74 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. clients.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 *.oauth.pinniped.dev* is reserved for future use, in case we want to create more + // buckets of names some day, e.g. something.oauth.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, ".oauth.pinniped.dev") { + return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot contain '.oauth.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 3713175a..6ccae9a3 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -167,12 +167,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. @@ -1115,6 +1117,48 @@ 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.oauth.pinniped.dev-something", // .oauth.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 '.oauth.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 @@ -1128,12 +1172,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, ) }) } @@ -1265,12 +1311,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) @@ -1438,7 +1486,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 { @@ -1479,7 +1530,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. @@ -1768,14 +1819,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"}, @@ -1787,7 +1846,18 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2. resp, err := httpClient.Do(req) require.NoError(t, err) + + // 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) + defer func() { _ = resp.Body.Close() }() var respBody struct { AccessToken string `json:"access_token"` @@ -1796,7 +1866,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) From b9272b27298e5b11be63a6926a21277c4710ef4b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 13 Jun 2022 12:08:11 -0700 Subject: [PATCH 2/2] Reserve all of *.pinniped.dev for requested aud in token exchanges Our previous plan was to reserve only *.oauth.pinniped.dev but we changed our minds during PR review. --- internal/oidc/token/token_handler_test.go | 8 +++---- internal/oidc/token_exchange.go | 10 ++++----- test/integration/supervisor_login_test.go | 27 ++++++++++++++++++++--- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index e3d70952..1211f7f4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -671,14 +671,14 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn authcodeExchange: doValidAuthCodeExchange, requestedAudience: "client.oauth.pinniped.dev-some-client-abc123", wantStatus: http.StatusBadRequest, - wantResponseBodyContains: "requested audience cannot contain '.oauth.pinniped.dev'", + wantResponseBodyContains: "requested audience cannot contain '.pinniped.dev'", }, { - name: "bad requested audience when it contains the substring .oauth.pinniped.dev because it is reserved for potential future usage", + name: "bad requested audience when it contains the substring .pinniped.dev because it is reserved for potential future usage", authcodeExchange: doValidAuthCodeExchange, - requestedAudience: "something.oauth.pinniped.dev/some_aud", + requestedAudience: "something.pinniped.dev/some_aud", wantStatus: http.StatusBadRequest, - wantResponseBodyContains: "requested audience cannot contain '.oauth.pinniped.dev'", + wantResponseBodyContains: "requested audience cannot contain '.pinniped.dev'", }, { name: "bad requested audience when it is the same name as the static public client pinniped-cli", diff --git a/internal/oidc/token_exchange.go b/internal/oidc/token_exchange.go index 94c37c74..a7a7812b 100644 --- a/internal/oidc/token_exchange.go +++ b/internal/oidc/token_exchange.go @@ -131,16 +131,16 @@ 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. clients.oauth.pinniped.dev-* is reserved to be the names of user-defined dynamic OAuth clients, which is also + // 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 *.oauth.pinniped.dev* is reserved for future use, in case we want to create more - // buckets of names some day, e.g. something.oauth.pinniped.dev/*. These names are also disallowed for this + // 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, ".oauth.pinniped.dev") { - return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot contain '.oauth.pinniped.dev'") + 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'") diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 16f33d98..d584eab0 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1124,7 +1124,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { return testlib.CreateTestOIDCIdentityProvider(t, basicOIDCIdentityProviderSpec(), idpv1alpha1.PhaseReady).Name }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, - requestTokenExchangeAud: "contains-disallowed-substring.oauth.pinniped.dev-something", // .oauth.pinniped.dev substring is not allowed + 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 @@ -1134,7 +1134,28 @@ func TestSupervisorLogin_Browser(t *testing.T) { 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 '.oauth.pinniped.dev'"}`, + `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) }, }, @@ -1846,6 +1867,7 @@ func doTokenExchange( resp, err := httpClient.Do(req) require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() // If a function was passed, call it, so it can make the desired assertions. if wantTokenExchangeResponse != nil { @@ -1858,7 +1880,6 @@ func doTokenExchange( // Else, want a successful response. require.Equal(t, resp.StatusCode, http.StatusOK) - defer func() { _ = resp.Body.Close() }() var respBody struct { AccessToken string `json:"access_token"` IssuedTokenType string `json:"issued_token_type"`