Detect otherName changes to CR trigger reissuance

Signed-off-by: SpectralHiss <houssem.elfekih@jetstack.io>

pkg/util/pki/match.go

@@ -21,6 +21,8 @@ import (
 	"crypto/ecdsa"
 	"crypto/ed25519"
 	"crypto/rsa"
+	"crypto/x509/pkix"
+	"encoding/asn1"
 	"net"
 
 	"fmt"
@@ -148,6 +150,30 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe
 		violations = append(violations, "spec.dnsNames")
 	}
 
+	if spec.OtherNames != nil {
+		sanExtension, err := extractSANExtension(x509req.Extensions)
+		if err != nil {
+			violations = append(violations, "spec.otherNames")
+		}
+
+		generalNames, err := UnmarshalSANs(sanExtension.Value)
+		if err != nil {
+			return nil, err
+		}
+
+		CertificateRequestOtherNameSpec, err := ToOtherNameSpec(generalNames.OtherNames)
+		if err != nil {
+			// This means the CertificateRequest's otherName was not a utf8 valued
+			violations = append(violations, "spec.otherName")
+		}
+		if !util.EqualOtherNamesUnsorted(CertificateRequestOtherNameSpec, spec.OtherNames) {
+
+			// This means the oid or utf8Value did not match
+			violations = append(violations, "spec.otherNames")
+		}
+
+	}
+
 	if spec.LiteralSubject == "" {
 		// Comparing Subject fields
 		if x509req.Subject.CommonName != spec.CommonName {
@@ -216,6 +242,27 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe
 	return violations, nil
 }
 
+func ToOtherNameSpec(parsedOtherName []OtherName) ([]cmapi.OtherName, error) {
+	ret := make([]cmapi.OtherName, len(parsedOtherName))
+	for index, otherName := range parsedOtherName {
+		var utf8OtherNameValue string
+		rest, err := asn1.Unmarshal(otherName.Value.Bytes, &utf8OtherNameValue)
+		if err != nil {
+			return ret, err
+		}
+		if len(rest) != 0 {
+			return ret, fmt.Errorf("Should not have trailing data")
+		}
+
+		ret[index] = cmapi.OtherName{
+			OID:       otherName.TypeID.String(),
+			UTF8Value: utf8OtherNameValue,
+		}
+	}
+
+	return ret, nil
+}
+
 // SecretDataAltNamesMatchSpec will compare a Secret resource containing certificate
 // data to a CertificateSpec and return a list of 'violations' for any fields that
 // do not match their counterparts.
@@ -267,3 +314,15 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp
 
 	return violations, nil
 }
+
+func extractSANExtension(extensions []pkix.Extension) (pkix.Extension, error) {
+	oidExtensionSubjectAltName := []int{2, 5, 29, 17}
+
+	for _, extension := range extensions {
+		if extension.Id.Equal(oidExtensionSubjectAltName) {
+			return extension, nil
+		}
+	}
+
+	return pkix.Extension{}, fmt.Errorf("SAN extension not present!")
+}

pkg/util/pki/match_test.go

@@ -17,11 +17,15 @@ limitations under the License.
 package pki
 
 import (
+	"bytes"
 	"crypto"
+	"crypto/x509"
+	"encoding/pem"
 	"reflect"
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 )
@@ -119,6 +123,79 @@ func TestPrivateKeyMatchesSpec(t *testing.T) {
 	}
 }
 
