From 38cd0accdbd7259b47350ff164d58840c81ad157 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 26 Apr 2024 16:14:31 +0200 Subject: [PATCH] graduate 'DisallowInsecureCSRUsageDefinition' to GA Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .golangci.yaml | 2 +- .../validation/certificaterequest.go | 34 +--- internal/controller/feature/features.go | 3 +- internal/webhook/feature/features.go | 1 + pkg/util/pki/certificatetemplate.go | 43 ++--- .../validation/certificaterequest_test.go | 165 ------------------ 6 files changed, 25 insertions(+), 223 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 7c830b1f0..2f87f0791 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -45,7 +45,7 @@ issues: text: "SA(1002|1006|4000|4006)" - linters: - staticcheck - text: "(NewCertManagerBasicCertificateRequest|DeprecatedCertificateTemplateFromCertificateRequestAndAllowInsecureCSRUsageDefinition)" + text: "(NewCertManagerBasicCertificateRequest)" linters: # Explicitly define all enabled linters disable-all: true diff --git a/internal/apis/certmanager/validation/certificaterequest.go b/internal/apis/certmanager/validation/certificaterequest.go index 9db156ec8..a57c984ad 100644 --- a/internal/apis/certmanager/validation/certificaterequest.go +++ b/internal/apis/certmanager/validation/certificaterequest.go @@ -27,11 +27,9 @@ import ( cmapi "github.com/cert-manager/cert-manager/internal/apis/certmanager" cmmeta "github.com/cert-manager/cert-manager/internal/apis/meta" - "github.com/cert-manager/cert-manager/internal/webhook/feature" "github.com/cert-manager/cert-manager/pkg/apis/acme" "github.com/cert-manager/cert-manager/pkg/apis/certmanager" cmapiv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" "github.com/cert-manager/cert-manager/pkg/util/pki" ) @@ -109,30 +107,14 @@ func validateCertificateRequestSpecRequest(crSpec *cmapi.CertificateRequestSpec, return el } - // If DisallowInsecureCSRUsageDefinition is disabled and usages is empty, - // then we should allow the request to be created without requiring that the - // CSR usages match the default usages, instead we only validate that the - // BasicConstraints are valid. - // TODO: simplify this logic when we remove the feature gate - if !utilfeature.DefaultMutableFeatureGate.Enabled(feature.DisallowInsecureCSRUsageDefinition) && len(crSpec.Usages) == 0 { - _, err = pki.CertificateTemplateFromCSRPEM( - crSpec.Request, - pki.CertificateTemplateValidateAndOverrideBasicConstraints(crSpec.IsCA, nil), - ) - if err != nil { - el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, err.Error())) - return el - } - } else { - _, err = pki.CertificateTemplateFromCSRPEM( - crSpec.Request, - pki.CertificateTemplateValidateAndOverrideBasicConstraints(crSpec.IsCA, nil), - pki.CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage), - ) - if err != nil { - el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, err.Error())) - return el - } + _, err = pki.CertificateTemplateFromCSRPEM( + crSpec.Request, + pki.CertificateTemplateValidateAndOverrideBasicConstraints(crSpec.IsCA, nil), + pki.CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage), + ) + if err != nil { + el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, err.Error())) + return el } return el diff --git a/internal/controller/feature/features.go b/internal/controller/feature/features.go index 011ef373a..d069f0b90 100644 --- a/internal/controller/feature/features.go +++ b/internal/controller/feature/features.go @@ -113,6 +113,7 @@ const ( // Owner: @inteon // Beta: v1.13 + // GA: v1.15 // // DisallowInsecureCSRUsageDefinition will prevent the webhook from allowing // CertificateRequest's usages to be only defined in the CSR, while leaving @@ -144,7 +145,7 @@ func init() { // To add a new feature, define a key for it above and add it here. The features will be // available on the cert-manager controller binary. var defaultCertManagerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - DisallowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.Beta}, + DisallowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.GA}, StableCertificateRequestName: {Default: true, PreRelease: featuregate.Beta}, SecretsFilteredCaching: {Default: true, PreRelease: featuregate.Beta}, diff --git a/internal/webhook/feature/features.go b/internal/webhook/feature/features.go index 873084507..4e8729525 100644 --- a/internal/webhook/feature/features.go +++ b/internal/webhook/feature/features.go @@ -55,6 +55,7 @@ const ( LiteralCertificateSubject featuregate.Feature = "LiteralCertificateSubject" // Owner: @inteon + // Beta: v1.13 // GA: v1.15 // // DisallowInsecureCSRUsageDefinition will prevent the webhook from allowing diff --git a/pkg/util/pki/certificatetemplate.go b/pkg/util/pki/certificatetemplate.go index 89f963ce3..f92184df2 100644 --- a/pkg/util/pki/certificatetemplate.go +++ b/pkg/util/pki/certificatetemplate.go @@ -321,39 +321,22 @@ func CertificateTemplateFromCertificate(crt *v1.Certificate) (*x509.Certificate, ) } -func makeCertificateTemplateFromCertificateRequestFunc(allowInsecureCSRUsageDefinition bool) func(cr *v1.CertificateRequest) (*x509.Certificate, error) { - return func(cr *v1.CertificateRequest) (*x509.Certificate, error) { - certDuration := apiutil.DefaultCertDuration(cr.Spec.Duration) - keyUsage, extKeyUsage, err := KeyUsagesForCertificateOrCertificateRequest(cr.Spec.Usages, cr.Spec.IsCA) - if err != nil { - return nil, err - } - - return CertificateTemplateFromCSRPEM( - cr.Spec.Request, - CertificateTemplateOverrideDuration(certDuration), - CertificateTemplateValidateAndOverrideBasicConstraints(cr.Spec.IsCA, nil), // Override the basic constraints, but make sure they match the constraints in the CSR if present - (func() CertificateTemplateValidatorMutator { - if allowInsecureCSRUsageDefinition && len(cr.Spec.Usages) == 0 { - // If the CertificateRequest does not specify any usages, and the AllowInsecureCSRUsageDefinition - // flag is set, then we allow the usages to be defined solely by the CSR blob, but we still override - // the usages to match the old behavior. - return certificateTemplateOverrideKeyUsages(keyUsage, extKeyUsage) - } - - // Override the key usages, but make sure they match the usages in the CSR if present - return CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage) - })(), - ) - } -} - // CertificateTemplateFromCertificateRequest will create a x509.Certificate for the given // CertificateRequest resource -var CertificateTemplateFromCertificateRequest = makeCertificateTemplateFromCertificateRequestFunc(false) +func CertificateTemplateFromCertificateRequest(cr *v1.CertificateRequest) (*x509.Certificate, error) { + certDuration := apiutil.DefaultCertDuration(cr.Spec.Duration) + keyUsage, extKeyUsage, err := KeyUsagesForCertificateOrCertificateRequest(cr.Spec.Usages, cr.Spec.IsCA) + if err != nil { + return nil, err + } -// Deprecated: Use CertificateTemplateFromCertificateRequest instead. -var DeprecatedCertificateTemplateFromCertificateRequestAndAllowInsecureCSRUsageDefinition = makeCertificateTemplateFromCertificateRequestFunc(true) + return CertificateTemplateFromCSRPEM( + cr.Spec.Request, + CertificateTemplateOverrideDuration(certDuration), + CertificateTemplateValidateAndOverrideBasicConstraints(cr.Spec.IsCA, nil), // Override the basic constraints, but make sure they match the constraints in the CSR if present + CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage), // Override the key usages, but make sure they match the usages in the CSR if present + ) +} // CertificateTemplateFromCertificateSigningRequest will create a x509.Certificate for the given // CertificateSigningRequest resource diff --git a/test/integration/validation/certificaterequest_test.go b/test/integration/validation/certificaterequest_test.go index a7c8d2d84..f0124e66d 100644 --- a/test/integration/validation/certificaterequest_test.go +++ b/test/integration/validation/certificaterequest_test.go @@ -33,7 +33,6 @@ import ( "github.com/cert-manager/cert-manager/pkg/api" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" - utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" "github.com/cert-manager/cert-manager/pkg/util/pki" ) @@ -176,170 +175,6 @@ func TestValidationCertificateRequests(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*40) defer cancel() - // The default is true, but we set it here to make sure it was not changed by other tests - utilfeature.DefaultMutableFeatureGate.Set("DisallowInsecureCSRUsageDefinition=true") - - config, stop := framework.RunControlPlane(t, ctx) - defer stop() - - framework.WaitForOpenAPIResourcesToBeLoaded(t, ctx, config, certGVK) - - // create the object to get any errors back from the webhook - cl, err := client.New(config, client.Options{Scheme: api.Scheme}) - if err != nil { - t.Fatal(err) - } - - err = cl.Create(ctx, cert) - if test.expectError != (err != nil) { - t.Errorf("unexpected error, exp=%t got=%v", - test.expectError, err) - } - if test.expectError && !strings.HasSuffix(err.Error(), test.errorSuffix) { - t.Errorf("unexpected error suffix, exp=%s got=%s", - test.errorSuffix, err) - } - }) - } -} - -// TestValidationCertificateRequests_DisallowInsecureCSRUsageDefinition_false makes sure that the -// validation webhook keeps working as before when the DisallowInsecureCSRUsageDefinition feature -// gate is disabled. -func TestValidationCertificateRequests_DisallowInsecureCSRUsageDefinition_false(t *testing.T) { - tests := map[string]struct { - input runtime.Object - errorSuffix string // is a suffix as the API server sends the whole value back in the error - expectError bool - }{ - "No errors on valid certificaterequest with no usages set": { - input: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - Spec: cmapi.CertificateRequestSpec{ - Request: mustGenerateCSR(t, &cmapi.Certificate{ - Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - }, - }), - Usages: []cmapi.KeyUsage{}, - IssuerRef: cmmeta.ObjectReference{Name: "test"}, - }, - }, - expectError: false, - }, - "No errors on valid certificaterequest with special usages set": { - input: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - Spec: cmapi.CertificateRequestSpec{ - Request: mustGenerateCSR(t, &cmapi.Certificate{ - Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, - }, - }), - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, - IssuerRef: cmmeta.ObjectReference{Name: "test"}, - }, - }, - expectError: false, - }, - "No errors on valid certificaterequest with special usages set only in CSR": { - input: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - Spec: cmapi.CertificateRequestSpec{ - Request: mustGenerateCSR(t, &cmapi.Certificate{ - Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, - }, - }), - IssuerRef: cmmeta.ObjectReference{Name: "test"}, - }, - }, - expectError: false, - }, - "No errors on valid certificaterequest with special usages only set in spec": { - input: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - Spec: cmapi.CertificateRequestSpec{ - Request: mustGenerateCSR(t, &cmapi.Certificate{ - Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{}, - EncodeUsagesInRequest: ptr.To(false), - }, - }), - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, - IssuerRef: cmmeta.ObjectReference{Name: "test"}, - }, - }, - expectError: false, - }, - "Errors on certificaterequest with mismatch of usages": { - input: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - Spec: cmapi.CertificateRequestSpec{ - Request: mustGenerateCSR(t, &cmapi.Certificate{ - Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, - }, - }), - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageCodeSigning}, - IssuerRef: cmmeta.ObjectReference{Name: "test"}, - }, - }, - expectError: true, - errorSuffix: "encoded CSR error: the ExtKeyUsages [ 'client auth' ] do not match the expected ExtKeyUsages [ 'code signing' ]", - }, - "Shouldn't error when setting user info, since this will be overwritten by the mutating webhook": { - input: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - Spec: cmapi.CertificateRequestSpec{ - Request: mustGenerateCSR(t, &cmapi.Certificate{ - Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{}, - EncodeUsagesInRequest: ptr.To(false), - }, - }), - Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, - IssuerRef: cmmeta.ObjectReference{Name: "test"}, - Username: "user-1", - Groups: []string{"group-1", "group-2"}, - }, - }, - expectError: false, - }, - } - for name, test := range tests { - t.Run(name, func(t *testing.T) { - cert := test.input.(*cmapi.CertificateRequest) - cert.SetGroupVersionKind(certGVK) - - ctx, cancel := context.WithTimeout(context.Background(), time.Second*40) - defer cancel() - - utilfeature.DefaultMutableFeatureGate.Set("DisallowInsecureCSRUsageDefinition=false") - config, stop := framework.RunControlPlane(t, ctx) defer stop()