diff --git a/README.md b/README.md index 54ee033c3081..8e233da5277e 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,7 @@ and configure these applications. - [Configuration](#configuration) - [Log mode values](#log-mode-values) - [Use custom application namespace](#use-custom-application-namespace) + - [OIDC Authentication Configuration](#oidc-authentication-configuration) - [Developer Guide](#developer-guide) - [Pre-requisites](#pre-requisites) - [Download manifests](#download-manifests) @@ -117,6 +118,51 @@ To enable it: - For cases in which ODH is already running in the cluster: - WARNING: Be aware that switching to a different application namespace can cause issues that require manual intervention to be fixed, therefore we suggest this to be done for new clusters only. +#### OIDC Authentication Configuration + +When using OIDC authentication mode (e.g., with external identity providers like Keycloak or Auth0), the operator creates a default Auth CR with placeholder values that **must be configured** by cluster administrators. + +**Default Auth CR in OIDC mode:** +```yaml +apiVersion: services.platform.opendatahub.io/v1alpha1 +kind: Auth +metadata: + name: auth +spec: + adminGroups: + - "REPLACE-WITH-OIDC-ADMIN-GROUP" # MUST be replaced with your OIDC admin group + allowedGroups: + - "system:authenticated" +``` + +**To configure Auth for OIDC:** + +Replace the placeholder with your OIDC provider's group names: + ```yaml + apiVersion: services.platform.opendatahub.io/v1alpha1 + kind: Auth + metadata: + name: auth + spec: + adminGroups: + - "my-oidc-admin-group" # Your actual OIDC admin group from your provider + allowedGroups: + - "system:authenticated" + - "my-oidc-users-group" # Optional: add specific OIDC user groups + ``` + +**Important Notes:** +- The groups must match the group names/claims from your OIDC provider (check your OIDC provider's configuration) +- `adminGroups` grants full administrative access to ODH resources via: + - ClusterRoleBinding: `admingroupcluster-rolebinding` and ClusterRole: `admingroupcluster-role` + - RoleBinding: `admingroup-rolebinding` and Role: `admingroup-role` +- `allowedGroups` grants read-only access to ODH resources via: + - ClusterRoleBinding: `allowedgroupcluster-rolebinding` and ClusterRole: `allowedgroupcluster-role` +- OpenShift Groups are NOT needed in OIDC mode (RBAC is managed via Kubernetes RoleBindings/ClusterRoleBindings) +- The groups are mapped from OIDC JWT tokens to Kubernetes RBAC automatically + +For OAuth mode (OpenShift's integrated authentication), the Auth CR is created with platform-specific defaults and typically does not require modification. + ## Developer Guide #### Pre-requisites diff --git a/internal/controller/dscinitialization/auth.go b/internal/controller/dscinitialization/auth.go index 1592e1978403..c1c839364869 100644 --- a/internal/controller/dscinitialization/auth.go +++ b/internal/controller/dscinitialization/auth.go @@ -51,24 +51,38 @@ func (r *DSCInitializationReconciler) CreateAuth(ctx context.Context, platform c return err } // Auth CR not found, create default Auth CR - if err := r.Client.Create(ctx, BuildDefaultAuth(platform)); err != nil && !k8serr.IsAlreadyExists(err) { + if err := r.Client.Create(ctx, BuildDefaultAuth(ctx, r.Client, platform)); err != nil && !k8serr.IsAlreadyExists(err) { return err } return nil } // BuildDefaultAuth creates a default Auth custom resource with platform-specific configuration. +// For OAuth mode, uses platform-specific admin groups (odh-admins, rhods-admins, etc.). +// For OIDC mode, uses a placeholder that cluster admin must replace with actual OIDC group names. // // Parameters: +// - ctx: Context for the operation +// - cli: Kubernetes client to detect authentication mode // - platform: The target platform type (OpenDataHub, SelfManagedRhoai, or ManagedRhoai) // // Returns: -// - client.Object: A serviceApi.Auth resource with platform-specific admin group and system:authenticated in allowed groups -func BuildDefaultAuth(platform common.Platform) client.Object { - // Get admin group for the platform, with fallback to OpenDataHub admin group - adminGroup := adminGroups[platform] - if adminGroup == "" { - adminGroup = adminGroups[cluster.OpenDataHub] +// - client.Object: A serviceApi.Auth resource with appropriate admin group and system:authenticated in allowed groups +func BuildDefaultAuth(ctx context.Context, cli client.Client, platform common.Platform) client.Object { + var adminGroup string + + // Detect authentication mode + isOAuth, err := cluster.IsIntegratedOAuth(ctx, cli) + + if err == nil && !isOAuth { + // OIDC mode: set an non-exist group that cluster admin should replace with actual OIDC group. + adminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" + } else { + // OAuth mode: Use platform-specific admin group + adminGroup = adminGroups[platform] + if adminGroup == "" { // fallback to OpenDataHub admin group. + adminGroup = adminGroups[cluster.OpenDataHub] + } } return &serviceApi.Auth{ diff --git a/internal/controller/dscinitialization/auth_test.go b/internal/controller/dscinitialization/auth_test.go index 27902b5143a9..d5cd3f4ad8c4 100644 --- a/internal/controller/dscinitialization/auth_test.go +++ b/internal/controller/dscinitialization/auth_test.go @@ -63,8 +63,13 @@ func TestBuildDefaultAuth(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) + ctx := t.Context() + + // Create fake client (will be Oauth mode by default) + cli, err := fakeclient.New() + g.Expect(err).ShouldNot(HaveOccurred()) - authObj := dscinitialization.BuildDefaultAuth(tt.platform) + authObj := dscinitialization.BuildDefaultAuth(ctx, cli, tt.platform) g.Expect(authObj).ShouldNot(BeNil(), "BuildDefaultAuth should not return nil") auth, ok := authObj.(*serviceApi.Auth) @@ -85,6 +90,19 @@ func TestBuildDefaultAuth(t *testing.T) { g.Expect(auth.Spec.AllowedGroups[0]).Should(Equal("system:authenticated")) }) } + t.Run("OIDC mode uses placeholder group", func(t *testing.T) { + g := NewWithT(t) + ctx := t.Context() + cli, err := fakeclient.New(fakeclient.WithClusterAuthType(cluster.AuthModeOIDC)) + g.Expect(err).ShouldNot(HaveOccurred()) + obj := dscinitialization.BuildDefaultAuth(ctx, cli, cluster.OpenDataHub) + auth, ok := obj.(*serviceApi.Auth) + g.Expect(ok).To(BeTrue()) + g.Expect(auth.Spec.AdminGroups).To(HaveLen(1)) + g.Expect(auth.Spec.AdminGroups[0]).To(Equal("REPLACE-WITH-OIDC-ADMIN-GROUP")) + // AllowedGroups should remain unchanged + g.Expect(auth.Spec.AllowedGroups).To(Equal([]string{"system:authenticated"})) + }) } func TestCreateAuth(t *testing.T) { diff --git a/internal/controller/services/auth/auth.go b/internal/controller/services/auth/auth.go index 852e9880241d..35725a59b107 100644 --- a/internal/controller/services/auth/auth.go +++ b/internal/controller/services/auth/auth.go @@ -1,36 +1,9 @@ package auth import ( - "context" - - configv1 "github.com/openshift/api/config/v1" - k8serr "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime/schema" - "sigs.k8s.io/controller-runtime/pkg/client" - serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/api/services/v1alpha1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) const ( ServiceName = serviceApi.AuthServiceName ) - -// IsDefaultAuthMethod returns true if the default authentication method is IntegratedOAuth or empty. -// This will give indication that Operator should create userGroups or not in the cluster. -func IsDefaultAuthMethod(ctx context.Context, cli client.Client) (bool, error) { - authenticationobj := &configv1.Authentication{} - if err := cli.Get(ctx, client.ObjectKey{Name: cluster.ClusterAuthenticationObj, Namespace: ""}, authenticationobj); err != nil { - if meta.IsNoMatchError(err) { // when CRD is missing, convert error type - return false, k8serr.NewNotFound(schema.GroupResource{Group: gvk.Auth.Group}, cluster.ClusterAuthenticationObj) - } - return false, err - } - - // for now, HPC support "" "None" "IntegratedOAuth"(default) "OIDC" - // other offering support "" "None" "IntegratedOAuth"(default) - // we only create userGroups for "IntegratedOAuth" or "" and leave other or new supported type value in the future - return authenticationobj.Spec.Type == configv1.AuthenticationTypeIntegratedOAuth || authenticationobj.Spec.Type == "", nil -} diff --git a/internal/controller/services/auth/auth_controller_actions.go b/internal/controller/services/auth/auth_controller_actions.go index b8c8045cc067..3d37f9c926cc 100644 --- a/internal/controller/services/auth/auth_controller_actions.go +++ b/internal/controller/services/auth/auth_controller_actions.go @@ -184,12 +184,12 @@ func addUserGroup(ctx context.Context, rr *odhtypes.ReconciliationRequest, userG } func createDefaultGroup(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { - ok, err := IsDefaultAuthMethod(ctx, rr.Client) + ok, err := cluster.IsIntegratedOAuth(ctx, rr.Client) if err != nil { return err } if !ok { - logf.Log.Info("default auth method is not enabled") + logf.Log.Info("Integrated OAuth not detected; skipping default group creation") return nil } diff --git a/internal/controller/services/auth/auth_controller_test.go b/internal/controller/services/auth/auth_controller_test.go index 30254b5fd3eb..6a6027b5a65e 100644 --- a/internal/controller/services/auth/auth_controller_test.go +++ b/internal/controller/services/auth/auth_controller_test.go @@ -3,17 +3,13 @@ package auth_test import ( "testing" - configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/opendatahub-io/opendatahub-operator/v2/api/common" dsciv2 "github.com/opendatahub-io/opendatahub-operator/v2/api/dscinitialization/v2" serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/api/services/v1alpha1" "github.com/opendatahub-io/opendatahub-operator/v2/internal/controller/services/auth" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/fakeclient" . "github.com/onsi/gomega" ) @@ -104,124 +100,3 @@ func TestServiceHandler_Init(t *testing.T) { err := handler.Init(cluster.OpenDataHub) g.Expect(err).ShouldNot(HaveOccurred()) } - -// TestIsDefaultAuthMethod validates the OpenShift authentication method detection logic. -// This function determines whether the operator should create default admin groups -// based on the cluster's authentication configuration. The test ensures: -// -// 1. IntegratedOAuth is correctly identified as default (should create groups) -// 2. Empty auth type is correctly identified as default (should create groups) -// 3. Custom auth types are correctly identified as non-default (should not create groups) -// 4. Missing authentication objects are handled with proper errors -// -// Authentication Types and Behavior: -// - IntegratedOAuth (default): Create default admin groups -// - "" (empty, default): Create default admin groups -// - "None": Do not create default admin groups -// - Custom types: Do not create default admin groups -// - Missing object: Return error (cluster configuration issue) -// -// This is critical for security because it determines whether the operator will -// automatically create admin groups that could grant elevated access. -func TestIsDefaultAuthMethod(t *testing.T) { - ctx := t.Context() - - tests := []struct { - name string - authObject *configv1.Authentication - expectError bool - expectedResult bool - description string - }{ - { - name: "should return true for IntegratedOAuth", - authObject: &configv1.Authentication{ - ObjectMeta: metav1.ObjectMeta{ - Name: cluster.ClusterAuthenticationObj, - }, - Spec: configv1.AuthenticationSpec{ - Type: configv1.AuthenticationTypeIntegratedOAuth, - }, - }, - expectError: false, - expectedResult: true, - description: "IntegratedOAuth should be considered default auth method", - }, - { - name: "should return true for empty type", - authObject: &configv1.Authentication{ - ObjectMeta: metav1.ObjectMeta{ - Name: cluster.ClusterAuthenticationObj, - }, - Spec: configv1.AuthenticationSpec{ - Type: "", - }, - }, - expectError: false, - expectedResult: true, - description: "Empty type should be considered default auth method", - }, - { - name: "should return false for other auth types", - authObject: &configv1.Authentication{ - ObjectMeta: metav1.ObjectMeta{ - Name: cluster.ClusterAuthenticationObj, - }, - Spec: configv1.AuthenticationSpec{ - Type: "CustomAuth", - }, - }, - expectError: false, - expectedResult: false, - description: "Other auth types should not be considered default auth method", - }, - { - name: "should return false for None", - authObject: &configv1.Authentication{ - ObjectMeta: metav1.ObjectMeta{ - Name: cluster.ClusterAuthenticationObj, - }, - Spec: configv1.AuthenticationSpec{ - Type: configv1.AuthenticationTypeNone, - }, - }, - expectError: false, - expectedResult: false, - description: "None should not be considered default auth method", - }, - { - name: "should handle missing authentication object", - authObject: nil, - expectError: true, - expectedResult: false, - description: "Should return error when authentication object doesn't exist", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - var cli client.Client - var err error - - if tt.authObject != nil { - cli, err = fakeclient.New( - fakeclient.WithObjects(tt.authObject), - ) - } else { - cli, err = fakeclient.New() - } - g.Expect(err).ShouldNot(HaveOccurred()) - - result, err := auth.IsDefaultAuthMethod(ctx, cli) - - if tt.expectError { - g.Expect(err).Should(HaveOccurred(), tt.description) - } else { - g.Expect(err).ShouldNot(HaveOccurred(), tt.description) - g.Expect(result).Should(Equal(tt.expectedResult), tt.description) - } - }) - } -} diff --git a/internal/controller/services/gateway/gateway_auth_actions.go b/internal/controller/services/gateway/gateway_auth_actions.go index 27cdba710e18..9c521edea4c8 100644 --- a/internal/controller/services/gateway/gateway_auth_actions.go +++ b/internal/controller/services/gateway/gateway_auth_actions.go @@ -9,6 +9,7 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/api/services/v1alpha1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) @@ -29,7 +30,8 @@ func createKubeAuthProxyInfrastructure(ctx context.Context, rr *odhtypes.Reconci return fmt.Errorf("failed to resolve domain: %w", err) } - authMode, err := detectClusterAuthMode(ctx, rr) + // Use cluster package function instead of local detectClusterAuthMode + authMode, err := cluster.GetClusterAuthenticationMode(ctx, rr.Client) if err != nil { return fmt.Errorf("failed to detect cluster authentication mode: %w", err) } @@ -43,7 +45,7 @@ func createKubeAuthProxyInfrastructure(ctx context.Context, rr *odhtypes.Reconci } var oidcConfig *serviceApi.OIDCConfig - if authMode == AuthModeOIDC { + if authMode == cluster.AuthModeOIDC { oidcConfig = gatewayConfig.Spec.OIDC } @@ -57,7 +59,7 @@ func createKubeAuthProxyInfrastructure(ctx context.Context, rr *odhtypes.Reconci return fmt.Errorf("failed to deploy auth proxy: %w", err) } - if authMode == AuthModeIntegratedOAuth { + if authMode == cluster.AuthModeIntegratedOAuth { if err := createOAuthClient(ctx, rr, clientSecret); err != nil { return fmt.Errorf("failed to create OAuth client: %w", err) } diff --git a/internal/controller/services/gateway/gateway_controller.go b/internal/controller/services/gateway/gateway_controller.go index b036981e1cce..ea447a3cd684 100644 --- a/internal/controller/services/gateway/gateway_controller.go +++ b/internal/controller/services/gateway/gateway_controller.go @@ -27,8 +27,8 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/api/common" dsciv2 "github.com/opendatahub-io/opendatahub-operator/v2/api/dscinitialization/v2" serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/api/services/v1alpha1" - "github.com/opendatahub-io/opendatahub-operator/v2/internal/controller/services/auth" sr "github.com/opendatahub-io/opendatahub-operator/v2/internal/controller/services/registry" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/gc" @@ -89,7 +89,7 @@ func (h *ServiceHandler) NewReconciler(ctx context.Context, mgr ctrl.Manager) er // Only watch OAuthClient if cluster uses IntegratedOAuth (not OIDC or None) // This prevents errors in ROSA environments where OAuthClient CRD doesn't exist - if isIntegratedOAuth, err := auth.IsDefaultAuthMethod(ctx, mgr.GetClient()); err == nil && isIntegratedOAuth { + if isIntegratedOAuth, err := cluster.IsIntegratedOAuth(ctx, mgr.GetClient()); err == nil && isIntegratedOAuth { reconcilerBuilder = reconcilerBuilder.OwnsGVK(gvk.OAuthClient) // OpenShift OAuth integration } diff --git a/internal/controller/services/gateway/gateway_support.go b/internal/controller/services/gateway/gateway_support.go index 7d5f6f8cb66e..cc0345210f02 100644 --- a/internal/controller/services/gateway/gateway_support.go +++ b/internal/controller/services/gateway/gateway_support.go @@ -9,7 +9,6 @@ import ( "os" "strings" - configv1 "github.com/openshift/api/config/v1" oauthv1 "github.com/openshift/api/oauth/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -30,9 +29,6 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) -// AuthMode represents different authentication modes supported by the gateway. -type AuthMode string - const ( // Gateway infrastructure constants. GatewayNamespace = "openshift-ingress" // Namespace where Gateway resources are deployed @@ -69,10 +65,6 @@ const ( EnvClientID = "OAUTH2_PROXY_CLIENT_ID" EnvClientSecret = "OAUTH2_PROXY_CLIENT_SECRET" //nolint:gosec // This is an environment variable name, not a secret EnvCookieSecret = "OAUTH2_PROXY_COOKIE_SECRET" //nolint:gosec // This is an environment variable name, not a secret - - AuthModeIntegratedOAuth AuthMode = "IntegratedOAuth" - AuthModeOIDC AuthMode = "OIDC" - AuthModeNone AuthMode = "None" ) var ( @@ -321,29 +313,8 @@ func createGateway(rr *odhtypes.ReconciliationRequest, certSecretName string, do return rr.AddResources(gateway) } -// detectClusterAuthMode determines the authentication mode from cluster configuration. -func detectClusterAuthMode(ctx context.Context, rr *odhtypes.ReconciliationRequest) (AuthMode, error) { - auth := &configv1.Authentication{} - err := rr.Client.Get(ctx, types.NamespacedName{Name: "cluster"}, auth) - if err != nil { - return "", fmt.Errorf("failed to get cluster authentication config: %w", err) - } - - switch auth.Spec.Type { - case "OIDC": - return AuthModeOIDC, nil - case "IntegratedOAuth", "": - // empty string is equivalent to IntegratedOAuth (default) - return AuthModeIntegratedOAuth, nil - case "None": - return AuthModeNone, nil - default: - return AuthModeIntegratedOAuth, nil - } -} - -func validateOIDCConfig(authMode AuthMode, oidcConfig *serviceApi.OIDCConfig) *common.Condition { - if authMode != AuthModeOIDC { +func validateOIDCConfig(authMode cluster.AuthenticationMode, oidcConfig *serviceApi.OIDCConfig) *common.Condition { + if authMode != cluster.AuthModeOIDC { return nil } @@ -378,8 +349,8 @@ func validateOIDCConfig(authMode AuthMode, oidcConfig *serviceApi.OIDCConfig) *c return nil } -func checkAuthModeNone(authMode AuthMode) *common.Condition { - if authMode == AuthModeNone { +func checkAuthModeNone(authMode cluster.AuthenticationMode) *common.Condition { + if authMode == cluster.AuthModeNone { return &common.Condition{ Type: status.ConditionTypeReady, Status: metav1.ConditionFalse, @@ -391,7 +362,7 @@ func checkAuthModeNone(authMode AuthMode) *common.Condition { } // getOrGenerateSecrets retrieves existing secrets or generates new ones for OAuth2 proxy. -func getOrGenerateSecrets(ctx context.Context, rr *odhtypes.ReconciliationRequest, authMode AuthMode) (string, string, error) { +func getOrGenerateSecrets(ctx context.Context, rr *odhtypes.ReconciliationRequest, authMode cluster.AuthenticationMode) (string, string, error) { existingSecret := &corev1.Secret{} secretErr := rr.Client.Get(ctx, types.NamespacedName{ Name: KubeAuthProxySecretsName, @@ -414,7 +385,7 @@ func getOrGenerateSecrets(ctx context.Context, rr *odhtypes.ReconciliationReques } var clientSecretValue string - if authMode == AuthModeIntegratedOAuth { + if authMode == cluster.AuthModeIntegratedOAuth { clientSecretGen, err := cluster.NewSecret("client-secret", "random", ClientSecretLength) if err != nil { return "", "", fmt.Errorf("failed to generate client secret: %w", err) diff --git a/internal/controller/services/gateway/gateway_support_test.go b/internal/controller/services/gateway/gateway_support_test.go index 90eab75ee95d..7f07cc883e16 100644 --- a/internal/controller/services/gateway/gateway_support_test.go +++ b/internal/controller/services/gateway/gateway_support_test.go @@ -15,6 +15,7 @@ import ( infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/api/infrastructure/v1" serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/api/services/v1alpha1" "github.com/opendatahub-io/opendatahub-operator/v2/internal/controller/status" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" . "github.com/onsi/gomega" @@ -329,14 +330,14 @@ func TestValidateOIDCConfig(t *testing.T) { testCases := []struct { name string - authMode AuthMode + authMode cluster.AuthenticationMode oidcConfig *serviceApi.OIDCConfig expectError bool description string }{ { name: "OIDC mode with valid config", - authMode: AuthModeOIDC, + authMode: cluster.AuthModeOIDC, oidcConfig: &serviceApi.OIDCConfig{ IssuerURL: testOIDCIssuerURL, ClientID: "test-client", @@ -349,14 +350,14 @@ func TestValidateOIDCConfig(t *testing.T) { }, { name: "OIDC mode with missing config", - authMode: AuthModeOIDC, + authMode: cluster.AuthModeOIDC, oidcConfig: nil, expectError: true, description: "should fail when OIDC mode has no configuration", }, { name: "OIDC mode with empty clientID", - authMode: AuthModeOIDC, + authMode: cluster.AuthModeOIDC, oidcConfig: &serviceApi.OIDCConfig{ IssuerURL: testOIDCIssuerURL, ClientID: "", @@ -369,7 +370,7 @@ func TestValidateOIDCConfig(t *testing.T) { }, { name: "OIDC mode with all fields empty", - authMode: AuthModeOIDC, + authMode: cluster.AuthModeOIDC, oidcConfig: &serviceApi.OIDCConfig{ IssuerURL: "", ClientID: "", @@ -382,14 +383,14 @@ func TestValidateOIDCConfig(t *testing.T) { }, { name: "IntegratedOAuth mode", - authMode: AuthModeIntegratedOAuth, + authMode: cluster.AuthModeIntegratedOAuth, oidcConfig: nil, expectError: false, description: "should pass for IntegratedOAuth mode regardless of OIDC config", }, { name: "None mode", - authMode: AuthModeNone, + authMode: cluster.AuthModeNone, oidcConfig: nil, expectError: false, description: "should pass for None mode regardless of OIDC config", @@ -422,25 +423,25 @@ func TestCheckAuthModeNone(t *testing.T) { testCases := []struct { name string - authMode AuthMode + authMode cluster.AuthenticationMode expectError bool description string }{ { name: "None auth mode", - authMode: AuthModeNone, + authMode: cluster.AuthModeNone, expectError: true, description: "should return condition when auth mode is None", }, { name: "IntegratedOAuth auth mode", - authMode: AuthModeIntegratedOAuth, + authMode: cluster.AuthModeIntegratedOAuth, expectError: false, description: "should return nil for IntegratedOAuth mode", }, { name: "OIDC auth mode", - authMode: AuthModeOIDC, + authMode: cluster.AuthModeOIDC, expectError: false, description: "should return nil for OIDC mode", }, diff --git a/pkg/cluster/cluster_config.go b/pkg/cluster/cluster_config.go index c5a6f07adbed..5ecd9a37cdb5 100644 --- a/pkg/cluster/cluster_config.go +++ b/pkg/cluster/cluster_config.go @@ -15,6 +15,7 @@ import ( "github.com/spf13/viper" corev1 "k8s.io/api/core/v1" k8serr "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -42,6 +43,15 @@ type InstallConfig struct { FIPS bool `json:"fips"` } +// AuthenticationMode represents the cluster authentication mode. +type AuthenticationMode string + +const ( + AuthModeIntegratedOAuth AuthenticationMode = "IntegratedOAuth" + AuthModeOIDC AuthenticationMode = "OIDC" + AuthModeNone AuthenticationMode = "None" +) + // Init initializes cluster configuration variables on startup // init() won't work since it is needed to check the error. func Init(ctx context.Context, cli client.Client) error { @@ -364,3 +374,39 @@ func setManagedMonitoringNamespace(ctx context.Context, cli client.Client) error } return nil } + +// GetClusterAuthenticationMode retrieves and returns the cluster authentication mode. +func GetClusterAuthenticationMode(ctx context.Context, cli client.Client) (AuthenticationMode, error) { + auth := &configv1.Authentication{} + if err := cli.Get(ctx, client.ObjectKey{Name: ClusterAuthenticationObj}, auth); err != nil { + if meta.IsNoMatchError(err) { // CRD missing + return "", k8serr.NewNotFound(schema.GroupResource{ + Group: configv1.GroupName, + Resource: "authentications", + }, ClusterAuthenticationObj) + } + return "", fmt.Errorf("failed to get cluster authentication config: %w", err) + } + + switch auth.Spec.Type { + case "OIDC": + return AuthModeOIDC, nil + case configv1.AuthenticationTypeNone: + return AuthModeNone, nil + case "", configv1.AuthenticationTypeIntegratedOAuth: + // IntegratedOAuth is the default for empty string and explicit IntegratedOAuth + return AuthModeIntegratedOAuth, nil + default: + // Custom/unknown auth types are not IntegratedOAuth + return AuthModeNone, nil + } +} + +// IsIntegratedOAuth returns true if the cluster uses IntegratedOAuth authentication mode which is the default in OCP. +func IsIntegratedOAuth(ctx context.Context, cli client.Client) (bool, error) { + authMode, err := GetClusterAuthenticationMode(ctx, cli) + if err != nil { + return false, err + } + return authMode == AuthModeIntegratedOAuth, nil +} diff --git a/pkg/cluster/cluster_config_test.go b/pkg/cluster/cluster_config_test.go index 4ba6a0d051de..e35f95aa8f9f 100644 --- a/pkg/cluster/cluster_config_test.go +++ b/pkg/cluster/cluster_config_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + configv1 "github.com/openshift/api/config/v1" corev1 "k8s.io/api/core/v1" k8serr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -15,6 +16,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + + . "github.com/onsi/gomega" ) // erroringClient is a wrapper around a client.Client that allows us to inject errors. @@ -32,6 +35,7 @@ func (c *erroringClient) Get(ctx context.Context, key types.NamespacedName, obj } func TestIsFipsEnabled(t *testing.T) { + t.Parallel() var genericError = errors.New("generic client error") // Define test cases @@ -149,6 +153,9 @@ invalid: yaml`, for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + // Create a fake client var fakeClient client.Client if tc.configMap != nil || tc.clientErr != nil { @@ -169,30 +176,163 @@ invalid: yaml`, } // Call the function under test - ctx := t.Context() + ctx := context.Background() result, err := cluster.IsFipsEnabled(ctx, fakeClient) // Check the result - if result != tc.expectedResult { - t.Errorf("IsFIPSEnabled() = %v, want %v", result, tc.expectedResult) - } + g.Expect(result).To(Equal(tc.expectedResult)) - // Check the error. We need to handle nil vs. non-nil errors carefully. + // Check the error if tc.expectedError != nil { - switch { - case err == nil: - t.Errorf("IsFIPSEnabled() error = nil, want %v", tc.expectedError) - case k8serr.IsNotFound(tc.expectedError): - if !k8serr.IsNotFound(err) { - t.Errorf("IsFipsEnabled() error = %v, want NotFound error", err) - } - default: - if err.Error() != tc.expectedError.Error() { - t.Errorf("IsFIPSEnabled() error = %v, want %v", err, tc.expectedError) - } + g.Expect(err).To(HaveOccurred()) + if k8serr.IsNotFound(tc.expectedError) { + g.Expect(k8serr.IsNotFound(err)).To(BeTrue()) + } else { + g.Expect(err.Error()).To(Equal(tc.expectedError.Error())) } - } else if err != nil { - t.Errorf("IsFIPSEnabled() error = %v, want nil", err) + } else { + g.Expect(err).ShouldNot(HaveOccurred()) + } + }) + } +} + +// TestIsIntegratedOAuth validates the OpenShift authentication method detection logic. +// This function determines whether the operator should create default admin groups +// based on the cluster's authentication configuration. The test ensures: +// +// 1. IntegratedOAuth is correctly identified as integrated (should create groups) +// 2. Empty auth type is correctly identified as integrated (should create groups) +// 3. Custom auth types are correctly identified as non-integrated (should not create groups) +// 4. Missing authentication objects are handled with proper errors +// +// Authentication Types and Behavior: +// - IntegratedOAuth (default): Create default admin groups +// - "" (empty, default): Create default admin groups +// - "None": Do not create default admin groups +// - "OIDC": Do not create default admin groups +// - Custom types: Do not create default admin groups +// - Missing object: Return error (cluster configuration issue) +// +// This is critical for security because it determines whether the operator will +// automatically create admin groups that could grant elevated access. +func TestIsIntegratedOAuth(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + authObject *configv1.Authentication + expectError bool + expectedResult bool + description string + }{ + { + name: "should return true for IntegratedOAuth", + authObject: &configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: cluster.ClusterAuthenticationObj, + }, + Spec: configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeIntegratedOAuth, + }, + }, + expectError: false, + expectedResult: true, + description: "IntegratedOAuth should be considered integrated auth method", + }, + { + name: "should return true for empty type", + authObject: &configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: cluster.ClusterAuthenticationObj, + }, + Spec: configv1.AuthenticationSpec{ + Type: "", + }, + }, + expectError: false, + expectedResult: true, + description: "Empty type should be considered integrated auth method (default)", + }, + { + name: "should return false for OIDC", + authObject: &configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: cluster.ClusterAuthenticationObj, + }, + Spec: configv1.AuthenticationSpec{ + Type: "OIDC", + }, + }, + expectError: false, + expectedResult: false, + description: "OIDC should not be considered integrated auth method", + }, + { + name: "should return false for None", + authObject: &configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: cluster.ClusterAuthenticationObj, + }, + Spec: configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeNone, + }, + }, + expectError: false, + expectedResult: false, + description: "None should not be considered integrated auth method", + }, + { + name: "should return false for other auth types", + authObject: &configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: cluster.ClusterAuthenticationObj, + }, + Spec: configv1.AuthenticationSpec{ + Type: "CustomAuth", + }, + }, + expectError: false, + expectedResult: false, + description: "Other auth types should not be considered integrated auth method", + }, + { + name: "should handle missing authentication object", + authObject: nil, + expectError: true, + expectedResult: false, + description: "Should return error when authentication object doesn't exist", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + var fakeClient client.Client + + // Build scheme with OpenShift API types + scheme := runtime.NewScheme() + err := configv1.AddToScheme(scheme) + g.Expect(err).ShouldNot(HaveOccurred()) + + if tt.authObject != nil { + fakeClient = fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(tt.authObject). + Build() + } else { + fakeClient = fake.NewClientBuilder(). + WithScheme(scheme). + Build() + } + + result, err := cluster.IsIntegratedOAuth(ctx, fakeClient) + + if tt.expectError { + g.Expect(err).Should(HaveOccurred(), tt.description) + } else { + g.Expect(err).ShouldNot(HaveOccurred(), tt.description) + g.Expect(result).Should(Equal(tt.expectedResult), tt.description) } }) } diff --git a/pkg/utils/test/fakeclient/fakeclient.go b/pkg/utils/test/fakeclient/fakeclient.go index 1a68a870d3f6..12375c0b6e81 100644 --- a/pkg/utils/test/fakeclient/fakeclient.go +++ b/pkg/utils/test/fakeclient/fakeclient.go @@ -3,12 +3,15 @@ package fakeclient import ( "fmt" + configv1 "github.com/openshift/api/config/v1" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" clientFake "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/scheme" @@ -39,6 +42,23 @@ func WithScheme(value *runtime.Scheme) ClientOpts { } } +// WithClusterAuthType creates a fake OpenShift Authentication object with the specified type. +// This is useful for testing authentication mode detection logic. +// authType should be one of cluster.AuthModeOIDC, cluster.AuthModeIntegratedOAuth, or cluster.AuthModeNone constants. +func WithClusterAuthType(authType cluster.AuthenticationMode) ClientOpts { + return func(o *clientOptions) { + auth := &configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: cluster.ClusterAuthenticationObj, + }, + Spec: configv1.AuthenticationSpec{ + Type: configv1.AuthenticationType(authType), + }, + } + o.objects = append(o.objects, auth) + } +} + func New(opts ...ClientOpts) (client.Client, error) { co := clientOptions{} for _, o := range opts {