Store a copy of the signed certificate on the Order resource after Finalize

Signed-off-by: James Munnelly <james@munnelly.eu>

docs/generated/reference/output/reference/api-docs/index.html

@@ -421,8 +421,8 @@ Appears In:
 </thead>
 <tbody>
 <tr>
-<td><code>certificateURL</code><br /> <em>string</em></td>
+<td><code>certificate</code><br /> <em>string</em></td>
-<td>CertificateURL is a URL that can be used to retrieve a copy of the signed TLS certificate for this order. It will be populated automatically once the order has completed successfully and the certificate is available for retrieval.</td>
+<td>Certificate is a copy of the PEM encoded certificate for this Order. This field will be populated after the order has been successfully finalized with the ACME server, and the order has transitioned to the &#39;valid&#39; state.</td>
 </tr>
 <tr>
 <td><code>challenges</code><br /> <em><a href="#challengespec-v1alpha1">ChallengeSpec</a> array</em></td>
@@ -430,7 +430,7 @@ Appears In:
 </tr>
 <tr>
 <td><code>failureTime</code><br /> <em><a href="#time-v1">Time</a></em></td>
-<td>FailureTime stores the time that this order failed. This is used to influence garbage collection and back-off. The order resource will be automatically deleted after 30 minutes has passed since the failure time.</td>
+<td>FailureTime stores the time that this order failed. This is used to influence garbage collection and back-off.</td>
 </tr>
 <tr>
 <td><code>finalizeURL</code><br /> <em>string</em></td>

pkg/apis/certmanager/v1alpha1/types_order.go

