From 4ee64681ade88b43c66b0858d5e6c4a7d8fe4e2d Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 9 Aug 2022 10:16:34 -0700 Subject: [PATCH] tailcfg, control/controlclient: make Debug settings sticky in a map session [capver 37] Fixes #4843 Change-Id: I3accfd91be474ac745cb47f5d6e866c37d5c5d2d Signed-off-by: Brad Fitzpatrick --- control/controlclient/direct.go | 33 +++---- control/controlclient/map.go | 43 ++++++++- control/controlclient/map_test.go | 151 ++++++++++++++++++++++++++++++ ipn/ipnlocal/local.go | 5 +- tailcfg/tailcfg.go | 18 ++++ 5 files changed, 228 insertions(+), 22 deletions(-) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 2966ac227..1c9892a35 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -710,7 +710,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool c.logf("[v1] PollNetMap: stream=%v ep=%v", allowStream, epStrs) vlogf := logger.Discard - if Debug.NetMap { + if DevKnob.DumpNetMaps { // TODO(bradfitz): update this to use "[v2]" prefix perhaps? but we don't // want to upload it always. vlogf = c.logf @@ -939,8 +939,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool if resp.Debug.GoroutineDumpURL != "" { go dumpGoroutinesToURL(c.httpc, resp.Debug.GoroutineDumpURL) } - controlUseDERPRoute.Store(resp.Debug.DERPRoute) - controlTrimWGConfig.Store(resp.Debug.TrimWGConfig) if sleep := time.Duration(resp.Debug.SleepSeconds * float64(time.Second)); sleep > 0 { if err := sleepAsRequested(ctx, c.logf, timeoutReset, sleep); err != nil { return err @@ -954,12 +952,17 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool return errors.New("MapResponse lacked node") } - if Debug.StripEndpoints { + if d := nm.Debug; d != nil { + controlUseDERPRoute.Store(d.DERPRoute) + controlTrimWGConfig.Store(d.TrimWGConfig) + } + + if DevKnob.StripEndpoints { for _, p := range resp.Peers { p.Endpoints = nil } } - if Debug.StripCaps { + if DevKnob.StripCaps { nm.SelfNode.Capabilities = nil } @@ -1125,25 +1128,23 @@ func loadServerPubKeys(ctx context.Context, httpc *http.Client, serverURL string return &out, nil } -// Debug contains temporary internal-only debug knobs. +// DevKnob contains temporary internal-only debug knobs. // They're unexported to not draw attention to them. -var Debug = initDebug() +var DevKnob = initDevKnob() -type debug struct { - NetMap bool - ProxyDNS bool - Disco bool +type devKnobs struct { + DumpNetMaps bool + ForceProxyDNS bool StripEndpoints bool // strip endpoints from control (only use disco messages) StripCaps bool // strip all local node's control-provided capabilities } -func initDebug() debug { - return debug{ - NetMap: envknob.Bool("TS_DEBUG_NETMAP"), - ProxyDNS: envknob.Bool("TS_DEBUG_PROXY_DNS"), +func initDevKnob() devKnobs { + return devKnobs{ + DumpNetMaps: envknob.Bool("TS_DEBUG_NETMAP"), + ForceProxyDNS: envknob.Bool("TS_DEBUG_PROXY_DNS"), StripEndpoints: envknob.Bool("TS_DEBUG_STRIP_ENDPOINTS"), StripCaps: envknob.Bool("TS_DEBUG_STRIP_CAPS"), - Disco: envknob.BoolDefaultTrue("TS_DEBUG_USE_DISCO"), } } diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 8f154728d..15a3840ba 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -15,6 +15,7 @@ "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/netmap" + "tailscale.com/types/opt" "tailscale.com/wgengine/filter" ) @@ -46,6 +47,7 @@ type mapSession struct { lastDomain string lastHealth []string lastPopBrowserURL string + stickyDebug tailcfg.Debug // accumulated opt.Bool values // netMapBuilding is non-nil during a netmapForResponse call, // containing the value to be returned, once fully populated. @@ -114,6 +116,28 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo ms.lastHealth = resp.Health } + debug := resp.Debug + if debug != nil { + if debug.RandomizeClientPort { + debug.SetRandomizeClientPort.Set(true) + } + if debug.ForceBackgroundSTUN { + debug.SetForceBackgroundSTUN.Set(true) + } + copyDebugOptBools(&ms.stickyDebug, debug) + } else if ms.stickyDebug != (tailcfg.Debug{}) { + debug = new(tailcfg.Debug) + } + if debug != nil { + copyDebugOptBools(debug, &ms.stickyDebug) + if !debug.ForceBackgroundSTUN { + debug.ForceBackgroundSTUN, _ = ms.stickyDebug.SetForceBackgroundSTUN.Get() + } + if !debug.RandomizeClientPort { + debug.RandomizeClientPort, _ = ms.stickyDebug.SetRandomizeClientPort.Get() + } + } + nm := &netmap.NetworkMap{ NodeKey: ms.privateNodeKey.Public(), PrivateKey: ms.privateNodeKey, @@ -126,7 +150,7 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo SSHPolicy: ms.lastSSHPolicy, CollectServices: ms.collectServices, DERPMap: ms.lastDERPMap, - Debug: resp.Debug, + Debug: debug, ControlHealth: ms.lastHealth, } ms.netMapBuilding = nm @@ -166,7 +190,7 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo } ms.addUserProfile(peer.User) } - if Debug.ProxyDNS { + if DevKnob.ForceProxyDNS { nm.DNS.Proxied = true } ms.netMapBuilding = nil @@ -344,3 +368,18 @@ func filterSelfAddresses(in []netip.Prefix) (ret []netip.Prefix) { return ret } } + +func copyDebugOptBools(dst, src *tailcfg.Debug) { + copy := func(v *opt.Bool, s opt.Bool) { + if s != "" { + *v = s + } + } + copy(&dst.DERPRoute, src.DERPRoute) + copy(&dst.DisableSubnetsIfPAC, src.DisableSubnetsIfPAC) + copy(&dst.DisableUPnP, src.DisableUPnP) + copy(&dst.OneCGNATRoute, src.OneCGNATRoute) + copy(&dst.SetForceBackgroundSTUN, src.SetForceBackgroundSTUN) + copy(&dst.SetRandomizeClientPort, src.SetRandomizeClientPort) + copy(&dst.TrimWGConfig, src.TrimWGConfig) +} diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index 3f94dd132..51c7d02b1 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -16,6 +16,8 @@ "tailscale.com/tailcfg" "tailscale.com/types/key" "tailscale.com/types/netmap" + "tailscale.com/types/opt" + "tailscale.com/util/must" ) func TestUndeltaPeers(t *testing.T) { @@ -449,3 +451,152 @@ func TestNetmapForResponse(t *testing.T) { } }) } + +// TestDeltaDebug tests that tailcfg.Debug values can be omitted in MapResposnes +// entirely or have their opt.Bool values unspecified between MapResponses in a +// session and that should mean no change. (as of capver 37). But two Debug +// fields existed prior to capver 37 that weren't opt.Bool; we test that we both +// still accept the non-opt.Bool form from control for RandomizeClientPort and +// ForceBackgroundSTUN and also accept the new form, keeping the old form in +// sync. +func TestDeltaDebug(t *testing.T) { + type step struct { + got *tailcfg.Debug + want *tailcfg.Debug + } + tests := []struct { + name string + steps []step + }{ + { + name: "nothing-to-nothing", + steps: []step{ + {nil, nil}, + {nil, nil}, + }, + }, + { + name: "sticky-with-old-style-randomize-client-port", + steps: []step{ + { + &tailcfg.Debug{RandomizeClientPort: true}, + &tailcfg.Debug{ + RandomizeClientPort: true, + SetRandomizeClientPort: "true", + }, + }, + { + nil, // not sent by server + &tailcfg.Debug{ + RandomizeClientPort: true, + SetRandomizeClientPort: "true", + }, + }, + }, + }, + { + name: "sticky-with-new-style-randomize-client-port", + steps: []step{ + { + &tailcfg.Debug{SetRandomizeClientPort: "true"}, + &tailcfg.Debug{ + RandomizeClientPort: true, + SetRandomizeClientPort: "true", + }, + }, + { + nil, // not sent by server + &tailcfg.Debug{ + RandomizeClientPort: true, + SetRandomizeClientPort: "true", + }, + }, + }, + }, + { + name: "opt-bool-sticky-changing-over-time", + steps: []step{ + {nil, nil}, + {nil, nil}, + { + &tailcfg.Debug{OneCGNATRoute: "true"}, + &tailcfg.Debug{OneCGNATRoute: "true"}, + }, + { + nil, + &tailcfg.Debug{OneCGNATRoute: "true"}, + }, + { + &tailcfg.Debug{OneCGNATRoute: "false"}, + &tailcfg.Debug{OneCGNATRoute: "false"}, + }, + { + nil, + &tailcfg.Debug{OneCGNATRoute: "false"}, + }, + }, + }, + { + name: "legacy-ForceBackgroundSTUN", + steps: []step{ + { + &tailcfg.Debug{ForceBackgroundSTUN: true}, + &tailcfg.Debug{ForceBackgroundSTUN: true, SetForceBackgroundSTUN: "true"}, + }, + }, + }, + { + name: "opt-bool-SetForceBackgroundSTUN", + steps: []step{ + { + &tailcfg.Debug{SetForceBackgroundSTUN: "true"}, + &tailcfg.Debug{ForceBackgroundSTUN: true, SetForceBackgroundSTUN: "true"}, + }, + }, + }, + { + name: "server-reset-to-default", + steps: []step{ + { + &tailcfg.Debug{SetForceBackgroundSTUN: "true"}, + &tailcfg.Debug{ForceBackgroundSTUN: true, SetForceBackgroundSTUN: "true"}, + }, + { + &tailcfg.Debug{SetForceBackgroundSTUN: "unset"}, + &tailcfg.Debug{ForceBackgroundSTUN: false, SetForceBackgroundSTUN: "unset"}, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ms := newTestMapSession(t) + for stepi, s := range tt.steps { + nm := ms.netmapForResponse(&tailcfg.MapResponse{Debug: s.got}) + if !reflect.DeepEqual(nm.Debug, s.want) { + t.Errorf("unexpected result at step index %v; got: %s", stepi, must.Get(json.Marshal(nm.Debug))) + } + } + }) + } +} + +// Verifies that copyDebugOptBools doesn't missing any opt.Bools. +func TestCopyDebugOptBools(t *testing.T) { + rt := reflect.TypeOf(tailcfg.Debug{}) + for i := 0; i < rt.NumField(); i++ { + sf := rt.Field(i) + if sf.Type != reflect.TypeOf(opt.Bool("")) { + continue + } + var src, dst tailcfg.Debug + reflect.ValueOf(&src).Elem().Field(i).Set(reflect.ValueOf(opt.Bool("true"))) + if src == (tailcfg.Debug{}) { + t.Fatalf("failed to set field %v", sf.Name) + } + copyDebugOptBools(&dst, &src) + if src != dst { + t.Fatalf("copyDebugOptBools didn't copy field %v", sf.Name) + } + } +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 53aebc0f9..1b5ab2cae 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1039,10 +1039,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { }) } - var discoPublic key.DiscoPublic - if controlclient.Debug.Disco { - discoPublic = b.e.DiscoPublicKey() - } + discoPublic := b.e.DiscoPublicKey() var err error if persistv == nil { diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 12107a14e..a3f401a4d 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -71,6 +71,7 @@ // 33: 2022-07-20: added MapResponse.PeersChangedPatch (DERPRegion + Endpoints) // 34: 2022-08-02: client understands CapabilityFileSharingTarget // 36: 2022-08-02: added PeersChangedPatch.{Key,DiscoKey,Online,LastSeen,KeyExpiry,Capabilities} +// 37: 2022-08-09: added Debug.{SetForceBackgroundSTUN,SetRandomizeClientPort}; Debug are sticky const CurrentCapabilityVersion CapabilityVersion = 36 type StableID string @@ -1335,6 +1336,15 @@ type Debug struct { // periodicReSTUN), regardless of inactivity. ForceBackgroundSTUN bool `json:",omitempty"` + // SetForceBackgroundSTUN controls whether magicsock should always do its + // background STUN queries (see magicsock's periodicReSTUN), regardless of + // inactivity. + // + // As of capver 37, this field is the preferred field for control to set on + // the wire and ForceBackgroundSTUN is only used within the code as the + // current map session value. But ForceBackgroundSTUN can still be used too. + SetForceBackgroundSTUN opt.Bool `json:",omitempty"` + // DERPRoute controls whether the DERP reverse path // optimization (see Issue 150) should be enabled or // disabled. The environment variable in magicsock is the @@ -1365,6 +1375,14 @@ type Debug struct { // fixed port. RandomizeClientPort bool `json:",omitempty"` + // SetRandomizeClientPort is whether magicsock should UDP bind to :0 to get + // a random local port, ignoring any configured fixed port. + // + // As of capver 37, this field is the preferred field for control to set on + // the wire and RandomizeClientPort is only used within the code as the + // current map session value. But RandomizeClientPort can still be used too. + SetRandomizeClientPort opt.Bool `json:",omitempty"` + // OneCGNATRoute controls whether the client should prefer to make one // big CGNAT /10 route rather than a /32 per peer. OneCGNATRoute opt.Bool `json:",omitempty"`