diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 092aede5..2a2a689e 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -14,11 +14,12 @@ import ( reflect "reflect" gomock "github.com/golang/mock/gomock" + oauth2 "golang.org/x/oauth2" + types "k8s.io/apimachinery/pkg/types" + nonce "go.pinniped.dev/pkg/oidcclient/nonce" oidctypes "go.pinniped.dev/pkg/oidcclient/oidctypes" pkce "go.pinniped.dev/pkg/oidcclient/pkce" - oauth2 "golang.org/x/oauth2" - types "k8s.io/apimachinery/pkg/types" ) // MockUpstreamOIDCIdentityProviderI is a mock of UpstreamOIDCIdentityProviderI interface. @@ -230,9 +231,9 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0 } // ValidateToken mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { +func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(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, arg3) + ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*oidctypes.Token) ret1, _ := ret[1].(error) return ret0, ret1 @@ -241,5 +242,5 @@ func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, // ValidateToken indicates an expected call of ValidateToken. 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, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3) } diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 2aa66f5c..5f0cf5f0 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 41f9e608..9986f018 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, requireIDToken bool) (*oidctypes.Token, error) + ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) } type UpstreamLDAPIdentityProviderI interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 9ec64dc5..b1888a18 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -128,7 +128,7 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, // 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) + validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(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)) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index b53987a4..24fea4ad 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -982,7 +982,6 @@ func TestRefreshGrant(t *testing.T) { want := happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(wantCustomSessionDataStored) // Should always try to perform an upstream refresh. want.wantUpstreamRefreshCall = happyOIDCUpstreamRefreshCall() - // Should only try to ValidateToken when there was an id token returned by the upstream refresh. if expectToValidateToken != nil { want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken) } @@ -1137,7 +1136,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), - refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateToken is called + refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateTokenAndMergeWithUserInfo is called ), }, }, @@ -1592,7 +1591,7 @@ func TestRefreshGrant(t *testing.T) { name: "when the upstream refresh returns an invalid ID token during the refresh request", 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 + // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go WithValidateTokenError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))). Build()), authcodeExchange: authcodeExchangeInputs{ @@ -1618,7 +1617,7 @@ func TestRefreshGrant(t *testing.T) { 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 + // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index a4f0b31e..aa682081 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -176,8 +176,6 @@ 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 @@ -188,8 +186,6 @@ type TestUpstreamOIDCIdentityProvider struct { revokeRefreshTokenArgs []*RevokeRefreshTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs - validateRefreshCallCount int - validateRefreshArgs []*ValidateRefreshArgs } var _ provider.UpstreamOIDCIdentityProviderI = &TestUpstreamOIDCIdentityProvider{} @@ -288,19 +284,6 @@ 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) @@ -335,7 +318,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, requireIDToken bool) (*oidctypes.Token, error) { +func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { if u.validateTokenArgs == nil { u.validateTokenArgs = make([]*ValidateTokenArgs, 0) } @@ -556,10 +539,10 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken( } } require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, - "should have been exactly one call to ValidateToken() by all OIDC upstreams", + "should have been exactly one call to ValidateTokenAndMergeWithUserInfo() by all OIDC upstreams", ) require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, - "ValidateToken() was called on the wrong OIDC upstream", + "ValidateTokenAndMergeWithUserInfo() was called on the wrong OIDC upstream", ) require.Equal(t, expectedArgs, actualArgs) } @@ -571,7 +554,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenCallCount } require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, - "expected exactly zero calls to ValidateToken()", + "expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()", ) } diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 53a6eab6..f9256f23 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -114,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, true) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true) } func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { @@ -128,7 +128,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return nil, err } - return p.ValidateToken(ctx, tok, expectedIDTokenNonce, true) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true) } func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { @@ -243,15 +243,17 @@ func ExtractUpstreamSubjectAndIssuerFromDownstream(downstreamSubject string) (st return "", "", errors.New("downstream subject did not contain original upstream subject") } split := strings.SplitN(downstreamSubject, "?sub=", 2) + iss := split[0] + sub := split[1] + if iss == "" || sub == "" { + return "", "", errors.New("downstream subject was malformed") + } return split[0], split[1], nil } -// ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, +// ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. -// 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) { +func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(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) @@ -281,7 +283,7 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) } maybeLogClaims("claims from ID token", p.Name, validatedClaims) - idTokenExpiry = validated.Expiry + idTokenExpiry = validated.Expiry // keep track of the id token expiry if we have an id token. Otherwise, it'll just be the zero value. } idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) @@ -320,31 +322,27 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t } // 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 // the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, // the UserInfo Response values MUST NOT be used. // // http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse + // If there is no ID token and it is not required, we must assume that the caller is performing other checks + // to ensure the subject is correct. 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 - } - + // keep track of the issuer from the ID token 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: + // The OIDC spec for the UserInfo 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. diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 91152c59..e5b90c80 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamoidc @@ -589,7 +589,7 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("ValidateToken", func(t *testing.T) { + t.Run("ValidateTokenAndMergeWithUserInfo", func(t *testing.T) { expiryTime := time.Now().Add(42 * time.Second) testTokenWithoutIDToken := &oauth2.Token{ AccessToken: "test-access-token", @@ -646,7 +646,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: false, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -673,7 +673,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -700,7 +700,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -727,7 +727,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -754,7 +754,7 @@ func TestProviderConfig(t *testing.T) { nonce: "", requireIDToken: false, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -799,7 +799,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", }, { @@ -808,7 +808,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: false, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", }, { @@ -817,7 +817,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", }, { @@ -826,7 +826,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-other-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "received ID token with invalid nonce: invalid nonce (expected \"some-other-nonce\", got \"some-nonce\")", }, { @@ -835,7 +835,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-other-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantErr: "received response missing ID token", }, { @@ -844,7 +844,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-other-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantErr: "received response missing ID token", }, { @@ -853,7 +853,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -898,7 +898,7 @@ func TestProviderConfig(t *testing.T) { userInfoErr: tt.userInfoErr, }, } - gotTok, err := p.ValidateToken(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) + gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) if tt.wantErr != "" { require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) @@ -924,24 +924,27 @@ func TestProviderConfig(t *testing.T) { wantUpstreamSubject: "some-subject", wantUpstreamIssuer: "https://some-issuer", }, - { - name: "happy path but sub is empty string", // todo i think this should not be the responsibility of this function, even though it's undesirable behavior... - downstreamSubject: "https://some-issuer?sub=", - wantUpstreamSubject: "", - wantUpstreamIssuer: "https://some-issuer", - }, - { - name: "happy path but iss is empty string", - downstreamSubject: "?sub=some-subject", - wantUpstreamSubject: "some-subject", - wantUpstreamIssuer: "", - }, { name: "subject in a subject", downstreamSubject: "https://some-other-issuer?sub=https://some-issuer?sub=some-subject", wantUpstreamSubject: "https://some-issuer?sub=some-subject", wantUpstreamIssuer: "https://some-other-issuer", }, + { + name: "sub is empty string", + downstreamSubject: "https://some-issuer?sub=", + wantErr: "downstream subject was malformed", + }, + { + name: "iss is empty string", + downstreamSubject: "?sub=some-subject", + wantErr: "downstream subject was malformed", + }, + { + name: "empty string", + downstreamSubject: "", + wantErr: "downstream subject did not contain original upstream subject", + }, { name: "doesn't contain sub=", downstreamSubject: "something-invalid", diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 941693e2..325ec4a4 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, "", true) + return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true) } func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) {