diff --git a/README.md b/README.md index d155902a..5a0c83c6 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ The business logic of the controllers can be provided to the libary through the If it returns a normal error, the controller will retry with backoff until the `Check` function succeeds. If the error is of type `signer.PermanentError`, the controller will not retry automatically. Instead, an increase in Generation is required to recheck the issuer. -- The `Sign` function is used by the CertificateRequest controller. +- The `Sign` function is used by the CertificateRequest controller. If it returns a normal error, the `Sign` function will be retried as long as we have not spent more than the configured `MaxRetryDuration` after the certificate request was created. If the error is of type `signer.IssuerError`, the error is an error that should be set on the issuer instead of the CertificateRequest. If the error is of type `signer.SetCertificateRequestConditionError`, the controller will, additional to setting the ready condition, also set the specified condition. This can be used in case we have to store some additional state in the status. diff --git a/controllers/request_controller.go b/controllers/request_controller.go index 3ba6cada..6da7cefb 100644 --- a/controllers/request_controller.go +++ b/controllers/request_controller.go @@ -165,10 +165,10 @@ func (r *RequestController) reconcileStatusPatch( } // Select first matching issuer type and construct an issuerObject and issuerName - issuerObject, issuerName, err := r.matchIssuerType(requestObject) + issuerObject, issuerName, matchErr := r.matchIssuerType(requestObject) // Ignore Request if issuerRef doesn't match one of our issuer Types - if err != nil { - logger.V(1).Info("Request has a foreign issuer. Ignoring.", "error", err) + if matchErr != nil { + logger.V(1).Info("Request has a foreign issuer. Ignoring.", "error", matchErr) return result, nil, nil // done } issuerGvk := issuerObject.GetObjectKind().GroupVersionKind() @@ -287,8 +287,8 @@ func (r *RequestController) reconcileStatusPatch( return result, statusPatch, nil // apply patch, done } - signedCertificate, err := r.Sign(log.IntoContext(ctx, logger), requestObjectHelper.RequestObject(), issuerObject) - if err == nil { + signedCertificate, signErr := r.Sign(log.IntoContext(ctx, logger), requestObjectHelper.RequestObject(), issuerObject) + if signErr == nil { logger.V(1).Info("Successfully finished the reconciliation.") statusPatch.SetIssued(signedCertificate) @@ -297,7 +297,7 @@ func (r *RequestController) reconcileStatusPatch( // An error in the issuer part of the operator should trigger a reconcile // of the issuer's state. - if issuerError := new(signer.IssuerError); errors.As(err, issuerError) { + if issuerError := new(signer.IssuerError); errors.As(signErr, issuerError) { if reportError := r.EventSource.ReportError( issuerGvk, client.ObjectKeyFromObject(issuerObject), issuerError.Err, @@ -312,8 +312,8 @@ func (r *RequestController) reconcileStatusPatch( } didCustomConditionTransition := false - if targetCustom := new(signer.SetCertificateRequestConditionError); errors.As(err, targetCustom) { - logger.V(1).Info("Set RequestCondition error. Setting condition.", "error", err) + if targetCustom := new(signer.SetCertificateRequestConditionError); errors.As(signErr, targetCustom) { + logger.V(1).Info("Set RequestCondition error. Setting condition.", "error", signErr) didCustomConditionTransition = statusPatch.SetCustomCondition( string(targetCustom.ConditionType), metav1.ConditionStatus(targetCustom.Status), @@ -324,8 +324,8 @@ func (r *RequestController) reconcileStatusPatch( // Check if we have still time to requeue & retry pendingError := &signer.PendingError{} - isPending := errors.As(err, pendingError) - isPermanentError := errors.As(err, &signer.PermanentError{}) + isPending := errors.As(signErr, pendingError) + isPermanentError := errors.As(signErr, &signer.PermanentError{}) pastMaxRetryDuration := r.Clock.Now().After(requestObject.GetCreationTimestamp().Add(r.MaxRetryDuration)) switch { case isPending: @@ -335,8 +335,8 @@ func (r *RequestController) reconcileStatusPatch( // it isn't an error. It just means that we should poll again later. // Its message gives the reason why the signing process is still in // progress. Thus, we don't log any error. - logger.V(1).WithValues("reason", err.Error()).Info("Signing in progress.") - statusPatch.SetPending(fmt.Sprintf("Signing still in progress. Reason: %s", err)) + logger.V(1).WithValues("reason", signErr.Error()).Info("Signing in progress.") + statusPatch.SetPending(fmt.Sprintf("Signing still in progress. Reason: %s", signErr)) // Let's not trigger an unnecessary reconciliation when we know that the // user-defined condition was changed and will trigger a reconciliation. @@ -351,24 +351,24 @@ func (r *RequestController) reconcileStatusPatch( return result, statusPatch, nil // apply patch, requeue with backoff } case isPermanentError: - logger.V(1).Error(err, "Permanent Request error. Marking as failed.") - statusPatch.SetPermanentError(err) - return result, statusPatch, reconcile.TerminalError(err) // apply patch, done + logger.V(1).Error(signErr, "Permanent Request error. Marking as failed.") + statusPatch.SetPermanentError(signErr) + return result, statusPatch, reconcile.TerminalError(signErr) // apply patch, done case pastMaxRetryDuration: - logger.V(1).Error(err, "Request has been retried for too long. Marking as failed.") - statusPatch.SetPermanentError(err) - return result, statusPatch, reconcile.TerminalError(err) // apply patch, done + logger.V(1).Error(signErr, "Request has been retried for too long. Marking as failed.") + statusPatch.SetPermanentError(signErr) + return result, statusPatch, reconcile.TerminalError(signErr) // apply patch, done default: // We consider all the other errors as being retryable. - logger.V(1).Error(err, "Got an error, will be retried.") - statusPatch.SetRetryableError(err) + logger.V(1).Error(signErr, "Got an error, will be retried.") + statusPatch.SetRetryableError(signErr) // Let's not trigger an unnecessary reconciliation when we know that the // user-defined condition was changed and will trigger a reconciliation. if didCustomConditionTransition { - return result, statusPatch, reconcile.TerminalError(err) // apply patch, done + return result, statusPatch, reconcile.TerminalError(signErr) // apply patch, done } else { - return result, statusPatch, err // apply patch, requeue with backoff + return result, statusPatch, signErr // apply patch, requeue with backoff } } } diff --git a/controllers/signer/certificate_request_object.go b/controllers/signer/certificate_request_object.go new file mode 100644 index 00000000..a7f00af6 --- /dev/null +++ b/controllers/signer/certificate_request_object.go @@ -0,0 +1,166 @@ +/* +Copyright 2023 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 signer + +import ( + "crypto/x509" + "time" + + apiutil "github.com/cert-manager/cert-manager/pkg/api/util" + cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + experimentalapi "github.com/cert-manager/cert-manager/pkg/apis/experimental/v1alpha1" + "github.com/cert-manager/cert-manager/pkg/util/pki" + certificatesv1 "k8s.io/api/certificates/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// CertificateRequestObject represents either a cert-manager CertificateRequest +// or a Kubernetes CertificateSigningRequest resource. The interface hides the +// underlying spec fields and exposes a certificate template and the raw CSR +// bytes. This lets the signer be agnostic to the underlying resource type and +// to how spec fields are interpreted (for example, defaulting logic). The +// signer can still access labels, annotations, or other metadata, and can use +// `GetConditions` to retrieve the resource's conditions. +type CertificateRequestObject interface { + metav1.Object + + // Return the Certificate details originating from the cert-manager + // CertificateRequest or Kubernetes CertificateSigningRequest resources. + GetCertificateDetails() (details CertificateDetails, err error) + + GetConditions() []metav1.Condition +} + +type CertificateDetails struct { + CSR []byte + Duration time.Duration + IsCA bool + MaxPathLen *int + KeyUsage x509.KeyUsage + ExtKeyUsage []x509.ExtKeyUsage +} + +// CertificateTemplate generates a certificate template for issuance, +// based on CertificateDetails extracted from the CertificateRequest or +// CertificateSigningRequest resource. +// +// This function internally calls CertificateTemplateFromCSRPEM, which performs +// additional work such as parsing the CSR and verifying signatures. Since this +// operation can be expensive, issuer implementations should call this function +// only when a certificate template is actually needed (e.g., not when proxying +// the X.509 CSR to a CA). +func (cd CertificateDetails) CertificateTemplate() (template *x509.Certificate, err error) { + return pki.CertificateTemplateFromCSRPEM( + cd.CSR, + pki.CertificateTemplateOverrideDuration(cd.Duration), + pki.CertificateTemplateValidateAndOverrideBasicConstraints(cd.IsCA, cd.MaxPathLen), // Override the basic constraints, but make sure they match the constraints in the CSR if present + pki.CertificateTemplateValidateAndOverrideKeyUsages(cd.KeyUsage, cd.ExtKeyUsage), // Override the key usages, but make sure they match the usages in the CSR if present + ) +} + +type certificateRequestImpl struct { + *cmapi.CertificateRequest +} + +var _ CertificateRequestObject = &certificateRequestImpl{} + +func CertificateRequestObjectFromCertificateRequest(cr *cmapi.CertificateRequest) CertificateRequestObject { + return &certificateRequestImpl{cr} +} + +func (c *certificateRequestImpl) GetCertificateDetails() (CertificateDetails, error) { + duration := apiutil.DefaultCertDuration(c.Spec.Duration) + + keyUsage, extKeyUsage, err := pki.KeyUsagesForCertificateOrCertificateRequest(c.Spec.Usages, c.Spec.IsCA) + if err != nil { + return CertificateDetails{}, err + } + + return CertificateDetails{ + CSR: c.Spec.Request, + Duration: duration, + IsCA: c.Spec.IsCA, + MaxPathLen: nil, + KeyUsage: keyUsage, + ExtKeyUsage: extKeyUsage, + }, nil +} + +func (c *certificateRequestImpl) GetConditions() []metav1.Condition { + conditions := make([]metav1.Condition, 0, len(c.Status.Conditions)) + for _, condition := range c.Status.Conditions { + var lastTransition metav1.Time + if lt := condition.LastTransitionTime; lt != nil { + lastTransition = *lt + } + conditions = append(conditions, metav1.Condition{ + Type: string(condition.Type), + Status: metav1.ConditionStatus(condition.Status), + LastTransitionTime: lastTransition, + Reason: condition.Reason, + Message: condition.Message, + }) + } + return conditions +} + +type certificateSigningRequestImpl struct { + *certificatesv1.CertificateSigningRequest +} + +var _ CertificateRequestObject = &certificateSigningRequestImpl{} + +func CertificateRequestObjectFromCertificateSigningRequest(csr *certificatesv1.CertificateSigningRequest) CertificateRequestObject { + return &certificateSigningRequestImpl{csr} +} + +func (c *certificateSigningRequestImpl) GetCertificateDetails() (CertificateDetails, error) { + duration, err := pki.DurationFromCertificateSigningRequest(c.CertificateSigningRequest) + if err != nil { + return CertificateDetails{}, err + } + + isCA := c.CertificateSigningRequest.Annotations[experimentalapi.CertificateSigningRequestIsCAAnnotationKey] == "true" + + keyUsage, extKeyUsage, err := pki.BuildKeyUsagesKube(c.Spec.Usages) + if err != nil { + return CertificateDetails{}, err + } + + return CertificateDetails{ + CSR: c.Spec.Request, + Duration: duration, + IsCA: isCA, + MaxPathLen: nil, + KeyUsage: keyUsage, + ExtKeyUsage: extKeyUsage, + }, nil +} + +func (c *certificateSigningRequestImpl) GetConditions() []metav1.Condition { + conditions := make([]metav1.Condition, 0, len(c.Status.Conditions)) + for _, condition := range c.Status.Conditions { + conditions = append(conditions, metav1.Condition{ + Type: string(condition.Type), + Status: metav1.ConditionStatus(condition.Status), + LastTransitionTime: condition.LastTransitionTime, + Reason: condition.Reason, + Message: condition.Message, + }) + } + return conditions +} diff --git a/controllers/signer/err_issuer.go b/controllers/signer/err_issuer.go index d88e484f..0f78e164 100644 --- a/controllers/signer/err_issuer.go +++ b/controllers/signer/err_issuer.go @@ -16,15 +16,13 @@ limitations under the License. package signer -// IssuerError is thrown by the CertificateRequest controller -// to indicate that er was an error in the issuer part of the -// reconcile process, and that the issuer's reconcile function -// should be retriggered. +// IssuerError is returned by the CertificateRequest controller to indicate +// there was an error in the issuer part of the reconcile process and that +// the issuer's reconcile function should be retried. // -// This error is useful to indicate that the Sign function got -// an error for an action that should have been checked by the -// Check function, and that has appeared after the Check function -// has been called. +// This error is useful when the Sign function encounters an error for an +// action that should have been handled by the Check function, and which +// surfaced after Check had already succeeded. // // > This error should be returned only by the Sign function. type IssuerError struct { diff --git a/controllers/signer/err_pending.go b/controllers/signer/err_pending.go index 9cd27cb9..b882afa9 100644 --- a/controllers/signer/err_pending.go +++ b/controllers/signer/err_pending.go @@ -18,21 +18,19 @@ package signer import "time" -// PendingError should be returned if we are certain that we will converge to a -// successful result or another type of error in a finite amount of time by -// just retrying the same operation. +// PendingError should be returned when retrying the same operation is expected +// to result in either success or another error within a finite time. // -// It can be used to circumvent the MaxRetryDuration -// check, which is useful for example when the signer is waiting for an async -// answer from an external service that is indicating that the request is still -// being processed. +// It can be used to bypass the MaxRetryDuration check, for example when the +// signer is waiting for an asynchronous response from an external service +// indicating the request is still being processed. // // > This error should be returned only by the Sign function. type PendingError struct { Err error - // RequeueAfter can be used to specify how long to wait before retrying. By default - // we wait for 1s before retrying. + // RequeueAfter can be set to control how long to wait before retrying. By + // default the controller waits 1s before retrying. RequeueAfter time.Duration } diff --git a/controllers/signer/err_permanent.go b/controllers/signer/err_permanent.go index 5c8c6fdb..eed79c71 100644 --- a/controllers/signer/err_permanent.go +++ b/controllers/signer/err_permanent.go @@ -16,19 +16,18 @@ limitations under the License. package signer -// PermanentError is returned if it is impossible for the resource to -// get in a Ready state without being changed. It should not be used -// if there is any way to fix the error by altering the environment/ -// other resources. The client should not try again after receiving -// this error. +// PermanentError is returned when it is impossible for the resource to +// become Ready without changing the resource itself. It must not be used +// when the issue can be resolved by modifying the environment or other +// resources. The controller should not retry after receiving this error. // // For the Check function, this error is useful when we detected an // invalid configuration/ setting in the Issuer or ClusterIssuer resource. // This should only happen very rarely, because of webhook validation. // -// For the Sign function, this error is useful when we detected an -// error that will only get resolved by creating a new CertificateRequest, -// for example when it is required to craft a new CSR. +// For the Sign function, this error is useful when the problem can only be +// resolved by creating a new CertificateRequest (for example, when a new +// CSR must be generated). // // > This error should be returned by the Sign or Check function. type PermanentError struct { diff --git a/controllers/signer/interface.go b/controllers/signer/interface.go index c940f6c4..7b938392 100644 --- a/controllers/signer/interface.go +++ b/controllers/signer/interface.go @@ -18,12 +18,8 @@ package signer import ( "context" - "crypto/x509" - "time" - cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/cert-manager/cert-manager/pkg/util/pki" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -34,64 +30,15 @@ import ( // The first certificate in the ChainPEM chain is the leaf certificate, and the // last certificate in the chain is the highest level non-self-signed certificate. // The CAPEM certificate is our best guess at the CA that issued the leaf. -// IMORTANT: the CAPEM certificate is only used when the SetCAOnCertificateRequest -// option is enabled in the controller. This option is for backwards compatibility -// only. The use of the CA field and the ca.crt field in the resulting Secret is -// discouraged, instead the CA should be provisioned separately (e.g. using trust-manager). +// IMPORTANT: the CAPEM certificate is only used when the SetCAOnCertificateRequest +// option is enabled in the controller. This option exists for backwards compatibility +// only. Use of the CA field and the `ca.crt` field in the resulting Secret is +// discouraged; the CA should instead be provisioned separately (for example, using trust-manager). type PEMBundle pki.PEMBundle type Sign func(ctx context.Context, cr CertificateRequestObject, issuerObject v1alpha1.Issuer) (PEMBundle, error) type Check func(ctx context.Context, issuerObject v1alpha1.Issuer) error -// CertificateRequestObject is an interface that represents either a -// cert-manager CertificateRequest or a Kubernetes CertificateSigningRequest -// resource. This interface hides the spec fields of the underlying resource -// and exposes a Certificate template and the raw CSR bytes instead. This -// allows the signer to be agnostic of the underlying resource type and also -// agnostic of the way the spec fields should be interpreted, such as the -// defaulting logic that is applied to it. It is still possible to access the -// labels and annotations of the underlying resource or any other metadata -// fields that might be useful to the signer. Also, the signer can use the -// GetConditions method to retrieve the conditions of the underlying resource. -// To update the conditions, the special error "SetCertificateRequestConditionError" -// can be returned from the Sign method. -type CertificateRequestObject interface { - metav1.Object - - // Return the Certificate details originating from the cert-manager - // CertificateRequest or Kubernetes CertificateSigningRequest resources. - GetCertificateDetails() (details CertificateDetails, err error) - - GetConditions() []cmapi.CertificateRequestCondition -} - -type CertificateDetails struct { - CSR []byte - Duration time.Duration - IsCA bool - MaxPathLen *int - KeyUsage x509.KeyUsage - ExtKeyUsage []x509.ExtKeyUsage -} - -// CertificateTemplate generates a certificate template for issuance, -// based on CertificateDetails extracted from the CertificateRequest or -// CertificateSigningRequest resource. -// -// This function internally calls CertificateTemplateFromCSRPEM, which performs -// additional work such as parsing the CSR and verifying signatures. Since this -// operation can be expensive, issuer implementations should call this function -// only when a certificate template is actually needed (e.g., not when proxying -// the X.509 CSR to a CA). -func (cd CertificateDetails) CertificateTemplate() (template *x509.Certificate, err error) { - return pki.CertificateTemplateFromCSRPEM( - cd.CSR, - pki.CertificateTemplateOverrideDuration(cd.Duration), - pki.CertificateTemplateValidateAndOverrideBasicConstraints(cd.IsCA, cd.MaxPathLen), // Override the basic constraints, but make sure they match the constraints in the CSR if present - pki.CertificateTemplateValidateAndOverrideKeyUsages(cd.KeyUsage, cd.ExtKeyUsage), // Override the key usages, but make sure they match the usages in the CSR if present - ) -} - // IgnoreIssuer is an optional function that can prevent the issuer controllers from // reconciling an issuer resource. By default, the controllers will reconcile all // issuer resources that match the owned types. diff --git a/controllers/signer/interface_implementations.go b/controllers/signer/interface_implementations.go deleted file mode 100644 index 22b0e803..00000000 --- a/controllers/signer/interface_implementations.go +++ /dev/null @@ -1,106 +0,0 @@ -/* -Copyright 2023 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 signer - -import ( - apiutil "github.com/cert-manager/cert-manager/pkg/api/util" - cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - experimentalapi "github.com/cert-manager/cert-manager/pkg/apis/experimental/v1alpha1" - cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" - "github.com/cert-manager/cert-manager/pkg/util/pki" - certificatesv1 "k8s.io/api/certificates/v1" -) - -type certificateRequestImpl struct { - *cmapi.CertificateRequest -} - -var _ CertificateRequestObject = &certificateRequestImpl{} - -func CertificateRequestObjectFromCertificateRequest(cr *cmapi.CertificateRequest) CertificateRequestObject { - return &certificateRequestImpl{cr} -} - -func (c *certificateRequestImpl) GetCertificateDetails() (CertificateDetails, error) { - duration := apiutil.DefaultCertDuration(c.Spec.Duration) - - keyUsage, extKeyUsage, err := pki.KeyUsagesForCertificateOrCertificateRequest(c.Spec.Usages, c.Spec.IsCA) - if err != nil { - return CertificateDetails{}, err - } - - return CertificateDetails{ - CSR: c.Spec.Request, - Duration: duration, - IsCA: c.Spec.IsCA, - MaxPathLen: nil, - KeyUsage: keyUsage, - ExtKeyUsage: extKeyUsage, - }, nil -} - -func (c *certificateRequestImpl) GetConditions() []cmapi.CertificateRequestCondition { - return c.Status.Conditions -} - -type certificateSigningRequestImpl struct { - *certificatesv1.CertificateSigningRequest -} - -var _ CertificateRequestObject = &certificateSigningRequestImpl{} - -func CertificateRequestObjectFromCertificateSigningRequest(csr *certificatesv1.CertificateSigningRequest) CertificateRequestObject { - return &certificateSigningRequestImpl{csr} -} - -func (c *certificateSigningRequestImpl) GetCertificateDetails() (CertificateDetails, error) { - duration, err := pki.DurationFromCertificateSigningRequest(c.CertificateSigningRequest) - if err != nil { - return CertificateDetails{}, err - } - - isCA := c.CertificateSigningRequest.Annotations[experimentalapi.CertificateSigningRequestIsCAAnnotationKey] == "true" - - keyUsage, extKeyUsage, err := pki.BuildKeyUsagesKube(c.Spec.Usages) - if err != nil { - return CertificateDetails{}, err - } - - return CertificateDetails{ - CSR: c.Spec.Request, - Duration: duration, - IsCA: isCA, - MaxPathLen: nil, - KeyUsage: keyUsage, - ExtKeyUsage: extKeyUsage, - }, nil -} - -func (c *certificateSigningRequestImpl) GetConditions() []cmapi.CertificateRequestCondition { - conditions := make([]cmapi.CertificateRequestCondition, 0, len(c.Status.Conditions)) - for _, condition := range c.Status.Conditions { - lastTransition := condition.LastTransitionTime - conditions = append(conditions, cmapi.CertificateRequestCondition{ - Type: cmapi.CertificateRequestConditionType(condition.Type), - Status: cmmeta.ConditionStatus(condition.Status), - LastTransitionTime: &lastTransition, - Reason: condition.Reason, - Message: condition.Message, - }) - } - return conditions -}