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 15, 2024
1 parent edef33d commit 7835988
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 9 deletions.
14 changes: 5 additions & 9 deletions pkg/registration/clientcert/certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import (
"crypto/x509/pkix"
"errors"
"fmt"
"reflect"
"time"

"github.com/openshift/library-go/pkg/operator/events"
certificates "k8s.io/api/certificates/v1"
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"
Expand Down Expand Up @@ -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
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 7835988

Please sign in to comment.