From 41404a7fd7076b458f7b8f6b26daf8805bd32089 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Wed, 3 Jan 2024 15:49:18 +0100 Subject: [PATCH] rename UseCertificateRequestNameConstraints to NameConstraints Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- deploy/crds/crd-certificates.yaml | 2 +- .../apis/certmanager/types_certificate.go | 2 +- .../certmanager/v1alpha2/types_certificate.go | 2 +- .../certmanager/v1alpha3/types_certificate.go | 2 +- .../certmanager/v1beta1/types_certificate.go | 2 +- .../certmanager/validation/certificate.go | 19 +++++++++--------- .../validation/certificate_test.go | 20 +++++++++---------- internal/controller/feature/features.go | 18 ++++++++--------- internal/webhook/feature/features.go | 14 ++++++------- make/e2e-setup.mk | 6 +++--- pkg/apis/certmanager/v1/types_certificate.go | 2 +- .../requestmanager_controller.go | 4 ++-- pkg/util/pki/csr.go | 14 ++++++------- pkg/util/pki/csr_test.go | 12 +++++------ 14 files changed, 59 insertions(+), 60 deletions(-) diff --git a/deploy/crds/crd-certificates.yaml b/deploy/crds/crd-certificates.yaml index 1ea817242..e6e9938f2 100644 --- a/deploy/crds/crd-certificates.yaml +++ b/deploy/crds/crd-certificates.yaml @@ -179,7 +179,7 @@ spec: description: "Requested X.509 certificate subject, represented using the LDAP \"String Representation of a Distinguished Name\" [1]. Important: the LDAP string format also specifies the order of the attributes in the subject, this is important when issuing certs for LDAP authentication. Example: `CN=foo,DC=corp,DC=example,DC=com` More info [1]: https://datatracker.ietf.org/doc/html/rfc4514 More info: https://github.com/cert-manager/cert-manager/issues/3203 More info: https://github.com/cert-manager/cert-manager/issues/4424 \n Cannot be set if the `subject` or `commonName` field is set. This is an Alpha Feature and is only enabled with the `--feature-gates=LiteralCertificateSubject=true` option set on both the controller and webhook components." type: string nameConstraints: - description: "x.509 certificate NameConstraint extension which MUST NOT be used in a non-CA certificate. More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 \n This is an Alpha Feature and is only enabled with the `--feature-gates=useCertificateRequestNameConstraints=true` option set on both the controller and webhook components." + description: "x.509 certificate NameConstraint extension which MUST NOT be used in a non-CA certificate. More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 \n This is an Alpha Feature and is only enabled with the `--feature-gates=NameConstraints=true` option set on both the controller and webhook components." type: object properties: critical: diff --git a/internal/apis/certmanager/types_certificate.go b/internal/apis/certmanager/types_certificate.go index c9657994b..dbffdbc06 100644 --- a/internal/apis/certmanager/types_certificate.go +++ b/internal/apis/certmanager/types_certificate.go @@ -249,7 +249,7 @@ type CertificateSpec struct { // More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 // // This is an Alpha Feature and is only enabled with the - // `--feature-gates=useCertificateRequestNameConstraints=true` option set on both + // `--feature-gates=NameConstraints=true` option set on both // the controller and webhook components. // +optional NameConstraints *NameConstraints diff --git a/internal/apis/certmanager/v1alpha2/types_certificate.go b/internal/apis/certmanager/v1alpha2/types_certificate.go index ca9742c32..332058ae3 100644 --- a/internal/apis/certmanager/v1alpha2/types_certificate.go +++ b/internal/apis/certmanager/v1alpha2/types_certificate.go @@ -235,7 +235,7 @@ type CertificateSpec struct { // More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 // // This is an Alpha Feature and is only enabled with the - // `--feature-gates=useCertificateRequestNameConstraints=true` option set on both + // `--feature-gates=NameConstraints=true` option set on both // the controller and webhook components. // +optional NameConstraints *NameConstraints `json:"nameConstraints,omitempty"` diff --git a/internal/apis/certmanager/v1alpha3/types_certificate.go b/internal/apis/certmanager/v1alpha3/types_certificate.go index 41b3f6c2a..8303549df 100644 --- a/internal/apis/certmanager/v1alpha3/types_certificate.go +++ b/internal/apis/certmanager/v1alpha3/types_certificate.go @@ -233,7 +233,7 @@ type CertificateSpec struct { // More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 // // This is an Alpha Feature and is only enabled with the - // `--feature-gates=useCertificateRequestNameConstraints=true` option set on both + // `--feature-gates=NameConstraints=true` option set on both // the controller and webhook components. // +optional NameConstraints *NameConstraints `json:"nameConstraints,omitempty"` diff --git a/internal/apis/certmanager/v1beta1/types_certificate.go b/internal/apis/certmanager/v1beta1/types_certificate.go index 806df3c25..6446e80cf 100644 --- a/internal/apis/certmanager/v1beta1/types_certificate.go +++ b/internal/apis/certmanager/v1beta1/types_certificate.go @@ -210,7 +210,7 @@ type CertificateSpec struct { // More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 // // This is an Alpha Feature and is only enabled with the - // `--feature-gates=useCertificateRequestNameConstraints=true` option set on both + // `--feature-gates=NameConstraints=true` option set on both // the controller and webhook components. // +optional NameConstraints *NameConstraints `json:"nameConstraints,omitempty"` diff --git a/internal/apis/certmanager/validation/certificate.go b/internal/apis/certmanager/validation/certificate.go index b1a02b8fa..a9c0e4182 100644 --- a/internal/apis/certmanager/validation/certificate.go +++ b/internal/apis/certmanager/validation/certificate.go @@ -180,17 +180,16 @@ func ValidateCertificateSpec(crt *internalcmapi.CertificateSpec, fldPath *field. } if crt.NameConstraints != nil { - if !utilfeature.DefaultFeatureGate.Enabled(feature.UseCertificateRequestNameConstraints) { - el = append(el, field.Forbidden(fldPath.Child("nameConstraints"), "feature gate UseCertificateRequestNameConstraints must be enabled")) - return el - } + if !utilfeature.DefaultFeatureGate.Enabled(feature.NameConstraints) { + el = append(el, field.Forbidden(fldPath.Child("nameConstraints"), "feature gate NameConstraints must be enabled")) + } else { + if !crt.IsCA { + el = append(el, field.Invalid(fldPath.Child("nameConstraints"), crt.NameConstraints, "isCa should be true when nameConstraints is set")) + } - if !crt.IsCA { - el = append(el, field.Invalid(fldPath.Child("nameConstraints"), crt.NameConstraints, "isCa should be true when nameConstraints is set")) - } - - if crt.NameConstraints.Permitted == nil && crt.NameConstraints.Excluded == nil { - el = append(el, field.Invalid(fldPath.Child("nameConstraints"), crt.NameConstraints, "either permitted or excluded must be set")) + if crt.NameConstraints.Permitted == nil && crt.NameConstraints.Excluded == nil { + el = append(el, field.Invalid(fldPath.Child("nameConstraints"), crt.NameConstraints, "either permitted or excluded must be set")) + } } } diff --git a/internal/apis/certmanager/validation/certificate_test.go b/internal/apis/certmanager/validation/certificate_test.go index 14757b895..6d3791d2e 100644 --- a/internal/apis/certmanager/validation/certificate_test.go +++ b/internal/apis/certmanager/validation/certificate_test.go @@ -61,11 +61,11 @@ func int32Ptr(i int32) *int32 { func TestValidateCertificate(t *testing.T) { fldPath := field.NewPath("spec") scenarios := map[string]struct { - cfg *internalcmapi.Certificate - a *admissionv1.AdmissionRequest - errs []*field.Error - warnings []string - useCertificateRequestNameConstraints bool + cfg *internalcmapi.Certificate + a *admissionv1.AdmissionRequest + errs []*field.Error + warnings []string + nameConstraintsFeatureEnabled bool }{ "valid basic certificate": { cfg: &internalcmapi.Certificate{ @@ -710,8 +710,8 @@ func TestValidateCertificate(t *testing.T) { }, }, }, - a: someAdmissionRequest, - useCertificateRequestNameConstraints: true, + a: someAdmissionRequest, + nameConstraintsFeatureEnabled: true, }, "invalid with name constraints": { cfg: &internalcmapi.Certificate{ @@ -730,7 +730,7 @@ func TestValidateCertificate(t *testing.T) { field.Invalid( fldPath.Child("nameConstraints"), &internalcmapi.NameConstraints{}, "either permitted or excluded must be set"), }, - useCertificateRequestNameConstraints: true, + nameConstraintsFeatureEnabled: true, }, "valid name constraints with feature gate disabled": { cfg: &internalcmapi.Certificate{ @@ -751,13 +751,13 @@ func TestValidateCertificate(t *testing.T) { a: someAdmissionRequest, errs: []*field.Error{ field.Forbidden( - fldPath.Child("nameConstraints"), "feature gate UseCertificateRequestNameConstraints must be enabled"), + fldPath.Child("nameConstraints"), "feature gate NameConstraints must be enabled"), }, }, } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.UseCertificateRequestNameConstraints, s.useCertificateRequestNameConstraints)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.NameConstraints, s.nameConstraintsFeatureEnabled)() errs, warnings := ValidateCertificate(s.a, s.cfg) assert.ElementsMatch(t, errs, s.errs) assert.ElementsMatch(t, warnings, s.warnings) diff --git a/internal/controller/feature/features.go b/internal/controller/feature/features.go index c339893ba..dcaa66de5 100644 --- a/internal/controller/feature/features.go +++ b/internal/controller/feature/features.go @@ -98,14 +98,6 @@ const ( // Github Issue: https://github.com/cert-manager/cert-manager/issues/5539 UseCertificateRequestBasicConstraints featuregate.Feature = "UseCertificateRequestBasicConstraints" - // Owner: @tanujd11 - // Alpha: v1.14 - // - // UseCertificateRequestNameConstraints will add Name Constraints section in the Extension Request of the Certificate Signing Request - // This feature will add NameConstraints section in CSR with CA field as true - // Github Issue: https://github.com/cert-manager/cert-manager/issues/3655 - UseCertificateRequestNameConstraints featuregate.Feature = "UseCertificateRequestNameConstraints" - // Owner: @irbekrm // Alpha v1.12 // Beta: v1.13 @@ -127,6 +119,14 @@ const ( // the usages field empty. DisallowInsecureCSRUsageDefinition featuregate.Feature = "DisallowInsecureCSRUsageDefinition" + // Owner: @tanujd11 + // Alpha: v1.14 + // + // NameConstraints adds support for Name Constraints in Certificate resources + // with IsCA=true. + // Github Issue: https://github.com/cert-manager/cert-manager/issues/3655 + NameConstraints featuregate.Feature = "NameConstraints" + // Owner: @SpectralHiss // Alpha: v1.14 // @@ -155,6 +155,6 @@ var defaultCertManagerFeatureGates = map[featuregate.Feature]featuregate.Feature ServerSideApply: {Default: false, PreRelease: featuregate.Alpha}, LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha}, UseCertificateRequestBasicConstraints: {Default: false, PreRelease: featuregate.Alpha}, - UseCertificateRequestNameConstraints: {Default: false, PreRelease: featuregate.Alpha}, + NameConstraints: {Default: false, PreRelease: featuregate.Alpha}, OtherNames: {Default: false, PreRelease: featuregate.Alpha}, } diff --git a/internal/webhook/feature/features.go b/internal/webhook/feature/features.go index be36a4780..89105eb9d 100644 --- a/internal/webhook/feature/features.go +++ b/internal/webhook/feature/features.go @@ -65,10 +65,10 @@ const ( // Owner: @tanujd11 // Alpha: v1.14 // - // UseCertificateRequestNameConstraints will add Name Constraints section in the Extension Request of the Certificate Signing Request - // This feature will add NameConstraints section in CSR with CA field as true + // NameConstraints adds support for Name Constraints in Certificate resources + // with IsCA=true. // Github Issue: https://github.com/cert-manager/cert-manager/issues/3655 - UseCertificateRequestNameConstraints featuregate.Feature = "UseCertificateRequestNameConstraints" + NameConstraints featuregate.Feature = "NameConstraints" // Owner: @SpectralHiss // Alpha: v1.14 @@ -93,8 +93,8 @@ func init() { var webhookFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ DisallowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.Beta}, - AdditionalCertificateOutputFormats: {Default: false, PreRelease: featuregate.Alpha}, - LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha}, - UseCertificateRequestNameConstraints: {Default: false, PreRelease: featuregate.Alpha}, - OtherNames: {Default: false, PreRelease: featuregate.Alpha}, + AdditionalCertificateOutputFormats: {Default: false, PreRelease: featuregate.Alpha}, + LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha}, + NameConstraints: {Default: false, PreRelease: featuregate.Alpha}, + OtherNames: {Default: false, PreRelease: featuregate.Alpha}, } diff --git a/make/e2e-setup.mk b/make/e2e-setup.mk index d7e548212..37659d54c 100644 --- a/make/e2e-setup.mk +++ b/make/e2e-setup.mk @@ -221,7 +221,7 @@ $(call local-image-tar,vaultretagged): $(call image-tar,vault) tar cf $@ -C /tmp/vault . @rm -rf /tmp/vault -FEATURE_GATES ?= AdditionalCertificateOutputFormats=true,ExperimentalCertificateSigningRequestControllers=true,ExperimentalGatewayAPISupport=true,ServerSideApply=true,LiteralCertificateSubject=true,UseCertificateRequestBasicConstraints=true,UseCertificateRequestNameConstraints=true,OtherNames=true +FEATURE_GATES ?= AdditionalCertificateOutputFormats=true,ExperimentalCertificateSigningRequestControllers=true,ExperimentalGatewayAPISupport=true,ServerSideApply=true,LiteralCertificateSubject=true,UseCertificateRequestBasicConstraints=true,NameConstraints=true,OtherNames=true ## Set this environment variable to a non empty string to cause cert-manager to ## be installed using best-practice configuration settings, and to install @@ -262,8 +262,8 @@ comma = , # Helm's "--set" interprets commas, which means we want to escape commas # for "--set featureGates". That's why we have "\$(comma)". -feature_gates_controller := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% ValidateCAA=% ExperimentalCertificateSigningRequestControllers=% ExperimentalGatewayAPISupport=% ServerSideApply=% LiteralCertificateSubject=% UseCertificateRequestBasicConstraints=% UseCertificateRequestNameConstraints=% SecretsFilteredCaching=% OtherNames=%, $(subst $(comma),$(space),$(FEATURE_GATES)))) -feature_gates_webhook := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% LiteralCertificateSubject=% UseCertificateRequestNameConstraints=% OtherNames=%, $(subst $(comma),$(space),$(FEATURE_GATES)))) +feature_gates_controller := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% ValidateCAA=% ExperimentalCertificateSigningRequestControllers=% ExperimentalGatewayAPISupport=% ServerSideApply=% LiteralCertificateSubject=% UseCertificateRequestBasicConstraints=% NameConstraints=% SecretsFilteredCaching=% OtherNames=%, $(subst $(comma),$(space),$(FEATURE_GATES)))) +feature_gates_webhook := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% LiteralCertificateSubject=% NameConstraints=% OtherNames=%, $(subst $(comma),$(space),$(FEATURE_GATES)))) feature_gates_cainjector := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% ServerSideApply=%, $(subst $(comma),$(space),$(FEATURE_GATES)))) # Install cert-manager with E2E specific images and deployment settings. diff --git a/pkg/apis/certmanager/v1/types_certificate.go b/pkg/apis/certmanager/v1/types_certificate.go index 8e6b81bb5..743cd3e35 100644 --- a/pkg/apis/certmanager/v1/types_certificate.go +++ b/pkg/apis/certmanager/v1/types_certificate.go @@ -275,7 +275,7 @@ type CertificateSpec struct { // More Info: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 // // This is an Alpha Feature and is only enabled with the - // `--feature-gates=useCertificateRequestNameConstraints=true` option set on both + // `--feature-gates=NameConstraints=true` option set on both // the controller and webhook components. // +optional NameConstraints *NameConstraints `json:"nameConstraints,omitempty"` diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index 7d59f9fc5..0bdb6628a 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -354,8 +354,8 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi crt, pki.WithUseLiteralSubject(utilfeature.DefaultMutableFeatureGate.Enabled(feature.LiteralCertificateSubject)), pki.WithEncodeBasicConstraintsInRequest(utilfeature.DefaultMutableFeatureGate.Enabled(feature.UseCertificateRequestBasicConstraints)), - pki.WithEncodeNameConstraintsInRequest(utilfeature.DefaultMutableFeatureGate.Enabled(feature.UseCertificateRequestNameConstraints)), - pki.WithEncodeOtherNames(utilfeature.DefaultMutableFeatureGate.Enabled(feature.OtherNames)), + pki.WithNameConstraints(utilfeature.DefaultMutableFeatureGate.Enabled(feature.NameConstraints)), + pki.WithOtherNames(utilfeature.DefaultMutableFeatureGate.Enabled(feature.OtherNames)), ) if err != nil { log.Error(err, "Failed to generate CSR - will not retry") diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index fd59ed391..c3f1833c7 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -187,7 +187,7 @@ func BuildCertManagerKeyUsages(ku x509.KeyUsage, eku []x509.ExtKeyUsage) []v1.Ke type generateCSROptions struct { EncodeBasicConstraintsInRequest bool - EncodeNameConstraintsInRequest bool + EncodeNameConstraints bool EncodeOtherNames bool UseLiteralSubject bool } @@ -203,15 +203,15 @@ func WithEncodeBasicConstraintsInRequest(encode bool) GenerateCSROption { } } -func WithEncodeNameConstraintsInRequest(encode bool) GenerateCSROption { +func WithNameConstraints(enabled bool) GenerateCSROption { return func(o *generateCSROptions) { - o.EncodeNameConstraintsInRequest = encode + o.EncodeNameConstraints = enabled } } -func WithEncodeOtherNames(encodeOtherNames bool) GenerateCSROption { +func WithOtherNames(enabled bool) GenerateCSROption { return func(o *generateCSROptions) { - o.EncodeOtherNames = encodeOtherNames + o.EncodeOtherNames = enabled } } @@ -228,7 +228,7 @@ func WithUseLiteralSubject(useLiteralSubject bool) GenerateCSROption { func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.CertificateRequest, error) { opts := &generateCSROptions{ EncodeBasicConstraintsInRequest: false, - EncodeNameConstraintsInRequest: false, + EncodeNameConstraints: false, EncodeOtherNames: false, UseLiteralSubject: false, } @@ -363,7 +363,7 @@ func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.Cert extraExtensions = append(extraExtensions, basicExtension) } - if opts.EncodeNameConstraintsInRequest && crt.Spec.NameConstraints != nil { + if opts.EncodeNameConstraints && crt.Spec.NameConstraints != nil { nameConstraints := &NameConstraints{} if crt.Spec.NameConstraints.Permitted != nil { diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index 2c41ad37c..6ad2f2279 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -409,7 +409,7 @@ func TestGenerateCSR(t *testing.T) { literalCertificateSubjectFeatureEnabled bool basicConstraintsFeatureEnabled bool nameConstraintsFeatureEnabled bool - encodeOtherNamesFeatureEnabled bool + otherNamesFeatureEnabled bool }{ { name: "Generate CSR from certificate with only DNS", @@ -562,7 +562,7 @@ func TestGenerateCSR(t *testing.T) { }, RawSubject: subjectGenerator(t, pkix.Name{}), }, - encodeOtherNamesFeatureEnabled: true, + otherNamesFeatureEnabled: true, }, { name: "Generate CSR from certificate with multiple valid otherName oids and emailSANs set", @@ -601,7 +601,7 @@ func TestGenerateCSR(t *testing.T) { }, RawSubject: subjectGenerator(t, pkix.Name{}), }, - encodeOtherNamesFeatureEnabled: true, + otherNamesFeatureEnabled: true, }, { name: "Generate CSR from certificate with malformed otherName oid type", @@ -771,7 +771,7 @@ func TestGenerateCSR(t *testing.T) { wantErr: false, }, { - name: "Generate CSR from certificate with UseCertificateRequestNameConstraints flag enabled", + name: "Generate CSR from certificate with NameConstraints flag enabled", crt: &cmapi.Certificate{Spec: cmapi.CertificateSpec{ CommonName: "example.org", IsCA: true, @@ -814,8 +814,8 @@ func TestGenerateCSR(t *testing.T) { got, err := GenerateCSR( tt.crt, WithEncodeBasicConstraintsInRequest(tt.basicConstraintsFeatureEnabled), - WithEncodeNameConstraintsInRequest(tt.nameConstraintsFeatureEnabled), - WithEncodeOtherNames(tt.encodeOtherNamesFeatureEnabled), + WithNameConstraints(tt.nameConstraintsFeatureEnabled), + WithOtherNames(tt.otherNamesFeatureEnabled), WithUseLiteralSubject(tt.literalCertificateSubjectFeatureEnabled), ) if (err != nil) != tt.wantErr {