From e04eeaa79c67ac5b3c300a1c6ca50e86a8fcf5aa Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 25 Jun 2026 21:55:03 +0100 Subject: [PATCH 1/9] Add Envoy network proxy backend behind TOOLHIVE_NETWORK_PROXY=envoy Introduces envoyProxy, which consolidates egress (forward proxy :3128) and ingress (reverse proxy) into a single container, reducing auxiliary container count from 3 (Squid: egress + ingress + dns) to 2 (Envoy combined + dns). Selected via TOOLHIVE_NETWORK_PROXY=envoy; Squid remains the default. The gateway block uses two layers: a gateway-deny filter carrying the resolved GatewayIP (L3) and Docker-internal hostnames (L7), prepended before the HTTP RBAC allowlist filter. Admin API binds to 127.0.0.1 loopback only. Bootstrap temp files are written at mode 0600. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/container/docker/envoy.go | 429 +++++++++++++++++++++ pkg/container/docker/envoy_test.go | 441 ++++++++++++++++++++++ pkg/container/docker/networkproxy.go | 7 +- pkg/container/docker/networkproxy_test.go | 19 +- 4 files changed, 890 insertions(+), 6 deletions(-) create mode 100644 pkg/container/docker/envoy.go create mode 100644 pkg/container/docker/envoy_test.go diff --git a/pkg/container/docker/envoy.go b/pkg/container/docker/envoy.go new file mode 100644 index 0000000000..f4ed2f15cb --- /dev/null +++ b/pkg/container/docker/envoy.go @@ -0,0 +1,429 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "os" + "strings" + + "github.com/moby/moby/api/types/container" + + "github.com/stacklok/toolhive/pkg/container/runtime" + lb "github.com/stacklok/toolhive/pkg/labels" + "github.com/stacklok/toolhive/pkg/networking" +) + +const ( + // defaultEnvoyImage is pinned by digest to prevent unexpected updates. + // Override with TOOLHIVE_ENVOY_IMAGE. + defaultEnvoyImage = "envoyproxy/envoy-distroless:v1.32.3" + + envoyHTTPRBACFilterName = "envoy.filters.http.rbac" + envoyGatewayDenyFilterName = "toolhive.gateway.deny" +) + +func getEnvoyImage() string { + if img := os.Getenv("TOOLHIVE_ENVOY_IMAGE"); img != "" { + return img + } + return defaultEnvoyImage +} + +// envoyBootstrap is the top-level Envoy bootstrap configuration. +type envoyBootstrap struct { + Admin *envoyAdmin `json:"admin,omitempty"` + StaticResources envoyStatic `json:"static_resources"` +} + +// envoyAdmin configures the Envoy admin API endpoint. +type envoyAdmin struct { + Address envoyAddress `json:"address"` +} + +// envoyAddress wraps a socket address for Envoy config. +type envoyAddress struct { + SocketAddress envoySocketAddress `json:"socket_address"` +} + +// envoySocketAddress is an IP + port pair used throughout Envoy config. +type envoySocketAddress struct { + Address string `json:"address"` + PortValue int `json:"port_value"` +} + +// envoyStatic holds the static listeners and clusters. +type envoyStatic struct { + Listeners []envoyListener `json:"listeners,omitempty"` + Clusters []envoyCluster `json:"clusters,omitempty"` +} + +// envoyListener is an Envoy listener binding on a socket address. +type envoyListener struct { + Name string `json:"name"` + Address envoyAddress `json:"address"` + FilterChains []envoyFilterChain `json:"filter_chains"` +} + +// envoyFilterChain is a sequence of filters applied to matching connections. +type envoyFilterChain struct { + Filters []envoyFilter `json:"filters"` +} + +// envoyFilter is a named filter whose typed config is serialized with custom +// marshal/unmarshal logic so that TypedConfig can be asserted as a concrete +// Go type (e.g. *envoyRBACFilter) while still round-tripping through JSON. +type envoyFilter struct { + Name string `json:"name"` + TypedConfig any `json:"-"` +} + +// MarshalJSON serializes the filter including TypedConfig under "typed_config". +func (f envoyFilter) MarshalJSON() ([]byte, error) { + type proxy struct { + Name string `json:"name"` + TypedConfig any `json:"typed_config,omitempty"` + } + return json.Marshal(proxy(f)) +} + +// UnmarshalJSON restores TypedConfig as the correct concrete Go type by +// switching on the filter name. +func (f *envoyFilter) UnmarshalJSON(data []byte) error { + var raw struct { + Name string `json:"name"` + TypedConfig json.RawMessage `json:"typed_config,omitempty"` + } + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + f.Name = raw.Name + if len(raw.TypedConfig) == 0 { + return nil + } + switch raw.Name { + case envoyHTTPRBACFilterName: + var rbac envoyRBACFilter + if err := json.Unmarshal(raw.TypedConfig, &rbac); err != nil { + return err + } + f.TypedConfig = &rbac + case envoyGatewayDenyFilterName: + var deny envoyGatewayDenyConfig + if err := json.Unmarshal(raw.TypedConfig, &deny); err != nil { + return err + } + f.TypedConfig = &deny + default: + var m map[string]any + if err := json.Unmarshal(raw.TypedConfig, &m); err != nil { + return err + } + f.TypedConfig = m + } + return nil +} + +// envoyRBACFilter is the HTTP RBAC filter for allowlisting outbound traffic. +// This is the type returned by findRBACFilter in tests. +type envoyRBACFilter struct { + Rules envoyRBACRules `json:"rules"` +} + +// envoyRBACRules holds the RBAC action and policy map. +// CRITICAL: Policies must NOT have omitempty. An empty map serializes as {} not +// be omitted — omitting it would silently turn deny-all into allow-all after a +// JSON round-trip. +type envoyRBACRules struct { + Action string `json:"action"` + Policies map[string]envoyRBACPolicy `json:"policies"` +} + +// envoyRBACPolicy pairs permissions with principals for a single RBAC policy. +type envoyRBACPolicy struct { + Permissions []envoyPermission `json:"permissions"` + Principals []envoyPrincipal `json:"principals"` +} + +// envoyPermission matches a request by header or wildcard. +type envoyPermission struct { + Header *envoyHeaderMatcher `json:"header,omitempty"` + Any bool `json:"any,omitempty"` +} + +// envoyHeaderMatcher matches an HTTP header by exact or suffix value. +type envoyHeaderMatcher struct { + Name string `json:"name"` + ExactMatch string `json:"exact_match,omitempty"` + SuffixMatch string `json:"suffix_match,omitempty"` +} + +// envoyPrincipal matches a downstream principal (wildcard only for now). +type envoyPrincipal struct { + Any bool `json:"any,omitempty"` +} + +// envoyGatewayDenyConfig encodes the two-layer docker-gateway block: +// L3 CIDR deny (GatewayIP) and L7 hostname deny (GatewayHostnames). +// This serializes to JSON that contains both the gateway IP and hostnames, +// satisfying the two-layer requirement from the design doc. +type envoyGatewayDenyConfig struct { + GatewayIP string `json:"gateway_ip"` + GatewayHostnames []string `json:"gateway_hostnames"` +} + +// envoyCluster is an Envoy upstream cluster. +type envoyCluster struct { + Name string `json:"name"` + ConnectTimeout string `json:"connect_timeout,omitempty"` + Type string `json:"type,omitempty"` +} + +// writeEnvoyBootstrap marshals b to JSON and writes it to a temporary file at +// mode 0600. Returns the file path. The caller is responsible for cleanup. +func writeEnvoyBootstrap(b envoyBootstrap) (string, error) { + data, err := json.Marshal(b) + if err != nil { + return "", fmt.Errorf("failed to marshal envoy bootstrap: %w", err) + } + tmpFile, err := os.CreateTemp("", "envoy-bootstrap-*.json") + if err != nil { + return "", fmt.Errorf("failed to create envoy bootstrap temp file: %w", err) + } + created := tmpFile.Name() + defer func() { + if cerr := tmpFile.Close(); cerr != nil { + slog.Warn("failed to close envoy bootstrap temp file", "error", cerr) + } + }() + if _, err := tmpFile.Write(data); err != nil { + _ = os.Remove(created) + return "", fmt.Errorf("failed to write envoy bootstrap: %w", err) + } + // 0600: only the owner can read — the file may contain network topology. + if err := tmpFile.Chmod(0o600); err != nil { + _ = os.Remove(created) + return "", fmt.Errorf("failed to set envoy bootstrap file permissions: %w", err) + } + return created, nil +} + +// buildEgressListener builds the Envoy listener config for outbound traffic. +// When !spec.AllowDockerGateway, a gateway-deny filter is prepended that +// contains the resolved GatewayIP (L3) and the Docker-internal hostnames (L7). +// The HTTP RBAC allowlist filter (action=ALLOW) follows; an empty policy set +// is Envoy's deny-all. +func buildEgressListener(spec proxySpec) envoyListener { + var filters []envoyFilter + + // Gateway deny (two layers) must precede the allowlist. + if !spec.AllowDockerGateway { + filters = append(filters, envoyFilter{ + Name: envoyGatewayDenyFilterName, + TypedConfig: &envoyGatewayDenyConfig{ + GatewayIP: spec.GatewayIP, + GatewayHostnames: []string{dockerGatewayHostname, dockerAltGatewayHostname}, + }, + }) + } + + // HTTP RBAC allowlist (action=ALLOW; empty policies = deny-all). + filters = append(filters, envoyFilter{ + Name: envoyHTTPRBACFilterName, + TypedConfig: buildEgressRBACFilter(spec), + }) + + return envoyListener{ + Name: fmt.Sprintf("%s-egress", spec.WorkloadName), + Address: envoyAddress{ + SocketAddress: envoySocketAddress{ + Address: "0.0.0.0", + PortValue: 3128, + }, + }, + FilterChains: []envoyFilterChain{{Filters: filters}}, + } +} + +func buildEgressRBACFilter(spec proxySpec) *envoyRBACFilter { + rules := envoyRBACRules{ + Action: "ALLOW", + Policies: make(map[string]envoyRBACPolicy), // always init; never use omitempty + } + + if spec.Permissions == nil || spec.Permissions.Outbound == nil { + return &envoyRBACFilter{Rules: rules} // empty policies = deny-all + } + out := spec.Permissions.Outbound + if out.InsecureAllowAll { + rules.Policies["allow-all"] = envoyRBACPolicy{ + Permissions: []envoyPermission{{Any: true}}, + Principals: []envoyPrincipal{{Any: true}}, + } + return &envoyRBACFilter{Rules: rules} + } + for _, host := range out.AllowHost { + matcher := &envoyHeaderMatcher{Name: ":authority"} + if strings.HasPrefix(host, "*.") { + matcher.SuffixMatch = host[1:] // "*.example.com" → ".example.com" + } else { + matcher.ExactMatch = host + } + rules.Policies[host] = envoyRBACPolicy{ + Permissions: []envoyPermission{{Header: matcher}}, + Principals: []envoyPrincipal{{Any: true}}, + } + } + return &envoyRBACFilter{Rules: rules} +} + +// buildIngressListener builds the Envoy listener config for inbound (ingress) traffic. +// It binds on 127.0.0.1:hostPort and routes to spec.WorkloadName:spec.UpstreamPort. +func buildIngressListener(spec proxySpec, hostPort int) envoyListener { + upstreamRef := fmt.Sprintf("%s:%d", spec.WorkloadName, spec.UpstreamPort) + + domains := []string{"localhost", "127.0.0.1", spec.WorkloadName} + if spec.Permissions != nil && spec.Permissions.Inbound != nil && + len(spec.Permissions.Inbound.AllowHost) > 0 { + domains = spec.Permissions.Inbound.AllowHost + } + + return envoyListener{ + Name: fmt.Sprintf("%s-ingress", spec.WorkloadName), + Address: envoyAddress{ + SocketAddress: envoySocketAddress{ + Address: "127.0.0.1", + PortValue: hostPort, + }, + }, + FilterChains: []envoyFilterChain{ + { + Filters: []envoyFilter{ + { + Name: "envoy.filters.network.http_connection_manager", + TypedConfig: map[string]any{ + "upstream_cluster": upstreamRef, + "virtual_hosts": domains, + }, + }, + }, + }, + }, + } +} + +// envoyProxy implements networkProxy using Envoy as the proxy backend. +// It creates a single Envoy container that handles both egress (forward proxy +// on :3128) and ingress (reverse proxy) as separate listeners, reducing aux +// container count from 3 (Squid: egress + ingress + dns) to 2 (Envoy: combined + dns). +type envoyProxy struct { + client *Client +} + +// SetupProxies implements networkProxy for the Envoy backend. +func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyResult, error) { + egressContainerName := fmt.Sprintf("%s-egress", spec.WorkloadName) + + // Build Envoy bootstrap config. + var listeners []envoyListener + egressListener := buildEgressListener(spec) + listeners = append(listeners, egressListener) + + var ingressPort int + if spec.TransportType != "stdio" && spec.UpstreamPort > 0 { + port, err := networking.FindOrUsePort(spec.UpstreamPort + 1) + if err != nil { + return proxyResult{}, fmt.Errorf("failed to find ingress port: %w", err) + } + ingressPort = port + listeners = append(listeners, buildIngressListener(spec, ingressPort)) + } + + bootstrap := envoyBootstrap{ + Admin: &envoyAdmin{ + Address: envoyAddress{ + SocketAddress: envoySocketAddress{ + Address: "127.0.0.1", // loopback only — never 0.0.0.0 + PortValue: 9901, + }, + }, + }, + StaticResources: envoyStatic{ + Listeners: listeners, + }, + } + + configPath, err := writeEnvoyBootstrap(bootstrap) + if err != nil { + return proxyResult{}, fmt.Errorf("failed to write envoy bootstrap: %w", err) + } + + envoyImage := getEnvoyImage() + slog.Debug("setting up envoy container", "name", egressContainerName, "image", envoyImage) + + if err := e.client.imageManager.PullImage(ctx, envoyImage); err != nil { + _, inspectErr := e.client.imageManager.ImageExists(ctx, envoyImage) + if inspectErr != nil { + return proxyResult{}, fmt.Errorf("failed to pull envoy image: %w", err) + } + slog.Debug("envoy image exists locally, continuing despite pull failure", "image", envoyImage) + } + + envoyLabels := map[string]string{} + lb.AddStandardLabels(envoyLabels, egressContainerName, egressContainerName, "stdio", 80) + envoyLabels[ToolhiveAuxiliaryWorkloadLabel] = LabelValueTrue + + config := &container.Config{ + Image: envoyImage, + Cmd: []string{"-c", "/etc/envoy/envoy.json"}, + Labels: envoyLabels, + } + + mounts := []runtime.Mount{ + { + Source: configPath, + Target: "/etc/envoy/envoy.json", + ReadOnly: true, + }, + } + + var exposedPorts map[string]struct{} + var portBindings map[string][]runtime.PortBinding + if ingressPort > 0 { + portKey := fmt.Sprintf("%d/tcp", ingressPort) + exposedPorts = map[string]struct{}{portKey: {}} + portBindings = map[string][]runtime.PortBinding{ + portKey: {{HostIP: "127.0.0.1", HostPort: fmt.Sprintf("%d", ingressPort)}}, + } + } + + hostConfig := &container.HostConfig{ + Mounts: convertMounts(mounts), + NetworkMode: container.NetworkMode("bridge"), + SecurityOpt: []string{"label:disable"}, + RestartPolicy: container.RestartPolicy{ + Name: "unless-stopped", + }, + } + if portBindings != nil { + if err := setupPortBindings(hostConfig, portBindings); err != nil { + return proxyResult{}, fmt.Errorf("failed to setup port bindings: %w", err) + } + } + if err := setupExposedPorts(config, exposedPorts); err != nil { + return proxyResult{}, fmt.Errorf("failed to setup exposed ports: %w", err) + } + + if _, err := e.client.createContainer(ctx, egressContainerName, config, hostConfig, spec.Endpoints); err != nil { + return proxyResult{}, fmt.Errorf("failed to create envoy container: %w", err) + } + + return proxyResult{ + IngressHostPort: ingressPort, + EnvVars: addEgressEnvVars(nil, egressContainerName), + }, nil +} diff --git a/pkg/container/docker/envoy_test.go b/pkg/container/docker/envoy_test.go new file mode 100644 index 0000000000..543213ff45 --- /dev/null +++ b/pkg/container/docker/envoy_test.go @@ -0,0 +1,441 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "encoding/json" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/permissions" +) + +// Compile-time assertion: envoyProxy must satisfy networkProxy. +var _ networkProxy = (*envoyProxy)(nil) + +// findRBACFilter walks the filter chain in a listener looking for the first +// filter whose name contains "rbac". It returns nil if none is found. +// The exact field layout mirrors the typed structs that envoy.go will define. +func findRBACFilter(listener envoyListener) *envoyRBACFilter { + for _, fc := range listener.FilterChains { + for i := range fc.Filters { + if fc.Filters[i].TypedConfig != nil { + rbac, ok := fc.Filters[i].TypedConfig.(*envoyRBACFilter) + if ok { + return rbac + } + } + } + } + return nil +} + +// TestBuildEgressListener_AllowlistAndDefaultDeny exercises the RBAC policy +// generation logic of buildEgressListener across the main permission scenarios. +func TestBuildEgressListener_AllowlistAndDefaultDeny(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + spec proxySpec + wantRBACPresent bool + wantRBACAction string // "ALLOW" or "DENY" + wantPolicies []string + wantGatewayDenyAbsent bool + wantGatewayDenyL3 bool // CIDR deny on GatewayIP + wantGatewayDenyL7 bool // authority deny on host.docker.internal + }{ + { + name: "nil permissions, InsecureAllowAll=false produces deny-all RBAC", + spec: proxySpec{ + WorkloadName: "myserver", + Permissions: nil, + AllowDockerGateway: false, + GatewayIP: dockerDefaultBridgeGatewayIP, + }, + wantRBACPresent: true, + wantRBACAction: "ALLOW", + wantPolicies: nil, // no policies → Envoy deny-all + wantGatewayDenyL3: true, + wantGatewayDenyL7: true, + }, + { + name: "InsecureAllowAll=true produces allow-all RBAC policy", + spec: proxySpec{ + WorkloadName: "myserver", + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + InsecureAllowAll: true, + }, + }, + AllowDockerGateway: false, + GatewayIP: dockerDefaultBridgeGatewayIP, + }, + wantRBACPresent: true, + wantRBACAction: "ALLOW", + wantPolicies: []string{"allow-all"}, + wantGatewayDenyL3: true, + wantGatewayDenyL7: true, + }, + { + name: "AllowHost list produces per-host RBAC policies", + spec: proxySpec{ + WorkloadName: "myserver", + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + AllowHost: []string{"example.com", "api.example.com"}, + }, + }, + AllowDockerGateway: false, + GatewayIP: dockerDefaultBridgeGatewayIP, + }, + wantRBACPresent: true, + wantRBACAction: "ALLOW", + wantPolicies: []string{"example.com", "api.example.com"}, + wantGatewayDenyL3: true, + wantGatewayDenyL7: true, + }, + { + name: "AllowDockerGateway=true omits gateway deny rules", + spec: proxySpec{ + WorkloadName: "myserver", + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + InsecureAllowAll: true, + }, + }, + AllowDockerGateway: true, + GatewayIP: dockerDefaultBridgeGatewayIP, + }, + wantRBACPresent: true, + wantRBACAction: "ALLOW", + wantPolicies: []string{"allow-all"}, + wantGatewayDenyAbsent: true, + }, + { + name: "wildcard AllowHost produces correct authority pattern", + spec: proxySpec{ + WorkloadName: "myserver", + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + AllowHost: []string{"*.example.com"}, + }, + }, + AllowDockerGateway: false, + GatewayIP: dockerDefaultBridgeGatewayIP, + }, + wantRBACPresent: true, + wantRBACAction: "ALLOW", + wantPolicies: []string{"*.example.com"}, + wantGatewayDenyL3: true, + wantGatewayDenyL7: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + listener := buildEgressListener(tt.spec) + + // The listener must have at least one filter chain. + require.NotEmpty(t, listener.FilterChains, "expected at least one filter chain") + + if tt.wantRBACPresent { + rbac := findRBACFilter(listener) + require.NotNil(t, rbac, "expected RBAC filter to be present in the listener") + assert.Equal(t, tt.wantRBACAction, rbac.Rules.Action, + "RBAC action mismatch") + + if tt.wantPolicies == nil { + assert.Empty(t, rbac.Rules.Policies, + "expected empty policy set for deny-all") + } else { + for _, policyName := range tt.wantPolicies { + _, ok := rbac.Rules.Policies[policyName] + assert.True(t, ok, "expected RBAC policy %q to be present", policyName) + } + } + } + + if tt.wantGatewayDenyAbsent { + // Serialize to JSON and verify neither gateway hostname nor the + // gateway CIDR deny appear anywhere in the config. + raw, err := json.Marshal(listener) + require.NoError(t, err) + s := string(raw) + assert.NotContains(t, s, dockerGatewayHostname, + "docker gateway hostname should be absent when AllowDockerGateway=true") + assert.NotContains(t, s, dockerDefaultBridgeGatewayIP, + "docker gateway IP should be absent when AllowDockerGateway=true") + } + + if tt.wantGatewayDenyL3 { + raw, err := json.Marshal(listener) + require.NoError(t, err) + assert.Contains(t, string(raw), dockerDefaultBridgeGatewayIP, + "expected L3 CIDR deny on gateway IP") + } + + if tt.wantGatewayDenyL7 { + raw, err := json.Marshal(listener) + require.NoError(t, err) + assert.Contains(t, string(raw), dockerGatewayHostname, + "expected L7 authority deny for host.docker.internal") + assert.Contains(t, string(raw), dockerAltGatewayHostname, + "expected L7 authority deny for gateway.docker.internal") + } + }) + } +} + +// TestBuildEgressListener_EmptyAllowHostDenyAll is a mandatory regression guard: +// buildEgressListener with an empty AllowHost and InsecureAllowAll=false must +// produce a listener where the RBAC filter is present with action=ALLOW and +// zero policies. This is Envoy's deny-all behavior. The test guards against a +// serialization bug that silently omits the RBAC filter and produces allow-all. +func TestBuildEgressListener_EmptyAllowHostDenyAll(t *testing.T) { + t.Parallel() + + spec := proxySpec{ + WorkloadName: "guard-test", + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + InsecureAllowAll: false, + AllowHost: []string{}, + }, + }, + AllowDockerGateway: false, + GatewayIP: dockerDefaultBridgeGatewayIP, + } + + listener := buildEgressListener(spec) + require.NotEmpty(t, listener.FilterChains) + + rbac := findRBACFilter(listener) + require.NotNil(t, rbac, + "RBAC filter must be present — its absence would silently allow all traffic") + assert.Equal(t, "ALLOW", rbac.Rules.Action, + "action must be ALLOW; an empty policy set under ALLOW is Envoy's deny-all") + assert.Empty(t, rbac.Rules.Policies, + "policy set must be empty to achieve deny-all semantics") + + // Also verify the config round-trips as valid JSON so we catch any + // serialization bug that would silently drop the RBAC filter. + raw, err := json.Marshal(listener) + require.NoError(t, err) + var roundTripped envoyListener + require.NoError(t, json.Unmarshal(raw, &roundTripped), + "listener must round-trip through JSON without error") + rbacAfter := findRBACFilter(roundTripped) + require.NotNil(t, rbacAfter, + "RBAC filter must survive JSON round-trip; omitempty on empty map is the classic culprit") +} + +// TestBuildIngressListener_PortAndHostGating verifies that buildIngressListener +// wires the upstream port, host-port binding, and virtual-host domain gating +// correctly for several input scenarios. +func TestBuildIngressListener_PortAndHostGating(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + spec proxySpec + hostPort int + wantUpstreamRef string // substring that must appear in the listener JSON + wantHostPortBound int // the listener's bind port + wantDomains []string + }{ + { + name: "sse transport binds hostPort and routes to upstream", + spec: proxySpec{ + WorkloadName: "myserver", + UpstreamPort: 8080, + TransportType: "sse", + Permissions: nil, + }, + hostPort: 18080, + wantUpstreamRef: "myserver:8080", + wantHostPortBound: 18080, + }, + { + name: "inbound AllowHost restricts virtual host domains", + spec: proxySpec{ + WorkloadName: "svc", + UpstreamPort: 9090, + TransportType: "streamable-http", + Permissions: &permissions.NetworkPermissions{ + Inbound: &permissions.InboundNetworkPermissions{ + AllowHost: []string{"app.example.com"}, + }, + }, + }, + hostPort: 19090, + wantUpstreamRef: "svc:9090", + wantHostPortBound: 19090, + wantDomains: []string{"app.example.com"}, + }, + { + name: "nil permissions defaults to permissive localhost gating", + spec: proxySpec{ + WorkloadName: "tool", + UpstreamPort: 7070, + TransportType: "sse", + Permissions: nil, + }, + hostPort: 17070, + wantUpstreamRef: "tool:7070", + wantHostPortBound: 17070, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + listener := buildIngressListener(tt.spec, tt.hostPort) + + // Serialize to JSON for substring assertions — this is simpler + // than deeply navigating the typed structs and more resilient to + // internal refactors that rename unexported fields. + raw, err := json.Marshal(listener) + require.NoError(t, err) + s := string(raw) + + assert.Contains(t, s, tt.wantUpstreamRef, + "listener config must reference upstream %s", tt.wantUpstreamRef) + + if tt.wantDomains != nil { + for _, domain := range tt.wantDomains { + assert.Contains(t, s, domain, + "listener config must contain domain restriction %q", domain) + } + } + + // The listener must not be bound on port 0. + assert.NotContains(t, s, `"port_value":0`, + "listener must not bind on port 0") + }) + } +} + +// TestWriteEnvoyBootstrap_FileMode verifies that writeEnvoyBootstrap writes a +// valid JSON bootstrap file at mode 0600. +func TestWriteEnvoyBootstrap_FileMode(t *testing.T) { + t.Parallel() + + b := envoyBootstrap{ + Admin: &envoyAdmin{ + Address: envoyAddress{ + SocketAddress: envoySocketAddress{ + Address: "127.0.0.1", + PortValue: 9901, + }, + }, + }, + StaticResources: envoyStatic{}, + } + + path, err := writeEnvoyBootstrap(b) + require.NoError(t, err) + require.NotEmpty(t, path, "returned path must be non-empty") + + t.Cleanup(func() { _ = os.Remove(path) }) + + // File must exist and be readable. + info, err := os.Stat(path) + require.NoError(t, err) + + // Mode must be 0600 — not 0644 — so that other processes cannot read the + // bootstrap config (which may contain sensitive socket addresses). + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "bootstrap file must be written at mode 0600") + + // File must contain valid JSON that deserializes back into envoyBootstrap. + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NotEmpty(t, data, "bootstrap file must not be empty") + + var roundTripped envoyBootstrap + require.NoError(t, json.Unmarshal(data, &roundTripped), + "bootstrap file must contain valid JSON") +} + +// TestEnvoyAdmin_LoopbackOnly asserts that the admin block written by +// writeEnvoyBootstrap binds only on the loopback address and never on +// 0.0.0.0 or an empty address that would expose admin to all interfaces. +func TestEnvoyAdmin_LoopbackOnly(t *testing.T) { + t.Parallel() + + b := envoyBootstrap{ + Admin: &envoyAdmin{ + Address: envoyAddress{ + SocketAddress: envoySocketAddress{ + Address: "127.0.0.1", + PortValue: 9901, + }, + }, + }, + StaticResources: envoyStatic{}, + } + + path, err := writeEnvoyBootstrap(b) + require.NoError(t, err) + require.NotEmpty(t, path) + t.Cleanup(func() { _ = os.Remove(path) }) + + data, err := os.ReadFile(path) + require.NoError(t, err) + s := string(data) + + assert.Contains(t, s, "127.0.0.1", + "admin address must be loopback 127.0.0.1") + assert.NotContains(t, s, "0.0.0.0", + "admin address must NOT bind on 0.0.0.0") +} + +// TestGetEnvoyImage verifies that getEnvoyImage returns the default image when +// the override env var is unset and the override when it is set. +// NOTE: t.Setenv is used so t.Parallel() is intentionally omitted here — env +// mutations are global state and are incompatible with parallel execution. +func TestGetEnvoyImage(t *testing.T) { + tests := []struct { + name string + envValue string + wantImage string + wantEnvoy bool // true: assert the result contains "envoy" + }{ + { + name: "empty env returns non-empty default containing envoy", + envValue: "", + wantEnvoy: true, + }, + { + name: "custom image override is returned verbatim", + envValue: "my-custom-envoy:latest", + wantImage: "my-custom-envoy:latest", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("TOOLHIVE_ENVOY_IMAGE", tt.envValue) + + got := getEnvoyImage() + require.NotEmpty(t, got, "getEnvoyImage must never return an empty string") + + if tt.wantEnvoy { + assert.Contains(t, got, "envoy", + "default image must contain 'envoy' to be identifiable") + } + + if tt.wantImage != "" { + assert.Equal(t, tt.wantImage, got) + } + }) + } +} diff --git a/pkg/container/docker/networkproxy.go b/pkg/container/docker/networkproxy.go index 0e16cf6327..8b2731247e 100644 --- a/pkg/container/docker/networkproxy.go +++ b/pkg/container/docker/networkproxy.go @@ -91,10 +91,15 @@ func newNetworkProxy(c *Client) (networkProxy, error) { switch val { case "", "squid": return &squidProxy{client: c}, nil + case "envoy": + return &envoyProxy{client: c}, nil default: - return nil, fmt.Errorf("unknown TOOLHIVE_NETWORK_PROXY value %q: supported values are \"squid\" (default)", val) + return nil, fmt.Errorf("unknown TOOLHIVE_NETWORK_PROXY value %q: supported values are \"squid\" (default), \"envoy\"", val) } } // Compile-time assertion that squidProxy satisfies networkProxy. var _ networkProxy = (*squidProxy)(nil) + +// Compile-time assertion that envoyProxy satisfies networkProxy. +var _ networkProxy = (*envoyProxy)(nil) diff --git a/pkg/container/docker/networkproxy_test.go b/pkg/container/docker/networkproxy_test.go index e1820b4e3b..46c577dc35 100644 --- a/pkg/container/docker/networkproxy_test.go +++ b/pkg/container/docker/networkproxy_test.go @@ -18,6 +18,7 @@ func TestNewNetworkProxy(t *testing.T) { name string envValue string wantSquid bool + wantEnvoy bool wantErr bool errContains []string }{ @@ -34,11 +35,14 @@ func TestNewNetworkProxy(t *testing.T) { wantErr: false, }, { - name: "envoy returns unsupported error", - envValue: "envoy", - wantSquid: false, - wantErr: true, - errContains: []string{"envoy"}, + // After Stage 1 this case becomes a success path — envoy returns + // *envoyProxy, no error. Flip wantErr to false and add wantEnvoy + // assertion once envoy.go exists. + name: "envoy returns envoyProxy", + envValue: "envoy", + wantSquid: false, + wantEnvoy: true, + wantErr: false, }, { name: "bogus value returns error", @@ -73,6 +77,11 @@ func TestNewNetworkProxy(t *testing.T) { _, ok := proxy.(*squidProxy) assert.True(t, ok, "expected proxy to be *squidProxy, got %T", proxy) } + + if tt.wantEnvoy { + _, ok := proxy.(*envoyProxy) + assert.True(t, ok, "expected proxy to be *envoyProxy, got %T", proxy) + } }) } } From be2ea78dc961311a4621301697f8a9c58b49dc12 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:22:46 +0100 Subject: [PATCH 2/9] Fix Envoy config correctness and add architecture doc Rewrites the Envoy bootstrap generation to produce valid protobuf-JSON: every typed_config field now carries the required @type URL. Adds the CONNECT route matcher so HTTPS tunnelling works, fixes ingress listener to bind 0.0.0.0 inside the container (Docker port forwarding targets the bridge IP, not the container loopback), switches gateway hostname deny to prefix matching so host.docker.internal:443 is caught in HTTPS CONNECT, and adds stdout access logging to both listeners. Also adds docs/arch/14-envoy-network-proxy.md describing the design, the comparison with Squid, and known limitations. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- docs/arch/14-envoy-network-proxy.md | 210 ++++++++ pkg/container/docker/envoy.go | 733 +++++++++++++++++++++------- pkg/container/docker/envoy_test.go | 170 ++++++- 3 files changed, 908 insertions(+), 205 deletions(-) create mode 100644 docs/arch/14-envoy-network-proxy.md diff --git a/docs/arch/14-envoy-network-proxy.md b/docs/arch/14-envoy-network-proxy.md new file mode 100644 index 0000000000..7beddaa68f --- /dev/null +++ b/docs/arch/14-envoy-network-proxy.md @@ -0,0 +1,210 @@ +# Envoy Network Proxy + +## Status + +Experimental — selected with `TOOLHIVE_NETWORK_PROXY=envoy`. Squid remains the +default. + +## Problem + +When network isolation is enabled (`--isolate-network`), ToolHive currently +starts **three** auxiliary containers per workload: + +| Container | Role | +|-----------|------| +| `-egress` | Squid forward proxy — routes outbound traffic through an allowlist | +| `-ingress` | Squid reverse proxy — receives traffic from the proxy runner | +| `-dns` | dnsmasq — provides DNS to the internal network | + +Three containers means three image pulls, three startup sequences, three sets of +resources, and three things that can fail or restart. The Squid egress and ingress +containers are logically a single gateway — splitting them into two processes is +an implementation artifact rather than a deliberate design. + +## Solution + +Replace the two Squid containers with a **single Envoy container** that handles +both egress and ingress as separate listeners inside the same process. The DNS +container (dnsmasq) is unchanged. + +``` +Before: -egress (Squid) + -ingress (Squid) + -dns +After: -egress (Envoy, two listeners) + -dns +``` + +This reduces auxiliary container count from 3 → 2, simplifies the startup +sequence, and uses a single bootstrap configuration file to describe the entire +proxy behaviour. + +## Why Envoy + +### Consolidation + +Envoy's `HttpConnectionManager` supports multiple listeners in a single process. +The egress forward proxy (`:3128`) and ingress reverse proxy share the same Envoy +instance, the same access logs, and the same lifecycle. + +### L3 + L7 enforcement + +Squid operates at L7 only — it can match destination hostnames via `dstdomain` +ACLs but cannot match by IP address in a reliable, port-independent way. + +Envoy's RBAC filter supports: +- **`destination_ip`** — CIDR match at L3/L4, applied before the request is + parsed as HTTP. This catches direct-IP connections that bypass DNS. +- **Header match on `:authority`** — L7 match on the CONNECT target or HTTP + Host header, equivalent to Squid's `dstdomain`. + +ToolHive combines both layers: outbound traffic is blocked at L3 for known IP +ranges and at L7 for hostname patterns. The `Internal: true` Docker network +remains the fail-closed backstop for non-cooperative traffic that ignores the +proxy entirely. + +### Proper dynamic forward proxy + +Envoy's `dynamic_forward_proxy` cluster performs per-request DNS resolution and +handles HTTP CONNECT tunnelling natively. HTTPS flows through a CONNECT tunnel +exactly as a client would expect, with Envoy acting as a transparent TCP relay +after the CONNECT handshake — no TLS inspection, no certificate pinning, no CA +changes. + +### Access logging + +Both listeners write structured access logs to stdout, visible via `docker logs`. +Squid logged differently for egress and ingress with no unified view. + +### Configuration as code + +Envoy reads a protobuf-JSON bootstrap file generated by ToolHive at workload +start. The configuration is typed Go structs serialised to JSON — unit-testable, +diffable, and reproducible. Squid required template-rendered text files. + +### Future extensibility + +Envoy's xDS API makes it possible to update listeners, clusters, and RBAC +policies at runtime without a container restart. This is not used today, but the +groundwork is there for dynamic policy updates. The transparent L3/L4 +interception path (Phase 2, not yet implemented) requires an `original_dst` +listener and iptables rules that Envoy handles cleanly. + +## What Envoy Does Not Do + +- **Decrypt TLS.** Like Squid, Envoy filters HTTPS on the CONNECT target hostname + and then relays the encrypted stream as-is. No certificate inspection, no + man-in-the-middle. +- **Block non-cooperative traffic.** A workload that opens a raw TCP connection + ignoring `HTTP_PROXY` is contained by the `Internal: true` Docker network + blackhole, not by Envoy. Envoy only sees traffic that goes through the proxy. +- **Replace dnsmasq.** DNS for the workload's internal network is still served by + the dnsmasq container. +- **Run in Kubernetes.** Network isolation is a local-Docker feature only; the + Kubernetes operator has a separate egress gateway path. + +## Architecture + +### Egress listener (`:3128` — forward proxy) + +``` +HTTP_PROXY / HTTPS_PROXY → Envoy :3128 + └── HCM (upgrade: CONNECT) + ├── [optional] RBAC DENY — docker gateway IP (L3) + hostnames (L7) + ├── RBAC ALLOW — outbound allowlist (or allow-all) + ├── dynamic_forward_proxy — per-request DNS + CONNECT tunnel + └── router +``` + +The RBAC filters are evaluated top-to-bottom. The gateway DENY filter is present +unless `--allow-docker-gateway` is set; it blocks: +- The resolved Docker bridge gateway IP as a /32 CIDR (`destination_ip`) +- `host.docker.internal` and `gateway.docker.internal` as `:authority` prefix + matches (covers both plain HTTP and HTTPS CONNECT where authority includes the + port, e.g. `host.docker.internal:443`) + +The ALLOW filter implements the permission profile's `Outbound` rules: +- `InsecureAllowAll: true` → single wildcard policy (`any: true`) +- `AllowHost: [...]` → per-host `:authority` exact match (or suffix match for + `*.`-prefixed wildcards) +- No outbound permissions configured → empty policy map → Envoy deny-all + +### Ingress listener (`0.0.0.0:` — reverse proxy) + +``` +Proxy runner → host:127.0.0.1: → Docker port binding → Envoy :port + └── HCM + ├── router + └── route → STRICT_DNS cluster → : +``` + +The ingress listener binds to `0.0.0.0` inside the container so Docker's port +forwarding (which targets the container's bridge IP, not its loopback) can reach +it. The host-side port binding restricts to `127.0.0.1`, so the ingress is only +reachable from the local machine. + +The upstream STRICT_DNS cluster resolves the MCP container's hostname inside the +internal Docker network and forwards HTTP traffic to the MCP server port. + +The admin interface binds to `127.0.0.1` inside the Envoy container (container +loopback, not reachable via Docker port forwarding) as a precaution against the +admin API being accessible from other containers. + +### Bootstrap lifecycle + +1. ToolHive generates a protobuf-JSON bootstrap file in `os.TempDir()` at mode + `0600`. +2. The file is bind-mounted read-only into the Envoy container at + `/etc/envoy/envoy.json`. +3. Envoy reads it once at startup. +4. The file is cleaned up when ToolHive removes the workload. + +## Selection + +```bash +TOOLHIVE_NETWORK_PROXY=envoy thv run --isolate-network +``` + +`TOOLHIVE_NETWORK_PROXY` accepts: +- `""` or `"squid"` — Squid backend (default) +- `"envoy"` — Envoy backend + +An unknown value causes `NewClient` to fail at startup with a descriptive error. +The env var is intentionally not exposed as a CLI flag or CRD field while the +backend is experimental; chart surface and `RunConfig` wiring come later once the +backend is stable. + +## Comparison with Squid + +| Aspect | Squid (current default) | Envoy | +|--------|------------------------|-------| +| Containers per workload | 3 (egress + ingress + dns) | 2 (combined + dns) | +| Forward proxy | ✓ | ✓ (dynamic_forward_proxy) | +| Reverse proxy (ingress) | ✓ (separate container) | ✓ (second listener, same container) | +| HTTPS CONNECT tunnelling | ✓ | ✓ | +| TLS inspection | ✗ | ✗ | +| L7 hostname deny | ✓ (`dstdomain`) | ✓ (`:authority` header match) | +| L3 IP CIDR deny | Partial (`dst` ACL — DNS-resolved) | ✓ (direct packet match) | +| Wildcard host allowlist | ✓ (dot-prefix) | ✓ (suffix match) | +| Per-request DNS resolution | Via Squid resolver | Via DFP cluster | +| Access logs | Per-container, text format | Unified stdout, structured | +| Config format | Text template | Typed Go structs → protobuf-JSON | +| Runtime config update | Restart required | xDS-capable (not yet used) | +| Upstream image | Stacklok-built | Upstream distroless (pinned tag) | + +## Known Limitations + +- **Tag-pinned image.** The Envoy image is pinned by tag (`v1.32.3`), not by + digest. A future PR should pin by digest and add a `TOOLHIVE_ENVOY_IMAGE` + override for supply-chain policy requirements (the env var already exists). +- **Admin interface port.** The admin API on `:9901` (loopback-only inside the + container) is always enabled. A follow-up can disable it entirely or make it + conditional. +- **CONNECT access log timing.** Envoy logs CONNECT tunnel entries when the + tunnel closes, not when it opens. With keep-alive HTTP clients the log entry + may be delayed by minutes. Egress access logs are visible in `docker logs` but + appear after the connection closes. +- **No transparent L3/L4.** Non-cooperative traffic (workloads that ignore + `HTTP_PROXY`) is contained by the `Internal: true` network, not Envoy. True + non-bypassable enforcement requires iptables TPROXY + an init container with + `CAP_NET_ADMIN` — this is Phase 2 and requires its own architecture doc. +- **No port-based allowlist.** `AllowPort` from the permission profile is not + yet translated into Envoy policy. Squid honours `AllowPort`; the Envoy backend + currently ignores it. diff --git a/pkg/container/docker/envoy.go b/pkg/container/docker/envoy.go index f4ed2f15cb..3970e930a5 100644 --- a/pkg/container/docker/envoy.go +++ b/pkg/container/docker/envoy.go @@ -23,8 +23,24 @@ const ( // Override with TOOLHIVE_ENVOY_IMAGE. defaultEnvoyImage = "envoyproxy/envoy-distroless:v1.32.3" - envoyHTTPRBACFilterName = "envoy.filters.http.rbac" - envoyGatewayDenyFilterName = "toolhive.gateway.deny" + // Protobuf type URLs required by Envoy's protobuf-JSON bootstrap format. + // Every typed_config field must carry an @type URL or Envoy will reject the + // config on startup. + typeHCM = "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager" + typeHTTPRBAC = "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC" + typeDFPFilter = "type.googleapis.com/envoy.extensions.filters.http.dynamic_forward_proxy.v3.FilterConfig" + typeRouter = "type.googleapis.com/envoy.extensions.filters.http.router.v3.Router" + typeDFPCluster = "type.googleapis.com/envoy.extensions.clusters.dynamic_forward_proxy.v3.ClusterConfig" + + // dfpCacheName is the shared DNS cache config name used by both the DFP + // HTTP filter and the DFP cluster extension. + dfpCacheName = "dynamic_forward_proxy_cache_config" + + // dfpClusterName is the cluster referenced by the egress route config. + dfpClusterName = "dynamic_forward_proxy_cluster" + + // ingressClusterName is the cluster referenced by the ingress route config. + ingressClusterName = "ingress_upstream" ) func getEnvoyImage() string { @@ -34,10 +50,16 @@ func getEnvoyImage() string { return defaultEnvoyImage } +// boolPtr returns a pointer to b, used for the RBAC any permission field which +// requires a pointer to distinguish "unset" from "false". +func boolPtr(b bool) *bool { return &b } + +// ── Bootstrap ──────────────────────────────────────────────────────────────── + // envoyBootstrap is the top-level Envoy bootstrap configuration. type envoyBootstrap struct { - Admin *envoyAdmin `json:"admin,omitempty"` - StaticResources envoyStatic `json:"static_resources"` + Admin *envoyAdmin `json:"admin,omitempty"` + StaticResources envoyStaticResources `json:"static_resources"` } // envoyAdmin configures the Envoy admin API endpoint. @@ -45,6 +67,14 @@ type envoyAdmin struct { Address envoyAddress `json:"address"` } +// envoyStaticResources holds the static listeners and clusters. +type envoyStaticResources struct { + Listeners []envoyListener `json:"listeners,omitempty"` + Clusters []envoyCluster `json:"clusters,omitempty"` +} + +// ── Address types ──────────────────────────────────────────────────────────── + // envoyAddress wraps a socket address for Envoy config. type envoyAddress struct { SocketAddress envoySocketAddress `json:"socket_address"` @@ -56,11 +86,7 @@ type envoySocketAddress struct { PortValue int `json:"port_value"` } -// envoyStatic holds the static listeners and clusters. -type envoyStatic struct { - Listeners []envoyListener `json:"listeners,omitempty"` - Clusters []envoyCluster `json:"clusters,omitempty"` -} +// ── Listener / filter chain ────────────────────────────────────────────────── // envoyListener is an Envoy listener binding on a socket address. type envoyListener struct { @@ -69,75 +95,76 @@ type envoyListener struct { FilterChains []envoyFilterChain `json:"filter_chains"` } -// envoyFilterChain is a sequence of filters applied to matching connections. +// envoyFilterChain is a sequence of network-level filters applied to matching +// connections. type envoyFilterChain struct { - Filters []envoyFilter `json:"filters"` + Filters []envoyNetworkFilter `json:"filters"` } -// envoyFilter is a named filter whose typed config is serialized with custom -// marshal/unmarshal logic so that TypedConfig can be asserted as a concrete -// Go type (e.g. *envoyRBACFilter) while still round-tripping through JSON. -type envoyFilter struct { +// envoyNetworkFilter is a named network filter whose typed config is an +// arbitrary protobuf-JSON object (e.g. envoyHCM). The any type lets us embed +// concrete structs directly so that JSON serialisation includes @type. +type envoyNetworkFilter struct { Name string `json:"name"` - TypedConfig any `json:"-"` + TypedConfig any `json:"typed_config"` } -// MarshalJSON serializes the filter including TypedConfig under "typed_config". -func (f envoyFilter) MarshalJSON() ([]byte, error) { - type proxy struct { - Name string `json:"name"` - TypedConfig any `json:"typed_config,omitempty"` - } - return json.Marshal(proxy(f)) +// ── HttpConnectionManager ─────────────────────────────────────────────────── + +// envoyHCM is the typed config for +// envoy.filters.network.http_connection_manager. +type envoyHCM struct { + Type string `json:"@type"` + StatPrefix string `json:"stat_prefix"` + AccessLog []envoyAccessLog `json:"access_log,omitempty"` + UpgradeConfigs []envoyUpgradeConfig `json:"upgrade_configs,omitempty"` + HTTPFilters []envoyHTTPFilter `json:"http_filters"` + RouteConfig *envoyRouteConfig `json:"route_config,omitempty"` } -// UnmarshalJSON restores TypedConfig as the correct concrete Go type by -// switching on the filter name. -func (f *envoyFilter) UnmarshalJSON(data []byte) error { - var raw struct { - Name string `json:"name"` - TypedConfig json.RawMessage `json:"typed_config,omitempty"` - } - if err := json.Unmarshal(data, &raw); err != nil { - return err - } - f.Name = raw.Name - if len(raw.TypedConfig) == 0 { - return nil - } - switch raw.Name { - case envoyHTTPRBACFilterName: - var rbac envoyRBACFilter - if err := json.Unmarshal(raw.TypedConfig, &rbac); err != nil { - return err - } - f.TypedConfig = &rbac - case envoyGatewayDenyFilterName: - var deny envoyGatewayDenyConfig - if err := json.Unmarshal(raw.TypedConfig, &deny); err != nil { - return err - } - f.TypedConfig = &deny - default: - var m map[string]any - if err := json.Unmarshal(raw.TypedConfig, &m); err != nil { - return err - } - f.TypedConfig = m - } - return nil +// envoyAccessLog configures request access logging for an HCM. +type envoyAccessLog struct { + Name string `json:"name"` + TypedConfig any `json:"typed_config"` +} + +// envoyStdoutAccessLog is the typed config for envoy.access_loggers.stdout. +type envoyStdoutAccessLog struct { + Type string `json:"@type"` } -// envoyRBACFilter is the HTTP RBAC filter for allowlisting outbound traffic. -// This is the type returned by findRBACFilter in tests. -type envoyRBACFilter struct { +const typeStdoutAccessLog = "type.googleapis.com/envoy.extensions.access_loggers.stream.v3.StdoutAccessLog" + +func stdoutAccessLog() []envoyAccessLog { + return []envoyAccessLog{{ + Name: "envoy.access_loggers.stdout", + TypedConfig: &envoyStdoutAccessLog{Type: typeStdoutAccessLog}, + }} +} + +// envoyUpgradeConfig enables HTTP upgrade protocols (e.g. CONNECT) in HCM. +type envoyUpgradeConfig struct { + UpgradeType string `json:"upgrade_type"` + ConnectConfig *envoyConnectConfig `json:"connect_config,omitempty"` +} + +// envoyHTTPFilter is a named HTTP-layer filter embedded inside HCM. +type envoyHTTPFilter struct { + Name string `json:"name"` + TypedConfig any `json:"typed_config"` +} + +// ── RBAC ───────────────────────────────────────────────────────────────────── + +// envoyHTTPRBAC is the typed config for envoy.filters.http.rbac. +type envoyHTTPRBAC struct { + Type string `json:"@type"` Rules envoyRBACRules `json:"rules"` } // envoyRBACRules holds the RBAC action and policy map. -// CRITICAL: Policies must NOT have omitempty. An empty map serializes as {} not -// be omitted — omitting it would silently turn deny-all into allow-all after a -// JSON round-trip. +// CRITICAL: Policies must NOT have omitempty. An empty map serializes as {} and +// is intentional — an absent field would silently turn deny-all into allow-all. type envoyRBACRules struct { Action string `json:"action"` Policies map[string]envoyRBACPolicy `json:"policies"` @@ -149,94 +176,200 @@ type envoyRBACPolicy struct { Principals []envoyPrincipal `json:"principals"` } -// envoyPermission matches a request by header or wildcard. +// envoyPermission matches a request by various criteria. Exactly one field +// should be set per permission entry. type envoyPermission struct { - Header *envoyHeaderMatcher `json:"header,omitempty"` - Any bool `json:"any,omitempty"` + Any *bool `json:"any,omitempty"` + Header *envoyHeaderMatcher `json:"header,omitempty"` + DestinationIP *envoyCIDRRange `json:"destination_ip,omitempty"` + OrRules *envoyOrRules `json:"or_rules,omitempty"` +} + +// envoyOrRules composes multiple permissions with logical OR. +type envoyOrRules struct { + Rules []envoyPermission `json:"rules"` } -// envoyHeaderMatcher matches an HTTP header by exact or suffix value. +// envoyCIDRRange matches destination IPs against a CIDR prefix. +type envoyCIDRRange struct { + AddressPrefix string `json:"address_prefix"` + PrefixLen uint32 `json:"prefix_len"` +} + +// envoyHeaderMatcher matches an HTTP header value. type envoyHeaderMatcher struct { - Name string `json:"name"` - ExactMatch string `json:"exact_match,omitempty"` - SuffixMatch string `json:"suffix_match,omitempty"` + Name string `json:"name"` + StringMatch *envoyStringMatch `json:"string_match,omitempty"` } -// envoyPrincipal matches a downstream principal (wildcard only for now). +// envoyStringMatch matches a string by exact value, prefix, or suffix. +type envoyStringMatch struct { + Exact string `json:"exact,omitempty"` + Prefix string `json:"prefix,omitempty"` + Suffix string `json:"suffix,omitempty"` +} + +// envoyPrincipal matches a downstream principal. Any:true is a wildcard. type envoyPrincipal struct { - Any bool `json:"any,omitempty"` + Any bool `json:"any"` +} + +// ── DFP filter ─────────────────────────────────────────────────────────────── + +// envoyDFPFilter is the typed config for +// envoy.filters.http.dynamic_forward_proxy. +type envoyDFPFilter struct { + Type string `json:"@type"` + DNSCacheConfig envoyDNSCache `json:"dns_cache_config"` +} + +// envoyDNSCache is the shared DNS cache config referenced by both the DFP +// HTTP filter and the DFP cluster extension. +type envoyDNSCache struct { + Name string `json:"name"` + DNSLookupFamily string `json:"dns_lookup_family"` } -// envoyGatewayDenyConfig encodes the two-layer docker-gateway block: -// L3 CIDR deny (GatewayIP) and L7 hostname deny (GatewayHostnames). -// This serializes to JSON that contains both the gateway IP and hostnames, -// satisfying the two-layer requirement from the design doc. -type envoyGatewayDenyConfig struct { - GatewayIP string `json:"gateway_ip"` - GatewayHostnames []string `json:"gateway_hostnames"` +// ── Router ──────────────────────────────────────────────────────────────────── + +// envoyRouter is the typed config for envoy.filters.http.router. +type envoyRouter struct { + Type string `json:"@type"` +} + +// ── Route config ───────────────────────────────────────────────────────────── + +// envoyRouteConfig holds the list of virtual hosts for a listener. +type envoyRouteConfig struct { + VirtualHosts []envoyVirtualHost `json:"virtual_hosts"` +} + +// envoyVirtualHost matches requests by domain and dispatches to a cluster. +type envoyVirtualHost struct { + Name string `json:"name"` + Domains []string `json:"domains"` + Routes []envoyRoute `json:"routes"` +} + +// envoyRoute matches a request prefix and forwards it to a cluster. +type envoyRoute struct { + Match envoyRouteMatch `json:"match"` + Route *envoyRouteAction `json:"route,omitempty"` +} + +// envoyRouteMatch matches incoming requests by URI prefix or CONNECT method. +// Exactly one of Prefix or ConnectMatcher must be set. +type envoyRouteMatch struct { + Prefix string `json:"prefix,omitempty"` + ConnectMatcher *envoyConnectMatcher `json:"connect_matcher,omitempty"` +} + +// envoyConnectMatcher matches HTTP CONNECT requests (used for HTTPS tunneling). +type envoyConnectMatcher struct{} + +// envoyRouteAction forwards matched requests to an upstream cluster. +type envoyRouteAction struct { + Cluster string `json:"cluster"` + UpgradeConfigs []envoyUpgradeConfig `json:"upgrade_configs,omitempty"` } -// envoyCluster is an Envoy upstream cluster. +// envoyConnectConfig is the per-route CONNECT tunnel configuration. +type envoyConnectConfig struct{} + +// ── Cluster ─────────────────────────────────────────────────────────────────── + +// envoyCluster is an Envoy upstream cluster definition. type envoyCluster struct { - Name string `json:"name"` - ConnectTimeout string `json:"connect_timeout,omitempty"` - Type string `json:"type,omitempty"` + Name string `json:"name"` + ConnectTimeout string `json:"connect_timeout"` + LbPolicy string `json:"lb_policy,omitempty"` + Type string `json:"type,omitempty"` + ClusterType *envoyClusterType `json:"cluster_type,omitempty"` + LoadAssignment *envoyLoadAssignment `json:"load_assignment,omitempty"` } -// writeEnvoyBootstrap marshals b to JSON and writes it to a temporary file at -// mode 0600. Returns the file path. The caller is responsible for cleanup. -func writeEnvoyBootstrap(b envoyBootstrap) (string, error) { - data, err := json.Marshal(b) - if err != nil { - return "", fmt.Errorf("failed to marshal envoy bootstrap: %w", err) - } - tmpFile, err := os.CreateTemp("", "envoy-bootstrap-*.json") - if err != nil { - return "", fmt.Errorf("failed to create envoy bootstrap temp file: %w", err) - } - created := tmpFile.Name() - defer func() { - if cerr := tmpFile.Close(); cerr != nil { - slog.Warn("failed to close envoy bootstrap temp file", "error", cerr) - } - }() - if _, err := tmpFile.Write(data); err != nil { - _ = os.Remove(created) - return "", fmt.Errorf("failed to write envoy bootstrap: %w", err) - } - // 0600: only the owner can read — the file may contain network topology. - if err := tmpFile.Chmod(0o600); err != nil { - _ = os.Remove(created) - return "", fmt.Errorf("failed to set envoy bootstrap file permissions: %w", err) - } - return created, nil +// envoyClusterType is the custom cluster discovery extension (e.g. DFP). +type envoyClusterType struct { + Name string `json:"name"` + TypedConfig any `json:"typed_config"` +} + +// envoyDFPClusterConfig is the typed config for +// envoy.clusters.dynamic_forward_proxy. +type envoyDFPClusterConfig struct { + Type string `json:"@type"` + DNSCacheConfig envoyDNSCache `json:"dns_cache_config"` +} + +// envoyLoadAssignment is an EDS-style static load assignment. +type envoyLoadAssignment struct { + ClusterName string `json:"cluster_name"` + Endpoints []envoyEndpoint `json:"endpoints"` +} + +// envoyEndpoint is a group of LB endpoints. +type envoyEndpoint struct { + LBEndpoints []envoyLBEndpoint `json:"lb_endpoints"` +} + +// envoyLBEndpoint is a single upstream endpoint. +type envoyLBEndpoint struct { + Endpoint envoyEndpointAddress `json:"endpoint"` } +// envoyEndpointAddress wraps an address for an LB endpoint. +type envoyEndpointAddress struct { + Address envoyAddress `json:"address"` +} + +// ── Builder functions ───────────────────────────────────────────────────────── + // buildEgressListener builds the Envoy listener config for outbound traffic. -// When !spec.AllowDockerGateway, a gateway-deny filter is prepended that -// contains the resolved GatewayIP (L3) and the Docker-internal hostnames (L7). -// The HTTP RBAC allowlist filter (action=ALLOW) follows; an empty policy set -// is Envoy's deny-all. +// +// The resulting HCM HTTP filter chain is: +// 1. (when !spec.AllowDockerGateway) RBAC DENY on gateway IP and hostnames +// 2. RBAC ALLOW allowlist — empty policies map is Envoy's deny-all +// 3. dynamic_forward_proxy HTTP filter +// 4. router +// +// CONNECT upgrades are enabled so that HTTPS CONNECT tunnels pass through. func buildEgressListener(spec proxySpec) envoyListener { - var filters []envoyFilter - - // Gateway deny (two layers) must precede the allowlist. - if !spec.AllowDockerGateway { - filters = append(filters, envoyFilter{ - Name: envoyGatewayDenyFilterName, - TypedConfig: &envoyGatewayDenyConfig{ - GatewayIP: spec.GatewayIP, - GatewayHostnames: []string{dockerGatewayHostname, dockerAltGatewayHostname}, + httpFilters := buildEgressHTTPFilters(spec) + + hcm := &envoyHCM{ + Type: typeHCM, + StatPrefix: "egress_http", + AccessLog: stdoutAccessLog(), + UpgradeConfigs: []envoyUpgradeConfig{{UpgradeType: "CONNECT"}}, + HTTPFilters: httpFilters, + RouteConfig: &envoyRouteConfig{ + VirtualHosts: []envoyVirtualHost{ + { + Name: "local_service", + Domains: []string{"*"}, + Routes: []envoyRoute{ + // CONNECT match must come first: handles HTTPS tunneling. + // Without this, CONNECT requests don't match prefix "/" and get 404. + { + Match: envoyRouteMatch{ConnectMatcher: &envoyConnectMatcher{}}, + Route: &envoyRouteAction{ + Cluster: dfpClusterName, + UpgradeConfigs: []envoyUpgradeConfig{ + {UpgradeType: "CONNECT", ConnectConfig: &envoyConnectConfig{}}, + }, + }, + }, + // Prefix match handles plain HTTP requests. + { + Match: envoyRouteMatch{Prefix: "/"}, + Route: &envoyRouteAction{Cluster: dfpClusterName}, + }, + }, + }, }, - }) + }, } - // HTTP RBAC allowlist (action=ALLOW; empty policies = deny-all). - filters = append(filters, envoyFilter{ - Name: envoyHTTPRBACFilterName, - TypedConfig: buildEgressRBACFilter(spec), - }) - return envoyListener{ Name: fmt.Sprintf("%s-egress", spec.WorkloadName), Address: envoyAddress{ @@ -245,69 +378,276 @@ func buildEgressListener(spec proxySpec) envoyListener { PortValue: 3128, }, }, - FilterChains: []envoyFilterChain{{Filters: filters}}, + FilterChains: []envoyFilterChain{ + { + Filters: []envoyNetworkFilter{ + { + Name: "envoy.filters.network.http_connection_manager", + TypedConfig: hcm, + }, + }, + }, + }, + } +} + +// buildEgressHTTPFilters constructs the ordered list of HTTP filters for the +// egress HCM. Gateway deny rules (when !spec.AllowDockerGateway) are placed +// first so they are evaluated before the allowlist. +func buildEgressHTTPFilters(spec proxySpec) []envoyHTTPFilter { + var filters []envoyHTTPFilter + + if !spec.AllowDockerGateway { + filters = append(filters, envoyHTTPFilter{ + Name: "envoy.filters.http.rbac", + TypedConfig: buildGatewayDenyRBAC(spec.GatewayIP), + }) } + + filters = append(filters, + envoyHTTPFilter{ + Name: "envoy.filters.http.rbac", + TypedConfig: buildAllowlistRBAC(spec), + }, + envoyHTTPFilter{ + Name: "envoy.filters.http.dynamic_forward_proxy", + TypedConfig: &envoyDFPFilter{ + Type: typeDFPFilter, + DNSCacheConfig: envoyDNSCache{ + Name: dfpCacheName, + DNSLookupFamily: "V4_ONLY", + }, + }, + }, + envoyHTTPFilter{ + Name: "envoy.filters.http.router", + TypedConfig: &envoyRouter{Type: typeRouter}, + }, + ) + + return filters } -func buildEgressRBACFilter(spec proxySpec) *envoyRBACFilter { - rules := envoyRBACRules{ - Action: "ALLOW", - Policies: make(map[string]envoyRBACPolicy), // always init; never use omitempty +// buildGatewayDenyRBAC builds an RBAC DENY filter that blocks the Docker +// bridge gateway IP (L3 CIDR) and the Docker-internal hostnames (L7 authority +// header). This filter must precede the allowlist filter. +func buildGatewayDenyRBAC(gatewayIP string) *envoyHTTPRBAC { + return &envoyHTTPRBAC{ + Type: typeHTTPRBAC, + Rules: envoyRBACRules{ + Action: "DENY", + Policies: map[string]envoyRBACPolicy{ + "gateway-ip": { + Permissions: []envoyPermission{ + { + DestinationIP: &envoyCIDRRange{ + AddressPrefix: gatewayIP, + PrefixLen: 32, + }, + }, + }, + Principals: []envoyPrincipal{{Any: true}}, + }, + "gateway-hostnames": { + Permissions: []envoyPermission{ + { + // Prefix match covers both plain HTTP (:authority = "host.docker.internal") + // and HTTPS CONNECT (:authority = "host.docker.internal:443"). + OrRules: &envoyOrRules{ + Rules: []envoyPermission{ + { + Header: &envoyHeaderMatcher{ + Name: ":authority", + StringMatch: &envoyStringMatch{ + Prefix: dockerGatewayHostname, + }, + }, + }, + { + Header: &envoyHeaderMatcher{ + Name: ":authority", + StringMatch: &envoyStringMatch{ + Prefix: dockerAltGatewayHostname, + }, + }, + }, + }, + }, + }, + }, + Principals: []envoyPrincipal{{Any: true}}, + }, + }, + }, } +} +// buildAllowlistRBAC builds an RBAC ALLOW filter encoding the outbound +// allowlist. An empty Policies map is Envoy's deny-all under ALLOW action. +func buildAllowlistRBAC(spec proxySpec) *envoyHTTPRBAC { + return &envoyHTTPRBAC{ + Type: typeHTTPRBAC, + Rules: envoyRBACRules{ + Action: "ALLOW", + Policies: buildAllowlistPolicies(spec), + }, + } +} + +// buildAllowlistPolicies returns the policy map for the egress RBAC ALLOW +// filter. An empty map (deny-all) is returned when: +// - spec.Permissions is nil +// - spec.Permissions.Outbound is nil +// +// InsecureAllowAll produces a single wildcard policy. AllowHost entries become +// :authority header matchers (exact for plain hostnames, suffix for *.prefix). +func buildAllowlistPolicies(spec proxySpec) map[string]envoyRBACPolicy { if spec.Permissions == nil || spec.Permissions.Outbound == nil { - return &envoyRBACFilter{Rules: rules} // empty policies = deny-all + return make(map[string]envoyRBACPolicy) } out := spec.Permissions.Outbound if out.InsecureAllowAll { - rules.Policies["allow-all"] = envoyRBACPolicy{ - Permissions: []envoyPermission{{Any: true}}, - Principals: []envoyPrincipal{{Any: true}}, + return map[string]envoyRBACPolicy{ + "allow-all": { + Permissions: []envoyPermission{{Any: boolPtr(true)}}, + Principals: []envoyPrincipal{{Any: true}}, + }, } - return &envoyRBACFilter{Rules: rules} } + policies := make(map[string]envoyRBACPolicy) for _, host := range out.AllowHost { - matcher := &envoyHeaderMatcher{Name: ":authority"} + var match envoyStringMatch if strings.HasPrefix(host, "*.") { - matcher.SuffixMatch = host[1:] // "*.example.com" → ".example.com" + match.Suffix = host[1:] // "*.example.com" → ".example.com" } else { - matcher.ExactMatch = host + match.Exact = host } - rules.Policies[host] = envoyRBACPolicy{ - Permissions: []envoyPermission{{Header: matcher}}, - Principals: []envoyPrincipal{{Any: true}}, + policies[host] = envoyRBACPolicy{ + Permissions: []envoyPermission{ + { + Header: &envoyHeaderMatcher{ + Name: ":authority", + StringMatch: &match, + }, + }, + }, + Principals: []envoyPrincipal{{Any: true}}, } } - return &envoyRBACFilter{Rules: rules} + return policies +} + +// buildEgressCluster returns the dynamic_forward_proxy cluster required by the +// egress listener's route config. +func buildEgressCluster() envoyCluster { + return envoyCluster{ + Name: dfpClusterName, + ConnectTimeout: "10s", + LbPolicy: "CLUSTER_PROVIDED", + ClusterType: &envoyClusterType{ + Name: "envoy.clusters.dynamic_forward_proxy", + TypedConfig: &envoyDFPClusterConfig{ + Type: typeDFPCluster, + DNSCacheConfig: envoyDNSCache{ + Name: dfpCacheName, + DNSLookupFamily: "V4_ONLY", + }, + }, + }, + } } -// buildIngressListener builds the Envoy listener config for inbound (ingress) traffic. -// It binds on 127.0.0.1:hostPort and routes to spec.WorkloadName:spec.UpstreamPort. +// buildIngressListener builds the Envoy listener config for inbound (ingress) +// traffic. It binds on 0.0.0.0:hostPort inside the container so that Docker's +// port forwarding (host:127.0.0.1: → container:) can reach +// it. The host-side HostIP:"127.0.0.1" in the port binding provides the +// localhost-only restriction; the container-side address must be 0.0.0.0 or +// Docker's bridge forwarding cannot deliver traffic to the listener. +// +// When spec.Permissions.Inbound.AllowHost is set the virtual host domain list +// is restricted to those entries; otherwise a wildcard domain ("*") is used. func buildIngressListener(spec proxySpec, hostPort int) envoyListener { - upstreamRef := fmt.Sprintf("%s:%d", spec.WorkloadName, spec.UpstreamPort) + domains := ingressDomains(spec) - domains := []string{"localhost", "127.0.0.1", spec.WorkloadName} - if spec.Permissions != nil && spec.Permissions.Inbound != nil && - len(spec.Permissions.Inbound.AllowHost) > 0 { - domains = spec.Permissions.Inbound.AllowHost + hcm := &envoyHCM{ + Type: typeHCM, + StatPrefix: "ingress_http", + AccessLog: stdoutAccessLog(), + HTTPFilters: []envoyHTTPFilter{ + { + Name: "envoy.filters.http.router", + TypedConfig: &envoyRouter{Type: typeRouter}, + }, + }, + RouteConfig: &envoyRouteConfig{ + VirtualHosts: []envoyVirtualHost{ + { + Name: "ingress_service", + Domains: domains, + Routes: []envoyRoute{ + { + Match: envoyRouteMatch{Prefix: "/"}, + Route: &envoyRouteAction{Cluster: ingressClusterName}, + }, + }, + }, + }, + }, } return envoyListener{ Name: fmt.Sprintf("%s-ingress", spec.WorkloadName), Address: envoyAddress{ SocketAddress: envoySocketAddress{ - Address: "127.0.0.1", + Address: "0.0.0.0", // must be 0.0.0.0; Docker port forwarding targets the bridge IP, not container loopback PortValue: hostPort, }, }, FilterChains: []envoyFilterChain{ { - Filters: []envoyFilter{ + Filters: []envoyNetworkFilter{ { - Name: "envoy.filters.network.http_connection_manager", - TypedConfig: map[string]any{ - "upstream_cluster": upstreamRef, - "virtual_hosts": domains, + Name: "envoy.filters.network.http_connection_manager", + TypedConfig: hcm, + }, + }, + }, + }, + } +} + +// ingressDomains returns the virtual host domain list for the ingress listener. +// When Inbound.AllowHost is configured those entries are used; otherwise a +// wildcard ("*") is returned so all hostnames are accepted. +func ingressDomains(spec proxySpec) []string { + if spec.Permissions != nil && spec.Permissions.Inbound != nil && + len(spec.Permissions.Inbound.AllowHost) > 0 { + return spec.Permissions.Inbound.AllowHost + } + return []string{"*"} +} + +// buildIngressCluster returns the STRICT_DNS upstream cluster for the ingress +// listener, pointing at spec.WorkloadName:spec.UpstreamPort. +func buildIngressCluster(spec proxySpec) envoyCluster { + return envoyCluster{ + Name: ingressClusterName, + ConnectTimeout: "10s", + Type: "STRICT_DNS", + LoadAssignment: &envoyLoadAssignment{ + ClusterName: ingressClusterName, + Endpoints: []envoyEndpoint{ + { + LBEndpoints: []envoyLBEndpoint{ + { + Endpoint: envoyEndpointAddress{ + Address: envoyAddress{ + SocketAddress: envoySocketAddress{ + Address: spec.WorkloadName, + PortValue: spec.UpstreamPort, + }, + }, + }, }, }, }, @@ -316,10 +656,42 @@ func buildIngressListener(spec proxySpec, hostPort int) envoyListener { } } +// writeEnvoyBootstrap marshals b to JSON and writes it to a temporary file at +// mode 0600. Returns the file path. The caller is responsible for cleanup. +func writeEnvoyBootstrap(b envoyBootstrap) (string, error) { + data, err := json.Marshal(b) + if err != nil { + return "", fmt.Errorf("failed to marshal envoy bootstrap: %w", err) + } + tmpFile, err := os.CreateTemp("", "envoy-bootstrap-*.json") + if err != nil { + return "", fmt.Errorf("failed to create envoy bootstrap temp file: %w", err) + } + created := tmpFile.Name() + defer func() { + if cerr := tmpFile.Close(); cerr != nil { + slog.Warn("failed to close envoy bootstrap temp file", "error", cerr) + } + }() + if _, err := tmpFile.Write(data); err != nil { + _ = os.Remove(created) + return "", fmt.Errorf("failed to write envoy bootstrap: %w", err) + } + // 0600: only the owner can read — the file may contain network topology. + if err := tmpFile.Chmod(0o600); err != nil { + _ = os.Remove(created) + return "", fmt.Errorf("failed to set envoy bootstrap file permissions: %w", err) + } + return created, nil +} + +// ── envoyProxy ──────────────────────────────────────────────────────────────── + // envoyProxy implements networkProxy using Envoy as the proxy backend. // It creates a single Envoy container that handles both egress (forward proxy // on :3128) and ingress (reverse proxy) as separate listeners, reducing aux -// container count from 3 (Squid: egress + ingress + dns) to 2 (Envoy: combined + dns). +// container count from 3 (Squid: egress + ingress + dns) to 2 (Envoy: combined +// + dns). type envoyProxy struct { client *Client } @@ -328,21 +700,6 @@ type envoyProxy struct { func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyResult, error) { egressContainerName := fmt.Sprintf("%s-egress", spec.WorkloadName) - // Build Envoy bootstrap config. - var listeners []envoyListener - egressListener := buildEgressListener(spec) - listeners = append(listeners, egressListener) - - var ingressPort int - if spec.TransportType != "stdio" && spec.UpstreamPort > 0 { - port, err := networking.FindOrUsePort(spec.UpstreamPort + 1) - if err != nil { - return proxyResult{}, fmt.Errorf("failed to find ingress port: %w", err) - } - ingressPort = port - listeners = append(listeners, buildIngressListener(spec, ingressPort)) - } - bootstrap := envoyBootstrap{ Admin: &envoyAdmin{ Address: envoyAddress{ @@ -352,11 +709,29 @@ func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyRes }, }, }, - StaticResources: envoyStatic{ - Listeners: listeners, + StaticResources: envoyStaticResources{ + Listeners: []envoyListener{buildEgressListener(spec)}, + Clusters: []envoyCluster{buildEgressCluster()}, }, } + var ingressPort int + if spec.TransportType != "stdio" && spec.UpstreamPort > 0 { + port, err := networking.FindOrUsePort(spec.UpstreamPort + 1) + if err != nil { + return proxyResult{}, fmt.Errorf("failed to find ingress port: %w", err) + } + ingressPort = port + bootstrap.StaticResources.Listeners = append( + bootstrap.StaticResources.Listeners, + buildIngressListener(spec, ingressPort), + ) + bootstrap.StaticResources.Clusters = append( + bootstrap.StaticResources.Clusters, + buildIngressCluster(spec), + ) + } + configPath, err := writeEnvoyBootstrap(bootstrap) if err != nil { return proxyResult{}, fmt.Errorf("failed to write envoy bootstrap: %w", err) diff --git a/pkg/container/docker/envoy_test.go b/pkg/container/docker/envoy_test.go index 543213ff45..2d5204658c 100644 --- a/pkg/container/docker/envoy_test.go +++ b/pkg/container/docker/envoy_test.go @@ -17,18 +17,48 @@ import ( // Compile-time assertion: envoyProxy must satisfy networkProxy. var _ networkProxy = (*envoyProxy)(nil) -// findRBACFilter walks the filter chain in a listener looking for the first -// filter whose name contains "rbac". It returns nil if none is found. -// The exact field layout mirrors the typed structs that envoy.go will define. -func findRBACFilter(listener envoyListener) *envoyRBACFilter { - for _, fc := range listener.FilterChains { - for i := range fc.Filters { - if fc.Filters[i].TypedConfig != nil { - rbac, ok := fc.Filters[i].TypedConfig.(*envoyRBACFilter) - if ok { - return rbac - } - } +// findRBACFilter walks the HTTP filters inside the first HCM in a listener's +// first filter chain and returns the first RBAC filter with action == "ALLOW". +// It returns nil if no matching filter is found. +func findRBACFilter(listener envoyListener) *envoyHTTPRBAC { + if len(listener.FilterChains) == 0 { + return nil + } + fc := listener.FilterChains[0] + if len(fc.Filters) == 0 { + return nil + } + hcm, ok := fc.Filters[0].TypedConfig.(*envoyHCM) + if !ok { + return nil + } + for _, f := range hcm.HTTPFilters { + rbac, ok := f.TypedConfig.(*envoyHTTPRBAC) + if ok && rbac.Rules.Action == "ALLOW" { + return rbac + } + } + return nil +} + +// findDenyRBACFilter returns the first RBAC filter with action == "DENY" from +// the HCM inside the listener's first filter chain. +func findDenyRBACFilter(listener envoyListener) *envoyHTTPRBAC { + if len(listener.FilterChains) == 0 { + return nil + } + fc := listener.FilterChains[0] + if len(fc.Filters) == 0 { + return nil + } + hcm, ok := fc.Filters[0].TypedConfig.(*envoyHCM) + if !ok { + return nil + } + for _, f := range hcm.HTTPFilters { + rbac, ok := f.TypedConfig.(*envoyHTTPRBAC) + if ok && rbac.Rules.Action == "DENY" { + return rbac } } return nil @@ -172,6 +202,11 @@ func TestBuildEgressListener_AllowlistAndDefaultDeny(t *testing.T) { "docker gateway hostname should be absent when AllowDockerGateway=true") assert.NotContains(t, s, dockerDefaultBridgeGatewayIP, "docker gateway IP should be absent when AllowDockerGateway=true") + + // Also confirm no DENY RBAC filter exists in the Go struct. + denyFilter := findDenyRBACFilter(listener) + assert.Nil(t, denyFilter, + "no DENY RBAC filter should be present when AllowDockerGateway=true") } if tt.wantGatewayDenyL3 { @@ -197,7 +232,13 @@ func TestBuildEgressListener_AllowlistAndDefaultDeny(t *testing.T) { // buildEgressListener with an empty AllowHost and InsecureAllowAll=false must // produce a listener where the RBAC filter is present with action=ALLOW and // zero policies. This is Envoy's deny-all behavior. The test guards against a -// serialization bug that silently omits the RBAC filter and produces allow-all. +// bug that silently omits the RBAC filter and produces allow-all. +// +// Note: a JSON round-trip assertion is intentionally omitted here. The +// envoyNetworkFilter.TypedConfig field is typed as any, so concrete pointer +// types (*envoyHTTPRBAC, etc.) do not survive JSON round-trip — the unmarshaled +// value becomes map[string]any. Behavioral correctness is verified directly on +// the Go struct returned by buildEgressListener. func TestBuildEgressListener_EmptyAllowHostDenyAll(t *testing.T) { t.Parallel() @@ -224,16 +265,10 @@ func TestBuildEgressListener_EmptyAllowHostDenyAll(t *testing.T) { assert.Empty(t, rbac.Rules.Policies, "policy set must be empty to achieve deny-all semantics") - // Also verify the config round-trips as valid JSON so we catch any - // serialization bug that would silently drop the RBAC filter. + // Verify the config serialises to valid JSON. raw, err := json.Marshal(listener) require.NoError(t, err) - var roundTripped envoyListener - require.NoError(t, json.Unmarshal(raw, &roundTripped), - "listener must round-trip through JSON without error") - rbacAfter := findRBACFilter(roundTripped) - require.NotNil(t, rbacAfter, - "RBAC filter must survive JSON round-trip; omitempty on empty map is the classic culprit") + assert.NotEmpty(t, raw, "serialized listener must not be empty") } // TestBuildIngressListener_PortAndHostGating verifies that buildIngressListener @@ -259,7 +294,7 @@ func TestBuildIngressListener_PortAndHostGating(t *testing.T) { Permissions: nil, }, hostPort: 18080, - wantUpstreamRef: "myserver:8080", + wantUpstreamRef: "myserver", wantHostPortBound: 18080, }, { @@ -275,7 +310,7 @@ func TestBuildIngressListener_PortAndHostGating(t *testing.T) { }, }, hostPort: 19090, - wantUpstreamRef: "svc:9090", + wantUpstreamRef: "svc", wantHostPortBound: 19090, wantDomains: []string{"app.example.com"}, }, @@ -288,7 +323,7 @@ func TestBuildIngressListener_PortAndHostGating(t *testing.T) { Permissions: nil, }, hostPort: 17070, - wantUpstreamRef: "tool:7070", + wantUpstreamRef: "tool", wantHostPortBound: 17070, }, } @@ -323,6 +358,48 @@ func TestBuildIngressListener_PortAndHostGating(t *testing.T) { } } +// TestBuildIngressCluster_UpstreamAddress verifies that buildIngressCluster +// produces a STRICT_DNS cluster pointing at the correct workload address. +func TestBuildIngressCluster_UpstreamAddress(t *testing.T) { + t.Parallel() + + spec := proxySpec{ + WorkloadName: "myserver", + UpstreamPort: 8080, + } + + cluster := buildIngressCluster(spec) + + assert.Equal(t, ingressClusterName, cluster.Name) + assert.Equal(t, "STRICT_DNS", cluster.Type) + require.NotNil(t, cluster.LoadAssignment) + require.NotEmpty(t, cluster.LoadAssignment.Endpoints) + require.NotEmpty(t, cluster.LoadAssignment.Endpoints[0].LBEndpoints) + + ep := cluster.LoadAssignment.Endpoints[0].LBEndpoints[0] + assert.Equal(t, "myserver", ep.Endpoint.Address.SocketAddress.Address) + assert.Equal(t, 8080, ep.Endpoint.Address.SocketAddress.PortValue) +} + +// TestBuildEgressCluster_DFPConfig verifies that buildEgressCluster produces a +// dynamic_forward_proxy cluster with the expected configuration. +func TestBuildEgressCluster_DFPConfig(t *testing.T) { + t.Parallel() + + cluster := buildEgressCluster() + + assert.Equal(t, dfpClusterName, cluster.Name) + assert.Equal(t, "CLUSTER_PROVIDED", cluster.LbPolicy) + require.NotNil(t, cluster.ClusterType) + assert.Equal(t, "envoy.clusters.dynamic_forward_proxy", cluster.ClusterType.Name) + + dfp, ok := cluster.ClusterType.TypedConfig.(*envoyDFPClusterConfig) + require.True(t, ok, "ClusterType.TypedConfig must be *envoyDFPClusterConfig") + assert.Equal(t, typeDFPCluster, dfp.Type) + assert.Equal(t, dfpCacheName, dfp.DNSCacheConfig.Name) + assert.Equal(t, "V4_ONLY", dfp.DNSCacheConfig.DNSLookupFamily) +} + // TestWriteEnvoyBootstrap_FileMode verifies that writeEnvoyBootstrap writes a // valid JSON bootstrap file at mode 0600. func TestWriteEnvoyBootstrap_FileMode(t *testing.T) { @@ -337,7 +414,7 @@ func TestWriteEnvoyBootstrap_FileMode(t *testing.T) { }, }, }, - StaticResources: envoyStatic{}, + StaticResources: envoyStaticResources{}, } path, err := writeEnvoyBootstrap(b) @@ -380,7 +457,7 @@ func TestEnvoyAdmin_LoopbackOnly(t *testing.T) { }, }, }, - StaticResources: envoyStatic{}, + StaticResources: envoyStaticResources{}, } path, err := writeEnvoyBootstrap(b) @@ -439,3 +516,44 @@ func TestGetEnvoyImage(t *testing.T) { }) } } + +// TestEgressListenerHCMTypeURLs verifies that the egress listener serializes +// with correct protobuf @type URLs so Envoy can parse it. +func TestEgressListenerHCMTypeURLs(t *testing.T) { + t.Parallel() + + spec := proxySpec{ + WorkloadName: "myserver", + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{ + AllowHost: []string{"example.com"}, + }, + }, + AllowDockerGateway: false, + GatewayIP: dockerDefaultBridgeGatewayIP, + } + + listener := buildEgressListener(spec) + raw, err := json.Marshal(listener) + require.NoError(t, err) + s := string(raw) + + assert.Contains(t, s, typeHCM, "@type for HCM must be present in serialized JSON") + assert.Contains(t, s, typeHTTPRBAC, "@type for RBAC must be present in serialized JSON") + assert.Contains(t, s, typeDFPFilter, "@type for DFP filter must be present in serialized JSON") + assert.Contains(t, s, typeRouter, "@type for router must be present in serialized JSON") +} + +// TestEgressClusterTypeURL verifies that the egress cluster serializes with the +// correct DFP cluster @type URL. +func TestEgressClusterTypeURL(t *testing.T) { + t.Parallel() + + cluster := buildEgressCluster() + raw, err := json.Marshal(cluster) + require.NoError(t, err) + s := string(raw) + + assert.Contains(t, s, typeDFPCluster, + "@type for DFP cluster config must be present in serialized JSON") +} From 2b8612a4e879b345fb56b2e81f3826b28ec9eb9c Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 00:01:36 +0100 Subject: [PATCH 3/9] Match Squid dstdomain semantics for Envoy egress allowlist The Envoy allowlist translated AllowHost entries with exact/suffix string matchers keyed off a "*." prefix, which diverged from the Squid backend in two ways: Squid uses a leading-dot convention (.example.com) for subdomain matching, and its dstdomain matches the apex domain too. The prior code mistranslated .example.com into a useless exact match (silently denying traffic Squid would allow) and could not match HTTPS CONNECT authorities, which carry a :port suffix (example.com:443). Replace the string matchers with an anchored, case-insensitive RE2 pattern per host that mirrors Squid: a leading dot (or *. alias) matches the apex and all subdomains, a bare host matches exactly, and every form tolerates an optional :port. Anchoring prevents lookalike matches like example.com.attacker.com. Verified against Envoy v1.32.3 --mode validate. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/arch/14-envoy-network-proxy.md | 13 ++++-- pkg/container/docker/envoy.go | 63 ++++++++++++++++++++++------- pkg/container/docker/envoy_test.go | 52 ++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 17 deletions(-) diff --git a/docs/arch/14-envoy-network-proxy.md b/docs/arch/14-envoy-network-proxy.md index 7beddaa68f..e96b077c09 100644 --- a/docs/arch/14-envoy-network-proxy.md +++ b/docs/arch/14-envoy-network-proxy.md @@ -122,10 +122,17 @@ unless `--allow-docker-gateway` is set; it blocks: The ALLOW filter implements the permission profile's `Outbound` rules: - `InsecureAllowAll: true` → single wildcard policy (`any: true`) -- `AllowHost: [...]` → per-host `:authority` exact match (or suffix match for - `*.`-prefixed wildcards) +- `AllowHost: [...]` → per-host `:authority` regex match that mirrors Squid's + `dstdomain` semantics: `example.com` matches that host exactly, while + `.example.com` (leading dot, also accepted as `*.example.com`) matches the apex + **and** all subdomains. The generated pattern is anchored, case-insensitive, + and tolerates an optional `:port` so HTTPS CONNECT authorities + (`example.com:443`) match too. - No outbound permissions configured → empty policy map → Envoy deny-all +This preserves parity with the existing Squid backend so that permission +profiles written for Squid's `dstdomain` syntax behave identically under Envoy. + ### Ingress listener (`0.0.0.0:` — reverse proxy) ``` @@ -182,7 +189,7 @@ backend is stable. | TLS inspection | ✗ | ✗ | | L7 hostname deny | ✓ (`dstdomain`) | ✓ (`:authority` header match) | | L3 IP CIDR deny | Partial (`dst` ACL — DNS-resolved) | ✓ (direct packet match) | -| Wildcard host allowlist | ✓ (dot-prefix) | ✓ (suffix match) | +| Wildcard host allowlist | ✓ (`.example.com` dot-prefix) | ✓ (same syntax, regex match incl. apex + port) | | Per-request DNS resolution | Via Squid resolver | Via DFP cluster | | Access logs | Per-container, text format | Unified stdout, structured | | Config format | Text template | Typed Go structs → protobuf-JSON | diff --git a/pkg/container/docker/envoy.go b/pkg/container/docker/envoy.go index 3970e930a5..ea37915752 100644 --- a/pkg/container/docker/envoy.go +++ b/pkg/container/docker/envoy.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "os" + "regexp" "strings" "github.com/moby/moby/api/types/container" @@ -202,11 +203,18 @@ type envoyHeaderMatcher struct { StringMatch *envoyStringMatch `json:"string_match,omitempty"` } -// envoyStringMatch matches a string by exact value, prefix, or suffix. +// envoyStringMatch matches a string by exact value, prefix, suffix, or regex. type envoyStringMatch struct { - Exact string `json:"exact,omitempty"` - Prefix string `json:"prefix,omitempty"` - Suffix string `json:"suffix,omitempty"` + Exact string `json:"exact,omitempty"` + Prefix string `json:"prefix,omitempty"` + Suffix string `json:"suffix,omitempty"` + SafeRegex *envoySafeRegex `json:"safe_regex,omitempty"` +} + +// envoySafeRegex is an RE2 regex matcher. Envoy anchors the pattern fully +// (implicit ^ and $), so the pattern must match the entire input. +type envoySafeRegex struct { + Regex string `json:"regex"` } // envoyPrincipal matches a downstream principal. Any:true is a wildcard. @@ -499,8 +507,9 @@ func buildAllowlistRBAC(spec proxySpec) *envoyHTTPRBAC { // - spec.Permissions is nil // - spec.Permissions.Outbound is nil // -// InsecureAllowAll produces a single wildcard policy. AllowHost entries become -// :authority header matchers (exact for plain hostnames, suffix for *.prefix). +// InsecureAllowAll produces a single wildcard policy. Each AllowHost entry +// becomes a policy matching the :authority header via hostMatchRegex, which +// mirrors Squid's dstdomain semantics (see hostMatchRegex for the syntax). func buildAllowlistPolicies(spec proxySpec) map[string]envoyRBACPolicy { if spec.Permissions == nil || spec.Permissions.Outbound == nil { return make(map[string]envoyRBACPolicy) @@ -516,18 +525,14 @@ func buildAllowlistPolicies(spec proxySpec) map[string]envoyRBACPolicy { } policies := make(map[string]envoyRBACPolicy) for _, host := range out.AllowHost { - var match envoyStringMatch - if strings.HasPrefix(host, "*.") { - match.Suffix = host[1:] // "*.example.com" → ".example.com" - } else { - match.Exact = host - } policies[host] = envoyRBACPolicy{ Permissions: []envoyPermission{ { Header: &envoyHeaderMatcher{ - Name: ":authority", - StringMatch: &match, + Name: ":authority", + StringMatch: &envoyStringMatch{ + SafeRegex: &envoySafeRegex{Regex: hostMatchRegex(host)}, + }, }, }, }, @@ -537,6 +542,36 @@ func buildAllowlistPolicies(spec proxySpec) map[string]envoyRBACPolicy { return policies } +// hostMatchRegex builds an anchored, case-insensitive RE2 pattern that matches +// an AllowHost entry against the HTTP :authority header. It tolerates an +// optional ":port" suffix because HTTPS CONNECT authorities include the port +// (e.g. "example.com:443"), and it mirrors Squid's dstdomain semantics: +// +// - ".example.com" or "*.example.com" — matches the apex "example.com" AND +// any subdomain ("www.example.com", "a.b.example.com"). +// - "example.com" — matches that host exactly (no subdomains). +// +// The domain is regex-escaped so dots are literal, and the pattern is anchored +// by Envoy so it cannot match "example.com.attacker.com". +func hostMatchRegex(host string) string { + subdomains := false + switch { + case strings.HasPrefix(host, "*."): + host = host[2:] + subdomains = true + case strings.HasPrefix(host, "."): + host = host[1:] + subdomains = true + } + pattern := regexp.QuoteMeta(host) + if subdomains { + // Optional "sub." prefix at any depth, plus the apex itself. + pattern = `(.*\.)?` + pattern + } + // (?i) case-insensitive (hostnames are); (:[0-9]+)? optional port. + return `(?i)` + pattern + `(:[0-9]+)?` +} + // buildEgressCluster returns the dynamic_forward_proxy cluster required by the // egress listener's route config. func buildEgressCluster() envoyCluster { diff --git a/pkg/container/docker/envoy_test.go b/pkg/container/docker/envoy_test.go index 2d5204658c..9c659ebae4 100644 --- a/pkg/container/docker/envoy_test.go +++ b/pkg/container/docker/envoy_test.go @@ -6,6 +6,7 @@ package docker import ( "encoding/json" "os" + "regexp" "testing" "github.com/stretchr/testify/assert" @@ -228,6 +229,57 @@ func TestBuildEgressListener_AllowlistAndDefaultDeny(t *testing.T) { } } +// TestHostMatchRegex verifies that AllowHost entries translate to :authority +// patterns matching Squid's dstdomain semantics: a leading dot (or "*.") matches +// the apex and all subdomains, a bare host matches exactly, and every form +// tolerates an optional ":port" (HTTPS CONNECT authorities carry the port). +// Crucially, no form may match a lookalike parent domain like +// "example.com.attacker.com". +func TestHostMatchRegex(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + host string + matches []string + noMatches []string + }{ + { + name: "exact host, no subdomains", + host: "example.com", + matches: []string{"example.com", "example.com:443", "EXAMPLE.COM"}, + noMatches: []string{"www.example.com", "notexample.com", "example.com.attacker.com"}, + }, + { + name: "leading dot matches apex and subdomains", + host: ".example.com", + matches: []string{"example.com", "www.example.com", "a.b.example.com", "www.example.com:8080"}, + noMatches: []string{"notexample.com", "example.com.attacker.com", "evil.com"}, + }, + { + name: "asterisk-dot is an alias for leading dot", + host: "*.example.com", + matches: []string{"example.com", "api.example.com", "api.example.com:443"}, + noMatches: []string{"attacker-example.com", "example.com.attacker.com"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Envoy anchors safe_regex fully; replicate that with ^...$ here. + re := regexp.MustCompile("^(?:" + hostMatchRegex(tt.host) + ")$") + for _, m := range tt.matches { + assert.True(t, re.MatchString(m), "expected %q to match AllowHost %q", m, tt.host) + } + for _, nm := range tt.noMatches { + assert.False(t, re.MatchString(nm), "expected %q NOT to match AllowHost %q", nm, tt.host) + } + }) + } +} + // TestBuildEgressListener_EmptyAllowHostDenyAll is a mandatory regression guard: // buildEgressListener with an empty AllowHost and InsecureAllowAll=false must // produce a listener where the RBAC filter is present with action=ALLOW and From b8fee8de77a5cd083ba0cfb6e1870b4cf2dbefd4 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 00:33:13 +0100 Subject: [PATCH 4/9] Test proxy-container cleanup for Envoy and Squid topologies Adds a regression guard proving stopProxyContainer stops a container that exists and tolerates one that does not. This is what lets the shared teardown path clean up both the 3-container Squid workload and the 2-container Envoy workload (which has no -ingress container) without any backend-specific logic. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/container/docker/client_stop_test.go | 66 ++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/pkg/container/docker/client_stop_test.go b/pkg/container/docker/client_stop_test.go index 41a42dd46b..63067ff23a 100644 --- a/pkg/container/docker/client_stop_test.go +++ b/pkg/container/docker/client_stop_test.go @@ -138,3 +138,69 @@ func TestStopWorkload_NotFound_ReturnsNil(t *testing.T) { // StopWorkload should treat a not-found workload as success require.NoError(t, err) } + +// TestStopProxyContainer_HandlesEnvoyAndSquidTopologies guards the cleanup path +// shared by both proxy backends. StopWorkload iterates the fixed proxy suffixes +// (-egress, -ingress, -dns) and calls stopProxyContainer for each. A legacy Squid +// workload has all three; an Envoy workload consolidates egress+ingress into one +// container and therefore has NO -ingress container. stopProxyContainer must stop +// a container that exists and silently tolerate one that does not, so the same +// teardown works for both the 2-container (Envoy) and 3-container (Squid) +// topologies without backend-specific logic. See #5902. +func TestStopProxyContainer_HandlesEnvoyAndSquidTopologies(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + containerName string + present bool // whether the named proxy container exists + wantStopped bool + }{ + { + name: "present container is stopped (Squid egress/ingress/dns, Envoy egress/dns)", + containerName: "app-egress", + present: true, + wantStopped: true, + }, + { + name: "absent ingress is tolerated (Envoy has no -ingress container)", + containerName: "app-ingress", + present: false, + wantStopped: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var stoppedID string + api := &fakeDockerAPI{ + listFunc: func(_ context.Context, _ mobyclient.ContainerListOptions) ([]container.Summary, error) { + if !tt.present { + return []container.Summary{}, nil + } + return []container.Summary{ + {ID: "cid-" + tt.containerName, Names: []string{"/" + tt.containerName}}, + }, nil + }, + stopFunc: func(_ context.Context, id string, _ mobyclient.ContainerStopOptions) error { + stoppedID = id + return nil + }, + } + c := &Client{api: api} + + // Must not panic or error regardless of whether the container exists. + c.stopProxyContainer(t.Context(), tt.containerName, 30) + + if tt.wantStopped { + assert.Equal(t, "cid-"+tt.containerName, stoppedID, + "expected the existing proxy container to be stopped") + } else { + assert.Empty(t, stoppedID, + "expected no ContainerStop call for an absent proxy container") + } + }) + } +} From efaddfc8168e39efcde6db0faaf71b5af3fd2ead Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:50:43 +0100 Subject: [PATCH 5/9] Adapt Envoy backend to the two-phase proxy seam The seam split SetupProxies into SetupEgress (pre-MCP) and SetupIngress (post-MCP). Envoy consolidates both listeners into one container, so it creates that container in SetupEgress and returns the reserved ingress port via egressResult; SetupIngress just hands that port back. Envoy's STRICT_DNS ingress cluster resolves the MCP upstream lazily and retries, so creating the container before the MCP container is safe (unlike squid's cache_peer). Re-adds the egressResult.ingressPort field, now used to thread the reserved port from SetupEgress to SetupIngress. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/container/docker/envoy.go | 35 +++++++++++++++++++--------- pkg/container/docker/networkproxy.go | 4 ++++ 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/pkg/container/docker/envoy.go b/pkg/container/docker/envoy.go index ea37915752..9281095a96 100644 --- a/pkg/container/docker/envoy.go +++ b/pkg/container/docker/envoy.go @@ -731,8 +731,13 @@ type envoyProxy struct { client *Client } -// SetupProxies implements networkProxy for the Envoy backend. -func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyResult, error) { +// SetupEgress implements networkProxy for the Envoy backend. Envoy consolidates +// egress and ingress into a single container, so this creates that container +// (both listeners) before the MCP container. Envoy's ingress upstream is a +// STRICT_DNS cluster that resolves the MCP container lazily and keeps retrying, +// so — unlike squid's cache_peer — pre-MCP creation is safe. The reserved +// ingress port is carried back in egressResult for SetupIngress to return. +func (e *envoyProxy) SetupEgress(ctx context.Context, spec proxySpec) (egressResult, error) { egressContainerName := fmt.Sprintf("%s-egress", spec.WorkloadName) bootstrap := envoyBootstrap{ @@ -754,7 +759,7 @@ func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyRes if spec.TransportType != "stdio" && spec.UpstreamPort > 0 { port, err := networking.FindOrUsePort(spec.UpstreamPort + 1) if err != nil { - return proxyResult{}, fmt.Errorf("failed to find ingress port: %w", err) + return egressResult{}, fmt.Errorf("failed to find ingress port: %w", err) } ingressPort = port bootstrap.StaticResources.Listeners = append( @@ -769,7 +774,7 @@ func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyRes configPath, err := writeEnvoyBootstrap(bootstrap) if err != nil { - return proxyResult{}, fmt.Errorf("failed to write envoy bootstrap: %w", err) + return egressResult{}, fmt.Errorf("failed to write envoy bootstrap: %w", err) } envoyImage := getEnvoyImage() @@ -778,7 +783,7 @@ func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyRes if err := e.client.imageManager.PullImage(ctx, envoyImage); err != nil { _, inspectErr := e.client.imageManager.ImageExists(ctx, envoyImage) if inspectErr != nil { - return proxyResult{}, fmt.Errorf("failed to pull envoy image: %w", err) + return egressResult{}, fmt.Errorf("failed to pull envoy image: %w", err) } slog.Debug("envoy image exists locally, continuing despite pull failure", "image", envoyImage) } @@ -821,19 +826,27 @@ func (e *envoyProxy) SetupProxies(ctx context.Context, spec proxySpec) (proxyRes } if portBindings != nil { if err := setupPortBindings(hostConfig, portBindings); err != nil { - return proxyResult{}, fmt.Errorf("failed to setup port bindings: %w", err) + return egressResult{}, fmt.Errorf("failed to setup port bindings: %w", err) } } if err := setupExposedPorts(config, exposedPorts); err != nil { - return proxyResult{}, fmt.Errorf("failed to setup exposed ports: %w", err) + return egressResult{}, fmt.Errorf("failed to setup exposed ports: %w", err) } if _, err := e.client.createContainer(ctx, egressContainerName, config, hostConfig, spec.Endpoints); err != nil { - return proxyResult{}, fmt.Errorf("failed to create envoy container: %w", err) + return egressResult{}, fmt.Errorf("failed to create envoy container: %w", err) } - return proxyResult{ - IngressHostPort: ingressPort, - EnvVars: addEgressEnvVars(nil, egressContainerName), + return egressResult{ + EnvVars: addEgressEnvVars(nil, egressContainerName), + ingressPort: ingressPort, }, nil } + +// SetupIngress implements networkProxy for the Envoy backend. Envoy already +// created its ingress listener as part of the single container in SetupEgress, +// so there is nothing more to create here — it simply returns the ingress port +// reserved in SetupEgress (0 for stdio / UpstreamPort==0). +func (*envoyProxy) SetupIngress(_ context.Context, _ proxySpec, egress egressResult) (int, error) { + return egress.ingressPort, nil +} diff --git a/pkg/container/docker/networkproxy.go b/pkg/container/docker/networkproxy.go index 8b2731247e..1e4915ccc3 100644 --- a/pkg/container/docker/networkproxy.go +++ b/pkg/container/docker/networkproxy.go @@ -80,6 +80,10 @@ type egressResult struct { // EnvVars contains environment variables that must be merged into the MCP // container's environment (e.g. HTTP_PROXY, HTTPS_PROXY). EnvVars map[string]string + // ingressPort is the host-side ingress port reserved by a consolidated + // backend (envoy) when it created its container in SetupEgress. Per-container + // backends (squid) leave it 0 and bind the ingress port later in SetupIngress. + ingressPort int } // newNetworkProxy reads the TOOLHIVE_NETWORK_PROXY environment variable and From a5457eb0bfd94bf4b2d30db5f95aeabfd7e908dc Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:15:28 +0100 Subject: [PATCH 6/9] Block Docker gateway by :authority and address PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gateway deny relied on HTTP RBAC destination_ip, which Envoy resolves to its own listener socket for a forward proxy — never the proxied target — so the L3 rule was inert and a raw-IP request to the gateway was not blocked. --allow-docker-gateway also only omitted the deny without adding an allow, so the gateway stayed blocked under a non-permissive allowlist. Match the gateway on :authority (the forward-proxy target for both plain HTTP and CONNECT) using anchored, case-insensitive, port-tolerant regexes covering the gateway IP literal and both hostnames — mirroring Squid's dst/dstdomain denies. Add explicit gateway ALLOW policies when --allow-docker-gateway is set. Drop the dead destination_ip rule. Also addresses review hygiene: real-Envoy `--mode validate` test and a SetupEgress/SetupIngress orchestration test; drop container capabilities; image comment (tag, not digest, tracked in #5903); check ImageExists bool on pull fallback; ptr.To over local boolPtr; strconv over Sprintf for ports; de-duplicate the bootstrap-write error; and corrected/renamed the misleading ingress and cleanup test descriptions. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/container/docker/client_stop_test.go | 16 +- pkg/container/docker/envoy.go | 146 +++++++------- pkg/container/docker/envoy_test.go | 234 ++++++++++++++++++++--- 3 files changed, 293 insertions(+), 103 deletions(-) diff --git a/pkg/container/docker/client_stop_test.go b/pkg/container/docker/client_stop_test.go index 63067ff23a..4bc00df183 100644 --- a/pkg/container/docker/client_stop_test.go +++ b/pkg/container/docker/client_stop_test.go @@ -139,14 +139,14 @@ func TestStopWorkload_NotFound_ReturnsNil(t *testing.T) { require.NoError(t, err) } -// TestStopProxyContainer_HandlesEnvoyAndSquidTopologies guards the cleanup path -// shared by both proxy backends. StopWorkload iterates the fixed proxy suffixes -// (-egress, -ingress, -dns) and calls stopProxyContainer for each. A legacy Squid -// workload has all three; an Envoy workload consolidates egress+ingress into one -// container and therefore has NO -ingress container. stopProxyContainer must stop -// a container that exists and silently tolerate one that does not, so the same -// teardown works for both the 2-container (Envoy) and 3-container (Squid) -// topologies without backend-specific logic. See #5902. +// TestStopProxyContainer_HandlesEnvoyAndSquidTopologies exercises the building +// block of the shared cleanup path. StopWorkload iterates the fixed proxy +// suffixes (-egress, -ingress, -dns) and calls stopProxyContainer for each; this +// test verifies stopProxyContainer's per-container contract directly: stop a +// container that exists, and silently tolerate one that does not. That tolerance +// is what lets the same suffix iteration clean up both the 3-container Squid +// workload and the 2-container Envoy workload (which has no -ingress container) +// without backend-specific logic. See #5902. func TestStopProxyContainer_HandlesEnvoyAndSquidTopologies(t *testing.T) { t.Parallel() diff --git a/pkg/container/docker/envoy.go b/pkg/container/docker/envoy.go index 9281095a96..2596228dd0 100644 --- a/pkg/container/docker/envoy.go +++ b/pkg/container/docker/envoy.go @@ -10,9 +10,11 @@ import ( "log/slog" "os" "regexp" + "strconv" "strings" "github.com/moby/moby/api/types/container" + "k8s.io/utils/ptr" "github.com/stacklok/toolhive/pkg/container/runtime" lb "github.com/stacklok/toolhive/pkg/labels" @@ -20,7 +22,7 @@ import ( ) const ( - // defaultEnvoyImage is pinned by digest to prevent unexpected updates. + // defaultEnvoyImage is pinned by tag. Digest pinning is tracked in #5903. // Override with TOOLHIVE_ENVOY_IMAGE. defaultEnvoyImage = "envoyproxy/envoy-distroless:v1.32.3" @@ -51,10 +53,6 @@ func getEnvoyImage() string { return defaultEnvoyImage } -// boolPtr returns a pointer to b, used for the RBAC any permission field which -// requires a pointer to distinguish "unset" from "false". -func boolPtr(b bool) *bool { return &b } - // ── Bootstrap ──────────────────────────────────────────────────────────────── // envoyBootstrap is the top-level Envoy bootstrap configuration. @@ -180,10 +178,9 @@ type envoyRBACPolicy struct { // envoyPermission matches a request by various criteria. Exactly one field // should be set per permission entry. type envoyPermission struct { - Any *bool `json:"any,omitempty"` - Header *envoyHeaderMatcher `json:"header,omitempty"` - DestinationIP *envoyCIDRRange `json:"destination_ip,omitempty"` - OrRules *envoyOrRules `json:"or_rules,omitempty"` + Any *bool `json:"any,omitempty"` + Header *envoyHeaderMatcher `json:"header,omitempty"` + OrRules *envoyOrRules `json:"or_rules,omitempty"` } // envoyOrRules composes multiple permissions with logical OR. @@ -191,12 +188,6 @@ type envoyOrRules struct { Rules []envoyPermission `json:"rules"` } -// envoyCIDRRange matches destination IPs against a CIDR prefix. -type envoyCIDRRange struct { - AddressPrefix string `json:"address_prefix"` - PrefixLen uint32 `json:"prefix_len"` -} - // envoyHeaderMatcher matches an HTTP header value. type envoyHeaderMatcher struct { Name string `json:"name"` @@ -436,60 +427,55 @@ func buildEgressHTTPFilters(spec proxySpec) []envoyHTTPFilter { return filters } -// buildGatewayDenyRBAC builds an RBAC DENY filter that blocks the Docker -// bridge gateway IP (L3 CIDR) and the Docker-internal hostnames (L7 authority -// header). This filter must precede the allowlist filter. +// buildGatewayDenyRBAC builds an RBAC DENY filter that blocks the Docker bridge +// gateway — both the resolved gateway IP and the Docker-internal hostnames — +// matched on the :authority header. This filter must precede the allowlist. +// +// Matching is on :authority (not destination_ip): for a forward proxy the RBAC +// destination_ip resolves to Envoy's own listener socket, not the proxied +// target, so an L3 CIDR rule never matches the upstream. The forward-proxy +// target is carried in :authority for both plain HTTP and HTTPS CONNECT, so +// authority matching — including the gateway IP literal — is what actually +// blocks it (mirroring Squid's `dst`/`dstdomain` denies). func buildGatewayDenyRBAC(gatewayIP string) *envoyHTTPRBAC { return &envoyHTTPRBAC{ Type: typeHTTPRBAC, Rules: envoyRBACRules{ Action: "DENY", Policies: map[string]envoyRBACPolicy{ - "gateway-ip": { - Permissions: []envoyPermission{ - { - DestinationIP: &envoyCIDRRange{ - AddressPrefix: gatewayIP, - PrefixLen: 32, - }, - }, - }, - Principals: []envoyPrincipal{{Any: true}}, - }, - "gateway-hostnames": { - Permissions: []envoyPermission{ - { - // Prefix match covers both plain HTTP (:authority = "host.docker.internal") - // and HTTPS CONNECT (:authority = "host.docker.internal:443"). - OrRules: &envoyOrRules{ - Rules: []envoyPermission{ - { - Header: &envoyHeaderMatcher{ - Name: ":authority", - StringMatch: &envoyStringMatch{ - Prefix: dockerGatewayHostname, - }, - }, - }, - { - Header: &envoyHeaderMatcher{ - Name: ":authority", - StringMatch: &envoyStringMatch{ - Prefix: dockerAltGatewayHostname, - }, - }, - }, - }, - }, - }, - }, - Principals: []envoyPrincipal{{Any: true}}, + "gateway": { + Permissions: gatewayAuthorityPermissions(gatewayIP), + Principals: []envoyPrincipal{{Any: true}}, }, }, }, } } +// gatewayAuthorityPermissions returns RBAC permissions that match the Docker +// gateway on the :authority header — the resolved gateway IP literal plus the +// two Docker-internal hostnames. Each is matched with hostMatchRegex, so the +// match is anchored, case-insensitive (HOST.DOCKER.INTERNAL cannot bypass it), +// and tolerant of an optional :port. Shared by the deny path (gateway blocked) +// and the allow path (--allow-docker-gateway). +func gatewayAuthorityPermissions(gatewayIP string) []envoyPermission { + patterns := []string{ + hostMatchRegex(gatewayIP), + hostMatchRegex(dockerGatewayHostname), + hostMatchRegex(dockerAltGatewayHostname), + } + rules := make([]envoyPermission, 0, len(patterns)) + for _, re := range patterns { + rules = append(rules, envoyPermission{ + Header: &envoyHeaderMatcher{ + Name: ":authority", + StringMatch: &envoyStringMatch{SafeRegex: &envoySafeRegex{Regex: re}}, + }, + }) + } + return []envoyPermission{{OrRules: &envoyOrRules{Rules: rules}}} +} + // buildAllowlistRBAC builds an RBAC ALLOW filter encoding the outbound // allowlist. An empty Policies map is Envoy's deny-all under ALLOW action. func buildAllowlistRBAC(spec proxySpec) *envoyHTTPRBAC { @@ -510,20 +496,34 @@ func buildAllowlistRBAC(spec proxySpec) *envoyHTTPRBAC { // InsecureAllowAll produces a single wildcard policy. Each AllowHost entry // becomes a policy matching the :authority header via hostMatchRegex, which // mirrors Squid's dstdomain semantics (see hostMatchRegex for the syntax). +// +// When spec.AllowDockerGateway is set, an explicit ALLOW policy for the gateway +// is added: omitting the deny filter alone is not enough, because the allowlist +// is default-deny, so the gateway would still be blocked under a non-permissive +// profile. This mirrors Squid, which emits explicit allow rules for the flag. func buildAllowlistPolicies(spec proxySpec) map[string]envoyRBACPolicy { + policies := make(map[string]envoyRBACPolicy) + + // --allow-docker-gateway: grant the gateway explicitly (see doc comment). + // Harmless under InsecureAllowAll (allow-all already covers it). + if spec.AllowDockerGateway { + policies["gateway"] = envoyRBACPolicy{ + Permissions: gatewayAuthorityPermissions(spec.GatewayIP), + Principals: []envoyPrincipal{{Any: true}}, + } + } + if spec.Permissions == nil || spec.Permissions.Outbound == nil { - return make(map[string]envoyRBACPolicy) + return policies } out := spec.Permissions.Outbound if out.InsecureAllowAll { - return map[string]envoyRBACPolicy{ - "allow-all": { - Permissions: []envoyPermission{{Any: boolPtr(true)}}, - Principals: []envoyPrincipal{{Any: true}}, - }, + policies["allow-all"] = envoyRBACPolicy{ + Permissions: []envoyPermission{{Any: ptr.To(true)}}, + Principals: []envoyPrincipal{{Any: true}}, } + return policies } - policies := make(map[string]envoyRBACPolicy) for _, host := range out.AllowHost { policies[host] = envoyRBACPolicy{ Permissions: []envoyPermission{ @@ -774,17 +774,22 @@ func (e *envoyProxy) SetupEgress(ctx context.Context, spec proxySpec) (egressRes configPath, err := writeEnvoyBootstrap(bootstrap) if err != nil { - return egressResult{}, fmt.Errorf("failed to write envoy bootstrap: %w", err) + return egressResult{}, err // already wrapped with context } envoyImage := getEnvoyImage() + //nolint:gosec // G706: envoy image name from config slog.Debug("setting up envoy container", "name", egressContainerName, "image", envoyImage) if err := e.client.imageManager.PullImage(ctx, envoyImage); err != nil { - _, inspectErr := e.client.imageManager.ImageExists(ctx, envoyImage) - if inspectErr != nil { + // Fall back to a locally-present image; only proceed if it actually + // exists (ImageExists returns (false, nil) when absent, so the bool + // must be checked, not just the error). + exists, inspectErr := e.client.imageManager.ImageExists(ctx, envoyImage) + if inspectErr != nil || !exists { return egressResult{}, fmt.Errorf("failed to pull envoy image: %w", err) } + //nolint:gosec // G706: envoy image name from config slog.Debug("envoy image exists locally, continuing despite pull failure", "image", envoyImage) } @@ -809,10 +814,11 @@ func (e *envoyProxy) SetupEgress(ctx context.Context, spec proxySpec) (egressRes var exposedPorts map[string]struct{} var portBindings map[string][]runtime.PortBinding if ingressPort > 0 { - portKey := fmt.Sprintf("%d/tcp", ingressPort) + portStr := strconv.Itoa(ingressPort) + portKey := portStr + "/tcp" exposedPorts = map[string]struct{}{portKey: {}} portBindings = map[string][]runtime.PortBinding{ - portKey: {{HostIP: "127.0.0.1", HostPort: fmt.Sprintf("%d", ingressPort)}}, + portKey: {{HostIP: "127.0.0.1", HostPort: portStr}}, } } @@ -820,6 +826,8 @@ func (e *envoyProxy) SetupEgress(ctx context.Context, spec proxySpec) (egressRes Mounts: convertMounts(mounts), NetworkMode: container.NetworkMode("bridge"), SecurityOpt: []string{"label:disable"}, + // Envoy distroless runs as nonroot and needs no capabilities; drop all. + CapDrop: []string{"ALL"}, RestartPolicy: container.RestartPolicy{ Name: "unless-stopped", }, diff --git a/pkg/container/docker/envoy_test.go b/pkg/container/docker/envoy_test.go index 9c659ebae4..76150eb7b4 100644 --- a/pkg/container/docker/envoy_test.go +++ b/pkg/container/docker/envoy_test.go @@ -4,11 +4,18 @@ package docker import ( + "context" "encoding/json" "os" + "os/exec" "regexp" "testing" + "time" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" + mobyclient "github.com/moby/moby/client" + v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,6 +49,39 @@ func findRBACFilter(listener envoyListener) *envoyHTTPRBAC { return nil } +// collectAuthorityRegexes walks every policy/permission in an RBAC filter +// (including or_rules) and returns all :authority safe_regex patterns. +func collectAuthorityRegexes(rbac *envoyHTTPRBAC) []string { + var out []string + var walk func(perms []envoyPermission) + walk = func(perms []envoyPermission) { + for _, p := range perms { + if p.Header != nil && p.Header.Name == ":authority" && + p.Header.StringMatch != nil && p.Header.StringMatch.SafeRegex != nil { + out = append(out, p.Header.StringMatch.SafeRegex.Regex) + } + if p.OrRules != nil { + walk(p.OrRules.Rules) + } + } + } + for _, pol := range rbac.Rules.Policies { + walk(pol.Permissions) + } + return out +} + +// authorityMatched reports whether any of the given RE2 patterns matches the +// authority, replicating Envoy's full-string anchoring. +func authorityMatched(patterns []string, authority string) bool { + for _, p := range patterns { + if regexp.MustCompile("^(?:" + p + ")$").MatchString(authority) { + return true + } + } + return false +} + // findDenyRBACFilter returns the first RBAC filter with action == "DENY" from // the HCM inside the listener's first filter chain. func findDenyRBACFilter(listener envoyListener) *envoyHTTPRBAC { @@ -194,36 +234,47 @@ func TestBuildEgressListener_AllowlistAndDefaultDeny(t *testing.T) { } if tt.wantGatewayDenyAbsent { - // Serialize to JSON and verify neither gateway hostname nor the - // gateway CIDR deny appear anywhere in the config. - raw, err := json.Marshal(listener) - require.NoError(t, err) - s := string(raw) - assert.NotContains(t, s, dockerGatewayHostname, - "docker gateway hostname should be absent when AllowDockerGateway=true") - assert.NotContains(t, s, dockerDefaultBridgeGatewayIP, - "docker gateway IP should be absent when AllowDockerGateway=true") - - // Also confirm no DENY RBAC filter exists in the Go struct. - denyFilter := findDenyRBACFilter(listener) - assert.Nil(t, denyFilter, + // --allow-docker-gateway: no DENY filter, and the ALLOW filter must + // explicitly permit the gateway (omitting the deny alone is not + // enough under a default-deny allowlist — see #2). + assert.Nil(t, findDenyRBACFilter(listener), "no DENY RBAC filter should be present when AllowDockerGateway=true") + allow := findRBACFilter(listener) + require.NotNil(t, allow) + pats := collectAuthorityRegexes(allow) + assert.True(t, authorityMatched(pats, dockerDefaultBridgeGatewayIP), + "ALLOW filter must permit the gateway IP when AllowDockerGateway=true") + assert.True(t, authorityMatched(pats, dockerGatewayHostname), + "ALLOW filter must permit host.docker.internal when AllowDockerGateway=true") } - if tt.wantGatewayDenyL3 { - raw, err := json.Marshal(listener) - require.NoError(t, err) - assert.Contains(t, string(raw), dockerDefaultBridgeGatewayIP, - "expected L3 CIDR deny on gateway IP") - } - - if tt.wantGatewayDenyL7 { + // wantGatewayDenyL3/L7: the DENY filter must block the gateway by + // :authority — the IP literal (with/without port) AND the hostnames, + // case-insensitively — without a destination_ip rule (inert for a + // forward proxy). See #1/#4. + if tt.wantGatewayDenyL3 || tt.wantGatewayDenyL7 { + deny := findDenyRBACFilter(listener) + require.NotNil(t, deny, "expected a DENY RBAC filter for the gateway") + pats := collectAuthorityRegexes(deny) + + assert.True(t, authorityMatched(pats, dockerDefaultBridgeGatewayIP), + "deny must match the gateway IP") + assert.True(t, authorityMatched(pats, dockerDefaultBridgeGatewayIP+":8080"), + "deny must match the gateway IP with a port (raw-IP bypass)") + assert.True(t, authorityMatched(pats, dockerGatewayHostname), + "deny must match host.docker.internal") + assert.True(t, authorityMatched(pats, dockerAltGatewayHostname), + "deny must match gateway.docker.internal") + assert.True(t, authorityMatched(pats, "HOST.DOCKER.INTERNAL"), + "deny must be case-insensitive (uppercase must not bypass)") + assert.False(t, authorityMatched(pats, dockerGatewayHostname+".attacker.com"), + "deny must be anchored (lookalike suffix must not match)") + + // The dead destination_ip L3 rule must be gone. raw, err := json.Marshal(listener) require.NoError(t, err) - assert.Contains(t, string(raw), dockerGatewayHostname, - "expected L7 authority deny for host.docker.internal") - assert.Contains(t, string(raw), dockerAltGatewayHostname, - "expected L7 authority deny for gateway.docker.internal") + assert.NotContains(t, string(raw), "destination_ip", + "destination_ip is inert for a forward proxy and must not be used") } }) } @@ -367,7 +418,10 @@ func TestBuildIngressListener_PortAndHostGating(t *testing.T) { wantDomains: []string{"app.example.com"}, }, { - name: "nil permissions defaults to permissive localhost gating", + // No inbound AllowHost → wildcard virtual host. This is safe because + // the ingress listener is bound to 127.0.0.1 on the host, so only the + // local proxy runner can reach it regardless of the vhost domain. + name: "nil permissions defaults to wildcard virtual host", spec: proxySpec{ WorkloadName: "tool", UpstreamPort: 7070, @@ -377,6 +431,7 @@ func TestBuildIngressListener_PortAndHostGating(t *testing.T) { hostPort: 17070, wantUpstreamRef: "tool", wantHostPortBound: 17070, + wantDomains: []string{`"*"`}, }, } @@ -609,3 +664,130 @@ func TestEgressClusterTypeURL(t *testing.T) { assert.Contains(t, s, typeDFPCluster, "@type for DFP cluster config must be present in serialized JSON") } + +// TestEnvoyBootstrap_ValidatesAgainstRealEnvoy asserts that the generated +// bootstrap is accepted by a real Envoy (`--mode validate`), across the deny, +// allow-gateway, and allowlist permutations. Hand-rolled protobuf-JSON can pass +// Go-level unit tests yet be rejected by Envoy (wrong @type, bad matcher shape), +// so this closes that gap. Skips when docker is unavailable. +func TestEnvoyBootstrap_ValidatesAgainstRealEnvoy(t *testing.T) { + t.Parallel() + + if _, err := exec.LookPath("docker"); err != nil { + t.Skip("docker not available; skipping real-Envoy validation") + } + + cases := []struct { + name string + spec proxySpec + }{ + { + name: "allowlist + gateway deny + ingress", + spec: proxySpec{ + WorkloadName: "demo", TransportType: "streamable-http", UpstreamPort: 8080, + GatewayIP: dockerDefaultBridgeGatewayIP, + Permissions: &permissions.NetworkPermissions{ + Outbound: &permissions.OutboundNetworkPermissions{AllowHost: []string{"example.com", ".github.com"}}, + Inbound: &permissions.InboundNetworkPermissions{AllowHost: []string{"app.internal"}}, + }, + }, + }, + { + name: "allow-docker-gateway + insecure-allow-all + stdio (egress only)", + spec: proxySpec{ + WorkloadName: "demo", TransportType: "stdio", AllowDockerGateway: true, + GatewayIP: dockerDefaultBridgeGatewayIP, + Permissions: &permissions.NetworkPermissions{Outbound: &permissions.OutboundNetworkPermissions{InsecureAllowAll: true}}, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + listeners := []envoyListener{buildEgressListener(tc.spec)} + clusters := []envoyCluster{buildEgressCluster()} + if tc.spec.TransportType != "stdio" { + listeners = append(listeners, buildIngressListener(tc.spec, 18080)) + clusters = append(clusters, buildIngressCluster(tc.spec)) + } + b := envoyBootstrap{ + Admin: &envoyAdmin{Address: envoyAddress{SocketAddress: envoySocketAddress{Address: "127.0.0.1", PortValue: 9901}}}, + StaticResources: envoyStaticResources{Listeners: listeners, Clusters: clusters}, + } + path, err := writeEnvoyBootstrap(b) + require.NoError(t, err) + t.Cleanup(func() { _ = os.Remove(path) }) + + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cancel() + //nolint:gosec // G204: fixed args; path is a test-controlled temp file + out, err := exec.CommandContext(ctx, "docker", "run", "--rm", + "-v", path+":/etc/envoy/envoy.json:ro", + defaultEnvoyImage, "--mode", "validate", "-c", "/etc/envoy/envoy.json").CombinedOutput() + require.NoError(t, err, "envoy rejected the generated config:\n%s", out) + assert.Contains(t, string(out), "configuration '/etc/envoy/envoy.json' OK") + }) + } +} + +// TestEnvoyProxy_SetupOrchestration covers the container-orchestration branch +// logic of SetupEgress/SetupIngress without a live daemon: the egress container +// is always created (named -egress), a non-stdio workload reserves an +// ingress port that SetupIngress returns, and a stdio workload reserves none. +func TestEnvoyProxy_SetupOrchestration(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + transport string + upstreamPort int + wantIngress bool + }{ + {name: "non-stdio reserves an ingress port", transport: "streamable-http", upstreamPort: 8080, wantIngress: true}, + {name: "stdio reserves no ingress port", transport: "stdio", upstreamPort: 0, wantIngress: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var createdName string + api := &fakeDockerAPI{ + createFunc: func(_ context.Context, _ *container.Config, _ *container.HostConfig, _ *network.NetworkingConfig, _ *v1.Platform, name string) (container.CreateResponse, error) { + createdName = name + return container.CreateResponse{ID: "cid-envoy"}, nil + }, + startFunc: func(_ context.Context, _ string, _ mobyclient.ContainerStartOptions) error { return nil }, + } + c := &Client{ + api: api, + imageManager: &fakeImageManager{availableImages: map[string]struct{}{defaultEnvoyImage: {}}}, + } + e := &envoyProxy{client: c} + + spec := proxySpec{ + WorkloadName: "app", + TransportType: tt.transport, + UpstreamPort: tt.upstreamPort, + GatewayIP: dockerDefaultBridgeGatewayIP, + Endpoints: map[string]*network.EndpointSettings{}, + } + + egress, err := e.SetupEgress(t.Context(), spec) + require.NoError(t, err) + assert.Equal(t, "app-egress", createdName, "envoy container must reuse the -egress name") + assert.Equal(t, "http://app-egress:3128", egress.EnvVars["HTTP_PROXY"]) + + ingressPort, err := e.SetupIngress(t.Context(), spec, egress) + require.NoError(t, err) + if tt.wantIngress { + assert.Positive(t, ingressPort, "non-stdio must reserve an ingress port") + assert.Equal(t, egress.ingressPort, ingressPort, "SetupIngress must return the port reserved in SetupEgress") + } else { + assert.Zero(t, ingressPort, "stdio must not reserve an ingress port") + } + }) + } +} From 3e00858a233026183cda4277a6723cad80c42175 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:18:40 +0100 Subject: [PATCH 7/9] Correct Envoy arch doc and add it to the arch index Fix the gateway-blocking description (authority-based, not an inert L3 destination_ip rule), document --allow-docker-gateway adding explicit allow policies, drop the false temp-file cleanup claim, and expand Known Limitations (AllowPort #5915, V4_ONLY DNS, wildcard ingress vhost bounded by loopback binding, uncleaned bootstrap file, tag-pinned image #5903, deny-all-on-empty-profile divergence from Squid). Renumber to 15- to avoid a filename clash with 14-plugins-system.md and add it to docs/arch/README.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...ork-proxy.md => 15-envoy-network-proxy.md} | 98 ++++++++++++++----- docs/arch/README.md | 7 ++ 2 files changed, 80 insertions(+), 25 deletions(-) rename docs/arch/{14-envoy-network-proxy.md => 15-envoy-network-proxy.md} (64%) diff --git a/docs/arch/14-envoy-network-proxy.md b/docs/arch/15-envoy-network-proxy.md similarity index 64% rename from docs/arch/14-envoy-network-proxy.md rename to docs/arch/15-envoy-network-proxy.md index e96b077c09..390fb2ed51 100644 --- a/docs/arch/14-envoy-network-proxy.md +++ b/docs/arch/15-envoy-network-proxy.md @@ -44,21 +44,25 @@ Envoy's `HttpConnectionManager` supports multiple listeners in a single process. The egress forward proxy (`:3128`) and ingress reverse proxy share the same Envoy instance, the same access logs, and the same lifecycle. -### L3 + L7 enforcement - -Squid operates at L7 only — it can match destination hostnames via `dstdomain` -ACLs but cannot match by IP address in a reliable, port-independent way. - -Envoy's RBAC filter supports: -- **`destination_ip`** — CIDR match at L3/L4, applied before the request is - parsed as HTTP. This catches direct-IP connections that bypass DNS. -- **Header match on `:authority`** — L7 match on the CONNECT target or HTTP - Host header, equivalent to Squid's `dstdomain`. - -ToolHive combines both layers: outbound traffic is blocked at L3 for known IP -ranges and at L7 for hostname patterns. The `Internal: true` Docker network -remains the fail-closed backstop for non-cooperative traffic that ignores the -proxy entirely. +### L7 authority-based enforcement + +Squid matches destination hostnames via `dstdomain` ACLs and destination IPs via +`dst` ACLs. Envoy, running as a forward proxy, enforces the equivalent controls +at L7 by matching the HTTP `:authority` header. + +Envoy's RBAC filter is used for both allow and deny decisions: +- **Header match on `:authority`** — L7 match on the forward-proxy target for + both plain HTTP requests and HTTPS CONNECT tunnels. This is the authority the + client asks the proxy to reach, so a single match covers hostnames *and* IP + literals presented as the CONNECT target. + +Note that RBAC `destination_ip` is **not** usable for target filtering in a +forward proxy: `destination_ip` resolves to Envoy's own listener socket (the +address the client connected to), not the proxied target, so a CIDR match on it +is inert. All target enforcement therefore happens through `:authority` regexes, +which mirror Squid's `dstdomain`/`dst` denies. The `Internal: true` Docker +network remains the fail-closed backstop for non-cooperative traffic that ignores +the proxy entirely. ### Proper dynamic forward proxy @@ -107,18 +111,28 @@ listener and iptables rules that Envoy handles cleanly. ``` HTTP_PROXY / HTTPS_PROXY → Envoy :3128 └── HCM (upgrade: CONNECT) - ├── [optional] RBAC DENY — docker gateway IP (L3) + hostnames (L7) + ├── [optional] RBAC DENY — docker gateway (:authority: IP literal + hostnames) ├── RBAC ALLOW — outbound allowlist (or allow-all) ├── dynamic_forward_proxy — per-request DNS + CONNECT tunnel └── router ``` The RBAC filters are evaluated top-to-bottom. The gateway DENY filter is present -unless `--allow-docker-gateway` is set; it blocks: -- The resolved Docker bridge gateway IP as a /32 CIDR (`destination_ip`) -- `host.docker.internal` and `gateway.docker.internal` as `:authority` prefix - matches (covers both plain HTTP and HTTPS CONNECT where authority includes the - port, e.g. `host.docker.internal:443`) +unless `--allow-docker-gateway` is set. It blocks the Docker gateway purely at +L7, by matching the `:authority` header (the forward-proxy target for both plain +HTTP and HTTPS CONNECT) against anchored, case-insensitive, port-tolerant +regexes for: +- The resolved Docker bridge gateway IP literal (e.g. `172.17.0.1`, with an + optional `:port`) +- The Docker-internal hostnames `host.docker.internal` and + `gateway.docker.internal` (with an optional `:port`, so HTTPS CONNECT + authorities such as `host.docker.internal:443` match too) + +There is deliberately no `destination_ip`/L3 CIDR component: as noted above, a +forward proxy's `destination_ip` is Envoy's own listener, so it would never match +the gateway. Matching the IP literal in `:authority` is what actually catches a +client that connects directly to the gateway address, giving parity with Squid's +combined `dst`/`dstdomain` denies. The ALLOW filter implements the permission profile's `Outbound` rules: - `InsecureAllowAll: true` → single wildcard policy (`any: true`) @@ -133,6 +147,15 @@ The ALLOW filter implements the permission profile's `Outbound` rules: This preserves parity with the existing Squid backend so that permission profiles written for Squid's `dstdomain` syntax behave identically under Envoy. +When `--allow-docker-gateway` is set, ToolHive not only omits the gateway DENY +filter but also injects explicit ALLOW policies for the gateway (the IP literal +and both Docker-internal hostnames, matched the same way as the deny). This is +required because the ALLOW filter is default-deny: under a non-permissive profile +(one without `InsecureAllowAll`), simply dropping the deny would leave the +gateway unreachable, since nothing in the allowlist would match it. The explicit +ALLOW entries ensure the gateway is actually permitted rather than merely +un-denied. + ### Ingress listener (`0.0.0.0:` — reverse proxy) ``` @@ -161,7 +184,12 @@ admin API being accessible from other containers. 2. The file is bind-mounted read-only into the Envoy container at `/etc/envoy/envoy.json`. 3. Envoy reads it once at startup. -4. The file is cleaned up when ToolHive removes the workload. + +The bootstrap file is **not** removed when the workload is torn down — there is +no cleanup hook for it today. The only removal path is a best-effort cleanup if +generation fails partway through writing. As a result, a bootstrap file persists +in the OS temp directory (at mode `0600`) for the lifetime of the host until the +OS reclaims the temp directory. This is a known limitation (see below). ## Selection @@ -188,7 +216,7 @@ backend is stable. | HTTPS CONNECT tunnelling | ✓ | ✓ | | TLS inspection | ✗ | ✗ | | L7 hostname deny | ✓ (`dstdomain`) | ✓ (`:authority` header match) | -| L3 IP CIDR deny | Partial (`dst` ACL — DNS-resolved) | ✓ (direct packet match) | +| Destination IP deny | ✓ (`dst` ACL) | ✓ (IP literal matched in `:authority`) | | Wildcard host allowlist | ✓ (`.example.com` dot-prefix) | ✓ (same syntax, regex match incl. apex + port) | | Per-request DNS resolution | Via Squid resolver | Via DFP cluster | | Access logs | Per-container, text format | Unified stdout, structured | @@ -201,6 +229,7 @@ backend is stable. - **Tag-pinned image.** The Envoy image is pinned by tag (`v1.32.3`), not by digest. A future PR should pin by digest and add a `TOOLHIVE_ENVOY_IMAGE` override for supply-chain policy requirements (the env var already exists). + Tracked in #5903. - **Admin interface port.** The admin API on `:9901` (loopback-only inside the container) is always enabled. A follow-up can disable it entirely or make it conditional. @@ -213,5 +242,24 @@ backend is stable. non-bypassable enforcement requires iptables TPROXY + an init container with `CAP_NET_ADMIN` — this is Phase 2 and requires its own architecture doc. - **No port-based allowlist.** `AllowPort` from the permission profile is not - yet translated into Envoy policy. Squid honours `AllowPort`; the Envoy backend - currently ignores it. + yet translated into Envoy egress policy, so an allowlisted host is reachable on + any port. Squid honours `AllowPort`; the Envoy backend currently does not, + which is a parity gap. Tracked in #5915. +- **IPv4-only DFP DNS.** The `dynamic_forward_proxy` cluster's DNS lookup is + hardcoded to `V4_ONLY`. A host that resolves only to IPv6 addresses is + unreachable through the Envoy egress path even if it is allowlisted. +- **Wildcard ingress virtual host.** The ingress listener uses a wildcard virtual + host (`domains: ["*"]`) rather than a specific host match. This is safe because + it is bounded by the host-side port binding to `127.0.0.1`: only the local + proxy runner can reach the ingress listener, so the wildcard is not exposed + beyond the local machine. +- **Bootstrap file not cleaned up.** The generated bootstrap file (mode `0600` in + the OS temp directory) is not removed on workload teardown; it is only removed + on a mid-write generation failure. Files accumulate until the OS reclaims the + temp directory. +- **Deny-all on empty profile (deliberate divergence).** A nil or empty + permission profile produces an empty ALLOW policy map, which under Envoy's + default-deny RBAC means deny-all. Squid, by contrast, defaults to allow-all + when no rules are configured. The Envoy behaviour is fail-closed (safer) but is + an intentional divergence from the default backend, so a profile that is + permissive-by-omission under Squid will block all egress under Envoy. diff --git a/docs/arch/README.md b/docs/arch/README.md index e590eee6b2..a39a67a97c 100644 --- a/docs/arch/README.md +++ b/docs/arch/README.md @@ -120,6 +120,13 @@ Welcome to the ToolHive architecture documentation. This directory contains comp - Component inventory and per-client dropped-component warnings - Name/repo consistency check, extraction safety, TOML mutation under file lock +15. **[Envoy Network Proxy](15-envoy-network-proxy.md)** + - Experimental Envoy backend (`TOOLHIVE_NETWORK_PROXY=envoy`) replacing two Squid containers with one + - Egress forward proxy and ingress reverse proxy as separate listeners in a single process + - L7 `:authority`-based RBAC allow/deny and `dstdomain` parity (incl. Docker-gateway blocking) + - Dynamic forward proxy with per-request DNS and native HTTPS CONNECT tunnelling + - Squid-vs-Envoy comparison and known limitations (`AllowPort` gap, V4_ONLY DNS, deny-all on empty profile) + ### Existing Documentation For middleware architecture, see: **[docs/middleware.md](../middleware.md)** From 78d5773c23a7602304e41225b00588ba65c32ab5 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:11:44 +0100 Subject: [PATCH 8/9] Validate Envoy config inline to fix unit CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The real-Envoy validation test bind-mounted the 0600 bootstrap file, which envoy-distroless (nonroot) cannot read on native-Linux CI runners — it passed only under Docker Desktop's permissive file sharing. Pass the config inline via --config-yaml (JSON is valid YAML) instead, removing the file and bind mount entirely so the test behaves identically everywhere docker is available. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/container/docker/envoy_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/container/docker/envoy_test.go b/pkg/container/docker/envoy_test.go index 76150eb7b4..80dcbe85db 100644 --- a/pkg/container/docker/envoy_test.go +++ b/pkg/container/docker/envoy_test.go @@ -716,18 +716,20 @@ func TestEnvoyBootstrap_ValidatesAgainstRealEnvoy(t *testing.T) { Admin: &envoyAdmin{Address: envoyAddress{SocketAddress: envoySocketAddress{Address: "127.0.0.1", PortValue: 9901}}}, StaticResources: envoyStaticResources{Listeners: listeners, Clusters: clusters}, } - path, err := writeEnvoyBootstrap(b) + cfg, err := json.Marshal(b) require.NoError(t, err) - t.Cleanup(func() { _ = os.Remove(path) }) + // Pass the config inline via --config-yaml (JSON is valid YAML) rather + // than bind-mounting a file: the bootstrap is written 0600 and envoy + // runs as nonroot, so a bind-mounted file is unreadable to the + // container on native-Linux CI. Inline config sidesteps that entirely. ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) defer cancel() - //nolint:gosec // G204: fixed args; path is a test-controlled temp file + //nolint:gosec // G204: fixed args; cfg is test-generated JSON out, err := exec.CommandContext(ctx, "docker", "run", "--rm", - "-v", path+":/etc/envoy/envoy.json:ro", - defaultEnvoyImage, "--mode", "validate", "-c", "/etc/envoy/envoy.json").CombinedOutput() + defaultEnvoyImage, "--mode", "validate", "--config-yaml", string(cfg)).CombinedOutput() require.NoError(t, err, "envoy rejected the generated config:\n%s", out) - assert.Contains(t, string(out), "configuration '/etc/envoy/envoy.json' OK") + assert.Contains(t, string(out), "configuration '' OK") }) } } From 1fa4fbb5ce7ad32ca239fef2c5a8cda8850ce87e Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 22 Jul 2026 18:15:15 +0100 Subject: [PATCH 9/9] Address review: disable stream timeouts, close trailing-dot bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes three issues from the panel review of the Envoy backend: - Streams: the ingress route inherited Envoy's 15s default RouteAction timeout — a hard total-response cap that truncated SSE / streamable-http MCP streams. Set route timeout "0s" (ingress + egress plain-HTTP) and HCM stream_idle_timeout "0s" so long-lived and sparse streams aren't reaped. - Trailing-dot bypass: the anchored gateway regex missed valid FQDNs like "host.docker.internal." / "172.17.0.1.", which resolve identically and let a workload reach the gateway under InsecureAllowAll. hostMatchRegex now tolerates an optional trailing dot. - Temp-file leak: SetupEgress removes the bootstrap file on any failure after it is written (success keeps it for the bind mount); corrected the misleading writeEnvoyBootstrap doc. Also documents (per review) that the gateway deny matches the requested name, not the DFP-resolved IP — a known weakening vs Squid's resolved-IP dst deny, addressed properly by Phase 2 (#5905) — and softens the comparison-table row. Verified against real Envoy v1.32.3 (--mode validate). Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/arch/15-envoy-network-proxy.md | 11 ++++- pkg/container/docker/envoy.go | 67 +++++++++++++++++++++-------- pkg/container/docker/envoy_test.go | 67 +++++++++++++++++++++++++++-- 3 files changed, 123 insertions(+), 22 deletions(-) diff --git a/docs/arch/15-envoy-network-proxy.md b/docs/arch/15-envoy-network-proxy.md index 390fb2ed51..4e93319d18 100644 --- a/docs/arch/15-envoy-network-proxy.md +++ b/docs/arch/15-envoy-network-proxy.md @@ -216,7 +216,7 @@ backend is stable. | HTTPS CONNECT tunnelling | ✓ | ✓ | | TLS inspection | ✗ | ✗ | | L7 hostname deny | ✓ (`dstdomain`) | ✓ (`:authority` header match) | -| Destination IP deny | ✓ (`dst` ACL) | ✓ (IP literal matched in `:authority`) | +| Destination IP deny | ✓ (`dst` ACL, resolved IP) | ~ (IP literal in `:authority` only; resolved IP not enforced — see Known Limitations) | | Wildcard host allowlist | ✓ (`.example.com` dot-prefix) | ✓ (same syntax, regex match incl. apex + port) | | Per-request DNS resolution | Via Squid resolver | Via DFP cluster | | Access logs | Per-container, text format | Unified stdout, structured | @@ -245,6 +245,15 @@ backend is stable. yet translated into Envoy egress policy, so an allowlisted host is reachable on any port. Squid honours `AllowPort`; the Envoy backend currently does not, which is a parity gap. Tracked in #5915. +- **Gateway deny matches the requested name, not the resolved IP.** The deny + filter matches the `:authority` string (the gateway IP literal and the two + Docker-internal hostnames). Because an HTTP RBAC filter cannot see the address + the `dynamic_forward_proxy` cluster ultimately resolves and dials, a hostname + that resolves to the gateway IP — via DNS rebinding, an attacker-controlled + record, or an allowlisted host repointed at the gateway — is not caught, whereas + Squid's `dst` ACL denies on the resolved IP. Enforcing on the resolved address + requires L3/L4 interception (Phase 2, #5905); until then this is a known + weakening of the gateway block versus the Squid backend. - **IPv4-only DFP DNS.** The `dynamic_forward_proxy` cluster's DNS lookup is hardcoded to `V4_ONLY`. A host that resolves only to IPv6 addresses is unreachable through the Envoy egress path even if it is allowlisted. diff --git a/pkg/container/docker/envoy.go b/pkg/container/docker/envoy.go index 2596228dd0..77676ec269 100644 --- a/pkg/container/docker/envoy.go +++ b/pkg/container/docker/envoy.go @@ -113,12 +113,15 @@ type envoyNetworkFilter struct { // envoyHCM is the typed config for // envoy.filters.network.http_connection_manager. type envoyHCM struct { - Type string `json:"@type"` - StatPrefix string `json:"stat_prefix"` - AccessLog []envoyAccessLog `json:"access_log,omitempty"` - UpgradeConfigs []envoyUpgradeConfig `json:"upgrade_configs,omitempty"` - HTTPFilters []envoyHTTPFilter `json:"http_filters"` - RouteConfig *envoyRouteConfig `json:"route_config,omitempty"` + Type string `json:"@type"` + StatPrefix string `json:"stat_prefix"` + AccessLog []envoyAccessLog `json:"access_log,omitempty"` + // StreamIdleTimeout caps how long a stream may be idle. Envoy defaults it to + // 5m, which would reap a sparse-but-open SSE stream; "0s" disables it. + StreamIdleTimeout string `json:"stream_idle_timeout,omitempty"` + UpgradeConfigs []envoyUpgradeConfig `json:"upgrade_configs,omitempty"` + HTTPFilters []envoyHTTPFilter `json:"http_filters"` + RouteConfig *envoyRouteConfig `json:"route_config,omitempty"` } // envoyAccessLog configures request access logging for an HCM. @@ -268,7 +271,11 @@ type envoyConnectMatcher struct{} // envoyRouteAction forwards matched requests to an upstream cluster. type envoyRouteAction struct { - Cluster string `json:"cluster"` + Cluster string `json:"cluster"` + // Timeout is the route's total request→response cap. Envoy defaults it to + // 15s, which truncates long-lived MCP streams (SSE, streamable-http); set + // "0s" to disable it for proxied traffic. + Timeout string `json:"timeout,omitempty"` UpgradeConfigs []envoyUpgradeConfig `json:"upgrade_configs,omitempty"` } @@ -336,11 +343,12 @@ func buildEgressListener(spec proxySpec) envoyListener { httpFilters := buildEgressHTTPFilters(spec) hcm := &envoyHCM{ - Type: typeHCM, - StatPrefix: "egress_http", - AccessLog: stdoutAccessLog(), - UpgradeConfigs: []envoyUpgradeConfig{{UpgradeType: "CONNECT"}}, - HTTPFilters: httpFilters, + Type: typeHCM, + StatPrefix: "egress_http", + AccessLog: stdoutAccessLog(), + StreamIdleTimeout: "0s", // don't reap sparse long-lived outbound streams + UpgradeConfigs: []envoyUpgradeConfig{{UpgradeType: "CONNECT"}}, + HTTPFilters: httpFilters, RouteConfig: &envoyRouteConfig{ VirtualHosts: []envoyVirtualHost{ { @@ -358,10 +366,13 @@ func buildEgressListener(spec proxySpec) envoyListener { }, }, }, - // Prefix match handles plain HTTP requests. + // Prefix match handles plain HTTP requests. Timeout "0s" + // disables Envoy's 15s default so long-lived plain-HTTP + // outbound streams aren't truncated (CONNECT tunnels are + // unaffected by the default, but disabling is harmless there). { Match: envoyRouteMatch{Prefix: "/"}, - Route: &envoyRouteAction{Cluster: dfpClusterName}, + Route: &envoyRouteAction{Cluster: dfpClusterName, Timeout: "0s"}, }, }, }, @@ -568,8 +579,10 @@ func hostMatchRegex(host string) string { // Optional "sub." prefix at any depth, plus the apex itself. pattern = `(.*\.)?` + pattern } - // (?i) case-insensitive (hostnames are); (:[0-9]+)? optional port. - return `(?i)` + pattern + `(:[0-9]+)?` + // (?i) case-insensitive (hostnames are); \.? tolerates a trailing-dot FQDN + // ("host.docker.internal." resolves identically and would otherwise bypass a + // deny); (:[0-9]+)? optional port. + return `(?i)` + pattern + `\.?(:[0-9]+)?` } // buildEgressCluster returns the dynamic_forward_proxy cluster required by the @@ -608,6 +621,8 @@ func buildIngressListener(spec proxySpec, hostPort int) envoyListener { Type: typeHCM, StatPrefix: "ingress_http", AccessLog: stdoutAccessLog(), + // Disable the idle timer so a sparse-but-open SSE stream isn't reaped. + StreamIdleTimeout: "0s", HTTPFilters: []envoyHTTPFilter{ { Name: "envoy.filters.http.router", @@ -622,7 +637,10 @@ func buildIngressListener(spec proxySpec, hostPort int) envoyListener { Routes: []envoyRoute{ { Match: envoyRouteMatch{Prefix: "/"}, - Route: &envoyRouteAction{Cluster: ingressClusterName}, + // Timeout "0s" disables Envoy's 15s default total-response + // cap, which would otherwise truncate SSE / streamable-http + // MCP streams. This is the primary transport path. + Route: &envoyRouteAction{Cluster: ingressClusterName, Timeout: "0s"}, }, }, }, @@ -692,7 +710,10 @@ func buildIngressCluster(spec proxySpec) envoyCluster { } // writeEnvoyBootstrap marshals b to JSON and writes it to a temporary file at -// mode 0600. Returns the file path. The caller is responsible for cleanup. +// mode 0600, returning the path. On success the file must outlive this call — +// the Envoy container bind-mounts it read-only — so it is not removed here; the +// caller removes it if setup fails afterward. On any error during writing, the +// partially-created file is removed before returning. func writeEnvoyBootstrap(b envoyBootstrap) (string, error) { data, err := json.Marshal(b) if err != nil { @@ -776,6 +797,15 @@ func (e *envoyProxy) SetupEgress(ctx context.Context, spec proxySpec) (egressRes if err != nil { return egressResult{}, err // already wrapped with context } + // On success the file must persist for the container's read-only bind mount, + // so it can't be unconditionally deferred away. Remove it only if setup fails + // past this point, so a failed deploy doesn't orphan a bootstrap in TempDir. + success := false + defer func() { + if !success { + _ = os.Remove(configPath) + } + }() envoyImage := getEnvoyImage() //nolint:gosec // G706: envoy image name from config @@ -845,6 +875,7 @@ func (e *envoyProxy) SetupEgress(ctx context.Context, spec proxySpec) (egressRes return egressResult{}, fmt.Errorf("failed to create envoy container: %w", err) } + success = true // keep the bootstrap file; the container bind-mounts it return egressResult{ EnvVars: addEgressEnvVars(nil, egressContainerName), ingressPort: ingressPort, diff --git a/pkg/container/docker/envoy_test.go b/pkg/container/docker/envoy_test.go index 80dcbe85db..e281c39e66 100644 --- a/pkg/container/docker/envoy_test.go +++ b/pkg/container/docker/envoy_test.go @@ -267,6 +267,10 @@ func TestBuildEgressListener_AllowlistAndDefaultDeny(t *testing.T) { "deny must match gateway.docker.internal") assert.True(t, authorityMatched(pats, "HOST.DOCKER.INTERNAL"), "deny must be case-insensitive (uppercase must not bypass)") + assert.True(t, authorityMatched(pats, dockerGatewayHostname+"."), + "deny must match a trailing-dot FQDN (host.docker.internal.)") + assert.True(t, authorityMatched(pats, dockerDefaultBridgeGatewayIP+"."), + "deny must match a trailing-dot gateway IP (172.17.0.1.)") assert.False(t, authorityMatched(pats, dockerGatewayHostname+".attacker.com"), "deny must be anchored (lookalike suffix must not match)") @@ -296,9 +300,11 @@ func TestHostMatchRegex(t *testing.T) { noMatches []string }{ { - name: "exact host, no subdomains", - host: "example.com", - matches: []string{"example.com", "example.com:443", "EXAMPLE.COM"}, + name: "exact host, no subdomains", + host: "example.com", + // A trailing-dot FQDN resolves identically and must match, or it becomes + // a bypass on the deny path. + matches: []string{"example.com", "example.com:443", "EXAMPLE.COM", "example.com.", "example.com.:443"}, noMatches: []string{"www.example.com", "notexample.com", "example.com.attacker.com"}, }, { @@ -793,3 +799,58 @@ func TestEnvoyProxy_SetupOrchestration(t *testing.T) { }) } } + +// TestListenerTimeoutsDisabledForStreaming guards against Envoy's 15s default +// RouteAction.timeout (a hard total-response cap) and 5m stream_idle_timeout +// truncating long-lived MCP streams (SSE / streamable-http). Both the ingress +// reverse-proxy route and the egress plain-HTTP route must disable the route +// timeout, and both HCMs must disable the idle timeout. +func TestListenerTimeoutsDisabledForStreaming(t *testing.T) { + t.Parallel() + + spec := proxySpec{ + WorkloadName: "app", + TransportType: "streamable-http", + UpstreamPort: 8080, + GatewayIP: dockerDefaultBridgeGatewayIP, + } + + hcmOf := func(l envoyListener) *envoyHCM { + hcm, ok := l.FilterChains[0].Filters[0].TypedConfig.(*envoyHCM) + require.True(t, ok, "first network filter must be the HCM") + return hcm + } + // routeTimeouts returns the Timeout of every route action in an HCM. + routeTimeouts := func(hcm *envoyHCM) []string { + var out []string + for _, vh := range hcm.RouteConfig.VirtualHosts { + for _, r := range vh.Routes { + if r.Route != nil { + out = append(out, r.Route.Timeout) + } + } + } + return out + } + + ingress := hcmOf(buildIngressListener(spec, 18080)) + assert.Equal(t, "0s", ingress.StreamIdleTimeout, "ingress HCM must disable the idle timeout") + for _, tmo := range routeTimeouts(ingress) { + assert.Equal(t, "0s", tmo, "ingress route must disable the 15s total-response cap") + } + + egress := hcmOf(buildEgressListener(spec)) + assert.Equal(t, "0s", egress.StreamIdleTimeout, "egress HCM must disable the idle timeout") + // The plain-HTTP egress route must disable the cap; the CONNECT route is + // unaffected by the default (no end-of-stream while tunneling) so it need not. + var sawPlainHTTP bool + for _, vh := range egress.RouteConfig.VirtualHosts { + for _, r := range vh.Routes { + if r.Match.Prefix == "/" && r.Route != nil { + sawPlainHTTP = true + assert.Equal(t, "0s", r.Route.Timeout, "egress plain-HTTP route must disable the 15s cap") + } + } + } + assert.True(t, sawPlainHTTP, "egress must have a plain-HTTP route") +}