Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve token exchange error messages and error test cases #1264

Merged
merged 3 commits into from
Aug 25, 2022
Merged

Improve token exchange error messages and error test cases #1264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1384f75
Improve token exchange error messages and error test cases
cfryanr Aug 24, 2022
65197d0
Merge branch 'main' into access_token_validation
cfryanr Aug 24, 2022
c404651
Merge branch 'main' into access_token_validation
cfryanr Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 123 additions & 44 deletions internal/oidc/token/token_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"go.pinniped.dev/internal/here"
"go.pinniped.dev/internal/httputil/httperr"
"go.pinniped.dev/internal/oidc"
"go.pinniped.dev/internal/oidc/clientregistry"
"go.pinniped.dev/internal/oidc/jwks"
"go.pinniped.dev/internal/oidc/provider"
"go.pinniped.dev/internal/psession"
Expand Down Expand Up @@ -646,11 +647,12 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn

authcodeExchange authcodeExchangeInputs
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

wantStatus int
wantResponseBodyContains string
wantStatus int
wantErrorType string
wantErrorDescContains string
}{
{
name: "happy path",
Expand All @@ -659,11 +661,12 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
wantStatus: http.StatusOK,
},
{
name: "missing audience",
authcodeExchange: doValidAuthCodeExchange,
requestedAudience: "",
wantStatus: http.StatusBadRequest,
wantResponseBodyContains: "missing audience parameter",
name: "missing audience",
authcodeExchange: doValidAuthCodeExchange,
requestedAudience: "",
wantStatus: http.StatusBadRequest,
wantErrorType: "invalid_request",
wantErrorDescContains: "Missing 'audience' parameter.",
},
{
name: "missing subject_token",
Expand All @@ -672,8 +675,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) {
params.Del("subject_token")
},
wantStatus: http.StatusBadRequest,
wantResponseBodyContains: "missing subject_token parameter",
wantStatus: http.StatusBadRequest,
wantErrorType: "invalid_request",
wantErrorDescContains: "Missing 'subject_token' parameter.",
},
{
name: "wrong subject_token_type",
Expand All @@ -682,8 +686,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("subject_token_type", "invalid")
},
wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `unsupported subject_token_type parameter value`,
wantStatus: http.StatusBadRequest,
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",
Expand All @@ -692,8 +697,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("requested_token_type", "invalid")
},
wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `unsupported requested_token_type parameter value`,
wantStatus: http.StatusBadRequest,
wantErrorType: "invalid_request",
wantErrorDescContains: `Unsupported 'requested_token_type' parameter value, must be 'urn:ietf:params:oauth:token-type:jwt'.`,
},
{
name: "unsupported RFC8693 parameter",
Expand All @@ -702,8 +708,9 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("resource", "some-resource-parameter-value")
},
wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `unsupported parameter resource`,
wantStatus: http.StatusBadRequest,
wantErrorType: "invalid_request",
wantErrorDescContains: `Unsupported parameter 'resource'.`,
},
{
name: "bogus access token",
Expand All @@ -712,20 +719,70 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
modifyRequestParams: func(t *testing.T, params url.Values) {
params.Set("subject_token", "some-bogus-value")
},
wantStatus: http.StatusBadRequest,
wantResponseBodyContains: `Invalid token format`,
wantStatus: http.StatusUnauthorized,
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,
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"), ".")
require.Len(t, parts, 2)
require.NoError(t, storage.DeleteAccessTokenSession(context.Background(), parts[1]))
},
wantStatus: http.StatusUnauthorized,
wantResponseBodyContains: `invalid subject_token`,
wantStatus: http.StatusUnauthorized,
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",
Expand All @@ -741,9 +798,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
wantGroups: goodGroups,
},
},
requestedAudience: "some-workload-cluster",
wantStatus: http.StatusForbidden,
wantResponseBodyContains: `missing the 'pinniped:request-audience' scope`,
requestedAudience: "some-workload-cluster",
wantStatus: http.StatusForbidden,
wantErrorType: "access_denied",
wantErrorDescContains: `Missing the 'pinniped:request-audience' scope.`,
},
{
name: "access token missing openid scope",
Expand All @@ -759,9 +817,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
wantGroups: goodGroups,
},
},
requestedAudience: "some-workload-cluster",
wantStatus: http.StatusForbidden,
wantResponseBodyContains: `missing the 'openid' scope`,
requestedAudience: "some-workload-cluster",
wantStatus: http.StatusForbidden,
wantErrorType: "access_denied",
wantErrorDescContains: `Missing the 'openid' scope.`,
},
{
name: "token minting failure",
Expand All @@ -773,9 +832,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
makeOathHelper: makeOauthHelperWithJWTKeyThatWorksOnlyOnce,
want: successfulAuthCodeExchange,
},
requestedAudience: "some-workload-cluster",
wantStatus: http.StatusServiceUnavailable,
wantResponseBodyContains: `The authorization server is currently unable to handle the request`,
requestedAudience: "some-workload-cluster",
wantStatus: http.StatusServiceUnavailable,
wantErrorType: "temporarily_unavailable",
wantErrorDescContains: `The authorization server is currently unable to handle the request`,
},
}
for _, test := range tests {
Expand All @@ -791,13 +851,13 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn

request := happyTokenExchangeRequest(test.requestedAudience, parsedAuthcodeExchangeResponseBody["access_token"].(string))
if test.modifyStorage != nil {
test.modifyStorage(t, storage, request)
test.modifyStorage(t, storage, secrets, request)
}
if test.modifyRequestParams != nil {
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")
rsp = httptest.NewRecorder()

Expand All @@ -815,12 +875,23 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn

require.Equal(t, test.wantStatus, rsp.Code)
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 {
// 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
}

Expand All @@ -830,15 +901,12 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn
err = firstIDTokenDecoded.UnsafeClaimsWithoutVerification(&claimsOfFirstIDToken)
require.NoError(t, err)

var responseBody map[string]interface{}
require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &responseBody))

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"])
require.Contains(t, parsedResponseBody, "access_token")
require.Equal(t, "N_A", parsedResponseBody["token_type"])
require.Equal(t, "urn:ietf:params:oauth:token-type:jwt", parsedResponseBody["issued_token_type"])

// Parse the returned token.
parsedJWT, err := jose.ParseSigned(responseBody["access_token"].(string))
parsedJWT, err := jose.ParseSigned(parsedResponseBody["access_token"].(string))
require.NoError(t, err)
var tokenClaims map[string]interface{}
require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims))
Expand Down Expand Up @@ -3678,3 +3746,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)
}
25 changes: 14 additions & 11 deletions internal/oidc/token_exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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) {
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.
Expand Down Expand Up @@ -100,19 +100,19 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er
// Validate some required parameters.
result.requestedAudience = params.Get("audience")
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")
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.
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 {
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.
Expand All @@ -123,21 +123,24 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er
"actor_token_type",
} {
if params.Get(param) != "" {
return nil, fosite.ErrInvalidRequest.WithHintf("unsupported parameter %s", param)
return nil, fosite.ErrInvalidRequest.WithHintf("Unsupported parameter %q.", param)
}
}

return &result, nil
}

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 {
return nil, errors.WithStack(err)
}
// Look up the access token's stored session data.
signature := t.accessTokenStrategy.AccessTokenSignature(accessToken)
originalRequester, err := t.accessTokenStorage.GetAccessTokenSession(ctx, signature, requester.GetSession())
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
}
Expand Down