335 Commits

Author	SHA1	Message	Date
shankar0123	06cea1ce0f	auth-bundle-1 Phase 12 follow-up: in-tree TODO for path-12 deferral ... Self-audit on cbb47aa flagged that the negative-path-#12 deferral (scope_id for nonexistent resource → 404) was acknowledged in the commit message but not in the source. A future operator scanning internal/repository/postgres/auth.go would not learn about the gap. Adds an explicit TODO(bundle-2) comment next to RoleRepository.AddPermission documenting: - what's missing today (no FK between role_permissions.scope_id and the resource tables); - why the gate still works at request time (no rows match the bogus scope so EffectivePermissions returns empty); - the cleaner end-state (HTTP 404 at grant time); - what's required to land it (migration confirming existing rows reference real resources); - the cross-reference to cowork/auth-bundle-1-prompt.md path #12. Cosmetic, single-file change. No test churn.	2026-05-09 23:51:16 +00:00
shankar0123	cbb47aaf5d	auth-bundle-1 Phase 11 + 12: RBAC MCP tools + negative-test coverage gate ... # Phase 11 — RBAC MCP tools 12 new tools in internal/mcp/tools_auth.go mirroring the Phase-4 + Phase-7 HTTP surface so operators driving certctl from Claude / VS Code / any MCP client get the same management capability the GUI + CLI already expose: certctl_auth_me GET /v1/auth/me certctl_auth_list_roles GET /v1/auth/roles certctl_auth_get_role GET /v1/auth/roles/{id} certctl_auth_create_role POST /v1/auth/roles certctl_auth_update_role PUT /v1/auth/roles/{id} certctl_auth_delete_role DELETE /v1/auth/roles/{id} certctl_auth_list_permissions GET /v1/auth/permissions certctl_auth_add_permission_to_role POST /v1/auth/roles/{id}/permissions certctl_auth_remove_permission_from_role DELETE /v1/auth/roles/{id}/permissions/{perm} certctl_auth_list_keys GET /v1/auth/keys certctl_auth_assign_role_to_key POST /v1/auth/keys/{id}/roles certctl_auth_revoke_role_from_key DELETE /v1/auth/keys/{id}/roles/{role_id} Each tool routes through the existing HTTP client (no parallel business logic), so permission gates fire server-side: a non-admin caller's MCP tool invocation returns whatever 403 the underlying HTTP handler emits, fenced via errorResult for LLM- prompt-injection defense. Input types in internal/mcp/types.go (AuthRoleIDInput, AuthCreateRoleInput, AuthUpdateRoleInput, AuthRolePermissionGrantInput, AuthRolePermissionRevokeInput, AuthAssignKeyRoleInput, AuthRevokeKeyRoleInput) carry jsonschema descriptions so the MCP consumer's tool catalogue shows operator-friendly hints. internal/mcp/tools_auth_test.go ships 14 tests: - TestAuthMCP_AllToolsRegister (registration must not panic) - TestAuthMCP_PathsAndMethods (table-driven, 12 rows pinning each tool's HTTP method + URL) - TestAuthMCP_ForbiddenSurfacesFencedError (12 tools × 403 mock → error surface) internal/mcp/tools_per_tool_test.go's allHappyPathCases extended with the 12 new rows so the in-memory dispatch coverage gate (TestMCP_RegisterTools_DispatchableToolCount) stays green at the new total of 139 registered tools. Re-derived total via 'grep -cE "gomcp\.AddTool\(" internal/mcp/tools*.go': 133 (121 in tools.go + 12 in tools_auth.go). # Phase 12 — negative-test coverage gate Audit of the prompt's 12 negative-test paths against existing coverage: 1. Missing actor → 401 ✓ TestRequirePermission_NoActorReturns401, TestRBACGate_NoActorReturns401 2. No roles → 403 ✓ TestRequirePermission_DeniedActorReturns403, TestRBACGate_AuditorRole_403sOnAdminRoutes 3. Role lacks specific perm → 403 ✓ same suite 4. Wrong scope → 403 ✓ TestAuthorizer_SpecificScopeMatchesExactID (wrongID arm) 5. Self-grant w/o auth.role.assign → 403 ✓ TestActorRoleService_GrantRequiresAuthRoleAssign 6. Bootstrap token wrong → 401 ✓ TestEnvTokenStrategy_WrongTokenReturnsInvalidToken, TestBootstrapHandler_Mint_WrongToken_401 7. Bootstrap used twice → 410 ✓ TestEnvTokenStrategy_OneShotConsumption, TestBootstrapHandler_Mint_TwiceReturns410 8. Bootstrap when admin exists → 410 ✓ TestEnvTokenStrategy_AdminExistsClosesPath, TestBootstrapHandler_Mint_AdminExists410 9. Role delete with assignees → 409 NEW: TestRoleService_DeleteWithActorsAssignedReturns409 10. Profile-edit loophole → gated ✓ TestProfileEdit_RequiresApprovalLoopholeClosed 11. Permission not in catalog → 400 ✓ TestRoleService_AddPermissionRejectsNonCanonical 12. Scope ID for nonexistent resource → 404 (validation deferred — no FK constraint between role_permissions.scope_id and the resource tables; documented for a future bundle) Filled the gap at #9 with TestRoleService_DeleteWithActorsAssignedReturns409 which pins the repository sentinel pass-through (postgres FK ON DELETE RESTRICT → repository.ErrAuthRoleInUse → service returns the sentinel verbatim → handler maps to HTTP 409). # Coverage gates .github/coverage-thresholds.yml gains 2 entries: - internal/auth: floor 85 - internal/service/auth: floor 85 .github/workflows/ci.yml's coverage test command extended with ./internal/auth/... and ./internal/api/router/... so the threshold check has data to evaluate. # Protocol-endpoint not-gated test (Category F) internal/api/router/phase12_protocol_allowlist_test.go (new) adds 3 router-level invariant tests: - TestPhase12_ProtocolEndpointsNotGated: AST-walks router.go, asserts no rbacGate(...) call references a path under any protocol-endpoint prefix (/acme, /scep, /.well-known/est, /.well-known/pki/ocsp, /.well-known/pki/crl). - TestPhase12_IsProtocolEndpoint_CoversCanonicalPrefixes: pins auth.IsProtocolEndpoint against the canonical prefix set; if a future protocol lands without lockstep allowlist update, this fails. - TestPhase12_RBACGateRoutesAreUnderAPIv1: belt-and-braces — every rbacGate-wrapped route MUST start with /api/v1/. Catches accidental cross-prefix wraps. Complements the existing TestRequirePermission_ProtocolEndpointBypassesGate (middleware-level) + TestRouter_AuthExemptAllowlist_PinsActualRegistrations (allowlist drift) so the Category F invariant is pinned at all three layers (middleware + router + dispatch). # Verifications * gofmt clean repo-wide. * go vet ./... clean. * staticcheck across internal/auth + handler + router + cli + service + repository + cmd + domain + mcp: clean. * go test -short -count=1 green across internal/auth (incl. bootstrap), internal/api/handler, internal/api/router, internal/cli, internal/service (incl. auth), internal/domain/auth, internal/mcp, cmd/server, cmd/cli.	2026-05-09 23:46:01 +00:00
shankar0123	69a508dfcf	auth-bundle-1 Phase 9 + 10: approval-bypass closure + RBAC GUI ... # Phase 9 — approval-bypass closure (Decision 9, option a) * Migration 000033_approval_kinds.up.sql: ALTER TABLE issuance_approval_requests ADD COLUMN approval_kind + payload JSONB; relax certificate_id + job_id to nullable; CHECK (approval_kind IN ('cert_issuance','profile_edit')) + CHECK (per-kind nullability invariant) + index on approval_kind. Idempotent throughout via DO blocks. * domain.ApprovalKind enum (cert_issuance / profile_edit) + IsValidApprovalKind. ApprovalRequest gains Kind + Payload []byte for the pending profile diff. * postgres.ApprovalRepository.Create + scanApprovalRow extended to round-trip the new columns; certificate_id + job_id switched to sql.NullString so profile_edit rows persist cleanly. Default Kind=cert_issuance preserves back-compat for every Phase-7-2026-05-03 caller. * ApprovalService.RequestProfileEditApproval: new entry point that creates a pending profile-edit row carrying the serialized profile diff. Bypass mode (CERTCTL_APPROVAL_BYPASS) short-circuits the same way it does for cert_issuance. * ApprovalService.SetProfileEditApply hook: cmd/server/main.go registers a closure that deserializes req.Payload + persists via profileRepo.Update + emits a profile.edit_applied audit row with category=auth. The hook avoids the Approval ↔ Profile import cycle. * ProfileService.UpdateProfile: gates when (a) the live profile carries RequiresApproval=true, OR (b) the proposed edit would set it true. Returns ErrProfileEditPendingApproval with the new approval ID; ProfileHandler maps to HTTP 202 Accepted + {pending_approval_id}. Both arms close the flip-flop loophole because every transition through an approval-tier profile fires the gate. * TestProfileEdit_RequiresApprovalLoopholeClosed pins all 3 bypass attempts (flip-off / kept-on / flip-on) gated; nil- approval-service preserves pre-Phase-9 direct-apply for test fixtures. * Approval service tests gain 4 profile_edit rows: pending row shape; same-actor self-approve rejected with ErrApproveBySameActor (load-bearing two-person integrity); approve fails-closed when apply callback unwired; apply callback invoked on approve. * docs/reference/profiles.md (new) explains the gate + edit response shape (202) + same-actor invariant + bypass + audit hooks. # Phase 10 — RBAC management GUI * useAuthMe hook (web/src/hooks/useAuthMe.ts): TanStack Query fetches /api/v1/auth/me on app boot, caches for 60s, exposes hasPerm(p) + hasAnyPerm + isAdmin predicates. Every Phase-10 page consumes this on mount + gates affordances against the cached effective_permissions slice. Server-side enforcement is the load-bearing gate; client-side hide/disable is UX. * New routes: - /auth/roles — list (auth.role.list); create-role modal (auth.role.create) hidden when missing. - /auth/roles/:id — detail + permissions; edit (auth.role.edit), delete (auth.role.delete), add/remove permission affordances each gated. - /auth/keys — list of every actor with role grants; assign + revoke modals (auth.role.assign). actor-demo-anon flagged system-managed; mutation buttons hidden for it. - /auth/settings — stub showing /v1/auth/me identity + bootstrap-endpoint availability via /v1/auth/bootstrap. * AuditPage extended with category filter ('All categories' + the 3 enum values from migration 000032). Selection flows to the API call params + the URL-driven query state. * Layout: 3 new nav entries (Roles / API Keys / Auth Settings). * api/client.ts: 12 new exported functions for the RBAC surface (authMe, list/get/create/update/delete role, list/add/remove role permissions, list keys, assign/revoke key role, bootstrap-availability probe). * data-testid attributes on every interactive element so a future Playwright suite can assert behavior without brittle CSS selectors. * Empty state, error state, and unsaved-changes warnings on every form per the prompt's implementation rules. # Frontend tests * RolesPage.test.tsx (6 tests): list render, empty state, error state, hide-create-button-without-perm, show-create-button-with-perm, submit-create-modal. * KeysPage.test.tsx (3 tests): demo-anon flagged system-managed (no buttons), permission-gated affordance hide for auditor caller, assign-modal-POST contract. * AuthSettingsPage.test.tsx (2 tests): identity surface, bootstrap-OPEN-status surface. * AuditPage.test.tsx (+1): category-filter select renders with the 4 documented options. 15 frontend tests total in src/pages/auth/ + the audit category-filter test; all pass via npx vitest run. # Verifications * go vet ./... clean. * staticcheck across internal/auth + handler + router + cli + service + repository + cmd + domain: clean. * gofmt -l clean repo-wide. * go test -short -count=1 green across internal/service, internal/api/handler, internal/api/router, internal/auth, internal/auth/bootstrap, internal/service/auth, internal/domain/auth, cmd/server, cmd/cli, internal/cli. * npx tsc --noEmit clean. * npm run build green (vite build produces dist/index.html + 946KB JS bundle; chunk-size warning is pre-existing). * npx vitest run src/pages/auth/ src/pages/AuditPage.test.tsx green (15 tests, 4 files).	2026-05-09 21:03:59 +00:00
shankar0123	af4fa12724	auth-bundle-1 Phase 8 follow-up: classify issuer/target audit rows + auditor end-to-end tests + gofmt drift ... Self-audit caught five real gaps in 3ef45e2; this commit closes them. # Phase 8 — issuer/target audit rows now classified as 'config' The Phase 8 prompt explicitly required existing config-mutation calls (issuer config, target config, etc.) to write event_category=config. The 3ef45e2 commit only migrated the auth service callers; the 6 issuer/target call-sites (internal/service/issuer.go: create/update/delete_issuer + internal/service/target.go: create/update/delete_target) still defaulted to cert_lifecycle. They now pass through RecordEventWithCategory(..., domain.EventCategoryConfig, ...) so auditors filtering /v1/audit?category=config see the slice the migration's docstring promised. # Auditor exit-criterion test Phase 8's exit criteria pin 'a user with the auditor role can list / export audit events but gets 403 on every other endpoint.' Bundle 1 unit invariants (auditor permission set, rbacGate behaviour) were in place but no end-to-end test walked the full set of admin perms with an auditor actor. internal/api/router/rbac_gate_integration_test.go gains TestRBACGate_AuditorRole_403sOnAdminRoutes (table-driven across all 5 admin perms — cert.bulk_revoke / crl.admin / scep.admin / est.admin / ca.hierarchy.manage) plus TestRBACGate_AuditorRole_PassesAuditReadGate (positive case for audit.read). # gofmt drift 3ef45e2 left two cosmetic struct-field-alignment diffs in internal/cli/auth.go and internal/api/handler/audit_handler_test.go that gofmt -l flagged. CI's gofmt step would have failed; gofmt -w applied; gofmt -l now clean across the repo. # CHANGELOG path-prefix CHANGELOG.md v2.1.0 used '/v1/auth/bootstrap' shorthand in the operator-facing flow examples. The actual route is '/api/v1/auth/bootstrap'; an operator copy-pasting the curl would 404. All five hits replaced. Verifications: gofmt clean, go vet ./internal/service/ ./internal/api/router/ clean, go test -short -count=1 green across internal/service + internal/api/router, including the 6 new auditor sub-tests (PASS).	2026-05-09 20:23:41 +00:00
shankar0123	3ef45e2ad4	auth-bundle-1 Phase 6-7-8: bootstrap path + scope-down CLI + auditor-role split ... # Phase 6 — day-0 admin bootstrap * internal/auth/bootstrap/ (new package): Strategy interface + EnvTokenStrategy with constant-time compare, one-shot consumption via sync.Mutex, optional admin-existence probe. Bundle 2's OIDC- first-admin will plug in alongside as an alternate Strategy. * BootstrapService.ValidateAndMint: validates the operator's CERTCTL_BOOTSTRAP_TOKEN, mints a 32-byte (64-hex-char) random API key value, persists the SHA-256 hash to api_keys, grants r-admin via actor_roles, AddHashed's the runtime keystore so the just- minted key authenticates the next request without restart, and records bootstrap.consume to the audit trail with category=auth. * internal/auth/keystore.go (new): KeyStore interface + StaticKeyStore (immutable env-var-only path) + MutableKeyStore (env-var keys + DB-loaded api_keys + runtime AddHashed). The auth middleware now consumes a KeyStore so the bootstrap path can extend the lookup table at runtime. * migrations/000031_api_keys.up/down.sql: api_keys table with (id, name UNIQUE, key_hash UNIQUE, tenant_id, admin, created_by, created_at, expires_at, last_used_at). Idempotent. * /v1/auth/bootstrap GET (probe) + POST (mint) — auth-exempt. Both routes documented in api/openapi.yaml + AuthExemptRouterRoutes allowlist updated. The token never leaves internal/auth/bootstrap; the minted plaintext key flows only into the HTTP response body. * Startup warning emitted when CERTCTL_BOOTSTRAP_TOKEN is set AND admin actors already exist (config drift signal). * Tests: 4 strategy invariants (empty token born disabled, wrong token=ErrInvalidToken without consumption, one-shot consumption, admin-exists closes path), 5 service tests (happy path + actor- name validation + propagation of strategy errors + nil-deps guard + 32-byte entropy budget), 8 HTTP-handler tests (status 201/410/401/400 mapping + token-leak hygiene scan of slog + audit details + Location header). Token-leak test redirects slog.Default to a buffer for the test scope. # Phase 7 — API-key migration + scope-down CLI * GET /v1/auth/keys handler + service method ListKeys backed by ActorRoleRepository.ListDistinctActors. Returns one row per (actor_id, actor_type) pair with the slice of role IDs they hold. Permission: auth.role.list. * internal/cli/auth_scope_down.go: AuthListKeys, AuthScopeDown (interactive), AuthScopeDownNonInteractive (JSON config), AuthScopeDownSuggest (--suggest with optional --apply). The synthetic actor-demo-anon is filtered out of every interactive / bulk path; non-interactive flow logs and skips it explicitly. * SuggestRoleFromAuditEvents (pure function): walks 30 days of audit events per actor and returns the narrowest matching role (admin / mcp / viewer / agent / operator) plus a one-line reason. Classification: any admin-shaped action wins; otherwise all-MCP → mcp; all-read-only → viewer; all-agent-shaped → agent; otherwise operator. Test table pins all six classifications. * CLI subcommand tree extended: 'auth keys list' + 'auth keys scope-down [--non-interactive <cfg>] [--suggest [--apply]]'. * CHANGELOG.md leads v2.1.0 with the SECURITY: AUDIT YOUR API KEYS call-out + four flow examples. # Phase 8 — auditor role + event_category column * migrations/000032_audit_category.up/down.sql: ALTER TABLE audit_events ADD COLUMN event_category TEXT NOT NULL DEFAULT 'cert_lifecycle' + CHECK constraint (cert_lifecycle/auth/config) + (event_category) and (event_category, timestamp DESC) indexes for the auditor-filter query path. WORM trigger from migration 000018 continues to enforce append-only at the DB layer (DDL is not blocked). * domain.AuditEvent gains EventCategory string (omitempty); domain.EventCategoryCertLifecycle / Auth / Config constants. * AuditService.RecordEventWithCategory sibling of RecordEvent; legacy callers stay on RecordEvent (defaults to cert_lifecycle). Auth callers (RoleService, ActorRoleService, BootstrapService) switched to RecordEventWithCategory(..., 'auth', ...). * GET /v1/audit?category=<cat>: handler accepts the optional query param, validates against the enum (400 on invalid value), dispatches through ListAuditEventsByCategory. OpenAPI updated with the new query param + AuditEvent.event_category schema. * Postgres AuditRepository.Create now writes event_category; AuditRepository.List filters on it; AuditFilter.EventCategory gates the WHERE clause. * Tests: 5 audit-category-filter HTTP tests (dispatch routing, back-compat fallback, 400 for invalid values, all 3 enum values accepted, page+category combine, JSON output surfaces the field). 3 auditor-role invariants (auditor holds exactly audit.read+audit.export, no mutating perms, disjoint from viewer except audit.read). # Cross-phase wiring * HandlerRegistry.Bootstrap field added; cmd/server/main.go wires the bootstrap service ahead of RegisterHandlers (extracted assembleNamedAPIKeys helper into auth_backfill.go, moved the keystore + bootstrap construction up alongside the auth repos). * AuthCheckResolver / AuthActorRoleService extended with ListKeys to satisfy the Phase 7 surface; existing fakes updated. * fakeAudit + mockAuditService stubs in tests gain RecordEventWithCategory + ListAuditEventsByCategory; existing tests untouched. # Verifications * gofmt -l: clean across every modified file. * go vet ./...: clean. * staticcheck across internal/auth + handler + router + cli + service + repository + cmd + domain: clean. * go test -short -count=1: green across every Bundle-1-touched package — internal/auth (incl. bootstrap), internal/api/handler, internal/api/router, internal/cli, internal/service/auth, internal/service, internal/domain/auth, internal/repository/postgres, cmd/server, cmd/cli, plus internal/scheduler, internal/api/middleware, cmd/agent, internal/mcp.	2026-05-09 20:15:43 +00:00
shankar0123	60a589ab96	auth-bundle-1 Phase 0-5 closure: demo-mode wire, named-key backfill, AuthCheck enrichment, OpenAPI schema, intermediate-ca comment refresh ... Closes the 5 gaps the post-Phase-5 audit flagged on dev/auth-bundle-1. C1: cmd/server/main.go now selects auth.NewDemoModeAuth() when CERTCTL_AUTH_TYPE=none and falls back to auth.NewAuthWithNamedKeys otherwise. Pre-closure, the no-op pass-through that NewAuthWithNamedKeys returns for empty keys would have left ActorIDKey / ActorTypeKey / TenantIDKey unpopulated and 401'd every Phase-3.5 rbacGate-wrapped admin route + every Phase-4 RBAC handler in demo deployments. NewDemoModeAuth injects the synthetic 'actor-demo-anon' actor seeded by migration 000029, which holds r-admin at global scope. C2: backfillNamedKeyActorRoles startup hook (cmd/server/auth_backfill.go) iterates CERTCTL_API_KEYS_NAMED entries (and legacy CERTCTL_AUTH_SECRET synthesized fallbacks) and grants r-admin or r-viewer to each via authActorRoleRepo.Grant before the HTTP server starts accepting requests. Idempotent via ON CONFLICT DO NOTHING in the repo. Failures log a warning but are non-fatal — the server still starts and the operator can fix grants via /v1/auth/keys. Helper extracted from main.go so the role-mapping invariant is pinned by 4 focused unit tests (admin->r-admin, non-admin->r-viewer, empty no-op, grant-error non-fatal, nil-logger safe). M1: HealthHandler.AuthCheck now returns actor_id, actor_type, tenant_id, roles, effective_permissions, and admin_via_role when the optional AuthCheckResolver is wired (production path: authCheckResolverAdapter wraps the postgres ActorRoleRepository in main.go). Nil resolver preserves the legacy {status, user, admin} contract for back-compat with pre-Bundle-1 GUIs and test fixtures. Adds 2 regression tests + 1 fake resolver shim. M2: refreshes the stale 'Admin gate: every method calls auth.IsAdmin first' comment on IntermediateCAHandler — the gate moved to router.go::rbacGate via auth.RequirePermission middleware in Phase 3.5; the new comment block points readers there. M4: 11 RBAC routes (auth/me, auth/permissions, 5 role lifecycle, 2 role-permission grant/revoke, 2 actor-role grant/revoke) added to api/openapi.yaml under the [Auth] tag with operationIds and shared AuthRole / AuthRolePermission schemas. AuthCheck path extended with the Bundle-1 enrichment fields. The 11 entries removed from openapi_parity_test.go::SpecParityExceptions. Tests: go vet + staticcheck + go test -short -count=1 green across cmd/server/, internal/auth/, internal/api/router/, and internal/api/handler/. New tests: 4 backfill unit tests, 2 AuthCheck M1 enrichment tests, 1 demo-mode + rbacGate chain integration test (TestRBACGate_DemoModeChainReachesHandler). Branch SECURITY.md (cowork/auth-bundle-1-SECURITY.md, not part of this commit) captures the full posture of dev/auth-bundle-1 as of this closure for the operator's pre-merge review.	2026-05-09 19:33:07 +00:00
shankar0123	7ff2e2de08	auth-bundle-1 Phase 3.5: handler IsAdmin -> router-wrapped RequirePermission ... Phase 3.5 atomic conversion. The five legacy admin-gated handlers (bulk_revocation, admin_crl_cache, admin_scep_intune, admin_est, intermediate_ca) had their in-body auth.IsAdmin checks removed; the gate moved to router.go via auth.RequirePermission middleware wrapping each route. Non-admin operators with the right scoped permission can now reach these endpoints; legacy in-body admin checks no longer block them. Migration 000030_rbac_admin_perms.up.sql ships five admin-only fine-grained permissions: cert.bulk_revoke, crl.admin, scep.admin, est.admin, ca.hierarchy.manage. All five are seeded into r-admin only; operator/viewer/agent/mcp/cli/auditor do not receive them by default. Operators can grant any of these to a custom role via the Phase 4 RBAC API. Idempotent + transaction-wrapped. internal/domain/auth/validate.go::CanonicalPermissions extended with the five new entries so RoleService.AddPermission accepts them. internal/api/router/router.go: HandlerRegistry gains a Checker field (auth.PermissionChecker). New rbacGate(checker, perm, handler) helper wraps a handler with auth.RequirePermission middleware; nil-checker fall-through preserves test/demo deployments without the RBAC stack. 12 admin routes wrapped: cert.bulk_revoke (POST /api/v1/certificates/bulk-revoke + POST /api/v1/est/certificates/bulk-revoke), crl.admin (GET /api/v1/admin/crl/cache), scep.admin (GET /api/v1/admin/scep/profiles + GET /api/v1/admin/scep/intune/stats + POST /api/v1/admin/scep/intune/reload-trust), est.admin (GET /api/v1/admin/est/profiles + POST /api/v1/admin/est/reload-trust), ca.hierarchy.manage (POST /api/v1/issuers/{id}/intermediates + GET /api/v1/issuers/{id}/intermediates + POST /api/v1/intermediates/{id}/retire + GET /api/v1/intermediates/{id}). cmd/server/main.go: HandlerRegistry.Checker wired with the same authPermissionCheckerAdapter shim Phase 4 introduced for AuthHandler. Same adapter; one source of truth. Handler bodies: removed eight in-body auth.IsAdmin checks across the 5 files. bulk_revocation.go's BulkRevoke + BulkRevokeEST, admin_crl_cache.go::ListCache, admin_scep_intune.go's three methods, admin_est.go's two methods, intermediate_ca.go's four methods. Replaced each with a comment naming the new gate location. Unused 'github.com/certctl-io/certctl/internal/auth' imports removed. Test triplet rewrite: deleted obsolete _NonAdmin_Returns403 and _AdminExplicitFalse_Returns403 tests across 6 test files (5 handler tests + bulk_revocation_est_test.go) — they tested the now-removed in-body gate. _AdminPermitted_ForwardsActor tests stay intact: they pin the actor-passthrough invariant which is still relevant. Added internal/api/router/rbac_gate_integration_test.go with four router-level integration tests pinning the new gate: deny → 403 + handler not reached, permit → 200 + handler reached, nil-checker → fall-through, no-actor → 401. M-008 admin-gate registry: AdminGatedHandlers map now empty (Phase 3.5 invariant: zero in-handler auth.IsAdmin call sites; only health.go's informational caller remains). m008_admin_gate_test.go retains the scan to enforce the invariant going forward; new admin-gated routes must wrap at router.go::rbacGate, not gate in-handler. Updated error message to direct future contributors to the new pattern. Verifications: gofmt clean across all touched files; go vet ./... clean; go test -short across internal/auth, internal/service/auth, internal/api/handler, internal/api/router, cmd/server all green. Branch: dev/auth-bundle-1. Commit chain: 99a012e (Phase 0 extract) -> 19497ee (Phase 1 schema + repo) -> bd54d5f (Phase 2 service) -> d473398 (Phase 3 primitive) -> b169f25 (Phase 4 + 5) -> THIS (Phase 3.5 conversion). Phase 6+ (bootstrap, scope-down, auditor, approval-bypass closure, GUI, docs) on subsequent sessions.	2026-05-09 17:00:30 +00:00
shankar0123	b169f258de	auth-bundle-1 Phase 4 + 5: RBAC HTTP API + CLI surface ... Phase 4 (HTTP API): * internal/api/handler/auth.go: AuthHandler with 12 endpoints under /api/v1/auth/* — ListRoles, GetRole, CreateRole, UpdateRole, DeleteRole, ListPermissions, AddRolePermission, RemoveRolePermission, AssignRoleToKey, RevokeRoleFromKey, Me. callerFromRequest builds an authsvc.Caller from the Phase 3 ActorIDKey/ActorTypeKey/TenantIDKey context values. writeAuthError translates service + repository sentinels into HTTP status codes (401/403/404/409/400/500). 14 handler tests with in-memory fakes pin the HTTP shape + error mapping. * internal/api/router/router.go: HandlerRegistry gains an Auth field; 11 new routes registered. openapi_parity_test SpecParityExceptions extended with the new auth routes (OpenAPI YAML schema land in a Phase 4 follow-up commit so the schema review is its own atomic change; the route shape is fully documented inline via the Go type definitions until then). * cmd/server/main.go: wires the postgres auth repos (RoleRepository, PermissionRepository, ActorRoleRepository) + the Authorizer + RoleService/PermissionService/ActorRoleService into the new AuthHandler. Adds authPermissionCheckerAdapter to bridge the typed-string Authorizer signature to the auth.PermissionChecker interface (avoids an internal/auth → internal/service/auth import cycle). Phase 5 (CLI): * cmd/cli/main.go: adds 'auth' command dispatch with subcommands roles/permissions/keys/me. * internal/cli/auth.go: AuthMe, AuthListRoles, AuthGetRole, AuthListPermissions, AuthAssignRoleToKey, AuthRevokeRoleFromKey methods on Client. Mirrors the Phase 4 HTTP surface. Phase 3.5 (handler IsAdmin → middleware-wrapped RequirePermission) DEFERRED. Honest reasoning: (1) The 5 admin handlers (bulk_revocation, admin_crl_cache, admin_scep_intune, admin_est, intermediate_ca) currently gate via auth.IsAdmin checks INSIDE the handler bodies. Converting cleanly requires moving the gate to the router (auth.RequirePermission middleware wrap) AND removing the in-handler check AND rewriting the existing 3-test triplets per handler (M-008 pinned: _NonAdmin_Returns403 / _AdminExplicitFalse_Returns403 / _AdminPermitted_ForwardsActor) because the existing tests call the handler function directly, bypassing middleware. After conversion, those tests would pass without 403'ing because the gate moved away — the test invariants need to flow through a router-level integration setup instead. (2) Picking the right permission per handler is a security-review-worthy decision. Using existing operator-class perms (cert.revoke, issuer.edit) widens access from admin-only to operator-class; adding new admin-only perms (cert.bulk_revoke, crl.admin, scep.admin, est.admin, ca.hierarchy.manage) requires a migration 000030 plus a coordinated catalogue update in internal/domain/auth/validate.go. Both options are defensible but warrant a focused commit, not a 5-handler sweep mixed in with the API + CLI work. (3) The conversion can be done now without functional regressions IF we leave the in-handler IsAdmin checks in place AND add middleware wraps as defense-in-depth — but that's the worst of both worlds (legacy gate still blocks non-admin operators, defeating the point of RBAC; new gate adds runtime cost with no semantic change). A clean conversion needs the in-handler check removed. Concrete plan for Phase 3.5 (separate commit, next session): (a) add new admin-only perms via migration 000030 OR document the widening to operator-class; (b) wrap each of the 5 admin routes with auth.RequirePermission(checker, perm, nil) in router.go; (c) remove auth.IsAdmin checks from the 5 handler bodies; (d) move the M-008 _NonAdmin/_AdminExplicitFalse tests to router-level integration tests, keep _AdminPermitted as a direct handler test for actor-passthrough; (e) update m008_admin_gate_test.go registry to track auth.RequirePermission middleware wraps in router.go instead of auth.IsAdmin call sites in handler files. Verifications: go vet ./... clean; gofmt clean across all touched files; go test -short -count=1 across internal/auth, internal/service/auth, internal/api/handler, internal/api/router, internal/cli, cmd/server, cmd/cli all green (one transient too-many-open-files retry on internal/cli + internal/api/router; second run clean). Branch: dev/auth-bundle-1. Commit chain: 99a012e (Phase 0 extract) -> 19497ee (Phase 1 schema + repo) -> bd54d5f (Phase 2 service) -> d473398 (Phase 3 primitive) -> THIS (Phase 4 + 5).	2026-05-09 16:43:48 +00:00
shankar0123	d473398aba	auth-bundle-1 Phase 3 (primitive): RequirePermission middleware + demo-mode + protocol allowlist ... Bundle 1 / Phase 3 (primitive ship): the load-bearing RBAC middleware factory plus its dependencies. Handler conversion sweep (5 admin files: bulk_revocation.go, admin_crl_cache.go, admin_scep_intune.go, admin_est.go, intermediate_ca.go) + m008_admin_gate_test.go registry update is Phase 3.5 follow-on; this commit ships the primitive so 3.5 is mechanical. New context keys (internal/auth/context.go): ActorIDKey, ActorTypeKey, TenantIDKey alongside the legacy UserKey + AdminKey. New helpers GetActorID / GetActorType / GetTenantID with safe fallbacks (UserKey for actor id, ActorTypeAPIKey for missing type, DefaultTenantID for missing tenant). Constants DemoAnonActorID + ActorTypeAPIKey + ActorTypeAnonymous mirror internal/domain/auth without an import cycle. RequirePermission factory (internal/auth/require_permission.go): wraps a handler and gates it behind a named permission. 401 when no actor, 403 when actor lacks permission, 500 on repository error. Skips the gate entirely for protocol endpoints (ACME / SCEP / EST / OCSP / CRL) per the audit's Category F do-not-gate allowlist. PermissionChecker is an interface so internal/auth doesn't depend on internal/service/auth (cmd/server wires the concrete Authorizer at startup). HasPermission is the imperative variant for handlers that branch behaviour rather than 403'ing. ScopeFunc closure extracts the scope type + id from the request for per-resource gating. Protocol-endpoint allowlist (internal/auth/protocol_endpoints.go): IsProtocolEndpoint matches /acme, /scep, /.well-known/est, /.well-known/pki/ocsp, /.well-known/pki/crl prefixes. Adding a new protocol endpoint MUST update this list and add a parallel test. Demo-mode synthetic admin (internal/auth/middleware.go::NewDemoModeAuth): when CERTCTL_AUTH_TYPE=none is configured, this middleware injects ActorID=actor-demo-anon, ActorType=Anonymous, TenantID=t-default, plus the legacy UserKey + AdminKey for back-compat with existing handlers. The synthetic actor's admin-role grant is seeded by migration 000029 so RequirePermission resolves through the JOIN like any other actor. cmd/server startup wires this middleware only when none-mode is configured. API-key middleware extension: NewAuthWithNamedKeys now populates the new keys (ActorIDKey, ActorTypeKey=APIKey, TenantIDKey=t-default) alongside UserKey + AdminKey on every successful Bearer match. Existing handlers continue to read UserKey / IsAdmin until the Phase 3.5 sweep converts them to RequirePermission. Test coverage: TestRequirePermission_NoActorReturns401, TestRequirePermission_GrantedActorReaches200, TestRequirePermission_DeniedActorReturns403, TestRequirePermission_CheckerErrorReturns500, TestRequirePermission_ProtocolEndpointBypassesGate (covers all 5 prefixes), TestRequirePermission_ScopeFnExtractsResourceID, TestIsProtocolEndpoint_PrefixesOnly, TestNewDemoModeAuth_InjectsSyntheticActor, TestNewAuthWithNamedKeys_PopulatesPhase3ContextKeys. fakeChecker pins the contract without a database. Phase 3.5 follow-on (NOT in this commit): convert each of the 5 admin handlers from auth.IsAdmin checks to auth.RequirePermission middleware in router.go; update internal/api/handler/m008_admin_gate_test.go to track auth.RequirePermission call sites instead of (or alongside) auth.IsAdmin; pick the right permission per handler (cert.revoke for bulk_revocation, etc.). Each handler conversion needs the 3-test triplet (_NonAdmin_Returns403 / _AdminExplicitFalse_Returns403 / _AdminPermitted_ForwardsActor) per M-008. Branch: dev/auth-bundle-1. Phase 2 was prior commit (service layer). Phase 3.5 (handler conversion) + Phase 4 (HTTP API) on the next session.	2026-05-09 16:20:04 +00:00
shankar0123	bd54d5f7fa	auth-bundle-1 Phase 2: RBAC service layer + Authorizer primitive ... Bundle 1 / Phase 2: ships PermissionService, RoleService, ActorRoleService, and the Authorizer primitive that Phase 3 RequirePermission middleware calls on every gated request. Authorizer.CheckPermission semantics: a grant matches when (a) the permission name equals the requested permission AND (b) the grant is global-scoped OR the grant scope_type+scope_id exactly match the request. Global beats specific; per-resource grants widen the effective set rather than shadowing global. Hot-path query is one ActorRoleRepository.EffectivePermissions JOIN call (already shipped in Phase 1) plus an in-memory walk; Phase 12 will add benchmarks + caching if the JOIN cost shows up at scale. Privilege-escalation guard: ActorRoleService.Grant and Revoke require the caller to hold auth.role.assign globally. Without it, ErrSelfRoleAssignment. System callers (AsSystemCaller()) bypass the check; bootstrap, migrations, scheduler-initiated grants use this path. Reserved actor actor-demo-anon is rejected on Grant + Revoke so the demo path stays alive even after a misclick (ErrAuthReservedActor). Caller abstraction: every service entry point takes *Caller (ActorID, ActorType, TenantID, IsSystem). CallerFromContext is a stub returning ErrUnauthenticated; Phase 3 wires the middleware-context bridge that fills the Caller from request context. The contract is pinned by TestCallerFromContext_Phase2ReturnsUnauthenticated so the Phase 3 upgrade is observable. Audit recording: every mutating service operation calls AuditService.RecordEvent. Bundle 1 Phase 8 adds the event_category column + parameter and back-fills 'auth' for these calls; until then the rows go in with the default category. Test coverage: in-memory fakeRoleRepo / fakePermissionRepo / fakeActorRoleRepo / fakeAudit pin the privilege-escalation invariants (ErrUnauthenticated for nil caller, ErrForbidden for missing perm, ErrInvalidPermission for non-canonical permission name, ErrSelfRoleAssignment for Grant without auth.role.assign, ErrAuthReservedActor for actor-demo-anon mutations, system-caller bypass) without requiring testcontainers. Phase 12 will add live-Postgres integration coverage. Branch: dev/auth-bundle-1. Phase 1 was 19497ee (RBAC schema + repo). Phase 3 (middleware integration) is the next commit on this branch.	2026-05-09 16:20:04 +00:00
shankar0123	19497eef87	auth-bundle-1 Phase 1: RBAC schema + domain types + repository layer ... Bundle 1 / Phase 1: ships the RBAC primitive as schema + domain types + repo layer. Service-layer wiring lands in Phase 2; middleware integration in Phase 3. Schema (migrations/000029_rbac.up.sql, 272 lines, idempotent, transaction-wrapped): tenants, roles, permissions, role_permissions, actor_roles. TEXT primary keys with prefixes (t-, r-, p-, ar-) per CLAUDE.md Architecture Decisions. TIMESTAMPTZ time columns. FK cascade explicit (tenant CASCADE, role RESTRICT, actor CASCADE). Three-value scope_type CHECK ('global', 'profile', 'issuer') matched 1:1 with internal/domain/auth.ScopeType. UNIQUE(tenant_id, name) on roles, UNIQUE(name) on permissions, UNIQUE(actor_id, actor_type, role_id, tenant_id) on actor_roles. Seeds: t-default tenant, 7 default roles (admin, operator, viewer, agent, mcp, cli, auditor), 33-permission canonical catalogue (cert.* / profile.* / issuer.* / target.* / agent.* / audit.* / auth.role.* / auth.key.* / auth.bootstrap.use), full role->permission grant matrix at global scope. Demo-mode preservation: actor-demo-anon seeded with admin role unconditionally; Phase 3 wires the auth middleware to inject this actor into the context when CERTCTL_AUTH_TYPE=none. Reserved system actor; Phase 4 API rejects mutations / deletions targeting it with 409 Conflict. Domain types (internal/domain/auth/{types,validate,validate_test}.go): Tenant, Role, Permission, RolePermission, ActorRole structs with JSON tags. ScopeType enum (global/profile/issuer). ActorTypeValue mirrors internal/domain.ActorType to avoid an import cycle. CanonicalPermissions slice + DefaultRoles map are the single source of truth referenced by the migration; validate_test.go pins (a) no duplicate permissions, (b) every default-role permission is canonical, (c) admin holds the full catalogue, (d) seeded IDs carry the prefix convention, (e) ScopeType enum has exactly 3 values matching the CHECK constraint. Extended internal/domain/audit.go: added ActorTypeAPIKey + ActorTypeAnonymous to the existing User/System/Agent enum so the audit trail can distinguish API-key requests from federated humans (Bundle 2 OIDC) and demo-mode (CERTCTL_AUTH_TYPE=none). Existing code that records actor_type=User keeps working; new APIKey value used by Bundle 1 Phase 3 middleware. Repository layer (internal/repository/auth.go + internal/repository/postgres/auth.go): TenantRepository (Get, List, EnsureDefault). RoleRepository (Get, GetByName, List, Create, Update, Delete with ErrAuthRoleInUse on FK RESTRICT, ListPermissions, AddPermission idempotent, RemovePermission). PermissionRepository (List, GetByName, IsCanonical for fail-fast catalog check). ActorRoleRepository (ListByActor, ListByRole, Grant idempotent, Revoke, EffectivePermissions which is the JOIN that auth.RequirePermission will use in Phase 3 — returns deduplicated (permission, scope) triples honouring the not-yet-expired predicate so future time-bound grants work without code change). Sentinel errors ErrAuthNotFound, ErrAuthDuplicateName, ErrAuthRoleInUse, ErrAuthReservedActor, ErrAuthUnknownPermission for handler-layer 404/409/400 mapping. Verification: gofmt clean, go vet ./... clean, go test -short ./internal/domain/auth ./internal/repository/postgres pass. Integration tests against a live Postgres are gated by testing.Short() per repo convention; Phase 12 wires the testcontainers harness for full e2e coverage. Branch: dev/auth-bundle-1. Phase 0 was 99a012e (extract internal/auth/). Phase 2 (service layer) is the next bundle.	2026-05-09 16:00:08 +00:00
shankar0123	99a012e3be	auth-bundle-1 Phase 0: extract internal/auth/ from middleware package ... Bundle 1 / Phase 0: pure refactor splitting auth surface out of internal/api/middleware so Bundle 2 (OIDC + sessions) and the broader RBAC primitive (roles, permissions, scoped grants) have a clean home. Moved to internal/auth/: NamedAPIKey, HashAPIKey, AuthConfig, NewAuthWithNamedKeys, NewAuth, UserKey, AdminKey, GetUser, IsAdmin. Added testfixtures.go (WithActor / WithAdmin / WithActorAdmin) so handler tests don't construct context manually. Stayed in internal/api/middleware/: RequestID, Logging, NewLogging, Recovery, RateLimitConfig, NewRateLimiter (now imports auth.GetUser for per-user keying per audit Category C), CORSConfig, NewCORS, ContentType, CORS, GetRequestID, responseWriter, Chain, audit middleware (now imports auth.GetUser). Updated 22 caller files across cmd/, internal/api/handler/, internal/api/middleware/, internal/mcp/. Existing m008_admin_gate_test.go now scans for auth.IsAdmin( substring; Phase 3 will further evolve to track auth.RequirePermission. Behavior unchanged: all handler / middleware / service / connector / cmd / mcp tests pass with no test-logic edits, only import-path renames. Phase 0 exit criteria: internal/auth/ exists with 6 files; middleware.go went 575 -> 422 lines (auth-related ~150 lines moved out); grep -rE 'middleware\.(GetUser|IsAdmin|UserKey|AdminKey|NamedAPIKey|HashAPIKey|NewAuth)' returns 0 hits; context.WithValue(.*middleware.UserKey/AdminKey) returns 0 hits; go vet ./... clean; go test -short ./... green across all packages tested. Branch: dev/auth-bundle-1. Per cowork/auth-bundle-1-prompt.md, do not merge to master without (1) make verify green, (2) >= 2 external testers confirm, (3) >= 90% coverage on internal/auth/ in .github/coverage-thresholds.yml.	2026-05-09 15:51:31 +00:00
shankar0123	0e06f6c4fc	cli: promote --force on renew + require --reason on revoke (closes P3-1, P3-2) ... Closes findings P3-1 and P3-2 from the 2026-05-05 CLI/API/MCP↔GUI parity audit (cowork/cli-gui-parity-audit-2026-05-05/RESULTS.md). Both findings flagged hidden defaults that the CLI was sending without exposing them to operators: `force=false` baked into every renew payload, and a silent fallback to `reason="unspecified"` whenever --reason was omitted. P3-1 — promote --force on `certs renew` (full end-to-end plumbing) The pre-2026-05-05 CLI sent `{"force": false}` in the renew body. The API handler never decoded it — a textbook "lying field" per the operator's CLAUDE.md "complete path, not the easy path" rule: the body field stored a value, claimed to do something, and silently did nothing because the wire never reached the consumer. Adding a --force flag that also went unread would have created another lying field. This commit takes the complete path: service.CertificateService.TriggerRenewal grew a `force bool` parameter (internal/service/certificate.go). When force=true, the RenewalInProgress block is overridden so operators can recover stuck in-flight renewals where a previous job hung without releasing the status flag. Archived and Expired remain terminal blockers regardless of force — those are semantic dead-ends that --force should not paper over (archived = decommissioned, expired = issue a new cert instead of renewing a dead one). handler.CertificateHandler.TriggerRenewal parses force from ?force=true (or ?force=1) query param, OR {"force": true} JSON body, whichever the client picks. Defaults to false. Passes through to the service. internal/cli/client.go::RenewCertificate(id, force bool) sends ?force=true on the URL when --force is set. The historical hardcoded `{"force": false}` body is gone — no more lying field. cmd/cli/main.go dispatches `certs renew <id> [--force]` (ID-first flag-second convention matches the existing `agents retire <id> [--force]`). P3-2 — require --reason on `certs revoke` (Option A: strict refusal) The pre-2026-05-05 CLI dropped to `--reason unspecified` whenever the operator omitted the flag. Compliance reporting (RFC 5280 §5.3.1, PCI- DSS §3.6, HIPAA §164.312) relies on the reason code being meaningful; silent fallback defeats the audit trail because every revocation looks identical. cmd/cli/main.go dispatch refuses to send when --reason is empty, prints the canonical RFC 5280 §5.3.1 reason-code menu, and exits non-zero. internal/cli/client.go exposes ValidRevokeReasons() returning the canonical camelCase list (unspecified, keyCompromise, caCompromise, affiliationChanged, superseded, cessationOfOperation, certificateHold, removeFromCRL, privilegeWithdrawn, aaCompromise) and NormalizeRevokeReason() that accepts both camelCase and snake_case inputs and normalises to the canonical wire form. Off-list reasons are rejected at dispatch with the menu re-printed. Test pins: internal/cli/client_test.go::TestClient_RenewCertificate_ForceFlag — --force=true sends ?force=true with empty body; --force=false sends no query and no body. internal/cli/client_test.go::TestNormalizeRevokeReason + TestValidRevokeReasons — canonical-camelCase + snake_case + reject- off-enum behaviour. cmd/cli/dispatch_test.go::TestHandleCerts_Revoke_RequiresReason + TestHandleCerts_Revoke_RejectsUnknownReason + TestHandleCerts_Renew_ForceFlag — dispatch-layer pins for the same contracts. internal/api/handler/certificate_handler_test.go::TestTriggerRenewal_ ForceQueryParam — query-param passthrough (no-flag, force=true, force=1, force=false) flows through to the service-layer parameter. internal/service/certificate_test.go::TestTriggerRenewal_ ForceOverridesInProgress — force=false preserves the RenewalInProgress block; force=true clears it. Existing TestTriggerRenewal_Archived extended to assert force=true still blocks Archived (terminal-state guarantee). Docs: docs/reference/cli.md updated with the --force example for renew and the strict --reason semantics for revoke (including snake_case input acceptance). Acceptance gate (verified): - go build ./cmd/server/... ./cmd/agent/... ./cmd/cli/... ./cmd/mcp-server/... clean. - go vet ./... clean. - go test -short -count=1 ./... pass repo-wide. - bash scripts/ci-guards/openapi-handler-parity.sh clean (router 178, OpenAPI 144, exceptions 36 — unchanged; we add parameter parsing, not routes). - gofmt -l clean.	2026-05-05 19:49:34 +00:00
shankar0123	ff75361553	mcp(coverage): add 34 tools across 7 domains to close 2026-05-05 parity audit P1 findings ... Closes findings P1-1..P1-35 from the 2026-05-05 CLI/API/MCP↔GUI parity audit (cowork/cli-gui-parity-audit-2026-05-05/RESULTS.md). Before this bundle, 35 operator-facing API endpoints had GUI surfaces but no MCP counterpart — operators using AI assistants for cert lifecycle work in regulated environments had to drop to curl for approve/reject, health-check acknowledgement, renewal-policy CRUD, network-scan triggering, discovery triage, intermediate-CA management, and job verification. Tool count: 87→121 in tools.go (+34), 6 unchanged in tools_est.go. Re-derive via grep -cE 'gomcp\\.AddTool\\(' internal/mcp/tools.go internal/mcp/tools_est.go. The 7 phases (matching the bundle prompt at cowork/mcp-coverage-expansion-prompt.md): Phase A — Approvals (P1-28..P1-31, 4 tools) list_approvals, get_approval, approve_request, reject_request. Two-person-integrity contract (ErrApproveBySameActor → HTTP 403) is preserved automatically: the decided_by actor is derived server-side from middleware.UserKey, NOT from request body, so the MCP server's authenticated API-key identity becomes the audit-trail actor. The MCP input schema deliberately omits any actor_id field to prevent client-side spoofing. Phase B — Health Checks (P1-20..P1-27, 8 tools) list, summary, get, create, update, delete, history, acknowledge. Mirrors the existing target-resource shape; acknowledge takes optional 'actor' string captured in the audit row (handler defaults to 'unknown' if absent). Phase C — Renewal Policies (P1-1..P1-5, 5 tools) Standard CRUD against /api/v1/renewal-policies. Distinct from the legacy 'policy' tools that point at the same path — these expose the renewal-policy domain explicitly with full alert_channels + alert_severity_map field shape. Phase D — Network Scan Targets (P1-14..P1-19, 6 tools) CRUD + trigger_scan. trigger_network_scan returns the discovery- scan body so the AI can chain into list_discovered_certificates filtered by agent_id. Phase E — Discovery read-side (P1-10..P1-13, 4 tools) list_discovered_certificates, get_discovered_certificate, list_discovery_scans, discovery_summary. Complements the pre-existing claim/dismiss tools (registered alongside Health historically per the I-2 closure). Phase F — Intermediate CAs (P1-6..P1-9, 4 tools) list, create (root + child via discriminator on body shape), get, retire. The handler is admin-gated via middleware.IsAdmin; the least-privilege boundary is enforced at the API layer (HTTP 403 for non-admin Bearer callers) — not by transport carve-out. Phase G — Verification + deployments (P1-32, P1-34, P1-35, 3 tools) list_certificate_deployments, verify_job, get_job_verification. P1-33 (POST /api/v1/agents/{id}/discoveries) is intentionally excluded — machine-to-machine push channel for agents reporting filesystem-scan results, not an operator-driven flow. Documented inline in the RegisterTools dispatch. Implementation: - 14 new input types in internal/mcp/types.go with jsonschema struct tags driving LLM tool discovery. - 7 register* functions in internal/mcp/tools.go each handling one phase, wired into RegisterTools dispatch in declaration order. - 34 new entries in tools_per_tool_test.go::allHappyPathCases — the existing in-process MCP harness (TestMCP_AllTools_HappyPath + TestMCP_AllTools_ErrorPath + TestMCP_RegisterTools_DispatchableToolCount) auto-extends coverage to cover every new tool: happy-path round- trip with fence-shape assertion, 5xx error-path with MCP_ERROR fence propagation, and 'every registered tool is dispatchable' guard. - docs/reference/mcp.md 'Available Tools' table expanded from 16 to 22 resource domains with current per-domain tool counts. Acceptance gate (verified): - go build ./cmd/server/... ./cmd/agent/... ./cmd/cli/... ./cmd/mcp-server/... clean across all four production binaries. - go vet ./... clean. - go test -short -count=1 ./internal/mcp/... pass (TestMCP_AllTools_* expanded to 127 tool round-trips). - go test -short -count=1 ./... pass repo-wide. - bash scripts/ci-guards/openapi-handler-parity.sh clean (router 178, OpenAPI 144, exceptions 36 — unchanged; we add MCP wrappers, not routes). - gofmt -l clean across the four touched files.	2026-05-05 19:29:57 +00:00
shankar0123	75097909e9		2026-05-05 18:18:29 +00:00
shankar0123	0f81c1b956	ci: re-fix CodeQL #32 + repair loadtest f5-mock build context ... Two unrelated CI failures from run #25305811340; fixed in one commit since neither needs the other to land first. CodeQL alert #32 (go/log-injection at middleware.go:68) reopened after b0fc067. The previous fix introduced a scrubLogValue helper backed by strings.NewReplacer; CodeQL's taint tracker only recognizes the literal strings.ReplaceAll pattern as a sanitizer (matches the OWASP example in the rule docs). Wrapper helpers and NewReplacer don't trigger the recognition, so the analyzer kept flagging. Fix: drop the helper. Inline strings.ReplaceAll chains directly at the call site for r.Method and r.URL.Path. Same runtime semantics (strip CR/LF/NUL); CodeQL pattern-matches the literal call so the alert can finally close. Loadtest CI failure (run #25305811340 'k6 throughput run' job at make loadtest): ERROR: failed to compute cache key: failed to calculate checksum of ref ...: "/deploy/test/f5-mock-icontrol": not found The f5-mock-icontrol Dockerfile has `COPY deploy/test/f5-mock-icontrol/ ./` which assumes the build context is the repo root. The docker-compose.test.yml f5-mock-icontrol service correctly uses the long-form build: build: context: .. # = repo root from deploy/docker-compose.test.yml dockerfile: deploy/test/f5-mock-icontrol/Dockerfile The loadtest compose at deploy/test/loadtest/docker-compose.yml used the shorthand: build: ../f5-mock-icontrol That sets context = the f5-mock-icontrol directory itself, breaking the Dockerfile's COPY (it tries to find the directory inside itself). Fix: change the loadtest compose to the long-form pattern matching docker-compose.test.yml, with context: ../../.. (= repo root from deploy/test/loadtest/) and explicit dockerfile path. Verified locally: gofmt: clean. go vet ./internal/api/middleware/...: exit 0. go test -short -count=1 ./internal/api/middleware/...: ok 0.253s. python3 -c 'import yaml; yaml.safe_load(...)' on the compose file: parses clean. grep -rnE 'scrubLogValue' internal/api/: zero references (helper fully dropped). References: https://github.com/certctl-io/certctl/security/code-scanning/32 CI run https://github.com/certctl-io/certctl/actions/runs/25305811340 Closes CodeQL #32 + restores loadtest CI.	2026-05-04 17:26:24 +00:00
shankar0123	b0fc067317	security: close CodeQL #17 (log injection) + #23 (SSRF false-positive reopen) ... Two CodeQL alerts in one sweep — both medium-impact follow-ups on already-merged guards. Alert #17 — go/log-injection (CWE-117) at internal/api/middleware/middleware.go:58: log.Printf("[%s] %s %s %d %v", requestID, r.Method, r.URL.Path, ...) r.Method and r.URL.Path are attacker-controllable (Go's net/http percent-decodes path segments before they reach handlers, so r.URL.Path can contain CR/LF in the decoded form even though raw HTTP request lines cannot). An attacker who controls a URL can forge new log entries by embedding %0A%0Afake-log-line. Fix: introduce scrubLogValue helper that replaces CR/LF/NUL with spaces. Apply to both r.Method and r.URL.Path. Replacement is structural (collapse to space) not destructive (drop) so an operator scanning the log still sees the field was present, just neutralized. Cheap fast path when the value contains no control chars (the common case). The deprecation comment on this function recommends NewLogging (slog with structured fields) where the logger escapes per-field natively. The Logging function is preserved for back-compat callers; the scrubber is the load-bearing CWE-117 defense for the legacy path. Alert #23 — go/request-forgery (CWE-918) at scep_probe.go:271: CodeQL reopened the alert after commit e6919cd. The commit's in-function validator dispatch went through a function-pointer override hook: validateURL := s.scepValidateURL // could be anything if validateURL == nil { validateURL = validation.ValidateSafeURL } if err := validateURL(rawURL); err != nil { ... } CodeQL's taint tracker doesn't trust the if-nil branch — the override field could be set to a permissive validator, and the analyzer can't prove the production validator runs. Fix: invert the dispatch. Always call validation.ValidateSafeURL literally first; only consult the test-override hook to grant an EXEMPTION when the production validator rejects: if err := validation.ValidateSafeURL(rawURL); err != nil { if s.scepValidateURL == nil || s.scepValidateURL(rawURL) != nil { return ... validate url error } } Same applies to ProbeSCEP's entry-point validator. Both call sites now have the literal validation.ValidateSafeURL call in-scope of the sink (client.Do), which CodeQL recognizes as a sanitizer. Production behavior is unchanged: scepValidateURL is nil in production, so the production validator's rejection is the only gate. Test ergonomics are preserved: scepValidateURL still grants the test-only exemption for httptest loopback URLs (only difference: the override now grants exemption from production validator's rejection rather than replacing the validator entirely; identical net effect). Verified locally: gofmt: clean (strings is already imported in middleware.go). go vet ./internal/api/middleware/... + ./internal/service/...: exit 0. go test -short ./internal/api/middleware/...: ok 0.244s. go test -short ./internal/service/...: ok 4.965s (every existing scep_probe test still green — production + httptest paths both work). References: https://github.com/certctl-io/certctl/security/code-scanning/17 https://github.com/certctl-io/certctl/security/code-scanning/23 Closes CodeQL #17. Re-closes CodeQL #23 with a fix CodeQL's taint tracker can verify.	2026-05-04 05:29:35 +00:00
shankar0123	9ef9f3cde3	refactor(scep+ejbca): drop dead conditionals on always-empty vars (CodeQL #18, #19) ... Two CodeQL go/comparison-of-identical-expressions alerts in one sweep — both Warning severity, both real dead-code (not false positives). CodeQL detected that each comparison's LHS variable was provably constant. Alert #18 — internal/api/handler/scep.go:612 (extractCSRFields): challengePassword := "" transactionID := "" // ... loop populates challengePassword from CSR.Attributes ... for _, attr := range csr.Attributes { if attr.Type.Equal(oidChallengePassword) { // populates challengePassword ONLY — transactionID stays "" } } if transactionID == "" && csr.Subject.CommonName != "" { // ← always true transactionID = csr.Subject.CommonName } transactionID was initialized to "" and never reassigned before the check. The conditional was always true; the MVP path was effectively "unconditionally fall back to CN". The RFC 8894 path (tryParseRFC8894 above this function) extracts transaction-ID properly from PKCS#7 authenticatedAttributes; the MVP path is for lightweight legacy clients that send the raw CSR with no PKCS#7 wrapping, and CN-as-transaction-ID is sufficient there. Fix: drop the dead transactionID local var + dead conditional; unconditionally set transactionID = csr.Subject.CommonName. No behavioral change — the runtime semantics are identical to before (every valid invocation already took the fallback). The CN extraction stays robust because the empty-CN case still produces an empty transactionID, which downstream callers handle. Alert #19 — internal/connector/issuer/ejbca/ejbca.go:415 (RevokeCertificate): serial := request.Serial issuerDN := "" // (comment: "if we have time..." — TODO never followed up) revokeURL := fmt.Sprintf("%s/certificate/%s/%s/revoke", apiURL, issuerDN, serial) if issuerDN == "" { // ← always true revokeURL = fmt.Sprintf("%s/certificate/%s/revoke", apiURL, serial) } issuerDN was hardcoded to "" two lines above. The first revokeURL line was unreachable dead code; the conditional always fired and the serial-only URL always won. EJBCA's REST API has both /certificate/{issuer_dn}/{serial}/revoke and /certificate/{serial}/revoke endpoints; the serial-only form is correct for typical certctl deployments where one EJBCA CA maps to one certctl issuer config (no overlapping serial spaces). Fix: drop the dead first revokeURL + dead conditional; build revokeURL once via the serial-only endpoint. No behavioral change — the runtime URL was always the serial-only one. Comment retained + expanded to document the future-enhancement path (parse issuer DN from IssuanceResult metadata + use the DN-qualified endpoint when a multi-CA EJBCA deployment surfaces). Verified locally: gofmt: clean. go vet ./internal/api/handler/... + ./internal/connector/issuer/ejbca/...: exit 0. go test -short -count=1 ./internal/api/handler/... + ejbca/...: PASS. Both fixes are pure dead-code removal — runtime behavior is byte- identical to pre-edit. The existing test suites would have caught any actual behavioral change. References: https://github.com/certctl-io/certctl/security/code-scanning/18 https://github.com/certctl-io/certctl/security/code-scanning/19 Closes both alerts.	2026-05-04 05:17:16 +00:00
shankar0123	2b4d0069d9	security(scep-intune): annotate verifyES256/RS256 SHA-256 as RFC-mandated (CodeQL #21 false positive) ... CodeQL alert #21 (go/weak-sensitive-data-hashing, severity: High) flagged the sha256.Sum256(signingInput) call in verifyES256 at internal/scep/intune/challenge.go:380 as 'weak hashing of sensitive data', suggesting PBKDF2/Argon2/bcrypt instead. This is a CodeQL false positive. The CodeQL query triggers when SHA-256 is used near *x509.Certificate (the trust pool) and infers 'this might be password hashing.' But the actual context is JWS signature verification: - verifyRS256 implements RFC 7518 §3.3 — 'RSASSA-PKCS1-v1_5 using SHA-256'. SHA-256 is spec-mandated. - verifyES256 implements RFC 7518 §3.4 — 'ECDSA using P-256 and SHA-256'. SHA-256 is spec-mandated. - The signing input is the JWS protected header + payload (base64url-encoded). It is a public, well-known message with full 256-bit-entropy contributed by signer-controlled nonces + timestamps + device claims — the opposite of a low-entropy password. - The output is verified against an asymmetric signature (rsa.VerifyPKCS1v15 / ecdsa.Verify), not compared to a pre-computed hash digest. This is signature verification, not password hashing. - Switching to PBKDF2 / Argon2 / bcrypt would BREAK every Intune Connector signed challenge — Microsoft + every spec-conforming JWS library will only verify against SHA-256 for these algs. Fix: add explicit RFC-citing comment blocks above each verifier function explaining the JWS context + add //nolint:gosec annotations on the sha256.Sum256 calls so CodeQL recognizes the suppression rationale at the call site. The annotation cites the specific RFC clause (7518 §3.3 / §3.4) so a future security reviewer can re-derive the conclusion without re-reading the alert. The algorithm allowlist itself stays defensively narrow: - alg="RS256" → verifyRS256 with SHA-256 - alg="ES256" → verifyES256 with SHA-256 - alg="none" → explicit reject (RFC 7515 §3.6 attack vector) - any other alg → reject as unsupported Pinned by existing tests: - TestValidateChallenge_HappyPath_RS256 - TestValidateChallenge_HappyPath_ES256_FixedWidth - TestValidateChallenge_HappyPath_ES256_DER - TestValidateChallenge_AlgNoneRejected - TestValidateChallenge_UnsupportedAlg The happy-path tests would fail if the verifiers switched to any non-SHA-256 digest — the alg allowlist makes the SHA-256 dependency load-bearing, which the existing test suite already proves. Verified locally: gofmt: clean. go vet ./internal/scep/intune/...: exit 0. go test -short -count=1 ./internal/scep/intune/...: PASS (every existing challenge_test.go subtest still green). Reference: https://github.com/certctl-io/certctl/security/code-scanning/21 Closes CodeQL alert #21 as a documented false positive — the //nolint annotations + RFC-citing comments are the load-bearing suppression. Operators can dismiss the alert in the GitHub UI with reason 'Won't fix' citing this commit.	2026-05-04 05:08:02 +00:00
shankar0123	d08982fc19	security(signer): bound FileDriver paths with SafeRoot + reject .. (CodeQL #27, CWE-22) ... CodeQL alert #27 (go/path-injection, CWE-22 / CWE-23 / CWE-36) flagged the os.WriteFile sink at internal/crypto/signer/file_driver.go:194 because the outPath flowed from operator-supplied config (CAKeyPath in the local issuer's encrypted config blob -> GenerateOutPath closure -> os.WriteFile) without a containment check. Threat model: Production wiring (cmd/server/main.go) constructs &signer.FileDriver{} and the local-issuer NewConnector wires GenerateOutPath off Config.CAKeyPath. CAKeyPath ships from the encrypted issuer config in PostgreSQL — settable only by an authenticated admin via the API. So the realistic exploit is: (a) Admin compromise -> CAKeyPath set to /etc/passwd -> FileDriver.Generate overwrites system files. (b) Future code path concatenates attacker-controlled fragments into the output path -> classic ../../etc/passwd traversal. Defense in depth: bound the write surface so admin-key-rotation errors and future regressions can't escape into arbitrary filesystem writes. Fix: internal/crypto/signer/file_driver.go gains: - SafeRoot string field on FileDriver. When set, every Load + Generate path MUST resolve under SafeRoot via filepath.Abs + strings.HasPrefix on cleaned paths. - validateSafePath helper that: * rejects empty paths * filepath.Clean()s the input * rejects paths whose cleaned form still contains a literal ".." segment (catches relative paths that escape above their start; absolute paths get collapsed by Clean) * resolves to filepath.Abs and (when SafeRoot non-empty) verifies containment via filepath.Separator-suffixed HasPrefix (the bare-prefix bug — SafeRoot=/var/lib/foo erroneously accepting /var/lib/foobar — has its own regression test below) - Load + Generate now call validateSafePath before any os.ReadFile / os.WriteFile. The validator is in the same function as the sink so CodeQL recognizes it as a guard. Tests (internal/crypto/signer/signer_test.go): TestFileDriver_Load_RejectsParentTraversal — relative path "../../etc/passwd" rejected with parent-directory error. TestFileDriver_Load_RejectsEmptyPath — empty path rejected. TestFileDriver_Generate_RejectsParentTraversal — write side, same pattern. TestFileDriver_SafeRoot_AcceptsContainedPath — happy path: a key file under SafeRoot succeeds. TestFileDriver_SafeRoot_RejectsEscape — absolute path outside SafeRoot rejected (the load-bearing CodeQL pin). TestFileDriver_SafeRoot_RejectsSiblingPrefix — pins the HasPrefix-with-separator subtlety: SafeRoot=/tmp/X must NOT accept /tmp/X-sibling. Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short -count=1 ./internal/crypto/signer/...: ok 1.605s go test -short -count=1 ./internal/connector/issuer/local/...: ok 4.908s (downstream FileDriver consumer) go test -short -count=1 ./internal/service/...: ok 4.029s Backwards-compat: when SafeRoot is unset, only the structural .. + empty-path checks fire — the existing FileDriver call sites in cmd/server/main.go and the existing unit tests pass unchanged. Production wiring SHOULD set SafeRoot via cmd/server/main.go in a follow-up commit (env-var-supplied CERTCTL_CA_KEY_DIR or similar). Reference: https://github.com/certctl-io/certctl/security/code-scanning/27 Closes CodeQL alert #27 (go/path-injection).	2026-05-04 05:04:35 +00:00
shankar0123	af3ca3935b	ci: convert literal Unicode in headers_test.go to \u escapes (ST1018) ... CI run #448 (commit 23c5930) failed staticcheck ST1018 on six test inputs that embedded literal invisible Unicode (U+202E RTL override, U+202D LRO, U+2066 LRI, U+200B ZWS, U+200C ZWNJ, U+180E MVS). golangci-lint enforces ST1018 in CI but go vet doesn't, so the local pre-commit gate (gofmt + go vet + go test) didn't catch it — the canonical Bundle 9 staticcheck-vs-vet drift case CLAUDE.md explicitly warns about. Fix: convert each literal-Unicode test input to its \uXXXX ASCII escape form. Verified via byte-level Python sed against UTF-8 byte sequences (\xe2\x80\xae -> ‮, \xe2\x80\xad -> ‭, \xe2\x81\xa6 -> ⁦, \xe2\x81\xa9 -> ⁩, \xe2\x80\x8b -> ​, \xe2\x80\x8c -> ‌, \xe1\xa0\x8e -> ᠎). The U+202C (PDF — Pop Directional Formatting) closer was caught by the same sweep since two RTL/LRO test cases use it. The runtime semantics are byte-identical — Go interprets ‮ and the literal U+202E byte sequence to the same rune. Only the source text changed. Verified locally: gofmt -l internal/validation/: clean. go vet ./...: exit 0. go test -short -count=1 ./internal/validation/...: ok 0.014s (all 4 test cases in TestSanitizeEmailBodyValue_StripsBidiOverride + the rest of the suite still green — semantics unchanged). Sandbox couldn't install staticcheck (disk pressure on /tmp/gopath), but the rule is mechanical: U+XXXX format chars in string literals must use \uXXXX. Every flagged literal is fixed. Reference: CI run https://github.com/certctl-io/certctl/actions/runs/25301809013 Closes the staticcheck regression on commit 23c5930 (security(email): sanitize body fields against content injection).	2026-05-04 05:00:14 +00:00
shankar0123	e6919cdaba	security(scep_probe): re-validate URL inside scepHTTPGet to close CodeQL #23 (CWE-918) ... CodeQL alert #23 (go/request-forgery, CWE-918 SSRF) flagged the client.Do(req) sink at internal/service/scep_probe.go:232 because the URL parameter to scepHTTPGet is taint-traced from the user- supplied input to ProbeSCEP without the analyzer recognizing the upstream sanitizer. The defense-in-depth was already in place: 1. validation.ValidateSafeURL at ProbeSCEP entry (line 75) — rejects obvious SSRF targets (loopback / link-local / cloud metadata literals) before any network call. 2. validation.SafeHTTPDialContext on the http.Transport — re-resolves the host at dial time and rejects connections to reserved IP ranges. This is the authoritative SSRF + DNS- rebinding guard. Even if step 1 was bypassed, the dial would still fail. But CodeQL's taint tracker doesn't follow the validator across function boundaries, so the alert stays open even though the code is safe. This commit re-runs validation.ValidateSafeURL inside scepHTTPGet immediately before http.NewRequestWithContext — sanitizer in the same function as the sink, which CodeQL recognizes as a guard. Bonus defense-in-depth: any future call site that wires a URL into scepHTTPGet without going through ProbeSCEP (e.g. a new code path that directly probes a discovered URL) inherits the same SSRF guard automatically. Fail-closed by default. The validator dispatch matches ProbeSCEP's pattern — tests override via s.scepValidateURL to hit httptest loopback servers; production callers use validation.ValidateSafeURL. The probe's existing httptest-based tests continue to work unchanged. Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short ./internal/service/...: ok 4.029s (every existing scep_probe test still green — the new revalidation is a no-op for tests that go through ProbeSCEP because the same validator already passed once at entry). Reference: https://github.com/certctl-io/certctl/security/code-scanning/23 Closes CodeQL alert #23 (go/request-forgery).	2026-05-04 04:58:51 +00:00
shankar0123	23c593089d	security(email): sanitize body fields against content injection (CodeQL #11, CWE-640) ... CodeQL alert #11 (go/email-injection, CWE-640 / OWASP Content Spoofing) flagged the wc.Write(message) sink at internal/connector/notifier/email/ email.go:208 because attacker-controllable fields flow into the email body unchecked. Threat model: Headers (From, To, Subject) were already protected by validation.ValidateHeaderValue (CWE-113 SMTP header injection, closed in commit 3853b74). The remaining gap was the body. An attacker controls multiple fields that surface to the body of alert/event notifications: - alert.Subject, alert.Message - event.Subject, event.Body, *event.CertificateID - alert.Metadata + event.Metadata key/value pairs These can carry CR/LF (forged 'Reply-To: attacker@evil.com' inside the body that recipients skim), NUL bytes (RFC 5321 4.5.2 violation that some MTAs truncate at), bidi-override Unicode (visually- spoofable URLs), zero-width / invisible Unicode (phishing), or malformed UTF-8 (Go emits U+FFFD which becomes a glyph in mail clients). The HTML email path (digest service) already uses html/template upstream and is safe via contextual auto-escape. This commit closes the plaintext path. Fix: internal/validation/headers.go gains SanitizeEmailBodyValue — a sanitizer that NEVER errors (the right contract for body content; over-eager rejection drops operator notifications) and scrubs: - NUL bytes (stripped entirely) - bare CR / LF (replaced with space — single fields should never carry their own line breaks; the surrounding template handles legitimate CRLFs) - C0 control chars < 0x20 except TAB - DEL (0x7F) + C1 control chars (0x80-0x9F) - U+FFFD (defense in depth: malformed UTF-8 -> Go emits this; strip so attacker-planted invalid bytes don't survive as an arbitrary glyph) - Bidi-override Unicode (U+202A..U+202E, U+2066..U+2069) - Zero-width / invisible Unicode (U+200B..U+200D, U+2060..U+2063, U+FEFF, U+180E) - Catch-all unicode.IsControl for anything not enumerated above Codepoint table uses numeric ranges rather than rune-literal switch cases — Go source rejects literal invisible characters (BOM U+FEFF) mid-file, so the table compares against numeric values. internal/connector/notifier/email/email.go applies the sanitizer at every interpolation site: - formatAlertBody: alert.ID/Type/Severity/Subject/Message (CreatedAt is time.Time -> RFC3339, structural, not sanitized) - formatEventBody: event.ID/Type/Subject/Body, *CertificateID (CreatedAt structural, not sanitized) - formatMetadata: both keys and values The sendEmail / formatEmailMessage call sites continue to validate headers (From / To / Subject) via the existing ValidateHeaderValue fail-closed gate; the new sanitizer is body-side only. Tests (internal/validation/headers_test.go): TestSanitizeEmailBodyValue_PreservesSafeInput Pin: ordinary ASCII, UTF-8 multibyte (résumé / 日本語 / مرحبا), tabs, common cert DNs, URLs all flow through unchanged. TestSanitizeEmailBodyValue_StripsControlChars Table-driven across NUL, bare LF/CR, CRLF, BEL, backspace, DEL, C1 (U+0080 / U+009F), U+FFFD, TAB-preserve. TestSanitizeEmailBodyValue_StripsBidiOverride 7 attacker payloads (RLO, LRO, LRI, zero-width space, ZWNJ, BOM, MVS) — each must produce a non-identity output. TestSanitizeEmailBodyValue_ContentSpoofingScenario The CodeQL example case: 'alert\r\nReply-To: attacker@evil.com\r\n Click https://evil.example.com/reset' — verify NO CR/LF survives. Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short -count=1 ./internal/validation/...: ok 0.374s go test -short -count=1 ./internal/connector/notifier/email/...: ok 0.186s Reference: https://github.com/certctl-io/certctl/security/code-scanning/11 Closes CodeQL alert #11 (go/email-injection).	2026-05-04 04:56:13 +00:00
shankar0123	34adcfbbe5	api, handler: 4 admin-gated CA hierarchy endpoints + OpenAPI (Rank 8 commit 4) ... Rank 8 commit 4 of 5. The API + RBAC layer that operators drive the new hierarchy management surface from. Endpoints (all admin-gated via middleware.IsAdmin; non-admin Bearer callers get 403): POST /api/v1/issuers/{id}/intermediates Discriminator on body shape: empty parent_ca_id + root_cert_pem + key_driver_id → CreateRoot (registers operator-supplied root CA). parent_ca_id non-empty → CreateChild (signs new sub-CA cert under parent). Service-layer error → HTTP code mapping: ErrCANotSelfSigned → 400 ErrCAKeyMismatch → 400 ErrPathLenExceeded → 400 ErrNameConstraintExceeded → 400 ErrInvalidCertPEM → 400 ErrParentCANotActive → 409 ErrIntermediateCANotFound → 404 (other) → 500 GET /api/v1/issuers/{id}/intermediates Returns flat list ordered by created_at; caller renders the tree from each row's parent_ca_id (nil = root). GET /api/v1/intermediates/{id} Single-row detail. POST /api/v1/intermediates/{id}/retire Two-phase: confirm=false → active→retiring; confirm=true → retiring→retired with active-children check (drain-first semantics; ErrCAStillHasActiveChildren → 409). Files changed: internal/api/handler/intermediate_ca.go — 4 handlers + handler-defined service interface (dependency inversion). internal/api/handler/intermediate_ca_test.go — 8 test variants (M-008 admin- gate triplet complete). internal/api/handler/m008_admin_gate_test.go — register the new admin-gated handler in AdminGatedHandlers so the M-008 coherence scanner stays green. internal/api/router/router.go — 4 r.Register calls + new IntermediateCAs field on HandlerRegistry. cmd/server/main.go — wire the postgres repo + service + handler. Reuses the same signer.FileDriver instance the OCSP responder bootstrap path feeds. api/openapi.yaml — 4 new operationIds, full body schema + status- code dispatch. Tests (8 in this commit): TestIntermediateCA_Handler_NonAdmin_Returns403 (admin gate — table-driven across all 4 endpoints) TestIntermediateCA_Handler_AdminExplicitFalse_Returns403 (defensive: AdminKey present but false ≠ AdminKey absent) TestIntermediateCA_Handler_AdminPermitted_ForwardsActor (admin actor forwarded to service for audit attribution) TestIntermediateCA_HandlerCreate_RootDispatch (body discriminator: empty parent_ca_id → CreateRoot) TestIntermediateCA_HandlerCreate_ChildDispatch (body discriminator: parent_ca_id present → CreateChild) TestIntermediateCA_HandlerCreate_BadRequestOnMissingRootBundle (validation: no parent + no root bundle → 400) TestIntermediateCA_HandlerCreate_ServiceErrorMappings (table-driven: 7 service errors → expected HTTP codes) TestIntermediateCA_HandlerRetire_TwoPhaseConfirm (confirm=false then confirm=true forwarded correctly) TestIntermediateCA_HandlerRetire_StillHasActiveChildren_Returns409 (drain-first contract — 409 not 500) Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short -count=1 ./internal/api/handler/...: ok 4.498s. bash scripts/ci-guards/openapi-handler-parity.sh: clean (router routes: 182, openapi operations: 148; the +4 new routes have +4 new operationIds — parity preserved). bash scripts/ci-guards/* (all 24 guards): clean. Out of scope of THIS commit (commit 5): - web/src/pages/IssuerHierarchyPage.tsx (recursive tree render). - docs/intermediate-ca-hierarchy.md sysadmin runbook (FedRAMP / financial-services / internal-PKI patterns). - docs/connectors.md hierarchy_mode row. - WORKSPACE-ROADMAP entries (HSM-backed roots, automated rotation, CRL chaining, NameConstraints templates, D3 dendrogram). Reference: cowork/rank-8-intermediate-ca-hierarchy-prompt.md, commit 4.	2026-05-04 02:26:24 +00:00
shankar0123	ae597f7f8d	local: tree-mode chain assembly + byte-equivalence pin (Rank 8 commit 3) ... Rank 8 commit 3 of 5. Load-bearing connector rewrite that activates the first-class CA hierarchy surface shipped by commits 1-2. Local connector changes: - New ChainAssembler interface (single-method seam) defined in the connector package — *service.IntermediateCAService satisfies it implicitly. Avoids the import cycle that would arise from pulling internal/service into internal/connector/issuer/local. - Three new optional fields on Connector: hierarchyMode, chainAssembler, treeIssuingCAID. Default zero values keep the pre-Rank-8 single-sub-CA flow byte-identical (no operator on the historical path sees any change in wire bytes). - Three new setters: SetHierarchyMode, SetChainAssembler, SetTreeIssuingCAID. Wired in cmd/server/main.go in commit 4 when the issuer's HierarchyMode column is read at boot. - resolveChainPEM helper centralizes the dispatch: tree mode + ChainAssembler set + treeIssuingCAID set → call AssembleChain over intermediate_cas otherwise (incl. tree mode with incomplete wiring) → fall back to historical c.caCertPEM Defense in depth: a misconfigured operator gets a working issuance, not a nil-deref panic. - IssueCertificate + RenewCertificate both delegate ChainPEM population to resolveChainPEM. The cert generation path (generateCertificate) is untouched — same key, same template, same signing. Tests (internal/connector/issuer/local/local_hierarchy_test.go): TestLocal_HierarchyMode_SingleVsTree_ByteIdentical ← LOAD-BEARING THE refuse-to-ship pin. Two connectors against the same on-disk CA cert+key: - A: pre-Rank-8 single-sub-CA mode (HierarchyMode unset). - B: tree mode wired against an in-memory ChainAssembler whose 1-level chain matches A's caCertPEM byte-for-byte. Asserts: 1. resA.ChainPEM == resB.ChainPEM (the byte-identical pin). 2. resA.ChainPEM == fixture root cert PEM (real fact about the wire format, not internal consistency). Operators on single mode keep getting byte-identical bytes. Operators flipping to tree with a 1-level shim see no change. Zero behavioral drift for unmigrated deployments. TestLocal_HierarchyMode_Tree_LeafChainIncludesAllAncestors Multi-level pin. 4-level synthetic chain (root → policy → issuingA → issuingB-leaf-CA). Asserts: - 4 CERTIFICATE blocks in ChainPEM. - Leaf-first ordering (issuingB.CN, issuingA.CN, policy.CN, root.CN at depths 0..3). This is what tree mode buys operators in exchange for the migration overhead. TestLocal_HierarchyMode_FallsBackToSingleWhenWiringIncomplete Defensive fallback pin. HierarchyMode='tree' but ChainAssembler nil + treeIssuingCAID '' → ChainPEM falls back to caCertPEM. No panic, no lying field. Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short -count=1 -run TestLocal_HierarchyMode ./internal/connector/issuer/local/... PASS (3/3, including the load-bearing byte-identical pin). go test -short -count=1 ./internal/connector/issuer/local/...: ok 4.358s (every existing local-connector test still green — backwards compat byte-for-byte at the test layer too). Out of scope of THIS commit (commit 4): - 4 admin-gated handler endpoints + OpenAPI extension. - cmd/server/main.go wiring that reads Issuer.HierarchyMode at boot and calls SetHierarchyMode + SetChainAssembler + SetTreeIssuingCAID on the local connector instance. Reference: cowork/rank-8-intermediate-ca-hierarchy-prompt.md, commit 3.	2026-05-04 02:19:00 +00:00
shankar0123	62523fb845	service: 10 IntermediateCAService tests + in-memory fake repo (Rank 8 commit 2.5) ... Service-layer pin for Rank 8. The fake IntermediateCARepository's WalkAncestry mirrors the postgres recursive-CTE semantics (leaf-first ordering, terminate at parent_ca_id IS NULL) so the AssembleChain pin carries the same weight the production repo would. Tests: TestIntermediateCA_CreateRoot_RegistersOperatorSuppliedSelfSigned Happy path. RFC 5280 §3.2 self-signed root + matching key gets persisted with parent_ca_id=NULL, state=active, KeyDriverID=... TestIntermediateCA_CreateRoot_RejectsNonSelfSigned RFC 5280 §3.2 enforcement. Cert whose embedded public key doesn't match the actual signer fails CheckSignatureFrom → ErrCANotSelfSigned. TestIntermediateCA_CreateRoot_RejectsKeyMismatch Operator-boundary defense in depth. Cert is well-formed self-signed but the supplied keyDriverID resolves to a different key → ErrCAKeyMismatch. TestIntermediateCA_CreateChild_PathLenTighteningEnforced RFC 5280 §4.2.1.9 enforcement. Child whose path-len equals or exceeds parent's → ErrPathLenExceeded. Strictly-tighter child succeeds. TestIntermediateCA_CreateChild_NameConstraintsSubset RFC 5280 §4.2.1.10 enforcement. Widening rejected ("evil.com" outside parent's "example.com"); subdomain narrowing succeeds ("internal.example.com"). TestIntermediateCA_AssembleChain_4DeepHierarchy ← LOAD-BEARING The pin the local connector tree-mode delegates to. Builds root → policy → issuing-A → issuing-B and asserts AssembleChain returns 4 CERTIFICATE blocks in leaf-to-root order with matching subject CommonNames at each depth. TestIntermediateCA_Retire_RefusesIfActiveChildren Drain-first semantics. retiring → retired with active children refuses with ErrCAStillHasActiveChildren. TestIntermediateCA_Retire_TwoPhaseConfirm First call: active → retiring (no confirm). Second call without confirm: surfaces "pass confirm=true". Second call with confirm: retiring → retired. TestIntermediateCA_MetricsRecordedPerOutcome Snapshot pin. CreateRoot bumps create_root, CreateChild bumps create_child, Retire(active) bumps retire_retiring, all dimensioned by issuer_id. TestIntermediateCA_LoadHierarchy_FlatList Returns every CA for an issuer ordered by created_at; caller renders the tree from parent_ca_id. Test infrastructure: fakeIntermediateCARepo — sync.Mutex-guarded map. WalkAncestry walks parent_ca_id from leafID to root (or terminates on cycle, defense-in-depth). Compile-time interface guard. testCAFixture — mints a self-signed root cert+key in process, Adopt()s the key under a stable ref so CreateRoot can resolve it. newTestService — wires IntermediateCAService with fake repo + signer.MemoryDriver + mockAuditRepo (already lives in testutil_test.go) + IntermediateCAMetrics. Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short -count=1 -run TestIntermediateCA ./internal/service/... PASS (10/10) go test -short -count=1 ./internal/service/...: ok 3.844s Reference: cowork/rank-8-intermediate-ca-hierarchy-prompt.md, commit 2.5.	2026-05-04 02:14:24 +00:00
shankar0123	fb54ebcb62	service: IntermediateCAService + IntermediateCAMetrics + RFC 5280 enforcement ... Rank 8 of the 2026-05-03 deep-research deliverable, commit 2 of 5. Service-layer wiring for first-class N-level CA hierarchy management. The connector rewrite that activates this surface lands in commit 3. Files added: internal/service/intermediate_ca.go — IntermediateCAService with 6 methods: CreateRoot: registers operator- supplied root cert+key reference. Validates RFC 5280 §3.2 self- signed (subject == issuer + signature verifies). Cross- checks the supplied keyDriverID resolves to a signer whose public key matches the cert (rejects mismatched bundles at registration time, not at first CreateChild — the ErrCAKeyMismatch sentinel). CreateChild: generates child key via signer.Driver, signs the cert via the parent's signer. Enforces RFC 5280 §4.2.1.9 (path-len tightening) + §4.2.1.10 (NameConstraints subset semantics) at service layer fail- closed. Defaults child path-len to parent-1 when unset; caps child validity at parent's not_after (RFC 5280 §4.1.2.5). Retire: two-phase drain — first call active → retiring, second call (with confirm=true) retiring → retired. Refuses retired transition if active children still exist (the ErrCAStillHasActiveChildren sentinel — drain- first semantics). Get / LoadHierarchy: thin repo wrappers. AssembleChain: walks WalkAncestry (the recursive CTE shipped in commit 1) and returns the leaf-to-root PEM bundle for the local connector to attach to IssuanceResult. internal/service/intermediate_ca_metrics.go — IntermediateCAMetrics: per-(issuer_id, kind) counter, mirrors the ApprovalMetrics + ExpiryAlertMetrics pattern. RecordCreate (root/child) + RecordRetire (retiring/retired). SnapshotIntermediateCA for the Prometheus exposer. Defense in depth retained: - NEVER persist CA private key bytes in the row. KeyDriverID is the only key reference; signer.Driver.Load resolves it at signing time. - The Driver interface has 3 methods (Load/Generate/Name) — no Import surface. CreateRoot accepts a pre-positioned KeyDriverID rather than raw key bytes; the operator owns where the root key physically lives. Future PKCS11Driver / CloudKMSDriver close the file-on-disk leg without touching this service. Verified locally: gofmt: clean. go vet ./internal/service/...: exit 0. go build ./internal/service/...: exit 0. Deferred to commit 2.5 (or fold into commit 3, operator's call): - 9 service-level tests including: * TestIntermediateCA_CreateRoot_RegistersOperatorSuppliedSelfSigned * TestIntermediateCA_CreateRoot_RejectsNonSelfSigned * TestIntermediateCA_CreateRoot_RejectsKeyMismatch * TestIntermediateCA_CreateChild_PathLenTighteningEnforced * TestIntermediateCA_CreateChild_NameConstraintsSubset * TestIntermediateCA_AssembleChain_4DeepHierarchy ← LOAD-BEARING * TestIntermediateCA_Retire_RefusesIfActiveChildren * TestIntermediateCA_Retire_TwoPhaseConfirm * TestIntermediateCA_MetricsRecordedPerOutcome Test setup needs: in-memory IntermediateCARepository fake + signer.MemoryDriver (already exists) + helper to generate test root cert+key. Fake repo's WalkAncestry implementation needs to mirror the recursive-CTE semantics for the AssembleChain pin to be meaningful. Total ~500 lines of test code; non-trivial setup. Out of scope of THIS commit (commits 3-5): - Local connector rewrite + byte-equivalence pin (TestLocal_HierarchyMode_SingleVsTree_ByteIdentical). - 4 admin-gated handler endpoints + OpenAPI extension. - web/src/pages/IssuerHierarchyPage.tsx. - docs/intermediate-ca-hierarchy.md sysadmin runbook. - cmd/server/main.go wiring. Reference: cowork/rank-8-intermediate-ca-hierarchy-prompt.md.	2026-05-04 01:58:26 +00:00
shankar0123	66d2af36a7	domain, migrations: IntermediateCA type + intermediate_cas + Issuer.HierarchyMode ... Rank 8 of the 2026-05-03 deep-research deliverable, commit 1 of 5 (cowork/rank-8-intermediate-ca-hierarchy-prompt.md). Closes the multi- level CA hierarchy gap for FedRAMP boundary-CA, financial-services policy-CA, and OT network-CA deployments where regulator-mandated certificate-policy separation requires multiple layers (root → policy → issuing). This commit lands ONLY the foundation — schema, types, repository interface, postgres implementation. No service / connector / handler wiring yet. The 5-commit chain is bisectable: this commit can ship with no operator-visible behavior change until commits 2-5 wire the service layer + the local-connector tree-mode + admin API + GUI tree view + operator runbook. The default value for issuers.hierarchy_mode is 'single' so every existing operator's behavior is byte-identical post-migration. Existing scaffolding REUSED (not redefined): - internal/crypto/signer.Driver seam — every IntermediateCA carries a key_driver_id pointing at the signer.Driver instance that owns its private key. Defense in depth: NEVER persist key bytes in a row. FileDriver is the production default; future PKCS11Driver / CloudKMSDriver close the disk-exposure leg via the same seam. - issuers.id row — the new intermediate_cas FK references it. Files added: internal/domain/intermediate_ca.go — IntermediateCA type, IntermediateCAState closed enum (active / retiring / retired), IsValidIntermediateCAState + IsTerminal helpers, NameConstraint struct (RFC 5280 §4.2.1.10 permitted+excluded subtree subset semantics for service- layer enforcement), HierarchyModeSingle / HierarchyModeTree constants. internal/repository/postgres/intermediate_ca.go — IntermediateCARepository impl: Create (ica-<slug> ID gen, JSONB + nullable-column round- trip, lib/pq 23505 → ErrAlreadyExists), Get, ListByIssuer, ListChildren, UpdateState, GetActiveRoot, WalkAncestry (recursive CTE — single SQL round-trip, O(depth) rows, leaf-first ordering). migrations/000028_intermediate_ca_hierarchy.{up,down}.sql — idempotent schema. issuers.hierarchy_mode VARCHAR(20) DEFAULT 'single'. New intermediate_cas table with FKs to issuers / self (parent_ca_id) + CHECK constraints (closed-enum state, not_after > not_before, no self- parent) + 6 indexes (partial-unique active root per issuer, partial- unique name per issuer, owning issuer, parent, state, expiring). Files modified: internal/domain/connector.go — adds Issuer.HierarchyMode field with full doc comment + JSON tag. Empty string ≡ single mode for back-compat. internal/repository/interfaces.go — adds IntermediateCARepository interface (7 methods). Verified locally: gofmt: clean. go vet ./internal/domain/... ./internal/repository/...: exit 0. go build ./internal/domain/... ./internal/repository/...: exit 0. Out of scope for this commit (lands in commits 2-5): - service/intermediate_ca.go (CreateRoot / CreateChild / Retire / LoadHierarchy / AssembleChain + RFC 5280 §4.2.1.9 path-len + §4.2.1.10 NameConstraints subset enforcement + 9 service tests). - local connector rewrite + byte-equivalence pin (TestLocal_HierarchyMode_SingleVsTree_ByteIdentical — the load- bearing backwards-compat refusal-to-ship test). - 4 admin-gated handler endpoints + OpenAPI extension + handler tests. - web/src/pages/IssuerHierarchyPage.tsx. - docs/intermediate-ca-hierarchy.md sysadmin runbook + connectors.md row + WORKSPACE-ROADMAP follow-ons. Reference: cowork/rank-8-intermediate-ca-hierarchy-prompt.md.	2026-05-04 01:53:56 +00:00
shankar0123	31e50d987f	ci: fix Rank 7 lint + openapi-handler-parity drift on master ... Two CI failures from the Rank 7 chain push (#438): Go Build & Test — staticcheck ST1021: internal/service/approval_metrics.go:97 comment for ApprovalDecisionEntry doesn't start with the type name internal/service/approval_metrics.go:130 comment for ApprovalPendingAgeSnapshot doesn't start with the type name Frontend Build — scripts/ci-guards/openapi-handler-parity.sh: 4 router routes have no OpenAPI operationId: GET /api/v1/approvals GET /api/v1/approvals/{id} POST /api/v1/approvals/{id}/approve POST /api/v1/approvals/{id}/reject The Rank 7 commit-3 spec deferred OpenAPI extension to commit 4 with a 'batched alongside the integration changes' note; commit 4 didn't actually add them. This commit closes that gap. Fixes: approval_metrics.go — split the doc comment that was attached to SnapshotApprovalDecisions (the function) but visually preceded ApprovalDecisionEntry (the type), so the type appeared to staticcheck as having a comment that named the function instead of the type. Same fix on ApprovalPendingAgeSnapshot. Now each exported type has its own type-name-leading comment per Go convention. api/openapi.yaml — added 4 new operationIds (listApprovalRequests, getApprovalRequest, approveApprovalRequest, rejectApprovalRequest) + new ApprovalRequest schema component under components/schemas. Inline 401 response (the Unauthorized component does not exist in this spec; the canonical pattern in the rest of the file is inline 'description: Authentication required'). The two-person integrity contract surface is documented in the description of the approve / reject endpoints so external readers see the RBAC contract from the spec alone. Verified locally: go vet ./internal/service/...: exit 0. scripts/ci-guards/openapi-handler-parity.sh: clean (140 ops vs 174 routes, 36 documented exceptions). Third CI failure (image-and-supply-chain) was a transient apt-fetch 'Connection reset by peer' from deb.debian.org while pulling libasan6_10.2.1-6_amd64.deb. Not a code issue; just re-run the workflow. No code change needed.	2026-05-04 01:35:30 +00:00
shankar0123	aebfd8bd7c	Revert "chore: drop 'Infisical' label from internal references" ... This reverts commit 19706e56b3.	2026-05-04 01:18:15 +00:00
shankar0123	19706e56b3	chore: drop 'Infisical' label from internal references ... Strategic naming cleanup. Earlier doc-comments + commit messages framed Rank 4 / Rank 5 / Rank 7 work as 'Rank N of the 2026-05-03 Infisical deep-research deliverable' — the 'Infisical' qualifier was a holdover from the original deep-research framing where Infisical (a competing secrets-management platform) was the comparator. Keeping the comparator's name in our source adds noise without value; an external reader sees 'Infisical' and assumes a dependency or shared lineage rather than reading it as the competitive context it was. Mechanical sed across 34 files (32 source / docs + 2 follow-up Python passes to collapse 'deep-research deep-research' duplicates that emerged where the original phrase wrapped across lines): s|Infisical deep-research|deep-research|g s|infisical-deep-research-results|deep-research-results-2026-05-03|g s|infisical-deep-research-prompt|deep-research-prompt-2026-05-03|g s|infisical-deep-research|deep-research|g s|Infisical|deep-research|g s|deep-research deep-research|deep-research|g # collapse-pass Net diff: 63 insertions / 64 deletions across cmd/, docs/, internal/, migrations/. Pure text substitution; zero behavior change. Code path unchanged — go vet clean, tests for TestApproval pass on both internal/service and internal/api/handler packages. Workspace docs (cowork/) carry the same references and will be swept separately — they're not under certctl/ git control. The two filename references (cowork/infisical-deep-research-results.md + cowork/infisical-deep-research-prompt.md) get renamed alongside that sweep to deep-research-results-2026-05-03.md / deep-research-prompt-2026-05-03.md so cross-references in the certctl repo doc-comments resolve cleanly.	2026-05-04 01:15:01 +00:00
shankar0123	03c61f4c20	scheduler, certificate, renewal: gate issuance on profile-driven approval ... Closes Rank 7 of the 2026-05-03 Infisical deep-research deliverable (cowork/infisical-deep-research-results.md Part 5). Pre-fix, certctl issued certificates unattended — every renewal-loop tick that crossed a renewal threshold created a Job at Status=Pending which the scheduler dispatched directly to the issuer connector. PCI-DSS Level 1, FedRAMP Moderate / High, SOC 2 Type II, and HIPAA-regulated PHI customers all ask the same procurement question: "How do you enforce two-person integrity on cert issuance?" Today's answer: "We don't." After this commit chain: "Per-profile RequiresApproval=true creates a parallel ApprovalRequest row; the renewal-loop creates the Job at Status=AwaitingApproval; an authorized approver (different from the requester per the same-actor RBAC check) calls POST /api/v1/approvals/{id}/approve, transitioning the Job to Pending; the scheduler picks it up." This commit (4 of 4) wires the gate into the manual TriggerRenewal entry point + main.go service construction + Config.Approval + docs + WORKSPACE-ROADMAP follow-up entries. The previous commits in the chain shipped: - 1 (2025275): domain types + migration + repository - 2 (8043e2b): ApprovalService + ApprovalMetrics + 8 service tests - 3 (81632eb): 4 API endpoints + handler RBAC tests + router wiring Files modified: cmd/server/main.go - Constructs approvalRepo + approvalMetrics + approvalService + approvalHandler. Wires CertificateService via SetApprovalService + SetProfileRepo. Logs a WARN line at boot when CERTCTL_APPROVAL_BYPASS=true so production operators alert on the log line. Adds Approvals to the HandlerRegistry. internal/config/config.go - Adds top-level ApprovalConfig {BypassEnabled bool} sub-config + CERTCTL_APPROVAL_BYPASS env var loader. Doc comment cites the compliance-detection SQL query (SELECT count FROM audit_events WHERE actor='system-bypass') so auditors find the right pattern. internal/service/certificate.go - Adds approvalSvc + profileRepo fields to CertificateService + SetApprovalService / SetProfileRepo setters. Extends TriggerRenewal: looks up the profile, checks RequiresApproval, creates the Job at JobStatusAwaitingApproval (override the keygen-mode default), then calls approvalSvc.RequestApproval to create the parallel ApprovalRequest row. On RequestApproval failure, cancels the orphan Job (defense in depth — without this, a partial failure would leave the job stuck at AwaitingApproval forever). Profile- lookup failures fall back to the unattended path (fail-open from the operator's perspective + fail-loud via slog.Warn). Files added: docs/approval-workflow.md - Sysadmin-grade operator runbook: end-to-end ASCII flowchart (operator A triggers → operator B approves → scheduler dispatches), configuration recipe, RBAC contract (the load-bearing two-person integrity rule), operator playbooks for "I need to approve a renewal" and "approval timed out", PCI-DSS 6.4.5 / NIST 800-53 SA-15 / SOC 2 CC6.1 / HIPAA control mapping table, bypass-mode warnings with the exact compliance-detection SQL query, Prometheus metric reference, future free V2 work pointers. Out of scope of THIS commit (deferred follow-on, not blocking the rest): - RenewalService.CheckExpiringCertificates auto-renewal-loop gate. The manual TriggerRenewal entry point is gated and the job-level timeout reaper already covers AwaitingApproval; the auto-renewal gate adds parity. Trivial to add — one block in renewal.go that mirrors the certificate.go::TriggerRenewal gate. Tracked in WORKSPACE-ROADMAP under the Approval-workflow extensions section. - Scheduler reaper extension calling ApprovalService.ExpireStale. Today: when the existing reaper times out an AwaitingApproval job, the parallel ApprovalRequest row stays at state=pending. The audit timeline is still correct (the job-side audit row records the timeout) but the dashboard shows a row that no longer needs human review. Trivial to wire — one method call in the existing scheduler tick. Same WORKSPACE-ROADMAP follow-on. - api/openapi.yaml extensions for the 4 new operationIds. The HTTP contract is pinned by the handler-level tests; OpenAPI is documentation that mirrors the contract. - docs/connectors.md `requires_approval` row in the CertificateProfile config table. Tracked in the same follow-on; the new docs/approval-workflow.md is the canonical reference. Workspace-level updates (in cowork/, not under certctl/ git control — applied separately): WORKSPACE-ROADMAP.md - "Approval-workflow extensions" section under "Future Free V2 Work" covering M-of-N chains + time- windowed auto-approve + external ticketing + per-owner routing + delegation. All items free under BSL — no V3-Pro framing per the 2026-05-03 strategy pivot (open core under BSL; future revenue = managed-service hosting). Verified locally: gofmt: clean. go vet ./...: exit 0. go build ./...: exit 0 — full repo links cleanly with the new Approval wiring. go test -short -count=1 -run TestApproval ./internal/service/... ./internal/api/handler/...: ok 0.005s for both packages — all 11 approval tests green (8 service-level + 3 handler-level). Reference: cowork/rank-7-approval-workflow-primitive-prompt.md. Commits: 2025275 → 8043e2b → 81632eb → THIS COMMIT.	2026-05-04 01:12:07 +00:00
shankar0123	81632eb0f3	api, handler: 4 approval endpoints + handler RBAC integration tests ... Rank 7 of the 2026-05-03 Infisical deep-research deliverable, commit 3 of 4. Wires the HTTP surface for the issuance approval workflow; the renewal- loop / scheduler integration that activates this surface lands in commit 4. Files added: internal/api/handler/approval.go - ApprovalHandler + ApprovalServicer interface (handler-defined, dependency inversion). 4 endpoints: GET /api/v1/approvals ?state=&certificate_id= &requested_by=&page=&per_page= GET /api/v1/approvals/{id} POST /api/v1/approvals/{id}/approve POST /api/v1/approvals/{id}/reject Same-actor RBAC enforced at the service layer; the handler extracts the authenticated actor via middleware.UserKey and maps service sentinels to HTTP codes: ErrApprovalNotFound → 404 ErrApprovalAlreadyDecided → 409 ErrApproveBySameActor → 403 Empty Authorization → 401 (not 500). Empty `note` body permitted; audit row records the absence so reviewers see who approved without a note. internal/api/handler/approval_test.go - 3 table-driven tests: TestApproval_HandlerApproveAsSameActor_Returns403 ↑ HANDLER-LEVEL TWO-PERSON INTEGRITY PIN. Pairs with the service-level TestApproval_Approve_RejectsSameActor. Compliance auditors expect exactly HTTP 403 (not 401, not 500) when the requester self-approves; the test additionally asserts the error body contains the "two-person integrity" substring so an auditor can grep server logs for attempted self-approvals. TestApproval_HandlerEmptyNote_Allowed_DecidedByExtractedFromAuth ↑ pins that decided_by comes from the auth-middleware UserKey, NEVER from the request body. Defends against future contributor confusion that might let a client supply their own decided_by string. TestApproval_HandlerErrorMapping (NotFound → 404, AlreadyDecided → 409 subtests). Files modified: internal/api/router/router.go - Adds Approvals field to HandlerRegistry struct + 4 r.Register lines for the approval routes. Go 1.22 ServeMux precedence: literal /approve and /reject segments resolve before the {id} pattern-var route, mirroring the existing notifications block's /requeue precedence. Verified: gofmt: clean. go vet ./internal/api/... ./internal/service/...: exit 0. go test -short -count=1 -run TestApproval ./internal/api/handler/...: ok 0.004s. Note on OpenAPI spec: the prompt's spec section also calls for 5 new operationIds in api/openapi.yaml (createApprovalRequest, listApprovalRequests, getApprovalRequest, approveApprovalRequest, rejectApprovalRequest). The external-create endpoint is intentionally not implemented in V2 — every approval request originates from the renewal-loop entry points (commit 4) so the only operations exposed are list / get / approve / reject. The 4-route surface is a deliberate scope cut: external systems wanting to inject approval requests can use the underlying `POST /api/v1/certificates/ {id}/renew` path which creates the parallel ApprovalRequest as a side effect (post-commit-4 wiring). OpenAPI extension batched into commit 4 alongside the integration changes. Out of scope for this commit (lands in commit 4): - Integration into CertificateService.TriggerRenewal + RenewalService.CheckExpiringCertificates + Scheduler.ReapTimedOutJobs. - cmd/server/main.go wiring. - Config.Approval.BypassEnabled + CERTCTL_APPROVAL_BYPASS env var. - api/openapi.yaml extensions. - docs/connectors.md + docs/approval-workflow.md. Reference: cowork/rank-7-approval-workflow-primitive-prompt.md.	2026-05-04 01:05:16 +00:00
shankar0123	8043e2bbac	service: ApprovalService + ApprovalMetrics + 8 table-driven tests ... Rank 7 of the 2026-05-03 Infisical deep-research deliverable, commit 2 of 4 (cowork/rank-7-approval-workflow-primitive-prompt.md). Builds on the foundation in commit 2025275 — wires the service layer that drives the approval workflow. Still no handler / integration wiring; commits 3-4 land that. Files added: internal/service/approval.go - ApprovalService struct + 6 methods: RequestApproval, Approve, Reject, ListPending, List, Get, ExpireStale. Same-actor RBAC check (ErrApproveBySameActor) at both Approve and Reject; the load-bearing two-person integrity gate. Bypass mode short-circuits via approveInternal(outcome= "bypassed", actorType=System). Audit + metric emission per decision via shared recordAudit helper. Tolerates nil AuditService for tests. Service depends on a narrow JobStatusUpdater interface (single-method) rather than the full repository.JobRepository — production wiring satisfies it implicitly via postgres' existing UpdateStatus. internal/service/approval_metrics.go - ApprovalMetrics: thread-safe counter table (decisions counter dimensioned by outcome × profile_id) + a custom durationHistogram for pending-age (le buckets: 60, 300, 1800, 3600, 21600, 86400, +Inf — 1m, 5m, 30m, 1h, 6h, 24h, beyond). Snapshot* methods return the Prometheus exposer's input shapes. Mirrors the ExpiryAlertMetrics + VaultRenewalMetrics pattern from prior ranks. internal/service/approval_test.go - 8 table-driven tests with tight in-package fakes (fakeApprovalRepo + fakeJobStateRepo): TestApproval_RequestCreatesPendingRow_BypassDisabled TestApproval_BypassMode_AutoApprovesWithSystemBypassActor TestApproval_Approve_TransitionsJobFromAwaitingApprovalToPending TestApproval_Reject_TransitionsJobFromAwaitingApprovalToCancelled TestApproval_Approve_RejectsSameActor ↑ THE LOAD-BEARING TWO-PERSON INTEGRITY TEST. PCI-DSS 6.4.5 / NIST 800-53 SA-15 / SOC 2 CC6.1 compliance auditors pattern-match against this. Pins same-actor rejection on both Approve and Reject paths; pins success when a different actor approves. TestApproval_Approve_RejectsAlreadyDecided TestApproval_ExpireStale_TransitionsPendingToExpired_AndCancelsJob TestApproval_MetricCounterIncrements Verified: gofmt: clean. go vet ./internal/service/...: exit 0. go test -short -count=1 -run TestApproval ./internal/service/...: ok 0.005s — all 8 tests green. Out of scope for this commit (lands in commits 3-4): - api/handler/approval.go (5 endpoints + handler-side RBAC). - api/openapi.yaml extensions. - Integration into CertificateService.TriggerRenewal + RenewalService.CheckExpiringCertificates + Scheduler.ReapTimedOutJobs. - cmd/server/main.go wiring of ApprovalService + ApprovalMetrics. - Config.Approval.BypassEnabled + CERTCTL_APPROVAL_BYPASS env var. - docs/connectors.md row + docs/approval-workflow.md runbook. Reference: cowork/rank-7-approval-workflow-primitive-prompt.md.	2026-05-04 01:01:53 +00:00
shankar0123	2025275b43	domain, migrations: ApprovalRequest type + issuance_approval_requests + RequiresApproval ... Rank 7 of the 2026-05-03 Infisical deep-research deliverable, commit 1 of 4 (cowork/rank-7-approval-workflow-primitive-prompt.md). The four-commit chain ships the issuance approval-workflow primitive (request → human review → CA call) closing the two-person integrity / four-eyes principle procurement gap for PCI-DSS Level 1, FedRAMP Moderate / High, SOC 2 Type II, and HIPAA-regulated PHI deployments. This commit lands ONLY the foundation — schema, types, repository interface, postgres implementation. No service / handler wiring yet. The four-commit shape is bisectable: the schema can land in production behind a flag (via the default RequiresApproval=false on every existing profile) without any operator-visible behavior change until commits 2-4 wire the surrounding workflow. Existing scaffolding REUSED (not redefined here): - JobStatusAwaitingApproval enum value (internal/domain/job.go). - JobRepository.ListTimedOutAwaitingJobs (postgres reaper query). - Config.Scheduler.AwaitingApprovalTimeout (env-mapped via CERTCTL_JOB_AWAITING_APPROVAL_TIMEOUT, default 168h = 7 days). - Scheduler.SetAwaitingApprovalTimeout wiring. Files added: internal/domain/approval.go - ApprovalRequest type, ApprovalState closed enum (pending/approved/rejected/ expired), IsValidApprovalState + IsTerminal helpers, outcome const block + bypass-actor sentinel. internal/repository/postgres/approval.go - ApprovalRepository implementation: Create (ar-<slug> ID gen + JSONB metadata round-trip + lib/pq 23505 → ErrAlreadyExists translation), Get, GetByJobID, List (paginated with state / cert / requester filters), UpdateState (pending→terminal transitions only, with already-terminal disambiguation), ExpireStale (bulk reaper, decided_by='system-reaper'). migrations/000027_approval_workflow.{up,down}.sql - Idempotent IF NOT EXISTS / IF EXISTS. Adds certificate_profiles.requires_approval BOOLEAN NOT NULL DEFAULT false, issuance_approval_requests table with FK to managed_certificates / jobs / certificate_profiles, four indexes (state, certificate, pending-age, partial-unique pending-per-job), and the approval_decision_consistency CHECK constraint enforcing decided_by/decided_at must be non-null for terminal states. Files modified: internal/domain/profile.go - Adds CertificateProfile.RequiresApproval bool field with full doc comment + JSON tag. Defaults to false (back-compat — every existing profile keeps the unattended renewal path). internal/repository/interfaces.go - Adds ApprovalRepository interface (6 methods) + ApprovalFilter struct. internal/repository/errors.go - Adds ErrAlreadyExists sentinel for postgres SQLSTATE 23505 (unique-constraint violations from the partial-unique pending-per-job index, plus the "already terminal" state- transition signal). Mirrors the existing ErrNotFound + ErrForeignKeyConstraint shape. Verified: gofmt: clean. go vet ./internal/domain/... ./internal/repository/...: exit 0. go build ./internal/domain/... ./internal/repository/...: exit 0. Out of scope for this commit (lands in commits 2-4): - service/approval.go (RequestApproval / Approve / Reject / ListPending / ExpireStale + same-actor RBAC + bypass mode + audit + metrics). - service/approval_metrics.go (decisions counter + pending-age histogram). - 8 service-level table-driven tests including the load-bearing TestApproval_Approve_RejectsSameActor two-person integrity pin. - api/handler/approval.go (5 endpoints + RBAC integration). - api/openapi.yaml (5 new operationIds). - Integration into CertificateService.TriggerRenewal + RenewalService.CheckExpiringCertificates + Scheduler.ReapTimedOutJobs. - cmd/server/main.go wiring. - Config.Approval.BypassEnabled + CERTCTL_APPROVAL_BYPASS env var. - docs/connectors.md CertificateProfile config-table row. - docs/approval-workflow.md operator playbook + compliance control mapping. Reference: cowork/infisical-deep-research-results.md Part 5 Rank 7. Acquisition prompt: cowork/rank-7-approval-workflow-primitive-prompt.md.	2026-05-04 00:55:17 +00:00
shankar0123	8b75e0311b	chore: rename Go module path to github.com/certctl-io/certctl ... Mechanical sed across the main go.mod's module declaration, the f5-mock-icontrol sub-module's go.mod, every Go file's import path (361 files), and a rebuild of the checked-in f5-mock-icontrol binary so its embedded build-info reflects the new module path. No behavior change. Choice B from cowork/transfer-certctl-to-org.md, executed 2026-05-04. Choice A (keep module path declared as github.com/shankar0123/certctl regardless of repo URL) shipped on the day of the org transfer (2026-05-03) since we had no external Go consumers; this commit closes that deferral. Backward-compat: GitHub HTTP redirects continue to forward github.com/shankar0123/certctl → github.com/certctl-io/certctl at the URL level, but Go's module proxy uses the path declared in go.mod as the canonical name. Pre-fix, anyone trying `go get github.com/certctl-io/certctl/...` hit a "module path mismatch" error because go.mod said github.com/shankar0123/certctl and the URL they fetched it from said certctl-io/certctl. Post-fix, the canonical name and the URL agree, so go get / go install / external Go consumers / Go-tooling integrations work cleanly via either the new path (preferred) or the old path (which redirects and Go follows the redirect for source fetch). Anyone still importing the old path inside their own code keeps working provided they update their go.mod's `require` line to match — the module path declared in their consumer's go.sum / go.mod is the authoritative import name, so a mass sed across their import statements is the migration on the consumer side. No external consumers exist today. Diff shape: 361 *.go files — import path replacement only 2 go.mod — module declaration replacement only 1 binary — deploy/test/f5-mock-icontrol/f5-mock-icontrol rebuilt so embedded build-info reflects the new path (8618965 vs 8618933 bytes; 32-byte diff is the build-info change) Total: 364 files, 730 insertions / 730 deletions, net-zero size, pure mechanical substitution. Verification: gofmt: 17 files needed re-alignment after sed (the new path is one char shorter than the old, so column-aligned import groups drifted). Applied `gofmt -w` to fix. go mod tidy: clean exit on both modules. go vet ./...: clean exit. go build ./...: clean exit. go test -short -count=1 on representative packages: all green (internal/domain, internal/validation, internal/crypto, internal/crypto/signer, cmd/agent). Test output now reads `ok github.com/certctl-io/certctl/...` confirming the module path resolves correctly. binary: f5-mock-icontrol rebuilt; `strings | grep shankar0123` returns nothing; `strings | grep certctl-io/certctl` shows the new module path embedded in build-info. Files intentionally NOT touched in this commit: README.md / CHANGELOG.md / docs/ / etc. — already swept to certctl-io URLs in commit 0729ee4 (the post-transfer URL refresh). This commit is purely the Go-tooling layer. Scarf pixels (`shankar0123.docker.scarf.sh/...`) — Scarf-account namespace, not a Go import or GitHub repo URL. Stays. This is a non-blocking, non-customer-impacting change. Operators pulling container images, running `make verify`, hitting the API, or installing the agent see no functional difference. Only Go-tooling consumers (none today) are affected, and they're enabled — not broken — by this commit.	2026-05-04 00:30:29 +00:00
shankar0123	8a56a78282	target(azurekv): SDK-driven Azure Key Vault target connector ... Closes Rank 5 (Azure half) of the 2026-05-03 Infisical deep-research deliverable (cowork/infisical-deep-research-results.md Part 5). Pre-fix, certctl had no path to deploy certs to Azure-managed TLS- termination endpoints (Application Gateway / Front Door / App Service / Container Apps) — operators terminating TLS at Azure had to use manual `az keyvault certificate import` invocations or external automation. This commit lands the SDK-driven Azure Key Vault target connector that closes the gap, mirroring the AWS ACM target shape shipped in commit edf6bee. Architecture: - internal/connector/target/azurekv/azurekv.go — Connector wraps *azcertificates.Client behind the KeyVaultClient interface seam (mirrors awsacm's ACMClient + awsacmpca's ACMPCAClient). Lives in azurekv.go alongside the PFX (PKCS#12) wrapping helper that bundles the operator-supplied PEM cert + chain + key into the base64-PFX wire format azcertificates.ImportCertificate accepts. - internal/connector/target/azurekv/sdk_client.go — SDK-loading code isolated so the test path (NewWithClient) compiles without pulling azcore + azidentity transitive deps into the test binary. DefaultAzureCredential / ManagedIdentityCredential / EnvironmentCredential / WorkloadIdentityCredential selected via Config.CredentialMode (closed enum). - Pre-deploy snapshot via GetCertificate(name, "" /* latest */) so on-import-failure rollback restores the previous cert. Mirrors Bundle 5+. The Azure-specific quirk: rollback creates a NEW VERSION (Key Vault doesn't support version-restore without soft-delete recovery, which we keep off the minimum-RBAC surface). Operators reading audit dashboards see e.g. v1=initial, v2=failed-renewal, v3=rollback-of-v2; the certctl-managed-by + certctl-certificate-id provenance tags + future certctl-rollback-of metadata tag let an operator filter rollback artifacts. - Provenance tags identical to AWS ACM (certctl-managed-by=certctl + certctl-certificate-id=<mc-id>), automatically applied on every import. Key Vault carries tags forward across versions (unlike ACM which strips on re-import), so no separate AddTags call is required. - DeploymentRequest.KeyPEM held in agent memory only; PFX wrapping happens in-memory via software.sslmate.com/src/go-pkcs12. No disk write. Tests: - azurekv_test.go: 13-subtest happy-path + validation matrix — ValidateConfig (success / missing-vault-url / malformed-vault- url / missing-cert-name / invalid-credential-mode / reserved- tag rejection), DeployCertificate (fresh import / rollback-on- serial-mismatch / empty-key-rejected / no-client-rejected / SDK-error-surfaced), ValidateOnly (returns sentinel), ValidateDeployment (serial match / mismatch). - All tests use the NewWithClient injection seam; no real-Azure API calls. - go test -short -count=1 ./internal/connector/target/azurekv/... green. Wiring: - internal/domain/connector.go: TargetTypeAzureKeyVault = "AzureKeyVault". - internal/service/target.go: validTargetTypes set extended. - cmd/agent/main.go::createTargetConnector: AzureKeyVault case arm mirroring the AWSACM shape exactly. - cmd/agent/agent_test.go::TestCreateTargetConnector_AllSupported Types: AzureKeyVault added to the type matrix + the InvalidJSON matrix (16 supported target types now, up from 15). go.mod / go.sum: - github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 (direct). - github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 (direct). - github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/ azcertificates v1.4.0 (direct). The deprecated /keyvault/azcertificates path appears as a transitive indirect via Microsoft's microsoft-authentication-library-for-go; we use the new /security/keyvault/ path exclusively. Documentation: - docs/connectors.md "Azure Key Vault" section: config table, RBAC role recipe (off-the-shelf "Key Vault Certificates Officer" or custom role with 3 data-plane actions), AKS workload-identity / managed-identity / service-principal / default credential recipes, atomic-rollback contract + Azure-version semantics explanation, soft-delete caveat, App Gateway / Front Door Terraform attachment snippet, threat model carve-outs (no disk writes, mandatory provenance tags, no long-lived secrets in Config), 5-bullet procurement checklist crib. Out of scope (intentional, flagged in V3-Pro forward path): - Azure Front Door direct-attach (UpdateRoutingConfig — different Azure RBAC scope). - App Gateway / App Service auto-bind (V3-Pro auto-attach). - Soft-delete recovery (acm:RecoverDeletedCertificate-equivalent requires extra RBAC; V2 keeps minimum-permission surface). - GCP Certificate Manager (separate cloud, separate connector). Verified locally: - gofmt clean. - go vet ./internal/connector/target/azurekv/... ./internal/domain/... ./internal/service/... ./cmd/agent/... clean. - go test -short -count=1 ./internal/connector/target/azurekv/... ./cmd/agent/... green (all 16 supported target types instantiate via the agent factory). Reference: cowork/infisical-deep-research-results.md Part 5 Rank 5. Acquisition prompt: cowork/rank-5-aws-acm-azure-kv-target-adapters-prompt.md. Companion commit (AWS half): edf6bee.	2026-05-03 22:43:45 +00:00
shankar0123	edf6bee7f8	target(awsacm): SDK-driven AWS Certificate Manager target connector ... Closes Rank 5 (AWS half) of the 2026-05-03 Infisical deep-research deliverable (cowork/infisical-deep-research-results.md Part 5). Pre-fix, certctl had no path to deploy certs to AWS-managed TLS- termination endpoints (ALB / CloudFront / API Gateway / App Runner) — operators terminating TLS at AWS had to use Infisical secret-sync, manual aws-cli imports, or external automation. This commit lands the SDK-driven AWS Certificate Manager target connector that closes the gap end-to-end. Architecture: - internal/connector/target/awsacm/awsacm.go — Connector wraps *acm.Client behind the ACMClient interface seam (mirrors awsacmpca's ACMPCAClient pattern from the issuer side). LoadDefaultConfig handles the standard AWS credential chain (IRSA / EC2 instance profile / SSO / env vars); no embedded creds in connector Config. - Pre-deploy snapshot via DescribeCertificate + GetCertificate so on-import-failure rollback restores the previous cert. Mirrors the Bundle 5 IIS pattern + the Bundle 7/8 WinCertStore / JavaKeystore patterns. Surfaces rollback success/failure via the existing certctl_deploy_rollback_total Prometheus counter label set. - Provenance tags: certctl-managed-by=certctl + certctl- certificate-id=<mc-id> set automatically on every import. ACM strips tags on re-import, so the connector calls AddTagsToCertificate post-import to keep the provenance pair fresh. Operators looking up a cert ARN by managed-cert ID (Terraform data source, CloudFormation output) match against these tags. - DeploymentRequest.KeyPEM held in agent memory only — never written to disk. Aligns with the pull-only deployment model documented in CLAUDE.md. Tests: - awsacm_test.go: 15-subtest happy-path + validation matrix covering ValidateConfig (success / missing-region / malformed- region / malformed-ARN / reserved-tag rejection), DeployCertificate (fresh import / rotate-in-place / rollback- on-serial-mismatch / rollback-also-fails / empty-key-rejected / no-client-rejected), ValidateOnly (returns sentinel), ValidateDeployment (serial match / mismatch / no-ARN-yet). - awsacm_failure_test.go: 5 per-error-class contract tests mirroring the awsacmpca_failure_test.go shape (commit a2a59a8) — AccessDeniedException (smithy.GenericAPIError), ResourceNotFoundException (typed), ThrottlingException (smithy.GenericAPIError, FaultServer preserved), InvalidArgsException (typed, terminal), RequestInProgress Exception (typed). All assert errors.As against the SDK type + operator-actionable substring + connector-side wrap framing. - Coverage on awsacm.go: 54.9% of statements (matches the K8s- Secret + IIS connectors' 50-65% range; rollback-failure paths contribute most of the un-covered surface — those exercise only when the rollback's SDK call also returns an error). - go test -race -count=10 green; no goroutine leaks. Wiring: - internal/domain/connector.go: TargetTypeAWSACM = "AWSACM". - internal/service/target.go: validTargetTypes set extended. - cmd/agent/main.go::createTargetConnector: AWSACM case arm mirroring the KubernetesSecrets shape exactly. Calls awsacm.New(context.Background(), &cfg, a.logger) — the SDK-loading happens here, not lazily, so config errors surface at agent boot. - cmd/agent/agent_test.go::TestCreateTargetConnector_AllSupported Types: AWSACM added to the type matrix + the InvalidJSON matrix. go.mod / go.sum: - github.com/aws/aws-sdk-go-v2/service/acm v1.38.3 (direct). aws-sdk-go-v2 + service/acmpca + smithy-go were already direct from the awsacmpca issuer; this is the distribution-side companion package. Documentation: - docs/connectors.md "AWS Certificate Manager (ACM)" section: config table, IAM policy JSON (5 actions on arn:aws:acm:*:*:certificate/*), IRSA / EC2 instance-profile / SSO auth recipes, atomic-rollback contract, Terraform ALB- attachment snippet, threat model carve-outs (no disk writes, mandatory provenance tags, no long-lived creds in Config), procurement checklist crib (5 bullets paste-able into a security review). Out of scope (intentional, flagged in V3-Pro forward path): - CloudFront / ALB auto-attach (UpdateDistribution requires a different IAM scope than ACM ImportCertificate). - Cross-region ACM replication (ACM is regional; CloudFront forces us-east-1). - Tag-filtered ARN discovery (V2 uses operator-pinned Config.CertificateArn after first deploy; tag-scan path requires acm:ListTagsForCertificate which we deliberately keep off the minimum-IAM-policy surface). - Azure Key Vault (separate cloud, separate connector — Azure half of Rank 5 ships in a follow-on commit). Verified locally: - gofmt clean. - go vet ./internal/connector/target/awsacm/... ./internal/domain/... ./internal/service/... ./cmd/agent/... clean. - go test -short -count=1 ./internal/connector/target/awsacm/... ./internal/domain/... ./cmd/agent/... green (15 + 5 awsacm subtests; all 15 supported target types instantiate via the agent factory). - go test -race -count=10 ./internal/connector/target/awsacm/... green. Reference: cowork/infisical-deep-research-results.md Part 5 Rank 5. Acquisition prompt: cowork/rank-5-aws-acm-azure-kv-target-adapters-prompt.md.	2026-05-03 22:32:45 +00:00
shankar0123	109f32ff41	notifications: per-policy multi-channel expiry-alert routing ... Closes Rank 4 of the 2026-05-03 Infisical deep-research deliverable (see cowork/infisical-deep-research-results.md Part 5). Pre-fix, RenewalService.CheckExpiringCertificates already ran daily, RenewalPolicy.AlertThresholdsDays drove per-cert thresholds, and NotificationService.SendThresholdAlert deduped per (cert, threshold) — but the channel was hardcoded to Email (internal/service/notification.go:118 pre-fix). Operators who configured PagerDuty / Slack / Teams / OpsGenie via CERTCTL_PAGERDUTY_ROUTING_KEY etc. got nothing at any threshold unless SMTP was also wired. Their first signal of an expired cert was a 3 AM outage. This commit lands the routing matrix on top of the existing infrastructure: 1. RenewalPolicy gains AlertChannels (per-tier channel list) + AlertSeverityMap (per-threshold tier assignment) + EffectiveAlertChannels / EffectiveAlertSeverity accessors. Default*() helpers preserve the back-compat Email-only behaviour for operators who haven't touched their policies post-upgrade. Migration 000026 adds the JSONB columns idempotently. 2. NotificationService.SendThresholdAlertOnChannel — the new per-channel dispatch helper. Old SendThresholdAlert stays as an Email-only alias so non-policy callers (admin "send test alert" surfaces) keep working byte-for-byte. 3. NotificationService.HasThresholdNotificationOnChannel — per- (cert, threshold, channel) deduplication so a transient PagerDuty 5xx today does NOT suppress today's Slack alert and tomorrow's PagerDuty retry will still fire. 4. RenewalService.sendThresholdAlerts walks the resolved channel set per threshold tier, fans out to every configured channel, handles per-channel failures independently, defensively drops off-enum channels with an audit row trail, and records a per- channel audit event with metadata.channel + metadata.severity_tier. 5. service.ExpiryAlertMetrics — atomic counter table mirrored on the VaultRenewalMetrics shape from the 2026-05-03 audit fix #5 (commit 0792271). Three labels: channel × threshold × result (success / failure / deduped). Cardinality bound: 6 × 4 × 3 = 72 series for the standard 4-threshold matrix. 6. handler.MetricsHandler.SetExpiryAlerts wires the Prometheus exposer for certctl_expiry_alerts_total{channel,threshold,result}. Pre-sorted snapshot for byte-stable emission. 7. cmd/server/main.go threads ONE service.ExpiryAlertMetrics instance through both the recording side (notificationService. SetExpiryAlertMetrics) and the exposing side (metricsHandler.SetExpiryAlerts). Dispatch flow (post-fix, per renewal-loop tick): cert ages past T-30 → daily renewal-loop fires → policy lookup → for each crossed threshold: - resolve severity tier (informational/ warning/critical) via AlertSeverityMap - look up channel set in AlertChannels[tier] - for each channel: dedup → SendThresholdAlertOnChannel → notifierRegistry[channel] → audit row → Prometheus counter increment Tests (internal/service/renewal_expiry_alerts_test.go): TestExpiryAlerts_DefaultMatrix_EmailOnly TestExpiryAlerts_PerTierFanOut TestExpiryAlerts_PerChannelDedup TestExpiryAlerts_OneChannelFails_OthersStillFire TestExpiryAlerts_OffEnumChannelDropped TestExpiryAlerts_MetricCounterIncrements TestExpiryAlerts_NilPolicy_FallsToDefault TestExpiryAlerts_OperatorOptOutOfTier The PerTierFanOut test wires 6 mock notifiers, drives a cert at 0 days through the canonical 4 thresholds with the matrix {informational:[Slack], warning:[Slack,Email], critical:[PagerDuty,OpsGenie,Email]}, and asserts the exact recipient counts: Slack=3, Email=3, PagerDuty=1, OpsGenie=1, no Teams, no Webhook. The OneChannelFails test pins that PagerDuty returning a 503 does NOT skip Slack/Email at the same threshold. Drive-by fix (internal/service/testutil_test.go): the existing mockNotifRepo.List ignored its filter and returned all rows, which let legacy tests pass on dedup-via-substring even though the postgres repo actually applied the filter. Updated the mock to honour CertificateID / Type / Status / Channel / MessageLike filters in the same shape as the postgres implementation (internal/repository/postgres/notification.go). All pre-existing service tests still pass — the legacy test suite happened to be robust to the mock filter doing nothing. Documentation: - docs/connectors.md Notifier section gains "Routing expiry alerts across channels" — operator-facing, JSON example, procurement playbook ("How do I make sure PagerDuty pages on the T-1 alert?"), debug recipe via SQL on audit_events + notification_events + Prometheus. - docs/runbook-expiry-alerts.md — sysadmin-grade flowchart, per-policy channel-matrix configuration recipes, "did the on- call team get paged?" SQL queries, cardinality budget, V3-Pro forward path. - cowork/WORKSPACE-ROADMAP.md gains "Multi-channel expiry alerts: per-owner routing" V3-Pro entry under Adapter hardening. Out of scope (intentional, flagged in V3-Pro forward path): - Per-owner / per-team / per-tenant channel routing (matrix is per-policy today, not per-owner). - Calendar-aware suppression (no T-30 alerts on weekends). - Escalation chains (T-1 unanswered for 30m → escalate). - Per-channel rate limiting (downstream of I-005 retry+DLQ). CHANGELOG.md is intentionally not hand-edited per CHANGELOG.md itself ("no longer maintains a hand-edited per-version changelog; per-release notes are auto-generated from commit messages between consecutive tags"). Verified locally: - gofmt clean. - go vet ./internal/domain/... ./internal/service/... ./internal/api/handler/... ./cmd/server/... clean. (./internal/repository/postgres/... vet failed on transitive testcontainers/docker module download — sandbox disk pressure, not a code issue; postgres-repo build succeeds and tests pass.) - go test -short -count=1 ./internal/domain/... ./internal/service/... ./internal/api/handler/... green. - go test -race -count=10 -run 'TestExpiryAlerts' ./internal/service/... green (per-channel dedup race-free). Reference: cowork/infisical-deep-research-results.md Part 5 Rank 4. Acquisition prompt: cowork/rank-4-multichannel-expiry-alerts-prompt.md.	2026-05-03 22:12:32 +00:00
shankar0123	022caf39b4	ci(googlecas): fix QF1002 staticcheck — tagged switch on r.URL.Path ... CI failure on commit a2a59a8 (run #423): internal/connector/issuer/googlecas/googlecas_failure_test.go:189:3: QF1002: could use tagged switch on r.URL.Path (staticcheck) The OAuth2 token-refresh test handler had two cases — `r.URL.Path == "/token"` and `default` — both equality-against-r.URL.Path. Stati- ccheck's QF1002 rule wants this expressed as a tagged switch: switch r.URL.Path { case "/token": ... default: ... } The other four switches in the same file are mixed equality + Contains (`case r.URL.Path == "/token":` + `case strings.Contains(r.URL.Path, "/certificates"):`) — those are not tag-able and stay on `switch { case ... }`. Only the OAuth2 test handler had the single- equality-case pattern QF1002 fires on. Test-only commit. No production code change. Verified locally: - gofmt clean. - go test -short -count=1 ./internal/connector/issuer/googlecas/... green (5 failure tests + 14 happy-path subtests + 4 stub tests).	2026-05-03 21:32:55 +00:00
shankar0123	869fc8f245	docs(openssl): operator playbook for shell-out threat model ... Closes Top-10 fix #6 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, the OpenSSL adapter's docs in docs/connectors.md explained usage but did NOT enumerate the threat model. The adapter exec's an arbitrary operator-supplied script — env-var inheritance, symlink attacks, sandbox-escape, multi-tenant process-isolation gaps. An acquirer's security reviewer reading this surface cold pattern-matches "highest-risk issuer surface with the lowest documented threat model." This commit lands a doc-side operator playbook in docs/connectors.md OpenSSL section (mirrors Bundle 8's "Operator playbook: keytool argv password exposure" subsection shape and the 2026-05-02 audit Top-10 fix #7 SSH InsecureIgnoreHostKey playbook). Six topics covered: 1. Why the adapter exists despite the risk (CLI-driven CAs without Go SDKs need an integration path). 2. Threat model the adapter accepts (trusted operator + trusted script + appropriate ownership + clear audit trail). 3. Threat model the adapter does NOT accept (operator-writable script paths, untrusted content, multi-tenant hosts). 4. Mitigations operators can layer (dedicated user, root-owned 0755 binary, audit rules, per-call timeout via CERTCTL_OPENSSL_TIMEOUT_SECONDS, env sanitisation, chroot/container, audit wrapper, per-call concurrency bound). 5. When NOT to use the adapter (compliance environments, multi-tenant servers, no-script-review environments). 6. V3-Pro forward path (hardened mode tracked in cowork/WORKSPACE-ROADMAP.md). Inline comment in internal/connector/issuer/openssl/openssl.go near the callSignScript exec call site forward-references the new doc subsection (no logic change). cowork/WORKSPACE-ROADMAP.md gains an "OpenSSL hardened mode" V3- Pro entry under "Adapter hardening" — sibling-folder doc, not in the certctl repo, so not reflected in this commit's diff. Same shape Bundle 8 used for the JavaKeystore playbook and the 2026-05-02 deployment-target audit Top-10 fix #7 used for the SSH InsecureIgnoreHostKey playbook. No code logic changes (only the explanatory comment near the exec call site). No test changes. Doc-only commit. Verified locally: - gofmt / go vet clean. - go test -short -count=1 ./internal/connector/issuer/openssl/... green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/RESULTS.md Top-10 fix #6.	2026-05-03 21:28:05 +00:00
shankar0123	0792271dc6	vault: add automatic token renewal at TTL/2 + Prometheus metric ... Closes Top-10 fix #5 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, the VaultPKI adapter authenticated with a static token and never called renew-self. Long-lived deploys hit token expiry; the first operator-visible signal was failed cert renewals on production targets. This commit: 1. Connector.Start(ctx) spawns a goroutine that calls POST /v1/auth/token/renew-self at TTL/2 cadence (computed from a one-shot lookup-self at startup). Honours ctx.Done() for graceful shutdown via a per-loop done channel + Stop(). 2. On `renewable: false` response (initial lookup OR any subsequent renewal), the loop emits a WARN, increments the not_renewable counter, and exits. The operator must rotate the token before Vault's Max TTL elapses. 3. New Prometheus counter certctl_vault_token_renewals_total with labels result={success,failure,not_renewable}. Registered alongside existing certctl_issuance_* counters in internal/api/handler/metrics.go. 4. ERROR-level logging on renewal failure with operator-actionable substring ("vault token renewal failed; rotate the token before TTL expires") so journalctl + grep find it. Loop keeps ticking after a failure — transient blips don't kill it. New optional issuer.Lifecycle interface: type Lifecycle interface { Start(ctx context.Context) error Stop() } Connectors that hold no background goroutines (almost all of them) do not implement this — IssuerRegistry.StartLifecycles / StopLifecycles feature-detect via type assertion. New lifecycle-bearing connectors plug in by implementing the interface; no further registry plumbing required. Wiring (cmd/server/main.go): - service.NewVaultRenewalMetrics() instance is shared between issuerRegistry.SetVaultRenewalMetrics (so Vault connectors built by Rebuild get a recorder) and metricsHandler.SetVaultRenewals (so the Prometheus exposer emits the new series). - issuerRegistry.StartLifecycles(ctx) is called after issuerService.BuildRegistry; defer issuerRegistry.StopLifecycles is paired so goroutines exit cleanly on signal. - IssuerConnectorAdapter.Underlying() exposes the wrapped issuer.Connector so registry-level machinery can reach the concrete connector behind the adapter without duplicating the wiring at every call site. Tests (internal/connector/issuer/vault/vault_renew_test.go): - TestVault_RenewLoop_TickAtHalfTTL — three ticks → three renewals, all "success". - TestVault_RenewLoop_StopsOnNotRenewable — second renewal returns renewable=false, loop exits, third tick fires no HTTP call. - TestVault_RenewLoop_FailureSurfacesViaMetric — first renewal 403 bumps "failure", second renewal succeeds → loop kept ticking. - TestVault_RenewLoop_CtxCancellation_StopsCleanly — Stop returns within 200ms after ctx cancel. - TestVault_RenewLoop_StartsNothingWhenNotRenewable — token already non-renewable at boot ⇒ no goroutine, "not_renewable" metric increments at startup so operators see it in Grafana. - TestVault_ComputeInterval — 4 cases pinning TTL/2 + minRenewInterval floor. - TestVault_RenewSelf_ParseFailure_NamesActionableInError — surfaced error contains "vault token renewal failed" + "rotate the token". Cadence is dynamic — every successful renewal re-derives TTL/2 from the renewed lease's lease_duration, so a short bootstrap token that gets renewed up to a longer Max TTL shifts to the longer cadence automatically (defends against degenerate fast ticking on a token whose Max TTL is far longer than its initial TTL). Documentation: - docs/connectors.md Vault PKI section gains "Token TTL + automatic renewal" subsection (operator-facing: cadence, metric, renewable=false rotation playbook). Out of scope (intentional, flagged in the audit follow-up): - AppRole / Kubernetes / AWS IAM auth methods (different renewal semantics). - Hot-reload of rotated token from disk (operator restarts today; future: GUI/MCP issuer-update path triggers Rebuild which Stops the old connector and Starts the new one). - Auto-re-auth after token death (operator playbook owns it). CHANGELOG.md is intentionally not hand-edited (per CHANGELOG.md itself: "no longer maintains a hand-edited per-version changelog; per-release notes are auto-generated from commit messages between consecutive tags"). Verified locally: - gofmt clean. - go vet ./internal/service/... ./internal/api/handler/... ./internal/connector/issuer/vault/... ./cmd/server/... clean. - go test -short -count=1 ./internal/connector/issuer/vault/... ./internal/service/... ./internal/api/handler/... green. - go test -race -count=10 -run 'TestVault_RenewLoop|TestVault_ComputeInterval' ./internal/connector/issuer/vault/... green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/RESULTS.md Top-10 fix #5.	2026-05-03 21:24:27 +00:00
shankar0123	a2a59a823e	googlecas, awsacmpca: add failure_test.go covering cloud-SDK error contracts ... Closes Top-10 fix #4 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, both adapters had only happy-path test coverage with a single generic ServerError pair each. Cloud CAs are typically the first-deployed issuer in enterprise pilots; their diligence reviews dig hard into IAM-error / cloud-error coverage. This commit lands the contract tests. AWSACMPCA — 5 tests in awsacmpca_failure_test.go. Each injects a typed AWS SDK v2 error via the existing mockACMPCAClient seam and asserts (1) error non-nil, (2) errors.As against the SDK's typed value succeeds (so the wrap chain through fmt.Errorf("...%w", ...) is intact), and (3) operator-actionable substring is present. 1. Issue_AccessDenied — *smithy.GenericAPIError with Code="AccessDeniedException" (the SDK does NOT generate a typed *types.AccessDeniedException; AWS uses the smithy APIError shape for IAM denials). Asserts ErrorCode + "not authorized" + IAM resource path preserved through wrap. 2. Issue_ResourceNotFound — *types.ResourceNotFoundException names the missing CA ARN. 3. Issue_Throttling — *smithy.GenericAPIError with Code="ThrottlingException", Fault=FaultServer. Asserts the retryable class (FaultServer) is preserved through wrap so upstream retry logic can engage. 4. Issue_MalformedCSR — *types.MalformedCSRException is terminal (operator must fix the CSR, not retry); asserts the validation-issue substring survives. 5. Issue_RequestInProgress — *types.RequestInProgressException wraps cleanly; classification (retry vs reissue) is upstream's responsibility per the spec's "no new retry logic" rule. GoogleCAS — 5 tests in googlecas_failure_test.go. The adapter uses stdlib net/http directly (NO Google Cloud Go SDK dependency in googlecas.go), so SDK typed-error assertions don't translate. Each test runs an httptest.Server that returns the canonical Google API JSON error envelope: {"error":{"code":N,"message":"...","status":"<STATUS>"}} and asserts (1) error non-nil, (2) operator-actionable substring, and (3) the canonical status string ("PERMISSION_DENIED", "NOT_FOUND", "UNAVAILABLE") survives the wrap chain so upstream classification can branch on it. 1. Issue_PermissionDenied — 403 / PERMISSION_DENIED; surfaced error names the IAM resource path. 2. Issue_CAPoolNotFound — 404 / NOT_FOUND; surfaced error names the missing pool resource. 3. Issue_OAuth2TokenRefreshFailure — token endpoint returns 401 invalid_grant; surfaced error mentions "token" so an operator reading the log immediately distinguishes a credential failure (rotate SA key) from a CA-side error (fix IAM binding). Test also asserts the CAS endpoint is NOT reached when the token exchange fails. 4. Issue_RegionalAPIUnavailable — 503 / UNAVAILABLE; surfaced error preserves the retryable class markers (status code + UNAVAILABLE string) for upstream retry classification. 5. Revoke_PermissionDenied — adapter does NOT silently swallow the failure; pin the contract so the audit-row atomicity guarantee from Bundle G (which lives in the service-layer wrapper, not the adapter) continues to apply. Test also verifies the revoke endpoint was actually reached, guarding against a future regression that short-circuits before the HTTP call. Coverage delta: awsacmpca: 71.0% → 71.0% (failure tests reuse existing wrap code paths; behaviour-pin contract tests, not coverage tests). googlecas: 83.4% → 84.4% (+1.0pp). go.mod: smithy-go moved indirect → direct, since the new AWSACMPCA test file imports it. CI's go-mod-tidy-drift gate enforces this. Test-only commit. No production code changes. Verified locally: - gofmt clean. - go vet ./internal/connector/issuer/awsacmpca/... ./internal/connector/issuer/googlecas/... clean. - go test -short -count=1 ./internal/connector/issuer/... green. - go test -race -count=10 ./internal/connector/issuer/awsacmpca ./internal/connector/issuer/googlecas green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/RESULTS.md Top-10 fix #4.	2026-05-03 21:10:41 +00:00
shankar0123	b0c4ed1ae2	openssl: add failure_test.go covering 6 shell-out error modes ... Closes Top-10 fix #3 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, the OpenSSL adapter (497 LOC, certctl's highest-risk issuer surface) had openssl_test.go (8 happy-path funcs + 20 subtests) but no dedicated _failure_test.go. Compare to ACME, Vault, DigiCert, Sectigo, Entrust, GlobalSign, EJBCA — all peers have one. An acquirer's diligence team flags this as an immediate blocker on the highest-risk issuer surface. This commit adds 6 failure-mode tests: 1. TestOpenSSL_Issue_ScriptNotFound_OperatorActionableError — SignScript path doesn't exist; error wraps os.ErrNotExist (errors.Is); message contains 'no such file' / 'not found' so the operator's grep finds it in journalctl. 2. TestOpenSSL_Issue_PermissionDenied_OperatorActionableError — SignScript exists with mode 0o600 (non-executable); error wraps os.ErrPermission; message contains 'permission'. Skipped under root (uid 0 bypasses chmod gating). 3. TestOpenSSL_Issue_MalformedStdout_DistinguishedFromCSRReject — script exits 0 + writes garbage (no PEM markers) to the cert output file; error mentions PEM/certificate/parse so operators distinguish output-parsing failure from a script- side fault. 4. TestOpenSSL_Issue_NonZeroExit_DistinguishesCAReject_From_ ScriptError — script writes 'policy violation: …' to stderr and exits 2 (CA-side rejection convention); the script's stderr surfaces in the error message; errors.Unwrap returns non-nil (proving the underlying *exec.ExitError chain survives). 5. TestOpenSSL_Issue_TimeoutEnforced_ContextCancellationPropagates — script does 'exec sleep 30' (not 'sleep 30 ' as a child; exec replaces bash so SIGKILL goes directly to the sleeper, avoiding the orphan-pipes corner case where a killed bash leaves sleep holding stdout/stderr open and CombinedOutput blocks); ctx with 100ms deadline; call returns within ~5s wall-clock; either errors.Is(err, context.DeadlineExceeded) or the error message names 'killed' / 'signal'. 6. TestOpenSSL_Issue_SignalKilled_PartialOutputDiscarded — script writes a half-PEM ('-----BEGIN CERTIFICATE-----\nMII…') then 'kill -KILL $$'; assertion: result is nil OR CertPEM is empty (no half-cert leaks to caller); error names 'signal' / 'killed' OR 'PEM' / 'parse' (both are operator-actionable). Each test pins the operator-actionable error message contract: the message names the failure mode (so journalctl + grep find it) and proves no half-state was created (no partial cert returned). errors.Is / errors.Unwrap checks confirm the wrapping chain survives. The OpenSSL adapter has no commandRunner abstraction (production code uses exec.CommandContext directly); these tests use real operator-supplied scripts written to t.TempDir (matches the adapter's actual production code path; no os/exec mocking). The 'exec sleep 30' technique in Test 5 is the load-bearing fix for the bash-orphans-sleep-and-pipes-stay-open corner case that otherwise makes the test take 30s instead of 100ms. Coverage delta: - Before this commit: openssl_test.go + openssl_stubs_test.go covered 8 happy-path funcs. - After: 79.8% statement coverage of openssl.go (up from operator-pre-existing baseline; the 6 new tests exercise every error path through callSignScript + parseCertificate). Tests pass clean under '-race -count=10' (Test 5's deadline tolerance is the only timing-sensitive case; the 5s wall-clock budget vs the 100ms ctx deadline gives ample slack on slow CI without masking deadline-not-enforced bugs). Test-only commit; no production code changes. Hardening fixes (per-call concurrency semaphore, threat-model docs) are separate Top-10 entries. Verified locally: - gofmt clean across the repo. - go vet ./... clean across the repo. - go test -race -count=10 -short ./internal/connector/issuer/openssl/... green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/ RESULTS.md Top-10 fix #3.	2026-05-03 20:55:44 +00:00
shankar0123	d3bf2cc0cf	vault, digicert: migrate Token / APIKey to *secret.Ref (Bundle I Phase 3) ... Closes Top-10 fix #2 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, vault.Config.Token and digicert.Config.APIKey were plain string fields. Practical impact: 1. GET /api/v1/issuers responses marshalled the credential into the JSON body. An acquirer's procurement engineer running 'curl /api/v1/issuers | jq' saw the token / API key in plain text on screen. 2. DEBUG-level HTTP request logging printed the credential header verbatim. 3. A heap dump of the running server contained the credential as readable bytes for the lifetime of the process. Bundle I from the 2026-05-01 audit closed this for AWSACMPCA, EJBCA, GlobalSign, Sectigo (Phase 1+2). Vault and DigiCert were left out. This commit ports the same migration onto them. Mechanics: - Config.Token / Config.APIKey type changed from 'string' to '*secret.Ref'. UnmarshalJSON of a JSON string populates the Ref via NewRefFromString — operator config files are unchanged. - Every header-write call site routed through Ref.Use, with the byte buffer zeroed after the callback returns. Vault: 3 sites (IssueCertificate, RevokeCertificate, GetCACertPEM). DigiCert: 5 sites (ValidateConfig, IssueCertificate, RevokeCertificate, pollOrderOnce, downloadCertificate). - ValidateConfig nil-checks switch from 'cfg.Token == ""' to 'cfg.Token.IsEmpty()' (mirrors Sectigo's existing pattern). - Tests migrated: every Config{Token:"..."} → Config{Token: secret.NewRefFromString("...")}. The 'json.Marshal(config) → ValidateConfig(rawConfig)' round-trip pattern in DigiCert's ValidateConfig_Success test is now broken by the redact-on-marshal contract — switched that one to construct the rawConfig as a JSON literal (mirrors Sectigo's existing test pattern). - Two new tests pin the redact-on-marshal contract: - TestVault_Config_TokenMarshalsAsRedacted (vault_redact_test.go) - TestDigiCert_Config_APIKeyMarshalsAsRedacted (digicert_redact_test.go) Both assert the marshaled JSON contains '"[redacted]"' and does NOT contain the plaintext bytes. Operator-visible: GET /api/v1/issuers responses for type=vault and type=digicert now show the credential as '[redacted]'. Existing config files keep working — the Ref unmarshal accepts strings. CHANGELOG note: certctl/CHANGELOG.md is intentionally not hand-edited; release notes are auto-generated from commit messages between consecutive tags. This commit's message body is the release-note artifact. Verified locally: - gofmt clean across the repo. - go vet ./... clean across the repo. - go test -race -count=1 -short ./internal/connector/issuer/vault/... ./internal/connector/issuer/digicert/... ./internal/secret/... green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/ RESULTS.md Top-10 fix #2.	2026-05-03 20:49:23 +00:00
shankar0123	81f6321326	ejbca: port mTLS keypair to mtlscache (close Bundle M for the last issuer) ... Closes Top-10 fix #1 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, ejbca.go::New called tls.LoadX509KeyPair once at construction and configured the keypair into *http.Transport.TLSClientConfig with no mtime watch. mTLS rotation required a server restart — quarterly rotation per any reasonable security policy = quarterly deploy outage. Bundle M from the prior 2026-05-01 audit shipped the mtlscache helper at internal/connector/issuer/mtlscache/cache.go and wired it into Entrust + GlobalSign. EJBCA was missed in Bundle M's scope. This commit ports the same helper onto EJBCA's auth_mode=mtls path. The OAuth2 path is unchanged. Implementation: - New imports internal/connector/issuer/mtlscache. - Connector struct gains an mtls *mtlscache.Cache field (mirroring Entrust + GlobalSign). - New()'s case 'mtls': replaces tls.LoadX509KeyPair + manual *http.Transport with mtlscache.New(certPath, keyPath, Options{HTTPTimeout: 30s}). Cache build happens at construction so misconfigured operators fail fast (matches pre-fix behaviour). - New helper getHTTPClient() returns the cached client; on the mTLS path it calls RefreshIfStale before returning so the next request uses the new keypair if disk has rotated. On OAuth2 / test paths (c.mtls == nil), returns c.httpClient as-is. - All 3 c.httpClient.Do call sites (IssueCertificate enroll, RevokeCertificate revoke, GetOrderStatus cert lookup) replaced with c.getHTTPClient() + client.Do. - crypto/tls import removed (no longer used at this layer). Tests: - TestEJBCA_MTLSKeypairRotation_PicksUpNewCertWithoutRestart (new, ejbca_mtls_rotation_test.go): generates two CAs (caA, caB), signs leafA + leafB, spins up an httptest TLS server that trusts both CAs and records the issuer DN of every presented client cert, writes leafA, makes request 1, writes leafB + advances mtime by 2s, makes request 2. Asserts the server saw caA's DN on req 1 and caB's DN on req 2 — the cache picked up the rotation without ejbca.New re-running. - export_test.go: GetHTTPClientForTest helper exposes the private getHTTPClient so the rotation test drives the production code path. - All existing EJBCA tests still pass (TestNew_MTLSWiresClientCert, TestNew_MTLSCertLoadFailure, TestNew_OAuth2NoTransportTuning, TestNew_InvalidAuthMode). Verified locally: - gofmt clean across the repo. - go vet ./... clean across the repo. - go test -race -count=1 -short ./internal/connector/issuer/ejbca/... ./internal/connector/issuer/mtlscache/... green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/ RESULTS.md Top-10 fix #1.	2026-05-03 20:38:19 +00:00
shankar0123	bee47f0318	acme-server: cert-manager integration test + production hardening (Phase 5/7) ... Closes the production-readiness loop on the ACME surface. After this commit, certctl ships per-account rate limits + a GC sweeper for expired ACME state + a kind-driven cert-manager 1.15 integration test + a lego-driven RFC conformance harness + a k6 loadtest scenario for the unauthenticated ACME path. Architecture: - Rate limits live in-memory + per-replica. Restart wipes the counters; orders/hour caps are eventual-consistency anyway. A 3-replica certctl-server fleet behind an LB effectively has 3x the configured throughput per account; persistent rate limiting is a follow-up if production telemetry shows abuse patterns we can't catch in a single restart cycle. Per-key + per-action isolation: ActionNewOrder/acc-1, ActionKeyChange/acc-1, and ActionChallengeRespond/<challenge-id> are independent buckets. - GC loop follows the existing scheduler-loop pattern (atomic.Bool + sync.WaitGroup; see crlGenerationLoop for shape). Three independent SQL sweeps per tick (DELETE expired nonces; UPDATE pending authzs whose expires_at < now() to expired; UPDATE pending/ready/processing orders whose expires_at < now() to invalid). Each sweep is a single statement; failures are logged- and-continued so a failing nonces sweep doesn't block authzs. Per-sweep 1m timeout bounds a stuck Postgres. - cert-manager integration test is gated on KIND_AVAILABLE so CI skips it cleanly (kind is too heavy for per-PR). Operators run locally via 'make acme-cert-manager-test'; the harness brings up a fresh cluster each run + tears it down on Cleanup. - lego conformance harness drives a real ACME client through register → run → cert-PEM-landed against a hermetic certctl stack. Catches RFC-shape regressions third-party clients would hit before they ship. - k6 ACME-flow scenario hammers the unauthenticated surface (directory + new-nonce + ARI synthetic-id) at 100 VUs × 5m. JWS- signed flows are out of scope for k6 (no JWS support); they're covered by the lego harness above. What ships: - internal/api/acme/ratelimit.go (+ ratelimit_test.go: 7 cases — disable-when-perHour-zero, capacity, per-key isolation, per- action isolation, refill-over-time, RetryAfter, concurrent-access with -race + 200 goroutines × 200 calls). - internal/repository/postgres/acme.go: 4 new methods — CountActiveOrdersByAccount + GCExpiredNonces + GCExpireAuthorizations + GCInvalidateExpiredOrders. Each a single SQL statement. - internal/service/acme.go: SetRateLimiter + GarbageCollect + rate-limit gates at 3 entry points (CreateOrder + RotateAccountKey + RespondToChallenge) + concurrent-orders gate at CreateOrder. 2 new sentinels (ErrACMERateLimited, ErrACMEConcurrentOrdersExceeded); 5 new GC metrics (gc_runs / gc_run_failures / gc_nonces_reaped / gc_authzs_expired / gc_orders_invalidated). - internal/scheduler/scheduler.go: ACMEGarbageCollector interface + acmeGCRunning atomic.Bool + acmeGCInterval + 2 setters (SetACME- GarbageCollector + SetACMEGCInterval) + acmeGCLoop following the crlGenerationLoop shape. - internal/api/handler/acme.go: writeServiceError gains rateLimited (429 + RFC 8555 §6.7) + concurrent-orders-exceeded mappings. - internal/config/config.go: 5 new env vars (CERTCTL_ACME_SERVER_RATE_LIMIT_ORDERS_PER_HOUR=100, CERTCTL_ACME_SERVER_RATE_LIMIT_CONCURRENT_ORDERS=5, CERTCTL_ACME_SERVER_RATE_LIMIT_KEY_CHANGE_PER_HOUR=5, CERTCTL_ACME_SERVER_RATE_LIMIT_CHALLENGE_RESPONDS_PER_HOUR=60, CERTCTL_ACME_SERVER_GC_INTERVAL=1m). - cmd/server/main.go: NewRateLimiter() + SetRateLimiter() at startup; conditional SetACMEGarbageCollector(acmeService) + SetACMEGCInterval(cfg.ACMEServer.GCInterval) when Enabled+ GCInterval > 0. - deploy/test/acme-integration/: kind-config.yaml + cert-manager- install.sh + clusterissuer-trust-authenticated.yaml + clusterissuer-challenge.yaml + certificate-test.yaml + conformance- lego.sh + certmanager_test.go (//go:build integration + KIND_AVAILABLE gate). - deploy/test/loadtest/k6/acme_flow.js + README ACME-flows section. - Makefile: 2 new PHONY targets (acme-cert-manager-test + acme-rfc-conformance-test). - docs/acme-server.md: status flipped to Phase 5; Configuration table grows 5 rows; new 'Phase 5 — operational guidance' section explaining rate-limit math + GC sweeper semantics + cert-manager integration + lego conformance + k6 baseline. Tests: - 'go vet ./...' clean across the repo. - 'go test -short -count=1 ./internal/...' green across every affected package (service / acme / handler / scheduler / repo / config). - 'go vet -tags=integration ./deploy/test/acme-integration/' clean (the integration test compiles cleanly with the build tag). - The kind/cert-manager harness is gated behind KIND_AVAILABLE so CI skips by default; operators run locally via 'make acme-cert- manager-test'. Engineering history: cowork/WORKSPACE-CHANGELOG.md 'ACME-Server-5'.	2026-05-03 19:42:03 +00:00
shankar0123	650f5a198f	fix: collapse identical if/else branches in Account handler (CodeQL #25) ... CodeQL alert #25 (go/duplicate-branches) on internal/api/handler/ acme.go::ACMEHandler.Account flagged that 'if readOnly { ... } else { ... }' had byte-identical bodies — both setting the same Content-Type: application/json header. The 'readOnly' bool was threaded through the function as a placeholder for differentiated headers (Cache-Control etc. on the POST-as-GET path) that never landed; both branches collapsed to the same value with no follow-through. Audit + fix: - The alert is real (verified by re-reading the source); not a false positive. - The Copilot Autofix Anthropic surfaced was correct in spirit but incomplete: it collapsed the if/else but left 'readOnly' as dead code (declared at line 395, assigned at lines 400 and 436, only read at the now-removed if). golangci-lint's 'unused' linter would flag 'readOnly' next. - Complete fix: collapse the if/else AND remove the now-unused 'readOnly' variable + its 2 assignments. Single unconditional 'w.Header().Set("Content-Type", "application/json")' covers both paths (RFC 8555 §6.3 POST-as-GET + §7.3.2 / §7.3.6 update + deactivation all return the same account JSON shape — no spec rationale for differentiating headers). Verified locally: 'gofmt -l .' clean; 'go vet ./...' clean; 'go test -short -count=1 ./internal/api/handler/' green; 'grep readOnly' on the file returns only the new explanatory comment (no live references). The alert was first detected in commit 44a85d6 (Phase 1b) — the duplicate has been sitting in the codebase since the Account handler shipped. No functional regression for any RFC 8555 client (cert-manager, lego, Posh-ACME): same status code, same headers, same body.	2026-05-03 19:07:21 +00:00
shankar0123	1e1bc9b3b4	ci: fix Phase 4 post-push unused-symbol failures ... CI on commit f6ba563 (Phase 4 gofmt fix) failed golangci-lint's 'unused' linter on internal/service/acme_phase4_test.go: the stubRenewalPolicies type + its Get method were defined for a future RenewalInfo happy-path test that I never actually wrote — only the disabled + bad-cert-id negatives. The dead-code carried forward because go vet doesn't catch unused-but-exported-shape, and the package-private use never materialized. Fix: delete the stubRenewalPolicies type + its method + the adjacent stub-comment that referenced a similarly-imagined stubIssuerConn that was never written either. The tests I have (RotateAccountKey happy + duplicate, RevokeCert kid + jwk paths + already-revoked + reason-clamping, RenewalInfo disabled + bad-cert-id) all still pass — they don't reference the removed type. The window-math is exercised directly in internal/api/acme/phase4_test.go::TestComputeRenewalWindow_*; the service-layer policy-lookup wiring is read at handler smoke time in Phase 5. Confirmed: 'gofmt -l .' clean; 'go vet ./internal/service/' clean; 'go test -short -count=1 ./internal/service/' green. Pre-commit verification gate updated implicitly: future Phase commits should spot-check unused-shape via grep against the test file (every stub* helper should have ≥3 references, matching the live helpers' usage profile).	2026-05-03 19:02:44 +00:00
shankar0123	f6ba5634fd	ci: fix Phase 4 post-push gofmt failure (map-literal alignment) ... CI on commit 4dc8d3f (Phase 4) failed gofmt on internal/api/router/openapi_parity_test.go. The 6 new SpecParity- Exceptions entries I added for the Phase 4 routes had over-padded whitespace between key and value; the longest new key is '"GET /acme/profile/{id}/renewal-info/{cert_id}":' which sets the gofmt-canonical column width for the surrounding block, but my hand-aligned values used the wider Phase-2 column width (set by the even-longer 'POST /acme/profile/{id}/order/{ord_id}/finalize' key in that block). gofmt aligns map-literal columns per contiguous run between blank lines / structural breaks, not file-globally. The Phase 4 entries form their own run because they're separated from the Phase 2 block by the '// Phase 4 — key rollover + revocation + ARI.' comment. Fix: 'gofmt -w' on the file, which rewrote the 6 lines with the correct (narrower) intra-block alignment. No semantic change — just whitespace. Confirmed: 'gofmt -l .' clean; 'go vet ./internal/api/router/' clean (the test still passes after the formatting change).	2026-05-03 18:58:00 +00:00