From e40e5429c23eaa69d1db6b616671531c7704fdbb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 21 Apr 2021 21:41:13 -0700 Subject: [PATCH] cmd/tailscale/cli: make 'tailscale up' protect --advertise-exit-node removal The new "tailscale up" checks previously didn't protect against --advertise-exit-node being omitted in the case that --advertise-routes was also provided. It wasn't done before because there is no corresponding pref for "--advertise-exit-node"; it's a helper flag that augments --advertise-routes. But that's an implementation detail and we can still help users. We just have to special case that pref and look whether the current routes include both the v4 and v6 /0 routes. Fixes #1767 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 91 +++++++++++++++++++++++++++++++++++ cmd/tailscale/cli/up.go | 27 +++++++++++ 2 files changed, 118 insertions(+) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 1ac15a225..f5d32fc09 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -181,6 +181,97 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { }, want: "", }, + { + name: "error_advertised_routes_exit_node_removed", + flagSet: f("advertise-routes"), + 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("10.0.42.0/24"), + }, + }, + AdvertiseRoutesSet: true, + }, + want: "'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node", + }, + { + name: "advertised_routes_exit_node_removed", + flagSet: f("advertise-routes", "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("10.0.42.0/24"), + }, + }, + AdvertiseRoutesSet: true, + }, + want: "", + }, + { + name: "advertised_routes_includes_the_0_routes", // but no --advertise-exit-node + flagSet: f("advertise-routes"), + 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"), + netaddr.MustParseIPPrefix("::/0"), + }, + }, + AdvertiseRoutesSet: true, + }, + want: "", + }, + { + name: "advertised_routes_includes_only_one_0_route", // and no --advertise-exit-node + flagSet: f("advertise-routes"), + 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: "'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 61339a406..0d6061479 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -527,6 +527,18 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre continue } flagName, hasFlag := flagForPref[prefName] + + // Special case for advertise-exit-node; which is a + // flag but doesn't have a corresponding pref. The + // flag augments advertise-routes, so we have to infer + // the imaginary pref's current value from the routes. + if prefName == "AdvertiseRoutes" && + hasExitNodeRoutes(curPrefs.AdvertiseRoutes) && + !hasExitNodeRoutes(curWithExplicitEdits.AdvertiseRoutes) && + !flagSet["advertise-exit-node"] { + errs = append(errs, errors.New("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node")) + } + if hasFlag && flagSet[flagName] { continue } @@ -540,6 +552,7 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre } } exi, imi := ex.Interface(), im.Interface() + if reflect.DeepEqual(exi, imi) { continue } @@ -575,3 +588,17 @@ func fmtSettingVal(v interface{}) string { } return fmt.Sprint(v) } + +func hasExitNodeRoutes(rr []netaddr.IPPrefix) bool { + var v4, v6 bool + for _, r := range rr { + if r.Bits == 0 { + if r.IP.Is4() { + v4 = true + } else if r.IP.Is6() { + v6 = true + } + } + } + return v4 && v6 +}