diff --git a/pkg/controller/certificaterequests/acme/BUILD.bazel b/pkg/controller/certificaterequests/acme/BUILD.bazel index 74eff8bb9..6e7536b68 100644 --- a/pkg/controller/certificaterequests/acme/BUILD.bazel +++ b/pkg/controller/certificaterequests/acme/BUILD.bazel @@ -18,6 +18,7 @@ go_library( "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", "//pkg/util:go_default_library", + "//pkg/util/errors:go_default_library", "//pkg/util/pki:go_default_library", "@io_k8s_apimachinery//pkg/api/errors:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index ef2f23caf..314051207 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -39,6 +39,7 @@ import ( issuerpkg "github.com/jetstack/cert-manager/pkg/issuer" logf "github.com/jetstack/cert-manager/pkg/logs" "github.com/jetstack/cert-manager/pkg/util" + "github.com/jetstack/cert-manager/pkg/util/errors" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -137,7 +138,6 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer return nil, nil } - if err != nil { // We are probably in a network error here so we should backoff and retry message := fmt.Sprintf("Failed to get order resource %s/%s", expectedOrder.Namespace, expectedOrder.Name) @@ -147,6 +147,15 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer return nil, err } + if !metav1.IsControlledBy(order, cr) { + // TODO: improve this behaviour - this issue occurs because someone + // else may create a CertificateRequest with a name that is equal to + // the name of the request we are creating, due to our hash function + // not account for parameters stored on the CSR (i.e. the public key). + // We should improve the way we hash input data or somehow avoid + // relying on deterministic names for Order resources. + return nil, fmt.Errorf("found Order resource not owned by this CertificateRequest, retrying") + } log = logf.WithRelatedResource(log, order) @@ -163,6 +172,20 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer // Order valid, return cert. The calling controller will update with ready if its happy with the cert. if order.Status.State == cmacme.Valid { + x509Cert, err := pki.DecodeX509CertificateBytes(order.Status.Certificate) + if errors.IsInvalidData(err) { + log.Error(err, "failed to decode x509 certificate data on Order resource") + return nil, a.acmeClientV.Orders(order.Namespace).Delete(order.Name, nil) + } + ok, err := pki.PublicKeyMatchesCertificate(csr.PublicKey, x509Cert) + if err != nil { + return nil, err + } + if !ok { + log.Error(err, "failed to decode x509 certificate data on Order resource, recreating...") + return nil, a.acmeClientV.Orders(order.Namespace).Delete(order.Name, nil) + } + log.Info("certificate issued") return &issuerpkg.IssueResponse{ diff --git a/pkg/controller/certificates/sync.go b/pkg/controller/certificates/sync.go index ce0680b4a..4f1cf322a 100644 --- a/pkg/controller/certificates/sync.go +++ b/pkg/controller/certificates/sync.go @@ -384,7 +384,10 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt // Validate the CertificateRequest's CSR is valid log.Info("validating existing CSR data") x509CSR, err := pki.DecodeX509CertificateRequestBytes(existingReq.Spec.CSRPEM) - // TODO: handle InvalidData + if errors.IsInvalidData(err) { + log.Info("failed to decode existing CSR on CertificateRequest, deleting resource...") + return c.cmClient.CertmanagerV1alpha2().CertificateRequests(existingReq.Namespace).Delete(existingReq.Name, nil) + } if err != nil { return err } @@ -466,6 +469,16 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt return err } + log.Info("checking stored private key is valid for stored x509 certificate on CertificateRequest") + publicKeyMatches, err := pki.PublicKeyMatchesCertificate(privateKey.Public(), x509Cert) + if err != nil { + return err + } + if !publicKeyMatches { + log.Info("private key stored in Secret does not match public key of issued certificate, deleting the old CertificateRequest resource") + return c.cmClient.CertmanagerV1alpha2().CertificateRequests(existingReq.Namespace).Delete(existingReq.Name, nil) + } + // Check if the Certificate requires renewal according to the renewBefore // specified on the Certificate resource. log.Info("checking if certificate stored on CertificateRequest is up to date")