diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 7b529a0f8..f3bfa8f73 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -195,14 +195,12 @@ func (m *peerMap) upsertEndpoint(ep *endpoint, oldDiscoKey key.DiscoPublic) { if oldDiscoKey != ep.discoKey { delete(m.nodesOfDisco[oldDiscoKey], ep.publicKey) } - if !ep.discoKey.IsZero() { - set := m.nodesOfDisco[ep.discoKey] - if set == nil { - set = map[key.NodePublic]bool{} - m.nodesOfDisco[ep.discoKey] = set - } - set[ep.publicKey] = true + set := m.nodesOfDisco[ep.discoKey] + if set == nil { + set = map[key.NodePublic]bool{} + m.nodesOfDisco[ep.discoKey] = set } + set[ep.publicKey] = true } // setNodeKeyForIPPort makes future peer lookups by ipp return the @@ -1012,16 +1010,6 @@ func (c *Conn) DiscoPublicKey() key.DiscoPublic { return c.discoPublic } -// PeerHasDiscoKey reports whether peer k supports discovery keys (client version 0.100.0+). -func (c *Conn) PeerHasDiscoKey(k key.NodePublic) bool { - c.mu.Lock() - defer c.mu.Unlock() - if ep, ok := c.peerMap.endpointForNodeKey(k); ok { - return ep.discoKey.IsZero() - } - return false -} - // c.mu must NOT be held. func (c *Conn) setNearestDERP(derpNum int) (wantDERP bool) { c.mu.Lock() @@ -2037,9 +2025,6 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netip.AddrPort, derpNodeSrc ke c.logf("magicsock: disco: ignoring CallMeMaybe from %v; %v is unknown", sender.ShortString(), derpNodeSrc.ShortString()) return } - if !ep.canP2PLocked() { - return - } if ep.discoKey != di.discoKey { metricRecvDiscoCallMeMaybeBadDisco.Add(1) c.logf("[unexpected] CallMeMaybe from peer via DERP whose netmap discokey != disco source") @@ -2390,19 +2375,9 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { return } - numNoDisco := 0 - for _, n := range nm.Peers { - if n.DiscoKey.IsZero() { - numNoDisco++ - } - } - metricNumPeers.Set(int64(len(nm.Peers))) c.logf("[v1] magicsock: got updated network map; %d peers", len(nm.Peers)) - if numNoDisco != 0 { - c.logf("magicsock: %d DERP-only peers (no discokey)", numNoDisco) - } c.netMap = nm heartbeatDisabled := debugEnableSilentDisco() || (c.netMap != nil && c.netMap.Debug != nil && c.netMap.Debug.EnableSilentDisco) @@ -2414,11 +2389,21 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { // handle full set updates. for _, n := range nm.Peers { if ep, ok := c.peerMap.endpointForNodeKey(n.Key); ok { + if n.DiscoKey.IsZero() { + // Discokey transitioned from non-zero to zero? Ignore. Server's confused. + c.peerMap.deleteEndpoint(ep) + continue + } oldDiscoKey := ep.discoKey ep.updateFromNode(n, heartbeatDisabled) c.peerMap.upsertEndpoint(ep, oldDiscoKey) // maybe update discokey mappings in peerMap continue } + if n.DiscoKey.IsZero() { + // Ancient pre-0.100 node. Ignore, so we can assume elsewhere in magicsock + // that all nodes have a DiscoKey. + continue + } ep := &endpoint{ c: c, @@ -2431,10 +2416,8 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { if len(n.Addresses) > 0 { ep.nodeAddr = n.Addresses[0].Addr() } - if !n.DiscoKey.IsZero() { - ep.discoKey = n.DiscoKey - ep.discoShort = n.DiscoKey.ShortString() - } + ep.discoKey = n.DiscoKey + ep.discoShort = n.DiscoKey.ShortString() ep.initFakeUDPAddr() if debugDisco() { // rather than making a new knob c.logf("magicsock: created endpoint key=%s: disco=%s; %v", n.Key.ShortString(), n.DiscoKey.ShortString(), logger.ArgWriter(func(w *bufio.Writer) { @@ -3395,7 +3378,7 @@ type endpoint struct { // mu protects all following fields. mu sync.Mutex // Lock ordering: Conn.mu, then endpoint.mu - discoKey key.DiscoPublic // for discovery messages. IsZero() if peer can't disco. + discoKey key.DiscoPublic // for discovery messages. Should never be the zero value. discoShort string // ShortString of discoKey. Empty if peer can't disco. heartBeatTimer *time.Timer // nil when idle @@ -3580,15 +3563,6 @@ func (de *endpoint) DstToString() string { return de.publicKeyHex } func (de *endpoint) DstIP() netip.Addr { panic("unused") } func (de *endpoint) DstToBytes() []byte { return packIPPort(de.fakeWGAddr) } -// canP2PLocked reports whether this endpoint understands the disco protocol -// and is expected to speak it. -// -// As of 2022-11-18, only a few dozen pre-0.100 clients understand -// DERP but not disco, so this returns false very rarely. -func (de *endpoint) canP2PLocked() bool { - return !de.discoKey.IsZero() -} - // addrForSendLocked returns the address(es) that should be used for // sending the next packet. Zero, one, or both of UDP address and DERP // addr may be non-zero. @@ -3617,11 +3591,6 @@ func (de *endpoint) heartbeat() { return } - if !de.canP2PLocked() { - // Cannot form p2p connections, no heartbeating necessary. - return - } - if de.lastSend.IsZero() { // Shouldn't happen. return @@ -3655,9 +3624,6 @@ func (de *endpoint) wantFullPingLocked(now mono.Time) bool { if runtime.GOOS == "js" { return false } - if !de.canP2PLocked() { - return false - } if !de.bestAddr.IsValid() || de.lastFullPing.IsZero() { return true } @@ -3675,7 +3641,7 @@ func (de *endpoint) wantFullPingLocked(now mono.Time) bool { func (de *endpoint) noteActiveLocked() { de.lastSend = mono.Now() - if de.heartBeatTimer == nil && de.canP2PLocked() && !de.heartbeatDisabled { + if de.heartBeatTimer == nil && !de.heartbeatDisabled { de.heartBeatTimer = time.AfterFunc(heartbeatInterval, de.heartbeat) } } @@ -3699,7 +3665,7 @@ func (de *endpoint) cliPing(res *ipnstate.PingResult, cb func(*ipnstate.PingResu // can look like they're bouncing between, say 10.0.0.0/9 and the peer's // IPv6 address, both 1ms away, and it's random who replies first. de.startPingLocked(udpAddr, now, pingCLI) - } else if de.canP2PLocked() { + } else { for ep := range de.endpointState { de.startPingLocked(ep, now, pingCLI) } @@ -3723,7 +3689,7 @@ func (de *endpoint) send(b []byte) error { now := mono.Now() udpAddr, derpAddr := de.addrForSendLocked(now) - if de.canP2PLocked() && (!udpAddr.IsValid() || now.After(de.trustBestAddrUntil)) { + if !udpAddr.IsValid() || now.After(de.trustBestAddrUntil) { de.sendPingsLocked(now, true) } de.noteActiveLocked() @@ -3819,9 +3785,6 @@ const ( ) func (de *endpoint) startPingLocked(ep netip.AddrPort, now mono.Time, purpose discoPingPurpose) { - if !de.canP2PLocked() { - panic("tried to disco ping a peer that can't disco") - } if runtime.GOOS == "js" { return } @@ -4126,11 +4089,6 @@ func (de *endpoint) handleCallMeMaybe(m *disco.CallMeMaybe) { de.mu.Lock() defer de.mu.Unlock() - if !de.canP2PLocked() { - // How did we receive a disco message from a peer that can't disco? - panic("got call-me-maybe from peer with no discokey") - } - now := time.Now() for ep := range de.isCallMeMaybeEP { de.isCallMeMaybeEP[ep] = false // mark for deletion diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 50833a30f..15c9acfa2 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -607,54 +607,6 @@ func TestTwoDevicePing(t *testing.T) { testTwoDevicePing(t, n) } -// Legacy clients appear to new code as peers that know about DERP and -// WireGuard, but don't have a disco key. Check that we can still -// communicate successfully with such peers. -func TestNoDiscoKey(t *testing.T) { - tstest.PanicOnLog() - tstest.ResourceCheck(t) - - derpMap, cleanup := runDERPAndStun(t, t.Logf, localhostListener{}, netaddr.IPv4(127, 0, 0, 1)) - defer cleanup() - - m1 := newMagicStack(t, t.Logf, localhostListener{}, derpMap) - defer m1.Close() - m2 := newMagicStack(t, t.Logf, localhostListener{}, derpMap) - defer m2.Close() - - removeDisco := func(idx int, nm *netmap.NetworkMap) { - for _, p := range nm.Peers { - p.DiscoKey = key.DiscoPublic{} - } - } - - cleanupMesh := meshStacks(t.Logf, removeDisco, m1, m2) - defer cleanupMesh() - - // Wait for both peers to know about each other before we try to - // ping. - for { - if s1 := m1.Status(); len(s1.Peer) != 1 { - time.Sleep(10 * time.Millisecond) - continue - } - if s2 := m2.Status(); len(s2.Peer) != 1 { - time.Sleep(10 * time.Millisecond) - continue - } - break - } - - pkt := tuntest.Ping(m2.IP(), m1.IP()) - m1.tun.Outbound <- pkt - select { - case <-m2.tun.Inbound: - t.Logf("ping m1>m2 ok") - case <-time.After(10 * time.Second): - t.Fatalf("timed out waiting for ping to transit") - } -} - func TestDiscokeyChange(t *testing.T) { tstest.PanicOnLog() tstest.ResourceCheck(t)