mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 16:01:30 +00:00
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.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user