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

@@ -1585,12 +1585,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
@@ -1659,6 +1657,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