From edef33de92cd9af45709f24f8b35d4e4e96af822 Mon Sep 17 00:00:00 2001
From: xuezhao <zxue@redhat.com>
Date: Thu, 14 Mar 2024 16:44:16 +0800
Subject: [PATCH] Retrigger CSR when subject org and ou doesn't match. (#377)

Signed-off-by: GitHub <noreply@github.com>
---
 pkg/registration/clientcert/certificate.go    | 30 +++++++++++++---
 .../clientcert/certificate_test.go            | 36 +++++++++++++++++--
 .../spokecluster_autoapproval_test.go         | 18 ++++++----
 3 files changed, 70 insertions(+), 14 deletions(-)

diff --git a/pkg/registration/clientcert/certificate.go b/pkg/registration/clientcert/certificate.go
index a8e52e55f..c54bd8a1e 100644
--- a/pkg/registration/clientcert/certificate.go
+++ b/pkg/registration/clientcert/certificate.go
@@ -2,9 +2,11 @@ package clientcert
 
 import (
 	"context"
+	"crypto/x509"
 	"crypto/x509/pkix"
 	"errors"
 	"fmt"
+	"reflect"
 	"time"
 
 	"github.com/openshift/library-go/pkg/operator/events"
@@ -92,17 +94,37 @@ func IsCertificateValid(logger klog.Logger, certData []byte, subject *pkix.Name)
 	}
 
 	// check subject of certificates
+	// if the subject is specified, make sure at least one cert in the certificate chain matches the subject
 	for _, cert := range certs {
-		if cert.Subject.CommonName != subject.CommonName {
-			continue
+		if certMatchSubject(cert, subject) {
+			return true, nil
 		}
-		return true, nil
 	}
 
-	logger.V(4).Info("Certificate is not issued for subject", "commonName", subject.CommonName)
+	logger.V(4).Info("Certificate is not issued for subject", "commonName", subject.CommonName, "organization",
+		subject.Organization, "organizationalUnit", subject.OrganizationalUnit)
 	return false, nil
 }
 
+func certMatchSubject(cert *x509.Certificate, subject *pkix.Name) bool {
+	// check commonName
+	if cert.Subject.CommonName != subject.CommonName {
+		return false
+	}
+
+	// check groups(origanization)
+	if !reflect.DeepEqual(cert.Subject.Organization, subject.Organization) {
+		return false
+	}
+
+	// check originzation unit
+	if !reflect.DeepEqual(cert.Subject.OrganizationalUnit, subject.OrganizationalUnit) {
+		return false
+	}
+
+	return true
+}
+
 // getCertValidityPeriod returns the validity period of the client certificate in the secret
 func getCertValidityPeriod(secret *corev1.Secret) (*time.Time, *time.Time, error) {
 	if secret.Data == nil {
diff --git a/pkg/registration/clientcert/certificate_test.go b/pkg/registration/clientcert/certificate_test.go
index fd1c9449c..81e4d5270 100644
--- a/pkg/registration/clientcert/certificate_test.go
+++ b/pkg/registration/clientcert/certificate_test.go
@@ -162,12 +162,42 @@ func TestIsCertificateValid(t *testing.T) {
 			},
 		},
 		{
-			name: "valid cert",
+			name: "invalid organization",
 			testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{
-				CommonName: "test",
+				CommonName:   "test",
+				Organization: []string{"org_foo"},
 			}, 60*time.Second),
 			subject: &pkix.Name{
-				CommonName: "test",
+				CommonName:   "test",
+				Organization: []string{"org_bar"},
+			},
+			isValid: false,
+		},
+		{
+			name: "invalid organization unit",
+			testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{
+				CommonName:         "test",
+				Organization:       []string{"org"},
+				OrganizationalUnit: []string{"ou_foo"},
+			}, 60*time.Second),
+			subject: &pkix.Name{
+				CommonName:         "test",
+				Organization:       []string{"org"},
+				OrganizationalUnit: []string{"ou_bar"},
+			},
+			isValid: false,
+		},
+		{
+			name: "valid cert",
+			testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{
+				CommonName:         "test",
+				Organization:       []string{"org"},
+				OrganizationalUnit: []string{"ou"},
+			}, 60*time.Second),
+			subject: &pkix.Name{
+				CommonName:         "test",
+				Organization:       []string{"org"},
+				OrganizationalUnit: []string{"ou"},
 			},
 			isValid: true,
 		},
diff --git a/test/integration/registration/spokecluster_autoapproval_test.go b/test/integration/registration/spokecluster_autoapproval_test.go
index a62c359c3..c5e4465c6 100644
--- a/test/integration/registration/spokecluster_autoapproval_test.go
+++ b/test/integration/registration/spokecluster_autoapproval_test.go
@@ -44,20 +44,24 @@ var _ = ginkgo.Describe("Cluster Auto Approval", func() {
 		defer cancel()
 
 		// after bootstrap the spokecluster should be accepted and its csr should be auto approved
-		gomega.Eventually(func() bool {
+		gomega.Eventually(func() error {
 			cluster, err := util.GetManagedCluster(clusterClient, managedClusterName)
 			if err != nil {
-				return false
+				return err
 			}
 
-			return cluster.Spec.HubAcceptsClient
-		}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
+			if !cluster.Spec.HubAcceptsClient {
+				return fmt.Errorf("cluster should be accepted")
+			}
+
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
 
 		var approvedCSR *certificates.CertificateSigningRequest
-		gomega.Eventually(func() bool {
+		gomega.Eventually(func() error {
 			approvedCSR, err = util.FindAutoApprovedSpokeCSR(kubeClient, managedClusterName)
-			return err == nil
-		}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
+			return err
+		}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
 
 		// simulate hub cluster to fill a certificate
 		now := time.Now()