From c090eb6a6231f0bf83ebbaed2eb1f97204f44842 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 8 Dec 2020 11:47:39 -0800 Subject: [PATCH] Supervisor token endpoint returns refresh tokens when requested --- internal/oidc/kube_storage.go | 184 ++++++++---- internal/oidc/nullstorage_test.go | 2 +- internal/oidc/oidc.go | 7 +- internal/oidc/token/token_handler_test.go | 329 ++++++++++++++-------- 4 files changed, 344 insertions(+), 178 deletions(-) diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index 874e3164..b4329722 100644 --- a/internal/oidc/kube_storage.go +++ b/internal/oidc/kube_storage.go @@ -41,73 +41,147 @@ func NewKubeStorage(secrets corev1client.SecretInterface) *KubeStorage { } } -func (k KubeStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { - return k.refreshTokenStorage.RevokeRefreshToken(ctx, requestID) +// +// Authorization Code sessions: +// +// These are keyed by the signature of the authcode. +// +// Fosite will create these in the authorize endpoint. +// +// Fosite will never delete them. Instead, it wants to mark them as invalidated once the authcode is used to redeem tokens. +// That way, it can later detect the case where an authcode that was already redeemed gets used again. +// + +func (k KubeStorage) CreateAuthorizeCodeSession(ctx context.Context, signatureOfAuthcode string, r fosite.Requester) (err error) { + return k.authorizationCodeStorage.CreateAuthorizeCodeSession(ctx, signatureOfAuthcode, r) +} + +func (k KubeStorage) GetAuthorizeCodeSession(ctx context.Context, signatureOfAuthcode string, s fosite.Session) (request fosite.Requester, err error) { + return k.authorizationCodeStorage.GetAuthorizeCodeSession(ctx, signatureOfAuthcode, s) +} + +func (k KubeStorage) InvalidateAuthorizeCodeSession(ctx context.Context, signatureOfAuthcode string) (err error) { + return k.authorizationCodeStorage.InvalidateAuthorizeCodeSession(ctx, signatureOfAuthcode) +} + +// +// PKCE sessions: +// +// These are keyed by the signature of the authcode. +// +// Fosite will create these in the authorize endpoint at the same time that it is creating an authcode. +// +// Fosite will delete these in the token endpoint during authcode redemption since they are no longer needed after that. +// If the user chooses to never redeem their authcode, then fosite will never delete these. +// + +func (k KubeStorage) CreatePKCERequestSession(ctx context.Context, signatureOfAuthcode string, requester fosite.Requester) error { + return k.pkceStorage.CreatePKCERequestSession(ctx, signatureOfAuthcode, requester) +} + +func (k KubeStorage) GetPKCERequestSession(ctx context.Context, signatureOfAuthcode string, session fosite.Session) (fosite.Requester, error) { + return k.pkceStorage.GetPKCERequestSession(ctx, signatureOfAuthcode, session) +} + +func (k KubeStorage) DeletePKCERequestSession(ctx context.Context, signatureOfAuthcode string) error { + return k.pkceStorage.DeletePKCERequestSession(ctx, signatureOfAuthcode) +} + +// +// OpenID Connect sessions: +// +// These are keyed by the full value of the authcode (not just the signature). +// +// Fosite will create these in the authorize endpoint when it creates an authcode, but only if the user +// requested the openid scope. +// +// Fosite will never delete these, which is likely a bug in fosite. Although there is a delete method below, fosite +// never calls it. Used during authcode redemption, they will never be accessed again after a successful authcode +// redemption. Although that implies that they should probably follow a lifecycle similar the the PKCE storage, they +// are, in fact, not deleted. +// + +func (k KubeStorage) CreateOpenIDConnectSession(ctx context.Context, fullAuthcode string, requester fosite.Requester) error { + return k.oidcStorage.CreateOpenIDConnectSession(ctx, fullAuthcode, requester) +} + +func (k KubeStorage) GetOpenIDConnectSession(ctx context.Context, fullAuthcode string, requester fosite.Requester) (fosite.Requester, error) { + return k.oidcStorage.GetOpenIDConnectSession(ctx, fullAuthcode, requester) +} + +func (k KubeStorage) DeleteOpenIDConnectSession(ctx context.Context, fullAuthcode string) error { + return k.oidcStorage.DeleteOpenIDConnectSession(ctx, fullAuthcode) +} + +// +// Access token sessions: +// +// These are keyed by the signature of the access token. +// +// Fosite will create these in the token endpoint whenever it wants to hand out an access token, including the original +// authcode redemption and also during refresh. +// +// Fosite will not use the delete method. Instead, it will use the revoke method to delete them. +// During a refresh in the token endpoint, the old access token is revoked just before the new access token is created. +// Also, if the token endpoint receives an authcode that was already used successfully, then it revokes the access token +// that was previously handed out for that authcode. If a user stops coming back to refresh their tokens, then that +// access token will never be deleted. +// + +func (k KubeStorage) CreateAccessTokenSession(ctx context.Context, signatureOfAccessToken string, requester fosite.Requester) (err error) { + return k.accessTokenStorage.CreateAccessTokenSession(ctx, signatureOfAccessToken, requester) +} + +func (k KubeStorage) GetAccessTokenSession(ctx context.Context, signatureOfAccessToken string, session fosite.Session) (request fosite.Requester, err error) { + return k.accessTokenStorage.GetAccessTokenSession(ctx, signatureOfAccessToken, session) +} + +func (k KubeStorage) DeleteAccessTokenSession(ctx context.Context, signatureOfAccessToken string) (err error) { + return k.accessTokenStorage.DeleteAccessTokenSession(ctx, signatureOfAccessToken) } func (k KubeStorage) RevokeAccessToken(ctx context.Context, requestID string) error { return k.accessTokenStorage.RevokeAccessToken(ctx, requestID) } -func (k KubeStorage) CreateRefreshTokenSession(ctx context.Context, signature string, request fosite.Requester) (err error) { - return k.refreshTokenStorage.CreateRefreshTokenSession(ctx, signature, request) +// +// Refresh token sessions: +// +// These are keyed by the signature of the refresh token. +// +// Fosite will create these in the token endpoint whenever it wants to hand out an refresh token, including the original +// authcode redemption and also during refresh. Refresh tokens are only handed out when the user requested the +// offline_access scope on the original authorization request. +// +// Fosite will not use the delete method. Instead, it will use the revoke method to delete them. +// During a refresh in the token endpoint, the old refresh token is revoked just before the new refresh token is created. +// Also, if the token endpoint receives an authcode that was already used successfully, then it revokes the refresh token +// that was previously handed out for that authcode. If a user stops coming back to refresh their tokens, then that +// refresh token will never be deleted. +// + +func (k KubeStorage) CreateRefreshTokenSession(ctx context.Context, signatureOfRefreshToken string, request fosite.Requester) (err error) { + return k.refreshTokenStorage.CreateRefreshTokenSession(ctx, signatureOfRefreshToken, request) } -func (k KubeStorage) GetRefreshTokenSession(ctx context.Context, signature string, session fosite.Session) (request fosite.Requester, err error) { - return k.refreshTokenStorage.GetRefreshTokenSession(ctx, signature, session) +func (k KubeStorage) GetRefreshTokenSession(ctx context.Context, signatureOfRefreshToken string, session fosite.Session) (request fosite.Requester, err error) { + return k.refreshTokenStorage.GetRefreshTokenSession(ctx, signatureOfRefreshToken, session) } -func (k KubeStorage) DeleteRefreshTokenSession(ctx context.Context, signature string) (err error) { - return k.refreshTokenStorage.DeleteRefreshTokenSession(ctx, signature) +func (k KubeStorage) DeleteRefreshTokenSession(ctx context.Context, signatureOfRefreshToken string) (err error) { + return k.refreshTokenStorage.DeleteRefreshTokenSession(ctx, signatureOfRefreshToken) } -func (k KubeStorage) CreateAccessTokenSession(ctx context.Context, signature string, requester fosite.Requester) (err error) { - return k.accessTokenStorage.CreateAccessTokenSession(ctx, signature, requester) +func (k KubeStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { + return k.refreshTokenStorage.RevokeRefreshToken(ctx, requestID) } -func (k KubeStorage) GetAccessTokenSession(ctx context.Context, signature string, session fosite.Session) (request fosite.Requester, err error) { - return k.accessTokenStorage.GetAccessTokenSession(ctx, signature, session) -} - -func (k KubeStorage) DeleteAccessTokenSession(ctx context.Context, signature string) (err error) { - return k.accessTokenStorage.DeleteAccessTokenSession(ctx, signature) -} - -func (k KubeStorage) CreateOpenIDConnectSession(ctx context.Context, authcode string, requester fosite.Requester) error { - return k.oidcStorage.CreateOpenIDConnectSession(ctx, authcode, requester) -} - -func (k KubeStorage) GetOpenIDConnectSession(ctx context.Context, authcode string, requester fosite.Requester) (fosite.Requester, error) { - return k.oidcStorage.GetOpenIDConnectSession(ctx, authcode, requester) -} - -func (k KubeStorage) DeleteOpenIDConnectSession(ctx context.Context, authcode string) error { - return k.oidcStorage.DeleteOpenIDConnectSession(ctx, authcode) -} - -func (k KubeStorage) GetPKCERequestSession(ctx context.Context, signature string, session fosite.Session) (fosite.Requester, error) { - return k.pkceStorage.GetPKCERequestSession(ctx, signature, session) -} - -func (k KubeStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error { - return k.pkceStorage.CreatePKCERequestSession(ctx, signature, requester) -} - -func (k KubeStorage) DeletePKCERequestSession(ctx context.Context, signature string) error { - return k.pkceStorage.DeletePKCERequestSession(ctx, signature) -} - -func (k KubeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, r fosite.Requester) (err error) { - return k.authorizationCodeStorage.CreateAuthorizeCodeSession(ctx, signature, r) -} - -func (k KubeStorage) GetAuthorizeCodeSession(ctx context.Context, signature string, s fosite.Session) (request fosite.Requester, err error) { - return k.authorizationCodeStorage.GetAuthorizeCodeSession(ctx, signature, s) -} - -func (k KubeStorage) InvalidateAuthorizeCodeSession(ctx context.Context, signature string) (err error) { - return k.authorizationCodeStorage.InvalidateAuthorizeCodeSession(ctx, signature) -} +// +// OAuth client definitions: +// +// For the time being, we only allow a single pre-defined client, so we do not need to interact with any underlying +// storage mechanism to fetch them. +// func (KubeStorage) GetClient(_ context.Context, id string) (fosite.Client, error) { client := PinnipedCLIOIDCClient() @@ -117,6 +191,10 @@ func (KubeStorage) GetClient(_ context.Context, id string) (fosite.Client, error return nil, fosite.ErrNotFound } +// +// Unused interface methods. +// + func (KubeStorage) ClientAssertionJWTValid(_ context.Context, _ string) error { return errKubeStorageNotImplemented } diff --git a/internal/oidc/nullstorage_test.go b/internal/oidc/nullstorage_test.go index d0c49b1e..4adbe807 100644 --- a/internal/oidc/nullstorage_test.go +++ b/internal/oidc/nullstorage_test.go @@ -27,7 +27,7 @@ func TestNullStorage_GetClient(t *testing.T) { Public: true, RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, - GrantTypes: []string{"authorization_code"}, + GrantTypes: []string{"authorization_code", "refresh_token"}, Scopes: []string{"openid", "offline_access", "profile", "email"}, }, TokenEndpointAuthMethod: "none", diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 10d633aa..1e76858d 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -84,7 +84,7 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { Public: true, RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, - GrantTypes: []string{"authorization_code"}, + GrantTypes: []string{"authorization_code", "refresh_token"}, Scopes: []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, "profile", "email"}, }, TokenEndpointAuthMethod: "none", @@ -112,8 +112,9 @@ func FositeOauth2Helper( EnforcePKCE: true, // follow current set of best practices and always require PKCE AllowedPromptValues: []string{"none"}, // TODO unclear what we should set here - RefreshTokenScopes: nil, // TODO decide what makes sense when we add refresh token support - MinParameterEntropy: 32, // 256 bits seems about right + RefreshTokenScopes: []string{coreosoidc.ScopeOfflineAccess}, // as per https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess + + MinParameterEntropy: 32, // 256 bits seems about right } return compose.Compose( diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 7a3e2ffe..d2a49e15 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -211,8 +211,9 @@ func TestTokenEndpoint(t *testing.T) { tests := []struct { name string - authRequest func(authRequest *http.Request) - storage func( + modifyAuthRequest func(authRequest *http.Request) + modifyTokenRequest func(r *http.Request, authCode string) + modifyStorage func( t *testing.T, s interface { oauth2.TokenRevocationStorage @@ -223,7 +224,6 @@ func TestTokenEndpoint(t *testing.T) { }, authCode string, ) - request func(r *http.Request, authCode string) makeOathHelper func( t *testing.T, authRequest *http.Request, @@ -236,73 +236,94 @@ func TestTokenEndpoint(t *testing.T) { }, ) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) - wantStatus int - wantBodyFields []string - wantExactBody string + wantStatus int + wantBodyFields []string + wantExactBody string + wantRequestedScopes []string }{ // happy path { - name: "request is valid and tokens are issued", - wantStatus: http.StatusOK, - wantBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, + name: "request is valid and tokens are issued", + wantStatus: http.StatusOK, + wantBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token + wantRequestedScopes: []string{"openid", "profile", "email"}, }, { name: "openid scope was not requested from authorize endpoint", - authRequest: func(authRequest *http.Request) { + modifyAuthRequest: func(authRequest *http.Request) { authRequest.Form.Set("scope", "profile email") }, - wantStatus: http.StatusOK, - wantBodyFields: []string{"access_token", "token_type", "scope", "expires_in"}, + wantStatus: http.StatusOK, + wantBodyFields: []string{"access_token", "token_type", "scope", "expires_in"}, // no id or refresh tokens + wantRequestedScopes: []string{"profile", "email"}, + }, + { + name: "offline_access and openid scopes were requested and granted from authorize endpoint", + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid offline_access") + }, + wantStatus: http.StatusOK, + wantBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in", "refresh_token"}, // all possible tokens + wantRequestedScopes: []string{"openid", "offline_access"}, + }, + { + name: "offline_access (without openid scope) was requested and granted from authorize endpoint", + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "offline_access") + }, + wantStatus: http.StatusOK, + wantBodyFields: []string{"access_token", "token_type", "scope", "expires_in", "refresh_token"}, // no id token + wantRequestedScopes: []string{"offline_access"}, }, // sad path { - name: "GET method is wrong", - request: func(r *http.Request, authCode string) { r.Method = http.MethodGet }, - wantStatus: http.StatusBadRequest, - wantExactBody: fositeInvalidMethodErrorBody("GET"), + name: "GET method is wrong", + modifyTokenRequest: func(r *http.Request, authCode string) { r.Method = http.MethodGet }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("GET"), }, { - name: "PUT method is wrong", - request: func(r *http.Request, authCode string) { r.Method = http.MethodPut }, - wantStatus: http.StatusBadRequest, - wantExactBody: fositeInvalidMethodErrorBody("PUT"), + name: "PUT method is wrong", + modifyTokenRequest: func(r *http.Request, authCode string) { r.Method = http.MethodPut }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("PUT"), }, { - name: "PATCH method is wrong", - request: func(r *http.Request, authCode string) { r.Method = http.MethodPatch }, - wantStatus: http.StatusBadRequest, - wantExactBody: fositeInvalidMethodErrorBody("PATCH"), + name: "PATCH method is wrong", + modifyTokenRequest: func(r *http.Request, authCode string) { r.Method = http.MethodPatch }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("PATCH"), }, { - name: "DELETE method is wrong", - request: func(r *http.Request, authCode string) { r.Method = http.MethodDelete }, - wantStatus: http.StatusBadRequest, - wantExactBody: fositeInvalidMethodErrorBody("DELETE"), + name: "DELETE method is wrong", + modifyTokenRequest: func(r *http.Request, authCode string) { r.Method = http.MethodDelete }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("DELETE"), }, { - name: "content type is invalid", - request: func(r *http.Request, authCode string) { r.Header.Set("Content-Type", "text/plain") }, - wantStatus: http.StatusBadRequest, - wantExactBody: fositeEmptyPayloadErrorBody, + name: "content type is invalid", + modifyTokenRequest: func(r *http.Request, authCode string) { r.Header.Set("Content-Type", "text/plain") }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeEmptyPayloadErrorBody, }, { name: "payload is not valid form serialization", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = ioutil.NopCloser(strings.NewReader("this newline character is not allowed in a form serialization: \n")) }, wantStatus: http.StatusBadRequest, wantExactBody: fositeMissingGrantTypeErrorBody, }, { - name: "payload is empty", - request: func(r *http.Request, authCode string) { r.Body = nil }, - wantStatus: http.StatusBadRequest, - wantExactBody: fositeInvalidPayloadErrorBody, + name: "payload is empty", + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = nil }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidPayloadErrorBody, }, { name: "grant type is missing in request", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithGrantType("").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -310,7 +331,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "grant type is not authorization_code", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithGrantType("bogus").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -318,7 +339,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "client id is missing in request", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithClientID("").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -326,7 +347,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "client id is wrong", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithClientID("bogus").ReadCloser() }, wantStatus: http.StatusUnauthorized, @@ -334,7 +355,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "auth code is missing in request", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithAuthCode("").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -342,7 +363,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "auth code has never been valid", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithAuthCode("bogus").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -350,7 +371,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "auth code is invalidated", - storage: func( + modifyStorage: func( t *testing.T, s interface { oauth2.TokenRevocationStorage @@ -369,7 +390,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "redirect uri is missing in request", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithRedirectURI("").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -377,7 +398,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "redirect uri is wrong", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithRedirectURI("bogus").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -385,7 +406,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "pkce is missing in request", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithPKCE("").ReadCloser() }, wantStatus: http.StatusBadRequest, @@ -393,7 +414,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "pkce is wrong", - request: func(r *http.Request, authCode string) { + modifyTokenRequest: func(r *http.Request, authCode string) { r.Body = happyBody(authCode).WithPKCE( "bogus-verifier-that-is-at-least-43-characters-for-the-sake-of-entropy", ).ReadCloser() @@ -412,8 +433,8 @@ func TestTokenEndpoint(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { authRequest := deepCopyRequestForm(happyAuthRequest) - if test.authRequest != nil { - test.authRequest(authRequest) + if test.modifyAuthRequest != nil { + test.modifyAuthRequest(authRequest) } client := fake.NewSimpleClientset() @@ -430,24 +451,26 @@ func TestTokenEndpoint(t *testing.T) { oauthHelper, authCode, jwtSigningKey = makeHappyOauthHelper(t, authRequest, oauthStore) } - if test.storage != nil { - test.storage(t, oauthStore, authCode) + if test.modifyStorage != nil { + test.modifyStorage(t, oauthStore, authCode) } subject := NewHandler(oauthHelper) + authorizeEndpointGrantedOpenIDScope := strings.Contains(authRequest.Form.Get("scope"), "openid") + expectedNumberOfIDSessionsStored := 0 + if authorizeEndpointGrantedOpenIDScope { + expectedNumberOfIDSessionsStored = 1 + } + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 1) - if strings.Contains(authRequest.Form.Get("scope"), "openid") { - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) - } else { - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) - } + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, expectedNumberOfIDSessionsStored) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2+expectedNumberOfIDSessionsStored) req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - if test.request != nil { - test.request(req, authCode) + if test.modifyTokenRequest != nil { + test.modifyTokenRequest(req, authCode) } rsp := httptest.NewRecorder() @@ -458,29 +481,38 @@ func TestTokenEndpoint(t *testing.T) { require.Equal(t, test.wantStatus, rsp.Code) testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), "application/json") if test.wantBodyFields != nil { - var m map[string]interface{} - require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &m)) - require.ElementsMatch(t, test.wantBodyFields, getMapKeys(m)) + var parsedResponseBody map[string]interface{} + require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedResponseBody)) + require.ElementsMatch(t, test.wantBodyFields, getMapKeys(parsedResponseBody)) + + wantIDToken := contains(test.wantBodyFields, "id_token") + wantRefreshToken := contains(test.wantBodyFields, "refresh_token") code := req.PostForm.Get("code") - wantOpenidScope := contains(test.wantBodyFields, "id_token") requireInvalidAuthCodeStorage(t, code, oauthStore) - requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) + requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, wantIDToken, wantRefreshToken) requireInvalidPKCEStorage(t, code, oauthStore) - requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + requireValidOIDCStorage(t, parsedResponseBody, code, oauthStore, test.wantRequestedScopes, wantIDToken, wantRefreshToken) + + expectedNumberOfRefreshTokenSessionsStored := 0 + if wantRefreshToken { + expectedNumberOfRefreshTokenSessionsStored = 1 + } + expectedNumberOfIDSessionsStored = 0 + if wantIDToken { + expectedNumberOfIDSessionsStored = 1 + requireValidIDToken(t, parsedResponseBody, jwtSigningKey) + } + if wantRefreshToken { + requireValidRefreshTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, wantIDToken, wantRefreshToken) + } testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) - - if wantOpenidScope { - requireValidIDToken(t, m, jwtSigningKey) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) - } else { - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) - } + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, expectedNumberOfRefreshTokenSessionsStored) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, expectedNumberOfIDSessionsStored) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2+expectedNumberOfRefreshTokenSessionsStored+expectedNumberOfIDSessionsStored) } else { require.JSONEq(t, test.wantExactBody, rsp.Body.String()) } @@ -488,7 +520,9 @@ func TestTokenEndpoint(t *testing.T) { } t.Run("auth code is used twice", func(t *testing.T) { + // TODO upgrade this test to use offline_access so we can be sure that the refresh token was also revoked authRequest := deepCopyRequestForm(happyAuthRequest) + wantRequestedScopes := []string{"openid", "profile", "email"} client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets("some-namespace") @@ -513,25 +547,27 @@ func TestTokenEndpoint(t *testing.T) { testutil.RequireEqualContentType(t, rsp0.Header().Get("Content-Type"), "application/json") require.Equal(t, http.StatusOK, rsp0.Code) - var m map[string]interface{} - require.NoError(t, json.Unmarshal(rsp0.Body.Bytes(), &m)) + var parsedResponseBody map[string]interface{} + require.NoError(t, json.Unmarshal(rsp0.Body.Bytes(), &parsedResponseBody)) wantBodyFields := []string{"id_token", "access_token", "token_type", "expires_in", "scope"} - require.ElementsMatch(t, wantBodyFields, getMapKeys(m)) + require.ElementsMatch(t, wantBodyFields, getMapKeys(parsedResponseBody)) + + requireValidIDToken(t, parsedResponseBody, jwtSigningKey) code := req.PostForm.Get("code") - wantOpenidScope := true + wantGrantedOpenidScope := true + wantGrantedOfflineAccessScope := false requireInvalidAuthCodeStorage(t, code, oauthStore) - requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) + requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, wantRequestedScopes, wantGrantedOpenidScope, wantGrantedOfflineAccessScope) requireInvalidPKCEStorage(t, code, oauthStore) - requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) - requireValidIDToken(t, m, jwtSigningKey) + requireValidOIDCStorage(t, parsedResponseBody, code, oauthStore, wantRequestedScopes, wantGrantedOpenidScope, wantGrantedOfflineAccessScope) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) // Second call - should be unsuccessful since auth code was already used. @@ -546,16 +582,21 @@ func TestTokenEndpoint(t *testing.T) { testutil.RequireEqualContentType(t, rsp1.Header().Get("Content-Type"), "application/json") require.JSONEq(t, fositeReusedAuthCodeErrorBody, rsp1.Body.String()) + // this was previously invalidated by the first request, so it remains invalidated requireInvalidAuthCodeStorage(t, code, oauthStore) - requireInvalidAccessTokenStorage(t, m, oauthStore) + // now invalidated the access token that was previously handed out by the first request + requireInvalidAccessTokenStorage(t, parsedResponseBody, oauthStore) + // this was previously invalidated by the first request, so it remains invalidated requireInvalidPKCEStorage(t, code, oauthStore) - requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + // fosite never cleans these up, so it is still there + requireValidOIDCStorage(t, parsedResponseBody, code, oauthStore, wantRequestedScopes, wantGrantedOpenidScope, wantGrantedOfflineAccessScope) + // Check that the access token storage was deleted, and the number of other storage objects did not change. testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) }) } @@ -671,6 +712,9 @@ func simulateAuthEndpointHavingAlreadyRun(t *testing.T, authRequest *http.Reques if strings.Contains(authRequest.Form.Get("scope"), "openid") { authRequester.GrantScope("openid") } + if strings.Contains(authRequest.Form.Get("scope"), "offline_access") { + authRequester.GrantScope("offline_access") + } authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) require.NoError(t, err) return authResponder @@ -705,11 +749,44 @@ func requireInvalidAuthCodeStorage( require.True(t, errors.Is(err, fosite.ErrInvalidatedAuthorizeCode)) } +func requireValidRefreshTokenStorage( + t *testing.T, + body map[string]interface{}, + storage oauth2.CoreStorage, + wantRequestedScopes []string, + wantGrantedOpenidScope bool, + wantGrantedOfflineAccessScope bool, +) { + t.Helper() + + // Get the refresh token, and make sure we can use it to perform a lookup on the storage. + refreshToken, ok := body["refresh_token"] + require.True(t, ok) + refreshTokenString, ok := refreshToken.(string) + require.Truef(t, ok, "wanted refresh_token to be a string, but got %T", refreshToken) + require.NotEmpty(t, refreshTokenString) + storedRequest, err := storage.GetRefreshTokenSession(context.Background(), getFositeDataSignature(t, refreshTokenString), nil) + require.NoError(t, err) + + // Fosite stores refresh tokens without any of the original request form parameters. + requireValidStoredRequest( + t, + storedRequest, + storedRequest.Sanitize([]string{}).GetRequestForm(), + wantRequestedScopes, + wantGrantedOpenidScope, + wantGrantedOfflineAccessScope, + true, + ) +} + func requireValidAccessTokenStorage( t *testing.T, body map[string]interface{}, storage oauth2.CoreStorage, + wantRequestedScopes []string, wantGrantedOpenidScope bool, + wantGrantedOfflineAccessScope bool, ) { t.Helper() @@ -718,7 +795,8 @@ func requireValidAccessTokenStorage( require.True(t, ok) accessTokenString, ok := accessToken.(string) require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) - authRequest, err := storage.GetAccessTokenSession(context.Background(), getFositeDataSignature(t, accessTokenString), nil) + require.NotEmpty(t, accessTokenString) + storedRequest, err := storage.GetAccessTokenSession(context.Background(), getFositeDataSignature(t, accessTokenString), nil) require.NoError(t, err) // Make sure the other body fields are valid. @@ -736,24 +814,33 @@ func requireValidAccessTokenStorage( scopes, ok := body["scope"] require.True(t, ok) - scopesString, ok := scopes.(string) + actualGrantedScopesString, ok := scopes.(string) require.Truef(t, ok, "wanted scopes to be an string, but got %T", scopes) - wantScopes := "" - if wantGrantedOpenidScope { - wantScopes += "openid" - } - require.Equal(t, wantScopes, scopesString) + require.Equal(t, strings.Join(wantGrantedScopes(wantGrantedOpenidScope, wantGrantedOfflineAccessScope), " "), actualGrantedScopesString) - // Fosite stores access tokens without any of the original request form pararmeters. - requireValidAuthRequest( + // Fosite stores access tokens without any of the original request form parameters. + requireValidStoredRequest( t, - authRequest, - authRequest.Sanitize([]string{}).GetRequestForm(), + storedRequest, + storedRequest.Sanitize([]string{}).GetRequestForm(), + wantRequestedScopes, wantGrantedOpenidScope, + wantGrantedOfflineAccessScope, true, ) } +func wantGrantedScopes(wantGrantedOpenidScope, wantGrantedOfflineAccessScope bool) []string { + scopesWanted := []string{} + if wantGrantedOpenidScope { + scopesWanted = append(scopesWanted, "openid") + } + if wantGrantedOfflineAccessScope { + scopesWanted = append(scopesWanted, "offline_access") + } + return scopesWanted +} + func requireInvalidAccessTokenStorage( t *testing.T, body map[string]interface{}, @@ -788,13 +875,15 @@ func requireValidOIDCStorage( body map[string]interface{}, code string, storage openid.OpenIDConnectRequestStorage, + wantRequestedScopes []string, wantGrantedOpenidScope bool, + wantGrantedOfflineAccessScope bool, ) { t.Helper() if wantGrantedOpenidScope { // Make sure the OIDC session is still there. Note that Fosite stores OIDC sessions using the full auth code as a key. - authRequest, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) + storedRequest, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) require.NoError(t, err) // Fosite stores OIDC sessions with only the nonce in the original request form. @@ -804,11 +893,13 @@ func requireValidOIDCStorage( require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) require.NotEmpty(t, accessTokenString) - requireValidAuthRequest( + requireValidStoredRequest( t, - authRequest, - authRequest.Sanitize([]string{"nonce"}).GetRequestForm(), - true, + storedRequest, + storedRequest.Sanitize([]string{"nonce"}).GetRequestForm(), + wantRequestedScopes, + wantGrantedOpenidScope, + wantGrantedOfflineAccessScope, false, ) } else { @@ -817,34 +908,30 @@ func requireValidOIDCStorage( } } -func requireValidAuthRequest( +func requireValidStoredRequest( t *testing.T, - authRequest fosite.Requester, + request fosite.Requester, wantRequestForm url.Values, + wantRequestedScopes []string, wantGrantedOpenidScope bool, + wantGrantedOfflineAccessScope bool, wantAccessTokenExpiresAt bool, ) { t.Helper() - // Assert that the getters on the authRequest return what we think they should. - wantRequestedScopes := []string{"profile", "email"} - wantGrantedScopes := []string{} - if wantGrantedOpenidScope { - wantRequestedScopes = append([]string{"openid"}, wantRequestedScopes...) - wantGrantedScopes = append([]string{"openid"}, wantGrantedScopes...) - } - require.NotEmpty(t, authRequest.GetID()) - testutil.RequireTimeInDelta(t, authRequest.GetRequestedAt(), time.Now().UTC(), timeComparisonFudgeSeconds*time.Second) - require.Equal(t, goodClient, authRequest.GetClient().GetID()) - require.Equal(t, fosite.Arguments(wantRequestedScopes), authRequest.GetRequestedScopes()) - require.Equal(t, fosite.Arguments(wantGrantedScopes), authRequest.GetGrantedScopes()) - require.Empty(t, authRequest.GetRequestedAudience()) - require.Empty(t, authRequest.GetGrantedAudience()) - require.Equal(t, wantRequestForm, authRequest.GetRequestForm()) // Fosite stores access token request without form + // Assert that the getters on the request return what we think they should. + require.NotEmpty(t, request.GetID()) + testutil.RequireTimeInDelta(t, request.GetRequestedAt(), time.Now().UTC(), timeComparisonFudgeSeconds*time.Second) + require.Equal(t, goodClient, request.GetClient().GetID()) + require.Equal(t, fosite.Arguments(wantRequestedScopes), request.GetRequestedScopes()) + require.Equal(t, fosite.Arguments(wantGrantedScopes(wantGrantedOpenidScope, wantGrantedOfflineAccessScope)), request.GetGrantedScopes()) + require.Empty(t, request.GetRequestedAudience()) + require.Empty(t, request.GetGrantedAudience()) + require.Equal(t, wantRequestForm, request.GetRequestForm()) // Fosite stores access token request without form // Cast session to the type we think it should be. - session, ok := authRequest.GetSession().(*openid.DefaultSession) - require.Truef(t, ok, "could not cast %T to %T", authRequest.GetSession(), &openid.DefaultSession{}) + session, ok := request.GetSession().(*openid.DefaultSession) + require.Truef(t, ok, "could not cast %T to %T", request.GetSession(), &openid.DefaultSession{}) // Assert that the session claims are what we think they should be, but only if we are doing OIDC. if wantGrantedOpenidScope {