Skip to content

Commit f104084

Browse files
committed
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
1 parent b250a40 commit f104084

5 files changed

Lines changed: 454 additions & 11 deletions

File tree

manifests/supervisorcluster/1.33/cns-csi.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,9 @@ rules:
785785
- apiGroups: ["vmoperator.vmware.com"]
786786
resources: ["virtualmachines"]
787787
verbs: ["get", "list"]
788+
- apiGroups: ["vmware.infrastructure.cluster.x-k8s.io"]
789+
resources: ["vsphereclusters"]
790+
verbs: ["get"]
788791
---
789792
kind: ClusterRoleBinding
790793
apiVersion: rbac.authorization.k8s.io/v1

manifests/supervisorcluster/1.34/cns-csi.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,9 @@ rules:
785785
- apiGroups: ["vmoperator.vmware.com"]
786786
resources: ["virtualmachines"]
787787
verbs: ["get", "list"]
788+
- apiGroups: ["vmware.infrastructure.cluster.x-k8s.io"]
789+
resources: ["vsphereclusters"]
790+
verbs: ["get"]
788791
---
789792
kind: ClusterRoleBinding
790793
apiVersion: rbac.authorization.k8s.io/v1

manifests/supervisorcluster/1.35/cns-csi.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,9 @@ rules:
785785
- apiGroups: ["vmoperator.vmware.com"]
786786
resources: ["virtualmachines"]
787787
verbs: ["get", "list"]
788+
- apiGroups: ["vmware.infrastructure.cluster.x-k8s.io"]
789+
resources: ["vsphereclusters"]
790+
verbs: ["get"]
788791
---
789792
kind: ClusterRoleBinding
790793
apiVersion: rbac.authorization.k8s.io/v1

pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go

