ACME v2 Finalize order support #3169

cpu · 2017-10-12T19:01:25Z

This PR implements order finalization for the ACME v2 API.

In broad strokes this means:

Removing the CSR from order objects & the new-order flow
Adding identifiers to the order object & new-order
Providing a finalization URL as part of orders returned by new-order
Adding support to the WFE's Order endpoint to receive finalization POST requests with a CSR
Updating the RA to accept finalization requests and to ensure orders are fully validated before issuance can proceed
Updating the SA to allow finding order authorizations & updating orders.
Updating the CA to accept an Order ID to log when issuing a certificate corresponding to an order object

Resolves #3123

This commit implements order finalization for the ACME v2 API. In broad strokes this means: * Removing the CSR from order objects & the new-order flow * Adding identifiers to the order object & new-order * Providing a finalization URL as part of orders returned by new-order * Adding support to the WFE's Order endpoint to receive finalization POST requests with a CSR * Updating the RA to accept finalization requests and to ensure orders are fully validated before issuance can proceed * Updating the SA to allow finding order authorizations & updating orders. * Updating the CA to accept an Order ID to log when issuing a certificate corresponding to an order object

cpu · 2017-10-12T20:34:16Z

ra/ra.go

+// unexpired authorizations for all of its associated names an error is
+// returned. Similarly we vet that all of the names in the order are acceptable
+// based on current policy and return an error if the order can't be fulfilled.
+func (ra *RegistrationAuthorityImpl) FinalizeOrder(ctx context.Context, req *rapb.FinalizeOrderRequest) error {


Note to self: I forgot to check that the order isn't expired here. Will address shortly with a fix & a test.

I ended up doing this in the WFE.

jsha

Phew, big branch! Mostly looks in order though.

*pun not intended but definitely intended once I noticed it

jsha · 2017-10-13T20:31:57Z

sa/proto/sa.proto

@@ -39,7 +39,9 @@ service StorageAuthority {
        rpc DeactivateRegistration(RegistrationID) returns (core.Empty) {}
        rpc DeactivateAuthorization(AuthorizationID) returns (core.Empty) {}
        rpc NewOrder(core.Order) returns (core.Order) {}
+        rpc UpdateOrder(core.Order) returns (core.Order) {}


I'd like to try and scope our RPCs more narrowly to limit the amount of harm that a breach of any given component can do. For instance, can we change this to "FinalizeOrder", and only allow it to modify the fields involved in finalizing? And reject requests to finalize already-finalized orders?

Sure. That makes sense. AFAICT there shouldn't be a need to change an order after creation except for the status & certificate serial.

@jsha So something like
rpc FinalizeOrder(core.Order) returns (core.Order) {}

Where the SA's implementation only pulled out the CertificateSerial field of the order provided as an argument & then created a model that had that field + status(final) and used it for an update?

Or would you rather something like:
rpc FinalizeOrder(orderID, certificateSerial) returns (core.Order) {} to make that limitation more explicit?

I implemented the former to keep this PR moving.

rpc FinalizeOrder(core.Order) returns (core.Order) {}
Where the SA's implementation only pulled out the CertificateSerial field of the order provided as an argument & then created a model that had that field + status(final) and used it for an update?

This is fine, though I'd recommend an explicit query UPDATE orders SET status = ? WHERE id = ? AND status = "pending" rather than using the whole Order model in this case.

Sounds good. I'll make that change shortly.

Done in 9496ef5

jsha · 2017-10-13T20:45:44Z

sa/sa.go

+		ID:                *req.Id,
+		RegistrationID:    *req.RegistrationID,
+		Expires:           time.Unix(0, *req.Expires),
+		Status:            core.AcmeStatus(*req.Status),


If we switch to a FinalizeOrder RPC, we can do something like SET status = "final" WHERE status = "pending", which will produce the "no order updated" error if the order is not pending.

jsha · 2017-10-13T21:58:50Z

ra/ra.go

+func (ra *RegistrationAuthorityImpl) issueCertificate(
+	ctx context.Context,
+	req core.CertificateRequest,
+	acctID int64,


It makes me pretty nervous that we have acctID and orderID here distinguished only by their order; it's very easy to make a mistake. Maybe we could define internal types, like type accountID int and type orderID int, and use those types for these parameters? Then the call site would have to cast appropriately, like accountID(account.ID). And if you saw accountID(order.ID) at a call site it would be more obviously broken.

Also, we should have a consistent ordering for orderID vs accountID in structs and parameters. I think in most places, accountID (regID) is first, with orderID second. Probably we should adopt that idiom here too.

Both good ideas 👍

Also, we should have a consistent ordering for orderID vs accountID in structs and parameters. I think in most places, accountID (regID) is first, with orderID second. Probably we should adopt that idiom here too.

I'm slightly confused by this comment with closer examination. That's the order being used now

checkOrderAuthorizations had the acct ID / order ID flipped, maybe that's what you meant? (Fixed).

Yep, I suspect that's what I meant. Thanks!

jsha · 2017-10-14T00:12:47Z

wfe2/wfe.go

+// URL and the ceritificate URL as appropriate.
+func (wfe *WebFrontEndImpl) orderToOrderJSON(request *http.Request, order *corepb.Order) orderJSON {
+	// Convert the corepb.Order.Names back into identifiers for display to the
+	// user. We do this rather than send back newOrderRequest.Identifiers because


I think this sentence may be left over from a previous iteration? There's no newOrderRequest in this scope, and we use this utility function in both NewOrder and getOrder.

Yup! You're right. This is some spilled copy pasta 🍝

jsha · 2017-10-14T00:19:26Z

wfe2/wfe.go

-	}
-	for i, authzID := range order.Authorizations {
-		respObj.Authorizations[i] = wfe.relativeEndpoint(request, fmt.Sprintf("%s%s", authzPath, authzID))
+	// Convert the corepb.Order.Names back into identifiers for display to the


I think this is taken care of by orderToOrderJSON and can be deleted from this function?

Agreed. I missed this when I made the orderToOrderJSON function to replace the duplicated code. Thanks!

jsha · 2017-10-14T00:27:43Z

wfe2/wfe.go

+	// If there are less than 2 fields there can't be both an account ID and an
+	// order ID so the path is invalid
+	if len(fields) < 2 {
+		wfe.sendError(response, logEvent, probs.Malformed("Invalid request path"), nil)


I think this and the Malformed below should both be 404's.

jsha · 2017-10-14T00:32:33Z

wfe2/wfe.go

+		return
+	}
+
+	// If there is already a certificate serial for this order then it has been


This comment doesn't seem to match the check.

Oops. This was leftover from before I remembered the Order status field existed and was determining if an order was finalized or not strictly based on the presence of the certificate serial.

jsha · 2017-10-14T00:50:17Z

wfe2/wfe.go

 	}
+	// Assuming a properly formatted CSR there should be two four byte SEQUENCE


For API v2 we can drop this special handling, since there are no legacy clients that will do it.

Good idea 👍

This commit normalizes the functions in the RA that accept both an order ID and an accountID such that the accountID is always provided first. To help prevent accidental usage of an orderID as an accountID (or vice versa) dedicated types are introduced that callers must cast to explicitly.

This commit replaces the broader `UpdateOrder` SA RPC with a `FinalizeOrder` RPC. Now only the Order ID and CertificateSerial provided in the request are used for the update. The final status is hardcoded as part of the update. This helps reduces the impact of an RA compromise.

cpu · 2017-10-17T16:21:23Z

@jsha Ready for another 🔍

rolandshoemaker

First pass, generally looks good, a few comments but nothing major.

One architectural question: Why do WFE -> SA then WFE -> RA for the finalization (i.e. sa.GetOrder then ra.FinalizeOrder) instead of say WFE -> RA -> SA. Is it simply so we can do more error checking at the WFE before dispatching anything to the RA? (This isn't necessarily a bad thing, just wondering)

rolandshoemaker · 2017-10-19T17:54:11Z

grpc/ra-wrappers.go

@@ -187,12 +187,19 @@ func (ras *RegistrationAuthorityClientWrapper) NewOrder(ctx context.Context, req
 	if err != nil {
 		return nil, err
 	}
-	if resp == nil || resp.RegistrationID == nil || resp.Expires == nil || resp.Csr == nil || resp.Authorizations == nil || resp.Id == nil || resp.Status == nil {
+	if resp == nil || !orderValid(resp) {


I don't think order.CertificateSerial will be set here so it'll always fail.

I think you're right, good catch.

rolandshoemaker · 2017-10-19T17:54:46Z

grpc/sa-wrappers.go

@@ -472,7 +472,18 @@ func (sas StorageAuthorityClientWrapper) NewOrder(ctx context.Context, request *
 	if err != nil {
 		return nil, err
 	}
-	if resp == nil || resp.Id == nil || resp.RegistrationID == nil || resp.Expires == nil || resp.Csr == nil || resp.Authorizations == nil || resp.Status == nil {
+	if resp == nil || !orderValid(resp) {


order.CertificateSerial won't be set here either I think?

rolandshoemaker · 2017-10-19T18:07:01Z

grpc/sa-wrappers.go

@@ -954,13 +990,25 @@ func (sas StorageAuthorityServerWrapper) DeactivateAuthorization(ctx context.Con
 }

 func (sas StorageAuthorityServerWrapper) NewOrder(ctx context.Context, request *corepb.Order) (*corepb.Order, error) {
-	if request == nil || request.RegistrationID == nil || request.Expires == nil || request.Csr == nil || request.Authorizations == nil || request.Status == nil {
+	if request == nil || !orderValid(request) {


order.CertificateSerial won't be set here either I think?

rolandshoemaker · 2017-10-19T18:47:12Z

ra/ra.go

 	if err != nil {
 		logEvent.Error = err.Error()
 		return emptyCert, err
 	}

 	// Verify the CSR
 	csr := req.CSR
-	if err := csrlib.VerifyCSR(csr, ra.maxNames, &ra.keyPolicy, ra.PA, ra.forceCNFromSAN, regID); err != nil {
+	if err := csrlib.VerifyCSR(csr, ra.maxNames, &ra.keyPolicy, ra.PA, ra.forceCNFromSAN, int64(acctID)); err != nil {


This appears to get called twice now? (Once in ra.FinalizeOrder and once in ra. issueCertificate )

Good catch - if it's already being done in ra.issueCertificate I'll remove it from ra.FinalizeOrder.

rolandshoemaker · 2017-10-19T18:48:42Z

ra/ra.go

+		// at the time of order creation, but we check again in case the policy has
+		// changed in the time since the order was accepted.
+		id := core.AcmeIdentifier{Value: name, Type: core.IdentifierDNS}
+		if err := ra.PA.WillingToIssue(id); err != nil {


This is already checked in VerifyCSR isn't it?

You're right, it is. I'll remove from here 👍

rolandshoemaker · 2017-10-19T19:00:50Z

sa/sa.go

+		err = tx.Insert(reqdName)
+		if err != nil {
+			err = Rollback(tx, err)
+			return nil, err


nit: the above assignment and a few below aren't necessary, can just be return nil, Rollback(tx, err). Not really required but a general style thing.

rolandshoemaker · 2017-10-19T19:06:38Z

sa/sa.go

+	if err != nil {
+		return nil, err
+	}
+	// The requested names are stored reversed to improve indexing performance. We


Question: there isn't an index on the name column, and we don't (at least in this PR) do a select using that column. Are we planning on doing this in the future at some point? Or can we just store these in their normal form?

I stored the names in reversed order after @jsha suggested it for consistency with issuedNames.

At the time he said: "I can't think of a strong reason to argue in favor of forward names, and reversedNames gives slightly nicer index locality for subdomains of the same domain."

I think I'd lean towards keeping them reversed to make it easier to exploit an index on the column down the road if we require it. It's tough to land DB migrations in prod so the future-proof approach might be better.

Hm, I think future proofing makes sense, lack of indexes has definitely bit us down the road in the past. That said if that is why we're doing it we should probably just add the index now, I want to say it'll be faster to remove an index we don't need than add one we do.

That said if that is why we're doing it we should probably just add the index now, I want to say it'll be faster to remove an index we don't need than add one we do.

I like that argument. I'm 👍 to adding the index now.

rolandshoemaker · 2017-10-19T19:15:57Z

wfe2/wfe.go

+	// bytes on the wire, and (b) the CA logs all rejections as audit events, but
+	// a bad key from the client is just a malformed request and doesn't need to
+	// be audited.
+	if err := wfe.keyPolicy.GoodKey(certificateRequest.CSR.PublicKey); err != nil {


This is also checked in the RA via VerifyCSR.

rolandshoemaker · 2017-10-19T19:17:15Z

wfe2/wfe.go

+		idents[i] = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: name}
+	}
+
+	respObj := wfe.orderToOrderJSON(request, order)
 	err = wfe.writeJsonResponse(response, logEvent, http.StatusOK, respObj)


Will this not be missing the certificate serial and still have a pending status since ra.FinalizeOrder isn't mutating order or am I missing something?

That's correct. The spec PR doesn't say that the finalize POST should return the order with the certificate serial populated. The client is meant to poll the order after POSTing the finalize endpoint so that the Order can be issued asynchronously from the finalize request.

I think we should probably be mutating the order state to "Processing" but this will require an adjustment to the SA's Order RPCs since there isn't a way to update just the status field of an order with the current Order RPCs.

Maybe since we are issuing synchronously anyway we should just skip the processing state and change the RA to return the certificate serial and the order in the valid state.

We will want to switch to async issuance pretty soon as part of the SCT embedding work, so we should be planning ahead for that. However, I think it would probably make sense for that to be its own RPC: SetOrderProcessing or similar.

Sounds good. I'll have the RA set the order to status processing with a dedicated RPC and the WFE can return the order without the CertificateSerial but with Status: Pending at the end of a successful finalize order request.

rolandshoemaker · 2017-10-19T19:18:13Z

wfe2/wfe.go

+		return
+	}
+	// Convert the corepb.Order.Names into identifiers for display to the user.
+	idents := make([]core.AcmeIdentifier, len(order.Names))


These don't appear to be used anywhere (and this code is duplicated in orderToOrderJSON).

Doh, I thought I caught the last of the leftover orderToOrderJSON duplication.

cpu · 2017-10-19T19:50:30Z

One architectural question: Why do WFE -> SA then WFE -> RA for the finalization (i.e. sa.GetOrder then ra.FinalizeOrder) instead of say WFE -> RA -> SA. Is it simply so we can do more error checking at the WFE before dispatching anything to the RA? (This isn't necessarily a bad thing, just wondering)

@rolandshoemaker I think this was a byproduct of the way I approached making the finalization endpoint a subpath of the Order endpoint. The existing Order() handler was already going WFE->SA to get the order in order to return it. It seemed like it could save an RPC for the easier error cases if the WFE passed that order to the RA instead of asking the RA to load it itself.

There's not a lot of deep thought behind the decision. I'm open to adjusting if that's preferred.

rolandshoemaker · 2017-10-19T23:40:39Z

There's not a lot of deep thought behind the decision. I'm open to adjusting if that's preferred.

I think there's probably some performance gain to wrapping all of the round trips (i.e. one round trip from the WFE to RA) but tbh I doubt it's that big of an issue. Given it'd end up being a relatively big change to make at this point I think it's probably fine to leave as is.

The NewOrder usage of `orderValid` means that `CertificateSerial` will be nil but the order _is_ valid, just not finalized. This commit updates the `grpc.orderValid` function to allow the nil `CertificateSerial` field. The wrappers for the SA's `finalizeOrder` RPCs are updated to additionally check that `orderValid` is true *and* `CertificateSerial` is not nil.

cpu · 2017-10-30T19:02:28Z

@rolandshoemaker @jsha Another round of feedback applied.

jsha

Generally looks good! Just a couple things:

jsha · 2017-10-31T01:05:19Z

grpc/pb-marshalling.go

@@ -406,8 +406,12 @@ func registrationValid(reg *corepb.Registration) bool {
 	return !(reg.Id == nil || reg.Key == nil || reg.Agreement == nil || reg.InitialIP == nil || reg.CreatedAt == nil || reg.Status == nil || reg.ContactsPresent == nil)
 }

+// orderValid checks that a corepb.Order is valid. It allows
+// `order.CertificateStatus` to be nil such that it can be used in places where
+// the order has not been finalized yet. Callers must additional ensure the


*additionally

jsha · 2017-10-31T22:50:41Z

sa/sa.go

 	err = tx.Commit()
 	if err != nil {
 		return nil, err
 	}

-	req.Id = &order.ID
+	// Set the status to Processing in the request ob we return since this is the


s/ob//

Also, do we definitely need to return the order? It seems like just returning an error or nil error would be clearer, since we wouldn't be claiming to fill out the order. I feel like we've run into issues elsewhere where we pass around partially-filled objects (like an authz with just a RegistrationID passed to the VA).

Also, do we definitely need to return the order? It seems like just returning an error or nil error would be clearer, since we wouldn't be claiming to fill out the order.

We don't - happy to rejig the two SA RPCs to not return the Order. Might as well save some bytes on the wire :-)

Fixed with b74f159

cpu · 2017-11-01T14:33:36Z

@rolandshoemaker @jsha This should be ready for another 🔍 pass when you folks have a chance.

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 :-)

Daniel added 5 commits October 12, 2017 14:44

Add missing pb-marshalling test

803e269

Remove commented import

768706d

Remove comment cruft from test

f58950c

Remove comment

8b2efa0

cpu self-assigned this Oct 12, 2017

Daniel added 2 commits October 12, 2017 15:06

Merge remote-tracking branch 'le/master' into cpu-v2-finalize-order

cefd8eb

Fix merge errors

d571522

cpu commented Oct 12, 2017

View reviewed changes

jsha reviewed Oct 14, 2017

View reviewed changes

cpu added the needs-revision label Oct 16, 2017

Daniel added 8 commits October 16, 2017 15:51

Remove legacy OpenSSL CSR bug check

55c4ac2

Fix inaccurate comment in finalizeOrder

5115791

Return 404 for bad Order req paths

5b13d17

Remove redundant identifier conversion

ed9c502

Remove inaccurate/old orderToOrderJSON comment.

4039dcd

Reject finalization of expired orders

5217e30

cpu removed the needs-revision label Oct 17, 2017

cpu requested a review from rolandshoemaker October 17, 2017 16:21

Replace tx.Update with tx.Exec of an UPDATE stmnt.

9496ef5

rolandshoemaker suggested changes Oct 19, 2017

View reviewed changes

Daniel added 4 commits October 20, 2017 13:09

Fix TestOrderValid, remove RA CSR check duplication.

a682049

Rename OrderAuthorizationsRequest to GetOrderAuthorizationsRequest.

43eee64

Clean up NewOrder rollbacks/error handling.

6f46f03

Daniel added 4 commits October 20, 2017 14:06

Remove duplicated Goodkey check

9db6b71

Remove unused idents from finalizeOrder.

e9effd6

Add index to reversedName, reduce reversedName size.

c42bacd

Mark order as processing before finalization.

5d8f6a2

Merge remote-tracking branch 'le/master' into cpu-v2-finalize-order

e3b8d73

jsha reviewed Oct 31, 2017

View reviewed changes

Daniel added 2 commits November 1, 2017 09:55

Fix typo.

ddffce0

Don't return Order from SA processing/finalize RPCs.

b74f159

jsha approved these changes Nov 1, 2017

View reviewed changes

rolandshoemaker approved these changes Nov 1, 2017

View reviewed changes

rolandshoemaker merged commit 2f263f8 into master Nov 1, 2017

cpu deleted the cpu-v2-finalize-order branch November 1, 2017 19:51

cpu mentioned this pull request Nov 6, 2017

Remove proactive issuance and add order finalization. letsencrypt/pebble#47

Merged

jsha mentioned this pull request Apr 16, 2024

sa: investigate removing requestedNames table #7432

Closed

		}
		// Assuming a properly formatted CSR there should be two four byte SEQUENCE

ACME v2 Finalize order support #3169

ACME v2 Finalize order support #3169

Conversation

cpu commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Oct 17, 2017

rolandshoemaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Oct 19, 2017

rolandshoemaker commented Oct 19, 2017

cpu commented Oct 30, 2017

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Nov 1, 2017

cpu Oct 19, 2017 •

edited

Loading