There is a race in natlab where we might start shutdown while natlab is still running
a goroutine or two to deliver packets. This adds a small grace period to try and receive
it before continuing shutdown.
Signed-off-by: David Anderson <danderson@tailscale.com>
The first packet to transit may take several seconds to do so, because
setup rates in wgengine may result in the initial WireGuard handshake
init to get dropped. So, we have to wait at least long enough for a
retransmit to correct the fault.
Signed-off-by: David Anderson <danderson@tailscale.com>
Active discovery lets us introspect the state of the network stack precisely
enough that it's unnecessary, and dropping the initial DERP packets greatly
slows down tests. Additionally, it's unrealistic since our production network
will never deliver _only_ discovery packets, it'll be all or nothing.
Signed-off-by: David Anderson <danderson@tailscale.com>
Uses natlab only, because the point of this active discovery test is going to be
that it should get through a lot of obstacles.
Signed-off-by: David Anderson <danderson@tailscale.com>
The deadlock was:
* Conn.Close was called, which acquired c.mu
* Then this goroutine scheduled:
if firstDerp {
startGate = c.derpStarted
go func() {
dc.Connect(ctx)
close(c.derpStarted)
}()
}
* The getRegion hook for that derphttp.Client then ran, which also
tries to acquire c.mu.
This change makes that hook first see if we're already in a closing
state and then it can pretend that region doesn't exist.
wireguard-go uses 3 goroutines per peer (with reasonably large stacks
& buffers).
Rather than tell wireguard-go about all our peers, only tell it about
peers we're actively communicating with. That means we need hooks into
magicsock's packet receiving path and tstun's packet sending path to
lazily create a wireguard peer on demand from the network map.
This frees up lots of memory for iOS (where we have almost nothing
left for larger domains with many users).
We should ideally do this in wireguard-go itself one day, but that'd
be a pretty big change.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
sync.Pools should almost always be packate globals, even though in this
case we only have exactly 1 TUN device anyway, so it matters less.
Still, it's unusual to see a Pool that's not a package global, so move it.
We originally picked those numbers somewhat at random, but with the idea
that 8 is a traditionally lucky number in Chinese culture. Unfortunately,
"88" is also neo-nazi shorthand language.
Use 52 instead, because those are the digits above the letters
"TS" (tailscale) on a qwerty keyboard, so we're unlikely to collide with
other users. 5, 2 and 52 are also pleasantly culturally meaningless.
Signed-off-by: David Anderson <danderson@tailscale.com>
Nameserver IP 10.11.12.13 would otherwise get written to resolv.conf as 13.12.11.10, as was happening on my client.
Signed-off-by: Eduardo Kienetz <eduardo@kienetz.com>
Before this patch, the 250ms sleep would not be interrupted by context cancellation,
which would result in the goroutine sometimes lingering in tests (100ms grace period).
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
Very rarely, cancellation occurs between a successful send on derpRecvCh
and a call to copyBuf on the receiving side.
Without this patch, this situation results in <-copyBuf blocking indefinitely.
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
Peers advertising a discovery key know how to speak the discovery
protocol and do their own heartbeats to get through NATs and keep NATs
open. No need for the pinger except for with legacy peers.
The new interface lets implementors more precisely distinguish
local traffic from forwarded traffic, and applies different
forwarding logic within Machines for each type. This allows
Machines to be packet forwarders, which didn't quite work
with the implementation of Inject.
Signed-off-by: David Anderson <danderson@tailscale.com>
This log line looks buggy, even though lacking a filter is expected during bringup.
We already know if we forget to SetFilter: it breaks the magicsock test,
so no useful information is lost.
Resolves#559.
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
The test demonstrates that magicsock can traverse two stateful
firewalls facing each other, that each require localhost to
initiate connections.
Signed-off-by: David Anderson <danderson@tailscale.com>
This change adds to tsdns the ability to delegate lookups to upstream nameservers.
This is crucial for setting Magic DNS as the system resolver.
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
Never return "nil, nil" anymore. The caller expected a usable
interface now. I missed some of these earlier.
Also, handle address deletion now.
Updates #532
There's a lot of confusion around what tailscale status shows, so make it better:
show region names, last write time, and put stars around DERP too if active.
Now stars are always present if activity, and always somewhere.
* fix tailscale status for peers using discovery
* as part of that, pull out disco address selection into reusable
and testable discoEndpoint.addrForSendLocked
* truncate ping/pong logged hex txids in half to eliminate noise
* move a bunch of random time constants into named constants
with docs
* track a history of per-endpoint pong replies for future use &
status display
* add "send" and " got" prefix to discovery message logging
immediately before the frame type so it's easier to read than
searching for the "<-" or "->" arrows earlier in the line; but keep
those as the more reasily machine readable part for later.
Updates #483
Update the mapping from ip:port to discokey, so when we retrieve a
packet from the network, we can find the same conn.Endpoint that we
gave to wireguard-go previously, without making it think we've
roamed. (We did, but we're not using its roaming.)
Updates #483
Ping messages now go out somewhat regularly, pong replies are sent,
and pong replies are now partially handled enough to upgrade off DERP
to LAN.
CallMeMaybe packets are sent & received over DERP, but aren't yet
handled. That's next (and regular maintenance timers), and then WAN
should work.
Updates #483
Starting at yesterday's e96f22e560 (convering some UDPAddrs to
IPPorts), Conn.ReceiveIPv4 could return a nil addr, which would make
its way through wireguard-go and blow up later. The DERP read path
wasn't initializing the addr result parameter any more, and wgRecvAddr
wasn't checking it either.
Fixes#515
And while plumbing, a bit of discovery work I'll need: the
endpointOfAddr map to map from validated paths to the discoEndpoint.
Not being populated yet.
Updates #483
This adds a new magicsock endpoint type only used when both sides
support discovery (that is, are advertising a discovery
key). Otherwise the old code is used.
So far the new code only communicates over DERP as proof that the new
code paths are wired up. None of the actually discovery messaging is
implemented yet.
Support for discovery (generating and advertising a key) are still
behind an environment variable for now.
Updates #483
The new deepprint package just walks a Go data structure and writes to
an io.Writer. It's not pretty like go-spew, etc.
We then use it to replace the use of UAPI (which we have a TODO to
remove) to generate signatures of data structures to detect whether
anything changed (without retaining the old copy).
This was necessary because the UAPI conversion ends up trying to do
DNS lookups which an upcoming change depends on not happening.
And track known peers.
Doesn't yet do anything with the messages. (nor does it send any yet)
Start of docs on the message format. More will come in subsequent changes.
Updates #483
The NetworkExtension brings up the interface itself and does not have
access to `ifconfig`, which the underlying BSD userspace router attempts
to use when Up is called.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
If there's been 5 minutes of inactivity, stop doing STUN lookups. That
means NAT mappings will expire, but they can resume later when there's
activity again.
We'll do this for all platforms later.
Updates tailscale/corp#320
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Darwin and FreeBSD are compatible enough to share the userspace router.
The OSX router delegates to the BSD userspace router unless `SetRoutesFunc` is set.
That preserves the mechanism that allows `ipn-go-bridge` to specify its own routing behavior.
Fixes#177
Signed-off-by: Reinaldo de Souza <github@rei.nal.do>
The magicsock derpReader was holding onto 65KB for each DERP
connection forever, just in case.
Make the derp{,http}.Client be in charge of memory instead. It can
reuse its bufio.Reader buffer space.
macOS incorrectly sends packets for the local Tailscale IP
into our tunnel interface. We have to turn the packets around
and send them back to the kernel.
Fixestailscale/corp#189.
Signed-off-by: David Anderson <danderson@tailscale.com>
iproute2 3.16.0-2 from Debian Jessie (oldoldstable) doesn't return
exit code 2 when deleting a non-existent IP rule.
Fixes#434
Signed-off-by: David Anderson <danderson@tailscale.com>
We have a filter in tailscaled itself now, which is more robust
against weird network topologies (such as the one Docker creates).
Signed-off-by: David Anderson <danderson@tailscale.com>
We'll use SO_BINDTODEVICE instead of fancy policy routing. This has
some limitations: for example, we will route all traffic through the
interface that has the main "default" (0.0.0.0/0) route, so machines
that have multiple physical interfaces might have to go through DERP to
get to some peers. But machines with multiple physical interfaces are
very likely to have policy routing (ip rule) support anyway.
So far, the only OS I know of that needs this feature is ChromeOS
(crostini). Fixes#245.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
Otherwise iOS/macOS will reconfigure their routing every time anything
minor changes in the netmap (in particular, endpoints and DERP homes),
which is way too often.
Some users reported "network reconfigured" errors from Chrome when this
happens.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
This allows tailscaled's own traffic to bypass Tailscale-managed routes,
so that things like tailscale-provided default routes don't break
tailscaled itself.
Progress on #144.
Signed-off-by: David Anderson <danderson@tailscale.com>
We canceled the pingers in Close, but didn't wait around for their
goroutines to be cleaned up. This caused the ipn/e2e_test to catch
pingers in its resource leak check.
This commit introduces an object, but also simplifies the semantics
around the pinger's cancel functions. They no longer need to be called
while holding the mutex.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
Specifically, this sequence:
iptables -N ts-forward
iptables -A ts-forward -m mark --mark 0x10000 -j ACCEPT
iptables -A FORWARD -j ts-forward
doesn't work on Debian-9-using-nftables, but this sequence:
iptables -N ts-forward
iptables -A FORWARD -j ts-forward
iptables -A ts-forward -m mark --mark 0x10000 -j ACCEPT
does work.
I'm sure the reason why is totally fascinating, but it's an old version
of iptables and the bug doesn't seem to exist on modern nftables, so
let's refactor our code to add rules in the always-safe order and
pretend this never happened.
Fixes#401.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
On startup, and when switching into =off and =nodivert, we were
deleting netfilter rules even if we weren't the ones that added them.
In order to avoid interfering with rules added by the sysadmin, we have
to be sure to delete rules only in the case that we added them in the
first place.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
Instead of retrieving the list of chains, or the list of rules in a
chain, just try deleting the ones we don't want and then adding the
ones we do want. An error in flushing/deleting still means the rule
doesn't exist anymore, so there was no need to check for it first.
This avoids the need to parse iptables output, which avoids the need to
ever call iptables -S, which fixes#403, among other things. It's also
much more future proof in case the iptables command line changes.
Unfortunately the iptables go module doesn't properly pass the iptables
command exit code back up when doing .Delete(), so we can't correctly
check the exit code there. (exit code 1 really means the rule didn't
exist, rather than some other weird problem).
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
This removes the use of suppress_ifgroup and fwmark "x/y" notation,
which are, among other things, not available in busybox and centos6.
We also use the return codes from the 'ip' program instead of trying to
parse its output.
I also had to remove the previous hack that routed all of 100.64.0.0/10
by default, because that would add the /10 route into the 'main' route
table instead of the new table 88, which is no good. It was a terrible
hack anyway; if we wanted to capture that route, we should have
captured it explicitly as a subnet route, not as part of the addr. Note
however that this change affects all platforms, so hopefully there
won't be any surprises elsewhere.
Fixes#405
Updates #320, #144
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
Instead of hard-coding the DERP map (except for cmd/tailscale netcheck
for now), get it from the control server at runtime.
And make the DERP map support multiple nodes per region with clients
picking the first one that's available. (The server will balance the
order presented to clients for load balancing)
This deletes the stunner package, merging it into the netcheck package
instead, to minimize all the config hooks that would've been
required.
Also fix some test flakes & races.
Fixes#387 (Don't hard-code the DERP map)
Updates #388 (Add DERP region support)
Fixes#399 (wgengine: flaky tests)
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The comment module is compiled out on several embedded systems (and
also gentoo, because netfilter can't go brrrr with comments holding it
back). Attempting to use comments results in a confusing error, and a
non-functional firewall.
Additionally, make the legacy rule cleanup non-fatal, because we *do*
have to probe for the existence of these -m comment rules, and doing
so will error out on these systems.
Signed-off-by: David Anderson <danderson@tailscale.com>
Our new build scripts try to build ipn-go-bridge on more than just
linux and darwin, so let's enable this file so it can be successful on
every platform.
This didn't catch anything yet, but it's good practice for detecting
goroutine leaks that we might not find otherwise.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
If a test calls log.Printf, 'go test' horrifyingly rearranges the
output to no longer be in chronological order, which makes debugging
virtually impossible. Let's stop that from happening by making
log.Printf panic if called from any module, no matter how deep, during
tests.
This required us to change the default error handler in at least one
http.Server, as well as plumbing a bunch of logf functions around,
especially in magicsock and wgengine, but also in logtail and backoff.
To add insult to injury, 'go test' also rearranges the output when a
parent test has multiple sub-tests (all the sub-test's t.Logf is always
printed after all the parent tests t.Logf), so we need to screw around
with a special Logf that can point at the "current" t (current_t.Logf)
in some places. Probably our entire way of using subtests is wrong,
since 'go test' would probably like to run them all in parallel if you
called t.Parallel(), but it definitely can't because the're all
manipulating the shared state created by the parent test. They should
probably all be separate toplevel tests instead, with common
setup/teardown logic. But that's a job for another time.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
Right now, filtering and packet injection in wgengine depend
on a patch to wireguard-go that probably isn't suitable for upstreaming.
This need not be the case: wireguard-go/tun.Device is an interface.
For example, faketun.go implements it to mock a TUN device for testing.
This patch implements the same interface to provide filtering
and packet injection at the tunnel device level,
at which point the wireguard-go patch should no longer be necessary.
This patch has the following performance impact on i7-7500U @ 2.70GHz,
tested in the following namespace configuration:
┌────────────────┐ ┌─────────────────────────────────┐ ┌────────────────┐
│ $ns1 │ │ $ns0 │ │ $ns2 │
│ client0 │ │ tailcontrol, logcatcher │ │ client1 │
│ ┌─────┐ │ │ ┌──────┐ ┌──────┐ │ │ ┌─────┐ │
│ │vethc│───────┼────┼──│vethrc│ │vethrs│──────┼─────┼──│veths│ │
│ ├─────┴─────┐ │ │ ├──────┴────┐ ├──────┴────┐ │ │ ├─────┴─────┐ │
│ │10.0.0.2/24│ │ │ │10.0.0.1/24│ │10.0.1.1/24│ │ │ │10.0.1.2/24│ │
│ └───────────┘ │ │ └───────────┘ └───────────┘ │ │ └───────────┘ │
└────────────────┘ └─────────────────────────────────┘ └────────────────┘
Before:
---------------------------------------------------
| TCP send | UDP send |
|------------------------|------------------------|
| 557.0 (±8.5) Mbits/sec | 3.03 (±0.02) Gbits/sec |
---------------------------------------------------
After:
---------------------------------------------------
| TCP send | UDP send |
|------------------------|------------------------|
| 544.8 (±1.6) Mbits/sec | 3.13 (±0.02) Gbits/sec |
---------------------------------------------------
The impact on receive performance is similar.
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
This saves a layer of translation, and saves us having to
pass in extra bits and pieces of the netmap and prefs to
wgengine. Now it gets one Wireguard config, and one OS
network stack config.
Signed-off-by: David Anderson <danderson@tailscale.com>
Defensive programming against #368 in environments other than Docker,
e.g. if you try using Tailscale in Alpine Linux directly, sans
container.
Signed-off-by: David Anderson <danderson@tailscale.com>
The iptables package we use doesn't include command output, so we're
left with guessing what went wrong most of the time. This will at
least narrow things down to which operation failed.
Signed-off-by: David Anderson <danderson@tailscale.com>
Instead, pass in only exactly the relevant configuration pieces
that the OS network stack cares about.
Signed-off-by: David Anderson <danderson@tailscale.com>
New logic installs precise filters for subnet routes,
plays nice with other users of netfilter, and lays the
groundwork for fixing routing loops via policy routing.
Signed-off-by: David Anderson <danderson@tailscale.com>
This depends on improved support from the control server, to send the
new subnet width (Bits) fields. If these are missing, we fall back to
assuming their value is /32.
Conversely, if the server sends Bits fields to an older client, it will
interpret them as /32 addresses. Since the only rules we allow are
"accept" rules, this will be narrower or equal to the intended rule, so
older clients will simply reject hosts on the wider subnet (fail
closed).
With this change, the internal filter.Matches format has diverged
from the wire format used by controlclient, so move the wire format
into tailcfg and convert it to filter.Matches in controlclient.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
Longer term, we should probably update the packet filter to be fully
stateful, for both TCP and ICMP. That is, only ICMP packets related to
a session *we* initiated should be allowed back in. But this is
reasonably secure for now, since wireguard is already trimming most
traffic. The current code would not protect against eg. Ping-of-Death style
attacks from VPN nodes.
Fixestailscale/tailscale#290.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
This was only done occasionally, but was extremely disruptive
when done and is no longer necessary.
It used to be that when switching links, we had to immediately
generate handshakes to everyone we were communicating with to
punch a hole in any NAT we were talking through. (This ended up
not really working, because in the process we got rid of our
session keys and ended up having a futile conversation for many
seconds.)
Now we have DERP, our link change propogates to the other side
as a new list of endpoints, so they start spraying packets.
We will definitely get one thanks to DERP, which will cause us
to spray, opening any NAT we are behind.
The result is that for good connections, we don't trash session
keys and cause an interruption.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
It was one of the top garbage producers on my phone.
It's slated to be deleted and replaced anyway, but this helps in the
meantime.
The go.sum changes look scary, but the new dep only adds 240 bytes to
the binary. The go.sum noise is just cmd/go being aggressive in
including a lot of stuff (which is being fixed in Go 1.15, for what I
understand). And I ran a go mod tidy, which added some too. (I had to
write a custom wrapper around go mod tidy because this mod tidy
normally breaks on tailscale.io/control being missing but referenced
in tests)
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The docs on magicsock.Conn stated that they implemented the
wireguard/device.Bind interface, yet this type does not exist. In
reality, the Conn type implements the wireguard/conn.Bind interface.
I also fixed a small typo in the same file.
Signed-off-by: Blake Gentry <blakesgentry@gmail.com>
* remove endpoint discovery noise when results unchanged
* consistently spell derp nodes as "derp-N"
* replace "127.3.3.40:" with "derp-" in CreateEndpoint log output
* stop early DERP setup before SetPrivateKey is called;
it just generates log nosie
* fix stringification of peer ShortStrings (it had an old %x on it,
rendering it garbage)
* describe why derp routes are changing, with one of:
shared home, their home, our home, alt
Add opt-in method to request IPv6 endpoints from the control plane.
For now they should just be skipped. A previous version of this CL was
unconditional and reportedly had problems that I can't reproduce. So
make it a knob until the mystery is solved.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Breaks something deep in wireguard or magicsock's brainstem, no packets at all
can flow. All received packets fail decryption with "invalid mac1".
This reverts commit 94024355ed.
Signed-off-by: David Anderson <dave@natulte.net>
More steps towards IPv6 transport.
We now send it to tailcontrol, which ignores it.
But it doesn't actually actually support IPv6 yet (outside of STUN).
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Use this when making the ipn state transition from Starting to
Running. This way a network of quiet nodes with no active
handshaking will still transition to Active.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
Typically the home DERP server is found and set on startup before
magicsock's SetPrivateKey can be called, so no DERP connection is
established. Make sure one is by kicking the home DERP tires in
SetPrivateKey.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
The code as written intended to do this, but it repeated the
comparison of derpNum and c.myDerp after c.myDerp had been
updated, so it never executed.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
Before, endpoint updates were constantly being interrupted and resumed
on Linux due to tons of LinkChange messages from over-zealous Linux
netlink messages (from router_linux.go)
Now that endpoint updates are fast and bounded in time anyway, just
let them run to completion, but note that another needs to be
scheduled after.
Now logs went from pages of noise to just:
root@taildoc:~# grep -i -E 'stun|endpoint update' log
2020/03/13 08:51:29 magicsock.Conn: starting endpoint update (initial)
2020/03/13 08:51:30 magicsock.Conn.ReSTUN: endpoint update active, need another later ("link-change-minor")
2020/03/13 08:51:31 magicsock.Conn: starting endpoint update (link-change-minor)
2020/03/13 08:51:31 magicsock.Conn.ReSTUN: endpoint update active, need another later ("link-change-minor")
2020/03/13 08:51:33 magicsock.Conn: starting endpoint update (link-change-minor)
2020/03/13 08:51:33 magicsock.Conn.ReSTUN: endpoint update active, need another later ("link-change-minor")
2020/03/13 08:51:35 magicsock.Conn: starting endpoint update (link-change-minor)
2020/03/13 08:51:35 magicsock.Conn.ReSTUN: endpoint update active, need another later ("link-change-minor")
Or, seen in another run:
2020/03/13 08:45:41 magicsock.Conn: starting endpoint update (periodic)
2020/03/13 08:46:09 magicsock.Conn: starting endpoint update (periodic)
2020/03/13 08:46:21 magicsock.Conn: starting endpoint update (link-change-major)
2020/03/13 08:46:37 magicsock.Conn: starting endpoint update (periodic)
2020/03/13 08:47:05 magicsock.Conn: starting endpoint update (periodic)
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The TODO above derphttp.NewClient suggests it does network I/O,
but the derphttp client connects lazily and so creating one is
very cheap.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
It used to make assumptions based on having Anycast IPs that are super
near. Now we're intentionally going to a bunch of different distant
IPs to measure latency.
Also, optimize how the hairpin detection works. No need to STUN on
that socket. Just use that separate socket for sending, once we know
the other UDP4 socket's endpoint. The trick is: make our test probe
also a STUN packet, so it fits through magicsock's existing STUN
routing.
This drops netcheck from ~5 seconds to ~250-500ms.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Failure to do this leads to fd exhaustion at -count=10000,
and increasingly poor execution north of -count=100.
Signed-off-by: David Anderson <danderson@tailscale.com>
Failure to do so triggers either a data race or a panic
in the testing package, due to racey use of t.Logf.
Signed-off-by: David Anderson <danderson@tailscale.com>
Basically, don't trust the OS-level link monitor to only tell you
interesting things. Sanity check it.
Also, move the interfaces package into the net directory now that we
have it.
This was (presumably) missing from wgengine because the
interactions between magicsock and wireguard-go meant that the
shutdown never worked. Now those are fixed, actually shut down.
Fixes occasional flake in expanded ipn/e2e_test.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
The device name "tailscale0" will be used for all platforms except for
OpenBSD where "tun" is enforced by the kernel. `CreateTUN()` in
`wireguard-go` will select the next available "tunX" device name on the
OpenBSD system.
Signed-off-by: Martin Baillie <martin@baillie.email>
The UDP reader goroutine was clobbering `n` and `err` from the
main goroutine, whose accesses are not synchronized the way `b` is.
Signed-off-by: David Anderson <danderson@tailscale.com>
wireguard-go closes magicsock, and expects this to unblock reads
so that its internal goroutines can wind down. We were incorrectly
blocking the read indefinitey and breaking this contract.
Signed-off-by: David Anderson <danderson@tailscale.com>
It's extremely flaky in several dimensions, as well as very slow.
It's making the CI completely red all the time without telling us
useful information.
Set RUN_CURSED_TESTS=1 to run locally.
This change just alters the semantics of the one flaky test, without
trying to speed up timeouts on the others. Empirically, speeding up
the timeouts causes _more_ flakes right now :(
The remaining flake occurs due to a mysterious packet loss. This
doesn't affect normal tailscaled operations, so until I track down
where the loss occurs and fix it, the flaky test is going to be
lenient about packet loss (but not about whether the spray logic
worked).
Signed-off-by: David Anderson <danderson@tailscale.com>
It previously passed incorrectly due to bugs. With those fixed,
it becomes flaky for 2 reasons. One of them is the wireguard handshake
race, which can eat the 1st sprayed packet and prevent roamAddr
discovery. This change fixes that failure, by spreading the test
traffic out enough that additional spraying occurs.
Signed-Off-By: David Anderson <danderson@tailscale.com>
The previous code would skip the DERP short-circuit if roamAddr
was set, which is not what we wanted. More generally, hitting
any of the fast path conditions is a direct return, so we can
just have 3 standalone branches rather than 'else if' stuff.
Signed-Off-By: David Anderson <danderson@tailscale.com>
The effect is subtle: when we're not spraying packets, and have not yet
figured out a curAddr, and we're not spraying, we end up sending to
whatever the first IP is in the iteration order. In English, that
means "when we have no idea where to send packets, and we've given
up on sending to everyone, just send to the first addr we see in
the list."
This is, in general, what we want, because the addrs are in sorted
preference order, low to high, and DERP is the least preferred
destination. So, when we have no idea where to send, send to DERP,
right?
... Except for very historical reasons, appendDests iterated through
addresses in _reverse_ order, most preferred to least preferred.
crawshaw@ believes this was part of the earliest handshaking
algorithm magicsock had, where it slowly iterated through possible
destinations and poked handshakes to them one at a time.
Anyway, because of this historical reverse iteration, in the case
described above of "we have no idea where to send", the code would
end up sending to the _most_ preferred candidate address, rather
than the _least_ preferred. So when in doubt, we'd end up firing
packets into the blackhole of some LAN address that doesn't work,
and connectivity would not work.
This case only comes up if all your non-DERP connectivity options
have failed, so we more or less failed to detect it because we
didn't have a pathological test box deployed. Worse, codependent
bug 2839854994 made DERP accidentally
work sometimes anyway by incorrectly exploiting roamAddr behavior,
albeit at the cost of making DERP traffic symmetric. In fixing
DERP to once again be asymmetric, we effectively removed the
bandaid that was concealing this bug.
Signed-Off-By: David Anderson <danderson@tailscale.com>
DERP traffic is asymmetric by design, with nodes always sending
to their peer's home DERP server. However, if roamAddr is set,
magicsock will always push data there, rather than let DERP
server selection do its thing, so we end up accidentally
creating a symmetric flow.
Signed-Off-By: David Anderson <danderson@tailscale.com>
I started to write a full DNS caching resolver and I realized it was
overkill and wouldn't work on Windows even in Go 1.14 yet, so I'm
doing this tiny one instead for now, just for all our netcheck STUN
derp lookups, and connections to DERP servers. (This will be caching a
exactly 8 DNS entries, all ours.)
Fixes#145 (can be better later, of course)
The value predates the introduction of AddrSet which replaces
the index by tracking curAddr directly.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
This avoids a non-obvious data race, where the JSON decoder ends
up creating do-nothing writes into global variables.
==================
WARNING: DATA RACE
Write at 0x0000011e1860 by goroutine 201:
tailscale.com/wgengine/packet.(*IP).UnmarshalJSON()
/home/crawshaw/repo/corp/oss/wgengine/packet/packet.go:83 +0x2d9
encoding/json.(*decodeState).literalStore()
/home/crawshaw/go/go/src/encoding/json/decode.go:877 +0x445e
...
encoding/json.Unmarshal()
/home/crawshaw/go/go/src/encoding/json/decode.go:107 +0x1de
tailscale.com/control/controlclient.(*Direct).decodeMsg()
/home/crawshaw/repo/corp/oss/control/controlclient/direct.go:615 +0x1ab
tailscale.com/control/controlclient.(*Direct).PollNetMap()
/home/crawshaw/repo/corp/oss/control/controlclient/direct.go:525 +0x1053
tailscale.com/control/controlclient.(*Client).mapRoutine()
/home/crawshaw/repo/corp/oss/control/controlclient/auto.go:428 +0x3a6
Previous read at 0x0000011e1860 by goroutine 86:
tailscale.com/wgengine/filter.matchIPWithoutPorts()
/home/crawshaw/repo/corp/oss/wgengine/filter/match.go:108 +0x91
tailscale.com/wgengine/filter.(*Filter).runIn()
/home/crawshaw/repo/corp/oss/wgengine/filter/filter.go:147 +0x3c6
tailscale.com/wgengine/filter.(*Filter).RunIn()
/home/crawshaw/repo/corp/oss/wgengine/filter/filter.go:127 +0xb0
tailscale.com/wgengine.(*userspaceEngine).SetFilter.func1()
/home/crawshaw/repo/corp/oss/wgengine/userspace.go:390 +0xfc
github.com/tailscale/wireguard-go/device.(*Device).RoutineDecryption()
/home/crawshaw/repo/corp/wireguard-go/device/receive.go:295 +0xa1f
For #112
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
Because wgLock is held while some wireguard-go methods run,
trying to hold wgLock during HandshakeDone potentially creates
lock cycles between wgengine and internals of wireguard-go.
Arguably wireguard-go should call HandshakeDone in a new goroutine,
but until its API promises that, don't make any assumptions here.
Maybe for #110.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
For 3 seconds after a successful handshake, wgengine will send a
ping packet every 300ms to its peer. This ensures the spray logic
in magicsock has something to spray.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
In particular, this is designed to catch the case where a
HandshakeInitiation packet is sent out but the intermediate NATs
have not been primed, so the packet passes over DERP.
In that case, the HandshakeResponse also comes back over DERP,
and the connection proceeds via DERP without ever trying to punch
through the NAT.
With this change, the HandshakeResponse (which was sprayed out
and so primed one NAT) triggers an UpdateDst, which triggers
the extra spray logic.
(For this to work, there has to be an initial supply of packets
to send on to a peer for the three seconds following a handshake.
The source of these packets is left as a future exercise.)
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
