Match the exact cluster name for csr renewal (#1477)

Signed-off-by: Jian Qiu <jqiu@redhat.com> Co-authored-by: Jian Qiu <jqiu@redhat.com>
2026-05-06 01:07:03 +00:00 · 2026-04-08 06:26:39 +02:00
parent 0d6cc5f6d8
commit 863fcfcbb3
3 changed files with 364 additions and 1 deletions
--- a/pkg/registration/register/csr/approve_reconciler.go
+++ b/pkg/registration/register/csr/approve_reconciler.go
@@ -165,7 +165,34 @@ func validateCSR(logger klog.Logger, signer string, csr CSRInfo) (bool, string,
 		return false, "", ""
 	}

-	if !strings.HasPrefix(x509cr.Subject.CommonName, expectedPerClusterOrg) {
+	// Parse CommonName to extract cluster name and validate exact match
+	// Expected format: system:open-cluster-management:clusterName:agentName
+	if !strings.HasPrefix(x509cr.Subject.CommonName, user.SubjectPrefix) {
+		return false, "", ""
+	}
+
+	// Extract the parts after the prefix
+	nameWithoutPrefix := strings.TrimPrefix(x509cr.Subject.CommonName, user.SubjectPrefix)
+	parts := strings.Split(nameWithoutPrefix, ":")
+	if len(parts) != 2 {
+		// CommonName must have exactly 2 parts: clusterName:agentName
+		return false, "", ""
+	}
+
+	clusterNameFromCN, agentName := parts[0], parts[1]
+	if clusterNameFromCN == "" || agentName == "" {
+		return false, "", ""
+	}
+
+	// Verify exact match: cluster name from CommonName must match the cluster name from label
+	if clusterNameFromCN != spokeClusterName {
+		return false, "", ""
+	}
+
+	// Verify cluster name in CommonName matches the one in organization units
+	// The org should be the same as expectedPerClusterOrg which we already validated exists
+	expectedOrgForCN := fmt.Sprintf("%s%s", user.SubjectPrefix, clusterNameFromCN)
+	if expectedOrgForCN != expectedPerClusterOrg {
 		return false, "", ""
 	}

--- a/pkg/registration/register/csr/approver_test.go
+++ b/pkg/registration/register/csr/approver_test.go
@@ -312,6 +312,83 @@ func TestIsSpokeClusterClientCertRenewal(t *testing.T) {
 			clusterName: "managedcluster1",
 			commonName:  validCSR.CN,
 		},
+		{
+			name: "prefix match attack - CN with extra suffix but org has correct cluster",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster1xyz:spokeagent1",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster1", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
+		{
+			name: "prefix match attack - org and CN agree on wrong cluster name",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster1xyz:spokeagent1",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster1xyz", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
+		{
+			name: "prefix match attack - label and CN match but org has wrong cluster",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster1:spokeagent1",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster1xyz", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
+		{
+			name: "mismatched cluster name between CN and label",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster2:spokeagent1",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster1", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
+		{
+			name: "mismatched cluster name in org - org has different cluster than label and CN",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster1:spokeagent1",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster2", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
+		{
+			name: "CN with invalid format - missing agent name",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster1",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster1", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
+		{
+			name: "CN with invalid format - too many colons",
+			csr: testinghelpers.CSRHolder{
+				Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
+				SignerName:   validCSR.SignerName,
+				CN:           user.SubjectPrefix + "managedcluster1:spokeagent1:extra",
+				Orgs:         []string{user.SubjectPrefix + "managedcluster1", user.ManagedClustersGroup},
+				ReqBlockType: validCSR.ReqBlockType,
+			},
+			isRenewal: false,
+		},
 	}

 	for _, c := range cases {
--- a/test/integration/registration/csr_security_validation_test.go
+++ b/test/integration/registration/csr_security_validation_test.go
@@ -0,0 +1,259 @@
+package registration_test
+
+import (
+	"context"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	cryptorand "crypto/rand"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"fmt"
+	"math/rand"
+	"path"
+	"time"
+
+	"github.com/onsi/ginkgo/v2"
+	"github.com/onsi/gomega"
+	certificatesv1 "k8s.io/api/certificates/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	commonoptions "open-cluster-management.io/ocm/pkg/common/options"
+	"open-cluster-management.io/ocm/pkg/registration/hub/user"
+	registerfactory "open-cluster-management.io/ocm/pkg/registration/register/factory"
+	"open-cluster-management.io/ocm/pkg/registration/spoke"
+	"open-cluster-management.io/ocm/test/integration/util"
+)
+
+var _ = ginkgo.Describe("CSR Security Validation", func() {
+	ginkgo.It("Should reject CSR with prefix-matched organization that doesn't exactly match cluster name", func() {
+		var err error
+
+		managedClusterName := "securitytest-cluster1"
+		//#nosec G101
+		hubKubeconfigSecret := "securitytest-hub-kubeconfig-secret"
+		hubKubeconfigDir := path.Join(util.TestDir, "securitytest", "hub-kubeconfig")
+
+		agentOptions := &spoke.SpokeAgentOptions{
+			BootstrapKubeconfig:      bootstrapKubeConfigFile,
+			HubKubeconfigSecret:      hubKubeconfigSecret,
+			ClusterHealthCheckPeriod: 1 * time.Minute,
+			RegisterDriverOption:     registerfactory.NewOptions(),
+		}
+
+		commOptions := commonoptions.NewAgentOptions()
+		commOptions.HubKubeconfigDir = hubKubeconfigDir
+		commOptions.SpokeClusterName = managedClusterName
+
+		// run registration agent
+		cancel := runAgent("securitytest", agentOptions, commOptions, spokeCfg)
+		defer cancel()
+
+		// after bootstrap the spokecluster and csr should be created
+		gomega.Eventually(func() error {
+			if _, err := util.GetManagedCluster(clusterClient, managedClusterName); err != nil {
+				return err
+			}
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+		gomega.Eventually(func() error {
+			if _, err := util.FindUnapprovedSpokeCSR(kubeClient, managedClusterName); err != nil {
+				return err
+			}
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+		// approve the initial CSR and accept the cluster
+		err = authn.ApproveSpokeClusterCSR(kubeClient, managedClusterName, 24*time.Hour)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		err = util.AcceptManagedCluster(clusterClient, managedClusterName)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		// the hub kubeconfig secret should be filled after the csr is approved
+		gomega.Eventually(func() error {
+			if _, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret); err != nil {
+				return err
+			}
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+		// Now create a malicious CSR where:
+		// - label has correct cluster name: "securitytest-cluster1"
+		// - CN has correct cluster name: "system:open-cluster-management:securitytest-cluster1:agent1"
+		// - BUT org has prefix-matched name: "system:open-cluster-management:securitytest-cluster1xyz"
+		// This should NOT be auto-approved
+		maliciousCSR := createMaliciousCSR(
+			managedClusterName,
+			user.SubjectPrefix+managedClusterName+":agent1",
+			[]string{user.SubjectPrefix + managedClusterName + "xyz", user.ManagedClustersGroup},
+		)
+
+		_, err = kubeClient.CertificatesV1().CertificateSigningRequests().Create(
+			context.Background(),
+			maliciousCSR,
+			metav1.CreateOptions{},
+		)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		// Wait and verify the malicious CSR is NOT auto-approved
+		// The CSR should remain unapproved for at least 10 seconds
+		gomega.Consistently(func() bool {
+			csr, err := kubeClient.CertificatesV1().CertificateSigningRequests().Get(
+				context.Background(),
+				maliciousCSR.Name,
+				metav1.GetOptions{},
+			)
+			if err != nil {
+				return false
+			}
+			// Check if CSR is still unapproved
+			return len(csr.Status.Conditions) == 0
+		}, 10*time.Second, 1*time.Second).Should(gomega.BeTrue(), "Malicious CSR should NOT be auto-approved")
+
+		// Clean up the malicious CSR
+		err = kubeClient.CertificatesV1().CertificateSigningRequests().Delete(
+			context.Background(),
+			maliciousCSR.Name,
+			metav1.DeleteOptions{},
+		)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	})
+
+	ginkgo.It("Should reject CSR where org and CN agree on wrong cluster name", func() {
+		var err error
+
+		managedClusterName := "securitytest-cluster2"
+		//#nosec G101
+		hubKubeconfigSecret := "securitytest2-hub-kubeconfig-secret"
+		hubKubeconfigDir := path.Join(util.TestDir, "securitytest2", "hub-kubeconfig")
+
+		agentOptions := &spoke.SpokeAgentOptions{
+			BootstrapKubeconfig:      bootstrapKubeConfigFile,
+			HubKubeconfigSecret:      hubKubeconfigSecret,
+			ClusterHealthCheckPeriod: 1 * time.Minute,
+			RegisterDriverOption:     registerfactory.NewOptions(),
+		}
+
+		commOptions := commonoptions.NewAgentOptions()
+		commOptions.HubKubeconfigDir = hubKubeconfigDir
+		commOptions.SpokeClusterName = managedClusterName
+
+		// run registration agent
+		cancel := runAgent("securitytest2", agentOptions, commOptions, spokeCfg)
+		defer cancel()
+
+		// after bootstrap the spokecluster and csr should be created
+		gomega.Eventually(func() error {
+			if _, err := util.GetManagedCluster(clusterClient, managedClusterName); err != nil {
+				return err
+			}
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+		gomega.Eventually(func() error {
+			if _, err := util.FindUnapprovedSpokeCSR(kubeClient, managedClusterName); err != nil {
+				return err
+			}
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+		// approve the initial CSR and accept the cluster
+		err = authn.ApproveSpokeClusterCSR(kubeClient, managedClusterName, 24*time.Hour)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		err = util.AcceptManagedCluster(clusterClient, managedClusterName)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		// the hub kubeconfig secret should be filled after the csr is approved
+		gomega.Eventually(func() error {
+			if _, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret); err != nil {
+				return err
+			}
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+		// Now create a malicious CSR where:
+		// - label has correct cluster name: "securitytest-cluster2"
+		// - BUT both CN and org have a different cluster name with prefix match: "securitytest-cluster2xyz"
+		// This should NOT be auto-approved
+		wrongClusterName := managedClusterName + "xyz"
+		maliciousCSR := createMaliciousCSR(
+			managedClusterName, // label still has correct name
+			user.SubjectPrefix+wrongClusterName+":agent1",
+			[]string{user.SubjectPrefix + wrongClusterName, user.ManagedClustersGroup},
+		)
+
+		_, err = kubeClient.CertificatesV1().CertificateSigningRequests().Create(
+			context.Background(),
+			maliciousCSR,
+			metav1.CreateOptions{},
+		)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		// Wait and verify the malicious CSR is NOT auto-approved
+		gomega.Consistently(func() bool {
+			csr, err := kubeClient.CertificatesV1().CertificateSigningRequests().Get(
+				context.Background(),
+				maliciousCSR.Name,
+				metav1.GetOptions{},
+			)
+			if err != nil {
+				return false
+			}
+			// Check if CSR is still unapproved
+			return len(csr.Status.Conditions) == 0
+		}, 10*time.Second, 1*time.Second).Should(gomega.BeTrue(), "Malicious CSR should NOT be auto-approved")
+
+		// Clean up the malicious CSR
+		err = kubeClient.CertificatesV1().CertificateSigningRequests().Delete(
+			context.Background(),
+			maliciousCSR.Name,
+			metav1.DeleteOptions{},
+		)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	})
+})
+
+// createMaliciousCSR creates a CSR with custom label, CN, and organizations for testing
+func createMaliciousCSR(clusterName, commonName string, orgs []string) *certificatesv1.CertificateSigningRequest {
+	insecureRand := rand.New(rand.NewSource(time.Now().UnixNano())) //nolint:gosec
+	pk, err := ecdsa.GenerateKey(elliptic.P256(), insecureRand)
+	if err != nil {
+		panic(err)
+	}
+
+	csrb, err := x509.CreateCertificateRequest(cryptorand.Reader, &x509.CertificateRequest{
+		Subject: pkix.Name{
+			CommonName:   commonName,
+			Organization: orgs,
+		},
+	}, pk)
+	if err != nil {
+		panic(err)
+	}
+
+	csrName := fmt.Sprintf("malicious-csr-%d", time.Now().UnixNano())
+	return &certificatesv1.CertificateSigningRequest{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: csrName,
+			Labels: map[string]string{
+				"open-cluster-management.io/cluster-name": clusterName,
+			},
+		},
+		Spec: certificatesv1.CertificateSigningRequestSpec{
+			Username: user.SubjectPrefix + clusterName + ":agent1",
+			Request: pem.EncodeToMemory(&pem.Block{
+				Type:  "CERTIFICATE REQUEST",
+				Bytes: csrb,
+			}),
+			SignerName: certificatesv1.KubeAPIServerClientSignerName,
+			Usages: []certificatesv1.KeyUsage{
+				certificatesv1.UsageDigitalSignature,
+				certificatesv1.UsageKeyEncipherment,
+				certificatesv1.UsageClientAuth,
+			},
+		},
+	}
+}