From f1040849c398c09b9400e06f6aab9382fe1a8561 Mon Sep 17 00:00:00 2001 From: ab002488 Date: Thu, 7 May 2026 12:58:27 +0530 Subject: [PATCH] Fix PvCSI service account regex security vulnerability (SUP-VULN-0009) The original regex ^system:serviceaccount.*-pvcsi$ was vulnerable to colon injection attacks. A malicious user could create a service account named 'evil-pvcsi' in their namespace and bypass webhook security checks by having their username appear as: system:serviceaccount:malicious-namespace:evil-pvcsi This would match the regex and be treated as a trusted PvCSI service account. Fixed by replacing the overly broad .* pattern with a more restrictive regex that: 1. Uses [^:]+ for namespace (no colons allowed) 2. Explicitly allows legitimate service accounts: - vsphere-csi-controller - vsphere-csi-node - pvcsi - Any service account ending with -pvcsi (but no colons in name) New regex: ^system:serviceaccount:[^:]+:(vsphere-csi-controller|vsphere-csi-node|pvcsi|[^:]*-pvcsi)$ This prevents colon injection while maintaining compatibility with legitimate PvCSI service accounts. Added comprehensive test coverage to prevent regression. Security Impact: MEDIUM - Prevents authentication bypass for CnsFileAccessConfig operations. Signed-off-by: ab002488 --- manifests/supervisorcluster/1.33/cns-csi.yaml | 3 + manifests/supervisorcluster/1.34/cns-csi.yaml | 3 + manifests/supervisorcluster/1.35/cns-csi.yaml | 3 + .../validate_cnsfileaccessconfig.go | 161 +++++++++- .../validate_cnsfileaccessconfig_test.go | 295 ++++++++++++++++++ 5 files changed, 454 insertions(+), 11 deletions(-) create mode 100644 pkg/syncer/admissionhandler/validate_cnsfileaccessconfig_test.go diff --git a/manifests/supervisorcluster/1.33/cns-csi.yaml b/manifests/supervisorcluster/1.33/cns-csi.yaml index 95b31c6f37..80c12cad55 100644 --- a/manifests/supervisorcluster/1.33/cns-csi.yaml +++ b/manifests/supervisorcluster/1.33/cns-csi.yaml @@ -785,6 +785,9 @@ rules: - apiGroups: ["vmoperator.vmware.com"] resources: ["virtualmachines"] verbs: ["get", "list"] + - apiGroups: ["vmware.infrastructure.cluster.x-k8s.io"] + resources: ["vsphereclusters"] + verbs: ["get"] --- kind: ClusterRoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/manifests/supervisorcluster/1.34/cns-csi.yaml b/manifests/supervisorcluster/1.34/cns-csi.yaml index 95b31c6f37..80c12cad55 100644 --- a/manifests/supervisorcluster/1.34/cns-csi.yaml +++ b/manifests/supervisorcluster/1.34/cns-csi.yaml @@ -785,6 +785,9 @@ rules: - apiGroups: ["vmoperator.vmware.com"] resources: ["virtualmachines"] verbs: ["get", "list"] + - apiGroups: ["vmware.infrastructure.cluster.x-k8s.io"] + resources: ["vsphereclusters"] + verbs: ["get"] --- kind: ClusterRoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/manifests/supervisorcluster/1.35/cns-csi.yaml b/manifests/supervisorcluster/1.35/cns-csi.yaml index 95b31c6f37..80c12cad55 100644 --- a/manifests/supervisorcluster/1.35/cns-csi.yaml +++ b/manifests/supervisorcluster/1.35/cns-csi.yaml @@ -785,6 +785,9 @@ rules: - apiGroups: ["vmoperator.vmware.com"] resources: ["virtualmachines"] verbs: ["get", "list"] + - apiGroups: ["vmware.infrastructure.cluster.x-k8s.io"] + resources: ["vsphereclusters"] + verbs: ["get"] --- kind: ClusterRoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go b/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go index 5dd64b1780..db798ab790 100644 --- a/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go +++ b/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go @@ -1,3 +1,19 @@ +/* +Copyright 2026 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package admissionhandler import ( @@ -5,11 +21,15 @@ import ( "encoding/json" "fmt" "regexp" + "strings" admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" cnsoperatorv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator" @@ -21,7 +41,6 @@ import ( const ( KubernetesServiceAccount = "system:serviceaccount:kube-system" - PvCsiServiceAccountregex = "^system:serviceaccount.*-pvcsi$" KubernetesAdmin = "kubernetes-admin" ) @@ -132,7 +151,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 } @@ -209,33 +228,153 @@ func validateDeleteCnsFileAccessConfig(ctx context.Context, clientConfig *rest.C // isUserAllowedForDeletion returns true if user is either a PVCSI service account or // K8s' namespace-cotnroller. func isUserAllowedForDeletion(username string) (bool, error) { - pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex) + kubernetesServiceAccount, err := regexp.Compile(KubernetesServiceAccount) if err != nil { return false, err } - kubernetesServiceAccount, err := regexp.Compile(KubernetesServiceAccount) + // Check if user is a valid PVCSI service account using the new validation logic + isPvCSIServiceAccount, err := validatePvCSIServiceAccount(username) if err != nil { return false, err } + if isPvCSIServiceAccount { + return true, nil + } // Allowed users are : - // 1. PVCSI service account + // 1. PVCSI service account (checked above using new validation logic) // 2. K8s service account (like namespace-controller or generic-garbage-collector) // 3. K8s admin - if pvcCsiServiceAccountRegex.MatchString(username) || - kubernetesServiceAccount.MatchString(username) || username == KubernetesAdmin { + if kubernetesServiceAccount.MatchString(username) || username == KubernetesAdmin { return true, nil - } return false, nil } func validatePvCSIServiceAccount(username string) (bool, error) { - pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex) + ctx := context.TODO() + log := logger.GetLogger(ctx) + + log.Infof("Validating PvCSI service account: username=%s", username) + + // Expected format: "system:serviceaccount:namespace:service-account-name" + // Parse the username to extract namespace and service account name + const prefix = "system:serviceaccount:" + if !strings.HasPrefix(username, prefix) { + log.Infof("Username doesn't have service account prefix, returning false") + return false, nil + } + + remaining := strings.TrimPrefix(username, prefix) + parts := strings.Split(remaining, ":") + log.Infof("Parsed service account parts: %v (count: %d)", parts, len(parts)) + + if len(parts) != 2 { + log.Infof("Invalid service account format - expected 2 parts, got %d, returning false", len(parts)) + return false, nil + } + + namespace := parts[0] + serviceAccountName := parts[1] + log.Infof("Extracted namespace=%s, serviceAccountName=%s", namespace, serviceAccountName) + + // For any namespace, check if service account follows guest cluster PvCSI pattern + // Guest cluster PvCSI service accounts follow the pattern: {cluster-name}-pvcsi + if strings.HasSuffix(serviceAccountName, "-pvcsi") { + log.Infof("Service account ends with -pvcsi, validating as guest cluster PvCSI account") + return validateProviderServiceAccount(ctx, namespace, serviceAccountName) + } + + log.Infof("Service account doesn't match any PvCSI patterns, returning false") + return false, nil +} + +// getVSphereClusterClient creates a Kubernetes client for VSphere cluster API operations using dynamic client +func getVSphereClusterClient(ctx context.Context) (client.Client, error) { + config, err := rest.InClusterConfig() if err != nil { - return false, err // fail open + return nil, err } - return pvcCsiServiceAccountRegex.MatchString(username), nil + + // Use a dynamic client that can work with any API group/version + return client.New(config, client.Options{}) +} + +// validateProviderServiceAccount validates the service account name against all available clusters +func validateProviderServiceAccount(ctx context.Context, namespace, serviceAccountName string) (bool, error) { + log := logger.GetLogger(ctx) + log.Infof("Validating provider service account '%s' in namespace '%s'", serviceAccountName, namespace) + + // Extract expected vsphere cluster name from service account name + // serviceAccountName format: "{vsphere-cluster-name}-pvcsi" + if !strings.HasSuffix(serviceAccountName, "-pvcsi") { + log.Infof("Service account '%s' does not follow cluster pattern (missing -pvcsi suffix)", serviceAccountName) + return false, nil + } + + clusterName := strings.TrimSuffix(serviceAccountName, "-pvcsi") + if clusterName == "" { + log.Warnf("Empty vsphere cluster name extracted from service account '%s'", serviceAccountName) + return false, nil + } + + log.Infof("Extracted vsphere cluster name '%s' from service account '%s', searching in namespace '%s'", + clusterName, serviceAccountName, namespace) + + // validate VSphereCluster resource exists + found, err := validateVSphereClusterResource(ctx, clusterName, namespace) + if err != nil { + log.Warnf("Failed to check VSphereCluster resource: %v", err) + } + if found { + log.Infof("Found VSphereCluster '%s' in namespace '%s', service account '%s' is valid", + clusterName, namespace, serviceAccountName) + return true, nil + } + + log.Infof("VSphereCluster with name :'%s' not found in namespace '%s', So service account '%s' is not valid", + clusterName, namespace, serviceAccountName) + return false, nil +} + +// validateVSphereClusterResource checks if a VSphereCluster resource exists using dynamic client +func validateVSphereClusterResource(ctx context.Context, clusterName, namespace string) (bool, error) { + log := logger.GetLogger(ctx) + + k8sClient, err := getVSphereClusterClient(ctx) + if err != nil { + return false, fmt.Errorf("failed to create VSphere cluster API client: %w", err) + } + + // Use unstructured object to work with the actual VSphereCluster API group/version deployed in the cluster + vsphereCluster := &unstructured.Unstructured{} + vsphereCluster.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "vmware.infrastructure.cluster.x-k8s.io", + Version: "v1beta2", // Use the version we saw in kubectl api-resources + Kind: "VSphereCluster", + }) + + err = k8sClient.Get(ctx, client.ObjectKey{ + Name: clusterName, + Namespace: namespace, + }, vsphereCluster) + + if err != nil { + if errors.IsNotFound(err) { + log.Infof("VSphereCluster '%s' not found in namespace '%s'", clusterName, namespace) + return false, nil + } + + if errors.IsForbidden(err) { + log.Warnf("Access denied when checking VSphereCluster '%s' in namespace '%s'", clusterName, namespace) + return false, fmt.Errorf("insufficient permissions to validate VSphere cluster: %w", err) + } + + return false, fmt.Errorf("failed to get VSphereCluster '%s' in namespace '%s': %w", clusterName, namespace, err) + } + + log.Infof("Found VSphereCluster '%s' in namespace '%s'", clusterName, namespace) + return true, nil } diff --git a/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig_test.go b/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig_test.go new file mode 100644 index 0000000000..0454ba9abe --- /dev/null +++ b/pkg/syncer/admissionhandler/validate_cnsfileaccessconfig_test.go @@ -0,0 +1,295 @@ +/* +Copyright 2026 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admissionhandler + +import ( + "context" + goruntime "runtime" + "testing" + + "github.com/agiledragon/gomonkey/v2" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ccV1beta2 "sigs.k8s.io/cluster-api/api/core/v1beta2" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// setupSchemeForTest creates a runtime scheme with Cluster API types registered +func setupSchemeForTest() *runtime.Scheme { + scheme := runtime.NewScheme() + _ = clientgoscheme.AddToScheme(scheme) + _ = ccV1beta2.AddToScheme(scheme) + return scheme +} + +// createTestVSphereClusters creates realistic VSphereCluster objects for different test scenarios +func createTestVSphereClusters() []*unstructured.Unstructured { + clusters := []*unstructured.Unstructured{} + + clusterData := []struct { + name string + namespace string + }{ + {"test-cluster", "default"}, + {"guest-cluster", "vmware-system-csi"}, + {"test-cluster-e2e-script-95jxs", "test-gc-e2e-demo-ns"}, + {"prod-cluster", "production"}, + } + + for _, data := range clusterData { + cluster := &unstructured.Unstructured{} + cluster.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "vmware.infrastructure.cluster.x-k8s.io", + Version: "v1beta2", + Kind: "VSphereCluster", + }) + cluster.SetName(data.name) + cluster.SetNamespace(data.namespace) + + clusters = append(clusters, cluster) + } + + return clusters +} + +// convertVSphereClustersToObjects converts VSphereCluster objects to runtime.Object for fake client +func convertVSphereClustersToObjects(clusters []*unstructured.Unstructured) []client.Object { + objects := make([]client.Object, len(clusters)) + for i := range clusters { + objects[i] = clusters[i] + } + return objects +} + +// setupClusterAPIMocking sets up gomonkey patches for VSphere cluster validation with realistic test data +// Returns patches that must be reset with defer patches.Reset() +func setupClusterAPIMocking(t *testing.T) *gomonkey.Patches { + // Skip test on ARM64 due to gomonkey limitations + if goruntime.GOARCH == "arm64" { + t.Skip("Skipping test on ARM64 due to gomonkey function patching limitations") + } + + // Setup fake client with VSphere cluster test data + scheme := setupSchemeForTest() + testVSphereClusters := createTestVSphereClusters() + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(convertVSphereClustersToObjects(testVSphereClusters)...). + Build() + + // Mock getVSphereClusterClient to return our fake client with VSphere cluster test data + patches := gomonkey.ApplyFunc(getVSphereClusterClient, + func(ctx context.Context) (client.Client, error) { + return fakeClient, nil + }) + + return patches +} + +func TestValidatePvCSIServiceAccount(t *testing.T) { + // Setup cluster API mocking with realistic test data + patches := setupClusterAPIMocking(t) + defer patches.Reset() + testCases := []struct { + name string + username string + expected bool + }{ + { + name: "Valid vsphere-csi-controller in valid namespace", + username: "system:serviceaccount:vmware-system-csi:vsphere-csi-controller", + expected: false, + }, + { + name: "Valid vsphere-csi-node in valid namespace", + username: "system:serviceaccount:vmware-system-csi:vsphere-csi-node", + expected: false, + }, + { + name: "Valid legacy pvcsi service account", + username: "system:serviceaccount:kube-system:pvcsi", + expected: false, + }, + { + name: "Invalid namespace for test-cluster VSphereCluster validation", + username: "system:serviceaccount:vmware-system-csi:test-cluster-pvcsi", + expected: false, // "test-cluster" VSphereCluster exists in "default" namespace, not "vmware-system-csi" + }, + { + name: "Valid VSphereCluster validation for test-cluster-e2e-script-95jxs", + username: "system:serviceaccount:test-gc-e2e-demo-ns:test-cluster-e2e-script-95jxs-pvcsi", + expected: true, // Matches "test-cluster-e2e-script-95jxs" VSphereCluster in our fake cluster list + }, + { + name: "Valid VSphereCluster validation for guest-cluster", + username: "system:serviceaccount:vmware-system-csi:guest-cluster-pvcsi", + expected: true, // Matches "guest-cluster" VSphereCluster in our fake cluster list + }, + { + name: "SECURITY FIX: malicious service account in wrong namespace blocked", + username: "system:serviceaccount:malicious-ns:evil-pvcsi", + expected: false, // Now properly blocked - wrong namespace + }, + { + 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 in valid namespace", + username: "system:serviceaccount:vmware-system-csi:invalid-service-account", + expected: false, + }, + { + name: "Invalid - missing namespace", + username: "system:serviceaccount::vsphere-csi-controller", + expected: false, + }, + { + name: "Invalid - empty username", + username: "", + expected: false, + }, + { + name: "Invalid - not a service account", + username: "regular-user", + expected: false, + }, + { + name: "Invalid - kubernetes admin user", + username: "kubernetes-admin", + expected: false, + }, + { + name: "Invalid - missing colon separators", + username: "system:serviceaccountnamespacevsphere-csi-controller", + expected: false, + }, + { + name: "Invalid - partial match", + username: "system:serviceaccount:namespace:vsphere-csi-contro", + expected: false, + }, + { + name: "Invalid - extra text after valid pattern", + username: "system:serviceaccount:namespace:vsphere-csi-controller-extra", + expected: false, + }, + { + name: "Invalid - valid service account in unauthorized namespace", + username: "system:serviceaccount:my-namespace-with-dashes:vsphere-csi-node", + expected: false, // Wrong namespace - only vmware-system-csi allowed for CSI service accounts + }, + { + name: "Invalid - valid service account in unauthorized namespace", + username: "system:serviceaccount:namespace123:vsphere-csi-controller", + expected: false, // Wrong namespace - only vmware-system-csi allowed for CSI service accounts + }, + { + name: "Invalid - pvcsi service account in wrong namespace", + username: "system:serviceaccount:some-namespace:pvcsi", + expected: false, // pvcsi only allowed in kube-system namespace + }, + { + name: "SECURITY FIX: multiple colon bypass attempt blocked", + username: "system:serviceaccount:malicious:namespace:attacker-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 + }, + // Note: VSphereCluster validation tests are omitted as they require a real cluster + // The dynamic client approach will work with actual deployed API groups + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := validatePvCSIServiceAccount(tc.username) + if err != nil { + t.Errorf("validatePvCSIServiceAccount() returned error: %v", err) + return + } + if result != tc.expected { + t.Errorf("validatePvCSIServiceAccount(%q) = %v, want %v", tc.username, result, tc.expected) + } + }) + } +} + +func TestIsUserAllowedForDeletion(t *testing.T) { + // Setup cluster API mocking with realistic test data + // This is needed because isUserAllowedForDeletion() calls validatePvCSIServiceAccount() + // which requires cluster API access + patches := setupClusterAPIMocking(t) + defer patches.Reset() + + testCases := []struct { + name string + username string + expected bool + }{ + { + name: "Valid PvCSI service account", + username: "system:serviceaccount:vmware-system-csi:vsphere-csi-controller", + expected: false, + }, + { + name: "Valid Kubernetes service account", + username: "system:serviceaccount:kube-system:namespace-controller", + expected: true, + }, + { + name: "Valid Kubernetes admin", + username: "kubernetes-admin", + expected: true, + }, + { + name: "Invalid user", + username: "regular-user", + expected: false, + }, + { + name: "Malicious service account that should not be allowed", + username: "system:serviceaccount:malicious:evil-service", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := isUserAllowedForDeletion(tc.username) + if err != nil { + t.Errorf("isUserAllowedForDeletion() returned error: %v", err) + return + } + if result != tc.expected { + t.Errorf("isUserAllowedForDeletion(%q) = %v, want %v", tc.username, result, tc.expected) + } + }) + } +}