From 0c9de9e0f15502883ec0e049d41c56669b1efad6 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 22 Apr 2026 15:29:43 -0700 Subject: [PATCH 1/3] sa: getAuthorizationStatuses checks results 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. --- sa/model.go | 4 ++++ sa/sa_test.go | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/sa/model.go b/sa/model.go index f3e0904e72e..99491e7e83a 100644 --- a/sa/model.go +++ b/sa/model.go @@ -1173,6 +1173,10 @@ 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, expected %d", len(validities), len(ids)) + } + return validities, nil } diff --git a/sa/sa_test.go b/sa/sa_test.go index e6183c8e542..2166e93e8f2 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -2687,10 +2687,18 @@ func TestGetOrderExpired(t *testing.T) { reg := createWorkingRegistration(t, sa) 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{identifier.NewDNS("example.com").ToProto()}, + }, + NewAuthzs: []*sapb.NewAuthzRequest{ + { + Identifier: &corepb.Identifier{Type: "dns", Value: "example.com"}, + RegistrationID: reg.Id, + Expires: timestamppb.New(now.Add(time.Hour)), + ChallengeTypes: []string{string(core.ChallengeTypeHTTP01)}, + Token: core.NewToken(), + }, }, }) test.AssertNotError(t, err, "NewOrderAndAuthzs failed") From e842d1448b4f7e0d336c4b63b70c1991d21894a5 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 6 May 2026 09:56:10 -0700 Subject: [PATCH 2/3] sa: orders don't contain dupe authzs --- sa/db/01-boulder_sa.sql | 2 ++ sa/db/01-boulder_sa_next.sql | 2 ++ sa/model.go | 30 +++++++++++++++++------------- sa/proto/sadb.proto | 1 + sa/sa.go | 30 +++++++++++++++++++++++++----- sa/sa_test.go | 32 ++++++++++++++++++++++++++++++-- 6 files changed, 77 insertions(+), 20 deletions(-) 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 99491e7e83a..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 @@ -1174,7 +1177,8 @@ func getAuthorizationStatuses(ctx context.Context, s db.Selector, ids []int64) ( } if len(validities) != len(ids) { - return nil, fmt.Errorf("getAuthorizationStatuses got %d results, expected %d", 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.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 2166e93e8f2..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,15 +2712,16 @@ 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()}, + Identifiers: []*corepb.Identifier{exampleDotCom}, }, NewAuthzs: []*sapb.NewAuthzRequest{ { - Identifier: &corepb.Identifier{Type: "dns", Value: "example.com"}, + Identifier: exampleDotCom, RegistrationID: reg.Id, Expires: timestamppb.New(now.Add(time.Hour)), ChallengeTypes: []string{string(core.ChallengeTypeHTTP01)}, From 0f57346055ea4e0809f638125e0252584c92231a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 6 May 2026 10:02:41 -0700 Subject: [PATCH 3/3] Update proto --- sa/proto/sadb.pb.go | 1 + 1 file changed, 1 insertion(+) 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"`