[management, client] Add IPv6 overlay support#5631
Merged
Conversation
lixmal
changed the title
lixmal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/netiputil/compact.go (1)
23-42: Consider validating prefix length bounds for robustness.The decoded prefix length (last byte) is not validated against valid bounds. For IPv4, valid bits are 0-32; for IPv6, 0-128. If the input contains out-of-range values,
netip.PrefixFromreturns an invalid prefix silently, requiring callers to checkIsValid().For a wire protocol decoder, explicit validation with a clear error message improves debuggability when malformed data is received.
🛡️ Proposed fix to validate prefix length
func DecodePrefix(b []byte) (netip.Prefix, error) { switch len(b) { case 5: var ip4 [4]byte copy(ip4[:], b) - return netip.PrefixFrom(netip.AddrFrom4(ip4), int(b[len(b)-1])), nil + bits := int(b[len(b)-1]) + if bits > 32 { + return netip.Prefix{}, fmt.Errorf("invalid IPv4 prefix length %d (max 32)", bits) + } + return netip.PrefixFrom(netip.AddrFrom4(ip4), bits), nil case 17: var ip6 [16]byte copy(ip6[:], b) addr := netip.AddrFrom16(ip6).Unmap() bits := int(b[len(b)-1]) + maxBits := 128 + if addr.Is4() { + maxBits = 32 + } + if bits > maxBits { + return netip.Prefix{}, fmt.Errorf("invalid prefix length %d (max %d)", bits, maxBits) + } if addr.Is4() && bits > 32 { bits = 32 } return netip.PrefixFrom(addr, bits), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/netiputil/compact.go` around lines 23 - 42, DecodePrefix does not validate the prefix length byte and can produce an invalid netip.Prefix; update DecodePrefix to read the trailing byte into bits and explicitly validate bounds: for the 5-byte (IPv4) case ensure 0 <= bits <= 32 (return a clear fmt.Errorf if not), and for the 17-byte (IPv6) case ensure 0 <= bits <= 128 (return error if out of range); keep the existing addr.Unmap() and the special case that an IPv6-mapped IPv4 address should not claim >32 bits (either validate before or after Unmap and return an error if inconsistent), and only call netip.PrefixFrom when bits are within the validated range so callers never receive silently invalid prefixes from DecodePrefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/netiputil/compact.go`:
- Around line 23-42: DecodePrefix does not validate the prefix length byte and
can produce an invalid netip.Prefix; update DecodePrefix to read the trailing
byte into bits and explicitly validate bounds: for the 5-byte (IPv4) case ensure
0 <= bits <= 32 (return a clear fmt.Errorf if not), and for the 17-byte (IPv6)
case ensure 0 <= bits <= 128 (return error if out of range); keep the existing
addr.Unmap() and the special case that an IPv6-mapped IPv4 address should not
claim >32 bits (either validate before or after Unmap and return an error if
inconsistent), and only call netip.PrefixFrom when bits are within the validated
range so callers never receive silently invalid prefixes from DecodePrefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c0facdd-fccf-46f0-b534-18bb5d96c612
📒 Files selected for processing (1)
shared/netiputil/compact.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/netiputil/compact.go (1)
16-21: Consider validating input prefix.The implementation is correct for valid prefixes. However, if an invalid/zero
netip.Prefixis passed,p.Bits()returns -1 (becomes 255 as byte) and a zeroAddrproduces a 1-byte result. Both would fail on decode, so it's fail-safe, but explicit validation could provide clearer error messages at the source.💡 Optional: Add validation for invalid prefixes
// EncodePrefix encodes a netip.Prefix into compact bytes. // The address is always unmapped before encoding. -func EncodePrefix(p netip.Prefix) []byte { +func EncodePrefix(p netip.Prefix) ([]byte, error) { + if !p.IsValid() { + return nil, fmt.Errorf("invalid prefix") + } addr := p.Addr().Unmap() - return append(addr.AsSlice(), byte(p.Bits())) + return append(addr.AsSlice(), byte(p.Bits())), nil }Note: This would require updating callers to handle the error. Given the current usage context where callers likely pass valid prefixes, keeping the current signature may be acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/netiputil/compact.go` around lines 16 - 21, Validate the input prefix in EncodePrefix by checking netip.Prefix validity and bit length before encoding: change EncodePrefix(p netip.Prefix) to return ([]byte, error), verify p.IsValid() and p.Bits() >= 0 (or other appropriate checks) and return a descriptive error when invalid instead of producing a malformed byte slice; update all callers to handle the error accordingly so decoding won't receive impossible values like 255 bits or a one-byte address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/netiputil/compact.go`:
- Around line 16-21: Validate the input prefix in EncodePrefix by checking
netip.Prefix validity and bit length before encoding: change EncodePrefix(p
netip.Prefix) to return ([]byte, error), verify p.IsValid() and p.Bits() >= 0
(or other appropriate checks) and return a descriptive error when invalid
instead of producing a malformed byte slice; update all callers to handle the
error accordingly so decoding won't receive impossible values like 255 bits or a
one-byte address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58f63604-5572-410f-90a1-859c6db3bd11
📒 Files selected for processing (2)
shared/netiputil/compact.goshared/netiputil/compact_test.go
✅ Files skipped from review due to trivial changes (1)
- shared/netiputil/compact_test.go
pascal-fischer
previously approved these changes
Mar 19, 2026
PeerCapability is reported in PeerSystemMeta.capabilities on login/sync. Management uses it instead of version gating to determine client features. disableIPv6 in Flags lets users opt out of IPv6 overlay.
lixmal
force-pushed
the
proto-ipv6-overlay
branch
from
b7be56f to
e2f7748
Compare
…anic, v6 validation
…anic, v6 validation
# Conflicts: # client/iface/wgaddr/address.go
…anic, v6 validation
# Conflicts: # client/iface/wgaddr/address.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/firewall/uspfilter/filter.go (1)
1533-1574:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete
blockRulesbefore the netstack/local-forwarding early return.
EnableRouting()now persists both temporary block rules, butDisableRouting()still returns early in netstack+local-forwarding mode before this cleanup runs. In that mode, routing is marked disabled while the overlay drop rules remain installed, so routed traffic keeps getting blocked until the manager is torn down.Suggested shape
func (m *Manager) DisableRouting() error { m.mutex.Lock() defer m.mutex.Unlock() fwder := m.forwarder.Load() if fwder == nil { return nil } m.routingEnabled.Store(false) m.nativeRouter.Store(false) + + var merr *multierror.Error + for _, rule := range m.blockRules { + if err := m.deleteRouteRule(rule); err != nil { + merr = multierror.Append(merr, fmt.Errorf("delete block rule: %w", err)) + } + } + m.blockRules = nil // don't stop forwarder if in use by netstack if m.netstack && m.localForwarding { - return nil + return nberrors.FormatErrorOrNil(merr) } fwder.Stop() m.forwarder.Store(nil) log.Debug("forwarder stopped") - - var merr *multierror.Error - for _, rule := range m.blockRules { - if err := m.deleteRouteRule(rule); err != nil { - merr = multierror.Append(merr, fmt.Errorf("delete block rule: %w", err)) - } - } - m.blockRules = nil return nberrors.FormatErrorOrNil(merr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/filter.go` around lines 1533 - 1574, DisableRouting currently returns early when m.netstack && m.localForwarding without removing persisted overlay drop rules, leaving m.blockRules in place and blocking routed traffic; fix by moving the loop that iterates m.blockRules and calls m.deleteRouteRule(rule) (and clears m.blockRules) to occur before the early return so rules are always deleted and m.blockRules set to nil even when netstack+localForwarding is true, while keeping the existing locking, multierror handling (multierror.Append) and avoiding stopping the forwarder when the early-return condition holds.
🤖 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/uspfilter/filter.go`:
- Around line 244-255: The IPv6 parser (parser6 created via
gopacket.NewDecodingLayerParser) ignores IPv6 extension headers so packets stop
decoding before TCP/UDP/ICMP and get dropped; update the parser6 initialization
in filter.go to include the IPv6 extension header decoding layers (e.g.,
HopByHopOptions, IPv6Routing, DestinationOptions, Fragment, Authentication/ESP
decoders as available in gopacket) so the decoding chain advances to &d.ip6,
&d.tcp, &d.udp, &d.icmp6; alternatively (or additionally) update isValidPacket()
to explicitly walk the IPv6 extension header chain from the IPv6 header and
locate the next-header/tcp/udp/icmp payload before deciding to drop, ensuring
forwarder.parseICMPv6 and the local/routed handlers receive packets with
extension headers.
---
Outside diff comments:
In `@client/firewall/uspfilter/filter.go`:
- Around line 1533-1574: DisableRouting currently returns early when m.netstack
&& m.localForwarding without removing persisted overlay drop rules, leaving
m.blockRules in place and blocking routed traffic; fix by moving the loop that
iterates m.blockRules and calls m.deleteRouteRule(rule) (and clears
m.blockRules) to occur before the early return so rules are always deleted and
m.blockRules set to nil even when netstack+localForwarding is true, while
keeping the existing locking, multierror handling (multierror.Append) and
avoiding stopping the forwarder when the early-return condition holds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5d5bd4b-d012-4c5c-879d-f039f8ba1710
📒 Files selected for processing (4)
client/firewall/nftables/manager_linux.goclient/firewall/nftables/router_linux.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/forwarder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/firewall/nftables/manager_linux.go
- client/firewall/nftables/router_linux.go
…add v6 ext-header TODO
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/internal/routemanager/server/server.go (1)
37-88:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRefresh retained routes when
useNewDNSRoutechanges.
UpdateRoutesonly reprograms routes that are added or removed in this call. If the DNS-route mode flips while a route stays inr.routes, its NAT rule keeps the old shape, but later removals/CleanUpwill use the new flag and miss that entry. Please track the mode per route or force a remove+re-add for existing routes when the flag changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/server/server.go` around lines 37 - 88, UpdateRoutes currently leaves existing routes unchanged when r.useNewDNSRoute flips, so NAT rules keep the old shape and later removals (or CleanUp) use the new flag and miss them. Fix: detect change by comparing prevUseNewDNSRoute and useNewDNSRoute, and for any route still present in routesMap but already in r.routes, call removeFromServerNetwork(oldRoute, prevUseNewDNSRoute) to tear down the old rule, then later (after setting r.useNewDNSRoute = useNewDNSRoute and enabling routing) call addToServerNetwork(newRoute, useNewDNSRoute) to reprogram it; ensure r.routes entries are replaced with the refreshed newRoute. Use the existing symbols UpdateRoutes, r.useNewDNSRoute, r.routes, removeFromServerNetwork, addToServerNetwork (or alternatively store per-route mode if preferred).
♻️ Duplicate comments (1)
client/iface/wgproxy/bind/proxy.go (1)
223-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the same port-range guard here before casting to
uint16.
RedirectAscan still pass a malformednet.UDPAddr, and this helper will silently wrap negative or out-of-range ports into the wrong endpoint.Suggested fix
func addrToEndpoint(addr *net.UDPAddr) (*bind.Endpoint, error) { if addr == nil { return nil, fmt.Errorf("invalid address") } + if addr.Port < 0 || addr.Port > 65535 { + return nil, fmt.Errorf("invalid UDP port: %d", addr.Port) + } ip, ok := netip.AddrFromSlice(addr.IP) if !ok { return nil, fmt.Errorf("convert %s to netip.Addr", addr) } addrPort := netip.AddrPortFrom(ip.Unmap(), uint16(addr.Port)) return &bind.Endpoint{AddrPort: addrPort}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/iface/wgproxy/bind/proxy.go` around lines 223 - 233, In addrToEndpoint ensure the UDP port is validated before casting to uint16: check that addr.Port is within 0..65535 and return an error if out of range or negative, then proceed to create netip.AddrPortFrom using the validated port; update any call sites (e.g., RedirectAs flows that pass net.UDPAddr) to rely on this guard so malformed addresses no longer produce wrapped/incorrect endpoints.
🧹 Nitpick comments (2)
client/firewall/uspfilter/filter_routeacl_test.go (1)
167-176: ⚡ Quick winAdd a dual-stack idempotency variant for
blockInvalidRouted.This test only exercises IPv4 (
routeRules == 1, first returned rule ID). With IPv6 overlay support, add a sibling case withIPv6Netset and assert both returned rule IDs are stable across repeated calls and total rule count is2.📌 Suggested test shape
+func TestBlockInvalidRoutedIdempotentDualStack(t *testing.T) { + ctrl := gomock.NewController(t) + dev := mocks.NewMockDevice(ctrl) + dev.EXPECT().MTU().Return(1500, nil).AnyTimes() + + wgNet4 := netip.MustParsePrefix("100.64.0.1/16") + wgNet6 := netip.MustParsePrefix("fd00::1/64") + + ifaceMock := &IFaceMock{ + SetFilterFunc: func(device.PacketFilter) error { return nil }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: wgNet4.Addr(), + Network: wgNet4, + IPv6: wgNet6.Addr(), + IPv6Net: wgNet6, + } + }, + GetDeviceFunc: func() *device.FilteredDevice { return &device.FilteredDevice{Device: dev} }, + GetWGDeviceFunc: func() *wgdevice.Device { return &wgdevice.Device{} }, + } + + manager, err := Create(ifaceMock, false, flowLogger, iface.DefaultMTU) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, manager.Close(nil)) }) + + rules1, err := manager.blockInvalidRouted(ifaceMock) + require.NoError(t, err) + require.Len(t, rules1, 2) + + rules2, err := manager.blockInvalidRouted(ifaceMock) + require.NoError(t, err) + require.Len(t, rules2, 2) + + assert.Equal(t, rules1[0].ID(), rules2[0].ID()) + assert.Equal(t, rules1[1].ID(), rules2[1].ID()) + + manager.mutex.RLock() + ruleCount := len(manager.routeRules) + manager.mutex.RUnlock() + assert.Equal(t, 2, ruleCount) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/filter_routeacl_test.go` around lines 167 - 176, Add a dual-stack idempotency variant for blockInvalidRouted: duplicate the existing case that uses wgNet (netip.MustParsePrefix("100.64.0.1/16")) and create a sibling case that also sets an IPv6 prefix (e.g. IPv6Net := netip.MustParsePrefix("fd00::1/64")) on the IFaceMock AddressFunc or IPv6Net field so the code exercises IPv6 overlay; call blockInvalidRouted twice and assert the returned rule IDs from the first and second call are identical and that the total number of route rules created is 2 (i.e. both IPv4 and IPv6 rules exist); reuse the same IFaceMock, SetFilterFunc, AddressFunc and wgaddr.Address setup so the test mirrors the IPv4 path and checks idempotency and stable rule IDs for both families.client/firewall/nftables/router_linux.go (1)
1851-1907: ⚡ Quick winAdd explicit address-family validation in DNAT entrypoints.
AddInboundDNATandAddOutputDNATbuild expressions from bothr.afandlocalAddr. If caller input ever mismatches router family, rules are built inconsistently and fail later at flush-time. Fail fast with a clear error.🛠️ Suggested patch
+func (r *router) validateLocalAddrFamily(addr netip.Addr) error { + isIPv4Router := r.af.tableFamily == nftables.TableFamilyIPv4 + if addr.Is4() != isIPv4Router { + return fmt.Errorf("address family mismatch: router=%s, addr=%s", familyName(r.af.tableFamily), addr) + } + return nil +} + func (r *router) AddInboundDNAT(localAddr netip.Addr, protocol firewall.Protocol, originalPort, translatedPort uint16) error { + if err := r.validateLocalAddrFamily(localAddr); err != nil { + return err + } ruleID := fmt.Sprintf("inbound-dnat-%s-%s-%d-%d", localAddr.String(), protocol, originalPort, translatedPort) ... func (r *router) AddOutputDNAT(localAddr netip.Addr, protocol firewall.Protocol, originalPort, translatedPort uint16) error { + if err := r.validateLocalAddrFamily(localAddr); err != nil { + return err + } ruleID := fmt.Sprintf("output-dnat-%s-%s-%d-%d", localAddr.String(), protocol, originalPort, translatedPort)Also applies to: 1982-2036
🤖 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 1851 - 1907, Add explicit validation that the provided localAddr matches the router's address family before building nft expressions in AddInboundDNAT (and likewise in AddOutputDNAT): check r.af (or r.af.tableFamily / address-family enum) against localAddr.Is6() (or netip.Addr being v4) and return a clear error if they mismatch so callers fail fast instead of creating invalid rules that later fail at flush-time; update both AddInboundDNAT and AddOutputDNAT to perform this check near the start (before using r.af or calling applyPrefix) and return a descriptive error referencing the expected family and the provided localAddr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/internal/routemanager/server/server.go`:
- Around line 37-88: UpdateRoutes currently leaves existing routes unchanged
when r.useNewDNSRoute flips, so NAT rules keep the old shape and later removals
(or CleanUp) use the new flag and miss them. Fix: detect change by comparing
prevUseNewDNSRoute and useNewDNSRoute, and for any route still present in
routesMap but already in r.routes, call removeFromServerNetwork(oldRoute,
prevUseNewDNSRoute) to tear down the old rule, then later (after setting
r.useNewDNSRoute = useNewDNSRoute and enabling routing) call
addToServerNetwork(newRoute, useNewDNSRoute) to reprogram it; ensure r.routes
entries are replaced with the refreshed newRoute. Use the existing symbols
UpdateRoutes, r.useNewDNSRoute, r.routes, removeFromServerNetwork,
addToServerNetwork (or alternatively store per-route mode if preferred).
---
Duplicate comments:
In `@client/iface/wgproxy/bind/proxy.go`:
- Around line 223-233: In addrToEndpoint ensure the UDP port is validated before
casting to uint16: check that addr.Port is within 0..65535 and return an error
if out of range or negative, then proceed to create netip.AddrPortFrom using the
validated port; update any call sites (e.g., RedirectAs flows that pass
net.UDPAddr) to rely on this guard so malformed addresses no longer produce
wrapped/incorrect endpoints.
---
Nitpick comments:
In `@client/firewall/nftables/router_linux.go`:
- Around line 1851-1907: Add explicit validation that the provided localAddr
matches the router's address family before building nft expressions in
AddInboundDNAT (and likewise in AddOutputDNAT): check r.af (or r.af.tableFamily
/ address-family enum) against localAddr.Is6() (or netip.Addr being v4) and
return a clear error if they mismatch so callers fail fast instead of creating
invalid rules that later fail at flush-time; update both AddInboundDNAT and
AddOutputDNAT to perform this check near the start (before using r.af or calling
applyPrefix) and return a descriptive error referencing the expected family and
the provided localAddr.
In `@client/firewall/uspfilter/filter_routeacl_test.go`:
- Around line 167-176: Add a dual-stack idempotency variant for
blockInvalidRouted: duplicate the existing case that uses wgNet
(netip.MustParsePrefix("100.64.0.1/16")) and create a sibling case that also
sets an IPv6 prefix (e.g. IPv6Net := netip.MustParsePrefix("fd00::1/64")) on the
IFaceMock AddressFunc or IPv6Net field so the code exercises IPv6 overlay; call
blockInvalidRouted twice and assert the returned rule IDs from the first and
second call are identical and that the total number of route rules created is 2
(i.e. both IPv4 and IPv6 rules exist); reuse the same IFaceMock, SetFilterFunc,
AddressFunc and wgaddr.Address setup so the test mirrors the IPv4 path and
checks idempotency and stable rule IDs for both families.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24c8e2d3-42fb-453d-832d-026ee7e686bd
📒 Files selected for processing (10)
client/firewall/nftables/manager_linux.goclient/firewall/nftables/router_linux.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/filter_routeacl_test.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/forwarder_test.goclient/iface/wgproxy/bind/proxy.goclient/internal/dns/local/local.goclient/internal/dns/server.goclient/internal/routemanager/server/server.go
✅ Files skipped from review due to trivial changes (4)
- client/internal/dns/local/local.go
- client/internal/dns/server.go
- client/firewall/uspfilter/forwarder/forwarder_test.go
- client/firewall/nftables/manager_linux.go
# Conflicts: # client/internal/debug/debug_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/internal/debug/debug.go (1)
1421-1430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSourcePrefixes anonymization is not fail-closed, unlike
AddressV6.The
AddressV6path above clears the field on decode/encode failure (lines 1299-1312), but here the loop usescontinueon decode error and silently leaves the entry untouched on encode error. In both failure cases the original rawSourcePrefixes[i]bytes survive and are later emitted as base64 innetwork_map.jsonbyprotojson, so--anonymizecan still leak real prefixes on malformed/unexpected input. Mirror the fail-closed handling used forAddressV6for consistency.🔒 Suggested fail-closed handling
for i, raw := range rule.GetSourcePrefixes() { p, err := netiputil.DecodePrefix(raw) if err != nil { - continue + rule.SourcePrefixes[i] = nil + continue } anonAddr := anonymizer.AnonymizeIP(p.Addr()) if b, err := netiputil.EncodePrefix(netip.PrefixFrom(anonAddr, p.Bits())); err == nil { rule.SourcePrefixes[i] = b + } else { + rule.SourcePrefixes[i] = nil } }🤖 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 `@client/internal/debug/debug.go` around lines 1421 - 1430, Loop over rule.GetSourcePrefixes() must be fail-closed like AddressV6: on any netiputil.DecodePrefix or netiputil.EncodePrefix error you should clear the stored entry instead of leaving the original bytes. Modify the block that uses netiputil.DecodePrefix, anonymizer.AnonymizeIP and netiputil.EncodePrefix so that when DecodePrefix fails or when EncodePrefix returns an error you set rule.SourcePrefixes[i] to nil/empty (same clearing behavior used for AddressV6), and only assign rule.SourcePrefixes[i] = b when encode succeeds.
🤖 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.
Duplicate comments:
In `@client/internal/debug/debug.go`:
- Around line 1421-1430: Loop over rule.GetSourcePrefixes() must be fail-closed
like AddressV6: on any netiputil.DecodePrefix or netiputil.EncodePrefix error
you should clear the stored entry instead of leaving the original bytes. Modify
the block that uses netiputil.DecodePrefix, anonymizer.AnonymizeIP and
netiputil.EncodePrefix so that when DecodePrefix fails or when EncodePrefix
returns an error you set rule.SourcePrefixes[i] to nil/empty (same clearing
behavior used for AddressV6), and only assign rule.SourcePrefixes[i] = b when
encode succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfe4907c-e11f-44d0-8fcc-239df22a9946
📒 Files selected for processing (2)
client/internal/debug/debug.goclient/internal/debug/debug_test.go
Collaborator
Author
|
@coderabbitai clean up old comments (NOT review threads) and only keep the latest one |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 409 🏁 Script executed: Length of output: 42
|
Collaborator
Author
|
@coderabbitai also clean up old comments like these: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 545 🏁 Script executed: Length of output: 3061 🏁 Script executed: Length of output: 512
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR comprehensively adds IPv6 overlay support across NetBird by introducing IPv6 fields to peer/account/configuration models, extending firewall managers with dual-stack rule dispatch, updating engine/interface handling for dual-stack addresses, adding DNS IPv6 reverse-zone support, and enabling IPv6-aware routing and peer management throughout the codebase. ChangesIPv6 Overlay Support & Core Infrastructure
Firewall & Network Filtering (Dual-Stack)
Engine & Interface Management
Client Configuration & CLI
DNS & Routing
Client SDKs & UI
Testing & Utilities
Sequence Diagram(s)sequenceDiagram
participant Client
participant Management
participant Firewall
participant LocalIface
Client->>Management: SyncPeer with PeerCapabilities
Management->>Management: Extract DisableIPv6 from metadata
Management->>Management: Allocate IPv6 from NetV6 if enabled
Management->>Client: SyncResponse with address_v6 bytes
Client->>Client: Decode address_v6 via netiputil
Client->>Client: Update EngineConfig with IPv6 overlay
Client->>LocalIface: UpdateAddr with wgaddr.Address
LocalIface->>LocalIface: Configure IPv4 and IPv6 on TUN
Client->>Firewall: AddPeerFiltering with peer IPv6
Firewall->>Firewall: Dispatch to IPv4 or IPv6 manager
Firewall->>LocalIface: Install dual-stack rules
Client->>Client: filterAllowedIPs for IPv6
Client->>Client: Detect IPv6 capability changes
Client->>Management: OnPeersUpdated when flipped
Management->>Client: Updated NetworkMap propagates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes The PR introduces foundational IPv6 dual-stack support across NetBird's core architecture. The scope is massive (~200+ files), touching firewall rule dispatch, peer/account/group management, DNS/routing subsystems, interface configuration, client SDKs, and test infrastructure. Changes are heterogeneous—ranging from data-shape updates (protobuf, netiputil compact encoding) to complex algorithms (dual-pool fake-IP allocation, firewall family-aware dispatching, IPv6 conntrack/ICMP handling) to multi-platform device configuration. Logic density is high, especially in firewall managers (iptables/nftables v4/v6 branching), route management (merge-set handling, v6-exit pairing), and engine peer-state synchronization (IPv6-change detection, allowed-IP filtering). The deep interdependencies between layers and the need to verify both IPv4 backward-compatibility and new IPv6 functionality across diverse subsystems demand careful, multi-pass review. Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
pappz
previously approved these changes
May 6, 2026
|
pascal-fischer
approved these changes
May 7, 2026
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
3 participants
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
This PR implements IPv6 overlay support in client and management.
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#667
Summary by CodeRabbit
New Features
Improvements