From 4ce15505cb9d07e5cdcd2230c43908032af8b792 Mon Sep 17 00:00:00 2001 From: David Crawshaw Date: Tue, 22 Jun 2021 13:13:59 -0700 Subject: [PATCH] wgengine: randomize client port if netmap says to For testing out #2187 Signed-off-by: David Crawshaw --- ipn/ipnlocal/local.go | 6 ++-- wgengine/bench/wg.go | 4 +-- wgengine/userspace.go | 24 +++++++++----- wgengine/userspace_test.go | 67 +++++++++++++++++++++++++++++++++++++- wgengine/watchdog.go | 4 +-- wgengine/wgengine.go | 4 ++- 6 files changed, 92 insertions(+), 17 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 92507b022..f669a336e 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1838,7 +1838,7 @@ func (b *LocalBackend) authReconfig() { } } - err = b.e.Reconfig(cfg, rcfg, &dcfg) + err = b.e.Reconfig(cfg, rcfg, &dcfg, nm.Debug) if err == wgengine.ErrNoChanges { return } @@ -2199,7 +2199,7 @@ func (b *LocalBackend) enterState(newState ipn.State) { b.blockEngineUpdates(true) fallthrough case ipn.Stopped: - err := b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{}) + err := b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{}, nil) if err != nil { b.logf("Reconfig(down): %v", err) } @@ -2319,7 +2319,7 @@ func (b *LocalBackend) stateMachine() { // a status update that predates the "I've shut down" update. func (b *LocalBackend) stopEngineAndWait() { b.logf("stopEngineAndWait...") - b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{}) + b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{}, nil) b.requestEngineStatusAndWait() b.logf("stopEngineAndWait: done.") } diff --git a/wgengine/bench/wg.go b/wgengine/bench/wg.go index ff9ea64c3..c23ccc4dd 100644 --- a/wgengine/bench/wg.go +++ b/wgengine/bench/wg.go @@ -127,7 +127,7 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd Endpoints: endpoint, } c2.Peers = []wgcfg.Peer{p} - e2.Reconfig(&c2, &router.Config{}, new(dns.Config)) + e2.Reconfig(&c2, &router.Config{}, new(dns.Config), nil) e1waitDoneOnce.Do(wait.Done) }) @@ -171,7 +171,7 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netadd Endpoints: endpoint, } c1.Peers = []wgcfg.Peer{p} - e1.Reconfig(&c1, &router.Config{}, new(dns.Config)) + e1.Reconfig(&c1, &router.Config{}, new(dns.Config), nil) e2waitDoneOnce.Do(wait.Done) }) diff --git a/wgengine/userspace.go b/wgengine/userspace.go index efcfc9dab..493ae4a1d 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -87,6 +87,7 @@ type userspaceEngine struct { tundev *tstun.Wrapper wgdev *device.Device router router.Router + confListenPort uint16 // original conf.ListenPort dns *dns.Manager magicConn *magicsock.Conn linkMon *monitor.Mon @@ -232,12 +233,13 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) closePool.add(tsTUNDev) e := &userspaceEngine{ - timeNow: time.Now, - logf: logf, - reqCh: make(chan struct{}, 1), - waitCh: make(chan struct{}), - tundev: tsTUNDev, - router: conf.Router, + timeNow: time.Now, + logf: logf, + reqCh: make(chan struct{}, 1), + waitCh: make(chan struct{}), + tundev: tsTUNDev, + router: conf.Router, + confListenPort: conf.ListenPort, } e.isLocalAddr.Store(tsaddr.NewContainsIPFunc(nil)) @@ -732,7 +734,7 @@ func (e *userspaceEngine) updateActivityMapsLocked(trackDisco []tailcfg.DiscoKey e.tundev.SetDestIPActivityFuncs(e.destIPActivityFuncs) } -func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config) error { +func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config, debug *tailcfg.Debug) error { if routerCfg == nil { panic("routerCfg must not be nil") } @@ -754,9 +756,14 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, } e.mu.Unlock() + listenPort := e.confListenPort + if debug != nil && debug.RandomizeClientPort { + listenPort = 0 + } + engineChanged := deephash.UpdateHash(&e.lastEngineSigFull, cfg) routerChanged := deephash.UpdateHash(&e.lastRouterSig, routerCfg, dnsCfg) - if !engineChanged && !routerChanged { + if !engineChanged && !routerChanged && listenPort == e.magicConn.LocalPort() { return ErrNoChanges } @@ -795,6 +802,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, e.logf("wgengine: Reconfig: SetPrivateKey: %v", err) } e.magicConn.UpdatePeers(peerSet) + e.magicConn.SetPreferredPort(listenPort) if err := e.maybeReconfigWireguardLocked(discoChanged); err != nil { return err diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 6954ce63a..ca14c39b4 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -109,7 +109,7 @@ func TestUserspaceEngineReconfig(t *testing.T) { }, } - err = e.Reconfig(cfg, routerCfg, &dns.Config{}) + err = e.Reconfig(cfg, routerCfg, &dns.Config{}, nil) if err != nil { t.Fatal(err) } @@ -130,6 +130,71 @@ func TestUserspaceEngineReconfig(t *testing.T) { } } +func TestUserspaceEnginePortReconfig(t *testing.T) { + const defaultPort = 49983 + // Keep making a wgengine until we find an unused port + var ue *userspaceEngine + for i := 0; i < 100; i++ { + attempt := uint16(defaultPort + i) + e, err := NewFakeUserspaceEngine(t.Logf, attempt) + if err != nil { + t.Fatal(err) + } + ue = e.(*userspaceEngine) + if ue.magicConn.LocalPort() == attempt { + break + } + ue.Close() + ue = nil + } + if ue == nil { + t.Fatal("could not create a wgengine with a specific port") + } + defer ue.Close() + + startingPort := ue.magicConn.LocalPort() + discoKey := dkFromHex("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + cfg := &wgcfg.Config{ + Peers: []wgcfg.Peer{ + { + AllowedIPs: []netaddr.IPPrefix{ + netaddr.IPPrefixFrom(netaddr.IPv4(100, 100, 99, 1), 32), + }, + Endpoints: wgcfg.Endpoints{DiscoKey: discoKey}, + }, + }, + } + routerCfg := &router.Config{} + if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}, nil); err != nil { + t.Fatal(err) + } + if got := ue.magicConn.LocalPort(); got != startingPort { + t.Errorf("no debug setting changed local port to %d from %d", got, startingPort) + } + if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}, &tailcfg.Debug{RandomizeClientPort: true}); err != nil { + t.Fatal(err) + } + if got := ue.magicConn.LocalPort(); got == startingPort { + t.Errorf("debug setting did not change local port from %d", startingPort) + } + + lastPort := ue.magicConn.LocalPort() + if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}, nil); err != nil { + t.Fatal(err) + } + if startingPort == defaultPort { + // Only try this if we managed to bind defaultPort the first time. + // Otherwise, assume someone else on the computer is using defaultPort + // and so Reconfig would have caused magicSockt to bind some other port. + if got := ue.magicConn.LocalPort(); got != defaultPort { + t.Errorf("debug setting did not change local port from %d to %d", startingPort, defaultPort) + } + } + if got := ue.magicConn.LocalPort(); got == lastPort { + t.Errorf("Reconfig did not change local port from %d", lastPort) + } +} + func dkFromHex(hex string) tailcfg.DiscoKey { if len(hex) != 64 { panic(fmt.Sprintf("%q is len %d; want 64", hex, len(hex))) diff --git a/wgengine/watchdog.go b/wgengine/watchdog.go index 8db843d7d..d040b2ae8 100644 --- a/wgengine/watchdog.go +++ b/wgengine/watchdog.go @@ -74,8 +74,8 @@ func (e *watchdogEngine) watchdog(name string, fn func()) { }) } -func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config) error { - return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, routerCfg, dnsCfg) }) +func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config, debug *tailcfg.Debug) error { + return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, routerCfg, dnsCfg, debug) }) } func (e *watchdogEngine) GetLinkMonitor() *monitor.Mon { return e.wrap.GetLinkMonitor() diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index 192126e2a..22d891215 100644 --- a/wgengine/wgengine.go +++ b/wgengine/wgengine.go @@ -56,8 +56,10 @@ type Engine interface { // This is called whenever tailcontrol (the control plane) // sends an updated network map. // + // The *tailcfg.Debug parameter can be nil. + // // The returned error is ErrNoChanges if no changes were made. - Reconfig(*wgcfg.Config, *router.Config, *dns.Config) error + Reconfig(*wgcfg.Config, *router.Config, *dns.Config, *tailcfg.Debug) error // GetFilter returns the current packet filter, if any. GetFilter() *filter.Filter