Fix PvCSI service account regex security vulnerability (SUP-VULN-0009)#4024
Fix PvCSI service account regex security vulnerability (SUP-VULN-0009)#4024anuj087 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuj087 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @anuj087. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
909d1c8 to
c111ba8
Compare
|
/ok-to-test |
c111ba8 to
4321bd1
Compare
4321bd1 to
04e70d3
Compare
04e70d3 to
016ae28
Compare
|
Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1471 |
88dd9e5 to
2a77623
Compare
|
Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
SUCCESS --- Jenkins Build #1517 |
7516465 to
f65d7b4
Compare
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
SUCCESS --- Jenkins Build #1226 |
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1229 |
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1225 |
|
FAILED --- Jenkins Build #1231 |
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1233 |
|
Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1619 |
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1234 |
|
Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
@anuj087: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
FAILED --- Jenkins Build #1235 |
|
SUCCESS --- Jenkins Build #1621 |
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
c9108d6 to
f104084
Compare
|
Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
FAILED --- Jenkins Build #1244 |
| // 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) |
There was a problem hiding this comment.
Why do we have this validation twice?
| return false, nil | ||
| } | ||
|
|
||
| if errors.IsForbidden(err) { |
There was a problem hiding this comment.
Why is this check required? Why can't we club it with the rest of the other errors?
| 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)) |
| // 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") |
|
|
||
| func validatePvCSIServiceAccount(username string) (bool, error) { | ||
| pvcCsiServiceAccountRegex, err := regexp.Compile(PvCsiServiceAccountregex) | ||
| ctx := context.TODO() |
| 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 |
There was a problem hiding this comment.
@nikhilbarge is this the correct version to use?
|
|
||
| // Allowed users are : | ||
| // 1. PVCSI service account | ||
| // 1. PVCSI service account (checked above using new validation logic) |
There was a problem hiding this comment.
This check no longer validates if it is a PVCSI service account. We already return at line 242. So this comment is outdated.
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 matching the username with the service account name which is "vsphereClusterName"-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.
Testing done: Both VKS and WCP precheckins are passed
https://jenkins-vcf-csifvt.devops.broadcom.net/view/instapp/job/vks-instapp-e2e-pre-checkin/1207/
https://jenkins-vcf-csifvt.devops.broadcom.net/job/wcp-instapp-e2e-pre-checkin/1621/