From 0fcb927b1f14dc0670009eacb32df49553eb9e7d Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 25 Jun 2026 20:22:08 +0100 Subject: [PATCH 1/2] Add tests for --allow-docker-gateway behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The --allow-docker-gateway flag controls whether the egress proxy reaches Docker gateway addresses (host.docker.internal, gateway.docker.internal, the bridge gateway IP) under network isolation. Reviewers noted the flag's end-to-end behavior was effectively untested, and the deny/allow interaction with permission profiles is subtle (Squid is first-match-wins), so add coverage that pins it down. - squid_test: assert that listing host.docker.internal in allow_host without the flag is still blocked (deny precedes the ACL allow), and is allowed with the flag — the exact profile/flag interaction users hit. - client_deploy_test: guard that AllowDockerGateway defaults to not-forwarded so the gateway deny rules stay in place, and that the DNS container is created on the isolation path. - e2e: verify the egress squid.conf carries the gateway deny rules by default and drops them with the flag, and (where the bridge gateway routes to the host) that a fetch to the gateway succeeds only with the flag. Relates to #5640 Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/container/docker/client_deploy_test.go | 34 +++++ pkg/container/docker/squid_test.go | 39 ++++++ test/e2e/network_isolation_test.go | 139 +++++++++++++++++++++ 3 files changed, 212 insertions(+) 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..c6fd0be2df 100644 --- a/test/e2e/network_isolation_test.go +++ b/test/e2e/network_isolation_test.go @@ -6,9 +6,13 @@ package e2e_test import ( "context" "fmt" + "io" + "net" "net/http" "os" + "os/exec" "path/filepath" + "strings" "time" . "github.com/onsi/ginkgo/v2" @@ -141,4 +145,139 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e verifyNetworkRestrictions("default") }) }) + + Describe("Reaching the host with --allow-docker-gateway", func() { + const gatewayHost = "host.docker.internal" + + // 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()) + } + } + + // fetchBlocked drives the fetch tool against the given server and reports + // whether the request was blocked (the fetch tool returns an error result + // when the egress proxy denies the request). + fetchBlocked := func(serverName, targetURL string) bool { + 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.IsError + } + + // 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("blocks the docker gateway by default and 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() }) + + gatewayIP := dockerBridgeGatewayIP() + target := fmt.Sprintf("http://%s:%d/", gatewayIP, port) + + By("Confirming the gateway is blocked by default, even under allow-all") + denyServer := startFetchServer("deny", "") + Expect(fetchBlocked(denyServer, target)).To(BeTrue(), + "gateway IP must be blocked without --allow-docker-gateway") + Expect(fetchBlocked(denyServer, fmt.Sprintf("http://%s:80/", gatewayHost))).To(BeTrue(), + "host.docker.internal must be blocked without --allow-docker-gateway") + retireServer(denyServer) + + By("Confirming the flag removes the deny so the host becomes reachable") + allowServer := startFetchServer("allow", "", "--allow-docker-gateway") + if fetchBlocked(allowServer, target) { + // The deny removal is proven by the negative assertions above and + // the config test; whether the bridge gateway actually routes to a + // host service is environment-specific (works on Linux Docker + // Engine; on Docker Desktop the host lives behind host.docker.internal + // instead). Skip the positive reachability leg where it does not apply. + Skip("docker bridge gateway is not routable to the host in this environment") + } + }) + + It("removes the egress proxy gateway deny rules only when the flag is set", 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: gateway 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")) + retireServer(denyServer) + + By("With --allow-docker-gateway: gateway deny rules are absent") + allowServer := startFetchServer("cfg-allow", "", "--allow-docker-gateway") + Expect(squidConf(allowServer)).ToNot(ContainSubstring("docker_gateway_hosts"), + "--allow-docker-gateway must remove the gateway deny rules") + }) + }) }) From d28bb69d5c5a81c86aee9fb61171571970eaee5c Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 25 Jun 2026 20:40:00 +0100 Subject: [PATCH 2/2] Address review feedback on gateway e2e tests - Drop the host.docker.internal runtime fetch leg: it could pass because DNS resolution fails (the isolated resolver can't resolve the name, per #5640) rather than because the egress deny fired, so it could not prove the security boundary. The gateway-IP leg is DNS-independent and genuinely exercises the deny; the hostname deny rule is pinned by the config test. - Make the positive leg affirmatively assert the fetched body ("host-service-ok") instead of asserting nothing on the reachable branch. - Pin the direct-IP deny (docker_gateway_ip) in the config test: assert it is present by default and absent with the flag, mirroring the hostname rule. - Clarify that the config test complements (not duplicates) the unit test: it proves thv generates and mounts the rules into the live egress container and pins the hostname rule the traffic test cannot exercise without the DNS confounder, alongside the real-traffic assertion. Co-Authored-By: Claude Opus 4.8 (1M context) --- test/e2e/network_isolation_test.go | 84 ++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 28 deletions(-) diff --git a/test/e2e/network_isolation_test.go b/test/e2e/network_isolation_test.go index c6fd0be2df..5d01fe8adc 100644 --- a/test/e2e/network_isolation_test.go +++ b/test/e2e/network_isolation_test.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "github.com/mark3labs/mcp-go/mcp" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -147,8 +148,6 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e }) Describe("Reaching the host with --allow-docker-gateway", func() { - const gatewayHost = "host.docker.internal" - // 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 @@ -189,10 +188,10 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e } } - // fetchBlocked drives the fetch tool against the given server and reports - // whether the request was blocked (the fetch tool returns an error result - // when the egress proxy denies the request). - fetchBlocked := func(serverName, targetURL string) bool { + // 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) @@ -208,7 +207,18 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e result, err := mcpClient.CallTool(ctx, "fetch", map[string]interface{}{"url": targetURL}) Expect(err).ToNot(HaveOccurred(), "CallTool should complete without transport error") - return result.IsError + 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 @@ -222,7 +232,7 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e return strings.TrimSpace(string(out)) } - It("blocks the docker gateway by default and reaches it with the flag", func() { + 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") @@ -234,30 +244,44 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e 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 is blocked by default, even under allow-all") + By("Confirming the gateway IP is blocked by the egress proxy by default") denyServer := startFetchServer("deny", "") - Expect(fetchBlocked(denyServer, target)).To(BeTrue(), - "gateway IP must be blocked without --allow-docker-gateway") - Expect(fetchBlocked(denyServer, fmt.Sprintf("http://%s:80/", gatewayHost))).To(BeTrue(), - "host.docker.internal must be blocked without --allow-docker-gateway") + Expect(fetchThrough(denyServer, target).IsError).To(BeTrue(), + "the egress proxy must deny the bridge gateway IP without --allow-docker-gateway") retireServer(denyServer) - By("Confirming the flag removes the deny so the host becomes reachable") + By("Confirming --allow-docker-gateway removes the deny so the host is reachable") allowServer := startFetchServer("allow", "", "--allow-docker-gateway") - if fetchBlocked(allowServer, target) { - // The deny removal is proven by the negative assertions above and - // the config test; whether the bridge gateway actually routes to a - // host service is environment-specific (works on Linux Docker - // Engine; on Docker Desktop the host lives behind host.docker.internal - // instead). Skip the positive reachability leg where it does not apply. - Skip("docker bridge gateway is not routable to the host in this environment") + 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") }) - It("removes the egress proxy gateway deny rules only when the flag is set", func() { + // 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", @@ -266,18 +290,22 @@ var _ = Describe("NetworkIsolation", Label("proxy", "network", "isolation", "e2e return string(out) } - By("Default profile: gateway deny rules are present") + 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")) + 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: gateway deny rules are absent") - allowServer := startFetchServer("cfg-allow", "", "--allow-docker-gateway") - Expect(squidConf(allowServer)).ToNot(ContainSubstring("docker_gateway_hosts"), - "--allow-docker-gateway must remove the gateway deny rules") + 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") }) }) })