From 781b7717b6e5d0313a37bb8f0d833cac7b5306f2 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Wed, 28 Aug 2024 15:19:27 -0500 Subject: [PATCH] ipn/ipnlocal: always send auth URL notifications when a user requests interactive login This is a backport of #13297 without extra stuff we need for multi-user improvements Fixes #13296 Signed-off-by: Nick Khyl --- ipn/ipnlocal/local.go | 99 +++++++++++++++++++++++--------------- ipn/ipnlocal/state_test.go | 9 +++- 2 files changed, 67 insertions(+), 41 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 3da12d7cc..b9f026241 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -263,6 +263,7 @@ type LocalBackend struct { keyExpired bool authURL string // non-empty if not Running authURLTime time.Time // when the authURL was received from the control server + interact bool // indicates whether a user requested interactive login egg bool prevIfState *netmon.State peerAPIServer *peerAPIServer // or nil @@ -1323,11 +1324,8 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control prefs.Persist = st.Persist.AsStruct() } } - if st.URL != "" { - b.authURL = st.URL - b.authURLTime = b.clock.Now() - } if (wasBlocked || b.seamlessRenewalEnabled()) && st.LoginFinished() { + b.resetAuthURLLocked() // Interactive login finished successfully (URL visited). // After an interactive login, the user always wants // WantRunning. @@ -1452,7 +1450,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control } if st.URL != "" { b.logf("Received auth URL: %.20v...", st.URL) - b.popBrowserAuthNow() + b.setAuthURL(st.URL) } b.stateMachine() // This is currently (2020-07-28) necessary; conditionally disabling it is fragile! @@ -2644,27 +2642,11 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa // TODO(marwan-at-work): streaming background logs? defer b.DeleteForegroundSession(sessionID) - var lastURLPop string // to dup suppress URL popups for { select { case <-ctx.Done(): return case n, ok := <-ch: - // URLs flow into Notify.BrowseToURL via two means: - // 1. From MapResponse.PopBrowserURL, which already says they're dup - // suppressed if identical, and that's done by the controlclient, - // so this added later adds nothing. - // - // 2. From the controlclient auth routes, on register. This makes sure - // we don't tell clients (mac, windows, android) to pop the same URL - // multiple times. - if n != nil && n.BrowseToURL != nil { - if v := *n.BrowseToURL; v == lastURLPop { - n.BrowseToURL = nil - } else { - lastURLPop = v - } - } if !ok || !fn(n) { return } @@ -2795,20 +2777,46 @@ func (b *LocalBackend) sendFileNotify() { b.send(n) } -// popBrowserAuthNow shuts down the data plane and sends an auth URL -// to the connected frontend, if any. -func (b *LocalBackend) popBrowserAuthNow() { +// setAuthURL sets the authURL and triggers [LocalBackend.popBrowserAuthNow] if the URL has changed. +// This method is called when a new authURL is received from the control plane, meaning that either a user +// has started a new interactive login (e.g., by running `tailscale login` or clicking Login in the GUI), +// or the control plane was unable to authenticate this node non-interactively (e.g., due to key expiration). +// b.interact indicates whether an interactive login is in progress. +func (b *LocalBackend) setAuthURL(url string) { + var popBrowser, keyExpired bool + b.mu.Lock() - url := b.authURL - expired := b.keyExpired + switch { + case url == "": + b.resetAuthURLLocked() + case b.authURL != url: + b.authURL = url + b.authURLTime = b.clock.Now() + // Always open the browser if the URL has changed. + // This includes the transition from no URL -> some URL. + popBrowser = true + default: + // Otherwise, only open it if the user explicitly requests interactive login. + popBrowser = b.interact + } + keyExpired = b.keyExpired + b.interact = false // consume the StartLoginInteractive call, if any, that caused the control plane to send us this URL b.mu.Unlock() - b.logf("popBrowserAuthNow: url=%v, key-expired=%v, seamless-key-renewal=%v", url != "", expired, b.seamlessRenewalEnabled()) + if popBrowser { + b.popBrowserAuthNow(url, keyExpired) + } +} + +// popBrowserAuthNow shuts down the data plane and sends an auth URL +// to the connected frontend, if any. +func (b *LocalBackend) popBrowserAuthNow(url string, keyExpired bool) { + b.logf("popBrowserAuthNow: url=%v, key-expired=%v, seamless-key-renewal=%v", url != "", keyExpired, b.seamlessRenewalEnabled()) // Deconfigure the local network data plane if: // - seamless key renewal is not enabled; // - key is expired (in which case tailnet connectivity is down anyway). - if !b.seamlessRenewalEnabled() || expired { + if !b.seamlessRenewalEnabled() || keyExpired { b.blockEngineUpdates(true) b.stopEngineAndWait() } @@ -3134,16 +3142,25 @@ func (b *LocalBackend) StartLoginInteractive(ctx context.Context) error { panic("LocalBackend.assertClient: b.cc == nil") } url := b.authURL + keyExpired := b.keyExpired timeSinceAuthURLCreated := b.clock.Since(b.authURLTime) - cc := b.cc - b.mu.Unlock() - b.logf("StartLoginInteractive: url=%v", url != "") - // Only use an authURL if it was sent down from control in the last // 6 days and 23 hours. Avoids using a stale URL that is no longer valid // server-side. Server-side URLs expire after 7 days. - if url != "" && timeSinceAuthURLCreated < ((7*24*time.Hour)-(1*time.Hour)) { - b.popBrowserAuthNow() + hasValidURL := url != "" && timeSinceAuthURLCreated < ((7*24*time.Hour)-(1*time.Hour)) + if !hasValidURL { + // A user wants to log in interactively, but we don't have a valid authURL. + // Set a flag to indicate that interactive login is in progress, forcing + // a BrowseToURL notification once the authURL becomes available. + b.interact = true + } + cc := b.cc + b.mu.Unlock() + + b.logf("StartLoginInteractive: url=%v", hasValidURL) + + if hasValidURL { + b.popBrowserAuthNow(url, keyExpired) } else { cc.Login(b.loginFlags | controlclient.LoginInteractive) } @@ -4678,8 +4695,7 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State, unlock unlock activeLogin := b.activeLogin authURL := b.authURL if newState == ipn.Running { - b.authURL = "" - b.authURLTime = time.Time{} + b.resetAuthURLLocked() // Start a captive portal detection loop if none has been // started. Create a new context if none is present, since it @@ -4961,7 +4977,7 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { return nil } - b.authURL = "" + b.resetAuthURLLocked() // When we clear the control client, stop any outstanding netmap expiry // timer; synthesizing a new netmap while we don't have a control @@ -4981,6 +4997,12 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { return prev } +func (b *LocalBackend) resetAuthURLLocked() { + b.authURL = "" + b.authURLTime = time.Time{} + b.interact = false +} + // ResetForClientDisconnect resets the backend for GUI clients running // in interactive (non-headless) mode. This is currently used only by // Windows. This causes all state to be cleared, lest an unrelated user @@ -5006,8 +5028,7 @@ func (b *LocalBackend) ResetForClientDisconnect() { b.currentUser = nil } b.keyExpired = false - b.authURL = "" - b.authURLTime = time.Time{} + b.resetAuthURLLocked() b.activeLogin = "" b.resetDialPlan() b.setAtomicValuesFromPrefsLocked(ipn.PrefsView{}) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index d9ed608d8..0b20539df 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -437,10 +437,13 @@ func TestStateMachine(t *testing.T) { // ask control to do anything. Instead backend will emit an event // indicating that the UI should browse to the given URL. t.Logf("\n\nLogin (interactive)") - notifies.expect(0) + notifies.expect(1) b.StartLoginInteractive(context.Background()) { + nn := notifies.drain(1) cc.assertCalls() + c.Assert(nn[0].BrowseToURL, qt.IsNotNil) + c.Assert(url1, qt.Equals, *nn[0].BrowseToURL) c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) } @@ -450,11 +453,13 @@ func TestStateMachine(t *testing.T) { // the login URL expired. If they start another interactive login, // we must always get a *new* login URL first. t.Logf("\n\nLogin2 (interactive)") + b.authURLTime = time.Now().Add(-time.Hour * 24 * 7) // simulate URL expiration notifies.expect(0) b.StartLoginInteractive(context.Background()) { + notifies.drain(0) // backend asks control for another login sequence - cc.assertCalls() + cc.assertCalls("Login") c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) }