Repository navigation
Test: Pin the zoned NAT64 refusal and correct the zero-value Guard doc - #1556
Merged
Merged
Conversation
Arms an opt-in SSRF dial guard on every outbound dial the forward proxy
makes: CONNECT tunnels, plain and bridged requests, transparent redirects,
and skip_hosts traffic too — a bypass must not become an SSRF bypass. Each
destination is resolved once, refused with 403 destination_private when the
address is loopback, link-local (including 169.254.169.254), RFC 1918 /
unique-local space, or one of this host's own interface addresses and no
permit covers it, and then dialed at the checked address, so DNS rebinding
cannot re-answer between the check and the connect. Link-local, unspecified
and multicast addresses are never permitted, whatever permit_cidrs says.
Off by default: a sidecar's ordinary upstreams — Keycloak, in-cluster
services, a laptop's local model — live in exactly the refused space, so
defaulting this on would break every existing deployment.
- core/listener/egressguard: the guard itself (CheckAddr / DialContext /
DialTimeout, typed Refusal), resolver injectable so the tests pin the
resolve-once property against a counting stub
- forwardproxy: WithEgressGuard option (NewServer signature unchanged);
under mTLS the guard sits inside the dialer so a refused address never
gets a TLS ClientHello; handleConnect and the transparent listener check
before the dial and reuse the checked address for the bridge fall-open
re-dial; serveOutbound renders a Refusal as the listener's standard 403
rejection with error.kind destination_private
- pipeline: SessionEvent.DialAddr on the wire (dialAddr, omitempty) — the
audit trail pairing the name the client asked for with the address
dialed; new tunnelReason "dest-private" (within agentop's 18-char PLUGIN
column cap)
- tlsbridge: WithDialContext so the bridge's re-origination client dials
through the guard — the bridge must not become the path around it
- config: listener.egress_guard {enabled, permit_cidrs}; a listener.*
field, so it inherits the restart-on-reload rule
The moca conformance shapes are pinned by tests: private_address_403
(CONNECT and GET to loopback, upstream never dialed),
metadata_address_403_even_if_permitted (169.254.169.254 under 0.0.0.0/0),
and the rebinding case's Cortex half — resolve exactly once, dial the
first checked answer, open row carries dialAddr.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Self-review of #1539, both accuracy rather than behavior: - SessionEvent.DialAddr's comment claimed the field is absent "on events that dialed nothing", but a guarded plain or bridged request dials through the guarded transport and records no dialAddr either — only a tunnel's open row carries it. Say that instead: present only on a tunnel's open row, absent when the guard is off. session-api.md already said exactly this. - The "egress guard on" boot log fired wherever the guard was configured, including a reverse-only deployment where no forward proxy exists to enforce it. The guard is still built before the TLS bridge (the bridge's re-origination client consumes it), but the announcement now logs in the forward-role block, where it is true. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
#1484) CodeQL on #1539 flagged "Log entries created from user input" at the serveOutbound refusal Debug: r.Host and r.Method are client-controlled, and a control rune forged into either would forge a log entry — the same CWE-150 shape the User-Agent path already guards against. - Rename pipeline.sanitizeUA to pipeline.SanitizeControlRunes. The rule (C0/DEL/C1/bidi/zero-width and invalid UTF-8 replaced with U+FFFD) and its rationale are unchanged; only the name now says what it is: the rule applies to any client-controlled string headed for a surface a terminal or a file renders, not only a User-Agent. Exported so the listeners can use the one copy rather than grow a third loop beside the ledger's sanitizeLabel. - Apply it at every log site this PR added: the serveOutbound refusal Debug (Host, Method), handleConnect's refusal Warn (Host) and resolution-failure Warn (Host and the error string, which embeds the name via net.DNSError), and the transparent listener's refusal Warn (host — sniffed off the client's own bytes as SNI or a Host header). Values that are not client-controlled — the guard's parsed address, the kernel's SO_ORIGINAL_DST literal — stay raw. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Hai's review of #1539, all six findings: - countsAsError only exempted upgrade_declined, so /v1/usage counted the guard's 403 destination_private as an error while four places of documentation said it does not. The kind is now exempted alongside upgrade_declined — a refusal is the policy working, not the proxy failing — with a test next to the declined-upgrade one that pins the distinction from an origin's own 403. - The "host's own addresses" check tested the interface's on-link PREFIX, refusing every LAN neighbour and, on a cloud VM with a public address on a large on-link prefix, ordinary public upstreams. hostOwns now compares the interface's address itself; a test pins the distinction with the exact prefixes from the review. - fd00:ec2::254 (AWS metadata, IPv6 form) is ULA and so sat in the permit-able set; it is never-permitted now. 100.64.0.0/10 (RFC 6598 CGNAT, home of Alibaba's metadata service) is refused like private space and permit-able like it. A well-known-prefix NAT64 address (64:ff9b::/96) is checked by the IPv4 it embeds, so the wrapped 127.0.0.1 is refused while the v6 form stays the dial target. All three are named in the package comment and the docs. - DialTimeout had no caller; deleted. - The port is parsed with net.LookupPort before the resolve: a service-name port ("example.com:https") dials again instead of 502ing, and a bad port costs no DNS lookup. - The SanitizeControlRunes doc block opened with "sanitizeUA replaces…", a function that no longer exists; the rename paragraph now leads the block and the seven other comment references follow the new name. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Three conflicts, all additive — main and this branch each appended to the same event-schema prose and the same projection count. - `core/sessionapi/summary_test.go`: main added `Ttfb` (#1251) and this branch added `DialAddr`, each as the 29th kept field. Both are kept, so the count is 30 and both comments stand. - `CLAUDE.md`: main rewrote the `bytesUp`/`bytesDown` bullet (a relayed response is counted now, with extproc's body-free arm the one exception) and added `ttfbMs`; this branch added the guard's `dest-private` sentence to `tunnelReason` and the `dialAddr` bullet. Took main's rewrite verbatim and re-applied both guard additions on top. - `docs/session-api.md`: same shape — main's `ttfbMs` bullet plus this branch's `destination_private` sentences. Verified: CGO_ENABLED=0 go build ./... clean; sessionapi, pipeline, egressguard, forwardproxy and cost/usage tests pass. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
…rent refusal (#1484) Two review findings from the CodeRabbit pass, each confirmed against the code before fixing and each with a test that fails without its fix. **A zoned IPv6 literal reached the host's own address.** netip.Addr equality includes the zone, and net.InterfaceAddrs reports the host's addresses unzoned, so `[2001:db8::5%eth0]` compared unequal to the `2001:db8::5` the host owns and walked past hostOwns — writing a zone the host itself would ignore was enough. CheckAddr now drops the zone for the CHECK and keeps it for the DIAL, since a link-scoped destination needs it to pick an interface. hostOwns was the whole exposure: loopback, link-local, private and CGNAT are range predicates, and a zone does not move an address out of a range (pinned by TestCheckAddr_ZonedV6StillRefused). Equality is the one place a zone bites. Stripping it also fixes a false refusal in the other direction — a permitted zoned ULA was being refused, because Prefix.Contains is false for any zoned address, so no permit could ever match one. The NAT64 decode now runs on the zone-stripped address too, which is strictly better: nat64Prefix.Contains was false for a zoned address, so a zoned NAT64 form previously skipped the embedded-v4 check entirely. Tests go through CheckAddr, not hostOwns: a test that strips the zone itself before calling hostOwns asserts the fix rather than the path, and passes against the unfixed code — I wrote that version first and it did. Verified both new tests fail before the fix (`= "[2001:db8::5%eth0]:443" with no refusal`) and pass after. `ifaceAddrs` joins `resolve` as a test seam, because the host-owned rule is the one refusal that depends on the machine the test runs on — on this laptop the only host-owned non-private address is IPv4, which cannot carry a zone, so the bug was untestable without it. **The transparent listener's refusal reported an error to Finishers.** handleConnect sets pctx.StatusCode = 403 before its tl.close; this path did not, and OutcomeFromContext reads a zero StatusCode as OutcomeError. So the three listener paths disagreed about the same refusal, this one reporting a failure for what CLAUDE.md, docs/session-api.md and framework-architecture.md all call a delivered 403 the Finisher sees as an allow. Nothing noticed because no status reaches the client on this path — it speaks no HTTP to the agent — so the field exists only for the Finishers and the row. Verified: full core suite, 73 packages, 0 failures. go vet and gofmt clean on every touched package. agentop's failures (launchd/systemd/service-manager and two server-picker TUI tests) reproduce identically on clean main — sandbox limitations, not this change. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
…1484) Both round-2 review comments. Neither blocks, but the first is a real coverage gap in my own previous commit. **The NAT64 half of the zone fix was unpinned.** 3873b1c moved the embedded-v4 decode onto the zone-stripped address, and I said so in the PR thread, but nothing tested it: reverting `nat64Embedded(checkAddr)` to `nat64Embedded(dialAddr)` left every test in egressguard and forwardproxy green. Confirmed, and confirmed what it costs — with the decode's input reverted, `[64:ff9b::7f00:1%eth0]:80` and `[64:ff9b::a9fe:a9fe%eth0]:80` are both ALLOWED, which is loopback and the cloud metadata service, while their unzoned forms are refused. The metadata one is never-permitted space, so a written zone was enough to reach it. Both addresses are now rows in `TestCheckAddr_ZonedV6StillRefused`, with the measurement in the comment. The two halves are one fix rather than two — `nat64Prefix.Contains` is false for a zoned address, so before the zone was stripped the decode never ran on one — and the test now says that, so a later edit cannot quietly separate them. **The zero-value Guard doc said the opposite of the truth**, and contradicted the fallback comment I added in the same commit. A zero `Guard{}` does refuse a loopback, private or metadata LITERAL: `check` runs and an empty permit list opens nothing. Only names fail, on the nil resolver. Verified all four cases. The doc now says "checks literal addresses but cannot resolve names" and names the public-literal and name behaviours; the fallback comment drops its "built before this field existed" clause, which was describing something that cannot happen at runtime. Verified: full core suite, 73 packages, 0 failures; go vet and gofmt clean. The two new rows fail against the pre-fix decode input and pass after. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 31 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolves the conflict with main, which has since merged #1539 (the forward proxy dial refusal) and #1537 (the plaintext-credential refusal). Both conflicts were add/add in core/listener/egressguard and both resolved to this branch's side, which is the change the PR exists to make: - guard.go: the corrected zero-value Guard doc. The old "refuses nothing and dials nothing" text on main is what this branch set out to fix, so main's side is the stale one. - guard_test.go: the two zoned NAT64 rows and the comment recording what they measured. Additive; main's side simply predates them. The branch was cut before #1537, so merging it without this would have deleted that feature — the plaintext gate, its three test files, the staticinject plugin changes and its doc, about 600 lines. Those merged cleanly and are kept; the diff against main is now only the 24 lines this PR actually adds. Verified: full core suite with CGO_ENABLED=0, 0 failures, including the staticinject, extproc and reverseproxy packages #1537 brought in; go vet and gofmt clean on the touched packages. TestCheckAddr_ZonedV6StillRefused passes with the two NAT64 rows. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Alan-Cha
approved these changes
Oct 29, 2026
Alan-Cha
left a comment
Alan-Cha
left a comment
Member
There was a problem hiding this comment.
Summary
Verified both changes on the PR head. I reverted only the NAT64 decode's input to the zoned address: the two new rows fail ([64:ff9b::7f00:1%eth0] and [64:ff9b::a9fe:a9fe%eth0] allowed), and with them removed every other egressguard and forwardproxy test stays green, so the rows close a real gap. Reverting the zone strip also trips them, which bears out the "one fix, not two" comment. The new zero-value doc matches a zero Guard{}: loopback, private and metadata literals refused, a public literal allowed, a name failing with no resolver configured.
Author: mrsabath (MEMBER — maintainer)
Areas reviewed: Go (core/listener/egressguard)
Agent/IDE config (.claude/.vscode): none
Commits: 8 commits, all signed-off: yes
CI status: passing
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1539, addressing @huang195's two round-2 comments. Both were marked
non-blocking, so #1539 merged on its own approval and these land separately.
Now based on
main. This originally stacked onfix/sbx-ssrf-guard(#1539),because the test pins code #1539 introduces and so could not compile against main
until it merged. #1539 has since merged and this branch is retargeted: it is 0
commits behind
main, and the net diff againstmainis exactly the two filesbelow.
The NAT64 pin was a real gap in #1539
#1539 moved the embedded-v4 decode onto the zone-stripped address
(
nat64Embedded(checkAddr)), and I described that in the review thread, butnothing tested it. @huang195 spotted that reverting it leaves every test in
egressguardandforwardproxygreen.Confirmed, and confirmed what it costs. With the decode's input reverted to the
zoned address:
[64:ff9b::7f00:1%eth0]:80(127.0.0.1)[64:ff9b::a9fe:a9fe%eth0]:80(169.254.169.254)[64:ff9b::7f00:1]:80(unzoned control)[64:ff9b::a9fe:a9fe]:80(unzoned control)So a written zone reached loopback and the cloud metadata service — and the
metadata address is never-permitted space, which no
permit_cidrssetting issupposed to be able to open.
Both addresses are now rows in
TestCheckAddr_ZonedV6StillRefused, with themeasurement in the comment. The zone-strip and the NAT64 decode are one fix,
not two:
nat64Prefix.Containsis false for a zoned address, so before thezone was stripped the decode never ran on one. The test says that explicitly, so
a later edit cannot quietly separate them.
The zero-value Guard doc was wrong
It read "the zero value refuses nothing and dials nothing", which contradicted
the fallback comment added in the same commit ("a zero-value Guard … still asks
the host"). @huang195 was right that the doc is the one that's wrong. Verified:
Guard{}127.0.0.1:44310.0.0.5:443169.254.169.254:8093.184.216.34:443(public)example.com:443(name)egressguard: no resolver configuredcheckruns normally; an empty permit list opens nothing. Only names fail, onthe nil resolver. The doc now says "checks literal addresses but cannot resolve
names" and spells out all three behaviours. The fallback comment drops its
"built before this field existed" clause — a struct cannot predate its own field
at runtime, as the review noted.
Test plan
= nil, want a refusalon both) and pass aftercoresuite: 73 packages, 0 failuresgo vetandgofmtcleanNo production behaviour changes here — #1539 already fixed the guard. This adds
the test that keeps it fixed, and corrects a doc comment.
Refs #1484
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com