diff --git a/sa/db/01-boulder_sa.sql b/sa/db/01-boulder_sa.sql index 15c6efa0375..ed3b1b39b8c 100644 --- a/sa/db/01-boulder_sa.sql +++ b/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`), diff --git a/sa/db/01-boulder_sa_next.sql b/sa/db/01-boulder_sa_next.sql index 4232c62d5f2..bd39c37a948 100644 --- a/sa/db/01-boulder_sa_next.sql +++ b/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`), diff --git a/sa/model.go b/sa/model.go index f3e0904e72e..8a60718c71c 100644 --- a/sa/model.go +++ b/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 } diff --git a/sa/proto/sadb.pb.go b/sa/proto/sadb.pb.go index 184a01df3bd..bc5472e03f2 100644 --- a/sa/proto/sadb.pb.go +++ b/sa/proto/sadb.pb.go @@ -22,6 +22,7 @@ const ( ) // Used internally for storage in the DB, not for RPCs. +// Invariant maintained by sa.NewOrderAndAuthzs: does not contain duplicates. type Authzs struct { state protoimpl.MessageState `protogen:"open.v1"` AuthzIDs []int64 `protobuf:"varint,1,rep,packed,name=authzIDs,proto3" json:"authzIDs,omitempty"` diff --git a/sa/proto/sadb.proto b/sa/proto/sadb.proto index 353993b14ee..1c417369804 100644 --- a/sa/proto/sadb.proto +++ b/sa/proto/sadb.proto @@ -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; } diff --git a/sa/sa.go b/sa/sa.go index 892aac7914c..3c6b9472d1c 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -451,11 +451,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 @@ -491,6 +496,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{ @@ -578,6 +587,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. diff --git a/sa/sa_test.go b/sa/sa_test.go index e6183c8e542..06f44603d7c 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.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")