ipn/ipnlocal: fix controlclient reentrancy when deleting profile during logoutmihaip/logout-async-start

Deletion of profiles on logout (#6297) added a LocalBackend.Start()
call within setClientStatus(), but that's a callback from the
controlclient. Start() ends calling back into the controlclient (to
shut it down), and we end up stuck in a deadlock waiting for the
authDone channel to be closed.

Fixed by making the Start call asynchronous. To reproduce this in a test
case, we need to do an asynchronous logout, so add a CLI option for
that.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>

5 files changed, 66 insertions, 8 deletions

diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go
index a9e6edc28..5dad2d667 100644
--- a/client/tailscale/localclient.go
+++ b/client/tailscale/localclient.go
@@ -548,6 +548,11 @@ func (lc *LocalClient) Logout(ctx context.Context) error {
 	return err
 }
 
+func (lc *LocalClient) LogoutAsync(ctx context.Context) error {
+	_, err := lc.send(ctx, "POST", "/localapi/v0/logout?async=true", http.StatusNoContent, nil)
+	return err
+}
+
 // SetDNS adds a DNS TXT record for the given domain name, containing
 // the provided TXT value. The intended use case is answering
 // LetsEncrypt/ACME dns-01 challenges.
diff --git a/cmd/tailscale/cli/logout.go b/cmd/tailscale/cli/logout.go
index 0bce01fda..a46409b54 100644
--- a/cmd/tailscale/cli/logout.go
+++ b/cmd/tailscale/cli/logout.go
@@ -6,6 +6,7 @@ package cli
 
 import (
 	"context"
+	"flag"
 	"fmt"
 	"strings"
 
@@ -23,11 +24,23 @@ the current node key, forcing a future use of it to cause
 a reauthentication.
 `),
 	Exec: runLogout,
+	FlagSet: (func() *flag.FlagSet {
+		fs := newFlagSet("logout")
+		fs.BoolVar(&logoutArgs.async, "async", false, "Does not wait for logout to be complete (status can be queried to determine success)")
+		return fs
+	})(),
+}
+
+var logoutArgs struct {
+	async bool
 }
 
 func runLogout(ctx context.Context, args []string) error {
 	if len(args) > 0 {
 		return fmt.Errorf("too many non-flag arguments: %q", args)
 	}
+	if logoutArgs.async {
+		return localClient.LogoutAsync(ctx)
+	}
 	return localClient.Logout(ctx)
 }
diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index a407e7242..6c56d5ac4 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -810,7 +810,9 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) {
 		if err := b.pm.DeleteProfile(b.pm.CurrentProfile().ID); err != nil {
 			b.logf("error deleting profile: %v", err)
 		}
-		b.resetForProfileChangeLockedOnEntry()
+		// Restart backend asychonously, so that we avoid reentrancy in the
+		// controclient (we're currently in a callback from it).
+		b.resetForProfileChangeLockedOnEntry(profileChangeStartAsync)
 		return
 	}
 
@@ -2110,7 +2112,7 @@ func (b *LocalBackend) SetCurrentUserID(uid string) {
 		b.mu.Unlock()
 		return
 	}
-	b.resetForProfileChangeLockedOnEntry()
+	b.resetForProfileChangeLockedOnEntry(profileChangeStartSync)
 }
 
 func (b *LocalBackend) CheckPrefs(p *ipn.Prefs) error {
@@ -4045,16 +4047,32 @@ func (b *LocalBackend) SwitchProfile(profile ipn.ProfileID) error {
 		b.mu.Unlock()
 		return err
 	}
-	return b.resetForProfileChangeLockedOnEntry()
+	return b.resetForProfileChangeLockedOnEntry(profileChangeStartSync)
 }
 
+type profileChangeStartMode int
+
+const (
+	profileChangeStartSync profileChangeStartMode = iota
+	profileChangeStartAsync
+)
+
 // resetForProfileChangeLockedOnEntry resets the backend for a profile change.
-func (b *LocalBackend) resetForProfileChangeLockedOnEntry() error {
+// It requires b.mu be held to call it, but it unlocks b.mu when done.
+func (b *LocalBackend) resetForProfileChangeLockedOnEntry(startMode profileChangeStartMode) error {
 	b.setNetMapLocked(nil) // Reset netmap.
 	// Reset the NetworkMap in the engine
 	b.e.SetNetworkMap(new(netmap.NetworkMap))
 	b.enterStateLockedOnEntry(ipn.NoState) // Reset state.
-	return b.Start(ipn.Options{})
+	switch startMode {
+	case profileChangeStartSync:
+		return b.Start(ipn.Options{})
+	case profileChangeStartAsync:
+		go b.Start(ipn.Options{})
+		return nil
+	default:
+		panic("unreachable")
+	}
 }
 
 // DeleteProfile deletes a profile with the given ID.
@@ -4072,7 +4090,7 @@ func (b *LocalBackend) DeleteProfile(p ipn.ProfileID) error {
 	if !needToRestart {
 		return nil
 	}
-	return b.resetForProfileChangeLockedOnEntry()
+	return b.resetForProfileChangeLockedOnEntry(profileChangeStartSync)
 }
 
 // CurrentProfile returns the current LoginProfile.
@@ -4087,7 +4105,7 @@ func (b *LocalBackend) CurrentProfile() ipn.LoginProfile {
 func (b *LocalBackend) NewProfile() error {
 	b.mu.Lock()
 	b.pm.NewProfile()
-	return b.resetForProfileChangeLockedOnEntry()
+	return b.resetForProfileChangeLockedOnEntry(profileChangeStartSync)
 }
 
 // ListProfiles returns a list of all LoginProfiles.
diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go
index c04b716b5..f236a131f 100644
--- a/ipn/localapi/localapi.go
+++ b/ipn/localapi/localapi.go
@@ -552,7 +552,12 @@ func (h *Handler) serveLogout(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "want POST", 400)
 		return
 	}
-	err := h.b.LogoutSync(r.Context())
+	var err error
+	if defBool(r.FormValue("async"), false) {
+		h.b.Logout()
+	} else {
+		err = h.b.LogoutSync(r.Context())
+	}
 	if err == nil {
 		w.WriteHeader(http.StatusNoContent)
 		return
diff --git a/tstest/integration/integration_test.go b/tstest/integration/integration_test.go
index 504597586..ccb2d122a 100644
--- a/tstest/integration/integration_test.go
+++ b/tstest/integration/integration_test.go
@@ -555,6 +555,23 @@ func TestLogoutRemovesAllPeers(t *testing.T) {
 	wantNode0PeerCount(expectedPeers) // all existing peers and the new node
 }
 
+func TestLogoutAsyncState(t *testing.T) {
+	t.Parallel()
+	env := newTestEnv(t)
+	node := newTestNode(t, env)
+	node.StartDaemon()
+	node.AwaitResponding()
+	node.MustUp()
+	node.AwaitIP()
+	node.AwaitRunning()
+
+	log.Printf("running logout CLI")
+	if err := node.Tailscale("logout", "--async").Run(); err != nil {
+		t.Fatalf("logout: %v", err)
+	}
+	node.AwaitNeedsLogin()
+}
+
 // testEnv contains the test environment (set of servers) used by one
 // or more nodes.
 type testEnv struct {