wgengine/wgcfg: use just the hexlified node key as the WireGuard endpoint.

The node key is all magicsock needs to find the endpoint that WireGuard
needs.

Updates #2752

Signed-off-by: David Anderson <danderson@tailscale.com>

util/deephash/deephash_test.go

@@ -19,6 +19,7 @@ import (
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/dnstype"
 	"tailscale.com/types/ipproto"
+	"tailscale.com/types/wgkey"
 	"tailscale.com/util/dnsname"
 	"tailscale.com/version"
 	"tailscale.com/wgengine/filter"
@@ -137,7 +138,7 @@ func getVal() []interface{} {
 			Addresses: []netaddr.IPPrefix{netaddr.IPPrefixFrom(netaddr.IPFrom16([16]byte{3: 3}), 5)},
 			Peers: []wgcfg.Peer{
 				{
-					Endpoints: wgcfg.Endpoints{},
+					PublicKey: wgkey.Key{},
 				},
 			},
 		},

wgengine/bench/wg.go

@@ -102,9 +102,6 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd
 		for _, ep := range st.LocalAddrs {
 			eps = append(eps, ep.Addr.String())
 		}
-		endpoint := wgcfg.Endpoints{
-			PublicKey: c1.PrivateKey.Public(),
-		}
 
 		n := tailcfg.Node{
 			ID:         tailcfg.NodeID(0),
@@ -122,7 +119,6 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd
 		p := wgcfg.Peer{
 			PublicKey:  c1.PrivateKey.Public(),
 			AllowedIPs: []netaddr.IPPrefix{a1},
-			Endpoints:  endpoint,
 		}
 		c2.Peers = []wgcfg.Peer{p}
 		e2.Reconfig(&c2, &router.Config{}, new(dns.Config), nil)
@@ -143,9 +139,6 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd
 		for _, ep := range st.LocalAddrs {
 			eps = append(eps, ep.Addr.String())
 		}
-		endpoint := wgcfg.Endpoints{
-			PublicKey: c2.PrivateKey.Public(),
-		}
 
 		n := tailcfg.Node{
 			ID:         tailcfg.NodeID(0),
@@ -163,7 +156,6 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd
 		p := wgcfg.Peer{
 			PublicKey:  c2.PrivateKey.Public(),
 			AllowedIPs: []netaddr.IPPrefix{a2},
-			Endpoints:  endpoint,
 		}
 		c1.Peers = []wgcfg.Peer{p}
 		e1.Reconfig(&c1, &router.Config{}, new(dns.Config), nil)

wgengine/magicsock/magicsock.go

@@ -11,7 +11,6 @@ import (
 	"context"
 	crand "crypto/rand"
 	"encoding/binary"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"hash/fnv"
@@ -55,7 +54,6 @@ import (
 	"tailscale.com/util/uniq"
 	"tailscale.com/version"
 	"tailscale.com/wgengine/monitor"
-	"tailscale.com/wgengine/wgcfg"
 )
 
 // useDerpRoute reports whether magicsock should enable the DERP
@@ -2101,20 +2099,7 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) {
 			ep.discoKey = n.DiscoKey
 			ep.discoShort = n.DiscoKey.ShortString()
 		}
-		epDef := wgcfg.Endpoints{
-			PublicKey: wgkey.Key(n.Key),
-			DiscoKey:  n.DiscoKey,
-		}
-		// We have to make the endpoint string we return to
-		// WireGuard be the right kind of json that wgcfg expects
-		// to get back out of uapi, so we have to do this somewhat
-		// unnecessary json encoding here.
-		// TODO(danderson): remove this in the wgcfg.Endpoints refactor.
-		epBytes, err := json.Marshal(epDef)
-		if err != nil {
-			c.logf("[unexpected] magicsock: creating endpoint: failed to marshal endpoints json %w", err)
-		}
-		ep.wgEndpoint = string(epBytes)
+		ep.wgEndpoint = (wgkey.Key(n.Key)).HexString()
 		ep.initFakeUDPAddr()
 		c.logf("magicsock: created endpoint key=%s: disco=%s; %v", n.Key.ShortString(), n.DiscoKey.ShortString(), logger.ArgWriter(func(w *bufio.Writer) {
 			const derpPrefix = "127.3.3.40:"
@@ -2636,14 +2621,12 @@ func packIPPort(ua netaddr.IPPort) []byte {
 }
 
 // ParseEndpoint is called by WireGuard to connect to an endpoint.
-// endpointStr is a json-serialized wgcfg.Endpoints struct.
-func (c *Conn) ParseEndpoint(endpointStr string) (conn.Endpoint, error) {
-	var endpoints wgcfg.Endpoints
-	err := json.Unmarshal([]byte(endpointStr), &endpoints)
+func (c *Conn) ParseEndpoint(nodeKeyStr string) (conn.Endpoint, error) {
+	k, err := wgkey.ParseHex(nodeKeyStr)
 	if err != nil {
-		return nil, fmt.Errorf("magicsock: ParseEndpoint: json.Unmarshal failed on %q: %w", endpointStr, err)
+		return nil, fmt.Errorf("magicsock: ParseEndpoint: parse failed on %q: %w", nodeKeyStr, err)
 	}
-	pk := key.Public(endpoints.PublicKey)
+	pk := tailcfg.NodeKey(k)
 
 	c.mu.Lock()
 	defer c.mu.Unlock()

wgengine/magicsock/magicsock_test.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	crand "crypto/rand"
 	"crypto/tls"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -941,12 +940,9 @@ func testTwoDevicePing(t *testing.T, d *devices) {
 		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(),
-				},
+				AllowedIPs: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.2/32")},
 			},
 		},
 	}
@@ -956,12 +952,9 @@ func testTwoDevicePing(t *testing.T, d *devices) {
 		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(),
-				},
+				AllowedIPs: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.1/32")},
 			},
 		},
 	}
@@ -1158,19 +1151,6 @@ func newTestConn(t testing.TB) *Conn {
 	return conn
 }
 
-func makeEndpoint(tb testing.TB, public tailcfg.NodeKey, disco tailcfg.DiscoKey) string {
-	tb.Helper()
-	ep := wgcfg.Endpoints{
-		PublicKey: wgkey.Key(public),
-		DiscoKey:  disco,
-	}
-	buf, err := json.Marshal(ep)
-	if err != nil {
-		tb.Fatal(err)
-	}
-	return string(buf)
-}
-
 // addTestEndpoint sets conn's network map to a single peer expected
 // to receive packets from sendConn (or DERP), and returns that peer's
 // nodekey and discokey.
@@ -1190,7 +1170,7 @@ func addTestEndpoint(tb testing.TB, conn *Conn, sendConn net.PacketConn) (tailcf
 		},
 	})
 	conn.SetPrivateKey(wgkey.Private{0: 1})
