fix: fix netstack to forward TCP sessions to local addresses #62
Conversation
This file contains 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
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
ethanndickson
approved these changes
Oct 2, 2024
deansheather
approved these changes
Oct 3, 2024
Merge activity
|
spikecurtis
added a commit
to coder/coder
that referenced
this pull request
Oct 4, 2024
fixes #14715 Configures agents to use an address both in the Tailscale service prefix and the new Coder service prefix. Also modifies the Coordinator auth to allow the new prefix. Updates `coder/tailscale` to include coder/tailscale#62 which fixes a bug around forwarding TCP connections to localhost. This functionality is tested in the modifications to `TestAgent_Dial`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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 toremoveSubnetAddress()in a defer. This was originally conditional onisTailscaleIP, but the check foraddSubnetAddress()on line 311 usesisLocalIP(). 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 callremoveSubnetAddress()without ever having calledaddSubnetAddress(), 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
netstackunit tests, but the real tests will be incoder/coderwhen we show that we can successfully make TCP connections.