From 7b13c72fede193cdfec66117ae2f66f014701193 Mon Sep 17 00:00:00 2001 From: SpectralHiss Date: Mon, 8 Jan 2024 18:13:17 +0000 Subject: [PATCH 1/7] 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. From b6fdcede90d77c8a0a15bbc0a36396aef2016658 Mon Sep 17 00:00:00 2001 From: SpectralHiss Date: Tue, 9 Jan 2024 11:39:17 +0000 Subject: [PATCH 2/7] Add test for different order OtherName value * Simplify sorting implementation for OtherName slice equality Signed-off-by: SpectralHiss --- pkg/util/pki/match_test.go | 29 +++++++++++++++++++++++++++++ pkg/util/util.go | 17 ++--------------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/pkg/util/pki/match_test.go b/pkg/util/pki/match_test.go index 7595737d6..50516aff4 100644 --- a/pkg/util/pki/match_test.go +++ b/pkg/util/pki/match_test.go @@ -175,6 +175,35 @@ func TestCertificateRequestOtherNamesMatchSpec(t *testing.T) { "spec.otherNames", }, }, + "should not report violation if Certificate otherName(s) match the CertificateRequest's (with different order)": { + crSpec: MustBuildCertificateRequest(&cmapi.Certificate{Spec: cmapi.CertificateSpec{ + CommonName: "cn", + OtherNames: []cmapi.OtherName{ + { + OID: "1.3.6.1.4.1.311.20.2.3", + UTF8Value: "anotherupn@testdomain.local", + }, + { + 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", + }, + { + OID: "1.3.6.1.4.1.311.20.2.3", + UTF8Value: "anotherupn@testdomain.local", + }, + }, + }, + err: "", + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { diff --git a/pkg/util/util.go b/pkg/util/util.go index 7e787308a..30a4030c5 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -88,23 +88,10 @@ 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 strings.Compare(a.UTF8Value, b.UTF8Value) } - - return 1 + return strings.Compare(a.OID, b.OID) }) } From 38c2b33a716772317c403fc2206adb0945645449 Mon Sep 17 00:00:00 2001 From: SpectralHiss Date: Tue, 9 Jan 2024 14:01:09 +0000 Subject: [PATCH 3/7] Add otherName detection to TestSecretDataAltNamesMatchSpec Signed-off-by: SpectralHiss --- pkg/util/pki/match.go | 52 ++++++++++++++++++++--------- pkg/util/pki/match_test.go | 67 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 16 deletions(-) diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index e59aec5cd..0024db5f9 100644 --- a/pkg/util/pki/match.go +++ b/pkg/util/pki/match.go @@ -151,27 +151,13 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe } if spec.OtherNames != nil { - sanExtension, err := extractSANExtension(x509req.Extensions) - if err != nil { - violations = append(violations, "spec.otherNames") - } - - generalNames, err := UnmarshalSANs(sanExtension.Value) + matched, err := matchOtherNames(x509req.Extensions, spec.OtherNames) 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 + if !matched { violations = append(violations, "spec.otherNames") } - } if spec.LiteralSubject == "" { @@ -242,6 +228,30 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe return violations, nil } +func matchOtherNames(extension []pkix.Extension, otherNames []cmapi.OtherName) (bool, error) { + sanExtension, err := extractSANExtension(extension) + if err != nil { + return false, nil + } + + generalNames, err := UnmarshalSANs(sanExtension.Value) + if err != nil { + return false, err + } + + CertificateRequestOtherNameSpec, err := ToOtherNameSpec(generalNames.OtherNames) + if err != nil { + // This means the CertificateRequest's otherName was not a utf8 valued + return false, nil + } + + if !util.EqualOtherNamesUnsorted(CertificateRequestOtherNameSpec, otherNames) { + return false, nil + } + + return true, nil +} + func ToOtherNameSpec(parsedOtherName []OtherName) ([]cmapi.OtherName, error) { ret := make([]cmapi.OtherName, len(parsedOtherName)) for index, otherName := range parsedOtherName { @@ -312,6 +322,16 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp violations = append(violations, "spec.emailAddresses") } + if spec.OtherNames != nil { + matched, err := matchOtherNames(x509cert.Extensions, spec.OtherNames) + if err != nil { + return nil, err + } + if !matched { + violations = append(violations, "spec.otherNames") + } + } + return violations, nil } diff --git a/pkg/util/pki/match_test.go b/pkg/util/pki/match_test.go index 50516aff4..66db88dff 100644 --- a/pkg/util/pki/match_test.go +++ b/pkg/util/pki/match_test.go @@ -346,6 +346,73 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { }), violations: []string{"spec.ipAddresses"}, }, + "should match if otherNames are equal": { + spec: cmapi.CertificateSpec{ + OtherNames: []cmapi.OtherName{ + { + OID: "1.3.6.1.4.1.311.20.2.3", + UTF8Value: "upn2@testdomain.local", + }, + { + OID: "1.3.6.1.4.1.311.20.2.3", + UTF8Value: "upn@testdomain.local", + }, + }, + }, // openssl req -nodes -newkey rsa:2048 -subj "/CN=someCN" \ + // -addext 'subjectAltName=otherName:msUPN;UTF8:upn@testdomain.local,otherName:msUPN;UTF8:upn2@testdomain.local' -x509 | + data: []byte(`-----BEGIN CERTIFICATE----- +MIIDOzCCAiOgAwIBAgIUJGyXr7GsoPVGC9PkG/QR5NQ3doQwDQYJKoZIhvcNAQEL +BQAwADAeFw0yNDAxMDkxMzQwNDZaFw0yNDAyMDgxMzQwNDZaMAAwggEiMA0GCSqG +SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDFVLGrwoVnLaVERh5l6/+Wc1bDrEOCrsZz +FUYOBJNpoJmbcl6Cp3DLyqrgkzAXWusUft77DmOpMz5C/2IWtI0Ju/NBg2wCwu6U ++NcL70WTx2h1v7fN0YHdzElGcO018bPpA9QEfzoB07G+G8dqTwUMrCq6qE5vbmY3 +PywXfvCKbES4AFvQAcrm8qBOs4RPMlHp59gTAh9G3oVp1xJBoAHJr4CWbg65+ed9 +d2YbVZjZ3aNbVGGc2Qp2vr9p/pcTtb1oyioCmryQmm3fIOMef6smn/LpFhnFoHUN +bJkBICG2JfHfygYkqukrhGFGv/UnVx7nmkeU5nooh7e0t5/cFbxzAgMBAAGjgaww +gakwHQYDVR0OBBYEFDIkbk6FammEuY6X2HODbctYOIHTMB8GA1UdIwQYMBaAFDIk +bk6FammEuY6X2HODbctYOIHTMA8GA1UdEwEB/wQFMAMBAf8wVgYDVR0RBE8wTaAk +BgorBgEEAYI3FAIDoBYMFHVwbkB0ZXN0ZG9tYWluLmxvY2FsoCUGCisGAQQBgjcU +AgOgFwwVdXBuMkB0ZXN0ZG9tYWluLmxvY2FsMA0GCSqGSIb3DQEBCwUAA4IBAQBq +jj/eTo0ZN6rNYPFYW3Uw4nZLasf3bEQlHG7QPJLaBvg87Yrt+1kWEzDhjlIK1bWi +ns56oLuaXIXjzF6KwkqBRLdqD/1bjPn7qX9uIhdncWs1Fi09mQMdI8Mnasx0IPOe +kosmem3A/RnylWmbaCLON/APhAXrPPbW1abI8gXyH5104T0470PY1CvR4Q6MTbXH +LCOnSiou3CO93H1Rnu9AWDXx5c6Fe1LO+AdaihdXLMAJN6NuMZRcXBChAo6d6/kh +/O44u3tp/z6trRdH+D8D68nyx/xjFqq2BFCfyau9T3KmFjZacUWXQv6tTpElFUlZ +7WkwZWxxkjzh9z529B9h +-----END CERTIFICATE-----`), + }, + "should not match if OtherNames are not equal": { + spec: cmapi.CertificateSpec{ + OtherNames: []cmapi.OtherName{ + { + OID: "1.3.6.1.4.1.311.20.2.3", + UTF8Value: "upn@testdomain.local", + }, + }, + }, + // generated with openssl with: openssl req -nodes -newkey rsa:2048 -subj "/" \ + // -addext 'subjectAltName=otherName:msUPN;UTF8:ANOTHERUPN@testdomain.local' -x509 + data: []byte(`-----BEGIN CERTIFICATE----- +MIIDGzCCAgOgAwIBAgIULTMrWMewcF6XSc8hM6TnL9L8NrgwDQYJKoZIhvcNAQEL +BQAwADAeFw0yNDAxMDkxMzQyMTVaFw0yNDAyMDgxMzQyMTVaMAAwggEiMA0GCSqG +SIb3DQEBAQUAA4IBDwAwggEKAoIBAQCnIvWN7Nj/+bST7R1tmxu0olvjwgfPBhCp +/6OiPuANxZtYkQiqIx4KxnA5KErpQHzp9zlExE2FJUd5Fn83V5+we8/tXRT4mdVg +uOhVab8KHXciW2Ia0B6zdJakYL0qy6ol6kQDUansZi+0vBVbRzJIDAJLRSHGjXRT +BlYuZxgyOawD19vdBKDg3zz2vszQprSONM5qefnk0S3nbsIN3rPprifwjCjn+GMc +pcVXF1UizhyGFTxX7CiTNQg2sD6eAxvNHwyPfYo0cAWVXk1Ctoy+nGWX70zYQIw5 +PI9+hagoFBy8AMhg2MgwAJV3Iay8JRnItCkE5xrh6XxMaGzBDTybAgMBAAGjgYww +gYkwHQYDVR0OBBYEFMjP9HapmDU06sI25oFVVX7h4mziMB8GA1UdIwQYMBaAFMjP +9HapmDU06sI25oFVVX7h4mziMA8GA1UdEwEB/wQFMAMBAf8wNgYDVR0RBC8wLaAr +BgorBgEEAYI3FAIDoB0MG0FOT1RIRVJVUE5AdGVzdGRvbWFpbi5sb2NhbDANBgkq +hkiG9w0BAQsFAAOCAQEAbQLZXPWqT78YmhWich59tiQ+3VStjamS/dI9qrgjo3CN +phYWiTe5anIv1tp2MOFD0eueO+zDLtSfFWLTBq4Qce+fDZK4WEPJrj9A/77WP55R +1IGvQVYhEAGVAiSFudp5loUx6LhcADcO45zWq/RBgWKDI4oUu744UZUJ5e68Vb/O +43QVvRF9qkte8X7LCBr1lX1mElh1d+qD2BiTuLzkMJeDNonmBfD1JM1zCZgYXCoE +20gLNilYVngZprTUOjjBYQMdrovC3XG2ByUTAXREyonQpmzRPKRnV+125kQooLXx +PvQpPM/KS8XNIJZXrbaEw0feitL6Pb+8+W5BHVcDkQ== +-----END CERTIFICATE-----`), + violations: []string{"spec.otherNames"}, + }, "should not match if ipAddresses has been made the commonName": { spec: cmapi.CertificateSpec{ IPAddresses: []string{"127.0.0.1"}, From 736896d2641772fa173fe156946ebac6bd7b0ce8 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 9 Jan 2024 16:40:32 +0100 Subject: [PATCH 4/7] introduce UniversalValue 'Type()' Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/util/pki/asn1_util.go | 65 ++++++++++++++++++++++++--------------- pkg/util/pki/match.go | 50 ++++++++++++------------------ 2 files changed, 60 insertions(+), 55 deletions(-) diff --git a/pkg/util/pki/asn1_util.go b/pkg/util/pki/asn1_util.go index a2d02c17e..ebbe8fc02 100644 --- a/pkg/util/pki/asn1_util.go +++ b/pkg/util/pki/asn1_util.go @@ -48,6 +48,15 @@ func ParseObjectIdentifier(oidString string) (oid asn1.ObjectIdentifier, err err return oid, nil } +type UniversalValueType int + +const ( + UniversalValueTypeBytes UniversalValueType = iota + UniversalValueTypeIA5String + UniversalValueTypeUTF8String + UniversalValueTypePrintableString +) + type UniversalValue struct { Bytes []byte IA5String string @@ -55,50 +64,56 @@ type UniversalValue struct { PrintableString string } -func MarshalUniversalValue(uv UniversalValue) ([]byte, error) { - // Make sure we have only one field set - { - var count int - if uv.Bytes != nil { - count++ - } - if uv.IA5String != "" { - count++ - } - if uv.UTF8String != "" { - count++ - } - if uv.PrintableString != "" { - count++ - } - if count != 1 { - return nil, fmt.Errorf("exactly one field must be set") - } +func (uv UniversalValue) Type() UniversalValueType { + isBytes := uv.Bytes != nil + isIA5String := uv.IA5String != "" + isUTF8String := uv.UTF8String != "" + isPrintableString := uv.PrintableString != "" + + switch { + case isBytes && !isIA5String && !isUTF8String && !isPrintableString: + return UniversalValueTypeBytes + case !isBytes && isIA5String && !isUTF8String && !isPrintableString: + return UniversalValueTypeIA5String + case !isBytes && !isIA5String && isUTF8String && !isPrintableString: + return UniversalValueTypeUTF8String + case !isBytes && !isIA5String && !isUTF8String && isPrintableString: + return UniversalValueTypePrintableString } + return -1 // Either no field is set or two fields are set. +} + +func MarshalUniversalValue(uv UniversalValue) ([]byte, error) { + // Make sure we have only one field set + uvType := uv.Type() var bytes []byte - if uv.Bytes != nil { + switch uvType { + case -1: + return nil, errors.New("UniversalValue should have exactly one field set") + case UniversalValueTypeBytes: bytes = uv.Bytes - } else { + default: rawValue := asn1.RawValue{ Class: asn1.ClassUniversal, IsCompound: false, } - switch { - case uv.IA5String != "": + + switch uvType { + case UniversalValueTypeIA5String: if err := isIA5String(uv.IA5String); err != nil { return nil, errors.New("asn1: invalid IA5 string") } rawValue.Tag = asn1.TagIA5String rawValue.Bytes = []byte(uv.IA5String) - case uv.UTF8String != "": + case UniversalValueTypeUTF8String: if !utf8.ValidString(uv.UTF8String) { return nil, errors.New("asn1: invalid UTF-8 string") } rawValue.Tag = asn1.TagUTF8String rawValue.Bytes = []byte(uv.UTF8String) - case uv.PrintableString != "": + case UniversalValueTypePrintableString: if !isPrintable(uv.PrintableString) { return nil, errors.New("asn1: invalid PrintableString string") } diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index 0024db5f9..d045abcc1 100644 --- a/pkg/util/pki/match.go +++ b/pkg/util/pki/match.go @@ -22,7 +22,6 @@ import ( "crypto/ed25519" "crypto/rsa" "crypto/x509/pkix" - "encoding/asn1" "net" "fmt" @@ -228,51 +227,42 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe return violations, nil } -func matchOtherNames(extension []pkix.Extension, otherNames []cmapi.OtherName) (bool, error) { - sanExtension, err := extractSANExtension(extension) +func matchOtherNames(extension []pkix.Extension, specOtherNames []cmapi.OtherName) (bool, error) { + x509SANExtension, err := extractSANExtension(extension) if err != nil { return false, nil } - generalNames, err := UnmarshalSANs(sanExtension.Value) + x509GeneralNames, err := UnmarshalSANs(x509SANExtension.Value) if err != nil { return false, err } - CertificateRequestOtherNameSpec, err := ToOtherNameSpec(generalNames.OtherNames) - if err != nil { - // This means the CertificateRequest's otherName was not a utf8 valued - return false, nil + x509OtherNames := make([]cmapi.OtherName, 0, len(x509GeneralNames.OtherNames)) + for _, otherName := range x509GeneralNames.OtherNames { + uv, err := UnmarshalUniversalValue(otherName.Value) + if err != nil { + return false, err + } + + if uv.Type() != UniversalValueTypeUTF8String { + // This means the CertificateRequest's otherName was not an utf8 value + return false, fmt.Errorf("otherName is not an utf8 value") + } + + x509OtherNames = append(x509OtherNames, cmapi.OtherName{ + OID: otherName.TypeID.String(), + UTF8Value: uv.UTF8String, + }) } - if !util.EqualOtherNamesUnsorted(CertificateRequestOtherNameSpec, otherNames) { + if !util.EqualOtherNamesUnsorted(x509OtherNames, specOtherNames) { return false, nil } return true, 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. From 3dad3f320babb131aed24dbf324b75552626c8cb Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 9 Jan 2024 16:41:13 +0100 Subject: [PATCH 5/7] don't check OtherNames when fuzzy matching Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/util/pki/match.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index d045abcc1..0b331ae6d 100644 --- a/pkg/util/pki/match.go +++ b/pkg/util/pki/match.go @@ -312,16 +312,6 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp violations = append(violations, "spec.emailAddresses") } - if spec.OtherNames != nil { - matched, err := matchOtherNames(x509cert.Extensions, spec.OtherNames) - if err != nil { - return nil, err - } - if !matched { - violations = append(violations, "spec.otherNames") - } - } - return violations, nil } From 0b83f78fffffde30e1215e8891a3a1cb76352926 Mon Sep 17 00:00:00 2001 From: SpectralHiss Date: Tue, 9 Jan 2024 17:02:24 +0000 Subject: [PATCH 6/7] Remove redundant otherName match tests * We do not need to include otherName in fuzzy certificate detection checks Signed-off-by: SpectralHiss --- pkg/util/pki/match_test.go | 67 -------------------------------------- 1 file changed, 67 deletions(-) diff --git a/pkg/util/pki/match_test.go b/pkg/util/pki/match_test.go index 66db88dff..50516aff4 100644 --- a/pkg/util/pki/match_test.go +++ b/pkg/util/pki/match_test.go @@ -346,73 +346,6 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { }), violations: []string{"spec.ipAddresses"}, }, - "should match if otherNames are equal": { - spec: cmapi.CertificateSpec{ - OtherNames: []cmapi.OtherName{ - { - OID: "1.3.6.1.4.1.311.20.2.3", - UTF8Value: "upn2@testdomain.local", - }, - { - OID: "1.3.6.1.4.1.311.20.2.3", - UTF8Value: "upn@testdomain.local", - }, - }, - }, // openssl req -nodes -newkey rsa:2048 -subj "/CN=someCN" \ - // -addext 'subjectAltName=otherName:msUPN;UTF8:upn@testdomain.local,otherName:msUPN;UTF8:upn2@testdomain.local' -x509 | - data: []byte(`-----BEGIN CERTIFICATE----- -MIIDOzCCAiOgAwIBAgIUJGyXr7GsoPVGC9PkG/QR5NQ3doQwDQYJKoZIhvcNAQEL -BQAwADAeFw0yNDAxMDkxMzQwNDZaFw0yNDAyMDgxMzQwNDZaMAAwggEiMA0GCSqG -SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDFVLGrwoVnLaVERh5l6/+Wc1bDrEOCrsZz -FUYOBJNpoJmbcl6Cp3DLyqrgkzAXWusUft77DmOpMz5C/2IWtI0Ju/NBg2wCwu6U -+NcL70WTx2h1v7fN0YHdzElGcO018bPpA9QEfzoB07G+G8dqTwUMrCq6qE5vbmY3 -PywXfvCKbES4AFvQAcrm8qBOs4RPMlHp59gTAh9G3oVp1xJBoAHJr4CWbg65+ed9 -d2YbVZjZ3aNbVGGc2Qp2vr9p/pcTtb1oyioCmryQmm3fIOMef6smn/LpFhnFoHUN -bJkBICG2JfHfygYkqukrhGFGv/UnVx7nmkeU5nooh7e0t5/cFbxzAgMBAAGjgaww -gakwHQYDVR0OBBYEFDIkbk6FammEuY6X2HODbctYOIHTMB8GA1UdIwQYMBaAFDIk -bk6FammEuY6X2HODbctYOIHTMA8GA1UdEwEB/wQFMAMBAf8wVgYDVR0RBE8wTaAk -BgorBgEEAYI3FAIDoBYMFHVwbkB0ZXN0ZG9tYWluLmxvY2FsoCUGCisGAQQBgjcU -AgOgFwwVdXBuMkB0ZXN0ZG9tYWluLmxvY2FsMA0GCSqGSIb3DQEBCwUAA4IBAQBq -jj/eTo0ZN6rNYPFYW3Uw4nZLasf3bEQlHG7QPJLaBvg87Yrt+1kWEzDhjlIK1bWi -ns56oLuaXIXjzF6KwkqBRLdqD/1bjPn7qX9uIhdncWs1Fi09mQMdI8Mnasx0IPOe -kosmem3A/RnylWmbaCLON/APhAXrPPbW1abI8gXyH5104T0470PY1CvR4Q6MTbXH -LCOnSiou3CO93H1Rnu9AWDXx5c6Fe1LO+AdaihdXLMAJN6NuMZRcXBChAo6d6/kh -/O44u3tp/z6trRdH+D8D68nyx/xjFqq2BFCfyau9T3KmFjZacUWXQv6tTpElFUlZ -7WkwZWxxkjzh9z529B9h ------END CERTIFICATE-----`), - }, - "should not match if OtherNames are not equal": { - spec: cmapi.CertificateSpec{ - OtherNames: []cmapi.OtherName{ - { - OID: "1.3.6.1.4.1.311.20.2.3", - UTF8Value: "upn@testdomain.local", - }, - }, - }, - // generated with openssl with: openssl req -nodes -newkey rsa:2048 -subj "/" \ - // -addext 'subjectAltName=otherName:msUPN;UTF8:ANOTHERUPN@testdomain.local' -x509 - data: []byte(`-----BEGIN CERTIFICATE----- -MIIDGzCCAgOgAwIBAgIULTMrWMewcF6XSc8hM6TnL9L8NrgwDQYJKoZIhvcNAQEL -BQAwADAeFw0yNDAxMDkxMzQyMTVaFw0yNDAyMDgxMzQyMTVaMAAwggEiMA0GCSqG -SIb3DQEBAQUAA4IBDwAwggEKAoIBAQCnIvWN7Nj/+bST7R1tmxu0olvjwgfPBhCp -/6OiPuANxZtYkQiqIx4KxnA5KErpQHzp9zlExE2FJUd5Fn83V5+we8/tXRT4mdVg -uOhVab8KHXciW2Ia0B6zdJakYL0qy6ol6kQDUansZi+0vBVbRzJIDAJLRSHGjXRT -BlYuZxgyOawD19vdBKDg3zz2vszQprSONM5qefnk0S3nbsIN3rPprifwjCjn+GMc -pcVXF1UizhyGFTxX7CiTNQg2sD6eAxvNHwyPfYo0cAWVXk1Ctoy+nGWX70zYQIw5 -PI9+hagoFBy8AMhg2MgwAJV3Iay8JRnItCkE5xrh6XxMaGzBDTybAgMBAAGjgYww -gYkwHQYDVR0OBBYEFMjP9HapmDU06sI25oFVVX7h4mziMB8GA1UdIwQYMBaAFMjP -9HapmDU06sI25oFVVX7h4mziMA8GA1UdEwEB/wQFMAMBAf8wNgYDVR0RBC8wLaAr -BgorBgEEAYI3FAIDoB0MG0FOT1RIRVJVUE5AdGVzdGRvbWFpbi5sb2NhbDANBgkq -hkiG9w0BAQsFAAOCAQEAbQLZXPWqT78YmhWich59tiQ+3VStjamS/dI9qrgjo3CN -phYWiTe5anIv1tp2MOFD0eueO+zDLtSfFWLTBq4Qce+fDZK4WEPJrj9A/77WP55R -1IGvQVYhEAGVAiSFudp5loUx6LhcADcO45zWq/RBgWKDI4oUu744UZUJ5e68Vb/O -43QVvRF9qkte8X7LCBr1lX1mElh1d+qD2BiTuLzkMJeDNonmBfD1JM1zCZgYXCoE -20gLNilYVngZprTUOjjBYQMdrovC3XG2ByUTAXREyonQpmzRPKRnV+125kQooLXx -PvQpPM/KS8XNIJZXrbaEw0feitL6Pb+8+W5BHVcDkQ== ------END CERTIFICATE-----`), - violations: []string{"spec.otherNames"}, - }, "should not match if ipAddresses has been made the commonName": { spec: cmapi.CertificateSpec{ IPAddresses: []string{"127.0.0.1"}, From 892e6eef01cb373631264348df61097e04c3a90e Mon Sep 17 00:00:00 2001 From: SpectralHiss Date: Wed, 10 Jan 2024 10:35:43 +0000 Subject: [PATCH 7/7] Fix OtherName Value UniversalValue .Type() detection Signed-off-by: SpectralHiss --- pkg/util/pki/match.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index 0b331ae6d..b735cab9f 100644 --- a/pkg/util/pki/match.go +++ b/pkg/util/pki/match.go @@ -22,6 +22,7 @@ import ( "crypto/ed25519" "crypto/rsa" "crypto/x509/pkix" + "encoding/asn1" "net" "fmt" @@ -240,14 +241,23 @@ func matchOtherNames(extension []pkix.Extension, specOtherNames []cmapi.OtherNam x509OtherNames := make([]cmapi.OtherName, 0, len(x509GeneralNames.OtherNames)) for _, otherName := range x509GeneralNames.OtherNames { - uv, err := UnmarshalUniversalValue(otherName.Value) + + var otherNameInnerValue asn1.RawValue + // We have to perform one more level of unwrapping because value is still context specific class + // tagged 0 + _, err := asn1.Unmarshal(otherName.Value.Bytes, &otherNameInnerValue) + if err != nil { + return false, err + } + + uv, err := UnmarshalUniversalValue(otherNameInnerValue) if err != nil { return false, err } if uv.Type() != UniversalValueTypeUTF8String { // This means the CertificateRequest's otherName was not an utf8 value - return false, fmt.Errorf("otherName is not an utf8 value") + return false, fmt.Errorf("otherName is not an utf8 value, got: %v", uv.Type()) } x509OtherNames = append(x509OtherNames, cmapi.OtherName{