[client] Fix exit node masquerade overrides per-route masquerade=false setting#5682
[client] Fix exit node masquerade overrides per-route masquerade=false setting#5682tobsec wants to merge 6 commits intonetbirdio:mainfrom
Conversation
When a routing peer acts as both an exit node (0.0.0.0/0) and a routing peer for specific subnets with masquerade disabled, the exit node's addNatRule() generates a catch-all prerouting mark rule matching all wt0 traffic. The blanket masquerade in netbird-rt-postrouting then fires on every packet regardless of per-route masquerade settings. Fix: when AddNatRule() is called with pair.Masquerade=false, insert a return verdict rule at the head of the postrouting NAT chain for that specific destination. Since InsertRule/Insert places it at position 0, it fires before the catch-all masquerade rule, causing the packet to skip masquerading and preserving the original source IP. Applied to both nftables and iptables backends. Related: netbirdio#2751 Related: netbirdio#4542
|
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds per-route "no-masquerade" postrouting rules: when a route has Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Router as Router (iptables/nftables)
participant Netfilter as Netfilter (postrouting chain)
participant Ipset as IpsetManager
Caller->>Router: AddNatRule(pair)
alt pair.Masquerade == true
Router->>Netfilter: insert MASQUERADE postrouting rules (in/out)
Router->>Ipset: increment set counters (if using ipsets)
else pair.Masquerade == false
Router->>Router: compute ruleKey, remove existing tracked rule
Router->>Netfilter: insert "-d <dest> -j RETURN" at head (both directions)
Router->>Ipset: increment/track set counters for destinations
end
Caller->>Router: RemoveNatRule(pair)
alt tracked RETURN rule exists
Router->>Netfilter: delete RETURN rule(s)
Router->>Ipset: decrement set counters and remove tracking
else missing or Handle==0
Router-->>Caller: log and no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/nftables/router_linux.go`:
- Around line 774-801: The rollback path fails to decrement ipset refs when
addNoMasqPostRoutingRule increments them for set destinations; update
rollbackRules to handle firewall.NoMasqPostRoutingFormat so that when AddNatRule
fails after addNoMasqPostRoutingRule has succeeded you call the same ipset
decrement logic used for ForwardingFormat/PreroutingFormat. Locate rollbackRules
and add a branch for NoMasqPostRoutingFormat that looks up the rule key in
r.rules, calls r.getIpSet()/network.IsSet() reversal (the same decrement/cleanup
used by addNatRule's rollback), and removes the rule entry from r.rules to
ensure counters and state are consistent.
- Around line 1415-1428: The removal block for the no-masquerade postrouting
rule is missing the stale-handle check used elsewhere; before calling
r.conn.DelRule or decrementSetCounter in the current block (the branch handling
rule, exists := r.rules[ruleKey]), check if rule.Handle == 0 and if so log a
debug message, delete the entry from r.rules and skip calling r.conn.DelRule or
r.decrementSetCounter (treat it as a stale/unflushed rule), otherwise proceed
with DelRule and decrementSetCounter as currently implemented; mirror the
behavior found in removeNatRule/DeleteRouteRule and reference the symbols
rule.Handle, r.conn.DelRule, r.decrementSetCounter, r.rules, and ruleKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42c3be8b-37e6-4f3f-81fb-67efe9ddc325
📒 Files selected for processing (3)
client/firewall/iptables/router_linux.goclient/firewall/manager/firewall.goclient/firewall/nftables/router_linux.go
- Move idempotency check to top of nftables addNoMasqPostRoutingRule, before applyNetwork() call, consistent with iptables version - Remove redundant updateState() from iptables addNoMasqPostRoutingRule; the caller AddNatRule already calls it unconditionally - Use deleteNftRule helper in nftables RemoveNatRule for the no-masq path, consistent with how other rules are removed in the same file
- Add NoMasqPostRoutingFormat to rollbackRules key list so the no-masquerade RETURN rule is cleaned up when conn.Flush() fails after addNoMasqPostRoutingRule has already written to r.rules - Add rule.Handle == 0 stale-entry guard in RemoveNatRule no-masq path, consistent with the same check in removeNatRule
|
Both points addressed in d007a62:
|
…lexity RemoveNatRule exceeded SonarCloud's cognitive complexity threshold of 20 after the no-masquerade path was added inline. Extract the logic into a dedicated removeNoMasqPostRoutingRule helper, mirroring the structure of removeNatRule.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
787-799: Scope the RETURN rule to the exit-node mark.
AddNatRule()installs this for everymasquerade=falsepair, but the match here is destination-only. That means any packet to the prefix exits the wholenetbird-rt-postroutingchain, even when no exit-node mark is present. Since this chain also hosts other NAT work, e.g.addDnatMasq()at Lines 1711-1748, matchingnbnet.PreroutingFwmarkMasqueradefirst would keep the opt-out limited to the exit-node path this PR is fixing.🎯 Suggested tightening
- exprs := append(destExp, &expr.Verdict{Kind: expr.VerdictReturn}) + exprs := []expr.Any{ + &expr.Meta{ + Key: expr.MetaKeyMARK, + Register: 1, + }, + &expr.Cmp{ + Op: expr.CmpOpEq, + Register: 1, + Data: binaryutil.NativeEndian.PutUint32(nbnet.PreroutingFwmarkMasquerade), + }, + } + exprs = append(exprs, destExp...) + exprs = append(exprs, &expr.Verdict{Kind: expr.VerdictReturn})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/nftables/router_linux.go` around lines 787 - 799, The RETURN verdict is currently unconditional for destination-only matches and will exit the entire netbird-rt-postrouting chain; tighten it by scoping the RETURN to packets with the exit-node mark: in AddNatRule (the block creating exprs with destExp and expr.Verdict{Kind: expr.VerdictReturn}), prepend a match expression that tests for nbnet.PreroutingFwmarkMasquerade (the prerouting/masquerade fwmark) before appending expr.VerdictReturn so the rule only returns when the fwmark is present; update the exprs built for nftables.Rule (the r.conn.InsertRule call referencing r.workTable, r.chains[chainNameRoutingNat], and ruleKey) to include that mark-match expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/nftables/router_linux.go`:
- Around line 782-785: The code currently returns early when ruleKey :=
firewall.GenKey(firewall.NoMasqPostRoutingFormat, pair) already exists in
r.rules, which leaves outdated destination matches in place; change this to
replace the existing no-masq rule instead of returning: detect the collision on
ruleKey, remove/uninstall the old rule entry from r.rules and from the active
ruleset, then continue with the normal install/registration flow (same approach
as addNatRule() and addLegacyRouteRule()) so the new destination match is
applied and r.rules is updated to the new rule for the given pair.
---
Nitpick comments:
In `@client/firewall/nftables/router_linux.go`:
- Around line 787-799: The RETURN verdict is currently unconditional for
destination-only matches and will exit the entire netbird-rt-postrouting chain;
tighten it by scoping the RETURN to packets with the exit-node mark: in
AddNatRule (the block creating exprs with destExp and expr.Verdict{Kind:
expr.VerdictReturn}), prepend a match expression that tests for
nbnet.PreroutingFwmarkMasquerade (the prerouting/masquerade fwmark) before
appending expr.VerdictReturn so the rule only returns when the fwmark is
present; update the exprs built for nftables.Rule (the r.conn.InsertRule call
referencing r.workTable, r.chains[chainNameRoutingNat], and ruleKey) to include
that mark-match expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fee3e00e-9d62-4696-98ef-a690d64a3289
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go
|
Good feature, but the PR has 2 flaws:
The more specific 10/24 would be swallowed by the exemption of 10/16 |
|
Thanks for the review, @lixmal — both points are valid. Issue 1 (inverse direction): Agreed. The masquerade=ON path calls Issue 2 (prefix overlap): This is the trickier one. The root cause is that masquerade=ON routes currently only add a prerouting mark rule — there is no corresponding postrouting rule per destination. The blanket postrouting masquerade fires for anything with the mark. Because of that, a RETURN in postrouting for a less-specific masquerade=OFF prefix will shadow more-specific masquerade=ON prefixes, regardless of insertion order. The correct fix seems to be: masquerade=ON routes should also get explicit per-destination postrouting rules (mark + destination → masquerade), and all postrouting rules should be ordered most-specific-first so the longest prefix match wins. This means changing the masquerade=ON path as well — a bigger refactor than the current PR. Before going down that path, does the project have a preferred approach for this? For example:
Happy to rework the implementation once there's consensus on the right direction. |
addNoMasqPostRoutingRule now matches the correct fwmark per direction: forward traffic (wt0→eth0) uses PreroutingFwmarkMasquerade (0x1BD21), inverse traffic (eth0→wt0) uses PreroutingFwmarkMasqueradeReturn (0x1BD22). AddNatRule and RemoveNatRule call the helper for both the forward and inverse RouterPair so that neither direction is masqueraded when the route has masquerade=false. rollbackRules also covers the inverse pair key to avoid counter leaks on flush failure. A removeNoMasqPostRoutingRule helper is extracted in the iptables backend (matching the existing nftables helper) and the idempotency path now replaces the existing rule instead of returning early.
tobsec
changed the title
|
Docs change: not needed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/iptables/router_linux.go`:
- Around line 528-533: When removing an existing NAT no-masq rule in the block
that checks r.rules[ruleKey], also decrement the ipset usage counter to avoid
leaks: call r.decrementSetCounter(rule) before deleting the rule entry. Locate
the code handling r.rules[ruleKey] (the branch that calls
r.iptablesClient.DeleteIfExists(tableNat, chainRTNAT, rule...)) and mirror the
behavior from removeNoMasqPostRoutingRule by invoking
r.decrementSetCounter(rule) prior to delete(r.rules, ruleKey) so applyNetwork
(and subsequent increments) do not cause counter drift.
🪄 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
Run ID: f241fa43-30ca-4c2c-8420-c70fe1e23b1e
📒 Files selected for processing (2)
client/firewall/iptables/router_linux.goclient/firewall/nftables/router_linux.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/firewall/nftables/router_linux.go
The idempotency path in addNoMasqPostRoutingRule deleted the old rule but did not decrement the ipset usage counter, which could cause counter drift. Mirror the behavior from removeNoMasqPostRoutingRule.
|
|
Issue 1 (inverse direction): Addressed in 4754f31. Issue 2 (prefix overlap): Agreed this is a real concern. The root cause is that masquerade=ON only adds a prerouting mark but no per-destination postrouting rule, so there's nothing more-specific to override the less-specific RETURN. A proper fix likely requires explicit per-route postrouting masquerade rules ordered by prefix length (most-specific first). Happy to take direction on the preferred approach — should this be addressed in this PR or as a follow-up? |
3 participants
Problem
When a peer acts as both an exit node (
0.0.0.0/0) and a routing peer for specific subnets with masquerade disabled, the exit node's postrouting masquerade rule fires for all traffic — silently ignoring the per-routemasquerade=falsesetting.Root cause:
addNatRule()sets fwmarkPreroutingFwmarkMasqueradeon allwt0traffic for the exit node.addPostroutingRules()then masquerades everything carrying that mark, regardless of per-route settings. There is no mechanism for a per-routemasquerade=falseto opt out of this blanket rule.Practical effect: A client routing through an exit node cannot reach services that rely on the original source IP being preserved (e.g. firewall rules matching the client's NetBird IP, IPsec policies, per-device ACLs). All traffic appears to arrive from the routing peer's WireGuard interface IP instead.
This was reported in #2751 and is related to #4542.
Fix
When
masquerade=falsefor a route pair, insert aRETURNverdict at the head of the postrouting NAT chain (netbird-rt-postrouting) matching the route's destination prefix. This short-circuits evaluation before the exit node's catch-all masquerade rule fires.Both the nftables and iptables backends are updated:
NoMasqPostRoutingFormatinfirewall/manager/firewall.goaddNoMasqPostRoutingRule()in bothnftables/router_linux.goandiptables/router_linux.goAddNatRule()/RemoveNatRule()updated to manage the return rule whenmasquerade=falseThe rule is keyed and tracked in
r.rulesthe same way as existing NAT rules, so it is correctly removed on route deletion or peer disconnect.Test scenario
0.0.0.0/0, masquerade cannot be disabled)192.168.0.0/24with masquerade=OFF192.168.0.1192.168.0.1with src = peer B's WireGuard IP (masqueraded)Related issues
Summary by CodeRabbit
New Features
Bug Fixes