diff --git a/internal/deepprint/deepprint_test.go b/internal/deepprint/deepprint_test.go index d7576f27a..573fc5941 100644 --- a/internal/deepprint/deepprint_test.go +++ b/internal/deepprint/deepprint_test.go @@ -39,7 +39,7 @@ func getVal() []interface{} { Addresses: []netaddr.IPPrefix{{Bits: 5, IP: netaddr.IPFrom16([16]byte{3: 3})}}, Peers: []wgcfg.Peer{ { - Endpoints: "foo:5", + Endpoints: wgcfg.Endpoints{HostPorts: []string{"foo:5"}}, }, }, }, diff --git a/wgengine/magicsock/legacy.go b/wgengine/magicsock/legacy.go index b55347054..7d42bf689 100644 --- a/wgengine/magicsock/legacy.go +++ b/wgengine/magicsock/legacy.go @@ -35,7 +35,7 @@ var ( errDisabled = errors.New("magicsock: legacy networking disabled") ) -func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs string) (conn.Endpoint, error) { +func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs []string) (conn.Endpoint, error) { if c.disableLegacy { return nil, errDisabled } @@ -46,14 +46,12 @@ func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs string) (conn.End curAddr: -1, } - if addrs != "" { - for _, ep := range strings.Split(addrs, ",") { - ipp, err := netaddr.ParseIPPort(ep) - if err != nil { - return nil, fmt.Errorf("bogus address %q", ep) - } - a.ipPorts = append(a.ipPorts, ipp) + for _, ep := range addrs { + ipp, err := netaddr.ParseIPPort(ep) + if err != nil { + return nil, fmt.Errorf("bogus address %q", ep) } + a.ipPorts = append(a.ipPorts, ipp) } // If this endpoint is being updated, remember its old set of diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index f34e721b2..ea6600827 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -11,6 +11,7 @@ import ( "context" crand "crypto/rand" "encoding/binary" + "encoding/json" "errors" "fmt" "hash/fnv" @@ -27,7 +28,6 @@ import ( "time" "github.com/tailscale/wireguard-go/conn" - "go4.org/mem" "golang.org/x/crypto/nacl/box" "golang.org/x/time/rate" "inet.af/netaddr" @@ -2755,17 +2755,23 @@ func (c *Conn) ParseEndpoint(keyAddrs string) (conn.Endpoint, error) { c.mu.Lock() defer c.mu.Unlock() - c.logf("magicsock: ParseEndpoint: key=%s: %s", pk.ShortString(), derpStr(addrs)) - - if !strings.HasSuffix(addrs, wgcfg.EndpointDiscoSuffix) { - return c.createLegacyEndpointLocked(pk, addrs) - } - - discoHex := strings.TrimSuffix(addrs, wgcfg.EndpointDiscoSuffix) - discoKey, err := key.NewPublicFromHexMem(mem.S(discoHex)) + var endpoints wgcfg.Endpoints + err := json.Unmarshal([]byte(addrs), &endpoints) if err != nil { - return nil, fmt.Errorf("magicsock: invalid discokey endpoint %q for %v: %w", addrs, pk.ShortString(), err) + c.logf("[unexpected] magicsock: failed to parse addrs %q", addrs) + return nil, err } + if pk != key.Public(endpoints.PublicKey) { + c.logf("[unexpected] magicsock: incorrect public key in addrs, want %x, addrs is %q", pk, addrs) + return nil, errors.New("bad public key in CreateEndpoint") + } + c.logf("magicsock: CreateEndpoint: key=%s: %s", pk.ShortString(), derpStr(addrs)) + + discoKey := endpoints.DiscoKey + if discoKey.IsZero() { + return c.createLegacyEndpointLocked(pk, endpoints.HostPorts) + } + de := &discoEndpoint{ c: c, publicKey: tailcfg.NodeKey(pk), // peer public key (for WireGuard + DERP) diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 8490dee66..9aa771b83 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -470,10 +470,14 @@ func makeConfigs(t *testing.T, addrs []netaddr.IPPort) []wgcfg.Config { if peerNum == i { continue } + publicKey := privKeys[peerNum].Public() peer := wgcfg.Peer{ - PublicKey: privKeys[peerNum].Public(), - AllowedIPs: addresses[peerNum], - Endpoints: addr.String(), + PublicKey: publicKey, + AllowedIPs: addresses[peerNum], + Endpoints: wgcfg.Endpoints{ + PublicKey: publicKey, + HostPorts: []string{addr.String()}, + }, PersistentKeepalive: 25, } cfg.Peers = append(cfg.Peers, peer) @@ -1060,12 +1064,12 @@ func testTwoDevicePing(t *testing.T, d *devices) { }) // Add DERP relay. - derpEp := "127.3.3.40:1" + derpEp := []string{"127.3.3.40:1"} ep0 := cfgs[0].Peers[0].Endpoints - ep0 = derpEp + "," + ep0 + ep0.HostPorts = append(derpEp[:1:1], ep0.HostPorts...) cfgs[0].Peers[0].Endpoints = ep0 ep1 := cfgs[1].Peers[0].Endpoints - ep1 = derpEp + "," + ep1 + ep1.HostPorts = append(derpEp[:1:1], ep1.HostPorts...) cfgs[1].Peers[0].Endpoints = ep1 if err := m1.Reconfig(&cfgs[0]); err != nil { t.Fatal(err) @@ -1081,8 +1085,8 @@ func testTwoDevicePing(t *testing.T, d *devices) { }) // Disable real route. - cfgs[0].Peers[0].Endpoints = derpEp - cfgs[1].Peers[0].Endpoints = derpEp + cfgs[0].Peers[0].Endpoints.HostPorts = derpEp + cfgs[1].Peers[0].Endpoints.HostPorts = derpEp if err := m1.Reconfig(&cfgs[0]); err != nil { t.Fatal(err) } @@ -1109,7 +1113,7 @@ func testTwoDevicePing(t *testing.T, d *devices) { // Give one peer a non-DERP endpoint. We expect the other to // accept it via roamAddr. cfgs[0].Peers[0].Endpoints = ep0 - if ep2 := cfgs[1].Peers[0].Endpoints; len(ep2) != 1 { + if ep2 := cfgs[1].Peers[0].Endpoints.HostPorts; len(ep2) != 1 { t.Errorf("unexpected peer endpoints in dev2: %v", ep2) } if err := m2.Reconfig(&cfgs[1]); err != nil { @@ -1134,7 +1138,7 @@ func testTwoDevicePing(t *testing.T, d *devices) { t.Fatal(err) } ep2 := cfg.Peers[0].Endpoints - if len(ep2) != 2 { + if len(ep2.HostPorts) != 2 { t.Error("handshake spray failed to find real route") } }) diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 5ab060efa..d4a4061d7 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -675,15 +675,7 @@ func isTrimmablePeer(p *wgcfg.Peer, numPeers int) bool { if forceFullWireguardConfig(numPeers) { return false } - if !isSingleEndpoint(p.Endpoints) { - return false - } - - host, _, err := net.SplitHostPort(p.Endpoints) - if err != nil { - return false - } - if !strings.HasSuffix(host, ".disco.tailscale") { + if p.Endpoints.DiscoKey.IsZero() { return false } @@ -753,26 +745,6 @@ func (e *userspaceEngine) isActiveSince(dk tailcfg.DiscoKey, ip netaddr.IP, t ti return unixTime >= t.Unix() } -// discoKeyFromPeer returns the DiscoKey for a wireguard config's Peer. -// -// Invariant: isTrimmablePeer(p) == true, so it should have 1 endpoint with -// Host of form "<64-hex-digits>.disco.tailscale". If invariant is violated, -// we return the zero value. -func discoKeyFromPeer(p *wgcfg.Peer) tailcfg.DiscoKey { - if len(p.Endpoints) < 64 { - return tailcfg.DiscoKey{} - } - host, rest := p.Endpoints[:64], p.Endpoints[64:] - if !strings.HasPrefix(rest, ".disco.tailscale") { - return tailcfg.DiscoKey{} - } - k, err := key.NewPublicFromHexMem(mem.S(host)) - if err != nil { - return tailcfg.DiscoKey{} - } - return tailcfg.DiscoKey(k) -} - // discoChanged are the set of peers whose disco keys have changed, implying they've restarted. // If a peer is in this set and was previously in the live wireguard config, // it needs to be first removed and then re-added to flush out its wireguard session key. @@ -820,7 +792,7 @@ func (e *userspaceEngine) maybeReconfigWireguardLocked(discoChanged map[key.Publ } continue } - dk := discoKeyFromPeer(p) + dk := p.Endpoints.DiscoKey trackDisco = append(trackDisco, dk) recentlyActive := false for _, cidr := range p.AllowedIPs { @@ -992,19 +964,19 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, // and a second time with it. discoChanged := make(map[key.Public]bool) { - prevEP := make(map[key.Public]string) + prevEP := make(map[key.Public]tailcfg.DiscoKey) for i := range e.lastCfgFull.Peers { - if p := &e.lastCfgFull.Peers[i]; isSingleEndpoint(p.Endpoints) { - prevEP[key.Public(p.PublicKey)] = p.Endpoints + if p := &e.lastCfgFull.Peers[i]; !p.Endpoints.DiscoKey.IsZero() { + prevEP[key.Public(p.PublicKey)] = p.Endpoints.DiscoKey } } for i := range cfg.Peers { p := &cfg.Peers[i] - if !isSingleEndpoint(p.Endpoints) { + if p.Endpoints.DiscoKey.IsZero() { continue } pub := key.Public(p.PublicKey) - if old, ok := prevEP[pub]; ok && old != p.Endpoints { + if old, ok := prevEP[pub]; ok && old != p.Endpoints.DiscoKey { discoChanged[pub] = true e.logf("wgengine: Reconfig: %s changed from %q to %q", pub.ShortString(), old, p.Endpoints) } diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 34a331233..545ba3ea7 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -104,7 +104,9 @@ func TestUserspaceEngineReconfig(t *testing.T) { AllowedIPs: []netaddr.IPPrefix{ {IP: netaddr.IPv4(100, 100, 99, 1), Bits: 32}, }, - Endpoints: discoHex + ".disco.tailscale:12345", + Endpoints: wgcfg.Endpoints{ + DiscoKey: dkFromHex(discoHex), + }, }, }, } diff --git a/wgengine/wgcfg/config.go b/wgengine/wgcfg/config.go index 08328bcea..943715101 100644 --- a/wgengine/wgcfg/config.go +++ b/wgengine/wgcfg/config.go @@ -7,6 +7,7 @@ package wgcfg import ( "inet.af/netaddr" + "tailscale.com/tailcfg" ) // EndpointDiscoSuffix is appended to the hex representation of a peer's discovery key @@ -28,10 +29,17 @@ type Config struct { type Peer struct { PublicKey Key AllowedIPs []netaddr.IPPrefix - Endpoints string // comma-separated host/port pairs: "1.2.3.4:56,[::]:80" + Endpoints Endpoints // comma-separated host/port pairs: "1.2.3.4:56,[::]:80" PersistentKeepalive uint16 } +// TODO: HostPorts always sorted? +type Endpoints struct { + PublicKey Key `json:"pk"` + DiscoKey tailcfg.DiscoKey `json:"dk,omitempty"` + HostPorts []string `json:"hp,omitempty"` +} + // Copy makes a deep copy of Config. // The result aliases no memory with the original. func (cfg Config) Copy() Config { diff --git a/wgengine/wgcfg/device_test.go b/wgengine/wgcfg/device_test.go index 1064b13ab..6b5eb7b74 100644 --- a/wgengine/wgcfg/device_test.go +++ b/wgengine/wgcfg/device_test.go @@ -9,6 +9,7 @@ import ( "bytes" "io" "os" + "reflect" "sort" "strings" "sync" @@ -126,7 +127,7 @@ func TestDeviceConfig(t *testing.T) { }) t.Run("device1 modify peer", func(t *testing.T) { - cfg1.Peers[0].Endpoints = "1.2.3.4:12345" + cfg1.Peers[0].Endpoints.HostPorts = []string{"1.2.3.4:12345"} if err := ReconfigDevice(device1, cfg1, t.Logf); err != nil { t.Fatal(err) } @@ -134,7 +135,7 @@ func TestDeviceConfig(t *testing.T) { }) t.Run("device1 replace endpoint", func(t *testing.T) { - cfg1.Peers[0].Endpoints = "1.1.1.1:123" + cfg1.Peers[0].Endpoints.HostPorts = []string{"1.1.1.1:123"} if err := ReconfigDevice(device1, cfg1, t.Logf); err != nil { t.Fatal(err) } @@ -175,7 +176,7 @@ func TestDeviceConfig(t *testing.T) { } peersEqual := func(p, q Peer) bool { return p.PublicKey == q.PublicKey && p.PersistentKeepalive == q.PersistentKeepalive && - p.Endpoints == q.Endpoints && cidrsEqual(p.AllowedIPs, q.AllowedIPs) + reflect.DeepEqual(p.Endpoints, q.Endpoints) && cidrsEqual(p.AllowedIPs, q.AllowedIPs) } if !peersEqual(peer0(origCfg), peer0(newCfg)) { t.Error("reconfig modified old peer") diff --git a/wgengine/wgcfg/nmcfg/nmcfg.go b/wgengine/wgcfg/nmcfg/nmcfg.go index 443598fdc..8913cdcaa 100644 --- a/wgengine/wgcfg/nmcfg/nmcfg.go +++ b/wgengine/wgcfg/nmcfg/nmcfg.go @@ -79,7 +79,10 @@ func WGCfg(nm *netmap.NetworkMap, logf logger.Logf, flags netmap.WGConfigFlags, } if !peer.DiscoKey.IsZero() { - cpeer.Endpoints = fmt.Sprintf("%x.disco.tailscale:12345", peer.DiscoKey[:]) + cpeer.Endpoints = wgcfg.Endpoints{ + PublicKey: wgcfg.Key(peer.Key), + DiscoKey: peer.DiscoKey, + } } else { if err := appendEndpoint(cpeer, peer.DERP); err != nil { return nil, err @@ -147,9 +150,6 @@ func appendEndpoint(peer *wgcfg.Peer, epStr string) error { if err != nil { return fmt.Errorf("invalid port in endpoint %q for peer %v", epStr, peer.PublicKey.ShortString()) } - if peer.Endpoints != "" { - peer.Endpoints += "," - } - peer.Endpoints += epStr + peer.Endpoints.HostPorts = append(peer.Endpoints.HostPorts, epStr) return nil } diff --git a/wgengine/wgcfg/parser.go b/wgengine/wgcfg/parser.go index ec66eb6bc..ba84dad87 100644 --- a/wgengine/wgcfg/parser.go +++ b/wgengine/wgcfg/parser.go @@ -7,6 +7,7 @@ package wgcfg import ( "bufio" "encoding/hex" + "encoding/json" "fmt" "io" "net" @@ -25,6 +26,7 @@ func (e *ParseError) Error() string { return fmt.Sprintf("%s: %q", e.why, e.offender) } +// TODO: delete?? func validateEndpoints(s string) error { if s == "" { // Otherwise strings.Split of the empty string produces [""]. @@ -167,11 +169,10 @@ func (cfg *Config) handlePublicKeyLine(value string) (*Peer, error) { func (cfg *Config) handlePeerLine(peer *Peer, key, value string) error { switch key { case "endpoint": - err := validateEndpoints(value) + err := json.Unmarshal([]byte(value), &peer.Endpoints) if err != nil { return err } - peer.Endpoints = value case "persistent_keepalive_interval": n, err := strconv.ParseUint(value, 10, 16) if err != nil { diff --git a/wgengine/wgcfg/writer.go b/wgengine/wgcfg/writer.go index f64b71bf3..9ce76dcb4 100644 --- a/wgengine/wgcfg/writer.go +++ b/wgengine/wgcfg/writer.go @@ -5,11 +5,11 @@ package wgcfg import ( + "encoding/json" "fmt" "io" - "sort" + "reflect" "strconv" - "strings" "inet.af/netaddr" ) @@ -52,8 +52,12 @@ func (cfg *Config) ToUAPI(w io.Writer, prev *Config) error { setPeer(p) set("protocol_version", "1") - if !endpointsEqual(oldPeer.Endpoints, p.Endpoints) { - set("endpoint", p.Endpoints) + if !reflect.DeepEqual(oldPeer.Endpoints, p.Endpoints) { + buf, err := json.Marshal(p.Endpoints) + if err != nil { + return err + } + set("endpoint", string(buf)) } // TODO: replace_allowed_ips is expensive. @@ -89,24 +93,6 @@ func (cfg *Config) ToUAPI(w io.Writer, prev *Config) error { return stickyErr } -func endpointsEqual(x, y string) bool { - // Cheap comparisons. - if x == y { - return true - } - xs := strings.Split(x, ",") - ys := strings.Split(y, ",") - if len(xs) != len(ys) { - return false - } - // Otherwise, see if they're the same, but out of order. - sort.Strings(xs) - sort.Strings(ys) - x = strings.Join(xs, ",") - y = strings.Join(ys, ",") - return x == y -} - func cidrsEqual(x, y []netaddr.IPPrefix) bool { // TODO: re-implement using netaddr.IPSet.Equal. if len(x) != len(y) {