Skip to content

Commit 96dd5d8

Browse files
authored
sa: remove Update from UpdateRevokedCertificate (#8631)
Replace it with a plain `UPDATE`. At the same time, remove a fallback that is no longer relevant (insert into revokedCertificates if the row doesn't exist). Part of #8630
1 parent 0298410 commit 96dd5d8

2 files changed

Lines changed: 19 additions & 25 deletions

File tree

sa/sa.go

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -910,37 +910,31 @@ func (ssa *SQLStorageAuthority) UpdateRevokedCertificate(ctx context.Context, re
910910
return nil, berrors.InternalServerError("no certificate with serial %s and revoked reason other than keyCompromise", req.Serial)
911911
}
912912

913-
var rcm revokedCertModel
914913
// Note: this query MUST be updated to enforce the same preconditions as
915914
// the "UPDATE certificateStatus SET revokedReason..." above if this
916915
// query ever becomes the first or only query in this transaction. We are
917916
// currently relying on the query above to exit early if the certificate
918917
// does not have an appropriate status and revocation reason.
919-
err = tx.SelectOne(
920-
ctx, &rcm, `SELECT * FROM revokedCertificates WHERE serial = ?`, req.Serial)
921-
if db.IsNoRows(err) {
922-
// TODO: Remove this fallback codepath once we know that all unexpired
923-
// certs marked as revoked in the certificateStatus table have
924-
// corresponding rows in the revokedCertificates table. That should be
925-
// 90+ days after the RA starts sending ShardIdx in its
926-
// RevokeCertificateRequest messages.
927-
err = addRevokedCertificate(ctx, tx, req, revokedDate)
928-
if err != nil {
929-
return nil, err
930-
}
931-
return nil, nil
932-
} else if err != nil {
933-
return nil, fmt.Errorf("retrieving revoked certificate row: %w", err)
934-
}
935-
936-
if rcm.ShardIdx != req.ShardIdx {
937-
return nil, berrors.InternalServerError("mismatched shard index %d != %d", req.ShardIdx, rcm.ShardIdx)
918+
res, err = tx.ExecContext(ctx,
919+
`UPDATE revokedCertificates
920+
SET revokedReason = ?
921+
WHERE serial = ?
922+
AND shardIdx = ?`,
923+
revocation.KeyCompromise,
924+
req.Serial,
925+
req.ShardIdx)
926+
if err != nil {
927+
return nil, fmt.Errorf("updating revoked certificate row: %w", err)
938928
}
939929

940-
rcm.RevokedReason = revocation.KeyCompromise
941-
_, err = tx.Update(ctx, &rcm)
930+
rows, err = res.RowsAffected()
942931
if err != nil {
943-
return nil, fmt.Errorf("updating revoked certificate row: %w", err)
932+
return nil, err
933+
}
934+
if rows == 0 {
935+
// InternalServerError because we expected this serial / shardIdx to exist.
936+
return nil, berrors.InternalServerError("no certificate with serial %s in CRL shard %d",
937+
req.Serial, req.ShardIdx)
944938
}
945939

946940
return nil, nil
@@ -1245,7 +1239,7 @@ func (ssa *SQLStorageAuthority) UpdateCRLShard(ctx context.Context, req *sapb.Up
12451239

12461240
rowsAffected, err := res.RowsAffected()
12471241
if err != nil {
1248-
return nil, err
1242+
return nil, fmt.Errorf("confirming update of selected shard: %w", err)
12491243
}
12501244
if rowsAffected == 0 {
12511245
return nil, fmt.Errorf("unable to update shard %d for issuer %d; possibly because shard exists", req.ShardIdx, req.IssuerNameID)

sa/sa_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2038,7 +2038,7 @@ func TestUpdateRevokedCertificate(t *testing.T) {
20382038
ShardIdx: 2,
20392039
})
20402040
test.AssertError(t, err, "UpdateRevokedCertificate should have failed")
2041-
test.AssertContains(t, err.Error(), "mismatched shard index")
2041+
test.AssertContains(t, err.Error(), "no certificate with")
20422042

20432043
// Try to update its revocation info correctly
20442044
_, err = sa.UpdateRevokedCertificate(context.Background(), &sapb.RevokeCertificateRequest{

0 commit comments

Comments
 (0)