mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-11 17:39:05 +00:00
F-001/F-002/F-003: CRL prefix-scan, digest error sanitization, ctx-aware sleeps
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.
This commit is contained in:
+8
-1
@@ -269,7 +269,14 @@ func (a *Agent) Run(ctx context.Context) error {
|
|||||||
a.logger.Warn("backing off due to consecutive failures",
|
a.logger.Warn("backing off due to consecutive failures",
|
||||||
"failures", a.consecutiveFailures,
|
"failures", a.consecutiveFailures,
|
||||||
"backoff", backoff.String())
|
"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)
|
a.pollForWork(ctx)
|
||||||
|
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ package handler
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -39,7 +40,8 @@ func (h *DigestHandler) PreviewDigest(w http.ResponseWriter, r *http.Request) {
|
|||||||
|
|
||||||
html, err := h.service.PreviewDigest(r.Context())
|
html, err := h.service.PreviewDigest(r.Context())
|
||||||
if err != nil {
|
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
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -64,9 +66,10 @@ func (h *DigestHandler) SendDigest(w http.ResponseWriter, r *http.Request) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if err := h.service.SendDigest(r.Context()); err != nil {
|
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.Header().Set("Content-Type", "application/json")
|
||||||
w.WriteHeader(http.StatusInternalServerError)
|
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
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -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)
|
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
|
propagationWait := time.Duration(c.config.DNSPropagationWait) * time.Second
|
||||||
c.logger.Info("waiting for DNS propagation",
|
c.logger.Info("waiting for DNS propagation",
|
||||||
"domain", domain,
|
"domain", domain,
|
||||||
"wait_seconds", c.config.DNSPropagationWait)
|
"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
|
// Tell the CA we're ready
|
||||||
if _, err := c.client.Accept(ctx, dnsChallenge); err != nil {
|
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)
|
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
|
propagationWait := time.Duration(c.config.DNSPropagationWait) * time.Second
|
||||||
c.logger.Info("waiting for DNS propagation",
|
c.logger.Info("waiting for DNS propagation",
|
||||||
"domain", domain,
|
"domain", domain,
|
||||||
"wait_seconds", c.config.DNSPropagationWait)
|
"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
|
// Tell the CA we're ready
|
||||||
if _, err := c.client.Accept(ctx, persistChallenge); err != nil {
|
if _, err := c.client.Accept(ctx, persistChallenge); err != nil {
|
||||||
|
|||||||
@@ -1371,6 +1371,16 @@ func (m *mockRevocationRepository) ListAll(ctx context.Context) ([]*domain.Certi
|
|||||||
return m.revocations, nil
|
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) {
|
func (m *mockRevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
|
||||||
var result []*domain.CertificateRevocation
|
var result []*domain.CertificateRevocation
|
||||||
for _, r := range m.revocations {
|
for _, r := range m.revocations {
|
||||||
|
|||||||
@@ -47,8 +47,15 @@ type RevocationRepository interface {
|
|||||||
// protocol endpoints carry it in the request path; RFC 5280 §5.2.3 guarantees
|
// protocol endpoints carry it in the request path; RFC 5280 §5.2.3 guarantees
|
||||||
// uniqueness only within a single issuer.
|
// uniqueness only within a single issuer.
|
||||||
GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.CertificateRevocation, error)
|
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)
|
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 returns all revocations for a certificate.
|
||||||
ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error)
|
ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error)
|
||||||
// MarkIssuerNotified updates the issuer_notified flag for a revocation.
|
// MarkIssuerNotified updates the issuer_notified flag for a revocation.
|
||||||
|
|||||||
@@ -81,6 +81,29 @@ func (r *RevocationRepository) ListAll(ctx context.Context) ([]*domain.Certifica
|
|||||||
return scanRevocations(rows)
|
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.
|
// ListByCertificate returns all revocations for a certificate.
|
||||||
func (r *RevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
|
func (r *RevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
|
||||||
rows, err := r.db.QueryContext(ctx, `
|
rows, err := r.db.QueryContext(ctx, `
|
||||||
|
|||||||
@@ -56,19 +56,19 @@ func (s *CAOperationsSvc) GenerateDERCRL(ctx context.Context, issuerID string) (
|
|||||||
return nil, fmt.Errorf("issuer not found: %s", issuerID)
|
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 {
|
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.
|
// Convert revocations to CRL entries. Short-lived certificates (profile
|
||||||
// Short-lived certificates (profile TTL < 1 hour) are excluded — expiry is sufficient revocation.
|
// TTL < 1 hour) are excluded — expiry is sufficient revocation.
|
||||||
var entries []CRLEntry
|
var entries []CRLEntry
|
||||||
for _, rev := range revocations {
|
for _, rev := range revocations {
|
||||||
if rev.IssuerID != issuerID {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
// Check short-lived exemption: look up the cert's profile
|
// Check short-lived exemption: look up the cert's profile
|
||||||
if s.profileRepo != nil && s.certRepo != nil {
|
if s.profileRepo != nil && s.certRepo != nil {
|
||||||
cert, err := s.certRepo.Get(ctx, rev.CertificateID)
|
cert, err := s.certRepo.Get(ctx, rev.CertificateID)
|
||||||
|
|||||||
@@ -75,6 +75,73 @@ func TestCAOperationsSvc_GenerateDERCRL_Success(t *testing.T) {
|
|||||||
t.Logf("DER CRL generated successfully: %d bytes", len(crl))
|
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) {
|
func TestCAOperationsSvc_GenerateDERCRL_EmptyCRL(t *testing.T) {
|
||||||
caSvc, revocationRepo, _ := newCAOperationsSvcTest()
|
caSvc, revocationRepo, _ := newCAOperationsSvcTest()
|
||||||
|
|
||||||
|
|||||||
@@ -1385,6 +1385,13 @@ type mockRevocationRepo struct {
|
|||||||
Revocations []*domain.CertificateRevocation
|
Revocations []*domain.CertificateRevocation
|
||||||
CreateErr error
|
CreateErr error
|
||||||
ListErr 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 {
|
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) {
|
func (m *mockRevocationRepo) ListAll(ctx context.Context) ([]*domain.CertificateRevocation, error) {
|
||||||
|
m.ListAllCalls++
|
||||||
if m.ListErr != nil {
|
if m.ListErr != nil {
|
||||||
return nil, m.ListErr
|
return nil, m.ListErr
|
||||||
}
|
}
|
||||||
return m.Revocations, nil
|
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) {
|
func (m *mockRevocationRepo) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
|
||||||
var result []*domain.CertificateRevocation
|
var result []*domain.CertificateRevocation
|
||||||
for _, r := range m.Revocations {
|
for _, r := range m.Revocations {
|
||||||
|
|||||||
Reference in New Issue
Block a user