From 97693f2e42294a0fe21126e8ff88aa88b6d82e2c Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 25 Aug 2021 19:39:20 -0700 Subject: [PATCH] wgengine/magicsock: delete legacy AddrSet endpoints. Instead of using the legacy codepath, teach discoEndpoint to handle peers that have a home DERP, but no disco key. We can still communicate with them, but only over DERP. Signed-off-by: David Anderson --- cmd/tailscaled/depaware.txt | 4 +- wgengine/bench/bench_test.go | 2 +- wgengine/bench/wg.go | 4 - wgengine/magicsock/legacy.go | 642 ---------------------- wgengine/magicsock/magicsock.go | 777 +++++++++++++++------------ wgengine/magicsock/magicsock_test.go | 420 +++++---------- wgengine/wgcfg/clone.go | 29 +- wgengine/wgcfg/config.go | 80 +-- wgengine/wgcfg/device_test.go | 5 +- wgengine/wgcfg/nmcfg/nmcfg.go | 25 - wgengine/wgcfg/writer.go | 2 +- 11 files changed, 596 insertions(+), 1394 deletions(-) delete mode 100644 wgengine/magicsock/legacy.go diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 44e8d12c4..8e1416352 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -48,7 +48,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de golang.zx2c4.com/wireguard/ratelimiter from golang.zx2c4.com/wireguard/device golang.zx2c4.com/wireguard/replay from golang.zx2c4.com/wireguard/device golang.zx2c4.com/wireguard/rwcancel from golang.zx2c4.com/wireguard/device+ - golang.zx2c4.com/wireguard/tai64n from golang.zx2c4.com/wireguard/device+ + golang.zx2c4.com/wireguard/tai64n from golang.zx2c4.com/wireguard/device 💣 golang.zx2c4.com/wireguard/tun from golang.zx2c4.com/wireguard/device+ W 💣 golang.zx2c4.com/wireguard/tun/wintun from golang.zx2c4.com/wireguard/tun+ W 💣 golang.zx2c4.com/wireguard/windows/tunnel/winipcfg from tailscale.com/net/interfaces+ @@ -180,7 +180,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de W 💣 tailscale.com/wgengine/winnet from tailscale.com/wgengine/router golang.org/x/crypto/acme from tailscale.com/ipn/localapi golang.org/x/crypto/blake2b from golang.org/x/crypto/nacl/box - golang.org/x/crypto/blake2s from golang.zx2c4.com/wireguard/device+ + golang.org/x/crypto/blake2s from golang.zx2c4.com/wireguard/device golang.org/x/crypto/chacha20 from golang.org/x/crypto/chacha20poly1305 golang.org/x/crypto/chacha20poly1305 from crypto/tls+ golang.org/x/crypto/cryptobyte from crypto/ecdsa+ diff --git a/wgengine/bench/bench_test.go b/wgengine/bench/bench_test.go index a34b2dcbe..20c09664d 100644 --- a/wgengine/bench/bench_test.go +++ b/wgengine/bench/bench_test.go @@ -42,7 +42,7 @@ func BenchmarkBatchTCP(b *testing.B) { } func BenchmarkWireGuardTest(b *testing.B) { - b.Skip("setup code doesn't support disco yet") + b.Skip("https://github.com/tailscale/tailscale/issues/2716") run(b, func(logf logger.Logf, traf *TrafficGen) { setupWGTest(b, logf, traf, Addr1, Addr2) }) diff --git a/wgengine/bench/wg.go b/wgengine/bench/wg.go index 84332c97f..a94364cae 100644 --- a/wgengine/bench/wg.go +++ b/wgengine/bench/wg.go @@ -99,10 +99,8 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd logf("e1 status: %v", *st) var eps []string - var ipps []netaddr.IPPort for _, ep := range st.LocalAddrs { eps = append(eps, ep.Addr.String()) - ipps = append(ipps, ep.Addr) } endpoint := wgcfg.Endpoints{ PublicKey: c1.PrivateKey.Public(), @@ -142,10 +140,8 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd logf("e2 status: %v", *st) var eps []string - var ipps []netaddr.IPPort for _, ep := range st.LocalAddrs { eps = append(eps, ep.Addr.String()) - ipps = append(ipps, ep.Addr) } endpoint := wgcfg.Endpoints{ PublicKey: c2.PrivateKey.Public(), diff --git a/wgengine/magicsock/legacy.go b/wgengine/magicsock/legacy.go deleted file mode 100644 index 373aef370..000000000 --- a/wgengine/magicsock/legacy.go +++ /dev/null @@ -1,642 +0,0 @@ -// Copyright (c) 2019 Tailscale Inc & AUTHORS All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package magicsock - -import ( - "bytes" - "crypto/hmac" - "crypto/subtle" - "encoding/binary" - "errors" - "hash" - "net" - "strings" - "sync" - "time" - - "golang.org/x/crypto/blake2s" - "golang.org/x/crypto/chacha20poly1305" - "golang.org/x/crypto/poly1305" - "golang.zx2c4.com/wireguard/conn" - "golang.zx2c4.com/wireguard/tai64n" - "inet.af/netaddr" - "tailscale.com/ipn/ipnstate" - "tailscale.com/tstime/mono" - "tailscale.com/types/key" - "tailscale.com/types/logger" - "tailscale.com/types/wgkey" - "tailscale.com/wgengine/wgcfg" -) - -var ( - errNoDestinations = errors.New("magicsock: no destinations") - errDisabled = errors.New("magicsock: legacy networking disabled") -) - -// createLegacyEndpointLocked creates a new wireguard-go endpoint for a legacy connection. -// pk is the public key of the remote peer. addrs is the ordered set of addresses for the remote peer. -// rawDest is the encoded wireguard-go endpoint string. It should be treated as a black box. -// It is provided so that addrSet.DstToString can return it when requested by wireguard-go. -func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs wgcfg.IPPortSet, rawDest string) (conn.Endpoint, error) { - if c.disableLegacy { - return nil, errDisabled - } - - a := &addrSet{ - Logf: c.logf, - publicKey: pk, - curAddr: -1, - rawdst: rawDest, - } - a.ipPorts = append(a.ipPorts, addrs.IPPorts()...) - - // If this endpoint is being updated, remember its old set of - // endpoints so we can remove any (from c.addrsByUDP) that are - // not in the new set. - var oldIPP []netaddr.IPPort - if preva, ok := c.addrsByKey[pk]; ok { - oldIPP = preva.ipPorts - } - c.addrsByKey[pk] = a - - // Add entries to c.addrsByUDP. - for _, ipp := range a.ipPorts { - if ipp.IP() == derpMagicIPAddr { - continue - } - c.addrsByUDP[ipp] = a - } - - // Remove previous c.addrsByUDP entries that are no longer in the new set. - for _, ipp := range oldIPP { - if ipp.IP() != derpMagicIPAddr && c.addrsByUDP[ipp] != a { - delete(c.addrsByUDP, ipp) - } - } - - return a, nil -} - -func (c *Conn) findLegacyEndpointLocked(ipp netaddr.IPPort, packet []byte) conn.Endpoint { - if c.disableLegacy { - return nil - } - - // Pre-disco: look up their addrSet. - if as, ok := c.addrsByUDP[ipp]; ok { - as.updateDst(ipp) - return as - } - - // We don't know who this peer is. It's possible that it's one of - // our legitimate peers and they've roamed to an address we don't - // know. If this is a handshake packet, we can try to identify the - // peer in question. - if as := c.peerFromPacketLocked(packet); as != nil { - as.updateDst(ipp) - return as - } - - // We have no idea who this is, drop the packet. - // - // In the past, when this magicsock implementation was the main - // one, we tried harder to find a match here: we would pass the - // packet into wireguard-go with a "singleEndpoint" implementation - // that wrapped the UDPAddr. Then, a patch we added to - // wireguard-go would call UpdateDst on that singleEndpoint after - // decrypting the packet and identifying the peer (if any), - // allowing us to update the relevant addrSet. - // - // This was a significant out of tree patch to wireguard-go, so we - // got rid of it, and instead switched to this logic you're - // reading now, which makes a best effort to identify sources for - // handshake packets (because they're relatively easy to turn into - // a peer public key statelessly), but otherwise drops packets - // that come from "roaming" addresses that aren't known to - // magicsock. - // - // The practical consequence of this is that some complex NAT - // traversal cases will now fail between a very old Tailscale - // client (0.96 and earlier) and a very new Tailscale - // client. However, those scenarios were likely also failing on - // all-old clients, because the probabilistic NAT opening didn't - // work reliably. So, in practice, this simplification means - // connectivity looks like this: - // - // - old+old client: unchanged - // - old+new client (easy network topology): unchanged - // - old+new client (hard network topology): was bad, now a bit worse - // - new+new client: unchanged - // - // This degradation is acceptable in that it continues to support - // the incremental upgrade of old clients that currently work - // well, which is our primary goal for the <100 clients still left - // on the oldest pre-DERP versions (as of 2021-01-12). - return nil -} - -func (c *Conn) resetAddrSetStatesLocked() { - for _, as := range c.addrsByKey { - as.curAddr = -1 - as.stopSpray = as.timeNow().Add(sprayPeriod) - } -} - -func (c *Conn) sendAddrSet(b []byte, as *addrSet) error { - if c.disableLegacy { - return errDisabled - } - - var addrBuf [8]netaddr.IPPort - dsts, roamAddr := as.appendDests(addrBuf[:0], b) - - if len(dsts) == 0 { - return errNoDestinations - } - - var success bool - var ret error - for _, addr := range dsts { - sent, err := c.sendAddr(addr, as.publicKey, b) - if sent { - success = true - } else if ret == nil { - ret = err - } - if err != nil && addr != roamAddr && c.sendLogLimit.Allow() { - if c.connCtx.Err() == nil { // don't log if we're closed - c.logf("magicsock: Conn.Send(%v): %v", addr, err) - } - } - } - if success { - return nil - } - return ret -} - -// peerFromPacketLocked extracts returns the addrSet for the peer who sent -// packet, if derivable. -// -// The derived addrSet is a hint, not a cryptographically strong -// assertion. The returned value MUST NOT be used for any security -// critical function. Callers MUST assume that the addrset can be -// picked by a remote attacker. -func (c *Conn) peerFromPacketLocked(packet []byte) *addrSet { - if len(packet) < 4 { - return nil - } - msgType := binary.LittleEndian.Uint32(packet[:4]) - if msgType != messageInitiationType { - // Can't get peer out of a non-handshake packet. - return nil - } - - var msg messageInitiation - reader := bytes.NewReader(packet) - err := binary.Read(reader, binary.LittleEndian, &msg) - if err != nil { - return nil - } - - // Process just enough of the handshake to extract the long-term - // peer public key. We don't verify the handshake all the way, so - // this may be a spoofed packet. The extracted peer MUST NOT be - // used for any security critical function. In our case, we use it - // as a hint for roaming addresses. - var ( - pub = c.privateKey.Public() - hash [blake2s.Size]byte - chainKey [blake2s.Size]byte - peerPK key.Public - boxKey [chacha20poly1305.KeySize]byte - ) - - mixHash(&hash, &initialHash, pub[:]) - mixHash(&hash, &hash, msg.Ephemeral[:]) - mixKey(&chainKey, &initialChainKey, msg.Ephemeral[:]) - - ss := c.privateKey.SharedSecret(key.Public(msg.Ephemeral)) - if isZero(ss[:]) { - return nil - } - - kdf2(&chainKey, &boxKey, chainKey[:], ss[:]) - aead, _ := chacha20poly1305.New(boxKey[:]) - _, err = aead.Open(peerPK[:0], zeroNonce[:], msg.Static[:], hash[:]) - if err != nil { - return nil - } - - return c.addrsByKey[peerPK] -} - -func shouldSprayPacket(b []byte) bool { - if len(b) < 4 { - return false - } - msgType := binary.LittleEndian.Uint32(b[:4]) - switch msgType { - case messageInitiationType, - messageResponseType, - messageCookieReplyType: // TODO: necessary? - return true - } - return false -} - -const sprayPeriod = 3 * time.Second - -// appendDests appends to dsts the destinations that b should be -// written to in order to reach as. Some of the returned IPPorts may -// be fake addrs representing DERP servers. -// -// It also returns as's current roamAddr, if any. -func (as *addrSet) appendDests(dsts []netaddr.IPPort, b []byte) (_ []netaddr.IPPort, roamAddr netaddr.IPPort) { - spray := shouldSprayPacket(b) // true for handshakes - now := as.timeNow() - - as.mu.Lock() - defer as.mu.Unlock() - - as.lastSend = now - - // Spray logic. - // - // After exchanging a handshake with a peer, we send some outbound - // packets to every endpoint of that peer. These packets are spaced out - // over several seconds to make sure that our peer has an opportunity to - // send its own spray packet to us before we are done spraying. - // - // Multiple packets are necessary because we have to both establish the - // NAT mappings between two peers *and use* the mappings to switch away - // from DERP to a higher-priority UDP endpoint. - const sprayFreq = 250 * time.Millisecond - if spray { - as.lastSpray = now - as.stopSpray = now.Add(sprayPeriod) - - // Reset our favorite route on new handshakes so we - // can downgrade to a worse path if our better path - // goes away. (https://github.com/tailscale/tailscale/issues/92) - as.curAddr = -1 - } else if now.Before(as.stopSpray) { - // We are in the spray window. If it has been sprayFreq since we - // last sprayed a packet, spray this packet. - if now.Sub(as.lastSpray) >= sprayFreq { - spray = true - as.lastSpray = now - } - } - - // Pick our destination address(es). - switch { - case spray: - // This packet is being sprayed to all addresses. - for i := range as.ipPorts { - dsts = append(dsts, as.ipPorts[i]) - } - if as.roamAddr != nil { - dsts = append(dsts, *as.roamAddr) - } - case as.roamAddr != nil: - // We have a roaming address, prefer it over other addrs. - // TODO(danderson): this is not correct, there's no reason - // roamAddr should be special like this. - dsts = append(dsts, *as.roamAddr) - case as.curAddr != -1: - if as.curAddr >= len(as.ipPorts) { - as.Logf("[unexpected] magicsock bug: as.curAddr >= len(as.ipPorts): %d >= %d", as.curAddr, len(as.ipPorts)) - break - } - // No roaming addr, but we've seen packets from a known peer - // addr, so keep using that one. - dsts = append(dsts, as.ipPorts[as.curAddr]) - default: - // We know nothing about how to reach this peer, and we're not - // spraying. Use the first address in the array, which will - // usually be a DERP address that guarantees connectivity. - if len(as.ipPorts) > 0 { - dsts = append(dsts, as.ipPorts[0]) - } - } - - if logPacketDests { - as.Logf("spray=%v; roam=%v; dests=%v", spray, as.roamAddr, dsts) - } - if as.roamAddr != nil { - roamAddr = *as.roamAddr - } - return dsts, roamAddr -} - -// addrSet is a set of UDP addresses that implements wireguard/conn.Endpoint. -// -// This is the legacy endpoint for peers that don't support discovery; -// it predates discoEndpoint. -type addrSet struct { - publicKey key.Public // peer public key used for DERP communication - - // ipPorts is an ordered priority list provided by wgengine, - // sorted from expensive+slow+reliable at the begnining to - // fast+cheap at the end. More concretely, it's typically: - // - // [DERP fakeip:node, Global IP:port, LAN ip:port] - // - // But there could be multiple or none of each. - ipPorts []netaddr.IPPort - - // clock, if non-nil, is used in tests instead of time.Now. - clock func() mono.Time - Logf logger.Logf // must not be nil - - mu sync.Mutex // guards following fields - - lastSend mono.Time - - // roamAddr is non-nil if/when we receive a correctly signed - // WireGuard packet from an unexpected address. If so, we - // remember it and send responses there in the future, but - // this should hopefully never be used (or at least used - // rarely) in the case that all the components of Tailscale - // are correctly learning/sharing the network map details. - roamAddr *netaddr.IPPort - - // curAddr is an index into addrs of the highest-priority - // address a valid packet has been received from so far. - // If no valid packet from addrs has been received, curAddr is -1. - curAddr int - - // stopSpray is the time after which we stop spraying packets. - stopSpray mono.Time - - // lastSpray is the last time we sprayed a packet. - lastSpray mono.Time - - // loggedLogPriMask is a bit field of that tracks whether - // we've already logged about receiving a packet from a low - // priority ("low-pri") address when we already have curAddr - // set to a better one. This is only to suppress some - // redundant logs. - loggedLogPriMask uint32 - - // rawdst is the destination string from/for wireguard-go. - rawdst string -} - -// derpID returns this addrSet's home DERP node, or 0 if none is found. -func (as *addrSet) derpID() int { - for _, ua := range as.ipPorts { - if ua.IP() == derpMagicIPAddr { - return int(ua.Port()) - } - } - return 0 -} - -func (as *addrSet) timeNow() mono.Time { - if as.clock != nil { - return as.clock() - } - return mono.Now() -} - -var noAddr, _ = netaddr.FromStdAddr(net.ParseIP("127.127.127.127"), 127, "") - -func (a *addrSet) dst() netaddr.IPPort { - a.mu.Lock() - defer a.mu.Unlock() - - if a.roamAddr != nil { - return *a.roamAddr - } - if len(a.ipPorts) == 0 { - return noAddr - } - i := a.curAddr - if i == -1 { - i = 0 - } - return a.ipPorts[i] -} - -func (a *addrSet) DstToBytes() []byte { - return packIPPort(a.dst()) -} -func (a *addrSet) DstToString() string { - return a.rawdst -} -func (a *addrSet) DstIP() net.IP { - return a.dst().IP().IPAddr().IP // TODO: add netaddr accessor to cut an alloc here? -} -func (a *addrSet) SrcIP() net.IP { return nil } -func (a *addrSet) SrcToString() string { return "" } -func (a *addrSet) ClearSrc() {} - -// updateDst records receipt of a packet from new. This is used to -// potentially update the transmit address used for this addrSet. -func (a *addrSet) updateDst(new netaddr.IPPort) error { - if new.IP() == derpMagicIPAddr { - // Never consider DERP addresses as a viable candidate for - // either curAddr or roamAddr. It's only ever a last resort - // choice, never a preferred choice. - // This is a hot path for established connections. - return nil - } - - a.mu.Lock() - defer a.mu.Unlock() - - if a.roamAddr != nil && new == *a.roamAddr { - // Packet from the current roaming address, no logging. - // This is a hot path for established connections. - return nil - } - if a.roamAddr == nil && a.curAddr >= 0 && new == a.ipPorts[a.curAddr] { - // Packet from current-priority address, no logging. - // This is a hot path for established connections. - return nil - } - - index := -1 - for i := range a.ipPorts { - if new == a.ipPorts[i] { - index = i - break - } - } - - publicKey := wgkey.Key(a.publicKey) - pk := publicKey.ShortString() - old := "" - if a.curAddr >= 0 { - old = a.ipPorts[a.curAddr].String() - } - - switch { - case index == -1: - if a.roamAddr == nil { - a.Logf("[v1] magicsock: rx %s from roaming address %s, set as new priority", pk, new) - } else { - a.Logf("[v1] magicsock: rx %s from roaming address %s, replaces roaming address %s", pk, new, a.roamAddr) - } - a.roamAddr = &new - - case a.roamAddr != nil: - a.Logf("[v1] magicsock: rx %s from known %s (%d), replaces roaming address %s", pk, new, index, a.roamAddr) - a.roamAddr = nil - a.curAddr = index - a.loggedLogPriMask = 0 - - case a.curAddr == -1: - a.Logf("[v1] magicsock: rx %s from %s (%d/%d), set as new priority", pk, new, index, len(a.ipPorts)) - a.curAddr = index - a.loggedLogPriMask = 0 - - case index < a.curAddr: - if 1 <= index && index <= 32 && (a.loggedLogPriMask&1<<(index-1)) == 0 { - a.Logf("[v1] magicsock: rx %s from low-pri %s (%d), keeping current %s (%d)", pk, new, index, old, a.curAddr) - a.loggedLogPriMask |= 1 << (index - 1) - } - - default: // index > a.curAddr - a.Logf("[v1] magicsock: rx %s from %s (%d/%d), replaces old priority %s", pk, new, index, len(a.ipPorts), old) - a.curAddr = index - a.loggedLogPriMask = 0 - } - - return nil -} - -func (a *addrSet) String() string { - a.mu.Lock() - defer a.mu.Unlock() - - buf := new(strings.Builder) - buf.WriteByte('[') - if a.roamAddr != nil { - buf.WriteString("roam:") - sbPrintAddr(buf, *a.roamAddr) - } - for i, addr := range a.ipPorts { - if i > 0 || a.roamAddr != nil { - buf.WriteString(", ") - } - sbPrintAddr(buf, addr) - if a.curAddr == i { - buf.WriteByte('*') - } - } - buf.WriteByte(']') - - return buf.String() -} - -func (as *addrSet) populatePeerStatus(ps *ipnstate.PeerStatus) { - as.mu.Lock() - defer as.mu.Unlock() - - ps.LastWrite = as.lastSend.WallTime() - for i, ua := range as.ipPorts { - if ua.IP() == derpMagicIPAddr { - continue - } - uaStr := ua.String() - ps.Addrs = append(ps.Addrs, uaStr) - if as.curAddr == i { - ps.CurAddr = uaStr - } - } - if as.roamAddr != nil { - ps.CurAddr = ippDebugString(*as.roamAddr) - } -} - -// Message types copied from wireguard-go/device/noise-protocol.go -const ( - messageInitiationType = 1 - messageResponseType = 2 - messageCookieReplyType = 3 -) - -// Cryptographic constants copied from wireguard-go/device/noise-protocol.go -var ( - noiseConstruction = "Noise_IKpsk2_25519_ChaChaPoly_BLAKE2s" - wgIdentifier = "WireGuard v1 zx2c4 Jason@zx2c4.com" - initialChainKey [blake2s.Size]byte - initialHash [blake2s.Size]byte - zeroNonce [chacha20poly1305.NonceSize]byte -) - -func init() { - initialChainKey = blake2s.Sum256([]byte(noiseConstruction)) - mixHash(&initialHash, &initialChainKey, []byte(wgIdentifier)) -} - -// messageInitiation is the same as wireguard-go's MessageInitiation, -// from wireguard-go/device/noise-protocol.go. -type messageInitiation struct { - Type uint32 - Sender uint32 - Ephemeral wgkey.Key - Static [wgkey.Size + poly1305.TagSize]byte - Timestamp [tai64n.TimestampSize + poly1305.TagSize]byte - MAC1 [blake2s.Size128]byte - MAC2 [blake2s.Size128]byte -} - -func mixKey(dst *[blake2s.Size]byte, c *[blake2s.Size]byte, data []byte) { - kdf1(dst, c[:], data) -} - -func mixHash(dst *[blake2s.Size]byte, h *[blake2s.Size]byte, data []byte) { - hash, _ := blake2s.New256(nil) - hash.Write(h[:]) - hash.Write(data) - hash.Sum(dst[:0]) - hash.Reset() -} - -func hmac1(sum *[blake2s.Size]byte, key, in0 []byte) { - mac := hmac.New(func() hash.Hash { - h, _ := blake2s.New256(nil) - return h - }, key) - mac.Write(in0) - mac.Sum(sum[:0]) -} - -func hmac2(sum *[blake2s.Size]byte, key, in0, in1 []byte) { - mac := hmac.New(func() hash.Hash { - h, _ := blake2s.New256(nil) - return h - }, key) - mac.Write(in0) - mac.Write(in1) - mac.Sum(sum[:0]) -} - -func kdf1(t0 *[blake2s.Size]byte, key, input []byte) { - hmac1(t0, key, input) - hmac1(t0, t0[:], []byte{0x1}) -} - -func kdf2(t0, t1 *[blake2s.Size]byte, key, input []byte) { - var prk [blake2s.Size]byte - hmac1(&prk, key, input) - hmac1(t0, prk[:], []byte{0x1}) - hmac2(t1, prk[:], t0[:], []byte{0x2}) - for i := range prk[:] { - prk[i] = 0 - } -} - -func isZero(val []byte) bool { - acc := 1 - for _, b := range val { - acc &= subtle.ConstantTimeByteEq(b, 0) - } - return acc == 1 -} diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index b3e83716a..f78002795 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -72,6 +72,203 @@ func useDerpRoute() bool { return false } +// peerInfo is all the information magicsock tracks about a particular +// peer. +type peerInfo struct { + node *tailcfg.Node // always present + ep *discoEndpoint // optional, if wireguard-go isn't currently talking to this peer. + // ipPorts is an inverted version of peerMap.byIPPort (below), so + // that when we're deleting this node, we can rapidly find out the + // keys that need deleting from peerMap.byIPPort without having to + // iterate over every IPPort known for any peer. + ipPorts map[netaddr.IPPort]bool +} + +func newPeerInfo() *peerInfo { + return &peerInfo{ + ipPorts: map[netaddr.IPPort]bool{}, + } +} + +// peerMap is an index of peerInfos by node (WireGuard) key, disco +// key, and discovered ip:port endpoints. +// +// Doesn't do any locking, all access must be done with Conn.mu held. +type peerMap struct { + byDiscoKey map[tailcfg.DiscoKey]*peerInfo + byNodeKey map[tailcfg.NodeKey]*peerInfo + byIPPort map[netaddr.IPPort]*peerInfo +} + +func newPeerMap() peerMap { + return peerMap{ + byDiscoKey: map[tailcfg.DiscoKey]*peerInfo{}, + byNodeKey: map[tailcfg.NodeKey]*peerInfo{}, + byIPPort: map[netaddr.IPPort]*peerInfo{}, + } +} + +// nodeCount returns the number of nodes currently in m. +func (m *peerMap) nodeCount() int { + return len(m.byNodeKey) +} + +// nodeForDiscoKey returns the tailcfg.Node for dk. ok is true only if +// the disco key is known to us. +func (m *peerMap) nodeForDiscoKey(dk tailcfg.DiscoKey) (n *tailcfg.Node, ok bool) { + if dk.IsZero() { + return nil, false + } + if info, ok := m.byDiscoKey[dk]; ok { + return info.node, true + } + return nil, false +} + +// nodeForNodeKey returns the tailcfg.Node for nk. ok is true only if +// the node key is known to us. +func (m *peerMap) nodeForNodeKey(nk tailcfg.NodeKey) (n *tailcfg.Node, ok bool) { + if nk.IsZero() { + return nil, false + } + if info, ok := m.byNodeKey[nk]; ok { + return info.node, true + } + return nil, false +} + +// discoEndpointForDiscoKey returns the discoEndpoint for dk, or nil +// if dk is not known to us. +func (m *peerMap) discoEndpointForDiscoKey(dk tailcfg.DiscoKey) (ep *discoEndpoint, ok bool) { + if dk.IsZero() { + return nil, false + } + if info, ok := m.byDiscoKey[dk]; ok && info.ep != nil { + return info.ep, true + } + return nil, false +} + +// discoEndpointForNodeKey returns the discoEndpoint for nk, or nil if +// nk is not known to us. +func (m *peerMap) discoEndpointForNodeKey(nk tailcfg.NodeKey) (ep *discoEndpoint, ok bool) { + if nk.IsZero() { + return nil, false + } + if info, ok := m.byNodeKey[nk]; ok && info.ep != nil { + return info.ep, true + } + return nil, false +} + +// discoEndpointForIPPort returns the discoEndpoint for the peer we +// believe to be at ipp, or nil if we don't know of any such peer. +func (m *peerMap) discoEndpointForIPPort(ipp netaddr.IPPort) (ep *discoEndpoint, ok bool) { + if info, ok := m.byIPPort[ipp]; ok && info.ep != nil { + return info.ep, true + } + return nil, false +} + +// forEachDiscoEndpoint invokes f on every discoEndpoint in m. +func (m *peerMap) forEachDiscoEndpoint(f func(ep *discoEndpoint)) { + for _, pi := range m.byNodeKey { + if pi.ep != nil { + f(pi.ep) + } + } +} + +// forEachNode invokes f on every tailcfg.Node in m. +func (m *peerMap) forEachNode(f func(n *tailcfg.Node)) { + for _, pi := range m.byNodeKey { + f(pi.node) + } +} + +// upsertDiscoEndpoint stores discoEndpoint in the peerInfo for +// ep.publicKey, and updates indexes. m must already have a +// tailcfg.Node for ep.publicKey. +func (m *peerMap) upsertDiscoEndpoint(ep *discoEndpoint) { + pi := m.byNodeKey[ep.publicKey] + if pi == nil { + panic("can't have disco endpoint for unknown node") + } + old := pi.ep + pi.ep = ep + if old != nil && old.discoKey != ep.discoKey { + delete(m.byDiscoKey, old.discoKey) + } + m.byDiscoKey[ep.discoKey] = pi +} + +// upsertNode stores n in the peerInfo for n.Key, creating the +// peerInfo if necessary, and updates indexes. +func (m *peerMap) upsertNode(n *tailcfg.Node) { + if n == nil { + panic("node can't be nil") + } + pi := m.byDiscoKey[n.DiscoKey] + if pi == nil { + pi = newPeerInfo() + } + old := pi.node + pi.node = n + m.byDiscoKey[n.DiscoKey] = pi + if old != nil && old.Key != n.Key { + delete(m.byNodeKey, old.Key) + } + m.byNodeKey[n.Key] = pi +} + +// SetDiscoKeyForIPPort makes future peer lookups by ipp return the +// same peer info as the lookup by dk. +func (m *peerMap) setDiscoKeyForIPPort(ipp netaddr.IPPort, dk tailcfg.DiscoKey) { + // Check for a prior mapping for ipp, may need to clean it up. + if pi := m.byIPPort[ipp]; pi != nil { + delete(pi.ipPorts, ipp) + delete(m.byIPPort, ipp) + } + if pi, ok := m.byDiscoKey[dk]; ok { + pi.ipPorts[ipp] = true + m.byIPPort[ipp] = pi + } +} + +// deleteDiscoEndpoint deletes the peerInfo associated with ep, and +// updates indexes. +func (m *peerMap) deleteDiscoEndpoint(ep *discoEndpoint) { + if ep == nil { + return + } + ep.stopAndReset() + pi := m.byDiscoKey[ep.discoKey] + delete(m.byDiscoKey, ep.discoKey) + delete(m.byNodeKey, ep.publicKey) + for ip := range pi.ipPorts { + delete(m.byIPPort, ip) + } +} + +// deleteNode deletes the peerInfo associated with n, and updates +// indexes. +func (m *peerMap) deleteNode(n *tailcfg.Node) { + if n == nil { + return + } + pi := m.byNodeKey[n.Key] + if pi != nil && pi.ep != nil { + pi.ep.stopAndReset() + } + delete(m.byNodeKey, n.Key) + if !n.DiscoKey.IsZero() { + delete(m.byDiscoKey, n.DiscoKey) + } + for ip := range pi.ipPorts { + delete(m.byIPPort, ip) + } +} + // A Conn routes UDP packets and actively manages a list of its endpoints. // It implements wireguard/conn.Bind. type Conn struct { @@ -85,7 +282,6 @@ type Conn struct { packetListener nettype.PacketListener noteRecvActivity func(tailcfg.DiscoKey) // or nil, see Options.NoteRecvActivity simulatedNetwork bool - disableLegacy bool // ================================================================ // No locking required to access these fields, either because @@ -210,39 +406,10 @@ type Conn struct { discoShort string // ShortString of discoPublic (to save logging work later) // nodeOfDisco tracks the networkmap Node entity for each peer // discovery key. - // - // TODO(danderson): the only thing we ever use from this is the - // peer's WireGuard public key. This could be a map of DiscoKey to - // NodeKey. - nodeOfDisco map[tailcfg.DiscoKey]*tailcfg.Node - discoOfNode map[tailcfg.NodeKey]tailcfg.DiscoKey - discoOfAddr map[netaddr.IPPort]tailcfg.DiscoKey // validated non-DERP paths only - // endpointsOfDisco tracks the wireguard-go endpoints for peers - // with recent activity. - endpointOfDisco map[tailcfg.DiscoKey]*discoEndpoint // those with activity only - sharedDiscoKey map[tailcfg.DiscoKey]*[32]byte // nacl/box precomputed key - - // addrsByUDP is a map of every remote ip:port to a priority - // list of endpoint addresses for a peer. - // The priority list is provided by wgengine configuration. - // - // Given a wgcfg describing: - // machineA: 10.0.0.1:1, 10.0.0.2:2 - // machineB: 10.0.0.3:3 - // the addrsByUDP map contains: - // 10.0.0.1:1 -> [10.0.0.1:1, 10.0.0.2:2] - // 10.0.0.2:2 -> [10.0.0.1:1, 10.0.0.2:2] - // 10.0.0.3:3 -> [10.0.0.3:3] - // - // Used only to communicate with legacy, pre-active-discovery - // clients. - addrsByUDP map[netaddr.IPPort]*addrSet - // addrsByKey maps from public keys (as seen by incoming DERP - // packets) to its addrSet (the same values as in addrsByUDP). - // - // Used only to communicate with legacy, pre-active-discovery - // clients. - addrsByKey map[key.Public]*addrSet + peerMap peerMap + // sharedDiscoKey is the precomputed nacl/box key for + // communication with the peer that has the given DiscoKey. + sharedDiscoKey map[tailcfg.DiscoKey]*[32]byte // netInfoFunc is a callback that provides a tailcfg.NetInfo when // discovered network conditions change. @@ -381,11 +548,6 @@ type Options struct { // "go test"). SimulatedNetwork bool - // DisableLegacyNetworking disables legacy peer handling. When - // enabled, only active discovery-aware nodes will be able to - // communicate with Conn. - DisableLegacyNetworking bool - // LinkMonitor is the link monitor to use. // With one, the portmapper won't be used. LinkMonitor *monitor.Mon @@ -416,16 +578,12 @@ func (o *Options) derpActiveFunc() func() { // of NewConn. Mostly for tests. func newConn() *Conn { c := &Conn{ - disableLegacy: true, - sendLogLimit: rate.NewLimiter(rate.Every(1*time.Minute), 1), - addrsByUDP: make(map[netaddr.IPPort]*addrSet), - addrsByKey: make(map[key.Public]*addrSet), - derpRecvCh: make(chan derpReadResult), - derpStarted: make(chan struct{}), - peerLastDerp: make(map[key.Public]int), - endpointOfDisco: make(map[tailcfg.DiscoKey]*discoEndpoint), - sharedDiscoKey: make(map[tailcfg.DiscoKey]*[32]byte), - discoOfAddr: make(map[netaddr.IPPort]tailcfg.DiscoKey), + sendLogLimit: rate.NewLimiter(rate.Every(1*time.Minute), 1), + derpRecvCh: make(chan derpReadResult), + derpStarted: make(chan struct{}), + peerLastDerp: make(map[key.Public]int), + peerMap: newPeerMap(), + sharedDiscoKey: make(map[tailcfg.DiscoKey]*[32]byte), } c.bind = &connBind{Conn: c, closed: true} c.muCond = sync.NewCond(&c.mu) @@ -448,7 +606,6 @@ func NewConn(opts Options) (*Conn, error) { c.packetListener = opts.PacketListener c.noteRecvActivity = opts.NoteRecvActivity c.simulatedNetwork = opts.SimulatedNetwork - c.disableLegacy = opts.DisableLegacyNetworking c.portMapper = portmapper.NewClient(logger.WithPrefix(c.logf, "portmapper: "), c.onPortMapChanged) if opts.LinkMonitor != nil { c.portMapper.SetGatewayLookupFunc(opts.LinkMonitor.GatewayAndSelfIP) @@ -693,35 +850,21 @@ func (c *Conn) pickDERPFallback() int { return 0 } - // See where our peers are. - var ( - peersOnDerp = map[int]int{} - best int - bestCount int - ) - for _, as := range c.addrsByKey { - if id := as.derpID(); id != 0 { - peersOnDerp[id]++ - if v := peersOnDerp[id]; v > bestCount { - bestCount = v - best = id - } - } - } + // TODO: figure out which DERP region most of our peers are using, + // and use that region as our fallback. + // + // If we already had selected something in the past and it has any + // peers, we want to stay on it. If there are no peers at all, + // stay on whatever DERP we previously picked. If we need to pick + // one and have no peer info, pick a region randomly. + // + // We used to do the above for legacy clients, but never updated + // it for disco. - // If we already had selected something in the past and it has - // any peers, stay on it. If there are no peers, though, also - // stay where we are. - if c.myDerp != 0 && (best == 0 || peersOnDerp[c.myDerp] != 0) { + if c.myDerp != 0 { return c.myDerp } - // Otherwise pick wherever the most peers are. - if best != 0 { - return best - } - - // Otherwise just pick something randomly. h := fnv.New64() h.Write([]byte(fmt.Sprintf("%p/%d", c, processStartUnixNano))) // arbitrary return ids[rand.New(rand.NewSource(int64(h.Sum64()))).Intn(len(ids))] @@ -758,7 +901,7 @@ func (c *Conn) callNetInfoCallbackLocked(ni *tailcfg.NetInfo) { func (c *Conn) addValidDiscoPathForTest(discoKey tailcfg.DiscoKey, addr netaddr.IPPort) { c.mu.Lock() defer c.mu.Unlock() - c.discoOfAddr[addr] = discoKey + c.peerMap.setDiscoKeyForIPPort(addr, discoKey) } func (c *Conn) SetNetInfoCallback(fn func(*tailcfg.NetInfo)) { @@ -780,7 +923,7 @@ func (c *Conn) SetNetInfoCallback(fn func(*tailcfg.NetInfo)) { func (c *Conn) LastRecvActivityOfDisco(dk tailcfg.DiscoKey) string { c.mu.Lock() defer c.mu.Unlock() - de, ok := c.endpointOfDisco[dk] + de, ok := c.peerMap.discoEndpointForDiscoKey(dk) if !ok { return "never" } @@ -812,17 +955,18 @@ func (c *Conn) Ping(peer *tailcfg.Node, res *ipnstate.PingResult, cb func(*ipnst } } - dk, ok := c.discoOfNode[peer.Key] - if !ok { // peer is using outdated Tailscale version (pre-0.100) - res.Err = "no discovery key for peer (pre Tailscale 0.100 version?). Try: ping 100.x.y.z" - cb(res) - return - } - de, ok := c.endpointOfDisco[dk] + de, ok := c.peerMap.discoEndpointForNodeKey(peer.Key) if !ok { + node, ok := c.peerMap.nodeForNodeKey(peer.Key) + if !ok { + res.Err = "unknown peer" + cb(res) + return + } + c.mu.Unlock() // temporarily release if c.noteRecvActivity != nil { - c.noteRecvActivity(dk) + c.noteRecvActivity(node.DiscoKey) } c.mu.Lock() // re-acquire @@ -833,13 +977,13 @@ func (c *Conn) Ping(peer *tailcfg.Node, res *ipnstate.PingResult, cb func(*ipnst return } - de, ok = c.endpointOfDisco[dk] + de, ok = c.peerMap.discoEndpointForNodeKey(peer.Key) if !ok { - res.Err = "internal error: failed to create endpoint for discokey" + res.Err = "internal error: failed to get endpoint for node key" cb(res) return } - c.logf("[v1] magicsock: started peer %v for ping to %v", dk.ShortString(), peer.Key.ShortString()) + c.logf("[v1] magicsock: started peer %v for ping to %v", de.discoKey.ShortString(), peer.Key.ShortString()) } de.cliPing(res, cb) } @@ -884,8 +1028,10 @@ func (c *Conn) DiscoPublicKey() tailcfg.DiscoKey { func (c *Conn) PeerHasDiscoKey(k tailcfg.NodeKey) bool { c.mu.Lock() defer c.mu.Unlock() - _, ok := c.discoOfNode[k] - return ok + if info, ok := c.peerMap.nodeForNodeKey(k); ok { + return info.DiscoKey.IsZero() + } + return false } // c.mu must NOT be held. @@ -1086,15 +1232,7 @@ func (c *Conn) Send(b []byte, ep conn.Endpoint) error { if c.networkDown() { return errNetworkDown } - - switch v := ep.(type) { - default: - panic(fmt.Sprintf("[unexpected] Endpoint type %T", v)) - case *discoEndpoint: - return v.send(b) - case *addrSet: - return c.sendAddrSet(b, v) - } + return ep.(*discoEndpoint).send(b) } var errConnClosed = errors.New("Conn closed") @@ -1521,26 +1659,12 @@ func (c *Conn) runDerpWriter(ctx context.Context, dc *derphttp.Client, ch <-chan // findEndpoint maps from a UDP address to a WireGuard endpoint, for // ReceiveIPv4/ReceiveIPv6. -// -// TODO(bradfitz): add a fast path that returns nil here for normal -// wireguard-go transport packets; wireguard-go only uses this -// Endpoint for the relatively rare non-data packets; but we need the -// Endpoint to find the UDPAddr to return to wireguard anyway, so no -// benefit unless we can, say, always return the same fake UDPAddr for -// all packets. -func (c *Conn) findEndpoint(ipp netaddr.IPPort, packet []byte) conn.Endpoint { +func (c *Conn) findEndpoint(ipp netaddr.IPPort, packet []byte) (_ *discoEndpoint, ok bool) { c.mu.Lock() defer c.mu.Unlock() - // See if they have a discoEndpoint, for a set of peers - // both supporting active discovery. - if dk, ok := c.discoOfAddr[ipp]; ok { - if ep, ok := c.endpointOfDisco[dk]; ok { - return ep - } - } - - return c.findLegacyEndpointLocked(ipp, packet) + // This can return nil, meaning we don't know any endpoint for ipp. + return c.peerMap.discoEndpointForIPPort(ipp) } // noteRecvActivityFromEndpoint calls the c.noteRecvActivity hook if @@ -1606,15 +1730,14 @@ func (c *Conn) receiveIP(b []byte, ipp netaddr.IPPort, cache *ippEndpointCache) if cache.ipp == ipp && cache.de != nil && cache.gen == cache.de.numStopAndReset() { ep = cache.de } else { - ep = c.findEndpoint(ipp, b) - if ep == nil { + de, ok := c.findEndpoint(ipp, b) + if !ok { return nil, false } - if de, ok := ep.(*discoEndpoint); ok { - cache.ipp = ipp - cache.de = de - cache.gen = de.numStopAndReset() - } + cache.ipp = ipp + cache.de = de + cache.gen = de.numStopAndReset() + ep = de } c.noteRecvActivityFromEndpoint(ep) return ep, true @@ -1642,7 +1765,7 @@ func (c *connBind) receiveDERP(b []byte) (n int, ep conn.Endpoint, err error) { return 0, nil, net.ErrClosed } -func (c *Conn) processDERPReadResult(dm derpReadResult, b []byte) (n int, ep conn.Endpoint) { +func (c *Conn) processDERPReadResult(dm derpReadResult, b []byte) (n int, ep *discoEndpoint) { if dm.copyBuf == nil { return 0, nil } @@ -1660,48 +1783,63 @@ func (c *Conn) processDERPReadResult(dm derpReadResult, b []byte) (n int, ep con return 0, nil } - var ( - didNoteRecvActivity bool - discoEp *discoEndpoint - asEp *addrSet - ) + didNoteRecvActivity := false c.mu.Lock() - if dk, ok := c.discoOfNode[tailcfg.NodeKey(dm.src)]; ok { - discoEp = c.endpointOfDisco[dk] - // If we know about the node (it's in discoOfNode) but don't know about the - // endpoint, that's because it's an idle peer that doesn't yet exist in the - // wireguard config. So run the receive hook, if defined, which should - // create the wireguard peer. - if discoEp == nil && c.noteRecvActivity != nil { - didNoteRecvActivity = true - c.mu.Unlock() // release lock before calling noteRecvActivity - c.noteRecvActivity(dk) // (calls back into ParseEndpoint) - // Now require the lock. No invariants need to be rechecked; just - // 1-2 map lookups follow that are harmless if, say, the peer has - // been deleted during this time. - c.mu.Lock() - - discoEp = c.endpointOfDisco[dk] - c.logf("magicsock: DERP packet received from idle peer %v; created=%v", dm.src.ShortString(), discoEp != nil) + nk := tailcfg.NodeKey(dm.src) + var ok bool + ep, ok = c.peerMap.discoEndpointForNodeKey(nk) + if !ok { + node, ok := c.peerMap.nodeForNodeKey(nk) + if !ok { + // We don't know anything about this node key, nothing to + // record or process. + c.mu.Unlock() + return 0, nil } - } - if !c.disableLegacy { - asEp = c.addrsByKey[dm.src] - } - c.mu.Unlock() - if discoEp != nil { - ep = discoEp - } else if asEp != nil { - ep = asEp - } else { - key := wgkey.Key(dm.src) - c.logf("magicsock: DERP packet from unknown key: %s", key.ShortString()) - ep = c.findEndpoint(ipp, b[:n]) - if ep == nil { + // We know about the node, but have no disco endpoint for + // it. That's because it's an idle peer that doesn't yet exist + // in the wireguard config. If we have a receive hook, run it + // to get the endpoint created. + if c.noteRecvActivity == nil { + // No hook to lazily create endpoints, nothing we can do. + c.mu.Unlock() + return 0, nil + } + + didNoteRecvActivity = true + // release lock before calling the activity callback, because + // it ends up calling back into magicsock to create the + // endpoint. + c.mu.Unlock() + c.noteRecvActivity(node.DiscoKey) + // Reacquire the lock. Because we were unlocked for a while, + // it's possible that even after all this, we still won't be + // able to find a disco endpoint for the node (e.g. because + // the peer was deleted from the netmap in the interim). Don't + // assume that ep != nil. + c.mu.Lock() + c.logf("magicsock: DERP packet received from idle peer %v; created=%v", dm.src.ShortString(), ep != nil) + ep, ok = c.peerMap.discoEndpointForNodeKey(nk) + if !ok { + // There are a few edge cases where we can still end up + // with a nil ep here. Among them are: the peer was + // deleted while we were unlocked above (harmless, we no + // longer want to talk to that peer anyway), or there is a + // race between magicsock becoming aware of a new peer and + // WireGuard becoming aware, *and* lazy wg reconfiguration + // is disabled (at least test code does this, as of + // 2021-08). + // + // Either way, the bottom line is: we thought we might + // know about this peer, but it turns out we don't know + // enough to hand it to WireGuard, so, we want to drop the + // packet. If this was in error due to a race, the peer + // will eventually retry and heal things. return 0, nil } } + c.mu.Unlock() if !didNoteRecvActivity { c.noteRecvActivityFromEndpoint(ep) @@ -1802,7 +1940,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo return } - peerNode, ok := c.nodeOfDisco[sender] + peerNode, ok := c.peerMap.nodeForDiscoKey(sender) if !ok { if debugDisco { c.logf("magicsock: disco: ignoring disco-looking frame, don't know node for %v", sender.ShortString()) @@ -1811,21 +1949,21 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo } needsRecvActivityCall := false - de, endpointFound0 := c.endpointOfDisco[sender] - if !endpointFound0 { - // We don't have an active endpoint for this sender but we knew about the node, so - // it's an idle endpoint that doesn't yet exist in the wireguard config. We now have - // to notify the userspace engine (via noteRecvActivity) so wireguard-go can create - // an Endpoint (ultimately calling our ParseEndpoint). + isLazyCreate := false + de, ok := c.peerMap.discoEndpointForDiscoKey(sender) + if !ok { + // We know about the node, but it doesn't currently have active WireGuard state. c.logf("magicsock: got disco message from idle peer, starting lazy conf for %v, %v", peerNode.Key.ShortString(), sender.ShortString()) if c.noteRecvActivity == nil { c.logf("magicsock: [unexpected] have node without endpoint, without c.noteRecvActivity hook") return } needsRecvActivityCall = true - } else { - needsRecvActivityCall = de.isFirstRecvActivityInAwhile() + isLazyCreate = true + } else if de.isFirstRecvActivityInAwhile() { + needsRecvActivityCall = true } + if needsRecvActivityCall && c.noteRecvActivity != nil { // We can't hold Conn.mu while calling noteRecvActivity. // noteRecvActivity acquires userspaceEngine.wgLock (and per our @@ -1841,22 +1979,30 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo if c.closed || c.privateKey.IsZero() { return } - de, ok = c.endpointOfDisco[sender] + de, ok = c.peerMap.discoEndpointForDiscoKey(sender) if !ok { - if _, ok := c.nodeOfDisco[sender]; !ok { + if _, ok := c.peerMap.nodeForDiscoKey(sender); !ok { // They just disappeared while we'd released the lock. return false } c.logf("magicsock: [unexpected] lazy endpoint not created for %v, %v", peerNode.Key.ShortString(), sender.ShortString()) return } - if !endpointFound0 { + if isLazyCreate { c.logf("magicsock: lazy endpoint created via disco message for %v, %v", peerNode.Key.ShortString(), sender.ShortString()) } } - // First, do we even know (and thus care) about this sender? If not, - // don't bother decrypting it. + if !de.canP2P() { + // This endpoint allegedly sent us a disco packet, but we know + // they can't speak disco. Drop. + return + } + + // We're now reasonably sure we're expecting communication from + // this peer, do the heavy crypto lifting to see what they want. + // + // From here on, peerNode and de are non-nil. var nonce [disco.NonceLen]byte copy(nonce[:], msg[len(disco.Magic)+len(key.Public{}):]) @@ -1869,7 +2015,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo // scheduled). This is particularly the case for LANs // or non-NATed endpoints. // Don't log in normal case. Pass on to wireguard, in case - // it's actually a a wireguard packet (super unlikely, + // it's actually a wireguard packet (super unlikely, // but). if debugDisco { c.logf("magicsock: disco: failed to open naclbox from %v (wrong rcpt?)", sender) @@ -1896,9 +2042,6 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo case *disco.Ping: c.handlePingLocked(dm, de, src, sender, peerNode) case *disco.Pong: - if de == nil { - return - } de.handlePongConnLocked(dm, src) case *disco.CallMeMaybe: if src.IP() != derpMagicIPAddr { @@ -1906,13 +2049,11 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo c.logf("[unexpected] CallMeMaybe packets should only come via DERP") return } - if de != nil { - c.logf("[v1] magicsock: disco: %v<-%v (%v, %v) got call-me-maybe, %d endpoints", - c.discoShort, de.discoShort, - de.publicKey.ShortString(), derpStr(src.String()), - len(dm.MyNumber)) - go de.handleCallMeMaybe(dm) - } + c.logf("[v1] magicsock: disco: %v<-%v (%v, %v) got call-me-maybe, %d endpoints", + c.discoShort, de.discoShort, + de.publicKey.ShortString(), derpStr(src.String()), + len(dm.MyNumber)) + go de.handleCallMeMaybe(dm) } return } @@ -1930,7 +2071,7 @@ func (c *Conn) handlePingLocked(dm *disco.Ping, de *discoEndpoint, src netaddr.I } // Remember this route if not present. - c.setAddrToDiscoLocked(src, sender, nil) + c.setAddrToDiscoLocked(src, sender) de.addCandidateEndpoint(src) ipDst := src @@ -1983,69 +2124,17 @@ func (c *Conn) enqueueCallMeMaybe(derpAddr netaddr.IPPort, de *discoEndpoint) { // setAddrToDiscoLocked records that newk is at src. // // c.mu must be held. -// -// If the caller already has a discoEndpoint mutex held as well, it -// can be passed in as alreadyLocked so it won't be re-acquired during -// any lazy cleanup of the mapping. -func (c *Conn) setAddrToDiscoLocked(src netaddr.IPPort, newk tailcfg.DiscoKey, alreadyLocked *discoEndpoint) { - oldk, ok := c.discoOfAddr[src] - if ok && oldk == newk { - return - } - if ok { - c.logf("[v1] magicsock: disco: changing mapping of %v from %x=>%x", src, oldk.ShortString(), newk.ShortString()) - } else { - c.logf("[v1] magicsock: disco: adding mapping of %v to %v", src, newk.ShortString()) - } - c.discoOfAddr[src] = newk +func (c *Conn) setAddrToDiscoLocked(src netaddr.IPPort, newk tailcfg.DiscoKey) { + oldEp, ok := c.peerMap.discoEndpointForIPPort(src) if !ok { - c.cleanDiscoOfAddrLocked(alreadyLocked) - } -} - -// cleanDiscoOfAddrLocked lazily checks a few entries in c.discoOfAddr -// and deletes them if they're stale. It has no pointers in it so we -// don't go through the effort of keeping it aggressively -// pruned. Instead, we lazily clean it whenever it grows. -// -// c.mu must be held. -// -// If the caller already has a discoEndpoint mutex held as well, it -// can be passed in as alreadyLocked so it won't be re-acquired. -func (c *Conn) cleanDiscoOfAddrLocked(alreadyLocked *discoEndpoint) { - // If it's small enough, don't worry about it. - if len(c.discoOfAddr) < 16 { + c.logf("[v1] magicsock: disco: adding mapping of %v to %v", src, newk.ShortString()) + } else if oldEp.discoKey != newk { + c.logf("[v1] magicsock: disco: changing mapping of %v from %x=>%x", src, oldEp.discoKey.ShortString(), newk.ShortString()) + } else { + // No change return } - - const checkEntries = 5 // per one unit of growth - - // Take advantage of Go's random map iteration to check & clean - // a few entries. - n := 0 - for ipp, dk := range c.discoOfAddr { - n++ - if n > checkEntries { - return - } - de, ok := c.endpointOfDisco[dk] - if !ok { - // This discokey isn't even known anymore. Clean. - delete(c.discoOfAddr, ipp) - continue - } - if de != alreadyLocked { - de.mu.Lock() - } - if _, ok := de.endpointState[ipp]; !ok { - // The discoEndpoint no longer knows about that endpoint. - // It must've changed. Clean. - delete(c.discoOfAddr, ipp) - } - if de != alreadyLocked { - de.mu.Unlock() - } - } + c.peerMap.setDiscoKeyForIPPort(src, newk) } func (c *Conn) sharedDiscoKeyLocked(k tailcfg.DiscoKey) *[32]byte { @@ -2131,9 +2220,9 @@ func (c *Conn) SetPrivateKey(privateKey wgkey.Private) error { } if newKey.IsZero() { - for _, de := range c.endpointOfDisco { - de.stopAndReset() - } + c.peerMap.forEachDiscoEndpoint(func(ep *discoEndpoint) { + ep.stopAndReset() + }) } return nil @@ -2154,7 +2243,6 @@ func (c *Conn) UpdatePeers(newPeers map[key.Public]struct{}) { // exist. for peer := range oldPeers { if _, ok := newPeers[peer]; !ok { - delete(c.addrsByKey, peer) delete(c.derpRoute, peer) delete(c.peerLastDerp, peer) } @@ -2207,56 +2295,65 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { c.mu.Lock() defer c.mu.Unlock() + if c.closed { + return + } + if c.netMap != nil && nodesEqual(c.netMap.Peers, nm.Peers) { return } - numDisco := 0 + // For disco-capable peers, update the disco endpoint's state and + // check if the disco key migrated to a new node key. + numNoDisco := 0 for _, n := range nm.Peers { if n.DiscoKey.IsZero() { + numNoDisco++ continue } - numDisco++ - if ep, ok := c.endpointOfDisco[n.DiscoKey]; ok && ep.publicKey == n.Key { + if ep, ok := c.peerMap.discoEndpointForDiscoKey(n.DiscoKey); ok && ep.publicKey == n.Key { ep.updateFromNode(n) - } else if ok { + } else if ep != nil { + // Endpoint no longer belongs to the same node. We'll + // create the new endpoint below. c.logf("magicsock: disco key %v changed from node key %v to %v", n.DiscoKey, ep.publicKey.ShortString(), n.Key.ShortString()) ep.stopAndReset() - delete(c.endpointOfDisco, n.DiscoKey) + c.peerMap.deleteDiscoEndpoint(ep) } } - c.logf("[v1] magicsock: got updated network map; %d peers (%d with discokey)", len(nm.Peers), numDisco) + c.logf("[v1] magicsock: got updated network map; %d peers", len(nm.Peers)) + if numNoDisco != 0 { + c.logf("[v1] magicsock: %d DERP-only peers (no discokey)", numNoDisco) + } c.netMap = nm - // Build and/or update node<->disco maps, only reallocating if - // the set of discokeys changed. - for pass := 1; pass <= 2; pass++ { - if c.nodeOfDisco == nil || pass == 2 { - c.nodeOfDisco = map[tailcfg.DiscoKey]*tailcfg.Node{} - c.discoOfNode = map[tailcfg.NodeKey]tailcfg.DiscoKey{} - } - for _, n := range nm.Peers { - if !n.DiscoKey.IsZero() { - c.nodeOfDisco[n.DiscoKey] = n - if old, ok := c.discoOfNode[n.Key]; ok && old != n.DiscoKey { - c.logf("magicsock: node %s changed discovery key from %x to %x", n.Key.ShortString(), old[:8], n.DiscoKey[:8]) - } - c.discoOfNode[n.Key] = n.DiscoKey - } - } - if len(c.nodeOfDisco) == numDisco && len(c.discoOfNode) == numDisco { - break - } + // Try a pass of just upserting nodes. If the set of nodes is the + // same, this is an efficient alloc-free update. If the set of + // nodes is different, we'll fall through to the next pass, which + // allocates but can handle full set updates. + for _, n := range nm.Peers { + c.peerMap.upsertNode(n) } - // Clean c.endpointOfDisco for discovery keys that are no longer present. - for dk, de := range c.endpointOfDisco { - if _, ok := c.nodeOfDisco[dk]; !ok { - de.stopAndReset() - delete(c.endpointOfDisco, dk) - delete(c.sharedDiscoKey, dk) + // If the set of nodes changed since the last SetNetworkMap, the + // upsert loop just above made c.peerMap contain the union of the + // old and new peers - which will be larger than the set from the + // current netmap. If that happens, go through the allocful + // deletion path to clean up moribund nodes. + if c.peerMap.nodeCount() != len(nm.Peers) { + keep := make(map[tailcfg.NodeKey]bool, len(nm.Peers)) + for _, n := range nm.Peers { + keep[n.Key] = true } + c.peerMap.forEachNode(func(n *tailcfg.Node) { + if !keep[n.Key] { + c.peerMap.deleteNode(n) + if !n.DiscoKey.IsZero() { + delete(c.sharedDiscoKey, n.DiscoKey) + } + } + }) } } @@ -2457,9 +2554,9 @@ func (c *Conn) Close() error { c.stopPeriodicReSTUNTimerLocked() c.portMapper.Close() - for _, ep := range c.endpointOfDisco { + c.peerMap.forEachDiscoEndpoint(func(ep *discoEndpoint) { ep.stopAndReset() - } + }) c.closed = true c.connCtxCancel() @@ -2726,17 +2823,15 @@ func (c *Conn) Rebind() { c.resetEndpointStates() } -// resetEndpointStates resets the preferred address for all peers and -// re-enables spraying. +// resetEndpointStates resets the preferred address for all peers. // This is called when connectivity changes enough that we no longer // trust the old routes. func (c *Conn) resetEndpointStates() { c.mu.Lock() defer c.mu.Unlock() - for _, de := range c.endpointOfDisco { - de.noteConnectivityChange() - } - c.resetAddrSetStatesLocked() + c.peerMap.forEachDiscoEndpoint(func(ep *discoEndpoint) { + ep.noteConnectivityChange() + }) } // packIPPort packs an IPPort into the form wanted by WireGuard. @@ -2769,31 +2864,32 @@ func (c *Conn) ParseEndpoint(endpointStr string) (conn.Endpoint, error) { c.mu.Lock() defer c.mu.Unlock() - if discoKey.IsZero() { - c.logf("magicsock: ParseEndpoint: key=%s: disco=%s legacy=%s", pk.ShortString(), discoKey.ShortString(), derpStr(endpoints.IPPorts.String())) - return c.createLegacyEndpointLocked(pk, endpoints.IPPorts, endpointStr) + if c.closed { + return nil, errors.New("magicsock: Conn closed") + } + node, ok := c.peerMap.nodeForNodeKey(tailcfg.NodeKey(pk)) + if !ok { + // We should never be telling WireGuard about a new peer + // before magicsock knows about it. + c.logf("[unexpected] magicsock: ParseEndpoint: unknown node key=%s", pk.ShortString()) + return nil, fmt.Errorf("magicsock: ParseEndpoint: unknown peer %q", pk.ShortString()) } - de := &discoEndpoint{ c: c, - publicKey: tailcfg.NodeKey(pk), // peer public key (for WireGuard + DERP) - discoKey: tailcfg.DiscoKey(discoKey), // for discovery messages - discoShort: tailcfg.DiscoKey(discoKey).ShortString(), + publicKey: tailcfg.NodeKey(pk), // peer public key (for WireGuard + DERP) wgEndpoint: endpointStr, sentPing: map[stun.TxID]sentPing{}, endpointState: map[netaddr.IPPort]*endpointState{}, } + if !discoKey.IsZero() { + de.discoKey = tailcfg.DiscoKey(discoKey) + de.discoShort = de.discoKey.ShortString() + } de.initFakeUDPAddr() - n := c.nodeOfDisco[de.discoKey] - de.updateFromNode(n) c.logf("magicsock: ParseEndpoint: key=%s: disco=%s; %v", pk.ShortString(), discoKey.ShortString(), logger.ArgWriter(func(w *bufio.Writer) { - if n == nil { - w.WriteString("nil node") - return - } const derpPrefix = "127.3.3.40:" - if strings.HasPrefix(n.DERP, derpPrefix) { - ipp, _ := netaddr.ParseIPPort(n.DERP) + if strings.HasPrefix(node.DERP, derpPrefix) { + ipp, _ := netaddr.ParseIPPort(node.DERP) regionID := int(ipp.Port()) code := c.derpRegionCodeLocked(regionID) if code != "" { @@ -2802,18 +2898,20 @@ func (c *Conn) ParseEndpoint(endpointStr string) (conn.Endpoint, error) { fmt.Fprintf(w, "derp=%v%s ", regionID, code) } - for _, a := range n.AllowedIPs { + for _, a := range node.AllowedIPs { if a.IsSingleIP() { fmt.Fprintf(w, "aip=%v ", a.IP()) } else { fmt.Fprintf(w, "aip=%v ", a) } } - for _, ep := range n.Endpoints { + for _, ep := range node.Endpoints { fmt.Fprintf(w, "ep=%v ", ep) } })) - c.endpointOfDisco[de.discoKey] = de + de.updateFromNode(node) + c.peerMap.upsertDiscoEndpoint(de) + return de, nil } @@ -3086,24 +3184,15 @@ func (c *Conn) UpdateStatus(sb *ipnstate.StatusBuilder) { ss.TailAddrDeprecated = tailAddr4 }) - for dk, n := range c.nodeOfDisco { + c.peerMap.forEachNode(func(n *tailcfg.Node) { ps := &ipnstate.PeerStatus{InMagicSock: true} ps.Addrs = append(ps.Addrs, n.Endpoints...) ps.Relay = c.derpRegionCodeOfAddrLocked(n.DERP) - if de, ok := c.endpointOfDisco[dk]; ok { - de.populatePeerStatus(ps) + if ep, ok := c.peerMap.discoEndpointForNodeKey(n.Key); ok { + ep.populatePeerStatus(ps) } sb.AddPeer(key.Public(n.Key), ps) - } - // Old-style (pre-disco) peers: - for k, as := range c.addrsByKey { - ps := &ipnstate.PeerStatus{ - InMagicSock: true, - Relay: c.derpRegionCodeOfIDLocked(as.derpID()), - } - as.populatePeerStatus(ps) - sb.AddPeer(k, ps) - } + }) c.foreachActiveDerpSortedLocked(func(node int, ad activeDerp) { // TODO(bradfitz): add to ipnstate.StatusBuilder @@ -3118,8 +3207,8 @@ func ippDebugString(ua netaddr.IPPort) string { return ua.String() } -// discoEndpoint is a wireguard/conn.Endpoint for new-style peers that -// advertise a DiscoKey and participate in active discovery. +// discoEndpoint is a wireguard/conn.Endpoint that picks the best +// available path to communicate with a peer. type discoEndpoint struct { // atomically accessed; declared first for alignment reasons lastRecv mono.Time @@ -3128,8 +3217,8 @@ type discoEndpoint struct { // These fields are initialized once and never modified. c *Conn publicKey tailcfg.NodeKey // peer public key (for WireGuard + DERP) - discoKey tailcfg.DiscoKey // for discovery messages - discoShort string // ShortString of discoKey + discoKey tailcfg.DiscoKey // for discovery messages. IsZero() if peer can't disco. + discoShort string // ShortString of discoKey. Empty if peer can't disco. fakeWGAddr netaddr.IPPort // the UDP address we tell wireguard-go we're using wgEndpoint string // string from ParseEndpoint, holds a JSON-serialized wgcfg.Endpoints @@ -3312,6 +3401,15 @@ func (de *discoEndpoint) DstToString() string { return de.wgEndpoint } func (de *discoEndpoint) DstIP() net.IP { panic("unused") } func (de *discoEndpoint) DstToBytes() []byte { return packIPPort(de.fakeWGAddr) } +// canP2P reports whether this endpoint understands the disco protocol +// and is expected to speak it. +// +// As of 2021-08-25, only a few hundred pre-0.100 clients understand +// DERP but not disco, so this returns false very rarely. +func (de *discoEndpoint) canP2P() bool { + return !de.discoKey.IsZero() +} + // addrForSendLocked returns the address(es) that should be used for // sending the next packet. Zero, one, or both of UDP address and DERP // addr may be non-zero. @@ -3335,6 +3433,11 @@ func (de *discoEndpoint) heartbeat() { de.heartBeatTimer = nil + if !de.canP2P() { + // Cannot form p2p connections, no heartbeating necessary. + return + } + if de.lastSend.IsZero() { // Shouldn't happen. return @@ -3365,6 +3468,9 @@ func (de *discoEndpoint) heartbeat() { // // de.mu must be held. func (de *discoEndpoint) wantFullPingLocked(now mono.Time) bool { + if !de.canP2P() { + return false + } if de.bestAddr.IsZero() || de.lastFullPing.IsZero() { return true } @@ -3382,7 +3488,7 @@ func (de *discoEndpoint) wantFullPingLocked(now mono.Time) bool { func (de *discoEndpoint) noteActiveLocked() { de.lastSend = mono.Now() - if de.heartBeatTimer == nil { + if de.heartBeatTimer == nil && de.canP2P() { de.heartBeatTimer = time.AfterFunc(heartbeatInterval, de.heartbeat) } } @@ -3406,7 +3512,7 @@ func (de *discoEndpoint) cliPing(res *ipnstate.PingResult, cb func(*ipnstate.Pin // can look like they're bouncing between, say 10.0.0.0/9 and the peer's // IPv6 address, both 1ms away, and it's random who replies first. de.startPingLocked(udpAddr, now, pingCLI) - } else { + } else if de.canP2P() { for ep := range de.endpointState { de.startPingLocked(ep, now, pingCLI) } @@ -3419,7 +3525,7 @@ func (de *discoEndpoint) send(b []byte) error { de.mu.Lock() udpAddr, derpAddr := de.addrForSendLocked(now) - if udpAddr.IsZero() || now.After(de.trustBestAddrUntil) { + if de.canP2P() && (udpAddr.IsZero() || now.After(de.trustBestAddrUntil)) { de.sendPingsLocked(now, true) } de.noteActiveLocked() @@ -3501,6 +3607,9 @@ const ( ) func (de *discoEndpoint) startPingLocked(ep netaddr.IPPort, now mono.Time, purpose discoPingPurpose) { + if !de.canP2P() { + panic("tried to disco ping a peer that can't disco") + } if purpose != pingCLI { st, ok := de.endpointState[ep] if !ok { @@ -3564,8 +3673,7 @@ func (de *discoEndpoint) sendDiscoMessage(dst netaddr.IPPort, dm disco.Message, func (de *discoEndpoint) updateFromNode(n *tailcfg.Node) { if n == nil { - // TODO: log, error, count? if this even happens. - return + panic("nil node when updating disco ep") } de.mu.Lock() defer de.mu.Unlock() @@ -3676,7 +3784,7 @@ func (de *discoEndpoint) handlePongConnLocked(m *disco.Pong, src netaddr.IPPort) return } - de.c.setAddrToDiscoLocked(src, de.discoKey, de) + de.c.setAddrToDiscoLocked(src, de.discoKey) st.addPongReplyLocked(pongReply{ latency: latency, @@ -3767,6 +3875,10 @@ func (st *endpointState) addPongReplyLocked(r pongReply) { // already sent to us via UDP, so their stateful firewall should be // open. Now we can Ping back and make it through. func (de *discoEndpoint) handleCallMeMaybe(m *disco.CallMeMaybe) { + if !de.canP2P() { + // How did we receive a disco message from a peer that can't disco? + panic("got call-me-maybe from peer with no discokey") + } de.mu.Lock() defer de.mu.Unlock() @@ -3841,8 +3953,9 @@ func (de *discoEndpoint) populatePeerStatus(ps *ipnstate.PeerStatus) { } // stopAndReset stops timers associated with de and resets its state back to zero. -// It's called when a discovery endpoint is no longer present in the NetworkMap, -// or when magicsock is transition from running to stopped state (via SetPrivateKey(zero)) +// It's called when a discovery endpoint is no longer present in the +// NetworkMap, or when magicsock is transitioning from running to +// stopped state (via SetPrivateKey(zero)) func (de *discoEndpoint) stopAndReset() { atomic.AddInt64(&de.numStopAndResetAtomic, 1) de.mu.Lock() diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index f6eb7458e..7a9153c7d 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -9,7 +9,6 @@ import ( "context" crand "crypto/rand" "crypto/tls" - "encoding/binary" "encoding/json" "errors" "fmt" @@ -38,7 +37,6 @@ import ( "tailscale.com/tailcfg" "tailscale.com/tstest" "tailscale.com/tstest/natlab" - "tailscale.com/tstime/mono" "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/netmap" @@ -137,7 +135,7 @@ type magicStack struct { // newMagicStack builds and initializes an idle magicsock and // friends. You need to call conn.SetNetworkMap and dev.Reconfig // before anything interesting happens. -func newMagicStack(t testing.TB, logf logger.Logf, l nettype.PacketListener, derpMap *tailcfg.DERPMap, disableLegacy bool) *magicStack { +func newMagicStack(t testing.TB, logf logger.Logf, l nettype.PacketListener, derpMap *tailcfg.DERPMap) *magicStack { t.Helper() privateKey, err := wgkey.NewPrivate() @@ -152,8 +150,7 @@ func newMagicStack(t testing.TB, logf logger.Logf, l nettype.PacketListener, der EndpointsFunc: func(eps []tailcfg.Endpoint) { epCh <- eps }, - SimulatedNetwork: l != nettype.Std{}, - DisableLegacyNetworking: disableLegacy, + SimulatedNetwork: l != nettype.Std{}, }) if err != nil { t.Fatalf("constructing magicsock: %v", err) @@ -236,9 +233,7 @@ func (s *magicStack) IP() netaddr.IP { // and WireGuard configs into everyone to form a full mesh that has up // to date endpoint info. Think of it as an extremely stripped down // and purpose-built Tailscale control plane. -// -// meshStacks only supports disco connections, not legacy logic. -func meshStacks(logf logger.Logf, ms []*magicStack) (cleanup func()) { +func meshStacks(logf logger.Logf, mutateNetmap func(idx int, nm *netmap.NetworkMap), ms ...*magicStack) (cleanup func()) { ctx, cancel := context.WithCancel(context.Background()) // Serialize all reconfigurations globally, just to keep things @@ -273,6 +268,9 @@ func meshStacks(logf logger.Logf, ms []*magicStack) (cleanup func()) { nm.Peers = append(nm.Peers, peer) } + if mutateNetmap != nil { + mutateNetmap(myIdx, nm) + } return nm } @@ -342,10 +340,9 @@ func TestNewConn(t *testing.T) { port := pickPort(t) conn, err := NewConn(Options{ - Port: port, - EndpointsFunc: epFunc, - Logf: t.Logf, - DisableLegacyNetworking: true, + Port: port, + EndpointsFunc: epFunc, + Logf: t.Logf, }) if err != nil { t.Fatal(err) @@ -443,60 +440,9 @@ func TestPickDERPFallback(t *testing.T) { t.Errorf("not sticky: got %v; want %v", got, someNode) } - // But move if peers are elsewhere. - const otherNode = 789 - c.addrsByKey = map[key.Public]*addrSet{ - {1}: {ipPorts: []netaddr.IPPort{netaddr.IPPortFrom(derpMagicIPAddr, otherNode)}}, - } - if got := c.pickDERPFallback(); got != otherNode { - t.Errorf("didn't join peers: got %v; want %v", got, someNode) - } -} - -func makeConfigs(t *testing.T, addrs []netaddr.IPPort) []wgcfg.Config { - t.Helper() - - var privKeys []wgkey.Private - var addresses [][]netaddr.IPPrefix - - for i := range addrs { - privKey, err := wgkey.NewPrivate() - if err != nil { - t.Fatal(err) - } - privKeys = append(privKeys, wgkey.Private(privKey)) - - addresses = append(addresses, []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix(fmt.Sprintf("1.0.0.%d/32", i+1)), - }) - } - - var cfgs []wgcfg.Config - for i := range addrs { - cfg := wgcfg.Config{ - Name: fmt.Sprintf("peer%d", i+1), - PrivateKey: privKeys[i], - Addresses: addresses[i], - } - for peerNum, addr := range addrs { - if peerNum == i { - continue - } - publicKey := privKeys[peerNum].Public() - peer := wgcfg.Peer{ - PublicKey: publicKey, - AllowedIPs: addresses[peerNum], - Endpoints: wgcfg.Endpoints{ - PublicKey: publicKey, - IPPorts: wgcfg.NewIPPortSet(addr), - }, - PersistentKeepalive: 25, - } - cfg.Peers = append(cfg.Peers, peer) - } - cfgs = append(cfgs, cfg) - } - return cfgs + // TODO: test that disco-based clients changing to a new DERP + // region causes this fallback to also move, once disco clients + // have fixed DERP fallback logic. } // TestDeviceStartStop exercises the startup and shutdown logic of @@ -510,9 +456,8 @@ func TestDeviceStartStop(t *testing.T) { tstest.ResourceCheck(t) conn, err := NewConn(Options{ - EndpointsFunc: func(eps []tailcfg.Endpoint) {}, - Logf: t.Logf, - DisableLegacyNetworking: true, + EndpointsFunc: func(eps []tailcfg.Endpoint) {}, + Logf: t.Logf, }) if err != nil { t.Fatal(err) @@ -552,12 +497,12 @@ func TestConnClosed(t *testing.T) { derpMap, cleanup := runDERPAndStun(t, logf, d.stun, d.stunIP) defer cleanup() - ms1 := newMagicStack(t, logger.WithPrefix(logf, "conn1: "), d.m1, derpMap, true) + ms1 := newMagicStack(t, logger.WithPrefix(logf, "conn1: "), d.m1, derpMap) defer ms1.Close() - ms2 := newMagicStack(t, logger.WithPrefix(logf, "conn2: "), d.m2, derpMap, true) + ms2 := newMagicStack(t, logger.WithPrefix(logf, "conn2: "), d.m2, derpMap) defer ms2.Close() - cleanup = meshStacks(t.Logf, []*magicStack{ms1, ms2}) + cleanup = meshStacks(t.Logf, nil, ms1, ms2) defer cleanup() pkt := tuntest.Ping(ms2.IP().IPAddr().IP, ms1.IP().IPAddr().IP) @@ -617,6 +562,54 @@ func TestTwoDevicePing(t *testing.T) { testTwoDevicePing(t, n) } +// Legacy clients appear to new code as peers that know about DERP and +// WireGuard, but don't have a disco key. Check that we can still +// communicate successfully with such peers. +func TestNoDiscoKey(t *testing.T) { + tstest.PanicOnLog() + tstest.ResourceCheck(t) + + derpMap, cleanup := runDERPAndStun(t, t.Logf, nettype.Std{}, netaddr.IPv4(127, 0, 0, 1)) + defer cleanup() + + m1 := newMagicStack(t, t.Logf, nettype.Std{}, derpMap) + defer m1.Close() + m2 := newMagicStack(t, t.Logf, nettype.Std{}, derpMap) + defer m2.Close() + + removeDisco := func(idx int, nm *netmap.NetworkMap) { + for _, p := range nm.Peers { + p.DiscoKey = tailcfg.DiscoKey{} + } + } + + cleanupMesh := meshStacks(t.Logf, removeDisco, m1, m2) + defer cleanupMesh() + + // Wait for both peers to know about each other before we try to + // ping. + for { + if s1 := m1.Status(); len(s1.Peer) != 1 { + time.Sleep(10 * time.Millisecond) + continue + } + if s2 := m2.Status(); len(s2.Peer) != 1 { + time.Sleep(10 * time.Millisecond) + continue + } + break + } + + pkt := tuntest.Ping(m2.IP().IPAddr().IP, m1.IP().IPAddr().IP) + m1.tun.Outbound <- pkt + select { + case <-m2.tun.Inbound: + t.Logf("ping m1>m2 ok") + case <-time.After(10 * time.Second): + t.Fatalf("timed out waiting for ping to transit") + } +} + func TestActiveDiscovery(t *testing.T) { t.Run("simple_internet", func(t *testing.T) { t.Parallel() @@ -685,11 +678,11 @@ func TestActiveDiscovery(t *testing.T) { inet := natlab.NewInternet() lan1 := &natlab.Network{ Name: "lan1", - Prefix4: mustPrefix("192.168.0.0/24"), + Prefix4: netaddr.MustParseIPPrefix("192.168.0.0/24"), } lan2 := &natlab.Network{ Name: "lan2", - Prefix4: mustPrefix("192.168.1.0/24"), + Prefix4: netaddr.MustParseIPPrefix("192.168.1.0/24"), } sif := mstun.Attach("eth0", inet) @@ -729,14 +722,6 @@ func TestActiveDiscovery(t *testing.T) { }) } -func mustPrefix(s string) netaddr.IPPrefix { - pfx, err := netaddr.ParseIPPrefix(s) - if err != nil { - panic(err) - } - return pfx -} - type devices struct { m1 nettype.PacketListener m1IP netaddr.IP @@ -838,12 +823,12 @@ func testActiveDiscovery(t *testing.T, d *devices) { derpMap, cleanup := runDERPAndStun(t, logf, d.stun, d.stunIP) defer cleanup() - m1 := newMagicStack(t, logger.WithPrefix(logf, "conn1: "), d.m1, derpMap, true) + m1 := newMagicStack(t, logger.WithPrefix(logf, "conn1: "), d.m1, derpMap) defer m1.Close() - m2 := newMagicStack(t, logger.WithPrefix(logf, "conn2: "), d.m2, derpMap, true) + m2 := newMagicStack(t, logger.WithPrefix(logf, "conn2: "), d.m2, derpMap) defer m2.Close() - cleanup = meshStacks(logf, []*magicStack{m1, m2}) + cleanup = meshStacks(logf, nil, m1, m2) defer cleanup() m1IP := m1.IP() @@ -894,21 +879,62 @@ func testTwoDevicePing(t *testing.T, d *devices) { derpMap, cleanup := runDERPAndStun(t, logf, d.stun, d.stunIP) defer cleanup() - m1 := newMagicStack(t, logf, d.m1, derpMap, false) + m1 := newMagicStack(t, logf, d.m1, derpMap) defer m1.Close() - m2 := newMagicStack(t, logf, d.m2, derpMap, false) + m2 := newMagicStack(t, logf, d.m2, derpMap) defer m2.Close() - addrs := []netaddr.IPPort{ - netaddr.IPPortFrom(d.m1IP, m1.conn.LocalPort()), - netaddr.IPPortFrom(d.m2IP, m2.conn.LocalPort()), - } - cfgs := makeConfigs(t, addrs) + cleanupMesh := meshStacks(logf, nil, m1, m2) + defer cleanupMesh() - if err := m1.Reconfig(&cfgs[0]); err != nil { + // Wait for magicsock to be told about peers from meshStacks. + tstest.WaitFor(10*time.Second, func() error { + if p := m1.Status().Peer[key.Public(m2.privateKey.Public())]; p == nil || !p.InMagicSock { + return errors.New("m1 not ready") + } + if p := m2.Status().Peer[key.Public(m1.privateKey.Public())]; p == nil || !p.InMagicSock { + return errors.New("m2 not ready") + } + return nil + }) + + m1cfg := &wgcfg.Config{ + Name: "peer1", + PrivateKey: m1.privateKey, + Addresses: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.1/32")}, + Peers: []wgcfg.Peer{ + wgcfg.Peer{ + PublicKey: m2.privateKey.Public(), + AllowedIPs: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.2/32")}, + Endpoints: wgcfg.Endpoints{ + PublicKey: m2.privateKey.Public(), + DiscoKey: m2.conn.DiscoPublicKey(), + }, + PersistentKeepalive: 25, + }, + }, + } + m2cfg := &wgcfg.Config{ + Name: "peer2", + PrivateKey: m2.privateKey, + Addresses: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.2/32")}, + Peers: []wgcfg.Peer{ + wgcfg.Peer{ + PublicKey: m1.privateKey.Public(), + AllowedIPs: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.1/32")}, + Endpoints: wgcfg.Endpoints{ + PublicKey: m1.privateKey.Public(), + DiscoKey: m1.conn.DiscoPublicKey(), + }, + PersistentKeepalive: 25, + }, + }, + } + + if err := m1.Reconfig(m1cfg); err != nil { t.Fatal(err) } - if err := m2.Reconfig(&cfgs[1]); err != nil { + if err := m2.Reconfig(m2cfg); err != nil { t.Fatal(err) } @@ -997,7 +1023,7 @@ func testTwoDevicePing(t *testing.T, d *devices) { t.Run("no-op dev1 reconfig", func(t *testing.T) { setT(t) defer setT(outerT) - if err := m1.Reconfig(&cfgs[0]); err != nil { + if err := m1.Reconfig(m1cfg); err != nil { t.Fatal(err) } ping1(t) @@ -1005,176 +1031,6 @@ func testTwoDevicePing(t *testing.T, d *devices) { }) } -// TestAddrSet tests addrSet appendDests and updateDst. -func TestAddrSet(t *testing.T) { - tstest.PanicOnLog() - tstest.ResourceCheck(t) - - mustIPPortPtr := func(s string) *netaddr.IPPort { - ipp := netaddr.MustParseIPPort(s) - return &ipp - } - ipps := func(ss ...string) (ret []netaddr.IPPort) { - t.Helper() - for _, s := range ss { - ret = append(ret, netaddr.MustParseIPPort(s)) - } - return ret - } - joinUDPs := func(in []netaddr.IPPort) string { - var sb strings.Builder - for i, ua := range in { - if i > 0 { - sb.WriteByte(',') - } - sb.WriteString(ua.String()) - } - return sb.String() - } - var ( - regPacket = []byte("some regular packet") - sprayPacket = []byte("0000") - ) - binary.LittleEndian.PutUint32(sprayPacket[:4], device.MessageInitiationType) - if !shouldSprayPacket(sprayPacket) { - t.Fatal("sprayPacket should be classified as a spray packet for testing") - } - - // A step is either a b+want appendDests tests, or an - // UpdateDst call, depending on which fields are set. - type step struct { - // advance is the time to advance the fake clock - // before the step. - advance time.Duration - - // updateDst, if set, does an UpdateDst call and - // b+want are ignored. - updateDst *netaddr.IPPort - - b []byte - want string // comma-separated - } - tests := []struct { - name string - as *addrSet - steps []step - logCheck func(t *testing.T, logged []byte) - }{ - { - name: "reg_packet_no_curaddr", - as: &addrSet{ - ipPorts: ipps("127.3.3.40:1", "123.45.67.89:123", "10.0.0.1:123"), - curAddr: -1, // unknown - roamAddr: nil, - }, - steps: []step{ - {b: regPacket, want: "127.3.3.40:1"}, - }, - }, - { - name: "reg_packet_have_curaddr", - as: &addrSet{ - ipPorts: ipps("127.3.3.40:1", "123.45.67.89:123", "10.0.0.1:123"), - curAddr: 1, // global IP - roamAddr: nil, - }, - steps: []step{ - {b: regPacket, want: "123.45.67.89:123"}, - }, - }, - { - name: "reg_packet_have_roamaddr", - as: &addrSet{ - ipPorts: ipps("127.3.3.40:1", "123.45.67.89:123", "10.0.0.1:123"), - curAddr: 2, // should be ignored - roamAddr: mustIPPortPtr("5.6.7.8:123"), - }, - steps: []step{ - {b: regPacket, want: "5.6.7.8:123"}, - {updateDst: mustIPPortPtr("10.0.0.1:123")}, // no more roaming - {b: regPacket, want: "10.0.0.1:123"}, - }, - }, - { - name: "start_roaming", - as: &addrSet{ - ipPorts: ipps("127.3.3.40:1", "123.45.67.89:123", "10.0.0.1:123"), - curAddr: 2, - }, - steps: []step{ - {b: regPacket, want: "10.0.0.1:123"}, - {updateDst: mustIPPortPtr("4.5.6.7:123")}, - {b: regPacket, want: "4.5.6.7:123"}, - {updateDst: mustIPPortPtr("5.6.7.8:123")}, - {b: regPacket, want: "5.6.7.8:123"}, - {updateDst: mustIPPortPtr("123.45.67.89:123")}, // end roaming - {b: regPacket, want: "123.45.67.89:123"}, - }, - }, - { - name: "spray_packet", - as: &addrSet{ - ipPorts: ipps("127.3.3.40:1", "123.45.67.89:123", "10.0.0.1:123"), - curAddr: 2, // should be ignored - roamAddr: mustIPPortPtr("5.6.7.8:123"), - }, - steps: []step{ - {b: sprayPacket, want: "127.3.3.40:1,123.45.67.89:123,10.0.0.1:123,5.6.7.8:123"}, - {advance: 300 * time.Millisecond, b: regPacket, want: "127.3.3.40:1,123.45.67.89:123,10.0.0.1:123,5.6.7.8:123"}, - {advance: 300 * time.Millisecond, b: regPacket, want: "127.3.3.40:1,123.45.67.89:123,10.0.0.1:123,5.6.7.8:123"}, - {advance: 3, b: regPacket, want: "5.6.7.8:123"}, - {advance: 2 * time.Millisecond, updateDst: mustIPPortPtr("10.0.0.1:123")}, - {advance: 3, b: regPacket, want: "10.0.0.1:123"}, - }, - }, - { - name: "low_pri", - as: &addrSet{ - ipPorts: ipps("127.3.3.40:1", "123.45.67.89:123", "10.0.0.1:123"), - curAddr: 2, - }, - steps: []step{ - {updateDst: mustIPPortPtr("123.45.67.89:123")}, - {updateDst: mustIPPortPtr("123.45.67.89:123")}, - }, - logCheck: func(t *testing.T, logged []byte) { - if n := bytes.Count(logged, []byte(", keeping current ")); n != 1 { - t.Errorf("low-prio keeping current logged %d times; want 1", n) - } - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - faket := mono.Time(0) - var logBuf bytes.Buffer - tt.as.Logf = func(format string, args ...interface{}) { - fmt.Fprintf(&logBuf, format, args...) - t.Logf(format, args...) - } - tt.as.clock = func() mono.Time { return faket } - for i, st := range tt.steps { - faket = faket.Add(st.advance) - - if st.updateDst != nil { - if err := tt.as.updateDst(*st.updateDst); err != nil { - t.Fatal(err) - } - continue - } - got, _ := tt.as.appendDests(nil, st.b) - if gotStr := joinUDPs(got); gotStr != st.want { - t.Errorf("step %d: got %v; want %v", i, gotStr, st.want) - } - } - if tt.logCheck != nil { - tt.logCheck(t, logBuf.Bytes()) - } - }) - } -} - func TestDiscoMessage(t *testing.T) { c := newConn() c.logf = t.Logf @@ -1182,16 +1038,15 @@ func TestDiscoMessage(t *testing.T) { peer1Pub := c.DiscoPublicKey() peer1Priv := c.discoPrivate - c.endpointOfDisco = map[tailcfg.DiscoKey]*discoEndpoint{ - tailcfg.DiscoKey(peer1Pub): { - // ... (enough for this test) - }, - } - c.nodeOfDisco = map[tailcfg.DiscoKey]*tailcfg.Node{ - tailcfg.DiscoKey(peer1Pub): { - // ... (enough for this test) - }, + n := &tailcfg.Node{ + Key: tailcfg.NodeKey(key.NewPrivate().Public()), + DiscoKey: peer1Pub, } + c.peerMap.upsertNode(n) + c.peerMap.upsertDiscoEndpoint(&discoEndpoint{ + publicKey: n.Key, + discoKey: n.DiscoKey, + }) const payload = "why hello" @@ -1242,8 +1097,8 @@ func Test32bitAlignment(t *testing.T) { } } -// newNonLegacyTestConn returns a new Conn with DisableLegacyNetworking set true. -func newNonLegacyTestConn(t testing.TB) *Conn { +// newTestConn returns a new Conn. +func newTestConn(t testing.TB) *Conn { t.Helper() port := pickPort(t) conn, err := NewConn(Options{ @@ -1252,7 +1107,6 @@ func newNonLegacyTestConn(t testing.TB) *Conn { EndpointsFunc: func(eps []tailcfg.Endpoint) { t.Logf("endpoints: %q", eps) }, - DisableLegacyNetworking: true, }) if err != nil { t.Fatal(err) @@ -1301,7 +1155,7 @@ func addTestEndpoint(tb testing.TB, conn *Conn, sendConn net.PacketConn) (tailcf } func setUpReceiveFrom(tb testing.TB) (roundTrip func()) { - conn := newNonLegacyTestConn(tb) + conn := newTestConn(tb) tb.Cleanup(func() { conn.Close() }) conn.logf = logger.Discard @@ -1451,7 +1305,7 @@ func logBufWriter(buf *bytes.Buffer) logger.Logf { // // https://github.com/tailscale/tailscale/issues/1391 func TestSetNetworkMapChangingNodeKey(t *testing.T) { - conn := newNonLegacyTestConn(t) + conn := newTestConn(t) t.Cleanup(func() { conn.Close() }) var logBuf bytes.Buffer conn.logf = logBufWriter(&logBuf) @@ -1488,14 +1342,14 @@ func TestSetNetworkMapChangingNodeKey(t *testing.T) { }) } - de := conn.endpointOfDisco[discoKey] - if de != nil && de.publicKey != nodeKey2 { + de, ok := conn.peerMap.discoEndpointForDiscoKey(discoKey) + if ok && de.publicKey != nodeKey2 { t.Fatalf("discoEndpoint public key = %q; want %q", de.publicKey[:], nodeKey2[:]) } log := logBuf.String() wantSub := map[string]int{ - "magicsock: got updated network map; 1 peers (1 with discokey)": 2, + "magicsock: got updated network map; 1 peers": 2, "magicsock: disco key discokey:0000000000000000000000000000000000000000000000000000000000000001 changed from node key [TksxA] to [TksyA]": 1, } for sub, want := range wantSub { @@ -1510,7 +1364,7 @@ func TestSetNetworkMapChangingNodeKey(t *testing.T) { } func TestRebindStress(t *testing.T) { - conn := newNonLegacyTestConn(t) + conn := newTestConn(t) var logBuf bytes.Buffer conn.logf = logBufWriter(&logBuf) diff --git a/wgengine/wgcfg/clone.go b/wgengine/wgcfg/clone.go index a29e0262d..5608c66fa 100644 --- a/wgengine/wgcfg/clone.go +++ b/wgengine/wgcfg/clone.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Code generated by tailscale.com/cmd/cloner -type Config,Peer,Endpoints,IPPortSet; DO NOT EDIT. +// Code generated by tailscale.com/cmd/cloner -type Config,Peer,Endpoints; DO NOT EDIT. package wgcfg @@ -30,7 +30,7 @@ func (src *Config) Clone() *Config { } // A compilation failure here means this code must be regenerated, with command: -// tailscale.com/cmd/cloner -type Config,Peer,Endpoints,IPPortSet +// tailscale.com/cmd/cloner -type Config,Peer,Endpoints var _ConfigNeedsRegeneration = Config(struct { Name string PrivateKey wgkey.Private @@ -49,12 +49,11 @@ func (src *Peer) Clone() *Peer { dst := new(Peer) *dst = *src dst.AllowedIPs = append(src.AllowedIPs[:0:0], src.AllowedIPs...) - dst.Endpoints = *src.Endpoints.Clone() return dst } // A compilation failure here means this code must be regenerated, with command: -// tailscale.com/cmd/cloner -type Config,Peer,Endpoints,IPPortSet +// tailscale.com/cmd/cloner -type Config,Peer,Endpoints var _PeerNeedsRegeneration = Peer(struct { PublicKey wgkey.Key AllowedIPs []netaddr.IPPrefix @@ -70,32 +69,12 @@ func (src *Endpoints) Clone() *Endpoints { } dst := new(Endpoints) *dst = *src - dst.IPPorts = *src.IPPorts.Clone() return dst } // A compilation failure here means this code must be regenerated, with command: -// tailscale.com/cmd/cloner -type Config,Peer,Endpoints,IPPortSet +// tailscale.com/cmd/cloner -type Config,Peer,Endpoints var _EndpointsNeedsRegeneration = Endpoints(struct { PublicKey wgkey.Key DiscoKey tailcfg.DiscoKey - IPPorts IPPortSet -}{}) - -// Clone makes a deep copy of IPPortSet. -// The result aliases no memory with the original. -func (src *IPPortSet) Clone() *IPPortSet { - if src == nil { - return nil - } - dst := new(IPPortSet) - *dst = *src - dst.ipp = append(src.ipp[:0:0], src.ipp...) - return dst -} - -// A compilation failure here means this code must be regenerated, with command: -// tailscale.com/cmd/cloner -type Config,Peer,Endpoints,IPPortSet -var _IPPortSetNeedsRegeneration = IPPortSet(struct { - ipp []netaddr.IPPort }{}) diff --git a/wgengine/wgcfg/config.go b/wgengine/wgcfg/config.go index 31dc4b238..76577b81f 100644 --- a/wgengine/wgcfg/config.go +++ b/wgengine/wgcfg/config.go @@ -6,15 +6,12 @@ package wgcfg import ( - "encoding/json" - "strings" - "inet.af/netaddr" "tailscale.com/tailcfg" "tailscale.com/types/wgkey" ) -//go:generate go run tailscale.com/cmd/cloner -type=Config,Peer,Endpoints,IPPortSet -output=clone.go +//go:generate go run tailscale.com/cmd/cloner -type=Config,Peer,Endpoints -output=clone.go // Config is a WireGuard configuration. // It only supports the set of things Tailscale uses. @@ -36,84 +33,13 @@ type Peer struct { // Endpoints represents the routes to reach a remote node. // It is serialized and provided to wireguard-go as a conn.Endpoint. +// +// TODO: change name, it's now just a pair of keys representing a peer. type Endpoints struct { // PublicKey is the public key for the remote node. PublicKey wgkey.Key `json:"pk"` // DiscoKey is the disco key associated with the remote node. DiscoKey tailcfg.DiscoKey `json:"dk,omitempty"` - // IPPorts is a set of possible ip+ports the remote node can be reached at. - // This is used only for legacy connections to pre-disco (pre-0.100) peers. - IPPorts IPPortSet `json:"ipp,omitempty"` -} - -func (e Endpoints) Equal(f Endpoints) bool { - if e.PublicKey != f.PublicKey { - return false - } - if e.DiscoKey != f.DiscoKey { - return false - } - return e.IPPorts.EqualUnordered(f.IPPorts) -} - -// IPPortSet is an immutable slice of netaddr.IPPorts. -type IPPortSet struct { - ipp []netaddr.IPPort -} - -// NewIPPortSet returns an IPPortSet containing the ports in ipp. -func NewIPPortSet(ipps ...netaddr.IPPort) IPPortSet { - return IPPortSet{ipp: append(ipps[:0:0], ipps...)} -} - -// String returns a comma-separated list of all IPPorts in s. -func (s IPPortSet) String() string { - buf := new(strings.Builder) - for i, ipp := range s.ipp { - if i > 0 { - buf.WriteByte(',') - } - buf.WriteString(ipp.String()) - } - return buf.String() -} - -// IPPorts returns a slice of netaddr.IPPorts containing the IPPorts in s. -func (s IPPortSet) IPPorts() []netaddr.IPPort { - return append(s.ipp[:0:0], s.ipp...) -} - -// EqualUnordered reports whether s and t contain the same IPPorts, regardless of order. -func (s IPPortSet) EqualUnordered(t IPPortSet) bool { - if len(s.ipp) != len(t.ipp) { - return false - } - // Check whether the endpoints are the same, regardless of order. - ipps := make(map[netaddr.IPPort]int, len(s.ipp)) - for _, ipp := range s.ipp { - ipps[ipp]++ - } - for _, ipp := range t.ipp { - ipps[ipp]-- - } - for _, n := range ipps { - if n != 0 { - return false - } - } - return true -} - -// MarshalJSON marshals s into JSON. -// It is necessary so that IPPortSet's fields can be unexported, to guarantee immutability. -func (s IPPortSet) MarshalJSON() ([]byte, error) { - return json.Marshal(s.ipp) -} - -// UnmarshalJSON unmarshals s from JSON. -// It is necessary so that IPPortSet's fields can be unexported, to guarantee immutability. -func (s *IPPortSet) UnmarshalJSON(b []byte) error { - return json.Unmarshal(b, &s.ipp) } // PeerWithKey returns the Peer with key k and reports whether it was found. diff --git a/wgengine/wgcfg/device_test.go b/wgengine/wgcfg/device_test.go index c9f9c1fe4..d10c9dc90 100644 --- a/wgengine/wgcfg/device_test.go +++ b/wgengine/wgcfg/device_test.go @@ -20,6 +20,7 @@ import ( "golang.zx2c4.com/wireguard/device" "golang.zx2c4.com/wireguard/tun" "inet.af/netaddr" + "tailscale.com/tailcfg" "tailscale.com/types/wgkey" ) @@ -128,7 +129,7 @@ func TestDeviceConfig(t *testing.T) { }) t.Run("device1 modify peer", func(t *testing.T) { - cfg1.Peers[0].Endpoints.IPPorts = NewIPPortSet(netaddr.MustParseIPPort("1.2.3.4:12345")) + cfg1.Peers[0].Endpoints.DiscoKey = tailcfg.DiscoKey{1} if err := ReconfigDevice(device1, cfg1, t.Logf); err != nil { t.Fatal(err) } @@ -136,7 +137,7 @@ func TestDeviceConfig(t *testing.T) { }) t.Run("device1 replace endpoint", func(t *testing.T) { - cfg1.Peers[0].Endpoints.IPPorts = NewIPPortSet(netaddr.MustParseIPPort("1.1.1.1:123")) + cfg1.Peers[0].Endpoints.DiscoKey = tailcfg.DiscoKey{2} if err := ReconfigDevice(device1, cfg1, t.Logf); err != nil { t.Fatal(err) } diff --git a/wgengine/wgcfg/nmcfg/nmcfg.go b/wgengine/wgcfg/nmcfg/nmcfg.go index eaf051c78..3a1ae4b6d 100644 --- a/wgengine/wgcfg/nmcfg/nmcfg.go +++ b/wgengine/wgcfg/nmcfg/nmcfg.go @@ -78,19 +78,6 @@ func WGCfg(nm *netmap.NetworkMap, logf logger.Logf, flags netmap.WGConfigFlags, } cpeer.Endpoints = wgcfg.Endpoints{PublicKey: wgkey.Key(peer.Key), DiscoKey: peer.DiscoKey} - if peer.DiscoKey.IsZero() { - // Legacy connection. Add IP+port endpoints. - var ipps []netaddr.IPPort - if err := appendEndpoint(cpeer, &ipps, peer.DERP); err != nil { - return nil, err - } - for _, ep := range peer.Endpoints { - if err := appendEndpoint(cpeer, &ipps, ep); err != nil { - return nil, err - } - } - cpeer.Endpoints.IPPorts = wgcfg.NewIPPortSet(ipps...) - } didExitNodeWarn := false for _, allowedIP := range peer.AllowedIPs { if allowedIP.Bits() == 0 && peer.StableID != exitNode { @@ -135,15 +122,3 @@ func WGCfg(nm *netmap.NetworkMap, logf logger.Logf, flags netmap.WGConfigFlags, return cfg, nil } - -func appendEndpoint(peer *wgcfg.Peer, ipps *[]netaddr.IPPort, epStr string) error { - if epStr == "" { - return nil - } - ipp, err := netaddr.ParseIPPort(epStr) - if err != nil { - return fmt.Errorf("malformed endpoint %q for peer %v", epStr, peer.PublicKey.ShortString()) - } - *ipps = append(*ipps, ipp) - return nil -} diff --git a/wgengine/wgcfg/writer.go b/wgengine/wgcfg/writer.go index c8d09a52a..2be1dca52 100644 --- a/wgengine/wgcfg/writer.go +++ b/wgengine/wgcfg/writer.go @@ -52,7 +52,7 @@ func (cfg *Config) ToUAPI(w io.Writer, prev *Config) error { setPeer(p) set("protocol_version", "1") - if !oldPeer.Endpoints.Equal(p.Endpoints) { + if oldPeer.Endpoints != p.Endpoints { buf, err := json.Marshal(p.Endpoints) if err != nil { return err