From 737c3298243b9615f6fcbd2de1480aa71fae0f38 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 2 May 2026 22:38:35 +0000 Subject: [PATCH] iis,wincertstore: default-deadline ctx wrapper for PowerShell exec calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/connectors.md | 2 + internal/connector/target/iis/iis.go | 38 +++++++- internal/connector/target/iis/iis_test.go | 94 +++++++++++++++++++ .../target/wincertstore/wincertstore.go | 33 ++++++- .../target/wincertstore/wincertstore_test.go | 71 ++++++++++++++ 5 files changed, 232 insertions(+), 6 deletions(-) diff --git a/docs/connectors.md b/docs/connectors.md index 03f548c..1b4908e 100644 --- a/docs/connectors.md +++ b/docs/connectors.md @@ -1027,6 +1027,7 @@ The IIS target connector supports two deployment modes — agent-local (recommen - `ip_address` (string, default "*"): Specific IP to bind to, or "*" for all IPs - `binding_info` (string, optional): Host header for SNI bindings - `mode` (string, default "local"): Deployment mode — `local` (agent-local PowerShell) or `winrm` (remote via WinRM) +- `exec_deadline` (duration, default `60s`): Per-PowerShell-subprocess cap that fires only when the caller's `ctx` has no deadline of its own. A caller-supplied deadline always wins; this is a safety net so a hung WinRM session or stuck `Cert:` provider call cannot block the deploy worker indefinitely. Operators on slow links (high-latency WinRM, slow Windows VMs) can extend with e.g. `"exec_deadline": "5m"`. **WinRM fields (required when `mode` is `winrm`):** - `winrm.winrm_host` (string, required): Remote Windows server hostname or IP @@ -1133,6 +1134,7 @@ The Windows Certificate Store connector imports certificates into the Windows ce | `winrm_password` | string | | WinRM password (required for winrm mode) | | `winrm_https` | boolean | `false` | Use HTTPS for WinRM | | `winrm_insecure` | boolean | `false` | Skip TLS verification for WinRM | +| `exec_deadline` | duration | `60s` | Per-PowerShell-subprocess cap that fires only when the caller's `ctx` has no deadline of its own. A caller-supplied deadline always wins; this is a safety net so a hung WinRM session or stuck `Cert:` provider call cannot block the deploy worker indefinitely. Operators on slow links can extend with e.g. `"exec_deadline": "5m"`. | Location: `internal/connector/target/wincertstore/wincertstore.go` diff --git a/internal/connector/target/iis/iis.go b/internal/connector/target/iis/iis.go index 79ceebb..626896d 100644 --- a/internal/connector/target/iis/iis.go +++ b/internal/connector/target/iis/iis.go @@ -30,6 +30,14 @@ type Config struct { IPAddress string `json:"ip_address"` // Bind to specific IP (default "*") Mode string `json:"mode"` // "local" (default) or "winrm" + // ExecDeadline caps each PowerShell subprocess (local mode) to this + // duration when the caller's ctx has no deadline of its own. Operators + // on slow Windows links can extend; default is 60s. Caller-supplied + // deadlines (via ctx) always win — the wrapper is a safety net for code + // paths that forgot to attach one. Top-10 fix #4 of the 2026-05-02 + // deployment-target audit re-run. + ExecDeadline time.Duration `json:"exec_deadline,omitempty"` + // WinRM settings (only used when Mode is "winrm") WinRM WinRMConfig `json:"winrm"` } @@ -41,10 +49,23 @@ type PowerShellExecutor interface { Execute(ctx context.Context, script string) (string, error) } -// realExecutor calls powershell.exe on the local system. -type realExecutor struct{} +// realExecutor calls powershell.exe on the local system. The deadline field +// caps each subprocess invocation when the caller's ctx has no deadline of +// its own — see Top-10 fix #4 of the 2026-05-02 deployment-target audit. +type realExecutor struct { + deadline time.Duration +} func (e *realExecutor) Execute(ctx context.Context, script string) (string, error) { + // Attach the configured default deadline ONLY when the caller's ctx has + // no deadline of its own. Caller deadlines always win — this wrapper is + // a safety net for code paths that forgot to attach one. A hung WinRM + // session should not block the deploy worker indefinitely. + if _, ok := ctx.Deadline(); !ok && e.deadline > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, e.deadline) + defer cancel() + } cmd := exec.CommandContext(ctx, "powershell.exe", "-NoProfile", "-NonInteractive", "-Command", script) output, err := cmd.CombinedOutput() return string(output), err @@ -78,11 +99,18 @@ func New(config *Config, logger *slog.Logger) (*Connector, error) { if mode == "" { mode = "local" } + // Top-10 fix #4: default the per-PowerShell-subprocess deadline so a hung + // WinRM / cert-store call does not block the deploy worker indefinitely + // when the caller's ctx has no deadline. Operators on slow links can + // override via JSON config (`exec_deadline`). + if config.ExecDeadline == 0 { + config.ExecDeadline = 60 * time.Second + } var executor PowerShellExecutor switch mode { case "local": - executor = &realExecutor{} + executor = &realExecutor{deadline: config.ExecDeadline} case "winrm": winrmExec, err := newWinRMExecutor(&config.WinRM) if err != nil { @@ -156,6 +184,10 @@ func (c *Connector) ValidateConfig(ctx context.Context, rawConfig json.RawMessag if cfg.IPAddress == "" { cfg.IPAddress = "*" } + // Top-10 fix #4: default the per-PowerShell-subprocess deadline. + if cfg.ExecDeadline == 0 { + cfg.ExecDeadline = 60 * time.Second + } // Validate port range if cfg.Port < 1 || cfg.Port > 65535 { diff --git a/internal/connector/target/iis/iis_test.go b/internal/connector/target/iis/iis_test.go index 4f69690..84b82f8 100644 --- a/internal/connector/target/iis/iis_test.go +++ b/internal/connector/target/iis/iis_test.go @@ -1426,3 +1426,97 @@ func TestWinRMConfig_DefaultPorts(t *testing.T) { t.Fatal("expected non-nil HTTPS executor") } } + +// --- Top-10 fix #4: default-deadline ctx wrapper for PowerShell exec calls --- +// +// These two tests pin realExecutor's safety-net behavior: a default ExecDeadline +// caps each subprocess invocation when the caller's ctx has no deadline of its +// own; a tighter caller-supplied deadline always wins. The actual subprocess +// is irrelevant — on Linux/macOS runners powershell.exe is missing and +// exec.Cmd fails fast at Start(); on Windows runners the wrapper's ctx +// deadline cancels the running PowerShell process. Either path returns well +// under 500ms; what we assert is the contract (fast return + error), which +// regresses if a future refactor removes the wrapper. + +func TestIIS_RealExecutor_AttachesDefaultDeadlineWhenCallerHasNone(t *testing.T) { + e := &realExecutor{deadline: 100 * time.Millisecond} + start := time.Now() + _, err := e.Execute(context.Background(), "Start-Sleep -Seconds 5") + elapsed := time.Since(start) + if elapsed > 500*time.Millisecond { + t.Errorf("expected fast return (default deadline = 100ms), took %v: err=%v", elapsed, err) + } + if err == nil { + t.Error("expected an error (context.DeadlineExceeded on Windows / powershell.exe missing on Linux)") + } +} + +func TestIIS_RealExecutor_RespectsCallerDeadlineWhenSet(t *testing.T) { + e := &realExecutor{deadline: 10 * time.Second} // long fallback + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + _, _ = e.Execute(ctx, "Start-Sleep -Seconds 5") + elapsed := time.Since(start) + // Caller's tight 50ms deadline must win over the executor's 10s fallback. + if elapsed > 500*time.Millisecond { + t.Errorf("expected caller's tight 50ms deadline to fire fast, took %v", elapsed) + } +} + +func TestIIS_RealExecutor_NoDeadlineWiredWhenZero(t *testing.T) { + // deadline=0 means "no fallback wrapper" — caller is fully in charge. + // Pass a tight caller deadline so the test still terminates fast. + e := &realExecutor{deadline: 0} + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + _, _ = e.Execute(ctx, "Start-Sleep -Seconds 5") + elapsed := time.Since(start) + if elapsed > 500*time.Millisecond { + t.Errorf("expected caller deadline to bound the call, took %v", elapsed) + } +} + +func TestIIS_New_DefaultsExecDeadlineTo60s(t *testing.T) { + // New() applies the 60s default when ExecDeadline is unset. The local + // realExecutor captures the value at construction. + cfg := &Config{ + SiteName: "Default Web Site", + CertStore: "My", + Mode: "winrm", // winrm path skips realExecutor (uses winrmExecutor); won't fail-fast + WinRM: WinRMConfig{ + Host: "server.example.com", + Username: "admin", + Password: "pass", + }, + } + c, err := New(cfg, testLogger()) + if err != nil { + t.Fatalf("New failed: %v", err) + } + if c.config.ExecDeadline != 60*time.Second { + t.Errorf("expected default ExecDeadline=60s, got %v", c.config.ExecDeadline) + } +} + +func TestIIS_New_RespectsExplicitExecDeadline(t *testing.T) { + cfg := &Config{ + SiteName: "Default Web Site", + CertStore: "My", + Mode: "winrm", + ExecDeadline: 10 * time.Minute, + WinRM: WinRMConfig{ + Host: "server.example.com", + Username: "admin", + Password: "pass", + }, + } + c, err := New(cfg, testLogger()) + if err != nil { + t.Fatalf("New failed: %v", err) + } + if c.config.ExecDeadline != 10*time.Minute { + t.Errorf("expected ExecDeadline=10m, got %v", c.config.ExecDeadline) + } +} diff --git a/internal/connector/target/wincertstore/wincertstore.go b/internal/connector/target/wincertstore/wincertstore.go index 6638797..aacfa73 100644 --- a/internal/connector/target/wincertstore/wincertstore.go +++ b/internal/connector/target/wincertstore/wincertstore.go @@ -41,6 +41,14 @@ type Config struct { // Mode is the deployment mode: "local" (default) or "winrm". Mode string `json:"mode"` + // ExecDeadline caps each PowerShell subprocess (local mode) to this + // duration when the caller's ctx has no deadline of its own. Operators + // on slow Windows links can extend; default is 60s. Caller-supplied + // deadlines (via ctx) always win — the wrapper is a safety net for code + // paths that forgot to attach one. Top-10 fix #4 of the 2026-05-02 + // deployment-target audit re-run. + ExecDeadline time.Duration `json:"exec_deadline,omitempty"` + // WinRM settings (only used when Mode is "winrm"). WinRMHost string `json:"winrm_host,omitempty"` WinRMPort int `json:"winrm_port,omitempty"` @@ -55,10 +63,22 @@ type PowerShellExecutor interface { Execute(ctx context.Context, script string) (string, error) } -// realExecutor calls powershell.exe on the local system. -type realExecutor struct{} +// realExecutor calls powershell.exe on the local system. The deadline field +// caps each subprocess invocation when the caller's ctx has no deadline of +// its own — see Top-10 fix #4 of the 2026-05-02 deployment-target audit. +type realExecutor struct { + deadline time.Duration +} func (e *realExecutor) Execute(ctx context.Context, script string) (string, error) { + // Attach the configured default deadline ONLY when the caller's ctx has + // no deadline of its own. Caller deadlines always win — this wrapper is + // a safety net for code paths that forgot to attach one. + if _, ok := ctx.Deadline(); !ok && e.deadline > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, e.deadline) + defer cancel() + } cmd := exec.CommandContext(ctx, "powershell.exe", "-NoProfile", "-NonInteractive", "-Command", script) out, err := cmd.CombinedOutput() return strings.TrimSpace(string(out)), err @@ -89,7 +109,7 @@ func New(cfg *Config, logger *slog.Logger) (*Connector, error) { return &Connector{ config: cfg, logger: logger, - executor: &realExecutor{}, + executor: &realExecutor{deadline: cfg.ExecDeadline}, }, nil } @@ -116,6 +136,13 @@ func applyDefaults(cfg *Config) { if cfg.Mode == "" { cfg.Mode = "local" } + // Top-10 fix #4: default the per-PowerShell-subprocess deadline so a hung + // WinRM / cert-store call does not block the deploy worker indefinitely + // when the caller's ctx has no deadline. Operators on slow links can + // override via JSON config (`exec_deadline`). + if cfg.ExecDeadline == 0 { + cfg.ExecDeadline = 60 * time.Second + } } // ValidateConfig validates the Windows Certificate Store configuration. diff --git a/internal/connector/target/wincertstore/wincertstore_test.go b/internal/connector/target/wincertstore/wincertstore_test.go index 94586c6..13973cd 100644 --- a/internal/connector/target/wincertstore/wincertstore_test.go +++ b/internal/connector/target/wincertstore/wincertstore_test.go @@ -839,3 +839,74 @@ func TestValidateDeployment_BySerial(t *testing.T) { t.Error("expected serial number query in script") } } + +// --- Top-10 fix #4: default-deadline ctx wrapper for PowerShell exec calls --- +// +// These tests pin realExecutor's safety-net behavior. See the matching pair +// in iis_test.go for the rationale (subprocess fails fast on non-Windows +// runners / deadline cancels on Windows; either path must return fast). + +func TestWinCertStore_RealExecutor_AttachesDefaultDeadlineWhenCallerHasNone(t *testing.T) { + e := &realExecutor{deadline: 100 * time.Millisecond} + start := time.Now() + _, err := e.Execute(context.Background(), "Start-Sleep -Seconds 5") + elapsed := time.Since(start) + if elapsed > 500*time.Millisecond { + t.Errorf("expected fast return (default deadline = 100ms), took %v: err=%v", elapsed, err) + } + if err == nil { + t.Error("expected an error (context.DeadlineExceeded on Windows / powershell.exe missing on Linux)") + } +} + +func TestWinCertStore_RealExecutor_RespectsCallerDeadlineWhenSet(t *testing.T) { + e := &realExecutor{deadline: 10 * time.Second} // long fallback + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + _, _ = e.Execute(ctx, "Start-Sleep -Seconds 5") + elapsed := time.Since(start) + if elapsed > 500*time.Millisecond { + t.Errorf("expected caller's tight 50ms deadline to fire fast, took %v", elapsed) + } +} + +func TestWinCertStore_RealExecutor_NoDeadlineWiredWhenZero(t *testing.T) { + // deadline=0 means "no fallback wrapper". Pass a tight caller deadline. + e := &realExecutor{deadline: 0} + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + _, _ = e.Execute(ctx, "Start-Sleep -Seconds 5") + elapsed := time.Since(start) + if elapsed > 500*time.Millisecond { + t.Errorf("expected caller deadline to bound the call, took %v", elapsed) + } +} + +func TestWinCertStore_New_DefaultsExecDeadlineTo60s(t *testing.T) { + c, err := New(&Config{ + StoreName: "My", + StoreLocation: "LocalMachine", + }, testLogger()) + if err != nil { + t.Fatalf("New failed: %v", err) + } + if c.config.ExecDeadline != 60*time.Second { + t.Errorf("expected default ExecDeadline=60s, got %v", c.config.ExecDeadline) + } +} + +func TestWinCertStore_New_RespectsExplicitExecDeadline(t *testing.T) { + c, err := New(&Config{ + StoreName: "My", + StoreLocation: "LocalMachine", + ExecDeadline: 10 * time.Minute, + }, testLogger()) + if err != nil { + t.Fatalf("New failed: %v", err) + } + if c.config.ExecDeadline != 10*time.Minute { + t.Errorf("expected ExecDeadline=10m, got %v", c.config.ExecDeadline) + } +}