From 1e6d6197ccdc45386d8aecb3745e7129e46f3bb5 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 18 Jun 2026 13:59:38 +0300 Subject: [PATCH] fix(upstream): prepend docker bundle dir to child PATH for credential helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docker-isolated (and user-supplied `docker run`) upstreams direct-exec the absolute docker binary (#696), but the spawned docker inherits mcpproxy's sanitized PATH, which omits the Docker Desktop bundle dir. So docker cannot exec the sibling binaries it shells out to — most importantly the credential helper from ~/.docker/config.json (`credsStore: desktop` → docker-credential-desktop). Any docker-isolated server whose isolation image isn't already cached fails to pull with: error getting credentials - err: exec: "docker-credential-desktop": executable file not found in $PATH resolveDockerSpawn / setupDockerIsolation now also return the resolved docker binary's bundle directory, and both stdio spawn paths plus the launcher prepend it to the child PATH via prependDockerDirToPath (dedup-aware, os.PathListSeparator, no-op when docker did not resolve to an absolute path). This generalizes the #696 fix from "find docker" to "let docker find its own tooling". Related #715 --- docs/docker-isolation.md | 20 +++ internal/upstream/core/connection_docker.go | 84 ++++++++++-- .../core/connection_docker_exec_e2e_test.go | 2 +- .../connection_docker_integration_test.go | 10 +- .../core/connection_docker_path_test.go | 129 ++++++++++++++++++ .../upstream/core/connection_docker_test.go | 20 +-- internal/upstream/core/connection_launcher.go | 7 +- internal/upstream/core/connection_stdio.go | 16 ++- 8 files changed, 259 insertions(+), 29 deletions(-) create mode 100644 internal/upstream/core/connection_docker_path_test.go diff --git a/docs/docker-isolation.md b/docs/docker-isolation.md index c65364ff..75253f35 100644 --- a/docs/docker-isolation.md +++ b/docs/docker-isolation.md @@ -277,6 +277,26 @@ docker stats succeeds. A `false` value with `docker_path: ""` means the CLI could not be resolved on the spawn path. +**`error getting credentials … docker-credential-desktop … not found in $PATH` on macOS (image not yet cached):** +- Docker Desktop's default `~/.docker/config.json` sets `"credsStore": "desktop"`, + so `docker` shells out to `docker-credential-desktop` for **every** registry + operation — even an anonymous pull of a public image. That helper lives in the + **same bundle dir** as the docker CLI + (`/Applications/Docker.app/Contents/Resources/bin/`), which mcpproxy's + sanitized spawn `PATH` omits. When the isolation image isn't cached locally, + the pull invokes the helper and fails; a pre-pulled image sidesteps it because + `docker run` then performs no registry op (which is why direct-exec alone + looked complete on cached images — issue #715 / MCP-2877). +- mcpproxy now **prepends the resolved docker binary's bundle dir to the child + `PATH`** whenever `docker` resolves to an absolute path, so the spawned docker + can exec its sibling tooling (`docker-credential-*`, `docker-compose`, + `docker-buildx`) exactly as it would from a normal Docker Desktop shell. This + is applied on every docker spawn path (isolated uvx/npx servers and + user-supplied `docker run …` upstreams) and is a no-op when `docker` did not + resolve to an absolute path. If you still see this error, confirm the helper + exists at the bundle path above, or pre-pull the image with + `docker pull `. + ## Security Considerations Docker isolation provides strong security boundaries but consider: diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 97effdfe..623b1adf 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -18,12 +18,22 @@ import ( var resolveDockerBinary = shellwrap.ResolveDockerPath // setupDockerIsolation configures Docker isolation for the MCP server process. -// Returns the docker command, arguments, and whether the returned command was -// wrapped in the user's login shell. shellWrapped governs how the caller -// inserts --cidfile: a direct-exec spawn (shellWrapped == false) uses the -// args-based insertCidfileIntoDockerArgs, while the login-shell fallback -// (shellWrapped == true) uses the string-based insertCidfileIntoShellDockerCommand. -func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) { +// Returns the docker command, arguments, whether the returned command was +// wrapped in the user's login shell, and the docker binary's bundle directory +// (empty unless docker resolved to a verified absolute executable). +// +// shellWrapped governs how the caller inserts --cidfile: a direct-exec spawn +// (shellWrapped == false) uses the args-based insertCidfileIntoDockerArgs, while +// the login-shell fallback (shellWrapped == true) uses the string-based +// insertCidfileIntoShellDockerCommand. +// +// dockerDir is the directory of the resolved absolute docker binary (e.g. +// /Applications/Docker.app/Contents/Resources/bin). The caller must prepend it +// to the child PATH (prependDockerDirToPath) so the spawned docker can exec its +// sibling tooling — most importantly the docker-credential-* helper named by +// ~/.docker/config.json's credsStore, which Docker Desktop installs into the +// same bundle dir and which docker shells out to for every registry op (#715). +func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string, shellWrapped bool, dockerDir string) { // Detect the runtime type from the command runtimeType := c.isolationManager.DetectRuntimeType(command) c.logger.Debug("Detected runtime type for Docker isolation", @@ -38,7 +48,7 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm zap.String("server", c.config.Name), zap.Error(err)) shellCmd, shellArgs := c.wrapWithUserShell(command, args) - return shellCmd, shellArgs, true + return shellCmd, shellArgs, true, "" } // Extract container name from Docker args for tracking @@ -95,6 +105,14 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm // the login-shell fallback (shellWrapped == true) uses the string-based // insertCidfileIntoShellDockerCommand. // +// dockerDir is the directory of the resolved absolute docker binary (empty +// unless docker resolved to a verified absolute executable). The caller must +// prepend it to the child PATH (prependDockerDirToPath) so the spawned docker +// can exec its sibling tooling — most importantly the docker-credential-* +// helper named by ~/.docker/config.json's credsStore, which Docker Desktop +// installs into the same bundle dir and which docker shells out to for every +// registry op (#715 / MCP-2877). +// // CRITICAL FIX (#696 / MCP-2744 / MCP-2753 / MCP-2868): resolve `docker` to an // ABSOLUTE path and exec it DIRECTLY — no login-shell wrap. Docker Desktop // installed the default way on macOS (without the optional, admin-gated "install @@ -133,7 +151,7 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm // When we fall back to the shell wrap we still prefer the resolved absolute path // as the wrapped command (it sidesteps the login shell's PATH re-derivation), // dropping to bare "docker" only when resolution failed. -func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) { +func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool, dockerDir string) { resolved, resErr := resolveDockerBinary(c.logger) switch { case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed(): @@ -145,7 +163,7 @@ func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, d zap.String("server", c.config.Name), zap.String("docker_path", resolved), zap.Bool("shell_wrapped", false)) - return resolved, finalArgs, false + return resolved, finalArgs, false, filepath.Dir(resolved) case resErr != nil: 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)", zap.String("server", c.config.Name), @@ -164,8 +182,14 @@ func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, d } dockerBin := cmdDocker + // Seed the bundle dir for PATH augmentation whenever docker resolved to a + // verified absolute executable — even though we keep the login-shell wrap + // here. It is harmless on the shell-wrap path and still helps if the login + // shell preserves the inherited PATH (#715). + var resolvedDir string if isDirectExecutable(resolved) { dockerBin = resolved + resolvedDir = filepath.Dir(resolved) } // INFO: the login-shell fallback is the ONLY path that can produce #696's // `command not found: docker`, so surface that we took it (and whether we at @@ -176,7 +200,47 @@ func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, d zap.Bool("docker_resolved", isDirectExecutable(resolved)), zap.Bool("shell_wrapped", true)) shellCmd, shellArgs := c.wrapWithUserShell(dockerBin, finalArgs) - return shellCmd, shellArgs, true + return shellCmd, shellArgs, true, resolvedDir +} + +// prependDockerDirToPath returns env with dockerDir prepended to its PATH entry +// so a spawned docker can resolve the sibling binaries it depends on — the +// docker-credential-* helper named by ~/.docker/config.json's credsStore, +// docker-compose, docker-buildx — which Docker Desktop installs into the same +// bundle dir as the docker CLI but which mcpproxy's sanitized PATH omits (#715). +// +// dockerDir is prepended (existing entries preserved) and deduped: if it is +// already present anywhere on PATH the env is returned unchanged. An empty +// dockerDir (docker not resolved to an absolute path) is a no-op. The input +// slice is never mutated. os.PathListSeparator keeps it correct on Windows. +func prependDockerDirToPath(env []string, dockerDir string) []string { + if dockerDir == "" { + return env + } + for i, kv := range env { + name, val, ok := strings.Cut(kv, "=") + if !ok || name != "PATH" { + continue + } + // Dedup: leave PATH untouched if the bundle dir is already on it. + for _, p := range filepath.SplitList(val) { + if p == dockerDir { + return env + } + } + out := make([]string, len(env)) + copy(out, env) + if val == "" { + out[i] = "PATH=" + dockerDir + } else { + out[i] = "PATH=" + dockerDir + string(os.PathListSeparator) + val + } + return out + } + // No PATH entry present — add one so docker still finds its bundle dir. + out := make([]string, len(env), len(env)+1) + copy(out, env) + return append(out, "PATH="+dockerDir) } // isDirectExecutable reports whether path is safe to hand to exec directly diff --git a/internal/upstream/core/connection_docker_exec_e2e_test.go b/internal/upstream/core/connection_docker_exec_e2e_test.go index 406b18a7..2e9751a0 100644 --- a/internal/upstream/core/connection_docker_exec_e2e_test.go +++ b/internal/upstream/core/connection_docker_exec_e2e_test.go @@ -69,7 +69,7 @@ func TestE2E_DockerSpawnExecsAbsoluteBundlePath(t *testing.T) { t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked") c := newIsolatedTestClient() - cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.False(t, shellWrapped, "#696: bundle-resolved docker must be direct-exec'd") // Exec it exactly as connection_stdio would (exec.CommandContext(cmd, args...)). diff --git a/internal/upstream/core/connection_docker_integration_test.go b/internal/upstream/core/connection_docker_integration_test.go index 9ee4bd28..df7986fc 100644 --- a/internal/upstream/core/connection_docker_integration_test.go +++ b/internal/upstream/core/connection_docker_integration_test.go @@ -42,7 +42,7 @@ func TestSpawnDecisionIsObservable_DirectExec(t *testing.T) { t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked") c, recorded := newObservedIsolatedClient(t) - _, _, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + _, _, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.False(t, shellWrapped) entries := recorded.FilterMessage("Docker spawn: direct-exec of resolved docker binary").All() @@ -63,7 +63,7 @@ func TestSpawnDecisionIsObservable_ShellWrapFallback(t *testing.T) { } c, recorded := newObservedIsolatedClient(t) - _, _, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + _, _, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.True(t, shellWrapped) entries := recorded.FilterMessage("Docker spawn: login-shell wrap fallback").All() @@ -122,7 +122,7 @@ func TestIntegration_DockerOnlyAtBundlePath_DirectExecs(t *testing.T) { t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked") c := newIsolatedTestClient() - cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.False(t, shellWrapped, "#696: docker resolved at the bundle path MUST be direct-exec'd, not shell-wrapped with bare docker") @@ -154,7 +154,7 @@ func TestIntegration_DockerOnPath_DirectExecs(t *testing.T) { t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked") c := newIsolatedTestClient() - cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.False(t, shellWrapped, "docker on PATH must resolve to an absolute path and direct-exec") assert.Equal(t, pathDocker, cmd, "spawn must exec the LookPath-resolved absolute path, got %q", cmd) @@ -183,7 +183,7 @@ func TestIntegration_DockerUnresolvable_FallsBackToBareDocker(t *testing.T) { t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked") c := newIsolatedTestClient() - _, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + _, shellArgs, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.True(t, shellWrapped, "unresolvable docker must fall back to the login-shell wrap") require.NotEmpty(t, shellArgs) diff --git a/internal/upstream/core/connection_docker_path_test.go b/internal/upstream/core/connection_docker_path_test.go new file mode 100644 index 00000000..fbe0db7d --- /dev/null +++ b/internal/upstream/core/connection_docker_path_test.go @@ -0,0 +1,129 @@ +package core + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// TestSetupDockerIsolationReturnsBundleDirOnDirectExec is the root-cause +// assertion for #715 / MCP-2877. When docker resolves to a verified absolute +// bundle-dir binary and is direct-exec'd, setupDockerIsolation must report that +// binary's directory so the caller can prepend it to the child PATH. Without +// the bundle dir on PATH, docker cannot exec its sibling credential helper +// (docker-credential-desktop, named by ~/.docker/config.json's credsStore) and +// any registry pull of an uncached isolation image fails. +func TestSetupDockerIsolationReturnsBundleDirOnDirectExec(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 := newIsolatedTestClient() + cmd, _, shellWrapped, dockerDir := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.False(t, shellWrapped) + require.Equal(t, fakeDocker, cmd) + assert.Equal(t, filepath.Dir(fakeDocker), dockerDir, + "setupDockerIsolation must report the resolved docker binary's bundle dir for PATH augmentation") +} + +// TestSetupDockerIsolationReturnsBundleDirOnShellWrapFallback verifies the dir +// is reported even when we keep the login-shell wrap (non-Darwin, no DOCKER_HOST +// in env): the issue specifies seeding the env PATH is harmless on the shell-wrap +// path and helps if the login shell preserves the inherited PATH. The gate is +// "resolved to a verified absolute executable", not "direct-exec'd". +func TestSetupDockerIsolationReturnsBundleDirOnShellWrapFallback(t *testing.T) { + fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, "linux") + t.Setenv("DOCKER_HOST", "") + t.Setenv("DOCKER_CONTEXT", "") + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newIsolatedTestClient() + _, _, shellWrapped, dockerDir := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.True(t, shellWrapped, "non-Darwin without DOCKER_HOST keeps the shell wrap") + assert.Equal(t, filepath.Dir(fakeDocker), dockerDir, + "a verified absolute resolved path must report its bundle dir even on the shell-wrap fallback") +} + +// TestSetupDockerIsolationNoBundleDirWhenUnresolved verifies no dir is reported +// when docker did not resolve to a verified absolute path (bare-'docker' +// fallback) — there is no filepath.Dir to take, and augmenting PATH would be +// meaningless / unsafe. +func TestSetupDockerIsolationNoBundleDirWhenUnresolved(t *testing.T) { + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return "docker", nil } + + c := newIsolatedTestClient() + _, _, shellWrapped, dockerDir := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.True(t, shellWrapped) + assert.Empty(t, dockerDir, "a non-absolute resolved value must not report a bundle dir") +} + +// TestPrependDockerDirToPath unit-tests the pure PATH-augmentation helper. +func TestPrependDockerDirToPath(t *testing.T) { + sep := string(os.PathListSeparator) + dir := filepath.FromSlash("/Applications/Docker.app/Contents/Resources/bin") + + t.Run("prepends to existing PATH preserving entries", func(t *testing.T) { + env := []string{"FOO=bar", "PATH=/usr/bin" + sep + "/bin"} + got := prependDockerDirToPath(env, dir) + var path string + for _, kv := range got { + if strings.HasPrefix(kv, "PATH=") { + path = strings.TrimPrefix(kv, "PATH=") + } + } + parts := filepath.SplitList(path) + require.NotEmpty(t, parts) + assert.Equal(t, dir, parts[0], "bundle dir must be prepended (front), got: %s", path) + assert.Contains(t, parts, "/usr/bin", "existing entries preserved") + assert.Contains(t, parts, "/bin", "existing entries preserved") + assert.Contains(t, got, "FOO=bar", "unrelated env vars untouched") + }) + + t.Run("dedup: already present leaves PATH unchanged", func(t *testing.T) { + env := []string{"PATH=/usr/bin" + sep + dir + sep + "/bin"} + got := prependDockerDirToPath(env, dir) + assert.Equal(t, env, got, "must not re-add a dir already on PATH") + }) + + t.Run("empty dir is a no-op", func(t *testing.T) { + env := []string{"PATH=/usr/bin"} + got := prependDockerDirToPath(env, "") + assert.Equal(t, env, got) + }) + + t.Run("adds PATH when none present", func(t *testing.T) { + env := []string{"FOO=bar"} + got := prependDockerDirToPath(env, dir) + assert.Contains(t, got, "PATH="+dir) + assert.Contains(t, got, "FOO=bar") + }) + + t.Run("empty PATH value yields just the dir", func(t *testing.T) { + env := []string{"PATH="} + got := prependDockerDirToPath(env, dir) + assert.Contains(t, got, "PATH="+dir) + }) + + t.Run("does not mutate the input slice", func(t *testing.T) { + env := []string{"PATH=/usr/bin"} + _ = prependDockerDirToPath(env, dir) + assert.Equal(t, "PATH=/usr/bin", env[0], "input slice must not be mutated") + }) +} diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go index 124a6db4..ae9493b8 100644 --- a/internal/upstream/core/connection_docker_test.go +++ b/internal/upstream/core/connection_docker_test.go @@ -90,7 +90,7 @@ func TestSetupDockerIsolationExecsResolvedBinaryDirectly(t *testing.T) { resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) assert.False(t, shellWrapped, "verified absolute executable must NOT shell-wrap the spawn") assert.Equal(t, fakeDocker, cmd, @@ -117,7 +117,7 @@ func TestSetupDockerIsolationUsesResolvedAbsolutePath(t *testing.T) { resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - cmd, _, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, _, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) assert.False(t, shellWrapped) assert.Equal(t, fakeDocker, cmd, @@ -139,7 +139,7 @@ func TestSetupDockerIsolationShellWrapsNonAbsoluteResolved(t *testing.T) { resolveDockerBinary = func(_ *zap.Logger) (string, error) { return "docker", nil } c := newIsolatedTestClient() - cmd, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, shellArgs, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.True(t, shellWrapped, "a non-absolute resolved value must be shell-wrapped, not direct-exec'd") assert.NotEqual(t, "docker", cmd, "must not direct-exec a bare command name") @@ -160,7 +160,7 @@ func TestSetupDockerIsolationShellWrapsNonExecutableResolved(t *testing.T) { } c := newIsolatedTestClient() - _, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + _, shellArgs, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.True(t, shellWrapped, "a non-existent absolute path must be shell-wrapped, not direct-exec'd") require.NotEmpty(t, shellArgs) @@ -181,7 +181,7 @@ func TestSetupDockerIsolationCidfileInsertionWithAbsolutePath(t *testing.T) { resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.False(t, shellWrapped) require.Equal(t, fakeDocker, cmd) @@ -240,7 +240,7 @@ func TestSetupDockerIsolationFallsBackToBareDocker(t *testing.T) { } c := newIsolatedTestClient() - _, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + _, shellArgs, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.True(t, shellWrapped, "on resolution failure the spawn must be shell-wrapped") require.NotEmpty(t, shellArgs) @@ -269,7 +269,7 @@ func TestSetupDockerIsolationShellWrapsWhenDaemonEnvMissingNonDarwin(t *testing. resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - cmd, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, shellArgs, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) require.True(t, shellWrapped, "non-Darwin with no DOCKER_HOST in env must keep the login-shell wrap to inherit rc-file DOCKER_*") @@ -298,7 +298,7 @@ func TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv(t *testing.T) { resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args) assert.False(t, shellWrapped, "DOCKER_HOST in os.Environ() guarantees daemon config — direct-exec is safe off macOS") @@ -330,7 +330,7 @@ func TestResolveDockerSpawnUserSuppliedDockerRunDirectExec(t *testing.T) { // flags into the docker run argv before deciding how to spawn. argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) - cmd, args, shellWrapped := c.resolveDockerSpawn(argsToWrap) + cmd, args, shellWrapped, _ := c.resolveDockerSpawn(argsToWrap) assert.False(t, shellWrapped, "verified absolute docker must direct-exec, not shell-wrap") assert.Equal(t, fakeDocker, cmd, @@ -368,7 +368,7 @@ func TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap(t *testing.T) { c := newUserSuppliedDockerTestClient() argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) - cmd, shellArgs, shellWrapped := c.resolveDockerSpawn(argsToWrap) + cmd, shellArgs, shellWrapped, _ := c.resolveDockerSpawn(argsToWrap) require.True(t, shellWrapped, "on resolution failure the user docker spawn must be shell-wrapped") assert.NotEqual(t, cmdDocker, cmd, "shell-wrap fallback exec's the login shell, not bare 'docker'") diff --git a/internal/upstream/core/connection_launcher.go b/internal/upstream/core/connection_launcher.go index 898cd163..caabc74b 100644 --- a/internal/upstream/core/connection_launcher.go +++ b/internal/upstream/core/connection_launcher.go @@ -248,7 +248,12 @@ func (c *Client) buildLauncherCmd(_ context.Context, willUseDocker bool) (*exec. if c.isolationManager != nil && c.isolationManager.ShouldIsolate(c.config) { var dockerShellWrapped bool - finalCommand, finalArgs, dockerShellWrapped = c.setupDockerIsolation(c.config.Command, args) + var dockerDir string + finalCommand, finalArgs, dockerShellWrapped, dockerDir = c.setupDockerIsolation(c.config.Command, args) + // Prepend the docker bundle dir to the child PATH so the spawned docker + // can exec its sibling credential helper / tooling on a registry pull + // (#715). No-op when docker did not resolve to an absolute path. + envVars = prependDockerDirToPath(envVars, dockerDir) if cidFile != "" { if dockerShellWrapped { finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile) diff --git a/internal/upstream/core/connection_stdio.go b/internal/upstream/core/connection_stdio.go index 94faa6c3..e5a06de2 100644 --- a/internal/upstream/core/connection_stdio.go +++ b/internal/upstream/core/connection_stdio.go @@ -146,9 +146,15 @@ func (c *Client) connectStdio(ctx context.Context) error { // back to a login-shell wrap. dockerShellWrapped selects the matching // cidfile helper. var dockerShellWrapped bool - finalCommand, finalArgs, dockerShellWrapped = c.setupDockerIsolation(c.config.Command, args) + var dockerDir string + finalCommand, finalArgs, dockerShellWrapped, dockerDir = c.setupDockerIsolation(c.config.Command, args) c.isDockerCommand = true + // Prepend the docker bundle dir to the child PATH so the spawned docker + // can exec its sibling credential helper / tooling on a registry pull + // (#715). No-op when docker did not resolve to an absolute path. + envVars = prependDockerDirToPath(envVars, dockerDir) + // Add cidfile to the Docker command if we have one if cidFile != "" { if dockerShellWrapped { @@ -177,7 +183,13 @@ func (c *Client) connectStdio(ctx context.Context) error { } var dockerShellWrapped bool - finalCommand, finalArgs, dockerShellWrapped = c.resolveDockerSpawn(argsToWrap) + var dockerDir string + finalCommand, finalArgs, dockerShellWrapped, dockerDir = c.resolveDockerSpawn(argsToWrap) + + // Prepend the docker bundle dir to the child PATH so the spawned docker + // can exec its sibling credential helper / tooling on a registry pull + // (#715). No-op when docker did not resolve to an absolute path. + envVars = prependDockerDirToPath(envVars, dockerDir) // Insert --cidfile via the helper that matches how we spawn: args-based on // the direct-exec path, string-based on the login-shell fallback.