Improve token exchange error messages and error test cases

This commit is contained in:
Ryan Richard 2022-08-23 17:20:30 -07:00
parent a876591765
commit 1384f75731
2 changed files with 138 additions and 56 deletions

View File

@ -47,6 +47,7 @@ import (
"go.pinniped.dev/internal/here" "go.pinniped.dev/internal/here"
"go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/httperr"
"go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc"
"go.pinniped.dev/internal/oidc/clientregistry"
"go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/jwks"
"go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/provider"
"go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/psession"
@ -647,11 +648,12 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
authcodeExchange authcodeExchangeInputs authcodeExchange authcodeExchangeInputs
modifyRequestParams func(t *testing.T, params url.Values) modifyRequestParams func(t *testing.T, params url.Values)
modifyStorage func(t *testing.T, storage *oidc.KubeStorage, pendingRequest *http.Request) modifyStorage func(t *testing.T, storage *oidc.KubeStorage, secrets v1.SecretInterface, pendingRequest *http.Request)
requestedAudience string requestedAudience string
wantStatus int wantStatus int
wantResponseBodyContains string wantErrorType string
wantErrorDescContains string
}{ }{
{ {
name: "happy path", name: "happy path",
@ -660,11 +662,12 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
wantStatus: http.StatusOK, wantStatus: http.StatusOK,
}, },
{ {
name: "missing audience", name: "missing audience",
authcodeExchange: doValidAuthCodeExchange, authcodeExchange: doValidAuthCodeExchange,
requestedAudience: "", requestedAudience: "",
wantStatus: http.StatusBadRequest, wantStatus: http.StatusBadRequest,
wantResponseBodyContains: "missing audience parameter", wantErrorType: "invalid_request",
wantErrorDescContains: "Missing 'audience' parameter.",
}, },
{ {
name: "missing subject_token", name: "missing subject_token",
@ -673,8 +676,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) { modifyRequestParams: func(t *testing.T, params url.Values) {
params.Del("subject_token") params.Del("subject_token")
}, },
wantStatus: http.StatusBadRequest, wantStatus: http.StatusBadRequest,
wantResponseBodyContains: "missing subject_token parameter", wantErrorType: "invalid_request",
wantErrorDescContains: "Missing 'subject_token' parameter.",
}, },
{ {
name: "wrong subject_token_type", name: "wrong subject_token_type",
@ -683,8 +687,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) { modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("subject_token_type", "invalid") params.Set("subject_token_type", "invalid")
}, },
wantStatus: http.StatusBadRequest, wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `unsupported subject_token_type parameter value`, wantErrorType: "invalid_request",
wantErrorDescContains: `Unsupported 'subject_token_type' parameter value, must be 'urn:ietf:params:oauth:token-type:access_token'.`,
}, },
{ {
name: "wrong requested_token_type", name: "wrong requested_token_type",
@ -693,8 +698,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) { modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("requested_token_type", "invalid") params.Set("requested_token_type", "invalid")
}, },
wantStatus: http.StatusBadRequest, wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `unsupported requested_token_type parameter value`, wantErrorType: "invalid_request",
wantErrorDescContains: `Unsupported 'requested_token_type' parameter value, must be 'urn:ietf:params:oauth:token-type:jwt'.`,
}, },
{ {
name: "unsupported RFC8693 parameter", name: "unsupported RFC8693 parameter",
@ -703,8 +709,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) { modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("resource", "some-resource-parameter-value") params.Set("resource", "some-resource-parameter-value")
}, },
wantStatus: http.StatusBadRequest, wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `unsupported parameter resource`, wantErrorType: "invalid_request",
wantErrorDescContains: `Unsupported parameter 'resource'.`,
}, },
{ {
name: "bogus access token", name: "bogus access token",
@ -713,20 +720,70 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) { modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("subject_token", "some-bogus-value") params.Set("subject_token", "some-bogus-value")
}, },
wantStatus: http.StatusBadRequest, wantStatus: http.StatusUnauthorized,
wantResponseBodyContains: `Invalid token format`, wantErrorType: "request_unauthorized",
wantErrorDescContains: `The request could not be authorized. Invalid 'subject_token' parameter value.`,
}, },
{ {
name: "valid access token, but deleted from storage", name: "valid access token, but it was already deleted from storage",
authcodeExchange: doValidAuthCodeExchange, authcodeExchange: doValidAuthCodeExchange,
requestedAudience: "some-workload-cluster", requestedAudience: "some-workload-cluster",
modifyStorage: func(t *testing.T, storage *oidc.KubeStorage, pendingRequest *http.Request) { modifyStorage: func(t *testing.T, storage *oidc.KubeStorage, secrets v1.SecretInterface, pendingRequest *http.Request) {
parts := strings.Split(pendingRequest.Form.Get("subject_token"), ".") parts := strings.Split(pendingRequest.Form.Get("subject_token"), ".")
require.Len(t, parts, 2) require.Len(t, parts, 2)
require.NoError(t, storage.DeleteAccessTokenSession(context.Background(), parts[1])) require.NoError(t, storage.DeleteAccessTokenSession(context.Background(), parts[1]))
}, },
wantStatus: http.StatusUnauthorized, wantStatus: http.StatusUnauthorized,
wantResponseBodyContains: `invalid subject_token`, wantErrorType: "request_unauthorized",
wantErrorDescContains: `Invalid 'subject_token' parameter value.`,
},
{
name: "valid access token, but it has already expired",
authcodeExchange: doValidAuthCodeExchange,
requestedAudience: "some-workload-cluster",
modifyStorage: func(t *testing.T, storage *oidc.KubeStorage, secrets v1.SecretInterface, pendingRequest *http.Request) {
// The fosite storage APIs don't offer a way to update an access token, so we will instead find the underlying
// storage Secret and update it in a more manual way. First get the access token's signature.
parts := strings.Split(pendingRequest.Form.Get("subject_token"), ".")
require.Len(t, parts, 2)
// Find the storage Secret for the access token by using its signature to compute the Secret name.
accessTokenSignature := parts[1]
accessTokenSecretName := getSecretNameFromSignature(t, accessTokenSignature, "access-token")
accessTokenSecret, err := secrets.Get(context.Background(), accessTokenSecretName, metav1.GetOptions{})
require.NoError(t, err)
// Parse the session from the storage Secret.
savedSessionJSON := accessTokenSecret.Data["pinniped-storage-data"]
// Declare the appropriate empty struct, similar to how our kubestorage implementation
// of GetAccessTokenSession() does when parsing a session from a storage Secret.
accessTokenSession := &accesstoken.Session{
Request: &fosite.Request{
Client: &clientregistry.Client{},
Session: &psession.PinnipedSession{},
},
}
// Parse the session JSON and fill the empty struct with its data.
err = json.Unmarshal(savedSessionJSON, accessTokenSession)
require.NoError(t, err)
// Change the access token's expiration time to be one hour ago, so it will be considered already expired.
oneHourAgoInUTC := time.Now().UTC().Add(-1 * time.Hour)
accessTokenSession.Request.Session.(*psession.PinnipedSession).Fosite.SetExpiresAt(fosite.AccessToken, oneHourAgoInUTC)
// Write the updated session back to the access token's storage Secret.
updatedSessionJSON, err := json.Marshal(accessTokenSession)
require.NoError(t, err)
accessTokenSecret.Data["pinniped-storage-data"] = updatedSessionJSON
_, err = secrets.Update(context.Background(), accessTokenSecret, metav1.UpdateOptions{})
require.NoError(t, err)
// Just to be sure that this test setup is valid, confirm that the code above correctly updated the
// access token's expiration time by reading it again, this time performing the read using the
// kubestorage API instead of the manual/direct approach used above.
session, err := storage.GetAccessTokenSession(context.Background(), accessTokenSignature, nil)
require.NoError(t, err)
expiresAt := session.GetSession().GetExpiresAt(fosite.AccessToken)
require.Equal(t, oneHourAgoInUTC, expiresAt)
},
wantStatus: http.StatusUnauthorized,
wantErrorType: "invalid_token",
wantErrorDescContains: `Token expired. Access token expired at `,
}, },
{ {
name: "access token missing pinniped:request-audience scope", name: "access token missing pinniped:request-audience scope",
@ -742,9 +799,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
wantGroups: goodGroups, wantGroups: goodGroups,
}, },
}, },
requestedAudience: "some-workload-cluster", requestedAudience: "some-workload-cluster",
wantStatus: http.StatusForbidden, wantStatus: http.StatusForbidden,
wantResponseBodyContains: `missing the 'pinniped:request-audience' scope`, wantErrorType: "access_denied",
wantErrorDescContains: `Missing the 'pinniped:request-audience' scope.`,
}, },
{ {
name: "access token missing openid scope", name: "access token missing openid scope",
@ -760,9 +818,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
wantGroups: goodGroups, wantGroups: goodGroups,
}, },
}, },
requestedAudience: "some-workload-cluster", requestedAudience: "some-workload-cluster",
wantStatus: http.StatusForbidden, wantStatus: http.StatusForbidden,
wantResponseBodyContains: `missing the 'openid' scope`, wantErrorType: "access_denied",
wantErrorDescContains: `Missing the 'openid' scope.`,
}, },
{ {
name: "token minting failure", name: "token minting failure",
@ -774,9 +833,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
makeOathHelper: makeOauthHelperWithJWTKeyThatWorksOnlyOnce, makeOathHelper: makeOauthHelperWithJWTKeyThatWorksOnlyOnce,
want: successfulAuthCodeExchange, want: successfulAuthCodeExchange,
}, },
requestedAudience: "some-workload-cluster", requestedAudience: "some-workload-cluster",
wantStatus: http.StatusServiceUnavailable, wantStatus: http.StatusServiceUnavailable,
wantResponseBodyContains: `The authorization server is currently unable to handle the request`, wantErrorType: "temporarily_unavailable",
wantErrorDescContains: `The authorization server is currently unable to handle the request`,
}, },
} }
for _, test := range tests { for _, test := range tests {
@ -792,13 +852,13 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
request := happyTokenExchangeRequest(test.requestedAudience, parsedAuthcodeExchangeResponseBody["access_token"].(string)) request := happyTokenExchangeRequest(test.requestedAudience, parsedAuthcodeExchangeResponseBody["access_token"].(string))
if test.modifyStorage != nil { if test.modifyStorage != nil {
test.modifyStorage(t, storage, request) test.modifyStorage(t, storage, secrets, request)
} }
if test.modifyRequestParams != nil { if test.modifyRequestParams != nil {
test.modifyRequestParams(t, request.Form) test.modifyRequestParams(t, request.Form)
} }
req := httptest.NewRequest("POST", "/path/shouldn't/matter", body(request.Form).ReadCloser()) req := httptest.NewRequest("POST", "/token/exchange/path/shouldn't/matter", body(request.Form).ReadCloser())
req.Header.Set("Content-Type", "application/x-www-form-urlencoded") req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
rsp = httptest.NewRecorder() rsp = httptest.NewRecorder()
@ -816,12 +876,23 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
require.Equal(t, test.wantStatus, rsp.Code) require.Equal(t, test.wantStatus, rsp.Code)
testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), "application/json") testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), "application/json")
if test.wantResponseBodyContains != "" {
require.Contains(t, rsp.Body.String(), test.wantResponseBodyContains)
}
// The remaining assertions apply only to the happy path. var parsedResponseBody map[string]interface{}
require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedResponseBody))
if rsp.Code != http.StatusOK { if rsp.Code != http.StatusOK {
// All error responses should have two JSON keys.
require.Len(t, parsedResponseBody, 2)
errorType := parsedResponseBody["error"]
require.NotEmpty(t, errorType)
require.Equal(t, test.wantErrorType, errorType)
errorDesc := parsedResponseBody["error_description"]
require.NotEmpty(t, errorDesc)
require.Contains(t, errorDesc, test.wantErrorDescContains)
// The remaining assertions apply only to the happy path.
return return
} }
@ -831,15 +902,12 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
err = firstIDTokenDecoded.UnsafeClaimsWithoutVerification(&claimsOfFirstIDToken) err = firstIDTokenDecoded.UnsafeClaimsWithoutVerification(&claimsOfFirstIDToken)
require.NoError(t, err) require.NoError(t, err)
var responseBody map[string]interface{} require.Contains(t, parsedResponseBody, "access_token")
require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &responseBody)) require.Equal(t, "N_A", parsedResponseBody["token_type"])
require.Equal(t, "urn:ietf:params:oauth:token-type:jwt", parsedResponseBody["issued_token_type"])
require.Contains(t, responseBody, "access_token")
require.Equal(t, "N_A", responseBody["token_type"])
require.Equal(t, "urn:ietf:params:oauth:token-type:jwt", responseBody["issued_token_type"])
// Parse the returned token. // Parse the returned token.
parsedJWT, err := jose.ParseSigned(responseBody["access_token"].(string)) parsedJWT, err := jose.ParseSigned(parsedResponseBody["access_token"].(string))
require.NoError(t, err) require.NoError(t, err)
var tokenClaims map[string]interface{} var tokenClaims map[string]interface{}
require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims)) require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims))
@ -3679,3 +3747,14 @@ func TestDiffSortedGroups(t *testing.T) {
}) })
} }
} }
func getSecretNameFromSignature(t *testing.T, signature string, typeLabel string) string {
t.Helper()
// try to decode base64 signatures to prevent double encoding of binary data
signatureBytes, err := base64.RawURLEncoding.DecodeString(signature)
require.NoError(t, err)
// lower case base32 encoding insures that our secret name is valid per ValidateSecretName in k/k
var b32 = base32.StdEncoding.WithPadding(base32.NoPadding)
signatureAsValidName := strings.ToLower(b32.EncodeToString(signatureBytes))
return fmt.Sprintf("pinniped-storage-%s-%s", typeLabel, signatureAsValidName)
}

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 // SPDX-License-Identifier: Apache-2.0
package oidc package oidc
@ -69,10 +69,10 @@ func (t *TokenExchangeHandler) PopulateTokenEndpointResponse(ctx context.Context
// Require that the incoming access token has the pinniped:request-audience and OpenID scopes. // Require that the incoming access token has the pinniped:request-audience and OpenID scopes.
if !originalRequester.GetGrantedScopes().Has(pinnipedTokenExchangeScope) { if !originalRequester.GetGrantedScopes().Has(pinnipedTokenExchangeScope) {
return errors.WithStack(fosite.ErrAccessDenied.WithHintf("missing the %q scope", pinnipedTokenExchangeScope)) return errors.WithStack(fosite.ErrAccessDenied.WithHintf("Missing the %q scope.", pinnipedTokenExchangeScope))
} }
if !originalRequester.GetGrantedScopes().Has(oidc.ScopeOpenID) { if !originalRequester.GetGrantedScopes().Has(oidc.ScopeOpenID) {
return errors.WithStack(fosite.ErrAccessDenied.WithHintf("missing the %q scope", oidc.ScopeOpenID)) return errors.WithStack(fosite.ErrAccessDenied.WithHintf("Missing the %q scope.", oidc.ScopeOpenID))
} }
// Use the original authorize request information, along with the requested audience, to mint a new JWT. // Use the original authorize request information, along with the requested audience, to mint a new JWT.
@ -100,19 +100,19 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er
// Validate some required parameters. // Validate some required parameters.
result.requestedAudience = params.Get("audience") result.requestedAudience = params.Get("audience")
if result.requestedAudience == "" { if result.requestedAudience == "" {
return nil, fosite.ErrInvalidRequest.WithHint("missing audience parameter") return nil, fosite.ErrInvalidRequest.WithHint("Missing 'audience' parameter.")
} }
result.subjectAccessToken = params.Get("subject_token") result.subjectAccessToken = params.Get("subject_token")
if result.subjectAccessToken == "" { if result.subjectAccessToken == "" {
return nil, fosite.ErrInvalidRequest.WithHint("missing subject_token parameter") return nil, fosite.ErrInvalidRequest.WithHint("Missing 'subject_token' parameter.")
} }
// Validate some parameters with hardcoded values we support. // Validate some parameters with hardcoded values we support.
if params.Get("subject_token_type") != tokenTypeAccessToken { if params.Get("subject_token_type") != tokenTypeAccessToken {
return nil, fosite.ErrInvalidRequest.WithHintf("unsupported subject_token_type parameter value, must be %q", tokenTypeAccessToken) return nil, fosite.ErrInvalidRequest.WithHintf("Unsupported 'subject_token_type' parameter value, must be %q.", tokenTypeAccessToken)
} }
if params.Get("requested_token_type") != tokenTypeJWT { if params.Get("requested_token_type") != tokenTypeJWT {
return nil, fosite.ErrInvalidRequest.WithHintf("unsupported requested_token_type parameter value, must be %q", tokenTypeJWT) return nil, fosite.ErrInvalidRequest.WithHintf("Unsupported 'requested_token_type' parameter value, must be %q.", tokenTypeJWT)
} }
// Validate that none of these unsupported parameters were sent. These are optional and we do not currently support them. // Validate that none of these unsupported parameters were sent. These are optional and we do not currently support them.
@ -123,7 +123,7 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er
"actor_token_type", "actor_token_type",
} { } {
if params.Get(param) != "" { if params.Get(param) != "" {
return nil, fosite.ErrInvalidRequest.WithHintf("unsupported parameter %s", param) return nil, fosite.ErrInvalidRequest.WithHintf("Unsupported parameter %q.", param)
} }
} }
@ -131,13 +131,16 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er
} }
func (t *TokenExchangeHandler) validateAccessToken(ctx context.Context, requester fosite.AccessRequester, accessToken string) (fosite.Requester, error) { func (t *TokenExchangeHandler) validateAccessToken(ctx context.Context, requester fosite.AccessRequester, accessToken string) (fosite.Requester, error) {
if err := t.accessTokenStrategy.ValidateAccessToken(ctx, requester, accessToken); err != nil { // Look up the access token's stored session data.
return nil, errors.WithStack(err)
}
signature := t.accessTokenStrategy.AccessTokenSignature(accessToken) signature := t.accessTokenStrategy.AccessTokenSignature(accessToken)
originalRequester, err := t.accessTokenStorage.GetAccessTokenSession(ctx, signature, requester.GetSession()) originalRequester, err := t.accessTokenStorage.GetAccessTokenSession(ctx, signature, requester.GetSession())
if err != nil { if err != nil {
return nil, fosite.ErrRequestUnauthorized.WithWrap(err).WithHint("invalid subject_token") // The access token was not found, or there was some other error while reading it.
return nil, fosite.ErrRequestUnauthorized.WithWrap(err).WithHint("Invalid 'subject_token' parameter value.")
}
// Validate the access token using its stored session data, which includes its expiration time.
if err := t.accessTokenStrategy.ValidateAccessToken(ctx, originalRequester, accessToken); err != nil {
return nil, errors.WithStack(err)
} }
return originalRequester, nil return originalRequester, nil
} }