Skip to content

Commit 10efe4a

Browse files
DumbrisPaperclip-Paperclip
Dumbris
and
Paperclip-Paperclip
authored
fix(docker): direct-exec resolved docker binary for isolated spawn (MCP-2753) (#703)
* fix(docker): direct-exec resolved docker binary for isolated spawn (MCP-2753) PR #697 resolved the docker CLI to an absolute path but still routed the isolated spawn through `$SHELL -l -c "<docker> run …"`, where the login shell re-derives $PATH from rc files and can drop the Docker Desktop bundle dir. The absolute path was only a token in the -c string, so docker-isolated servers still failed with `command not found: docker` when the CLI was not on the LaunchAgent/tray spawn PATH — even though `upstream_servers list` reported a good docker_path. On successful resolution to a VERIFIED absolute executable, setupDockerIsolation now returns the resolved binary as the exec command with raw docker argv (no shell wrap), mirroring the management path's newDockerCmd. It returns a shellWrapped flag so callers pick the matching cidfile helper: the new args-based insertCidfileIntoDockerArgs inserts `--cidfile` after the `run` token (platform-agnostic, sidestepping the -c vs /c quirk), while the login-shell fallback keeps the string-based insertCidfileIntoShellDockerCommand. The direct-exec branch is gated by isDirectExecutable (filepath.IsAbs + os.Stat + exec-bit). ResolveDockerPath's last resort runs `command -v docker` in the login shell, which can emit a shell function/alias/bare name; such a value is rejected and shell-wrapped instead of failing a direct exec. Login-shell wrap of bare `docker` is also kept on the resolution-failure fallback. The DOCKER_* daemon-env half of MCP-2753 is already satisfied by the startup login-shell hydration shipped in MCP-2751 (#701): DOCKER_* are hydrated into os.Environ() and carried through BuildSecureEnvironment's default allow-list, so the direct-exec'd docker process still reaches Colima/rootless/remote daemons without a per-spawn shell capture. Related #699 Related #696 * fix(docker): gate direct-exec on guaranteed daemon env (non-Darwin rootless regression) Codex round-4 (PR #703 REQUEST_CHANGES, P2): the direct-exec branch regressed non-Darwin rootless/remote Docker. Dropping the `$SHELL -l` wrap also drops rc-file env inheritance, and HydrateFromLoginShell (MCP-2751) is Darwin-only while BuildSecureEnvironment only forwards DOCKER_* already in os.Environ(). So on Linux a daemon whose DOCKER_HOST/DOCKER_CONTEXT lives only in .profile would be lost by direct-exec — the same DOCKER_* loss that made #699 keep the shell wrap. Add a second precondition (dockerDaemonEnvGuaranteed) alongside the verified- absolute-executable check: direct-exec only when the daemon env is guaranteed without the login shell — macOS (startup hydration captured DOCKER_* into os.Environ()) OR DOCKER_HOST/DOCKER_CONTEXT already present in the process env on any platform. Otherwise keep the `$SHELL -l` wrap so `docker run` inherits the rc-file daemon config; the wrap now prefers the resolved absolute path (still sidestepping the login shell's PATH re-derivation) and drops to bare "docker" only when resolution failed. dockerDaemonEnvGOOS is a package var so the Darwin branch is testable on a non-Darwin CI host. New tests: non-Darwin + DOCKER_HOST-only-in-rc stays shell-wrapped with the absolute path; non-Darwin + DOCKER_HOST in env direct- execs; and a table test for the gate. Related #703 Related #699 Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
1 parent 0521e90 commit 10efe4a

5 files changed

Lines changed: 428 additions & 59 deletions

File tree

‎docs/docker-isolation.md‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -251,11 +251,24 @@ docker stats
251251
it is **not** on a standard `PATH` dir unless you ran the optional,
252252
admin-gated "install CLI tools" step. When mcpproxy is launched from a
253253
LaunchAgent / tray, the captured login-shell `PATH` may omit this directory.
254-
- mcpproxy resolves the `docker` binary to its **absolute path** before
255-
spawning an isolated server (and also adds the bundle bin dir to the enhanced
256-
spawn `PATH`), so isolation works even without the CLI-tools step. If you
257-
still see this error, confirm the binary exists at the bundle path above, or
258-
run Docker Desktop's "install CLI tools".
254+
- mcpproxy resolves the `docker` binary to its **absolute path** and then
255+
**exec's it directly** (no login-shell wrap) when spawning an isolated server,
256+
so the spawn bypasses `PATH` entirely and works even without the CLI-tools
257+
step. (The enhanced spawn `PATH` still includes the bundle bin dir as a
258+
belt-and-suspenders measure.) Earlier builds resolved the absolute path but
259+
still routed the spawn through `$SHELL -l -c "<docker> run …"`, where the
260+
login shell re-derived `PATH` from rc files and could drop the bundle dir —
261+
so the error persisted; direct exec fixes that. Direct exec is used only when
262+
(a) the resolved value is a verified absolute executable and (b) the docker
263+
daemon-config env is guaranteed without the login shell — on macOS via the
264+
startup login-shell hydration, or on any platform when `DOCKER_HOST` /
265+
`DOCKER_CONTEXT` are already exported into mcpproxy's environment. A
266+
non-absolute result (e.g. a shell function/alias from `command -v docker`), or
267+
a rootless/remote daemon on Linux whose `DOCKER_HOST` lives only in the
268+
login-shell rc, falls back to the `$SHELL -l` wrap (still using the resolved
269+
absolute path when one was found) so `docker run` keeps inheriting the daemon
270+
config. If you still see this error, confirm the binary exists at the bundle
271+
path above, or run Docker Desktop's "install CLI tools".
259272
- `upstream_servers list` reports `docker_status.docker_path` (the resolved
260273
binary) and reports `docker_status.available` / per-server `docker_available`
261274
as `true` **only** when the CLI is actually resolvable *and* `docker info`

‎internal/upstream/core/connection_docker.go‎

Lines changed: 142 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ package core
22

33
import (
44
"fmt"
5+
"os"
6+
"path/filepath"
7+
"runtime"
58
"strings"
69

710
"github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap"
@@ -15,8 +18,12 @@ import (
1518
var resolveDockerBinary = shellwrap.ResolveDockerPath
1619

1720
// setupDockerIsolation configures Docker isolation for the MCP server process.
18-
// Returns the docker command and arguments to execute.
19-
func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string) {
21+
// Returns the docker command, arguments, and whether the returned command was
22+
// wrapped in the user's login shell. shellWrapped governs how the caller
23+
// inserts --cidfile: a direct-exec spawn (shellWrapped == false) uses the
24+
// args-based insertCidfileIntoDockerArgs, while the login-shell fallback
25+
// (shellWrapped == true) uses the string-based insertCidfileIntoShellDockerCommand.
26+
func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) {
2027
// Detect the runtime type from the command
2128
runtimeType := c.isolationManager.DetectRuntimeType(command)
2229
c.logger.Debug("Detected runtime type for Docker isolation",
@@ -30,7 +37,8 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
3037
c.logger.Error("Failed to build Docker args, falling back to shell wrapping",
3138
zap.String("server", c.config.Name),
3239
zap.Error(err))
33-
return c.wrapWithUserShell(command, args)
40+
shellCmd, shellArgs := c.wrapWithUserShell(command, args)
41+
return shellCmd, shellArgs, true
3442
}
3543

3644
// Extract container name from Docker args for tracking
@@ -61,27 +69,142 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
6169
zap.String("container_command", containerCommand))
6270
}
6371

64-
// CRITICAL FIX (#696): resolve `docker` to an ABSOLUTE path before shell-wrapping.
65-
// Docker Desktop installed the default way on macOS (without the optional,
66-
// admin-gated "install CLI tools" step) leaves the docker CLI only inside the
67-
// app bundle at /Applications/Docker.app/Contents/Resources/bin/docker, which
68-
// is NOT on any standard PATH dir nor on the (often unreliable) login-shell
69-
// PATH a LaunchAgent captures. Invoking docker by absolute path — mirroring
70-
// newDockerCmd — bypasses PATH entirely so isolated servers spawn successfully.
71-
// Fall back to the bare "docker" command only when resolution fails.
72+
// CRITICAL FIX (#696 / MCP-2744 / MCP-2753): resolve `docker` to an ABSOLUTE
73+
// path and exec it DIRECTLY — no login-shell wrap. Docker Desktop installed
74+
// the default way on macOS (without the optional, admin-gated "install CLI
75+
// tools" step) leaves the docker CLI only inside the app bundle at
76+
// /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on
77+
// any standard PATH dir nor on the (often unreliable) login-shell PATH a
78+
// LaunchAgent captures.
7279
//
73-
// We still wrap in the user login shell so env-var inheritance and the
74-
// existing cidfile insertion (which scans the command string for "docker run")
75-
// keep working; the trailing "docker run" substring matches the absolute path.
80+
// #697 resolved the absolute path but still routed the spawn through
81+
// `$SHELL -l -c "<docker> run …"`. There the absolute path is only a token
82+
// inside the -c string: the login shell re-derives $PATH from rc files and
83+
// can drop the bundle dir, so the bug persisted. Exec'ing the absolute path
84+
// directly (mirroring newDockerCmd's exec.CommandContext(dockerBin, …))
85+
// bypasses PATH entirely.
86+
//
87+
// Two preconditions gate the direct-exec:
88+
//
89+
// 1. The resolved value must be a VERIFIED absolute executable
90+
// (isDirectExecutable). ResolveDockerPath's last resort runs
91+
// `command -v docker` in the login shell, which can emit a shell
92+
// function name, an alias, or a bare command name rather than a real
93+
// binary path; direct-exec'ing that would fail with "no such file".
94+
//
95+
// 2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be
96+
// guaranteed in the spawn env WITHOUT the login shell
97+
// (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops
98+
// rc-file env inheritance. On macOS the startup hydration (MCP-2751)
99+
// already captured DOCKER_* from the login shell into os.Environ(), and
100+
// secureenv forwards them — but that hydration is Darwin-only. On Linux
101+
// a rootless/remote daemon whose DOCKER_HOST lives only in .profile would
102+
// be lost by direct-exec (the regression #699 kept the shell wrap to
103+
// avoid). So on non-Darwin we only direct-exec when DOCKER_HOST/
104+
// DOCKER_CONTEXT are already in os.Environ(); otherwise we keep the
105+
// login-shell wrap so `docker run` still finds the daemon.
106+
//
107+
// When we fall back to the shell wrap we still prefer the resolved absolute
108+
// path as the wrapped command (it sidesteps the login shell's PATH
109+
// re-derivation), dropping to bare "docker" only when resolution failed.
110+
resolved, resErr := resolveDockerBinary(c.logger)
111+
switch {
112+
case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed():
113+
return resolved, finalArgs, false
114+
case resErr != nil:
115+
c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' via login shell (isolated server may fail if docker is not on the spawn PATH)",
116+
zap.String("server", c.config.Name),
117+
zap.Error(resErr))
118+
case !isDirectExecutable(resolved):
119+
c.logger.Warn("Resolved docker value is not a verified absolute executable; falling back to login-shell wrap",
120+
zap.String("server", c.config.Name),
121+
zap.String("resolved", resolved))
122+
default:
123+
// Verified absolute executable, but the daemon env is not guaranteed
124+
// without the login shell (non-Darwin, no DOCKER_HOST/DOCKER_CONTEXT in
125+
// os.Environ()). Keep the shell wrap so rc-file DOCKER_* are inherited.
126+
c.logger.Debug("docker daemon env not guaranteed in process env; keeping login-shell wrap so rc-file DOCKER_* are inherited",
127+
zap.String("server", c.config.Name),
128+
zap.String("resolved", resolved))
129+
}
130+
76131
dockerBin := cmdDocker
77-
if resolved, resErr := resolveDockerBinary(c.logger); resErr == nil && resolved != "" {
132+
if isDirectExecutable(resolved) {
78133
dockerBin = resolved
79-
} else if resErr != nil {
80-
c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' (isolated server may fail if docker is not on the spawn PATH)",
134+
}
135+
shellCmd, shellArgs := c.wrapWithUserShell(dockerBin, finalArgs)
136+
return shellCmd, shellArgs, true
137+
}
138+
139+
// isDirectExecutable reports whether path is safe to hand to exec directly
140+
// (without a login-shell wrap): it must be an ABSOLUTE path to an existing,
141+
// non-directory file that is executable. This is the Codex round-3 guard for
142+
// MCP-2753 — a resolved value such as a shell function/alias name or a bare
143+
// command name (which `command -v docker` can emit) is rejected so it is
144+
// shell-wrapped instead of failing a direct exec.
145+
func isDirectExecutable(path string) bool {
146+
if path == "" || !filepath.IsAbs(path) {
147+
return false
148+
}
149+
info, err := os.Stat(path)
150+
if err != nil || info.IsDir() {
151+
return false
152+
}
153+
if runtime.GOOS == osWindows {
154+
// Windows file mode bits don't convey the executable flag; an absolute
155+
// path to a regular file is the strongest portable signal here.
156+
return true
157+
}
158+
return info.Mode().Perm()&0o111 != 0
159+
}
160+
161+
// dockerDaemonEnvGOOS mirrors runtime.GOOS; a package var so tests can exercise
162+
// the Darwin branch on a non-Darwin CI host.
163+
var dockerDaemonEnvGOOS = runtime.GOOS
164+
165+
// dockerDaemonEnvGuaranteed reports whether the docker daemon-config env
166+
// (DOCKER_HOST/DOCKER_CONTEXT) is guaranteed to reach a direct-exec'd docker
167+
// process WITHOUT a login-shell wrap.
168+
//
169+
// On macOS the startup login-shell hydration (shellwrap.HydrateFromLoginShell,
170+
// MCP-2751) captures DOCKER_* into os.Environ() — its gate fires whenever a
171+
// docker var is missing — so the secureenv allow-list forwards them and the
172+
// shell wrap is unnecessary. That hydration is Darwin-only, so on other
173+
// platforms direct-exec is only safe to skip the shell wrap when DOCKER_HOST or
174+
// DOCKER_CONTEXT are ALREADY exported into mcpproxy's own environment. When they
175+
// are configured only in the user's login-shell rc (Codex's non-Darwin rootless
176+
// regression on PR #703), we must keep the `$SHELL -l` wrap so `docker run`
177+
// inherits them.
178+
func dockerDaemonEnvGuaranteed() bool {
179+
if dockerDaemonEnvGOOS == osDarwin {
180+
return true
181+
}
182+
return os.Getenv("DOCKER_HOST") != "" || os.Getenv("DOCKER_CONTEXT") != ""
183+
}
184+
185+
// insertCidfileIntoDockerArgs inserts "--cidfile <file>" immediately after the
186+
// "run" token in a direct-exec docker argument slice (no shell wrap). It is the
187+
// args-based sibling of insertCidfileIntoShellDockerCommand used on the
188+
// direct-exec spawn path (MCP-2753). Operating on argv (rather than a shell
189+
// command string) makes it platform-agnostic and sidesteps the -c vs /c shell
190+
// quirk #697 had to patch.
191+
func (c *Client) insertCidfileIntoDockerArgs(args []string, cidFile string) []string {
192+
if len(args) == 0 || args[0] != cmdRun {
193+
c.logger.Warn("Could not find 'run' as the first docker arg for cidfile insertion - container ID tracking may be limited",
81194
zap.String("server", c.config.Name),
82-
zap.Error(resErr))
195+
zap.Strings("args", args))
196+
return args
83197
}
84-
return c.wrapWithUserShell(dockerBin, finalArgs)
198+
199+
newArgs := make([]string, 0, len(args)+2)
200+
newArgs = append(newArgs, args[0]) // "run"
201+
newArgs = append(newArgs, "--cidfile", cidFile)
202+
newArgs = append(newArgs, args[1:]...)
203+
204+
c.logger.Debug("Inserted cidfile into direct-exec docker args",
205+
zap.String("server", c.config.Name),
206+
zap.String("cid_file", cidFile))
207+
return newArgs
85208
}
86209

87210
// injectEnvVarsIntoDockerArgs injects environment variables as -e flags into Docker run args

0 commit comments

Comments
 (0)