🐛 Fix the valid client certificate check

[mprahl]

Summary

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.

[mprahl]

/cc @xuezhaojun

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.40%. Comparing base (edef33d) to head (7835988).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
- Coverage   61.41%   61.40%   -0.01%     
==========================================
  Files         132      132              
  Lines       14064    14061       -3     
==========================================
- Hits         8637     8634       -3     
  Misses       4672     4672              
  Partials      755      755              

Flag	Coverage Δ
unit	61.40% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mprahl]

/cc @qiujian16 @zhujian7

[qiujian16]

Do you have an idea why the order is not consistent given it is a slice? Alternatively, I would suggest to use Set[string].

[mprahl]

Do you have an idea why the order is not consistent given it is a slice? Alternatively, I would suggest to use Set[string].

I don't know why it's not ordered but I can try and figure that out.

Thanks for the suggestion! I didn't know apimachinery included a set library.

[mprahl]

Do you have an idea why the order is not consistent given it is a slice? Alternatively, I would suggest to use Set[string].

It seems that x509.CreateCertificateRequest uses a set under the hood. You can see this simple example of how the parsed CSR returns the organizations as sorted:

package main

import (
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/x509"
	"crypto/x509/pkix"
	"fmt"
	"math/rand"
	"net"
)

func main() {
	insecureRand := rand.New(rand.NewSource(0)) //nolint:gosec
	pk, err := ecdsa.GenerateKey(elliptic.P256(), insecureRand)
	if err != nil {
		panic(err)
	}

	csrb, err := x509.CreateCertificateRequest(insecureRand, &x509.CertificateRequest{
		Subject: pkix.Name{
			CommonName: "system:open-cluster-management:cluster:cluster1:addon:config-policy-controller:agent:config-policy-controller",
			Organization: []string{
				"system:open-cluster-management:cluster:cluster1:addon:config-policy-controller",
				"system:open-cluster-management:addon:config-policy-controller",
				"system:authenticated",
			},
		},
		DNSNames:       []string{},
		EmailAddresses: []string{},
		IPAddresses:    []net.IP{},
	}, pk)
	if err != nil {
		panic(err)
	}

	csrParsed, err := x509.ParseCertificateRequest(csrb)
	if err != nil {
		panic(err)
	}

	fmt.Println(csrParsed.Subject.Organization)
}

The output is:

/tmp/temp-csr via 🐹 v1.22.0 ❯ go run main.go
[system:authenticated system:open-cluster-management:addon:config-policy-controller system:open-cluster-management:cluster:cluster1:addon:config-policy-controller]

[qiujian16]

ah thanks for the info.

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl, qiujian16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/registration/OWNERS [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment