diff --git a/internal/config/supervisor/config_test.go b/internal/config/supervisor/config_test.go index 8bd5a9d2f..532cc24cd 100644 --- a/internal/config/supervisor/config_test.go +++ b/internal/config/supervisor/config_test.go @@ -60,6 +60,8 @@ func TestFromPath(t *testing.T) { audit: logUsernamesAndGroups: enabled logInternalPaths: enabled + oidc: + ignoreUserInfoEndpoint: true `), wantConfig: &Config{ APIGroupSuffix: ptr.To("some.suffix.com"), @@ -104,6 +106,9 @@ func TestFromPath(t *testing.T) { LogUsernamesAndGroups: "enabled", LogInternalPaths: "enabled", }, + OIDC: OIDCSpec{ + IgnoreUserInfoEndpoint: true, + }, }, }, { @@ -148,6 +153,9 @@ func TestFromPath(t *testing.T) { AggregatedAPIServerDisableAdmissionPlugins: nil, TLS: TLSSpec{}, Log: plog.LogSpec{}, + OIDC: OIDCSpec{ + IgnoreUserInfoEndpoint: false, + }, }, }, { @@ -182,6 +190,36 @@ func TestFromPath(t *testing.T) { }, }, }, + { + name: "ignoreUserInfoEndpoint can be disabled explicitly", + yaml: here.Doc(` + --- + names: + defaultTLSCertificateSecret: my-secret-name + oidc: + ignoreUserInfoEndpoint: false + `), + wantConfig: &Config{ + APIGroupSuffix: ptr.To("pinniped.dev"), + Labels: map[string]string{}, + NamesConfig: NamesConfigSpec{ + DefaultTLSCertificateSecret: "my-secret-name", + }, + Endpoints: &Endpoints{ + HTTPS: &Endpoint{ + Network: "tcp", + Address: ":8443", + }, + HTTP: &Endpoint{ + Network: "disabled", + }, + }, + AggregatedAPIServerPort: ptr.To[int64](10250), + OIDC: OIDCSpec{ + IgnoreUserInfoEndpoint: false, + }, + }, + }, { name: "all endpoints disabled", yaml: here.Doc(` @@ -370,6 +408,17 @@ func TestFromPath(t *testing.T) { `), wantError: "validate aggregatedAPIServerDisableAdmissionPlugins: admission plugin names not recognized: [foobar foobaz] (each must be one of [NamespaceLifecycle MutatingAdmissionPolicy MutatingAdmissionWebhook ValidatingAdmissionPolicy ValidatingAdmissionWebhook])", }, + { + name: "invalid ignoreUserInfoEndpoint", + yaml: here.Doc(` + --- + names: + defaultTLSCertificateSecret: my-secret-name + oidc: + ignoreUserInfoEndpoint: "should be a boolean, but is a string" + `), + wantError: "decode yaml: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field OIDCSpec.oidc.ignoreUserInfoEndpoint of type bool", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/internal/config/supervisor/types.go b/internal/config/supervisor/types.go index 18587857e..72146ff4e 100644 --- a/internal/config/supervisor/types.go +++ b/internal/config/supervisor/types.go @@ -23,6 +23,7 @@ type Config struct { AggregatedAPIServerDisableAdmissionPlugins []string `json:"aggregatedAPIServerDisableAdmissionPlugins"` TLS TLSSpec `json:"tls"` Audit AuditSpec `json:"audit"` + OIDC OIDCSpec `json:"oidc"` } type AuditInternalPaths string @@ -40,6 +41,29 @@ type AuditSpec struct { LogUsernamesAndGroups AuditUsernamesAndGroups `json:"logUsernamesAndGroups"` } +type OIDCSpec struct { + // IgnoreUserInfoEndpoint, when true, will cause all OIDCIdentityProviders to ignore the potential existence + // of any userinfo endpoint offered by the external OIDC provider(s) when those OIDC providers return refresh + // tokens. Please exercise caution when using this setting. + // + // Note that enabling this setting causes ALL configured OIDCIdentityProviders to skip calling the userinfo + // endpoint, which is not the behavior that you want for some providers which return more information from + // the userinfo endpoint than they put into the ID token itself. Pinniped will normally merge the claims + // from the ID token with the response from the userinfo endpoint, but this setting disables that behavior. + // + // This was added as a workaround for Microsoft ADFS, which does not correctly implement the userinfo + // endpoint as described in the OIDC specification. There are several circumstances where calls to the + // ADFS userinfo endpoint will result in "403 Forbidden" responses, which cause Pinniped to reject a user's + // login and/or session refresh. + // + // This setting is only designed to be used in the case where the only OIDCIdentityProvider(s) that are + // configured for a Pinniped Supervisor are ADFS servers. + // + // We do not currently have plans to implement better ADFS support because Microsoft no longer recommends + // the use of ADFS. + IgnoreUserInfoEndpoint bool `json:"ignoreUserInfoEndpoint"` +} + type TLSSpec struct { OneDotTwo TLSProtocolSpec `json:"onedottwo"` } diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index e6ab2f448..33ab3f194 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package oidcupstreamwatcher implements a controller which watches OIDCIdentityProviders. @@ -129,6 +129,10 @@ func (c *ttlProviderCache) putProvider(key oidcDiscoveryCacheKey, value *oidcDis c.cache.Set(key, value, oidcValidatorCacheTTL) } +type GlobalOIDCConfig struct { + IgnoreUserInfoEndpoint bool +} + type oidcWatcherController struct { cache UpstreamOIDCIdentityProviderICache log plog.Logger @@ -137,6 +141,7 @@ type oidcWatcherController struct { secretInformer corev1informers.SecretInformer configMapInformer corev1informers.ConfigMapInformer validatorCache oidcDiscoveryCache + globalOIDCConfig GlobalOIDCConfig } // New instantiates a new controllerlib.Controller which will populate the provided UpstreamOIDCIdentityProviderICache. @@ -149,6 +154,7 @@ func New( log plog.Logger, withInformer pinnipedcontroller.WithInformerOptionFunc, validatorCache *cache.Expiring, + globalOIDCConfig GlobalOIDCConfig, ) controllerlib.Controller { c := oidcWatcherController{ cache: idpCache, @@ -158,6 +164,7 @@ func New( secretInformer: secretInformer, configMapInformer: configMapInformer, validatorCache: &ttlProviderCache{cache: validatorCache}, + globalOIDCConfig: globalOIDCConfig, } return controllerlib.New( controllerlib.Config{Name: oidcControllerName, Syncer: &c}, @@ -235,6 +242,7 @@ func (c *oidcWatcherController) validateUpstream(ctx controllerlib.Context, upst AdditionalAuthcodeParams: additionalAuthcodeAuthorizeParameters, AdditionalClaimMappings: upstream.Spec.Claims.AdditionalClaimMappings, ResourceUID: upstream.UID, + IgnoreUserInfoEndpoint: c.globalOIDCConfig.IgnoreUserInfoEndpoint, } conditions := []*metav1.Condition{ diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index d8aa2f106..e805ce9ef 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidcupstreamwatcher @@ -123,6 +123,7 @@ func TestOIDCUpstreamWatcherControllerFilterSecret(t *testing.T) { logger, withInformer.WithInformer, expiringcache.NewExpiring(), + GlobalOIDCConfig{}, ) unrelated := corev1.Secret{} @@ -182,6 +183,7 @@ func TestOIDCUpstreamWatcherControllerFilterConfigMaps(t *testing.T) { logger, withInformer.WithInformer, expiringcache.NewExpiring(), + GlobalOIDCConfig{}, ) unrelated := corev1.ConfigMap{} @@ -242,6 +244,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { inputUpstreams []runtime.Object inputResources []runtime.Object inputValidatorCache func(*testing.T) map[oidcDiscoveryCacheKey]*oidcDiscoveryCacheValue + inputGlobalOIDCConfig *GlobalOIDCConfig wantErr string wantLogs []string wantResultingCache []*oidctestutil.TestUpstreamOIDCIdentityProvider @@ -951,6 +954,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: append(testExpectedScopes, "xyz"), // includes openid only once UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1014,6 +1018,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: testDefaultExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1106,6 +1111,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: testDefaultExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1174,6 +1180,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: testDefaultExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1240,6 +1247,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: testDefaultExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1304,6 +1312,140 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: nil, // no revocation URL is set in the cached provider because none was returned by discovery + UserInfoURL: true, + Scopes: testDefaultExpectedScopes, + UsernameClaim: testUsernameClaim, + GroupsClaim: testGroupsClaim, + AllowPasswordGrant: false, + AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map + ResourceUID: testUID, + }, + }, + wantResultingUpstreams: []idpv1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Status: idpv1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []metav1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "discovered issuer configuration", ObservedGeneration: 1234}, + {Type: "TLSConfigurationValid", Status: "True", LastTransitionTime: now, Reason: "Success", + Message: "spec.tls is valid: using configured CA bundle", ObservedGeneration: 1234}, + }, + }, + }}, + }, + { + name: "existing valid upstream with no userinfo endpoint in the discovery document", + inputUpstreams: []runtime.Object{&idpv1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Spec: idpv1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/valid-without-userinfo", + TLS: &idpv1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: idpv1alpha1.OIDCClient{SecretName: testSecretName}, + Claims: idpv1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim}, + }, + Status: idpv1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []metav1.Condition{ + happyAdditionalAuthorizeParametersValidConditionEarlier, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "loaded client credentials"}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "discovered issuer configuration"}, + }, + }, + }}, + inputResources: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantLogs: []string{ + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"ClientCredentialsSecretValid","status":"True","reason":"Success","message":"loaded client credentials"}`, + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"OIDCDiscoverySucceeded","status":"True","reason":"Success","message":"discovered issuer configuration"}`, + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"TLSConfigurationValid","status":"True","reason":"Success","message":"spec.tls is valid: using configured CA bundle"}`, + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"AdditionalAuthorizeParametersValid","status":"True","reason":"Success","message":"additionalAuthorizeParameters parameter names are allowed"}`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { + Name: testName, + ClientID: testClientID, + AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, + UserInfoURL: false, + Scopes: testDefaultExpectedScopes, + UsernameClaim: testUsernameClaim, + GroupsClaim: testGroupsClaim, + AllowPasswordGrant: false, + AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map + ResourceUID: testUID, + }, + }, + wantResultingUpstreams: []idpv1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Status: idpv1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []metav1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "discovered issuer configuration", ObservedGeneration: 1234}, + {Type: "TLSConfigurationValid", Status: "True", LastTransitionTime: now, Reason: "Success", + Message: "spec.tls is valid: using configured CA bundle", ObservedGeneration: 1234}, + }, + }, + }}, + }, + { + name: "existing valid upstream with userinfo endpoint in the discovery document, but global OIDC config includes setting to ignore provider's userinfo endpoint", + inputGlobalOIDCConfig: &GlobalOIDCConfig{ + IgnoreUserInfoEndpoint: true, + }, + inputUpstreams: []runtime.Object{&idpv1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Spec: idpv1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL, + TLS: &idpv1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: idpv1alpha1.OIDCClient{SecretName: testSecretName}, + Claims: idpv1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim}, + }, + Status: idpv1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []metav1.Condition{ + happyAdditionalAuthorizeParametersValidConditionEarlier, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "loaded client credentials"}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "discovered issuer configuration"}, + }, + }, + }}, + inputResources: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantLogs: []string{ + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"ClientCredentialsSecretValid","status":"True","reason":"Success","message":"loaded client credentials"}`, + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"OIDCDiscoverySucceeded","status":"True","reason":"Success","message":"discovered issuer configuration"}`, + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"TLSConfigurationValid","status":"True","reason":"Success","message":"spec.tls is valid: using configured CA bundle"}`, + `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"oidc-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"test-namespace","name":"test-name","type":"AdditionalAuthorizeParametersValid","status":"True","reason":"Success","message":"additionalAuthorizeParameters parameter names are allowed"}`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { + Name: testName, + ClientID: testClientID, + AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, + UserInfoURL: false, // expecting false due to global OIDC configuration override (this provider actually has a userinfo endpoint) Scopes: testDefaultExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1371,6 +1513,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: testExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1446,6 +1589,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, RevocationURL: testIssuerRevocationURL, + UserInfoURL: true, Scopes: testExpectedScopes, // does not include the default scopes UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -1634,6 +1778,11 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { } } + globalOIDCConfig := &GlobalOIDCConfig{} + if tt.inputGlobalOIDCConfig != nil { + globalOIDCConfig = tt.inputGlobalOIDCConfig + } + controller := New( cache, fakePinnipedClient, @@ -1643,6 +1792,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { logger, controllerlib.WithInformer, validatorCache, + *globalOIDCConfig, ) ctx, cancel := context.WithCancel(context.Background()) @@ -1677,6 +1827,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { require.Equal(t, tt.wantResultingCache[i].GetAdditionalClaimMappings(), actualIDP.GetAdditionalClaimMappings()) require.Equal(t, tt.wantResultingCache[i].GetResourceUID(), actualIDP.GetResourceUID()) require.Equal(t, tt.wantResultingCache[i].GetRevocationURL(), actualIDP.GetRevocationURL()) + require.Equal(t, tt.wantResultingCache[i].HasUserInfoURL(), actualIDP.HasUserInfoURL()) require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes()) // We always want to use the proxy from env on these clients, so although the following assertions @@ -1754,6 +1905,7 @@ func newTestIssuer(t *testing.T) (string, string) { TokenURL string `json:"token_endpoint"` RevocationURL string `json:"revocation_endpoint,omitempty"` JWKSURL string `json:"jwks_uri"` + UserInfoURL string `json:"userinfo_endpoint"` } // At the root of the server, serve an issuer with a valid discovery response. @@ -1764,6 +1916,7 @@ func newTestIssuer(t *testing.T) (string, string) { AuthURL: "https://example.com/authorize", RevocationURL: "https://example.com/revoke", TokenURL: "https://example.com/token", + UserInfoURL: "https://example.com/userinfo", }) }) @@ -1775,6 +1928,19 @@ func newTestIssuer(t *testing.T) (string, string) { AuthURL: "https://example.com/authorize", RevocationURL: "", // none TokenURL: "https://example.com/token", + UserInfoURL: "https://example.com/userinfo", + }) + }) + + // At "/valid-without-userinfo", serve an issuer with a valid discovery response which does not have a userinfo endpoint. + mux.HandleFunc("/valid-without-userinfo/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&providerJSON{ + Issuer: server.URL + "/valid-without-userinfo", + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", + UserInfoURL: "", // none }) }) @@ -1865,6 +2031,7 @@ func newTestIssuer(t *testing.T) (string, string) { AuthURL: "https://example.com/authorize", RevocationURL: "https://example.com/revoke", TokenURL: "https://example.com/token", + UserInfoURL: "https://example.com/userinfo", }) }) diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 9ef324bca..24accacf7 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -311,6 +311,9 @@ func prepareControllers( plog.New(), controllerlib.WithInformer, cache.NewExpiring(), + oidcupstreamwatcher.GlobalOIDCConfig{ + IgnoreUserInfoEndpoint: cfg.OIDC.IgnoreUserInfoEndpoint, + }, ), singletonWorker). WithController( diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 55581b608..c792845ff 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions. @@ -46,6 +46,7 @@ type ProviderConfig struct { AdditionalAuthcodeParams map[string]string AdditionalClaimMappings map[string]string RevocationURL *url.URL // will commonly be nil: many providers do not offer this + IgnoreUserInfoEndpoint bool Provider interface { Verifier(*coreosoidc.Config) *coreosoidc.IDTokenVerifier Claims(v any) error @@ -64,6 +65,12 @@ func (p *ProviderConfig) GetRevocationURL() *url.URL { } func (p *ProviderConfig) HasUserInfoURL() bool { + if p.IgnoreUserInfoEndpoint { + // When asked via configuration to ignore the userinfo endpoint, then + // return false, regardless of the external provider's discovery response. + return false + } + providerJSON := &struct { UserInfoURL string `json:"userinfo_endpoint"` }{} @@ -398,7 +405,9 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t } func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token, requireUserInfo bool) (*coreosoidc.UserInfo, error) { - // implementing the user info endpoint is not required by the OIDC spec, but we may require it in certain situations. + // Implementing the user info endpoint is not required by the OIDC spec, but we may require it in certain situations. + // Even when configuration asks us to skip calling the userinfo endpoint, we should not skip it on those situations + // where it is our only source of getting the claims that we need to perform our validations. if !p.HasUserInfoURL() { if requireUserInfo { // TODO should these all be http errors? diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 201021709..02d4ef804 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamoidc @@ -62,6 +62,13 @@ func TestProviderConfig(t *testing.T) { require.False(t, p.AllowsPasswordGrant()) require.True(t, p.HasUserInfoURL()) + + // Even when the underlying provider does have a userinfo endpoint, + // this setting should be able to override it. + p.IgnoreUserInfoEndpoint = true + require.False(t, p.HasUserInfoURL()) + p.IgnoreUserInfoEndpoint = false + p.Provider = &mockProvider{ rawClaims: []byte(`{"some_other_endpoint": "https://example.com/blah"}`), } @@ -100,12 +107,13 @@ func TestProviderConfig(t *testing.T) { t.Run("PasswordCredentialsGrantAndValidateTokens", func(t *testing.T) { tests := []struct { - name string - disallowPasswordGrant bool - returnIDTok string - tokenStatusCode int - wantErr string - wantToken oidctypes.Token + name string + disallowPasswordGrant bool + ignoreUserInfoEndpoint bool + returnIDTok string + tokenStatusCode int + wantErr string + wantToken oidctypes.Token rawClaims []byte userInfo *coreosoidc.UserInfo @@ -170,6 +178,37 @@ func TestProviderConfig(t *testing.T) { userInfo: forceUserInfoWithClaims("test-user", `{"foo":"awesomeness","groups":"fancy-group"}`), wantUserInfoCalled: true, }, + { + name: "valid with userinfo when configured to ignore the userinfo endpoint", + ignoreUserInfoEndpoint: true, + 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]any{ + "foo": "bar", // does not overwrite existing claim because userinfo is skipped + "bat": "baz", + "aud": "test-client-id", + "iat": 1.606768593e+09, + "jti": "test-jti", + "nbf": 1.606768593e+09, + "sub": "test-user", + // groups claim is not added because userinfo is skipped + }, + }, + }, + // claims is private field so we have to use hacks to set it + userInfo: forceUserInfoWithClaims("test-user", `{"foo":"awesomeness","groups":"fancy-group"}`), + wantUserInfoCalled: false, + }, { name: "password grant not allowed", disallowPasswordGrant: true, // password grant is not allowed in this ProviderConfig @@ -294,8 +333,9 @@ func TestProviderConfig(t *testing.T) { userInfo: tt.userInfo, userInfoErr: tt.userInfoErr, }, - AllowPasswordGrant: !tt.disallowPasswordGrant, - Client: http.DefaultClient, + AllowPasswordGrant: !tt.disallowPasswordGrant, + Client: http.DefaultClient, + IgnoreUserInfoEndpoint: tt.ignoreUserInfoEndpoint, } tok, err := p.PasswordCredentialsGrantAndValidateTokens( @@ -741,16 +781,17 @@ func TestProviderConfig(t *testing.T) { goodIDToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJub25jZSI6InNvbWUtbm9uY2UiLCJpc3MiOiJzb21lLWlzc3VlciJ9.eGvzOihLUqzn3M4k6fHsToedgy7Fu89_Xu_u4mwMgRlIyRWZqmEMV76RVLnZd9Ihm9j_VpvrpirIkaj4JM9eRNfLX1n328cmBivBwnTKAzHuTm17dUKO5EvdTmQzmwnN0WZ8nWk4GfR7RzcvE1V8G9tIiWD8FkO3Dr-NR_zTun3N37onAazVLCmF0SDtATDfUH1ETqviHEp8xGx5HD5mv5T3HEjOuer5gxTEnfncef0LurBH3po-C0tXHKu74PD8x88CMJ1DLsRdCalnctwa850slKPkBSTP-ssh0JVg7cdMXoosVpwiXtKYaBkrhu8VS018aFP-cBbW0mYwsHmt3g" //nolint:gosec tests := []struct { - name string - tok *oauth2.Token - nonce nonce.Nonce - requireIDToken bool - requireUserInfo bool - userInfo *coreosoidc.UserInfo - rawClaims []byte - userInfoErr error - wantErr string - wantMergedTokens *oidctypes.Token + name string + tok *oauth2.Token + nonce nonce.Nonce + ignoreUserInfoEndpoint bool + requireIDToken bool + requireUserInfo bool + userInfo *coreosoidc.UserInfo + rawClaims []byte + userInfoErr error + wantErr string + wantMergedTokens *oidctypes.Token }{ { name: "token with id, access and refresh tokens, valid nonce, and no userinfo", @@ -831,6 +872,34 @@ func TestProviderConfig(t *testing.T) { }, }, }, + { + name: "token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token, when configured to ignore userinfo", + tok: testTokenWithoutIDToken.WithExtra(map[string]any{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + ignoreUserInfoEndpoint: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "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{ + Token: goodIDToken, + Claims: map[string]any{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + // "name" claim returned by userinfo endpoint is not merged, because userinfo was not called due to ignoreUserInfoEndpoint configuration + }, + }, + }, + }, { name: "userinfo is required, token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token", tok: testTokenWithoutIDToken.WithExtra(map[string]any{"id_token": goodIDToken}), @@ -1037,6 +1106,15 @@ func TestProviderConfig(t *testing.T) { rawClaims: []byte(`{}`), wantErr: "could not fetch user info claims: userinfo endpoint not found, but is required", }, + { + name: "expected to have userinfo, but doesn't, even when configured to otherwise ignore userinfo", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireUserInfo: true, + ignoreUserInfoEndpoint: true, + rawClaims: []byte(`{}`), + wantErr: "could not fetch user info claims: userinfo endpoint not found, but is required", + }, { name: "expected to have id token and userinfo, but doesn't have either", tok: testTokenWithoutIDToken, @@ -1104,6 +1182,7 @@ func TestProviderConfig(t *testing.T) { userInfo: tt.userInfo, userInfoErr: tt.userInfoErr, }, + IgnoreUserInfoEndpoint: tt.ignoreUserInfoEndpoint, } gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken, tt.requireUserInfo) if tt.wantErr != "" { @@ -1119,12 +1198,13 @@ func TestProviderConfig(t *testing.T) { t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) { tests := []struct { - name string - authCode string - expectNonce nonce.Nonce - returnIDTok string - wantErr string - wantToken oidctypes.Token + name string + authCode string + ignoreUserInfoEndpoint bool + expectNonce nonce.Nonce + returnIDTok string + wantErr string + wantToken oidctypes.Token rawClaims []byte userInfo *coreosoidc.UserInfo @@ -1301,6 +1381,38 @@ func TestProviderConfig(t *testing.T) { userInfo: forceUserInfoWithClaims("test-user", `{"foo":"awesomeness","groups":"fancy-group"}`), wantUserInfoCalled: true, }, + { + name: "valid with user info, when configured to ignore userinfo", + authCode: "valid", + ignoreUserInfoEndpoint: true, + 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]any{ + "foo": "bar", // does not overwrite existing claim because userinfo is skipped + "bat": "baz", + "aud": "test-client-id", + "iat": 1.606768593e+09, + "jti": "test-jti", + "nbf": 1.606768593e+09, + "sub": "test-user", + // groups claim is not added because userinfo is skipped + }, + }, + }, + // claims is private field so we have to use hacks to set it + userInfo: forceUserInfoWithClaims("test-user", `{"foo":"awesomeness","groups":"fancy-group"}`), + wantUserInfoCalled: false, + }, { name: "invalid sub claim", authCode: "valid", @@ -1383,7 +1495,8 @@ func TestProviderConfig(t *testing.T) { userInfo: tt.userInfo, userInfoErr: tt.userInfoErr, }, - Client: http.DefaultClient, + Client: http.DefaultClient, + IgnoreUserInfoEndpoint: tt.ignoreUserInfoEndpoint, } tok, err := p.ExchangeAuthcodeAndValidateTokens(