diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 046f1849..092aede5 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -230,16 +230,16 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0 } // ValidateToken mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce) (*oidctypes.Token, error) { +func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ValidateToken", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "ValidateToken", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*oidctypes.Token) ret1, _ := ret[1].(error) return ret0, ret1 } // ValidateToken indicates an expected call of ValidateToken. -func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateToken), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateToken), arg0, arg1, arg2, arg3) } diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 5f0cf5f0..2aa66f5c 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -97,7 +97,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( if err != nil { return "", "", err } - subject := downstreamSubjectFromUpstreamOIDC(upstreamIssuer, upstreamSubject) + subject := DownstreamSubjectFromUpstreamOIDC(upstreamIssuer, upstreamSubject) usernameClaimName := upstreamIDPConfig.GetUsernameClaim() if usernameClaimName == "" { @@ -176,7 +176,7 @@ func DownstreamLDAPSubject(uid string, ldapURL url.URL) string { return ldapURL.String() } -func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { +func DownstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject)) } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 5207945b..41f9e608 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -74,7 +74,7 @@ type UpstreamOIDCIdentityProviderI interface { // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated // tokens, or an error. - ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) + ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) } type UpstreamLDAPIdentityProviderI interface { @@ -103,13 +103,6 @@ type StoredRefreshAttributes struct { AdditionalAttributes map[string]string } -type StoredRefreshAttributes struct { - Username string - Subject string - DN string - AuthTime time.Time -} - type DynamicUpstreamIDPProvider interface { SetOIDCIdentityProviders(oidcIDPs []UpstreamOIDCIdentityProviderI) GetOIDCIdentityProviders() []UpstreamOIDCIdentityProviderI diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index afaee0b1..2630fadb 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -16,6 +16,7 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" + "go.pinniped.dev/internal/upstreamoidc" ) var ( @@ -88,7 +89,7 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, switch customSessionData.ProviderType { case psession.ProviderTypeOIDC: - return upstreamOIDCRefresh(ctx, customSessionData, providerCache) + return upstreamOIDCRefresh(ctx, session, providerCache) case psession.ProviderTypeLDAP: return upstreamLDAPRefresh(ctx, providerCache, session) case psession.ProviderTypeActiveDirectory: @@ -98,7 +99,8 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, } } -func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister) error { +func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister) error { + s := session.Custom if s.OIDC == nil || s.OIDC.UpstreamRefreshToken == "" { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -122,17 +124,31 @@ func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, pro // "the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token." // https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse _, hasIDTok := refreshedTokens.Extra("id_token").(string) - if hasIDTok { - // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at - // least some providers do not include one, so we skip the nonce validation here (but not other validations). - _, err = p.ValidateToken(ctx, refreshedTokens, "") + + // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at + // least some providers do not include one, so we skip the nonce validation here (but not other validations). + validatedTokens, err := p.ValidateToken(ctx, refreshedTokens, "", hasIDTok) + if err != nil { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + + claims := validatedTokens.IDToken.Claims + // if we have any claims at all, we better have a subject, and it better match the previous value. + // but it's possible that we don't because both returning a new refresh token on refresh and having a userinfo + // endpoint are optional. + if len(validatedTokens.IDToken.Claims) != 0 { + newSub := claims["sub"] + oldDownstreamSubject := session.Fosite.Claims.Subject + oldSub, err := upstreamoidc.ExtractUpstreamSubjectFromDownstream(oldDownstreamSubject) if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh returned an invalid ID token.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + "Could not verify upstream refresh subject against previous value").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + if oldSub != newSub { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Subject in upstream refresh does not match previous value").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } - } else { - plog.Debug("upstream refresh request did not return a new ID token", - "providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID) } // Upstream refresh may or may not return a new refresh token. If we got a new refresh token, then update it in diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 0c021de1..6fb6e2e8 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -22,6 +22,8 @@ import ( "testing" "time" + "go.pinniped.dev/pkg/oidcclient/oidctypes" + "github.com/ory/fosite" fositeoauth2 "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" @@ -1046,7 +1048,13 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant with openid scope granted (id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1062,7 +1070,11 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant without openid scope granted (no id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, @@ -1089,7 +1101,11 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant when the upstream refresh does not return a new ID token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1098,14 +1114,20 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), - nil, // expect ValidateToken is *not* called + refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateToken is called ), }, }, { name: "happy path refresh grant when the upstream refresh does not return a new refresh token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1121,7 +1143,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request adds a new scope to the list of requested scopes then it is ignored", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1140,7 +1168,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request removes a scope which was originally granted from the list of requested scopes then it is granted anyway", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") }, @@ -1170,7 +1204,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request does not include a scope param then it gets all the same scopes as the original authorization request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1545,7 +1585,39 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token." + "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token or UserInfo response." + } + `), + }, + }, + }, + { + name: "when the upstream refresh returns an ID token with a different subject than the original", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). + WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). + // This is the current format of the errors returned by the production code version of ValidateToken, see ValidateToken in upstreamoidc.go + WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "something-different", + }, + }, + }). + Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Subject in upstream refresh does not match previous value" } `), }, diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 88e54a73..a4f0b31e 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -83,6 +83,12 @@ type ValidateTokenArgs struct { ExpectedIDTokenNonce nonce.Nonce } +type ValidateRefreshArgs struct { + Ctx context.Context + Tok *oauth2.Token + StoredAttributes provider.StoredRefreshAttributes +} + type TestUpstreamLDAPIdentityProvider struct { Name string ResourceUID types.UID @@ -170,6 +176,8 @@ type TestUpstreamOIDCIdentityProvider struct { ValidateTokenFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) + ValidateRefreshFunc func(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error + exchangeAuthcodeAndValidateTokensCallCount int exchangeAuthcodeAndValidateTokensArgs []*ExchangeAuthcodeAndValidateTokenArgs passwordCredentialsGrantAndValidateTokensCallCount int @@ -180,6 +188,8 @@ type TestUpstreamOIDCIdentityProvider struct { revokeRefreshTokenArgs []*RevokeRefreshTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs + validateRefreshCallCount int + validateRefreshArgs []*ValidateRefreshArgs } var _ provider.UpstreamOIDCIdentityProviderI = &TestUpstreamOIDCIdentityProvider{} @@ -278,6 +288,19 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefresh(ctx context.Context, r return u.PerformRefreshFunc(ctx, refreshToken) } +func (u *TestUpstreamOIDCIdentityProvider) ValidateRefresh(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error { + if u.validateRefreshArgs == nil { + u.validateRefreshArgs = make([]*ValidateRefreshArgs, 0) + } + u.validateRefreshCallCount++ + u.validateRefreshArgs = append(u.validateRefreshArgs, &ValidateRefreshArgs{ + Ctx: ctx, + Tok: tok, + StoredAttributes: storedAttributes, + }) + return u.ValidateRefreshFunc(ctx, tok, storedAttributes) +} + func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshToken(ctx context.Context, refreshToken string) error { if u.revokeRefreshTokenArgs == nil { u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) @@ -312,7 +335,7 @@ func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *Rev return u.revokeRefreshTokenArgs[call] } -func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { +func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { if u.validateTokenArgs == nil { u.validateTokenArgs = make([]*ValidateTokenArgs, 0) } diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index abc2fd8e..0ac4dbd5 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -13,6 +13,7 @@ import ( "net/http" "net/url" "strings" + "time" coreosoidc "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" @@ -113,7 +114,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C // There is no nonce to validate for a resource owner password credentials grant because it skips using // the authorize endpoint and goes straight to the token endpoint. const skipNonceValidation nonce.Nonce = "" - return p.ValidateToken(ctx, tok, skipNonceValidation) + return p.ValidateToken(ctx, tok, skipNonceValidation, true) } func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { @@ -127,10 +128,9 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return nil, err } - return p.ValidateToken(ctx, tok, expectedIDTokenNonce) + return p.ValidateToken(ctx, tok, expectedIDTokenNonce, true) } -// TODO this is reused between the client and the supervisor... don't change it. func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { // Use the provided HTTP client to benefit from its CA, proxy, and other settings. httpClientContext := coreosoidc.ClientContext(ctx, p.Client) @@ -238,88 +238,54 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } } -func (p *ProviderConfig) ValidateRefresh(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error { - idTok, hasIDTok := tok.Extra("id_token").(string) - var validatedClaims = make(map[string]interface{}) - if hasIDTok { - coreosConfig := &coreosoidc.Config{ClientID: p.GetClientID()} - coreosClientContext := coreosoidc.ClientContext(ctx, p.Client) - verifier := p.Provider.Verifier(coreosConfig) - validated, err := verifier.Verify(coreosClientContext, idTok) - if err != nil { - return httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - if err := validated.Claims(&validatedClaims); err != nil { - return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) - } - maybeLogClaims("claims from ID token", p.Name, validatedClaims) - } - - originalUpstreamSubject, err := extractUpstreamSubjectFromDownstream(storedAttributes.Subject) - if err != nil { - return httperr.Wrap(http.StatusInternalServerError, "could not parse stored subject", err) - } - - // it's okay to not have an id token. It's okay to have an id token with a subject. - // but if we have an id token without a subject that's a problem. - idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) - switch { - case len(idTokenSubject) > 0: - // TODO url escape - if url.QueryEscape(idTokenSubject) != originalUpstreamSubject { - return httperr.Newf(http.StatusInternalServerError, "subject from id token did not match previous stored value. New subject: %s. Old subject: %s", idTokenSubject, originalUpstreamSubject) - } - case len(validatedClaims) == 0: - validatedClaims[oidc.IDTokenSubjectClaim] = originalUpstreamSubject - default: - return httperr.New(http.StatusInternalServerError, "id token did not have a subject") - } - - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { - return err - } - - return nil -} - -func extractUpstreamSubjectFromDownstream(downstreamSubject string) (string, error) { +func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, error) { if !strings.Contains(downstreamSubject, "?sub=") { return "", errors.New("downstream subject did not contain original upstream subject") } - return strings.Split(downstreamSubject, "?sub=")[1], nil + return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil // TODO test for ?sub= occurring twice (imagine if you ran the supervisor with another supervisor as the upstream idp...) } // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. -func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { +// TODO check: +// - whether the userinfo response must exist (maybe just based on whether there is a refresh token??? +// - -> for the next story: userinfo has to exist but only if there isn't a refresh token. +func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { + var validatedClaims = make(map[string]interface{}) + idTok, hasIDTok := tok.Extra("id_token").(string) - if !hasIDTok { - return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") - } - validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) - if err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - if validated.AccessTokenHash != "" { - if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { + var idTokenExpiry time.Time + // if we require the id token, make sure we have it. + // also, if it exists but wasn't required, still make sure it passes these checks. + // nolint:nestif + if hasIDTok || requireIDToken { + if !hasIDTok { + return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") + } + validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) + if err != nil { return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) } - } - if expectedIDTokenNonce != "" { - if err := expectedIDTokenNonce.Validate(validated); err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + if validated.AccessTokenHash != "" { + if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { + return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } } + if expectedIDTokenNonce != "" { + if err := expectedIDTokenNonce.Validate(validated); err != nil { + return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + } + } + if err := validated.Claims(&validatedClaims); err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) + } + maybeLogClaims("claims from ID token", p.Name, validatedClaims) + idTokenExpiry = validated.Expiry } - - var validatedClaims map[string]interface{} - if err := validated.Claims(&validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) - } - maybeLogClaims("claims from ID token", p.Name, validatedClaims) - idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) - if len(idTokenSubject) > 0 { - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { + + if len(idTokenSubject) > 0 || !requireIDToken { + if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) } } @@ -335,18 +301,13 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e }, IDToken: &oidctypes.IDToken{ Token: idTok, - Expiry: metav1.NewTime(validated.Expiry), + Expiry: metav1.NewTime(idTokenExpiry), Claims: validatedClaims, }, }, nil } -func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}) error { - // TODO separate this. - // extract: fetching userinfo - // - validate some userinfo? subject stuff: for refresh subjects must match but also match stored subject - // - extract: merging claims - // - deciding when to do each of those things +func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) error { idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) userInfo, err := p.fetchUserInfo(ctx, tok) @@ -357,8 +318,9 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t return nil } - // TODO if there is no idTokenSubject, defer to checking the stored claims. // The sub (subject) Claim MUST always be returned in the UserInfo Response. + // However there may not be an id token. If there is an ID token, we must + // check it against the userinfo's subject. // // NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not // guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in @@ -366,14 +328,29 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t // the UserInfo Response values MUST NOT be used. // // http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse - if (len(idTokenSubject) > 0) && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { + checkIDToken := requireIDToken || len(idTokenSubject) > 0 + if checkIDToken && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { return httperr.Newf(http.StatusUnprocessableEntity, "userinfo 'sub' claim (%s) did not match id_token 'sub' claim (%s)", userInfo.Subject, idTokenSubject) } + if !checkIDToken { + claims["sub"] = userInfo.Subject // do this so other validations can check this subject later + } + + idTokenIssuer := claims["iss"] + // merge existing claims with user info claims if err := userInfo.Claims(&claims); err != nil { return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal user info claims", err) } + // The OIDC spec for user info response does not make any guarantees about the iss claim's existence or validity: + // "If signed, the UserInfo Response SHOULD contain the Claims iss (issuer) and aud (audience) as members. The iss value SHOULD be the OP's Issuer Identifier URL." + // See https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse + // So we just ignore it and use it the version from the id token, which has stronger guarantees. + delete(claims, "iss") + if idTokenIssuer != nil { + claims["iss"] = idTokenIssuer + } maybeLogClaims("claims from ID token and userinfo", p.Name, claims) diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 30f1318a..949a56d7 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -16,8 +16,6 @@ import ( "time" "unsafe" - "go.pinniped.dev/internal/oidc/provider" - "github.com/coreos/go-oidc/v3/oidc" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" @@ -591,82 +589,292 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("ValidateRefresh", func(t *testing.T) { + t.Run("ValidateToken", func(t *testing.T) { + expiryTime := time.Now().Add(42 * time.Second) testTokenWithoutIDToken := &oauth2.Token{ AccessToken: "test-access-token", // the library sets the original refresh token into the result, even though the server did not return that RefreshToken: "test-initial-refresh-token", TokenType: "test-token-type", - Expiry: time.Now().Add(42 * time.Second), + Expiry: expiryTime, } + // generated from jwt.io + // sub: some-subject + // iss: some-issuer + // nonce: some-nonce + goodIDToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJub25jZSI6InNvbWUtbm9uY2UiLCJpc3MiOiJzb21lLWlzc3VlciJ9.eGvzOihLUqzn3M4k6fHsToedgy7Fu89_Xu_u4mwMgRlIyRWZqmEMV76RVLnZd9Ihm9j_VpvrpirIkaj4JM9eRNfLX1n328cmBivBwnTKAzHuTm17dUKO5EvdTmQzmwnN0WZ8nWk4GfR7RzcvE1V8G9tIiWD8FkO3Dr-NR_zTun3N37onAazVLCmF0SDtATDfUH1ETqviHEp8xGx5HD5mv5T3HEjOuer5gxTEnfncef0LurBH3po-C0tXHKu74PD8x88CMJ1DLsRdCalnctwa850slKPkBSTP-ssh0JVg7cdMXoosVpwiXtKYaBkrhu8VS018aFP-cBbW0mYwsHmt3g" //nolint:gosec tests := []struct { name string - token *oauth2.Token - storedAttributes provider.StoredRefreshAttributes + tok *oauth2.Token + nonce nonce.Nonce + requireIDToken bool userInfo *oidc.UserInfo rawClaims []byte userInfoErr error wantErr string + wantMergedTokens *oidctypes.Token }{ { - name: "id, access and refresh token returned from refresh", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + name: "token with id, access and refresh tokens, valid nonce, and no userinfo", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + }, + }, + }, }, { - name: "id token exists but has no subject", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.FmkkY7MCnhD98F5Aupq_YdW54pH90O60O9SwjxWHZb6_k6LoJcjZEMyxba1Fg5tlGLOSqH7BffZOfMUZ5YqVeA7ZIZ9fhAiudGdJqKz5nI394xzR-EejGqBI0fTAuxa_0VsLSC5mV0hTctK01kvemPwMLhIUn_HHr0-khugPFZH8zlYxAFmBpe9YxWK5w1JcTWuSMjRn6_b1RDSgxpff-LcmvVz6akcnlb2j8Od7S9JinsSqKjb7zBjQmcVGT6BGdN43tsBJMEOzil6xpvsH_KIToWlyqmvCQFp_udcKWuUNNLdypGW4eMXvPF5GINMDhki0GZDoKLkPnHVk_mZFXw"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: &oidc.UserInfo{Subject: "some-subject"}, - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "id token did not have a subject", + name: "id token not required but is provided", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "id token doesn't exist but userinfo does", - token: testTokenWithoutIDToken, - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + name: "token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "subject in id token doesn't match stored subject", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"}, - wantErr: "subject from id token did not match previous stored value. New subject: some-subject. Old subject: some-other-subject", + name: "claims from userinfo override id token claims", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA", + Claims: map[string]interface{}{ + "iss": "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable. + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "malformed stored subject", - token: testTokenWithoutIDToken, - storedAttributes: provider.StoredRefreshAttributes{Subject: "wrong-format"}, - wantErr: "could not parse stored subject: downstream subject did not contain original upstream subject", + name: "token with id, access and refresh tokens and valid nonce, but userinfo has a different issuer", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable. + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "subject in user info doesn't match stored subject", - token: testTokenWithoutIDToken, - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"}, - userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "userinfo 'sub' claim (some-subject) did not match id_token 'sub' claim (some-other-subject)", // TODO is this a good enough error message?... + name: "token with no id token but valid userinfo", + tok: testTokenWithoutIDToken, + nonce: "", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "", + Claims: map[string]interface{}{ + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "subject in id token doesn't match userinfo", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.P_97A2ONBt-YV0JHM3DDugNhG-codkK8fLU4EOdlGHcVBmWVbbW1g7-U0jjB9gcvUt24TS5z0TxEnKNUG8LiqS-8FbHGjv8DN1gAz4huBxNqmMYwzGsFNKDBcUMpv9DNFT-oIrxSf3pPYFlg7N0YKzbMPRwVaL1iQmzeQDCQieV3mDCWXpxpdLa1Mfj_NRsEe4027xOZYJn14pn83h92rG0VcoV2e39LuuEir-tRtDniY4WFIMPudNJdC0NwoMoRIQuHStCIN5AkbHPuhGQivqoObWX6RgNU18qb6CcCi86WhG45uHO77gfz5TbGsyVhr5LRBEZP5HPqsq5D6853zQ"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: &oidc.UserInfo{Subject: "some-other-subject"}, - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + name: "token with neither id token nor userinfo", + tok: testTokenWithoutIDToken, + nonce: "", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }, }, { - name: "id token with format that isn't a jwt returned from refresh", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-jwt-format"}), - userInfo: &oidc.UserInfo{Subject: "test-user-2"}, - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + name: "token with id, access and refresh tokens, valid nonce, and userinfo subject that doesn't match", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "id token not required but is provided, and subjects don't match", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "invalid id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-an-id-token"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + }, + { + name: "invalid nonce", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received ID token with invalid nonce: invalid nonce (expected \"some-other-nonce\", got \"some-nonce\")", + }, + { + name: "expected to have id token, but doesn't", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received response missing ID token", + }, + { + name: "mismatched access token hash", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received response missing ID token", + }, + { + name: "id token missing subject, skip userinfo check", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ", + Claims: map[string]interface{}{ + "iss": "some-issuer", + "name": "John Doe", + "nonce": "some-nonce", + }, + }, + }, }, } + for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { @@ -690,13 +898,13 @@ func TestProviderConfig(t *testing.T) { userInfoErr: tt.userInfoErr, }, } - - err := p.ValidateRefresh(context.Background(), tt.token, tt.storedAttributes) + gotTok, err := p.ValidateToken(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) if tt.wantErr != "" { require.Error(t, err) - require.Equal(t, err.Error(), tt.wantErr) + require.Equal(t, tt.wantErr, err.Error()) } else { require.NoError(t, err) + require.Equal(t, tt.wantMergedTokens, gotTok) } }) } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 470ed3dd..941693e2 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -822,7 +822,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least // some providers do not include one, so we skip the nonce validation here (but not other validations). - return upstreamOIDCIdentityProvider.ValidateToken(ctx, refreshed, "") + return upstreamOIDCIdentityProvider.ValidateToken(ctx, refreshed, "", true) } func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 2bf4620c..e55f8e03 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -406,7 +406,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). @@ -453,7 +453,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(nil, fmt.Errorf("some validation error")) mock.EXPECT(). PerformRefresh(gomock.Any(), "test-refresh-token-returning-invalid-id-token"). @@ -1648,7 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token).