Skip to content

Translate AllowPort into Envoy egress RBAC policy - #5927

Merged
ChrisJBurns merged 6 commits into
mainfrom
cburns/envoy-allow-port
Jul 23, 2026
Merged

Translate AllowPort into Envoy egress RBAC policy#5927
ChrisJBurns merged 6 commits into
mainfrom
cburns/envoy-allow-port

Conversation

@ChrisJBurns

@ChrisJBurns ChrisJBurns commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Translates AllowPort into the Envoy egress RBAC policy, closing the parity gap with Squid's allowed_ports ACL. Also addresses all review findings from #5926 and pins the Ginkgo CLI version to eliminate a test environment mismatch.

Closes #5915. Part of #5900.

AllowPort enforcement

AllowPort entries are encoded into a single combined host+port safe_regex per policy. The key insight: plain-HTTP requests omit the default port 80 from :authority (example.com, not example.com:80). When port 80 is in AllowPort the port suffix is made optional, so bare hostnames (plain HTTP) are permitted alongside explicit :80 — matching Squid's allowed_ports behaviour.

Profile Result
AllowHost + AllowPort:[80,443] host AND (port 80 or 443 or bare hostname)
AllowHost + AllowPort:[443] host AND explicit :443 only (bare hostname denied)
AllowPort:[80] only any host, port 80 or bare hostname
AllowHost only any port (unchanged)
InsecureAllowAll everything (ignores AllowPort)
Review findings from #5926 addressed
  • F2 networkproxy.go docstrings: correctly describe that Envoy creates no container in SetupEgress; both listeners land in one container created in SetupIngress.
  • F3/F5 Documentation: buildAllowlistPolicies comment and arch doc comparison table updated; plain-HTTP divergence (port 443-only profiles) documented in Known Limitations.
  • F6 listenerAllowsAuthority extracted as a package-level helper replacing the ad-hoc closure, making structural test assertions independent of the reimplementation.
  • F7 New table cases: multi-port OR shape, port-less authority (now asserts Squid parity, not divergence), AllowPort:[443]-only correctly denies bare hostnames.
  • F8 Test asserting deny-before-allow ordering holds under AllowPort-only policy.
  • F9 Comment at container creation explaining why the Envoy container retains the -egress suffix.
  • CI Ginkgo CLI pinned to v2.28.1 (matching go.mod) to eliminate the version mismatch that caused "flag parsing errors" and intermittent test-isolation failures.

Type of change

  • Bug fix / parity fix

Test plan

  • go build ./pkg/container/docker/... passes
  • golangci-lint clean
  • TestBuildAllowlistPolicies_AllowPort — 4 table cases including bare-hostname parity
  • TestBuildAllowlistPolicies_AllowPort_Structure — asserts combined regex shape
  • TestBuildAllowlistPolicies_AdditionalCases — multi-port OR, port-less authority parity, gateway-deny ordering
  • TestBuildIngressCluster_UpstreamAddress — asserts V4_ONLY
  • go test -c ./test/e2e/ compiles
  • CI network-isolation shard — Envoy e2e tests pass

Generated with Claude Code

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 22, 2026
@ChrisJBurns
ChrisJBurns force-pushed the cburns/envoy-allow-port branch 2 times, most recently from 346d75d to 1d45cc2 Compare July 22, 2026 21:30
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 22, 2026
@codecov

codecov Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.80%. Comparing base (4529806) to head (0d5d561).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5927      +/-   ##
==========================================
+ Coverage   71.77%   71.80%   +0.02%     
==========================================
  Files         705      705              
  Lines       72157    72192      +35     
==========================================
+ Hits        51794    51834      +40     
+ Misses      16662    16659       -3     
+ Partials     3701     3699       -2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ChrisJBurns added a commit that referenced this pull request Jul 22, 2026
@ChrisJBurns @claude
Fix ingress virtual host blocking the proxy runner health check
e46bd56 
The transparent proxy rewrites the Host header to include the port
("127.0.0.1:22354"), but ingressDomains was returning bare hostnames from
Inbound.AllowHost (e.g. "127.0.0.1") without ports. Envoy's virtual host
matching failed, so every initialize request from waitForInitializeSuccess
was rejected — the server never reached running state.

