NM-295: TCP encapsulation over udp - #1351
Conversation
Drive proxy client/server from use_tcp_uplink and tcp_proxy_enabled, route uplink peers through userspace Bind, and authenticate HELLO with WG key proofs instead of control-plane JWT.
Queue TCP sends off Bind.Send, clear the client relay route on stop, and skip reconcile when restarting for userspace/kernel mode changes.
Close the live userspace Device even after needTCPUplinkBind flips off, and drain TCP bind I/O before Device.Close so disable can return to kernel WG without hanging wg show.
|
Review complete. 12 potential issues to review. Files Reviewed: 19 By Severity:
This PR introduces TCP proxy uplink and WireGuard relay TCP bind support, but contains a critical authorization bypass in ValidateClientHello, TLS certificate verification disabled, multiple data races and goroutine leak risks that must be resolved before merge. Files Reviewed (19 files) |
There was a problem hiding this comment.
Review complete. 12 potential issues to review.
Summary
PR #1351 adds TCP proxy uplink and WireGuard relay TCP bind functionality to the Netclient agent, enabling relay-based WireGuard connectivity through a proxy server. The changes span 19 files across wireguard, proxyuplink, and functions packages.
Critical Issues
- finding-001:
internal/proxyuplink/server.go:153—ValidateClientHelloauthorization bypass: when gatewayRelayedNodesis empty, any node can connect without authentication. This is a critical security vulnerability. - finding-004:
internal/proxyuplink/tls.go:68— TLS client certificate verification is disabled (InsecureSkipVerify: true), exposing the TCP uplink to MITM attacks.
Medium Severity Issues
- finding-002:
wireguard/relay_tcp_out.go:45— Send-on-closed-channel panic and goroutine leak inenqueueTCPOut/DrainTCPOutQueuerace. - finding-003:
wireguard/relay_tcp_hooks.go:80— Send-on-closed-channel panic inpushInboundracing withcloseInboundduring shutdown. - finding-005:
functions/tcp_uplink.go:164— Cumulative goroutine leak on TCP uplink client/server manager restart. - finding-009:
functions/tcp_uplink.go:48— Data race ontcpUplinkWasOninprepareTCPUplinkWireGuard. - finding-011:
internal/proxyuplink/manager.go:145— TOCTOU race betweenManager.SendPacketandManager.Stop.
Low Severity Issues
- finding-006: Debug output via
fmt.Printlninstead ofslogin TCP uplink code. - finding-007:
SetVerbositydefault case silently changed from Error to Debug. - finding-008: Redundant local
min()function shadows Go 1.21+ builtin. - finding-010: Implicit global-state dependency between
prepareTCPUplinkWireGuardandnc.Create. - finding-012: Data race on
needTCPUplinkBindacross goroutines.
Recommendation
Request changes. The critical authorization bypass and disabled TLS verification must be fixed before merge. The concurrency issues (panics, data races, goroutine leaks) also need resolution. Consider adding integration tests for the TCP uplink and relay bind paths.
Added authentication step for private Go modules using GitHub token.
Updated GitHub Actions workflow to patch netclient with Docker build and upload steps. Removed build job and added build-and-patch job for streamlined processing.
Refactor GitHub Actions workflow to build and upload netclient binaries for multiple OS and architectures. Update input descriptions and streamline build steps.
Added authentication step for private Go modules using GitHub token.
Added environment variables for private Go module access and authentication in the GitHub Actions workflow.
Describe your changes
Provide Issue ticket number if applicable/not in title
Provide link to Netmaker PR if required
Provide testing steps
Checklist before requesting a review