Repro failure

ra/ra.go

@@ -1656,6 +1656,15 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 			continue
 		}
 		authz := nameToExistingAuthz[name]
+		// If the existing authz isn't valid, note that its missing and continue.
+		// Since we reuse whole pending orders there isn't any benefit to reusing
+		// individual pending authorizations between orders. Worse, it makes the
+		// process of invalidating an order when an authorization is invalidated
+		// confusing because multiple orders would be invalidated simultaneously.
+		if *authz.Status != string(core.StatusValid) {
+			missingAuthzNames = append(missingAuthzNames, name)
+			continue
+		}
 		// If the identifier is a wildcard and the existing authz only has one
 		// DNS-01 type challenge we can reuse it. In theory we will
 		// never get back an authorization for a domain with a wildcard prefix

ra/ra_test.go

@@ -13,6 +13,7 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"reflect"
 	"sort"
 	"strings"
 	"sync"
@@ -2166,7 +2167,14 @@ func TestNewOrder(t *testing.T) {
 	// Abuse the order of the queries used to extract the reused authorizations
 	existing := orderC.Authorizations[:3]
 	sort.Strings(existing)
-	test.AssertDeepEquals(t, existing, orderA.Authorizations)
+
+	// We expect the pending authorizations were not reused between separate
+	// orders
+	// NOTE(@cpu): There's no test.AssertNotDeepEquals to use here so we call
+	// reflect.DeepEqual ourselves.
+	if reflect.DeepEqual(existing, orderA.Authorizations) {
+		t.Fatal("existing authorizations matched orderA authorizations")
+	}
 
 	_, err = ra.NewOrder(context.Background(), &rapb.NewOrderRequest{
 		RegistrationID: &id,
@@ -2284,6 +2292,62 @@ func TestNewOrderReuse(t *testing.T) {
 	}
 }
 
+func TestNewOrderReuseInvalidAuthz(t *testing.T) {
+	if os.Getenv("BOULDER_CONFIG_DIR") != "test/config-next" {
+		return
+	}
+
+	_, _, ra, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+
+	ctx := context.Background()
+	regA := int64(1)
+	names := []string{"zombo.com"}
+
+	// Create an initial request with regA and names
+	orderReq := &rapb.NewOrderRequest{
+		RegistrationID: &regA,
+		Names:          names,
+	}
+
+	// First, add an order with `names` for regA
+	order, err := ra.NewOrder(ctx, orderReq)
+	// It shouldn't fail
+	test.AssertNotError(t, err, "Adding an initial order for regA failed")
+	// It should have an ID
+	test.AssertNotNil(t, order.Id, "Initial order had a nil ID")
+	// It should have one authorization
+	test.AssertEquals(t, len(order.Authorizations), 1)
+
+	// Fetch the full authz by the ID
+	authzID := order.Authorizations[0]
+	authz, err := ra.SA.GetAuthorization(ctx, authzID)
+	test.AssertNotError(t, err, "Error getting order authorization")
+
+	// Finalize the authz to an invalid status
+	authz.Status = core.StatusInvalid
+	err = ra.SA.FinalizeAuthorization(ctx, authz)
+	test.AssertNotError(t, err, fmt.Sprintf("Could not finalize authorization %q", authzID))
+
+	// The order associated with the authz should now be invalid
+	updatedOrder, err := ra.SA.GetOrder(ctx, &sapb.OrderRequest{Id: order.Id})
+	test.AssertNotError(t, err, "Error getting order to check status")
+	test.AssertEquals(t, *updatedOrder.Status, "invalid")
+
+	// Create a second order for the same names/regID
+	secondOrder, err := ra.NewOrder(ctx, orderReq)
+	// It shouldn't fail
+	test.AssertNotError(t, err, "Adding an initial order for regA failed")
+	// It should have a different ID than the first now-invalid order
+	test.AssertNotEquals(t, *secondOrder.Id, *order.Id)
+	// It should be status pending
+	test.AssertEquals(t, *secondOrder.Status, "pending")
+	// It should have one authorization
+	test.AssertEquals(t, len(secondOrder.Authorizations), 1)
+	// It should have a different authorization than the first order's now-invalid authorization
+	test.AssertNotEquals(t, secondOrder.Authorizations[0], order.Authorizations[0])
+}
+
 func TestNewOrderWildcard(t *testing.T) {
 	// Only run under test/config-next config where 20170731115209_AddOrders.sql
 	// has been applied

sa/sa.go

@@ -791,7 +791,7 @@ func (ssa *SQLStorageAuthority) UpdatePendingAuthorization(ctx context.Context,
 
 // FinalizeAuthorization converts a Pending Authorization to a final one. If the
 // Authorization is not found a berrors.NotFound result is returned. If the
-// Authorization is not valid a berrors.InternalServer error is returned.
+// Authorization is status pending an berrors.InternalServer error is returned.
 func (ssa *SQLStorageAuthority) FinalizeAuthorization(ctx context.Context, authz core.Authorization) error {
 	tx, err := ssa.dbMap.Begin()
 	if err != nil {
@@ -832,6 +832,30 @@ func (ssa *SQLStorageAuthority) FinalizeAuthorization(ctx context.Context, authz
 		return Rollback(tx, err)
 	}
 
+	// When an authorization is being finalized to an invalid state we need to see
+	// if there is an order associated with this authorization that itself should
+	// now become invalid as a result of the authz being invalid.
+	if authz.Status == core.StatusInvalid {
+		// Try to find an order associated with this authz ID. There may not be one if
+		// this is a legacy V1 authorization from the new-authz endpoint.
+		orderID, err := ssa.orderIDForAuthz(tx, authz.ID)
+		// If there was an error, and it wasn't a no-result error, then we encountered
+		// something unexpected and must rollback
+		if err != nil && err != sql.ErrNoRows {
+			return Rollback(tx, err)
+		}
+		// If the err was nil then orderID is an associated order for this authz
+		if err == nil {
+			// Set the order to invalid
+			err := ssa.setOrderInvalid(ctx, tx, &corepb.Order{
+				Id: &orderID,
+			})
+			if err != nil {
+				return Rollback(tx, err)
+			}
+		}
+	}
+
 	return tx.Commit()
 }
 
@@ -1456,6 +1480,29 @@ func (ssa *SQLStorageAuthority) SetOrderProcessing(ctx context.Context, req *cor
 	return tx.Commit()
 }
 
+// setOrderInvalid updates a provided *corepb.Order in pending status to be in
+// invalid status by updating the status field of the corresponding row in the
+// DB.
+func (ssa *SQLStorageAuthority) setOrderInvalid(ctx context.Context, tx *gorp.Transaction, req *corepb.Order) error {
+	result, err := tx.Exec(`
+		UPDATE orders
+		SET status = ?
+		WHERE id = ?
+		AND status = ?`,
+		string(core.StatusInvalid),
+		*req.Id,
+		string(core.StatusPending))
+	if err != nil {
+		return err
+	}
+
+	n, err := result.RowsAffected()
+	if err != nil || n == 0 {
+		return berrors.InternalServerError("no order updated to invalid status")
+	}
+	return nil
+}
+
 // FinalizeOrder finalizes a provided *corepb.Order by persisting the
 // CertificateSerial and a valid status to the database. No fields other than
 // CertificateSerial and the order ID on the provided order are processed (e.g.
@@ -1504,6 +1551,26 @@ func (ssa *SQLStorageAuthority) authzForOrder(orderID int64) ([]string, error) {
 	return ids, nil
 }
 
+// orderForAuthz finds an order ID associated with a given authz (If any).
+func (ssa *SQLStorageAuthority) orderIDForAuthz(tx *gorp.Transaction, authzID string) (int64, error) {
+	var orderID int64
+
+	err := tx.SelectOne(&orderID, `
+	SELECT orderID
+	FROM orderToAuthz
+	WHERE authzID = ?`,
+		authzID)
+
+	// NOTE(@cpu): orderIDForAuthz does not handle the sql.ErrNoRows that could be
+	// returned by `SelectOne`. The caller must do so.
+	if err != nil {
+		// If there is an err, return it as-is.
+		return 0, err
+	}
+
+	return orderID, nil
+}
+
 // namesForOrder finds all of the requested names associated with an order. The
 // names are returned in their reversed form (see `sa.ReverseName`).
 func (ssa *SQLStorageAuthority) namesForOrder(orderID int64) ([]string, error) {
@@ -1631,8 +1698,16 @@ func (ssa *SQLStorageAuthority) GetOrderForNames(
 		return nil, err
 	}
 
-	// Get & return the order
-	return ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &orderID})
+	// Get the order
+	order, err := ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &orderID})
+	if err != nil {
+		return nil, err
+	}
+	// Only return a pending order
+	if *order.Status != string(core.StatusPending) {
+		return nil, berrors.NotFoundError("no order matching request found")
+	}
+	return order, nil
 }
 
 func (ssa *SQLStorageAuthority) getAuthorizations(ctx context.Context, table string, status string,

sa/sa_test.go

@@ -1864,3 +1864,74 @@ func TestGetOrderForNames(t *testing.T) {
 	// already
 	test.Assert(t, result == nil, "sa.GetOrderForNames returned non-nil result for finalized order case")
 }
+
+func TestUpdatePendingAuthorizationInvalidOrder(t *testing.T) {
+	if os.Getenv("BOULDER_CONFIG_DIR") != "test/config-next" {
+		return
+	}
+
+	sa, fc, cleanUp := initSA(t)
+	defer cleanUp()
+
+	expires := fc.Now().Add(time.Hour)
+	ctx := context.Background()
+
+	// Create a registration to work with
+	reg := satest.CreateWorkingRegistration(t, sa)
+
+	// Create a pending authz, not associated with any orders
+	authz := core.Authorization{
+		RegistrationID: reg.ID,
+		Expires:        &expires,
+		Status:         core.StatusPending,
+	}
+	pendingAuthz, err := sa.NewPendingAuthorization(ctx, authz)
+	test.AssertNotError(t, err, "Couldn't create new pending authorization")
+
+	// Update the pending authz to be invalid. This shouldn't error.
+	pendingAuthz.Status = core.StatusInvalid
+	err = sa.FinalizeAuthorization(ctx, pendingAuthz)
+	test.AssertNotError(t, err, "Couldn't finalize legacy pending authz to invalid")
+
+	// Create a pending authz that will be associated with an order
+	authz = core.Authorization{
+		RegistrationID: reg.ID,
+		Expires:        &expires,
+		Status:         core.StatusPending,
+	}
+	pendingAuthz, err = sa.NewPendingAuthorization(ctx, authz)
+	test.AssertNotError(t, err, "Couldn't create new pending authorization")
+
+	// Add a new order that references the above pending authz
+	status := string(core.StatusPending)
+	expiresNano := expires.UnixNano()
+	order, err := sa.NewOrder(ctx, &corepb.Order{
+		RegistrationID: &reg.ID,
+		Expires:        &expiresNano,
+		Status:         &status,
+		Authorizations: []string{pendingAuthz.ID},
+		Names:          []string{"your.order.is.up"},
+	})
+	// It shouldn't error
+	test.AssertNotError(t, err, "sa.NewOrder failed")
+	// The order ID shouldn't be nil
+	test.AssertNotNil(t, *order.Id, "NewOrder returned with a nil Id")
+	// The order should be pending
+	test.AssertEquals(t, *order.Status, string(core.StatusPending))
+	// The order should have one authz with the correct ID
+	test.AssertEquals(t, len(order.Authorizations), 1)
+	test.AssertEquals(t, order.Authorizations[0], pendingAuthz.ID)
+
+	// Now finalize the authz to an invalid status.
+	pendingAuthz.Status = core.StatusInvalid
+	err = sa.FinalizeAuthorization(ctx, pendingAuthz)
+	test.AssertNotError(t, err, "Couldn't finalize pending authz associated with order to invalid")
+
+	// Fetch the order to get its updated status
+	updatedOrder, err := sa.GetOrder(
+		context.Background(),
+		&sapb.OrderRequest{Id: order.Id})
+	test.AssertNotError(t, err, "GetOrder failed")
+	// We expect the updated order status to be invalid
+	test.AssertEquals(t, *updatedOrder.Status, string(core.StatusInvalid))
+}

test/integration-test-v2.py

@@ -34,6 +34,7 @@ def main():
     test_overlapping_wildcard()
     test_wildcard_exactblacklist()
     test_wildcard_authz_reuse()
+    test_order_reuse_failed_authz()
 
     if not startservers.check():
         raise Exception("startservers.check failed")
@@ -118,6 +119,52 @@ def test_wildcard_authz_reuse():
             raise Exception("order for %s included a non-pending authorization (status: %s) from a previous HTTP-01 order" %
                     ((domains), str(authz.body.status)))
 
+def test_order_reuse_failed_authz():
+    """
+    Test that creating an order for a domain name, failing an authorization in
+    that order, and submitting another new order request for the same name
+    doesn't reuse a failed authorizaton in the new order.
+    """
+
+    client = make_client(None)
+    domains = [ random_domain() ]
+    csr_pem = make_csr(domains)
+
+    order = client.new_order(csr_pem)
+    firstOrderURI = order.uri
+
+    # Pick the first authz's first challenge, doesn't matter what type it is
+    chall_body = order.authorizations[0].body.challenges[0]
+    # Answer it, but with nothing set up to solve the challenge request
+    client.answer_challenge(chall_body, chall_body.response(client.key))
+
+    # Wait for validation to occur
+    # TODO(@cpu): This should be handled better but it involves Python
+    # concurrency and I'm going to punt on that while working on other parts of
+    # this PR
+    import time
+    time.sleep(10)
+
+    # Make another order with the same domains
+    order = client.new_order(csr_pem)
+
+    # It should not be the same order as before
+    if order.uri == firstOrderURI:
+        raise Exception("new-order for %s returned a , now-invalid, order" % domains)
+
+    # We expect all of the returned authorizations to be pending status
+    for authz in order.authorizations:
+        if authz.body.status != Status("pending"):
+            raise Exception("order for %s included a non-pending authorization (status: %s) from a previous order" %
+                    ((domains), str(authz.body.status)))
+
+    # We expect the new order can be fulfilled
+    cleanup = do_http_challenges(client, order.authorizations)
+    try:
+        order = client.poll_order_and_request_issuance(order)
+    finally:
+        cleanup()
+
 if __name__ == "__main__":
     try:
         main()