all: add extra information to serialized endpoints

magicsock.Conn.ParseEndpoint requires a peer's public key,
disco key, and legacy ip/ports in order to do its job.
We currently accomplish that by:

* adding the public key in our wireguard-go fork
* encoding the disco key as magic hostname
* using a bespoke comma-separated encoding

It's a bit messy.

Instead, switch to something simpler: use a json-encoded struct
containing exactly the information we need, in the form we use it.

Our wireguard-go fork still adds the public key to the
address when it passes it to ParseEndpoint, but now the code
compensating for that is just a couple of simple, well-commented lines.
Once this commit is in, we can remove that part of the fork
and remove the compensating code.

Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>

wgengine/magicsock/legacy.go

@@ -10,7 +10,6 @@ import (
 	"crypto/subtle"
 	"encoding/binary"
 	"errors"
-	"fmt"
 	"hash"
 	"net"
 	"strings"
@@ -27,6 +26,7 @@ import (
 	"tailscale.com/types/key"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/wgkey"
+	"tailscale.com/wgengine/wgcfg"
 )
 
 var (
@@ -34,7 +34,11 @@ var (
 	errDisabled       = errors.New("magicsock: legacy networking disabled")
 )
 
-func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs string) (conn.Endpoint, error) {
+// 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
 	}
@@ -43,18 +47,9 @@ func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs string) (conn.End
 		Logf:      c.logf,
 		publicKey: pk,
 		curAddr:   -1,
-		rawdst:    addrs,
-	}
-
-	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)
-		}
+		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

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"
@@ -2736,42 +2736,36 @@ func packIPPort(ua netaddr.IPPort) []byte {
 }
 
 // ParseEndpoint is called by WireGuard to connect to an endpoint.
-//
-// keyAddrs is the 32 byte public key of the peer followed by addrs.
-// Addrs is either:
-//
-//  1) a comma-separated list of UDP ip:ports (the peer doesn't have a discovery key)
-//  2) "<hex-discovery-key>.disco.tailscale:12345", a magic value that means the peer
-//     is running code that supports active discovery, so ParseEndpoint returns
-//     a discoEndpoint.
-func (c *Conn) ParseEndpoint(keyAddrs string) (conn.Endpoint, error) {
-	if len(keyAddrs) < 32 {
-		c.logf("[unexpected] ParseEndpoint keyAddrs too short: %q", keyAddrs)
-		return nil, errors.New("endpoint string too short")
+// endpointStr is a json-serialized wgcfg.Endpoints struct.
+// (It it currently prefixed by 32 bytes of junk, but that will change shortly.)
+// If those Endpoints contain an active discovery key, ParseEndpoint returns a discoEndpoint.
+// Otherwise it returns a legacy endpoint.
+func (c *Conn) ParseEndpoint(endpointStr string) (conn.Endpoint, error) {
+	// Our wireguard-go fork prepends the public key to endpointStr.
+	// We don't need it; strip it off.
+	// This code will be deleted soon.
+	endpointStr = endpointStr[32:]
+
+	var endpoints wgcfg.Endpoints
+	err := json.Unmarshal([]byte(endpointStr), &endpoints)
+	if err != nil {
+		return nil, fmt.Errorf("magicsock: ParseEndpoint: json.Unmarshal failed on %q: %w", endpointStr, err)
 	}
-	var pk key.Public
-	copy(pk[:], keyAddrs)
-	addrs := keyAddrs[len(pk):]
+	pk := key.Public(endpoints.PublicKey)
+	discoKey := endpoints.DiscoKey
+	c.logf("magicsock: ParseEndpoint: key=%s: disco=%s ipps=%s", pk.ShortString(), discoKey.ShortString(), derpStr(endpoints.IPPorts.String()))
+
 	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))
-	if err != nil {
-		return nil, fmt.Errorf("magicsock: invalid discokey endpoint %q for %v: %w", addrs, pk.ShortString(), err)
+	if discoKey.IsZero() {
+		return c.createLegacyEndpointLocked(pk, endpoints.IPPorts, endpointStr)
 	}
 	de := &discoEndpoint{
 		c:             c,
 		publicKey:     tailcfg.NodeKey(pk),        // peer public key (for WireGuard + DERP)
 		discoKey:      tailcfg.DiscoKey(discoKey), // for discovery mesages
 		discoShort:    tailcfg.DiscoKey(discoKey).ShortString(),
-		wgEndpoint:    addrs,
+		wgEndpoint:    endpointStr,
 		sentPing:      map[stun.TxID]sentPing{},
 		endpointState: map[netaddr.IPPort]*endpointState{},
 	}
@@ -3115,7 +3109,7 @@ type discoEndpoint struct {
 	discoKey   tailcfg.DiscoKey // for discovery mesages
 	discoShort string           // ShortString of discoKey
 	fakeWGAddr netaddr.IPPort   // the UDP address we tell wireguard-go we're using
-	wgEndpoint string           // string from ParseEndpoint: "<hex-discovery-key>.disco.tailscale:12345"
+	wgEndpoint string           // string from ParseEndpoint, holds a JSON-serialized wgcfg.Endpoints
 
 	// Owned by Conn.mu:
 	lastPingFrom netaddr.IPPort

wgengine/magicsock/magicsock_test.go

@@ -10,6 +10,7 @@ import (
 	crand "crypto/rand"
 	"crypto/tls"
 	"encoding/binary"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -468,10 +469,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,
+					IPPorts:   wgcfg.NewIPPortSet(addr),
+				},
 				PersistentKeepalive: 25,
 			}
 			cfg.Peers = append(cfg.Peers, peer)
@@ -1242,6 +1247,23 @@ func newNonLegacyTestConn(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)
+	}
+	// Our wireguard-go fork prepends the public key when calling ParseEndpoint.
+	// We no longer use it; add some junk there to emulate wireguard-go.
+	// This will go away soon.
+	junk := strings.Repeat(" ", 32)
+	return junk + 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.
@@ -1261,7 +1283,7 @@ func addTestEndpoint(tb testing.TB, conn *Conn, sendConn net.PacketConn) (tailcf
 		},
 	})
 	conn.SetPrivateKey(wgkey.Private{0: 1})
-	_, err := conn.ParseEndpoint(string(nodeKey[:]) + "0000000000000000000000000000000000000000000000000000000000000001.disco.tailscale:12345")
+	_, err := conn.ParseEndpoint(makeEndpoint(tb, nodeKey, discoKey))
 	if err != nil {
 		tb.Fatal(err)
 	}
@@ -1435,7 +1457,7 @@ func TestSetNetworkMapChangingNodeKey(t *testing.T) {
 			},
 		},
 	})
-	_, err := conn.ParseEndpoint(string(nodeKey1[:]) + "0000000000000000000000000000000000000000000000000000000000000001.disco.tailscale:12345")
+	_, err := conn.ParseEndpoint(makeEndpoint(t, nodeKey1, discoKey))
 	if err != nil {
 		t.Fatal(err)
 	}