From 2aaaa4cb9e227c063a2ab77c0b9a10b76b0fc49c Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 1 Jul 2026 14:56:14 +0200 Subject: [PATCH] POC: decouple oauthapiserver auth config generation for shared library use Refactor the AuthenticationConfigurationGenerator so it can be extracted into a shared library (oauth-apiserver) usable by both the standalone auth operator and HyperShift. Key changes: - Replace featuregates.FeatureGate with With*() methods that set bools, removing the dependency on OpenShift feature gate packages - Replace corev1listers with ResolverFunc callbacks, removing the dependency on k8s client-go listers - Convert standalone functions to methods, eliminating parameter threading of struct fields - Remove hardcoded "openshift-config" namespace; callers bind namespace into their resolver closures --- .../externaloidc/externaloidc_controller.go | 46 +- .../externaloidc_controller_test.go | 2 +- .../generation/kubeapiserver/generate.go | 6 +- .../generation/kubeapiserver/generate_test.go | 2 +- .../generation/oauthapiserver/generate.go | 139 ++--- .../oauthapiserver/generate_test.go | 532 ++++-------------- 6 files changed, 237 insertions(+), 490 deletions(-) diff --git a/pkg/controllers/externaloidc/externaloidc_controller.go b/pkg/controllers/externaloidc/externaloidc_controller.go index 02f4cc40f2..b76d30a793 100644 --- a/pkg/controllers/externaloidc/externaloidc_controller.go +++ b/pkg/controllers/externaloidc/externaloidc_controller.go @@ -32,7 +32,7 @@ const ( ) type authConfigGenerator interface { - GenerateAuthenticationConfiguration(*configv1.Authentication) (runtime.Object, error) + GenerateAuthenticationConfiguration(*configv1.AuthenticationSpec) (runtime.Object, error) } type externalOIDCController struct { @@ -57,7 +57,19 @@ func NewExternalOIDCController( authCfgGenerator = kubeapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), featureGates) if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { - authCfgGenerator = oauthapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), kubeInformersForNamespaces.SecretLister(), featureGates) + gen := oauthapiserver.NewAuthenticationConfigurationGenerator( + newCABundleResolver(kubeInformersForNamespaces.ConfigMapLister(), configNamespace), + newClientSecretResolver(kubeInformersForNamespaces.SecretLister(), configNamespace), + ).WithExternalClaimsSourcing() + + if featureGates.Enabled(features.FeatureGateExternalOIDCWithUpstreamParity) { + gen.WithUpstreamParity() + } + if featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings) { + gen.WithAdditionalClaimMappings() + } + + authCfgGenerator = gen } c := &externalOIDCController{ @@ -95,7 +107,7 @@ func (c *externalOIDCController) sync(ctx context.Context, syncCtx factory.SyncC return c.deleteAuthConfig(ctx, syncCtx) } - authConfig, err := c.authConfigGenerator.GenerateAuthenticationConfiguration(auth) + authConfig, err := c.authConfigGenerator.GenerateAuthenticationConfiguration(&auth.Spec) if err != nil { return err } @@ -174,3 +186,31 @@ func (c *externalOIDCController) getExistingApplyConfig() (*corev1ac.ConfigMapAp return existingCMApplyConfig, nil } + +func newCABundleResolver(cmLister corev1listers.ConfigMapLister, namespace string) oauthapiserver.ResolverFunc { + return func(name string) (string, error) { + cm, err := cmLister.ConfigMaps(namespace).Get(name) + if err != nil { + return "", fmt.Errorf("could not retrieve configmap %s/%s: %v", namespace, name, err) + } + caData, ok := cm.Data["ca-bundle.crt"] + if !ok || len(caData) == 0 { + return "", fmt.Errorf("configmap %s/%s key \"ca-bundle.crt\" missing or empty", namespace, name) + } + return caData, nil + } +} + +func newClientSecretResolver(secretLister corev1listers.SecretLister, namespace string) oauthapiserver.ResolverFunc { + return func(name string) (string, error) { + secret, err := secretLister.Secrets(namespace).Get(name) + if err != nil { + return "", fmt.Errorf("could not retrieve secret %s/%s: %v", namespace, name, err) + } + clientSecret, ok := secret.Data["client-secret"] + if !ok || len(clientSecret) == 0 { + return "", fmt.Errorf("secret %s/%s key \"client-secret\" missing or empty", namespace, name) + } + return string(clientSecret), nil + } +} diff --git a/pkg/controllers/externaloidc/externaloidc_controller_test.go b/pkg/controllers/externaloidc/externaloidc_controller_test.go index 2fa623139a..5b5ed26979 100644 --- a/pkg/controllers/externaloidc/externaloidc_controller_test.go +++ b/pkg/controllers/externaloidc/externaloidc_controller_test.go @@ -636,6 +636,6 @@ type mockAuthConfigGenerator[T runtime.Object] struct { err error } -func (macg *mockAuthConfigGenerator[T]) GenerateAuthenticationConfiguration(_ *configv1.Authentication) (runtime.Object, error) { +func (macg *mockAuthConfigGenerator[T]) GenerateAuthenticationConfiguration(_ *configv1.AuthenticationSpec) (runtime.Object, error) { return macg.cfg, macg.err } diff --git a/pkg/controllers/externaloidc/generation/kubeapiserver/generate.go b/pkg/controllers/externaloidc/generation/kubeapiserver/generate.go index ac32c57d8e..f2bfc21e3a 100644 --- a/pkg/controllers/externaloidc/generation/kubeapiserver/generate.go +++ b/pkg/controllers/externaloidc/generation/kubeapiserver/generate.go @@ -62,7 +62,7 @@ func NewAuthenticationConfigurationGenerator(cmlister corev1listers.ConfigMapLis // GenerateAuthenticationConfiguration creates a structured JWT AuthenticationConfiguration for OIDC // in the kube-apiserver from the configuration found in the authentication/cluster resource. -func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfiguration(auth *configv1.Authentication) (runtime.Object, error) { +func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfiguration(authSpec *configv1.AuthenticationSpec) (runtime.Object, error) { authConfig := &apiserverv1beta1.AuthenticationConfiguration{ TypeMeta: metav1.TypeMeta{ Kind: kindAuthenticationConfiguration, @@ -71,8 +71,8 @@ func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfigura } errs := []error{} - for _, provider := range auth.Spec.OIDCProviders { - jwt, err := generateJWTForProvider(provider, acg.configMapLister, acg.featureGates, auth.Spec.ServiceAccountIssuer) + for _, provider := range authSpec.OIDCProviders { + jwt, err := generateJWTForProvider(provider, acg.configMapLister, acg.featureGates, authSpec.ServiceAccountIssuer) if err != nil { errs = append(errs, err) continue diff --git a/pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go b/pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go index f94ab3a699..1098509cd2 100644 --- a/pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go +++ b/pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go @@ -1178,7 +1178,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration c := NewAuthenticationConfigurationGenerator(corev1listers.NewConfigMapLister(tt.configMapIndexer), tt.featureGates) c.validationFn = tt.configValidator - gotConfig, err := c.GenerateAuthenticationConfiguration(&tt.auth) + gotConfig, err := c.GenerateAuthenticationConfiguration(&tt.auth.Spec) if tt.expectError && err == nil { t.Fatalf("expected error but didn't get any") } diff --git a/pkg/controllers/externaloidc/generation/oauthapiserver/generate.go b/pkg/controllers/externaloidc/generation/oauthapiserver/generate.go index f78c0016b9..e437b67968 100644 --- a/pkg/controllers/externaloidc/generation/oauthapiserver/generate.go +++ b/pkg/controllers/externaloidc/generation/oauthapiserver/generate.go @@ -13,8 +13,6 @@ import ( "time" configv1 "github.com/openshift/api/config/v1" - "github.com/openshift/api/features" - "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" "github.com/openshift/library-go/pkg/operator/resource/retry" authenticationv1alpha1 "github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1" @@ -22,7 +20,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" authenticationcel "k8s.io/apiserver/pkg/authentication/cel" - corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/util/cert" "k8s.io/utils/ptr" @@ -40,32 +37,58 @@ type oidcGenerationState struct { } const ( - configNamespace = "openshift-config" kindAuthenticationConfiguration = "AuthenticationConfiguration" oidcDiscoveryEndpointPath = "/.well-known/openid-configuration" ) type validationFunc func(*authenticationv1alpha1.AuthenticationConfiguration) error +// ResolverFunc resolves a named resource to its string content. Callers provide +// implementations that encapsulate how and where the data is fetched (e.g. from +// a ConfigMap lister, a Secret lister, or any other source). +type ResolverFunc func(name string) (string, error) + type AuthenticationConfigurationGenerator struct { - configMapLister corev1listers.ConfigMapLister - secretLister corev1listers.SecretLister - featureGates featuregates.FeatureGate - validationFn validationFunc + caResolver ResolverFunc + clientSecretResolver ResolverFunc + validationFn validationFunc + withUpstreamParity bool + withAdditionalClaimMappings bool + withExternalClaimsSourcing bool } -func NewAuthenticationConfigurationGenerator(cmlister corev1listers.ConfigMapLister, secretLister corev1listers.SecretLister, gates featuregates.FeatureGate) *AuthenticationConfigurationGenerator { +func NewAuthenticationConfigurationGenerator(caResolver, clientSecretResolver ResolverFunc) *AuthenticationConfigurationGenerator { return &AuthenticationConfigurationGenerator{ - configMapLister: cmlister, - secretLister: secretLister, - featureGates: gates, - validationFn: validateOAuthApiserverAuthenticationConfiguration, + caResolver: caResolver, + clientSecretResolver: clientSecretResolver, + validationFn: validateOAuthApiserverAuthenticationConfiguration, } } +// WithUpstreamParity enables upstream-parity features: CEL expression support +// for username and groups claim mappings, email_verified validation, and user +// validation rules. +func (acg *AuthenticationConfigurationGenerator) WithUpstreamParity() *AuthenticationConfigurationGenerator { + acg.withUpstreamParity = true + return acg +} + +// WithAdditionalClaimMappings enables generation of UID and extra claim mappings. +func (acg *AuthenticationConfigurationGenerator) WithAdditionalClaimMappings() *AuthenticationConfigurationGenerator { + acg.withAdditionalClaimMappings = true + return acg +} + +// WithExternalClaimsSourcing enables generation of external claims sources, +// which allow fetching additional claims from external endpoints. +func (acg *AuthenticationConfigurationGenerator) WithExternalClaimsSourcing() *AuthenticationConfigurationGenerator { + acg.withExternalClaimsSourcing = true + return acg +} + // GenerateAuthenticationConfiguration creates a structured JWT AuthenticationConfiguration for OIDC // in the oauth-apiserver from the configuration found in the authentication/cluster resource. -func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfiguration(auth *configv1.Authentication) (runtime.Object, error) { +func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfiguration(authSpec *configv1.AuthenticationSpec) (runtime.Object, error) { authConfig := &authenticationv1alpha1.AuthenticationConfiguration{ TypeMeta: metav1.TypeMeta{ Kind: kindAuthenticationConfiguration, @@ -74,8 +97,8 @@ func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfigura } errs := []error{} - for _, provider := range auth.Spec.OIDCProviders { - jwt, err := generateJWTForProvider(provider, acg.configMapLister, acg.secretLister, acg.featureGates, auth.Spec.ServiceAccountIssuer) + for _, provider := range authSpec.OIDCProviders { + jwt, err := acg.generateJWTForProvider(provider, authSpec.ServiceAccountIssuer) if err != nil { errs = append(errs, err) continue @@ -97,17 +120,17 @@ func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfigura return authConfig, nil } -func generateJWTForProvider(provider configv1.OIDCProvider, configMapLister corev1listers.ConfigMapLister, secretLister corev1listers.SecretLister, featureGates featuregates.FeatureGate, serviceAccountIssuer string) (authenticationv1alpha1.JWTAuthenticator, error) { +func (acg *AuthenticationConfigurationGenerator) generateJWTForProvider(provider configv1.OIDCProvider, serviceAccountIssuer string) (authenticationv1alpha1.JWTAuthenticator, error) { out := authenticationv1alpha1.JWTAuthenticator{} - issuer, err := generateIssuer(provider.Issuer, configMapLister, serviceAccountIssuer) + issuer, err := acg.generateIssuer(provider.Issuer, serviceAccountIssuer) if err != nil { return authenticationv1alpha1.JWTAuthenticator{}, fmt.Errorf("generating issuer for provider %q: %v", provider.Name, err) } state := &oidcGenerationState{} - claimMappings, err := generateClaimMappings(provider.ClaimMappings, issuer.URL, featureGates, state) + claimMappings, err := acg.generateClaimMappings(provider.ClaimMappings, issuer.URL, state) if err != nil { return authenticationv1alpha1.JWTAuthenticator{}, fmt.Errorf("generating claimMappings for provider %q: %v", provider.Name, err) } @@ -117,7 +140,7 @@ func generateJWTForProvider(provider configv1.OIDCProvider, configMapLister core return authenticationv1alpha1.JWTAuthenticator{}, fmt.Errorf("generating claimValidationRules for provider %q: %v", provider.Name, err) } - if featureGates.Enabled(features.FeatureGateExternalOIDCWithUpstreamParity) { + if acg.withUpstreamParity { if err := validateEmailVerifiedUsage(state); err != nil { return authenticationv1alpha1.JWTAuthenticator{}, fmt.Errorf("validating email claim usage for provider %q: %v", provider.Name, err) } @@ -129,8 +152,8 @@ func generateJWTForProvider(provider configv1.OIDCProvider, configMapLister core out.UserValidationRules = userValidationRules } - if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { - externalClaimsSources, err := generateExternalClaimsSources(configMapLister, secretLister, provider.ExternalClaimsSources...) + if acg.withExternalClaimsSourcing { + externalClaimsSources, err := acg.generateExternalClaimsSources(provider.ExternalClaimsSources...) if err != nil { return authenticationv1alpha1.JWTAuthenticator{}, fmt.Errorf("generating externalClaimsSources for provider %q: %v", provider.Name, err) } @@ -145,7 +168,7 @@ func generateJWTForProvider(provider configv1.OIDCProvider, configMapLister core return out, nil } -func generateIssuer(issuer configv1.TokenIssuer, configMapLister corev1listers.ConfigMapLister, serviceAccountIssuer string) (authenticationv1alpha1.Issuer, error) { +func (acg *AuthenticationConfigurationGenerator) generateIssuer(issuer configv1.TokenIssuer, serviceAccountIssuer string) (authenticationv1alpha1.Issuer, error) { out := authenticationv1alpha1.Issuer{} if len(serviceAccountIssuer) > 0 { @@ -187,7 +210,7 @@ func generateIssuer(issuer configv1.TokenIssuer, configMapLister corev1listers.C out.DiscoveryURL = issuer.DiscoveryURL } if len(issuer.CertificateAuthority.Name) > 0 { - ca, err := getCertificateAuthorityFromConfigMap(issuer.CertificateAuthority.Name, configMapLister) + ca, err := acg.getCertificateAuthority(issuer.CertificateAuthority.Name) if err != nil { return authenticationv1alpha1.Issuer{}, fmt.Errorf("getting CertificateAuthority for issuer: %v", err) } @@ -197,41 +220,31 @@ func generateIssuer(issuer configv1.TokenIssuer, configMapLister corev1listers.C return out, nil } -func getCertificateAuthorityFromConfigMap(name string, configMapLister corev1listers.ConfigMapLister) (string, error) { +func (acg *AuthenticationConfigurationGenerator) getCertificateAuthority(name string) (string, error) { if len(name) == 0 { return "", nil } - caConfigMap, err := configMapLister.ConfigMaps(configNamespace).Get(name) - if err != nil { - return "", fmt.Errorf("could not retrieve auth configmap %s/%s to check CA bundle: %v", configNamespace, name, err) - } - - caData, ok := caConfigMap.Data["ca-bundle.crt"] - if !ok || len(caData) == 0 { - return "", fmt.Errorf("configmap %s/%s key \"ca-bundle.crt\" missing or empty", configNamespace, name) - } - - return caData, nil + return acg.caResolver(name) } -func generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL string, featureGates featuregates.FeatureGate, state *oidcGenerationState) (authenticationv1alpha1.ClaimMappings, error) { +func (acg *AuthenticationConfigurationGenerator) generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL string, state *oidcGenerationState) (authenticationv1alpha1.ClaimMappings, error) { out := authenticationv1alpha1.ClaimMappings{} - username, usernameResult, err := generateUsernameClaimMapping(claimMappings.Username, issuerURL, featureGates) + username, usernameResult, err := generateUsernameClaimMapping(claimMappings.Username, issuerURL, acg.withUpstreamParity) if err != nil { return authenticationv1alpha1.ClaimMappings{}, fmt.Errorf("generating username claim mapping: %v", err) } state.UsernameResult = usernameResult - groups, err := generateGroupsClaimMapping(claimMappings.Groups, featureGates) + groups, err := generateGroupsClaimMapping(claimMappings.Groups, acg.withUpstreamParity) if err != nil { return authenticationv1alpha1.ClaimMappings{}, fmt.Errorf("generating group claim mapping: %v", err) } out.Username = username out.Groups = groups - if featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings) { + if acg.withAdditionalClaimMappings { uid, err := generateUIDClaimMapping(claimMappings.UID) if err != nil { return authenticationv1alpha1.ClaimMappings{}, fmt.Errorf("generating uid claim mapping: %v", err) @@ -250,8 +263,8 @@ func generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL return out, nil } -func generateUsernameClaimMapping(usernameClaimMapping configv1.UsernameClaimMapping, issuerURL string, featureGates featuregates.FeatureGate) (authenticationv1alpha1.PrefixedClaimOrExpression, *authenticationcel.CompilationResult, error) { - if featureGates.Enabled(features.FeatureGateExternalOIDCWithUpstreamParity) { +func generateUsernameClaimMapping(usernameClaimMapping configv1.UsernameClaimMapping, issuerURL string, withUpstreamParity bool) (authenticationv1alpha1.PrefixedClaimOrExpression, *authenticationcel.CompilationResult, error) { + if withUpstreamParity { return generateUsernameClaimMappingWithParity(usernameClaimMapping, issuerURL) } return generateUsernameClaimMappingLegacy(usernameClaimMapping, issuerURL) @@ -339,9 +352,9 @@ func generateUsernameClaimMappingLegacy(usernameClaimMapping configv1.UsernameCl return out, nil, nil } -func generateGroupsClaimMapping(groupsMapping configv1.PrefixedClaimMapping, featureGates featuregates.FeatureGate) (authenticationv1alpha1.PrefixedClaimOrExpression, error) { +func generateGroupsClaimMapping(groupsMapping configv1.PrefixedClaimMapping, withUpstreamParity bool) (authenticationv1alpha1.PrefixedClaimOrExpression, error) { out := authenticationv1alpha1.PrefixedClaimOrExpression{} - if featureGates.Enabled(features.FeatureGateExternalOIDCWithUpstreamParity) { + if withUpstreamParity { if len(groupsMapping.Expression) > 0 && len(groupsMapping.Claim) > 0 { return out, fmt.Errorf("groups claim mapping must not set both claim and expression") } @@ -767,11 +780,11 @@ func isConstField(exp *exprpb.Expr, field string) bool { return c != nil && c.GetStringValue() == field } -func generateExternalClaimsSources(cmLister corev1listers.ConfigMapLister, secretLister corev1listers.SecretLister, sources ...configv1.ExternalClaimsSource) ([]authenticationv1alpha1.ExternalClaimsSource, error) { +func (acg *AuthenticationConfigurationGenerator) generateExternalClaimsSources(sources ...configv1.ExternalClaimsSource) ([]authenticationv1alpha1.ExternalClaimsSource, error) { out := []authenticationv1alpha1.ExternalClaimsSource{} seenClaimNames := sets.New[string]() for _, source := range sources { - externalSource, err := generateExternalClaimsSource(source, cmLister, secretLister, seenClaimNames) + externalSource, err := acg.generateExternalClaimsSource(source, seenClaimNames) if err != nil { return nil, err } @@ -784,8 +797,8 @@ func generateExternalClaimsSources(cmLister corev1listers.ConfigMapLister, secre return out, nil } -func generateExternalClaimsSource(source configv1.ExternalClaimsSource, cmLister corev1listers.ConfigMapLister, secretLister corev1listers.SecretLister, seenClaimNames sets.Set[string]) (*authenticationv1alpha1.ExternalClaimsSource, error) { - authentication, err := generateExternalClaimsSourceAuthentication(source.Authentication, secretLister, cmLister) +func (acg *AuthenticationConfigurationGenerator) generateExternalClaimsSource(source configv1.ExternalClaimsSource, seenClaimNames sets.Set[string]) (*authenticationv1alpha1.ExternalClaimsSource, error) { + authentication, err := acg.generateExternalClaimsSourceAuthentication(source.Authentication) if err != nil { return nil, err } @@ -793,7 +806,7 @@ func generateExternalClaimsSource(source configv1.ExternalClaimsSource, cmLister zeroValueExternalSourceTLS := configv1.ExternalSourceTLS{} var tls *authenticationv1alpha1.TLS if source.TLS != zeroValueExternalSourceTLS { - tls, err = generateExternalClaimsSourceTLS(source.TLS, cmLister) + tls, err = acg.generateExternalClaimsSourceTLS(source.TLS) if err != nil { return nil, err } @@ -823,7 +836,7 @@ func generateExternalClaimsSource(source configv1.ExternalClaimsSource, cmLister }, nil } -func generateExternalClaimsSourceAuthentication(externalSourceAuthentication configv1.ExternalSourceAuthentication, secretLister corev1listers.SecretLister, cmLister corev1listers.ConfigMapLister) (*authenticationv1alpha1.Authentication, error) { +func (acg *AuthenticationConfigurationGenerator) generateExternalClaimsSourceAuthentication(externalSourceAuthentication configv1.ExternalSourceAuthentication) (*authenticationv1alpha1.Authentication, error) { switch externalSourceAuthentication.Type { case "": // signals the omitted case which is valid and means to use anonymous auth. This means we should omit it as well so anonymous auth takes place. return nil, nil @@ -832,7 +845,7 @@ func generateExternalClaimsSourceAuthentication(externalSourceAuthentication con Type: ptr.To(authenticationv1alpha1.AuthenticationTypeRequestProvidedToken), }, nil case configv1.ExternalSourceAuthenticationTypeClientCredential: - cc, err := generateExternalClaimsSourceAuthenticationClientCredential(externalSourceAuthentication.ClientCredential, secretLister, cmLister) + cc, err := acg.generateExternalClaimsSourceAuthenticationClientCredential(externalSourceAuthentication.ClientCredential) if err != nil { return nil, fmt.Errorf("generating client credentials configuration: %w", err) } @@ -846,7 +859,7 @@ func generateExternalClaimsSourceAuthentication(externalSourceAuthentication con } } -func generateExternalClaimsSourceAuthenticationClientCredential(clientCredentialConfig configv1.ClientCredentialConfig, secretLister corev1listers.SecretLister, cmLister corev1listers.ConfigMapLister) (*authenticationv1alpha1.ClientCredentialConfig, error) { +func (acg *AuthenticationConfigurationGenerator) generateExternalClaimsSourceAuthenticationClientCredential(clientCredentialConfig configv1.ClientCredentialConfig) (*authenticationv1alpha1.ClientCredentialConfig, error) { // TODO: enable validation when it is possible to do so. Currently blocked // due to oauth-apiserver not being rebased on 1.35 and the KAS library changes // not existing in the 1.35 branch. @@ -864,7 +877,7 @@ func generateExternalClaimsSourceAuthenticationClientCredential(clientCredential } */ - clientSecret, err := getClientSecretFromSecret(clientCredentialConfig.ClientSecret.Name, secretLister) + clientSecret, err := acg.getClientSecret(clientCredentialConfig.ClientSecret.Name) if err != nil { return nil, fmt.Errorf("getting client secret: %w", err) } @@ -889,7 +902,7 @@ func generateExternalClaimsSourceAuthenticationClientCredential(clientCredential var certificateAuthority *string if len(clientCredentialConfig.TLS.CertificateAuthority.Name) > 0 { - ca, err := getCertificateAuthorityFromConfigMap(clientCredentialConfig.TLS.CertificateAuthority.Name, cmLister) + ca, err := acg.getCertificateAuthority(clientCredentialConfig.TLS.CertificateAuthority.Name) if err != nil { return nil, fmt.Errorf("getting certificate authority: %w", err) } @@ -933,22 +946,12 @@ func generateClientCredentialScopes(scopes ...configv1.OAuth2Scope) ([]string, e return out, errors.Join(errs...) } -func getClientSecretFromSecret(name string, secretLister corev1listers.SecretLister) (string, error) { - secret, err := secretLister.Secrets(configNamespace).Get(name) - if err != nil { - return "", fmt.Errorf("could not retrieve auth secret %s/%s to get client secret: %v", configNamespace, name, err) - } - - clientSecret, ok := secret.Data["client-secret"] - if !ok || len(clientSecret) == 0 { - return "", fmt.Errorf("secret %s/%s key \"client-secret\" missing or empty", configNamespace, name) - } - - return string(clientSecret), nil +func (acg *AuthenticationConfigurationGenerator) getClientSecret(name string) (string, error) { + return acg.clientSecretResolver(name) } -func generateExternalClaimsSourceTLS(externalSourceTLS configv1.ExternalSourceTLS, cmLister corev1listers.ConfigMapLister) (*authenticationv1alpha1.TLS, error) { - caData, err := getCertificateAuthorityFromConfigMap(externalSourceTLS.CertificateAuthority.Name, cmLister) +func (acg *AuthenticationConfigurationGenerator) generateExternalClaimsSourceTLS(externalSourceTLS configv1.ExternalSourceTLS) (*authenticationv1alpha1.TLS, error) { + caData, err := acg.getCertificateAuthority(externalSourceTLS.CertificateAuthority.Name) if err != nil { return nil, fmt.Errorf("getting certificate authority for external source: %w", err) } diff --git a/pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go b/pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go index 6b7c5efddc..afce3dee49 100644 --- a/pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go +++ b/pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go @@ -23,8 +23,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" configv1 "github.com/openshift/api/config/v1" - "github.com/openshift/api/features" - "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" authenticationv1alpha1 "github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1" corev1 "k8s.io/api/core/v1" @@ -35,6 +33,8 @@ import ( "k8s.io/utils/ptr" ) +const configNamespace = "openshift-config" + var ( testCertData = "fake-ca-cert" @@ -171,51 +171,32 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration secretIndexer cache.Indexer configValidator validationFunc - expectedAuthConfig *authenticationv1alpha1.AuthenticationConfiguration - expectError bool - featureGates featuregates.FeatureGate + expectedAuthConfig *authenticationv1alpha1.AuthenticationConfiguration + expectError bool + withUpstreamParity bool + withAdditionalClaimMappings bool + withExternalClaimsSourcing bool }{ { name: "ca bundle configmap lister error", auth: baseAuthResource, configMapIndexer: cache.Indexer(&everFailingIndexer{}), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "ca bundle configmap without required key", auth: baseAuthResource, caBundleConfigMap: &caBundleConfigMapInvalidKey, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "ca bundle configmap with no data", auth: baseAuthResource, caBundleConfigMap: &caBundleConfigMapNoData, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config nil prefix when required", @@ -232,14 +213,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config invalid prefix policy", @@ -255,14 +229,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with nil claim in validation rule", @@ -284,14 +251,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "valid auth config", @@ -309,14 +269,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "valid auth config during generation, validator fails", @@ -329,14 +282,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + configValidator: func(_ *authenticationv1alpha1.AuthenticationConfiguration) error { return errors.New("boom") }, @@ -358,14 +304,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with default prefix policy", @@ -393,14 +332,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with default prefix policy and username claim email", @@ -428,14 +360,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with no prefix policy", @@ -463,14 +388,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with username claim prefix", @@ -501,14 +419,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with empty string for username claim", @@ -523,14 +434,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, { name: "auth config with no uid claim or expression", @@ -544,14 +448,9 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{}, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, + withExternalClaimsSourcing: true, }, { name: "auth config with uid claim and expression", @@ -567,15 +466,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with uid expression", @@ -599,15 +491,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with extra missing key", @@ -624,15 +509,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with extra missing valueExpression", @@ -649,15 +527,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with valid extra mappings", @@ -688,15 +559,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "invalid discovery URL (http instead of https)", @@ -707,12 +571,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, // ensure CA bundle exists expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (identical to issuer URL)", @@ -724,12 +583,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (identical to issuer URL except trailing slash)", @@ -741,12 +595,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (missing host)", @@ -758,12 +607,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (contains user info)", @@ -775,12 +619,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (contains query string)", @@ -792,12 +631,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (contains fragment)", @@ -809,12 +643,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "invalid discovery URL (parse error)", @@ -826,12 +655,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }), caBundleConfigMap: &baseCABundleConfigMap, expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "user validation rule invalid expression", @@ -846,12 +670,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, }, { name: "auth config with invalid username expression, error", @@ -866,15 +685,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with invalid groups expression, error", @@ -891,15 +703,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username expression mapping", @@ -926,15 +731,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with groups expression mapping", @@ -963,15 +761,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username claim and expression both set, error", @@ -987,15 +778,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with groups claim and expression both set, error", @@ -1013,15 +797,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username expression and prefix set, error", @@ -1040,15 +817,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with groups expression and prefix set, error", @@ -1066,15 +836,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username expression using claims.email without claims.email_verified, error", @@ -1089,15 +852,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username expression using claims.email with claims.email_verified in claimValidationRule, success", @@ -1139,15 +895,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username expression using both claims.email and claims.email_verified, success", @@ -1174,15 +923,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "auth config with username expression using claims.email with claims.email_verified in extra, success", @@ -1223,15 +965,8 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + withUpstreamParity: true, + withAdditionalClaimMappings: true, }, { name: "valid auth config with external claims source using request provided token auth and conditions, success", @@ -1316,15 +1051,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { name: "valid auth config with external claims source using request provided token auth and conditions, no ca bundle specified, success", @@ -1390,15 +1117,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { name: "valid auth config with external claims source using anonymous auth, success", @@ -1467,15 +1186,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { name: "valid auth config with external claims source using client credential auth", @@ -1594,15 +1305,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { name: "auth config with external claims source with unknown auth type, error", @@ -1637,15 +1340,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { name: "auth config with external claims source with missing TLS CA configmap, error", @@ -1680,15 +1375,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { name: "auth config with external claims source with client secret key missing in secret, error", @@ -1743,15 +1430,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, { @@ -1779,14 +1458,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: false, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{}, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - ), + }, // TODO: Add tests for validating currently unvalidated fields due to dependency issues (CEL expression validation) // The following jira tickets track the work necessary to eventually enable this validation: @@ -1860,15 +1532,7 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration }, }), expectError: true, - featureGates: featuregates.NewFeatureGate( - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCExternalClaimsSourcing, - }, - []configv1.FeatureGateName{ - features.FeatureGateExternalOIDCWithAdditionalClaimMappings, - features.FeatureGateExternalOIDCWithUpstreamParity, - }, - ), + withExternalClaimsSourcing: true, }, */ } { @@ -1881,10 +1545,22 @@ func TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration tt.configMapIndexer.Add(tt.caBundleConfigMap) } - c := NewAuthenticationConfigurationGenerator(corev1listers.NewConfigMapLister(tt.configMapIndexer), corev1listers.NewSecretLister(tt.secretIndexer), tt.featureGates) + c := NewAuthenticationConfigurationGenerator( + newTestCABundleResolver(corev1listers.NewConfigMapLister(tt.configMapIndexer)), + newTestClientSecretResolver(corev1listers.NewSecretLister(tt.secretIndexer)), + ) + if tt.withUpstreamParity { + c.WithUpstreamParity() + } + if tt.withAdditionalClaimMappings { + c.WithAdditionalClaimMappings() + } + if tt.withExternalClaimsSourcing { + c.WithExternalClaimsSourcing() + } c.validationFn = tt.configValidator - gotConfig, err := c.GenerateAuthenticationConfiguration(&tt.auth) + gotConfig, err := c.GenerateAuthenticationConfiguration(&tt.auth.Spec) if tt.expectError && err == nil { t.Fatalf("expected error but didn't get any") } @@ -2222,3 +1898,31 @@ func generateServingCert(caCert *x509.Certificate, caPrivateKey crypto.Signer) ( return &serverCert, nil } + +func newTestCABundleResolver(cmLister corev1listers.ConfigMapLister) ResolverFunc { + return func(name string) (string, error) { + cm, err := cmLister.ConfigMaps(configNamespace).Get(name) + if err != nil { + return "", fmt.Errorf("could not retrieve configmap %s/%s: %v", configNamespace, name, err) + } + caData, ok := cm.Data["ca-bundle.crt"] + if !ok || len(caData) == 0 { + return "", fmt.Errorf("configmap %s/%s key \"ca-bundle.crt\" missing or empty", configNamespace, name) + } + return caData, nil + } +} + +func newTestClientSecretResolver(secretLister corev1listers.SecretLister) ResolverFunc { + return func(name string) (string, error) { + secret, err := secretLister.Secrets(configNamespace).Get(name) + if err != nil { + return "", fmt.Errorf("could not retrieve secret %s/%s: %v", configNamespace, name, err) + } + clientSecret, ok := secret.Data["client-secret"] + if !ok || len(clientSecret) == 0 { + return "", fmt.Errorf("secret %s/%s key \"client-secret\" missing or empty", configNamespace, name) + } + return string(clientSecret), nil + } +}