Skip to content

Commit 11ceb64

Browse files
committed
Fix PvCSI service account validation using correct cluster-based approach
Replace vulnerable regex pattern matching with proper cluster-based validation that constructs expected service account names and does exact matching. Implementation based on reference code: https://github-vcf.devops.broadcom.net/vcf/kubernetes-service/blob/0319c5f7c9a4300b0a97296e2b3ad6283fc6bae0/addons/controllers/csi/vspherecsiconfig_controller.go#L326C3-L331C4 Key changes: - Query cluster API to get actual cluster names - Construct expected service account names using: fmt.Sprintf("%s-%s", cluster.Name, "pvcsi") - Do exact string matching instead of vulnerable regex patterns - Maintain explicit allowlist for standard service accounts - Graceful fallback to basic validation when cluster API unavailable Security improvements: - Prevents regex bypass attacks with crafted service account names - Validates against actual existing clusters only - Blocks attacks with non-existent cluster names - Maintains namespace isolation (vmware-system-csi for ProviderServiceAccount) - Prevents format confusion with colon injection attacks This approach follows the principle of "construct expected names and validate exactly" rather than "match patterns that can be bypassed". Author: ab002488 Fixes: Bug 3687875 Vulnerability-ID: SUP-VULN-0009
1 parent 998c1b6 commit 11ceb64

1 file changed

Lines changed: 116 additions & 9 deletions

File tree

pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go

Lines changed: 116 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +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

1128
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1229
"k8s.io/apimachinery/pkg/labels"
1330
"sigs.k8s.io/controller-runtime/pkg/client"
1431
cnsoperatorv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
32+
ccV1beta2 "sigs.k8s.io/cluster-api/api/core/v1beta2"
1533

