Skip to content

feat: Add transparent listener + enforce-redirect for proxy-sidecar egress#485

Merged
huang195 merged 7 commits into
rossoctl:mainfrom
huang195:feat/transparent-proxy-egress-redirect
Jun 6, 2026
Merged

feat: Add transparent listener + enforce-redirect for proxy-sidecar egress#485
huang195 merged 7 commits into
rossoctl:mainfrom
huang195:feat/transparent-proxy-egress-redirect

Conversation

@huang195

@huang195 huang195 commented Jun 5, 2026

Copy link
Copy Markdown
Member

Problem

proxy-sidecar egress enforcement could previously only DROP bypass traffic (MODE=enforce-drop, #484). The forward proxy only speaks explicit CONNECT — it reads the destination from the request line/Host header — so an iptables-REDIRECTed raw connection arrives with no recoverable destination. There was nothing on the proxy that could serve a redirected connection, which is exactly why enforcement had to drop rather than redirect.

DROP is fail-closed but breaks any agent that doesn't honor HTTP_PROXY, which is why enforce-drop stays opt-in and off by default.

envoy-sidecar mode handles redirected traffic only because Envoy supplies the original_dst listener filter + ORIGINAL_DST cluster; the auth pipeline itself is already shared between modes.

What this PR does

Adds the missing half so enforcement can capture instead of drop — and therefore eventually be always-on with no breakage.

Transparent listener (authlib/listener/transparentproxy)

  • Outbound-only listener that accepts iptables-REDIRECTed raw TCP and recovers the original destination via SO_ORIGINAL_DST (Linux, build-tagged; a !linux stub keeps dev hosts buildable). The pure sockaddr parser is cross-platform and unit-tested.
  • The Go analogue of Envoy's original_dst filter; dispatches to a ConnHandler.

forwardproxy.Server.HandleTransparentConn

  • Gates the connection on destination/identity through the same outbound pipeline as the CONNECT path, then blind-tunnels — reusing the existing tunnel + session-recording logic (extracted into shared recordTunnelOpened/tunnel helpers, so handleConnect is simplified too).
  • Emits no proxy-protocol bytes back to the agent (no 200 Connection Established, no hijack): the agent believes it is talking directly to the destination, so its end-to-end TLS is preserved — identical to today's CONNECT blind-tunnel and Envoy's tcp_proxy passthrough. (HTTPS remains opaque; this is L4 capture + destination policy, not content inspection.)
  • Wired into both authbridge-proxy and authbridge-lite (the proxy-sidecar shapes). New listener.transparent_proxy_addr config, default :8082.

init-iptables.sh MODE=enforce-redirect

  • A nat OUTPUT chain (AB_REDIRECT) inserted at position 1 to preempt Istio ambient's nat redirect. Exempts (RETURN) ztunnel fwmark 0x539, the proxy UID, loopback, and CLUSTER_CIDRS (in-cluster stays direct).
  • REDIRECTs external TCP to TRANSPARENT_PORT; DROPs non-TCP external egress so HTTP/3 (QUIC) cannot bypass — clients fall back to TCP and get captured.
  • enforce-drop is retained as a no-transparent-listener fallback.

Testing

  • transparentproxy: unit test for the SO_ORIGINAL_DST sockaddr parser (table-driven, cross-platform).
  • forwardproxy: net.Pipe tunnel test for HandleTransparentConn — asserts the agent's first bytes are the upstream's, proving no proxy-protocol leak.
  • test-enforce-redirect.sh: unshare --net harness proving (1) AB_REDIRECT structure + position-1 hook, (2) external TCP captured to TRANSPARENT_PORT while preempting a simulated ISTIO_OUTPUT redirect, (3) external UDP dropped — all via packet counters.
  • authlib full test suite passes; both binaries build; Linux cross-compile exercises the syscall path.

Rollout / coordination

  • Inert until wired: nothing redirects to the transparent listener until the operator emits MODE=enforce-redirect. The companion kagenti-operator PR (wiring + removing the egressEnforcement flag, making enforcement always-on for proxy-sidecar) lands after this, and can only be released once a proxy-init + authbridge/authbridge-lite build containing this PR is published.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

Summary by CodeRabbit

  • New Features

    • Added configurable outbound transparent-proxy listener (default :8082) and startup in proxy-sidecar/lite modes; transparent listener accepts redirected TCP and dispatches to proxy.
  • Behavior Changes

    • Forward/transparent tunneling records tunnel-open events and uses coordinated bidirectional tunneling for cleaner teardown.
    • Added connection sniffing to recover destination hostnames (SNI/Host) for policy while replaying bytes.
  • Documentation

    • Added enforce-redirect docs, iptables setup, env vars, and testing guidance.
  • Platform

    • Linux support to recover original destination; non-Linux stub retains buildability.
  • Tests

    • New tests for transparent tunneling, sniffing, and sockaddr parsing; iptables-mode CI test added.

…gress

proxy-sidecar egress enforcement could previously only DROP bypass traffic
(MODE=enforce-drop), because the forward proxy only speaks explicit CONNECT —
an iptables-REDIRECTed raw connection arrives with no destination it can
recover, so there was nothing to redirect to. DROP is fail-closed but breaks
agents that ignore HTTP_PROXY, which is why it stayed opt-in.

This adds the missing half: a transparent listener that captures redirected
egress, so enforcement can REDIRECT instead of DROP (capture, do not break).

- authlib/listener/transparentproxy: an outbound transparent listener that
  recovers the original destination via SO_ORIGINAL_DST (Linux, build-tagged;
  pure cross-platform sockaddr parser with a unit test) and dispatches to a
  ConnHandler. This is the Go analogue of Envoy's original_dst listener filter.
- forwardproxy.Server.HandleTransparentConn: gates on destination then
  blind-tunnels, reusing the exact CONNECT-path pipeline + tunnel (extracted
  into shared recordTunnelOpened/tunnel helpers). Emits no proxy-protocol bytes
  so the agent's end-to-end TLS is preserved.
- Wire the listener into authbridge-proxy and authbridge-lite (proxy-sidecar
  shapes); new listener.transparent_proxy_addr config, default :8082.
- init-iptables.sh MODE=enforce-redirect: nat OUTPUT chain that REDIRECTs
  external TCP to TRANSPARENT_PORT (exempting proxy UID, loopback, cluster
  CIDRs, ztunnel mark) and DROPs non-TCP external egress so HTTP/3 cannot
  bypass. Inserted at position 1 to preempt Istio ambient's nat redirect.
- test-enforce-redirect.sh: netns harness proving capture + ambient preemption
  + non-TCP drop via packet counters. README documents the new mode.

enforce-drop is retained as a no-transparent-listener fallback. Inert until the
operator wires MODE=enforce-redirect (companion kagenti-operator PR).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place.

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an optional outbound transparent-proxy listener and supporting plumbing for proxy-sidecar "enforce-redirect": config default, Linux original-destination recovery, transparent listener server, forward-proxy sniffing/tunneling and session events, binary wiring, iptables setup and tests, and README updates.

Changes

Transparent Proxy Listener

Layer / File(s) Summary
Configuration and Defaults
authbridge/authlib/config/config.go, authbridge/authlib/config/presets.go, authbridge/authlib/go.mod
TransparentProxyAddr field added to ListenerConfig with :8082 default for ModeProxySidecar; golang.org/x/sys promoted to direct dependency.
Original Destination Recovery (Linux)
authbridge/authlib/listener/transparentproxy/origdst_linux.go, authbridge/authlib/listener/transparentproxy/origdst_other.go
Implements originalDst using SyscallConn + getsockopt for SO_ORIGINAL_DST; non-Linux build stub returns unsupported error.
Socket Address Parsing
authbridge/authlib/listener/transparentproxy/sockaddr.go, authbridge/authlib/listener/transparentproxy/sockaddr_test.go
Parses sockaddr_in/sockaddr_in6 from SO_ORIGINAL_DST to host:port strings; table-driven tests for IPv4/IPv6 and error cases.
Transparent Listener Server
authbridge/authlib/listener/transparentproxy/server.go
TCP listener that accepts REDIRECTed connections, recovers original destinations, defends against self-redirect loops, and dispatches connections to a ConnHandler; logs and closes unrecoverable connections.
Forward-proxy sniffing and tests
authbridge/authlib/listener/forwardproxy/sniff.go, authbridge/authlib/listener/forwardproxy/sniff_test.go
Peek-based TLS SNI / HTTP Host sniffing with replaying wrapper; tests validate SNI, Host, opaque replay, and sniff routing logic.
Forward-proxy transparent tunnel
authbridge/authlib/listener/forwardproxy/transparent.go, authbridge/authlib/listener/forwardproxy/transparent_test.go, authbridge/authlib/listener/forwardproxy/server.go
Adds HandleTransparentConn, recordTunnelOpened, and tunnel; refactors CONNECT handling to call these; includes an end-to-end tunneling test validating no proxy-response leakage.
Runtime Integration (Binaries)
authbridge/cmd/authbridge-lite/main.go, authbridge/cmd/authbridge-proxy/main.go
Conditionally starts the transparent listener when TransparentProxyAddr is set, serves redirected connections with transparentproxy.NewServer(fp.HandleTransparentConn), and closes listeners on shutdown.
Iptables Configuration and Testing
authbridge/proxy-init/init-iptables.sh, authbridge/proxy-init/test-enforce-redirect.sh
Adds enforce-redirect mode, TRANSPARENT_PORT (default 8082), setup_enforce_redirect() to install AB_REDIRECT (nat) and AB_NOTCP (mangle) at position 1, and a CI test validating rule structure, capture/preemption, and non-TCP drop behavior.
Documentation Updates
authbridge/proxy-init/README.md
Documents enforce-redirect mode, chain behavior, environment variables, testing requirements, and injection selection for proxy-sidecar/lite.

Sequence Diagram (high-level transparent tunnel flow):

sequenceDiagram
  participant Client
  participant HandleTransparent as HandleTransparentConn
  participant Pipeline
  participant Upstream
  Client->>HandleTransparent: clientConn, dst
  HandleTransparent->>Pipeline: OutboundPipeline.Run
  alt Pipeline allows
    HandleTransparent->>Upstream: Dial dst
    HandleTransparent->>HandleTransparent: recordTunnelOpened
    HandleTransparent->>HandleTransparent: tunnel(client, upstream)
    loop io.Copy both directions
      Client<<-->>Upstream: Transparent data
    end
  else Pipeline denies
    HandleTransparent->>Client: Close
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • pdettori
  • mrsabath

Poem

🐰 I nudged kernels and parsed sockaddr lore,
I chased the original dst to its core,
With iptables redirect and a sidecar ear,
I hop packets home without extra cheer,
A rabbit's little tunnel — safe paths once more.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the main change: adding a transparent listener and enforce-redirect feature for proxy-sidecar egress capture.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
authbridge/cmd/authbridge-proxy/main.go (1)

396-422: 💤 Low value

Consider extracting startTransparentProxy to a shared helper.

This function is nearly identical to the one in authbridge-lite/main.go (lines 352-377). Both could delegate to a shared helper in authlib to reduce duplication. That said, the current duplication is minimal (~25 lines) and may be intentional to keep binary entrypoints independent.

🤖 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/cmd/authbridge-proxy/main.go` around lines 396 - 422, The
startTransparentProxy function is duplicated; extract its logic into a single
shared helper in authlib (e.g., StartTransparentProxy) and have both
startTransparentProxy callers delegate to it. Implement a helper that takes the
forward proxy handler (or fp *forwardproxy.Server and its HandleTransparentConn)
and addr string, resolves and listens on the TCP address, creates
transparentproxy.NewServer(fp.HandleTransparentConn), serves in a goroutine with
the same logging/fatal-on-error behavior, and returns the *net.TCPListener; then
replace the duplicate implementations (startTransparentProxy in both
entrypoints) with calls to the new authlib helper.
🤖 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/authlib/config/presets.go`:
- Around line 19-23: The presets file currently calls
setDefault(&cfg.Listener.TransparentProxyAddr, ":8082") which overwrites an
explicitly-empty value and breaks the TransparentProxyAddr doc-comment
semantics; change this so empty string continues to mean “disabled”: remove that
setDefault call and instead set the default only when the value is truly absent
(e.g., introduce and use a new helper like setDefaultIfAbsent for string fields
or adjust setDefault to detect an explicit empty vs. absent value) and apply it
to cfg.Listener.TransparentProxyAddr so that TransparentProxyAddr == "" remains
unmodified and retains the “disable listener” behavior.

In `@authbridge/authlib/listener/transparentproxy/server.go`:
- Around line 36-37: NewServer currently stores the provided ConnHandler
directly and a nil handle will panic at dispatch (s.handle(conn, dst)); modify
NewServer to guard against nil by replacing a nil handle with a safe no-op
ConnHandler (for example a closure that logs and closes the connection or
returns immediately) so dispatch won't panic. Update NewServer to set
Server.handle = handle if non-nil else assign the safe default; also audit the
call site referenced by s.handle(conn, dst) and ensure you do not call a nil
handler elsewhere (the same nil-guard pattern should apply wherever Server is
constructed).

In `@authbridge/proxy-init/init-iptables.sh`:
- Around line 353-361: The current iptables rules in the script add a TCP
REDIRECT for all external TCP (using ${IPT}, ${CHAIN}, and ${TRANSPARENT_PORT})
but do not exclude SSH; add a rule that matches TCP destination port ${SSH_PORT}
and issues a RETURN (i.e., ${IPT} -t nat -A "${CHAIN}" -p tcp --dport
"${SSH_PORT}" -j RETURN) immediately before any REDIRECT entries that redirect
TCP to ${TRANSPARENT_PORT} (this must be done for each place that adds the
REDIRECT for TCP so SSH traffic is returned instead of redirected).

In `@authbridge/proxy-init/README.md`:
- Around line 11-12: The README mode table currently lists `enforce-redirect`
and `enforce-drop` as applicable only to `proxy-sidecar`, but the injection
section (lines ~186-192) also references `lite`; update the top mode table
entries for `enforce-redirect` and `enforce-drop` to include `lite` (or remove
`lite` from the injection section if that is intended) so both sections are
consistent; look for the mode strings `enforce-redirect`, `enforce-drop`,
`proxy-sidecar`, and `lite` in the README and make the table and injection
section match.

---

Nitpick comments:
In `@authbridge/cmd/authbridge-proxy/main.go`:
- Around line 396-422: The startTransparentProxy function is duplicated; extract
its logic into a single shared helper in authlib (e.g., StartTransparentProxy)
and have both startTransparentProxy callers delegate to it. Implement a helper
that takes the forward proxy handler (or fp *forwardproxy.Server and its
HandleTransparentConn) and addr string, resolves and listens on the TCP address,
creates transparentproxy.NewServer(fp.HandleTransparentConn), serves in a
goroutine with the same logging/fatal-on-error behavior, and returns the
*net.TCPListener; then replace the duplicate implementations
(startTransparentProxy in both entrypoints) with calls to the new authlib
helper.
🪄 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: 4adda382-a3c0-4690-8bbc-3996b9ea155f

📥 Commits

Reviewing files that changed from the base of the PR and between dcc1a92 and bb15d20.

📒 Files selected for processing (16)
  • authbridge/authlib/config/config.go
  • authbridge/authlib/config/presets.go
  • authbridge/authlib/go.mod
  • authbridge/authlib/listener/forwardproxy/server.go
  • authbridge/authlib/listener/forwardproxy/transparent.go
  • authbridge/authlib/listener/forwardproxy/transparent_test.go
  • authbridge/authlib/listener/transparentproxy/origdst_linux.go
  • authbridge/authlib/listener/transparentproxy/origdst_other.go
  • authbridge/authlib/listener/transparentproxy/server.go
  • authbridge/authlib/listener/transparentproxy/sockaddr.go
  • authbridge/authlib/listener/transparentproxy/sockaddr_test.go
  • authbridge/cmd/authbridge-lite/main.go
  • authbridge/cmd/authbridge-proxy/main.go
  • authbridge/proxy-init/README.md
  • authbridge/proxy-init/init-iptables.sh
  • authbridge/proxy-init/test-enforce-redirect.sh

Comment thread authbridge/authlib/config/presets.go
Comment thread authbridge/authlib/listener/transparentproxy/server.go
Comment thread authbridge/proxy-init/init-iptables.sh Outdated
Comment thread authbridge/proxy-init/README.md Outdated
An end-to-end test (real transparent listener + real init-iptables.sh in a
privileged container, agent dialing an off-box upstream) caught that the nat
table forbids `-j DROP` ("the use of DROP is therefore inhibited"). The
original enforce-redirect put the non-TCP DROP in the nat AB_REDIRECT chain, so
the init script errored at runtime.

Fix: two chains.
- nat OUTPUT / AB_REDIRECT: exemptions + REDIRECT external TCP to
  TRANSPARENT_PORT (no DROP).
- mangle OUTPUT / AB_NOTCP: ESTABLISHED,RELATED + same exemptions, then
  `-p tcp -j RETURN` (TCP falls through to the nat REDIRECT) and a terminal
  DROP for external non-TCP (UDP/QUIC). mangle runs before nat in the OUTPUT
  hook, so non-TCP drops on its original destination and TCP reaches the nat
  REDIRECT. Both inserted at position 1 to preempt Istio's appended chains.

Updates test-enforce-redirect.sh to assert the two-chain structure (nat has no
DROP; mangle AB_NOTCP has the tcp-RETURN + DROP) and reads the non-TCP DROP
counter from mangle. README updated to document the two-chain design.

End-to-end verified: under enforce-redirect a uid!=PROXY_UID agent dialing an
external upstream directly is captured (reaches it via the transparent listener,
SO_ORIGINAL_DST recovers the original dst); under enforce-drop the same
connection times out; re-enabling enforce-redirect recovers delivery.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
@huang195

huang195 commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

End-to-end verification (capture vs. drop)

Ran the real transparent listener (forwardproxy.HandleTransparentConn via the transparentproxy package) + the real init-iptables.sh in a privileged Linux container, with the "agent" running as a non-proxy UID and the upstream in a separate container reached over the docker network (genuinely off-box, outside CLUSTER_CIDRS):

### TEST A — MODE=enforce-redirect (expect CAPTURE)
agent curl -> http_code=200 body='CAPTURED-OK'
PASS: enforce-redirect captured the bypass connection
  driver log: CAPTURED bypass connection from 172.31.0.3:50334 -> original-dst=172.31.0.2:80

### TEST B — MODE=enforce-drop (expect DROP: same connection)
agent curl -> exit=28 http_code=000
PASS: enforce-drop dropped the same bypass connection

### Control — MODE=enforce-redirect again
agent curl -> http_code=200 body='CAPTURED-OK'
PASS: re-enabling enforce-redirect recovers delivery

The CAPTURED ... original-dst=172.31.0.2:80 line proves the bypass traffic traversed the transparent listener and that SO_ORIGINAL_DST recovered the real destination — i.e. the packets enforce-drop drops are exactly the ones enforce-redirect now captures and tunnels through AuthBridge.

This test also caught a bug, now fixed in f884dc4: the nat table forbids -j DROP, so the non-TCP drop had to move to mangle (AB_NOTCP) while TCP REDIRECT stays in nat (AB_REDIRECT). The committed test-enforce-redirect.sh netns harness asserts the corrected two-chain structure + capture/preemption/non-TCP-drop via packet counters.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

huang195 added 2 commits June 5, 2026 17:56
- config: clarify TransparentProxyAddr doc — the proxy-sidecar/lite presets
  default it to :8082 (always-on by design); empty only disables it for modes
  without a preset default. Removes the misleading 'empty disables' claim that
  the preset contradicts.
- transparentproxy.NewServer: guard against a nil ConnHandler (fall back to a
  close-and-log handler) so a misconfiguration degrades to no-capture instead of
  a dispatch-time panic.
- proxy-init README: list 'lite' alongside 'proxy-sidecar' in the mode table to
  match the injection section.

Declined: SSH (port 22) exclusion for enforce-redirect. Excluding 22 would punch
a deliberate egress bypass hole, contradicting the fail-closed purpose; the
transparent listener blind-tunnels any TCP so captured SSH still works (unlike
redirect mode, which forwards to Envoy's HTTP listener — the reason it excludes
22). enforce-drop does not exclude 22 either.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Address follow-up review on the transparent listener:

- transparentproxy.dispatch: drop connections whose SO_ORIGINAL_DST is a
  loopback address. A genuinely REDIRECTed connection never has a loopback
  original destination (the enforce-redirect iptables RETURN loopback before the
  REDIRECT), so a loopback dst means a direct dial to the listener port where
  SO_ORIGINAL_DST reports the listener's own address — tunnelling to it would
  spiral into ever more connections/goroutines. Defense-in-depth.

- HandleTransparentConn: document the gating-key difference. CONNECT gates on
  the hostname (r.Host); captured traffic gates on the SO_ORIGINAL_DST IP:port,
  so domain-based egress policy does not match captured bypass traffic the way
  it matches explicit-proxy traffic. Acceptable while opt-in; the always-on
  operator PR must first add SNI-peek for parity or consciously accept IP-level
  policy. Tracked as a prerequisite for the default flip.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
@huang195

huang195 commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Follow-up review items — resolved (commit 6646b57)

1. Capture gates on IP:port (SO_ORIGINAL_DST) vs CONNECT's hostname — host/domain policy gap.
Real and important for always-on. There's no reusable SNI parser in the tree (tlssniff only does TLS-vs-plaintext byte-peek), and a correct ClientHello-SNI parser + replaying peek-conn is non-trivial. Since enforce-redirect stays opt-in until the operator PR, I've documented the limitation prominently in HandleTransparentConn and made SNI-parity (or a conscious acceptance of IP-level policy) an explicit prerequisite gate for the always-on default flip in the operator PR. This is the "document the IP-level limitation" path the review offered; SNI-peek can land in the operator PR where always-on is decided.

2. No guard against a recovered dst pointing at the listener itself (self-redirect spiral).
Fixed. dispatch now drops any connection whose SO_ORIGINAL_DST is a loopback address. A genuinely REDIRECTed connection never has a loopback original destination (the enforce-redirect iptables RETURN loopback before the REDIRECT), so a loopback dst means a direct dial to the listener port where SO_ORIGINAL_DST reports the listener's own address — the exact self-loop vector. One-line defense-in-depth, with a warning log.

3. --to-port (script) vs --to-ports (test) spelling.
No change — confirmed harmless. The whole script uses --to-port (singular) consistently (redirect-mode lines 534/619 too); the netns test greps iptables' -S output, which normalizes REDIRECT to --to-ports, so it matches at runtime regardless of the literal spelling.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/authlib/listener/transparentproxy/server.go`:
- Around line 79-92: The current loop-guard only drops loopback dsts but misses
self-dials to the listener's non-loopback address; update the check (in the
block handling dst before calling HandleTransparentConn) to also detect and drop
connections whose original destination equals the listener itself: parse dst
with net.SplitHostPort/net.ParseIP, compare the host:port to the listener's own
address (listener.Addr().String() or the configured listenAddr) and/or check if
the dst IP matches any of the machine's local interface addresses
(net.InterfaceAddrs()) with the same port; if it matches, log a warning (same
style as existing slog.Warn) and conn.Close() to prevent recursive re-dials via
HandleTransparentConn.
🪄 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: 82a63596-9874-4d48-8cc0-2c6d786b6a45

