djpbessems
/
ContainerImage.Pinniped
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #1189 from vmware-tanzu/token_exchange_aud 

Disallow certain requested audience strings in token exchange
This commit is contained in:
Mo Khan 2022-06-14 16:41:51 -04:00 committed by GitHub
parent 268e1108d1 b9272b2729
commit c77bee67c1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 138 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
internal/oidc/token/token_handler_test.go
View File

@ -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,

21
internal/oidc/token_exchange.go
View File

@ -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
}

103
test/integration/supervisor_login_test.go
View File

@ -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)