Skip to content

Commit b54d0b0

Browse files
committed
fix(docker): preserve Docker daemon env vars on direct-exec spawn path
The MCP-2744 direct-exec docker-isolation spawn replaced the $SHELL -l -c login-shell wrap, so the child inherits only the secure-env allowlist. That allowlist excluded DOCKER_HOST/DOCKER_CONTEXT/DOCKER_CONFIG/DOCKER_TLS_VERIFY/ DOCKER_CERT_PATH, so for Colima/rootless/remote daemons the docker binary resolved but 'docker run' could not reach the daemon (silent regression). Add DockerDaemonEnvVars() to the secure-env allowlist (DefaultEnvConfig) and thread them through for command/stdio servers even when an operator supplies a custom Environment allowlist. These are connection pointers, not secrets. Related #699 Related #696
1 parent 1cf0401 commit b54d0b0

4 files changed

Lines changed: 127 additions & 0 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package secureenv
2+
3+
import (
4+
"strings"
5+
"testing"
6+
)
7+
8+
// TestDefaultEnvConfigAllowsDockerDaemonVars guards the MCP-2744 regression:
9+
// the direct-exec docker spawn path (no login-shell wrap) inherits ONLY the
10+
// vars BuildSecureEnvironment passes through. Docker daemon-access vars
11+
// (DOCKER_HOST, DOCKER_CONTEXT, …) used by Colima / rootless / remote Docker
12+
// MUST survive into the spawn env, or `docker run` resolves the binary but
13+
// cannot reach the daemon. See PR #699 Codex review.
14+
func TestDefaultEnvConfigAllowsDockerDaemonVars(t *testing.T) {
15+
cfg := DefaultEnvConfig()
16+
mgr := NewManager(cfg)
17+
18+
for _, key := range DockerDaemonEnvVars() {
19+
if !mgr.isKeyAllowed(key) {
20+
t.Errorf("DefaultEnvConfig should allow docker daemon var %q so direct-exec docker isolation can reach the daemon (Colima/rootless)", key)
21+
}
22+
}
23+
}
24+
25+
// TestBuildSecureEnvironmentPreservesDockerHost is the Colima regression test
26+
// requested in the PR #699 review: DOCKER_HOST set in the mcpproxy environment
27+
// must survive into the spawned child's secure environment.
28+
func TestBuildSecureEnvironmentPreservesDockerHost(t *testing.T) {
29+
t.Setenv("DOCKER_HOST", "tcp://127.0.0.1:2375")
30+
t.Setenv("DOCKER_CONTEXT", "colima")
31+
32+
mgr := NewManager(DefaultEnvConfig())
33+
env := mgr.BuildSecureEnvironment()
34+
35+
got := map[string]string{}
36+
for _, kv := range env {
37+
if i := strings.IndexByte(kv, '='); i >= 0 {
38+
got[kv[:i]] = kv[i+1:]
39+
}
40+
}
41+
42+
if got["DOCKER_HOST"] != "tcp://127.0.0.1:2375" {
43+
t.Errorf("DOCKER_HOST should survive into spawn env, got %q", got["DOCKER_HOST"])
44+
}
45+
if got["DOCKER_CONTEXT"] != "colima" {
46+
t.Errorf("DOCKER_CONTEXT should survive into spawn env, got %q", got["DOCKER_CONTEXT"])
47+
}
48+
}

internal/secureenv/manager.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,27 @@ type PathDiscovery struct {
4141
AvailableTools map[string]string
4242
}
4343