+func TestCertificateRequestOtherNamesMatchSpec(t *testing.T) {
+	tests := map[string]struct {
+		crSpec     *cmapi.CertificateRequest
+		certSpec   cmapi.CertificateSpec
+		err        string
+		violations []string
+	}{
+		"should not report any violation if Certificate otherName(s) match the CertificateRequest's": {
+			crSpec: MustBuildCertificateRequest(&cmapi.Certificate{Spec: cmapi.CertificateSpec{
+				CommonName: "cn",
+				OtherNames: []cmapi.OtherName{
+					{
+						OID:       "1.3.6.1.4.1.311.20.2.3",
+						UTF8Value: "upn@testdomain.local",
+					},
+				},
+			}}, t),
+			certSpec: cmapi.CertificateSpec{
+				CommonName: "cn",
+				OtherNames: []cmapi.OtherName{
+					{
+						OID:       "1.3.6.1.4.1.311.20.2.3",
+						UTF8Value: "upn@testdomain.local",
+					},
+				},
+			},
+			err: "",
+		},
+		"should report violation if Certificate otherName(s) mismatch the CertificateRequest's": {
+			crSpec: MustBuildCertificateRequest(&cmapi.Certificate{Spec: cmapi.CertificateSpec{
+				CommonName: "cn",
+				OtherNames: []cmapi.OtherName{
+					{
+						OID:       "1.3.6.1.4.1.311.20.2.3",
+						UTF8Value: "upn@testdomain.local",
+					},
+				},
+			}}, t),
+			certSpec: cmapi.CertificateSpec{
+				CommonName: "cn",
+				OtherNames: []cmapi.OtherName{
+					{
+						OID:       "1.3.6.1.4.1.311.20.2.3",
+						UTF8Value: "upn2@testdomain.local",
+					},
+				},
+			},
+			err: "",
+			violations: []string{
+				"spec.otherNames",
+			},
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			violations, err := RequestMatchesSpec(test.crSpec, test.certSpec)
+			if err != nil {
+				if test.err == "" {
+					t.Errorf("Unexpected error: %s", err.Error())
+				} else {
+					if test.err != err.Error() {
+						t.Errorf("Expected error: %s but got: %s instead", err.Error(), test.err)
+					}
+				}
+			}
+
+			if !reflect.DeepEqual(violations, test.violations) {
+				t.Errorf("violations did not match, got=%s, exp=%s", violations, test.violations)
+			}
+		})
+	}
+}
+
 func TestSecretDataAltNamesMatchSpec(t *testing.T) {
 	tests := map[string]struct {
 		data       []byte
@@ -289,3 +366,38 @@ func selfSignCertificate(t *testing.T, spec cmapi.CertificateSpec) []byte {
 
 	return pemData
 }
+
+func MustBuildCertificateRequest(crt *cmapi.Certificate, t *testing.T) *cmapi.CertificateRequest {
+	pk, err := GenerateRSAPrivateKey(2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	csrTemplate, err := GenerateCSR(crt, WithOtherNames(true))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var buffer bytes.Buffer
+	csr, err := x509.CreateCertificateRequest(&buffer, csrTemplate, pk)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pemData := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csr})
+	cr := &cmapi.CertificateRequest{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        t.Name(),
+			Annotations: crt.Annotations,
+			Labels:      crt.Labels,
+		},
+		Spec: cmapi.CertificateRequestSpec{
+			Request:   pemData,
+			Duration:  crt.Spec.Duration,
+			IssuerRef: crt.Spec.IssuerRef,
+			IsCA:      crt.Spec.IsCA,
+			Usages:    crt.Spec.Usages,
+		},
+	}
+
+	return cr
+}

pkg/util/util.go

@@ -85,6 +85,30 @@ func EqualURLsUnsorted(s1, s2 []*url.URL) bool {
 	})
 }
 
+// Test for equal cmapi.OtherName slices even if unsorted. Panics if any element is nil
+func EqualOtherNamesUnsorted(s1, s2 []cmapi.OtherName) bool {
+	return genericEqualUnsorted(s1, s2, func(a cmapi.OtherName, b cmapi.OtherName) int {
+		if a.OID < b.OID {
+			return -1
+		}
+
+		if a.OID == b.OID {
+			if a.UTF8Value < b.UTF8Value {
+				return -1
+			}
+
+			if a.UTF8Value == b.UTF8Value {
+				return 0
+			}
+
+			return 1
+		}
+
+		return 1
+	})
+
+}
+
 // EqualIPsUnsorted checks if the given slices of IP addresses contain the same elements, even if in a different order
 func EqualIPsUnsorted(s1, s2 []net.IP) bool {
 	// Two IPv4 addresses can compare unequal with bytes.Equal which is why net.IP.Equal exists.