-	_, err := conn.ParseEndpoint(makeEndpoint(tb, nodeKey, discoKey))
+	_, err := conn.ParseEndpoint(wgkey.Key(nodeKey).HexString())
 	if err != nil {
 		tb.Fatal(err)
 	}
@@ -1374,7 +1354,7 @@ func TestSetNetworkMapChangingNodeKey(t *testing.T) {
 			},
 		},
 	})
-	_, err := conn.ParseEndpoint(makeEndpoint(t, nodeKey1, discoKey))
+	_, err := conn.ParseEndpoint(wgkey.Key(nodeKey1).HexString())
 	if err != nil {
 		t.Fatal(err)
 	}

wgengine/userspace.go

@@ -843,19 +843,19 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 	{
 		prevEP := make(map[tailcfg.NodeKey]tailcfg.DiscoKey)
 		for i := range e.lastCfgFull.Peers {
-			if p := &e.lastCfgFull.Peers[i]; !p.Endpoints.DiscoKey.IsZero() {
-				prevEP[tailcfg.NodeKey(p.PublicKey)] = p.Endpoints.DiscoKey
+			if p := &e.lastCfgFull.Peers[i]; !p.DiscoKey.IsZero() {
+				prevEP[tailcfg.NodeKey(p.PublicKey)] = p.DiscoKey
 			}
 		}
 		for i := range cfg.Peers {
 			p := &cfg.Peers[i]
-			if p.Endpoints.DiscoKey.IsZero() {
+			if p.DiscoKey.IsZero() {
 				continue
 			}
 			pub := tailcfg.NodeKey(p.PublicKey)
-			if old, ok := prevEP[pub]; ok && old != p.Endpoints.DiscoKey {
+			if old, ok := prevEP[pub]; ok && old != p.DiscoKey {
 				discoChanged[pub] = true
-				e.logf("wgengine: Reconfig: %s changed from %q to %q", pub.ShortString(), old, p.Endpoints)
+				e.logf("wgengine: Reconfig: %s changed from %q to %q", pub.ShortString(), old, p.DiscoKey)
 			}
 		}
 	}

wgengine/userspace_test.go

@@ -114,7 +114,6 @@ func TestUserspaceEngineReconfig(t *testing.T) {
 					AllowedIPs: []netaddr.IPPrefix{
 						netaddr.IPPrefixFrom(netaddr.IPv4(100, 100, 99, 1), 32),
 					},
-					Endpoints: wgcfg.Endpoints{PublicKey: wgkey.Key(nkFromHex(nodeHex))},
 				},
 			},
 		}
@@ -168,10 +167,10 @@ func TestUserspaceEnginePortReconfig(t *testing.T) {
 	cfg := &wgcfg.Config{
 		Peers: []wgcfg.Peer{
 			{
+				PublicKey: wgkey.Key(nodeKey),
 				AllowedIPs: []netaddr.IPPrefix{
 					netaddr.IPPrefixFrom(netaddr.IPv4(100, 100, 99, 1), 32),
 				},
-				Endpoints: wgcfg.Endpoints{PublicKey: wgkey.Key(nodeKey)},
 			},
 		},
 	}

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; DO NOT EDIT.
+// Code generated by tailscale.com/cmd/cloner -type Config,Peer; 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
+//   tailscale.com/cmd/cloner -type Config,Peer
 var _ConfigNeedsRegeneration = Config(struct {
 	Name       string
 	PrivateKey wgkey.Private
@@ -53,28 +53,10 @@ func (src *Peer) Clone() *Peer {
 }
 
 // A compilation failure here means this code must be regenerated, with command:
-//   tailscale.com/cmd/cloner -type Config,Peer,Endpoints
+//   tailscale.com/cmd/cloner -type Config,Peer
 var _PeerNeedsRegeneration = Peer(struct {
 	PublicKey           wgkey.Key
+	DiscoKey            tailcfg.DiscoKey
 	AllowedIPs          []netaddr.IPPrefix
-	Endpoints           Endpoints
 	PersistentKeepalive uint16
 }{})
-
-// Clone makes a deep copy of Endpoints.
-// The result aliases no memory with the original.
-func (src *Endpoints) Clone() *Endpoints {
-	if src == nil {
-		return nil
-	}
-	dst := new(Endpoints)
-	*dst = *src
-	return dst
-}
-
-// A compilation failure here means this code must be regenerated, with command:
-//   tailscale.com/cmd/cloner -type Config,Peer,Endpoints
-var _EndpointsNeedsRegeneration = Endpoints(struct {
-	PublicKey wgkey.Key
-	DiscoKey  tailcfg.DiscoKey
-}{})

wgengine/wgcfg/config.go

@@ -11,7 +11,7 @@ import (
 	"tailscale.com/types/wgkey"
 )
 
-//go:generate go run tailscale.com/cmd/cloner -type=Config,Peer,Endpoints -output=clone.go
+//go:generate go run tailscale.com/cmd/cloner -type=Config,Peer -output=clone.go
 
 // Config is a WireGuard configuration.
 // It only supports the set of things Tailscale uses.
@@ -26,22 +26,11 @@ type Config struct {
 
 type Peer struct {
 	PublicKey           wgkey.Key
+	DiscoKey            tailcfg.DiscoKey // present only so we can handle restarts within wgengine, not passed to WireGuard
 	AllowedIPs          []netaddr.IPPrefix
-	Endpoints           Endpoints
 	PersistentKeepalive uint16
 }
 
-// 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"`
-}
-
 // PeerWithKey returns the Peer with key k and reports whether it was found.
 func (config Config) PeerWithKey(k wgkey.Key) (Peer, bool) {
 	for _, p := range config.Peers {

wgengine/wgcfg/device_test.go

@@ -10,7 +10,6 @@ import (
 	"io"
 	"net"
 	"os"
-	"reflect"
 	"sort"
 	"strings"
 	"sync"
@@ -129,7 +128,7 @@ func TestDeviceConfig(t *testing.T) {
 	})
 
 	t.Run("device1 modify peer", func(t *testing.T) {
-		cfg1.Peers[0].Endpoints.DiscoKey = tailcfg.DiscoKey{1}
+		cfg1.Peers[0].DiscoKey = tailcfg.DiscoKey{1}
 		if err := ReconfigDevice(device1, cfg1, t.Logf); err != nil {
 			t.Fatal(err)
 		}
@@ -137,7 +136,7 @@ func TestDeviceConfig(t *testing.T) {
 	})
 
 	t.Run("device1 replace endpoint", func(t *testing.T) {
-		cfg1.Peers[0].Endpoints.DiscoKey = tailcfg.DiscoKey{2}
+		cfg1.Peers[0].DiscoKey = tailcfg.DiscoKey{2}
 		if err := ReconfigDevice(device1, cfg1, t.Logf); err != nil {
 			t.Fatal(err)
 		}
@@ -177,8 +176,7 @@ func TestDeviceConfig(t *testing.T) {
 			return p
 		}
 		peersEqual := func(p, q Peer) bool {
-			return p.PublicKey == q.PublicKey && p.PersistentKeepalive == q.PersistentKeepalive &&
-				reflect.DeepEqual(p.Endpoints, q.Endpoints) && cidrsEqual(p.AllowedIPs, q.AllowedIPs)
+			return p.PublicKey == q.PublicKey && p.DiscoKey == q.DiscoKey && p.PersistentKeepalive == q.PersistentKeepalive && cidrsEqual(p.AllowedIPs, q.AllowedIPs)
 		}
 		if !peersEqual(peer0(origCfg), peer0(newCfg)) {
 			t.Error("reconfig modified old peer")

wgengine/wgcfg/nmcfg/nmcfg.go

@@ -71,13 +71,13 @@ func WGCfg(nm *netmap.NetworkMap, logf logger.Logf, flags netmap.WGConfigFlags,
 		}
 		cfg.Peers = append(cfg.Peers, wgcfg.Peer{
 			PublicKey: wgkey.Key(peer.Key),
+			DiscoKey:  peer.DiscoKey,
 		})
 		cpeer := &cfg.Peers[len(cfg.Peers)-1]
 		if peer.KeepAlive {
 			cpeer.PersistentKeepalive = 25 // seconds
 		}
 
-		cpeer.Endpoints = wgcfg.Endpoints{PublicKey: wgkey.Key(peer.Key), DiscoKey: peer.DiscoKey}
 		didExitNodeWarn := false
 		for _, allowedIP := range peer.AllowedIPs {
 			if allowedIP.Bits() == 0 && peer.StableID != exitNode {

wgengine/wgcfg/parser.go

@@ -7,7 +7,6 @@ package wgcfg
 import (
 	"bufio"
 	"encoding/hex"
-	"encoding/json"
 	"fmt"
 	"io"
 	"net"
@@ -156,8 +155,17 @@ func (cfg *Config) handlePublicKeyLine(valueBytes []byte) (*Peer, error) {
 func (cfg *Config) handlePeerLine(peer *Peer, key, value mem.RO, valueBytes []byte) error {
 	switch {
 	case key.EqualString("endpoint"):
-		if err := json.Unmarshal(valueBytes, &peer.Endpoints); err != nil {
-			return err
+		// TODO: our key types are all over the place, and this
+		// particular one can't parse a mem.RO or a []byte without
+		// allocating. We don't reconfigure wireguard often though, so
+		// this is okay.
+		s := value.StringCopy()
+		k, err := wgkey.ParseHex(s)
+		if err != nil {
+			return fmt.Errorf("invalid endpoint %q for peer %q, expected a hex public key", s, peer.PublicKey.ShortString())
+		}
+		if k != peer.PublicKey {
+			return fmt.Errorf("unexpected endpoint %q for peer %q, expected the peer's public key", s, peer.PublicKey.ShortString())
 		}
 	case key.EqualString("persistent_keepalive_interval"):
 		n, err := mem.ParseUint(value, 10, 16)

wgengine/wgcfg/parser_test.go

@@ -75,7 +75,6 @@ func BenchmarkFromUAPI(b *testing.B) {
 	peer := Peer{
 		PublicKey:  k1,
 		AllowedIPs: []netaddr.IPPrefix{ip1},
-		Endpoints:  Endpoints{PublicKey: k1},
 	}
 	cfg1 := &Config{
 		PrivateKey: wgkey.Private(pk1),

wgengine/wgcfg/writer.go

@@ -5,7 +5,6 @@
 package wgcfg
 
 import (
-	"encoding/json"
 	"fmt"
 	"io"
 	"strconv"
@@ -48,16 +47,15 @@ func (cfg *Config) ToUAPI(w io.Writer, prev *Config) error {
 
 	// Add/configure all new peers.
 	for _, p := range cfg.Peers {
-		oldPeer := old[p.PublicKey]
+		oldPeer, wasPresent := old[p.PublicKey]
 		setPeer(p)
 		set("protocol_version", "1")
 
-		if oldPeer.Endpoints != p.Endpoints {
-			buf, err := json.Marshal(p.Endpoints)
-			if err != nil {
-				return err
-			}
-			set("endpoint", string(buf))
+		// Avoid setting endpoints if the correct one is already known
+		// to WireGuard, because doing so generates a bit more work in
+		// calling magicsock's ParseEndpoint for effectively a no-op.
+		if !wasPresent {
+			set("endpoint", p.PublicKey.HexString())
 		}
 
 		// TODO: replace_allowed_ips is expensive.