📥 Commits

Reviewing files that changed from the base of the PR and between cae8cfe and 6646b57.

📒 Files selected for processing (2)
  • authbridge/authlib/listener/forwardproxy/transparent.go
  • authbridge/authlib/listener/transparentproxy/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • authbridge/authlib/listener/forwardproxy/transparent.go

Comment thread authbridge/authlib/listener/transparentproxy/server.go
… policy parity

Captured (iptables-REDIRECTed) traffic previously gated on the SO_ORIGINAL_DST
IP:port, while the explicit-proxy CONNECT path gates on the hostname — so
host/domain egress policy did not apply to captured bypass traffic the same way.

Add forwardproxy/sniff.go: peek the connection's first bytes (without consuming
them — replayed into the tunnel via a bufio-backed peekedConn) and recover the
destination hostname:
- HTTPS: parse the TLS ClientHello SNI, leaning on crypto/tls's own parser via a
  throwaway GetConfigForClient handshake (no hand-rolled ClientHello parsing).
- HTTP: parse the Host header from the request preamble.
Fall back to the IP when neither is recoverable. HandleTransparentConn uses the
recovered name as pctx.Host; the dial target ALWAYS stays the SO_ORIGINAL_DST IP
(never re-resolve the name). Sniffing is gated to HTTP/TLS-typical ports
(80/443/8080/8443) so non-HTTP, often server-first protocols (SSH, SMTP, ...)
are not delayed by the peek; a read deadline bounds it regardless.

