From bb39c5cf7969925b02431553f004188bb0729b25 Mon Sep 17 00:00:00 2001 From: Sathyanarayanan Saravanamuthu Date: Thu, 3 Nov 2022 15:34:25 +0530 Subject: [PATCH 1/3] Fixing CA flag in basic constraints extension Signed-off-by: Sathyanarayanan Saravanamuthu --- pkg/util/pki/csr.go | 29 +++++++++++++++++++++++++++++ pkg/util/pki/keyusage.go | 1 + 2 files changed, 30 insertions(+) diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 2afe88e76..34dae8bbc 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -216,6 +216,14 @@ func GenerateCSR(crt *v1.Certificate) (*x509.CertificateRequest, error) { } } + if crt.Spec.IsCA { + extension, err := buildBasicConstraintsExtensionsForCertificate() + if err != nil { + return nil, err + } + extraExtensions = append(extraExtensions, extension) + } + if isLiteralCertificateSubjectEnabled() && len(crt.Spec.LiteralSubject) > 0 { rawSubject, err := ParseSubjectStringToRawDerBytes(crt.Spec.LiteralSubject) if err != nil { @@ -298,6 +306,27 @@ func buildKeyUsagesExtensionsForCertificate(crt *v1.Certificate) ([]pkix.Extensi return extraExtensions, nil } +func buildBasicConstraintsExtensionsForCertificate() (pkix.Extension, error) { + + basicConstraints := pkix.Extension{ + Id: OIDExtensionBasicConstraints, + } + + constraint := struct { + IsCA bool + }{ + IsCA: true, + } + + var err error + basicConstraints.Value, err = asn1.Marshal(constraint) + if err != nil { + return pkix.Extension{}, err + } + + return basicConstraints, nil +} + // GenerateTemplate will create a x509.Certificate for the given Certificate resource. // This should create a Certificate template that is equivalent to the CertificateRequest // generated by GenerateCSR. diff --git a/pkg/util/pki/keyusage.go b/pkg/util/pki/keyusage.go index 37f414401..7d6215672 100644 --- a/pkg/util/pki/keyusage.go +++ b/pkg/util/pki/keyusage.go @@ -26,6 +26,7 @@ import ( var ( OIDExtensionKeyUsage = []int{2, 5, 29, 15} OIDExtensionExtendedKeyUsage = []int{2, 5, 29, 37} + OIDExtensionBasicConstraints = []int{2, 5, 29, 19} ) // RFC 5280, 4.2.1.12 Extended Key Usage From d4de98d35b13a0553d1019e85cc0816262a9bdc7 Mon Sep 17 00:00:00 2001 From: Sathyanarayanan Saravanamuthu Date: Sun, 6 Nov 2022 09:36:26 +0530 Subject: [PATCH 2/3] Adding unit tests Signed-off-by: Sathyanarayanan Saravanamuthu --- pkg/util/pki/csr_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index 4430db6d4..b2f746732 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -416,6 +416,32 @@ func TestGenerateCSR(t *testing.T) { }, } + basicConstraintsValue, err := asn1.Marshal(struct { + IsCA bool + }{ + IsCA: true, + }) + if err != nil { + t.Fatal(err) + } + + // 0xa0 = DigitalSignature, Encipherment and KeyCertSign usage + asn1KeyUsageWithCa, err := asn1.Marshal(asn1.BitString{Bytes: []byte{0xa4}, BitLength: asn1BitLength([]byte{0xa4})}) + if err != nil { + t.Fatal(err) + } + + basicConstraintsExtensions := []pkix.Extension{ + { + Id: OIDExtensionKeyUsage, + Value: asn1KeyUsageWithCa, + }, + { + Id: OIDExtensionBasicConstraints, + Value: basicConstraintsValue, + }, + } + exampleLiteralSubject := "CN=actual-cn, OU=FooLong, OU=Bar, O=example.org" rawExampleLiteralSubject, err := ParseSubjectStringToRawDerBytes(exampleLiteralSubject) if err != nil { @@ -457,6 +483,17 @@ func TestGenerateCSR(t *testing.T) { ExtraExtensions: defaultExtraExtensions, }, }, + { + name: "Generate CSR from certificate with isCA set", + crt: &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.org", IsCA: true}}, + want: &x509.CertificateRequest{ + Version: 0, + SignatureAlgorithm: x509.SHA256WithRSA, + PublicKeyAlgorithm: x509.RSA, + Subject: pkix.Name{CommonName: "example.org"}, + ExtraExtensions: basicConstraintsExtensions, + }, + }, { name: "Generate CSR from certificate with extended key usages", crt: &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.org", Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageIPsecEndSystem}}}, From 860ba8465a9fc5f680743f1f7e3888f98573cfcc Mon Sep 17 00:00:00 2001 From: Sathyanarayanan Saravanamuthu Date: Thu, 10 Nov 2022 14:27:26 +0530 Subject: [PATCH 3/3] Addressing review comments Signed-off-by: Sathyanarayanan Saravanamuthu --- internal/controller/feature/features.go | 7 +++ pkg/util/pki/csr.go | 8 +-- pkg/util/pki/csr_test.go | 81 +++++++++++++++++++------ 3 files changed, 75 insertions(+), 21 deletions(-) diff --git a/internal/controller/feature/features.go b/internal/controller/feature/features.go index db4e65a68..05c7b39e6 100644 --- a/internal/controller/feature/features.go +++ b/internal/controller/feature/features.go @@ -64,6 +64,12 @@ const ( // This feature gate will disable auto-generated CertificateRequest name // Github Issue: https://github.com/cert-manager/cert-manager/issues/4956 StableCertificateRequestName featuregate.Feature = "StableCertificateRequestName" + + // Alpha: v1.11 + // UseCertificateRequestBasicConstraints will add Basic Constraints section in the Extension Request of the Certificate Signing Request + // This feature will add BasicConstraints section with CA field defaulting to false; CA field will be set true if the Certificate resource spec has isCA as true + // Github Issue: https://github.com/cert-manager/cert-manager/issues/5539 + UseCertificateRequestBasicConstraints featuregate.Feature = "UseCertificateRequestBasicConstraints" ) func init() { @@ -81,4 +87,5 @@ var defaultCertManagerFeatureGates = map[featuregate.Feature]featuregate.Feature ServerSideApply: {Default: false, PreRelease: featuregate.Alpha}, LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha}, StableCertificateRequestName: {Default: false, PreRelease: featuregate.Alpha}, + UseCertificateRequestBasicConstraints: {Default: false, PreRelease: featuregate.Alpha}, } diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 34dae8bbc..bb4776fa6 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -216,8 +216,8 @@ func GenerateCSR(crt *v1.Certificate) (*x509.CertificateRequest, error) { } } - if crt.Spec.IsCA { - extension, err := buildBasicConstraintsExtensionsForCertificate() + if utilfeature.DefaultFeatureGate.Enabled(feature.UseCertificateRequestBasicConstraints) { + extension, err := buildBasicConstraintsExtensionsForCertificate(crt.Spec.IsCA) if err != nil { return nil, err } @@ -306,7 +306,7 @@ func buildKeyUsagesExtensionsForCertificate(crt *v1.Certificate) ([]pkix.Extensi return extraExtensions, nil } -func buildBasicConstraintsExtensionsForCertificate() (pkix.Extension, error) { +func buildBasicConstraintsExtensionsForCertificate(isCA bool) (pkix.Extension, error) { basicConstraints := pkix.Extension{ Id: OIDExtensionBasicConstraints, @@ -315,7 +315,7 @@ func buildBasicConstraintsExtensionsForCertificate() (pkix.Extension, error) { constraint := struct { IsCA bool }{ - IsCA: true, + IsCA: isCA, } var err error diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index b2f746732..ddfb784e6 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -416,11 +416,20 @@ func TestGenerateCSR(t *testing.T) { }, } - basicConstraintsValue, err := asn1.Marshal(struct { - IsCA bool - }{ - IsCA: true, - }) + basicConstraintsGenerator := func(isCA bool) ([]byte, error) { + return asn1.Marshal(struct { + IsCA bool + }{ + IsCA: isCA, + }) + } + + basicConstraintsWithCA, err := basicConstraintsGenerator(true) + if err != nil { + t.Fatal(err) + } + + basicConstraintsWithoutCA, err := basicConstraintsGenerator(false) if err != nil { t.Fatal(err) } @@ -431,17 +440,6 @@ func TestGenerateCSR(t *testing.T) { t.Fatal(err) } - basicConstraintsExtensions := []pkix.Extension{ - { - Id: OIDExtensionKeyUsage, - Value: asn1KeyUsageWithCa, - }, - { - Id: OIDExtensionBasicConstraints, - Value: basicConstraintsValue, - }, - } - exampleLiteralSubject := "CN=actual-cn, OU=FooLong, OU=Bar, O=example.org" rawExampleLiteralSubject, err := ParseSubjectStringToRawDerBytes(exampleLiteralSubject) if err != nil { @@ -460,6 +458,7 @@ func TestGenerateCSR(t *testing.T) { want *x509.CertificateRequest wantErr bool literalCertificateSubjectFeatureEnabled bool + basicConstraintsFeatureEnabled bool }{ { name: "Generate CSR from certificate with only DNS", @@ -491,9 +490,56 @@ func TestGenerateCSR(t *testing.T) { SignatureAlgorithm: x509.SHA256WithRSA, PublicKeyAlgorithm: x509.RSA, Subject: pkix.Name{CommonName: "example.org"}, - ExtraExtensions: basicConstraintsExtensions, + ExtraExtensions: []pkix.Extension{ + { + Id: OIDExtensionKeyUsage, + Value: asn1KeyUsageWithCa, + }, + }, }, }, + { + name: "Generate CSR from certificate with isCA not set and with UseCertificateRequestBasicConstraints flag enabled", + crt: &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.org"}}, + want: &x509.CertificateRequest{ + Version: 0, + SignatureAlgorithm: x509.SHA256WithRSA, + PublicKeyAlgorithm: x509.RSA, + Subject: pkix.Name{CommonName: "example.org"}, + ExtraExtensions: []pkix.Extension{ + { + Id: OIDExtensionKeyUsage, + Value: asn1KeyUsage, + }, + { + Id: OIDExtensionBasicConstraints, + Value: basicConstraintsWithoutCA, + }, + }, + }, + basicConstraintsFeatureEnabled: true, + }, + { + name: "Generate CSR from certificate with isCA set and with UseCertificateRequestBasicConstraints flag enabled", + crt: &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.org", IsCA: true}}, + want: &x509.CertificateRequest{ + Version: 0, + SignatureAlgorithm: x509.SHA256WithRSA, + PublicKeyAlgorithm: x509.RSA, + Subject: pkix.Name{CommonName: "example.org"}, + ExtraExtensions: []pkix.Extension{ + { + Id: OIDExtensionKeyUsage, + Value: asn1KeyUsageWithCa, + }, + { + Id: OIDExtensionBasicConstraints, + Value: basicConstraintsWithCA, + }, + }, + }, + basicConstraintsFeatureEnabled: true, + }, { name: "Generate CSR from certificate with extended key usages", crt: &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.org", Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageIPsecEndSystem}}}, @@ -555,6 +601,7 @@ func TestGenerateCSR(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.LiteralCertificateSubject, tt.literalCertificateSubjectFeatureEnabled)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.UseCertificateRequestBasicConstraints, tt.basicConstraintsFeatureEnabled)() got, err := GenerateCSR(tt.crt) if (err != nil) != tt.wantErr { t.Errorf("GenerateCSR() error = %v, wantErr %v", err, tt.wantErr)