From 577797d5696a126ebb89804bf6ff6f57d026c920 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 27 Aug 2025 13:22:17 -0700 Subject: [PATCH] add new supervisor configmap option to ignore userinfo endpoints by matching issuer URLs --- internal/config/supervisor/config_test.go | 31 ++++++++------ internal/config/supervisor/types.go | 30 ++++++++------ .../oidc_upstream_watcher.go | 16 +++++++- .../oidc_upstream_watcher_test.go | 41 ++++++++++++++++++- internal/supervisor/server/server.go | 5 ++- 5 files changed, 93 insertions(+), 30 deletions(-) diff --git a/internal/config/supervisor/config_test.go b/internal/config/supervisor/config_test.go index 532cc24cd..5125d3d31 100644 --- a/internal/config/supervisor/config_test.go +++ b/internal/config/supervisor/config_test.go @@ -61,7 +61,10 @@ func TestFromPath(t *testing.T) { logUsernamesAndGroups: enabled logInternalPaths: enabled oidc: - ignoreUserInfoEndpoint: true + ignoreUserInfoEndpoint: + whenIssuerExactlyMatches: + - https://foo.com + - https://bar.com `), wantConfig: &Config{ APIGroupSuffix: ptr.To("some.suffix.com"), @@ -107,7 +110,12 @@ func TestFromPath(t *testing.T) { LogInternalPaths: "enabled", }, OIDC: OIDCSpec{ - IgnoreUserInfoEndpoint: true, + IgnoreUserInfoEndpoint: IgnoreUserInfoEndpointSpec{ + WhenIssuerExactlyMatches: []string{ + "https://foo.com", + "https://bar.com", + }, + }, }, }, }, @@ -151,11 +159,9 @@ func TestFromPath(t *testing.T) { LogUsernamesAndGroups: "", }, AggregatedAPIServerDisableAdmissionPlugins: nil, - TLS: TLSSpec{}, - Log: plog.LogSpec{}, - OIDC: OIDCSpec{ - IgnoreUserInfoEndpoint: false, - }, + TLS: TLSSpec{}, + Log: plog.LogSpec{}, + OIDC: OIDCSpec{}, }, }, { @@ -191,13 +197,14 @@ func TestFromPath(t *testing.T) { }, }, { - name: "ignoreUserInfoEndpoint can be disabled explicitly", + name: "ignoreUserInfoEndpoint can be explicitly an empty list", yaml: here.Doc(` --- names: defaultTLSCertificateSecret: my-secret-name oidc: - ignoreUserInfoEndpoint: false + ignoreUserInfoEndpoint: + whenIssuerExactlyMatches: [] `), wantConfig: &Config{ APIGroupSuffix: ptr.To("pinniped.dev"), @@ -216,7 +223,7 @@ func TestFromPath(t *testing.T) { }, AggregatedAPIServerPort: ptr.To[int64](10250), OIDC: OIDCSpec{ - IgnoreUserInfoEndpoint: false, + IgnoreUserInfoEndpoint: IgnoreUserInfoEndpointSpec{WhenIssuerExactlyMatches: []string{}}, }, }, }, @@ -415,9 +422,9 @@ func TestFromPath(t *testing.T) { names: defaultTLSCertificateSecret: my-secret-name oidc: - ignoreUserInfoEndpoint: "should be a boolean, but is a string" + ignoreUserInfoEndpoint: "should be a struct, 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", + wantError: "decode yaml: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field OIDCSpec.oidc.ignoreUserInfoEndpoint of type supervisor.IgnoreUserInfoEndpointSpec", }, } for _, test := range tests { diff --git a/internal/config/supervisor/types.go b/internal/config/supervisor/types.go index 72146ff4e..6f5821378 100644 --- a/internal/config/supervisor/types.go +++ b/internal/config/supervisor/types.go @@ -41,27 +41,31 @@ type AuditSpec struct { LogUsernamesAndGroups AuditUsernamesAndGroups `json:"logUsernamesAndGroups"` } +type IgnoreUserInfoEndpointSpec struct { + // WhenIssuerExactlyMatches is a list of exact OIDC issuer URLs for which the userinfo endpoint should be avoided. + // This will only take effect for OIDCIdentityProviders who have a spec.issuer which is exactly equal to any one + // of these strings (using exact string equality). + WhenIssuerExactlyMatches []string `json:"whenIssuerExactlyMatches"` +} + 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. + // IgnoreUserInfoEndpoint, when configured, will cause all matching OIDCIdentityProviders to ignore the + // potential existence of any userinfo endpoint offered by the external OIDC provider(s) when those OIDC providers + // return refresh tokens. // - // 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. + // Please exercise caution when using this setting. Some OIDC 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 for matching + // OIDC providers. // // 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"` + // We do not currently have plans to implement ADFS support options directly on the OIDCIdentityProvider CRD + // because Microsoft no longer recommends the use of ADFS. + IgnoreUserInfoEndpoint IgnoreUserInfoEndpointSpec `json:"ignoreUserInfoEndpoint"` } type TLSSpec struct { diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 33ab3f194..b9071af05 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -129,8 +129,20 @@ func (c *ttlProviderCache) putProvider(key oidcDiscoveryCacheKey, value *oidcDis c.cache.Set(key, value, oidcValidatorCacheTTL) } +type UserInfoEndpointConfigI interface { + IgnoreUserInfoEndpoint(issuerURL string) bool +} + +type IgnoreUserInfoEndpointForExactIssuerMatches struct { + Issuers sets.Set[string] // a set of issuer URLs +} + +func (i *IgnoreUserInfoEndpointForExactIssuerMatches) IgnoreUserInfoEndpoint(issuerURL string) bool { + return i.Issuers.Has(issuerURL) +} + type GlobalOIDCConfig struct { - IgnoreUserInfoEndpoint bool + UserInfoEndpointConfig UserInfoEndpointConfigI } type oidcWatcherController struct { @@ -242,7 +254,7 @@ func (c *oidcWatcherController) validateUpstream(ctx controllerlib.Context, upst AdditionalAuthcodeParams: additionalAuthcodeAuthorizeParameters, AdditionalClaimMappings: upstream.Spec.Claims.AdditionalClaimMappings, ResourceUID: upstream.UID, - IgnoreUserInfoEndpoint: c.globalOIDCConfig.IgnoreUserInfoEndpoint, + IgnoreUserInfoEndpoint: c.globalOIDCConfig.UserInfoEndpointConfig.IgnoreUserInfoEndpoint(upstream.Spec.Issuer), } 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 e805ce9ef..388632354 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/types" expiringcache "k8s.io/apimachinery/pkg/util/cache" "k8s.io/apimachinery/pkg/util/net" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" @@ -1407,7 +1408,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { { 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, + UserInfoEndpointConfig: &IgnoreUserInfoEndpointAlways{}, }, inputUpstreams: []runtime.Object{&idpv1alpha1.OIDCIdentityProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -1778,7 +1779,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { } } - globalOIDCConfig := &GlobalOIDCConfig{} + globalOIDCConfig := &GlobalOIDCConfig{ + // By default, do not ignore the userinfo endpoint in tests. + UserInfoEndpointConfig: &IgnoreUserInfoEndpointNever{}, + } if tt.inputGlobalOIDCConfig != nil { globalOIDCConfig = tt.inputGlobalOIDCConfig } @@ -2043,3 +2047,36 @@ func newTestIssuer(t *testing.T) (string, string) { return server.URL, string(serverCA) } + +type IgnoreUserInfoEndpointAlways struct{} + +func (i *IgnoreUserInfoEndpointAlways) IgnoreUserInfoEndpoint(_issuerURL string) bool { + return true +} + +type IgnoreUserInfoEndpointNever struct{} + +func (i *IgnoreUserInfoEndpointNever) IgnoreUserInfoEndpoint(_issuerURL string) bool { + return false +} + +func TestIgnoreUserInfoEndpointForExactIssuerMatches(t *testing.T) { + t.Parallel() + + // With an empty set. + s := &IgnoreUserInfoEndpointForExactIssuerMatches{} + require.False(t, s.IgnoreUserInfoEndpoint("https://foo.com")) + + // An alternate way to initialize an empty set. + var empty []string + s = &IgnoreUserInfoEndpointForExactIssuerMatches{Issuers: sets.New(empty...)} + require.False(t, s.IgnoreUserInfoEndpoint("https://foo.com")) + + // With a non-empty set. + s = &IgnoreUserInfoEndpointForExactIssuerMatches{ + Issuers: sets.New("https://foo.com", "https://bar.com"), + } + require.True(t, s.IgnoreUserInfoEndpoint("https://foo.com")) + require.True(t, s.IgnoreUserInfoEndpoint("https://bar.com")) + require.False(t, s.IgnoreUserInfoEndpoint("https://baz.com")) +} diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 24accacf7..71a1d273f 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/cache" + "k8s.io/apimachinery/pkg/util/sets" apimachineryversion "k8s.io/apimachinery/pkg/version" genericapifilters "k8s.io/apiserver/pkg/endpoints/filters" openapinamer "k8s.io/apiserver/pkg/endpoints/openapi" @@ -312,7 +313,9 @@ func prepareControllers( controllerlib.WithInformer, cache.NewExpiring(), oidcupstreamwatcher.GlobalOIDCConfig{ - IgnoreUserInfoEndpoint: cfg.OIDC.IgnoreUserInfoEndpoint, + UserInfoEndpointConfig: &oidcupstreamwatcher.IgnoreUserInfoEndpointForExactIssuerMatches{ + Issuers: sets.New(cfg.OIDC.IgnoreUserInfoEndpoint.WhenIssuerExactlyMatches...), + }, }, ), singletonWorker).