From 387fb555acde9a3dd8e48a03f24a21d7d6f7a88e Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Thu, 16 Apr 2026 21:49:59 +0000 Subject: [PATCH] security: scope revocation unique index to (issuer_id, serial_number) (fixes H-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 4 +- internal/repository/interfaces.go | 11 +- internal/repository/postgres/repo_test.go | 112 +++++++++++++++++- internal/repository/postgres/revocation.go | 21 +++- internal/service/ca_operations.go | 10 +- internal/service/testutil_test.go | 4 +- .../000012_revocation_index_scope.down.sql | 12 ++ .../000012_revocation_index_scope.up.sql | 31 +++++ 8 files changed, 184 insertions(+), 21 deletions(-) create mode 100644 migrations/000012_revocation_index_scope.down.sql create mode 100644 migrations/000012_revocation_index_scope.up.sql diff --git a/internal/integration/lifecycle_test.go b/internal/integration/lifecycle_test.go index 1b3245a..385da76 100644 --- a/internal/integration/lifecycle_test.go +++ b/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 } } diff --git a/internal/repository/interfaces.go b/internal/repository/interfaces.go index 335049d..71716f3 100644 --- a/internal/repository/interfaces.go +++ b/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. - GetBySerial(ctx context.Context, serial string) (*domain.CertificateRevocation, error) + // GetByIssuerAndSerial retrieves a revocation by the (issuer_id, serial_number) + // 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. diff --git a/internal/repository/postgres/repo_test.go b/internal/repository/postgres/repo_test.go index 66d1894..7896151 100644 --- a/internal/repository/postgres/repo_test.go +++ b/internal/repository/postgres/repo_test.go @@ -703,10 +703,10 @@ func TestRevocationRepository_CRUD(t *testing.T) { t.Fatalf("Idempotent create failed: %v", err) } - // GetBySerial - got, err := repo.GetBySerial(ctx, "DEADBEEF01") + // GetByIssuerAndSerial — lookups are scoped to (issuer_id, serial) per RFC 5280 §5.2.3. + 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 // ============================================================ diff --git a/internal/repository/postgres/revocation.go b/internal/repository/postgres/revocation.go index 6cfd413..07cf2c7 100644 --- a/internal/repository/postgres/revocation.go +++ b/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. -func (r *RevocationRepository) GetBySerial(ctx context.Context, serial string) (*domain.CertificateRevocation, error) { +// GetByIssuerAndSerial retrieves a revocation by the (issuer_id, serial) pair. +// +// 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 - `, serial).Scan(&rev.ID, &rev.CertificateID, &rev.SerialNumber, + WHERE issuer_id = $1 AND serial_number = $2 + `, 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 diff --git a/internal/service/ca_operations.go b/internal/service/ca_operations.go index 1de7869..835c6cd 100644 --- a/internal/service/ca_operations.go +++ b/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 - rev, _ := s.revocationRepo.GetBySerial(context.Background(), serialHex) + // Look up cert by (issuer_id, serial) — per RFC 5280 §5.2.3, serial numbers + // 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 - rev, err := s.revocationRepo.GetBySerial(context.Background(), serialHex) + // Check if this (issuer_id, serial) is revoked — RFC 5280 §5.2.3 scoping. + rev, err := s.revocationRepo.GetByIssuerAndSerial(context.Background(), issuerID, serialHex) if err != nil { // Not revoked — return "good" status return issuerConn.SignOCSPResponse(context.Background(), OCSPSignRequest{ diff --git a/internal/service/testutil_test.go b/internal/service/testutil_test.go index 0d6dece..9f1ffb3 100644 --- a/internal/service/testutil_test.go +++ b/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 } } diff --git a/migrations/000012_revocation_index_scope.down.sql b/migrations/000012_revocation_index_scope.down.sql new file mode 100644 index 0000000..1f1c208 --- /dev/null +++ b/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); diff --git a/migrations/000012_revocation_index_scope.up.sql b/migrations/000012_revocation_index_scope.up.sql new file mode 100644 index 0000000..e9428d7 --- /dev/null +++ b/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);