[client] Add non-root ICMP support to userspace firewall forwarder #4792
Conversation
WalkthroughAtomic MTU added to endpoint; Forwarder probes raw‑ICMP access, adds ping fallback and semaphore rate limiting; ICMP echo handling refactored into socket and ping paths with helpers; UDP handler now returns bool; logger gains Warn4; receiver creation generalized; broad dependency and CI Go toolchain upgrades; minor error-formatting fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ICMP Request
participant Forwarder as Forwarder.handleICMP()
participant Echo as handleICMPEcho()
participant SocketPath as handleICMPViaSocket()
participant PingPath as handleICMPViaPing()
participant RawSock as Raw ICMP Socket
participant PingCmd as Ping Command
participant Injector as injectICMPReply()
Client->>Forwarder: ICMP Echo Request
Forwarder->>Forwarder: generate flowID & emit TypeStart
Forwarder->>Echo: route to handleICMPEcho()
alt hasRawICMPAccess
Echo->>SocketPath: use raw-socket path
SocketPath->>RawSock: forwardICMPPacket() send
RawSock-->>SocketPath: receive reply
SocketPath->>Injector: injectICMPReply()
Injector-->>Client: Echo Reply
else fallback
Echo->>PingPath: use ping fallback (rate-limited via pingSemaphore)
PingPath->>PingCmd: build & run ping
PingCmd-->>PingPath: ping response/status
PingPath->>Injector: synthesize & inject reply
Injector-->>Client: Echo Reply (synthesized)
end
Forwarder->>Forwarder: emit TypeEnd
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
lixmal
force-pushed
the
forwarder-ping
branch
from
81ce68d to
a7a5407
Compare
lixmal
force-pushed
the
forwarder-ping
branch
from
a7a5407 to
e7d5cdc
Compare
![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
🧹 Nitpick comments (1)
client/firewall/uspfilter/forwarder/icmp.go (1)
57-65: Consider adding panic recovery to the goroutine.If
handleICMPViaSocketorhandleICMPViaPingpanic, the goroutine will crash silently. Consider adding a deferred panic recovery handler to log any unexpected panics.Apply this diff to add panic recovery:
go func() { defer func() { <-f.pingSemaphore }() + defer func() { + if r := recover(); r != nil { + f.logger.Error1("forwarder: Panic in ICMP echo handler: %v", r) + } + }() if f.hasRawICMPAccess { f.handleICMPViaSocket(flowID, id, icmpHdr, icmpData, rxBytes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/uspfilter/forwarder/icmp.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4777
File: client/firewall/iptables/acl_linux.go:439-450
Timestamp: 2025-11-13T15:19:32.799Z
Learning: In the netbird client firewall iptables implementation (client/firewall/iptables/), IPv6 is not currently handled or supported.
🧬 Code graph analysis (1)
client/firewall/uspfilter/forwarder/icmp.go (1)
client/firewall/uspfilter/forwarder/forwarder.go (2)
Forwarder(35-49)New(51-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: test-getting-started-script
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (8)
client/firewall/uspfilter/forwarder/icmp.go (8)
5-9: LGTM! Necessary imports for ping fallback.The new imports support the ping binary fallback mechanism and platform-specific command construction.
20-48: LGTM! Clean separation of echo and non-echo ICMP handling.The refactored logic properly delegates echo requests to asynchronous handling and forwards non-echo ICMP types when raw socket access is available.
73-99: LGTM! Proper resource management.The function correctly creates a raw ICMP socket, sends the packet, and handles errors appropriately. The caller is responsible for closing the returned connection as documented.
125-141: LGTM! Proper timeout and buffer sizing.The function correctly sets a read deadline and sizes the response buffer using the atomic MTU value.
233-245: LGTM! Correct ICMP echo reply synthesis.The function properly creates an echo reply by copying the data, changing the type, and recalculating the checksum following the standard ICMP checksum algorithm.
247-272: LGTM! Proper IP header construction and injection.The function correctly constructs an IPv4 header, wraps the ICMP payload, and bypasses the netstack to avoid re-processing by the ICMP handler. The TTL value of 64 is appropriate for local responses.
195-196: TheWarn4logger method exists and is properly implemented.Verification confirms that
Warn4has been added to the Logger type atclient/firewall/uspfilter/log/log.go:171with the correct signature supporting four arguments. The method call at line 195-196 is valid.
212-231: The original review comment's reasoning is fundamentally incorrect.The review claims
-tis TTL on macOS/BSD, but macOS uses-t <timeout>to specify timeout in seconds and FreeBSD uses-t <timeout>for timeout in seconds. The current code is correct for both platforms.However, there IS a real issue: OpenBSD uses
-w <maxwait>, not-t(which is TTL on OpenBSD), and NetBSD uses-w <deadline>for timeout. The current code incorrectly groups all three BSD variants together on line 225, which fails for OpenBSD and NetBSD.The correct fix requires splitting BSD platforms:
- FreeBSD: keep
-t- OpenBSD: change to
-w- NetBSD: change to
-wThe original review's suggested fix is partially correct by accident but based on false premises.
Likely an incorrect or invalid review comment.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
client/firewall/uspfilter/forwarder/icmp.go (2)
101-123: Send TypeEnd event on error to complete flow tracking.When
forwardICMPPacketfails at line 107, the function returns early without sending aTypeEndevent. This leaves the flow incomplete with only aTypeStartevent.Apply this diff to send an end event on error:
conn, err := f.forwardICMPPacket(id, payload, icmpHdr, 5*time.Second) if err != nil { f.logger.Error2("forwarder: Failed to send ICMP packet for %v: %v", epID(id), err) + f.sendICMPEvent(nftypes.TypeEnd, flowID, id, uint8(icmpHdr.Type()), uint8(icmpHdr.Code()), uint64(rxBytes), 0) return }
184-210: Send TypeEnd event on error to complete flow tracking.When the ping binary fails at line 194, the function returns without sending a
TypeEndevent, leaving the flow incomplete.Apply this diff to send an end event on error:
pingStart := time.Now() if err := cmd.Run(); err != nil { f.logger.Warn4("forwarder: Ping binary failed for %v type %v code %v: %v", epID(id), icmpHdr.Type(), icmpHdr.Code(), err) + f.sendICMPEvent(nftypes.TypeEnd, flowID, id, uint8(icmpHdr.Type()), uint8(icmpHdr.Code()), uint64(rxBytes), 0) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/firewall/uspfilter/forwarder/icmp.go(4 hunks)client/firewall/uspfilter/forwarder/udp.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4777
File: client/firewall/iptables/acl_linux.go:439-450
Timestamp: 2025-11-13T15:19:32.799Z
Learning: In the netbird client firewall iptables implementation (client/firewall/iptables/), IPv6 is not currently handled or supported.
🧬 Code graph analysis (2)
client/firewall/uspfilter/forwarder/udp.go (1)
client/firewall/uspfilter/forwarder/forwarder.go (1)
Forwarder(35-49)
client/firewall/uspfilter/forwarder/icmp.go (1)
client/firewall/uspfilter/forwarder/forwarder.go (2)
Forwarder(35-49)New(51-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: test-getting-started-script
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
🔇 Additional comments (4)
client/firewall/uspfilter/forwarder/udp.go (2)
352-354: Good addition of io.EOF to closed-error detection.Treating
io.EOFas a terminal condition inisClosedErroris appropriate, as EOF signals a closed connection and should not be logged as an unexpected error.
180-180: Signature change verified and correct.The web search confirms that
gonet.NewUDPConnin the upgraded gvisor/netstack has signaturefunc NewUDPConn(wq *waiter.Queue, ep tcpip.Endpoint) *UDPConn. The code at line 180 (gonet.NewUDPConn(&wq, ep)) correctly matches this signature, with the stack parameter properly removed as part of the netstack upgrade.client/firewall/uspfilter/forwarder/icmp.go (2)
247-272: Good implementation of ICMP reply injection.The
injectICMPReplymethod correctly:
- Constructs a proper IPv4 header with appropriate fields
- Calculates and sets the IP checksum
- Bypasses netstack to avoid looping through the ICMP handler
- Returns the injected size for flow tracking
212-231: I need to verify the flags across all platforms mentioned. The web search already revealed important information about macOS that contradicts the review comment. Let me search for the other platforms:Based on my verification, the review comment contains fundamental inaccuracies regarding the macOS ping flags.
The review comment is incorrect and should be disregarded.
Evidence from official documentation:
Linux/Android:
-Wspecifies time to wait for a response in seconds, with-wsetting the timeout for the entire program session. The code correctly uses-Wfor timeout.FreeBSD/OpenBSD/NetBSD:
-tspecifies a timeout in seconds before ping exits. The code correctly uses-tfor timeout on these platforms.Windows:
-wsets timeout in milliseconds. The code correctly multipliestimeoutSecby 1000 to convert to milliseconds.macOS/iOS: The review's claim is incorrect. From the earlier web search,
-t timeouton macOS is "overall timeout in seconds before ping exits regardless of packets received." The code correctly uses-twith timeout in seconds. The review mistakenly claims-tis TTL and suggests using-W, but on macOS,-Wuses milliseconds, not seconds, which would be inconsistent with the code's handling of seconds.The platform-specific ping flags in the code are correct and consistent with official documentation across all platforms.
Likely an incorrect or invalid review comment.
Describe your changes
Depends on netbirdio/wireguard-go#12
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
Refactor
Chores
Bug Fixes