diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go index 971fde6c9..ca9b5464d 100644 --- a/util/deephash/deephash_test.go +++ b/util/deephash/deephash_test.go @@ -19,6 +19,7 @@ "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{}, }, }, }, diff --git a/wgengine/bench/wg.go b/wgengine/bench/wg.go index a94364cae..ad05ed848 100644 --- a/wgengine/bench/wg.go +++ b/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) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 2c14308d3..f8f6d39af 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -11,7 +11,6 @@ "context" crand "crypto/rand" "encoding/binary" - "encoding/json" "errors" "fmt" "hash/fnv" @@ -55,7 +54,6 @@ "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() diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 51d4df05e..142e0087d 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -9,7 +9,6 @@ "context" crand "crypto/rand" "crypto/tls" - "encoding/json" "errors" "fmt" "io/ioutil" @@ -942,11 +941,8 @@ func testTwoDevicePing(t *testing.T, d *devices) { Peers: []wgcfg.Peer{ wgcfg.Peer{ PublicKey: m2.privateKey.Public(), + DiscoKey: m2.conn.DiscoPublicKey(), AllowedIPs: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.2/32")}, - Endpoints: wgcfg.Endpoints{ - PublicKey: m2.privateKey.Public(), - DiscoKey: m2.conn.DiscoPublicKey(), - }, }, }, } @@ -957,11 +953,8 @@ func testTwoDevicePing(t *testing.T, d *devices) { Peers: []wgcfg.Peer{ wgcfg.Peer{ PublicKey: m1.privateKey.Public(), + DiscoKey: m1.conn.DiscoPublicKey(), AllowedIPs: []netaddr.IPPrefix{netaddr.MustParseIPPrefix("1.0.0.1/32")}, - Endpoints: wgcfg.Endpoints{ - PublicKey: m1.privateKey.Public(), - DiscoKey: m1.conn.DiscoPublicKey(), - }, }, }, } @@ -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) } diff --git a/wgengine/userspace.go b/wgengine/userspace.go index bc1eb41ae..e4ff19245 100644 --- a/wgengine/userspace.go +++ b/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) } } } diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 8fb88abf3..e6e2b4115 100644 --- a/wgengine/userspace_test.go +++ b/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)}, }, }, } diff --git a/wgengine/wgcfg/clone.go b/wgengine/wgcfg/clone.go index 5608c66fa..f8e10cece 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; 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 -}{}) diff --git a/wgengine/wgcfg/config.go b/wgengine/wgcfg/config.go index 76577b81f..3e22aef22 100644 --- a/wgengine/wgcfg/config.go +++ b/wgengine/wgcfg/config.go @@ -11,7 +11,7 @@ "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 { diff --git a/wgengine/wgcfg/device_test.go b/wgengine/wgcfg/device_test.go index d10c9dc90..c636785ce 100644 --- a/wgengine/wgcfg/device_test.go +++ b/wgengine/wgcfg/device_test.go @@ -10,7 +10,6 @@ "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") diff --git a/wgengine/wgcfg/nmcfg/nmcfg.go b/wgengine/wgcfg/nmcfg/nmcfg.go index 3a1ae4b6d..5fec0be04 100644 --- a/wgengine/wgcfg/nmcfg/nmcfg.go +++ b/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 { diff --git a/wgengine/wgcfg/parser.go b/wgengine/wgcfg/parser.go index af009af4f..b17ae40f2 100644 --- a/wgengine/wgcfg/parser.go +++ b/wgengine/wgcfg/parser.go @@ -7,7 +7,6 @@ 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) diff --git a/wgengine/wgcfg/parser_test.go b/wgengine/wgcfg/parser_test.go index 35f4919b0..59f448702 100644 --- a/wgengine/wgcfg/parser_test.go +++ b/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), diff --git a/wgengine/wgcfg/writer.go b/wgengine/wgcfg/writer.go index 2be1dca52..e83dfccd2 100644 --- a/wgengine/wgcfg/writer.go +++ b/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.