From 7258b07cbd12f4d4a19b7a5c3dc6d1b4b0ff566a Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 21 Nov 2023 03:48:49 +0000 Subject: [PATCH 1/5] feat: add magicsock opt to block direct endpoints --- wgengine/magicsock/magicsock.go | 34 +++++++++++++++++++ wgengine/magicsock/magicsock_test.go | 51 ++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index b2971b290a27d..481e9ce9aec81 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -214,6 +214,10 @@ type Conn struct { // that will call Conn.doPeriodicSTUN. periodicReSTUNTimer *time.Timer + // blockEndpoints is whether to avoid capturing, storing and sending + // endpoints gathered from local interfaces or STUN. Only DERP endpoints + // will be sent. + blockEndpoints bool // endpointsUpdateActive indicates that updateEndpoints is // currently running. It's used to deduplicate concurrent endpoint // update requests. @@ -330,6 +334,13 @@ type Options struct { // endpoints change. The called func does not own the slice. EndpointsFunc func([]tailcfg.Endpoint) + // BlockEndpoints is whether to avoid capturing, storing and sending + // endpoints gathered from local interfaces or STUN. Only DERP endpoints + // will be sent. + // This does not disable the UDP socket or portmapping attempts as this + // setting can be toggled at runtime. + BlockEndpoints bool + // DERPActiveFunc optionally provides a func to be called when // a connection is made to a DERP server. DERPActiveFunc func() @@ -580,6 +591,11 @@ func (c *Conn) setEndpoints(endpoints []tailcfg.Endpoint) (changed bool) { c.mu.Lock() defer c.mu.Unlock() + if c.blockEndpoints { + anySTUN = false + endpoints = []tailcfg.Endpoint{} + } + if !anySTUN && c.derpMap == nil && !inTest() { // Don't bother storing or reporting this yet. We // don't have a DERP map or any STUN entries, so we're @@ -826,6 +842,21 @@ func (c *Conn) DiscoPublicKey() key.DiscoPublic { return c.discoPublic } +// SetBlockEndpoints sets the blockEndpoints field. If changed, endpoints will +// be updated to apply the new settings. Existing connections may continue to +// use the old setting until they are reestablished. Disabling endpoints does +// not affect the UDP socket or portmapper. +func (c *Conn) SetBlockEndpoints(block bool) { + c.mu.Lock() + didChange := c.blockEndpoints != block + c.blockEndpoints = block + c.mu.Unlock() + + if didChange { + go c.updateEndpoints("SetBlockEndpoints") + } +} + // determineEndpoints returns the machine's endpoint addresses. It // does a STUN lookup (via netcheck) to determine its public address. // @@ -1648,6 +1679,9 @@ func (c *Conn) enqueueCallMeMaybe(derpAddr netip.AddrPort, de *endpoint) { for _, ep := range c.lastEndpoints { eps = append(eps, ep.Addr) } + // NOTE: sending an empty call-me-maybe (e.g. when BlockEndpoints is true) + // is still valid and results in the other side forgetting all the endpoints + // it knows of ours. go de.c.sendDiscoMessage(derpAddr, de.publicKey, epDisco.key, &disco.CallMeMaybe{MyNumber: eps}, discoLog) if debugSendCallMeUnknownPeer() { // Send a callMeMaybe packet to a non-existent peer diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 254e0ccb0e54b..4881a32ce951c 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -3000,3 +3000,54 @@ func TestDERPForceWebsockets(t *testing.T) { t.Errorf("no websocket upgrade requests seen") } } + +func TestBlockEndpoints(t *testing.T) { + logf, closeLogf := logger.LogfCloser(t.Logf) + defer closeLogf() + + derpMap, cleanup := runDERPAndStun(t, t.Logf, localhostListener{}, netaddr.IPv4(127, 0, 0, 1)) + defer cleanup() + + m := &natlab.Machine{Name: "m1"} + ms := newMagicStackFunc(t, logger.WithPrefix(logf, "conn1: "), m, derpMap, nil) + defer ms.Close() + + // Check that some endpoints exist. This should be the case as we should use + // interface addressess as endpoints instantly on startup, and we already + // have a DERP connection due to newMagicStackFunc. + ms.conn.mu.Lock() + haveEndpoint := false + for _, ep := range ms.conn.lastEndpoints { + if ep.Addr.Addr() == tailcfg.DerpMagicIPAddr { + t.Fatal("DERP IP in endpoints list?", ep.Addr) + } + haveEndpoint = true + break + } + ms.conn.mu.Unlock() + if !haveEndpoint { + t.Fatal("no endpoints found") + } + + // Block endpoints, should result in an update. + ms.conn.SetBlockEndpoints(true) + + // Wait for endpoints to finish updating. + ok := false +parentLoop: + for i := 0; i < 50; i++ { + time.Sleep(100 * time.Millisecond) + ms.conn.mu.Lock() + for _, ep := range ms.conn.lastEndpoints { + t.Errorf("endpoint %v was not blocked", ep.Addr) + ms.conn.mu.Unlock() + continue parentLoop + } + ms.conn.mu.Unlock() + ok = true + break + } + if !ok { + t.Fatal("endpoints were not blocked after 50 attempts") + } +} From 8a038c6667d900af9a126862dcb9faa4499be6ba Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 21 Nov 2023 04:20:25 +0000 Subject: [PATCH 2/5] fixup! feat: add magicsock opt to block direct endpoints --- wgengine/magicsock/magicsock.go | 16 +++++- wgengine/magicsock/magicsock_test.go | 86 ++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 481e9ce9aec81..8a6d0f82d5bfd 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -848,12 +848,22 @@ func (c *Conn) DiscoPublicKey() key.DiscoPublic { // not affect the UDP socket or portmapper. func (c *Conn) SetBlockEndpoints(block bool) { c.mu.Lock() + defer c.mu.Unlock() didChange := c.blockEndpoints != block c.blockEndpoints = block - c.mu.Unlock() + if !didChange { + return + } - if didChange { - go c.updateEndpoints("SetBlockEndpoints") + const why = "SetBlockEndpoints" + if c.endpointsUpdateActive { + if c.wantEndpointsUpdate != why { + c.dlogf("[v1] magicsock: SetBlockEndpoints: endpoint update active, need another later") + c.wantEndpointsUpdate = why + } + } else { + c.endpointsUpdateActive = true + go c.updateEndpoints(why) } } diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 4881a32ce951c..f5c07c2d7cc56 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -3033,17 +3033,95 @@ func TestBlockEndpoints(t *testing.T) { ms.conn.SetBlockEndpoints(true) // Wait for endpoints to finish updating. + waitForNoEndpoints(t, ms.conn) +} + +func TestBlockEndpointsDERPOK(t *testing.T) { + // This test is similar to TestBlockEndpoints, but it tests that we don't + // mess up DERP somehow. + + mstun := &natlab.Machine{Name: "stun"} + m1 := &natlab.Machine{Name: "m1"} + m2 := &natlab.Machine{Name: "m2"} + inet := natlab.NewInternet() + sif := mstun.Attach("eth0", inet) + m1if := m1.Attach("eth0", inet) + m2if := m2.Attach("eth0", inet) + + d := &devices{ + m1: m1, + m1IP: m1if.V4(), + m2: m2, + m2IP: m2if.V4(), + stun: mstun, + stunIP: sif.V4(), + } + + logf, closeLogf := logger.LogfCloser(t.Logf) + defer closeLogf() + + derpMap, cleanup := runDERPAndStun(t, t.Logf, localhostListener{}, netaddr.IPv4(127, 0, 0, 1)) + defer cleanup() + + ms1 := newMagicStack(t, logger.WithPrefix(logf, "conn1: "), d.m1, derpMap) + defer ms1.Close() + ms2 := newMagicStack(t, logger.WithPrefix(logf, "conn2: "), d.m2, derpMap) + defer ms2.Close() + + cleanup = meshStacks(logf, nil, ms1, ms2) + defer cleanup() + + m1IP := ms1.IP() + m2IP := ms2.IP() + logf("IPs: %s %s", m1IP, m2IP) + + // SetBlockEndpoints is called later since it's incompatible with the test + // meshStacks implementations. + ms1.conn.SetBlockEndpoints(true) + ms2.conn.SetBlockEndpoints(true) + waitForNoEndpoints(t, ms1.conn) + waitForNoEndpoints(t, ms2.conn) + + cleanup = newPinger(t, logf, ms1, ms2) + defer cleanup() + + // Wait for both peers to know about each other. + for { + if s1 := ms1.Status(); len(s1.Peer) != 1 { + time.Sleep(10 * time.Millisecond) + continue + } + if s2 := ms2.Status(); len(s2.Peer) != 1 { + time.Sleep(10 * time.Millisecond) + continue + } + break + } + + cleanup = newPinger(t, t.Logf, ms1, ms2) + defer cleanup() + + if len(ms1.conn.activeDerp) == 0 { + t.Errorf("unexpected DERP empty got: %v want: >0", len(ms1.conn.activeDerp)) + } + if len(ms2.conn.activeDerp) == 0 { + t.Errorf("unexpected DERP empty got: %v want: >0", len(ms2.conn.activeDerp)) + } +} + +func waitForNoEndpoints(t *testing.T, ms *Conn) { + t.Helper() ok := false parentLoop: for i := 0; i < 50; i++ { time.Sleep(100 * time.Millisecond) - ms.conn.mu.Lock() - for _, ep := range ms.conn.lastEndpoints { + ms.mu.Lock() + for _, ep := range ms.lastEndpoints { t.Errorf("endpoint %v was not blocked", ep.Addr) - ms.conn.mu.Unlock() + ms.mu.Unlock() continue parentLoop } - ms.conn.mu.Unlock() + ms.mu.Unlock() ok = true break } From d8aa41d89edbd74da9fddc1462b3e8cd95686a74 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 21 Nov 2023 04:31:23 +0000 Subject: [PATCH 3/5] fixup! feat: add magicsock opt to block direct endpoints --- wgengine/magicsock/magicsock_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index f5c07c2d7cc56..444f5604933e9 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -3013,7 +3013,7 @@ func TestBlockEndpoints(t *testing.T) { defer ms.Close() // Check that some endpoints exist. This should be the case as we should use - // interface addressess as endpoints instantly on startup, and we already + // interface addresses as endpoints instantly on startup, and we already // have a DERP connection due to newMagicStackFunc. ms.conn.mu.Lock() haveEndpoint := false From 614a18f7835b3ab0781c666fe8ff8bb0b08b0669 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 22 Nov 2023 13:15:10 +0000 Subject: [PATCH 4/5] fixup! feat: add magicsock opt to block direct endpoints --- wgengine/magicsock/magicsock.go | 1 + wgengine/magicsock/magicsock_test.go | 7 +++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 8a6d0f82d5bfd..06dc19afec897 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -431,6 +431,7 @@ func NewConn(opts Options) (*Conn, error) { c.logf = opts.logf() c.epFunc = opts.endpointsFunc() c.derpActiveFunc = opts.derpActiveFunc() + c.blockEndpoints = opts.BlockEndpoints c.idleFunc = opts.IdleFunc c.testOnlyPacketListener = opts.TestOnlyPacketListener c.noteRecvActivity = opts.NoteRecvActivity diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 444f5604933e9..051ead8d0acc8 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -3112,14 +3112,13 @@ func TestBlockEndpointsDERPOK(t *testing.T) { func waitForNoEndpoints(t *testing.T, ms *Conn) { t.Helper() ok := false -parentLoop: for i := 0; i < 50; i++ { time.Sleep(100 * time.Millisecond) ms.mu.Lock() - for _, ep := range ms.lastEndpoints { - t.Errorf("endpoint %v was not blocked", ep.Addr) + if len(ms.lastEndpoints) != 0 { + t.Errorf("some endpoints were not blocked: %v", ms.lastEndpoints) ms.mu.Unlock() - continue parentLoop + continue } ms.mu.Unlock() ok = true From 2cdd5b8cc7cc4c3c3b2d8d7b63ad4b5b59c6a014 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 12 Dec 2023 13:10:02 +0000 Subject: [PATCH 5/5] Avoid constant rebinding on blocked endpoints --- wgengine/magicsock/magicsock.go | 6 +++--- wgengine/magicsock/magicsock_test.go | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 06dc19afec897..b0cf73232fbe6 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -548,7 +548,7 @@ func (c *Conn) updateEndpoints(why string) { c.muCond.Broadcast() }() c.dlogf("[v1] magicsock: starting endpoint update (%s)", why) - if c.noV4Send.Load() && c.derpMapHasSTUNNodes() && runtime.GOOS != "js" { + if c.noV4Send.Load() && c.shouldRebindOnFailedNetcheckV4Send() && runtime.GOOS != "js" { c.mu.Lock() closed := c.closed c.mu.Unlock() @@ -573,10 +573,10 @@ func (c *Conn) updateEndpoints(why string) { } // c.mu must NOT be held. -func (c *Conn) derpMapHasSTUNNodes() bool { +func (c *Conn) shouldRebindOnFailedNetcheckV4Send() bool { c.mu.Lock() defer c.mu.Unlock() - return c.derpMap != nil && c.derpMap.HasSTUN() + return c.derpMap != nil && c.derpMap.HasSTUN() && !c.blockEndpoints } // setEndpoints records the new endpoints, reporting whether they're changed. diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 051ead8d0acc8..d9eb2cd12d206 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -3127,4 +3127,5 @@ func waitForNoEndpoints(t *testing.T, ms *Conn) { if !ok { t.Fatal("endpoints were not blocked after 50 attempts") } + t.Log("endpoints are blocked") }