security: scope revocation unique index to (issuer_id, serial_number) (fixes H-1)

RFC 5280 §5.2.3 defines certificate serial number uniqueness per issuing CA,
not globally. The prior unique index on `certificate_revocations.serial_number`
enforced a stricter invariant than the spec: with 12 issuer connectors (Local
CA, ACME, Vault, step-ca, OpenSSL, DigiCert, Sectigo, Google CAS, AWS ACM PCA,
Entrust, GlobalSign, EJBCA), two distinct certificates legitimately issued by
different CAs can share a serial number. Recording a revocation for the second
collision silently dropped via `ON CONFLICT DO NOTHING`, leaving the second
cert persistently absent from OCSP/CRL responses.

Changes:

- Migration 000012 drops `idx_certificate_revocations_serial` and creates
  `idx_certificate_revocations_issuer_serial` UNIQUE ON (issuer_id,
  serial_number). Adds a non-unique `idx_certificate_revocations_serial_lookup`
  to preserve the serial-only fast path for OCSP/CRL probes that already know
  the issuer scope.
- `CertificateRevocationRepository.Create` targets the new composite key in
  `ON CONFLICT` — same-issuer idempotency preserved, cross-issuer collisions
  now recorded as distinct rows.
- `GetBySerial(serial)` renamed `GetByIssuerAndSerial(issuerID, serial)` on
  the interface and Postgres impl. All callers (OCSP responder, CRL
  generator, short-lived-cert exemption check) already have `issuerID` in
  scope because the protocol paths carry it (`/api/v1/ocsp/{issuer_id}/{serial}`,
  `/api/v1/crl/{issuer_id}`).
- Repository integration test added: `TestRevocationRepository_CrossIssuerSerialCollision`
  asserts that serial `CAFEBABE01` can be stored under two issuers
  simultaneously, that lookups return the correct row per (issuer, serial),
  and that same-issuer idempotency still works (re-inserting (issuer, serial)
  does not error and does not duplicate).
- Existing tests and service/integration mocks updated for the rename.

Wire-format invariants preserved: CRL DER bytes, OCSP response bytes, and
AES-256-GCM config encryption are unaffected — this change touches only
revocation-record uniqueness scope.

CWE-664.

internal/integration/lifecycle_test.go

@@ -1139,9 +1139,9 @@ func (m *mockRevocationRepository) Create(ctx context.Context, revocation *domai
 	return nil
 }
 
