From e921482548e8cfeea9bf8ea5e03eec002feb501f Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 15 Oct 2021 16:42:24 -0700 Subject: [PATCH] wgengine/magicsock: pass src NodeKey to handleDiscoMessage for DERP disco msgs And then use it to avoid another lookup-by-DiscoKey. Updates #3088 (cherry picked from commit 3e80806804aef4b0ae4a264b60e50fafcdd67bfc) --- wgengine/magicsock/magicsock.go | 26 ++++++++++++++++++-------- wgengine/magicsock/magicsock_test.go | 11 ++++++++--- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 369e79273..ac9ad5d6e 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -1392,8 +1392,8 @@ func (c *Conn) setPeerLastDerpLocked(peer key.Public, regionID, homeID int) { // out, which also releases the buffer. type derpReadResult struct { regionID int - n int // length of data received - src key.Public // may be zero until server deployment if v2+ + n int // length of data received + src key.Public // copyBuf is called to copy the data to dst. It returns how // much data was copied, which will be n if dst is large // enough. copyBuf can only be called once. @@ -1596,7 +1596,7 @@ func (c *Conn) receiveIP(b []byte, ipp netaddr.IPPort, cache *ippEndpointCache) c.stunReceiveFunc.Load().(func([]byte, netaddr.IPPort))(b, ipp) return nil, false } - if c.handleDiscoMessage(b, ipp) { + if c.handleDiscoMessage(b, ipp, key.Public{}) { return nil, false } if !c.havePrivateKey.Get() { @@ -1659,7 +1659,7 @@ func (c *Conn) processDERPReadResult(dm derpReadResult, b []byte) (n int, ep *en } ipp := netaddr.IPPortFrom(derpMagicIPAddr, uint16(regionID)) - if c.handleDiscoMessage(b[:n], ipp) { + if c.handleDiscoMessage(b[:n], ipp, dm.src) { return 0, nil } @@ -1732,9 +1732,11 @@ func (c *Conn) sendDiscoMessage(dst netaddr.IPPort, dstKey tailcfg.NodeKey, dstD // * nonce [24]byte // * naclbox of payload (see tailscale.com/disco package for inner payload format) // -// For messages received over DERP, the addr will be derpMagicIP (with -// port being the region) -func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bool) { +// For messages received over DERP, the src.IP() will be derpMagicIP (with +// src.Port() being the region ID) and the derpNodeSrc will be the node key +// it was received from at the DERP layer. derpNodeSrc is zero when received +// over UDP. +func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort, derpNodeSrc key.Public) (isDiscoMsg bool) { const headerLen = len(disco.Magic) + len(tailcfg.DiscoKey{}) + disco.NonceLen if len(msg) < headerLen || string(msg[:len(disco.Magic)]) != disco.Magic { return false @@ -1828,11 +1830,19 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bo case *disco.Pong: ep.handlePongConnLocked(dm, src) case *disco.CallMeMaybe: - if src.IP() != derpMagicIPAddr { + if src.IP() != derpMagicIPAddr || derpNodeSrc.IsZero() { // CallMeMaybe messages should only come via DERP. c.logf("[unexpected] CallMeMaybe packets should only come via DERP") return } + ep, ok := c.peerMap.endpointForNodeKey(tailcfg.NodeKey(derpNodeSrc)) + if !ok { + c.logf("magicsock: disco: ignoring CallMeMaybe from %v; %v is unknown", sender.ShortString(), derpNodeSrc.ShortString()) + return + } + if !ep.canP2P() { + return + } c.logf("[v1] magicsock: disco: %v<-%v (%v, %v) got call-me-maybe, %d endpoints", c.discoShort, ep.discoShort, ep.publicKey.ShortString(), derpStr(src.String()), diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 2a54f9dce..eb99c90ac 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -1158,7 +1158,7 @@ func TestDiscoMessage(t *testing.T) { pkt = append(pkt, nonce[:]...) pkt = box.Seal(pkt, []byte(payload), &nonce, c.discoPrivate.Public().B32(), peer1Priv.B32()) - got := c.handleDiscoMessage(pkt, netaddr.IPPort{}) + got := c.handleDiscoMessage(pkt, netaddr.IPPort{}, key.Public{}) if !got { t.Error("failed to open it") } @@ -1434,15 +1434,20 @@ func TestSetNetworkMapChangingNodeKey(t *testing.T) { }) } - de, ok := conn.peerMap.endpointForDiscoKey(discoKey) + de, ok := conn.peerMap.endpointForNodeKey(nodeKey2) if ok && de.publicKey != nodeKey2 { t.Fatalf("discoEndpoint public key = %q; want %q", de.publicKey[:], nodeKey2[:]) } + if de.discoKey != discoKey { + t.Errorf("discoKey = %v; want %v", de.discoKey, discoKey) + } + if _, ok := conn.peerMap.endpointForNodeKey(nodeKey1); ok { + t.Errorf("didn't expect to find node for key1") + } log := buf.String() wantSub := map[string]int{ "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 { got := strings.Count(log, sub)