From a52909b4d9615c1aeb48d8313eb630c47d64e3e3 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 2 May 2021 15:35:55 -0700 Subject: [PATCH] cmd/tailscale: make pref-revert checks ignore OS-irrelevant prefs This fixes #1833 in two ways: * stop setting NoSNAT on non-Linux. It only matters on Linux and the flag is hidden on non-Linux, but the code was still setting it. Because of that, the new pref-reverting safety checks were failing when it was changing. * Ignore the two Linux-only prefs changing on non-Linux. Fixes #1833 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 65 +++++++++++++++++++++++++++++++++-- cmd/tailscale/cli/up.go | 26 ++++++++++---- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index f177bd01c..069149571 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -41,6 +41,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { flagSet map[string]bool curPrefs *ipn.Prefs curUser string // os.Getenv("USER") on the client side + goos string // empty means "linux" mp *ipn.MaskedPrefs want string }{ @@ -348,11 +349,72 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { }, want: accidentalUpPrefix + " --hostname=newhostname --accept-routes --exit-node=100.64.5.6 --accept-dns --shields-up --advertise-tags=tag:foo,tag:bar --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice", }, + { + name: "loggedout_is_implicit", + flagSet: f("advertise-exit-node"), + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + LoggedOut: true, + }, + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + AdvertiseRoutes: []netaddr.IPPrefix{ + netaddr.MustParseIPPrefix("0.0.0.0/0"), + }, + }, + AdvertiseRoutesSet: true, + }, + // not an error. LoggedOut is implicit. + want: "", + }, + { + // Test that a pre-1.8 version of Tailscale with bogus NoSNAT pref + // values is able to enable exit nodes without warnings. + name: "make_windows_exit_node", + flagSet: f("advertise-exit-node"), + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + NoSNAT: true, // assume this no-op accidental pre-1.8 value + }, + goos: "windows", + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + AdvertiseRoutes: []netaddr.IPPrefix{ + netaddr.MustParseIPPrefix("192.168.0.0/16"), + }, + }, + AdvertiseRoutesSet: true, + }, + want: "", // not an error + }, + { + name: "ignore_netfilter_change_non_linux", + flagSet: f("accept-dns"), + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + NetfilterMode: preftype.NetfilterNoDivert, // we never had this bug, but pretend it got set non-zero on Windows somehow + }, + goos: "windows", + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + CorpDNS: false, + }, + CorpDNSSet: true, + }, + want: "", // not an error + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + goos := "linux" + if tt.goos != "" { + goos = tt.goos + } var got string - if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp, tt.curUser); err != nil { + if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp, goos, tt.curUser); err != nil { got = err.Error() } if strings.TrimSpace(got) != tt.want { @@ -411,7 +473,6 @@ func TestPrefsFromUpArgs(t *testing.T) { CorpDNS: true, AllowSingleHosts: true, NetfilterMode: preftype.NetfilterOn, - NoSNAT: true, }, }, { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 55f4999c0..50cc941fb 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -216,12 +216,13 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo prefs.ShieldsUp = upArgs.shieldsUp prefs.AdvertiseRoutes = routes prefs.AdvertiseTags = tags - prefs.NoSNAT = !upArgs.snat prefs.Hostname = upArgs.hostname prefs.ForceDaemon = upArgs.forceDaemon prefs.OperatorUser = upArgs.opUser if goos == "linux" { + prefs.NoSNAT = !upArgs.snat + switch upArgs.netfilterMode { case "on": prefs.NetfilterMode = preftype.NetfilterOn @@ -304,7 +305,7 @@ func runUp(ctx context.Context, args []string) error { }) if !upArgs.reset { - if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp, os.Getenv("USER")); err != nil { + if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp, runtime.GOOS, os.Getenv("USER")); err != nil { fatalf("%s", err) } } @@ -530,7 +531,7 @@ func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) { // // mp is the mask of settings actually set, where mp.Prefs is the new // preferences to set, including any values set from implicit flags. -func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Prefs, mp *ipn.MaskedPrefs, curUser string) error { +func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Prefs, mp *ipn.MaskedPrefs, goos, curUser string) error { if len(flagSet) == 0 { // A bare "tailscale up" is a special case to just // mean bringing the network up without any changes. @@ -588,10 +589,21 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre if reflect.DeepEqual(exi, imi) { continue } - if flagName == "operator" && imi == "" && exi == curUser { - // Don't require setting operator if the current user matches - // the configured operator. - continue + switch flagName { + case "operator": + if imi == "" && exi == curUser { + // Don't require setting operator if the current user matches + // the configured operator. + continue + } + case "snat-subnet-routes", "netfilter-mode": + if goos != "linux" { + // Issue 1833: we used to accidentally set the NoSNAT + // pref for non-Linux nodes. It only affects Linux, so + // ignore it if it changes. Likewise, ignore + // Linux-only netfilter-mode on non-Linux. + continue + } } switch flagName { case "":