Tests: real ClientHello (via crypto/tls) yields the SNI; a plaintext request
yields the Host header (port stripped); opaque bytes yield no name; all three
prove the peeked bytes are replayed verbatim. End-to-end (container) verified:
an agent dialing the IP while sending Host: api.example.test has the listener
recover api.example.test, and capture-vs-drop still holds.

Trust caveat (documented): for captured traffic the agent controls both the
name and the IP, so name-based policy is reliable against a cooperative or
misconfigured agent but is not a hard control against a hostile one — only the
IP is ground truth. The always-on operator PR must decide that threat model.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
@huang195

huang195 commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Update: hostname recovery now implemented (commit 66f82e9)

Earlier I deferred the IP-vs-hostname gating gap to the operator PR. On reflection it was worth doing here, for both HTTP and HTTPS, so captured traffic gates the same way the CONNECT path does:

  • HTTPS — peek the TLS ClientHello and extract SNI (via crypto/tls's own parser through a throwaway GetConfigForClient handshake — no hand-rolled ClientHello parsing).
  • HTTP — parse the Host header from the request preamble.
  • Fallback — IP, when neither is recoverable.

The peeked bytes are replayed into the tunnel verbatim (bufio-backed peekedConn), the dial target always stays the SO_ORIGINAL_DST IP (the name is only the policy key, never re-resolved), and sniffing is gated to HTTP/TLS ports (80/443/8080/8443) so server-first protocols (SSH, SMTP, …) aren't delayed by the peek.

Tests: real ClientHello → SNI; plaintext request → Host header (port stripped); opaque bytes → no name; all assert verbatim replay. End-to-end (container) confirmed: an agent dialing the IP while sending Host: api.example.test has the listener log recovered destination host=api.example.test dst=172.31.0.2:80, and capture-vs-drop still holds.

Residual caveat (left for the always-on operator PR): for captured traffic the agent controls both the name and the IP the bytes actually go to, so name-based policy is reliable against a cooperative/misconfigured agent but is not a hard control against a hostile one — only the IP is ground truth. Hard enforcement would need IP-set allowlists or SNI/cert cross-checks; that's a threat-model decision for the default flip, now documented in HandleTransparentConn.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

enforce-drop existed only because, before the transparent listener, a
REDIRECT had no receiver so DROP was the only fail-closed option. Now that
every proxy-sidecar/lite image ships the transparent listener,
enforce-redirect captures bypass egress instead of dropping it, and the
operator only emits enforce-redirect. enforce-drop is dead.

Remove the enforce-drop MODE (setup_enforce_drop + AB_EGRESS chain + the
dispatch + the mode-table/env-var/injection docs) and test-enforce-drop.sh.
The init script now accepts only redirect | enforce-redirect.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>

@mrsabath mrsabath left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid, production-careful PR — approving.

What it does: adds the "capture instead of drop" half of proxy-sidecar egress enforcement. The new transparentproxy listener recovers the original destination of iptables-REDIRECTed connections via SO_ORIGINAL_DST and tunnels them through the same outbound auth pipeline as the CONNECT path (shared recordTunnelOpened/tunnel helpers), emitting no proxy-protocol bytes so the agent's end-to-end TLS is preserved. enforce-drop becomes enforce-redirect (nat REDIRECT for TCP + mangle DROP for non-TCP, position-1 to preempt ambient).

Why it's in good shape:

  • The design reasoning is captured where it matters: the nat-vs-mangle split (nat forbids DROP), the OUTPUT hook ordering for ambient preemption, SNI/Host sniffing with the IP kept as ground truth, and the explicit trust caveat that name-based policy is reliable against a cooperative agent but not a hostile one.
  • Tests are meaningful: the sockaddr parser is pure + table-driven, TestHandleTransparentConn_Tunnels proves no 200 Connection Established leaks (the agent's first bytes are the upstream's), sniff tests cover TLS/HTTP/opaque, and the unshare --net harness proves capture + preemption + non-TCP drop via packet counters.
  • go.mod change is clean: golang.org/x/sys promoted indirect→direct at the same v0.42.0, no go.sum churn.
  • Bind failures are fatal (good — a dead transparent port would otherwise break all egress silently); nil ConnHandler degrades to no-capture rather than panicking.
  • I agree with declining the SSH-port exemption: the listener blind-tunnels any TCP, so SSH still works end-to-end, and exempting :22 would punch exactly the bypass hole enforce-redirect exists to close.

Areas reviewed: Go (listeners, entrypoints, config), Shell (init-iptables.sh + netns test), Docs (README), go.mod.
Commits: 6, all signed-off, conventional prefixes.
CI: all green.

One optional defense-in-depth note inline on the self-loop guard. No blocking issues.

// means a direct dial to the listener port, where SO_ORIGINAL_DST reports the
// listener's own address — tunnelling to it would spiral into ever more
// connections/goroutines. Drop it.
if host, _, splitErr := net.SplitHostPort(dst); splitErr == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): the loopback-only self-loop guard is correct for the realistic cases — a direct dial to the listener with no DNAT record already fails originalDst() and is dropped above, and traffic to the pod's own IP is RETURNed by the CLUSTER_CIDRS rule before the REDIRECT, so it never reaches here.

