Clean up dead code and fix issues in PvCSI service account validation

- Remove unused vulnerable regex constant PvCsiServiceAccountregex
- Fix test cases to match new secure validation logic
- Correct test expectations for security fixes (malicious accounts now properly blocked)
- Fix typo in error message (namesapace -> namespace)
- Refactor duplicated cluster API client creation into helper function
- Update test comments to highlight security improvements

Removed dead/duplicate code:
- PvCsiServiceAccountregex constant (line 42) - no longer used
- Outdated test cases expecting malicious accounts to be allowed
- Duplicated cluster API client creation code

Security test improvements:
- malicious-ns:evil-pvcsi now correctly blocked (was allowing bypass)
- Wrong namespace tests now properly expect false
- Added tests for empty cluster names and colon injection attacks

Code quality improvements:
- getClusterAPIClient() helper eliminates code duplication
- Fixed spelling error in log message
- All string literals appropriately scoped as local constants

Author: ab002488

Author: anuj087

pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go

@@ -39,7 +39,6 @@ import (
 
 const (
 	KubernetesServiceAccount = "system:serviceaccount:kube-system"
-	PvCsiServiceAccountregex = "^system:serviceaccount:[^:]+:(vsphere-csi-controller|vsphere-csi-node|pvcsi|[^:]*-pvcsi)$"
 	KubernetesAdmin          = "kubernetes-admin"
 )
 
@@ -150,7 +149,7 @@ func cnsFileAccessConfigAlreadyExists(ctx context.Context, clientConfig *rest.Co
 		LabelSelector: labelSelector,
 	})
 	if err != nil {
-		log.Errorf("failed to list CnsFileAccessConfigList CRs from %s namesapace. Error: %+v",
+		log.Errorf("failed to list CnsFileAccessConfigList CRs from %s namespace. Error: %+v",
 			namespace, err)
 		return "", err
 	}
@@ -321,17 +320,20 @@ func validateProviderServiceAccount(serviceAccountName string) (bool, error) {
 	return validateAgainstAllClusters(ctx, serviceAccountName)
 }
 
-// getExpectedProviderServiceAccountName gets the expected service account name 
-// for the current cluster context following the reference implementation
-func getExpectedProviderServiceAccountName(ctx context.Context) (string, error) {
-	// Get in-cluster config
+// getClusterAPIClient creates a Kubernetes client for cluster API operations
+func getClusterAPIClient(ctx context.Context) (client.Client, error) {
 	config, err := rest.InClusterConfig()
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
-	// Create client for cluster API
-	k8sClient, err := k8s.NewClientForGroup(ctx, config, ccV1beta2.GroupVersion.Group)
+	return k8s.NewClientForGroup(ctx, config, ccV1beta2.GroupVersion.Group)
+}
+
+// getExpectedProviderServiceAccountName gets the expected service account name 
+// for the current cluster context following the reference implementation
+func getExpectedProviderServiceAccountName(ctx context.Context) (string, error) {
+	k8sClient, err := getClusterAPIClient(ctx)
 	if err != nil {
 		return "", err
 	}
@@ -356,17 +358,9 @@ func getExpectedProviderServiceAccountName(ctx context.Context) (string, error)
 
 // validateAgainstAllClusters validates the service account name against all available clusters
 func validateAgainstAllClusters(ctx context.Context, serviceAccountName string) (bool, error) {
-	// Get in-cluster config
-	config, err := rest.InClusterConfig()
-	if err != nil {
-		// Fall back to basic validation if we can't get config
-		return validateBasicProviderServiceAccountPattern(serviceAccountName), nil
-	}
-	
-	// Create client for cluster API
-	k8sClient, err := k8s.NewClientForGroup(ctx, config, ccV1beta2.GroupVersion.Group)
+	k8sClient, err := getClusterAPIClient(ctx)
 	if err != nil {
-		// Fall back to basic validation if we can't create client
+		// Fall back to basic validation if we can't get client
 		return validateBasicProviderServiceAccountPattern(serviceAccountName), nil
 	}
 

pkg/syncer/admissionhandler/validate_cnsfileaccessconfig_test.go

@@ -37,27 +37,27 @@ func TestValidatePvCSIServiceAccount(t *testing.T) {
 			expected: true,
 		},
 		{
-			name:     "Valid pvcsi service account",
-			username: "system:serviceaccount:vmware-system-csi:some-pvcsi",
+			name:     "Valid legacy pvcsi service account",
+			username: "system:serviceaccount:kube-system:pvcsi",
 			expected: true,
 		},
 		{
-			name:     "Valid service account ending with -pvcsi",
-			username: "system:serviceaccount:vmware-system-csi:tenant-pvcsi",
-			expected: true,
+			name:     "Valid cluster-pvcsi pattern (fallback validation)",
+			username: "system:serviceaccount:vmware-system-csi:test-cluster-pvcsi",
+			expected: true, // Falls back to basic validation when cluster API unavailable
 		},
 		{
-			name:     "Security vulnerability - malicious service account with colon bypass",
+			name:     "SECURITY FIX: malicious service account in wrong namespace blocked",
 			username: "system:serviceaccount:malicious-ns:evil-pvcsi",
-			expected: true, // This should still be allowed as it's a legitimate pattern
+			expected: false, // Now properly blocked - wrong namespace
 		},
 		{
-			name:     "Security vulnerability - attempt to bypass with multiple colons",
+			name:     "SECURITY FIX: attempt to bypass with multiple colons blocked",
 			username: "system:serviceaccount:namespace:extra:vsphere-csi-controller",
 			expected: false,
 		},
 		{
-			name:     "Invalid - wrong service account name",
+			name:     "Invalid - wrong service account name in valid namespace",
 			username: "system:serviceaccount:vmware-system-csi:invalid-service-account",
 			expected: false,
 		},
@@ -97,29 +97,34 @@ func TestValidatePvCSIServiceAccount(t *testing.T) {
 			expected: false,
 		},
 		{
-			name:     "Edge case - namespace with dashes",
+			name:     "Invalid - valid service account in unauthorized namespace",
 			username: "system:serviceaccount:my-namespace-with-dashes:vsphere-csi-node",
-			expected: true,
+			expected: false, // Wrong namespace - only vmware-system-csi allowed for CSI service accounts
 		},
 		{
-			name:     "Edge case - namespace with numbers",
+			name:     "Invalid - valid service account in unauthorized namespace",
 			username: "system:serviceaccount:namespace123:vsphere-csi-controller",
-			expected: true,
+			expected: false, // Wrong namespace - only vmware-system-csi allowed for CSI service accounts
 		},
 		{
-			name:     "Test exact pvcsi service account",
+			name:     "Invalid - pvcsi service account in wrong namespace",
 			username: "system:serviceaccount:some-namespace:pvcsi",
-			expected: true,
+			expected: false, // pvcsi only allowed in kube-system namespace
 		},
 		{
-			name:     "Original vulnerability - multiple colon bypass attempt",
+			name:     "SECURITY FIX: multiple colon bypass attempt blocked",
 			username: "system:serviceaccount:malicious:namespace:attacker-pvcsi",
 			expected: false,
 		},
 		{
-			name:     "Service account with colon in name (should be blocked)",
-			username: "system:serviceaccount:namespace:service:account-pvcsi",
-			expected: false,
+			name:     "SECURITY FIX: service account with colon in name blocked",
+			username: "system:serviceaccount:vmware-system-csi:service:account-pvcsi",
+			expected: false, // Colon in cluster name blocked by fallback validation
+		},
+		{
+			name:     "Invalid - empty cluster name with -pvcsi suffix",
+			username: "system:serviceaccount:vmware-system-csi:-pvcsi",
+			expected: false, // Empty cluster name blocked by fallback validation
 		},
 	}