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

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.
This commit is contained in:
Ryan Richard 2022-06-13 12:08:11 -07:00
parent 0b6b8b4fcd
commit b9272b2729
3 changed files with 33 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

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

10
internal/oidc/token_exchange.go
View File

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

27
test/integration/supervisor_login_test.go
View File

@ -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"`