diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 5024b5a0c..036890a26 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -3,6 +3,8 @@ package acme import ( "bytes" "context" + "crypto" + "crypto/x509" "encoding/pem" "fmt" @@ -48,14 +50,8 @@ func (a *Acme) obtainCertificate(ctx context.Context, crt *v1alpha1.Certificate) return nil, nil, fmt.Errorf("error getting order details: %v", err) } - if order.Status != acmeapi.StatusReady { - err := fmt.Errorf("expected certificate status to be %q, but it is %q", acmeapi.StatusReady, order.Status) - // print a more helpful message to users when an order is marked 'valid'. - // this happens when all challenges have been completed successfully, but - // the acme server has not finished processing the order. - if order.Status == acmeapi.StatusValid { - err = fmt.Errorf("%v. Waiting until Order transitions into %q state", err, acmeapi.StatusReady) - } + if order.Status != acmeapi.StatusReady && order.Status != acmeapi.StatusValid { + err := fmt.Errorf("expected certificate status to be %q or %q, but it is %q", acmeapi.StatusReady, acmeapi.StatusValid, order.Status) crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, err.Error(), false) return nil, nil, err } @@ -63,6 +59,14 @@ func (a *Acme) obtainCertificate(ctx context.Context, crt *v1alpha1.Certificate) // get existing certificate private key key, err := kube.SecretTLSKey(a.secretsLister, crt.Namespace, crt.Spec.SecretName) if k8sErrors.IsNotFound(err) || errors.IsInvalidData(err) { + if order.Status == acmeapi.StatusValid { + // if this order is already marked 'valid', but we do not have the private + // key for the Certificate, we must create a new order as we cannot call + // Finalize on the order twice with different CSRs. + crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, "Creating new order due to lost TLS private key", false) + crt.Status.ACMEStatus().Order.URL = "" + return nil, nil, fmt.Errorf("marking certificate as failed to trigger a new order to be created due to lost private key") + } key, err = pki.GeneratePrivateKeyForCertificate(crt) if err != nil { crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, fmt.Sprintf("Failed to generate certificate private key: %v", err), false) @@ -74,38 +78,88 @@ func (a *Acme) obtainCertificate(ctx context.Context, crt *v1alpha1.Certificate) return nil, nil, fmt.Errorf("error getting certificate private key: %s", err.Error()) } - // generate a csr - template, err := pki.GenerateCSR(a.issuer, crt) - if err != nil { - // TODO: this should probably be classed as a permanant failure - return nil, nil, err - } - - derBytes, err := pki.EncodeCSR(template, key) - if err != nil { - crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, fmt.Sprintf("Failed to generate certificate request: %v", err), false) - return nil, nil, err - } - - // obtain a certificate from the acme server - certSlice, err := cl.FinalizeOrder(ctx, order.FinalizeURL, derBytes) - if err != nil { - // this handles an edge case where a certificate ends out with an order - // that is in an invalid state. - // ideally we would instead call GetCertificate on the ACME client - // instead of FinalizeOrder, which would save us creating a new order - // just to issue a new certificate. - // The underlying ACME client doesn't expose this though yet. - if acmeErr, ok := err.(*acmeapi.Error); ok { - if acmeErr.StatusCode >= 400 && acmeErr.StatusCode <= 499 { - crt.Status.ACMEStatus().Order.URL = "" - } + var certSlice [][]byte + // StatusReady indicates that the order is ready to be finalized. + // Once the order has been finalized, it will transition into the 'valid' + // state. + // The only way to obtain a certificate from an already 'valid' order is to + // call the orders GetCertificate function. + // You can see we do this below *iff* certSlice is nil. + if order.Status == acmeapi.StatusReady { + // generate a csr + template, err := pki.GenerateCSR(a.issuer, crt) + if err != nil { + // TODO: this should probably be classed as a permanant failure + return nil, nil, err } - // TODO: should we also set the FailedValidation status - // condition here so back off can be applied? - crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, fmt.Sprintf("Failed to finalize order: %v", err), false) - a.Recorder.Eventf(crt, corev1.EventTypeWarning, errorIssueError, "Failed to finalize order: %v", err) - return nil, nil, fmt.Errorf("error getting certificate from acme server: %s", err) + + derBytes, err := pki.EncodeCSR(template, key) + if err != nil { + crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, fmt.Sprintf("Failed to generate certificate request: %v", err), false) + return nil, nil, err + } + + // obtain a certificate from the acme server + certSlice, err = cl.FinalizeOrder(ctx, order.FinalizeURL, derBytes) + if err != nil { + // this handles an edge case where a certificate ends out with an order + // that is in an invalid state. + // ideally we would instead call GetCertificate on the ACME client + // instead of FinalizeOrder, which would save us creating a new order + // just to issue a new certificate. + // The underlying ACME client doesn't expose this though yet. + if acmeErr, ok := err.(*acmeapi.Error); ok { + if acmeErr.StatusCode >= 400 && acmeErr.StatusCode <= 499 { + crt.Status.ACMEStatus().Order.URL = "" + } + } + // TODO: should we also set the FailedValidation status + // condition here so back off can be applied? + crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, fmt.Sprintf("Failed to finalize order: %v", err), false) + a.Recorder.Eventf(crt, corev1.EventTypeWarning, errorIssueError, "Failed to finalize order: %v", err) + return nil, nil, fmt.Errorf("error getting certificate from acme server: %s", err) + } + + } + + // if the Certificate was marked 'valid', we need to retrieve the certificate + // from the URL specified on the Order resource. + if certSlice == nil { + certSlice, err = cl.GetCertificate(ctx, order.CertificateURL) + if err != nil { + crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, fmt.Sprintf("Failed to retrieve certificate: %v", err), false) + return nil, nil, fmt.Errorf("error retrieving certificate: %v", err) + } + } + + if len(certSlice) == 0 { + return nil, nil, fmt.Errorf("invalid certificate returned from acme server") + } + + x509Cert, err := x509.ParseCertificate(certSlice[0]) + if err != nil { + return nil, nil, fmt.Errorf("failed to parse returned x509 certificate: %v", err.Error()) + } + + if a.Context.IssuerOptions.CertificateNeedsRenew(x509Cert) { + // existing orders certificate is near expiry, so we clear the order URL + // field and trigger a retry to create a new order for a new certificate + crt.Status.ACMEStatus().Order.URL = "" + return nil, nil, fmt.Errorf("triggering new order creation as existing order's certificate is nearing expiry") + } + + cryptoSigner, ok := key.(crypto.Signer) + if !ok { + return nil, nil, fmt.Errorf("unreachable: private key is not of type crypto.Signer") + } + validForPrivKey, err := pki.PublicKeyMatchesCertificate(cryptoSigner.Public(), x509Cert) + if err != nil { + return nil, nil, fmt.Errorf("error verifying certificate: %v", err) + } + if !validForPrivKey { + crt.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorIssueError, "Creating new order due to lost TLS private key", false) + crt.Status.ACMEStatus().Order.URL = "" + return nil, nil, fmt.Errorf("marking certificate as failed to trigger a new order to be created due to lost private key") } // encode the retrieved certificate diff --git a/pkg/util/pki/generate.go b/pkg/util/pki/generate.go index 4b1d5b7cb..b013b7a6f 100644 --- a/pkg/util/pki/generate.go +++ b/pkg/util/pki/generate.go @@ -112,3 +112,35 @@ func PublicKeyForPrivateKey(pk crypto.PrivateKey) (crypto.PublicKey, error) { return nil, fmt.Errorf("unknown private key type: %T", pk) } } + +// PublicKeyMatchesCertificate can be used to verify the given public key +// is the correct counter-part to the given x509 Certificate. +// It will return false and no error if the public key is *not* valid for the +// given Certificate. +// It will return true if the public key *is* valid for the given Certificate. +// It will return an error if either of the passed parameters are of an +// unrecognised type (i.e. non RSA/ECDSA) +func PublicKeyMatchesCertificate(check crypto.PublicKey, crt *x509.Certificate) (bool, error) { + switch pub := crt.PublicKey.(type) { + case *rsa.PublicKey: + rsaCheck, ok := check.(*rsa.PublicKey) + if !ok { + return false, nil + } + if pub.N.Cmp(rsaCheck.N) != 0 { + return false, nil + } + return true, nil + case *ecdsa.PublicKey: + ecdsaCheck, ok := check.(*ecdsa.PublicKey) + if !ok { + return false, nil + } + if pub.X.Cmp(ecdsaCheck.X) != 0 || pub.Y.Cmp(ecdsaCheck.Y) != 0 { + return false, nil + } + return true, nil + default: + return false, fmt.Errorf("unrecognised Certificate public key type") + } +}