Fix order reuse, calc order status by authz status

core/proto/core.proto

@@ -85,6 +85,7 @@ message Order {
         repeated string authorizations = 6;
         optional string status = 7;
         repeated string names = 8;
+        optional bool beganProcessing = 9;
 }
 
 message Empty {}

grpc/pb-marshalling.go

@@ -407,19 +407,20 @@ func registrationValid(reg *corepb.Registration) bool {
 }
 
 // orderValid checks that a corepb.Order is valid. In addition to the checks
-// from `newOrderValid` it ensures the order ID is not nil.
+// from `newOrderValid` it ensures the order ID and the BeganProcessing fields
+// are not nil.
 func orderValid(order *corepb.Order) bool {
-	return order.Id != nil && newOrderValid(order)
+	return order.Id != nil && order.BeganProcessing != nil && newOrderValid(order)
 }
 
 // newOrderValid checks that a corepb.Order is valid. It allows for a nil
 // `order.Id` because the order has not been assigned an ID yet when it is being
-// created initially. It also allows `order.CertificateSerial` to be nil such
-// that it can be used in places where the order has not been finalized yet.
-// Callers must additionally ensure the `CertificateSerial` field is non-nil if
-// they intend to use it.
+// created initially. It allows `order.BeganProcessing` to be nil because
+// `sa.NewOrder` explicitly sets it to the default value. It also allows
+// `order.CertificateSerial` to be nil such that it can be used in places where
+// the order has not been finalized yet.
 func newOrderValid(order *corepb.Order) bool {
-	return !(order.RegistrationID == nil || order.Expires == nil || order.Authorizations == nil || order.Status == nil || order.Names == nil)
+	return !(order.RegistrationID == nil || order.Expires == nil || order.Authorizations == nil || order.Names == nil)
 }
 
 func authorizationValid(authz *corepb.Authorization) bool {

grpc/pb-marshalling_test.go

@@ -339,6 +339,7 @@ func TestOrderValid(t *testing.T) {
 	testID := int64(1)
 	testExpires := int64(1)
 	emptyString := ""
+	falseBool := false
 
 	testCases := []struct {
 		Name          string
@@ -353,20 +354,20 @@ func TestOrderValid(t *testing.T) {
 				Expires:           &testExpires,
 				CertificateSerial: &emptyString,
 				Authorizations:    []string{},
-				Status:            &emptyString,
 				Names:             []string{},
+				BeganProcessing:   &falseBool,
 			},
 			ExpectedValid: true,
 		},
 		{
 			Name: "Serial nil",
 			Order: &corepb.Order{
-				Id:             &testID,
-				RegistrationID: &testID,
-				Expires:        &testExpires,
-				Authorizations: []string{},
-				Status:         &emptyString,
-				Names:          []string{},
+				Id:              &testID,
+				RegistrationID:  &testID,
+				Expires:         &testExpires,
+				Authorizations:  []string{},
+				Names:           []string{},
+				BeganProcessing: &falseBool,
 			},
 			ExpectedValid: true,
 		},
@@ -381,8 +382,8 @@ func TestOrderValid(t *testing.T) {
 				Expires:           &testExpires,
 				CertificateSerial: &emptyString,
 				Authorizations:    []string{},
-				Status:            &emptyString,
 				Names:             []string{},
+				BeganProcessing:   &falseBool,
 			},
 		},
 		{
@@ -392,8 +393,8 @@ func TestOrderValid(t *testing.T) {
 				Expires:           &testExpires,
 				CertificateSerial: &emptyString,
 				Authorizations:    []string{},
-				Status:            &emptyString,
 				Names:             []string{},
+				BeganProcessing:   &falseBool,
 			},
 		},
 		{
@@ -403,8 +404,8 @@ func TestOrderValid(t *testing.T) {
 				RegistrationID:    &testID,
 				CertificateSerial: &emptyString,
 				Authorizations:    []string{},
-				Status:            &emptyString,
 				Names:             []string{},
+				BeganProcessing:   &falseBool,
 			},
 		},
 		{
@@ -414,12 +415,12 @@ func TestOrderValid(t *testing.T) {
 				RegistrationID:    &testID,
 				Expires:           &testExpires,
 				CertificateSerial: &emptyString,
-				Status:            &emptyString,
 				Names:             []string{},
+				BeganProcessing:   &falseBool,
 			},
 		},
 		{
-			Name: "Status nil",
+			Name: "BeganProcessing nil",
 			Order: &corepb.Order{
 				Id:                &testID,
 				RegistrationID:    &testID,
@@ -437,7 +438,7 @@ func TestOrderValid(t *testing.T) {
 				Expires:           &testExpires,
 				CertificateSerial: &emptyString,
 				Authorizations:    []string{},
-				Status:            &emptyString,
+				BeganProcessing:   &falseBool,
 			},
 		},
 	}

ra/ra.go

@@ -1582,12 +1582,10 @@ func (ra *RegistrationAuthorityImpl) DeactivateAuthorization(ctx context.Context
 // NewOrder creates a new order object
 func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.NewOrderRequest) (*corepb.Order, error) {
 	expires := ra.clk.Now().Add(ra.orderLifetime).UnixNano()
-	status := string(core.StatusPending)
 	order := &corepb.Order{
 		RegistrationID: req.RegistrationID,
 		Names:          core.UniqueLowerNames(req.Names),
 		Expires:        &expires,
-		Status:         &status,
 	}
 
 	// Check that the registration ID in question has rate limit space for another
@@ -1656,6 +1654,14 @@ 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.
+		// Reusing pending authorizations between orders is primarily an effort to
+		// prevent clients hitting the pending authz limit, but in a V2 world order
+		// reuse accomplishes the same thing.
+		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
@@ -1708,6 +1714,7 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 	}
 
 	storedOrder, err := ra.SA.NewOrder(ctx, order)
+	fmt.Printf("ra.SA.NewOrder err: %#v\n", err)
 	if err != nil {
 		return nil, err
 	}

ra/ra_test.go

@@ -13,6 +13,7 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"reflect"
 	"sort"
 	"strings"
 	"sync"
@@ -1231,6 +1232,11 @@ func TestNewOrderRateLimiting(t *testing.T) {
 	validCSRBlock, _ := pem.Decode(CSRPEM)
 	validCSR, _ := x509.ParseCertificateRequest(validCSRBlock.Bytes)
 
+	// Artificially set the order's status to pending since it didn't come from
+	// a sa.GetOrder call that populates the field.
+	pendingStatus := string(core.StatusPending)
+	order.Status = &pendingStatus
+
 	// Finalize the order with a CSR to change it from pending status to valid status
 	_, err = ra.FinalizeOrder(ctx, &rapb.FinalizeOrderRequest{
 		Order: order,
@@ -2154,7 +2160,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,
@@ -2266,6 +2279,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) {
 	_, _, ra, _, cleanUp := initAuthorities(t)
 	defer cleanUp()