From 0acde5b1a43c0d764d1303658de921f65b8250dd Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Wed, 7 Feb 2024 11:01:34 +0100 Subject: [PATCH] fix changed behavior: set critical flag of SANs extension based on subject Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/util/pki/certificatetemplate.go | 20 +++ pkg/util/pki/certificatetemplate_test.go | 173 +++++++++++++++++++++++ pkg/util/pki/csr.go | 6 +- pkg/util/pki/subject.go | 9 ++ 4 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 pkg/util/pki/certificatetemplate_test.go diff --git a/pkg/util/pki/certificatetemplate.go b/pkg/util/pki/certificatetemplate.go index 5c57a994c..1ad62164d 100644 --- a/pkg/util/pki/certificatetemplate.go +++ b/pkg/util/pki/certificatetemplate.go @@ -261,6 +261,26 @@ func CertificateTemplateFromCSR(csr *x509.CertificateRequest, validatorMutators } } + // Finally, we fix up the certificate template to ensure that it is valid + { + // If the certificate has an empty Subject, we set any SAN extensions to be critical + var asn1Subject []byte + if cert.RawSubject != nil { + asn1Subject = cert.RawSubject + } else { + asn1Subject, err = asn1.Marshal(cert.Subject.ToRDNSequence()) + if err != nil { + return nil, fmt.Errorf("failed to marshal subject to ASN.1 DER: %s", err.Error()) + } + } + + for i := range cert.ExtraExtensions { + if cert.ExtraExtensions[i].Id.Equal(oidExtensionSubjectAltName) { + cert.ExtraExtensions[i].Critical = IsASN1SubjectEmpty(asn1Subject) + } + } + } + return cert, nil } diff --git a/pkg/util/pki/certificatetemplate_test.go b/pkg/util/pki/certificatetemplate_test.go new file mode 100644 index 000000000..ded264a9c --- /dev/null +++ b/pkg/util/pki/certificatetemplate_test.go @@ -0,0 +1,173 @@ +/* +Copyright 2020 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pki + +import ( + "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" + "reflect" + "testing" +) + +func TestCertificateTemplateFromCSR(t *testing.T) { + subjectGenerator := func(t *testing.T, name pkix.Name) []byte { + data, err := MarshalRDNSequenceToRawDERBytes(name.ToRDNSequence()) + if err != nil { + t.Fatal(err) + } + return data + } + + sansGenerator := func(t *testing.T, generalNames []asn1.RawValue, critical bool) pkix.Extension { + val, err := asn1.Marshal(generalNames) + if err != nil { + panic(err) + } + + return pkix.Extension{ + Id: oidExtensionSubjectAltName, + Critical: critical, + Value: val, + } + } + + testCases := []struct { + name string + csr *x509.CertificateRequest + expected *x509.Certificate + }{ + { + name: "should copy subject", + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + Country: []string{"US"}, + Organization: []string{"cert-manager"}, + OrganizationalUnit: []string{"test"}, + CommonName: "test", + }, + }, + expected: &x509.Certificate{ + Version: 3, + Subject: pkix.Name{ + Country: []string{"US"}, + Organization: []string{"cert-manager"}, + OrganizationalUnit: []string{"test"}, + CommonName: "test", + }, + }, + }, + { + name: "should copy raw subject + SANs", + csr: &x509.CertificateRequest{ + RawSubject: subjectGenerator(t, pkix.Name{ + Country: []string{"US"}, + Organization: []string{"cert-manager"}, + OrganizationalUnit: []string{"test"}, + }), + DNSNames: []string{"test.example.com"}, + }, + expected: &x509.Certificate{ + Version: 3, + RawSubject: subjectGenerator(t, pkix.Name{ + Country: []string{"US"}, + Organization: []string{"cert-manager"}, + OrganizationalUnit: []string{"test"}, + }), + DNSNames: []string{"test.example.com"}, + }, + }, + { + name: "should ignore unknown extensions", + csr: &x509.CertificateRequest{ + ExtraExtensions: []pkix.Extension{ + { + Id: []int{1, 2, 3}, + Value: []byte("test"), + }, + }, + }, + expected: &x509.Certificate{ + Version: 3, + }, + }, + { + name: "should copy SANs and not fix critical flag subject is set", + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + Country: []string{"US"}, + Organization: []string{"cert-manager"}, + }, + ExtraExtensions: []pkix.Extension{ + sansGenerator(t, []asn1.RawValue{ + {Tag: 2, Class: 2, Bytes: []byte("test.example.com")}, + }, false), + }, + }, + expected: &x509.Certificate{ + Version: 3, + Subject: pkix.Name{ + Country: []string{"US"}, + Organization: []string{"cert-manager"}, + }, + ExtraExtensions: []pkix.Extension{ + sansGenerator(t, []asn1.RawValue{ + {Tag: 2, Class: 2, Bytes: []byte("test.example.com")}, + }, false), + }, + }, + }, + { + name: "should copy SANs and fix its critical flag", + csr: &x509.CertificateRequest{ + ExtraExtensions: []pkix.Extension{ + sansGenerator(t, []asn1.RawValue{ + {Tag: 2, Class: 2, Bytes: []byte("test.example.com")}, + }, false), + }, + }, + expected: &x509.Certificate{ + Version: 3, + ExtraExtensions: []pkix.Extension{ + sansGenerator(t, []asn1.RawValue{ + {Tag: 2, Class: 2, Bytes: []byte("test.example.com")}, + }, true), + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := CertificateTemplateFromCSR(tc.csr) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if result.SerialNumber == nil { + t.Errorf("expected serial number to be set") + } + + // Set serial number to nil to avoid comparing it + result.SerialNumber = nil + + if !reflect.DeepEqual(result, tc.expected) { + t.Errorf("unexpected result: %v", result) + } + }) + } +} diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 1874c84a8..40727e4b9 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -321,11 +321,7 @@ func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.Cert var extraExtensions []pkix.Extension if !sans.Empty() { - // emptyASN1Subject is the ASN.1 DER encoding of an empty Subject, which is - // just an empty SEQUENCE. - var emptyASN1Subject = []byte{0x30, 0} - - sanExtension, err := MarshalSANs(sans, !bytes.Equal(asn1Subject, emptyASN1Subject)) + sanExtension, err := MarshalSANs(sans, !IsASN1SubjectEmpty(asn1Subject)) if err != nil { return nil, err } diff --git a/pkg/util/pki/subject.go b/pkg/util/pki/subject.go index 11e636839..bf94e637c 100644 --- a/pkg/util/pki/subject.go +++ b/pkg/util/pki/subject.go @@ -17,6 +17,7 @@ limitations under the License. package pki import ( + "bytes" "crypto/x509/pkix" "encoding/asn1" "errors" @@ -88,6 +89,14 @@ func UnmarshalSubjectStringToRDNSequence(subject string) (pkix.RDNSequence, erro return rdns, nil } +func IsASN1SubjectEmpty(asn1Subject []byte) bool { + // emptyASN1Subject is the ASN.1 DER encoding of an empty Subject, which is + // just an empty SEQUENCE. + var emptyASN1Subject = []byte{0x30, 0} + + return bytes.Equal(asn1Subject, emptyASN1Subject) +} + func MarshalRDNSequenceToRawDERBytes(rdnSequence pkix.RDNSequence) ([]byte, error) { return asn1.Marshal(rdnSequence) }