[client] Add IPv6 support to UDP WireGuard proxy #5169
Conversation
Add IPv6 packet header support in UDP raw socket proxy to handle both IPv4 and IPv6 source addresses. Refactor error handling in proxy bind implementations to validate endpoints before acquiring locks.
📝 WalkthroughWalkthroughCentralizes address-to-endpoint validation, makes RedirectAs guard against nil/invalid endpoints and reorder state updates, and adds dual‑stack (IPv4/IPv6) raw‑socket handling and header generation across eBPF and UDP proxy paths and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BindProxy as Bind Proxy
participant EBPFWrap as eBPF Wrapper
participant EBPFProxy as eBPF Proxy
participant RawSockets as RawSocket(s)
participant Network
Caller->>BindProxy: RedirectAs(addr)
BindProxy->>BindProxy: addrToEndpoint(addr)
alt resolved
BindProxy->>BindProxy: lock → set wgCurrentUsed → clear paused → signal cond → unlock
BindProxy->>EBPFWrap: RedirectAs(endpoint)
EBPFWrap->>EBPFWrap: validate endpoint & endpoint.IP
EBPFWrap->>EBPFProxy: sendPkg(endpointAddr)
EBPFProxy->>EBPFProxy: choose IPv4 or IPv6 path
EBPFProxy->>RawSockets: build ip/net layers, checksum, serialize
RawSockets->>Network: write packet to dst (127.0.0.1 or ::1)
else failed
BindProxy-->>Caller: return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/iface/wgproxy/ebpf/wrapper.go (1)
93-105: Minor typo in error message.The error message says "package redirection" but should likely be "packet redirection" to be consistent with network terminology.
Suggested fix
func (p *ProxyWrapper) RedirectAs(endpoint *net.UDPAddr) { if endpoint == nil || endpoint.IP == nil { - log.Errorf("failed to start package redirection, endpoint is nil") + log.Errorf("failed to start packet redirection, endpoint is nil") return }
🧹 Nitpick comments (1)
client/iface/wgproxy/bind/proxy.go (1)
116-130: Good refactor with early validation; minor typo in error message.The refactored flow correctly validates the endpoint before acquiring the lock, preventing state changes on invalid input. However, the error message says "package redirection" but should be "packet redirection" (same typo as in wrapper.go).
Suggested fix
func (p *ProxyBind) RedirectAs(endpoint *net.UDPAddr) { ep, err := addrToEndpoint(endpoint) if err != nil { - log.Errorf("failed to start package redirection: %v", err) + log.Errorf("failed to start packet redirection: %v", err) return }
The sendPkg function now detects IP version and creates appropriate IPv4 or IPv6 headers, preventing IPv6 addresses from being placed into IPv4 packet headers.
- PrepareSenderRawSocket() now returns both IPv4 and IPv6 sockets - UDP proxy creates only the needed socket based on address family - eBPF proxy maintains both sockets and selects appropriate one - Added warning logs for Close() errors during cleanup
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/iface/wgproxy/rawsocket/rawsocket.go (1)
82-99: File descriptor leak:filemust be closed after successfulnet.FilePacketConncall.
net.FilePacketConnduplicates the underlying file descriptor usingpoll.DupCloseOnExecon Unix systems. The original*os.Filemust be explicitly closed; closing one side does not close the other. Without closingfileafter line 90 succeeds, the original file descriptor leaks.Proposed fix
file := os.NewFile(uintptr(fd), fmt.Sprintf("fd %d", fd)) if file == nil { if closeErr := syscall.Close(fd); closeErr != nil { log.Warnf("failed to close raw socket fd: %v", closeErr) } return nil, fmt.Errorf("converting fd to file failed") } + defer file.Close() + packetConn, err := net.FilePacketConn(file) if err != nil { - if closeErr := file.Close(); closeErr != nil { - log.Warnf("failed to close file: %v", closeErr) - } return nil, fmt.Errorf("converting file to packet conn failed: %w", err) } return packetConn, nil
🤖 Fix all issues with AI agents
In `@client/iface/wgproxy/ebpf/proxy.go`:
- Around line 242-249: In WGEBPFProxy.sendPkg add a defensive nil check for the
endpointAddr parameter before accessing endpointAddr.IP to avoid a nil pointer
dereference; if endpointAddr is nil, return a non-nil error (or
wrap/contextualize an existing error) immediately, e.g., validate at the top of
sendPkg and return an error like "nil endpointAddr" so subsequent logic that
uses endpointAddr.IP, endpointAddr.Port or assigns dstIP only runs when
endpointAddr is non-nil.
In `@client/iface/wgproxy/udp/rawsocket.go`:
- Around line 40-55: The function NewSrcFaker must guard against a nil srcAddr
to avoid a panic when accessing srcAddr.IP; add an early nil check at the top of
NewSrcFaker that returns a descriptive error if srcAddr == nil, then proceed
with the existing branch that calls
rawsocket.PrepareSenderRawSocketIPv4/PrepareSenderRawSocketIPv6 and sets
localHostNetIPAddrV4/localHostNetIPAddrV6.
🧹 Nitpick comments (1)
client/iface/wgproxy/udp/rawsocket.go (1)
108-128: Consider using global constants for localhost IPs to avoid duplication.Lines 109 and 120 use hardcoded
net.ParseIP("127.0.0.1")andnet.ParseIP("::1")respectively, while the file already haslocalHostNetIPAddrV4andlocalHostNetIPAddrV6defined.Proposed fix
if srcAddr.IP.To4() != nil { // IPv4 ipv4 := &layers.IPv4{ - DstIP: net.ParseIP("127.0.0.1"), + DstIP: localHostNetIPAddrV4.IP, SrcIP: srcAddr.IP, Version: 4, TTL: 64, Protocol: layers.IPProtocolUDP, } ipH = ipv4 networkLayer = ipv4 } else { // IPv6 ipv6 := &layers.IPv6{ - DstIP: net.ParseIP("::1"), + DstIP: localHostNetIPAddrV6.IP, SrcIP: srcAddr.IP, Version: 6, HopLimit: 64, NextHeader: layers.IPProtocolUDP, }
…variables for both IPv4 and IPv6 in raw socket headers
- Split PrepareSenderRawSocket into IPv4 and IPv6 variants - Make IPv6 raw socket optional, log warning on failure
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/iface/wgproxy/rawsocket/rawsocket.go`:
- Around line 63-77: The os.File created by os.NewFile (variable "file") is only
closed on error, leaking the original FD because net.FilePacketConn duplicates
the descriptor; after successfully calling net.FilePacketConn(file) (where the
result is "packetConn"), close the original file (call file.Close()) and log any
close error (e.g., with log.Warnf) but do not close packetConn—ensure this
happens immediately after the successful net.FilePacketConn call so the original
FD is released while preserving the duplicated descriptor used by packetConn.
♻️ Duplicate comments (1)
client/iface/wgproxy/ebpf/proxy.go (1)
250-257: Guard against nilendpointAddr.Line 257 dereferences
endpointAddr.IP; a nil pointer will panic.🛡️ Proposed fix
func (p *WGEBPFProxy) sendPkg(data []byte, endpointAddr *net.UDPAddr) error { + if endpointAddr == nil { + return fmt.Errorf("endpoint address is nil") + } var ipH gopacket.SerializableLayer
- Close original os.File after net.FilePacketConn duplicates the FD to prevent leaking the descriptor. - Add tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/iface/wgproxy/rawsocket/rawsocket.go`:
- Around line 33-43: The IPv6 handling comment and socket options are wrong:
update the IPv6 branch in the raw socket setup (the block using isIPv4 and fd
and syscall.SetsockoptInt) to explicitly set IPV6_HDRINCL for IPv6 sockets
instead of relying on IPPROTO_RAW, and change the comment to state that IPv6
requires IPV6_HDRINCL to accept application-provided IPv6 headers (use
syscall.SetsockoptInt with the IPV6 level and IPV6_HDRINCL option and handle
errors the same way as the IPv4 branch).
🧹 Nitpick comments (3)
client/iface/wgproxy/udp/proxy.go (1)
67-67: Consider usingDebugfinstead ofInfoffor connection logging.This log line will emit for every TURN connection addition. If this is primarily for debugging/development purposes,
Debugfwould be more appropriate to avoid noise in production logs.- log.Infof("remote conn: %v", remoteConn.RemoteAddr()) + log.Debugf("remote conn: %v", remoteConn.RemoteAddr())client/iface/wgproxy/redirect_test.go (2)
29-123: Hardcoded ports may cause test flakiness in parallel execution.Each test uses a unique hardcoded port (51850-51853), but these could conflict with other tests or system services. Consider using port 0 for dynamic allocation where possible, or add
t.Parallel()with port allocation from a pool.However, since these tests require root privileges (eBPF/raw sockets) and likely run sequentially, this may be acceptable for now.
218-222: Time-based synchronization is fragile but acceptable for tests.The 100ms sleep for redirect processing is a common test pattern but could cause flakiness on slow CI systems. Consider adding a longer timeout or retry mechanism if flakiness is observed.
|
Add IPv6 packet header support in UDP raw socket proxy to handle both IPv4 and IPv6 source addresses.
Refactor error handling in proxy bind implementations to validate endpoints before acquiring locks. With this error handling logic, the WireGuard connection is not broken.
Describe your changes
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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.