From 758ebc3835adf3c6daeb920fb6b9159387b51fc1 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 12 Apr 2018 13:21:32 -0400 Subject: [PATCH] SA: Fix NewOrder ready status regression. In #3614 we adjusted the `SA.NewOrder` function to conditionally call `ssa.statusForOrder` on the new order when `features.OrderReadyStatus` was enabled. Unfortunately this call to `ssa.statusForOrder` happened *before* the `req.BeganProcessing` field was initialized with a pointer to a `false` bool. The `ssa.statusForOrder` function (correctly) assumes that `req.BeganProcessing == nil` is illegal and doesn't correspond to a known status. This results in NewOrder requests returning a 500 error of the form: > Internal error - Error creating new order - Order XXX is in an invalid > state. No state known for this order's authorizations Our integration tests missed this because we didn't have a test case that issued for a set of names with one account, and then issued again for the same set of names with the same account. This commit fixes the original bug by moving the `BeganProcessing` initialization before the call to `statusForOrder`. This commit also adds an integration test to catch this sort of bug again in the future. Prior to the SA fix this test failed with the 500 server internal error observed by the Certbot team. With the SA fix in place the test passes again. --- sa/sa.go | 6 +++--- test/v2_integration.py | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index aa9a9fc54..da20f95df 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -1460,6 +1460,9 @@ func (ssa *SQLStorageAuthority) NewOrder(ctx context.Context, req *corepb.Order) // Update the request with the created timestamp from the model createdTS := order.Created.UnixNano() req.Created = &createdTS + // A new order is never processing because it can't have been finalized yet + processingStatus := false + req.BeganProcessing = &processingStatus // If the OrderReadyStatus feature is enabled we need to calculate the order // status before returning it. Since it may have reused all valid @@ -1478,9 +1481,6 @@ func (ssa *SQLStorageAuthority) NewOrder(ctx context.Context, req *corepb.Order) pendingStatus := string(core.StatusPending) req.Status = &pendingStatus } - // A new order is never processing because it can't have been finalized yet - processingStatus := false - req.BeganProcessing = &processingStatus // Return the new order return req, nil } diff --git a/test/v2_integration.py b/test/v2_integration.py index f0c51fdfa..ae9143fea 100644 --- a/test/v2_integration.py +++ b/test/v2_integration.py @@ -115,6 +115,16 @@ def test_bad_overlap_wildcard(): chisel2.expect_problem("urn:ietf:params:acme:error:malformed", lambda: chisel2.auth_and_issue(["*.example.com", "www.example.com"])) +def test_duplicate_orders(): + """ + Test that the same client issuing for the same domain names twice in a row + works without error. + """ + client = chisel2.make_client(None) + domains = [ random_domain() ] + chisel2.auth_and_issue(domains, client=client) + chisel2.auth_and_issue(domains, client=client) + def test_order_reuse_failed_authz(): """ Test that creating an order for a domain name, failing an authorization in