From f33f5f99c00f187644bf807b5336e2008e0228b7 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Mon, 13 Jan 2025 17:42:12 -0600 Subject: [PATCH] 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 --- ipn/ipnlocal/local.go | 26 ++++++++++++-------------- ipn/ipnserver/server.go | 23 +---------------------- ipn/ipnserver/server_test.go | 29 +++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 36 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 576f01b6b..c506f1376 100644 --- a/ipn/ipnlocal/local.go +++ b/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. diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 574d1a55c..c0e99f5d8 100644 --- a/ipn/ipnserver/server.go +++ b/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) } } diff --git a/ipn/ipnserver/server_test.go b/ipn/ipnserver/server_test.go index 8a9324fab..7f6131328 100644 --- a/ipn/ipnserver/server_test.go +++ b/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)