From ff4c2dbec96f9c1bec7114f7d43c525ef5ef52bb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 6 May 2021 15:27:02 -0700 Subject: [PATCH] cmd/tailscale: fix another up warning with exit nodes The --advertise-routes and --advertise-exit-node flags both mutating one pref is the gift that keeps on giving. I need to rewrite the this up warning code to first map prefs back to flag values and then just compare flags instead of comparing prefs, but this is the minimal fix for now. This also includes work on the tests, to make them easier to write (and more accurate), by letting you write the flag args directly and have that parse into the upArgs/MaskedPrefs directly, the same as the code, rather than them being possibly out of sync being written by hand. Fixes https://twitter.com/EXPbits/status/1390418145047887877 Signed-off-by: Brad Fitzpatrick (cherry picked from commit e78e26b6fbae898104d0ac4f176d424211b36fd3) --- cmd/tailscale/cli/cli_test.go | 246 +++++++++++++++------------------- cmd/tailscale/cli/up.go | 25 ++-- 2 files changed, 124 insertions(+), 147 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 589d07da1..ed42013c9 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -38,48 +38,42 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { return m } tests := []struct { - name string - flagSet map[string]bool + name string + + // flags, if non-nil populates flagSet and mp. + flags []string // if non-nil, + + // flagSet and mp are required if flags is nil. + // flagSet is the set of flags that were explicitly set in flags. + // mp is the mutation to apply to server. + flagSet map[string]bool + mp *ipn.MaskedPrefs + curPrefs *ipn.Prefs curUser string // os.Getenv("USER") on the client side goos string // empty means "linux" - mp *ipn.MaskedPrefs want string }{ { - name: "bare_up_means_up", - flagSet: f(), + name: "bare_up_means_up", + flags: []string{}, curPrefs: &ipn.Prefs{ ControlURL: ipn.DefaultControlURL, WantRunning: false, Hostname: "foo", }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - WantRunning: true, - }, - WantRunningSet: true, - }, want: "", }, { - name: "losing_hostname", - flagSet: f("accept-dns"), + name: "losing_hostname", + flags: []string{"--accept-dns"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: false, - Hostname: "foo", - CorpDNS: true, - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: true, - CorpDNS: true, - }, - ControlURLSet: true, - WantRunningSet: true, - CorpDNSSet: true, + ControlURL: ipn.DefaultControlURL, + WantRunning: false, + Hostname: "foo", + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + AllowSingleHosts: true, }, want: accidentalUpPrefix + " --accept-dns --hostname=foo", }, @@ -184,47 +178,35 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: "", }, { - name: "error_advertised_routes_exit_node_removed", - flagSet: f("advertise-routes"), + name: "error_advertised_routes_exit_node_removed", + flags: []string{"--advertise-routes=10.0.42.0/24"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("10.0.42.0/24"), netaddr.MustParseIPPrefix("0.0.0.0/0"), netaddr.MustParseIPPrefix("::/0"), }, }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("10.0.42.0/24"), - }, - }, - AdvertiseRoutesSet: true, - }, want: accidentalUpPrefix + " --advertise-routes=10.0.42.0/24 --advertise-exit-node", }, { - name: "advertised_routes_exit_node_removed", - flagSet: f("advertise-routes", "advertise-exit-node"), + name: "advertised_routes_exit_node_removed", + flags: []string{"--advertise-routes=10.0.42.0/24", "--advertise-exit-node=false"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("10.0.42.0/24"), netaddr.MustParseIPPrefix("0.0.0.0/0"), netaddr.MustParseIPPrefix("::/0"), }, }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("10.0.42.0/24"), - }, - }, - AdvertiseRoutesSet: true, - }, want: "", }, { @@ -252,91 +234,44 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: "", }, { - name: "advertised_routes_includes_only_one_0_route", // and no --advertise-exit-node - flagSet: f("advertise-routes"), + name: "advertise_exit_node", // Issue 1859 + flags: []string{"--advertise-exit-node"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("10.0.42.0/24"), - netaddr.MustParseIPPrefix("0.0.0.0/0"), - netaddr.MustParseIPPrefix("::/0"), - }, - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("11.1.43.0/24"), - netaddr.MustParseIPPrefix("0.0.0.0/0"), - }, - }, - AdvertiseRoutesSet: true, - }, - want: accidentalUpPrefix + " --advertise-routes=11.1.43.0/24,0.0.0.0/0 --advertise-exit-node", - }, - { - name: "advertise_exit_node", // Issue 1859 - flagSet: f("advertise-exit-node"), - curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("0.0.0.0/0"), - netaddr.MustParseIPPrefix("::/0"), - }, - }, - // Note: without setting "AdvertiseRoutesSet", as - // updateMaskedPrefsFromUpFlag doesn't set that. + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, }, want: "", }, { - name: "advertise_exit_node_over_existing_routes", - flagSet: f("advertise-exit-node"), + name: "advertise_exit_node_over_existing_routes", + flags: []string{"--advertise-exit-node"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("1.2.0.0/16"), }, }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("0.0.0.0/0"), - netaddr.MustParseIPPrefix("::/0"), - }, - }, - // Note: without setting "AdvertiseRoutesSet", as - // updateMaskedPrefsFromUpFlag doesn't set that. - }, want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16", }, { - name: "advertise_exit_node_over_existing_routes_and_exit_node", - flagSet: f("advertise-exit-node"), + name: "advertise_exit_node_over_existing_routes_and_exit_node", + flags: []string{"--advertise-exit-node"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("0.0.0.0/0"), netaddr.MustParseIPPrefix("::/0"), netaddr.MustParseIPPrefix("1.2.0.0/16"), }, }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("0.0.0.0/0"), - netaddr.MustParseIPPrefix("::/0"), - }, - }, - // Note: without setting "AdvertiseRoutesSet", as - // updateMaskedPrefsFromUpFlag doesn't set that. - }, want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16", }, { @@ -356,15 +291,15 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: "", }, { - name: "remove_all_implicit", - flagSet: f("force-reauth"), + name: "remove_all_implicit", + flags: []string{"--force-reauth"}, curPrefs: &ipn.Prefs{ WantRunning: true, ControlURL: ipn.DefaultControlURL, RouteAll: true, AllowSingleHosts: false, ExitNodeIP: netaddr.MustParseIP("100.64.5.6"), - CorpDNS: true, + CorpDNS: false, ShieldsUp: true, AdvertiseTags: []string{"tag:foo", "tag:bar"}, Hostname: "myhostname", @@ -376,24 +311,18 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { OperatorUser: "alice", }, curUser: "eve", - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: true, - }, - }, - want: accidentalUpPrefix + " --force-reauth --accept-routes --exit-node=100.64.5.6 --accept-dns --shields-up --advertise-tags=tag:foo,tag:bar --hostname=myhostname --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice", + want: accidentalUpPrefix + " --force-reauth --accept-routes --host-routes=false --exit-node=100.64.5.6 --accept-dns=false --shields-up --advertise-tags=tag:foo,tag:bar --hostname=myhostname --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice", }, { - name: "remove_all_implicit_except_hostname", - flagSet: f("hostname"), + name: "remove_all_implicit_except_hostname", + flags: []string{"--hostname=newhostname"}, curPrefs: &ipn.Prefs{ WantRunning: true, ControlURL: ipn.DefaultControlURL, RouteAll: true, AllowSingleHosts: false, ExitNodeIP: netaddr.MustParseIP("100.64.5.6"), - CorpDNS: true, + CorpDNS: false, ShieldsUp: true, AdvertiseTags: []string{"tag:foo", "tag:bar"}, Hostname: "myhostname", @@ -405,15 +334,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { OperatorUser: "alice", }, curUser: "eve", - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: true, - Hostname: "newhostname", - }, - HostnameSet: true, - }, - 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", + want: accidentalUpPrefix + " --hostname=newhostname --accept-routes --host-routes=false --exit-node=100.64.5.6 --accept-dns=false --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", @@ -472,6 +393,38 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { }, want: "", // not an error }, + { + name: "operator_losing_routes_step1", // https://twitter.com/EXPbits/status/1390418145047887877 + flags: []string{"--operator=expbits"}, + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + AdvertiseRoutes: []netaddr.IPPrefix{ + netaddr.MustParseIPPrefix("0.0.0.0/0"), + netaddr.MustParseIPPrefix("::/0"), + netaddr.MustParseIPPrefix("1.2.0.0/16"), + }, + }, + want: accidentalUpPrefix + " --operator=expbits --advertise-routes=1.2.0.0/16 --advertise-exit-node", + }, + { + name: "operator_losing_routes_step2", // https://twitter.com/EXPbits/status/1390418145047887877 + flags: []string{"--operator=expbits", "--advertise-routes=1.2.0.0/16"}, + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + AdvertiseRoutes: []netaddr.IPPrefix{ + netaddr.MustParseIPPrefix("0.0.0.0/0"), + netaddr.MustParseIPPrefix("::/0"), + netaddr.MustParseIPPrefix("1.2.0.0/16"), + }, + }, + want: accidentalUpPrefix + " --advertise-routes=1.2.0.0/16 --operator=expbits --advertise-exit-node", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -479,8 +432,23 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { if tt.goos != "" { goos = tt.goos } + flagSet := tt.flagSet + mp := tt.mp + if tt.flags != nil { + if tt.flagSet != nil || tt.mp != nil { + t.Fatal("flags and flagSet/mp are mutually exclusive") + } + var upArgs upArgsT + fs := newUpFlagSet(goos, &upArgs) + fs.Parse(tt.flags) + prefs, err := prefsFromUpArgs(upArgs, t.Logf, new(ipnstate.Status), goos) + if err != nil { + t.Fatal(err) + } + flagSet, mp = flagSetAndMaskedPrefs(prefs, fs) + } var got string - if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp, goos, tt.curUser); err != nil { + if err := checkForAccidentalSettingReverts(flagSet, tt.curPrefs, mp, goos, tt.curUser); err != nil { got = err.Error() } if strings.TrimSpace(got) != tt.want { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 56bc8f0ec..cca6509de 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -295,14 +295,7 @@ func runUp(ctx context.Context, args []string) error { return err } - flagSet := map[string]bool{} - mp := new(ipn.MaskedPrefs) - mp.WantRunningSet = true - mp.Prefs = *prefs - upFlagSet.Visit(func(f *flag.Flag) { - updateMaskedPrefsFromUpFlag(mp, f.Name) - flagSet[f.Name] = true - }) + flagSet, mp := flagSetAndMaskedPrefs(prefs, upFlagSet) if !upArgs.reset { if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp, runtime.GOOS, os.Getenv("USER")); err != nil { @@ -498,6 +491,18 @@ func addPrefFlagMapping(flagName string, prefNames ...string) { } } +func flagSetAndMaskedPrefs(prefs *ipn.Prefs, fs *flag.FlagSet) (flagSetMap map[string]bool, mp *ipn.MaskedPrefs) { + flagSetMap = map[string]bool{} + mp = new(ipn.MaskedPrefs) + mp.WantRunningSet = true + mp.Prefs = *prefs + fs.Visit(func(f *flag.Flag) { + updateMaskedPrefsFromUpFlag(mp, f.Name) + flagSetMap[f.Name] = true + }) + return flagSetMap, mp +} + func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) { if prefs, ok := prefsOfFlag[flagName]; ok { for _, pref := range prefs { @@ -635,8 +640,12 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi))) } case "advertise-routes": + hadExitNode := hasExitNodeRoutes(exi.([]netaddr.IPPrefix)) routes := withoutExitNodes(exi.([]netaddr.IPPrefix)) missing = append(missing, fmtFlagValueArg("advertise-routes", fmtSettingVal(routes))) + if hadExitNode && !flagSet["advertise-exit-node"] { + missing = append(missing, "--advertise-exit-node") + } default: missing = append(missing, fmtFlagValueArg(flagName, fmtSettingVal(exi))) }