Add logic to handle ready vs valid ACME orders properly #800
Conversation
jetstack-bot
added
release-note
| @@ -175,16 +167,6 @@ func (c *Controller) getGenericIssuer(crt *v1alpha1.Certificate) (v1alpha1.Gener | |||
| } | |||
| } | |||
|
|
|||
| func needsRenew(cert *x509.Certificate) bool { | |||
There was a problem hiding this comment.
This function was not used
munnerz
force-pushed
the
fix-verify-ready
branch
from
e833e71
to
19763fa
Compare
munnerz
force-pushed
the
fix-verify-ready
branch
from
19763fa
to
503186c
Compare
| 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. |
There was a problem hiding this comment.
This comment was incorrect. 'ready' indicates an order is ready to be finalized, whereas 'valid' indicates the order has already been finalized and can be retrieved with GetCertificate.
| 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") | ||
| } |
There was a problem hiding this comment.
This handles the case where the private key for a 'valid' order has been lost. If this happens, we trigger a new order to be created.
| // 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 { |
There was a problem hiding this comment.
As the comment explains, if the order is 'ready' then we need to finalize it (i.e. submit a CSR)
| if acmeErr.StatusCode >= 400 && acmeErr.StatusCode <= 499 { | ||
| crt.Status.ACMEStatus().Order.URL = "" | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we might be able to remove this check now. Leaving it in for the time being however, because if the server responds 4xx then we probably should create a new order because we don't have permission/we are sending back requests to it.
|
|
||
| // if the Certificate was marked 'valid', we need to retrieve the certificate | ||
| // from the URL specified on the Order resource. | ||
| if certSlice == nil { |
There was a problem hiding this comment.
certSlice will be nil if the order status is 'valid'
| return nil, nil, fmt.Errorf("failed to parse returned x509 certificate: %v", err.Error()) | ||
| } | ||
|
|
||
| if a.Context.IssuerOptions.CertificateNeedsRenew(x509Cert) { |
There was a problem hiding this comment.
Because we may have retrieved an existing certificate associated with an order, we must ensure that we have the correct private key for it and that the certificate is not nearing expiry (else we'd enter a loop of constantly requesting the same, expiring certificate).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kragniz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jetstack-bot
added
the
approved
|
@munnerz in your PR initial statement I think there's a slight error: |
|
Yes, thank you @damienwebdev! |
What this PR does / why we need it:
Recent changes to Let's Encrypt have meant that they now use the 'ready' ACME status condition to indicate an order is ready to be finalized.
Order's that have already been finalized will have a state 'valid'.
If an order is ready, we must finalize it (i.e. submit a CSR to the ACME server)
If an order is valid, all we can do is retrieve the existing Certificate (as signed by the server in response to the CSR initially submitted when the order was 'ready').
This PR adapts our Issue function to handle these cases properly.
It is slightly more complex due to case (2) (where the order is already valid). If we have lost the certificate's private key, we need to detect this and trigger a new order to be created so we can re-finalize with the new private key.
Which issue this PR fixes: fixes #778
Special notes for your reviewer:
Release note:
/cc @kragniz