44+
// DockerDaemonEnvVars returns the environment variables the docker CLI uses to
45+
// locate and authenticate to the daemon (local, remote, Colima, or rootless).
46+
//
47+
// MCP-2744 / PR #699: the docker-isolation spawn now direct-execs the resolved
48+
// docker binary instead of routing through a `$SHELL -l -c` login-shell wrap.
49+
// The login shell used to re-derive these from the user's rc files; the
50+
// direct-exec path inherits only what BuildSecureEnvironment passes through, so
51+
// they must be on the allowlist or `docker run` resolves the binary yet cannot
52+
// reach the daemon (silent regression for Colima/rootless users). These are
53+
// connection pointers (host URL, context name, config/cert directory paths) and
54+
// not secrets, so allowing them through is safe.
55+
func DockerDaemonEnvVars() []string {
56+
return []string{
57+
"DOCKER_HOST", // Daemon socket/URL (e.g. tcp://… for Colima/remote)
58+
"DOCKER_CONTEXT", // Named docker context selecting the daemon
59+
"DOCKER_CONFIG", // Path to the docker config dir (~/.docker by default)
60+
"DOCKER_TLS_VERIFY", // Enable TLS verification against the daemon
61+
"DOCKER_CERT_PATH", // Path to TLS client certs for the daemon
62+
}
63+
}
64+
4465
// DefaultEnvConfig returns default environment configuration with safe system variables
4566
func DefaultEnvConfig() *EnvConfig {
4667
allowedVars := []string{
@@ -56,6 +77,10 @@ func DefaultEnvConfig() *EnvConfig {
5677
"USERNAME", // Current user (Windows)
5778
}
5879

80+
// Docker daemon-access vars (MCP-2744): required so direct-exec docker
81+
// isolation can reach a non-default daemon (Colima/rootless/remote).
82+
allowedVars = append(allowedVars, DockerDaemonEnvVars()...)
83+
5984
// Add Windows-specific variables
6085
if runtime.GOOS == osWindows {
6186
allowedVars = append(allowedVars,

internal/upstream/core/client.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,20 @@ func NewClientWithOptions(id string, serverConfig *config.ServerConfig, logger *
182182
// Create a copy of the config to avoid modifying the original
183183
envConfigCopy := *envConfig
184184
envConfigCopy.EnhancePath = true
185+
186+
// MCP-2744: the docker-isolation spawn direct-execs the resolved docker
187+
// binary (no login-shell wrap), so it inherits only the secure-env
188+
// allowlist. Ensure docker daemon-access vars (DOCKER_HOST/CONTEXT/…)
189+
// survive even when an operator supplied a custom Environment allowlist,
190+
// or Colima/rootless/remote daemons become unreachable. DefaultEnvConfig
191+
// already includes these; this covers the custom-config path. The slice
192+
// is rebuilt (not appended in place) so the shared source config is not
193+
// mutated.
194+
allowed := make([]string, 0, len(envConfigCopy.AllowedSystemVars)+len(secureenv.DockerDaemonEnvVars()))
195+
allowed = append(allowed, envConfigCopy.AllowedSystemVars...)
196+
allowed = append(allowed, secureenv.DockerDaemonEnvVars()...)
197+
envConfigCopy.AllowedSystemVars = allowed
198+
185199
envConfig = &envConfigCopy
186200
}
187201

internal/upstream/core/connection_docker_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,52 @@ import (
66
"testing"
77

88
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
9+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/secureenv"
910

1011
"github.com/stretchr/testify/assert"
1112
"github.com/stretchr/testify/require"
1213
"go.uber.org/zap"
1314
)
1415

16+
// TestDockerIsolationSpawnEnvPreservesDockerHost is the Colima regression test
17+
// for the PR #699 review (MCP-2744): the direct-exec docker spawn inherits only
18+
// the secure-env allowlist, so docker daemon-access vars must survive into the
19+
// child env even when the operator supplied a RESTRICTIVE custom Environment
20+
// allowlist that omits them. Otherwise Colima/rootless/remote daemons become
21+
// unreachable ("binary resolves but `docker run` can't reach the daemon").
22+
func TestDockerIsolationSpawnEnvPreservesDockerHost(t *testing.T) {
23+
t.Setenv("DOCKER_HOST", "tcp://127.0.0.1:2375")
24+
25+
globalCfg := &config.Config{
26+
// Operator-supplied allowlist that deliberately omits DOCKER_* — the
27+
// client must still thread daemon vars through for command servers.
28+
Environment: &secureenv.EnvConfig{
29+
InheritSystemSafe: true,
30+
AllowedSystemVars: []string{"PATH", "HOME"},
31+
},
32+
}
33+
serverCfg := &config.ServerConfig{
34+
Name: "iso-server",
35+
Command: "python",
36+
Args: []string{"-m", "mcp_server"},
37+
}
38+
39+
c, err := NewClient("iso-server", serverCfg, zap.NewNop(), nil, globalCfg, nil, nil)
40+
require.NoError(t, err)
41+
42+
mgr, ok := c.GetEnvManager().(*secureenv.Manager)
43+
require.True(t, ok, "env manager must be a *secureenv.Manager")
44+
45+
var found bool
46+
for _, kv := range mgr.BuildSecureEnvironment() {
47+
if strings.HasPrefix(kv, "DOCKER_HOST=") {
48+
found = true
49+
assert.Equal(t, "DOCKER_HOST=tcp://127.0.0.1:2375", kv)
50+
}
51+
}
52+
assert.True(t, found, "DOCKER_HOST must survive into the docker-isolation spawn env even with a custom allowlist")
53+
}
54+
1555
func newIsolatedTestClient() *Client {
1656
return &Client{
1757
config: &config.ServerConfig{

0 commit comments

Comments
 (0)