fix(csrapprover): validate CSR requester username matches CommonName for node certificates

[nishantbkl3345-ship-it]

What this PR does / why we need it

Fixes a security vulnerability in csrapprover where node client and serving CSRs were auto-approved without verifying that csr.Spec.Username matched the requested CommonName.

Previously, isYurtHubNodeCert and isYurtTLSServerCert only checked that the CertificateRequest CommonName had prefix system:node:. They never examined csr.Spec.Username. If an authenticated node (system:node:node-a) submitted a CSR requesting a certificate for system:node:node-b, csrapprover auto-approved it — allowing any node to mint client certificates for any other node's identity and perform lateral movement to read secrets/pods.

This PR adds a validation check requiring that when csr.Spec.Username starts with system:node:, it must match x509cr.Subject.CommonName, mirroring upstream Kubernetes node CSR approver behavior.

Which issue(s) this PR fixes

N/A

Special notes for your reviewer

• Added unit test case yurthub node client CSR with mismatched node username and commonName in pkg/yurtmanager/controller/csrapprover/csr_approver_controller_test.go.
• Verified all unit tests and go test -race pass cleanly.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

This PR hardens several controller/cache paths, primarily aiming to close a privilege-escalation risk in csrapprover by validating node CSR requester identity, while also addressing correctness issues in hub leader selection and yurthub cache/GC behaviors.

Changes:

• csrapprover: add a Username vs CommonName validation gate for node client/serving CSRs.
• hubleader: avoid panics when LeaderNodeLabelSelector is nil under mark election strategy, and add a regression test.
• yurthub: fix pod GC key construction to include API version; improve list resourceVersion aggregation to handle large RVs safely and add unit tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/yurtmanager/controller/hubleader/hubleader_controller.go	Avoid overwriting matchLabels with a potentially nil selector map.
pkg/yurtmanager/controller/hubleader/hubleader_controller_test.go	Add a test case for mark strategy with nil LeaderNodeLabelSelector.
pkg/yurtmanager/controller/csrapprover/csr_approver_controller.go	Add Username/CommonName validation in node cert recognizers.
pkg/yurtmanager/controller/csrapprover/csr_approver_controller_test.go	Add unit test for mismatched node Username vs CommonName CSR.
pkg/yurthub/gc/gc.go	Include Version: "v1" when building pod storage keys during restart GC.
pkg/yurthub/gc/gc_test.go	New tests intended to validate pod GC key consistency.
pkg/yurthub/cachemanager/cache_manager.go	Parse list resourceVersions as uint64 and handle invalid/empty RVs safely.
pkg/yurthub/cachemanager/cache_manager_test.go	Add tests for completeListObjWithObjs RV aggregation behavior.

Suppressed comments (2)

pkg/yurtmanager/controller/csrapprover/csr_approver_controller.go:369

• This check only enforces Username==CommonName when Username already has the "system:node:" prefix. If Username is non-node (or empty), a crafted CSR with CN "system:node:..." and the expected orgs can still be auto-approved, because approval is based largely on CSR contents. Tighten this recognizer to require csr.Spec.Username to be a node identity and to exactly match x509cr.Subject.CommonName; otherwise return false.

	if strings.HasPrefix(csr.Spec.Username, "system:node:") && csr.Spec.Username != x509cr.Subject.CommonName {
		return false
	}

pkg/yurthub/gc/gc_test.go:153

• In this test, errors from KeyFunc are ignored and the “buggy lookup should MISS” path only logs instead of asserting, so the test can pass without actually validating the expected failure mode. Consider failing when the buggy lookup finds the cached key in enhancement mode, and handle KeyFunc errors explicitly.

	buggyKey, _ := store.KeyFunc(storage.KeyBuildInfo{
		Component: "kubelet",
		Namespace: "kube-system",
		Name:      "coredns-abc123",
		Resources: "pods",
	})
	currentPodKeysBuggy[buggyKey] = struct{}{}

	// FIXED: with Version
	currentPodKeysFixed := make(map[storage.Key]struct{})
	fixedKey, _ := store.KeyFunc(storage.KeyBuildInfo{
		Component: "kubelet",
		Namespace: "kube-system",
		Name:      "coredns-abc123",
		Resources: "pods",
		Version:   "v1",
	})
	currentPodKeysFixed[fixedKey] = struct{}{}

	// Check buggy lookup — this should MISS (the bug)
	if _, found := currentPodKeysBuggy[cachedKey]; found {
		t.Logf("Buggy code found the cached key (storage in legacy mode)")
	} else {
		t.Logf("CONFIRMED BUG: Buggy code did NOT find cached key %q in currentPodKeys — pod would be incorrectly marked for GC", cachedKey.Key())
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.