diff --git a/docs/server-entry-spec.md b/docs/server-entry-spec.md index 6b97522b..0236a9c9 100644 --- a/docs/server-entry-spec.md +++ b/docs/server-entry-spec.md @@ -38,10 +38,16 @@ allowHosts: |-------|------|----------|-------------| | `image` | string | Yes* | Docker image reference (can be SHA256 digest or tag). Required for `server` type. | | `command` | []string | No | Command-line arguments to pass to the container. | -| `volumes` | []string | No | Volume mount specifications (format: `host:container` or `host:container:ro`). | +| `volumes` | []string | No | Volume mount specifications (format: `host:container`, `host:container:ro`, or `host:container:rw`). | | `user` | string | No | User to run the container as (e.g., `1000:1000`). | | `longLived` | boolean | No | Whether the server should remain running (true) or start on-demand (false). Default: false. | +#### Host Bind Mount Safety + +Host path bind mounts default to read-only when no mode is specified. Explicit writable host path bind mounts, and no-mode bind mounts that should become writable, are rejected unless the resolved host source exactly matches an entry in `MCP_GATEWAY_DOCKER_BIND_ALLOW_WRITABLE_PATHS`. Use the OS path-list separator when setting multiple entries. + +Setting `MCP_GATEWAY_DOCKER_BIND_ALLOW_WRITABLE_PATHS` means any catalog or profile you load can request that exact host path as writable. Scope writable entries to dedicated data directories, not source trees or broad project directories. + ### Remote Configuration (for type: "remote") | Field | Type | Required | Description | @@ -369,4 +375,3 @@ icon: https://www.cloudflare.com/favicon.ico - Use `allowHosts` to restrict network access - Use `disableNetwork: true` for tools that don't need network - Always use secrets for credentials, never hardcode in `env` - diff --git a/pkg/gateway/clientpool_test.go b/pkg/gateway/clientpool_test.go index e9eb5c1e..3b984a5e 100644 --- a/pkg/gateway/clientpool_test.go +++ b/pkg/gateway/clientpool_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "testing" "time" @@ -157,6 +158,66 @@ volumes: assert.Empty(t, env) } +func TestApplyConfigVolumeFilterDefaultsHostBindsToReadOnly(t *testing.T) { + hostPath := t.TempDir() + expectedHostPath, err := cleanDockerHostPath(hostPath) + require.NoError(t, err) + catalogYAML := ` +volumes: + - '{{filesystem.paths|volume|into}}' + ` + configYAML := fmt.Sprintf(` +filesystem: + paths: + - %s +`, hostPath) + + args, env := argsAndEnv(t, "filesystem", catalogYAML, configYAML, nil) + + mountIndex := -1 + for i, arg := range args { + if arg == "-v" { + mountIndex = i + 1 + break + } + } + require.NotEqual(t, -1, mountIndex) + require.Less(t, mountIndex, len(args)) + assert.True(t, strings.HasPrefix(args[mountIndex], expectedHostPath+":")) + assert.True(t, strings.HasSuffix(args[mountIndex], ":ro")) + assert.Empty(t, env) +} + +func TestApplyConfigVolumeFilterAllowsWritableHostBindOverride(t *testing.T) { + hostPath := t.TempDir() + expectedHostPath, err := cleanDockerHostPath(hostPath) + require.NoError(t, err) + t.Setenv(dockerBindWritableAllowedPathsEnv, hostPath) + catalogYAML := ` +volumes: + - '{{filesystem.paths|volume|into}}' + ` + configYAML := fmt.Sprintf(` +filesystem: + paths: + - %s +`, hostPath) + + args, env := argsAndEnv(t, "filesystem", catalogYAML, configYAML, nil) + + mountIndex := -1 + for i, arg := range args { + if arg == "-v" { + mountIndex = i + 1 + break + } + } + require.NotEqual(t, -1, mountIndex) + require.Less(t, mountIndex, len(args)) + assert.Equal(t, expectedHostPath+":"+filepath.ToSlash(hostPath)+":rw", args[mountIndex]) + assert.Empty(t, env) +} + func TestApplyConfigMountAsReadOnly(t *testing.T) { hostPath := t.TempDir() expectedHostPath, err := cleanDockerHostPath(hostPath) @@ -222,7 +283,7 @@ func TestApplyConfigLongLivedRejectsWritableHostBind(t *testing.T) { catalogYAML := ` longLived: true volumes: - - '/local/data:/data' + - '/local/data:/data:rw' ` _, _, err := argsAndEnvErr(t, "longlived", catalogYAML, "", nil) @@ -232,7 +293,7 @@ volumes: func TestApplyConfigShortLivedRejectsWritableHostBind(t *testing.T) { catalogYAML := ` volumes: - - '/local/data:/data' + - '/local/data:/data:rw' ` _, _, err := argsAndEnvErr(t, "shortlived", catalogYAML, "", nil) @@ -310,11 +371,48 @@ func TestValidateDockerVolumeBinds(t *testing.T) { require.NoError(t, validateDockerVolumeBinds([]string{t.TempDir() + ":/data:ro"})) }) + t.Run("defaults host binds without mode to read-only", func(t *testing.T) { + hostPath := t.TempDir() + expectedSource, err := cleanDockerHostPath(hostPath) + require.NoError(t, err) + + normalized, err := normalizeDockerVolumeBind(hostPath + ":/data") + require.NoError(t, err) + require.Equal(t, expectedSource+":/data:ro", normalized) + }) + t.Run("allows read-only binds from configured roots", func(t *testing.T) { t.Setenv(dockerBindAllowedPathsEnv, "/opt/mcp-data") require.NoError(t, validateDockerVolumeBinds([]string{"/opt/mcp-data/project:/data:ro"})) }) + t.Run("allows writable binds from configured writable paths", func(t *testing.T) { + hostPath := t.TempDir() + expectedSource, err := cleanDockerHostPath(hostPath) + require.NoError(t, err) + t.Setenv(dockerBindWritableAllowedPathsEnv, hostPath) + + require.NoError(t, validateDockerVolumeBinds([]string{hostPath + ":/data:rw"})) + + normalized, err := normalizeDockerVolumeBind(hostPath + ":/data") + require.NoError(t, err) + require.Equal(t, expectedSource+":/data:rw", normalized) + + normalized, err = normalizeDockerVolumeBind(hostPath + ":/data:rw") + require.NoError(t, err) + require.Equal(t, expectedSource+":/data:rw", normalized) + }) + + t.Run("configured writable paths do not allow child paths", func(t *testing.T) { + root := t.TempDir() + project := filepath.Join(root, "project") + require.NoError(t, os.MkdirAll(project, 0o755)) + t.Setenv(dockerBindWritableAllowedPathsEnv, root) + + err := validateDockerVolumeBinds([]string{project + ":/data:rw"}) + require.ErrorContains(t, err, "host path bind mounts must be read-only") + }) + t.Run("allows home paths from configured roots", func(t *testing.T) { home := t.TempDir() project := filepath.Join(home, "trusted", "project") @@ -333,11 +431,13 @@ func TestValidateDockerVolumeBinds(t *testing.T) { t.Run("rejects host paths outside allowed roots", func(t *testing.T) { err := validateDockerVolumeBinds([]string{"/opt/mcp-data:/data:ro"}) require.ErrorContains(t, err, "outside allowed roots") + require.ErrorContains(t, err, "MCP_GATEWAY_DOCKER_BIND_ALLOWED_PATHS=/opt/mcp-data") + require.ErrorContains(t, err, "MCP_GATEWAY_DOCKER_BIND_ALLOW_WRITABLE_PATHS=/opt/mcp-data") }) t.Run("rejects relative host paths with slash", func(t *testing.T) { err := validateDockerVolumeBinds([]string{"foo/bar:/data"}) - require.ErrorContains(t, err, "host path bind mounts must be read-only") + require.ErrorContains(t, err, "outside allowed roots") err = validateDockerVolumeBinds([]string{"foo/bar:/data:ro"}) require.ErrorContains(t, err, "outside allowed roots") @@ -345,15 +445,16 @@ func TestValidateDockerVolumeBinds(t *testing.T) { t.Run("rejects relative host paths escaping upward", func(t *testing.T) { err := validateDockerVolumeBinds([]string{"subdir/../../../etc:/data"}) - require.ErrorContains(t, err, "host path bind mounts must be read-only") + require.ErrorContains(t, err, "outside allowed roots") err = validateDockerVolumeBinds([]string{"subdir/../../../etc:/data:ro"}) require.ErrorContains(t, err, "outside allowed roots") }) - t.Run("rejects writable host path binds", func(t *testing.T) { - err := validateDockerVolumeBinds([]string{t.TempDir() + ":/data"}) + t.Run("rejects explicitly writable host path binds", func(t *testing.T) { + err := validateDockerVolumeBinds([]string{t.TempDir() + ":/data:rw"}) require.ErrorContains(t, err, "host path bind mounts must be read-only") + require.ErrorContains(t, err, "MCP_GATEWAY_DOCKER_BIND_ALLOW_WRITABLE_PATHS=") }) t.Run("rejects docker socket binds", func(t *testing.T) { @@ -376,6 +477,33 @@ func TestValidateDockerVolumeBinds(t *testing.T) { err := validateDockerVolumeBinds([]string{filepath.Join(t.TempDir(), ".ssh") + ":/ssh:ro"}) require.ErrorContains(t, err, "credential path") }) + + t.Run("rejects credential paths even under writable paths", func(t *testing.T) { + sshPath := filepath.Join(t.TempDir(), ".ssh") + t.Setenv(dockerBindWritableAllowedPathsEnv, sshPath) + + err := validateDockerVolumeBinds([]string{sshPath + ":/ssh:rw"}) + require.ErrorContains(t, err, "credential path") + }) + + t.Run("rejects symlink system paths even under writable paths", func(t *testing.T) { + root := t.TempDir() + link := filepath.Join(root, "etc-link") + if err := os.Symlink("/etc", link); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + t.Setenv(dockerBindWritableAllowedPathsEnv, link) + + err := validateDockerVolumeBinds([]string{filepath.ToSlash(link) + ":/data:rw"}) + require.ErrorContains(t, err, "sensitive system path") + }) + + t.Run("rejects docker socket even under writable paths", func(t *testing.T) { + t.Setenv(dockerBindWritableAllowedPathsEnv, "/var/run/docker.sock") + + err := validateDockerVolumeBinds([]string{"/var/run/docker.sock:/var/run/docker.sock:rw"}) + require.ErrorContains(t, err, "blocked") + }) } func argsAndEnv(t *testing.T, name, catalogYAML, configYAML string, secrets map[string]string) ([]string, []string) { diff --git a/pkg/gateway/docker_binds.go b/pkg/gateway/docker_binds.go index 9af610a8..c83fa5f8 100644 --- a/pkg/gateway/docker_binds.go +++ b/pkg/gateway/docker_binds.go @@ -17,6 +17,10 @@ import ( // default read-only temp-root allowlist. Use the OS path-list separator. const dockerBindAllowedPathsEnv = "MCP_GATEWAY_DOCKER_BIND_ALLOWED_PATHS" +// MCP_GATEWAY_DOCKER_BIND_ALLOW_WRITABLE_PATHS adds trusted host paths that may +// be mounted writable. Use the OS path-list separator. +const dockerBindWritableAllowedPathsEnv = "MCP_GATEWAY_DOCKER_BIND_ALLOW_WRITABLE_PATHS" + type dockerVolumeBind struct { raw string source string @@ -29,8 +33,9 @@ type dockerVolumeBind struct { func validateDockerVolumeBinds(volumes []string) error { allowedRoots := dockerBindAllowedRoots() + writableAllowedPaths := dockerBindWritableAllowedPaths() for _, raw := range volumes { - if _, err := normalizeDockerVolumeBindWithRoots(raw, allowedRoots); err != nil { + if _, err := normalizeDockerVolumeBindWithRoots(raw, allowedRoots, writableAllowedPaths); err != nil { return err } } @@ -38,10 +43,10 @@ func validateDockerVolumeBinds(volumes []string) error { } func normalizeDockerVolumeBind(raw string) (string, error) { - return normalizeDockerVolumeBindWithRoots(raw, dockerBindAllowedRoots()) + return normalizeDockerVolumeBindWithRoots(raw, dockerBindAllowedRoots(), dockerBindWritableAllowedPaths()) } -func normalizeDockerVolumeBindWithRoots(raw string, allowedRoots []string) (string, error) { +func normalizeDockerVolumeBindWithRoots(raw string, allowedRoots, writableAllowedPaths []string) (string, error) { bind, err := parseDockerVolumeBind(raw) if err != nil { return "", err @@ -49,15 +54,25 @@ func normalizeDockerVolumeBindWithRoots(raw string, allowedRoots []string) (stri if !bind.hostPath { return bind.raw, nil } - if !bind.readOnly { - return "", fmt.Errorf("unsafe docker volume %q: host path bind mounts must be read-only", bind.raw) - } if disallowed, reason := disallowedDockerHostPath(bind.sourcePath); disallowed { return "", fmt.Errorf("unsafe docker volume %q: host path %q is blocked (%s)", bind.raw, bind.source, reason) } - if !isPathUnderAnyRoot(bind.sourcePath, allowedRoots) { - return "", fmt.Errorf("unsafe docker volume %q: host path %q is outside allowed roots %s", - bind.raw, bind.source, strings.Join(allowedRoots, ", ")) + writableAllowed := slices.Contains(writableAllowedPaths, bind.sourcePath) + if bind.mode == "" { + if writableAllowed { + bind.mode = "rw" + } else { + bind.mode = "ro" + bind.readOnly = true + } + } + if !bind.readOnly && !writableAllowed { + return "", fmt.Errorf("unsafe docker volume %q: host path bind mounts must be read-only unless the exact host path is allowed for writable binds. To allow this writable host path, run with %s=%s", + bind.raw, dockerBindWritableAllowedPathsEnv, bind.source) + } + if !isPathUnderAnyRoot(bind.sourcePath, allowedRoots) && !slices.Contains(writableAllowedPaths, bind.sourcePath) { + return "", fmt.Errorf("unsafe docker volume %q: host path %q is outside allowed roots %s. To allow read-only access, run with %s=%s or another trusted parent directory. To allow writable access, run with %s=%s", + bind.raw, bind.source, strings.Join(allowedRoots, ", "), dockerBindAllowedPathsEnv, bind.source, dockerBindWritableAllowedPathsEnv, bind.source) } return bind.sourcePath + ":" + bind.target + ":" + bind.mode, nil } @@ -258,7 +273,17 @@ func dockerBindAllowedRoots() []string { if env := os.Getenv(dockerBindAllowedPathsEnv); env != "" { roots = append(roots, filepath.SplitList(env)...) } + return cleanDockerBindRoots(roots) +} + +func dockerBindWritableAllowedPaths() []string { + if env := os.Getenv(dockerBindWritableAllowedPathsEnv); env != "" { + return cleanDockerBindRoots(filepath.SplitList(env)) + } + return nil +} +func cleanDockerBindRoots(roots []string) []string { out := make([]string, 0, len(roots)) for _, root := range roots { root = strings.TrimSpace(root)