The one caveat: that pod-IP safety is load-bearing on CLUSTER_CIDRS being set correctly, which the README itself flags as a required operator override off Kind. As cheap insurance against a misconfigured CLUSTER_CIDRS, you could also drop when dst equals the listener's own conn.LocalAddr() (host+port), matching CodeRabbit's suggested diff in the sibling thread. Entirely optional — your call whether the extra branch earns its keep given the iptables-level protection is the primary control.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huang195 to be clear, this is a nice-to-have, not something I'd hold the merge on — the iptables layer is doing the real work here. 防患于未然 (fáng huàn yú wèi rán, "guard against trouble before it arises"): the extra LocalAddr() check only matters in the misconfigured-CLUSTER_CIDRS world, so it's purely belt-and-suspenders. Happy either way — feel free to resolve if you'd rather keep the guard lean. 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 178c66e — added the dst == conn.LocalAddr() check alongside the loopback drop, so a podIP:8082 self-dial is caught even if CLUSTER_CIDRS is misconfigured. Agreed it's belt-and-suspenders behind the iptables RETURN, but it's a cheap branch and removes the load-bearing dependency on CLUSTER_CIDRS for the self-loop case. Thanks for the catch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

防患于未然 indeed — added it. The extra LocalAddr() check is one line and only fires in the misconfigured-CIDR world, so the guard stays lean while no longer relying solely on CLUSTER_CIDRS for self-loop protection. 👍

Extend the self-loop guard (review follow-up from mrsabath + CodeRabbit): in
addition to a loopback original destination, drop when the recovered dst equals
the connection's own LocalAddr (host:port). This covers a podIP:<port> self-dial
that could slip past the iptables CLUSTER_CIDRS RETURN if CLUSTER_CIDRS is
misconfigured — belt-and-suspenders behind the iptables layer, which remains the
primary control.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants