Remove proactive issuance and add order finalization. #47
Conversation
This commit implements ietf-wg-acme/acme#342 - replacing proactive issuance and CSR as part of new-order with an explicit order finalization step that delivers the CSR. This is largely a port of the work done to add order finalization to the WIP ACMEv2 support in Boulder: letsencrypt/boulder#3169 I haven't tested this end-to-end yet - There are likely bugs lurking :-)
cpu
changed the title
|
@shred Reports that ACME4J was able to provision a new-order with this branch but upon finalizing it the order remained in pending state. This is likely a bug in this PR - I'm going to try and investigate further in the next few days when I complete the Boulder work I have assigned. |
|
@cpu - you mention above that the order remained pending after completion; have you had a chance to look at that? |
ca/ca.go
Outdated
| // Update the order to reflect that we're now processing it | ||
| order.Lock() | ||
| defer order.Unlock() | ||
| // Lock the order to read order properties |
There was a problem hiding this comment.
The semantics of the locks here are slightly off. I think it would be a little nicer to take a write lock at the top, check the status, and change the status to "processing" before we unlock. That way we can actually be confident no other thread is trying to complete the order at the same time.
wfe/wfe.go
Outdated
| if csr == nil { | ||
| return acme.InternalErrorProblem("Parsed CSR is nil") | ||
| idents := order.Identifiers | ||
| if idents == nil || len(idents) == 0 { |
There was a problem hiding this comment.
len(nil) == 0, so you can simplify this to if len(idents) == 0
wfe/wfe.go
Outdated
| orderNames = append(orderNames, ident.Value) | ||
| } | ||
|
|
||
| // Store the unique lower version of the names on the order ob |
Apologies, I lost this thread over the past couple of weeks. I applied your review feedback and then tested an order finalization against 60d0ba8 successfully. After finalizing a fully authorized order a certificate was issued asynchronously and a subsequent poll of the order returned it in valid status with a certificate URL. A side-effect of moving the state change from pending -> processing from the wfe to the CA component to help simplify the locking semantics is that when the finalize API request returns the order to the user its still in pending status. The CA processes the order & status change in a separate go routine. I don't think the spec says that the order is to be reflected back in a new-order response, just a 200 OK - we might be able to just drop that response entirely instead of sending it back with a pending status when it should ~probably be processing. Thoughts? |
I'm wrong. When I specified this I wrote: "A valid request to finalize an order will return the order to be finalized. " I'm not sure how inappropriate it is for the order to remain "pending". In some sense it is, the CA isn't processing it yet. |
|
I think it's fine to return the order in a "pending" state. |
This commit implements ietf-wg-acme/acme#342 - replacing proactive issuance and CSR as part of new-order with an explicit order finalization step that delivers the CSR.
This is largely a port of the work done to add order finalization to the WIP ACMEv2 support in Boulder:
letsencrypt/boulder#3169
I haven't tested this end-to-end yet - There are likely bugs lurking :-)