diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go index 1e056cd2a128..714395d4f354 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go @@ -8,7 +8,6 @@ import ( "net/url" "strings" - hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/control-plane-operator/featuregates" component "github.com/openshift/hypershift/support/controlplane-component" "github.com/openshift/hypershift/support/supportedversion" @@ -37,7 +36,7 @@ func adaptAuthConfig(cpContext component.WorkloadContext, config *corev1.ConfigM return nil } - authConfig, err := generateAuthConfig(cpContext, cpContext.Client, cpContext.HCP) + authConfig, err := GenerateAuthConfig(cpContext, cpContext.HCP.Spec.Configuration.Authentication, cpContext.Client, cpContext.HCP.Namespace) if err != nil { return fmt.Errorf("failed to generate authentication config: %w", err) } @@ -59,7 +58,11 @@ func adaptAuthConfig(cpContext component.WorkloadContext, config *corev1.ConfigM return nil } -func generateAuthConfig(ctx context.Context, c crclient.Reader, hcp *hyperv1.HostedControlPlane) (*AuthenticationConfiguration, error) { +func GenerateAuthConfig(ctx context.Context, spec *configv1.AuthenticationSpec, c crclient.Reader, namespace string) (*AuthenticationConfiguration, error) { + if spec == nil { + return nil, fmt.Errorf("authentication spec cannot be nil") + } + config := &AuthenticationConfiguration{ TypeMeta: metav1.TypeMeta{ Kind: "AuthenticationConfiguration", @@ -68,8 +71,8 @@ func generateAuthConfig(ctx context.Context, c crclient.Reader, hcp *hyperv1.Hos JWT: []JWTAuthenticator{}, } - for _, provider := range hcp.Spec.Configuration.Authentication.OIDCProviders { - jwt, err := generateJWTForProvider(ctx, provider, c, hcp.Namespace) + for _, provider := range spec.OIDCProviders { + jwt, err := generateJWTForProvider(ctx, provider, c, namespace) if err != nil { return nil, fmt.Errorf("generating JWT authenticator for provider %q: %v", provider.Name, err) } diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go index 65c4ed51f8c1..85177fda298f 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go @@ -1,6 +1,7 @@ package kas import ( + "context" "encoding/json" "testing" @@ -60,6 +61,184 @@ MlubpsoEK2bYQDZskgDGCHI= -----END CERTIFICATE----- ` +func TestGenerateAuthConfig(t *testing.T) { + type testCase struct { + name string + ctx context.Context + spec *configv1.AuthenticationSpec + client crclient.Reader + namespace string + expectedAuthenticationConfiguration *AuthenticationConfiguration + shouldError bool + featureGates []featuregate.Feature + } + + testCases := []testCase{ + { + name: "When authentication spec is nil, it should return an error", + ctx: context.Background(), + spec: nil, + client: nil, + namespace: "test", + shouldError: true, + }, + { + name: "When valid OIDC provider is provided, it should generate valid authentication configuration", + ctx: context.Background(), + spec: &configv1.AuthenticationSpec{ + OIDCProviders: []configv1.OIDCProvider{ + { + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://test.example.com", + Audiences: []configv1.TokenAudience{ + "test-audience", + }, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + }, + }, + }, + client: fake.NewClientBuilder().Build(), + namespace: "test-namespace", + expectedAuthenticationConfiguration: &AuthenticationConfiguration{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiserver.config.k8s.io/v1alpha1", + Kind: "AuthenticationConfiguration", + }, + JWT: []JWTAuthenticator{ + { + Issuer: Issuer{ + URL: "https://test.example.com", + AudienceMatchPolicy: AudienceMatchPolicyMatchAny, + Audiences: []string{"test-audience"}, + }, + ClaimMappings: ClaimMappings{ + Username: PrefixedClaimOrExpression{ + Prefix: ptr.To(""), + Claim: "email", + }, + Groups: PrefixedClaimOrExpression{ + Prefix: ptr.To(""), + Claim: "", + }, + UID: ClaimOrExpression{ + Claim: "sub", + }, + Extra: []ExtraMapping{}, + }, + ClaimValidationRules: []ClaimValidationRule{}, + }, + }, + }, + shouldError: false, + }, + { + name: "When OIDC provider with CEL validation is provided, it should generate configuration with validation rules", + ctx: context.Background(), + spec: &configv1.AuthenticationSpec{ + OIDCProviders: []configv1.OIDCProvider{ + { + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://test.example.com", + Audiences: []configv1.TokenAudience{ + "test-audience", + }, + }, + ClaimValidationRules: []configv1.TokenClaimValidationRule{ + { + Type: configv1.TokenValidationRuleTypeCEL, + CEL: configv1.TokenClaimValidationCELRule{ + Expression: "claims.email_verified == true", + Message: "email must be verified", + }, + }, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Expression: "claims.email", + }, + }, + }, + }, + }, + client: fake.NewClientBuilder().Build(), + namespace: "test-namespace", + featureGates: []featuregate.Feature{ + featuregates.ExternalOIDCWithUpstreamParity, + }, + expectedAuthenticationConfiguration: &AuthenticationConfiguration{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiserver.config.k8s.io/v1alpha1", + Kind: "AuthenticationConfiguration", + }, + JWT: []JWTAuthenticator{ + { + Issuer: Issuer{ + URL: "https://test.example.com", + AudienceMatchPolicy: AudienceMatchPolicyMatchAny, + Audiences: []string{"test-audience"}, + }, + ClaimMappings: ClaimMappings{ + Username: PrefixedClaimOrExpression{ + Expression: "claims.email", + }, + Groups: PrefixedClaimOrExpression{ + Prefix: ptr.To(""), + Claim: "", + }, + UID: ClaimOrExpression{ + Claim: "sub", + }, + Extra: []ExtraMapping{}, + }, + ClaimValidationRules: []ClaimValidationRule{ + { + Expression: "claims.email_verified == true", + Message: "email must be verified", + }, + }, + UserValidationRules: []UserValidationRule{}, + }, + }, + }, + shouldError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if len(tc.featureGates) > 0 { + for _, feature := range tc.featureGates { + fgtesting.SetFeatureGateDuringTest(t, featuregates.Gate(), feature, true) + } + } + + actualConfig, err := GenerateAuthConfig(tc.ctx, tc.spec, tc.client, tc.namespace) + + switch { + case tc.shouldError && err == nil: + t.Fatal("expected an error to have occurred but got none") + case !tc.shouldError && err != nil: + t.Fatalf("unexpected error: %v", err) + case tc.shouldError && err != nil: + // as expected + return + } + + if diff := cmp.Diff(tc.expectedAuthenticationConfiguration, actualConfig); diff != "" { + t.Fatalf("actual authentication configuration does not match expected (-want +got):\n%s", diff) + } + }) + } +} + func TestAdaptAuthConfig(t *testing.T) { type testCase struct { name string diff --git a/support/validations/authentication.go b/support/validations/authentication.go index 53bbef32646f..12a98f0326a7 100644 --- a/support/validations/authentication.go +++ b/support/validations/authentication.go @@ -4,7 +4,7 @@ import ( "context" "fmt" - "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/kas" + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/kas" "github.com/openshift/hypershift/support/supportedversion" configv1 "github.com/openshift/api/config/v1" @@ -43,7 +43,7 @@ func ValidateAuthenticationSpecForTypeOIDC(ctx context.Context, client crclient. return nil } - authConfig, err := kas.GenerateAuthConfig(authn, ctx, client, namespace) + authConfig, err := kas.GenerateAuthConfig(ctx, authn, client, namespace) if err != nil { return fmt.Errorf("generating structured authentication configuration: %w", err) } diff --git a/support/validations/authentication_test.go b/support/validations/authentication_test.go index ae50dddf8c49..45453cb3c536 100644 --- a/support/validations/authentication_test.go +++ b/support/validations/authentication_test.go @@ -3,8 +3,13 @@ package validations import ( "testing" + "github.com/openshift/hypershift/control-plane-operator/featuregates" + configv1 "github.com/openshift/api/config/v1" + "k8s.io/component-base/featuregate" + fgtesting "k8s.io/component-base/featuregate/testing" + "github.com/stretchr/testify/require" ) @@ -13,11 +18,12 @@ func TestValidateAuthenticationSpec(t *testing.T) { name string authentication *configv1.AuthenticationSpec shouldError bool + featureGates []featuregate.Feature } testcases := []testcase{ { - name: "valid OIDC authentication config", + name: "When valid OIDC authentication config is provided, it should succeed", authentication: &configv1.AuthenticationSpec{ Type: configv1.AuthenticationTypeOIDC, OIDCProviders: []configv1.OIDCProvider{ @@ -60,7 +66,7 @@ func TestValidateAuthenticationSpec(t *testing.T) { shouldError: false, }, { - name: "invalid OIDC authentication config", + name: "When invalid OIDC authentication config is provided, it should return an error", authentication: &configv1.AuthenticationSpec{ Type: configv1.AuthenticationTypeOIDC, OIDCProviders: []configv1.OIDCProvider{ @@ -105,10 +111,86 @@ func TestValidateAuthenticationSpec(t *testing.T) { }, shouldError: true, }, + { + name: "When OIDC provider has CEL validation and username expression, it should succeed", + featureGates: []featuregate.Feature{ + featuregates.ExternalOIDCWithUpstreamParity, + }, + authentication: &configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeOIDC, + OIDCProviders: []configv1.OIDCProvider{ + { + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://test.example.com", + Audiences: []configv1.TokenAudience{ + "test-audience", + }, + }, + ClaimValidationRules: []configv1.TokenClaimValidationRule{ + { + Type: configv1.TokenValidationRuleTypeCEL, + CEL: configv1.TokenClaimValidationCELRule{ + Expression: "claims.email_verified == true", + Message: "email must be verified", + }, + }, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Expression: "has(claims.email) ? claims.email : claims.sub", + }, + }, + }, + }, + }, + shouldError: false, + }, + { + name: "When OIDC provider has invalid CEL expression syntax, it should return an error", + featureGates: []featuregate.Feature{ + featuregates.ExternalOIDCWithUpstreamParity, + }, + authentication: &configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeOIDC, + OIDCProviders: []configv1.OIDCProvider{ + { + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://test.example.com", + Audiences: []configv1.TokenAudience{ + "test-audience", + }, + }, + ClaimValidationRules: []configv1.TokenClaimValidationRule{ + { + Type: configv1.TokenValidationRuleTypeCEL, + CEL: configv1.TokenClaimValidationCELRule{ + Expression: "invalid CEL syntax!!!", + Message: "this should fail", + }, + }, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + }, + }, + }, + shouldError: true, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { + if len(tc.featureGates) > 0 { + for _, feature := range tc.featureGates { + fgtesting.SetFeatureGateDuringTest(t, featuregates.Gate(), feature, true) + } + } err := ValidateAuthenticationSpec(t.Context(), nil, tc.authentication, "foo", []string{}) require.Equal(t, err != nil, tc.shouldError, "expected error state mismatch", "expected an error?", tc.shouldError, "received", err) })