feat: Add inboundInterception=transparent (drops port stealing) - #511
Conversation
|
Adds Review found that proxy-init's ambient DNAT target was keyed off
Assisted-By: Claude Code |
|
Companion rossoctl/cortex#776 is now merged, so the authbridge listener and proxy-init rules this PR switches on are in place. CI green (Unit, Integration, Lint, Shellcheck, Trivy, action pinning). E2E verified 10/10 on Kind — see rossoctl/rossoctl#2393; the operator-side injection specifically was confirmed live (agent keeps its port with no One sequencing note before this mergesThe default image tags are Loud rather than dangerous, and it cannot happen by accident since the feature is opt-in and defaults to Assisted-By: Claude Code |
…ing)
Adds an opt-in inbound shape for proxy-sidecar / lite that stops stealing the
agent's port. When inboundInterception is "transparent", proxy-init installs a
PREROUTING REDIRECT and AuthBridge recovers each connection's real destination
via SO_ORIGINAL_DST, so the agent keeps the port it already binds.
That removes the three problems with port stealing, all of which the existing
code acknowledges:
- The relocated port (originalPort+1) is declared in the pod spec and directly
reachable, so any pod could reach the agent without JWT validation. The pod
is the granularity Kubernetes NetworkPolicy and ztunnel both enforce at, so
this was the boundary that mattered and it was open.
- Relocation depends on the agent honoring PORT. pod_mutator.go concedes that
agents which hardcode their listen port "won't be affected" — in practice
they collide with AuthBridge on the stolen port and the pod never starts.
- Only Ports[0] of the first container with ports was relocated, so a
second declared port was never proxied.
Default is "reverse-proxy" (port stealing). Transparent costs a privileged
proxy-init container, so it must be chosen deliberately; with the field unset
every existing path is byte-identical.
The switch is the namespace authbridge-runtime-config ConfigMap
(inboundInterception), resolved namespace > cluster default, with a
proxy.allowedInboundInterception allowlist so a platform admin can forbid it (no
NET_ADMIN) or mandate it. Two fallbacks are deliberately biased toward the
unprivileged shape:
- An unrecognized value falls back to reverse-proxy, so a typo cannot silently
grant a NET_ADMIN init container.
- transparent + egressEnforcement=none falls back to reverse-proxy. The two
features share one proxy-init container; without it nothing would REDIRECT
to the inbound listener and inbound would be silently unenforced — worse
than port stealing, which at least validates Service-routed traffic.
Deliberately NOT adding an AgentRuntime spec field. The obvious move was to
mirror spec.egressEnforcement, but that field is dead surface: the pod mutator
has no access to the CR and resolves everything from the namespace ConfigMap, so
nothing reads spec.egressEnforcement (spec.mtlsMode reaches the pod only as a
rollout-triggering annotation, not as mutator input). An enum-validated spec
field that silently does nothing is worse than no field, and plumbing CR ->
mutator properly is a separate change that would have to fix mtlsMode and
egressEnforcement too.
Wiring details:
- The per-agent ConfigMap gets inbound_interception + transparent_inbound_addr
and must NOT get reverse_proxy_addr / reverse_proxy_backend: authbridge's
config validation rejects that combination, so emitting both would
crash-loop the pod.
- proxy-init gets INBOUND_TRANSPARENT_PORT and POD_IP (downward API). POD_IP is
not optional — the init script uses it as the DNAT target for the Istio
ambient inbound path, which arrives through OUTPUT rather than PREROUTING,
and refuses to start without it rather than install PREROUTING-only rules
that wave all mesh traffic through.
- SIDECAR_PORTS_EXCLUDE carries the RESOLVED forward-proxy port, not the
script's 8081 default: findFreePort may have moved it, and an unexempted
forward-proxy port would be swallowed by the inbound REDIRECT.
- The sidecar declares its inbound port as "transparent-in" rather than
"reverse-proxy", so which shape is running is visible in the pod spec.
- Config validation rejects transparentInboundPort == transparentPort. Both are
listeners in one container, so a shared value fails the second bind at pod
start, long after admission succeeded.
Drive-by lint cleanups in touched code: extracted the duplicated
resolution-source strings ("cluster-default", "default-invalid-fallback") into
constants alongside the existing sourceNamespaceConfigMap, and dropped an unused
parameter from the new private sidecar builder.
make test: 21 packages green. No CRD changes (none needed).
Refs: rossoctl/cortex#330
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
proxy-init's ambient DNAT target must match the address family of the traffic. POD_IP is the pod's PRIMARY address (usually v4), so on a dual-stack pod the other family's HBONE delivery passed unvalidated while that family's PREROUTING rules were installed — half-enforcement, which the POD_IP guard refuses to ship in the equivalent case. Adds POD_IPS from the Downward API (status.podIPs) alongside POD_IP. proxy-init falls back to POD_IP when absent, so an older init image still works for its own family. Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Review found that transparent mode could produce a crash-looping pod the user cannot diagnose. usedPorts recorded the inbound port as reserved but never checked whether the agent had already DECLARED it. Under port stealing such a collision was accidentally survivable — Ports[0] got relocated off the conflicting port — and transparent mode deliberately removes that, so an agent declaring 8083 left two processes binding one port in a shared netns: admission succeeds, then the sidecar dies on "address already in use" in a container the user never wrote. Same failure class Validate() already guards for the two sidecar listeners, but applied to the agent, which is the case this mode newly creates. Two related holes closed at once: - The transparent EGRESS port (8082) was never reserved either — on main or here — so an agent declaring 8081 could push findFreePort's forward proxy onto a listener that is always on in proxy-sidecar mode. All sidecar-owned ports (8082, 8083, 9091, 9093, 9094) are now reserved unconditionally, so port assignment is also stable if a namespace later flips mechanism. - Reservation is separated from collision: containerPort is informational, so under reverse-proxy nothing binds 8083 and an agent declaring it is harmless. Only ports the sidecar actually binds in the resolved mechanism trigger the guard. Falls back to port stealing rather than rejecting admission. Rejecting would block pods that declare a colliding port informationally without ever binding it — which works today — and for a first-port collision the relocation genuinely resolves it. The warning names the port, the listener that owns it, and the effective mechanism, since the earlier "resolved inbound interception" line is no longer true at that point. Also from review: - The inboundInterception resolver moved to AFTER mode resolution, so an envoy-sidecar or waypoint namespace carrying the field no longer logs a resolved mechanism that is then silently ignored; it logs that the value is not applicable instead. (The reviewer's stated consequence — that authbridge would refuse — does not hold: the operator writes listener.inbound_interception only in the proxy-sidecar branch, so those modes never see it. The misleading log was the real issue.) - Dropped BuildProxySidecarContainerWithPorts' unused agentBackendPort parameter instead of annotating it with `_ =`, which did nothing the compiler needed. A pre-existing test expected the forward proxy on 8082 when 8081 was taken. That expectation encoded the bug above — 8082 is the transparent egress listener — so it is now 8084, with a comment on why, plus an assertion that the port is none of the sidecar's. New tests cover each sidecar port colliding (all five), a non-colliding port staying transparent, and the forward proxy never landing on a sidecar port in either mechanism. The gap the reviewer noted is real: all ten existing tests used ContainerPort 8000. Rebased onto a12afd0 (was CONFLICTING). make test: 21 packages green. Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
3b66af0 to
eee000e
Compare
All four addressed (
|
cwiklik
left a comment
There was a problem hiding this comment.
Summary
Well-engineered, well-tested, opt-in feature that replaces port-stealing with transparent inbound interception (iptables PREROUTING REDIRECT + SO_ORIGINAL_DST), closing a real JWT-bypass hole — the relocated originalPort+1 was a directly-reachable, unvalidated entrance. Default stays reverse-proxy, so no blast radius on merge. The PR body is exemplary: it enumerates the port-stealing failure modes it fixes and honestly flags what's still unverified.
Verified correct:
- Dual-mode ConfigMap parity — strict
if transparentInbound / elseinlistenerOverrides: transparent emitsinbound_interception+transparent_inbound_addrand never thereverse_proxy_*keys (which authbridge would reject together); reverse-proxy emits them. No path emits both or neither, and the ConfigMap, sidecar container, and proxy-init env all read onetransparentInboundlocal, so the three artifacts can't disagree. - Port reservation — all five sidecar ports (8082, 8083, 9091, 9093, 9094) reserved unconditionally in
usedPorts; a declared agent collision on a port the sidecar actually binds falls back to port-stealing with a named warning. - Config validation (
types.go:213-220) — rejectstransparentInboundPort == transparentPortand out-of-range. - Enum defaults fail-safe — empty/unknown →
reverse-proxy;allowedInbound[0]index is length-guarded; no unguarded map/slice access. - Feature-gated / off by default — complies with the feature-flag policy.
Non-blocking findings:
SIDECAR_PORTS_EXCLUDEomits 8082 (pod_mutator.go:753buildsforwardProxyPort,9091,9093,9094). 8083 is correctly omitted (it's the REDIRECT target), but 8082 — the transparent egress listener you note is "always on in this mode" — is left out while 8081 (forward proxy) is excluded. Likely fine if the init script applies exclusions only to PREROUTING (8082 takes OUTPUT-originated traffic, never inbound), but the asymmetry with 8081 is worth confirming — it lands squarely in the ambient/HBONE path you flagged as unverified.- Re-injection idempotency — the transparent inbound env is added only when a proxy-init container is absent, so on re-admission the sidecar still binds the transparent listener and the ConfigMap still emits
transparent_inbound_addr, but no PREROUTING rules get (re)installed → inbound silently unenforced. Mirrors the existing egress pattern, but the consequence (a bound listener nothing redirects to) is sharper here. - Your self-disclosed follow-ups — ambient/HBONE runtime still reasoned-not-observed (single-istiod cluster, ambient DNAT counter 0),
egressEnforcementwire-or-remove, and the:latestsequencing (crash-loops loudly rather than bypassing on a pre-cortex#776 image) — are all legitimate follow-ups, not merge blockers, given default-off and the confirmed 401-on-real-port result.
Approving. The security posture strictly improves over port-stealing, the closed path is verified live, and the residual items are either opt-in-only or already flagged.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go webhook injector/config, Helm values, dual-mode parity, port/iptables safety, tests
CI: 15 green; E2E Tests pending.
Review (cwiklik) flagged a re-injection divergence. The proxy-init injection is skipped when a container of that name already exists, but the sidecar container and the per-agent ConfigMap are configured unconditionally. So a pod arriving with a hand-authored proxy-init that lacks inbound capture would get authbridge binding :8083 and a ConfigMap saying transparent, while nothing installed the PREROUTING rules — inbound silently unenforced, with no error anywhere. That is the one outcome this mode exists to prevent, so it is now checked rather than left to reasoning about reachability. The check sits in the pre-flight block alongside the port-collision guard, before any artifact is built, so the ConfigMap, sidecar and proxy-init env cannot disagree: an existing proxy-init without INBOUND_TRANSPARENT_PORT falls back to reverse-proxy with a warning naming the effective mechanism and how to fix it. An existing proxy-init that IS armed is consistent and stays transparent. Narrow in practice — the webhook is CREATE-only on pods and a fresh pod comes from an unmutated Deployment template, so this needs a hand-written container — but the failure being silent is what makes it worth a guard rather than a note. Also documents why SIDECAR_PORTS_EXCLUDE omits 8082/8083, which the same review had to guess at. It carries only the ports the init script cannot know: the forward-proxy port (findFreePort may have moved it off 8081) and the health/stats/session constants. The two transparent ports are exempted unconditionally inside emit_inbound_exemptions, on BOTH the PREROUTING and ambient hooks, since the script already has them as TRANSPARENT_PORT / INBOUND_TRANSPARENT_PORT — listing them again would only duplicate RETURN rules. The asymmetry with 8081 is therefore intentional, and cortex#776's harness asserts the 8082 exemption directly. make test: 21 packages green. Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Thanks @cwiklik — both findings checked. One was a non-issue with an undocumented reason; the other was real and is now fixed ( #1
|
Summary
Adds an opt-in inbound shape for
proxy-sidecar/litethat stops stealing the agent's port. Companion to rossoctl/cortex#776 (merged), which implements the listener and iptables rules; this is the switch that turns them on. Until this lands, that code is inert — nothing setsinbound_interceptionorINBOUND_TRANSPARENT_PORT.With
inboundInterception: transparent, proxy-init installs a PREROUTING REDIRECT and AuthBridge recovers each connection's real destination viaSO_ORIGINAL_DST, so the agent keeps the port it already binds.Why
Port stealing has three problems, all of which the existing code acknowledges:
originalPort+1is declared in the pod spec and directly reachable, so any pod can reach the agent without JWT validation. Pods are the granularity NetworkPolicy and ztunnel enforce at, so this was the boundary that mattered.PORT.pod_mutator.goconcedes that agents which hardcode their listen port "won't be affected" — in practice they collide with AuthBridge on the stolen port and the pod never starts. No config can fix that.Ports[0]of the first container with ports is relocated, so a second declared port was never proxied. Undeclared ports aren't even inusedPorts, sofindFreePortcan hand the agent a port it already uses.Transparent interception removes all three: no relocation, no
PORTenv var, no second port to discover, and every port the agent listens on is covered.Default is unchanged
Default stays
reverse-proxy. Transparent costs a privileged proxy-init container, so it is chosen deliberately; with the field unset every existing path is byte-identical. Two fallbacks are deliberately biased toward the unprivileged shape:reverse-proxy, so a typo cannot silently grant aNET_ADMINinit container.transparent+egressEnforcement: nonefalls back toreverse-proxy. The two features share one proxy-init container; without it nothing would REDIRECT to the inbound listener and inbound would be silently unenforced — worse than port stealing, which at least validates Service-routed traffic.proxy.allowedInboundInterceptionlets a platform admin forbid it (["reverse-proxy"]) or mandate it (["transparent"]).Deliberately NOT adding an AgentRuntime spec field
The obvious move was to mirror
spec.egressEnforcement— but that field is dead surface. The pod mutator has no access to the CR and resolves everything from the namespace ConfigMap, so nothing readsspec.egressEnforcement(spec.mtlsModereaches the pod only as a rollout-triggering annotation, not as mutator input). An enum-validated spec field that silently does nothing is worse than no field, and plumbing CR → mutator properly is a separate change that would have to fixmtlsModeandegressEnforcementtoo.Worth deciding separately:
spec.egressEnforcementshould probably be either wired up or removed.Details
inbound_interception+transparent_inbound_addrand must not getreverse_proxy_addr/reverse_proxy_backend— authbridge's config validation rejects that combination, so emitting both would crash-loop the pod.SIDECAR_PORTS_EXCLUDEcarries the resolved forward-proxy port, not the script's8081default:findFreePortmay have moved it, and an unexempted forward-proxy port would be swallowed by the inbound REDIRECT. Gating9091would put kubelet probes behind JWT validation.transparent-in, so which shape is running is visible in the pod spec.transparentInboundPort == transparentPort: both are listeners in one container, so a shared value fails the second bind at pod start, long after admission succeeded.Upgrade-safe: the config loader overlays YAML onto compiled defaults, so an existing ConfigMap without the new keys keeps
8083/ both-allowed. Verified by running this operator against an unmodified 22-day-old cluster ConfigMap.Verification
make test— 21 packages green, including 13 new injector tests and 3 new config-validation tests (table-driven, 9 further cases).make manifests generateproduces no drift.:8000with noPORToverride, sidecar declarestransparent-in=8083+forward-proxy=8081, proxy-init receivesINBOUND_TRANSPARENT_PORT=8083/POD_IP(downward API) /SIDECAR_PORTS_EXCLUDE=8081,9091,9093,9094, and the per-agent ConfigMap containsinbound_interception: transparent+transparent_inbound_addr: :8083with noreverse_proxy_*keys.Port safety
usedPortspreviously recorded the inbound port as reserved but never checked whether the agent had declared it. Under port stealing such a collision was accidentally survivable (Ports[0]got relocated); transparent mode deliberately removes that, so an agent declaring 8083 would have left two processes binding one port in a shared netns — admission succeeds, then the sidecar dies onaddress already in usein a container the user never wrote.All sidecar-owned ports (8082, 8083, 9091, 9093, 9094) are now reserved unconditionally, and a declared collision on a port the sidecar actually binds falls back to port stealing with a warning naming the port and the owning listener. Reservation is separate from collision because
containerPortis informational: under reverse-proxy nothing binds 8083, so declaring it is harmless.This also closed a pre-existing hole — the transparent egress port (8082) was never reserved on main either, so an agent declaring 8081 could push the forward proxy onto a listener that is always on in this mode.
Sequencing note
Default image tags are
:latest(values.yaml:250-252,defaults.go:38-45). If a workload selectstransparentwhile running an authbridge image predating cortex#776, the pod crash-loops rather than silently bypassing: the old binary ignores the unknowninbound_interceptionkey, finds noreverse_proxy_addr/reverse_proxy_backend(deliberately omitted for transparent), and fails config validation. Loud rather than dangerous, and unreachable by accident since the default isreverse-proxy— but on a cluster pinned to an older tag, opting in needs the image bumped first.Verified end-to-end, with one gap
E2E suite 10 passed, 0 skipped on Kind (rossoctl/rossoctl#2393). The operator-side injection was confirmed live: the agent keeps its port with no
PORToverride, the sidecar declarestransparent-in=8083, proxy-init receivesINBOUND_TRANSPARENT_PORT/POD_IP/POD_IPS/SIDECAR_PORTS_EXCLUDE, and the per-agent ConfigMap correctly omitsreverse_proxy_*. The A/B that matters: the transparent agent's real port returns 401 while the reverse-proxy control's relocated port returns 200 — the bypass this closes.Still unverified: the ambient (HBONE) path's runtime behavior. The cluster used runs only
istiod— no ztunnel DaemonSet, no istio-cni — so there is no ambient data plane, and counters confirm all traffic took plain PREROUTING (AB_INBOUNDREDIRECT 4 packets, ambient DNAT 0). The rules are verified installed and correctly ordered; whether ztunnel's re-originated connection matchesmark 0x539 + dst-type LOCALis still reasoned from redirect-mode precedent rather than observed. Needs a cluster with ztunnel deployed.