From 2cd6a6fb99278055fa3682431f747cafd749c3ff Mon Sep 17 00:00:00 2001 From: Matt Prahl Date: Fri, 15 Mar 2024 11:51:36 -0400 Subject: [PATCH] Fix the valid client certificate check (#378) The commit edef33de92cd9af45709f24f8b35d4e4e96af822 introduced additional checks on the certificate subject. The issue is that the order of the groups and organizational units are not consistent, so it was causing the addon-framework to generate several CSRs with many coming back as invalid. This takes the approach of sorting the strings before doing the comparison. Signed-off-by: mprahl --- pkg/registration/clientcert/certificate.go | 14 +++++--------- pkg/registration/clientcert/certificate_test.go | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/pkg/registration/clientcert/certificate.go b/pkg/registration/clientcert/certificate.go index c54bd8a1e..d05be7322 100644 --- a/pkg/registration/clientcert/certificate.go +++ b/pkg/registration/clientcert/certificate.go @@ -6,7 +6,6 @@ import ( "crypto/x509/pkix" "errors" "fmt" - "reflect" "time" "github.com/openshift/library-go/pkg/operator/events" @@ -14,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" certificatesinformers "k8s.io/client-go/informers/certificates" certificatesv1informers "k8s.io/client-go/informers/certificates/v1" "k8s.io/client-go/kubernetes" @@ -112,17 +112,13 @@ func certMatchSubject(cert *x509.Certificate, subject *pkix.Name) bool { return false } - // check groups(origanization) - if !reflect.DeepEqual(cert.Subject.Organization, subject.Organization) { + // check groups (organization) + if !sets.New(cert.Subject.Organization...).Equal(sets.New(subject.Organization...)) { return false } - // check originzation unit - if !reflect.DeepEqual(cert.Subject.OrganizationalUnit, subject.OrganizationalUnit) { - return false - } - - return true + // check organizational units + return sets.New(cert.Subject.OrganizationalUnit...).Equal(sets.New(subject.OrganizationalUnit...)) } // getCertValidityPeriod returns the validity period of the client certificate in the secret diff --git a/pkg/registration/clientcert/certificate_test.go b/pkg/registration/clientcert/certificate_test.go index 81e4d5270..18afb1b75 100644 --- a/pkg/registration/clientcert/certificate_test.go +++ b/pkg/registration/clientcert/certificate_test.go @@ -201,6 +201,20 @@ func TestIsCertificateValid(t *testing.T) { }, isValid: true, }, + { + name: "valid cert different order", + testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{ + CommonName: "test", + Organization: []string{"org", "org2"}, + OrganizationalUnit: []string{"ou", "ou2"}, + }, 60*time.Second), + subject: &pkix.Name{ + CommonName: "test", + Organization: []string{"org2", "org"}, + OrganizationalUnit: []string{"ou2", "ou"}, + }, + isValid: true, + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) {