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 }