diff --git a/design/20190708.certificate-request-crd.md b/design/20190708.certificate-request-crd.md index 4d2551abd..9b990c830 100644 --- a/design/20190708.certificate-request-crd.md +++ b/design/20190708.certificate-request-crd.md @@ -11,7 +11,7 @@ approvers: - "@munnerz" editor: "@joshvanl" creation-date: 2019-07-08 -last-updated: 2021-03-24 +last-updated: 2023-03-24 status: implementable --- @@ -52,6 +52,10 @@ status: implementable * [Version Skew Strategy](#version-skew-strategy) +:warning: Parts of this design are out of date with regards to the current implementation. + +See also https://cert-manager.io/docs/concepts/certificaterequest/. + ## Summary Currently, certificates issued via cert-manager rely on the `Certificate` @@ -223,11 +227,9 @@ implementation of the approver. For example, the name of the resource that approves this request, the violations which caused the request to be denied, or the team to who manually approved the request. -When a CertificateRequest has been Denied, it is the responsibility of the -referenced issuer to then add a Ready condition with the status of "False", -along with a relevant Reason and Message. - - +A CertificateRequest that is Denied is considered to be in a final, failed +state. If it was created for an issuance of a Certificate, the associated +issuance will be failed. ##### RBAC Approved and Denied conditions are set by requesting against the `/status` @@ -397,8 +399,13 @@ minimal as possible in that the single goal of them is to enable its owning `CertificateRequest` has been observed, the general flow is as follows: - Check the group belongs to the owning `Issuer`, exit if not. -- Check if `CertificateRequest` is in a failed state, exit if true. TODO: more - tightly define what a 'failed state' exactly is. +- Check if `CertificateRequest` is in a terminal failed state. + A controller may choose to add additional conditions to a failed `CertificateRequest`, but must not attempt to issue a certificate. + Currently terminal failed states are: + - `Ready` condition with a `Failed` reason // usually set by the issuer + - `InvalidRequest` condition with `True` status // usually set by the issuer + - `Denied` condition with `True` status // usually set by approver + - Check the `Issuer` type is of the same type, exit if not. - Verify the Spec of the `CertificateRequest`. - If a certificate exits then update the status if needed and exit. @@ -427,50 +434,21 @@ this resource. #### Issuing Controller -Since external issuers have been built before the addition of Approved and -Denied conditions, the issuing controller needs to be permissive. An external -issuer may not honour an Approved condition and will sign and set a -CertificateRequest as being Ready, before the request has been approved. The -issuing controller must mark issuance as being successful in this case. In -practice, this means that the issuing controller is never concerned with -Approved conditions. +Issuing controller considers all Denied CertificateRequests to be in a final failed state. +The issuance will be failed and will be repeatedly retried with an exponential backoff ../20220118.certificate-issuance-exponential-backoff.md. +If the cause of the denial was a misconfigured Certificate spec, the issuance will be retried immediately once the spec is corrected. +If the cause of the denial was misconfigured policy resources, a user who has fixed the resources and wants to retry immediately can do so using [cmctl renew](https://cert-manager.io/docs/reference/cmctl/#renew) -External issuers that do not honour Denied conditions will sign -CertificateRequests, even if they have a Denied condition set. In this case, the -issuing controller will successfully complete the issuance of the Certificate. - - -External Issuers and internal issuers that honour the Denied condition will -never sign CertificateRequests with the Denied condition set, and thus never set -Ready condition. In this case, the issuing controller will consider this -CertificateRequest as failed, and will set the condition `Issuing=False` -as well as setting the status field `lastFailureTime`. Note that the issuing -controller is not responsible for setting the `Ready=False` condition on -the CertificateRequests; that's the issuer's responsibility. - -- The Certificate is clearly reported as Failed to users who may miss the - Denied request from a cursory view. -- The Spec may genuinely be violating the policy, and so can be changed by the - user. This will cause an immediate reissue. -- The policy may be misconfigured, and as such, the Certificate will be retired - later with no user intervention. -- The [manual renew - command](https://cert-manager.io/docs/usage/kubectl-plugin/#renew) relies on - the Issuing condition. In the case of policy being misconfigured, the user - is able to immediately retry the request using the CLI plugin. +The issuing controller does not check Approved condition. It is the issuer's +responsibility not to issue certificates for CertificateRequests that have not +been approved. ### Failure -The `CertificateRequest` resource has a `FailureTime` field in its Status. If -the `CertificateRequest` fails for any reason then this field is set to the -current time. This field can then be used by a higher order controller, such as -the `Certificate` controller, to take further action and facilitate a backoff. - -The `Certificate` controller will retry all failed `CertificateRequest` resources -by creating a new request with an identical Spec, only when the `FailureTime` -field is a least 1 hour in the past. The old failed `CertificateRequest` will be -deleted and the new `CertificateRequest` resource will be created with the same -name. +A `CertificateRequest` is considered in a final failed state if: +- it has a Ready condition with Failed reason +- it has a Denied condition with True status +- it has InvalidRequest reason with True status ### Internal API Resource Behaviour diff --git a/pkg/controller/certificaterequests/sync.go b/pkg/controller/certificaterequests/sync.go index 5ba016351..1c806f99b 100644 --- a/pkg/controller/certificaterequests/sync.go +++ b/pkg/controller/certificaterequests/sync.go @@ -63,6 +63,12 @@ func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (er return nil } + // If CertificateRequest is invalid, do not process it + if apiutil.CertificateRequestHasInvalidRequest(cr) { + dbg.Info("certificate request is invalid and will not be further processed") + return nil + } + // If CertificateRequest has not been approved, exit early. if !apiutil.CertificateRequestIsApproved(cr) { dbg.Info("certificate request has not been approved") diff --git a/pkg/controller/certificaterequests/sync_test.go b/pkg/controller/certificaterequests/sync_test.go index cba4dcdb2..1ce6548d4 100644 --- a/pkg/controller/certificaterequests/sync_test.go +++ b/pkg/controller/certificaterequests/sync_test.go @@ -372,6 +372,21 @@ func TestSync(t *testing.T) { ExpectedActions: []testpkg.Action{}, }, }, + "should return nil (no action) if certificate request invalidrequest is set to true": { + certificateRequest: gen.CertificateRequestFrom(baseCRNotApproved, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "Certificate request is invalid", + LastTransitionTime: &nowMetaTime, + }), + ), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{baseIssuer, baseCR}, + ExpectedActions: []testpkg.Action{}, + }, + }, "should return nil (no action) if certificate request is ready and reason Issued": { certificateRequest: gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index 49816f435..ec2f9d4ec 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -279,25 +279,37 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return nil } - // Some issuers won't honor the "Denied=True" condition, and we don't want - // to break these issuers. To avoid breaking these issuers, we skip bubbling - // up the "Denied=True" condition from the certificate request object to the - // certificate object when the issuer ignores the "Denied" state. - // - // To know whether or not an issuer ignores the "Denied" state, we pay - // attention to the "Ready" condition on the certificate request. If a - // certificate request is "Denied=True" and that the issuer still proceeds - // to adding the "Ready" condition (to either true or false), then we - // consider that this issuer has ignored the "Denied" state. + // Now check if CertificateRequest is in any of the final states so that + // this issuance can be completed as either succeeded or failed. Failed + // issuance will be retried with a delay (the logic for that lives in + // certificates-trigger controller). Final states are: Denied condition + // with status True => fail issuance InvalidRequest condition with + // status True => fail issuance Ready conidtion with reason Failed => + // fail issuance Ready condition with reason Issued => finalize issuance + // as succeeded. + + // In case of a non-compliant issuer, a CertificateRequest can have both + // Denied status True (set by an approver) and Ready condition with + // reason Issued (set by the issuer). In this case, we prioritize the + // Denied condition and fail the issuance. This is done for consistency + // and also to avoid race conditions between the non-compliant issuer + // and this control loop. + + // If the certificate request was denied, set the last failure time to + // now, bump the issuance attempts and set the Issuing status condition + // to False. + if apiutil.CertificateRequestIsDenied(req) { + return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied)) + } + + // If the certificate request is invalid, set the last failure time to + // now, bump the issuance attempts and set the Issuing status condition + // to False. + if apiutil.CertificateRequestHasInvalidRequest(req) { + return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionInvalidRequest)) + } + if crReadyCond == nil { - if apiutil.CertificateRequestIsDenied(req) { - return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied)) - } - - if apiutil.CertificateRequestHasInvalidRequest(req) { - return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionInvalidRequest)) - } - log.V(logf.DebugLevel).Info("CertificateRequest does not have Ready condition, waiting...") return nil } diff --git a/pkg/controller/certificates/issuing/issuing_controller_test.go b/pkg/controller/certificates/issuing/issuing_controller_test.go index 2b911fed0..859f95986 100644 --- a/pkg/controller/certificates/issuing/issuing_controller_test.go +++ b/pkg/controller/certificates/issuing/issuing_controller_test.go @@ -1010,7 +1010,7 @@ func TestIssuingController(t *testing.T) { }, expectedErr: false, }, - "if certificate is in Issuing state, one CertificateRequest, but has been denied, report denial and set last failed time and issuance attempts": { + "if certificate is in Issuing state, one CertificateRequest without Ready condition, but with Denied condition, report denial and set last failed time and issuance attempts": { certificate: exampleBundle.Certificate, builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{ @@ -1062,6 +1062,290 @@ func TestIssuingController(t *testing.T) { }, expectedErr: false, }, + "if certificate is in Issuing state, one CertificateRequest with a pending Ready condition and a Denied condition, report denial and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "DeniedReason", + Message: "The certificate request has been denied", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: "Pending", + Message: "The certificate request is pending", + }), + )}, + KubeObjects: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nextPrivateKeySecretName, + Namespace: exampleBundle.Certificate.Namespace, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, + }, + }, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "DeniedReason", + Message: "The certificate request has failed to complete and will be retried: The certificate request has been denied", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning DeniedReason The certificate request has failed to complete and will be retried: The certificate request has been denied", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has been issued, but also has a Denied condition, report denial and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "DeniedReason", + Message: "The certificate request has been denied", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionTrue, + Reason: "Issued", + Message: "The certificate request has been issued", + }), + )}, + KubeObjects: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nextPrivateKeySecretName, + Namespace: exampleBundle.Certificate.Namespace, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, + }, + }, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "DeniedReason", + Message: "The certificate request has failed to complete and will be retried: The certificate request has been denied", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning DeniedReason The certificate request has failed to complete and will be retried: The certificate request has been denied", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has no ready condition and has been marked as invalid, report error and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "The certificate request is invalid", + }), + )}, + KubeObjects: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nextPrivateKeySecretName, + Namespace: exampleBundle.Certificate.Namespace, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, + }, + }, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "InvalidRequest", + Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has a pending ready condition and has been marked as invalid, report error and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "The certificate request is invalid", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: "Pending", + Message: "The certificate request is being issued", + }), + )}, + KubeObjects: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nextPrivateKeySecretName, + Namespace: exampleBundle.Certificate.Namespace, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, + }, + }, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "InvalidRequest", + Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has been issued, but has also been marked as invalid, report error and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "The certificate request is invalid", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionTrue, + Reason: "Issued", + Message: "The certificate request has been issued", + }), + )}, + KubeObjects: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nextPrivateKeySecretName, + Namespace: exampleBundle.Certificate.Namespace, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, + }, + }, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "InvalidRequest", + Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid", + }, + }, + expectedErr: false, + }, } for name, test := range tests {