From 9972c02b60d74f177eee6a5214573cf7a8dcfc5e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 10 Apr 2021 21:56:18 -0700 Subject: [PATCH] cmd/tailscale/cli: don't let up change prefs based on implicit flag values This changes the behavior of "tailscale up". Previously "tailscale up" always did a new Start and reset all the settings. Now "tailscale up" with no flags just brings the world [back] up. (The opposite of "tailscale down"). But with flags, "tailscale up" now only is allowed to change preferences if they're explicitly named in the flags. Otherwise it's an error. Or you need to use --reset to explicitly nuke everything. RELNOTE=tailscale up change Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 117 ++++++++++++ cmd/tailscale/cli/up.go | 346 +++++++++++++++++++++++++++------- cmd/tailscale/depaware.txt | 1 + ipn/ipnlocal/local.go | 8 +- 4 files changed, 404 insertions(+), 68 deletions(-) create mode 100644 cmd/tailscale/cli/cli_test.go diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go new file mode 100644 index 000000000..f8efc3b4d --- /dev/null +++ b/cmd/tailscale/cli/cli_test.go @@ -0,0 +1,117 @@ +// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package cli + +import ( + "flag" + "testing" + + "tailscale.com/ipn" +) + +// Test that checkForAccidentalSettingReverts's updateMaskedPrefsFromUpFlag can handle +// all flags. This will panic if a new flag creeps in that's unhandled. +func TestUpdateMaskedPrefsFromUpFlag(t *testing.T) { + mp := new(ipn.MaskedPrefs) + upFlagSet.VisitAll(func(f *flag.Flag) { + updateMaskedPrefsFromUpFlag(mp, f.Name) + }) +} + +func TestCheckForAccidentalSettingReverts(t *testing.T) { + f := func(flags ...string) map[string]bool { + m := make(map[string]bool) + for _, f := range flags { + m[f] = true + } + return m + } + tests := []struct { + name string + flagSet map[string]bool + curPrefs *ipn.Prefs + mp *ipn.MaskedPrefs + want string + }{ + { + name: "bare_up_means_up", + flagSet: f(), + curPrefs: &ipn.Prefs{ + WantRunning: false, + Hostname: "foo", + }, + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + WantRunning: true, + }, + WantRunningSet: true, + }, + want: "", + }, + { + name: "losing_hostname", + flagSet: f("accept-dns"), + curPrefs: &ipn.Prefs{ + WantRunning: false, + Hostname: "foo", + CorpDNS: true, + }, + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + WantRunning: true, + CorpDNS: true, + }, + WantRunningSet: true, + CorpDNSSet: true, + }, + want: `'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --hostname is not specified but its default value of "" differs from current value "foo"`, + }, + { + name: "hostname_changing_explicitly", + flagSet: f("hostname"), + curPrefs: &ipn.Prefs{ + WantRunning: false, + Hostname: "foo", + }, + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + WantRunning: true, + Hostname: "bar", + }, + WantRunningSet: true, + HostnameSet: true, + }, + want: "", + }, + { + name: "hostname_changing_empty_explicitly", + flagSet: f("hostname"), + curPrefs: &ipn.Prefs{ + WantRunning: false, + Hostname: "foo", + }, + mp: &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + WantRunning: true, + Hostname: "", + }, + WantRunningSet: true, + HostnameSet: true, + }, + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got string + if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp); err != nil { + got = err.Error() + } + if got != tt.want { + t.Errorf("unexpected result\n got: %s\nwant: %s\n", got, tt.want) + } + }) + } +} diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index d6a78628f..a950f7dec 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -9,13 +9,15 @@ import ( "errors" "flag" "fmt" - "log" "os" + "reflect" "runtime" "sort" + "strconv" "strings" "sync" + "github.com/go-multierror/multierror" "github.com/peterbourgon/ff/v2/ffcli" "inet.af/netaddr" "tailscale.com/client/tailscale" @@ -34,32 +36,43 @@ var upCmd = &ffcli.Command{ "tailscale up" connects this machine to your Tailscale network, triggering authentication if necessary. -The flags passed to this command are specific to this machine. If you don't -specify any flags, options are reset to their default. +With no flags, "tailscale up" brings the network online without +changing any settings. (That is, it's the opposite of "tailscale +down"). + +If flags are specified, the flags must be the complete set of desired +settings. An error is returned if any setting would be changed as a +result of an unspecified flag's default value, unless the --reset +flag is also used. `), - FlagSet: (func() *flag.FlagSet { - upf := flag.NewFlagSet("up", flag.ExitOnError) - upf.StringVar(&upArgs.server, "login-server", "https://login.tailscale.com", "base URL of control server") - upf.BoolVar(&upArgs.acceptRoutes, "accept-routes", false, "accept routes advertised by other Tailscale nodes") - upf.BoolVar(&upArgs.acceptDNS, "accept-dns", true, "accept DNS configuration from the admin panel") - upf.BoolVar(&upArgs.singleRoutes, "host-routes", true, "install host routes to other Tailscale nodes") - upf.StringVar(&upArgs.exitNodeIP, "exit-node", "", "Tailscale IP of the exit node for internet traffic") - upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections") - upf.BoolVar(&upArgs.forceReauth, "force-reauth", false, "force reauthentication") - upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "ACL tags to request (comma-separated, e.g. \"tag:eng,tag:montreal,tag:ssh\")") - upf.StringVar(&upArgs.authKey, "authkey", "", "node authorization key") - upf.StringVar(&upArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS") - upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\")") - upf.BoolVar(&upArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet") - if runtime.GOOS == "linux" { - upf.BoolVar(&upArgs.snat, "snat-subnet-routes", true, "source NAT traffic to local routes advertised with --advertise-routes") - upf.StringVar(&upArgs.netfilterMode, "netfilter-mode", defaultNetfilterMode(), "netfilter mode (one of on, nodivert, off)") - } - return upf - })(), - Exec: runUp, + FlagSet: upFlagSet, + Exec: runUp, } +var upFlagSet = (func() *flag.FlagSet { + upf := flag.NewFlagSet("up", flag.ExitOnError) + + upf.BoolVar(&upArgs.forceReauth, "force-reauth", false, "force reauthentication") + upf.BoolVar(&upArgs.reset, "reset", false, "reset unspecified settings to their default values") + + upf.StringVar(&upArgs.server, "login-server", "https://login.tailscale.com", "base URL of control server") + upf.BoolVar(&upArgs.acceptRoutes, "accept-routes", false, "accept routes advertised by other Tailscale nodes") + upf.BoolVar(&upArgs.acceptDNS, "accept-dns", true, "accept DNS configuration from the admin panel") + upf.BoolVar(&upArgs.singleRoutes, "host-routes", true, "install host routes to other Tailscale nodes") + upf.StringVar(&upArgs.exitNodeIP, "exit-node", "", "Tailscale IP of the exit node for internet traffic") + upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections") + upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "comma-separated ACL tags to request; each must start with \"tag:\" (e.g. \"tag:eng,tag:montreal,tag:ssh\")") + upf.StringVar(&upArgs.authKey, "authkey", "", "node authorization key") + upf.StringVar(&upArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS") + upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\")") + upf.BoolVar(&upArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet") + if runtime.GOOS == "linux" { + upf.BoolVar(&upArgs.snat, "snat-subnet-routes", true, "source NAT traffic to local routes advertised with --advertise-routes") + upf.StringVar(&upArgs.netfilterMode, "netfilter-mode", defaultNetfilterMode(), "netfilter mode (one of on, nodivert, off)") + } + return upf +})() + func defaultNetfilterMode() string { if distro.Get() == distro.Synology { return "off" @@ -68,6 +81,7 @@ func defaultNetfilterMode() string { } var upArgs struct { + reset bool server string acceptRoutes bool acceptDNS bool @@ -95,7 +109,12 @@ var ( func runUp(ctx context.Context, args []string) error { if len(args) > 0 { - log.Fatalf("too many non-flag arguments: %q", args) + fatalf("too many non-flag arguments: %q", args) + } + + st, err := tailscale.Status(ctx) + if err != nil { + fatalf("can't fetch status from tailscaled: %v", err) } if distro.Get() == distro.Synology { @@ -165,6 +184,14 @@ func runUp(ctx context.Context, args []string) error { } } + if !exitNodeIP.IsZero() { + for _, ip := range st.TailscaleIPs { + if exitNodeIP == ip { + fatalf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", exitNodeIP) + } + } + } + var tags []string if upArgs.advertiseTags != "" { tags = strings.Split(upArgs.advertiseTags, ",") @@ -209,27 +236,70 @@ func runUp(ctx context.Context, args []string) error { } } - c, bc, ctx, cancel := connect(ctx) - defer cancel() + curPrefs, err := tailscale.GetPrefs(ctx) + if err != nil { + return err + } - if !prefs.ExitNodeIP.IsZero() { - st, err := tailscale.Status(ctx) - if err != nil { - fatalf("can't fetch status from tailscaled: %v", err) - } - for _, ip := range st.TailscaleIPs { - if prefs.ExitNodeIP == ip { - fatalf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", ip) - } + 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 + }) + + if !upArgs.reset { + if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp); err != nil { + fatalf("%s", err) } } - var printed bool + controlURLChanged := curPrefs.ControlURL != prefs.ControlURL + if controlURLChanged && st.BackendState == ipn.Running.String() && !upArgs.forceReauth { + fatalf("can't change --login-server without --force-reauth") + } + + // If we're already running and none of the flags require a + // restart, we can just do an EditPrefs call and change the + // prefs at runtime (e.g. changing hostname, changinged + // advertised tags, routes, etc) + justEdit := st.BackendState == ipn.Running.String() && + !upArgs.forceReauth && + !upArgs.reset && + upArgs.authKey == "" && + !controlURLChanged + if justEdit { + _, err := tailscale.EditPrefs(ctx, mp) + return err + } + + // simpleUp is whether we're running a simple "tailscale up" + // to transition to running from a previously-logged-in but + // down state, without changing any settings. + simpleUp := len(flagSet) == 0 && curPrefs.Persist != nil && curPrefs.Persist.LoginName != "" + + // At this point we need to subscribe to the IPN bus to watch + // for state transitions and possible need to authenticate. + c, bc, pumpCtx, cancel := connect(ctx) + defer cancel() + + startingOrRunning := make(chan bool, 1) // gets value once starting or running + gotEngineUpdate := make(chan bool, 1) // gets value upon an engine update + go pump(pumpCtx, bc, c) + + printed := !simpleUp var loginOnce sync.Once startLoginInteractive := func() { loginOnce.Do(func() { bc.StartLoginInteractive() }) } - bc.SetPrefs(prefs) bc.SetNotifyCallback(func(n ipn.Notify) { + if n.Engine != nil { + select { + case gotEngineUpdate <- true: + default: + } + } if n.ErrMessage != nil { msg := *n.ErrMessage if msg == ipn.ErrMsgPermissionDenied { @@ -256,46 +326,188 @@ func runUp(ctx context.Context, args []string) error { // Only need to print an update if we printed the "please click" message earlier. fmt.Fprintf(os.Stderr, "Success.\n") } + select { + case startingOrRunning <- true: + default: + } cancel() } } if url := n.BrowseToURL; url != nil { + printed = true fmt.Fprintf(os.Stderr, "\nTo authenticate, visit:\n\n\t%s\n\n", *url) } }) + // Wait for backend client to be connected so we know + // we're subscribed to updates. Otherwise we can miss + // an update upon its transition to running. Do so by causing some traffic + // back to the bus that we then wait on. + bc.RequestEngineStatus() + <-gotEngineUpdate - opts := ipn.Options{ - StateKey: ipn.GlobalDaemonStateKey, - AuthKey: upArgs.authKey, - } + // Special case: bare "tailscale up" means to just start + // running, if there's ever been a login. + if simpleUp { + _, err := tailscale.EditPrefs(ctx, &ipn.MaskedPrefs{ + Prefs: ipn.Prefs{ + WantRunning: true, + }, + WantRunningSet: true, + }) + if err != nil { + return err + } + } else { + bc.SetPrefs(prefs) - // On Windows, we still run in mostly the "legacy" way that - // predated the server's StateStore. That is, we send an empty - // StateKey and send the prefs directly. Although the Windows - // supports server mode, though, the transition to StateStore - // is only half complete. Only server mode uses it, and the - // Windows service (~tailscaled) is the one that computes the - // StateKey based on the connection identity. So for now, just - // do as the Windows GUI's always done: - if runtime.GOOS == "windows" { - // The Windows service will set this as needed based - // on our connection's identity. - opts.StateKey = "" - opts.Prefs = prefs - } + opts := ipn.Options{ + StateKey: ipn.GlobalDaemonStateKey, + AuthKey: upArgs.authKey, + } + // On Windows, we still run in mostly the "legacy" way that + // predated the server's StateStore. That is, we send an empty + // StateKey and send the prefs directly. Although the Windows + // supports server mode, though, the transition to StateStore + // is only half complete. Only server mode uses it, and the + // Windows service (~tailscaled) is the one that computes the + // StateKey based on the connection identity. So for now, just + // do as the Windows GUI's always done: + if runtime.GOOS == "windows" { + // The Windows service will set this as needed based + // on our connection's identity. + opts.StateKey = "" + opts.Prefs = prefs + } - // We still have to Start for now. This causes a bunch of churn - // every time the CLI touches anything. - // - // TODO(danderson): redo the frontend/backend API to assume - // ephemeral frontends that read/modify/write state, once - // Windows/Mac state is moved into backend. - bc.Start(opts) - if upArgs.forceReauth { - printed = true + bc.Start(opts) startLoginInteractive() } - pump(ctx, bc, c) - return nil + select { + case <-startingOrRunning: + return nil + case <-ctx.Done(): + select { + case <-startingOrRunning: + return nil + default: + } + return ctx.Err() + } +} + +var ( + flagForPref = map[string]string{} // "ExitNodeIP" => "exit-node" + prefsOfFlag = map[string][]string{} +) + +func init() { + addPrefFlagMapping("accept-dns", "CorpDNS") + addPrefFlagMapping("accept-routes", "RouteAll") + addPrefFlagMapping("advertise-routes", "AdvertiseRoutes") + addPrefFlagMapping("advertise-tags", "AdvertiseTags") + addPrefFlagMapping("host-routes", "AllowSingleHosts") + addPrefFlagMapping("hostname", "Hostname") + addPrefFlagMapping("login-server", "ControlURL") + addPrefFlagMapping("netfilter-mode", "NetfilterMode") + addPrefFlagMapping("shields-up", "ShieldsUp") + addPrefFlagMapping("snat-subnet-routes", "NoSNAT") + addPrefFlagMapping("exit-node", "ExitNodeIP", "ExitNodeIP") +} + +func addPrefFlagMapping(flagName string, prefNames ...string) { + prefsOfFlag[flagName] = prefNames + for _, pref := range prefNames { + flagForPref[pref] = flagName + } +} + +func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) { + if prefs, ok := prefsOfFlag[flagName]; ok { + for _, pref := range prefs { + reflect.ValueOf(mp).Elem().FieldByName(pref + "Set").SetBool(true) + } + return + } + switch flagName { + case "authkey", "force-reauth", "reset": + // Not pref-related flags. + case "advertise-exit-node": + // This pref is a shorthand for advertise-routes. + default: + panic("internal error: unhandled flag " + flagName) + } +} + +// checkForAccidentalSettingReverts checks for people running +// "tailscale up" with a subset of the flags they originally ran it +// with. +// +// For example, in Tailscale 1.6 and prior, a user might've advertised +// a tag, but later tried to change just one other setting and forgot +// to mention the tag later and silently wiped it out. We now +// require --reset to change preferences to flag default values when +// the flag is not mentioned on the command line. +// +// curPrefs is what's currently active on the server. +// +// 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) error { + if len(flagSet) == 0 { + // A bare "tailscale up" is a special case to just + // mean bringing the network up without any changes. + return nil + } + curWithExplicitEdits := curPrefs.Clone() + curWithExplicitEdits.ApplyEdits(mp) + + prefType := reflect.TypeOf(ipn.Prefs{}) + + // Explicit values (current + explicit edit): + ev := reflect.ValueOf(curWithExplicitEdits).Elem() + // Implicit values (what we'd get if we replaced everything with flag defaults): + iv := reflect.ValueOf(&mp.Prefs).Elem() + var errs []error + var didExitNodeErr bool + for i := 0; i < prefType.NumField(); i++ { + prefName := prefType.Field(i).Name + if prefName == "Persist" { + continue + } + flagName, hasFlag := flagForPref[prefName] + if hasFlag && flagSet[flagName] { + continue + } + // Get explicit value and implicit value + evi, ivi := ev.Field(i).Interface(), iv.Field(i).Interface() + if reflect.DeepEqual(evi, ivi) { + continue + } + switch flagName { + case "": + errs = append(errs, fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; this command would change the value of flagless pref %q", prefName)) + case "exit-node": + if !didExitNodeErr { + didExitNodeErr = true + errs = append(errs, errors.New("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --exit-node is not specified but an exit node is currently configured")) + } + default: + errs = append(errs, fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --%s is not specified but its default value of %v differs from current value %v", + flagName, fmtSettingVal(ivi), fmtSettingVal(evi))) + } + } + return multierror.New(errs) +} + +func fmtSettingVal(v interface{}) string { + switch v := v.(type) { + case bool: + return strconv.FormatBool(v) + case string, preftype.NetfilterMode: + return fmt.Sprintf("%q", v) + case []string: + return strings.Join(v, ",") + } + return fmt.Sprint(v) } diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 9ae3d95a3..be3a096b8 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -2,6 +2,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep W 💣 github.com/alexbrainman/sspi from github.com/alexbrainman/sspi/negotiate W 💣 github.com/alexbrainman/sspi/negotiate from tailscale.com/net/tshttpproxy + github.com/go-multierror/multierror from tailscale.com/cmd/tailscale/cli github.com/peterbourgon/ff/v2 from github.com/peterbourgon/ff/v2/ffcli github.com/peterbourgon/ff/v2/ffcli from tailscale.com/cmd/tailscale/cli github.com/tcnksm/go-httpstat from tailscale.com/net/netcheck diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 0eb87442c..4b6e3b7d9 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1331,8 +1331,14 @@ func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) (*ipn.Prefs, error) { b.mu.Unlock() return p1, nil } + cc := b.cc b.logf("EditPrefs: %v", mp.Pretty()) - b.setPrefsLockedOnEntry("EditPrefs", p1) + b.setPrefsLockedOnEntry("EditPrefs", p1) // does a b.mu.Unlock + + if !p0.WantRunning && p1.WantRunning { + b.logf("EditPrefs: transitioning to running; doing Login...") + cc.Login(nil, controlclient.LoginDefault) + } return p1, nil }