From 02d96d731fb1906cc381ad6c3c68956df13230c7 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 9 Dec 2020 13:56:53 -0600 Subject: [PATCH] Finish TestTokenExchange unit tests and add missing scope check. Signed-off-by: Margo Crawford --- internal/oidc/token/token_handler_test.go | 256 ++++++++++++++++++++-- internal/oidc/token_exchange.go | 25 ++- 2 files changed, 256 insertions(+), 25 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index a134410e..0cf67f66 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -26,6 +26,7 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" @@ -569,14 +570,16 @@ func TestTokenExchange(t *testing.T) { tests := []struct { name string - authcodeExchange authcodeExchangeInputs - modifyTokenExchangeRequest func(r *http.Request) - requestedAudience string + authcodeExchange authcodeExchangeInputs + modifyRequestParams func(t *testing.T, params url.Values) + modifyStorage func(t *testing.T, storage *oidc.KubeStorage, pendingRequest *http.Request) + requestedAudience string - wantStatus int + wantStatus int + wantResponseBodyContains string }{ { - name: "token exchange happy path", + name: "happy path", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") @@ -589,37 +592,226 @@ func TestTokenExchange(t *testing.T) { requestedAudience: "some-workload-cluster", wantStatus: http.StatusOK, }, - // TODO add the unhappy path table entries: wrong client id, invalid access token, access token does not have pinniped.sts.unrestricted, etc. - // Can use modifyAuthRequest to change the request params for the original authorize request (e.g. don't request pinniped.sts.unrestricted). - // Can use modifyTokenExchangeRequest to change to request params for the token exchange call (e.g. bad access token or wrong http verb). - // Will need to add some more fields to the test table to declare the expected values of the error response, etc. + { + name: "missing audience", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: "missing audience parameter", + }, + { + name: "missing subject_token", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("subject_token") + }, + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: "missing subject_token parameter", + }, + { + name: "wrong subject_token_type", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Set("subject_token_type", "invalid") + }, + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: `unsupported subject_token_type parameter value`, + }, + { + name: "wrong requested_token_type", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Set("requested_token_type", "invalid") + }, + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: `unsupported requested_token_type parameter value`, + }, + { + name: "unsupported RFC8693 parameter", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Set("resource", "some-resource-parameter-value") + }, + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: `unsupported parameter resource`, + }, + { + name: "bogus access token", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Set("subject_token", "some-bogus-value") + }, + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: `Invalid token format`, + }, + { + name: "valid access token, but deleted from storage", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + modifyStorage: func(t *testing.T, storage *oidc.KubeStorage, 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`, + }, + { + name: "access token missing required scopes", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid") + }, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid"}, + wantGrantedScopes: []string{"openid"}, + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusForbidden, + wantResponseBodyContains: `missing the \"pinniped.sts.unrestricted\" scope`, + }, + { + name: "token minting failure", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + }, + // Fail to fetch a JWK signing key after the authcode exchange has happened. + makeOathHelper: makeOauthHelperWithJWTKeyThatWorksOnlyOnce, + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusServiceUnavailable, + wantResponseBodyContains: `The authorization server is currently unable to handle the request`, + }, } for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject, rsp, _, _, _ := exchangeAuthcodeForTokens(t, test.authcodeExchange) + subject, rsp, _, secrets, storage := exchangeAuthcodeForTokens(t, test.authcodeExchange) var parsedResponseBody map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedResponseBody)) request := happyTokenExchangeRequest(test.requestedAudience, parsedResponseBody["access_token"].(string)) + if test.modifyStorage != nil { + test.modifyStorage(t, storage, request) + } + if test.modifyRequestParams != nil { + test.modifyRequestParams(t, request.Form) + } req := httptest.NewRequest("POST", "/path/shouldn't/matter", body(request.Form).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - if test.modifyTokenExchangeRequest != nil { - test.modifyTokenExchangeRequest(req) - } rsp = httptest.NewRecorder() + // Measure the secrets in storage after the auth code flow. + existingSecrets, err := secrets.List(context.Background(), metav1.ListOptions{}) + require.NoError(t, err) + subject.ServeHTTP(rsp, req) t.Logf("response: %#v", rsp) t.Logf("response body: %q", rsp.Body.String()) 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) + } - // TODO write lots more assertions about the happy path - // e.g. that nothing was changed in the fosite k8s storage - // e.g. that the response body is correct, and the decoded token has the right claims + // The remaining assertions apply only the the happy path. + if rsp.Code != http.StatusOK { + return + } + + 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"]) + + // Assert that the returned token has expected claims. + parsedJWT, err := jose.ParseSigned(responseBody["access_token"].(string)) + require.NoError(t, err) + var tokenClaims map[string]interface{} + require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims)) + require.Contains(t, tokenClaims, "iat") + require.Contains(t, tokenClaims, "rat") + require.Contains(t, tokenClaims, "jti") + require.Len(t, tokenClaims["aud"], 1) + require.Contains(t, tokenClaims["aud"], test.requestedAudience) + require.Equal(t, goodSubject, tokenClaims["sub"]) + require.Equal(t, goodIssuer, tokenClaims["iss"]) + + // Assert that nothing in storage has been modified. + newSecrets, err := secrets.List(context.Background(), metav1.ListOptions{}) + require.NoError(t, err) + require.ElementsMatch(t, existingSecrets.Items, newSecrets.Items) }) } } @@ -795,6 +987,38 @@ func makeHappyOauthHelper( return oauthHelper, authResponder.GetCode(), jwtSigningKey } +type singleUseJWKProvider struct { + jwks.DynamicJWKSProvider + calls int +} + +func (s *singleUseJWKProvider) GetJWKS(issuerName string) (jwks *jose.JSONWebKeySet, activeJWK *jose.JSONWebKey) { + s.calls += 1 + if s.calls > 1 { + return nil, nil + } + return s.DynamicJWKSProvider.GetJWKS(issuerName) +} + +func makeOauthHelperWithJWTKeyThatWorksOnlyOnce( + t *testing.T, + authRequest *http.Request, + store interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager + }, +) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) { + t.Helper() + + jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}) + authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) + return oauthHelper, authResponder.GetCode(), jwtSigningKey +} + func makeOauthHelperWithNilPrivateJWTSigningKey( t *testing.T, authRequest *http.Request, diff --git a/internal/oidc/token_exchange.go b/internal/oidc/token_exchange.go index bc97bfe1..a47d9cd6 100644 --- a/internal/oidc/token_exchange.go +++ b/internal/oidc/token_exchange.go @@ -7,6 +7,7 @@ import ( "context" "net/url" + "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/compose" "github.com/ory/fosite/handler/oauth2" @@ -15,8 +16,9 @@ import ( ) const ( - tokenTypeAccessToken = "urn:ietf:params:oauth:token-type:access_token" //nolint: gosec - tokenTypeJWT = "urn:ietf:params:oauth:token-type:jwt" //nolint: gosec + tokenTypeAccessToken = "urn:ietf:params:oauth:token-type:access_token" //nolint: gosec + tokenTypeJWT = "urn:ietf:params:oauth:token-type:jwt" //nolint: gosec + pinnipedTokenExchangeScope = "pinniped.sts.unrestricted" //nolint: gosec ) type stsParams struct { @@ -63,6 +65,11 @@ func (t *TokenExchangeHandler) PopulateTokenEndpointResponse(ctx context.Context return errors.WithStack(err) } + // Require that the incoming access token has the STS and OpenID scopes . + if !originalRequester.GetGrantedScopes().Has(pinnipedTokenExchangeScope, oidc.ScopeOpenID) { + return errors.WithStack(fosite.ErrAccessDenied.WithHintf("missing the %q scope", pinnipedTokenExchangeScope)) + } + // Use the original authorize request information, along with the requested audience, to mint a new JWT. responseToken, err := t.mintJWT(ctx, originalRequester, params.requestedAudience) if err != nil { @@ -72,7 +79,7 @@ func (t *TokenExchangeHandler) PopulateTokenEndpointResponse(ctx context.Context // Format the response parameters according to RFC8693. responder.SetAccessToken(responseToken) responder.SetTokenType("N_A") - responder.SetExtra("issued_token_type", "urn:ietf:params:oauth:token-type:jwt") + responder.SetExtra("issued_token_type", tokenTypeJWT) return nil } @@ -88,19 +95,19 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er // Validate some required parameters. result.requestedAudience = params.Get("audience") if result.requestedAudience == "" { - return nil, errors.WithMessagef(fosite.ErrInvalidRequest, "missing audience parameter") + return nil, fosite.ErrInvalidRequest.WithHint("missing audience parameter") } result.subjectAccessToken = params.Get("subject_token") if result.subjectAccessToken == "" { - return nil, errors.WithMessagef(fosite.ErrInvalidRequest, "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, errors.WithMessagef(fosite.ErrInvalidRequest, "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, errors.WithMessagef(fosite.ErrInvalidRequest, "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. @@ -111,7 +118,7 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er "actor_token_type", } { if params.Get(param) != "" { - return nil, errors.WithMessagef(fosite.ErrInvalidRequest, "unsupported parameter %s", param) + return nil, fosite.ErrInvalidRequest.WithHintf("unsupported parameter %s", param) } } @@ -125,7 +132,7 @@ func (t *TokenExchangeHandler) validateAccessToken(ctx context.Context, requeste signature := t.accessTokenStrategy.AccessTokenSignature(accessToken) originalRequester, err := t.accessTokenStorage.GetAccessTokenSession(ctx, signature, requester.GetSession()) if err != nil { - return nil, errors.WithStack(err) + return nil, fosite.ErrRequestUnauthorized.WithCause(err).WithHint("invalid subject_token") } return originalRequester, nil }