From d91baba240636bac09a199dcaf18e6af31fd8d2c Mon Sep 17 00:00:00 2001 From: Aram Price Date: Mon, 7 Dec 2020 17:22:34 -0800 Subject: [PATCH 1/4] authorize and callback endpoints now handle the offline_access scope - This is in preparation for the token endpoint to support the refresh grant Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 14 ++--- internal/oidc/auth/auth_handler_test.go | 22 +++++++- internal/oidc/callback/callback_handler.go | 14 ++--- .../oidc/callback/callback_handler_test.go | 56 ++++++++++++++----- internal/oidc/nullstorage_test.go | 2 +- internal/oidc/oidc.go | 11 +++- 6 files changed, 83 insertions(+), 36 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 966bfb54..1998b72a 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -9,6 +9,7 @@ import ( "net/http" "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -57,7 +58,10 @@ func NewHandler( } // Grant the openid scope (for now) if they asked for it so that `NewAuthorizeResponse` will perform its OIDC validations. - grantOpenIDScopeIfRequested(authorizeRequester) + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) + // There don't seem to be any validations inside `NewAuthorizeResponse` related to the offline_access scope + // at this time, however we will temporarily grant the scope just in case that changes in a future release of fosite. + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) now := time.Now() _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{ @@ -148,14 +152,6 @@ func readCSRFCookie(r *http.Request, codec oidc.Codec) csrftoken.CSRFToken { return csrfFromCookie } -func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) { - for _, scope := range authorizeRequester.GetRequestedScopes() { - if scope == "openid" { - authorizeRequester.GrantScope(scope) - } - } -} - func chooseUpstreamIDP(idpListGetter oidc.IDPListGetter) (provider.UpstreamOIDCIdentityProviderI, error) { allUpstreamIDPs := idpListGetter.GetIDPList() if len(allUpstreamIDPs) == 0 { diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 30a8dc5d..0175c64c 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -119,7 +119,7 @@ func TestAuthorizationEndpoint(t *testing.T) { Name: "some-idp", ClientID: "some-client-id", AuthorizationURL: *upstreamAuthURL, - Scopes: []string{"scope1", "scope2"}, + Scopes: []string{"scope1", "scope2"}, // the scopes to request when starting the upstream authorization flow } // Configure fosite the same way that the production code would, using NullStorage to turn off storage. @@ -372,6 +372,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "happy path when downstream requested scopes include offline_access", + issuer: downstreamIssuer, + idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"scope": "openid offline_access"}), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ + "scope": "openid offline_access", + }, "", "")), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "downstream redirect uri does not match what is configured for client", issuer: downstreamIssuer, diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index e102dc62..3c85ee4c 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -11,6 +11,7 @@ import ( "net/url" "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -70,8 +71,9 @@ func NewHandler( return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } - // Grant the openid scope only if it was requested. - grantOpenIDScopeIfRequested(authorizeRequester) + // Automatically grant the openid and offline_access scopes, but only if they were requested. + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) token, err := upstreamIDPConfig.ExchangeAuthcodeAndValidateTokens( r.Context(), @@ -189,14 +191,6 @@ func readState(r *http.Request, stateDecoder oidc.Decoder) (*oidc.UpstreamStateP return &state, nil } -func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) { - for _, scope := range authorizeRequester.GetRequestedScopes() { - if scope == "openid" { - authorizeRequester.GrantScope(scope) - } - } -} - func getUsernameFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index ed4c6b01..67857f6a 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -66,7 +66,8 @@ const ( var ( upstreamGroupMembership = []string{"test-pinniped-group-0", "test-pinniped-group-1"} - happyDownstreamScopesRequested = []string{"openid", "profile", "email"} + happyDownstreamScopesRequested = []string{"openid"} + happyDownstreamScopesGranted = []string{"openid"} happyDownstreamRequestParamsQuery = url.Values{ "response_type": []string{"code"}, @@ -127,7 +128,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus int wantBody string wantRedirectLocationRegexp string - wantGrantedOpenidScope bool + wantDownstreamGrantedScopes []string wantDownstreamIDTokenSubject string wantDownstreamIDTokenGroups []string wantDownstreamRequestedScopes []string @@ -145,11 +146,11 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, - wantGrantedOpenidScope: true, wantBody: "", wantDownstreamIDTokenSubject: upstreamUsername, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -163,11 +164,11 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, - wantGrantedOpenidScope: true, wantBody: "", wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamIDTokenGroups: nil, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -181,11 +182,11 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, - wantGrantedOpenidScope: true, wantBody: "", wantDownstreamIDTokenSubject: upstreamSubject, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -316,6 +317,28 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, + { + name: "state's downstream auth params also included offline_access scope", + idp: happyUpstream().Build(), + method: http.MethodGet, + path: newRequestPath(). + WithState( + happyUpstreamStateParam(). + WithAuthorizeRequestParams(shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, map[string]string{"scope": "openid offline_access"}).Encode()). + Build(t, happyStateCodec), + ).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusFound, + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid%20offline_access&state=` + happyDownstreamState, + wantDownstreamIDTokenSubject: upstreamUsername, + wantDownstreamRequestedScopes: []string{"openid", "offline_access"}, + wantDownstreamGrantedScopes: []string{"openid", "offline_access"}, + wantDownstreamIDTokenGroups: upstreamGroupMembership, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, + }, { name: "the UpstreamOIDCProvider CRD has been deleted", idp: otherUpstreamOIDCIdentityProvider, @@ -481,7 +504,7 @@ func TestCallbackEndpoint(t *testing.T) { // Several Secrets should have been created expectedNumberOfCreatedSecrets := 2 - if test.wantGrantedOpenidScope { + if includesOpenIDScope(test.wantDownstreamGrantedScopes) { expectedNumberOfCreatedSecrets++ } require.Len(t, client.Actions(), expectedNumberOfCreatedSecrets) @@ -493,7 +516,7 @@ func TestCallbackEndpoint(t *testing.T) { t, oauthStore, authcodeDataAndSignature[1], // Authcode store key is authcode signature - test.wantGrantedOpenidScope, + test.wantDownstreamGrantedScopes, test.wantDownstreamIDTokenSubject, test.wantDownstreamIDTokenGroups, test.wantDownstreamRequestedScopes, @@ -513,7 +536,7 @@ func TestCallbackEndpoint(t *testing.T) { ) // One IDSession should have been stored, if the downstream actually requested the "openid" scope - if test.wantGrantedOpenidScope { + if includesOpenIDScope(test.wantDownstreamGrantedScopes) { testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) validateIDSessionStorage( @@ -530,6 +553,15 @@ func TestCallbackEndpoint(t *testing.T) { } } +func includesOpenIDScope(scopes []string) bool { + for _, scope := range scopes { + if scope == "openid" { + return true + } + } + return false +} + type requestPath struct { code, state *string } @@ -704,7 +736,7 @@ func validateAuthcodeStorage( t *testing.T, oauthStore *oidc.KubeStorage, storeKey string, - wantGrantedOpenidScope bool, + wantDownstreamGrantedScopes []string, wantDownstreamIDTokenSubject string, wantDownstreamIDTokenGroups []string, wantDownstreamRequestedScopes []string, @@ -719,11 +751,7 @@ func validateAuthcodeStorage( storedRequestFromAuthcode, storedSessionFromAuthcode := castStoredAuthorizeRequest(t, storedAuthorizeRequestFromAuthcode) // Check which scopes were granted. - if wantGrantedOpenidScope { - require.Contains(t, storedRequestFromAuthcode.GetGrantedScopes(), "openid") - } else { - require.NotContains(t, storedRequestFromAuthcode.GetGrantedScopes(), "openid") - } + require.ElementsMatch(t, wantDownstreamGrantedScopes, storedRequestFromAuthcode.GetGrantedScopes()) // Check all the other fields of the stored request. require.NotEmpty(t, storedRequestFromAuthcode.ID) diff --git a/internal/oidc/nullstorage_test.go b/internal/oidc/nullstorage_test.go index 2983a800..d0c49b1e 100644 --- a/internal/oidc/nullstorage_test.go +++ b/internal/oidc/nullstorage_test.go @@ -28,7 +28,7 @@ func TestNullStorage_GetClient(t *testing.T) { RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, GrantTypes: []string{"authorization_code"}, - Scopes: []string{"openid", "profile", "email"}, + Scopes: []string{"openid", "offline_access", "profile", "email"}, }, TokenEndpointAuthMethod: "none", }, diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 7c550751..10d633aa 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -7,6 +7,7 @@ package oidc import ( "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/compose" @@ -84,7 +85,7 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, GrantTypes: []string{"authorization_code"}, - Scopes: []string{"openid", "profile", "email"}, + Scopes: []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, "profile", "email"}, }, TokenEndpointAuthMethod: "none", } @@ -156,3 +157,11 @@ func FositeErrorForLog(err error) []interface{} { type IDPListGetter interface { GetIDPList() []provider.UpstreamOIDCIdentityProviderI } + +func GrantScopeIfRequested(authorizeRequester fosite.AuthorizeRequester, scopeName string) { + for _, scope := range authorizeRequester.GetRequestedScopes() { + if scope == scopeName { + authorizeRequester.GrantScope(scope) + } + } +} From 8d2b8ae6b5df2b53ed6dabc57e57bcaa3054fce3 Mon Sep 17 00:00:00 2001 From: aram price Date: Tue, 8 Dec 2020 10:46:05 -0800 Subject: [PATCH 2/4] Use constants for scope values --- cmd/pinniped/cmd/login_oidc.go | 3 ++- pkg/oidcclient/login.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 753a227b..9070adf2 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" + "github.com/coreos/go-oidc" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" @@ -49,7 +50,7 @@ func oidcLoginCommand(loginFunc func(issuer string, clientID string, opts ...oid cmd.Flags().StringVar(&issuer, "issuer", "", "OpenID Connect issuer URL.") cmd.Flags().StringVar(&clientID, "client-id", "", "OpenID Connect client ID.") cmd.Flags().Uint16Var(&listenPort, "listen-port", 0, "TCP port for localhost listener (authorization code flow only).") - cmd.Flags().StringSliceVar(&scopes, "scopes", []string{"offline_access", "openid"}, "OIDC scopes to request during login.") + cmd.Flags().StringSliceVar(&scopes, "scopes", []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID}, "OIDC scopes to request during login.") cmd.Flags().BoolVar(&skipBrowser, "skip-browser", false, "Skip opening the browser (just print the URL).") cmd.Flags().StringVar(&sessionCachePath, "session-cache", filepath.Join(mustGetConfigDir(), "sessions.yaml"), "Path to session cache file.") cmd.Flags().StringSliceVar(&caBundlePaths, "ca-bundle", nil, "Path to TLS certificate authority bundle (PEM format, optional, can be repeated).") diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 510bd63f..62c380c9 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -174,7 +174,7 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er issuer: issuer, clientID: clientID, listenAddr: "localhost:0", - scopes: []string{"offline_access", "openid", "email", "profile"}, + scopes: []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID, "email", "profile"}, cache: &nopCache{}, callbackPath: "/callback", ctx: context.Background(), From c090eb6a6231f0bf83ebbaed2eb1f97204f44842 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 8 Dec 2020 11:47:39 -0800 Subject: [PATCH 3/4] 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 { From 18d90a727ea4c812a39591cf968decab688e8529 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 8 Dec 2020 12:12:55 -0800 Subject: [PATCH 4/4] token_handler_test.go: refresh token gets deleted when authcode reused --- internal/oidc/token/token_handler_test.go | 25 ++++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index d2a49e15..c6c1bcc8 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -520,9 +520,13 @@ 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"} + authRequest.Form.Set("scope", "openid offline_access profile email") + + wantRequestedScopes := []string{"openid", "offline_access", "profile", "email"} + wantBodyFields := []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"} + wantGrantedOpenidScope := true + wantGrantedOfflineAccessScope := true client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets("some-namespace") @@ -550,14 +554,11 @@ func TestTokenEndpoint(t *testing.T) { 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(parsedResponseBody)) requireValidIDToken(t, parsedResponseBody, jwtSigningKey) code := req.PostForm.Get("code") - wantGrantedOpenidScope := true - wantGrantedOfflineAccessScope := false requireInvalidAuthCodeStorage(t, code, oauthStore) requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, wantRequestedScopes, wantGrantedOpenidScope, wantGrantedOfflineAccessScope) requireInvalidPKCEStorage(t, code, oauthStore) @@ -566,9 +567,9 @@ func TestTokenEndpoint(t *testing.T) { 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: refreshtoken.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 4) // Second call - should be unsuccessful since auth code was already used. // @@ -582,16 +583,16 @@ 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 + // This was previously invalidated by the first request, so it remains invalidated requireInvalidAuthCodeStorage(t, code, oauthStore) - // now invalidated the access token that was previously handed out by the first request + // Has 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 + // This was previously invalidated by the first request, so it remains invalidated requireInvalidPKCEStorage(t, code, oauthStore) - // fosite never cleans these up, so it is still there + // Fosite never cleans up OpenID Connect session storage, 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. + // Check that the access token and refresh token storage were both 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)