From c6ffec552457220be09d5b4b0d8b9c4c61d2a31c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 18 Jun 2026 10:13:11 +0300 Subject: [PATCH] fix(security): redact upstream secrets in docker spawn argv debug logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At logging.level=debug, docker-command upstreams injected Slack/Jira tokens as `-e KEY=VALUE` flags that several argv/command log sites rendered in cleartext to main.log. Masking was inconsistent — only the shellwrap line was partially handled. Add a shared redaction helper in internal/shellwrap that masks the VALUE of docker env flags (`-e`/`--env`) while leaving keys, flags, and image names intact. It covers the two-token form (`-e`, `KEY=VALUE`), the glued single-token form (`-eKEY=VALUE`), and the embedded command-string form (the `-c` argument of a login-shell wrap), reusing secret.MaskSecretValue. Apply it at every argv/command debug log site in the spawn path: shellwrap.WrapWithUserShell (wrapped_command + original_args), connection_stdio.go (original_args/modified_args/final_args), and the process-group CommandFunc on unix + windows. Related #712 --- internal/shellwrap/redact.go | 98 ++++++++++++++ internal/shellwrap/redact_test.go | 120 ++++++++++++++++++ internal/shellwrap/shellwrap.go | 6 +- internal/upstream/core/connection_stdio.go | 10 +- internal/upstream/core/process_redact_test.go | 48 +++++++ internal/upstream/core/process_unix.go | 3 +- internal/upstream/core/process_windows.go | 3 +- 7 files changed, 279 insertions(+), 9 deletions(-) create mode 100644 internal/shellwrap/redact.go create mode 100644 internal/shellwrap/redact_test.go create mode 100644 internal/upstream/core/process_redact_test.go diff --git a/internal/shellwrap/redact.go b/internal/shellwrap/redact.go new file mode 100644 index 000000000..42cdb4519 --- /dev/null +++ b/internal/shellwrap/redact.go @@ -0,0 +1,98 @@ +package shellwrap + +import ( + "regexp" + "strings" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" +) + +// dockerEnvFlagRegex matches a docker env flag (`-e` / `--env`) followed by a +// whitespace-separated value token within a command STRING. The value token is +// either single/double-quoted (it may then contain spaces) or a run of +// non-whitespace characters. Group 1 anchors the flag to a word boundary +// (start-of-string or preceding whitespace) so it does not match inside other +// flags or image names. +var dockerEnvFlagRegex = regexp.MustCompile(`(^|\s)(--env|-e)(\s+)('[^']*'|"[^"]*"|\S+)`) + +// RedactDockerArgs returns a copy of a docker-style argv slice with the VALUE +// of every `KEY=VALUE` env injection masked, leaving keys, flags, image names, +// and other args untouched. It handles three shapes the spawn path produces: +// +// - two-token form: ["-e", "KEY=VALUE"] (direct-exec docker) +// - glued single tok: ["-eKEY=VALUE"] / ["--env=KEY=VALUE"] +// - command-string el: ["-l", "-c", "docker run -e KEY=VALUE img"] (shell wrap) +// +// The input slice is never mutated. A nil input returns nil. +func RedactDockerArgs(args []string) []string { + if args == nil { + return nil + } + out := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + a := args[i] + // Two-token form: `-e`/`--env` followed by `KEY=VALUE`. + if (a == "-e" || a == "--env") && i+1 < len(args) { + out = append(out, a, maskEnvToken(args[i+1])) + i++ + continue + } + out = append(out, redactArgElement(a)) + } + return out +} + +// redactArgElement masks env secrets inside a single argv element, covering the +// glued single-token forms and the embedded command-string form. +func redactArgElement(a string) string { + // Command-string element (e.g. the `-c` argument of a login-shell wrap): + // mask any embedded `-e KEY=VALUE` occurrences. + if strings.ContainsAny(a, " \t") { + return RedactDockerCommandString(a) + } + // Glued long form: `--env=KEY=VALUE`. + if rest, ok := strings.CutPrefix(a, "--env="); ok { + return "--env=" + maskEnvToken(rest) + } + // Glued short form: `-eKEY=VALUE`. + if strings.HasPrefix(a, "-e") && len(a) > 2 && strings.Contains(a[2:], "=") { + return "-e" + maskEnvToken(a[2:]) + } + return a +} + +// RedactDockerCommandString masks the VALUE of any `-e KEY=VALUE` / +// `--env KEY=VALUE` flag found within a single command string, leaving the rest +// of the string (subcommand, flags, image name) intact. +func RedactDockerCommandString(s string) string { + return dockerEnvFlagRegex.ReplaceAllStringFunc(s, func(m string) string { + sub := dockerEnvFlagRegex.FindStringSubmatch(m) + // sub[1]=lead, sub[2]=flag, sub[3]=whitespace, sub[4]=value token + return sub[1] + sub[2] + sub[3] + maskEnvToken(sub[4]) + }) +} + +// maskEnvToken masks the value portion of a `KEY=VALUE` token, preserving the +// key and any surrounding single/double quotes. Tokens without a `=` or with an +// empty value are returned unchanged (they are not secret-bearing env values). +func maskEnvToken(tok string) string { + quote := "" + inner := tok + if len(inner) >= 2 { + if (inner[0] == '\'' && inner[len(inner)-1] == '\'') || + (inner[0] == '"' && inner[len(inner)-1] == '"') { + quote = string(inner[0]) + inner = inner[1 : len(inner)-1] + } + } + eq := strings.IndexByte(inner, '=') + if eq < 0 { + return tok + } + key := inner[:eq] + val := inner[eq+1:] + if val == "" { + return tok + } + return quote + key + "=" + secret.MaskSecretValue(val) + quote +} diff --git a/internal/shellwrap/redact_test.go b/internal/shellwrap/redact_test.go new file mode 100644 index 000000000..2a65a651e --- /dev/null +++ b/internal/shellwrap/redact_test.go @@ -0,0 +1,120 @@ +package shellwrap + +import ( + "fmt" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +const ( + secretValueSlack = "xoxc-abc123456789" + secretValueJira = "xoxd-zzz987654321" +) + +// assertNoSecret fails if s contains any of the cleartext secret values that +// must never reach a log. +func assertNoSecret(t *testing.T, s string) { + t.Helper() + for _, sv := range []string{secretValueSlack, secretValueJira} { + if strings.Contains(s, sv) { + t.Errorf("cleartext secret %q leaked in %q", sv, s) + } + } +} + +func TestRedactDockerArgs_TwoTokenForm(t *testing.T) { + in := []string{"run", "--rm", "-i", "-e", "SLACK_TOKEN=" + secretValueSlack, "myimage:latest"} + out := RedactDockerArgs(in) + + joined := strings.Join(out, " ") + assertNoSecret(t, joined) + + // Non-secret args untouched. + for _, want := range []string{"run", "--rm", "-i", "myimage:latest"} { + if !contains(out, want) { + t.Errorf("expected non-secret arg %q to be preserved, got %v", want, out) + } + } + // Key and flag preserved. + if !strings.Contains(joined, "-e") || !strings.Contains(joined, "SLACK_TOKEN=") { + t.Errorf("expected -e and key SLACK_TOKEN= preserved, got %q", joined) + } + // Input slice must not be mutated. + if in[4] != "SLACK_TOKEN="+secretValueSlack { + t.Errorf("RedactDockerArgs mutated its input slice: %q", in[4]) + } +} + +func TestRedactDockerArgs_SingleGluedToken(t *testing.T) { + in := []string{"run", "-eSLACK_TOKEN=" + secretValueSlack, "myimage"} + out := RedactDockerArgs(in) + joined := strings.Join(out, " ") + assertNoSecret(t, joined) + if !strings.Contains(joined, "SLACK_TOKEN=") { + t.Errorf("expected key preserved, got %q", joined) + } +} + +func TestRedactDockerArgs_CommandStringElement(t *testing.T) { + in := []string{"-l", "-c", "docker run --rm -e SLACK_TOKEN=" + secretValueSlack + " -e JIRA_TOKEN=" + secretValueJira + " myimage:latest"} + out := RedactDockerArgs(in) + joined := strings.Join(out, " ") + assertNoSecret(t, joined) + for _, want := range []string{"-l", "docker run", "--rm", "myimage:latest", "SLACK_TOKEN=", "JIRA_TOKEN="} { + if !strings.Contains(joined, want) { + t.Errorf("expected %q preserved in %q", want, joined) + } + } +} + +func TestRedactDockerCommandString(t *testing.T) { + in := "docker run --rm -e SLACK_TOKEN=" + secretValueSlack + " --env JIRA_TOKEN=" + secretValueJira + " myimage" + out := RedactDockerCommandString(in) + assertNoSecret(t, out) + for _, want := range []string{"docker run", "--rm", "myimage", "SLACK_TOKEN=", "JIRA_TOKEN="} { + if !strings.Contains(out, want) { + t.Errorf("expected %q preserved in %q", want, out) + } + } +} + +func TestRedactDockerArgs_NoEnvFlagsUnchanged(t *testing.T) { + in := []string{"run", "--rm", "-i", "ghcr.io/foo/bar:latest", "--config", "/etc/x.json"} + out := RedactDockerArgs(in) + if strings.Join(in, " ") != strings.Join(out, " ") { + t.Errorf("non-env args were altered: in=%v out=%v", in, out) + } +} + +// Guard test: the WrapWithUserShell debug log must not emit cleartext secrets +// in its wrapped_command / original_args fields. +func TestWrapWithUserShell_LogsAreRedacted(t *testing.T) { + obsCore, recorded := observer.New(zapcore.DebugLevel) + logger := zap.New(obsCore) + + WrapWithUserShell(logger, "docker", []string{"run", "--rm", "-e", "SLACK_TOKEN=" + secretValueSlack, "myimage"}) + + if recorded.Len() == 0 { + t.Fatal("expected a debug log entry from WrapWithUserShell") + } + for _, entry := range recorded.All() { + for k, v := range entry.ContextMap() { + assertNoSecret(t, k) + assertNoSecret(t, fmt.Sprintf("%v", v)) + } + assertNoSecret(t, entry.Message) + } +} + +func contains(ss []string, want string) bool { + for _, s := range ss { + if s == want { + return true + } + } + return false +} diff --git a/internal/shellwrap/shellwrap.go b/internal/shellwrap/shellwrap.go index 0598ae670..0c5f2c556 100644 --- a/internal/shellwrap/shellwrap.go +++ b/internal/shellwrap/shellwrap.go @@ -106,11 +106,13 @@ func WrapWithUserShell(logger *zap.Logger, command string, args []string) (shell commandString := strings.Join(parts, " ") if logger != nil { + // Redact secret env values (`-e KEY=VALUE`) before logging — docker-command + // upstreams inject Slack/Jira tokens here and debug logs are written to disk. logger.Debug("shellwrap: wrapping command with user login shell", zap.String("original_command", command), - zap.Strings("original_args", args), + zap.Strings("original_args", RedactDockerArgs(args)), zap.String("shell", shell), - zap.String("wrapped_command", commandString)) + zap.String("wrapped_command", RedactDockerCommandString(commandString))) } isBash := isBashLikeShell(shell) diff --git a/internal/upstream/core/connection_stdio.go b/internal/upstream/core/connection_stdio.go index 6e4ac0a71..095d0ed56 100644 --- a/internal/upstream/core/connection_stdio.go +++ b/internal/upstream/core/connection_stdio.go @@ -102,7 +102,7 @@ func (c *Client) connectStdio(ctx context.Context) error { c.logger.Debug("Docker command detected, setting up container ID tracking", zap.String("server", c.config.Name), zap.String("command", c.config.Command), - zap.Strings("original_args", args)) + zap.Strings("original_args", shellwrap.RedactDockerArgs(args))) // CRITICAL: Clean up any existing containers first to prevent duplicates // This makes container creation idempotent and safe to call multiple times @@ -166,7 +166,7 @@ func (c *Client) connectStdio(ctx context.Context) error { c.logger.Debug("Injected env vars into direct docker command", zap.String("server", c.config.Name), zap.Int("env_count", len(c.config.Env)), - zap.Strings("modified_args", argsToWrap)) + zap.Strings("modified_args", shellwrap.RedactDockerArgs(argsToWrap))) } // Use shell wrapping for environment inheritance @@ -205,9 +205,9 @@ func (c *Client) connectStdio(ctx context.Context) error { c.logger.Debug("Initialized stdio transport", zap.String("server", c.config.Name), zap.String("final_command", finalCommand), - zap.Strings("final_args", finalArgs), + zap.Strings("final_args", shellwrap.RedactDockerArgs(finalArgs)), zap.String("original_command", c.config.Command), - zap.Strings("original_args", args), + zap.Strings("original_args", shellwrap.RedactDockerArgs(args)), zap.String("working_dir", c.config.WorkingDir), zap.Bool("docker_isolation", c.isDockerCommand)) @@ -392,7 +392,7 @@ func (c *Client) wrapWithUserShell(command string, args []string) (shellCommand c.logger.Debug("Wrapping command with user shell for full environment inheritance", zap.String("server", c.config.Name), zap.String("original_command", command), - zap.Strings("original_args", args), + zap.Strings("original_args", shellwrap.RedactDockerArgs(args)), zap.String("shell", shellCommand)) return shellCommand, shellArgs } diff --git a/internal/upstream/core/process_redact_test.go b/internal/upstream/core/process_redact_test.go new file mode 100644 index 000000000..e613da5f8 --- /dev/null +++ b/internal/upstream/core/process_redact_test.go @@ -0,0 +1,48 @@ +//go:build unix + +package core + +import ( + "context" + "fmt" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +// TestProcessGroupCommandFunc_RedactsSecretArgs is a guard test for GH #712: +// the "Process group configuration applied" debug log must not emit cleartext +// upstream secrets passed as `-e KEY=VALUE` to docker-command upstreams. +func TestProcessGroupCommandFunc_RedactsSecretArgs(t *testing.T) { + const secretValue = "xoxc-abc123456789" + + obsCore, recorded := observer.New(zapcore.DebugLevel) + logger := zap.New(obsCore) + + // client=nil keeps this off the spawn path: the CommandFunc builds the + // *exec.Cmd and logs but never starts a process. + fn := createProcessGroupCommandFunc(nil, "", logger) + + cases := [][]string{ + // Shell-wrapped form (secret embedded in the -c command string). + {"-l", "-c", "docker run --rm -e SLACK_TOKEN=" + secretValue + " myimage"}, + // Direct-exec form (two-token -e KEY=VALUE). + {"run", "--rm", "-e", "SLACK_TOKEN=" + secretValue, "myimage"}, + } + for _, args := range cases { + if _, err := fn(context.Background(), "/bin/sh", nil, args); err != nil { + t.Fatalf("CommandFunc returned error: %v", err) + } + } + + for _, entry := range recorded.All() { + for _, v := range entry.ContextMap() { + if strings.Contains(fmt.Sprintf("%v", v), secretValue) { + t.Errorf("cleartext secret leaked in log field: %v", v) + } + } + } +} diff --git a/internal/upstream/core/process_unix.go b/internal/upstream/core/process_unix.go index c115859b0..282d3cffc 100644 --- a/internal/upstream/core/process_unix.go +++ b/internal/upstream/core/process_unix.go @@ -8,6 +8,7 @@ import ( "syscall" "time" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" "go.uber.org/zap" ) @@ -39,7 +40,7 @@ func createProcessGroupCommandFunc(client *Client, workingDir string, logger *za logger.Debug("Process group configuration applied", zap.String("command", command), - zap.Strings("args", args), + zap.Strings("args", shellwrap.RedactDockerArgs(args)), zap.String("working_dir", workingDir)) if client != nil { diff --git a/internal/upstream/core/process_windows.go b/internal/upstream/core/process_windows.go index 51dfb773a..873fc9b0e 100644 --- a/internal/upstream/core/process_windows.go +++ b/internal/upstream/core/process_windows.go @@ -6,6 +6,7 @@ import ( "context" "os/exec" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" "go.uber.org/zap" ) @@ -33,7 +34,7 @@ func createProcessGroupCommandFunc(client *Client, workingDir string, logger *za logger.Debug("Process group configuration applied (Windows)", zap.String("command", command), - zap.Strings("args", args), + zap.Strings("args", shellwrap.RedactDockerArgs(args)), zap.String("working_dir", workingDir)) if client != nil {