From b9272b27298e5b11be63a6926a21277c4710ef4b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 13 Jun 2022 12:08:11 -0700 Subject: [PATCH] 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"`