From eabc62a9ddc45646bf55f20928832b6c4e4ad2d8 Mon Sep 17 00:00:00 2001 From: James 'zofrex' Sanderson Date: Tue, 7 Oct 2025 11:52:41 +0100 Subject: [PATCH] ipn/ipnlocal: don't send LoginFinished unless auth was in progress (#17266) Before we introduced seamless, the "blocked" state was used to track: * Whether a login was required for connectivity, and therefore we should keep the engine deconfigured until that happened * Whether authentication was in progress "blocked" would stop authReconfig from running. We want this when a login is required: if your key has expired we want to deconfigure the engine and keep it down, so that you don't keep using exit nodes (which won't work because your key has expired). Taking the engine down while auth was in progress was undesirable, so we don't do that with seamless renewal. However, not entering the "blocked" state meant that we needed to change the logic for when to send LoginFinished on the IPN bus after seeing StateAuthenticated from the controlclient. Initially we changed the "if blocked" check to "if blocked or seamless is enabled" which was correct in other places. In this place however, it introduced a bug: we are sending LoginFinished every time we see StateAuthenticated, which happens even on a down & up, or a profile switch. This in turn made it harder for UI clients to track when authentication is complete. Instead we should only send it out if we were blocked (i.e. seamless is disabled, or our key expired) or an auth was in progress. Updates tailscale/corp#31476 Updates tailscale/corp#32645 Fixes #17363 Signed-off-by: James Sanderson --- ipn/ipnlocal/local.go | 3 ++- ipn/ipnlocal/state_test.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c8b49de75..c07cc42a1 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1600,6 +1600,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control } wasBlocked := b.blocked + authWasInProgress := b.authURL != "" keyExpiryExtended := false if st.NetMap != nil { wasExpired := b.keyExpired @@ -1617,7 +1618,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control b.blockEngineUpdates(false) } - if st.LoginFinished() && (wasBlocked || b.seamlessRenewalEnabled()) { + if st.LoginFinished() && (wasBlocked || authWasInProgress) { if wasBlocked { // Auth completed, unblock the engine b.blockEngineUpdates(false) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index d773f7227..a4b9ba1f4 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -348,6 +348,14 @@ func (b *LocalBackend) nonInteractiveLoginForStateTest() { // predictable, but maybe a bit less thorough. This is more of an overall // state machine test than a test of the wgengine+magicsock integration. func TestStateMachine(t *testing.T) { + runTestStateMachine(t, false) +} + +func TestStateMachineSeamless(t *testing.T) { + runTestStateMachine(t, true) +} + +func runTestStateMachine(t *testing.T, seamless bool) { envknob.Setenv("TAILSCALE_USE_WIP_CODE", "1") defer envknob.Setenv("TAILSCALE_USE_WIP_CODE", "") c := qt.New(t) @@ -545,6 +553,13 @@ func TestStateMachine(t *testing.T) { notifies.expect(3) cc.persist.UserProfile.LoginName = "user1" cc.persist.NodeID = "node1" + + // even if seamless is being enabled by default rather than by policy, this is + // the point where it will first get enabled. + if seamless { + sys.ControlKnobs().SeamlessKeyRenewal.Store(true) + } + cc.send(nil, "", true, &netmap.NetworkMap{}) { nn := notifies.drain(3)