-func (m *mockRevocationRepository) GetBySerial(ctx context.Context, serial string) (*domain.CertificateRevocation, error) {
+func (m *mockRevocationRepository) GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.CertificateRevocation, error) {
 	for _, r := range m.revocations {
-		if r.SerialNumber == serial {
+		if r.IssuerID == issuerID && r.SerialNumber == serial {
 			return r, nil
 		}
 	}

internal/repository/interfaces.go

@@ -31,10 +31,15 @@ type CertificateRepository interface {
 
 // RevocationRepository defines operations for managing certificate revocations.
 type RevocationRepository interface {
-	// Create records a new certificate revocation.
+	// Create records a new certificate revocation. Uniqueness is scoped to
+	// (issuer_id, serial_number) per RFC 5280 §5.2.3, so duplicate serials
+	// across different issuers are permitted.
 	Create(ctx context.Context, revocation *domain.CertificateRevocation) error
-	// GetBySerial retrieves a revocation by serial number.
+	// GetByIssuerAndSerial retrieves a revocation by the (issuer_id, serial_number)
-	GetBySerial(ctx context.Context, serial string) (*domain.CertificateRevocation, error)
+	// pair. Callers (OCSP, CRL generation) always know the issuer because
+	// protocol endpoints carry it in the request path; RFC 5280 §5.2.3 guarantees
+	// uniqueness only within a single issuer.
+	GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.CertificateRevocation, error)
 	// ListAll returns all revocations, ordered by revocation time (for CRL generation).
 	ListAll(ctx context.Context) ([]*domain.CertificateRevocation, error)
 	// ListByCertificate returns all revocations for a certificate.

internal/repository/postgres/repo_test.go

@@ -703,10 +703,10 @@ func TestRevocationRepository_CRUD(t *testing.T) {
 		t.Fatalf("Idempotent create failed: %v", err)
 	}
 
-	// GetBySerial
+	// GetByIssuerAndSerial — lookups are scoped to (issuer_id, serial) per RFC 5280 §5.2.3.
-	got, err := repo.GetBySerial(ctx, "DEADBEEF01")
+	got, err := repo.GetByIssuerAndSerial(ctx, issuerID, "DEADBEEF01")
 	if err != nil {
-		t.Fatalf("GetBySerial failed: %v", err)
+		t.Fatalf("GetByIssuerAndSerial failed: %v", err)
 	}
 	if got.Reason != "keyCompromise" {
 		t.Errorf("Reason = %q, want %q", got.Reason, "keyCompromise")
@@ -734,12 +734,116 @@ func TestRevocationRepository_CRUD(t *testing.T) {
 	if err := repo.MarkIssuerNotified(ctx, "rev-test-1"); err != nil {
 		t.Fatalf("MarkIssuerNotified failed: %v", err)
 	}
-	got, _ = repo.GetBySerial(ctx, "DEADBEEF01")
+	got, _ = repo.GetByIssuerAndSerial(ctx, issuerID, "DEADBEEF01")
 	if !got.IssuerNotified {
 		t.Error("expected IssuerNotified=true after marking")
 	}
 }
 
+// TestRevocationRepository_CrossIssuerSerialCollision verifies that the same
+// serial number can coexist under two different issuers — RFC 5280 §5.2.3
+// defines serial uniqueness only within a single CA, and certctl supports
+// multi-issuer deployments where serial collisions across issuers are
+// legitimate (e.g., Local CA serial 0x01 and Vault PKI serial 0x01).
+//
+// This test locks in the behavior change from migration 000012: the unique
+// index is on (issuer_id, serial_number), not on serial_number alone.
+func TestRevocationRepository_CrossIssuerSerialCollision(t *testing.T) {
+	tdb := getTestDB(t)
+	db := tdb.freshSchema(t)
+	repo := postgres.NewRevocationRepository(db)
+	certRepo := postgres.NewCertificateRepository(db)
+	ctx := context.Background()
+
+	now := time.Now().Truncate(time.Microsecond)
+
+	// First issuer + cert + revocation with serial "CAFEBABE01".
+	ownerID1, teamID1, issuerID1, policyID1 := insertCertPrereqsRaw(t, db, ctx, "dup-a")
+	cert1 := &domain.ManagedCertificate{
+		ID: "mc-dup-a", Name: "dup-a", CommonName: "a.example.com",
+		SANs: []string{}, OwnerID: ownerID1, TeamID: teamID1,
+		IssuerID: issuerID1, RenewalPolicyID: policyID1,
+		Status:    domain.CertificateStatusRevoked,
+		ExpiresAt: now.Add(30 * 24 * time.Hour), Tags: map[string]string{},
+		CreatedAt: now, UpdatedAt: now,
+	}
+	if err := certRepo.Create(ctx, cert1); err != nil {
+		t.Fatalf("Create cert1 failed: %v", err)
+	}
+	if err := repo.Create(ctx, &domain.CertificateRevocation{
+		ID: "rev-dup-a", CertificateID: "mc-dup-a", SerialNumber: "CAFEBABE01",
+		Reason: "keyCompromise", RevokedBy: "admin", RevokedAt: now,
+		IssuerID: issuerID1, CreatedAt: now,
+	}); err != nil {
+		t.Fatalf("Create revocation under issuer1 failed: %v", err)
+	}
+
+	// Second issuer + cert + revocation with the SAME serial "CAFEBABE01".
+	// Under the pre-000012 global-unique index this would silently drop via
+	// ON CONFLICT DO NOTHING. Under the new (issuer_id, serial_number) scope
+	// it must succeed.
+	ownerID2, teamID2, issuerID2, policyID2 := insertCertPrereqsRaw(t, db, ctx, "dup-b")
+	cert2 := &domain.ManagedCertificate{
+		ID: "mc-dup-b", Name: "dup-b", CommonName: "b.example.com",
+		SANs: []string{}, OwnerID: ownerID2, TeamID: teamID2,
+		IssuerID: issuerID2, RenewalPolicyID: policyID2,
+		Status:    domain.CertificateStatusRevoked,
+		ExpiresAt: now.Add(30 * 24 * time.Hour), Tags: map[string]string{},
+		CreatedAt: now, UpdatedAt: now,
+	}
+	if err := certRepo.Create(ctx, cert2); err != nil {
+		t.Fatalf("Create cert2 failed: %v", err)
+	}
+	if err := repo.Create(ctx, &domain.CertificateRevocation{
+		ID: "rev-dup-b", CertificateID: "mc-dup-b", SerialNumber: "CAFEBABE01",
+		Reason: "superseded", RevokedBy: "admin", RevokedAt: now,
+		IssuerID: issuerID2, CreatedAt: now,
+	}); err != nil {
+		t.Fatalf("Create revocation under issuer2 failed (cross-issuer duplicate serial must be allowed): %v", err)
+	}
+
+	// Both revocations must be retrievable under their respective issuers.
+	revA, err := repo.GetByIssuerAndSerial(ctx, issuerID1, "CAFEBABE01")
+	if err != nil {
+		t.Fatalf("GetByIssuerAndSerial(issuer1) failed: %v", err)
+	}
+	if revA.ID != "rev-dup-a" || revA.Reason != "keyCompromise" {
+		t.Errorf("issuer1 lookup returned wrong row: id=%q reason=%q", revA.ID, revA.Reason)
+	}
+
+	revB, err := repo.GetByIssuerAndSerial(ctx, issuerID2, "CAFEBABE01")
+	if err != nil {
+		t.Fatalf("GetByIssuerAndSerial(issuer2) failed: %v", err)
+	}
+	if revB.ID != "rev-dup-b" || revB.Reason != "superseded" {
+		t.Errorf("issuer2 lookup returned wrong row: id=%q reason=%q", revB.ID, revB.Reason)
+	}
+
+	// ListAll should see both revocations.
+	all, err := repo.ListAll(ctx)
+	if err != nil {
+		t.Fatalf("ListAll failed: %v", err)
+	}
+	if len(all) != 2 {
+		t.Errorf("len(all) = %d, want 2 (cross-issuer duplicate serials)", len(all))
+	}
+
+	// Same-issuer idempotency guard still works (ON CONFLICT DO NOTHING on
+	// (issuer_id, serial_number) — re-inserting the same (issuer, serial)
+	// pair must not error and must not duplicate the row).
+	if err := repo.Create(ctx, &domain.CertificateRevocation{
+		ID: "rev-dup-a-repeat", CertificateID: "mc-dup-a", SerialNumber: "CAFEBABE01",
+		Reason: "superseded", RevokedBy: "admin", RevokedAt: now,
+		IssuerID: issuerID1, CreatedAt: now,
+	}); err != nil {
+		t.Fatalf("Idempotent create under same issuer failed: %v", err)
+	}
+	all, _ = repo.ListAll(ctx)
+	if len(all) != 2 {
+		t.Errorf("len(all) after idempotent re-insert = %d, want 2", len(all))
+	}
+}
+
 // ============================================================
 // Team Repository Tests
 // ============================================================

internal/repository/postgres/revocation.go

@@ -19,13 +19,18 @@ func NewRevocationRepository(db *sql.DB) *RevocationRepository {
 }
 
 // Create records a new certificate revocation.
+//
+// Uniqueness is scoped to (issuer_id, serial_number) per RFC 5280 §5.2.3.
+// Serial numbers are only unique within an issuer, so certctl supports
+// collisions across different issuer connectors. The composite ON CONFLICT
+// target matches migration 000012's unique index.
 func (r *RevocationRepository) Create(ctx context.Context, revocation *domain.CertificateRevocation) error {
 	_, err := r.db.ExecContext(ctx, `
 		INSERT INTO certificate_revocations (
 			id, certificate_id, serial_number, reason, revoked_by, revoked_at,
 			issuer_id, issuer_notified, created_at
 		) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
-		ON CONFLICT (serial_number) DO NOTHING
+		ON CONFLICT (issuer_id, serial_number) DO NOTHING
 	`, revocation.ID, revocation.CertificateID, revocation.SerialNumber,
 		revocation.Reason, revocation.RevokedBy, revocation.RevokedAt,
 		revocation.IssuerID, revocation.IssuerNotified, revocation.CreatedAt)
@@ -37,20 +42,24 @@ func (r *RevocationRepository) Create(ctx context.Context, revocation *domain.Ce
 	return nil
 }
 
-// GetBySerial retrieves a revocation by serial number.
+// GetByIssuerAndSerial retrieves a revocation by the (issuer_id, serial) pair.
-func (r *RevocationRepository) GetBySerial(ctx context.Context, serial string) (*domain.CertificateRevocation, error) {
+//
+// Per RFC 5280 §5.2.3, serial numbers are unique only within a single issuer.
+// Callers (OCSP handlers, CRL generation) always know the issuer because the
+// OCSP URL carries it as a path parameter and CRLs are generated per-issuer.
+func (r *RevocationRepository) GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.CertificateRevocation, error) {
 	var rev domain.CertificateRevocation
 	err := r.db.QueryRowContext(ctx, `
 		SELECT id, certificate_id, serial_number, reason, revoked_by, revoked_at,
 		       issuer_id, issuer_notified, created_at
 		FROM certificate_revocations
-		WHERE serial_number = $1
+		WHERE issuer_id = $1 AND serial_number = $2
-	`, serial).Scan(&rev.ID, &rev.CertificateID, &rev.SerialNumber,
+	`, issuerID, serial).Scan(&rev.ID, &rev.CertificateID, &rev.SerialNumber,
 		&rev.Reason, &rev.RevokedBy, &rev.RevokedAt,
 		&rev.IssuerID, &rev.IssuerNotified, &rev.CreatedAt)
 
 	if err != nil {
-		return nil, fmt.Errorf("failed to get revocation by serial: %w", err)
+		return nil, fmt.Errorf("failed to get revocation by issuer and serial: %w", err)
 	}
 
 	return &rev, nil

internal/service/ca_operations.go

@@ -117,8 +117,10 @@ func (s *CAOperationsSvc) GetOCSPResponse(issuerID string, serialHex string) ([]
 	// Short-lived cert exemption: if the cert's profile has TTL < 1 hour,
 	// always return "good" — expiry is sufficient revocation for short-lived certs.
 	if s.profileRepo != nil && s.certRepo != nil {
-		// Look up cert by serial through revocation table
+		// Look up cert by (issuer_id, serial) — per RFC 5280 §5.2.3, serial numbers
-		rev, _ := s.revocationRepo.GetBySerial(context.Background(), serialHex)
+		// are unique only within a single issuer. The OCSP URL path carries issuer_id,
+		// so we scope the lookup to avoid cross-issuer collisions.
+		rev, _ := s.revocationRepo.GetByIssuerAndSerial(context.Background(), issuerID, serialHex)
 		if rev != nil {
 			cert, err := s.certRepo.Get(context.Background(), rev.CertificateID)
 			if err == nil && cert.CertificateProfileID != "" {
@@ -135,8 +137,8 @@ func (s *CAOperationsSvc) GetOCSPResponse(issuerID string, serialHex string) ([]
 		}
 	}
 
-	// Check if this serial is revoked
+	// Check if this (issuer_id, serial) is revoked — RFC 5280 §5.2.3 scoping.
-	rev, err := s.revocationRepo.GetBySerial(context.Background(), serialHex)
+	rev, err := s.revocationRepo.GetByIssuerAndSerial(context.Background(), issuerID, serialHex)
 	if err != nil {
 		// Not revoked — return "good" status
 		return issuerConn.SignOCSPResponse(context.Background(), OCSPSignRequest{

internal/service/testutil_test.go

@@ -929,9 +929,9 @@ func (m *mockRevocationRepo) Create(ctx context.Context, revocation *domain.Cert
 	return nil
 }
 
-func (m *mockRevocationRepo) GetBySerial(ctx context.Context, serial string) (*domain.CertificateRevocation, error) {
+func (m *mockRevocationRepo) GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.CertificateRevocation, error) {
 	for _, r := range m.Revocations {
-		if r.SerialNumber == serial {
+		if r.IssuerID == issuerID && r.SerialNumber == serial {
 			return r, nil
 		}
 	}

migrations/000012_revocation_index_scope.down.sql

@@ -0,0 +1,12 @@
+-- Rollback Migration 000012: Restore global-serial uniqueness.
+--
+-- Reverts to the pre-000012 behavior: uniqueness on `serial_number` alone.
+-- Operators must ensure no duplicate serial_numbers exist across different
+-- issuers before rolling back, otherwise the unique-index creation will fail.
+
+DROP INDEX IF EXISTS idx_certificate_revocations_serial_lookup;
+
+DROP INDEX IF EXISTS idx_certificate_revocations_issuer_serial;
+
+CREATE UNIQUE INDEX IF NOT EXISTS idx_certificate_revocations_serial
+    ON certificate_revocations(serial_number);

migrations/000012_revocation_index_scope.up.sql

@@ -0,0 +1,31 @@
+-- Migration 000012: Scope Revocation Uniqueness to (issuer_id, serial_number)
+--
+-- RFC 5280 §5.2.3 defines certificate serial number uniqueness per issuing CA.
+-- The prior global-unique index on `certificate_revocations.serial_number` was
+-- too strict: certctl supports multiple issuer connectors (Local CA, Vault,
+-- DigiCert, Sectigo, Google CAS, AWS ACM PCA, step-ca, Entrust, GlobalSign,
+-- EJBCA, ACME, OpenSSL), and different CAs legitimately issue distinct certs
+-- that share a serial-number value. Under the old index, recording a
+-- revocation for such a collision silently dropped via ON CONFLICT DO NOTHING.
+--
+-- This migration scopes uniqueness to the (issuer_id, serial_number) pair,
+-- which matches RFC 5280 and the revocation-recording call site's intent
+-- (see RevocationSvc.RevokeCertificateWithActor, which already populates
+-- IssuerID at Create time).
+--
+-- Duplicate detection: if any row pairs exist with identical (issuer_id,
+-- serial_number), the unique-index creation will fail — this is intentional.
+-- Operators must resolve duplicates manually before re-running the migration.
+
+-- Drop the overly broad global-serial unique index.
+DROP INDEX IF EXISTS idx_certificate_revocations_serial;
+
+-- Recreate uniqueness scoped to (issuer_id, serial_number) per RFC 5280 §5.2.3.
+CREATE UNIQUE INDEX IF NOT EXISTS idx_certificate_revocations_issuer_serial
+    ON certificate_revocations(issuer_id, serial_number);
+
+-- Preserve fast serial-only lookup for OCSP/CRL paths that search within a
+-- known issuer scope. Non-unique — uniqueness is enforced by the composite
+-- index above.
+CREATE INDEX IF NOT EXISTS idx_certificate_revocations_serial_lookup
+    ON certificate_revocations(serial_number);