add new supervisor configmap option to ignore userinfo endpoints by matching issuer URLs

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 {

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 {

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{

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

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