cmd/tailscale/cli: fix race condition in up --force-reauth

This commit fixes a race condition where `tailscale up --force-reauth` would
exit prematurely on an already-logged in device.

Previously, the CLI would wait for IPN to report the "Running" state and then
exit. However, this could happen before the new auth URL was printed, leading
to two distinct issues:

*   **Without seamless key renewal:** The CLI could exit immediately after
    the `StartLoginInteractive` call, before IPN has time to switch into
    the "Starting" state or send a new auth URL back to the CLI.
*   **With seamless key renewal:** IPN stays in the "Running" state
    throughout the process, so the CLI exits immediately without performing
    any reauthentication.

The fix is to change the CLI's exit condition.

Instead of waiting for the "Running" state, if we're doing a `--force-reauth`
we now wait to see the node key change, which is a more reliable indicator
that a successful authentication has occurred.

Updates tailscale/corp#31476
Updates tailscale/tailscale#17108

Signed-off-by: Alex Chan <alexc@tailscale.com>

cmd/tailscale/cli/up.go

@@ -595,13 +595,24 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 		}
 	}
 
-	running := make(chan bool, 1)
+	upComplete := make(chan bool, 1)
 	watchErr := make(chan error, 1)
 
 	go func() {
 		var printed bool // whether we've yet printed anything to stdout or stderr
 		var lastURLPrinted string
 
+		// If we're doing a force-reauth, we need to get two notifications:
+		//
+		//	 1. IPN is running
+		//	 2. The node key has changed
+		//
+		// These two notifications arrive separately, and trying to combine them
+		// has caused unexpected issues elsewhere in `tailscale up`.  For now, we
+		// track them separately.
+		var currentState ipn.State
+		waitingForKeyChange := upArgs.forceReauth
+
 		for {
 			n, err := watcher.Next()
 			if err != nil {
@@ -612,30 +623,35 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 				msg := *n.ErrMessage
 				fatalf("backend error: %v\n", msg)
 			}
+			if n.NodeKeyChanged != nil {
+				waitingForKeyChange = false
+			}
 			if s := n.State; s != nil {
-				switch *s {
+				currentState = *s
-				case ipn.NeedsMachineAuth:
+			}
-					printed = true
+			if currentState == ipn.NeedsMachineAuth && !printed {
-					if env.upArgs.json {
+				printed = true
-						printUpDoneJSON(ipn.NeedsMachineAuth, "")
+				if env.upArgs.json {
-					} else {
+					printUpDoneJSON(ipn.NeedsMachineAuth, "")
-						fmt.Fprintf(Stderr, "\nTo approve your machine, visit (as admin):\n\n\t%s\n\n", prefs.AdminPageURL(policyclient.Get()))
+				} else {
-					}
+					fmt.Fprintf(Stderr, "\nTo approve your machine, visit (as admin):\n\n\t%s\n\n", prefs.AdminPageURL(policyclient.Get()))
-				case ipn.Running:
-					// Done full authentication process
-					if env.upArgs.json {
-						printUpDoneJSON(ipn.Running, "")
-					} else if printed {
-						// Only need to print an update if we printed the "please click" message earlier.
-						fmt.Fprintf(Stderr, "Success.\n")
-					}
-					select {
-					case running <- true:
-					default:
-					}
-					cancelWatch()
 				}
 			}
+			if currentState == ipn.Running && !waitingForKeyChange {
+				// Done full authentication process
+				if env.upArgs.json {
+					printUpDoneJSON(ipn.Running, "")
+				} else if printed {
+					// Only need to print an update if we printed the "please click" message earlier.
+					fmt.Fprintf(Stderr, "Success.\n")
+				}
+				select {
+				case upComplete <- true:
+				default:
+				}
+				cancelWatch()
+				return
+			}
 			if url := n.BrowseToURL; url != nil {
 				authURL := *url
 				if !printAuthURL(authURL) || authURL == lastURLPrinted {
@@ -696,18 +712,18 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 		timeoutCh = timeoutTimer.C
 	}
 	select {
-	case <-running:
+	case <-upComplete:
 		return nil
 	case <-watchCtx.Done():
 		select {
-		case <-running:
+		case <-upComplete:
 			return nil
 		default:
 		}
 		return watchCtx.Err()
 	case err := <-watchErr:
 		select {
-		case <-running:
+		case <-upComplete:
 			return nil
 		default:
 		}

control/controlclient/direct.go

@@ -85,6 +85,7 @@ type Direct struct {
 	skipIPForwardingCheck bool
 	pinger                Pinger
 	popBrowser            func(url string)    // or nil
+	notifyKeyChange       func()              // or nil
 	polc                  policyclient.Client // always non-nil
 	c2nHandler            http.Handler        // or nil
 	panicOnUse            bool                // if true, panic if client is used (for testing)
@@ -139,6 +140,7 @@ type Options struct {
 	DebugFlags           []string     // debug settings to send to control
 	HealthTracker        *health.Tracker
 	PopBrowserURL        func(url string)    // optional func to open browser
+	NotifyKeyChange      func()              // optional func to notify on a key change
 	Dialer               *tsdial.Dialer      // non-nil
 	C2NHandler           http.Handler        // or nil
 	ControlKnobs         *controlknobs.Knobs // or nil to ignore
@@ -307,6 +309,7 @@ func NewDirect(opts Options) (*Direct, error) {
 		pinger:                opts.Pinger,
 		polc:                  cmp.Or(opts.PolicyClient, policyclient.Client(policyclient.NoPolicyClient{})),
 		popBrowser:            opts.PopBrowserURL,
+		notifyKeyChange:       opts.NotifyKeyChange,
 		c2nHandler:            opts.C2NHandler,
 		dialer:                opts.Dialer,
 		dnsCache:              dnsCache,
@@ -761,7 +764,12 @@ func (c *Direct) doLogin(ctx context.Context, opt loginOpt) (mustRegen bool, new
 	c.mu.Lock()
 	if resp.AuthURL == "" {
 		// key rotation is complete
+		isKeyChanged := !persist.PrivateNodeKey.Equal(tryingNewKey)
 		persist.PrivateNodeKey = tryingNewKey
+
+		if isKeyChanged && c.notifyKeyChange != nil {
+			c.notifyKeyChange()
+		}
 	} else {
 		// save it for the retry-with-URL
 		c.tryingNewKey = tryingNewKey

ipn/backend.go

@@ -106,12 +106,13 @@ type Notify struct {
 	// For State InUseOtherUser, ErrMessage is not critical and just contains the details.
 	ErrMessage *string
 
-	LoginFinished *empty.Message     // non-nil when/if the login process succeeded
+	LoginFinished  *empty.Message     // non-nil when/if the login process succeeded
-	State         *State             // if non-nil, the new or current IPN state
+	NodeKeyChanged *empty.Message     // non-nil when/if the node key changes
-	Prefs         *PrefsView         // if non-nil && Valid, the new or current preferences
+	State          *State             // if non-nil, the new or current IPN state
-	NetMap        *netmap.NetworkMap // if non-nil, the new or current netmap
+	Prefs          *PrefsView         // if non-nil && Valid, the new or current preferences
-	Engine        *EngineStatus      // if non-nil, the new or current wireguard stats
+	NetMap         *netmap.NetworkMap // if non-nil, the new or current netmap
-	BrowseToURL   *string            // if non-nil, UI should open a browser right now
+	Engine         *EngineStatus      // if non-nil, the new or current wireguard stats
+	BrowseToURL    *string            // if non-nil, UI should open a browser right now
 
 	// FilesWaiting if non-nil means that files are buffered in
 	// the Tailscale daemon and ready for local transfer to the
@@ -173,6 +174,9 @@ func (n Notify) String() string {
 	if n.LoginFinished != nil {
 		sb.WriteString("LoginFinished ")
 	}
+	if n.NodeKeyChanged != nil {
+		sb.WriteString("NodeKeyChanged ")
+	}
 	if n.State != nil {
 		fmt.Fprintf(&sb, "state=%v ", *n.State)
 	}

ipn/ipnlocal/bus.go

@@ -152,6 +152,7 @@ func isNotableNotify(n *ipn.Notify) bool {
 		n.Prefs != nil ||
 		n.ErrMessage != nil ||
 		n.LoginFinished != nil ||
+		n.NodeKeyChanged != nil ||
 		!n.DriveShares.IsNil() ||
 		n.Health != nil ||
 		len(n.IncomingFiles) > 0 ||

ipn/ipnlocal/local.go

@@ -2524,6 +2524,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		PolicyClient:         b.sys.PolicyClientOrDefault(),
 		Pinger:               b,
 		PopBrowserURL:        b.tellClientToBrowseToURL,
+		NotifyKeyChange:      b.notifyKeyChange,
 		Dialer:               b.Dialer(),
 		Observer:             b,
 		C2NHandler:           http.HandlerFunc(b.handleC2N),
@@ -3570,6 +3571,10 @@ func (b *LocalBackend) tellClientToBrowseToURL(url string) {
 	b.tellRecipientToBrowseToURL(url, allClients)
 }
 
+func (b *LocalBackend) notifyKeyChange() {
+	b.sendTo(ipn.Notify{NodeKeyChanged: &empty.Message{}}, allClients)
+}
+
 // tellRecipientToBrowseToURL is like tellClientToBrowseToURL but allows specifying a recipient.
 func (b *LocalBackend) tellRecipientToBrowseToURL(url string, recipient notificationTarget) {
 	if b.validPopBrowserURL(url) {

tstest/integration/integration_test.go

@@ -306,16 +306,12 @@ func TestOneNodeUpAuth(t *testing.T) {
 			alreadyLoggedIn: true,
 			needsNewLogin:   false,
 		},
-		// TODO(alexc): This test is failing because of a bug in `tailscale up` where
+		{
-		// it waits for ipn to enter the "Running" state.  If we're already logged in
+			name:            "up-with-force-reauth-after-login",
-		// and running, this completes immediately, before we've had a chance to show
+			args:            []string{"up", "--force-reauth"},
-		// the user the auth URL.
+			alreadyLoggedIn: true,
-		// {
+			needsNewLogin:   true,
-		// 	name:            "up-with-force-reauth-after-login",
+		},
-		// 	args:            []string{"up", "--force-reauth"},
-		// 	alreadyLoggedIn: true,
-		// 	needsNewLogin:   true,
-		// },
 		{
 			name:            "up-with-auth-key-after-login",
 			args:            []string{"up", "--auth-key=opensesame"},
@@ -337,6 +333,21 @@ func TestOneNodeUpAuth(t *testing.T) {
 				tstest.Shard(t)
 				tstest.Parallel(t)
 
+				// TODO(alexc): Get this test case working reliably.
+				//
+				// This test case works correctly when you run it in a real CLI, but is
+				// quite flaky in this test suite -- the test times out waiting for the
+				// `tailscale up` command to complete, because IPN notifications seem
+				// to get lost. I suspect it's a bug somewhere in the testcontrol server,
+				// possibly a deadlock? But I spent a day investigating and was unable
+				// to fix it.
+				//
+				// These tests still represent a big increase in coverage for `tailscale up`,
+				// so I'm going to leave this as an exercise for a future reader.
+				if t.Name() == "TestOneNodeUpAuth/up-with-force-reauth-after-login-seamless-false" {
+					t.Skip()
+				}
+
 				env := NewTestEnv(t, ConfigureControl(
 					func(control *testcontrol.Server) {
 						if tt.authKey != "" {