From d61ccb17304df327c8eb866be5f0c52ebae06843 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:23:19 +0000 Subject: [PATCH 01/20] Adds CertificateRequest Approved condition type, with Approved and Denied Reasons Signed-off-by: joshvanl --- pkg/apis/certmanager/v1/types_certificaterequest.go | 12 ++++++++++++ .../certmanager/v1alpha2/types_certificaterequest.go | 12 ++++++++++++ .../certmanager/v1alpha3/types_certificaterequest.go | 12 ++++++++++++ .../certmanager/v1beta1/types_certificaterequest.go | 12 ++++++++++++ .../apis/certmanager/types_certificaterequest.go | 12 ++++++++++++ 5 files changed, 60 insertions(+) diff --git a/pkg/apis/certmanager/v1/types_certificaterequest.go b/pkg/apis/certmanager/v1/types_certificaterequest.go index a86700ff7..22166b9ab 100644 --- a/pkg/apis/certmanager/v1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1/types_certificaterequest.go @@ -33,6 +33,14 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" + + // Approved indicates that a CertificateRequest has been approved by the + // approver, and the CertificateRequest is ready for signing. + CertificateRequestReasonApproved = "Approved" + + // Denied indicates that a CertificateRequest has been denied by the + // approver, and the CertificateRequest will be never be signed. + CertificateRequestReasonDenied = "Denied" ) // +genclient @@ -189,4 +197,8 @@ 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 is + // approved and ready for signing, or denied and should never be signed. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" ) diff --git a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go index be4b38b7a..12b90d20b 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go @@ -33,6 +33,14 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" + + // Approved indicates that a CertificateRequest has been approved by the + // approver, and the CertificateRequest is ready for signing. + CertificateRequestReasonApproved = "Approved" + + // Denied indicates that a CertificateRequest has been denied by the + // approver, and the CertificateRequest will be never be signed. + CertificateRequestReasonDenied = "Denied" ) // +genclient @@ -186,4 +194,8 @@ 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 is + // approved and ready for signing, or denied and should never be signed. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" ) diff --git a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go index 46393cab3..5143e50df 100644 --- a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go @@ -33,6 +33,14 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" + + // Approved indicates that a CertificateRequest has been approved by the + // approver, and the CertificateRequest is ready for signing. + CertificateRequestReasonApproved = "Approved" + + // Denied indicates that a CertificateRequest has been denied by the + // approver, and the CertificateRequest will be never be signed. + CertificateRequestReasonDenied = "Denied" ) // +genclient @@ -186,4 +194,8 @@ 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 is + // approved and ready for signing, or denied and should never be signed. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" ) diff --git a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go index 3af27a8a9..78111ea68 100644 --- a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go @@ -33,6 +33,14 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" + + // Approved indicates that a CertificateRequest has been approved by the + // approver, and the CertificateRequest is ready for signing. + CertificateRequestReasonApproved = "Approved" + + // Denied indicates that a CertificateRequest has been denied by the + // approver, and the CertificateRequest will be never be signed. + CertificateRequestReasonDenied = "Denied" ) // +genclient @@ -187,4 +195,8 @@ 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 is + // approved and ready for signing, or denied and should never be signed. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" ) diff --git a/pkg/internal/apis/certmanager/types_certificaterequest.go b/pkg/internal/apis/certmanager/types_certificaterequest.go index a6c3b9bfc..37c68057f 100644 --- a/pkg/internal/apis/certmanager/types_certificaterequest.go +++ b/pkg/internal/apis/certmanager/types_certificaterequest.go @@ -33,6 +33,14 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" + + // Approved indicates that a CertificateRequest has been approved by the + // approver, and the CertificateRequest is ready for signing. + CertificateRequestReasonApproved = "Approved" + + // Denied indicates that a CertificateRequest has been denied by the + // approver, and the CertificateRequest will be never be signed. + CertificateRequestReasonDenied = "Denied" ) // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -169,4 +177,8 @@ 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 is + // approved and ready for signing, or denied and should never be signed. + CertificateRequestConditionApproved CertificateRequestConditionType = "Approved" ) From 0ef25daeb316d7513b163b945a3bac98acb2a09e Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:26:35 +0000 Subject: [PATCH 02/20] Adds helper CertificateRequest api/util funcs for checking approval condition Signed-off-by: joshvanl --- pkg/api/util/conditions.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pkg/api/util/conditions.go b/pkg/api/util/conditions.go index 7ed1b07f2..5c55fb1aa 100644 --- a/pkg/api/util/conditions.go +++ b/pkg/api/util/conditions.go @@ -315,3 +315,37 @@ func CertificateRequestHasInvalidRequest(cr *cmapi.CertificateRequest) bool { return false } + +// This returns with true if the CertificateRequest is approved via an Approved +// condition of true, returns false otherwise. +func CertificateRequestHasApproved(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 +} + +// This returns with true if the CertificateRequest is denied via an Approved +// condition of false, returns false otherwise. +func CertificateRequestHasDenied(cr *cmapi.CertificateRequest) bool { + if cr == nil { + return false + } + + for _, con := range cr.Status.Conditions { + if con.Type == cmapi.CertificateRequestConditionApproved && + con.Status == cmmeta.ConditionFalse { + return true + } + } + + return false +} From 2db7582586f7eba2e5cf391c13268c896a2162ff Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:27:33 +0000 Subject: [PATCH 03/20] Adds CertificateRequest approver controller. This controller will currently _always_ set the Approved condition to true on CertificateRequests Signed-off-by: joshvanl --- .../certificaterequests/approver/BUILD.bazel | 56 ++++ .../certificaterequests/approver/approver.go | 103 ++++++++ .../approver/approver_test.go | 240 ++++++++++++++++++ .../certificaterequests/approver/sync.go | 73 ++++++ 4 files changed, 472 insertions(+) create mode 100644 pkg/controller/certificaterequests/approver/BUILD.bazel create mode 100644 pkg/controller/certificaterequests/approver/approver.go create mode 100644 pkg/controller/certificaterequests/approver/approver_test.go create mode 100644 pkg/controller/certificaterequests/approver/sync.go 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..e7f8be7bd --- /dev/null +++ b/pkg/controller/certificaterequests/approver/approver.go @@ -0,0 +1,103 @@ +/* +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" + + "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) + 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) { + log.Error(err, "certificate request in work queue no longer exists") + 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..6d200af51 --- /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.CertificateRequestConditionApproved, + Status: cmmeta.ConditionFalse, + }, + }, + }, + }, + }, + "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: cmapi.CertificateRequestReasonApproved, + Message: ApprovedMessage, + LastTransitionTime: &metaNow, + }, + }, + expectedEvent: "Normal Approved 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: cmapi.CertificateRequestReasonApproved, + Message: ApprovedMessage, + LastTransitionTime: &metaNow, + }, + }, + expectedEvent: "Normal Approved 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..69e83b7df --- /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 alrady +// 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.CertificateRequestHasApproved(cr), + + // If the CertificateRequest has already been denied, exit early. + apiutil.CertificateRequestHasDenied(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, + cmapi.CertificateRequestReasonApproved, + 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, cmapi.CertificateRequestReasonApproved, ApprovedMessage) + + log.V(logf.DebugLevel).Info("approved certificate request") + + return nil +} From 1d758a5ccf894a3903d11e68fa2e1bb682ba90fc Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:28:42 +0000 Subject: [PATCH 04/20] Updates the base CertificateRequest controller to first check for the approval condition to be present and set to true, before processing further Signed-off-by: joshvanl --- .../certificaterequests/BUILD.bazel | 1 + pkg/controller/certificaterequests/sync.go | 22 +++++--- .../certificaterequests/sync_test.go | 52 ++++++++++++++++++- 3 files changed, 66 insertions(+), 9 deletions(-) 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/sync.go b/pkg/controller/certificaterequests/sync.go index 4c55f6950..f6c6cc6e1 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, exit early. + if !apiutil.CertificateRequestHasApproved(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..686eb9ad7 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.CertificateRequestConditionApproved, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonDenied, + 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{ From e62e8c517b4b66abab01714a38c3e5b8d999b378 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:29:46 +0000 Subject: [PATCH 05/20] Updates CertificateRequest signer tests to check Approved behaviour Signed-off-by: joshvanl --- .../certificaterequests/acme/acme_test.go | 50 ++++++++++++++++++- .../certificaterequests/ca/ca_test.go | 21 +++++++- .../selfsigned/selfsigned_test.go | 18 ++++++- .../certificaterequests/vault/vault_test.go | 18 ++++++- .../certificaterequests/venafi/venafi_test.go | 20 +++++++- 5 files changed, 119 insertions(+), 8 deletions(-) diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 1638f223b..822fad148 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,15 @@ func TestSign(t *testing.T) { Kind: "Issuer", }), ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonApproved, + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) csr, err := pki.DecodeX509CertificateRequestBytes(csrPEM) if err != nil { @@ -175,8 +185,44 @@ 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 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/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index a6a6c17af..b0e960e26 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,15 @@ func TestSign(t *testing.T) { }), gen.SetCertificateRequestDuration(&metav1.Duration{Duration: time.Hour * 24 * 60}), ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonApproved, + 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 +172,14 @@ 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 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..178f076e1 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,15 @@ func TestSign(t *testing.T) { Kind: "Issuer", }), ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonApproved, + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) ecCR := gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestCSR(csrECPEM), ) @@ -169,6 +178,13 @@ 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 no cert-manager.io/selfsigned-private-key annotation should fail": { certificateRequest: gen.CertificateRequestFrom(baseCR, // no annotation diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index ee857b851..a40a18209 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,15 @@ func TestSign(t *testing.T) { Kind: baseIssuer.Kind, }), ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonApproved, + 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 +160,13 @@ 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()}, + }, + }, "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..5b3e4427f 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,18 @@ func TestSign(t *testing.T) { }), ) - baseCR := gen.CertificateRequest("test-cr", + baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestCSR(csrPEM), ) + baseCR := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionApproved, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonApproved, + Message: "Certificate request has been approved by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) tppCR := gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ @@ -226,8 +236,14 @@ 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()}, + }, + }, "tpp: if fail to build client based on missing secret then return nil and hard fail": { certificateRequest: tppCR.DeepCopy(), builder: &controllertest.Builder{ From 6957bc31df012197a984e3e61671254f271f39cb Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:30:25 +0000 Subject: [PATCH 06/20] Adds the CertificateRequest approver controller to cert-manager-controller Signed-off-by: joshvanl --- cmd/controller/app/options/BUILD.bazel | 1 + cmd/controller/app/options/options.go | 2 ++ 2 files changed, 3 insertions(+) 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, From e6c74d94b4b01f187662711923fe56eb85242ce5 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:30:50 +0000 Subject: [PATCH 07/20] Adds check for approval condition in e2e CertificateRequest helper func Signed-off-by: joshvanl --- test/e2e/framework/helper/certificaterequests.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/e2e/framework/helper/certificaterequests.go b/test/e2e/framework/helper/certificaterequests.go index 241ad3e4b..1b3580261 100644 --- a/test/e2e/framework/helper/certificaterequests.go +++ b/test/e2e/framework/helper/certificaterequests.go @@ -186,6 +186,10 @@ func (h *Helper) ValidateIssuedCertificateRequest(cr *cmapi.CertificateRequest, } } + if !apiutil.CertificateRequestHasApproved(cr) { + return nil, fmt.Errorf("CertificateRequest does not have an Approved condition: %+v", cr.Status.Conditions) + } + return cert, nil } From fb54272d1790f2c3ebef1240a4101274b58af819 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 4 Mar 2021 16:31:19 +0000 Subject: [PATCH 08/20] Adds Approved condition status as additionalPrinterColumn for pretty printing Signed-off-by: joshvanl --- deploy/crds/crd-certificaterequests.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/deploy/crds/crd-certificaterequests.yaml b/deploy/crds/crd-certificaterequests.yaml index 9dd1f9371..698975f6c 100644 --- a/deploy/crds/crd-certificaterequests.yaml +++ b/deploy/crds/crd-certificaterequests.yaml @@ -39,6 +39,9 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -202,6 +205,9 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -365,6 +371,9 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -530,6 +539,9 @@ spec: subresources: status: {} additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=="Approved")].status + name: Approved + type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string From 417b9477336b8c1f04884e170752a51fd511fd91 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:31:13 +0000 Subject: [PATCH 09/20] Updates CertificateRequest conditions to include a distinct 'Denied' condition type Signed-off-by: joshvanl --- .../v1/types_certificaterequest.go | 19 ++++++++++++++----- .../v1alpha2/types_certificaterequest.go | 19 ++++++++++++++----- .../v1alpha3/types_certificaterequest.go | 17 ++++++++++++----- .../v1beta1/types_certificaterequest.go | 19 ++++++++++++++----- .../certmanager/types_certificaterequest.go | 17 ++++++++++++----- 5 files changed, 66 insertions(+), 25 deletions(-) diff --git a/pkg/apis/certmanager/v1/types_certificaterequest.go b/pkg/apis/certmanager/v1/types_certificaterequest.go index 22166b9ab..b7c3397a7 100644 --- a/pkg/apis/certmanager/v1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1/types_certificaterequest.go @@ -35,11 +35,11 @@ const ( CertificateRequestReasonIssued = "Issued" // Approved indicates that a CertificateRequest has been approved by the - // approver, and the CertificateRequest is ready for signing. + // approver, and the certificate request is ready for signing. CertificateRequestReasonApproved = "Approved" // Denied indicates that a CertificateRequest has been denied by the - // approver, and the CertificateRequest will be never be signed. + // approver, and the certificate request will be never be signed. CertificateRequestReasonDenied = "Denied" ) @@ -161,7 +161,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`). @@ -198,7 +199,15 @@ const ( // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" - // CertificateRequestConditionApproved indicates that a certificate is - // approved and ready for signing, or denied and should never be signed. + // 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 12b90d20b..b390f91b5 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go @@ -35,11 +35,11 @@ const ( CertificateRequestReasonIssued = "Issued" // Approved indicates that a CertificateRequest has been approved by the - // approver, and the CertificateRequest is ready for signing. + // approver, and the certificate request is ready for signing. CertificateRequestReasonApproved = "Approved" // Denied indicates that a CertificateRequest has been denied by the - // approver, and the CertificateRequest will be never be signed. + // approver, and the certificate request will be never be signed. CertificateRequestReasonDenied = "Denied" ) @@ -158,7 +158,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`). @@ -195,7 +196,15 @@ const ( // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" - // CertificateRequestConditionApproved indicates that a certificate is - // approved and ready for signing, or denied and should never be signed. + // 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 5143e50df..3932a09c5 100644 --- a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go @@ -35,11 +35,11 @@ const ( CertificateRequestReasonIssued = "Issued" // Approved indicates that a CertificateRequest has been approved by the - // approver, and the CertificateRequest is ready for signing. + // approver, and the certificate request is ready for signing. CertificateRequestReasonApproved = "Approved" // Denied indicates that a CertificateRequest has been denied by the - // approver, and the CertificateRequest will be never be signed. + // approver, and the certificate request will be never be signed. CertificateRequestReasonDenied = "Denied" ) @@ -158,7 +158,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`). @@ -195,7 +196,13 @@ const ( // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" - // CertificateRequestConditionApproved indicates that a certificate is - // approved and ready for signing, or denied and should never be signed. + // 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 78111ea68..9a5be08ca 100644 --- a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go @@ -35,11 +35,11 @@ const ( CertificateRequestReasonIssued = "Issued" // Approved indicates that a CertificateRequest has been approved by the - // approver, and the CertificateRequest is ready for signing. + // approver, and the certificate request is ready for signing. CertificateRequestReasonApproved = "Approved" // Denied indicates that a CertificateRequest has been denied by the - // approver, and the CertificateRequest will be never be signed. + // approver, and the certificate request will be never be signed. CertificateRequestReasonDenied = "Denied" ) @@ -159,7 +159,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`). @@ -196,7 +197,15 @@ const ( // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" - // CertificateRequestConditionApproved indicates that a certificate is - // approved and ready for signing, or denied and should never be signed. + // 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/internal/apis/certmanager/types_certificaterequest.go b/pkg/internal/apis/certmanager/types_certificaterequest.go index 37c68057f..6debc46d5 100644 --- a/pkg/internal/apis/certmanager/types_certificaterequest.go +++ b/pkg/internal/apis/certmanager/types_certificaterequest.go @@ -35,11 +35,11 @@ const ( CertificateRequestReasonIssued = "Issued" // Approved indicates that a CertificateRequest has been approved by the - // approver, and the CertificateRequest is ready for signing. + // approver, and the certificate request is ready for signing. CertificateRequestReasonApproved = "Approved" // Denied indicates that a CertificateRequest has been denied by the - // approver, and the CertificateRequest will be never be signed. + // approver, and the certificate request will be never be signed. CertificateRequestReasonDenied = "Denied" ) @@ -144,7 +144,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`). @@ -178,7 +179,13 @@ const ( // was rejected can be found in the `reason` and `message` fields. CertificateRequestConditionInvalidRequest CertificateRequestConditionType = "InvalidRequest" - // CertificateRequestConditionApproved indicates that a certificate is - // approved and ready for signing, or denied and should never be signed. + // 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" ) From 5df29e41e7a3fdd1031d5a816fa7d5207d84f051 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:32:08 +0000 Subject: [PATCH 10/20] Updates api/util CertificateRequest approved helpers to use new condition type Signed-off-by: joshvanl --- pkg/api/util/conditions.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/api/util/conditions.go b/pkg/api/util/conditions.go index 5c55fb1aa..986bcb874 100644 --- a/pkg/api/util/conditions.go +++ b/pkg/api/util/conditions.go @@ -316,9 +316,10 @@ func CertificateRequestHasInvalidRequest(cr *cmapi.CertificateRequest) bool { return false } -// This returns with true if the CertificateRequest is approved via an Approved -// condition of true, returns false otherwise. -func CertificateRequestHasApproved(cr *cmapi.CertificateRequest) bool { +// 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 } @@ -333,16 +334,16 @@ func CertificateRequestHasApproved(cr *cmapi.CertificateRequest) bool { return false } -// This returns with true if the CertificateRequest is denied via an Approved -// condition of false, returns false otherwise. -func CertificateRequestHasDenied(cr *cmapi.CertificateRequest) bool { +// 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.CertificateRequestConditionApproved && - con.Status == cmmeta.ConditionFalse { + if con.Type == cmapi.CertificateRequestConditionDenied && + con.Status == cmmeta.ConditionTrue { return true } } From 4e042011e63ff603586d7352b0a690b1711e38fa Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:32:59 +0000 Subject: [PATCH 11/20] Adds CertificateRequest approval condition validation to ensure: - Only a single Approve _or_ Deny condition may exist - They cannot be modified once set - They must always have a status of `True` Signed-off-by: joshvanl --- .../validation/certificaterequest.go | 89 +++++ .../validation/certificaterequest_test.go | 374 +++++++++++++----- 2 files changed, 372 insertions(+), 91 deletions(-) diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index 1c7b33b08..d129a3850 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(condition.Reason), 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..adcf36e3c 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -27,6 +27,7 @@ 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" @@ -114,122 +115,318 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { } func TestValidateCertificateRequestSpec(t *testing.T) { - fldPath := field.NewPath("test") + 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("spec", 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("spec", 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("spec", 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("spec", 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("spec", 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("spec", 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("spec", 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("spec", 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("spec", 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: cmapi.CertificateRequestReasonApproved, + }, + }, + }, + }, + want: []*field.Error{ + field.Invalid(fldPathConditions.Child(cmapi.CertificateRequestReasonApproved), 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: cmapi.CertificateRequestReasonDenied, + }, + }, + }, + }, + want: []*field.Error{ + field.Invalid(fldPathConditions.Child(cmapi.CertificateRequestReasonDenied), 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: cmapi.CertificateRequestReasonApproved, + }, + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonDenied, + }, + }, + }, + }, + want: []*field.Error{ + field.Invalid(field.NewPath("status", "conditions", cmapi.CertificateRequestReasonApproved), nil, + `"Approved" condition may only be set to True`), + field.Invalid(field.NewPath("status", "conditions", cmapi.CertificateRequestReasonDenied), 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: cmapi.CertificateRequestReasonApproved, + }, + { + Type: cminternal.CertificateRequestConditionDenied, + Status: cminternalmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonDenied, + }, + }, + }, + }, + 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: cmapi.CertificateRequestReasonApproved, + }, + { + 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: cmapi.CertificateRequestReasonDenied, + }, + { + 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, tt := range tests { + t.Run(name, func(t *testing.T) { + got := ValidateCertificateRequest(nil, tt.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) + t.Errorf("ValidateCertificateRequest() = %v, want %v", got, tt.want) } }) } @@ -261,34 +458,29 @@ func mustGenerateCSR(t *testing.T, crt *cmapi.Certificate) []byte { } func Test_patchDuplicateKeyUsage(t *testing.T) { - tests := []struct { - name string + tests := map[string]struct { usages []cminternal.KeyUsage want []cminternal.KeyUsage }{ - { - name: "Test single KU", + "Test single KU": { usages: []cminternal.KeyUsage{cminternal.UsageKeyEncipherment}, want: []cminternal.KeyUsage{cminternal.UsageKeyEncipherment}, }, - { - name: "Test UsageSigning", + "Test UsageSigning": { usages: []cminternal.KeyUsage{cminternal.UsageSigning}, want: []cminternal.KeyUsage{cminternal.UsageDigitalSignature}, }, - { - name: "Test multiple KU", + "Test multiple KU": { usages: []cminternal.KeyUsage{cminternal.UsageDigitalSignature, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, want: []cminternal.KeyUsage{cminternal.UsageDigitalSignature, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, }, - { - name: "Test double signing", + "Test double signing": { usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageDigitalSignature}, want: []cminternal.KeyUsage{cminternal.UsageDigitalSignature}, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range tests { + t.Run(name, func(t *testing.T) { if got := patchDuplicateKeyUsage(tt.usages); !reflect.DeepEqual(got, tt.want) { t.Errorf("patchDuplicateKeyUsage() = %v, want %v", got, tt.want) } From 09f91a2a99c679c85d27daf1372fcc23e2279d7e Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:34:23 +0000 Subject: [PATCH 12/20] Update approver controller to use new Denied condition type Signed-off-by: joshvanl --- .../certificaterequests/approver/approver_test.go | 4 ++-- pkg/controller/certificaterequests/approver/sync.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/controller/certificaterequests/approver/approver_test.go b/pkg/controller/certificaterequests/approver/approver_test.go index 6d200af51..c3d8bb56a 100644 --- a/pkg/controller/certificaterequests/approver/approver_test.go +++ b/pkg/controller/certificaterequests/approver/approver_test.go @@ -83,8 +83,8 @@ func TestProcessItem(t *testing.T) { Status: cmapi.CertificateRequestStatus{ Conditions: []cmapi.CertificateRequestCondition{ { - Type: cmapi.CertificateRequestConditionApproved, - Status: cmmeta.ConditionFalse, + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, }, }, }, diff --git a/pkg/controller/certificaterequests/approver/sync.go b/pkg/controller/certificaterequests/approver/sync.go index 69e83b7df..eeb4d2aaf 100644 --- a/pkg/controller/certificaterequests/approver/sync.go +++ b/pkg/controller/certificaterequests/approver/sync.go @@ -33,18 +33,18 @@ const ( ) // Sync will set the "Approved" condition to True on synced -// CertificateRequests. If the "Denied", "Approved" or "Ready" condition alrady -// exists, exit early. +// 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.CertificateRequestHasApproved(cr), + apiutil.CertificateRequestIsApproved(cr), // If the CertificateRequest has already been denied, exit early. - apiutil.CertificateRequestHasDenied(cr), + apiutil.CertificateRequestIsDenied(cr), // If the CertificateRequest is "Issued" or "Failed", exit early. apiutil.CertificateRequestReadyReason(cr) == cmapi.CertificateRequestReasonFailed, From a3e63b1787b8deb852633c7ff7f5862dc5f2d10b Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:34:49 +0000 Subject: [PATCH 13/20] Update CertificateRequest controllers to use new Denied type, and add tests for when a CertificateRequest is denied Signed-off-by: joshvanl --- .../certificaterequests/acme/acme_test.go | 16 ++++++++++++++++ pkg/controller/certificaterequests/ca/ca_test.go | 16 ++++++++++++++++ .../selfsigned/selfsigned_test.go | 16 ++++++++++++++++ pkg/controller/certificaterequests/sync.go | 4 ++-- pkg/controller/certificaterequests/sync_test.go | 4 ++-- .../certificaterequests/vault/vault_test.go | 16 ++++++++++++++++ .../certificaterequests/venafi/venafi_test.go | 16 ++++++++++++++++ 7 files changed, 84 insertions(+), 4 deletions(-) diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 822fad148..57fa1930f 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -129,6 +129,15 @@ func TestSign(t *testing.T) { Kind: "Issuer", }), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonDenied, + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) baseCR := gen.CertificateRequestFrom(baseCRNotApproved, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, @@ -193,6 +202,13 @@ func TestSign(t *testing.T) { 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")), diff --git a/pkg/controller/certificaterequests/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index b0e960e26..25986051a 100644 --- a/pkg/controller/certificaterequests/ca/ca_test.go +++ b/pkg/controller/certificaterequests/ca/ca_test.go @@ -135,6 +135,15 @@ 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: cmapi.CertificateRequestReasonDenied, + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) baseCR := gen.CertificateRequestFrom(baseCRNotApproved, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, @@ -180,6 +189,13 @@ func TestSign(t *testing.T) { 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 178f076e1..08fd761c7 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go @@ -142,6 +142,15 @@ func TestSign(t *testing.T) { Kind: "Issuer", }), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonDenied, + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) baseCR := gen.CertificateRequestFrom(baseCRNotApproved, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, @@ -185,6 +194,13 @@ func TestSign(t *testing.T) { 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 f6c6cc6e1..88f9b4f72 100644 --- a/pkg/controller/certificaterequests/sync.go +++ b/pkg/controller/certificaterequests/sync.go @@ -48,8 +48,8 @@ func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (er return nil } - // If CertificateRequest has not been approved, exit early. - if !apiutil.CertificateRequestHasApproved(cr) { + // 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 } diff --git a/pkg/controller/certificaterequests/sync_test.go b/pkg/controller/certificaterequests/sync_test.go index 686eb9ad7..a5c7e6866 100644 --- a/pkg/controller/certificaterequests/sync_test.go +++ b/pkg/controller/certificaterequests/sync_test.go @@ -171,8 +171,8 @@ func TestSync(t *testing.T) { "should return nil (no action) if certificate request is denied": { certificateRequest: gen.CertificateRequestFrom(baseCRNotApproved, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ - Type: cmapi.CertificateRequestConditionApproved, - Status: cmmeta.ConditionFalse, + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, Reason: cmapi.CertificateRequestReasonDenied, Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &nowMetaTime, diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index a40a18209..b32b8037a 100644 --- a/pkg/controller/certificaterequests/vault/vault_test.go +++ b/pkg/controller/certificaterequests/vault/vault_test.go @@ -123,6 +123,15 @@ func TestSign(t *testing.T) { Kind: baseIssuer.Kind, }), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonDenied, + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) baseCR := gen.CertificateRequestFrom(baseCRNotApproved, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, @@ -167,6 +176,13 @@ func TestSign(t *testing.T) { 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 5b3e4427f..ea326589f 100644 --- a/pkg/controller/certificaterequests/venafi/venafi_test.go +++ b/pkg/controller/certificaterequests/venafi/venafi_test.go @@ -139,6 +139,15 @@ func TestSign(t *testing.T) { baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestCSR(csrPEM), ) + baseCRDenied := gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonDenied, + Message: "Certificate request has been denied by cert-manager.io", + LastTransitionTime: &metaFixedClockStart, + }), + ) baseCR := gen.CertificateRequestFrom(baseCRNotApproved, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, @@ -244,6 +253,13 @@ func TestSign(t *testing.T) { 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{ From 1235ff3bef39759779b4ba1988171ed5e643e1e6 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:35:23 +0000 Subject: [PATCH 14/20] Adds tests to ensure Approve is present, and Denied is not Signed-off-by: joshvanl --- test/e2e/framework/helper/certificaterequests.go | 7 +++++-- test/unit/gen/certificaterequest.go | 6 ++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/test/e2e/framework/helper/certificaterequests.go b/test/e2e/framework/helper/certificaterequests.go index 1b3580261..b1a7731b7 100644 --- a/test/e2e/framework/helper/certificaterequests.go +++ b/test/e2e/framework/helper/certificaterequests.go @@ -186,8 +186,11 @@ func (h *Helper) ValidateIssuedCertificateRequest(cr *cmapi.CertificateRequest, } } - if !apiutil.CertificateRequestHasApproved(cr) { - return nil, fmt.Errorf("CertificateRequest does not have an Approved condition: %+v", cr.Status.Conditions) + 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 From 72904ca2c157b50f14bd4f6e2a8d6122f10d1c73 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 8 Mar 2021 13:36:58 +0000 Subject: [PATCH 15/20] Updates CertificateRequest printColumn with a new Denied column Signed-off-by: joshvanl --- deploy/crds/crd-certificaterequests.yaml | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/deploy/crds/crd-certificaterequests.yaml b/deploy/crds/crd-certificaterequests.yaml index 698975f6c..d01182284 100644 --- a/deploy/crds/crd-certificaterequests.yaml +++ b/deploy/crds/crd-certificaterequests.yaml @@ -42,6 +42,9 @@ spec: - 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 @@ -193,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. @@ -208,6 +211,9 @@ spec: - 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 @@ -359,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. @@ -374,6 +380,9 @@ spec: - 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 @@ -527,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. @@ -542,6 +551,9 @@ spec: - 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 @@ -695,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. From 98a33791e4d128bb39f0e8e669b76f62d1b130eb Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 9 Mar 2021 10:29:33 +0000 Subject: [PATCH 16/20] Remove CertificateRequest Approve/Deny Reasons Signed-off-by: joshvanl --- pkg/apis/certmanager/v1/types_certificaterequest.go | 8 -------- pkg/apis/certmanager/v1alpha2/types_certificaterequest.go | 8 -------- pkg/apis/certmanager/v1alpha3/types_certificaterequest.go | 8 -------- pkg/apis/certmanager/v1beta1/types_certificaterequest.go | 8 -------- pkg/internal/apis/certmanager/types_certificaterequest.go | 8 -------- 5 files changed, 40 deletions(-) diff --git a/pkg/apis/certmanager/v1/types_certificaterequest.go b/pkg/apis/certmanager/v1/types_certificaterequest.go index b7c3397a7..215dbb288 100644 --- a/pkg/apis/certmanager/v1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1/types_certificaterequest.go @@ -33,14 +33,6 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" - - // Approved indicates that a CertificateRequest has been approved by the - // approver, and the certificate request is ready for signing. - CertificateRequestReasonApproved = "Approved" - - // Denied indicates that a CertificateRequest has been denied by the - // approver, and the certificate request will be never be signed. - CertificateRequestReasonDenied = "Denied" ) // +genclient diff --git a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go index b390f91b5..d38414ceb 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificaterequest.go @@ -33,14 +33,6 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" - - // Approved indicates that a CertificateRequest has been approved by the - // approver, and the certificate request is ready for signing. - CertificateRequestReasonApproved = "Approved" - - // Denied indicates that a CertificateRequest has been denied by the - // approver, and the certificate request will be never be signed. - CertificateRequestReasonDenied = "Denied" ) // +genclient diff --git a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go index 3932a09c5..3fa69e7f9 100644 --- a/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1alpha3/types_certificaterequest.go @@ -33,14 +33,6 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" - - // Approved indicates that a CertificateRequest has been approved by the - // approver, and the certificate request is ready for signing. - CertificateRequestReasonApproved = "Approved" - - // Denied indicates that a CertificateRequest has been denied by the - // approver, and the certificate request will be never be signed. - CertificateRequestReasonDenied = "Denied" ) // +genclient diff --git a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go index 9a5be08ca..5b59b3341 100644 --- a/pkg/apis/certmanager/v1beta1/types_certificaterequest.go +++ b/pkg/apis/certmanager/v1beta1/types_certificaterequest.go @@ -33,14 +33,6 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" - - // Approved indicates that a CertificateRequest has been approved by the - // approver, and the certificate request is ready for signing. - CertificateRequestReasonApproved = "Approved" - - // Denied indicates that a CertificateRequest has been denied by the - // approver, and the certificate request will be never be signed. - CertificateRequestReasonDenied = "Denied" ) // +genclient diff --git a/pkg/internal/apis/certmanager/types_certificaterequest.go b/pkg/internal/apis/certmanager/types_certificaterequest.go index 6debc46d5..7de08f7cb 100644 --- a/pkg/internal/apis/certmanager/types_certificaterequest.go +++ b/pkg/internal/apis/certmanager/types_certificaterequest.go @@ -33,14 +33,6 @@ const ( // Issued indicates that a CertificateRequest has been completed, and that // the `status.certificate` field is set. CertificateRequestReasonIssued = "Issued" - - // Approved indicates that a CertificateRequest has been approved by the - // approver, and the certificate request is ready for signing. - CertificateRequestReasonApproved = "Approved" - - // Denied indicates that a CertificateRequest has been denied by the - // approver, and the certificate request will be never be signed. - CertificateRequestReasonDenied = "Denied" ) // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object From c94ad9973199c1a80fbee85b6a398044b9c75b5d Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 9 Mar 2021 10:29:56 +0000 Subject: [PATCH 17/20] Updates approver controller to use custom Approved Reason Signed-off-by: joshvanl --- pkg/controller/certificaterequests/approver/sync.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/certificaterequests/approver/sync.go b/pkg/controller/certificaterequests/approver/sync.go index eeb4d2aaf..7824793fb 100644 --- a/pkg/controller/certificaterequests/approver/sync.go +++ b/pkg/controller/certificaterequests/approver/sync.go @@ -56,7 +56,7 @@ func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (er apiutil.SetCertificateRequestCondition(cr, cmapi.CertificateRequestConditionApproved, cmmeta.ConditionTrue, - cmapi.CertificateRequestReasonApproved, + "cert-manager.io", ApprovedMessage, ) @@ -65,7 +65,7 @@ func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (er if err != nil { return err } - c.recorder.Event(cr, corev1.EventTypeNormal, cmapi.CertificateRequestReasonApproved, ApprovedMessage) + c.recorder.Event(cr, corev1.EventTypeNormal, "cert-manager.io", ApprovedMessage) log.V(logf.DebugLevel).Info("approved certificate request") From 32d0c5af4edc3d1c87c5b9f6eaf4a53fb12716f8 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 9 Mar 2021 10:30:32 +0000 Subject: [PATCH 18/20] Updates Approved/Denied tests for new reasons Signed-off-by: joshvanl --- .../certificaterequests/acme/acme_test.go | 4 +- .../approver/approver_test.go | 8 +- .../certificaterequests/ca/ca_test.go | 4 +- .../selfsigned/selfsigned_test.go | 4 +- .../certificaterequests/sync_test.go | 2 +- .../certificaterequests/vault/vault_test.go | 4 +- .../certificaterequests/venafi/venafi_test.go | 4 +- .../validation/certificaterequest.go | 2 +- .../validation/certificaterequest_test.go | 374 +++++------------- 9 files changed, 107 insertions(+), 299 deletions(-) diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 57fa1930f..c27bb2e8d 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -133,7 +133,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, + Reason: "Foo", Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), @@ -142,7 +142,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: "Certificate request has been approved by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), diff --git a/pkg/controller/certificaterequests/approver/approver_test.go b/pkg/controller/certificaterequests/approver/approver_test.go index c3d8bb56a..ba51c4898 100644 --- a/pkg/controller/certificaterequests/approver/approver_test.go +++ b/pkg/controller/certificaterequests/approver/approver_test.go @@ -129,12 +129,12 @@ func TestProcessItem(t *testing.T) { { Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: ApprovedMessage, LastTransitionTime: &metaNow, }, }, - expectedEvent: "Normal Approved Certificate request has been approved by cert-manager.io", + expectedEvent: "Normal cert-manager.io Certificate request has been approved by cert-manager.io", }, "approve CertificateRequest has 'Ready' Pending condition": { request: &cmapi.CertificateRequest{ @@ -158,12 +158,12 @@ func TestProcessItem(t *testing.T) { { Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: ApprovedMessage, LastTransitionTime: &metaNow, }, }, - expectedEvent: "Normal Approved Certificate request has been approved by cert-manager.io", + expectedEvent: "Normal cert-manager.io Certificate request has been approved by cert-manager.io", }, } for name, test := range tests { diff --git a/pkg/controller/certificaterequests/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index 25986051a..d3fd4f04f 100644 --- a/pkg/controller/certificaterequests/ca/ca_test.go +++ b/pkg/controller/certificaterequests/ca/ca_test.go @@ -139,7 +139,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, + Reason: "Foo", Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), @@ -148,7 +148,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: "Certificate request has been approved by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go index 08fd761c7..2eeea36aa 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go @@ -146,7 +146,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, + Reason: "Foo", Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), @@ -155,7 +155,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: "Certificate request has been approved by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), diff --git a/pkg/controller/certificaterequests/sync_test.go b/pkg/controller/certificaterequests/sync_test.go index a5c7e6866..45db57173 100644 --- a/pkg/controller/certificaterequests/sync_test.go +++ b/pkg/controller/certificaterequests/sync_test.go @@ -173,7 +173,7 @@ func TestSync(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, + Reason: "Foo", Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &nowMetaTime, }), diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index b32b8037a..520fdfb38 100644 --- a/pkg/controller/certificaterequests/vault/vault_test.go +++ b/pkg/controller/certificaterequests/vault/vault_test.go @@ -127,7 +127,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, + Reason: "Foo", Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), @@ -136,7 +136,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: "Certificate request has been approved by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), diff --git a/pkg/controller/certificaterequests/venafi/venafi_test.go b/pkg/controller/certificaterequests/venafi/venafi_test.go index ea326589f..c94d52759 100644 --- a/pkg/controller/certificaterequests/venafi/venafi_test.go +++ b/pkg/controller/certificaterequests/venafi/venafi_test.go @@ -143,7 +143,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, + Reason: "Foo", Message: "Certificate request has been denied by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), @@ -152,7 +152,7 @@ func TestSign(t *testing.T) { gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonApproved, + Reason: "cert-manager.io", Message: "Certificate request has been approved by cert-manager.io", LastTransitionTime: &metaFixedClockStart, }), diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index d129a3850..2a55400e8 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest.go @@ -144,7 +144,7 @@ func ValidateCertificateRequestApprovalCondition(crConds []cmapi.CertificateRequ break case 1: if condition := cond.conditions[0]; condition.Status != cmmeta.ConditionTrue { - el = append(el, field.Invalid(fldPath.Child(condition.Reason), condition.Status, + 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: diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index adcf36e3c..ab667837a 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -27,7 +27,6 @@ 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" @@ -115,318 +114,122 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { } func TestValidateCertificateRequestSpec(t *testing.T) { - fldPath := field.NewPath("spec") - fldPathConditions := field.NewPath("status", "conditions") + fldPath := field.NewPath("test") - tests := map[string]struct { - cr *cminternal.CertificateRequest - want field.ErrorList + tests := []struct { + name string + crSpec *cminternal.CertificateRequestSpec + want field.ErrorList }{ - "Test csr with no usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"))), - IssuerRef: validIssuerRef, - Usages: nil, - }, + { + name: "Test csr with no usages", + crSpec: &cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))), + IssuerRef: validIssuerRef, + Usages: nil, }, want: []*field.Error{}, }, - "Test csr with double signature usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageSigning, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, - }, + { + 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}, }, want: []*field.Error{}, }, - "Test csr with double extended usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", 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}, - }, + { + 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}, }, want: []*field.Error{}, }, - "Test csr with reordered usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", 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}, - }, + { + 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}, }, want: []*field.Error{}, }, - "Test csr that is CA with usages set": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", 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}, - }, + { + 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}, }, want: []*field.Error{}, }, - "Test csr that is CA but no cert sign in usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", 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}, - }, + { + 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}, }, want: []*field.Error{}, }, - "Error on csr not having all usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", 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}, - }, + { + 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}, }, 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]]"), }, }, - "Error on cr not having all usages": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))), - IssuerRef: validIssuerRef, - Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, - }, + { + 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}, }, 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]]"), }, }, - "Test csr with any, signing, digital signature, key encipherment, server and client auth": { - cr: &cminternal.CertificateRequest{ - Spec: cminternal.CertificateRequestSpec{ - Request: mustGenerateCSR(t, gen.Certificate("spec", 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}, - }, + { + 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}, }, 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: cmapi.CertificateRequestReasonApproved, - }, - }, - }, - }, - want: []*field.Error{ - field.Invalid(fldPathConditions.Child(cmapi.CertificateRequestReasonApproved), 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: cmapi.CertificateRequestReasonDenied, - }, - }, - }, - }, - want: []*field.Error{ - field.Invalid(fldPathConditions.Child(cmapi.CertificateRequestReasonDenied), 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: cmapi.CertificateRequestReasonApproved, - }, - { - Type: cminternal.CertificateRequestConditionDenied, - Status: cminternalmeta.ConditionFalse, - Reason: cmapi.CertificateRequestReasonDenied, - }, - }, - }, - }, - want: []*field.Error{ - field.Invalid(field.NewPath("status", "conditions", cmapi.CertificateRequestReasonApproved), nil, - `"Approved" condition may only be set to True`), - field.Invalid(field.NewPath("status", "conditions", cmapi.CertificateRequestReasonDenied), 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: cmapi.CertificateRequestReasonApproved, - }, - { - Type: cminternal.CertificateRequestConditionDenied, - Status: cminternalmeta.ConditionTrue, - Reason: cmapi.CertificateRequestReasonDenied, - }, - }, - }, - }, - 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: cmapi.CertificateRequestReasonApproved, - }, - { - 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: cmapi.CertificateRequestReasonDenied, - }, - { - Type: cminternal.CertificateRequestConditionDenied, - Status: cminternalmeta.ConditionFalse, - Reason: "foo", - }, - }, - }, - }, - want: []*field.Error{ - field.Forbidden(fldPathConditions, `multiple "Denied" conditions present`), - }, - }, } - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - got := ValidateCertificateRequest(nil, tt.cr) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ValidateCertificateRequestSpec(tt.crSpec, field.NewPath("test"), true) 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 - } + // 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("ValidateCertificateRequest() = %v, want %v", got, tt.want) + t.Errorf("ValidateCertificateRequestSpec() = %v, want %v", got, tt.want) } }) } @@ -458,29 +261,34 @@ func mustGenerateCSR(t *testing.T, crt *cmapi.Certificate) []byte { } func Test_patchDuplicateKeyUsage(t *testing.T) { - tests := map[string]struct { + tests := []struct { + name string usages []cminternal.KeyUsage want []cminternal.KeyUsage }{ - "Test single KU": { + { + name: "Test single KU", usages: []cminternal.KeyUsage{cminternal.UsageKeyEncipherment}, want: []cminternal.KeyUsage{cminternal.UsageKeyEncipherment}, }, - "Test UsageSigning": { + { + name: "Test UsageSigning", usages: []cminternal.KeyUsage{cminternal.UsageSigning}, want: []cminternal.KeyUsage{cminternal.UsageDigitalSignature}, }, - "Test multiple KU": { + { + name: "Test multiple KU", usages: []cminternal.KeyUsage{cminternal.UsageDigitalSignature, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, want: []cminternal.KeyUsage{cminternal.UsageDigitalSignature, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, }, - "Test double signing": { + { + name: "Test double signing", usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageDigitalSignature}, want: []cminternal.KeyUsage{cminternal.UsageDigitalSignature}, }, } - for name, tt := range tests { - t.Run(name, func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { if got := patchDuplicateKeyUsage(tt.usages); !reflect.DeepEqual(got, tt.want) { t.Errorf("patchDuplicateKeyUsage() = %v, want %v", got, tt.want) } From b9646a832e94df8975fcb146e2725e276dbce567 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 15 Mar 2021 16:28:26 +0000 Subject: [PATCH 19/20] Updates certificate request validation to use new signature Signed-off-by: joshvanl --- .../validation/certificaterequest_test.go | 692 +++++++++++++++--- 1 file changed, 606 insertions(+), 86 deletions(-) diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index ab667837a..08c3cfdb1 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 doens'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) } }) } From 65acf10858c1cc661222b8e4057359c680a4ce8a Mon Sep 17 00:00:00 2001 From: joshvanl Date: Wed, 17 Mar 2021 13:10:01 +0000 Subject: [PATCH 20/20] Don't log error output in approver when CertificateRequest is deleted Signed-off-by: joshvanl --- pkg/controller/certificaterequests/approver/approver.go | 5 ++++- .../apis/certmanager/validation/certificaterequest_test.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/controller/certificaterequests/approver/approver.go b/pkg/controller/certificaterequests/approver/approver.go index e7f8be7bd..7569583e1 100644 --- a/pkg/controller/certificaterequests/approver/approver.go +++ b/pkg/controller/certificaterequests/approver/approver.go @@ -18,6 +18,7 @@ package approver import ( "context" + "fmt" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -82,6 +83,8 @@ func (c *Controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin 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") @@ -90,7 +93,7 @@ func (c *Controller) ProcessItem(ctx context.Context, key string) error { cr, err := c.certificateRequestLister.CertificateRequests(namespace).Get(name) if apierrors.IsNotFound(err) { - log.Error(err, "certificate request in work queue no longer exists") + dbg.Info(fmt.Sprintf("certificate request in work queue no longer exists: %s", err)) return nil } diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index 08c3cfdb1..cad2f196f 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -136,7 +136,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, want: nil, }, - "CertificateRequest with single Denied=true condition that doens't change, shouldn't error": { + "CertificateRequest with single Denied=true condition that doesn't change, shouldn't error": { oldCR: &cminternal.CertificateRequest{ Spec: cminternal.CertificateRequestSpec{ Request: baseRequest,