Skip to content

Commit

Permalink
Fix the valid client certificate check
Browse files Browse the repository at this point in the history
The commit edef33d 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 <mprahl@users.noreply.github.com>
  • Loading branch information
mprahl committed Mar 14, 2024
1 parent edef33d commit 78b4262
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 7 deletions.
24 changes: 17 additions & 7 deletions pkg/registration/clientcert/certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"reflect"
"sort"
"time"

"github.com/openshift/library-go/pkg/operator/events"
Expand Down Expand Up @@ -112,17 +113,26 @@ 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 !unorderedEqualStrings(cert.Subject.Organization, subject.Organization) {
return false
}

// check originzation unit
if !reflect.DeepEqual(cert.Subject.OrganizationalUnit, subject.OrganizationalUnit) {
return false
}
// check organizational units
return unorderedEqualStrings(cert.Subject.OrganizationalUnit, subject.OrganizationalUnit)
}

// unorderedEqualStrings compares to string slices without considering the order.
func unorderedEqualStrings(a []string, b []string) bool {
sortedA := make([]string, len(a))
copy(sortedA, a)
sort.Strings(sortedA)

sortedB := make([]string, len(a))
copy(sortedB, b)
sort.Strings(sortedB)

return true
return reflect.DeepEqual(sortedA, sortedB)
}

// getCertValidityPeriod returns the validity period of the client certificate in the secret
Expand Down
14 changes: 14 additions & 0 deletions pkg/registration/clientcert/certificate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 78b4262

Please sign in to comment.