From 651d392b00e465a781bec95dafb2390a795cdcc7 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 11 Jan 2022 15:40:38 -0800 Subject: [PATCH] Refuse logins when no upstream refresh token and no userinfo endpoint Signed-off-by: Margo Crawford --- .../kubecertagent/mocks/mockdynamiccert.go | 2 +- .../mocks/mockpodcommandexecutor.go | 2 +- .../credentialrequestmocks.go | 2 +- internal/mocks/issuermocks/issuermocks.go | 2 +- internal/mocks/mockkeyset/mockkeyset.go | 2 +- internal/mocks/mockldapconn/mockldapconn.go | 2 +- .../mocksecrethelper/mocksecrethelper.go | 2 +- .../mocktokenauthenticator.go | 2 +- .../mocktokenauthenticatorcloser.go | 2 +- .../mockupstreamoidcidentityprovider.go | 19 ++++- internal/oidc/auth/auth_handler_test.go | 48 ++++++++++-- .../oidc/callback/callback_handler_test.go | 18 ++++- .../downstreamsession/downstream_session.go | 29 ++++--- .../provider/dynamic_upstream_idp_provider.go | 3 + .../testutil/oidctestutil/oidctestutil.go | 17 +++++ internal/upstreamoidc/upstreamoidc.go | 23 +++--- internal/upstreamoidc/upstreamoidc_test.go | 75 +++++++++++++++++-- 17 files changed, 200 insertions(+), 50 deletions(-) diff --git a/internal/controller/kubecertagent/mocks/mockdynamiccert.go b/internal/controller/kubecertagent/mocks/mockdynamiccert.go index 030d3e07..fda36b65 100644 --- a/internal/controller/kubecertagent/mocks/mockdynamiccert.go +++ b/internal/controller/kubecertagent/mocks/mockdynamiccert.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 // diff --git a/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go b/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go index 637f0927..f9aef2d0 100644 --- a/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go +++ b/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.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 // diff --git a/internal/mocks/credentialrequestmocks/credentialrequestmocks.go b/internal/mocks/credentialrequestmocks/credentialrequestmocks.go index 68ff7f2e..db5c5cce 100644 --- a/internal/mocks/credentialrequestmocks/credentialrequestmocks.go +++ b/internal/mocks/credentialrequestmocks/credentialrequestmocks.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 // diff --git a/internal/mocks/issuermocks/issuermocks.go b/internal/mocks/issuermocks/issuermocks.go index 045fbed3..737b9ab1 100644 --- a/internal/mocks/issuermocks/issuermocks.go +++ b/internal/mocks/issuermocks/issuermocks.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 // diff --git a/internal/mocks/mockkeyset/mockkeyset.go b/internal/mocks/mockkeyset/mockkeyset.go index a2cb28e6..81f4b012 100644 --- a/internal/mocks/mockkeyset/mockkeyset.go +++ b/internal/mocks/mockkeyset/mockkeyset.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 // diff --git a/internal/mocks/mockldapconn/mockldapconn.go b/internal/mocks/mockldapconn/mockldapconn.go index 24b0b1aa..b8839a04 100644 --- a/internal/mocks/mockldapconn/mockldapconn.go +++ b/internal/mocks/mockldapconn/mockldapconn.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 // diff --git a/internal/mocks/mocksecrethelper/mocksecrethelper.go b/internal/mocks/mocksecrethelper/mocksecrethelper.go index 051c7548..d7520b20 100644 --- a/internal/mocks/mocksecrethelper/mocksecrethelper.go +++ b/internal/mocks/mocksecrethelper/mocksecrethelper.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 // diff --git a/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go b/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go index 31349d33..6946e967 100644 --- a/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go +++ b/internal/mocks/mocktokenauthenticator/mocktokenauthenticator.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 // diff --git a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go index b8c7c28c..3f651eb1 100644 --- a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go +++ b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.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 // diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 07bff23e..6467b4ae 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -14,12 +14,11 @@ 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. @@ -186,6 +185,20 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) GetUsernameClaim() *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUsernameClaim", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).GetUsernameClaim)) } +// HasUserInfoURL mocks base method. +func (m *MockUpstreamOIDCIdentityProviderI) HasUserInfoURL() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasUserInfoURL") + ret0, _ := ret[0].(bool) + return ret0 +} + +// HasUserInfoURL indicates an expected call of HasUserInfoURL. +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) HasUserInfoURL() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasUserInfoURL", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).HasUserInfoURL)) +} + // PasswordCredentialsGrantAndValidateTokens mocks base method. func (m *MockUpstreamOIDCIdentityProviderI) PasswordCredentialsGrantAndValidateTokens(arg0 context.Context, arg1, arg2 string) (*oidctypes.Token, error) { m.ctrl.T.Helper() diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 65f21c76..0198c83a 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -161,6 +161,12 @@ func TestAuthorizationEndpoint(t *testing.T) { "state": happyState, } + fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery = map[string]string{ + "error": "access_denied", + "error_description": "The resource owner or authorization server denied the request. Reason: access token was returned by upstream provider but there was no userinfo endpoint.", + "state": happyState, + } + fositeAccessDeniedWithPasswordGrantDisallowedHintErrorQuery = map[string]string{ "error": "access_denied", "error_description": "The resource owner or authorization server denied the request. Resource owner password credentials grant is not allowed for this upstream provider according to its configuration.", @@ -886,8 +892,8 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyStringWithLocationInHref: true, }, { - name: "OIDC password grant happy path when upstream IDP returned empty refresh token but it did return an access token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).Build()), + name: "OIDC password grant happy path when upstream IDP returned empty refresh token but it did return an access token and has a userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), @@ -908,8 +914,8 @@ func TestAuthorizationEndpoint(t *testing.T) { wantDownstreamCustomSessionData: expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken, }, { - name: "OIDC password grant happy path when upstream IDP did not return a refresh token but it did return an access token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).Build()), + name: "OIDC password grant happy path when upstream IDP did not return a refresh token but it did return an access token and has a userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), @@ -1071,7 +1077,33 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP returns empty refresh token and empty access token", + name: "password grant returns an error when upstream IDP returns no refresh token with an access token but has no userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery), + wantBodyString: "", + }, + { + name: "password grant returns an error when upstream IDP returns empty refresh token with an access token but has no userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery), + wantBodyString: "", + }, + { + name: "password grant returns an error when upstream IDP returns empty refresh token and empty access token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithEmptyAccessToken().Build()), method: http.MethodGet, path: happyGetRequestPath, @@ -1084,7 +1116,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP returns no refresh and no access token", + name: "password grant returns an error when upstream IDP returns no refresh and no access token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithoutAccessToken().Build()), method: http.MethodGet, path: happyGetRequestPath, @@ -1097,7 +1129,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP returns no refresh token and empty access token", + name: "password grant returns an error when upstream IDP returns no refresh token and empty access token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithEmptyAccessToken().Build()), method: http.MethodGet, path: happyGetRequestPath, @@ -1110,7 +1142,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP returns empty refresh token and no access token", + name: "password grant returns an error when upstream IDP returns empty refresh token and no access token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithoutAccessToken().Build()), method: http.MethodGet, path: happyGetRequestPath, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 784bde99..a43b2926 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -212,8 +212,8 @@ func TestCallbackEndpoint(t *testing.T) { }, }, { - name: "GET with authcode exchange that returns an access token but no refresh token returns 303 to downstream client callback with its state and code", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).Build()), + name: "GET with authcode exchange that returns an access token but no refresh token when there is a userinfo endpoint returns 303 to downstream client callback with its state and code", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyState).String(), csrfCookie: happyCSRFCookie, @@ -356,6 +356,20 @@ func TestCallbackEndpoint(t *testing.T) { args: happyExchangeAndValidateTokensArgs, }, }, + { + name: "return an error when upstream IDP returned no refresh token with an access token when there is no userinfo endpoint", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: htmlContentType, + wantBody: "Unprocessable Entity: access token was returned by upstream provider but there was no userinfo endpoint\n", + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, { name: "return an error when upstream IDP returned no refresh token and no access token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithoutAccessToken().Build()), diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 4fa90e2d..64ac1418 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -80,27 +80,32 @@ func MakeDownstreamOIDCCustomSessionData(oidcUpstream provider.UpstreamOIDCIdent }, } + const pleaseCheck = "please check configuration of OIDCIdentityProvider and the client in the " + + "upstream provider's API/UI and try to get a refresh token if possible" + logKV := []interface{}{ + "upstreamName", oidcUpstream.GetName(), + "scopes", oidcUpstream.GetScopes(), + "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams(), + } + hasRefreshToken := token.RefreshToken != nil && token.RefreshToken.Token != "" hasAccessToken := token.AccessToken != nil && token.AccessToken.Token != "" switch { case hasRefreshToken: // we prefer refresh tokens, so check for this first customSessionData.OIDC.UpstreamRefreshToken = token.RefreshToken.Token - case hasAccessToken: - plog.Info("refresh token not returned by upstream provider during password grant, using access token instead. "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI "+ - "and try to get a refresh token if possible", - "upstreamName", oidcUpstream.GetName(), - "scopes", oidcUpstream.GetScopes(), - "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams()) + case hasAccessToken: // as a fallback, we can use the access token as long as there is a userinfo endpoint + if !oidcUpstream.HasUserInfoURL() { + plog.Warning("access token was returned by upstream provider during login without a refresh token "+ + "and there was no userinfo endpoint available on the provider. "+pleaseCheck, logKV...) + return nil, errors.New("access token was returned by upstream provider but there was no userinfo endpoint") + } + plog.Info("refresh token not returned by upstream provider during login, using access token instead. "+pleaseCheck, logKV...) customSessionData.OIDC.UpstreamAccessToken = token.AccessToken.Token default: - plog.Warning("refresh token and access token not returned by upstream provider during password grant, "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", - "upstreamName", oidcUpstream.GetName(), - "scopes", oidcUpstream.GetScopes(), - "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams()) + plog.Warning("refresh token and access token not returned by upstream provider during login. "+pleaseCheck, logKV...) return nil, errors.New("neither access token nor refresh token returned by upstream provider") } + return customSessionData, nil } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 24b821e7..a0691ed2 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -31,6 +31,9 @@ type UpstreamOIDCIdentityProviderI interface { // GetAuthorizationURL returns the Authorization Endpoint fetched from discovery. GetAuthorizationURL() *url.URL + // HasUserInfoURL returns whether there is a non-empty value for userinfo_endpoint fetched from discovery. + HasUserInfoURL() bool + // GetScopes returns the scopes to request in authorization (authcode or password grant) flow. GetScopes() []string diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 567212d1..fa290017 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -150,6 +150,7 @@ type TestUpstreamOIDCIdentityProvider struct { ClientID string ResourceUID types.UID AuthorizationURL url.URL + UserInfoURL bool RevocationURL *url.URL UsernameClaim string GroupsClaim string @@ -210,6 +211,10 @@ func (u *TestUpstreamOIDCIdentityProvider) GetAuthorizationURL() *url.URL { return &u.AuthorizationURL } +func (u *TestUpstreamOIDCIdentityProvider) HasUserInfoURL() bool { + return u.UserInfoURL +} + func (u *TestUpstreamOIDCIdentityProvider) GetRevocationURL() *url.URL { return u.RevocationURL } @@ -612,6 +617,7 @@ type TestUpstreamOIDCIdentityProviderBuilder struct { refreshedTokens *oauth2.Token validatedTokens *oidctypes.Token authorizationURL url.URL + hasUserInfoURL bool additionalAuthcodeParams map[string]string allowPasswordGrant bool authcodeExchangeErr error @@ -641,6 +647,16 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAuthorizationURL(value url return u } +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithUserInfoURL() *TestUpstreamOIDCIdentityProviderBuilder { + u.hasUserInfoURL = true + return u +} + +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutUserInfoURL() *TestUpstreamOIDCIdentityProviderBuilder { + u.hasUserInfoURL = false + return u +} + func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAllowPasswordGrant(value bool) *TestUpstreamOIDCIdentityProviderBuilder { u.allowPasswordGrant = value return u @@ -763,6 +779,7 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdent Scopes: u.scopes, AllowPasswordGrant: u.allowPasswordGrant, AuthorizationURL: u.authorizationURL, + UserInfoURL: u.hasUserInfoURL, AdditionalAuthcodeParams: u.additionalAuthcodeParams, ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.authcodeExchangeErr != nil { diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index b9d371ed..5a1006a7 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -61,6 +61,19 @@ func (p *ProviderConfig) GetRevocationURL() *url.URL { return p.RevocationURL } +func (p *ProviderConfig) HasUserInfoURL() bool { + providerJSON := &struct { + UserInfoURL string `json:"userinfo_endpoint"` + }{} + if err := p.Provider.Claims(providerJSON); err != nil { + // This should never happen in practice because we should have already successfully + // parsed these claims when p.Provider was created. + return false + } + + return len(providerJSON.UserInfoURL) > 0 +} + func (p *ProviderConfig) GetAdditionalAuthcodeParams() map[string]string { return p.AdditionalAuthcodeParams } @@ -356,16 +369,8 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t } func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) { - providerJSON := &struct { - UserInfoURL string `json:"userinfo_endpoint"` - }{} - if err := p.Provider.Claims(providerJSON); err != nil { - // this should never happen because we should have already parsed these claims at an earlier stage - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err) - } - // implementing the user info endpoint is not required, skip this logic when it is absent - if len(providerJSON.UserInfoURL) == 0 { + if !p.HasUserInfoURL() { return nil, nil } diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 90dc6f1f..0cd2a727 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -40,6 +40,9 @@ func TestProviderConfig(t *testing.T) { Endpoint: oauth2.Endpoint{AuthURL: "https://example.com"}, Scopes: []string{"scope1", "scope2"}, }, + Provider: &mockProvider{ + rawClaims: []byte(`{"userinfo_endpoint": "https://example.com/userinfo"}`), + }, } require.Equal(t, "test-name", p.GetName()) require.Equal(t, "test-client-id", p.GetClientID()) @@ -54,6 +57,16 @@ func TestProviderConfig(t *testing.T) { require.True(t, p.AllowsPasswordGrant()) p.AllowPasswordGrant = false require.False(t, p.AllowsPasswordGrant()) + + require.True(t, p.HasUserInfoURL()) + p.Provider = &mockProvider{ + rawClaims: []byte(`{"some_other_endpoint": "https://example.com/blah"}`), + } + require.False(t, p.HasUserInfoURL()) + p.Provider = &mockProvider{ + rawClaims: []byte(`{`), + } + require.False(t, p.HasUserInfoURL()) }) const ( @@ -748,6 +761,31 @@ func TestProviderConfig(t *testing.T) { }, }, }, + { + name: "token with id, access and refresh tokens and valid nonce, but no userinfo endpoint from discovery", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"not_the_userinfo_endpoint": "some-other-endpoint"}`), + 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: "token with no id token but valid userinfo", tok: testTokenWithoutIDToken, @@ -982,6 +1020,36 @@ func TestProviderConfig(t *testing.T) { rawClaims: []byte(`{}`), // user info not supported wantUserInfoCalled: false, }, + { + name: "valid but userinfo endpoint could not be found due to parse error", + authCode: "valid", + returnIDTok: validIDToken, + wantToken: oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Expiry: metav1.Time{}, + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: validIDToken, + Expiry: metav1.Time{}, + Claims: map[string]interface{}{ + "foo": "bar", + "bat": "baz", + "aud": "test-client-id", + "iat": 1.606768593e+09, + "jti": "test-jti", + "nbf": 1.606768593e+09, + "sub": "test-user", + }, + }, + }, + // cannot be parsed as json, but note that in this case constructing a real provider would have failed + rawClaims: []byte(`{`), + wantUserInfoCalled: false, + }, { name: "valid", authCode: "valid", @@ -1011,13 +1079,6 @@ func TestProviderConfig(t *testing.T) { rawClaims: []byte(`{}`), // user info not supported wantUserInfoCalled: false, }, - { - name: "user info discovery parse error", - authCode: "valid", - returnIDTok: validIDToken, - rawClaims: []byte(`junk`), // user info discovery fails - wantErr: "could not fetch user info claims: could not unmarshal discovery JSON: invalid character 'j' looking for beginning of value", - }, { name: "user info fetch error", authCode: "valid",