mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-11 14:18:53 +00:00
345bafe5aa
Closes M-006 + M-007 + M-008 + M-015 + M-016 + M-019 + M-020 from
comprehensive-audit-2026-04-25. M-028 was already closed by the
Bundle B CI follow-up.
M-006 (CWE-913) — Idempotent migration 000014
migrations/000014_policy_violation_severity_check.up.sql:
Prepended ALTER TABLE ... DROP CONSTRAINT IF EXISTS before the
ADD. Mirrors the down migration's existing IF EXISTS shape and
the M-7 idempotent-index idiom. Re-runs against partially-applied
DBs now succeed.
M-007 — Bulk-op partial-failure tests (3 new)
internal/api/handler/bulk_partial_failure_test.go:
TestBulkRevoke_PartialFailure_ReportsBoth
TestBulkRenew_PartialFailure_ReportsBoth
TestBulkReassign_PartialFailure_ReportsBoth
Each asserts HTTP 200 + both success/failure counters round-trip
+ per-cert errors[] preserved with non-empty messages so operators
can correlate each failure to its certificate ID.
M-008 — Admin-gated handler enumeration pin (verified-already-clean)
Recon: only one admin-gated handler — bulk_revocation.go — with
full 3-branch test triplet already in place. health.go calls
IsAdmin informationally to surface the flag to the GUI without
gating.
internal/api/handler/m008_admin_gate_test.go:
Walks every handler .go file, asserts every middleware.IsAdmin
call site is in AdminGatedHandlers (with required test triplet)
or InformationalIsAdminCallers (justified). Adding a new admin
gate without updating both the constant AND adding the test
triplet fails CI.
M-015 — Single-profile cardinality pin (verified-already-clean)
Audit claim 'no cardinality validation' was wrong — enforced at
struct level. domain.ManagedCertificate.{CertificateProfileID,
RenewalPolicyID,IssuerID,OwnerID} and RenewalPolicy.
CertificateProfileID are bare strings, not slices.
internal/domain/m015_cardinality_test.go:
reflect-based pin on kind=String. Schema change to N:N would
have to update renewal.go's lookup loop in the same commit.
M-016 (CWE-754) — Reap stale-agent jobs
internal/repository/postgres/job.go::ListJobsWithOfflineAgents:
JOIN jobs to agents on agent_id, filter (status=Running AND
a.last_heartbeat_at < cutoff), exclude server-keygen jobs.
internal/service/job.go::ReapJobsWithOfflineAgents:
Flips matched jobs to Failed reason agent_offline so I-001
retry loop re-queues them on a healthy agent. Records audit
event per reap.
internal/scheduler/scheduler.go:
Scheduler.runJobTimeout cycle now calls both reaper arms.
agentOfflineJobTTL default 5min (5x agent-health-check default);
SetAgentOfflineJobTTL knob for operator override.
internal/service/job_offline_agent_reaper_test.go: 6 unit tests
cover happy path, server-keygen-skip, non-Running-skip, non-
positive-TTL fail-loud, repo-error propagation, audit-event
recording.
M-019 — Configurable ARI HTTP timeout
Audit claim 'no fallback timeout' was wrong — ari.go:52 already
had a 15s timeout. Bundle C makes it configurable.
internal/connector/issuer/acme/acme.go:
Config.ARIHTTPTimeoutSeconds field with env path
CERTCTL_ACME_ARI_HTTP_TIMEOUT_SECONDS.
internal/connector/issuer/acme/ari.go:
Both HTTP clients (GetRenewalInfo + getARIEndpoint) now use the
new ariHTTPTimeout() helper. Zero / negative / nil-config all
fall back to the historic 15s default.
ari_timeout_test.go: 4 dispatch arm tests.
M-020 (CWE-770) — OCSP DoS hardening
Pre-bundle the noAuthHandler chain had no rate limit. An attacker
could DoS the OCSP responder, which for fail-open relying parties
is a revocation bypass.
cmd/server/main.go:
noAuthHandler refactored from fixed middleware.Chain(...) to a
conditional slice that appends middleware.NewRateLimiter when
cfg.RateLimit.Enabled. Per-IP keying applies; OCSP/CRL/EST/SCEP
are unauth.
docs/security.md (NEW):
Operator runbook documenting Must-Staple TLS Feature extension
RFC 7633 as the architectural fix for fail-open relying parties.
Profile-flip guidance + nginx/Apache/HAProxy/Envoy stapling
snippets + explicit scope statement on what the rate limiter
alone does NOT solve.
Audit deliverables:
cowork/comprehensive-audit-2026-04-25/audit-report.md: score
31/55 -> 38/55 closed (Medium 13/27 -> 20/27).
cowork/comprehensive-audit-2026-04-25/findings.yaml: 7 status
flips open -> closed with closure notes citing the Bundle C
mechanism.
certctl/CHANGELOG.md: Bundle C section under [unreleased].
Verification:
go vet ./internal/service ./internal/scheduler ./internal/connector/issuer/acme
./internal/api/handler ./internal/domain ./cmd/server clean
go test -count=1 -short on the same packages all green
helm template + helm lint clean
internal/repository/postgres setup-fail sandbox disk
pressure (same on master HEAD before this branch)
170 lines
5.7 KiB
Go
170 lines
5.7 KiB
Go
package service
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"io"
|
|
"log/slog"
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/shankar0123/certctl/internal/domain"
|
|
)
|
|
|
|
// Bundle C / Audit M-016 (CWE-754): regression suite for the new
|
|
// ReapJobsWithOfflineAgents path. Pre-bundle the reaper only handled
|
|
// AwaitingCSR / AwaitingApproval timeouts; jobs claimed by an agent
|
|
// that subsequently dies sat in Running indefinitely. These tests pin
|
|
// the new behavior end-to-end through the JobService → mockJobRepo
|
|
// boundary.
|
|
|
|
func newOfflineReaperService(t *testing.T) (*JobService, *mockJobRepo, *mockAuditRepo) {
|
|
t.Helper()
|
|
jobRepo := &mockJobRepo{
|
|
Jobs: map[string]*domain.Job{},
|
|
Agents: map[string]*domain.Agent{},
|
|
}
|
|
auditRepo := newMockAuditRepository()
|
|
auditService := NewAuditService(auditRepo)
|
|
svc := NewJobService(jobRepo, nil, nil, nil, nil, slog.New(slog.NewTextHandler(io.Discard, nil)))
|
|
svc.SetAuditService(auditService)
|
|
return svc, jobRepo, auditRepo
|
|
}
|
|
|
|
func mkRunningJob(id, agentID string) *domain.Job {
|
|
a := agentID
|
|
now := time.Now()
|
|
return &domain.Job{
|
|
ID: id,
|
|
AgentID: &a,
|
|
Status: domain.JobStatusRunning,
|
|
CreatedAt: now.Add(-2 * time.Hour),
|
|
}
|
|
}
|
|
|
|
func mkAgentWithHeartbeat(id string, hbAge time.Duration) *domain.Agent {
|
|
hb := time.Now().Add(-hbAge)
|
|
return &domain.Agent{
|
|
ID: id,
|
|
Name: id,
|
|
LastHeartbeatAt: &hb,
|
|
}
|
|
}
|
|
|
|
func TestReapJobsWithOfflineAgents_FlipsRunningToFailed(t *testing.T) {
|
|
svc, repo, _ := newOfflineReaperService(t)
|
|
|
|
repo.Agents["agt-stale"] = mkAgentWithHeartbeat("agt-stale", 30*time.Minute)
|
|
repo.Agents["agt-fresh"] = mkAgentWithHeartbeat("agt-fresh", 1*time.Minute)
|
|
repo.Jobs["j-stale"] = mkRunningJob("j-stale", "agt-stale")
|
|
repo.Jobs["j-fresh"] = mkRunningJob("j-fresh", "agt-fresh")
|
|
|
|
if err := svc.ReapJobsWithOfflineAgents(context.Background(), 10*time.Minute); err != nil {
|
|
t.Fatalf("reaper returned error: %v", err)
|
|
}
|
|
|
|
if got := repo.Jobs["j-stale"].Status; got != domain.JobStatusFailed {
|
|
t.Errorf("stale-agent job status = %s, want Failed", got)
|
|
}
|
|
if got := repo.Jobs["j-fresh"].Status; got != domain.JobStatusRunning {
|
|
t.Errorf("fresh-agent job status = %s, want Running (must NOT be reaped)", got)
|
|
}
|
|
|
|
stale := repo.Jobs["j-stale"]
|
|
if stale.LastError == nil || !strings.Contains(*stale.LastError, "agent offline") {
|
|
t.Errorf("stale job LastError must cite agent offline; got: %v", stale.LastError)
|
|
}
|
|
}
|
|
|
|
func TestReapJobsWithOfflineAgents_SkipsServerKeygenJobs(t *testing.T) {
|
|
// Jobs without an agent_id (server-side keygen) must NOT be reaped
|
|
// by this path — they have no agent to be "offline".
|
|
svc, repo, _ := newOfflineReaperService(t)
|
|
noAgent := &domain.Job{
|
|
ID: "j-server",
|
|
Status: domain.JobStatusRunning,
|
|
CreatedAt: time.Now().Add(-time.Hour),
|
|
}
|
|
repo.Jobs["j-server"] = noAgent
|
|
|
|
if err := svc.ReapJobsWithOfflineAgents(context.Background(), 1*time.Minute); err != nil {
|
|
t.Fatalf("reaper returned error: %v", err)
|
|
}
|
|
if got := repo.Jobs["j-server"].Status; got != domain.JobStatusRunning {
|
|
t.Errorf("server-keygen job (no agent_id) status = %s, want Running", got)
|
|
}
|
|
}
|
|
|
|
func TestReapJobsWithOfflineAgents_SkipsNonRunningJobs(t *testing.T) {
|
|
// Pending / AwaitingCSR / AwaitingApproval jobs are NOT in scope —
|
|
// they're handled by ReapTimedOutJobs (I-003) or ClaimPendingJobs.
|
|
svc, repo, _ := newOfflineReaperService(t)
|
|
repo.Agents["agt-stale"] = mkAgentWithHeartbeat("agt-stale", 1*time.Hour)
|
|
repo.Jobs["j-pending"] = func() *domain.Job {
|
|
j := mkRunningJob("j-pending", "agt-stale")
|
|
j.Status = domain.JobStatusPending
|
|
return j
|
|
}()
|
|
|
|
if err := svc.ReapJobsWithOfflineAgents(context.Background(), 1*time.Minute); err != nil {
|
|
t.Fatalf("reaper returned error: %v", err)
|
|
}
|
|
if got := repo.Jobs["j-pending"].Status; got != domain.JobStatusPending {
|
|
t.Errorf("Pending job status = %s, want Pending (out of scope for offline-agent reaper)", got)
|
|
}
|
|
}
|
|
|
|
func TestReapJobsWithOfflineAgents_RejectsNonPositiveTTL(t *testing.T) {
|
|
svc, _, _ := newOfflineReaperService(t)
|
|
if err := svc.ReapJobsWithOfflineAgents(context.Background(), 0); err == nil {
|
|
t.Error("expected error for zero TTL — fail-loud guard against misconfig")
|
|
}
|
|
if err := svc.ReapJobsWithOfflineAgents(context.Background(), -time.Hour); err == nil {
|
|
t.Error("expected error for negative TTL — fail-loud guard against misconfig")
|
|
}
|
|
}
|
|
|
|
func TestReapJobsWithOfflineAgents_PropagatesRepoError(t *testing.T) {
|
|
svc, repo, _ := newOfflineReaperService(t)
|
|
repo.ListOfflineAgentJobsErr = errors.New("simulated db down")
|
|
|
|
err := svc.ReapJobsWithOfflineAgents(context.Background(), 5*time.Minute)
|
|
if err == nil {
|
|
t.Fatal("expected error to propagate from repo")
|
|
}
|
|
if !strings.Contains(err.Error(), "simulated db down") {
|
|
t.Errorf("expected wrapped repo error, got: %v", err)
|
|
}
|
|
}
|
|
|
|
func TestReapJobsWithOfflineAgents_RecordsAuditEvent(t *testing.T) {
|
|
svc, repo, audit := newOfflineReaperService(t)
|
|
repo.Agents["agt-stale"] = mkAgentWithHeartbeat("agt-stale", 30*time.Minute)
|
|
repo.Jobs["j-stale"] = mkRunningJob("j-stale", "agt-stale")
|
|
|
|
if err := svc.ReapJobsWithOfflineAgents(context.Background(), 5*time.Minute); err != nil {
|
|
t.Fatalf("reaper: %v", err)
|
|
}
|
|
|
|
audit.mu.Lock()
|
|
events := append([]*domain.AuditEvent(nil), audit.Events...)
|
|
audit.mu.Unlock()
|
|
var found *domain.AuditEvent
|
|
for i := range events {
|
|
if events[i].Action == "job_offline_agent_reap" {
|
|
found = events[i]
|
|
break
|
|
}
|
|
}
|
|
if found == nil {
|
|
t.Fatal("expected job_offline_agent_reap audit event, got none")
|
|
}
|
|
if found.Actor != "system" {
|
|
t.Errorf("audit Actor = %q, want system", found.Actor)
|
|
}
|
|
if found.ResourceType != "job" || found.ResourceID != "j-stale" {
|
|
t.Errorf("audit resource binding wrong: %s/%s", found.ResourceType, found.ResourceID)
|
|
}
|
|
}
|