3 Commits

Author	SHA1	Message	Date
shankar0123	02438ad9e1	ci: floor raise + doc drift (Phase 3 closure — TEST-H1/H2/M1/M2/M3/M4/L1, ARCH-H3/L1/L2/L3/L4) ... Twelve findings from the architecture diligence audit's Phase 3 bundle closed in one PR. All touch the CI workflows + small doc-drift fixes across the production Go tree + migration headers. CI workflow changes ==================== TEST-H1 — Race detection on ./... -short .github/workflows/ci.yml:106 was a 9-package explicit list. Audit finding TEST-H1 flagged that 25+ packages (internal/auth/*, internal/repository/*, internal/mcp, internal/scep, internal/pkcs7, internal/api/router, internal/api/acme, internal/cli, internal/cms, internal/config, internal/deploy, internal/integration, internal/ratelimit, internal/secret, internal/trustanchor, all of cmd/) silently dropped off race coverage. Post-fix: 'go test -race -short ./... -count=1 -timeout 600s'. 76 testing.Short() guards already cover testcontainers + live-DB integration suites, so -short keeps the long-running tests out. TEST-H2 — Cross-platform build matrix New 'cross-platform-build' job in ci.yml. Matrix: ubuntu-latest + windows-latest + macos-latest, fail-fast: false. Builds cmd/server + cmd/agent + cmd/cli + cmd/mcp-server on each. Catches Windows-specific regressions (path separators, file permissions, exec.Command semantics) the pre-Phase-3 Ubuntu-only CI missed. TEST-L1 — actions/setup-go cache: true (explicit) setup-go v5 defaults cache: true; making it explicit so a future setup-go upgrade can't silently flip it. Re-runs hit the Go module + build cache instead of recompiling cold. TEST-M1 — Mutation-testing floor at 55% security-deep-scan.yml::go-mutesting step rewritten. Removed continue-on-error + per-package '|| true'. New post-loop check extracts every 'The mutation score is X.YZ' line and fails the step if any package drops below 0.55. Floor rationale: starter ratio catches major regressions without rejecting the audit's 'this is OK' steady state; raise quarterly. TEST-M2 — 3 advisory deep-scan gates promoted to blocking Removed continue-on-error: true from: - gosec (filtered to G201/G202/G304/G108 high-signal rules: SQL-injection + path-traversal + pprof-exposed) - osv-scanner (multi-ecosystem CVE; complements govulncheck which is already blocking in ci.yml) - trivy image scan (--severity HIGH,CRITICAL --exit-code 1) continue-on-error count: 15 → 11. ZAP / schemathesis / nuclei / testssl stay advisory because their false-positive rates on https://localhost:8443-targeted DAST runs are high. TEST-M3 — Playwright harness stub web/package.json adds '@playwright/test' devDep + 'e2e' / 'e2e:install' npm scripts. web/playwright.config.ts ships single chromium project with webServer block pointing at 'npm run dev'. web/src/__tests__/ e2e/smoke.spec.ts proves the harness wires through. The full 15-flow suite ships in frontend-design-audit Phase 8 (TEST-H1 in THAT audit); this is the wiring + a single smoke test as the regression floor. New Makefile target: 'make e2e-test'. Doc/code drift fixes ==================== TEST-M4 + ARCH-L2 — Skip inventory artifact + CI guard scripts/skip-inventory.sh walks every t.Skip site under cmd/ + internal/ + deploy/test/ and emits docs/testing/skip-inventory.md grouped by package with file:line:expression triples. Current inventory: 142 t.Skip sites, 76 testing.Short() guards. scripts/ci-guards/skip-inventory-drift.sh regenerates and fails on diff (excluding the 'Last reviewed' timestamp line which drifts daily). The Markdown is the canonical acquisition-diligence artifact for 'what tests are being skipped and why.' ARCH-H3 — MCP catalogue floor reconciliation Audit framing was '121 vs floor 150 — doc/code drift.' Live count via the test's actual regex over all 5 tool files (tools.go + tools_audit_fix.go + tools_auth.go + tools_auth_bundle2.go + tools_est.go): 155 unique 'Name: "certctl_*"' declarations. Pre-Phase-3 audit measured tools.go in isolation (121) and missed the other 4 files (+34 unique names). The test at internal/ciparity/surface_parity_test.go::TestSurfaceParity_MCP passes today (155 ≥ 150). Added a clarifying comment near mcpBaselineFloor explaining the measurement scope so future reviewers don't repeat the audit's framing error. STATUS: stale — no code drift, just a measurement scoping error in the audit. ARCH-L1 — panic() rationale comments 5 panic sites in production Go (excluding _test.go): - internal/repository/postgres/tx.go:84 - internal/service/issuer.go:861 (mustJSON) - internal/service/est.go:728 (mustParseTime) - internal/service/acme.go:1288 (rand source failure — already documented) - internal/pkcs7/certrep.go:270 (OID marshal — already documented) Added ARCH-L1 rationale comments to the 3 sites that didn't have them. All 5 are defensible impossible-path / rethrow / hardcoded- constant guards. ARCH-L3 — Migration IF-NOT-EXISTS carve-outs 4 migrations skip the literal 'IF NOT EXISTS' token but ARE idempotent via different Postgres patterns: - 000014_policy_violation_severity_check.up.sql: ALTER TABLE ADD CONSTRAINT CHECK doesn't accept IF NOT EXISTS; idempotency via DROP CONSTRAINT IF EXISTS preamble. - 000018_audit_events_worm.up.sql: CREATE OR REPLACE FUNCTION + DROP TRIGGER IF EXISTS + CREATE TRIGGER + DO $$ pg_roles existence check. CREATE TRIGGER doesn't take IF NOT EXISTS. - 000030_rbac_admin_perms.up.sql: INSERT ... ON CONFLICT DO NOTHING. - 000039_audit_crit1_perms.up.sql: same INSERT + ON CONFLICT pattern. Added ARCH-L3 header comments to each explaining the carve-out so reviewers don't flag the missing literal token. STATUS: largely stale — migrations are already idempotent. ARCH-L4 — TODO/FIXME → see #<descriptor> 5 TODOs rewritten to the allowed 'see #<descriptor>' pattern: - internal/repository/postgres/auth.go:220 → see #bundle-2-scope-fk - internal/connector/discovery/gcpsm/gcpsm.go:547 → see #gcpsm-pagination - internal/service/audit.go:244 → see #audit-pagination-count - internal/service/job.go:295, 299 → see #validation-job-impl New CI guard scripts/ci-guards/no-todo-in-prod.sh grep-fails any new TODO/FIXME in cmd/ + internal/ (excluding _test.go); allows 'see #N' / 'see #<descriptor>' patterns. Sandbox limitation ================== The 6.1 GB certctl working tree fills the sandbox volume; go1.25.10 toolchain download fails with 'no space left on device' (sandbox has 1.25.9; go.mod requires 1.25.10). Local 'go test' / 'go build' NOT run in this commit. Operator must run 'make verify' on their workstation before push per CLAUDE.md operating rules. The smoke.spec.ts NOT executed in the sandbox (no chromium installed). Operator runs 'cd web && npm install && npx playwright install --with-deps chromium && npm run e2e' on first wire-up. All CI guards (no-todo-in-prod, skip-inventory-drift, G-3 env-docs-drift, doc-rot-detector, and every existing guard) verified clean by running each individually. Closes: cowork/certctl-architecture-diligence-audit.html#fix-TEST-H1, cowork/certctl-architecture-diligence-audit.html#fix-TEST-H2, cowork/certctl-architecture-diligence-audit.html#fix-TEST-M1, cowork/certctl-architecture-diligence-audit.html#fix-TEST-M2, cowork/certctl-architecture-diligence-audit.html#fix-TEST-M3, cowork/certctl-architecture-diligence-audit.html#fix-TEST-M4, cowork/certctl-architecture-diligence-audit.html#fix-TEST-L1, cowork/certctl-architecture-diligence-audit.html#fix-ARCH-H3, cowork/certctl-architecture-diligence-audit.html#fix-ARCH-L1, cowork/certctl-architecture-diligence-audit.html#fix-ARCH-L2, cowork/certctl-architecture-diligence-audit.html#fix-ARCH-L3, cowork/certctl-architecture-diligence-audit.html#fix-ARCH-L4	2026-05-13 20:10:08 +00:00
shankar0123	62a412c488	Bundle C: Renewal/reliability cluster — 7 findings closed ... 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)	2026-04-27 00:08:25 +00:00
shankar0123	b3cc7cbdb2	fix(policies): close the D-006 loop — TitleCase seed canonicals + severity-aware, config-consuming rule engine (D-008) ... D-008 was a three-part drift in the policy engine that made the D-005/D-006 remediation cosmetic below the DB layer: (a) migrations/seed.sql INSERTed rules with pre-D-005 lowercase types ('ownership', 'environment', 'lifetime', 'renewal_window') that the handler validator rejects on Create/Update but that raw SQL INSERTs bypassed entirely. At runtime evaluateRule's switch fell through to the default "unknown policy rule type" error branch on every demo rule × every cert × every cycle, flooding logs while emitting zero violations. (b) migrations/seed_demo.sql persisted lowercase severity values ('critical', 'error', 'warning') on policy_violations rows. INSERT succeeded because that column had no CHECK, but any frontend comparing against the canonical PolicySeverity enum mis-categorized every seeded violation. (c) evaluateRule hardcoded Severity: PolicySeverityWarning on every emitted violation and ignored rule.Config entirely — so the D-006 per-rule severity column (000013) and every per-arm Config JSON ({allowed_issuer_ids, allowed_domains, required_keys, allowed, lead_time_days, max_days}) was dead data below the evaluation layer. This commit lands (a)+(b)+(c) atomically. Shipping any subset leaves the feature half-working. ## Changes Domain (internal/domain/policy.go): * Add PolicyTypeCertificateLifetime as the 6th TitleCase canonical. Pre-D-008 the seeded "max-certificate-lifetime" rule had no engine arm — routing it through RenewalLeadTime would conflate "how close to expiry before we renew" with "how long can the cert possibly be", two distinct semantics. The new type accepts config {"max_days": int} and flags certs whose NotAfter - NotBefore exceeds the cap. Handler validator (internal/api/handler/validation.go): * ValidatePolicyType allowlist grown to 6 canonicals (AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime, CertificateLifetime). OpenAPI (api/openapi.yaml): * PolicyType enum grown to match domain. Frontend (web/src/api/types.ts, types.test.ts): * POLICY_TYPES tuple gains CertificateLifetime; pin test asserts all 6 canonicals and rejects casing drift. Migration 000014 (policy_violations severity CHECK): * Named CHECK constraint (policy_violations_severity_check) mirroring 000013's allowlist, defense-in-depth at the DB layer against future drift from bypassed writes (migrations, psql sessions, future callers). Symmetric down migration drops by name. Seed data: * migrations/seed.sql rewritten to emit TitleCase canonicals with per-arm config JSON that actually exercises the config-consuming paths (not the missing-field backstops): - pr-require-owner → RequiredMetadata {"required_keys":["owner"]} Warning - pr-allowed-environments → AllowedEnvironments {"allowed":["production","staging","development"]} Error - pr-max-certificate-lifetime → CertificateLifetime {"max_days":90} Critical - pr-min-renewal-window → RenewalLeadTime {"lead_time_days":14} Warning Severities are now differentiated per rule (D-006 intent). * migrations/seed_demo.sql violation rows flipped to TitleCase severity ('Critical', 'Error', 'Warning') so migration 000014 applies cleanly on upgrade paths. Engine rewrite (internal/service/policy.go): * evaluateRule rewritten. All six arms now: 1. Parse rule.Config into the per-arm typed struct. 2. Bad JSON → log at ValidateCertificate boundary and skip this rule (no co-located poisoning of other rules in the same batch). 3. Empty/null Config → emit the pre-D-008 missing-field violation (backwards compat invariant — operators who haven't reconfigured still see the same output). 4. Violations emitted carry rule.Severity (no more hardcoded Warning); D-006 column is now load-bearing. * CertificateLifetime arm reads NotBefore/NotAfter from the certificate's latest version via CertRepo. Injected via PolicyService.SetCertRepo() setter — avoids churning ~36 NewPolicyService call sites while keeping the lifetime arm optional (degrades to a log+skip if the setter is not wired). Server wiring (cmd/server/main.go): * policyService.SetCertRepo(certRepo) wired after construction. Tests (internal/service/policy_test.go): * 25 new subtests across 5 groups: - TestEvaluateRule_SeverityPassThrough (6): every rule type emits violations carrying rule.Severity, not hardcoded. - TestEvaluateRule_ConfigConsumed (12): every per-arm Config path exercised positive + negative. - TestEvaluateRule_EmptyConfig_BackCompat (3): empty/null Config still emits pre-D-008 missing-field violations. - TestEvaluateRule_BadConfig_SkipsRule: malformed JSON logs and skips cleanly without poisoning neighbors. - TestEvaluateRule_CertificateLifetime_RepoScenarios (3): ok when repo wired, log+skip when not, handles missing NotBefore/NotAfter edges. Provenance: D-008 surfaced during D-005/D-006 remediation review in eef1db0. That commit added persistence and CI pins for the severity field but did not re-verify the evaluation layer consumed it; this finding and fix close the audit-process gap.	2026-04-18 14:55:56 +00:00