diff --git a/internal/apis/certmanager/validation/certificaterequest.go b/internal/apis/certmanager/validation/certificaterequest.go index 856f76a3f61..87f34517207 100644 --- a/internal/apis/certmanager/validation/certificaterequest.go +++ b/internal/apis/certmanager/validation/certificaterequest.go @@ -30,9 +30,11 @@ 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" "github.com/cert-manager/cert-manager/pkg/util" + utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" "github.com/cert-manager/cert-manager/pkg/util/pki" ) @@ -95,16 +97,26 @@ func ValidateCertificateRequestSpec(crSpec *cmapi.CertificateRequestSpec, fldPat if err != nil { el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("failed to decode csr: %s", err))) } else { - // only compare usages if set on CR and in the CSR - if len(crSpec.Usages) > 0 && len(csr.Extensions) > 0 && !reflect.DeepEqual(crSpec.Usages, defaultInternalKeyUsages) { + // in case DontAllowInsecureCSRUsageDefinition is disabled: only compare usages if set on the CR + // otherwise: always compare usages + if utilfeature.DefaultMutableFeatureGate.Enabled(feature.DontAllowInsecureCSRUsageDefinition) || len(crSpec.Usages) > 0 { + // set capacity to length to obtain a "copy-on-append" slice + crUsages := crSpec.Usages[:len(crSpec.Usages):len(crSpec.Usages)] + if len(crUsages) == 0 { + crUsages = defaultInternalKeyUsages[:len(defaultInternalKeyUsages):len(defaultInternalKeyUsages)] + } + if crSpec.IsCA { - crSpec.Usages = ensureCertSignIsSet(crSpec.Usages) + crUsages = ensureCertSignIsSet(crUsages) } + csrUsages, err := getCSRKeyUsage(crSpec, fldPath, csr, el) if len(err) > 0 { el = append(el, err...) - } else if len(csrUsages) > 0 && !isUsageEqual(csrUsages, crSpec.Usages) && !isUsageEqual(csrUsages, defaultInternalKeyUsages) { - el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("csr key usages do not match specified usages, these should match if both are set: %s", pretty.Diff(patchDuplicateKeyUsage(csrUsages), patchDuplicateKeyUsage(crSpec.Usages))))) + } + + if len(csrUsages) > 0 && !isUsageEqual(csrUsages, crUsages) { + el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("csr key usages do not match specified usages, these should match if both are set: %s", pretty.Diff(patchDuplicateKeyUsage(csrUsages), patchDuplicateKeyUsage(crUsages))))) } } } diff --git a/internal/webhook/feature/features.go b/internal/webhook/feature/features.go index d9a44dcffd3..30aaaf6fd28 100644 --- a/internal/webhook/feature/features.go +++ b/internal/webhook/feature/features.go @@ -47,6 +47,13 @@ const ( // This feature gate must be used together with LiteralCertificateSubject webhook feature gate. // See https://github.com/cert-manager/cert-manager/issues/3203 and https://github.com/cert-manager/cert-manager/issues/4424 for context. LiteralCertificateSubject featuregate.Feature = "LiteralCertificateSubject" + + // Owner (responsible for graduating feature through to GA): @inteon + // GA: v1.13 + // DontAllowInsecureCSRUsageDefinition will prevent the webhook from allowing + // CertificateRequest's usages to be only defined in the CSR, while leaving + // the usages field empty. + DontAllowInsecureCSRUsageDefinition featuregate.Feature = "DontAllowInsecureCSRUsageDefinition" ) func init() { @@ -61,6 +68,8 @@ func init() { // // Where utilfeature is github.com/cert-manager/cert-manager/pkg/util/feature. var webhookFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ + DontAllowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.GA}, + AdditionalCertificateOutputFormats: {Default: false, PreRelease: featuregate.Alpha}, LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha}, } diff --git a/pkg/api/util/kube.go b/pkg/api/util/kube.go index abab8f23450..1956c337b63 100644 --- a/pkg/api/util/kube.go +++ b/pkg/api/util/kube.go @@ -91,6 +91,10 @@ func KubeExtKeyUsageStrings(usage []x509.ExtKeyUsage) []certificatesv1.KeyUsage // kubeKeyUsageString returns the cmapi.KeyUsage and "unknown" if not found func kubeKeyUsageString(usage x509.KeyUsage) certificatesv1.KeyUsage { + if usage == x509.KeyUsageDigitalSignature { + return certificatesv1.UsageDigitalSignature // we have two keys that map to KeyUsageDigitalSignature in our map, we should be consistent when parsing + } + for k, v := range keyUsagesKube { if usage == v { return k @@ -102,6 +106,10 @@ func kubeKeyUsageString(usage x509.KeyUsage) certificatesv1.KeyUsage { // kubeExtKeyUsageString returns the cmapi.ExtKeyUsage and "unknown" if not found func kubeExtKeyUsageString(usage x509.ExtKeyUsage) certificatesv1.KeyUsage { + if usage == x509.ExtKeyUsageEmailProtection { + return certificatesv1.UsageEmailProtection // we have two keys that map to ExtKeyUsageEmailProtection in our map, we should be consistent when parsing + } + for k, v := range extKeyUsagesKube { if usage == v { return k diff --git a/pkg/api/util/usages.go b/pkg/api/util/usages.go index 82e13a060b8..6dc8156a159 100644 --- a/pkg/api/util/usages.go +++ b/pkg/api/util/usages.go @@ -90,10 +90,11 @@ func ExtKeyUsageStrings(usage []x509.ExtKeyUsage) []cmapi.KeyUsage { // keyUsageString returns the cmapi.KeyUsage and "unknown" if not found func keyUsageString(usage x509.KeyUsage) cmapi.KeyUsage { + if usage == x509.KeyUsageDigitalSignature { + return cmapi.UsageDigitalSignature // we have two keys that map to KeyUsageDigitalSignature in our map, we should be consistent when parsing + } + for k, v := range keyUsages { - if usage == x509.KeyUsageDigitalSignature { - return cmapi.UsageDigitalSignature // we have KeyUsageDigitalSignature twice in our array, we should be consistent when parsing - } if usage == v { return k } @@ -104,6 +105,10 @@ func keyUsageString(usage x509.KeyUsage) cmapi.KeyUsage { // extKeyUsageString returns the cmapi.ExtKeyUsage and "unknown" if not found func extKeyUsageString(usage x509.ExtKeyUsage) cmapi.KeyUsage { + if usage == x509.ExtKeyUsageEmailProtection { + return cmapi.UsageEmailProtection // we have two keys that map to ExtKeyUsageEmailProtection in our map, we should be consistent when parsing + } + for k, v := range extKeyUsages { if usage == v { return k diff --git a/test/integration/validation/certificaterequest_test.go b/test/integration/validation/certificaterequest_test.go index 56c56dfcbec..27bc7d97508 100644 --- a/test/integration/validation/certificaterequest_test.go +++ b/test/integration/validation/certificaterequest_test.go @@ -26,12 +26,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/cert-manager/cert-manager/integration-tests/framework" "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" ) @@ -100,7 +102,8 @@ func TestValidationCertificateRequests(t *testing.T) { IssuerRef: cmmeta.ObjectReference{Name: "test"}, }, }, - expectError: false, + expectError: true, + errorSuffix: "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[3] != []certmanager.KeyUsage[2]]", }, "No errors on valid certificaterequest with special usages only set in spec": { input: &cmapi.CertificateRequest{ @@ -111,8 +114,9 @@ func TestValidationCertificateRequests(t *testing.T) { Spec: cmapi.CertificateRequestSpec{ Request: mustGenerateCSR(t, &cmapi.Certificate{ Spec: cmapi.CertificateSpec{ - DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{}, + DNSNames: []string{"example.com"}, + Usages: []cmapi.KeyUsage{}, + EncodeUsagesInRequest: pointer.Bool(false), }, }), Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, @@ -142,6 +146,73 @@ func TestValidationCertificateRequests(t *testing.T) { errorSuffix: "csr key usages do not match specified usages, these should match if both are set: [[2]: \"client auth\" != \"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: pointer.Bool(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() + + // The default is true, but we set it here to make sure it was not changed by other tests + utilfeature.DefaultMutableFeatureGate.Set("DontAllowInsecureCSRUsageDefinition=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_DontAllowInsecureCSRUsageDefinition_false makes sure that the +// validation webhook keeps working as before when the DontAllowInsecureCSRUsageDefinition feature +// gate is disabled. +func TestValidationCertificateRequests_DontAllowInsecureCSRUsageDefinition_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", @@ -151,7 +222,103 @@ func TestValidationCertificateRequests(t *testing.T) { Request: mustGenerateCSR(t, &cmapi.Certificate{ Spec: cmapi.CertificateSpec{ DNSNames: []string{"example.com"}, - Usages: []cmapi.KeyUsage{}, + }, + }), + 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: pointer.Bool(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: "csr key usages do not match specified usages, these should match if both are set: [[2]: \"client auth\" != \"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: pointer.Bool(false), }, }), Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth}, @@ -171,6 +338,8 @@ func TestValidationCertificateRequests(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*40) defer cancel() + utilfeature.DefaultMutableFeatureGate.Set("DontAllowInsecureCSRUsageDefinition=false") + config, stop := framework.RunControlPlane(t, ctx) defer stop()