Skip to content

Commit c105cfa

Browse files
cpuRoland Bracewell Shoemaker
cpu
authored and
Roland Bracewell Shoemaker
committed
WFE2: Don't allow finalizing pending orders, impl BadOrderState err type (letsencrypt#4075)
We've been using the newer "ready" order status for longer than the lifetime of any previously "pending" orders. I believe this means we can drop the legacy allowance for finalizing pending orders and enforce finalization only occur for "ready" orders without any feature flags. This is implemented in [c85d4b0](letsencrypt@c85d4b0). There is a new error type added in the draft spec (`orderNotReady`) that should be returned to clients that finalize an order in state other than "ready". This is implemented in [6008202](letsencrypt@6008202). Resolves letsencrypt#4073
1 parent b886817 commit c105cfa

File tree

5 files changed

+24
-49
lines changed

5 files changed

+24
-49
lines changed

‎mocks/mocks.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -515,10 +515,10 @@ func (sa *StorageAuthority) GetOrder(_ context.Context, req *sapb.OrderRequest)
515515
validOrder.RegistrationID = &six
516516
}
517517

518-
// Order ID 7 is expired
518+
// Order ID 7 is ready, but expired
519519
if *req.Id == 7 {
520-
pending := string(core.StatusPending)
521-
validOrder.Status = &pending
520+
ready := string(core.StatusReady)
521+
validOrder.Status = &ready
522522
exp = sa.clk.Now().AddDate(-30, 0, 0).Unix()
523523
validOrder.Expires = &exp
524524
}

‎probs/probs.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ const (
2121
CAAProblem = ProblemType("caa")
2222
DNSProblem = ProblemType("dns")
2323
AlreadyRevokedProblem = ProblemType("alreadyRevoked")
24+
OrderNotReadyProblem = ProblemType("orderNotReady")
2425

2526
V1ErrorNS = "urn:acme:error:"
2627
V2ErrorNS = "urn:ietf:params:acme:error:"
@@ -262,3 +263,12 @@ func DNS(detail string, a ...interface{}) *ProblemDetails {
262263
HTTPStatus: http.StatusBadRequest,
263264
}
264265
}
266+
267+
// OrderNotReady returns a ProblemDetails representing a OrderNotReadyProblem
268+
func OrderNotReady(detail string, a ...interface{}) *ProblemDetails {
269+
return &ProblemDetails{
270+
Type: OrderNotReadyProblem,
271+
Detail: fmt.Sprintf(detail, a...),
272+
HTTPStatus: http.StatusForbidden,
273+
}
274+
}

‎test/v2_integration.py‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -440,28 +440,10 @@ def test_order_finalize_early():
440440

441441
deadline = datetime.datetime.now() + datetime.timedelta(seconds=5)
442442

