8 Commits

Author	SHA1	Message	Date
shankar0123	737c329824	iis,wincertstore: default-deadline ctx wrapper for PowerShell exec calls ... Closes Top-10 fix #4 of the 2026-05-02 deployment-target audit re-run (see cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md). Pre-fix, both IIS and WinCertStore's realExecutor invoked PowerShell via exec.CommandContext(ctx, ...) and relied entirely on the caller's ctx to provide a deadline. If the caller forgot to attach one (context.Background() in a deeply-nested path; an operator running an ad-hoc deploy via a CLI that doesn't default-deadline its ctx), a hung WinRM session blocked the deploy worker thread indefinitely. S2 (failure isolation) bar from the audit: "does a hung WinRM take down the deploy worker pool?" — today's answer was "potentially yes" for these two connectors. Post-fix the answer is "no, capped at the configured ExecDeadline (default 60s)". This commit: 1. Adds Config.ExecDeadline (time.Duration, json: "exec_deadline") to both connectors, defaulted to 60 seconds. WinCertStore defaults via the existing applyDefaults helper; IIS defaults inline at New() and inside ValidateConfig (the IIS connector has no shared applyDefaults helper today; out-of-scope to refactor one in for this minor fix). Operators on slow Windows links can override via the JSON config field exec_deadline. 2. Wraps realExecutor.Execute with a fallback context.WithTimeout that fires ONLY when ctx has no deadline of its own. Caller- supplied deadlines always win — the wrapper is a safety net, not a hard cap. defer cancel() guards against goroutine leaks. 3. Tests: - TestIIS_RealExecutor_AttachesDefaultDeadlineWhenCallerHasNone (passes context.Background; asserts the call returns within 500ms with an error). On Linux/macOS runners powershell.exe is missing and exec.Cmd fails fast; on Windows the wrapper's ctx deadline cancels the running PowerShell process. Either path returns well under 500ms. - TestIIS_RealExecutor_RespectsCallerDeadlineWhenSet (10s fallback executor deadline, 50ms caller ctx; asserts caller deadline wins). - TestIIS_RealExecutor_NoDeadlineWiredWhenZero (deadline=0 means no fallback wrapper; caller's tight ctx still bounds). - TestIIS_New_DefaultsExecDeadlineTo60s + TestIIS_New_RespectsExplicitExecDeadline pin the constructor's defaulting behavior (uses winrm mode so the test doesn't need powershell.exe in PATH). - Same five tests in wincertstore_test.go. 4. docs/connectors.md IIS + WinCertStore sections document the new exec_deadline field with: what it is (per-PowerShell- subprocess cap), default (60 seconds), override semantics (caller ctx deadline wins). No change to behavior when the caller already attaches a deadline (the common case in production code paths). Tests using the mock executor (mockExecutor in iis_test.go / wincertstore_test.go) are unaffected — they bypass realExecutor entirely. S2 cross-cutting scorecard rating in cowork/deployment-target-audit-2026-05-02-rerun/findings.json flips from "gap" to "pass" for IIS and WinCertStore (in any future re-audit). Verified locally: - gofmt / go vet / staticcheck clean across both packages. - go test -race -count=1 ./internal/connector/target/iis/... ./internal/connector/target/wincertstore/... green. Audit reference: cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md Top-10 fix #4.	2026-05-02 22:38:35 +00:00
shankar0123	6d3d861acc	iis,wincertstore,javakeystore: SHA-256 idempotency short-circuit ... Closes Top-10 fix #3 of the 2026-05-02 deployment-target audit re-run (see cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md). Pre-fix, the three PowerShell-driven connectors (IIS / WinCertStore / JavaKeystore) bypass internal/deploy.Apply because they write to the Windows cert store / Java keystore via PowerShell + keytool rather than the local filesystem. They don't get deploy.Apply's SHA-256 idempotency short-circuit for free, so every renewal triggers a full Remove+Import cycle even on byte- identical material. Operators with 60-day rotation see unnecessary cert-store / keystore churn, briefly bumping CPU and possibly disrupting connections in flight. This commit adds a per-connector idempotency probe modeled on Bundle 9's Caddy api-mode SHA-256 short-circuit (commit 8cda860). Each probe runs at the top of DeployCertificate, BEFORE the destructive step, with a unique # CERTCTL_IDEM_PROBE PowerShell comment tag so test mocks match deterministically. IIS: Get-ChildItem Cert:\... + Get-WebBinding; matches when both the cert is in the store AND the active binding's certificateHash equals the new thumbprint. WinCertStore: Get-ChildItem Cert:\...\<thumbprint>; matches when the cert exists in the configured store AND its NotAfter is still in the future. JavaKeystore: keytool -list -alias -v; matches when the parsed SHA-256 fingerprint equals sha256(certPEM_DER). On match: return Success=true with Metadata["idempotent"]="true", no destructive operation. On any error during the probe (network, parse, etc.): fall through to today's full deploy path. False negatives are safe; false positives are dangerous. Tests added (one positive + one negative per connector): - TestIIS_Idempotent_SkipsDeployWhenBindingMatches - TestIIS_Idempotent_DifferentBinding_FallsThroughToDeploy - TestWinCertStore_Idempotent_SkipsImportWhenCertInStore - TestWinCertStore_Idempotent_NotInStore_FallsThroughToDeploy - TestJKS_Idempotent_SkipsDeployWhenAliasMatches - TestJKS_Idempotent_DifferentAlias_FallsThroughToDeploy Verified locally: - gofmt clean across all three connectors. - Syntax-validated via gofmt. Audit reference: cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md Top-10 fix #3.	2026-05-02 22:09:30 +00:00
shankar0123	1dd1dd4e0a	wincertstore: pre-deploy snapshot + on-import-failure rollback ... Closes Bundle 7 of the 2026-05-02 deployment-target coverage audit (see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix, DeployCertificate at wincertstore.go:162-215 ran a single PowerShell script that imported the PFX, optionally set FriendlyName, and optionally removed expired same-Subject certs. Import-PfxCertificate is atomic at the cert-store level, but the wider sequence (import → friendly name → remove expired) is not. Failure in any post-import step left the new cert in the store with no clean recovery path. docs/deployment-atomicity.md L93 promised "Get-ChildItem snapshot for rollback"; the code didn't deliver. This commit: 1. Pre-deploy snapshot. New PowerShell script (tagged `# CERTCTL_SNAPSHOT`) runs Get-ChildItem over the target store, captures every thumbprint, and for each cert with the same Subject as the new one calls Export-PfxCertificate to a tempdir using a transient snapshotExportPassword (32-byte random, distinct from the import PFX password). Output parsed into a snapshotState{Entries: []{Thumbprint, PfxPath}, AllThumbprints, TempDir, ExportPassword}. The new cert's Subject is parsed from request.CertPEM via certutil.ParseCertificatePEM before any cert-store mutation; PEM-parse failure aborts the deploy cleanly. 2. On-import-failure rollback. When the import-script Execute returns error, run a rollback script (tagged `# CERTCTL_ROLLBACK`) that: - Test-Path on the new cert path; Remove-Item if present. - Import-PfxCertificate -FilePath <pfxPath> for each snapshot entry (restores prior state). - Remove-Item -Recurse on the snapshot tempdir. 3. Post-rollback verification. Re-read Get-ChildItem (tagged `# CERTCTL_VERIFY`); assert every original thumbprint is back. On mismatch, append a warning to the DeploymentResult message (rollback ran but final state is suspect — operator inspection recommended). Skipped when AllThumbprints is empty (first-time deploy). 4. Success-path tempdir cleanup. New script tagged `# CERTCTL_CLEANUP` runs after a successful import to remove the snapshot tempdir on a best-effort basis. Failure here is non-fatal (debug log only). 5. Helper extraction. rollbackImport(ctx, snapshot, newThumbprint) + verifyRollback(ctx, snapshot) + cleanupSnapshot(ctx, snapshot) + parseSnapshotOutput are private methods/functions on Connector for clean test seams. Each script emits a unique `# CERTCTL_*` PowerShell comment tag so test mocks can match scripts deterministically — the snapshot/rollback/verify/cleanup scripts all reference Cert:\<store> paths, so the comment tags are the only deterministic substring under randomized map iteration. DeploymentResult shape on failure: - import OK, rollback OK → Success=false, "PowerShell import failed; rolled back" (clean recoverable failure). - import FAIL, rollback OK → same. - rollback FAIL → operator-actionable wrapped error containing both errors; metadata flags manual_action_required=true and surfaces import_error / rollback_error verbatim. Tests added to wincertstore_test.go: - TestWinCertStore_ImportFails_RemovesNewCert_RestoresOldFromSnapshot — happy rollback path with one same-Subject cert in the snapshot. Asserts rollback script contains Remove-Item for the new thumbprint AND Import-PfxCertificate referencing the snapshotted PFX path. - TestWinCertStore_ImportFails_NoExistingSameSubject_RemovesNewCertOnly — snapshot has THUMB: lines but no SNAPSHOT: entries; rollback removes the new cert but does NOT call Import-PfxCertificate. - TestWinCertStore_FriendlyNameFails_NewCertRemoved_OldCertsRestored — variant where the import script's failure originates from Set-ItemProperty FriendlyName; same rollback path. Asserts metadata.import_error preserves the FriendlyName-related PowerShell output for operator visibility. - TestWinCertStore_ImportFails_RollbackAlsoFails_OperatorActionable — wrapped-error escalation. Asserts the error mentions both "PowerShell import failed" and "rollback also failed", and metadata flags manual_action_required=true. Three existing tests (Success, ImportFailed, WithFriendlyName, WithRemoveExpired) updated to match the new contract: success path runs 3 PowerShell scripts (snapshot + import + cleanup), import-failure path runs 4 (snapshot + import + rollback + verify), and the import script lives at mock.scripts[1] not [0]. PowerShell injection note: the new cert's Subject DN is embedded in the snapshot script as a single-quoted literal. Subject DNs can contain apostrophes (e.g. CN=O'Reilly), so escapePowerShellSingleQuoted doubles them per the PowerShell single-quoted-literal escape rule. The export password and thumbprints come from certutil.GenerateRandomPassword (alphanumeric only) and the cert's SHA-1 thumbprint hex (alphanumeric); no escaping needed for those. docs/deployment-atomicity.md L93 unchanged from today's text — Bundle 1 doc-realignment hasn't shipped, so the "Get-ChildItem snapshot for rollback" line was never softened. Post-Bundle-7 the claim is honest (was aspirational pre-fix). Verified locally (sandbox lacks staticcheck install due to disk pressure; CI runs the full lint gate): - gofmt -l ./internal/connector/target/wincertstore/ clean - go vet ./internal/connector/target/wincertstore/ clean - go build ./cmd/agent/... clean - go test -race -count=1 ./internal/connector/target/wincertstore/ green Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md Bundle 7.	2026-05-02 18:13:40 +00:00
shankar0123	482c7e8047	chore(fmt): repo-wide gofmt -w sweep — close drift surfaced by ci-pipeline-cleanup Phase 4 ... Mechanical reformat. The new 'gofmt drift' CI step (added in ci-pipeline-cleanup Phase 4, commit 71b2245) surfaced 111 files with accumulated gofmt drift across cmd/, internal/, and deploy/test/. Each file's diff is gofmt-standard: whitespace adjustments, intra- group import sorting (alphabetical by import path within blank-line- separated groups), and struct-tag column alignment. No semantic changes — verified via 'git diff --ignore-all-space' which shows only the line-position deltas from import reordering. The gate stays in place after this commit. Going forward it catches gofmt drift at PR time.	2026-04-30 22:33:57 +00:00
claude	975d1850eb	feat(ssh,wincertstore,javakeystore,k8ssecret): explicit ValidateOnly + leverage existing connectors ... Phase 9 of the deploy-hardening I master bundle. The four non-file-server connectors get real ValidateOnly probes that operators use to preview a deploy without touching the live cert. Existing DeployCertificate paths already have explicit backup + rollback semantics (SCP backup / WinCertStore Get-ChildItem snapshot / keytool snapshot / K8s atomic API). SSH (validate_only.go): - Probes via SSHClient.Connect. Confirms agent reachability + credentials. Cheap (no remote command runs); released cleanly via defer Close. - A true SCP dry-run requires a no-commit upload (SCP doesn't have one). V2 ships the auth probe as the load-bearing check. - 3 new tests in validate_only_test.go. WinCertStore (validate_only.go): - Probes via PowerShell `Get-ChildItem -Path Cert:\<loc>\<store>` using the configured StoreLocation + StoreName (defaults LocalMachine\My). - Confirms agent has Windows + the IIS module + the right ACLs. - 4 new tests including default-store-path verification. JavaKeystore (validate_only.go): - Probes via `keytool -list -keystore <path> -storepass <pass>` using the configured KeystorePath / KeystorePassword and KeytoolPath (default "keytool"). - Confirms keystore exists, password is correct, JRE is on PATH. - 4 new tests covering succeeds / fails / no-path-sentinel / nil-executor-sentinel. K8s Secret (validate_only.go): - Probes via K8sClient.GetSecret on the configured Namespace + SecretName. Returns nil on success or "not found" (the CreateSecret path on Deploy will handle it). Other errors (forbidden/unreachable) surface as wrapped. - 4 new tests covering succeeds / RBAC-error wrapped / no-config-sentinel / nil-client-sentinel. Smoke test connectorsAtPhase3 list shrunk from 7 to 3 entries (ssh + wincertstore + javakeystore + k8ssecret removed). Only caddy (file-mode) + envoy + traefik remain — those three genuinely have no validate-with-target command available. Race detector clean across all 13 connectors. golangci-lint v2.11.4 clean. Phase 10 next: DeployCounters + Prometheus exposer mirroring the production-hardening-II OCSP counter pattern.	2026-04-30 15:22:17 +00:00
claude	720e773766	feat(target): ValidateOnly dry-run method on Connector interface (default returns ErrValidateOnlyNotSupported) ... Phase 3 of the deploy-hardening I master bundle. Extends the target.Connector interface with the dry-run method that operators will use to preview a deploy before committing — but ships only the default-stub for all 13 connectors. Phases 4-9 replace each stub with the real validate-with-the-target implementation. interface.go: - Add ErrValidateOnlyNotSupported sentinel (frozen decision 0.6 — connectors that cannot dry-run, like K8s, return this rather than nil so operator triage can errors.Is for "not supported" vs "validated successfully"). - Add ValidateOnly(ctx, request DeploymentRequest) error to Connector interface. 13 new validate_only.go files (one per connector at internal/connector/target/<name>/validate_only.go): - apache, caddy, envoy, f5, haproxy, iis, javakeystore, k8ssecret, nginx, postfix, ssh, traefik, wincertstore. - Each file is identical except for the package declaration: a one-method default stub returning target.ErrValidateOnlyNotSupported. - Per-connector files (rather than a single embed-method approach) let Phases 4-9 replace each connector's stub independently without churning a shared base. Tests: - internal/connector/target/validate_only_test.go pins the sentinel contract (errors.Is identity, Error() string, %w wrap propagation). - internal/connector/target/validate_only_smoke_test.go (external test package) constructs a zero-value &<pkg>.Connector{} for each of the 13 connectors and asserts ValidateOnly returns ErrValidateOnlyNotSupported. The test's connectorsAtPhase3 list is the load-bearing CI guard: - A 14th connector added without wiring ValidateOnly fails the `len(connectorsAtPhase3) != 13` invariant. - A connector whose real ValidateOnly lands (Phase 4 NGINX, Phase 5 Apache, etc.) MUST be removed from this list or the smoke test fails (real impl no longer returns the sentinel). That removal IS the bookkeeping that the operator-visible bit + behavior change are wired together end-to-end. Compile + go vet + golangci-lint v2.11.4 + go test all 0 issues. Phase 4 next: NGINX canonical real-impl — replace the stub with nginx -t -c <temp>; same time replace the existing os.WriteFile flow in DeployCertificate with deploy.Apply(...).	2026-04-30 14:40:51 +00:00
Shankar	ecee45967e	fix: resolve golangci-lint issues in wincertstore connector ... Remove unnecessary fmt.Sprintf wrapping a string literal (staticcheck S1039), remove unused tempFileForPFX function, and clean up unused os import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>	2026-04-05 19:16:34 -04:00
Shankar	e05424d188	feat(M46): Windows Certificate Store + Java Keystore target connectors, shared certutil package ... Extract shared certutil helpers (CreatePFX, ParsePrivateKey, ComputeThumbprint, GenerateRandomPassword, ParseCertificatePEM) from IIS connector for reuse. Add WinCertStore connector (PowerShell Import-PfxCertificate, dual local/WinRM mode, configurable store/location, expired cert cleanup) and JavaKeystore connector (PEM→PKCS#12→keytool pipeline, JKS/PKCS12 support, shell injection prevention, path traversal protection). 53 new tests, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>	2026-04-05 19:14:32 -04:00