From 34c359005249ae8652a15d5a9a1d02df5821ee99 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 26 Nov 2018 22:39:49 +0000 Subject: [PATCH] Store a copy of the signed certificate on the Order resource after Finalize Signed-off-by: James Munnelly --- .../output/reference/api-docs/index.html | 6 +-- pkg/apis/certmanager/v1alpha1/types_order.go | 32 ++++++------- .../v1alpha1/zz_generated.deepcopy.go | 5 ++ pkg/controller/acmeorders/sync.go | 26 +++++++++- pkg/controller/acmeorders/sync_test.go | 10 ++-- pkg/issuer/acme/issue.go | 47 +++++-------------- 6 files changed, 66 insertions(+), 60 deletions(-) diff --git a/docs/generated/reference/output/reference/api-docs/index.html b/docs/generated/reference/output/reference/api-docs/index.html index 672445e9b..760ea28d7 100755 --- a/docs/generated/reference/output/reference/api-docs/index.html +++ b/docs/generated/reference/output/reference/api-docs/index.html @@ -421,8 +421,8 @@ Appears In: -certificateURL
string -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. +certificate
string +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 'valid' state. challenges
ChallengeSpec array @@ -430,7 +430,7 @@ Appears In: failureTime
Time -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. +FailureTime stores the time that this order failed. This is used to influence garbage collection and back-off. finalizeURL
string diff --git a/pkg/apis/certmanager/v1alpha1/types_order.go b/pkg/apis/certmanager/v1alpha1/types_order.go index 51bd0ac12..75b535c8b 100644 --- a/pkg/apis/certmanager/v1alpha1/types_order.go +++ b/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 - // TLS certificate for this order. - // It will be populated automatically once the order has completed successfully - // and the certificate is available for retrieval. - // +optional - CertificateURL string `json:"certificateURL,omitempty"` + // 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 + // 'valid' state. + Certificate []byte `json:"certificate"` // 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. -// Clients utilising this type **must** also gracefully handle unknown +// 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 // 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 - // have been completed successfully. - // This is a transient state as of ACME draft-12 + // If an order is 'valid', it has been finalized with the ACME server and + // the certificate can be retrieved from the ACME server using the + // 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 - // is ready and can be obtained. - // This is a final state. + // If an order is 'ready', all of its challenges have been completed + // successfully and the order is ready to be finalized. + // 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. diff --git a/pkg/apis/certmanager/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/certmanager/v1alpha1/zz_generated.deepcopy.go index 36b7bdb5d..d5c3e7cf5 100644 --- a/pkg/apis/certmanager/v1alpha1/zz_generated.deepcopy.go +++ b/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)) diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 1f130f1f8..5bfe2b0ab 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/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 { diff --git a/pkg/controller/acmeorders/sync_test.go b/pkg/controller/acmeorders/sync_test.go index a5f742304..1549df7e5 100644 --- a/pkg/controller/acmeorders/sync_test.go +++ b/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 - // for now we can return nil, nil. - return nil, nil + testData := []byte("test") + return [][]byte{testData}, nil }, }, CheckFn: func(t *testing.T, s *controllerFixture, args ...interface{}) { diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index c7482277f..170795c22 100644 --- a/pkg/issuer/acme/issue.go +++ b/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. - // First obtain an ACME client to make this easier. - 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 { + // this should never happen + if existingOrder.Status.Certificate == nil { 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. - // This may not be the case at renewal time if the old order is still available. + x509Cert := x509Certs[0] + + // 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 }