Refactor validatetoken to handle refresh case without id token

Signed-off-by: Margo Crawford <margaretc@vmware.com>

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)
 }

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))
 }
 

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

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
+	// 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).
+	// least some providers do not include one, so we skip the nonce validation here (but not other validations).
-		_, err = p.ValidateToken(ctx, refreshedTokens, "")
+	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

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"
 						}
 					`),
 				},

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)
 	}

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 {
+func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, 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) {
 	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 {
+	var idTokenExpiry time.Time
-		return nil, httperr.New(http.StatusBadRequest, "received response missing ID token")
+	// 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.
-	validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok)
+	// nolint:nestif
-	if err != nil {
+	if hasIDTok || requireIDToken {
-		return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err)
+		if !hasIDTok {
-	}
+			return nil, httperr.New(http.StatusBadRequest, "received response missing ID token")
-	if validated.AccessTokenHash != "" {
+		}
-		if err := validated.VerifyAccessToken(tok.AccessToken); err != nil {
+		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 expectedIDTokenNonce != "" {
+			if err := validated.VerifyAccessToken(tok.AccessToken); err != nil {
-		if err := expectedIDTokenNonce.Validate(validated); err != nil {
+				return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err)
-			return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", 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 {
+func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) 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
 	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)
 

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
+			tok              *oauth2.Token
-			storedAttributes provider.StoredRefreshAttributes
+			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",
+				name:           "token with id, access and refresh tokens, valid nonce, and no userinfo",
-				token:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}),
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"},
+				nonce:          "some-nonce",
-				userInfo:         forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`),
+				requireIDToken: true,
-				rawClaims:        []byte(`{"userinfo_endpoint": "not-empty"}`),
+				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",
+				name:           "id token not required but is provided",
-				token:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.FmkkY7MCnhD98F5Aupq_YdW54pH90O60O9SwjxWHZb6_k6LoJcjZEMyxba1Fg5tlGLOSqH7BffZOfMUZ5YqVeA7ZIZ9fhAiudGdJqKz5nI394xzR-EejGqBI0fTAuxa_0VsLSC5mV0hTctK01kvemPwMLhIUn_HHr0-khugPFZH8zlYxAFmBpe9YxWK5w1JcTWuSMjRn6_b1RDSgxpff-LcmvVz6akcnlb2j8Od7S9JinsSqKjb7zBjQmcVGT6BGdN43tsBJMEOzil6xpvsH_KIToWlyqmvCQFp_udcKWuUNNLdypGW4eMXvPF5GINMDhki0GZDoKLkPnHVk_mZFXw"}),
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"},
+				nonce:          "some-nonce",
-				userInfo:         &oidc.UserInfo{Subject: "some-subject"},
+				requireIDToken: false,
-				rawClaims:        []byte(`{"userinfo_endpoint": "not-empty"}`),
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
-				wantErr:          "id token did not have a subject",
+				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",
+				name:           "token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token",
-				token:            testTokenWithoutIDToken,
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"},
+				nonce:          "some-nonce",
-				userInfo:         forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`),
+				requireIDToken: true,
-				rawClaims:        []byte(`{"userinfo_endpoint": "not-empty"}`),
+				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",
+				name:           "claims from userinfo override id token claims",
-				token:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}),
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}),
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"},
+				nonce:          "some-nonce",
-				wantErr:          "subject from id token did not match previous stored value. New subject: some-subject. Old subject: some-other-subject",
+				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",
+				name:           "token with id, access and refresh tokens and valid nonce, but userinfo has a different issuer",
-				token:            testTokenWithoutIDToken,
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "wrong-format"},
+				nonce:          "some-nonce",
-				wantErr:          "could not parse stored subject: downstream subject did not contain original upstream subject",
+				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",
+				name:           "token with no id token but valid userinfo",
-				token:            testTokenWithoutIDToken,
+				tok:            testTokenWithoutIDToken,
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"},
+				nonce:          "",
-				userInfo:         forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`),
+				requireIDToken: false,
-				rawClaims:        []byte(`{"userinfo_endpoint": "not-empty"}`),
+				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?...
+				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",
+				name:           "token with neither id token nor userinfo",
-				token:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.P_97A2ONBt-YV0JHM3DDugNhG-codkK8fLU4EOdlGHcVBmWVbbW1g7-U0jjB9gcvUt24TS5z0TxEnKNUG8LiqS-8FbHGjv8DN1gAz4huBxNqmMYwzGsFNKDBcUMpv9DNFT-oIrxSf3pPYFlg7N0YKzbMPRwVaL1iQmzeQDCQieV3mDCWXpxpdLa1Mfj_NRsEe4027xOZYJn14pn83h92rG0VcoV2e39LuuEir-tRtDniY4WFIMPudNJdC0NwoMoRIQuHStCIN5AkbHPuhGQivqoObWX6RgNU18qb6CcCi86WhG45uHO77gfz5TbGsyVhr5LRBEZP5HPqsq5D6853zQ"}),
+				tok:            testTokenWithoutIDToken,
-				storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"},
+				nonce:          "",
-				userInfo:         &oidc.UserInfo{Subject: "some-other-subject"},
+				requireIDToken: false,
-				rawClaims:        []byte(`{"userinfo_endpoint": "not-empty"}`),
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
-				wantErr:          "userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)",
+				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",
+				name:           "token with id, access and refresh tokens, valid nonce, and userinfo subject that doesn't match",
-				token:     testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-jwt-format"}),
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
-				userInfo:  &oidc.UserInfo{Subject: "test-user-2"},
+				nonce:          "some-nonce",
-				rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`),
+				requireIDToken: true,
-				wantErr:   "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts",
+				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,
 					},
 				}
-
+				gotTok, err := p.ValidateToken(context.Background(), tt.tok, tt.nonce, tt.requireIDToken)
-				err := p.ValidateRefresh(context.Background(), tt.token, tt.storedAttributes)
 				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)
 				}
 			})
 		}

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) {

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).