mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 22:21:30 +00:00
62a412c488
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)
181 lines
6.4 KiB
Go
181 lines
6.4 KiB
Go
package handler
|
|
|
|
import (
|
|
"bytes"
|
|
"context"
|
|
"encoding/json"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"testing"
|
|
|
|
"github.com/shankar0123/certctl/internal/domain"
|
|
)
|
|
|
|
// Bundle C / Audit M-007 (CWE-754): partial-failure tests for the three
|
|
// bulk endpoints. Pre-bundle all three handlers had only happy-path
|
|
// (TotalRevoked = TotalMatched, no Errors) and full-failure (service
|
|
// returns err) tests. The mixed-result branch — where some certs
|
|
// succeed and others fail — is the most operationally common shape
|
|
// and was completely uncovered.
|
|
//
|
|
// Each test asserts:
|
|
// 1. HTTP 200 (mixed result is a successful HTTP response carrying
|
|
// both succeeded and failed counters).
|
|
// 2. The response body's TotalMatched / Total<verb> / TotalFailed
|
|
// counters all round-trip from the service mock.
|
|
// 3. The Errors[] array is preserved and operators can correlate
|
|
// each failure to its certificate ID.
|
|
|
|
// --- bulk-revoke ----------------------------------------------------------
|
|
|
|
func TestBulkRevoke_PartialFailure_ReportsBoth(t *testing.T) {
|
|
svc := &mockBulkRevocationService{
|
|
BulkRevokeFn: func(ctx context.Context, criteria domain.BulkRevocationCriteria, reason string, actor string) (*domain.BulkRevocationResult, error) {
|
|
return &domain.BulkRevocationResult{
|
|
TotalMatched: 3,
|
|
TotalRevoked: 2,
|
|
TotalSkipped: 0,
|
|
TotalFailed: 1,
|
|
Errors: []domain.BulkRevocationError{
|
|
{CertificateID: "mc-failed", Error: "issuer connector unreachable"},
|
|
},
|
|
}, nil
|
|
},
|
|
}
|
|
h := NewBulkRevocationHandler(svc)
|
|
|
|
body := `{"reason":"keyCompromise","certificate_ids":["mc-1","mc-2","mc-failed"]}`
|
|
req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-revoke", bytes.NewBufferString(body))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req = req.WithContext(adminContext())
|
|
w := httptest.NewRecorder()
|
|
|
|
h.BulkRevoke(w, req)
|
|
|
|
if w.Code != http.StatusOK {
|
|
t.Fatalf("partial failure must still return HTTP 200, got %d", w.Code)
|
|
}
|
|
|
|
var result domain.BulkRevocationResult
|
|
if err := json.NewDecoder(w.Body).Decode(&result); err != nil {
|
|
t.Fatalf("decode response: %v", err)
|
|
}
|
|
if result.TotalMatched != 3 {
|
|
t.Errorf("TotalMatched = %d, want 3", result.TotalMatched)
|
|
}
|
|
if result.TotalRevoked != 2 {
|
|
t.Errorf("TotalRevoked = %d, want 2", result.TotalRevoked)
|
|
}
|
|
if result.TotalFailed != 1 {
|
|
t.Errorf("TotalFailed = %d, want 1", result.TotalFailed)
|
|
}
|
|
if len(result.Errors) != 1 {
|
|
t.Fatalf("Errors len = %d, want 1", len(result.Errors))
|
|
}
|
|
if result.Errors[0].CertificateID != "mc-failed" {
|
|
t.Errorf("error CertificateID = %q, want mc-failed", result.Errors[0].CertificateID)
|
|
}
|
|
if result.Errors[0].Error == "" {
|
|
t.Error("error message must be non-empty so operators can triage")
|
|
}
|
|
}
|
|
|
|
// --- bulk-renew -----------------------------------------------------------
|
|
|
|
func TestBulkRenew_PartialFailure_ReportsBoth(t *testing.T) {
|
|
svc := &mockBulkRenewalService{
|
|
BulkRenewFn: func(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) {
|
|
return &domain.BulkRenewalResult{
|
|
TotalMatched: 3,
|
|
TotalEnqueued: 2,
|
|
TotalSkipped: 0,
|
|
TotalFailed: 1,
|
|
Errors: []domain.BulkOperationError{
|
|
{CertificateID: "mc-failed", Error: "renewal job enqueue failed: db timeout"},
|
|
},
|
|
}, nil
|
|
},
|
|
}
|
|
h := NewBulkRenewalHandler(svc)
|
|
|
|
body := `{"certificate_ids":["mc-1","mc-2","mc-failed"]}`
|
|
req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-renew", bytes.NewBufferString(body))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req = req.WithContext(authenticatedContext("test-actor"))
|
|
w := httptest.NewRecorder()
|
|
|
|
h.BulkRenew(w, req)
|
|
|
|
if w.Code != http.StatusOK {
|
|
t.Fatalf("partial failure must still return HTTP 200, got %d", w.Code)
|
|
}
|
|
|
|
var result domain.BulkRenewalResult
|
|
if err := json.NewDecoder(w.Body).Decode(&result); err != nil {
|
|
t.Fatalf("decode response: %v", err)
|
|
}
|
|
if result.TotalMatched != 3 || result.TotalEnqueued != 2 || result.TotalFailed != 1 {
|
|
t.Errorf("counters mismatch: matched=%d enqueued=%d failed=%d, want 3/2/1",
|
|
result.TotalMatched, result.TotalEnqueued, result.TotalFailed)
|
|
}
|
|
if len(result.Errors) != 1 || result.Errors[0].CertificateID != "mc-failed" {
|
|
t.Errorf("Errors not preserved: %+v", result.Errors)
|
|
}
|
|
}
|
|
|
|
// --- bulk-reassign --------------------------------------------------------
|
|
|
|
func TestBulkReassign_PartialFailure_ReportsBoth(t *testing.T) {
|
|
svc := &mockBulkReassignmentService{
|
|
BulkReassignFn: func(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) {
|
|
return &domain.BulkReassignmentResult{
|
|
TotalMatched: 3,
|
|
TotalReassigned: 2,
|
|
TotalSkipped: 0,
|
|
TotalFailed: 1,
|
|
Errors: []domain.BulkOperationError{
|
|
{CertificateID: "mc-failed", Error: "FK violation: cert no longer exists"},
|
|
},
|
|
}, nil
|
|
},
|
|
}
|
|
h := NewBulkReassignmentHandler(svc)
|
|
|
|
body := `{"certificate_ids":["mc-1","mc-2","mc-failed"],"owner_id":"o-bob"}`
|
|
req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-reassign", bytes.NewBufferString(body))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req = req.WithContext(authenticatedContext("test-actor"))
|
|
w := httptest.NewRecorder()
|
|
|
|
h.BulkReassign(w, req)
|
|
|
|
if w.Code != http.StatusOK {
|
|
t.Fatalf("partial failure must still return HTTP 200, got %d", w.Code)
|
|
}
|
|
|
|
var result domain.BulkReassignmentResult
|
|
if err := json.NewDecoder(w.Body).Decode(&result); err != nil {
|
|
t.Fatalf("decode response: %v", err)
|
|
}
|
|
if result.TotalMatched != 3 || result.TotalReassigned != 2 || result.TotalFailed != 1 {
|
|
t.Errorf("counters mismatch: matched=%d reassigned=%d failed=%d, want 3/2/1",
|
|
result.TotalMatched, result.TotalReassigned, result.TotalFailed)
|
|
}
|
|
if len(result.Errors) != 1 || result.Errors[0].CertificateID != "mc-failed" {
|
|
t.Errorf("Errors not preserved: %+v", result.Errors)
|
|
}
|
|
}
|
|
|
|
// --- helper context for unauth-allowed handlers (renew + reassign aren't admin-gated) ---
|
|
|
|
func authenticatedContext(actor string) context.Context {
|
|
type userKey struct{}
|
|
// The middleware UserKey is a private type in the middleware package, so
|
|
// in this handler test we can't construct one directly. Bulk-renew and
|
|
// bulk-reassign read the actor through the same middleware.GetUser path
|
|
// that bulk-revoke does — adminContext() in the existing test suite is
|
|
// the canonical helper. Reuse it (delivers both UserKey and AdminKey).
|
|
_ = userKey{}
|
|
return adminContext()
|
|
}
|