From 7b13c72fede193cdfec66117ae2f66f014701193 Mon Sep 17 00:00:00 2001 From: SpectralHiss Date: Mon, 8 Jan 2024 18:13:17 +0000 Subject: [PATCH] Detect otherName changes to CR trigger reissuance Signed-off-by: SpectralHiss --- pkg/util/pki/match.go | 59 +++++++++++++++++++ pkg/util/pki/match_test.go | 112 +++++++++++++++++++++++++++++++++++++ pkg/util/util.go | 24 ++++++++ 3 files changed, 195 insertions(+) diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index 7f5c83f64..e59aec5cd 100644 --- a/pkg/util/pki/match.go +++ b/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!") +} diff --git a/pkg/util/pki/match_test.go b/pkg/util/pki/match_test.go index e9d961787..7595737d6 100644 --- a/pkg/util/pki/match_test.go +++ b/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 +} diff --git a/pkg/util/util.go b/pkg/util/util.go index 868e862fc..7e787308a 100644 --- a/pkg/util/util.go +++ b/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.