2 Commits

Author	SHA1	Message	Date
shankar0123	037dab7b6f	fix(agent,service): SEC-002 — validate certificate_id shape + contain key path ... Sprint 1 unified-master-audit closure. Pre-fix the agent built its on-disk key path via: keyPath := filepath.Join(a.config.KeyDir, job.CertificateID+".key") migrations/000001_initial_schema.up.sql declares managed_certificates.id as TEXT PRIMARY KEY with no shape constraint, so a compromised control plane (or a poisoned database row) could deliver a job whose certificate_id is '../../etc/passwd', '/absolute/path', a NUL-byte payload, or a Windows-separator-laden string — driving arbitrary file write or read on the agent host. Fix (two ends; both load-bearing): Server side: - New internal/validation/certificate_id.go: ValidateCertificateID pins the canonical TEXT-PK shape (^[A-Za-z0-9._-]{1,128}$, plus explicit '.'/'..' rejection). - CertificateService.Create now invokes ValidateCertificateID after the existing required-fields check; malformed IDs are refused before persistence or downstream job creation. Agent side: - cmd/agent/keymem.go: validateAgentCertID mirrors the server-side shape regex. safeAgentKeyPath additionally asserts the joined path is contained within KeyDir via filepath.Rel — even if a future refactor bypasses the shape check, a path that escapes KeyDir fails closed. - poll.go + deploy.go: both filepath.Join call sites routed through safeAgentKeyPath; rejection surfaces via reportJobStatus so the control plane sees the failure. Regression coverage: - internal/validation/certificate_id_test.go: production shapes accepted; explicit rejection table for empty, overlong, posix traversal, absolute, Windows traversal, Windows separator, NUL byte, newline/tab injection, drive prefix, space, unicode dots. - cmd/agent/keymem_test.go: validateAgentCertID acceptance + rejection tables; safeAgentKeyPath happy path + the 8 audit vectors plus empty-keyDir refusal. Closes SEC-002.	2026-05-16 03:31:59 +00:00
shankar0123	3094010880	refactor(cmd/agent): split main.go into poll + deploy + discovery sibling files (Phase 9, 12 of N — LAST hotspot) ... Phase 9 ARCH-M2 closure Sprint 12 — the LAST of the audit's named hotspot sub-splits. Splits cmd/agent/main.go (1489 LOC, the sixth-largest backend hotspot at audit time) via the Option B sibling-file pattern (mirrors the Sprint 8 cmd/server cut). Package stays `main`; every method is still defined on *Agent so each call site continues to resolve through Go's same-package method-set — no import-path or signature change. Audit prescription vs reality ============================= The audit's Tasks-Deferred row prescribed "main + poll + deploy + register sibling files." The actual cmd/agent/main.go has no `register` function — agent registration happens via the control-plane REST API (POST /api/v1/agents) before the agent process starts. The closest analogue in the agent binary is the filesystem-discovery scan (runDiscoveryScan + the parsePEMFile / parseDERFile / certToEntry / sha256Sum / certKeyInfo helpers), which is the agent's other "outbound report-to-server" surface alongside the inbound work-poll path. Sprint 12 substitutes `discovery` for `register` in the prescription and keeps the other three buckets as named: `main` (lifecycle + HTTP infrastructure + entrypoint), `poll` (work-poll + CSR-job execution), `deploy` (deployment-job execution + target connector factory). What moved ========== New `cmd/agent/poll.go` (279 LOC) — work-poll + CSR-job execution: - pollForWork: GET /api/v1/agents/{id}/work each tick; dispatches each returned JobItem to the right executor. - executeCSRJob: handles AwaitingCSR jobs by generating an ECDSA P-256 key locally, persisting it with 0600 permissions (key NEVER leaves the agent — CLAUDE.md "Agent-based key management"), creating + submitting the CSR. New `cmd/agent/deploy.go` (443 LOC) — deployment + target factory: - executeDeploymentJob: handles Pending deployment jobs by fetching the cert PEM, loading the locally-held private key (agent keygen mode), instantiating the appropriate target connector, calling DeployCertificate, and reporting status. - createTargetConnector: the 170-LOC switch over target_type that instantiates 14 different target connectors (apache / awsacm / azurekv / caddy / envoy / f5 / haproxy / iis / javakeystore / k8ssecret / nginx / postfix / ssh / traefik / wincertstore). Context is threaded through to SDK-driven connectors (AWSACM, AzureKeyVault) per the contextcheck linter fix in CI commit 502823d. - splitPEMChain + fetchCertificate (deploy-only helpers). New `cmd/agent/discovery.go` (275 LOC) — filesystem cert discovery: - runDiscoveryScan: walks each configured discovery directory, dispatches each candidate file to parsePEMFile / parseDERFile, batches the parsed entries, and POSTs them to /api/v1/agents/{id}/discoveries (the machine-to-machine surface that is intentionally NOT exposed via MCP). - parsePEMFile + parseDERFile + certToEntry + sha256Sum + certKeyInfo + the discoveredCertEntry struct that ties them together. What stays in main.go (644 LOC, down from 1489) ================================================ - Types: AgentConfig, Agent struct, ErrAgentRetired var, WorkResponse, JobItem. - Lifecycle: NewAgent constructor, Run, markRetired, sendHeartbeat, getOutboundIP, targetDeployMutex method. - Shared HTTP infrastructure: makeRequest (consumed by poll + deploy + discovery + lifecycle), reportJobStatus (consumed by poll + deploy). - Entrypoint: main(), getEnvDefault, getEnvBoolDefault, validateHTTPSScheme. Side-effect import cleanup ========================== 21 imports drop from cmd/agent/main.go as a clean side effect: Standard library (7): - crypto/ecdsa, crypto/elliptic (poll only) - crypto/rand (poll only) - crypto/rsa (discovery only) - crypto/sha256 (discovery only) - crypto/x509/pkix (poll only) - encoding/pem (poll + deploy + discovery) - path/filepath (poll + deploy + discovery) Target connectors (14): - internal/connector/target + apache + awsacm + azurekv + caddy + envoy + f5 + haproxy + iis + javakeystore + k8ssecret + nginx + postfix + ssh + traefik + wincertstore — all 14 were used ONLY by createTargetConnector and moved with the factory to deploy.go. The surviving main.go now imports 20 stdlib packages + zero internal packages — the leanest the agent binary's entrypoint has been since the agent first shipped target-connector orchestration. Per-import audit on every new sibling file is in the diff: - poll.go: context, crypto/ecdsa, crypto/elliptic, crypto/rand, crypto/x509, crypto/x509/pkix, encoding/json, encoding/pem, fmt, io, net/http, os, path/filepath, strings (no sync — the sync.Once / sync.Mutex / sync.Map usages all live in the surviving main.go's lifecycle code). - deploy.go: context, encoding/json, encoding/pem, fmt, io, net/http, os, path/filepath, strings + target + 14 connector packages. - discovery.go: context, crypto/ecdsa, crypto/rsa, crypto/sha256, crypto/x509, encoding/pem, fmt, io, net/http, os, path/filepath, strings, time. Net effect ========== main.go: 1489 → 644 LOC (-845 = -56.7%). Three new sibling files at 997 LOC total (845 moved + ~152 LOC of header + Phase 9 doc-comment overhead). Matches the Sprint 8 cmd/server pattern in shape (main + wire + migrations) and size reduction (-23.8% there vs -56.7% here — the agent had more concentrated single-purpose functions than the server's wiring-heavy main). Cumulative Phase 9 progress (all 6 named hotspots) ================================================== config.go 3403 → 1342 (-60.6%, Sprints 1-7) cmd/server/main.go 2966 → 2260 (-23.8%, Sprints 8 + 8b) service/acme.go 1965 → 1162 (-40.9%, Sprints 9 + 9b) mcp/tools.go 1867 → 109 (-94.2%, Sprint 10) auth_session_oidc 1577 → 452 (-71.3%, Sprint 11) cmd/agent/main.go 1489 → 644 (-56.7%, Sprint 12) TOTAL across 6 files: 13,267 → 5,969 LOC = -7,298 (-55.0%) All 6 named hotspots from the audit's top-6 list are now below 1,500 LOC. The largest remaining hotspot from the top-6 is cmd/server/main.go at 2,260 LOC (intentional — every backend service the server wires is one line in main(), so the size is roughly proportional to surface area, not concern-tangling). Behavior preservation contract ============================== 1. gofmt -l clean across all 4 affected files. 2. go vet ./cmd/agent/... — no findings. 3. staticcheck ./cmd/agent/... — no findings. 4. go test -short -count=1 ./cmd/agent/... — green (includes agent_test.go 1716-LOC suite that pins every moved function: pollForWork / executeCSRJob / executeDeploymentJob / createTargetConnector / runDiscoveryScan plus dispatch_test.go, deploy_mutex_test.go, keymem_test.go). 5. Broader-importer build green: go build ./... . Same-package resolution means every cross-file call (poll → makeRequest, deploy → makeRequest + reportJobStatus + verifyAnd- ReportDeployment in verify.go, discovery → makeRequest) resolves through Go's package-level method-set with zero compile-time cost + zero runtime overhead. The public surface of the cmd/agent binary is unchanged. What this commit closes ======================= Sprint 12 is the LAST of the audit's named top-6 hotspot sub-splits. The ARCH-M2 finding now reflects: - 6 of 6 named backend hotspots below 1,500 LOC. - 24 of 24 named sub-splits shipped across Sprints 1-12 (config family ×7 + cmd/server ×2 + service/acme ×2 + mcp/tools ×6 + auth_session_oidc ×4 + cmd/agent ×3). - 7,298 LOC of code-locality concentration removed across the top 6 files. Whether to flip ARCH-M2 from 🛠 Scaffolded to ✓ Shipped is now an operator-discretion call — every named target landed, but the finding's spirit ("split god-files by responsibility") is a continuous discipline rather than a binary done/not-done. Refs: ARCH-M2 (god-files), Phase 9 audit. Sprint 12 is the named- hotspot conclusion of Phase 9.	2026-05-14 10:36:08 +00:00