sa: getAuthorizationStatuses checks results (#8726)

If this function gets fewer responses than expected, that means some of
the authorizations were deleted before their containing order expired
(or were never written due to a bug). Error out.

Fix a test that fails with this extra check in place.

Author: jsha

sa/db/01-boulder_sa.sql

@@ -149,6 +149,8 @@ CREATE TABLE `orders` (
   `created` datetime NOT NULL,
   `certificateProfileName` varchar(32) DEFAULT NULL,
   `replaces` varchar(255) DEFAULT NULL,
+  -- Contains protobuf-encoded list of authorization IDs without duplicates.
+  -- See sa/proto/sadb.proto
   `authzs` blob DEFAULT NULL,
   PRIMARY KEY (`id`),
   KEY `reg_expires` (`registrationID`,`expires`),

sa/db/01-boulder_sa_next.sql

@@ -153,6 +153,8 @@ CREATE TABLE `orders` (
   `created` datetime NOT NULL,
   `certificateProfileName` varchar(32) DEFAULT NULL,
   `replaces` varchar(255) DEFAULT NULL,
+  -- Contains protobuf-encoded list of authorization IDs without duplicates.
+  -- See sa/proto/sadb.proto
   `authzs` blob DEFAULT NULL,
   PRIMARY KEY (`id`),
   KEY `reg_expires` (`registrationID`,`expires`),

sa/model.go

@@ -335,19 +335,22 @@ func (model certificateStatusModel) toPb() *corepb.CertificateStatus {
 	}
 }
 
-// orderModel represents one row in the orders table. The CertificateProfileName
-// column is a pointer because the column is NULL-able.
+// orderModel represents one row in the orders table.
 type orderModel struct {
-	ID                     int64
-	RegistrationID         int64
-	Expires                time.Time
-	Created                time.Time
-	Error                  []byte
-	CertificateSerial      string
-	BeganProcessing        bool
+	ID                int64
+	RegistrationID    int64
+	Expires           time.Time
+	Created           time.Time
+	Error             []byte
+	CertificateSerial string
+	BeganProcessing   bool
+	// CertificateProfileName is a pointer because the column is NULL-able.
 	CertificateProfileName *string
-	Replaces               *string
-	Authzs                 []byte
+	// Replaces is a pointer because the column is NULL-able.
+	Replaces *string
+	// Contains protobuf-encoded list of authorization IDs without duplicates.
+	// See sa/proto/sadb.proto
+	Authzs []byte
 }
 
 func modelToOrder(om *orderModel) (*corepb.Order, error) {
@@ -1154,7 +1157,7 @@ type authzValidity struct {
 	Expires         time.Time `db:"expires"`
 }
 
-// getAuthorizationStatuses takes a sequence of authz IDs, and returns the
+// getAuthorizationStatuses takes a sequence of distinct authz IDs, and returns the
 // status and expiration date of each of them.
 func getAuthorizationStatuses(ctx context.Context, s db.Selector, ids []int64) ([]authzValidity, error) {
 	var params []any
@@ -1173,6 +1176,11 @@ func getAuthorizationStatuses(ctx context.Context, s db.Selector, ids []int64) (
 		return nil, err
 	}
 
+	if len(validities) != len(ids) {
+		return nil, fmt.Errorf("getAuthorizationStatuses got %d results for for %d ids: %v",
+			len(validities), len(ids), ids)
+	}
+
 	return validities, nil
 }
 

sa/proto/sadb.pb.go

@@ -4,6 +4,7 @@ package sa;
 option go_package = "github.com/letsencrypt/boulder/sa/proto";
 
 // Used internally for storage in the DB, not for RPCs.
+// Invariant maintained by sa.NewOrderAndAuthzs: does not contain duplicates.
 message Authzs {
   repeated int64 authzIDs = 1;
 }

sa/proto/sadb.proto

@@ -450,11 +450,16 @@ func (ssa *SQLStorageAuthority) DeactivateAuthorization2(ctx context.Context, re
 	return &emptypb.Empty{}, nil
 }
 
-// NewOrderAndAuthzs adds the given authorizations to the database, adds their
-// autogenerated IDs to the given order, and then adds the order to the db.
-// This is done inside a single transaction to prevent situations where new
-// authorizations are created, but then their corresponding order is never
-// created, leading to "invisible" pending authorizations.
+// NewOrderAndAuthzs creates an order in the database.
+//
+// The order will include reused authorization IDs from the V2Authorizations slice
+// and newly created authorizations based on NewAuthzs slice. Either slice may be
+// empty but not both. Duplicate authorization IDs in V2Authorizations will error.
+//
+// Creation of new authorizations is done inside a transaction along with creation
+// of the order to prevent situations where new authorizations are created, but then
+// their corresponding order is never created, leading to "invisible" pending
+// authorizations.
 func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb.NewOrderAndAuthzsRequest) (*corepb.Order, error) {
 	if req.NewOrder == nil {
 		return nil, errIncompleteRequest
@@ -490,6 +495,10 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 		// Combine the already-existing and newly-created authzs.
 		allAuthzIds := append(req.NewOrder.V2Authorizations, newAuthzIDs...)
 
+		if containsDuplicates(allAuthzIds) {
+			return nil, errors.New("cannot add duplicate authorizations to order")
+		}
+
 		// Second, insert the new order.
 		created := ssa.clk.Now()
 		encodedAuthzs, err := proto.Marshal(&sapb.Authzs{
@@ -577,6 +586,17 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 	return order, nil
 }
 
+func containsDuplicates(ids []int64) bool {
+	seen := make(map[int64]bool, len(ids))
+	for _, id := range ids {
+		if seen[id] {
+			return true
+		}
+		seen[id] = true
+	}
+	return false
+}
+
 // SetOrderProcessing updates an order from pending status to processing
 // status by updating the `beganProcessing` field of the corresponding
 // Order table row in the DB.

sa/sa.go

@@ -1005,6 +1005,33 @@ func TestNewOrderAndAuthzs(t *testing.T) {
 	test.Assert(t, newAuthzIDs[0] != newAuthzIDs[1], "expected distinct new authz IDs")
 }
 
+func TestNewOrderAndAuthzsRejectsDuplicates(t *testing.T) {
+	sa, fc := initSA(t)
+
+	reg := createWorkingRegistration(t, sa)
+
+	idA := createPendingAuthorization(t, sa, reg.Id, identifier.NewDNS("a.com"), sa.clk.Now().Add(time.Hour))
+	_, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
+		NewOrder: &sapb.NewOrderRequest{
+			RegistrationID: reg.Id,
+			Expires:        timestamppb.New(fc.Now().Add(2 * time.Hour)),
+			Identifiers: []*corepb.Identifier{
+				identifier.NewDNS("a.com").ToProto(),
+				identifier.NewDNS("b.com").ToProto(),
+			},
+			V2Authorizations: []int64{idA, idA},
+		},
+	})
+
+	if err == nil {
+		t.Fatal("sa.NewOrderAndAuthzs with duplicate authorizations: got nil error, want error")
+	}
+	expected := "cannot add duplicate authorizations to order"
+	if err.Error() != expected {
+		t.Errorf("sa.NewOrderAndAuthzs with duplicate authorizations: got error %q, want error %q", err, expected)
+	}
+}
+
 func TestNewOrderAndAuthzs_ReuseOnly(t *testing.T) {
 	sa, fc := initSA(t)
 
@@ -2685,12 +2712,21 @@ func TestGetOrderExpired(t *testing.T) {
 	fc.Add(time.Hour * 5)
 	now := fc.Now()
 	reg := createWorkingRegistration(t, sa)
+	exampleDotCom := identifier.NewDNS("example.com").ToProto()
 	order, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
 		NewOrder: &sapb.NewOrderRequest{
-			RegistrationID:   reg.Id,
-			Expires:          timestamppb.New(now.Add(-time.Hour)),
-			Identifiers:      []*corepb.Identifier{identifier.NewDNS("example.com").ToProto()},
-			V2Authorizations: []int64{666},
+			RegistrationID: reg.Id,
+			Expires:        timestamppb.New(now.Add(-time.Hour)),
+			Identifiers:    []*corepb.Identifier{exampleDotCom},
+		},
+		NewAuthzs: []*sapb.NewAuthzRequest{
+			{
+				Identifier:     exampleDotCom,
+				RegistrationID: reg.Id,
+				Expires:        timestamppb.New(now.Add(time.Hour)),
+				ChallengeTypes: []string{string(core.ChallengeTypeHTTP01)},
+				Token:          core.NewToken(),
+			},
 		},
 	})
 	test.AssertNotError(t, err, "NewOrderAndAuthzs failed")