Add Envoy network-proxy backend behind TOOLHIVE_NETWORK_PROXY=envoy - #5907
Conversation
e9c2cac to
f26a6d1
Compare
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
f26a6d1 to
3e00858
Compare
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) <noreply@anthropic.com>
JAORMX
left a comment
There was a problem hiding this comment.
Panel review — Envoy network-proxy backend
Reviewed across four lenses (security, Go correctness, architecture/parity, MCP transport). This is high-quality work — clean typed-struct → protobuf-JSON generation, exhaustive table tests, a real-Envoy --mode validate test, honest self-documented limitations, and container hardening that's actually better than Squid (CapDrop:[ALL], 0600 bootstrap). The seam wiring, network attachment, teardown, and hostPort flow are all at parity with Squid, and the STRICT_DNS-before-MCP design is correctly immune to the negative-DNS-cache problem that forces Squid to defer ingress.
I'm leaving this as a comment rather than a formal block, but there are three issues I'd resolve before this ships, plus a coverage gap that explains why green CI didn't catch them. Inline comments have detail + suggested fixes.
🔴 Would-block:
- SSE / streamable-http streams die at 15s — the ingress route sets no timeout, so it inherits Envoy's 15s default
RouteAction.timeout(a hard total-response cap, not idle-based). Breaks the two primary networked MCP transports. Regression vs Squid. → set routetimeout: "0s"+ HCMstream_idle_timeout: 0. - Gateway deny enforced only on the
:authoritystring, not the resolved IP — any hostname resolving to172.17.0.1(DNS rebinding / attacker record / repointed allowlist host) bypasses it; Squid'sdstACL denies on resolved IP. Silently weakens the core--isolate-networkguarantee. - Trailing-dot bypass —
host.docker.internal./172.17.0.1.don't match the anchored deny regex but resolve identically (reproduced). One-line regex fix.
🟡 Medium:
4. AllowPort silently dropped (allowlisted host reachable on any port) — already documented + tracked in #5915.
5. No E2E lane sets TOOLHIVE_NETWORK_PROXY=envoy, so the whole backend is runtime-untested in CI — this is why #1 slipped through green checks.
🟢 Minor / confirmed-sound: deny-all-on-empty-profile is an intentional, fail-closed divergence (safe, but a migration gotcha worth calling out); bootstrap temp-file leak + reboot bind-mount exposure are parity with Squid and fail-closed; listeners binding 0.0.0.0 on the bridge is parity. The empty-Policies=deny-all invariant (no omitempty), RBAC filter ordering, allowlist regex anchoring, and the PullImage→ImageExists fallback (an improvement over Squid's bug) are all correct.
Nice work overall — the design is sound and the divergences are mostly conscious. Fixing #1–#3 gets it there.
Generated with Claude Code
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) <noreply@anthropic.com>
JAORMX
left a comment
There was a problem hiding this comment.
Approving — review feedback addressed and verified
Re-reviewed at 1fa4fbb5. All three blockers from the panel review are resolved and verified:
- SSE / streamable-http 15s cap —
envoyRouteAction.TimeoutandenvoyHCM.StreamIdleTimeoutfields added and set to"0s"on both the ingress route/HCM and the egress route/HCM (nice — the egress plain-HTTP path was covered too). Long-lived MCP streams will no longer be truncated. - Trailing-dot gateway bypass —
hostMatchRegexnow appends\.?before the optional port group. Verified:host.docker.internal.,172.17.0.1., andHOST.DOCKER.INTERNAL.are now denied, whileexample.com.attacker.comandexample.com..remain correctly rejected (no over-broadening). - Bootstrap temp-file leak —
SetupEgressnow uses asuccessflag with a deferredos.Remove, so the file is cleaned up on every post-write failure path and only persists on success for the bind mount. - Doc accuracy — the comparison-table row is softened to
~ (IP literal in :authority only; resolved IP not enforced)and the resolved-IP limitation is documented.
The two consciously-deferred gaps are acceptable for an experimental, opt-in backend (Squid remains the untouched default, and the Internal:true network is the real fail-closed backstop):
- Resolved-IP gateway deny — genuinely not fixable at L7 for a forward proxy (RBAC can't see the DFP-resolved address); documented as a known limitation.
AllowPortparity — tracked in #5915.- E2E coverage — the
{squid, envoy}parametrization is in flight in #5918. Worth landing that reasonably soon so the backend isn't runtime-untested going forward.
CI is green across all 104 checks on this head. Thanks for the fast, thorough turnaround.
Generated with Claude Code
Summary
Adds an Envoy-based network-proxy backend that consolidates the egress (forward proxy) and ingress (reverse proxy) into a single container, cutting the per-workload auxiliary count from 3 → 2. Opt-in via
TOOLHIVE_NETWORK_PROXY=envoy; Squid stays the default and is untouched.Closes #5902. Part of #5900.
Medium level
envoyProxybackend implementing thenetworkProxy.SetupProxiesseam. Selected whenTOOLHIVE_NETWORK_PROXY=envoy;newNetworkProxygains theenvoycase and a compile-time assertion.0600temp file and bind-mounted read-only. Everytyped_configcarries the required@typeURL.:3128): HTTP connection manager with CONNECT upgrade + a CONNECT route matcher (HTTPS tunnelling), adynamic_forward_proxycluster, and two RBAC filters — a gateway DENY (L3destination_ipCIDR + L7:authority) evaluated before an allowlist ALLOW.0.0.0.0:<port>inside the container (Docker port-forwarding targets the bridge IP, not loopback), routing to a STRICT_DNS upstream cluster targeting the MCP container;Inbound.AllowHostrestricts the virtual-host domains.dstdomain: a leading dot (or*.) matches apex + subdomains, a bare host matches exactly; the generated regex is anchored, case-insensitive, and tolerates the:portthat HTTPS CONNECT authorities carry.127.0.0.1; stdout access logging on both listeners.docs/arch/14-envoy-network-proxy.md(design, Squid-vs-Envoy comparison, known limitations).Low level
pkg/container/docker/envoy.gobuildEgressListener/buildIngressListener, cluster builders,writeEnvoyBootstrap,getEnvoyImage,hostMatchRegex,envoyProxy.SetupProxiespkg/container/docker/networkproxy.goenvoycase tonewNetworkProxy+ compile-time assertionpkg/container/docker/envoy_test.go@typeURLspkg/container/docker/networkproxy_test.goenvoyfactory casepkg/container/docker/client_stop_test.godocs/arch/14-envoy-network-proxy.mdType of change
Test plan
task buildpassestask testpasses forpkg/container/docker/...with-racegolangci-lintclean on the changed package--mode validate) with allowlist + gateway deny + ingressTestBuildEgressListener_AllowlistAndDefaultDeny,TestBuildEgressListener_EmptyAllowHostDenyAll,TestHostMatchRegex— allowlist, default-deny,InsecureAllowAll, SquiddstdomainparityTestBuildIngressListener_PortAndHostGating,TestBuildIngressCluster_UpstreamAddress— ingress reverse proxy +Inbound.AllowHostTestStopProxyContainer_HandlesEnvoyAndSquidTopologies— cleanup handles both 2- and 3-container workloadsTestNewNetworkProxy— Squid default,envoyselectable, unknown value errorsTOOLHIVE_NETWORK_PROXY=envoy thv run io.github.stacklok/fetch:docker psshows-egress(Envoy) +-dnsonly (no-ingress); allowed hosts reachable over HTTP+HTTPS; docker gateway (IP +host.docker.internal+gateway.docker.internal) blockedSpecial notes for reviewers
Size:
envoy.gois ~600 production lines, over the soft 400-line limit, but it is one cohesive addition — roughly half is JSON-tagged struct definitions for the Envoy bootstrap, and the config generator is exhaustively table-tested. Splitting the config model from the ~150-lineSetupProxieswiring is possible but leaves the first half as inert dead code. Flagging for a conscious call.Config generation: hand-rolled typed structs → protobuf-JSON. Unit tests assert the Go-level shape and
@typeURLs; the--mode validaterun confirms real Envoy accepts the full config. Known limitations (tag-pinned image, always-on admin, health-check log noise) are tracked as hardening follow-ups in #5903.Generated with Claude Code