From 2585edfaebd42dcd69c15035103072bec7b4ba3c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 5 May 2021 20:17:23 -0700 Subject: [PATCH] cmd/tailscale: fix tailscale up --advertise-exit-node validation Fixes #1859 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 92 ++++++++++++++++++++++++++++++++--- cmd/tailscale/cli/up.go | 31 ++++++++++++ 2 files changed, 117 insertions(+), 6 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index f8297c4f9..589d07da1 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -274,6 +274,71 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { }, 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. + }, + want: "", + }, + { + name: "advertise_exit_node_over_existing_routes", + flagSet: f("advertise-exit-node"), + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + 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"), + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + 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", + }, { name: "exit_node_clearing", // Issue 1777 flagSet: f("exit-node"), @@ -317,7 +382,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { WantRunning: true, }, }, - want: accidentalUpPrefix + " --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 --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", }, { name: "remove_all_implicit_except_hostname", @@ -426,7 +491,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { } func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs { - upArgs := defaultUpArgsByGOOS(goos) + upArgs := upArgsFromOSArgs(goos) prefs, err := prefsFromUpArgs(upArgs, logger.Discard, new(ipnstate.Status), "linux") if err != nil { t.Fatalf("defaultPrefsFromUpArgs: %v", err) @@ -435,9 +500,9 @@ func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs { return prefs } -func defaultUpArgsByGOOS(goos string) (args upArgsT) { +func upArgsFromOSArgs(goos string, flagArgs ...string) (args upArgsT) { fs := newUpFlagSet(goos, &args) - fs.Parse(nil) // populates args + fs.Parse(flagArgs) // populates args return } @@ -454,7 +519,7 @@ func TestPrefsFromUpArgs(t *testing.T) { { name: "default_linux", goos: "linux", - args: defaultUpArgsByGOOS("linux"), + args: upArgsFromOSArgs("linux"), want: &ipn.Prefs{ ControlURL: ipn.DefaultControlURL, WantRunning: true, @@ -467,7 +532,7 @@ func TestPrefsFromUpArgs(t *testing.T) { { name: "default_windows", goos: "windows", - args: defaultUpArgsByGOOS("windows"), + args: upArgsFromOSArgs("windows"), want: &ipn.Prefs{ ControlURL: ipn.DefaultControlURL, WantRunning: true, @@ -476,6 +541,21 @@ func TestPrefsFromUpArgs(t *testing.T) { NetfilterMode: preftype.NetfilterOn, }, }, + { + name: "advertise_default_route", + args: upArgsFromOSArgs("linux", "--advertise-exit-node"), + want: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + WantRunning: true, + AllowSingleHosts: true, + CorpDNS: true, + AdvertiseRoutes: []netaddr.IPPrefix{ + netaddr.MustParseIPPrefix("0.0.0.0/0"), + netaddr.MustParseIPPrefix("::/0"), + }, + NetfilterMode: preftype.NetfilterOn, + }, + }, { name: "error_advertise_route_invalid_ip", args: upArgsT{ diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index f705fb524..56bc8f0ec 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -587,6 +587,16 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre flagExplicitValue[flagName] = ev.Field(i).Interface() continue } + + if prefName == "AdvertiseRoutes" && + (len(curPrefs.AdvertiseRoutes) == 0 || + hasExitNodeRoutes(curPrefs.AdvertiseRoutes) && len(curPrefs.AdvertiseRoutes) == 2) && + hasExitNodeRoutes(mp.Prefs.AdvertiseRoutes) && + len(mp.Prefs.AdvertiseRoutes) == 2 && + flagSet["advertise-exit-node"] { + continue + } + // Get explicit value and implicit value ex, im := ev.Field(i), iv.Field(i) switch ex.Kind() { @@ -624,6 +634,9 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre if prefName == "ExitNodeIP" { missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi))) } + case "advertise-routes": + routes := withoutExitNodes(exi.([]netaddr.IPPrefix)) + missing = append(missing, fmtFlagValueArg("advertise-routes", fmtSettingVal(routes))) default: missing = append(missing, fmtFlagValueArg(flagName, fmtSettingVal(exi))) } @@ -642,6 +655,8 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre for _, flagName := range flagSetSorted { if ev, ok := flagExplicitValue[flagName]; ok { fmt.Fprintf(&sb, " %s", fmtFlagValueArg(flagName, fmtSettingVal(ev))) + } else { + fmt.Fprintf(&sb, " --%s", flagName) } } for _, a := range missing { @@ -698,3 +713,19 @@ func hasExitNodeRoutes(rr []netaddr.IPPrefix) bool { } return v4 && v6 } + +// withoutExitNodes returns rr unchanged if it has only 1 or 0 /0 +// routes. If it has both IPv4 and IPv6 /0 routes, then it returns +// a copy with all /0 routes removed. +func withoutExitNodes(rr []netaddr.IPPrefix) []netaddr.IPPrefix { + if !hasExitNodeRoutes(rr) { + return rr + } + var out []netaddr.IPPrefix + for _, r := range rr { + if r.Bits > 0 { + out = append(out, r) + } + } + return out +}