443-
# Finalizing an order early should generate an unauthorized error and we
444-
# should check that the order is invalidated.
445-
chisel2.expect_problem("urn:ietf:params:acme:error:unauthorized",
443+
# Finalizing an order early should generate an orderNotReady error.
444+
chisel2.expect_problem("urn:ietf:params:acme:error:orderNotReady",
446445
lambda: client.finalize_order(order, deadline))
447446

448-
# Poll for a fixed amount of time checking for the order to become invalid
449-
# from the early finalization attempt initiated above failing
450-
while datetime.datetime.now() < deadline:
451-
time.sleep(1)
452-
updatedOrder = requests.get(order.uri).json()
453-
if updatedOrder['status'] == "invalid":
454-
break
455-
456-
# If the loop ended and the status isn't invalid then we reached the
457-
# deadline waiting for the order to become invalid, fail the test
458-
if updatedOrder['status'] != "invalid":
459-
raise Exception("timed out waiting for order %s to become invalid" % order.uri)
460-
461-
# The order should have an error with the expected type
462-
if updatedOrder['error']['type'] != 'urn:ietf:params:acme:error:unauthorized':
463-
raise Exception("order %s has incorrect error field type: \"%s\"" % (order.uri, updatedOrder['error']['type']))
464-
465447
def test_revoke_by_issuer():
466448
client = chisel2.make_client(None)
467449
order = chisel2.auth_and_issue([random_domain()], client=client)

‎wfe2/wfe.go‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1805,15 +1805,10 @@ func (wfe *WebFrontEndImpl) FinalizeOrder(ctx context.Context, logEvent *web.Req
18051805
return
18061806
}
18071807

1808-
// Prior to ACME draft-10 the "ready" status did not exist and orders in
1809-
// a pending status with valid authzs were finalizable. We accept both states
1810-
// here for deployability ease. In the future we will only allow ready orders
1811-
// to be finalized.
1812-
// TODO(@cpu): Forbid finalizing "Pending" orders
1813-
if *order.Status != string(core.StatusPending) &&
1814-
*order.Status != string(core.StatusReady) {
1808+
// Only ready orders can be finalized.
1809+
if *order.Status != string(core.StatusReady) {
18151810
wfe.sendError(response, logEvent,
1816-
probs.Malformed(
1811+
probs.OrderNotReady(
18171812
"Order's status (%q) is not acceptable for finalization",
18181813
*order.Status),
18191814
nil)

‎wfe2/wfe_test.go‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2165,7 +2165,7 @@ func TestFinalizeOrder(t *testing.T) {
21652165
Name: "Order is already finalized",
21662166
// mocks/mocks.go's StorageAuthority's GetOrder mock treats ID 1 as an Order with a Serial
21672167
Request: signAndPost(t, "1/1", "http://localhost/1/1", goodCertCSRPayload, 1, wfe.nonceService),
2168-
ExpectedBody: `{"type":"` + probs.V2ErrorNS + `malformed","detail":"Order's status (\"valid\") is not acceptable for finalization","status":400}`,
2168+
ExpectedBody: `{"type":"` + probs.V2ErrorNS + `orderNotReady","detail":"Order's status (\"valid\") is not acceptable for finalization","status":403}`,
21692169
},
21702170
{
21712171
Name: "Order is expired",
@@ -2174,21 +2174,9 @@ func TestFinalizeOrder(t *testing.T) {
21742174
ExpectedBody: `{"type":"` + probs.V2ErrorNS + `malformed","detail":"Order 7 is expired","status":404}`,
21752175
},
21762176
{
2177-
Name: "Good CSR, Pending Order",
2178-
Request: signAndPost(t, "1/4", "http://localhost/1/4", goodCertCSRPayload, 1, wfe.nonceService),
2179-
ExpectedHeaders: map[string]string{"Location": "http://localhost/acme/order/1/4"},
2180-
ExpectedBody: `
2181-
{
2182-
"status": "processing",
2183-
"expires": "1970-01-01T00:00:00.9466848Z",
2184-
"identifiers": [
2185-
{"type":"dns","value":"example.com"}
2186-
],
2187-
"authorizations": [
2188-
"http://localhost/acme/authz/hello"
2189-
],
2190-
"finalize": "http://localhost/acme/finalize/1/4"
2191-
}`,
2177+
Name: "Good CSR, Pending Order",
2178+
Request: signAndPost(t, "1/4", "http://localhost/1/4", goodCertCSRPayload, 1, wfe.nonceService),
2179+
ExpectedBody: `{"type":"` + probs.V2ErrorNS + `orderNotReady","detail":"Order's status (\"pending\") is not acceptable for finalization","status":403}`,
21922180
},
21932181
{
21942182
Name: "Good CSR, Ready Order",
@@ -2229,7 +2217,7 @@ func TestFinalizeOrder(t *testing.T) {
22292217
// to match the whole response body because the "detail" of a bad CSR problem
22302218
// contains a verbose Go error message that can change between versions (e.g.
22312219
// Go 1.10.4 to 1.11 changed the expected format)
2232-
badCSRReq := signAndPost(t, "1/4", "http://localhost/1/4", `{"CSR": "ABCD"}`, 1, wfe.nonceService)
2220+
badCSRReq := signAndPost(t, "1/8", "http://localhost/1/8", `{"CSR": "ABCD"}`, 1, wfe.nonceService)
22332221
responseWriter.Body.Reset()
22342222
responseWriter.HeaderMap = http.Header{}
22352223
wfe.FinalizeOrder(ctx, newRequestEvent(), responseWriter, badCSRReq)
@@ -2819,7 +2807,7 @@ func TestFinalizeSCTError(t *testing.T) {
28192807
}`
28202808

28212809
// Create a finalization request with the above payload
2822-
request := signAndPost(t, "1/4", "http://localhost/1/4", goodCertCSRPayload, 1, wfe.nonceService)
2810+
request := signAndPost(t, "1/8", "http://localhost/1/8", goodCertCSRPayload, 1, wfe.nonceService)
28232811

28242812
// POST the finalize order request.
28252813
wfe.FinalizeOrder(ctx, newRequestEvent(), responseWriter, request)

0 commit comments

Comments
 (0)