Skip to content

Commit

Permalink
fix: fix netstack to forward TCP sessions to local addresses (#62)
Browse files Browse the repository at this point in the history
relates to: coder/coder#14715

For CoderVPN, we need Agents to operate on a separate Unique Local Address (ULA) prefix than Tailscale, so that CoderVPN and Tailscale can both run on the same computer.

This PR fixes an issue in the Tailscale netstack, where it uses the hardcoded Tailscale ULA to decide whether to forward TCP connections to localhost (127.0.0.1), rather than just checking whether the destination is an address assigned to the node.

Pretty sure this is just a bug / another case of assumptions that are true for Tailscale but not for us.  `acceptTCP()` makes a call to `removeSubnetAddress()` in a defer. This was originally conditional on `isTailscaleIP`, but the check for `addSubnetAddress()` on line 311 uses `isLocalIP()`.  Stepping thru the code, if we accept a TCP connection for an address that is local, but not in the Tailscale service prefix (i.e. one in our new Coder service prefix), we call `removeSubnetAddress()` without ever having called `addSubnetAddress()`, and decrement the connection count on that address to -1, which is almost certainly incorrect.  It worked fine for Tailscale because they could safely assume that all local addresses were also Tailscale IPs, but we can't anymore.

Note also that UDP forwarding already uses `isLocalIP()` to decide whether to forward.

This change passes the local `netstack` unit tests, but the real tests will be in `coder/coder` when we show that we can successfully make TCP connections.
  • Loading branch information
spikecurtis authored Oct 3, 2024
2 parents ddd4a72 + b3b2e15 commit 02286e5
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions wgengine/netstack/netstack.go
Original file line number Diff line number Diff line change
Expand Up @@ -878,17 +878,17 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) {
clientRemoteAddrPort := netip.AddrPortFrom(clientRemoteIP, clientRemotePort)

dialIP := netaddrIPFromNetstackIP(reqDetails.LocalAddress)
isTailscaleIP := tsaddr.IsTailscaleIP(dialIP)
isLocal := ns.isLocalIP(dialIP)

dstAddrPort := netip.AddrPortFrom(dialIP, reqDetails.LocalPort)

if viaRange.Contains(dialIP) {
isTailscaleIP = false
isLocal = false
dialIP = tsaddr.UnmapVia(dialIP)
}

defer func() {
if !isTailscaleIP {
if !isLocal {
// if this is a subnet IP, we added this in before the TCP handshake
// so netstack is happy TCP-handshaking as a subnet IP
ns.removeSubnetAddress(dialIP)
Expand Down Expand Up @@ -975,7 +975,7 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) {
return
}
}
if isTailscaleIP {
if isLocal {
dialIP = netaddr.IPv4(127, 0, 0, 1)
}
dialAddr := netip.AddrPortFrom(dialIP, uint16(reqDetails.LocalPort))
Expand Down

0 comments on commit 02286e5

Please sign in to comment.