From 4f3315f3da8ed73034924d09d1ca97b677f61f1e Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@tailscale.com>
Date: Fri, 30 Apr 2021 04:29:22 -0400
Subject: [PATCH] ipnlocal: setting WantRunning with EditPrefs was special.
EditPrefs should be just a wrapper around the action of changing prefs,
but someone had added a side effect of calling Login() sometimes. The
side effect happened *after* running the state machine, which would
sometimes result in us going into NeedsLogin immediately before calling
cc.Login().
This manifested as the macOS app not being able to Connect if you
launched it with LoggedOut=false and WantRunning=false. Trying to
Connect() would sent us to the NeedsLogin state instead.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
---
ipn/ipnlocal/local.go | 31 +++++++++++++++++++++++++------
ipn/ipnlocal/state_test.go | 38 +++++++++++++-------------------------
2 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index 87996b08a..c5a9413ba 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -1438,14 +1438,12 @@ func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) (*ipn.Prefs, error) {
b.mu.Unlock()
return p1, nil
}
- cc := b.cc
b.logf("EditPrefs: %v", mp.Pretty())
b.setPrefsLockedOnEntry("EditPrefs", p1) // does a b.mu.Unlock
- if !p0.WantRunning && p1.WantRunning {
- b.logf("EditPrefs: transitioning to running; doing Login...")
- cc.Login(nil, controlclient.LoginDefault)
- }
+ // Note: don't perform any actions for the new prefs here. Not
+ // every prefs change goes through EditPrefs. Put your actions
+ // in setPrefsLocksOnEntry instead.
return p1, nil
}
@@ -1479,6 +1477,7 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
b.hostinfo = newHi
hostInfoChanged := !oldHi.Equal(newHi)
userID := b.userID
+ cc := b.cc
b.mu.Unlock()
@@ -1518,6 +1517,11 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
b.e.SetDERPMap(netMap.DERPMap)
}
+ if !oldp.WantRunning && newp.WantRunning {
+ b.logf("transitioning to running; doing Login...")
+ cc.Login(nil, controlclient.LoginDefault)
+ }
+
if oldp.WantRunning != newp.WantRunning {
b.stateMachine()
} else {
@@ -2145,8 +2149,23 @@ func (b *LocalBackend) nextState() ipn.State {
// Auth was interrupted or waiting for URL visit,
// so it won't proceed without human help.
return ipn.NeedsLogin
+ } else if state == ipn.Stopped {
+ // If we were already in the Stopped state, then
+ // we can assume auth is in good shape (or we would
+ // have been in NeedsLogin), so transition to Starting
+ // right away.
+ return ipn.Starting
+ } else if state == ipn.NoState {
+ // Our first time connecting to control, and we
+ // don't know if we'll NeedsLogin or not yet.
+ // UIs should print "Loading..." in this state.
+ return ipn.NoState
+ } else if state == ipn.Starting ||
+ state == ipn.Running ||
+ state == ipn.NeedsLogin {
+ return state
} else {
- // Auth or map request needs to finish
+ b.logf("unexpected no-netmap state transition for %v", state)
return state
}
case !wantRunning:
diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go
index a0e73f294..a2c1b2e9a 100644
--- a/ipn/ipnlocal/state_test.go
+++ b/ipn/ipnlocal/state_test.go
@@ -106,20 +106,24 @@ func (cc *mockControl) SetStatusFunc(fn func(controlclient.Status)) {
cc.statusFunc = fn
}
-func (cc *mockControl) populateKeys() {
+func (cc *mockControl) populateKeys() (newKeys bool) {
cc.mu.Lock()
defer cc.mu.Unlock()
if cc.machineKey.IsZero() {
cc.logf("Copying machineKey.")
cc.machineKey, _ = cc.opts.GetMachinePrivateKey()
+ newKeys = true
}
if cc.persist.PrivateNodeKey.IsZero() {
cc.logf("Generating a new nodekey.")
cc.persist.OldPrivateNodeKey = cc.persist.PrivateNodeKey
cc.persist.PrivateNodeKey, _ = wgkey.NewPrivate()
+ newKeys = true
}
+
+ return newKeys
}
// send publishes a controlclient.Status notification upstream.
@@ -191,7 +195,11 @@ func (cc *mockControl) Shutdown() {
func (cc *mockControl) Login(t *tailcfg.Oauth2Token, flags controlclient.LoginFlags) {
cc.logf("Login token=%v flags=%v", t, flags)
cc.called("Login")
- cc.populateKeys()
+ newKeys := cc.populateKeys()
+
+ interact := (flags & controlclient.LoginInteractive) != 0
+ cc.logf("Login: interact=%v newKeys=%v", interact, newKeys)
+ cc.setAuthBlocked(interact || newKeys)
}
func (cc *mockControl) StartLogout() {
@@ -526,7 +534,7 @@ func TestStateMachine(t *testing.T) {
nn := notifies.drain(2)
// BUG: UpdateEndpoints isn't needed here.
// BUG: Login isn't needed here. We never logged out.
- assert.Equal([]string{"unpause", "UpdateEndpoints", "Login"}, cc.getCalls())
+ assert.Equal([]string{"Login", "unpause", "UpdateEndpoints"}, cc.getCalls())
// BUG: I would expect Prefs to change first, and state after.
assert.NotNil(nn[0].State)
assert.NotNil(nn[1].Prefs)
@@ -752,26 +760,6 @@ func TestStateMachine(t *testing.T) {
assert.Equal(ipn.Stopped, *nn[1].State)
}
- // This time, let's have the control server spontaneously
- // authenticate us, even though you didn't ask. That wouldn't
- // normally happen, but the state machine should be able to handle
- // it. (Normally we should call b.Login() first, but we already
- // tested that up above.)
- t.Logf("\n\nLoginFinished4")
- notifies.expect(1)
- cc.setAuthBlocked(false)
- cc.send(nil, "", true, &netmap.NetworkMap{
- MachineStatus: tailcfg.MachineAuthorized,
- })
- {
- nn := notifies.drain(1)
- // NOTE: No call to Login() here since !WantRunning at
- // startup time.
- assert.Equal([]string{}, cc.getCalls())
- assert.NotNil(nn[0].LoginFinished)
- // State is still ipn.Stopped, as expected.
- }
-
// Request connection.
// The state machine didn't call Login() earlier, so now it needs to.
t.Logf("\n\nWantRunning4 -> true")
@@ -782,7 +770,7 @@ func TestStateMachine(t *testing.T) {
})
{
nn := notifies.drain(2)
- assert.Equal([]string{"unpause", "Login"}, cc.getCalls())
+ assert.Equal([]string{"Login", "unpause"}, cc.getCalls())
// BUG: I would expect Prefs to change first, and state after.
assert.NotNil(nn[0].State)
assert.NotNil(nn[1].Prefs)
@@ -804,9 +792,9 @@ func TestStateMachine(t *testing.T) {
nn := notifies.drain(2)
assert.Equal([]string{}, cc.getCalls())
assert.NotNil(nn[0].Prefs)
- assert.NotNil(nn[1].State)
assert.Equal(false, nn[0].Prefs.LoggedOut)
assert.Equal(true, nn[0].Prefs.WantRunning)
+ assert.NotNil(nn[1].State)
assert.Equal(ipn.Starting, *nn[1].State)
assert.Equal(ipn.Starting, b.State())
}