1634
"k8s.io/client-go/rest"
1735
cnsfileaccessconfigv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsfileaccessconfig/v1alpha1"
@@ -209,33 +227,122 @@ func validateDeleteCnsFileAccessConfig(ctx context.Context, clientConfig *rest.C
209227
// isUserAllowedForDeletion returns true if user is either a PVCSI service account or
210228
// K8s' namespace-cotnroller.
211229
func isUserAllowedForDeletion(username string) (bool, error) {
212-
pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex)
230+
kubernetesServiceAccount, err := regexp.Compile(KubernetesServiceAccount)
213231
if err != nil {
214232
return false, err
215233
}
216234

217-
kubernetesServiceAccount, err := regexp.Compile(KubernetesServiceAccount)
235+
// Check if user is a valid PVCSI service account using the new validation logic
236+
isPvCSIServiceAccount, err := validatePvCSIServiceAccount(username)
218237
if err != nil {
219238
return false, err
220239
}
240+
if isPvCSIServiceAccount {
241+
return true, nil
242+
}
221243

222244
// Allowed users are :
223-
// 1. PVCSI service account
245+
// 1. PVCSI service account (checked above using new validation logic)
224246
// 2. K8s service account (like namespace-controller or generic-garbage-collector)
225247
// 3. K8s admin
226-
if pvcCsiServiceAccountRegex.MatchString(username) ||
227-
kubernetesServiceAccount.MatchString(username) || username == KubernetesAdmin {
248+
if kubernetesServiceAccount.MatchString(username) || username == KubernetesAdmin {
228249
return true, nil
229-
230250
}
231251

232252
return false, nil
233253
}
234254

235255
func validatePvCSIServiceAccount(username string) (bool, error) {
236-
pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex)
256+
// Expected format: "system:serviceaccount:namespace:service-account-name"
257+
// Parse the username to extract namespace and service account name
258+
const prefix = "system:serviceaccount:"
259+
if !strings.HasPrefix(username, prefix) {
260+
return false, nil
261+
}
262+
263+
remaining := strings.TrimPrefix(username, prefix)
264+
parts := strings.Split(remaining, ":")
265+
if len(parts) != 2 {
266+
return false, nil
267+
}
268+
269+
namespace := parts[0]
270+
serviceAccountName := parts[1]
271+
272+
// Check explicit service account names first
273+
if isExplicitPvCSIServiceAccount(namespace, serviceAccountName) {
274+
return true, nil
275+
}
276+
277+
// For vmware-system-csi namespace, construct the expected ProviderServiceAccount name
278+
// and do exact matching based on the reference implementation
279+
if namespace == "vmware-system-csi" {
280+
return validateProviderServiceAccount(serviceAccountName)
281+
}
282+
283+
return false, nil
284+
}
285+
286+
// isExplicitPvCSIServiceAccount checks for known explicit service account names
287+
func isExplicitPvCSIServiceAccount(namespace, serviceAccountName string) bool {
288+
switch namespace {
289+
case "vmware-system-csi":
290+
return serviceAccountName == "vsphere-csi-controller" || serviceAccountName == "vsphere-csi-node"
291+
case "kube-system":
292+
return serviceAccountName == "pvcsi"
293+
default:
294+
return false
295+
}
296+
}
297+
298+
// validateProviderServiceAccount validates if the service account name matches
299+
// the expected ProviderServiceAccount name pattern based on the reference implementation:
300+
// https://github-vcf.devops.broadcom.net/vcf/kubernetes-service/blob/0319c5f7c9a4300b0a97296e2b3ad6283fc6bae0/addons/controllers/csi/vspherecsiconfig_controller.go#L326C3-L331C4
301+
func validateProviderServiceAccount(serviceAccountName string) (bool, error) {
302+
// The service account name should follow the pattern: fmt.Sprintf("%s-%s", vsphereCluster.Name, "pvcsi")
303+
// Instead of using regex patterns, we need to validate against actual cluster names
304+
305+
// Get in-cluster config to access the cluster API
306+
config, err := rest.InClusterConfig()
307+
if err != nil {
308+
// If we can't get config, fall back to basic validation
309+
return validateBasicProviderServiceAccountPattern(serviceAccountName), nil
310+
}
311+
312+
// Create a client to access cluster API resources
313+
k8sClient, err := k8s.NewClient(context.TODO(), config)
237314
if err != nil {
238-
return false, err // fail open
315+
// Fall back to basic validation if we can't create client
316+
return validateBasicProviderServiceAccountPattern(serviceAccountName), nil
317+
}
318+
319+
// Get all clusters in the system to validate against actual cluster names
320+
clusterList := &ccV1beta2.ClusterList{}
321+
if err := k8sClient.List(context.TODO(), clusterList); err != nil {
322+
// Fall back to basic validation if we can't list clusters
323+
return validateBasicProviderServiceAccountPattern(serviceAccountName), nil
324+
}
325+
326+
// Check if the service account name matches any actual cluster's expected ProviderServiceAccount name
327+
for _, cluster := range clusterList.Items {
328+
expectedServiceAccountName := fmt.Sprintf("%s-%s", cluster.Name, "pvcsi")
329+
if serviceAccountName == expectedServiceAccountName {
330+
return true, nil
331+
}
332+
}
333+
334+
// Service account name doesn't match any existing cluster's expected ProviderServiceAccount name
335+
return false, nil
336+
}
337+
338+
// validateBasicProviderServiceAccountPattern provides fallback validation when cluster API is not available
339+
func validateBasicProviderServiceAccountPattern(serviceAccountName string) bool {
340+
// Basic validation: must end with -pvcsi, not be empty, and not contain colons (format confusion attack prevention)
341+
if !strings.HasSuffix(serviceAccountName, "-pvcsi") {
342+
return false
239343
}
240-
return pvcCsiServiceAccountRegex.MatchString(username), nil
344+
345+
clusterName := strings.TrimSuffix(serviceAccountName, "-pvcsi")
346+
// Cluster name should not be empty and not contain colons
347+
return len(clusterName) > 0 && !strings.Contains(clusterName, ":")
241348
}

0 commit comments

Comments
 (0)