@@ -91,12 +91,11 @@ type OrderStatus struct {
 	// This is used to obtain certificates for this order once it has been completed.
 	FinalizeURL string `json:"finalizeURL"`
 
-	// CertificateURL is a URL that can be used to retrieve a copy of the signed
+	// Certificate is a copy of the PEM encoded certificate for this Order.
-	// TLS certificate for this order.
+	// This field will be populated after the order has been successfully
-	// It will be populated automatically once the order has completed successfully
+	// finalized with the ACME server, and the order has transitioned to the
-	// and the certificate is available for retrieval.
+	// 'valid' state.
-	// +optional
+	Certificate []byte `json:"certificate"`
-	CertificateURL string `json:"certificateURL,omitempty"`
 
 	// State contains the current state of this Order resource.
 	// States 'success' and 'expired' are 'final'
@@ -112,17 +111,14 @@ type OrderStatus struct {
 
 	// FailureTime stores the time that this order failed.
 	// This is used to influence garbage collection and back-off.
-	// The order resource will be automatically deleted after 30 minutes has
-	// passed since the failure time.
-	// +optional
 	FailureTime *metav1.Time `json:"failureTime,omitempty"`
 }
 
 // State represents the state of an ACME resource, such as an Order.
 // The possible options here map to the corresponding values in the
 // ACME specification.
-// Full details of these values can be found there.
+// Full details of these values can be found here: https://tools.ietf.org/html/draft-ietf-acme-acme-15#section-7.1.6
-// Clients utilising this type **must** also gracefully handle unknown
+// Clients utilising this type must also gracefully handle unknown
 // values, as the contents of this enumeration may be added to over time.
 type State string
 
@@ -132,15 +128,17 @@ const (
 	Unknown State = ""
 
 	// Valid signifies that an ACME resource is in a valid state.
-	// If an Order is marked 'valid', all validations on that Order
+	// If an order is 'valid', it has been finalized with the ACME server and
-	// have been completed successfully.
+	// the certificate can be retrieved from the ACME server using the
-	// This is a transient state as of ACME draft-12
+	// certificate URL stored in the Order's status subresource.
+	// This is a final state.
 	Valid State = "valid"
 
 	// Ready signifies that an ACME resource is in a ready state.
-	// If an Order is marked 'Ready', the corresponding certificate
+	// If an order is 'ready', all of its challenges have been completed
-	// is ready and can be obtained.
+	// successfully and the order is ready to be finalized.
-	// This is a final state.
+	// Once finalized, it will transition to the Valid state.
+	// This is a transient state.
 	Ready State = "ready"
 
 	// Pending signifies that an ACME resource is still pending and is not yet ready.

pkg/apis/certmanager/v1alpha1/zz_generated.deepcopy.go

@@ -1091,6 +1091,11 @@ func (in *OrderSpec) DeepCopy() *OrderSpec {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *OrderStatus) DeepCopyInto(out *OrderStatus) {
 	*out = *in
+	if in.Certificate != nil {
+		in, out := &in.Certificate, &out.Certificate
+		*out = make([]byte, len(*in))
+		copy(*out, *in)
+	}
 	if in.Challenges != nil {
 		in, out := &in.Challenges, &out.Challenges
 		*out = make([]ChallengeSpec, len(*in))

pkg/controller/acmeorders/sync.go

@@ -17,7 +17,9 @@ limitations under the License.
 package acmeorders
 
 import (
+	"bytes"
 	"context"
+	"encoding/pem"
 	"fmt"
 	"reflect"
 
@@ -99,6 +101,8 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) {
 	// left for us to do here.
 	// TODO: we should find a way to periodically update the state of the resource
 	// to reflect the current/actual state in the ACME server.
+	// TODO: if the certificate bytes are nil, we should attempt to retrieve
+	// the certificate for the order using GetCertificate
 	if acme.IsFinalState(o.Status.State) {
 		return nil
 	}
@@ -140,16 +144,35 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) {
 	// if the current state is 'ready', we need to generate a CSR and finalize
 	// the order
 	case cmapi.Ready:
-		_, err := cl.FinalizeOrder(ctx, o.Status.FinalizeURL, o.Spec.CSR)
+		// TODO: we could retrieve a copy of the certificate resource here and
+		// stored it on the Order resource to prevent extra calls to the API
+		certSlice, err := cl.FinalizeOrder(ctx, o.Status.FinalizeURL, o.Spec.CSR)
+
+		// always update the order status after calling Finalize - this allows
+		// us to record the current orders status on this order resource
+		// despite it not being returned directly by the acme client.
 		errUpdate := c.syncOrderStatus(ctx, cl, o)
 		if errUpdate != nil {
 			// TODO: mark permenant failure?
 			return fmt.Errorf("error syncing order status: %v", errUpdate)
 		}
+
+		// check for errors from FinalizeOrder
 		if err != nil {
 			return fmt.Errorf("error finalizing order: %v", err)
 		}
 
+		// encode the retrieved certificates (including the chain)
+		certBuffer := bytes.NewBuffer([]byte{})
+		for _, cert := range certSlice {
+			err := pem.Encode(certBuffer, &pem.Block{Type: "CERTIFICATE", Bytes: cert})
+			if err != nil {
+				// TODO: something else?
+				return err
+			}
+		}
+
+		o.Status.Certificate = certBuffer.Bytes()
 		c.Recorder.Event(o, corev1.EventTypeNormal, "OrderValid", "Order completed successfully")
 
 		return nil
@@ -414,7 +437,6 @@ func (c *Controller) setOrderStatus(o *cmapi.OrderStatus, acmeOrder *acmeapi.Ord
 
 	o.URL = acmeOrder.URL
 	o.FinalizeURL = acmeOrder.FinalizeURL
-	o.CertificateURL = acmeOrder.CertificateURL
 }
 
 func challengeLabelsForOrder(o *cmapi.Order) map[string]string {

pkg/controller/acmeorders/sync_test.go

@@ -88,6 +88,11 @@ func TestSyncHappyPath(t *testing.T) {
 	testOrderInvalid.Status.FailureTime = &nowMetaTime
 	testOrderValid := testOrderPending.DeepCopy()
 	testOrderValid.Status.State = v1alpha1.Valid
+	// pem encoded word 'test'
+	testOrderValid.Status.Certificate = []byte(`-----BEGIN CERTIFICATE-----
+dGVzdA==
+-----END CERTIFICATE-----
+`)
 	testOrderReady := testOrderPending.DeepCopy()
 	testOrderReady.Status.State = v1alpha1.Ready
 
@@ -219,9 +224,8 @@ func TestSyncHappyPath(t *testing.T) {
 					return testACMEOrderValid, nil
 				},
 				FakeFinalizeOrder: func(_ context.Context, url string, csr []byte) ([][]byte, error) {
-					// Order controller does not currently use the 'der' field, so
+					testData := []byte("test")
-					// for now we can return nil, nil.
+					return [][]byte{testData}, nil
-					return nil, nil
 				},
 			},
 			CheckFn: func(t *testing.T, s *controllerFixture, args ...interface{}) {

pkg/issuer/acme/issue.go

@@ -17,12 +17,10 @@ limitations under the License.
 package acme
 
 import (
-	"bytes"
 	"context"
 	"crypto"
 	"crypto/x509"
 	"encoding/json"
-	"encoding/pem"
 	"fmt"
 	"hash/fnv"
 	"time"
@@ -160,51 +158,30 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss
 
 	a.Recorder.Eventf(crt, corev1.EventTypeNormal, "OrderComplete", "Order %q completed successfully", existingOrder.Name)
 
-	// If the order is valid, we can attempt to retrieve the Certificate.
+	// this should never happen
-	// First obtain an ACME client to make this easier.
+	if existingOrder.Status.Certificate == nil {
-	cl, err := a.helper.ClientForIssuer(a.issuer)
-	if err != nil {
-		return issuer.IssueResponse{}, err
-	}
-
-	// Check the current Order's Certificate resource to see if it's nearing expiry.
-	// If it is, this implies that it is an old order that is now out of date so
-	// we retry the order.
-	certSlice, err := cl.GetCertificate(ctx, existingOrder.Status.CertificateURL)
-	if err != nil {
-		a.Recorder.Eventf(crt, corev1.EventTypeWarning, "Failed to retrieve Certificate for Order %q (%s): %v", existingOrder.Name, existingOrder.Status.CertificateURL, err)
-		return issuer.IssueResponse{}, err
-	}
-
-	// TODO: retry the order?
-	// this is a weird error we'd not expect to see
-	if len(certSlice) == 0 {
 		a.Recorder.Eventf(crt, corev1.EventTypeWarning, "BadCertificate", "Empty certificate data retrieved from ACME server")
-		return issuer.IssueResponse{}, fmt.Errorf("invalid certificate returned from acme server")
+		return issuer.IssueResponse{}, fmt.Errorf("Order in a valid state but certificate data not set")
 	}
 
-	x509Cert, err := x509.ParseCertificate(certSlice[0])
+	x509Certs, err := x509.ParseCertificates(existingOrder.Status.Certificate)
 	if err != nil {
 		// if parsing the certificate fails, recreate the order
 		return a.retryOrder(crt, existingOrder)
 	}
 
-	// Check if the currently available Certificate from the Order is up to date.
+	x509Cert := x509Certs[0]
-	// This may not be the case at renewal time if the old order is still available.
+
+	// we check if the certificate stored on the existing order resource is
+	// nearing expiry.
+	// If it is, we recreate the order so we can obtain a fresh certificate.
+	// If not, we return the existing order's certificate to save additional
+	// orders.
 	if a.Context.IssuerOptions.CertificateNeedsRenew(x509Cert, crt.Spec.RenewBefore) {
 		// existing order's certificate is near expiry
 		return a.retryOrder(crt, existingOrder)
 	}
 
-	// encode the retrieved certificates (including the chain)
-	certBuffer := bytes.NewBuffer([]byte{})
-	for _, cert := range certSlice {
-		err := pem.Encode(certBuffer, &pem.Block{Type: "CERTIFICATE", Bytes: cert})
-		if err != nil {
-			return issuer.IssueResponse{}, err
-		}
-	}
-
 	// encode the private key and return
 	keyPem, err := pki.EncodePrivateKey(key)
 	if err != nil {
@@ -213,7 +190,7 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss
 	}
 
 	return issuer.IssueResponse{
-		Certificate: certBuffer.Bytes(),
+		Certificate: existingOrder.Status.Certificate,
 		PrivateKey:  keyPem,
 	}, nil
 }