feat(proxy-init): add enforce-drop mode for proxy-sidecar egress enforcement#484
Conversation
…rcement
proxy-sidecar mode routes outbound traffic through AuthBridge via the
HTTP_PROXY env var, which is purely cooperative: a workload that ignores
HTTP_PROXY (or sets NO_PROXY) egresses directly and bypasses AuthBridge
entirely. Add a fail-closed egress guard so all external traffic is
forced through the forward proxy regardless of whether the workload
honors HTTP_PROXY.
init-iptables.sh gains a MODE switch. Default `redirect` preserves the
existing envoy-sidecar behavior byte-for-byte. MODE=enforce-drop builds
an AB_EGRESS chain hooked from mangle OUTPUT at position 1:
- RETURN ztunnel sockets (fwmark 0x539), the proxy UID, loopback, and
the in-cluster CIDRs (pods/services/DNS)
- DROP everything else (direct external egress, including UDP/QUIC)
plus an IPv6 mirror that drops external v6 egress.
Placement is the mangle table, not filter: when Istio ambient is active
it installs an in-pod `nat OUTPUT` REDIRECT (ISTIO_OUTPUT -> ztunnel
:15001). The netfilter OUTPUT hook order is raw -> mangle -> nat ->
filter, so a DROP in mangle evaluates the original destination and fires
before ambient's nat redirect can rewrite it; a DROP in filter would run
after nat and be defeated. -I 1 also keeps us ahead of Istio's appended
mangle chain. This makes the guard robust with no ambient, in-pod
ambient, or node-level ambient.
test-enforce-drop.sh validates the rule structure and proves the mangle
DROP preempts a simulated ISTIO_OUTPUT nat REDIRECT via packet counters,
in an unshare --net namespace.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds MODE selection (redirect|enforce-drop) to proxy-init; implements enforce-drop by creating an AB_EGRESS mangle OUTPUT chain that RETURNs exempt traffic and DROPs other egress (IPv6 mirrored), updates README and env docs, and adds a namespace test validating chain wiring and preemption. ChangesEnforce-drop egress enforcement feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/proxy-init/test-enforce-drop.sh`:
- Line 19: The script currently enables only "set -u" which leaves command
failures unhandled; update the top-level shell options in
authbridge/proxy-init/test-enforce-drop.sh by replacing the existing set -u with
set -euo pipefail so the test harness exits on errors, treats unset variables as
errors, and makes pipelines fail if any stage fails; ensure this change is
applied where the current "set -u" appears so the new strict mode governs the
whole script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98032e74-8a0b-4903-b59c-1151191ebff3
📒 Files selected for processing (3)
authbridge/proxy-init/README.mdauthbridge/proxy-init/init-iptables.shauthbridge/proxy-init/test-enforce-drop.sh
Address review on rossoctl#484: the AB_EGRESS chain filtered purely on dest/owner/mark, so reply packets for inbound connections (which also traverse mangle OUTPUT) to an off-cluster peer would hit the terminal DROP — e.g. kubelet health-probe responses to the Kind node IP (172.18.0.0/16, outside the default CLUSTER_CIDRS=10.0.0.0/8), causing probe failures and pod restarts once enforcement is enabled. - Add `-m conntrack --ctstate ESTABLISHED,RELATED -j RETURN` as the first rule (IPv4 + IPv6). Only NEW app-initiated flows are gated; a reply is never a bypass. - IPv6: also allow link-local multicast (ff02::/16) so NDP neighbor/router solicitations work, and correct the comment that overstated NDP preservation. - README: clarify the enforce-drop injection lands in the follow-up operator PR rather than reading as already-available. - test: assert the established/related RETURN exists and is the first rule in AB_EGRESS. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Thanks for the review — all three addressed in 17c05ec: 1 (ESTABLISHED,RELATED — the real one): Spot on. Replies to inbound connections traverse 2 (IPv6 NDP): Correct — only 3 (README honesty): Qualified the enforce-drop injection bullet — it now states the operator wiring lands in the follow-up PR; this PR only adds the mode to the image. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Genuinely nice piece of work, Hai. The mangle OUTPUT placement is the right call: hooking before nat evaluates the original destination and survives an in-pod ambient ISTIO_OUTPUT REDIRECT — and the test proves it via packet counters rather than asserting on rule text alone. 滴水不漏 (dī shuǐ bù lòu — "not letting a single drop through"); apt for a fail-closed egress guard. The PR-1-of-3 framing is also helpful — keeping this commit a no-op until the operator wires MODE=enforce-drop is the right way to land it.
Areas reviewed: Shell, Docs
Commits: 1, signed-off + Assisted-By ✓
CI status: all green; CodeRabbit's strict-mode nit on the test harness is left inline below — agree it's worth promoting.
Left three non-blocking suggestions on docs and the test harness. None should hold this up.
| # bash, the dummy kernel module. Runs on Linux / CI (e.g. ubuntu-latest); not | ||
| # on macOS. Uses `unshare --net` (not named `ip netns`) so it also works inside | ||
| # nested containers. Exit code 0 = all pass. | ||
| set -u |
There was a problem hiding this comment.
suggestion (CodeRabbit's point, agree): promoting set -u to set -euo pipefail would catch silent failures in the harness — there are several awk-piped extractions and ${IPT} -t ... invocations whose non-zero exit should fail the test, not just be observed. Especially because the test runs in a fresh netns inside unshare, so a malformed pipeline would otherwise look like a clean run that just "didn't trigger."
-set -u
+set -euo pipefailBash shebang on line 1 supports it.
There was a problem hiding this comment.
Already landed in 4f66a6b — switched to set -euo pipefail, verified all 13 assertions still run and the harness exits 0. Thanks (and to CodeRabbit) for promoting it.
| | `OUTBOUND_PORTS_EXCLUDE` | (empty) | redirect | Comma-separated outbound port list to skip (e.g. `8080`) | | ||
| | `INBOUND_PORTS_EXCLUDE` | (empty) | redirect | Comma-separated inbound port list to skip | | ||
| | `POD_IP` | (required in `redirect`) | redirect | Set via Downward API; DNAT target for ambient-mesh inbound. Not used by `enforce-drop`. | | ||
| | `CLUSTER_CIDRS` | `10.0.0.0/8` | enforce-drop | Comma-separated in-cluster CIDRs allowed direct (pods/services/DNS) | |
There was a problem hiding this comment.
suggestion: the 10.0.0.0/8 default covers Kind (pods 10.244/16 + services 10.96/16) cleanly, but the same operator dropped onto a stock OpenShift cluster will see services in 172.30.0.0/16 and pods in 10.128.0.0/14. Both are outside 10/8 for services. Could be worth a one-line callout in the README that the default is Kind-shaped and OCP/EKS users should override CLUSTER_CIDRS (or that init-iptables.sh could log the resolved value at startup — which it already does, just confirming that's the intended diagnostic path).
There was a problem hiding this comment.
Good catch on OCP — 172.30.0.0/16 services are outside 10/8 and would be dropped. Added a README callout (f3101d7): the default is Kind-shaped and OCP/EKS must override CLUSTER_CIDRS, with the startup log as the diagnostic path. The follow-up operator PR will set CLUSTER_CIDRS from the cluster's real pod+service ranges, so the default only matters for standalone/Kind testing.
| | `PROXY_UID` | `1337` | both | UID of the AuthBridge sidecar process; exempted from redirect / drop | | ||
| | `PROXY_PORT` | `15123` | redirect | AuthBridge outbound listener port | | ||
| | `INBOUND_PROXY_PORT` | `15124` | redirect | AuthBridge inbound listener port | | ||
| | `OUTBOUND_PORTS_EXCLUDE` | (empty) | redirect | Comma-separated outbound port list to skip (e.g. `8080`) | |
There was a problem hiding this comment.
clarifying question: the OUTBOUND_PORTS_EXCLUDE / INBOUND_PORTS_EXCLUDE rows being redirect-only matches the implementation. I want to confirm the design intent: in enforce-drop, an operator who currently uses OUTBOUND_PORTS_EXCLUDE=11434 (Ollama) for redirect would lose direct egress to that port — they'd be forced through the forward proxy, which is exactly what enforce-drop exists to do. Just flagging because the docs/operator wiring (PR 2) might want to surface this as "if you've been relying on OUTBOUND_PORTS_EXCLUDE to bypass the proxy in proxy-sidecar mode, enforce-drop will close that off — that's the point."
There was a problem hiding this comment.
Confirmed — that's the intended design. enforce-drop deliberately does not honor OUTBOUND_PORTS_EXCLUDE: a destination previously bypassed via it (e.g. Ollama at host.docker.internal:11434) is now dropped unless it goes through the forward proxy or is in CLUSTER_CIDRS. Added a README note saying exactly that ("enforce-drop closes direct-egress holes"). Agreed the operator PR should call this out in its rollout notes.
Address CodeRabbit on rossoctl#484: switch the test harness from set -u to set -euo pipefail (the repo's shell convention) so command/pipe failures in the setup phase surface instead of being masked. The run-all-assertions design is unaffected — asserts return 0 via if/else and expected-nonzero commands are guarded with || true; verified all 13 assertions still run and the harness exits 0. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
authbridge/proxy-init/test-enforce-drop.sh (1)
46-76: ⚡ Quick winAdd IPv6 rule validation.
The test sets
IP6TABLES_CMD=ip6tables-nftat line 48, enabling IPv6 rule creation ininit-iptables.sh, but never validates that IPv6 rules were installed. Since IPv6 support is documented (README mentions "An IPv6 mirror") and part of the feature, the test should verify the IPv6AB_EGRESSchain exists and has the expected structure.Consider adding after line 76:
# Validate IPv6 mirror (if ip6tables-nft is available) if command -v ip6tables-nft >/dev/null 2>&1; then dump6=$(ip6tables-nft -t mangle -S) echo "--- mangle IPv6 ruleset ---"; echo "${dump6}" assert6() { if echo "${dump6}" | grep -qE "$2"; then echo "PASS: [IPv6] $1"; else echo "FAIL: [IPv6] $1"; fail=1; fi; } assert6 "AB_EGRESS hooked from OUTPUT" '^-A OUTPUT -j AB_EGRESS' assert6 "established/related RETURN" 'AB_EGRESS -m conntrack --ctstate (ESTABLISHED,RELATED|RELATED,ESTABLISHED) -j RETURN' assert6 "terminal DROP" 'AB_EGRESS -j DROP' fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/proxy-init/test-enforce-drop.sh` around lines 46 - 76, Add IPv6 validation similar to the IPv4 checks: when IP6TABLES_CMD (or ip6tables-nft) is available, capture the IPv6 mangle rules into a variable (e.g., dump6), print it, and run parallel assertions (assert6) against the AB_EGRESS chain to verify the hook from OUTPUT, the established/related conntrack RETURN, and the terminal DROP; place this block after the IPv4 mangle assertions/natcount check so it executes as part of the same test flow and use the same pattern of setting fail=1 on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@authbridge/proxy-init/test-enforce-drop.sh`:
- Around line 46-76: Add IPv6 validation similar to the IPv4 checks: when
IP6TABLES_CMD (or ip6tables-nft) is available, capture the IPv6 mangle rules
into a variable (e.g., dump6), print it, and run parallel assertions (assert6)
against the AB_EGRESS chain to verify the hook from OUTPUT, the
established/related conntrack RETURN, and the terminal DROP; place this block
after the IPv4 mangle assertions/natcount check so it executes as part of the
same test flow and use the same pattern of setting fail=1 on failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0eba212d-1bd3-4c32-b881-7fa62a8255df
📒 Files selected for processing (1)
authbridge/proxy-init/test-enforce-drop.sh
…semantics Address mrsabath review on rossoctl#484 (non-blocking): - CLUSTER_CIDRS=10.0.0.0/8 default is Kind-shaped; OCP (services 172.30.0.0/16, pods 10.128.0.0/14) and EKS users must override it or in-cluster service traffic is dropped. Operator wiring (follow-up PR) sets it from the cluster's real CIDRs. - enforce-drop intentionally ignores OUTBOUND_PORTS_EXCLUDE (redirect-only); a destination previously bypassed that way is now dropped unless routed through the proxy or in CLUSTER_CIDRS — by design. The third suggestion (set -euo pipefail) already landed in 4f66a6b. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Summary
proxy-sidecarmode enforces egress purely via theHTTP_PROXYenv var, which is cooperative — a workload that ignoresHTTP_PROXYor setsNO_PROXYegresses directly and bypasses AuthBridge. This adds a fail-closed egress guard (MODE=enforce-drop) to the sharedproxy-initimage so all external traffic is forced through the forward proxy regardless of whether the app cooperates.This is PR 1 of 3 in the proxy-sidecar egress-enforcement series:
proxy-initenforce-dropmode — lands first, no behavior change until something setsMODE=enforce-dropRunAsUserto the dedicated1337(currently hardcoded to the app's UID) + annotation-gatedproxy-initinjection in proxy-sidecar modeWhat changed (
authbridge/proxy-init/)init-iptables.sh— adds aMODEswitch. Defaultredirectpreserves the existing envoy-sidecar behavior byte-for-byte.MODE=enforce-dropbuilds anAB_EGRESSchain hooked frommangleOUTPUT at position 1:RETURNztunnel sockets (0x539), the proxy--uid-owner, loopback, andCLUSTER_CIDRS; thenDROPeverything else (incl. UDP/QUIC). Includes an IPv6 mirror.POD_IPis now required only inredirectmode.test-enforce-drop.sh(new) —unshare --netharness that asserts the rule structure and proves (via packet counters) themangleDROP preempts a simulated Istio ambientnat OUTPUTREDIRECT.README.md— two-mode model, env-var table (MODE,CLUSTER_CIDRS,CLUSTER_CIDRS6,IP6TABLES_CMD), the mangle-vs-filter rationale, testing/injection notes.Why
mangle, notfilterWhen Istio ambient is active it installs an in-pod
nat OUTPUTREDIRECT (ISTIO_OUTPUT→ ztunnel:15001). The netfilter OUTPUT hook order israw → mangle → nat → filter, so a DROP inmangleevaluates the original destination and fires before ambient rewrites it; afilterDROP runs after nat and would be defeated. This keeps the guard robust with no ambient, in-pod ambient, or node-level ambient.Testing
All assertions pass, including the ambient-robustness gate:
Compatibility / scope
MODEdefaults toredirect; envoy-sidecar is unchanged. Nothing setsMODE=enforce-dropuntil the follow-up operator PR.Dockerfile.initchange needed — Alpine'siptables/iptables-legacypackages already provide theip6tablesvariants (the v6 path iscommand -v-guarded and degrades cleanly).MODE=enforce-drop, cluster CIDRs, pinned image tag) and CI test integration land in the follow-up kagenti-operator PR.Assisted-By: Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests