diff --git a/cmd/controller/app/options/BUILD.bazel b/cmd/controller/app/options/BUILD.bazel index 439ebbf33..fa99b39d7 100644 --- a/cmd/controller/app/options/BUILD.bazel +++ b/cmd/controller/app/options/BUILD.bazel @@ -10,6 +10,7 @@ go_library( "//pkg/controller/acmechallenges:go_default_library", "//pkg/controller/acmeorders:go_default_library", "//pkg/controller/certificaterequests/acme:go_default_library", + "//pkg/controller/certificaterequests/approver:go_default_library", "//pkg/controller/certificaterequests/ca:go_default_library", "//pkg/controller/certificaterequests/selfsigned:go_default_library", "//pkg/controller/certificaterequests/vault:go_default_library", diff --git a/cmd/controller/app/options/options.go b/cmd/controller/app/options/options.go index dcf4fac4a..f83d3ae88 100644 --- a/cmd/controller/app/options/options.go +++ b/cmd/controller/app/options/options.go @@ -27,6 +27,7 @@ import ( challengescontroller "github.com/jetstack/cert-manager/pkg/controller/acmechallenges" orderscontroller "github.com/jetstack/cert-manager/pkg/controller/acmeorders" cracmecontroller "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/acme" + crapprovercontroller "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/approver" crcacontroller "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/ca" crselfsignedcontroller "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/selfsigned" crvaultcontroller "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/vault" @@ -144,6 +145,7 @@ var ( orderscontroller.ControllerName, challengescontroller.ControllerName, cracmecontroller.CRControllerName, + crapprovercontroller.ControllerName, crcacontroller.CRControllerName, crselfsignedcontroller.CRControllerName, crvaultcontroller.CRControllerName, diff --git a/deploy/crds/crd-certificaterequests.yaml b/deploy/crds/crd-certificaterequests.yaml index 9dd1f9371..d01182284 100644 --- a/deploy/crds/crd-certificaterequests.yaml +++ b/deploy/crds/crd-certificaterequests.yaml @@ -39,6 +39,12 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string + - jsonPath: .status.conditions[?(@.type=="Denied")].status + name: Denied + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -190,7 +196,7 @@ spec: - "False" - Unknown type: - description: Type of the condition, known values are (`Ready`, `InvalidRequest`). + description: Type of the condition, known values are (`Ready`, `InvalidRequest`, `Approved`, `Denied`). type: string failureTime: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. @@ -202,6 +208,12 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string + - jsonPath: .status.conditions[?(@.type=="Denied")].status + name: Denied + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -353,7 +365,7 @@ spec: - "False" - Unknown type: - description: Type of the condition, known values are (`Ready`, `InvalidRequest`). + description: Type of the condition, known values are (`Ready`, `InvalidRequest`, `Approved`, `Denied`). type: string failureTime: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. @@ -365,6 +377,12 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string + - jsonPath: .status.conditions[?(@.type=="Denied")].status + name: Denied + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -518,7 +536,7 @@ spec: - "False" - Unknown type: - description: Type of the condition, known values are (`Ready`, `InvalidRequest`). + description: Type of the condition, known values are (`Ready`, `InvalidRequest`, `Approved`, `Denied`). type: string failureTime: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. @@ -530,6 +548,12 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string + - jsonPath: .status.conditions[?(@.type=="Denied")].status + name: Denied + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -683,7 +707,7 @@ spec: - "False" - Unknown type: - description: Type of the condition, known values are (`Ready`, `InvalidRequest`). + description: Type of the condition, known values are (`Ready`, `InvalidRequest`, `Approved`, `Denied`). type: string failureTime: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. diff --git a/pkg/api/util/conditions.go b/pkg/api/util/conditions.go index 7ed1b07f2..986bcb874 100644 --- a/pkg/api/util/conditions.go +++ b/pkg/api/util/conditions.go @@ -315,3 +315,38 @@ func CertificateRequestHasInvalidRequest(cr *cmapi.CertificateRequest) bool { return false } + +// CertificateRequestIsApproved returns true if the CertificateRequest is +// approved via an Approved condition of status `True`, returns false +// otherwise. +func CertificateRequestIsApproved(cr *cmapi.CertificateRequest) bool { + if cr == nil { + return false + } + + for _, con := range cr.Status.Conditions { + if con.Type == cmapi.CertificateRequestConditionApproved && + con.Status == cmmeta.ConditionTrue { + return true + } + } + + return false +} + +// CertificateRequestIsDenied returns true if the CertificateRequest is denied +// via a Denied condition of status `True`, returns false otherwise. +func CertificateRequestIsDenied(cr *cmapi.CertificateRequest) bool { + if cr == nil { + return false + } + + for _, con := range cr.Status.Conditions { + if con.Type == cmapi.CertificateRequestConditionDenied && + con.Status == cmmeta.ConditionTrue { + return true + } + } + + return false +} diff --git a/pkg/apis/certmanager/v1/types_certificaterequest.go b/pkg/apis/certmanager/v1/types_certificaterequest.go index a86700ff7..215dbb288 100644 --- a/pkg/apis/certmanager/v1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1/types_certificaterequest.go @@ -153,7 +153,8 @@ type CertificateRequestStatus struct { // CertificateRequestCondition contains condition information for a CertificateRequest. type CertificateRequestCondition struct { - // Type of the condition, known values are (`Ready`, `InvalidRequest`). + // Type of the condition, known values are (`Ready`, `InvalidRequest`, + // `Approved`, `Denied`). Type CertificateRequestConditionType `json:"type"` // Status of the condition, one of (`True`, `False`, `Unknown`). @@ -189,4 +190,16 @@ const ( // parameters being invalid. Additional information about why the request // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" + + // CertificateRequestConditionApproved indicates that a certificate request + // is approved and ready for signing. Condition must never have a status of + // `False`, and cannot be modified once set. Cannot be set alongside + // `Denied`. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" + + // CertificateRequestConditionDenied indicates that a certificate request is + // denied, and must never be signed. Condition must never have a status of + // `False`, and cannot be modified once set. Cannot be set alongside + // `Approved`. + CertificateRequestConditionDenied CertificateRequestConditionType = "Denied" ) diff --git a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go index be4b38b7a..d38414ceb 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go @@ -150,7 +150,8 @@ type CertificateRequestStatus struct { // CertificateRequestCondition contains condition information for a CertificateRequest. type CertificateRequestCondition struct { - // Type of the condition, known values are (`Ready`, `InvalidRequest`). + // Type of the condition, known values are (`Ready`, + // `InvalidRequest`, `Approved`, `Denied`). Type CertificateRequestConditionType `json:"type"` // Status of the condition, one of (`True`, `False`, `Unknown`). @@ -186,4 +187,16 @@ const ( // parameters being invalid. Additional information about why the request // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" + + // CertificateRequestConditionApproved indicates that a certificate request + // is approved and ready for signing. Condition must never have a status of + // `False`, and cannot be modified once set. Cannot be set alongside + // `Denied`. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" + + // CertificateRequestConditionDenied indicates that a certificate request is + // denied, and must never be signed. Condition must never have a status of + // `False`, and cannot be modified once set. Cannot be set alongside + // `Approved`. + CertificateRequestConditionDenied CertificateRequestConditionType = "Denied" ) diff --git a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go index 46393cab3..3fa69e7f9 100644 --- a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go @@ -150,7 +150,8 @@ type CertificateRequestStatus struct { // CertificateRequestCondition contains condition information for a CertificateRequest. type CertificateRequestCondition struct { - // Type of the condition, known values are (`Ready`, `InvalidRequest`). + // Type of the condition, known values are (`Ready`, + // `InvalidRequest`, `Approved`, `Denied`). Type CertificateRequestConditionType `json:"type"` // Status of the condition, one of (`True`, `False`, `Unknown`). @@ -186,4 +187,14 @@ const ( // parameters being invalid. Additional information about why the request // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" + + // CertificateRequestConditionApproved indicates that a certificate request + // is approved and ready for signing. Condition must never have a status of + // `False`, and cannot be modified once set. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" + + // CertificateRequestConditionDenied indicates that a certificate request is + // denied, and must never be signed. Condition must never have a status of + // `False`, and cannot be modified once set. + CertificateRequestConditionDenied CertificateRequestConditionType = "Denied" ) diff --git a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go index 3af27a8a9..5b59b3341 100644 --- a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go @@ -151,7 +151,8 @@ type CertificateRequestStatus struct { // CertificateRequestCondition contains condition information for a CertificateRequest. type CertificateRequestCondition struct { - // Type of the condition, known values are (`Ready`, `InvalidRequest`). + // Type of the condition, known values are (`Ready`, + // `InvalidRequest`, `Approved`, `Denied`). Type CertificateRequestConditionType `json:"type"` // Status of the condition, one of (`True`, `False`, `Unknown`). @@ -187,4 +188,16 @@ const ( // parameters being invalid. Additional information about why the request // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" + + // CertificateRequestConditionApproved indicates that a certificate request + // is approved and ready for signing. Condition must never have a status of + // `False`, and cannot be modified once set. Cannot be set alongside + // `Denied`. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" + + // CertificateRequestConditionDenied indicates that a certificate request is + // denied, and must never be signed. Condition must never have a status of + // `False`, and cannot be modified once set. Cannot be set alongside + // `Approved`. + CertificateRequestConditionDenied CertificateRequestConditionType = "Denied" ) diff --git a/pkg/controller/certificaterequests/BUILD.bazel b/pkg/controller/certificaterequests/BUILD.bazel index 53ec20aa4..b0ca735ff 100644 --- a/pkg/controller/certificaterequests/BUILD.bazel +++ b/pkg/controller/certificaterequests/BUILD.bazel @@ -68,6 +68,7 @@ filegroup( srcs = [ ":package-srcs", "//pkg/controller/certificaterequests/acme:all-srcs", + "//pkg/controller/certificaterequests/approver:all-srcs", "//pkg/controller/certificaterequests/ca:all-srcs", "//pkg/controller/certificaterequests/fake:all-srcs", "//pkg/controller/certificaterequests/selfsigned:all-srcs", diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 1638f223b..c27bb2e8d 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -102,6 +102,7 @@ func generateCSRWithIPs(t *testing.T, secretKey crypto.Signer, commonName string } func TestSign(t *testing.T) { + metaFixedClockStart := metav1.NewTime(fixedClockStart) baseIssuer := gen.Issuer("test-issuer", gen.SetIssuerACME(cmacme.ACMEIssuer{}), gen.AddIssuerCondition(cmapi.IssuerCondition{ @@ -118,7 +119,7 @@ func TestSign(t *testing.T) { csrPEM := generateCSR(t, sk, "example.com", "example.com", "foo.com") csrPEMExampleNotPresent := generateCSR(t, sk, "example.com", "foo.com") - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestCSR(csrPEM), gen.SetCertificateRequestIsCA(false), gen.SetCertificateRequestDuration(&metav1.Duration{Duration: time.Hour * 24 * 60}), @@ -128,6 +129,24 @@ func TestSign(t *testing.T) { Kind: "Issuer", }), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "Foo", + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) csr, err := pki.DecodeX509CertificateRequestBytes(csrPEM) if err != nil { @@ -175,8 +194,51 @@ func TestSign(t *testing.T) { t.Fatalf("failed to build order during testing: %s", err) } - metaFixedClockStart := metav1.NewTime(fixedClockStart) tests := map[string]testT{ + "a CertificateRequest without an approved condition should do nothing": { + certificateRequest: baseCRNotApproved.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRNotApproved.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, + "a CertificateRequest with a denied condition should do nothing": { + certificateRequest: baseCRDenied.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRDenied.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, + "a badly formed CSR should report failure": { + certificateRequest: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestCSR([]byte("a bad csr")), + ), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()}, + ExpectedEvents: []string{ + "Warning RequestParsingError Failed to decode CSR in spec.request: error decoding certificate request PEM block", + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificaterequests"), + "status", + gen.DefaultTestNamespace, + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestCSR([]byte("a bad csr")), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonFailed, + Message: "Failed to decode CSR in spec.request: error decoding certificate request PEM block", + LastTransitionTime: &metaFixedClockStart, + }), + gen.SetCertificateRequestFailureTime(metaFixedClockStart), + ), + )), + }, + }, + }, "if the common name is not present in the DNS names then should hard fail": { certificateRequest: gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestCSR(csrPEMExampleNotPresent), diff --git a/pkg/controller/certificaterequests/approver/BUILD.bazel b/pkg/controller/certificaterequests/approver/BUILD.bazel new file mode 100644 index 000000000..e0dd4f641 --- /dev/null +++ b/pkg/controller/certificaterequests/approver/BUILD.bazel @@ -0,0 +1,56 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "approver.go", + "sync.go", + ], + importpath = "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/approver", + visibility = ["//visibility:public"], + deps = [ + "//pkg/api/util:go_default_library", + "//pkg/apis/certmanager/v1:go_default_library", + "//pkg/apis/meta/v1:go_default_library", + "//pkg/client/clientset/versioned:go_default_library", + "//pkg/client/listers/certmanager/v1:go_default_library", + "//pkg/controller:go_default_library", + "//pkg/logs:go_default_library", + "@com_github_go_logr_logr//:go_default_library", + "@io_k8s_api//core/v1:go_default_library", + "@io_k8s_apimachinery//pkg/api/errors:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_client_go//tools/cache:go_default_library", + "@io_k8s_client_go//tools/record:go_default_library", + "@io_k8s_client_go//util/workqueue:go_default_library", + ], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) + +go_test( + name = "go_default_test", + srcs = ["approver_test.go"], + embed = [":go_default_library"], + deps = [ + "//pkg/apis/certmanager/v1:go_default_library", + "//pkg/apis/meta/v1:go_default_library", + "//pkg/controller:go_default_library", + "//pkg/controller/test:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_client_go//testing:go_default_library", + "@io_k8s_utils//clock/testing:go_default_library", + ], +) diff --git a/pkg/controller/certificaterequests/approver/approver.go b/pkg/controller/certificaterequests/approver/approver.go new file mode 100644 index 000000000..7569583e1 --- /dev/null +++ b/pkg/controller/certificaterequests/approver/approver.go @@ -0,0 +1,106 @@ +/* +Copyright 2021 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 approver + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/tools/cache" + "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/workqueue" + + cmclient "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" + cmlisters "github.com/jetstack/cert-manager/pkg/client/listers/certmanager/v1" + controllerpkg "github.com/jetstack/cert-manager/pkg/controller" + logf "github.com/jetstack/cert-manager/pkg/logs" +) + +const ( + ControllerName = "certificaterequests-approver" +) + +// Controller is a CertificateRequest controller which manages the "Approved" +// condition. In the absence of any automated policy engine, this controller +// will _always_ set the "Approved" condition to True. All CertificateRequest +// signing controllers should wait until the "Approved" condition is set to +// True before processing. +type Controller struct { + // logger to be used by this controller + log logr.Logger + + certificateRequestLister cmlisters.CertificateRequestLister + cmClient cmclient.Interface + + recorder record.EventRecorder + + queue workqueue.RateLimitingInterface +} + +func init() { + // create certificate request approver controller + controllerpkg.Register(ControllerName, func(ctx *controllerpkg.Context) (controllerpkg.Interface, error) { + return controllerpkg.NewBuilder(ctx, ControllerName). + For(new(Controller)).Complete() + }) +} + +// Register registers and constructs the controller using the provided context. +// It returns the workqueue to be used to enqueue items, a list of +// InformerSynced functions that must be synced, or an error. +func (c *Controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitingInterface, []cache.InformerSynced, error) { + c.log = logf.FromContext(ctx.RootContext, ControllerName) + c.queue = workqueue.NewNamedRateLimitingQueue(controllerpkg.DefaultItemBasedRateLimiter(), ControllerName) + + certificateRequestInformer := ctx.SharedInformerFactory.Certmanager().V1().CertificateRequests() + mustSync := append([]cache.InformerSynced{certificateRequestInformer.Informer().HasSynced}) + certificateRequestInformer.Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{Queue: c.queue}) + + c.certificateRequestLister = certificateRequestInformer.Lister() + c.cmClient = ctx.CMClient + c.recorder = ctx.Recorder + + c.log.V(logf.DebugLevel).Info("certificate request approver controller registered") + + return c.queue, mustSync, nil +} + +func (c *Controller) ProcessItem(ctx context.Context, key string) error { + log := logf.FromContext(ctx) + dbg := log.V(logf.DebugLevel) + + namespace, name, err := cache.SplitMetaNamespaceKey(key) + if err != nil { + log.Error(err, "invalid resource key") + return nil + } + + cr, err := c.certificateRequestLister.CertificateRequests(namespace).Get(name) + if apierrors.IsNotFound(err) { + dbg.Info(fmt.Sprintf("certificate request in work queue no longer exists: %s", err)) + return nil + } + + if err != nil { + return err + } + + ctx = logf.NewContext(ctx, logf.WithResource(log, cr)) + return c.Sync(ctx, cr) +} diff --git a/pkg/controller/certificaterequests/approver/approver_test.go b/pkg/controller/certificaterequests/approver/approver_test.go new file mode 100644 index 000000000..ba51c4898 --- /dev/null +++ b/pkg/controller/certificaterequests/approver/approver_test.go @@ -0,0 +1,240 @@ +/* +Copyright 2021 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 approver + +import ( + "context" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + coretesting "k8s.io/client-go/testing" + fakeclock "k8s.io/utils/clock/testing" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" + controllerpkg "github.com/jetstack/cert-manager/pkg/controller" + testpkg "github.com/jetstack/cert-manager/pkg/controller/test" +) + +func TestProcessItem(t *testing.T) { + // now time is the current time at the start of the test (the clock is fixed) + now := time.Now() + metaNow := metav1.NewTime(now) + tests := map[string]struct { + // key that should be passed to ProcessItem. + // if not set, the 'namespace/name' of the 'CertificateRequest' field will be used. + // if neither is set, the key will be "" + key string + + // CertificateRequest to be synced for the test. + // if not set, the 'key' will be passed to ProcessItem instead. + request *cmapi.CertificateRequest + + // expectedEvent, if set, is an 'event string' that is expected to be fired. + expectedEvent string + + // expectedConditions is the expected set of conditions on the + // CertificateRequest resource if an Update is made. + // If nil, no update is expected. + // If empty, an update to the empty set/nil is expected. + expectedConditions []cmapi.CertificateRequestCondition + + // err is the expected error text returned by the controller, if any. + err string + }{ + "do nothing if an empty 'key' is used": {}, + "do nothing if an invalid 'key' is used": { + key: "abc/def/ghi", + }, + "do nothing if a key references a Certificate that does not exist": { + key: "namespace/name", + }, + "do nothing if CertificateRequest already has 'Approved' True condition": { + request: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}, + Status: cmapi.CertificateRequestStatus{ + Conditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + }, + }, + }, + }, + }, + "do nothing if CertificateRequest already has 'Denied' True condition": { + request: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}, + Status: cmapi.CertificateRequestStatus{ + Conditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + }, + }, + }, + }, + }, + "do nothing if CertificateRequest already has 'Ready' Failed condition": { + request: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}, + Status: cmapi.CertificateRequestStatus{ + Conditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonFailed, + }, + }, + }, + }, + }, + "do nothing if CertificateRequest already has 'Ready' Issued condition": { + request: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}, + Status: cmapi.CertificateRequestStatus{ + Conditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonIssued, + }, + }, + }, + }, + }, + "approve CertificateRequest if no condition": { + request: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}, + Status: cmapi.CertificateRequestStatus{ + Conditions: []cmapi.CertificateRequestCondition{}, + }, + }, + expectedConditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: ApprovedMessage, + LastTransitionTime: &metaNow, + }, + }, + expectedEvent: "Normal cert-manager.io Certificate request has been approved by cert-manager.io", + }, + "approve CertificateRequest has 'Ready' Pending condition": { + request: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}, + Status: cmapi.CertificateRequestStatus{ + Conditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonPending, + }, + }, + }, + }, + expectedConditions: []cmapi.CertificateRequestCondition{ + { + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonPending, + }, + { + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: ApprovedMessage, + LastTransitionTime: &metaNow, + }, + }, + expectedEvent: "Normal cert-manager.io Certificate request has been approved by cert-manager.io", + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // Create and initialise a new unit test builder + builder := &testpkg.Builder{ + T: t, + Clock: fakeclock.NewFakeClock(now), + } + if test.request != nil { + builder.CertManagerObjects = append(builder.CertManagerObjects, test.request) + } + builder.Init() + + c := new(Controller) + _, _, err := c.Register(builder.Context) + if err != nil { + t.Fatal(err) + } + if test.expectedConditions != nil { + if test.request == nil { + t.Fatal("cannot expect an Update operation if test.request is nil") + } + expectedRequest := test.request.DeepCopy() + expectedRequest.Status.Conditions = test.expectedConditions + builder.ExpectedActions = append(builder.ExpectedActions, + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificaterequests"), + "status", + test.request.Namespace, + expectedRequest, + )), + ) + } + if test.expectedEvent != "" { + builder.ExpectedEvents = []string{test.expectedEvent} + } + // Start the informers and begin processing updates + builder.Start() + defer builder.Stop() + + key := test.key + if key == "" && test.request != nil { + key, err = controllerpkg.KeyFunc(test.request) + if err != nil { + t.Fatal(err) + } + } + + // Call ProcessItem + err = c.ProcessItem(context.Background(), key) + switch { + case err != nil: + if test.err != err.Error() { + t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err) + } + default: + if test.err != "" { + t.Errorf("got no error but expected: %s", test.err) + } + } + + if err := builder.AllEventsCalled(); err != nil { + builder.T.Error(err) + } + if err := builder.AllActionsExecuted(); err != nil { + builder.T.Error(err) + } + if err := builder.AllReactorsCalled(); err != nil { + builder.T.Error(err) + } + }) + } +} diff --git a/pkg/controller/certificaterequests/approver/sync.go b/pkg/controller/certificaterequests/approver/sync.go new file mode 100644 index 000000000..7824793fb --- /dev/null +++ b/pkg/controller/certificaterequests/approver/sync.go @@ -0,0 +1,73 @@ +/* +Copyright 2021 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 approver + +import ( + "context" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + apiutil "github.com/jetstack/cert-manager/pkg/api/util" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" + logf "github.com/jetstack/cert-manager/pkg/logs" +) + +const ( + ApprovedMessage = "Certificate request has been approved by cert-manager.io" +) + +// Sync will set the "Approved" condition to True on synced +// CertificateRequests. If the "Denied", "Approved" or "Ready" condition +// already exists, exit early. +func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (err error) { + log := logf.FromContext(ctx, "approver") + + switch { + case + // If the CertificateRequest has already been approved, exit early. + apiutil.CertificateRequestIsApproved(cr), + + // If the CertificateRequest has already been denied, exit early. + apiutil.CertificateRequestIsDenied(cr), + + // If the CertificateRequest is "Issued" or "Failed", exit early. + apiutil.CertificateRequestReadyReason(cr) == cmapi.CertificateRequestReasonFailed, + apiutil.CertificateRequestReadyReason(cr) == cmapi.CertificateRequestReasonIssued: + return nil + } + + // Update the CertificateRequest approved condition to true. + apiutil.SetCertificateRequestCondition(cr, + cmapi.CertificateRequestConditionApproved, + cmmeta.ConditionTrue, + "cert-manager.io", + ApprovedMessage, + ) + + // Update CertificateRequest with + _, err = c.cmClient.CertmanagerV1().CertificateRequests(cr.Namespace).UpdateStatus(ctx, cr, metav1.UpdateOptions{}) + if err != nil { + return err + } + c.recorder.Event(cr, corev1.EventTypeNormal, "cert-manager.io", ApprovedMessage) + + log.V(logf.DebugLevel).Info("approved certificate request") + + return nil +} diff --git a/pkg/controller/certificaterequests/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index a6a6c17af..d3fd4f04f 100644 --- a/pkg/controller/certificaterequests/ca/ca_test.go +++ b/pkg/controller/certificaterequests/ca/ca_test.go @@ -105,6 +105,8 @@ func generateSelfSignedCertFromCR(t *testing.T, cr *cmapi.CertificateRequest, ke } func TestSign(t *testing.T) { + metaFixedClockStart := metav1.NewTime(fixedClockStart) + baseIssuer := gen.Issuer("test-issuer", gen.SetIssuerCA(cmapi.CAIssuer{SecretName: "root-ca-secret"}), gen.AddIssuerCondition(cmapi.IssuerCondition{ @@ -123,7 +125,7 @@ func TestSign(t *testing.T) { skRSAPEM := pki.EncodePKCS1PrivateKey(skRSA) rsaCSR := generateCSR(t, skRSA) - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestIsCA(true), gen.SetCertificateRequestCSR(rsaCSR), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ @@ -133,6 +135,24 @@ func TestSign(t *testing.T) { }), gen.SetCertificateRequestDuration(&metav1.Duration{Duration: time.Hour * 24 * 60}), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "Foo", + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) // generate a self signed root ca valid for 60d _, rsaPEMCert := generateSelfSignedCertFromCR(t, baseCR, skRSA, time.Hour*24*60) @@ -161,8 +181,21 @@ func TestSign(t *testing.T) { t.FailNow() } - metaFixedClockStart := metav1.NewTime(fixedClockStart) tests := map[string]testT{ + "a CertificateRequest without an approved condition should do nothing": { + certificateRequest: baseCRNotApproved.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRNotApproved.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, + "a CertificateRequest with a denied condition should do nothing": { + certificateRequest: baseCRDenied.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRDenied.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, "a missing CA key pair should set the condition to pending and wait for a re-sync": { certificateRequest: baseCR.DeepCopy(), builder: &testpkg.Builder{ diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go index edaf419eb..2eeea36aa 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go @@ -129,7 +129,7 @@ func TestSign(t *testing.T) { } csrECPEM := generateCSR(t, skEC, x509.ECDSAWithSHA256) - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestAnnotations( map[string]string{ cmapi.CertificateRequestPrivateKeyAnnotationKey: rsaKeySecret.Name, @@ -142,6 +142,24 @@ func TestSign(t *testing.T) { Kind: "Issuer", }), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "Foo", + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) ecCR := gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestCSR(csrECPEM), ) @@ -169,6 +187,20 @@ func TestSign(t *testing.T) { } tests := map[string]testT{ + "a CertificateRequest without an approved condition should do nothing": { + certificateRequest: baseCRNotApproved.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRNotApproved.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, + "a CertificateRequest with a denied condition should do nothing": { + certificateRequest: baseCRDenied.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRDenied.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, "a CertificateRequest with no cert-manager.io/selfsigned-private-key annotation should fail": { certificateRequest: gen.CertificateRequestFrom(baseCR, // no annotation diff --git a/pkg/controller/certificaterequests/sync.go b/pkg/controller/certificaterequests/sync.go index 4c55f6950..88f9b4f72 100644 --- a/pkg/controller/certificaterequests/sync.go +++ b/pkg/controller/certificaterequests/sync.go @@ -29,17 +29,17 @@ import ( apiutil "github.com/jetstack/cert-manager/pkg/api/util" "github.com/jetstack/cert-manager/pkg/apis/certmanager" - v1 "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" logf "github.com/jetstack/cert-manager/pkg/logs" "github.com/jetstack/cert-manager/pkg/util/pki" ) var ( - certificateRequestGvk = v1.SchemeGroupVersion.WithKind(v1.CertificateRequestKind) + certificateRequestGvk = cmapi.SchemeGroupVersion.WithKind(cmapi.CertificateRequestKind) ) -func (c *Controller) Sync(ctx context.Context, cr *v1.CertificateRequest) (err error) { +func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (err error) { log := logf.FromContext(ctx) dbg := log.V(logf.DebugLevel) @@ -48,12 +48,18 @@ func (c *Controller) Sync(ctx context.Context, cr *v1.CertificateRequest) (err e return nil } + // If CertificateRequest has not been approved or is denied, exit early. + if !apiutil.CertificateRequestIsApproved(cr) || apiutil.CertificateRequestIsDenied(cr) { + dbg.Info("certificate request has not been approved") + return nil + } + switch apiutil.CertificateRequestReadyReason(cr) { - case v1.CertificateRequestReasonFailed: + case cmapi.CertificateRequestReasonFailed: dbg.Info("certificate request Ready condition failed so skipping processing") return - case v1.CertificateRequestReasonIssued: + case cmapi.CertificateRequestReasonIssued: dbg.Info("certificate request Ready condition true so skipping processing") return } @@ -99,8 +105,8 @@ func (c *Controller) Sync(ctx context.Context, cr *v1.CertificateRequest) (err e } // check ready condition - if !apiutil.IssuerHasCondition(issuerObj, v1.IssuerCondition{ - Type: v1.IssuerConditionReady, + if !apiutil.IssuerHasCondition(issuerObj, cmapi.IssuerCondition{ + Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, }) { c.reporter.Pending(crCopy, nil, "IssuerNotReady", @@ -148,7 +154,7 @@ func (c *Controller) Sync(ctx context.Context, cr *v1.CertificateRequest) (err e return nil } -func (c *Controller) updateCertificateRequestStatusAndAnnotations(ctx context.Context, old, new *v1.CertificateRequest) (*v1.CertificateRequest, error) { +func (c *Controller) updateCertificateRequestStatusAndAnnotations(ctx context.Context, old, new *cmapi.CertificateRequest) (*cmapi.CertificateRequest, error) { log := logf.FromContext(ctx, "updateStatus") // if annotations changed we have to call .Update() and not .UpdateStatus() diff --git a/pkg/controller/certificaterequests/sync_test.go b/pkg/controller/certificaterequests/sync_test.go index bf9830681..45db57173 100644 --- a/pkg/controller/certificaterequests/sync_test.go +++ b/pkg/controller/certificaterequests/sync_test.go @@ -122,7 +122,7 @@ func TestSync(t *testing.T) { }), ) - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestIsCA(false), gen.SetCertificateRequestCSR(csrRSAPEM), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ @@ -131,6 +131,16 @@ func TestSync(t *testing.T) { }), ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &nowMetaTime, + }), + ) + certRSAPEM := generateSelfSignedCert(t, baseCR, skRSA, fixedClockStart, fixedClockStart.Add(time.Hour*12)) certRSAPEMExpired := generateSelfSignedCert(t, baseCR, skRSA, fixedClockStart.Add(-time.Hour*13), fixedClockStart.Add(-time.Hour*12)) @@ -150,6 +160,46 @@ func TestSync(t *testing.T) { ExpectedActions: []testpkg.Action{}, }, }, + "should return nil (no action) if certificate request is not approved": { + certificateRequest: gen.CertificateRequestFrom(baseCRNotApproved), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{baseIssuer, baseCR}, + ExpectedEvents: []string{}, + ExpectedActions: []testpkg.Action{}, + }, + }, + "should return nil (no action) if certificate request is denied": { + certificateRequest: gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "Foo", + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &nowMetaTime, + }), + ), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{baseIssuer, baseCR}, + ExpectedEvents: []string{}, + ExpectedActions: []testpkg.Action{}, + }, + }, + "should return nil (no action) if certificate request approved is set to false": { + certificateRequest: gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionFalse, + Reason: "cert-manager.io", + Message: "Certificate request has not been approved", + LastTransitionTime: &nowMetaTime, + }), + ), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{baseIssuer, baseCR}, + ExpectedEvents: []string{}, + ExpectedActions: []testpkg.Action{}, + }, + }, "should return nil (no action) if certificate request is ready and reason Issued": { certificateRequest: gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index ee857b851..520fdfb38 100644 --- a/pkg/controller/certificaterequests/vault/vault_test.go +++ b/pkg/controller/certificaterequests/vault/vault_test.go @@ -113,7 +113,7 @@ func TestSign(t *testing.T) { csrPEM := generateCSR(t, rsaSK) - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestIsCA(true), gen.SetCertificateRequestCSR(csrPEM), gen.SetCertificateRequestDuration(&metav1.Duration{Duration: time.Hour * 24 * 60}), @@ -123,6 +123,24 @@ func TestSign(t *testing.T) { Kind: baseIssuer.Kind, }), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "Foo", + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) rsaPEMCert, err := generateSelfSignedCertFromCR(baseCR, rsaSK, time.Hour*24*60) if err != nil { @@ -151,6 +169,20 @@ func TestSign(t *testing.T) { } tests := map[string]testT{ + "a CertificateRequest without an approved condition should do nothing": { + certificateRequest: baseCRNotApproved.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRNotApproved.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, + "a CertificateRequest with a denied condition should do nothing": { + certificateRequest: baseCRDenied.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRDenied.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, "no token, app role secret or kubernetes auth reference should report pending": { certificateRequest: baseCR.DeepCopy(), builder: &testpkg.Builder{ diff --git a/pkg/controller/certificaterequests/venafi/venafi_test.go b/pkg/controller/certificaterequests/venafi/venafi_test.go index 67a015ae4..c94d52759 100644 --- a/pkg/controller/certificaterequests/venafi/venafi_test.go +++ b/pkg/controller/certificaterequests/venafi/venafi_test.go @@ -78,6 +78,7 @@ func generateCSR(t *testing.T, secretKey crypto.Signer, alg x509.SignatureAlgori } func TestSign(t *testing.T) { + metaFixedClockStart := metav1.NewTime(fixedClockStart) rsaSK, err := pki.GenerateRSAPrivateKey(2048) if err != nil { t.Error(err) @@ -135,9 +136,27 @@ func TestSign(t *testing.T) { }), ) - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestCSR(csrPEM), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "Foo", + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: "cert-manager.io", + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) tppCR := gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ @@ -226,8 +245,21 @@ func TestSign(t *testing.T) { }, } - metaFixedClockStart := metav1.NewTime(fixedClockStart) tests := map[string]testT{ + "a CertificateRequest without an approved condition should do nothing": { + certificateRequest: baseCRNotApproved.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRNotApproved.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, + "a CertificateRequest with a denied condition should do nothing": { + certificateRequest: baseCRDenied.DeepCopy(), + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{}, + CertManagerObjects: []runtime.Object{baseCRDenied.DeepCopy(), baseIssuer.DeepCopy()}, + }, + }, "tpp: if fail to build client based on missing secret then return nil and hard fail": { certificateRequest: tppCR.DeepCopy(), builder: &controllertest.Builder{ diff --git a/pkg/internal/apis/certmanager/types_certificaterequest.go b/pkg/internal/apis/certmanager/types_certificaterequest.go index a6c3b9bfc..7de08f7cb 100644 --- a/pkg/internal/apis/certmanager/types_certificaterequest.go +++ b/pkg/internal/apis/certmanager/types_certificaterequest.go @@ -136,7 +136,8 @@ type CertificateRequestStatus struct { // CertificateRequestCondition contains condition information for a CertificateRequest. type CertificateRequestCondition struct { - // Type of the condition, known values are (`Ready`, `InvalidRequest`). + // Type of the condition, known values are (`Ready`, + // `InvalidRequest`, `Approved`, `Denied`). Type CertificateRequestConditionType // Status of the condition, one of (`True`, `False`, `Unknown`). @@ -169,4 +170,14 @@ const ( // parameters being invalid. Additional information about why the request // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" + + // CertificateRequestConditionApproved indicates that a certificate request + // is approved and ready for signing. Condition must never have a status of + // `False`, and cannot be modified once set. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" + + // CertificateRequestConditionDenied indicates that a certificate request is + // denied, and must never be signed. Condition must never have a status of + // `False`, and cannot be modified once set. + CertificateRequestConditionDenied CertificateRequestConditionType = "Denied" ) diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index 1c7b33b08..2a55400e8 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest.go @@ -31,6 +31,7 @@ import ( "github.com/jetstack/cert-manager/pkg/apis/acme" "github.com/jetstack/cert-manager/pkg/apis/certmanager" cmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" + cmmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -40,6 +41,9 @@ var defaultInternalKeyUsages = []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cma func ValidateCertificateRequest(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { cr := obj.(*cmapi.CertificateRequest) allErrs := ValidateCertificateRequestSpec(&cr.Spec, field.NewPath("spec"), true) + allErrs = append(allErrs, + ValidateCertificateRequestApprovalCondition(cr.Status.Conditions, field.NewPath("status", "conditions"))...) + return allErrs } @@ -55,6 +59,8 @@ func ValidateUpdateCertificateRequest(_ *admissionv1.AdmissionRequest, oldObj, n annotationField := field.NewPath("metadata", "annotations") el = append(el, validateCertificateRequestAnnotations(oldCR, newCR, annotationField)...) el = append(el, validateCertificateRequestAnnotations(newCR, oldCR, annotationField)...) + el = append(el, + ValidateUpdateCertificateRequestApprovalCondition(oldCR.Status.Conditions, newCR.Status.Conditions, field.NewPath("status", "conditions"))...) if !reflect.DeepEqual(oldCR.Spec, newCR.Spec) { el = append(el, field.Forbidden(field.NewPath("spec"), "cannot change spec after creation")) @@ -107,6 +113,80 @@ func ValidateCertificateRequestSpec(crSpec *cmapi.CertificateRequestSpec, fldPat return el } +// ValidateCertificateRequestApprovalCondition will ensure that only a single +// 'Approved' or 'Denied' condition may exist, and that they are set to True. +func ValidateCertificateRequestApprovalCondition(crConds []cmapi.CertificateRequestCondition, fldPath *field.Path) field.ErrorList { + var ( + approvedConditions []cmapi.CertificateRequestCondition + deniedConditions []cmapi.CertificateRequestCondition + el = field.ErrorList{} + ) + + for _, cond := range crConds { + if cond.Type == cmapi.CertificateRequestConditionApproved { + approvedConditions = append(approvedConditions, cond) + } + + if cond.Type == cmapi.CertificateRequestConditionDenied { + deniedConditions = append(deniedConditions, cond) + } + } + + for _, cond := range []struct { + condType cmapi.CertificateRequestConditionType + conditions []cmapi.CertificateRequestCondition + }{ + {cmapi.CertificateRequestConditionApproved, approvedConditions}, + {cmapi.CertificateRequestConditionDenied, deniedConditions}, + } { + switch len(cond.conditions) { + case 0: + break + case 1: + if condition := cond.conditions[0]; condition.Status != cmmeta.ConditionTrue { + el = append(el, field.Invalid(fldPath.Child(string(condition.Type)), condition.Status, + fmt.Sprintf("%q condition may only be set to True", cond.condType))) + } + default: + el = append(el, field.Forbidden(fldPath, fmt.Sprintf("multiple %q conditions present", cond.condType))) + } + } + + if len(deniedConditions) > 0 && len(approvedConditions) > 0 { + el = append(el, field.Forbidden(fldPath, "both 'Denied' and 'Approved' conditions cannot coexist")) + } + + return el +} + +// ValidateUpdateCertificateRequestApprovalCondition will ensure that the +// 'Approved' and 'Denied' conditions may not be changed once set, i.e. if they +// exist, they are not modified in the updated resource. Also runs the base +// approval validation on the updated CertificateRequest conditions. +func ValidateUpdateCertificateRequestApprovalCondition(oldCRConds, newCRConds []cmapi.CertificateRequestCondition, fldPath *field.Path) field.ErrorList { + var ( + el = field.ErrorList{} + oldCRDenied = getCertificateRequestCondition(oldCRConds, cmapi.CertificateRequestConditionDenied) + oldCRApproved = getCertificateRequestCondition(oldCRConds, cmapi.CertificateRequestConditionApproved) + ) + + // If the approval condition has been set, ensure it hasn't been modified. + if oldCRApproved != nil && !reflect.DeepEqual(oldCRApproved, + getCertificateRequestCondition(newCRConds, cmapi.CertificateRequestConditionApproved), + ) { + el = append(el, field.Forbidden(fldPath, "'Approved' condition may not be modified once set")) + } + + // If the denied condition has been set, ensure it hasn't been modified. + if oldCRDenied != nil && !reflect.DeepEqual(oldCRDenied, + getCertificateRequestCondition(newCRConds, cmapi.CertificateRequestConditionDenied), + ) { + el = append(el, field.Forbidden(fldPath, "'Denied' condition may not be modified once set")) + } + + return append(el, ValidateCertificateRequestApprovalCondition(newCRConds, fldPath)...) +} + func getCSRKeyUsage(crSpec *cmapi.CertificateRequestSpec, fldPath *field.Path, csr *x509.CertificateRequest, el field.ErrorList) ([]cmapi.KeyUsage, field.ErrorList) { var ekus []x509.ExtKeyUsage var ku x509.KeyUsage @@ -200,3 +280,12 @@ func ensureCertSignIsSet(list []cmapi.KeyUsage) []cmapi.KeyUsage { return append(list, cmapi.UsageCertSign) } + +func getCertificateRequestCondition(conds []cmapi.CertificateRequestCondition, conditionType cmapi.CertificateRequestConditionType) *cmapi.CertificateRequestCondition { + for _, cond := range conds { + if cond.Type == conditionType { + return &cond + } + } + return nil +} diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index ab667837a..cad2f196f 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -27,12 +27,17 @@ import ( cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cminternal "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" + cminternalmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" "github.com/jetstack/cert-manager/pkg/util/pki" utilpki "github.com/jetstack/cert-manager/pkg/util/pki" "github.com/jetstack/cert-manager/test/unit/gen" ) func TestValidateCertificateRequestUpdate(t *testing.T) { + fldPathConditions := field.NewPath("status", "conditions") + + baseRequest := mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))) + baseCR := &cminternal.CertificateRequest{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -42,7 +47,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))), + Request: baseRequest, IssuerRef: validIssuerRef, Usages: nil, UID: "abc", @@ -100,136 +105,651 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { newCR: baseCR.DeepCopy(), want: nil, }, + "CertificateRequest with single Approved=true condition that doesn't change, shouldn't error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + want: nil, + }, + "CertificateRequest with single Denied=true condition that doesn't change, shouldn't error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + want: nil, + }, + "CertificateRequest with single Approved=false condition that changes, should error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionFalse, + Reason: "Foo", + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + Reason: "cert-manager.io", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "'Approved' condition may not be modified once set"), + }, + }, + "CertificateRequest with single Denied=false condition that changes, should error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: "Foo", + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionFalse, + Reason: "test", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "'Denied' condition may not be modified once set"), + field.Invalid(fldPathConditions.Child("Denied"), nil, `"Denied" condition may only be set to True`), + }, + }, + "CertificateRequest with single Denied=true condition that changes to Approve=true, should error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: "Foo", + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + Reason: "cert-manager.io", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "'Denied' condition may not be modified once set"), + }, + }, + "CertificateRequest with single Approved=true condition that changes to Denied=true, should error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + Reason: "cert-manager.io", + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: "Foo", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "'Approved' condition may not be modified once set"), + }, + }, + "CertificateRequest with no condition that changes to Approve=true, shouldn't error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{}, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + Reason: "cert-manager.io", + }, + }, + }, + }, + want: nil, + }, + "CertificateRequest with no condition that changes to Denied=true, shouldn't error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{}, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: "Foo", + }, + }, + }, + }, + want: nil, + }, + "CertificateRequest with single Approved=true condition that is removed, should error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{}, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "'Approved' condition may not be modified once set"), + }, + }, + "CertificateRequest with single Denied=true condition that is removed, should error": { + oldCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + newCR: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: baseRequest, + IssuerRef: validIssuerRef, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{}, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "'Denied' condition may not be modified once set"), + }, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - err := ValidateUpdateCertificateRequest(nil, test.oldCR, test.newCR) - if !reflect.DeepEqual(err, test.want) { - t.Errorf("got unexpected error response, exp=%v got=%v", - test.want, err) + got := ValidateUpdateCertificateRequest(nil, test.oldCR, test.newCR) + for i := range got { + if got[i].Type != field.ErrorTypeForbidden { + // filter out the value so it does not print the full CSR in tests + got[i].BadValue = nil + } + } + + if !reflect.DeepEqual(got, test.want) { + t.Errorf("ValidateUpdateCertificateRequest() = %v, want %v", got, test.want) } }) } } -func TestValidateCertificateRequestSpec(t *testing.T) { - fldPath := field.NewPath("test") +func TestValidateCertificateRequest(t *testing.T) { + fldPath := field.NewPath("spec") + fldPathConditions := field.NewPath("status", "conditions") - tests := []struct { - name string - crSpec *cminternal.CertificateRequestSpec - want field.ErrorList + tests := map[string]struct { + cr *cminternal.CertificateRequest + want field.ErrorList }{ - { - name: "Test csr with no usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))), - IssuerRef: validIssuerRef, - Usages: nil, + "Test csr with no usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))), + IssuerRef: validIssuerRef, + Usages: nil, + }, }, want: []*field.Error{}, }, - { - name: "Test csr with double signature usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageSigning, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, + "Test csr with double signature usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageSigning, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment))), + IssuerRef: validIssuerRef, + Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, + }, }, want: []*field.Error{}, }, - { - name: "Test csr with double extended usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, + "Test csr with double extended usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), + IssuerRef: validIssuerRef, + Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, + }, }, want: []*field.Error{}, }, - { - name: "Test csr with reordered usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageServerAuth, cminternal.UsageClientAuth, cminternal.UsageKeyEncipherment, cminternal.UsageDigitalSignature}, + "Test csr with reordered usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), + IssuerRef: validIssuerRef, + Usages: []cminternal.KeyUsage{cminternal.UsageServerAuth, cminternal.UsageClientAuth, cminternal.UsageKeyEncipherment, cminternal.UsageDigitalSignature}, + }, }, want: []*field.Error{}, }, - { - name: "Test csr that is CA with usages set", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageCertSign), gen.SetCertificateIsCA(true))), - IssuerRef: validIssuerRef, - IsCA: true, - Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageCertSign}, + "Test csr that is CA with usages set": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageCertSign), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageCertSign}, + }, }, want: []*field.Error{}, }, - { - name: "Test csr that is CA but no cert sign in usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth, cmapi.UsageServerAuth), gen.SetCertificateIsCA(true))), - IssuerRef: validIssuerRef, - IsCA: true, - Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, + "Test csr that is CA but no cert sign in usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth, cmapi.UsageServerAuth), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, + }, }, want: []*field.Error{}, }, - { - name: "Error on csr not having all usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, + "Error on csr not having all usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth))), + IssuerRef: validIssuerRef, + Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, + }, }, want: []*field.Error{ field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[3] != []certmanager.KeyUsage[4]]"), }, }, - { - name: "Error on cr not having all usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, + "Error on cr not having all usages": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), + IssuerRef: validIssuerRef, + Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, + }, }, want: []*field.Error{ field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[4] != []certmanager.KeyUsage[2]]"), }, }, - { - name: "Error on cr not having all usages", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageSigning}, - }, - want: []*field.Error{ - field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[4] != []certmanager.KeyUsage[2]]"), - }, - }, - { - name: "Test csr with any, signing, digital signature, key encipherment, server and client auth", - crSpec: &cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageSigning, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth, cmapi.UsageServerAuth), gen.SetCertificateIsCA(true))), - IssuerRef: validIssuerRef, - IsCA: true, - Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, + "Test csr with any, signing, digital signature, key encipherment, server and client auth": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageSigning, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth, cmapi.UsageServerAuth), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, + }, }, want: []*field.Error{}, }, + "CertificateRequest with single Approved=true condition, shouldn't error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + want: []*field.Error{}, + }, + "CertificateRequest with single Denied=true condition, shouldn't error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + }, + }, + }, + }, + want: []*field.Error{}, + }, + "CertificateRequest with single Approved=false condition, should error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionFalse, + Reason: "cert-manager.io", + }, + }, + }, + }, + want: []*field.Error{ + field.Invalid(fldPathConditions.Child("Approved"), nil, + `"Approved" condition may only be set to True`), + }, + }, + "CertificateRequest with single Denied=false condition, should error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionFalse, + Reason: "Foo", + }, + }, + }, + }, + want: []*field.Error{ + field.Invalid(fldPathConditions.Child("Denied"), nil, + `"Denied" condition may only be set to True`), + }, + }, + "CertificateRequest with both Denied=false and Approved=false conditions, should error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionFalse, + Reason: "cert-manager.io", + }, + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionFalse, + Reason: "Foo", + }, + }, + }, + }, + want: []*field.Error{ + field.Invalid(field.NewPath("status", "conditions", "Approved"), nil, + `"Approved" condition may only be set to True`), + field.Invalid(field.NewPath("status", "conditions", "Denied"), nil, + `"Denied" condition may only be set to True`), + field.Forbidden(fldPathConditions, "both 'Denied' and 'Approved' conditions cannot coexist"), + }, + }, + "CertificateRequest with both Denied=true and Approved=true conditions, should error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + Reason: "cert-manager.io", + }, + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: "Foo", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, "both 'Denied' and 'Approved' conditions cannot coexist"), + }, + }, + "CertificateRequest with multiple Approved conditions, should error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionTrue, + Reason: "cert-manager.io", + }, + { + Type: cminternal.CertificateRequestConditionApproved, + Status: cminternalmeta.ConditionFalse, + Reason: "foo", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, `multiple "Approved" conditions present`), + }, + }, + "CertificateRequest with multiple Denied conditions, should error": { + cr: &cminternal.CertificateRequest{ + Spec: cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny}, + }, + Status: cminternal.CertificateRequestStatus{ + Conditions: []cminternal.CertificateRequestCondition{ + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: "Foo", + }, + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionFalse, + Reason: "Foo", + }, + }, + }, + }, + want: []*field.Error{ + field.Forbidden(fldPathConditions, `multiple "Denied" conditions present`), + }, + }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := ValidateCertificateRequestSpec(tt.crSpec, field.NewPath("test"), true) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + got := ValidateCertificateRequest(nil, test.cr) for i := range got { - // filter out the value so it does not print the full CSR in tests - got[i].BadValue = nil + if got[i].Type != field.ErrorTypeForbidden { + // filter out the value so it does not print the full CSR in tests + got[i].BadValue = nil + } } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("ValidateCertificateRequestSpec() = %v, want %v", got, tt.want) + if !reflect.DeepEqual(got, test.want) { + t.Errorf("ValidateCertificateRequest() = %v, want %v", got, test.want) } }) } diff --git a/test/e2e/framework/helper/certificaterequests.go b/test/e2e/framework/helper/certificaterequests.go index 241ad3e4b..b1a7731b7 100644 --- a/test/e2e/framework/helper/certificaterequests.go +++ b/test/e2e/framework/helper/certificaterequests.go @@ -186,6 +186,13 @@ func (h *Helper) ValidateIssuedCertificateRequest(cr *cmapi.CertificateRequest, } } + if !apiutil.CertificateRequestIsApproved(cr) { + return nil, fmt.Errorf("CertificateRequest does not have an Approved condition set to True: %+v", cr.Status.Conditions) + } + if apiutil.CertificateRequestIsDenied(cr) { + return nil, fmt.Errorf("CertificateRequest has a Denied conditon set to True: %+v", cr.Status.Conditions) + } + return cert, nil } diff --git a/test/unit/gen/certificaterequest.go b/test/unit/gen/certificaterequest.go index a24fbd0fe..39a1335a3 100644 --- a/test/unit/gen/certificaterequest.go +++ b/test/unit/gen/certificaterequest.go @@ -96,6 +96,12 @@ func SetCertificateRequestStatusCondition(c v1.CertificateRequestCondition) Cert } } +func AddCertificateRequestStatusCondition(c v1.CertificateRequestCondition) CertificateRequestModifier { + return func(cr *v1.CertificateRequest) { + cr.Status.Conditions = append(cr.Status.Conditions, c) + } +} + func SetCertificateRequestNamespace(namespace string) CertificateRequestModifier { return func(cr *v1.CertificateRequest) { cr.ObjectMeta.Namespace = namespace