Commit Graph

89 Commits (643bf14653484295400dd6f11fc5bd98e53c8f3e)

Author SHA1 Message Date
David Anderson 643bf14653 wgengine/magicsock: disable the new ping test.
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.
5 years ago
David Anderson c8ebac2def wgengine/magicsock: try deflaking again.
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 :(
5 years ago
David Anderson cd1ac63b4c Revert "wgengine/magicsock: temporarily deflake."
This reverts commit c5835c6ced.
5 years ago
David Anderson c5835c6ced wgengine/magicsock: temporarily deflake.
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>
5 years ago
Brad Fitzpatrick 61d83f759b wgengine/magicsock: remove redundant derpMagicIP comparison
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
David Anderson bd60a750e8 wgengine/magicsock: fix packet spraying test to (mostly) pass.
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>
5 years ago
David Anderson ef31dd7bb5 wgengine/magicsock: check all 3 fast paths independently.
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>
5 years ago
David Anderson 05a52746a4 wgengine/magicsock: fix destination selection logic to work with DERP.
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>
5 years ago
David Anderson 97e58ad44d wgengine/magicsock: only set addrByKey once in CreateEndpoint.
Signed-Off-By: David Anderson <danderson@tailscale.com>
5 years ago
Brad Fitzpatrick fbab12c94c wgengine/magicsock: skip netcheck if external STUN aren't in use
Updates #146 (not a complete fix yet probably)
5 years ago
Brad Fitzpatrick fe0051fafd wgengine/magicsock: expand AddrSet.addrs comment 5 years ago
David Anderson 2839854994 wgengine/magicsock: never set a DERP server as a roamAddr.
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>
5 years ago
David Anderson 4f5c0da1ae wgengine/magicsock: log when home DERP server changes. 5 years ago
Brad Fitzpatrick 6978b93bdd derp, magicsock: track home (preferred) vs visiting connections for stats 5 years ago
Brad Fitzpatrick 12b77f30ad wgengine/magicsock: close stale DERP connections 5 years ago
Brad Fitzpatrick 2cff9016e4 net/dnscache: add overly simplistic DNS cache package for selective use
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)
5 years ago
Brad Fitzpatrick a36ccb8525 wgengine/magicsock: actually add to the activeDerp map
Fixes bug just introduced in 8f9849c140; not tested enough :(
5 years ago
Brad Fitzpatrick 8f9849c140 wgengine/magicsock: collapse three DERP maps down into one 5 years ago
Brad Fitzpatrick 40ebba1373 magicsock: use [unexpected] convention more
Fixes #136 (not entirely, but we have a convention now)
5 years ago
Brad Fitzpatrick 848a2bddf0 wgengine/magicsock: update set of DERP nodes 5 years ago
David Crawshaw 7932481b95 magicsock: lookup AddrSet by key from DERP
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
Brad Fitzpatrick eac62ec5ff ipn, wgengine/magicsock: add ipn.Prefs.DisableDERP bool
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
Brad Fitzpatrick bf704a5218 derp: protocol negotiation, add v2: send src pub keys to clients in packets
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
David Crawshaw a65b2a0efd magicsock: add some DERP tests
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw a33419167b magicsock: plumb through derpTLSConfig variable (for testing)
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw caec2c7e8b magicsock: test sequence of pings
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw 9f584414d9 magicsock: simple ping test via magicsock
Passes `go test -count=20 -race ./wgengine/magicsock`

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw 34859f8e7d wgengine, magicsock: add a CreateBind method
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw 75e62d318f magicsock: use local STUN server in tests
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
Brad Fitzpatrick b27d4c017a magicsock, wgengine, ipn, controlclient: plumb regular netchecks to map poll
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
Brad Fitzpatrick 724c37fb41 wgengine/magicsock: start tracking nearest DERP node 5 years ago
Brad Fitzpatrick 89a2c3eb04 wgengine: don't create duplicate iptables rules on Linux, clean up
Fixes #131

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
Brad Fitzpatrick 657f9593ae Reduce some logspam. 5 years ago
Brad Fitzpatrick 4675c70464 wgengine/magicsock: check STUN regularly 5 years ago
Brad Fitzpatrick bc7bc43fb8 magicsock, interfaces: move some code from magicsock to interfaces 5 years ago
Brad Fitzpatrick af7a01d6f0 wgengine/magicsock: drop donec channel, rename epUpdateCtx to serve its purpose 5 years ago
David Crawshaw cc4afa775f magicsock: rate limit send error log messages
The x/time/rate dependency adds 24kb to tailscaled binary size.

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw 0752c77dc2 magicsock: keep DERP magic IPs out of the address map
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw c6550135d5 magicsock: remove the index from indexedAddrs
The value predates the introduction of AddrSet which replaces
the index by tracking curAddr directly.

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
David Crawshaw d417be6a4b controlclinet: clone filter.MatchAllowAll
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>
5 years ago
Brad Fitzpatrick 1abf2da392 wgengine/magicsock: reset favorite address on handshakes
Updates #92 (not a complete fix; could be better/faster?)

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
Brad Fitzpatrick 67ede8d6d2 wgengine, magicsock: fix SetPrivateKey data race
Updates #112

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
Brad Fitzpatrick 19b54d0ae7 wgengine: fix a data race on StatusCallback
Updates tailscale/tailscale#112

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
David Crawshaw c576a57067 wgengine: avoid holding any locks during HandshakeDone
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>
5 years ago
Brad Fitzpatrick c185e6b4b0 stunner: support IPv6, add latency info to callbacks, use unique TxIDs per retry
And some more docs.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago
David Crawshaw 44670d0da9 wgengine: revert wgdev.Close on Close from last commit
Causes as-yet-unknown problems in some tests.

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
5 years ago
wardn 9390a3ef55 wgengine: properly clean up freebsd routes and interfaces on close
Signed-off-by: wardn <wardn@users.noreply.github.com>
5 years ago
David Crawshaw 7a3be96199 wgengine: add pinger to generate initial spray packets
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>
5 years ago
David Crawshaw a6ad3c46e2 magicsock: spray some normal packets after a handshake
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>
5 years ago
Brad Fitzpatrick 8696b17b5f wgengine/magicsock: turn off DERP log spamminess by default
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
5 years ago