diff --git a/pkg/container/docker/client_deploy_test.go b/pkg/container/docker/client_deploy_test.go index 592bf51d7e..3b1a98f223 100644 --- a/pkg/container/docker/client_deploy_test.go +++ b/pkg/container/docker/client_deploy_test.go @@ -312,6 +312,40 @@ func TestDeployWorkload_AllowDockerGateway_ForwardedToEgress(t *testing.T) { assert.True(t, fops.egressAllowDockerGW, "AllowDockerGateway must be forwarded to createEgressSquidContainer") } +// TestDeployWorkload_AllowDockerGateway_DefaultsToNotForwarded guards against a +// default flip: when the caller does not opt in, the egress proxy must keep its +// Docker-gateway deny rules. It also confirms the DNS container is spawned on the +// isolation path (the MCP server's resolver). +func TestDeployWorkload_AllowDockerGateway_DefaultsToNotForwarded(t *testing.T) { + t.Parallel() + + fops := &fakeDeployOps{dnsIP: "172.18.0.10"} + c := newClientWithOps(fops) + + opts := runtime.NewDeployWorkloadOptions() + opts.AttachStdio = true + // AllowDockerGateway intentionally left at its zero value (false). + + _, err := c.DeployWorkload( + t.Context(), + "ghcr.io/example/mcp:latest", + "app", + []string{"serve"}, + map[string]string{}, + map[string]string{}, + &permissions.Profile{}, + "stdio", + opts, + true, // isolateNetwork required for egress container to be created + ) + require.NoError(t, err) + + require.True(t, fops.egressCalled, "egress container must be created when isolateNetwork=true") + assert.False(t, fops.egressAllowDockerGW, + "AllowDockerGateway must default to false so the gateway deny rules stay in place") + assert.True(t, fops.dnsCalled, "DNS container must be created on the isolation path") +} + func TestDeployWorkload_UnsupportedTransport_PropagatesError(t *testing.T) { t.Parallel() diff --git a/pkg/container/docker/squid_test.go b/pkg/container/docker/squid_test.go index 341f630230..62cc1a8d80 100644 --- a/pkg/container/docker/squid_test.go +++ b/pkg/container/docker/squid_test.go @@ -358,6 +358,45 @@ func TestCreateTempEgressSquidConf_DockerGatewayBlocking(t *testing.T) { "http_access allow allowed_ports allowed_dsts", }, }, + { + // Listing host.docker.internal in allow_host is NOT sufficient on its + // own: without the opt-in the gateway deny is still written, and + // because Squid is first-match-wins the deny (asserted to precede the + // allow below) blocks the request before the allowed_dsts allow is + // reached. Reaching the gateway requires BOTH the flag and the host. + name: "host.docker.internal in allow_host without opt-in is still blocked", + permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + AllowHost: []string{"host.docker.internal"}, + AllowPort: []int{8080}, + }, + }, + allowDockerGateway: false, + expectDenyRule: true, + expectAllowAll: false, + expectContains: []string{ + "acl allowed_dsts dstdomain host.docker.internal", + "http_access allow allowed_ports allowed_dsts", + }, + }, + { + // With the opt-in the deny is dropped and the ACL allow for + // host.docker.internal takes effect. + name: "host.docker.internal in allow_host with opt-in is allowed via ACL", + permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + AllowHost: []string{"host.docker.internal"}, + AllowPort: []int{8080}, + }, + }, + allowDockerGateway: true, + expectDenyRule: false, + expectAllowAll: false, + expectContains: []string{ + "acl allowed_dsts dstdomain host.docker.internal", + "http_access allow allowed_ports allowed_dsts", + }, + }, } for _, tt := range tests { diff --git a/test/e2e/network_isolation_test.go b/test/e2e/network_isolation_test.go index 525103430f..5d01fe8adc 100644 --- a/test/e2e/network_isolation_test.go +++ b/test/e2e/network_isolation_test.go @@ -6,11 +6,16 @@ package e2e_test import ( "context" "fmt" + "io" + "net" "net/http" "os" + "os/exec" "path/filepath" + "strings" "time" + "github.com/mark3labs/mcp-go/mcp" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -141,4 +146,166 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e verifyNetworkRestrictions("default") }) }) + + Describe("Reaching the host with --allow-docker-gateway", func() { + // startFetchServer runs the fetch MCP server under network isolation with + // the given extra run args (e.g. --allow-docker-gateway), waits for it to + // be running, and returns its workload name. An empty profileJSON uses the + // default (allow-all) network profile. + startFetchServer := func(nameSuffix, profileJSON string, extraRunArgs ...string) string { + serverName := fmt.Sprintf("ni-gw-%s-%d", nameSuffix, GinkgoRandomSeed()) + DeferCleanup(func() { + if config.CleanupAfter { + // Best-effort: a test may have already removed this server to + // avoid running two isolation stacks at once. + _ = e2e.StopAndRemoveMCPServer(config, serverName) + } + }) + + runArgs := append([]string{"run", "--name", serverName}, extraRunArgs...) + if profileJSON != "" { + profilePath := filepath.Join(permissionProfileDir, nameSuffix+".json") + err := os.WriteFile(profilePath, []byte(profileJSON), 0644) + Expect(err).ToNot(HaveOccurred(), "Should be able to write permission profile") + runArgs = append(runArgs, "--permission-profile", profilePath) + } + runArgs = append(runArgs, "fetch") + + e2e.NewTHVCommand(config, runArgs...).ExpectSuccess() + // The fetch server can take ~70s to become healthy; allow generous + // headroom so the test does not flake. + err := e2e.WaitForMCPServer(config, serverName, 120*time.Second) + Expect(err).ToNot(HaveOccurred(), "Server should be running within 120 seconds") + return serverName + } + + // retireServer tears a server down mid-test so only one network-isolation + // stack (MCP + dns + egress + ingress containers) runs at a time, avoiding + // resource contention that slows the next server's startup. + retireServer := func(serverName string) { + if config.CleanupAfter { + Expect(e2e.StopAndRemoveMCPServer(config, serverName)).To(Succeed()) + } + } + + // fetchThrough drives the fetch tool against the given server and returns + // the tool result. A denied request comes back as an error result + // (result.IsError); a successful one carries the fetched body. + fetchThrough := func(serverName, targetURL string) *mcp.CallToolResult { + serverURL, err := e2e.GetMCPServerURL(config, serverName) + Expect(err).ToNot(HaveOccurred(), "Should be able to get server URL") + err = e2e.WaitForMCPServerReady(config, serverURL, "streamable-http", 60*time.Second) + Expect(err).ToNot(HaveOccurred(), "Server should be ready") + + mcpClient, err := e2e.NewMCPClientForStreamableHTTP(config, serverURL) + Expect(err).ToNot(HaveOccurred(), "Should be able to create MCP client") + defer mcpClient.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + Expect(mcpClient.Initialize(ctx)).To(Succeed(), "Should be able to initialize MCP client") + + result, err := mcpClient.CallTool(ctx, "fetch", map[string]interface{}{"url": targetURL}) + Expect(err).ToNot(HaveOccurred(), "CallTool should complete without transport error") + return result + } + + // resultText concatenates the text content of a tool result. + resultText := func(result *mcp.CallToolResult) string { + var sb strings.Builder + for _, c := range result.Content { + if tc, ok := mcp.AsTextContent(c); ok { + sb.WriteString(tc.Text) + } + } + return sb.String() + } + + // dockerBridgeGatewayIP returns the host gateway IP of the default Docker + // bridge — the same address the egress proxy denies by default, and the + // address a container uses to reach the host on Linux. + dockerBridgeGatewayIP := func() string { + //nolint:gosec // fixed, test-controlled arguments + out, err := exec.Command("docker", "network", "inspect", "bridge", + "-f", "{{range .IPAM.Config}}{{.Gateway}}{{end}}").Output() + Expect(err).ToNot(HaveOccurred(), "Should be able to inspect the docker bridge network") + return strings.TrimSpace(string(out)) + } + + It("denies the bridge gateway IP by default and, where routable, reaches it with the flag", func() { + By("Starting a host service reachable from containers") + listener, err := net.Listen("tcp", ":0") //nolint:gosec // binds an ephemeral port for the test + Expect(err).ToNot(HaveOccurred(), "Should be able to listen on an ephemeral port") + port := listener.Addr().(*net.TCPAddr).Port + srv := &http.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = io.WriteString(w, "host-service-ok") }), + ReadHeaderTimeout: 5 * time.Second, + } + go func() { _ = srv.Serve(listener) }() + DeferCleanup(func() { _ = srv.Close() }) + + // Target the bridge gateway IP, not host.docker.internal: an IP needs no + // DNS resolution, so a blocked fetch unambiguously means the egress + // `docker_gateway_ip` deny fired (see #5640 — the isolated resolver + // cannot resolve host.docker.internal, which would confound a + // hostname-based assertion). The hostname deny rule is pinned by the + // config test below instead. + gatewayIP := dockerBridgeGatewayIP() + target := fmt.Sprintf("http://%s:%d/", gatewayIP, port) + + By("Confirming the gateway IP is blocked by the egress proxy by default") + denyServer := startFetchServer("deny", "") + Expect(fetchThrough(denyServer, target).IsError).To(BeTrue(), + "the egress proxy must deny the bridge gateway IP without --allow-docker-gateway") + retireServer(denyServer) + + By("Confirming --allow-docker-gateway removes the deny so the host is reachable") + allowServer := startFetchServer("allow", "", "--allow-docker-gateway") + result := fetchThrough(allowServer, target) + if result.IsError { + // The deny removal is pinned deterministically by the config test + // below; whether the bridge gateway actually routes to a host + // service is environment-specific — it works on Linux Docker Engine + // (where the bridge gateway is the host), but on Docker Desktop the + // host lives behind host.docker.internal, not the bridge gateway. Skip + // the positive reachability leg where the gateway is not host-routable. + Skip("docker bridge gateway is not routable to the host in this environment (e.g. Docker Desktop)") + } + Expect(resultText(result)).To(ContainSubstring("host-service-ok"), + "with --allow-docker-gateway the fetch must reach the host service through the egress proxy") + }) + + // This test pins the egress ACL rules deterministically on a real, deployed + // container — both the hostname deny (which the traffic test above cannot + // exercise without the host.docker.internal DNS confounder of #5640) and the + // direct-IP deny. It complements, rather than duplicates, the unit test for + // createTempEgressSquidConf: it proves thv actually generates and mounts the + // config into the running egress proxy, alongside the real-traffic assertion above. + It("carries both gateway deny rules in the egress config by default and drops them with the flag", func() { + squidConf := func(serverName string) string { + //nolint:gosec // container name is test-controlled + out, err := exec.Command("docker", "exec", serverName+"-egress", + "cat", "/etc/squid/squid.conf").CombinedOutput() + Expect(err).ToNot(HaveOccurred(), "Should be able to read egress squid.conf: %s", string(out)) + return string(out) + } + + By("Default profile: both the hostname and direct-IP deny rules are present") + denyServer := startFetchServer("cfg-deny", "") + denyConf := squidConf(denyServer) + Expect(denyConf).To(ContainSubstring("http_access deny docker_gateway_hosts"), + "default config must deny the docker gateway hostnames") + Expect(denyConf).To(ContainSubstring("dstdomain host.docker.internal gateway.docker.internal")) + Expect(denyConf).To(ContainSubstring("http_access deny docker_gateway_ip"), + "default config must deny the docker gateway IP (the DNS-bypass path)") + retireServer(denyServer) + + By("With --allow-docker-gateway: both deny rules are absent") + allowConf := squidConf(startFetchServer("cfg-allow", "", "--allow-docker-gateway")) + Expect(allowConf).ToNot(ContainSubstring("docker_gateway_hosts"), + "--allow-docker-gateway must remove the gateway hostname deny rule") + Expect(allowConf).ToNot(ContainSubstring("docker_gateway_ip"), + "--allow-docker-gateway must remove the gateway IP deny rule") + }) + }) })