ipn/{ipnlocal,ipnserver}: remove redundant (*LocalBackend).ResetForClientDisconnect

In this commit, we add a failing test to verify that ipn/ipnserver.Server correctly
sets and unsets the current user when two different users connect sequentially
(A connects, A disconnects, B connects, B disconnects).

We then fix the test by updating (*ipn/ipnserver.Server).addActiveHTTPRequest
to avoid calling (*LocalBackend).ResetForClientDisconnect again after a new user
has connected and been set as the current user with (*LocalBackend).SetCurrentUser().

Since ipn/ipnserver.Server does not allow simultaneous connections from different
Windows users and relies on the LocalBackend's current user, and since we already
reset the LocalBackend's state by calling ResetForClientDisconnect when the last
active request completes (indicating the server is idle and can accept connections
from any Windows user), it is unnecessary to track the last connected user on the
ipnserver.Server side or call ResetForClientDisconnect again when the user changes.

Additionally, the second call to ResetForClientDisconnect occurs after the new user
has been set as the current user, resetting the correct state for the new user
instead of the old state of the now-disconnected user, causing issues.

Updates tailscale/corp#25804

Signed-off-by: Nick Khyl <[email protected]>

ipn/ipnlocal/local.go

@@ -3620,7 +3620,7 @@ func (b *LocalBackend) shouldUploadServices() bool {
 }
 
 // SetCurrentUser is used to implement support for multi-user systems (only
-// Windows 2022-11-25). On such systems, the uid is used to determine which
+// Windows 2022-11-25). On such systems, the actor is used to determine which
 // user's state should be used. The current user is maintained by active
 // connections open to the backend.
 //
@@ -3634,11 +3634,8 @@ func (b *LocalBackend) shouldUploadServices() bool {
 // unattended mode. The user must disable unattended mode before the user can be
 // changed.
 //
-// On non-multi-user systems, the user should be set to nil.
-//
-// SetCurrentUser returns the ipn.WindowsUserID associated with the user
-// when successful.
-func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) (ipn.WindowsUserID, error) {
+// On non-multi-user systems, the actor should be set to nil.
+func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) {
 	var uid ipn.WindowsUserID
 	if actor != nil {
 		uid = actor.UserID()
@@ -3647,16 +3644,17 @@ func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) (ipn.WindowsUserID, e
 	unlock := b.lockAndGetUnlock()
 	defer unlock()
 
-	if b.pm.CurrentUserID() == uid {
-		return uid, nil
+	if actor != b.currentUser {
+		if c, ok := b.currentUser.(ipnauth.ActorCloser); ok {
+			c.Close()
+		}
+		b.currentUser = actor
 	}
-	b.pm.SetCurrentUserID(uid)
-	if c, ok := b.currentUser.(ipnauth.ActorCloser); ok {
-		c.Close()
+
+	if b.pm.CurrentUserID() != uid {
+		b.pm.SetCurrentUserID(uid)
+		b.resetForProfileChangeLockedOnEntry(unlock)
 	}
-	b.currentUser = actor
-	b.resetForProfileChangeLockedOnEntry(unlock)
-	return uid, nil
 }
 
 // CurrentUserForTest returns the current user and the associated WindowsUserID.

ipn/ipnserver/server.go

@@ -21,7 +21,6 @@ import (
 	"unicode"
 
 	"tailscale.com/envknob"
-	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnauth"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/ipn/localapi"
@@ -51,7 +50,6 @@ type Server struct {
 	// mu guards the fields that follow.
 	// lock order: mu, then LocalBackend.mu
 	mu            sync.Mutex
-	lastUserID    ipn.WindowsUserID // tracks last userid; on change, Reset state for paranoia
 	activeReqs    map[*http.Request]ipnauth.Actor
 	backendWaiter waiterSet // of LocalBackend waiters
 	zeroReqWaiter waiterSet // of blockUntilZeroConnections waiters
@@ -376,16 +374,6 @@ func (s *Server) addActiveHTTPRequest(req *http.Request, actor ipnauth.Actor) (o
 
 	lb := s.mustBackend()
 
-	// If the connected user changes, reset the backend server state to make
-	// sure node keys don't leak between users.
-	var doReset bool
-	defer func() {
-		if doReset {
-			s.logf("identity changed; resetting server")
-			lb.ResetForClientDisconnect()
-		}
-	}()
-
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -400,16 +388,7 @@ func (s *Server) addActiveHTTPRequest(req *http.Request, actor ipnauth.Actor) (o
 			// Tell the LocalBackend about the identity we're now running as,
 			// unless its the SYSTEM user. That user is not a real account and
 			// doesn't have a home directory.
-			uid, err := lb.SetCurrentUser(actor)
-			if err != nil {
-				return nil, err
-			}
-			if s.lastUserID != uid {
-				if s.lastUserID != "" {
-					doReset = true
-				}
-				s.lastUserID = uid
-			}
+			lb.SetCurrentUser(actor)
 		}
 	}
 

ipn/ipnserver/server_test.go

@@ -158,6 +158,35 @@ func TestIPNAlreadyInUseOnWindows(t *testing.T) {
 	server.checkCurrentUser(clientA.User)
 }
 
+func TestSequentialOSUserSwitchingOnWindows(t *testing.T) {
+	enableLogging := false
+	setGOOSForTest(t, "windows")
+
+	ctx := context.Background()
+	server := startDefaultTestIPNServer(t, ctx, enableLogging)
+
+	connectDisconnectAsUser := func(name string) {
+		// User connects and starts watching the IPN bus.
+		client := server.getClientAs(name)
+		watcher, cancelWatcher := client.WatchIPNBus(ctx, 0)
+		defer cancelWatcher()
+		go pumpIPNBus(watcher)
+
+		// It should be the current user from the LocalBackend's perspective...
+		server.checkCurrentUser(client.User)
+		// until it disconnects.
+		cancelWatcher()
+		server.blockWhileInUse(ctx)
+		// Now, the current user should be unset.
+		server.checkCurrentUser(nil)
+	}
+
+	// UserA logs in, uses Tailscale for a bit, then logs out.
+	connectDisconnectAsUser("UserA")
+	// Same for UserB.
+	connectDisconnectAsUser("UserB")
+}
+
 func setGOOSForTest(tb testing.TB, goos string) {
 	tb.Helper()
 	envknob.Setenv("TS_DEBUG_FAKE_GOOS", goos)