[client] Add dual-stack iptables manager with ip6tables support#5708
Open
lixmal wants to merge 5 commits intoclient-ipv6-nftablesfrom
Open
[client] Add dual-stack iptables manager with ip6tables support#5708lixmal wants to merge 5 commits intoclient-ipv6-nftablesfrom
lixmal wants to merge 5 commits intoclient-ipv6-nftablesfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds dual-stack (IPv6) support across firewall, routing, and related components, plus CIDR-aware IP anonymization and several address/formatting adjustments; tests expanded for IPv6 compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client CLI
participant Manager as Firewall Manager
participant RouterV4 as Router (v4)
participant RouterV6 as Router (v6)
participant IPTables as iptables/ip6tables
CLI->>Manager: AddPeerFiltering(peerIP)
alt peerIP is IPv6 and v6 enabled
Manager->>RouterV6: applyPeerFiltering(peerIP)
RouterV6->>IPTables: create/update rules (proto=ipv6)
else peerIP is IPv4 or v6 not enabled
Manager->>RouterV4: applyPeerFiltering(peerIP)
RouterV4->>IPTables: create/update rules (proto=ipv4)
end
Manager-->>CLI: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
7 tasks
lixmal
force-pushed
the
client-ipv6-iptables
branch
from
e682cf0 to
cb35e33
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
lixmal
force-pushed
the
client-ipv6-iptables
branch
from
cb35e33 to
8ef22c8
Compare
This was referenced Mar 26, 2026
lixmal
force-pushed
the
client-ipv6-iptables
branch
from
8ef22c8 to
f79dcee
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
client/internal/lazyconn/activity/listener_bind.go (1)
86-96: Minor optimization: avoid duplicatewgIface.Address()call.
wgIface.Address()is called at line 66 (for.Network) and again at line 87 (for.IPv6Net). Consider storing the full address result once to avoid the redundant call.♻️ Suggested refactor
func deriveFakeIP(wgIface WgInterface, allowedIPs []netip.Prefix) (netip.Addr, error) { if len(allowedIPs) == 0 { return netip.Addr{}, fmt.Errorf("no allowed IPs for peer") } - ourNetwork := wgIface.Address().Network + addr := wgIface.Address() + ourNetwork := addr.Network // Try v4 first (preferred: deterministic from overlay IP) var peerIP netip.Addr for _, allowedIP := range allowedIPs { ip := allowedIP.Addr() if !ip.Is4() { continue } if ourNetwork.Contains(ip) { peerIP = ip break } } if peerIP.IsValid() { octets := peerIP.As4() return netip.AddrFrom4([4]byte{127, 2, octets[2], octets[3]}), nil } // Fallback: use last two bytes of first v6 overlay IP - addr := wgIface.Address() if addr.IPv6Net.IsValid() { for _, allowedIP := range allowedIPs {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/lazyconn/activity/listener_bind.go` around lines 86 - 96, Store the result of wgIface.Address() once into a local variable (already named addr in the diff) before the IPv6 fallback block and reuse it instead of calling wgIface.Address() again; update the code that checks addr.IPv6Net.IsValid() and later uses addr.IPv6Net.Contains(ip) to reference that cached addr, leaving the allowedIPs loop and netip.AddrFrom4([4]byte{127, 2, raw[14], raw[15]}) logic unchanged.
🤖 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 1257-1268: The code currently swallows iptables errors when
falling back to nftables; change the logic in the iptables/nftables cleanup path
so that any iptables-related error is not masked when the nftables fallback
succeeds. Specifically, in the block that calls
iptables.NewWithProtocol(r.iptablesProto()) and in the block that calls
r.removeAcceptFilterRulesIptables(ipt), attempt the nftables cleanup via
r.removeAcceptRulesFromTable(r.filterTable) but if that nftables call succeeds
still return the original iptables error (or return an aggregated error
combining the iptables error and the nftables result) instead of returning nil;
reference the symbols iptables.NewWithProtocol, r.iptablesProto,
r.removeAcceptFilterRulesIptables, and r.removeAcceptRulesFromTable when making
the change.
In `@client/firewall/uspfilter/allow_netbird_windows.go`:
- Around line 32-41: The current cleanup returns immediately on the first
manageFirewallRule error, so deleting the IPv4 rule can short-circuit IPv6
removal; change the logic in the shutdown/cleanup function that calls
isFirewallRuleActive and manageFirewallRule (using firewallRuleName and
firewallRuleName+"-v6" with deleteRule) to attempt both deletes regardless of
the first error, collect any errors (e.g., append to a slice or wrap with
fmt.Errorf) and after both attempts return either nil if none failed or a
combined error describing which deletions failed; ensure both calls to
manageFirewallRule are executed even if the first returns an error.
In `@client/firewall/uspfilter/filter_test.go`:
- Around line 1408-1430: The new dual-stack subtests are environment-dependent
because Create() sets manager.netstack from netstack.IsEnabled(), so set
manager.netstack = false in each v6Cases subtest (before calling shouldForward)
to force non-netstack behavior; specifically, after setting
manager.localForwarding = true in the t.Run body, assign manager.netstack =
false so shouldForward will exercise the IPv4/IPv6 destination comparison paths
added.
In `@client/iface/wgproxy/bind/proxy.go`:
- Around line 202-205: The function fakeAddress currently dereferences
peerAddress.IP without checking for nil; update fakeAddress to first guard
against a nil peerAddress and return a clear error (e.g., "nil peerAddress") if
it is nil, then proceed to call netip.AddrFromSlice(peerAddress.IP) and handle
the existing invalid-IP error path—ensure the check references the fakeAddress
parameter name peerAddress so the panic is avoided.
In `@client/internal/debug/debug_test.go`:
- Around line 556-580: The test uses a hardcoded IPv6 anonymization prefix
("100::") which is brittle; instead retrieve the configured default IPv6 base
from anonymize.DefaultAddresses() (or from the anonymizer used to create
anonNftables/anonIp6tablesSave) and use that value in the assertions—replace the
two assert.Contains checks that reference "100::" with assertions that check for
the actual DefaultAddresses().IPv6Base (or the anonymizer's IPv6 base) so the
test stays correct if the default prefix changes.
In `@client/internal/engine.go`:
- Around line 2355-2357: blockLanAccess() currently only uses an IPv4 source
prefix (v4 := netip.PrefixFrom(netip.IPv4Unspecified(), 0)) so IPv6 LAN prefixes
from getInterfacePrefixes() are never matched; update blockLanAccess() to create
both v4 and v6 unspecified source prefixes (IPv4Unspecified/IPv6Unspecified) and
when iterating the interface prefixes choose the matching source prefix based on
each prefix's address family (e.g., prefix.Addr().Is4() / Is6()) and apply the
corresponding rule for IPv4 vs IPv6 destinations so IPv6 LAN prefixes are
properly blocked.
In `@client/internal/routemanager/systemops/systemops.go`:
- Around line 110-118: The current check only tests prefix.Addr() against
isOwnAddress(addr); update validation to reject any candidate prefix that
overlaps the WireGuard address/prefix, not just matching base addresses: modify
validateRoute (or isOwnAddress) to accept the candidate netip.Prefix and check
both directions for overlap using the wg interface address/prefix from
r.wgInterface.Address() — e.g., reject when
candidatePrefix.Contains(wgAddr.Addr()) OR
wgAddr.Network.Contains(candidatePrefix.Addr()), and for IPv6 also check
wgAddr.IPv6Net.IsValid() && (candidatePrefix.Contains(wgAddr.IPv6Net.Addr()) ||
wgAddr.IPv6Net.Contains(candidatePrefix.Addr())); return vars.ErrRouteNotAllowed
when any overlap is found.
---
Nitpick comments:
In `@client/internal/lazyconn/activity/listener_bind.go`:
- Around line 86-96: Store the result of wgIface.Address() once into a local
variable (already named addr in the diff) before the IPv6 fallback block and
reuse it instead of calling wgIface.Address() again; update the code that checks
addr.IPv6Net.IsValid() and later uses addr.IPv6Net.Contains(ip) to reference
that cached addr, leaving the allowedIPs loop and netip.AddrFrom4([4]byte{127,
2, raw[14], raw[15]}) logic unchanged.
🪄 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: a8268a42-1c96-456a-ad15-3dac2fd76962
📒 Files selected for processing (28)
client/anonymize/anonymize.goclient/cmd/ssh.goclient/cmd/ssh_test.goclient/firewall/iptables/acl_linux.goclient/firewall/iptables/manager_linux.goclient/firewall/iptables/router_linux.goclient/firewall/iptables/rule.goclient/firewall/iptables/state_linux.goclient/firewall/nftables/manager_linux.goclient/firewall/nftables/manager_linux_test.goclient/firewall/nftables/router_linux.goclient/firewall/uspfilter/allow_netbird_windows.goclient/firewall/uspfilter/conntrack/common.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/filter_test.goclient/firewall/uspfilter/localip_test.goclient/iface/configurer/usp.goclient/iface/wgproxy/bind/proxy.goclient/internal/debug/debug_test.goclient/internal/dns/service_listener.goclient/internal/dnsfwd/manager.goclient/internal/engine.goclient/internal/lazyconn/activity/listener_bind.goclient/internal/routemanager/server/server.goclient/internal/routemanager/systemops/systemops.goclient/internal/routemanager/systemops/systemops_generic.goproxy/internal/debug/handler.goshared/relay/client/dialer/quic/quic.go
💤 Files with no reviewable changes (1)
- client/firewall/uspfilter/localip_test.go
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
lixmal
force-pushed
the
client-ipv6-iptables
branch
from
076b6c6 to
dacdd7f
Compare
lixmal
force-pushed
the
client-ipv6-iptables
branch
2 times, most recently
from
4e7f1b3 to
84b6716
Compare
lixmal
force-pushed
the
client-ipv6-iptables
branch
from
84b6716 to
faef5a5
Compare
|
7 tasks
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
1 participant
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.
Describe your changes
iptables
ipv6Client/aclMgr6/router6in the iptables manager, dispatching operations by address family-v6suffix for ip6 ipset names (kernel ipsets are global, need family separation)Misc
blockLanAccessto use matching source family for IPv6 LAN prefixesshouldForwardin USP filter for dual-stack address comparisonfakeAddressfor v6 peer address handlingnormalizeLocalHost("*")returns""for dual-stack listeningStacked on #5707.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#594