Fix: always return wildcard for the ingress virtual host domain. The
Inbound.AllowHost restriction is already enforced by the 127.0.0.1
host-side port binding, which limits the ingress to local connections only.
The virtual host domain list adds no security value here and breaks the
health check.

Also removes the AllowPort e2e test from this branch (it belongs on #5927).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Base automatically changed from cburns/envoy-readiness-fix to main July 23, 2026 13:19
ChrisJBurns added a commit that referenced this pull request Jul 23, 2026
@ChrisJBurns @claude
Fix Envoy isolated-server readiness and enable e2e tests (#5926)
54f62dd 
* Fix Envoy isolated-server readiness on Linux Docker Engine (#5922)

Creating the Envoy container in SetupEgress (before createMcpContainer)
caused the STRICT_DNS ingress cluster to probe the MCP hostname before
the container existed. On Linux Docker Engine this resulted in a cached
negative DNS response that prevented the ingress from ever connecting,
leaving the server stuck in "starting" past the readiness window.

Move all Envoy container creation to SetupIngress (after the MCP
container exists) so the STRICT_DNS cluster resolves the upstream
hostname on its first probe. SetupEgress now only computes and returns
the proxy env vars — the container name is deterministic, so no state
needs to be threaded through egressResult. Remove the now-unused
egressResult.ingressPort field and update the orchestration test.

Also remove the BeforeEach(Skip(...)) guard from the Envoy e2e suite so
the tests run now that the readiness issue is resolved.

Closes #5922.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Pre-pull Envoy image in CI and raise readiness timeout to 180s

The Envoy e2e tests were timing out on Linux CI because the Envoy
distroless image (~47MB) was being pulled cold inside the 120s readiness
window. Pre-pull it in the proxy shard's image setup step alongside the
other proxy-suite images. Raise the WaitForMCPServer timeout to 180s as
a safety margin for any remaining startup variability.

Closes #5922.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Add CI diagnostics for Envoy container logs on failure

* Inject dnsmasq DNS server into Envoy container on Linux Docker Engine

The Envoy STRICT_DNS ingress cluster resolves the MCP container's hostname
before forwarding inbound traffic. On Linux Docker Engine, the default
bridge DNS (127.0.0.11) cannot resolve names from custom internal networks,
so resolution fails and the ingress never connects to the MCP upstream.

Fix: pass the dnsmasq container's IP (already used by the MCP container
as additionalDNS) into a new DNSServers field on proxySpec, and set it as
the Envoy container's DNS server via HostConfig.DNS. Dnsmasq serves all
names on the internal Docker network, so the STRICT_DNS cluster can now
resolve the MCP container hostname.

Closes #5922.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Fix bootstrap file permissions for Envoy on Linux Docker Engine

The writeEnvoyBootstrap function wrote the config file at 0o600, which
prevented the Envoy distroless container (UID 101) from reading its own
config when bind-mounted from a host file owned by a different UID. On
Linux Docker Engine, strict POSIX permissions apply — the container user
cannot read a 0o600 file owned by the runner user. macOS Docker Desktop's
VirtioFS layer relaxed these permissions, masking the bug.

Change the mode to 0o644. The bootstrap contains only network topology
(hostnames, ports, RBAC rules) — no credentials or secrets — so
world-readable is safe and necessary.

Also add dns_lookup_family: V4_ONLY to the STRICT_DNS ingress cluster to
prevent slow AAAA lookup timeouts when IPv6 is unavailable.

Closes #5922.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Capture Envoy container logs before cleanup in e2e tests

* Split network-isolation tests into dedicated e2e shard

The isolation tests (Squid + Envoy) are slow (60-180s per server) and
unrelated to the proxy transport tests (stdio, SSE, OAuth, tunnels). Running
them together in the proxy shard meant debugging isolation failures required
waiting for all transport tests to complete first.

New shard: label_filter=isolation, test_timeout=25m. The proxy shard now
excludes isolation tests via "proxy && !isolation". Envoy image pre-pull
moves to the network-isolation shard; the time server pre-pull stays in proxy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Fix ingress virtual host blocking the proxy runner health check

The transparent proxy rewrites the Host header to include the port
("127.0.0.1:22354"), but ingressDomains was returning bare hostnames from
Inbound.AllowHost (e.g. "127.0.0.1") without ports. Envoy's virtual host
matching failed, so every initialize request from waitForInitializeSuccess
was rejected — the server never reached running state.

Fix: always return wildcard for the ingress virtual host domain. The
Inbound.AllowHost restriction is already enforced by the 127.0.0.1
host-side port binding, which limits the ingress to local connections only.
The virtual host domain list adds no security value here and breaks the
health check.

Also removes the AllowPort e2e test from this branch (it belongs on #5927).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Increase slow-stream test timeout to 60s for CI headroom

The 30s fetchThrough context was too tight for the 16s upstream sleep plus
MCP client round-trip overhead under CI infrastructure load (38s observed).
Increase to 60s so the test proves the Envoy route timeout is disabled
without racing against CI slowness.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Remove DNS injection, log capture step; silence skipped specs

- DNS injection (dnsmasq as Envoy DNS server) was belt-and-suspenders;
  Docker's embedded DNS at 127.0.0.11 resolves names for all networks a
  container is attached to, which covers the internal network where the
  MCP container lives. Removing it simplifies the code without risk.

- Remove the 'Capture Envoy container logs on failure' CI step. Logs are
  now captured inside the test via DeferCleanup before teardown.

- Add --silence-skips to the Ginkgo command. Without it, --label-filter
  runs all 475 specs but marks ~466 as skipped, flooding CI output. With
  --silence-skips (Ginkgo v2), only matching specs appear in the log.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Remove debug docker log capture from Envoy e2e test

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChrisJBurns and others added 3 commits July 23, 2026 14:20
@ChrisJBurns @claude
Translate AllowPort into Envoy egress RBAC policy (#5915)
4c37001 
Squid enforces AllowPort via the allowed_ports ACL, AND-d with AllowHost.
The Envoy backend previously ignored AllowPort, leaving allowlisted hosts
reachable on any port — a parity regression.

Translate AllowPort into :authority suffix matchers (":80", ":443", …) in
the RBAC ALLOW filter, mirroring Squid's AND semantics:
  - AllowHost + AllowPort  → host AND port must match in the same policy
  - AllowPort only         → any host on listed ports
  - AllowHost only         → any port (unchanged)
  - InsecureAllowAll       → ignores AllowPort (same as Squid)

Uses :authority suffix matching so plain HTTP ("host:port") and HTTPS
CONNECT ("host:port") are both covered. Remove the Known Limitation entry
and add a parity row to the comparison table.

Closes #5915.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ChrisJBurns @claude
Add e2e test for AllowPort enforcement via real Envoy
0bc9195 
Exercises the AllowPort fix end-to-end: starts an Envoy isolated server
with AllowHost:[example.com] + AllowPort:[443], then asserts that
https://example.com succeeds and http://example.com is blocked. This
closes the gap between unit tests (which verify the config shape) and
runtime behaviour through a real Envoy container.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ChrisJBurns @claude
Address review nitpicks from #5926
6880ea2 
- networkproxy.go interface docstring: update SetupEgress/SetupIngress
  comments to reflect the current model where both backends create their
  ingress container in SetupIngress (not in SetupEgress as was the case
  before the negative-DNS-cache fix).

- envoy.go buildIngressListener docstring: replace the stale "Inbound.AllowHost
  restricts the virtual host domain list" comment with the correct explanation:
  the domain is always "*"; inbound host filtering uses the egress RBAC
  :authority matcher, not the ingress vhost list.

- envoy_test.go TestBuildIngressCluster_UpstreamAddress: add an assertion for
  DnsLookupFamily="V4_ONLY" so a silent regression dropping it would be caught.

- envoy_test.go TestEnvoyProxy_SetupOrchestration header: rewrite to describe
  the current behaviour (SetupEgress returns env vars only; SetupIngress
  creates the container).

- network_isolation_envoy_test.go: fix stale "120 seconds" in the WaitForMCPServer
  failure message to match the 180s timeout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ChrisJBurns
ChrisJBurns force-pushed the cburns/envoy-allow-port branch from 1d45cc2 to 6880ea2 Compare July 23, 2026 13:22
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 23, 2026
@ChrisJBurns @claude
Address review findings F2-F10 and fix Ginkgo CLI version mismatch
7e26341 
F2: networkproxy.go docstrings now accurately describe that Envoy creates
  no container in SetupEgress; both listeners land in one container created
  in SetupIngress.
F3/F5: document the plain-HTTP AllowPort divergence from Squid in both the
  arch comparison table and the buildAllowlistPolicies function comment.
F6: extract authorityAllowed closure into a package-level listenerAllowsAuthority
  helper so test assertions don't hide behind a reimplemented matcher.
F7: add TestBuildAllowlistPolicies_AllowPort_Structure (structural shape
  assertion) and TestBuildAllowlistPolicies_AdditionalCases (multi-port OR,
  port-less authority divergence).
F8: assert deny-before-allow ordering holds under AllowPort-only policy.
F9: add comment at the container creation site explaining why the Envoy
  container retains the -egress name suffix.
CI: pin Ginkgo CLI to v2.28.1 (matching go.mod) to eliminate the version
  mismatch warning that could cause unpredictable test behaviour.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 23, 2026
@ChrisJBurns @claude
Fix F1: plain-HTTP bare-hostname allowed when port 80 is in AllowPort
3d4203d 
Plain-HTTP requests omit the default port from :authority ("example.com"
not "example.com:80"). The previous implementation used a separate port
suffix AND-d with the host regex, which failed to match the portless form
and over-blocked legitimate traffic — diverging from Squid's allowed_ports
ACL which derives the port from the URL.

Fix: replace the two-permission AND with a single combined safe_regex per
policy that encodes both the host pattern and the port alternatives. When
port 80 is in AllowPort the port suffix is made optional so bare hostnames
(plain HTTP) are permitted alongside explicit ":80", matching Squid. When
port 80 is NOT listed the port suffix is required. The same logic applies
to the AllowPort-only case via anyHostPortRegex.

Updates tests to reflect the new behaviour (F7b now asserts Squid parity
rather than documenting a known divergence), structural assertion (F6),
and arch doc comparison table.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 23, 2026
@ChrisJBurns @claude
Address re-review findings N1-N4
0d5d561 
N1: Extract portGroupRegex and hostBasePattern helpers to eliminate the
  duplicated port-alternation and host-escaping logic across hostPortMatchRegex,
  anyHostPortRegex, and hostMatchRegex. A future change to port encoding or
  subdomain anchoring now needs to be made in one place.

N2: Replace the weak assert.Contains(re,"?") structural assertion with
  behavioural assertions that compile and evaluate the actual regex — bare
  hostname must match, explicit ports must match, non-listed ports and wrong
  hosts must not. The previous assertion was vacuous because "?" appears in
  non-capturing groups unrelated to the port-optional logic.

N3: Add a TestBuildAllowlistPolicies_AdditionalCases subtest for AllowPort-only
  with port 443 (no port 80), exercising the includes80=false branch of
  anyHostPortRegex: bare hostname denied, explicit :443 allowed, :80 denied.

N4: Add a wildcard AllowHost + AllowPort subtest (.example.com) to exercise
  the subdomain (.*\.)? path in hostPortMatchRegex, guarding against a bug
  in the wildcard+port combination going undetected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 23, 2026
@ChrisJBurns
ChrisJBurns merged commit a7d5e4c into main Jul 23, 2026
48 checks passed
@ChrisJBurns
ChrisJBurns deleted the cburns/envoy-allow-port branch July 23, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JAORMX JAORMX JAORMX approved these changes
@amirejaz amirejaz Awaiting requested review from amirejaz amirejaz is a code owner
@rdimitrov rdimitrov Awaiting requested review from rdimitrov rdimitrov is a code owner
@jhrozek jhrozek Awaiting requested review from jhrozek jhrozek is a code owner
@tgrunnagle tgrunnagle Awaiting requested review from tgrunnagle tgrunnagle is a code owner
@blkt blkt Awaiting requested review from blkt blkt is a code owner
@reyortiz3 reyortiz3 Awaiting requested review from reyortiz3 reyortiz3 is a code owner
@aponcedeleonch aponcedeleonch Awaiting requested review from aponcedeleonch aponcedeleonch is a code owner

Assignees

No one assigned

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Envoy backend: translate AllowPort into egress policy

2 participants

@ChrisJBurns @JAORMX