From a8766815a43d8bd415032346f869f0ca54a35dbf Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Wed, 28 Apr 2021 09:41:55 -0400 Subject: [PATCH] ipn: cc.Login(noninteractive) at start even if WantRunning=false. We were not properly initializing controlclient at startup, if Prefs.WantRunning was initially false. This originally would cause the state machine to get stuck in NewState, but an earlier erroneous change tried to make it get stuck in NeedsLogin instead, which is incorrect; NeedsLogin means we need *interactive* login, which is not true. The correct fix is to not get it stuck. While we're here: - Add a bunch of comments to explain how these work. - Unexport the Status.state var from controlclient. There has not been any need for outsiders to inspect it for a long time; it's needed only by unit tests. - Remove a very suspicious check from AuthCantContinue that its self pointer != nil. - Remove an extremely suspicious "defer b.stateMachine()" from Start(). There is no need to run the state machine when nothing has happened yet; any apparent need for this is a sign of some other bug. Fixes tailscale/corp#1660 (iOS app startup bug) Signed-off-by: Avery Pennarun --- control/controlclient/auto.go | 29 ++++++++++++++++-------- ipn/backend.go | 14 ++++++------ ipn/ipnlocal/local.go | 10 ++++----- ipn/ipnlocal/local_test.go | 42 ----------------------------------- 4 files changed, 32 insertions(+), 63 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index e0f17e172..7e48a1e70 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -67,13 +67,17 @@ func (s State) String() string { type Status struct { _ structs.Incomparable - LoginFinished *empty.Message + LoginFinished *empty.Message // nonempty when login finishes Err string - URL string + URL string // interactive URL to visit to finish logging in Persist *persist.Persist // locally persisted configuration NetMap *netmap.NetworkMap // server-pushed configuration Hostinfo *tailcfg.Hostinfo // current Hostinfo data - State State + + // the internal state machine is not exposed outside this package. + // This field is here mainly for checking the flow during unit + // tests. + state State } // Equal reports whether s and s2 are equal. @@ -88,7 +92,7 @@ func (s *Status) Equal(s2 *Status) bool { reflect.DeepEqual(s.Persist, s2.Persist) && reflect.DeepEqual(s.NetMap, s2.NetMap) && reflect.DeepEqual(s.Hostinfo, s2.Hostinfo) && - s.State == s2.State + s.state == s2.state } func (s Status) String() string { @@ -96,7 +100,7 @@ func (s Status) String() string { if err != nil { panic(err) } - return s.State.String() + " " + string(b) + return s.state.String() + " " + string(b) } type LoginGoal struct { @@ -596,10 +600,15 @@ func (c *Client) mapRoutine() { } } +// AuthCantContinue() returns true if the auth state machine is ready to try +// logging in, but doesn't have enough information to do so. +// +// After freshly creating the controlclient, this is always true, because +// you haven't called Login() yet (even non-interactively). After you call +// Login(), this function will return false for a while. It may become true +// again after a Status{URL!=""} is emitted, indicating that you need +// to visit an interactive login URL. func (c *Client) AuthCantContinue() bool { - if c == nil { - return true - } c.mu.Lock() defer c.mu.Unlock() @@ -670,7 +679,7 @@ func (c *Client) sendStatus(who string, err error, url string, nm *netmap.Networ Persist: p, NetMap: nm, Hostinfo: hi, - State: state, + state: state, } if err != nil { new.Err = err.Error() @@ -706,6 +715,7 @@ func (c *Client) StartLogout() { wantLoggedIn: false, } c.mu.Unlock() + c.cancelAuth() } @@ -720,6 +730,7 @@ func (c *Client) Logout(ctx context.Context) error { loggedOutResult: errc, } c.mu.Unlock() + c.cancelAuth() timer := time.NewTimer(10 * time.Second) diff --git a/ipn/backend.go b/ipn/backend.go index b5aa78c2d..89356c30b 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -17,13 +17,13 @@ import ( type State int const ( - NoState = State(iota) - InUseOtherUser - NeedsLogin - NeedsMachineAuth - Stopped - Starting - Running + NoState = State(iota) + InUseOtherUser // backend is in use by another user + NeedsLogin // an *interactive* user login is required; URL available + NeedsMachineAuth // logged in, but machine key needs approval + Stopped // user requested WantRunning=false + Starting // in transition from stopped to running + Running // network is connected ) // GoogleIDToken Type is the tailcfg.Oauth2Token.TokenType for the Google diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index b04061b7f..503df874d 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -627,7 +627,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { return errors.New("no state key or prefs provided") } - defer b.stateMachine() if opts.Prefs != nil { b.logf("Start: %v", opts.Prefs.Pretty()) } else { @@ -786,9 +785,10 @@ func (b *LocalBackend) Start(opts ipn.Options) error { b.send(ipn.Notify{BackendLogID: &blid}) b.send(ipn.Notify{Prefs: prefs}) - if wantRunning { - cc.Login(nil, controlclient.LoginDefault) - } + // Even if we don't want to be *connected* to tailscale right now, + // we can attempt the non-interactive login process right away, + // which will make connecting fast later. + cc.Login(nil, controlclient.LoginDefault) return nil } @@ -2105,7 +2105,7 @@ func (b *LocalBackend) nextState() ipn.State { switch { case netMap == nil: - if cc.AuthCantContinue() { + if cc.AuthCantContinue() && b.authURL != "" { // Auth was interrupted or waiting for URL visit, // so it won't proceed without human help. return ipn.NeedsLogin diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index fa2f7cef2..733a7a066 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -5,20 +5,15 @@ package ipnlocal import ( - "bytes" - "fmt" "net/http" "reflect" - "sync" "testing" "inet.af/netaddr" - "tailscale.com/ipn" "tailscale.com/net/interfaces" "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" "tailscale.com/types/netmap" - "tailscale.com/wgengine" "tailscale.com/wgengine/wgcfg" ) @@ -433,40 +428,3 @@ func (panicOnUseTransport) RoundTrip(*http.Request) (*http.Response, error) { } var nl = []byte("\n") - -func TestStartsInNeedsLoginState(t *testing.T) { - var ( - mu sync.Mutex - logBuf bytes.Buffer - ) - logf := func(format string, a ...interface{}) { - mu.Lock() - defer mu.Unlock() - fmt.Fprintf(&logBuf, format, a...) - if !bytes.HasSuffix(logBuf.Bytes(), nl) { - logBuf.Write(nl) - } - } - store := new(ipn.MemoryStore) - eng, err := wgengine.NewFakeUserspaceEngine(logf, 0) - if err != nil { - t.Fatalf("NewFakeUserspaceEngine: %v", err) - } - lb, err := NewLocalBackend(logf, "logid", store, eng) - if err != nil { - t.Fatalf("NewLocalBackend: %v", err) - } - - lb.SetHTTPTestClient(&http.Client{ - Transport: panicOnUseTransport{}, // validate we don't send HTTP requests - }) - - if err := lb.Start(ipn.Options{ - StateKey: ipn.GlobalDaemonStateKey, - }); err != nil { - t.Fatalf("Start: %v", err) - } - if st := lb.State(); st != ipn.NeedsLogin { - t.Errorf("State = %v; want NeedsLogin", st) - } -}