Lines changed: 150 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,35 @@
1+
/*
2+
Copyright 2026 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package admissionhandler
218

319
import (
420
"context"
521
"encoding/json"
622
"fmt"
723
"regexp"
24+
"strings"
825

926
admissionv1 "k8s.io/api/admission/v1"
1027

28+
"k8s.io/apimachinery/pkg/api/errors"
1129
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
30+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1231
"k8s.io/apimachinery/pkg/labels"
32+
"k8s.io/apimachinery/pkg/runtime/schema"
1333
"sigs.k8s.io/controller-runtime/pkg/client"
1434
cnsoperatorv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
1535

@@ -21,7 +41,6 @@ import (
2141

2242
const (
2343
KubernetesServiceAccount = "system:serviceaccount:kube-system"
24-
PvCsiServiceAccountregex = "^system:serviceaccount.*-pvcsi$"
2544
KubernetesAdmin = "kubernetes-admin"
2645
)
2746

@@ -132,7 +151,7 @@ func cnsFileAccessConfigAlreadyExists(ctx context.Context, clientConfig *rest.Co
132151
LabelSelector: labelSelector,
133152
})
134153
if err != nil {
135-
log.Errorf("failed to list CnsFileAccessConfigList CRs from %s namesapace. Error: %+v",
154+
log.Errorf("failed to list CnsFileAccessConfigList CRs from %s namespace. Error: %+v",
136155
namespace, err)
137156
return "", err
138157
}
@@ -209,33 +228,153 @@ func validateDeleteCnsFileAccessConfig(ctx context.Context, clientConfig *rest.C
209228
// isUserAllowedForDeletion returns true if user is either a PVCSI service account or
210229
// K8s' namespace-cotnroller.
211230
func isUserAllowedForDeletion(username string) (bool, error) {
212-
pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex)
231+
kubernetesServiceAccount, err := regexp.Compile(KubernetesServiceAccount)
213232
if err != nil {
214233
return false, err
215234
}
216235

217-
kubernetesServiceAccount, err := regexp.Compile(KubernetesServiceAccount)
236+
// Check if user is a valid PVCSI service account using the new validation logic
237+
isPvCSIServiceAccount, err := validatePvCSIServiceAccount(username)
218238
if err != nil {
219239
return false, err
220240
}
241+
if isPvCSIServiceAccount {
242+
return true, nil
243+
}
221244

222245
// Allowed users are :
223-
// 1. PVCSI service account
246+
// 1. PVCSI service account (checked above using new validation logic)
224247
// 2. K8s service account (like namespace-controller or generic-garbage-collector)
225248
// 3. K8s admin
226-
if pvcCsiServiceAccountRegex.MatchString(username) ||
227-
kubernetesServiceAccount.MatchString(username) || username == KubernetesAdmin {
249+
if kubernetesServiceAccount.MatchString(username) || username == KubernetesAdmin {
228250
return true, nil
229-
230251
}
231252

232253
return false, nil
233254
}
234255

235256
func validatePvCSIServiceAccount(username string) (bool, error) {
236-
pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex)
257+
ctx := context.TODO()
258+
log := logger.GetLogger(ctx)
259+
260+
log.Infof("Validating PvCSI service account: username=%s", username)
261+
262+
// Expected format: "system:serviceaccount:namespace:service-account-name"
263+
// Parse the username to extract namespace and service account name
264+
const prefix = "system:serviceaccount:"
265+
if !strings.HasPrefix(username, prefix) {
266+
log.Infof("Username doesn't have service account prefix, returning false")
267+
return false, nil
268+
}
269+
270+
remaining := strings.TrimPrefix(username, prefix)
271+
parts := strings.Split(remaining, ":")
272+
log.Infof("Parsed service account parts: %v (count: %d)", parts, len(parts))
273+
274+
if len(parts) != 2 {
275+
log.Infof("Invalid service account format - expected 2 parts, got %d, returning false", len(parts))
276+
return false, nil
277+
}
278+
279+
namespace := parts[0]
280+
serviceAccountName := parts[1]
281+
log.Infof("Extracted namespace=%s, serviceAccountName=%s", namespace, serviceAccountName)
282+
283+
// For any namespace, check if service account follows guest cluster PvCSI pattern
284+
// Guest cluster PvCSI service accounts follow the pattern: {cluster-name}-pvcsi
285+
if strings.HasSuffix(serviceAccountName, "-pvcsi") {
286+
log.Infof("Service account ends with -pvcsi, validating as guest cluster PvCSI account")
287+
return validateProviderServiceAccount(ctx, namespace, serviceAccountName)
288+
}
289+
290+
log.Infof("Service account doesn't match any PvCSI patterns, returning false")
291+
return false, nil
292+
}
293+
294+
// getVSphereClusterClient creates a Kubernetes client for VSphere cluster API operations using dynamic client
295+
func getVSphereClusterClient(ctx context.Context) (client.Client, error) {
296+
config, err := rest.InClusterConfig()
237297
if err != nil {
238-
return false, err // fail open
298+
return nil, err
239299
}
240-
return pvcCsiServiceAccountRegex.MatchString(username), nil
300+
301+
// Use a dynamic client that can work with any API group/version
302+
return client.New(config, client.Options{})
303+
}
304+
305+
// validateProviderServiceAccount validates the service account name against all available clusters
306+
func validateProviderServiceAccount(ctx context.Context, namespace, serviceAccountName string) (bool, error) {
307+
log := logger.GetLogger(ctx)
308+
log.Infof("Validating provider service account '%s' in namespace '%s'", serviceAccountName, namespace)
309+
310+
// Extract expected vsphere cluster name from service account name
311+
// serviceAccountName format: "{vsphere-cluster-name}-pvcsi"
312+
if !strings.HasSuffix(serviceAccountName, "-pvcsi") {
313+
log.Infof("Service account '%s' does not follow cluster pattern (missing -pvcsi suffix)", serviceAccountName)
314+
return false, nil
315+
}
316+
317+
clusterName := strings.TrimSuffix(serviceAccountName, "-pvcsi")
318+
if clusterName == "" {
319+
log.Warnf("Empty vsphere cluster name extracted from service account '%s'", serviceAccountName)
320+
return false, nil
321+
}
322+
323+
log.Infof("Extracted vsphere cluster name '%s' from service account '%s', searching in namespace '%s'",
324+
clusterName, serviceAccountName, namespace)
325+
326+
// validate VSphereCluster resource exists
327+
found, err := validateVSphereClusterResource(ctx, clusterName, namespace)
328+
if err != nil {
329+
log.Warnf("Failed to check VSphereCluster resource: %v", err)
330+
}
331+
if found {
332+
log.Infof("Found VSphereCluster '%s' in namespace '%s', service account '%s' is valid",
333+
clusterName, namespace, serviceAccountName)
334+
return true, nil
335+
}
336+
337+
log.Infof("VSphereCluster with name :'%s' not found in namespace '%s', So service account '%s' is not valid",
338+
clusterName, namespace, serviceAccountName)
339+
return false, nil
340+
}
341+
342+
// validateVSphereClusterResource checks if a VSphereCluster resource exists using dynamic client
343+
func validateVSphereClusterResource(ctx context.Context, clusterName, namespace string) (bool, error) {
344+
log := logger.GetLogger(ctx)
345+
346+
k8sClient, err := getVSphereClusterClient(ctx)
347+
if err != nil {
348+
return false, fmt.Errorf("failed to create VSphere cluster API client: %w", err)
349+
}
350+
351+
// Use unstructured object to work with the actual VSphereCluster API group/version deployed in the cluster
352+
vsphereCluster := &unstructured.Unstructured{}
353+
vsphereCluster.SetGroupVersionKind(schema.GroupVersionKind{
354+
Group: "vmware.infrastructure.cluster.x-k8s.io",
355+
Version: "v1beta2", // Use the version we saw in kubectl api-resources
356+
Kind: "VSphereCluster",
357+
})
358+
359+
err = k8sClient.Get(ctx, client.ObjectKey{
360+
Name: clusterName,
361+
Namespace: namespace,
362+
}, vsphereCluster)
363+
364+
if err != nil {
365+
if errors.IsNotFound(err) {
366+
log.Infof("VSphereCluster '%s' not found in namespace '%s'", clusterName, namespace)
367+
return false, nil
368+
}
369+
370+
if errors.IsForbidden(err) {
371+
log.Warnf("Access denied when checking VSphereCluster '%s' in namespace '%s'", clusterName, namespace)
372+
return false, fmt.Errorf("insufficient permissions to validate VSphere cluster: %w", err)
373+
}
374+
375+
return false, fmt.Errorf("failed to get VSphereCluster '%s' in namespace '%s': %w", clusterName, namespace, err)
376+
}
377+
378+
log.Infof("Found VSphereCluster '%s' in namespace '%s'", clusterName, namespace)
379+
return true, nil
241380
}

0 commit comments

Comments
 (0)