From 11a6549bb3cbf73399b09af5006c71528fdb40e1 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 24 Sep 2025 15:10:52 -0700 Subject: [PATCH 1/5] Add AlreadyRevoked handling to revokedCertificates table inserts --- sa/sa.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sa/sa.go b/sa/sa.go index 857b3cd53b8..5abb1d11fdf 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -835,6 +835,12 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke NotAfterHour: serial.Expires.Add(time.Hour).Truncate(time.Hour), }) if err != nil { + if db.IsDuplicate(err) { + // An attempted duplicate insert means that this certificate was already + // revoked. The RA has special logic for that case, so use the specific + // error for it. + return berrors.AlreadyRevokedError("certificate with serial %s already in revokedCertificates table", req.Serial) + } return fmt.Errorf("inserting revoked certificate row: %w", err) } From b0a65bd6cdb767a43f8bcaf9cdd643b2aa608d61 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 2 Oct 2025 17:01:53 -0700 Subject: [PATCH 2/5] Replace IsDuplicate with an explicit SELECT-then-INSERT --- sa/sa.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index d43c034d61a..14278e240f6 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -786,15 +786,34 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke return errors.New("cannot add revoked certificate with shard index 0") } + // If this serial already exists in the revokedCertificates table, then the + // certificate has already been revoked. The RA has special logic for that + // case, so return the specific error that the RA is looking for. + var row struct { + Count int64 + } + err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM revokedCertificates WHERE serial = ?", req.Serial) + if err != nil { + return fmt.Errorf("checking if certificate is already revoked: %w", err) + } + if row.Count > 0 { + return berrors.AlreadyRevokedError("serial %s already in revokedCertificates table", req.Serial) + } + + // The revokedCertificates table includes each cert's notAfter to make it easy + // for crl-updater to stop including entries after they've expired, so we need + // to look up that value. var serial struct { Expires time.Time } - err := tx.SelectOne( - ctx, &serial, `SELECT expires FROM serials WHERE serial = ?`, req.Serial) + err = tx.SelectOne(ctx, &serial, `SELECT expires FROM serials WHERE serial = ?`, req.Serial) if err != nil { return fmt.Errorf("retrieving revoked certificate expiration: %w", err) } + // We're guaranteed that this won't be a duplicate insert because we've + // already checked for existence of a row with the same serial above, in the + // same transaction. err = tx.Insert(ctx, &revokedCertModel{ IssuerID: req.IssuerID, Serial: req.Serial, @@ -806,12 +825,6 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke NotAfterHour: serial.Expires.Add(time.Hour).Truncate(time.Hour), }) if err != nil { - if db.IsDuplicate(err) { - // An attempted duplicate insert means that this certificate was already - // revoked. The RA has special logic for that case, so use the specific - // error for it. - return berrors.AlreadyRevokedError("certificate with serial %s already in revokedCertificates table", req.Serial) - } return fmt.Errorf("inserting revoked certificate row: %w", err) } From 880055ed1a561a0ac1e258c9b90d0d17d7e84dc3 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 20 Oct 2025 13:13:05 -0700 Subject: [PATCH 3/5] FOR UPDATE --- sa/sa.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index 14278e240f6..6a65c5a71ff 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -792,7 +792,7 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke var row struct { Count int64 } - err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM revokedCertificates WHERE serial = ?", req.Serial) + err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM revokedCertificates WHERE serial = ? FOR UPDATE", req.Serial) if err != nil { return fmt.Errorf("checking if certificate is already revoked: %w", err) } @@ -812,8 +812,8 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke } // We're guaranteed that this won't be a duplicate insert because we've - // already checked for existence of a row with the same serial above, in the - // same transaction. + // already checked for existence of a row with the same serial above, using + // a FOR UPDATE select in this same transaction. err = tx.Insert(ctx, &revokedCertModel{ IssuerID: req.IssuerID, Serial: req.Serial, From d444864a96d8ae77c51510a0fa7f2070d4c12742 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Tue, 21 Oct 2025 17:45:02 -0700 Subject: [PATCH 4/5] Use a unique index and duplicate detection --- .../20251002000000_AddRevokedSerialsIndex.sql | 9 ------ .../20251021000000_AddRevokedSerialsIndex.sql | 12 ++++++++ sa/sa.go | 29 +++++-------------- 3 files changed, 20 insertions(+), 30 deletions(-) delete mode 100644 sa/db-next/boulder_sa/20251002000000_AddRevokedSerialsIndex.sql create mode 100644 sa/db-next/boulder_sa/20251021000000_AddRevokedSerialsIndex.sql diff --git a/sa/db-next/boulder_sa/20251002000000_AddRevokedSerialsIndex.sql b/sa/db-next/boulder_sa/20251002000000_AddRevokedSerialsIndex.sql deleted file mode 100644 index 44815cc4edf..00000000000 --- a/sa/db-next/boulder_sa/20251002000000_AddRevokedSerialsIndex.sql +++ /dev/null @@ -1,9 +0,0 @@ --- +migrate Up --- SQL in section 'Up' is executed when this migration is applied - -ALTER TABLE `revokedCertificates` ADD KEY `serial` (`serial`); - --- +migrate Down --- SQL section 'Down' is executed when this migration is rolled back - -ALTER TABLE `revokedCertificates` DROP KEY `serial`; diff --git a/sa/db-next/boulder_sa/20251021000000_AddRevokedSerialsIndex.sql b/sa/db-next/boulder_sa/20251021000000_AddRevokedSerialsIndex.sql new file mode 100644 index 00000000000..87ab4e3aa5c --- /dev/null +++ b/sa/db-next/boulder_sa/20251021000000_AddRevokedSerialsIndex.sql @@ -0,0 +1,12 @@ +-- +migrate Up +-- SQL in section 'Up' is executed when this migration is applied + +ALTER TABLE `revokedCertificates` REMOVE PARTITIONING; +ALTER TABLE `revokedCertificates` ADD UNIQUE INDEX `serial` (`serial`); + +-- +migrate Down +-- SQL section 'Down' is executed when this migration is rolled back + +ALTER TABLE `revokedCertificates` DROP UNIQUE INDEX `serial`; +ALTER TABLE `revokedCertificates` PARTITION BY RANGE(id) +(PARTITION p_start VALUES LESS THAN (MAXVALUE)); diff --git a/sa/sa.go b/sa/sa.go index 6a65c5a71ff..d43c034d61a 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -786,34 +786,15 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke return errors.New("cannot add revoked certificate with shard index 0") } - // If this serial already exists in the revokedCertificates table, then the - // certificate has already been revoked. The RA has special logic for that - // case, so return the specific error that the RA is looking for. - var row struct { - Count int64 - } - err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM revokedCertificates WHERE serial = ? FOR UPDATE", req.Serial) - if err != nil { - return fmt.Errorf("checking if certificate is already revoked: %w", err) - } - if row.Count > 0 { - return berrors.AlreadyRevokedError("serial %s already in revokedCertificates table", req.Serial) - } - - // The revokedCertificates table includes each cert's notAfter to make it easy - // for crl-updater to stop including entries after they've expired, so we need - // to look up that value. var serial struct { Expires time.Time } - err = tx.SelectOne(ctx, &serial, `SELECT expires FROM serials WHERE serial = ?`, req.Serial) + err := tx.SelectOne( + ctx, &serial, `SELECT expires FROM serials WHERE serial = ?`, req.Serial) if err != nil { return fmt.Errorf("retrieving revoked certificate expiration: %w", err) } - // We're guaranteed that this won't be a duplicate insert because we've - // already checked for existence of a row with the same serial above, using - // a FOR UPDATE select in this same transaction. err = tx.Insert(ctx, &revokedCertModel{ IssuerID: req.IssuerID, Serial: req.Serial, @@ -825,6 +806,12 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke NotAfterHour: serial.Expires.Add(time.Hour).Truncate(time.Hour), }) if err != nil { + if db.IsDuplicate(err) { + // An attempted duplicate insert means that this certificate was already + // revoked. The RA has special logic for that case, so use the specific + // error for it. + return berrors.AlreadyRevokedError("certificate with serial %s already in revokedCertificates table", req.Serial) + } return fmt.Errorf("inserting revoked certificate row: %w", err) } From cdc763062b0866621b7fca2560ab23c909ea47a1 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Tue, 28 Oct 2025 16:22:24 -0700 Subject: [PATCH 5/5] Simplify error message --- sa/sa.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sa/sa.go b/sa/sa.go index d43c034d61a..9266d4031dc 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -810,7 +810,7 @@ func addRevokedCertificate(ctx context.Context, tx db.Executor, req *sapb.Revoke // An attempted duplicate insert means that this certificate was already // revoked. The RA has special logic for that case, so use the specific // error for it. - return berrors.AlreadyRevokedError("certificate with serial %s already in revokedCertificates table", req.Serial) + return berrors.AlreadyRevokedError("certificate already revoked") } return fmt.Errorf("inserting revoked certificate row: %w", err) }