From 4e5522a999c702eb126a14be072ca542e78f05ef Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 20 Apr 2026 16:51:52 +0000 Subject: [PATCH] F-001/F-002/F-003: CRL prefix-scan, digest error sanitization, ctx-aware sleeps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F-001 (P3): GenerateDERCRL scoped to issuer via composite index - Add RevocationRepository.ListByIssuer leveraging migration 000012's idx_certificate_revocations_issuer_serial composite index as a prefix-scan target. Previously CAOperationsSvc.GenerateDERCRL called ListAll() and filtered by IssuerID in Go — O(total revocations) regardless of how many revocations belonged to the target issuer. - Rewrite GenerateDERCRL to call ListByIssuer(ctx, issuerID) so PostgreSQL drives a prefix scan of the composite index. Drops the in-memory filter. - New regression test in ca_operations_test.go asserts the CRL hot path invokes ListByIssuer exactly once and never ListAll, and that the issuerID is threaded through correctly. F-002 (P3): digest.go admin-auth endpoints no longer leak internal errors - PreviewDigest (GET /api/v1/digest/preview) and SendDigest (POST /api/v1/digest/send) previously wrote err.Error() into the HTTP response body on 500s. Replace with slog.Error server-side logging plus a generic "internal error" response body, matching the house pattern in certificates.go and export.go. F-003 (P4): three blocking time.Sleep sites now honor ctx cancellation - internal/connector/issuer/acme/acme.go:672 (DNS-01 propagation wait) now runs under a select{case <-ctx.Done(): CleanUp + return ctx.Err(); case <-time.After(d):} so graceful shutdown doesn't get stuck behind the propagation delay. - internal/connector/issuer/acme/acme.go:786 (dns-persist-01 propagation wait) same pattern, returns ctx.Err() on cancel. - cmd/agent/main.go:272 (polling backoff inside the heartbeat loop) now wraps the sleep in select{case <-ctx.Done(): continue; case <-time.After(backoff):} so the outer <-ctx.Done() case on the parent loop fires cleanly. Verification: build, vet, and race-enabled short tests green across all 55+ packages. govulncheck reports zero vulnerabilities in the code path. No migration needed — F-001 reuses the existing 000012 composite index. No frontend changes. --- cmd/agent/main.go | 9 ++- internal/api/handler/digest.go | 7 ++- internal/connector/issuer/acme/acme.go | 17 ++++-- internal/integration/lifecycle_test.go | 10 ++++ internal/repository/interfaces.go | 9 ++- internal/repository/postgres/revocation.go | 23 ++++++++ internal/service/ca_operations.go | 16 +++--- internal/service/ca_operations_test.go | 67 ++++++++++++++++++++++ internal/service/testutil_test.go | 23 ++++++++ 9 files changed, 165 insertions(+), 16 deletions(-) diff --git a/cmd/agent/main.go b/cmd/agent/main.go index ee66c60..27f898b 100644 --- a/cmd/agent/main.go +++ b/cmd/agent/main.go @@ -269,7 +269,14 @@ func (a *Agent) Run(ctx context.Context) error { a.logger.Warn("backing off due to consecutive failures", "failures", a.consecutiveFailures, "backoff", backoff.String()) - time.Sleep(backoff) + // F-003: ctx-aware wait so graceful shutdown does not stall on + // a long backoff. If ctx cancels mid-backoff, return to the + // outer loop so the <-ctx.Done() case can trigger clean exit. + select { + case <-ctx.Done(): + continue + case <-time.After(backoff): + } } a.pollForWork(ctx) diff --git a/internal/api/handler/digest.go b/internal/api/handler/digest.go index 312157e..fb2448c 100644 --- a/internal/api/handler/digest.go +++ b/internal/api/handler/digest.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + "log/slog" "net/http" ) @@ -39,7 +40,8 @@ func (h *DigestHandler) PreviewDigest(w http.ResponseWriter, r *http.Request) { html, err := h.service.PreviewDigest(r.Context()) if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + slog.Error("digest preview failed", "error", err.Error()) + http.Error(w, "internal error", http.StatusInternalServerError) return } @@ -64,9 +66,10 @@ func (h *DigestHandler) SendDigest(w http.ResponseWriter, r *http.Request) { } if err := h.service.SendDigest(r.Context()); err != nil { + slog.Error("digest send failed", "error", err.Error()) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusInternalServerError) - json.NewEncoder(w).Encode(map[string]string{"error": err.Error()}) + json.NewEncoder(w).Encode(map[string]string{"error": "internal error"}) return } diff --git a/internal/connector/issuer/acme/acme.go b/internal/connector/issuer/acme/acme.go index 2ca37c2..af00d46 100644 --- a/internal/connector/issuer/acme/acme.go +++ b/internal/connector/issuer/acme/acme.go @@ -664,12 +664,17 @@ func (c *Connector) solveAuthorizationsDNS01(ctx context.Context, authzURLs []st return fmt.Errorf("failed to present DNS record for %s: %w", domain, err) } - // Wait for DNS propagation + // Wait for DNS propagation (ctx-aware so graceful shutdown can interrupt — F-003) propagationWait := time.Duration(c.config.DNSPropagationWait) * time.Second c.logger.Info("waiting for DNS propagation", "domain", domain, "wait_seconds", c.config.DNSPropagationWait) - time.Sleep(propagationWait) + select { + case <-ctx.Done(): + _ = c.dnsSolver.CleanUp(ctx, domain, dnsChallenge.Token, keyAuth) + return ctx.Err() + case <-time.After(propagationWait): + } // Tell the CA we're ready if _, err := c.client.Accept(ctx, dnsChallenge); err != nil { @@ -773,12 +778,16 @@ func (c *Connector) solveAuthorizationsDNSPersist01(ctx context.Context, authzUR return fmt.Errorf("failed to create persistent DNS record for %s: %w", domain, err) } - // Wait for DNS propagation + // Wait for DNS propagation (ctx-aware so graceful shutdown can interrupt — F-003) propagationWait := time.Duration(c.config.DNSPropagationWait) * time.Second c.logger.Info("waiting for DNS propagation", "domain", domain, "wait_seconds", c.config.DNSPropagationWait) - time.Sleep(propagationWait) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(propagationWait): + } // Tell the CA we're ready if _, err := c.client.Accept(ctx, persistChallenge); err != nil { diff --git a/internal/integration/lifecycle_test.go b/internal/integration/lifecycle_test.go index ae4d5a7..eed3b12 100644 --- a/internal/integration/lifecycle_test.go +++ b/internal/integration/lifecycle_test.go @@ -1371,6 +1371,16 @@ func (m *mockRevocationRepository) ListAll(ctx context.Context) ([]*domain.Certi return m.revocations, nil } +func (m *mockRevocationRepository) ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) { + var result []*domain.CertificateRevocation + for _, r := range m.revocations { + if r.IssuerID == issuerID { + result = append(result, r) + } + } + return result, nil +} + func (m *mockRevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) { var result []*domain.CertificateRevocation for _, r := range m.revocations { diff --git a/internal/repository/interfaces.go b/internal/repository/interfaces.go index 0e63a05..247d5d8 100644 --- a/internal/repository/interfaces.go +++ b/internal/repository/interfaces.go @@ -47,8 +47,15 @@ type RevocationRepository interface { // 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 returns all revocations, ordered by revocation time (for global + // revocation admin views). CRL generation should prefer ListByIssuer to + // avoid loading and discarding rows that belong to other issuers. ListAll(ctx context.Context) ([]*domain.CertificateRevocation, error) + // ListByIssuer returns revocations for a single issuer, ordered by revocation + // time (for CRL generation). Pushing the issuer filter into the query lets + // the migration 000012 composite index (issuer_id, serial_number) drive a + // prefix scan instead of a full table read + in-memory filter. + ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) // ListByCertificate returns all revocations for a certificate. ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) // MarkIssuerNotified updates the issuer_notified flag for a revocation. diff --git a/internal/repository/postgres/revocation.go b/internal/repository/postgres/revocation.go index 07cf2c7..c8c5326 100644 --- a/internal/repository/postgres/revocation.go +++ b/internal/repository/postgres/revocation.go @@ -81,6 +81,29 @@ func (r *RevocationRepository) ListAll(ctx context.Context) ([]*domain.Certifica return scanRevocations(rows) } +// ListByIssuer returns all revocations for a single issuer, ordered by revocation time. +// +// This is the hot path for CRL generation. Pushing the issuer filter into the +// SQL query lets the composite index `idx_certificate_revocations_issuer_serial` +// (migration 000012) drive a prefix scan on issuer_id rather than forcing +// callers to load every row in the table and discard the ones belonging to +// other issuers. +func (r *RevocationRepository) ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) { + rows, err := r.db.QueryContext(ctx, ` + SELECT id, certificate_id, serial_number, reason, revoked_by, revoked_at, + issuer_id, issuer_notified, created_at + FROM certificate_revocations + WHERE issuer_id = $1 + ORDER BY revoked_at ASC + `, issuerID) + if err != nil { + return nil, fmt.Errorf("failed to list revocations by issuer: %w", err) + } + defer rows.Close() + + return scanRevocations(rows) +} + // ListByCertificate returns all revocations for a certificate. func (r *RevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) { rows, err := r.db.QueryContext(ctx, ` diff --git a/internal/service/ca_operations.go b/internal/service/ca_operations.go index 3255df9..e2e5fab 100644 --- a/internal/service/ca_operations.go +++ b/internal/service/ca_operations.go @@ -56,19 +56,19 @@ func (s *CAOperationsSvc) GenerateDERCRL(ctx context.Context, issuerID string) ( return nil, fmt.Errorf("issuer not found: %s", issuerID) } - revocations, err := s.revocationRepo.ListAll(ctx) + // Scope the query to this issuer so the migration 000012 composite index + // drives a prefix scan; previously this path read every revocation in the + // table and filtered in Go, which did not scale as the revocation table + // grew across many issuers (F-001). + revocations, err := s.revocationRepo.ListByIssuer(ctx, issuerID) if err != nil { - return nil, fmt.Errorf("failed to list revocations: %w", err) + return nil, fmt.Errorf("failed to list revocations for issuer %s: %w", issuerID, err) } - // Filter to this issuer and convert to CRL entries. - // Short-lived certificates (profile TTL < 1 hour) are excluded — expiry is sufficient revocation. + // Convert revocations to CRL entries. Short-lived certificates (profile + // TTL < 1 hour) are excluded — expiry is sufficient revocation. var entries []CRLEntry for _, rev := range revocations { - if rev.IssuerID != issuerID { - continue - } - // Check short-lived exemption: look up the cert's profile if s.profileRepo != nil && s.certRepo != nil { cert, err := s.certRepo.Get(ctx, rev.CertificateID) diff --git a/internal/service/ca_operations_test.go b/internal/service/ca_operations_test.go index 2d8990d..e4325ef 100644 --- a/internal/service/ca_operations_test.go +++ b/internal/service/ca_operations_test.go @@ -75,6 +75,73 @@ func TestCAOperationsSvc_GenerateDERCRL_Success(t *testing.T) { t.Logf("DER CRL generated successfully: %d bytes", len(crl)) } +// TestCAOperationsSvc_GenerateDERCRL_UsesListByIssuer_NotListAll guards F-001. +// Before the fix, GenerateDERCRL called revocationRepo.ListAll(ctx) and filtered +// results in Go (if rev.IssuerID != issuerID { continue }). That was O(N) in the +// size of the entire revocation table and did not scale as revocations piled up +// across many issuers. Migration 000012 added the composite index +// idx_certificate_revocations_issuer_serial(issuer_id, serial_number), which is +// a prefix scan target — so the hot path must now call ListByIssuer(ctx, id) to +// drive an indexed query. This regression test asserts the hot path invokes +// ListByIssuer exactly once and never falls back to the full-table ListAll scan, +// and also double-checks that cross-issuer revocations are correctly excluded +// from the generated CRL (no in-Go filter left to catch them). +func TestCAOperationsSvc_GenerateDERCRL_UsesListByIssuer_NotListAll(t *testing.T) { + caSvc, revocationRepo, _ := newCAOperationsSvcTest() + + // Pre-populate with revocations from TWO issuers. If the hot path regresses + // and calls ListAll instead of ListByIssuer, the generated CRL would either + // include the wrong rows or — with the in-Go filter gone — pull in both + // issuers' revocations. ListByIssuer scopes at the query level so only + // iss-local rows come back. + now := time.Now() + revocationRepo.Revocations = []*domain.CertificateRevocation{ + { + SerialNumber: "LOCAL-001", + CertificateID: "cert-local-1", + IssuerID: "iss-local", + Reason: "keyCompromise", + RevokedAt: now.Add(-24 * time.Hour), + RevokedBy: "admin", + }, + { + SerialNumber: "LOCAL-002", + CertificateID: "cert-local-2", + IssuerID: "iss-local", + Reason: "superseded", + RevokedAt: now.Add(-12 * time.Hour), + RevokedBy: "admin", + }, + { + SerialNumber: "OTHER-001", + CertificateID: "cert-other-1", + IssuerID: "iss-other", + Reason: "keyCompromise", + RevokedAt: now.Add(-6 * time.Hour), + RevokedBy: "admin", + }, + } + + crl, err := caSvc.GenerateDERCRL(context.Background(), "iss-local") + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if len(crl) == 0 { + t.Fatal("expected non-empty CRL") + } + + // The contractual assertion: the CRL hot path MUST use the scoped query. + if got, want := revocationRepo.ListByIssuerCalls, 1; got != want { + t.Errorf("ListByIssuerCalls = %d, want %d — CRL hot path must call the scoped query driven by migration 000012 index", got, want) + } + if got := revocationRepo.ListAllCalls; got != 0 { + t.Errorf("ListAllCalls = %d, want 0 — CRL hot path must NOT fall back to the full-table scan after F-001", got) + } + if got, want := revocationRepo.LastListIssuerID, "iss-local"; got != want { + t.Errorf("LastListIssuerID = %q, want %q — issuer scoping argument lost", got, want) + } +} + func TestCAOperationsSvc_GenerateDERCRL_EmptyCRL(t *testing.T) { caSvc, revocationRepo, _ := newCAOperationsSvcTest() diff --git a/internal/service/testutil_test.go b/internal/service/testutil_test.go index 10c6819..a05e176 100644 --- a/internal/service/testutil_test.go +++ b/internal/service/testutil_test.go @@ -1385,6 +1385,13 @@ type mockRevocationRepo struct { Revocations []*domain.CertificateRevocation CreateErr error ListErr error + // F-001 regression instrumentation: track which list method was invoked + // so tests can assert that the CRL generation hot path uses the scoped + // ListByIssuer query (migration 000012 composite index) rather than + // ListAll followed by in-Go filtering. + ListAllCalls int + ListByIssuerCalls int + LastListIssuerID string } func (m *mockRevocationRepo) Create(ctx context.Context, revocation *domain.CertificateRevocation) error { @@ -1405,12 +1412,28 @@ func (m *mockRevocationRepo) GetByIssuerAndSerial(ctx context.Context, issuerID, } func (m *mockRevocationRepo) ListAll(ctx context.Context) ([]*domain.CertificateRevocation, error) { + m.ListAllCalls++ if m.ListErr != nil { return nil, m.ListErr } return m.Revocations, nil } +func (m *mockRevocationRepo) ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) { + m.ListByIssuerCalls++ + m.LastListIssuerID = issuerID + if m.ListErr != nil { + return nil, m.ListErr + } + var result []*domain.CertificateRevocation + for _, r := range m.Revocations { + if r.IssuerID == issuerID { + result = append(result, r) + } + } + return result, nil +} + func (m *mockRevocationRepo) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) { var result []*domain.CertificateRevocation for _, r := range m.Revocations {