Add logic to handle ready vs valid ACME orders

This commit is contained in:
James Munnelly 2018-08-08 10:33:34 +01:00
parent 1ed6855bde
commit 0dd3155fb2
2 changed files with 125 additions and 39 deletions

View File

@ -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

View File

@ -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")
}
}