diff --git a/docs/docker-isolation.md b/docs/docker-isolation.md index 82381a101..a8bcbc481 100644 --- a/docs/docker-isolation.md +++ b/docs/docker-isolation.md @@ -321,7 +321,17 @@ Environment variables from server configuration are automatically passed to cont } ``` -These become Docker arguments: `-e API_KEY=your-secret-key -e DEBUG=true` +These become key-only Docker arguments, `-e API_KEY -e DEBUG`, and the values are set in the +environment of the `docker` process, which reads them from there. The same applies to a server +whose `command` is `docker run …` itself. A value never appears in the `docker` command line, so +`ps aux` and `/proc//cmdline`, which every local user can read, do not show it +([#1809](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/1809)). It is still visible to the +process owner and root in `/proc//environ` of the `docker` client, and inside the container +(`docker inspect`), as any container environment variable is. `PATH` is the one value still passed +inline (`-e PATH=…`), so a configured container `PATH` is exact; it is not a secret. Whenever +`docker` is started through your login shell (it could not be resolved to an absolute path, or on +Linux with neither `DOCKER_HOST` nor `DOCKER_CONTEXT` exported), a profile that exports or unsets +one of these names before `docker` runs wins over the configured value. ## Docker-in-Docker Prevention diff --git a/docs/features/keyring-integration.md b/docs/features/keyring-integration.md index 9fcc0ff99..f579980cd 100644 --- a/docs/features/keyring-integration.md +++ b/docs/features/keyring-integration.md @@ -50,8 +50,12 @@ MCPProxy supports three types of secret references that can be used anywhere in ### File references -`${file:/absolute/path}` keeps a secret out of the config file and out of the process environment -(`docker inspect`, `/proc//environ`). The same rules apply wherever it is used: +`${file:/absolute/path}` keeps a secret out of the config file and out of mcpproxy's own process +environment. A reference used in a server's `env` is resolved when the server connects and the value +is passed to that server as an environment variable, which for a Docker server also means the +container's environment (`docker inspect`) and the `docker` client's `/proc//environ`; it is +never put on the `docker` command line ([#1809](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/1809)). +The same rules apply wherever it is used: - The path must be absolute. A symlink is followed (Kubernetes secret volumes are symlinks) but must point at a regular file. On Windows an absolute path has a drive letter (`C:\ProgramData\mcpproxy\key` diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 257c55a21..a5e87bfe5 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -1,10 +1,10 @@ package core import ( - "fmt" "os" "path/filepath" "runtime" + "sort" "strings" "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" @@ -324,7 +324,13 @@ func (c *Client) insertCidfileIntoDockerArgs(args []string, cidFile string) []st // injectEnvVarsIntoDockerArgs injects environment variables as -e flags into Docker run args // The flags are inserted after "run" but before the image name -// Example: ["run", "-i", "--rm", "image"] -> ["run", "-e", "KEY=VAL", "-i", "--rm", "image"] +// Example: ["run", "-i", "--rm", "image"] -> ["run", "-e", "KEY", "-i", "--rm", "image"] +// +// Only the KEY is placed in argv (#1809). Docker's `-e KEY` form reads the +// value from the docker CLI's own environment, which the spawn paths populate +// from config.Env (connection_stdio.go / connection_launcher.go env merge). +// A `-e KEY=VALUE` token would land in the docker process's argv, where +// `ps aux` and /proc//cmdline expose resolved secrets to every local user. func (c *Client) injectEnvVarsIntoDockerArgs(args []string, envVars map[string]string) []string { if len(args) < 2 || args[0] != cmdRun { return args @@ -333,11 +339,7 @@ func (c *Client) injectEnvVarsIntoDockerArgs(args []string, envVars map[string]s // Build new args with env vars injected after "run" newArgs := make([]string, 0, len(args)+len(envVars)*2) newArgs = append(newArgs, args[0]) // "run" - - // Add -e flags for each env var - for key, value := range envVars { - newArgs = append(newArgs, "-e", fmt.Sprintf("%s=%s", key, value)) - } + newArgs = append(newArgs, dockerEnvKeyFlags(envVars)...) // Add remaining args (flags and image name) newArgs = append(newArgs, args[1:]...) @@ -345,6 +347,39 @@ func (c *Client) injectEnvVarsIntoDockerArgs(args []string, envVars map[string]s return newArgs } +// dockerEnvKeyFlags renders config.Env as key-only `-e KEY` docker flags, in +// sorted key order so argv and its log line are deterministic. The values +// travel in the docker process environment, never in argv (#1809). +// +// PATH is the one exception: the spawn paths rewrite the docker client's PATH +// after the config.Env merge (prependDockerDirToPath, #715), so `-e PATH` would +// forward that host-side value into the container instead of the configured +// one. PATH is never a secret, so it is inlined as `-e PATH=`. The match +// is case-insensitive (Windows treats `Path` and `PATH` as one variable) and +// keeps the configured spelling. +// +// Whenever docker is started through the user's login shell (resolveDockerSpawn: +// docker not resolvable to a verified absolute path, or a non-Darwin host with +// neither DOCKER_HOST nor DOCKER_CONTEXT exported) a profile that exports or +// unsets one of these names before docker runs wins over the configured value; +// the direct-exec path has no such step. +func dockerEnvKeyFlags(envVars map[string]string) []string { + keys := make([]string, 0, len(envVars)) + for key := range envVars { + keys = append(keys, key) + } + sort.Strings(keys) + flags := make([]string, 0, len(keys)*2) + for _, key := range keys { + if strings.EqualFold(key, "PATH") { + flags = append(flags, "-e", key+"="+envVars[key]) + continue + } + flags = append(flags, "-e", key) + } + return flags +} + // insertCidfileIntoShellDockerCommand inserts --cidfile into a shell-wrapped Docker command func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile string) []string { // Shell args look like: diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go index 9b2d91bbc..c8a27bba8 100644 --- a/internal/upstream/core/connection_docker_test.go +++ b/internal/upstream/core/connection_docker_test.go @@ -320,7 +320,7 @@ func TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv(t *testing.T) { // isolation-injection path. With docker resolved to a verified absolute // executable and the daemon env guaranteed (macOS), the spawn must direct-exec // the ABSOLUTE docker path (no `$SHELL -l -c` wrap), carry the injected -// `-e KEY=VALUE` env flags, and accept an args-based cidfile insertion. Before +// key-only `-e KEY` env flags (#1809), and accept an args-based cidfile insertion. Before // this fix the user path shell-wrapped bare `docker`, producing GH #696's // `zsh:1: command not found: docker` on a default Docker Desktop macOS install // (docker CLI only inside the app bundle, not on the login-shell PATH). @@ -346,8 +346,8 @@ func TestResolveDockerSpawnUserSuppliedDockerRunDirectExec(t *testing.T) { assert.Equal(t, cmdRun, args[0], "first arg must be the raw 'run' token (not a shell -c string), got: %v", args) // The injected -e env flag must survive into the docker run argv. - assert.Subset(t, args, []string{"-e", "SLACK_TOKEN=xoxb-secret"}, - "injected -e env flags must be present in the direct-exec argv, got: %v", args) + assert.Subset(t, args, []string{"-e", "SLACK_TOKEN"}, + "injected key-only -e env flags must be present in the direct-exec argv, got: %v", args) for _, a := range args { assert.NotContains(t, a, " run ", "args must be raw argv tokens, not a single shell command string, got: %v", args) @@ -383,10 +383,10 @@ func TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap(t *testing.T) { cmdStr := shellArgs[len(shellArgs)-1] assert.True(t, strings.HasPrefix(cmdStr, "docker run"), "on resolution failure the spawn must fall back to bare 'docker', got: %s", cmdStr) - // The real command string still carries the actual env value (redaction is a - // log-only concern, not a spawn concern). - assert.Contains(t, cmdStr, "-e SLACK_TOKEN=xoxb-secret", - "injected -e env flag must survive into the shell command string, got: %s", cmdStr) + // The command string carries the key only; the value reaches docker through + // the spawn environment (#1809). + assert.Contains(t, cmdStr, "-e SLACK_TOKEN ", + "injected key-only -e env flag must survive into the shell command string, got: %s", cmdStr) } // TestDockerDaemonEnvGuaranteed unit-tests the gate directly. diff --git a/internal/upstream/core/connection_launcher_test.go b/internal/upstream/core/connection_launcher_test.go index 404b75a88..b58435715 100644 --- a/internal/upstream/core/connection_launcher_test.go +++ b/internal/upstream/core/connection_launcher_test.go @@ -47,8 +47,8 @@ func TestBuildLauncherCmdUserSuppliedDockerRunDirectExec(t *testing.T) { "second argv token must be the raw 'run' token, not a shell -c string, got: %v", cmd.Args) // Injected -e env flag must survive into the docker run argv. - assert.Subset(t, cmd.Args, []string{"-e", "SLACK_TOKEN=xoxb-secret"}, - "injected -e env flags must be present in the direct-exec argv, got: %v", cmd.Args) + assert.Subset(t, cmd.Args, []string{"-e", "SLACK_TOKEN"}, + "injected key-only -e env flags must be present in the direct-exec argv, got: %v", cmd.Args) // cidfile inserted right after 'run' via the args-based helper. require.NotEmpty(t, cidFile) @@ -92,8 +92,8 @@ func TestBuildLauncherCmdUserSuppliedDockerRunShellWrapFallback(t *testing.T) { last := cmd.Args[len(cmd.Args)-1] assert.Contains(t, last, "docker run", "fallback must shell-wrap a 'docker run' command string, got: %s", last) - assert.Contains(t, last, "-e SLACK_TOKEN=xoxb-secret", - "injected -e env flag must survive into the shell command string, got: %s", last) + assert.Contains(t, last, "-e SLACK_TOKEN ", + "injected key-only -e env flag must survive into the shell command string, got: %s", last) } // TestBuildLauncherCmdPlainCommandStillShellWraps is the no-regression guard: a diff --git a/internal/upstream/core/docker_env_argv_test.go b/internal/upstream/core/docker_env_argv_test.go new file mode 100644 index 000000000..cc2f4e2b4 --- /dev/null +++ b/internal/upstream/core/docker_env_argv_test.go @@ -0,0 +1,115 @@ +package core + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secureenv" +) + +// Issue #1809: a server's `env` values (including resolved ${keyring:…}, +// ${env:…} and ${file:…} secrets) must never appear in the docker process's +// argv, where `ps aux` / `/proc//cmdline` expose them to every local +// user. Docker's `-e KEY` (no `=`) form takes the value from the docker CLI's +// own environment, which the spawn paths already populate from config.Env. + +const argvSecret = "xoxb-secret" + +func assertNoSecretInArgv(t *testing.T, args []string) { + t.Helper() + for _, a := range args { + assert.NotContains(t, a, argvSecret, + "env value must not appear in docker argv, got: %v", args) + } +} + +func TestInjectEnvVarsIntoDockerArgs_KeyOnlyNoValueInArgv(t *testing.T) { + c := newUserSuppliedDockerTestClient() + c.config.Env = map[string]string{"SLACK_TOKEN": argvSecret, "A_FIRST": "v1"} + + args := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) + + assertNoSecretInArgv(t, args) + assert.Equal(t, cmdRun, args[0]) + // Keys are emitted sorted so argv (and the log line) is deterministic. + assert.Equal(t, []string{cmdRun, "-e", "A_FIRST", "-e", "SLACK_TOKEN", "-i", "--rm", "mcp/example"}, args) +} + +// PATH is rewritten by prependDockerDirToPath after the env merge, so a +// configured container PATH must be inlined to stay exact; it is not a secret. +func TestInjectEnvVarsIntoDockerArgs_PathStaysInline(t *testing.T) { + c := newUserSuppliedDockerTestClient() + c.config.Env = map[string]string{"PATH": "/opt/tool/bin:/usr/bin", "SLACK_TOKEN": argvSecret} + + args := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) + + assertNoSecretInArgv(t, args) + assert.Equal(t, []string{cmdRun, "-e", "PATH=/opt/tool/bin:/usr/bin", "-e", "SLACK_TOKEN", "-i", "--rm", "mcp/example"}, args) + + // Windows spells it `Path`; the match is case-insensitive and the configured + // spelling is kept. + c.config.Env = map[string]string{"Path": `C:\tool;C:\Windows`} + args = c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) + assert.Equal(t, []string{cmdRun, "-e", `Path=C:\tool;C:\Windows`, "-i", "--rm", "mcp/example"}, args) +} + +func TestBuildDockerArgs_IsolationKeyOnlyNoValueInArgv(t *testing.T) { + im := NewIsolationManager(config.DefaultDockerIsolationConfig()) + serverConfig := &config.ServerConfig{ + Name: "iso-server", + Command: "python", + Args: []string{"-m", "mcp_server"}, + Env: map[string]string{"SLACK_TOKEN": argvSecret}, + } + + args, err := im.BuildDockerArgs(serverConfig, "python") + require.NoError(t, err) + + assertNoSecretInArgv(t, args) + assert.Subset(t, args, []string{"-e", "SLACK_TOKEN"}, + "isolation argv must carry the key-only -e flag, got: %v", args) +} + +func TestBuildLauncherCmd_UserDockerRun_SecretInEnvNotArgv(t *testing.T) { + fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, osDarwin) + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newUserSuppliedDockerTestClient() + c.envManager = secureenv.NewManager(nil) + + cmd, _, _, err := c.buildLauncherCmd(context.Background(), true) + require.NoError(t, err) + + assertNoSecretInArgv(t, cmd.Args) + assert.Subset(t, cmd.Args, []string{"-e", "SLACK_TOKEN"}) + assert.Contains(t, cmd.Env, "SLACK_TOKEN="+argvSecret, + "the value must reach docker through its environment, got env: %v", cmd.Env) +} + +func TestBuildLauncherCmd_UserDockerRun_ShellWrapSecretInEnvNotArgv(t *testing.T) { + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return "", fmt.Errorf("docker not found") } + + c := newUserSuppliedDockerTestClient() + c.envManager = secureenv.NewManager(nil) + + cmd, _, _, err := c.buildLauncherCmd(context.Background(), true) + require.NoError(t, err) + + assertNoSecretInArgv(t, cmd.Args) + last := cmd.Args[len(cmd.Args)-1] + assert.True(t, strings.Contains(last, "-e SLACK_TOKEN "), + "shell command string must carry the key-only -e flag, got: %s", last) + assert.Contains(t, cmd.Env, "SLACK_TOKEN="+argvSecret) +} diff --git a/internal/upstream/core/isolation.go b/internal/upstream/core/isolation.go index 96c0bfc47..68a92fd1e 100644 --- a/internal/upstream/core/isolation.go +++ b/internal/upstream/core/isolation.go @@ -623,10 +623,10 @@ func (im *IsolationManager) BuildDockerArgs(serverConfig *config.ServerConfig, r args = append(args, "--workdir", serverConfig.Isolation.WorkingDir) } - // Add environment variables from server config - for key, value := range serverConfig.Env { - args = append(args, "-e", fmt.Sprintf("%s=%s", key, value)) - } + // Add environment variables from server config as key-only `-e KEY` flags. + // The values are in the docker process environment (the connect paths merge + // config.Env into the spawn env), never in argv (#1809). + args = append(args, dockerEnvKeyFlags(serverConfig.Env)...) // Add global extra args args = append(args, im.globalConfig.ExtraArgs...)