ipn, ipn/ipnserver: add IPN state for server in use, handle explicitly

On Windows, we were previously treating a server used by different
users as a fatal error, which meant the second user (upon starting
Tailscale, explicitly or via Start Up programs) got an invasive error
message dialog.

Instead, give it its own IPN state and change the Notify.ErrMessage to
be details in that state. Then the Windows GUI can be less aggresive
about that happening.

Also,

* wait to close the IPN connection until the server ownership state
  changes so the GUI doesn't need to repeatedly reconnect to discover
  changes.

* fix a bug discovered during testing: on system reboot, the
  ipnserver's serverModeUser was getting cleared while the state
  transitioned from Unknown to Running. Instead, track 'inServerMode'
  explicitly and remove the old accessor method which was error prone.

* fix a rare bug where the client could start up and set the server
  mode prefs in its Start call and we wouldn't persist that to the
  StateStore storage's prefs start key. (Previously it was only via a
  prefs toggle at runtime)
pull/892/head
Brad Fitzpatrick 4 years ago
parent 437142daa5
commit 20a357b386

@ -21,6 +21,7 @@ type State int
const ( const (
NoState = State(iota) NoState = State(iota)
InUseOtherUser
NeedsLogin NeedsLogin
NeedsMachineAuth NeedsMachineAuth
Stopped Stopped
@ -33,8 +34,14 @@ const (
const GoogleIDTokenType = "ts_android_google_login" const GoogleIDTokenType = "ts_android_google_login"
func (s State) String() string { func (s State) String() string {
return [...]string{"NoState", "NeedsLogin", "NeedsMachineAuth", return [...]string{
"Stopped", "Starting", "Running"}[s] "NoState",
"InUseOtherUser",
"NeedsLogin",
"NeedsMachineAuth",
"Stopped",
"Starting",
"Running"}[s]
} }
// EngineStatus contains WireGuard engine stats. // EngineStatus contains WireGuard engine stats.
@ -53,7 +60,7 @@ type EngineStatus struct {
type Notify struct { type Notify struct {
_ structs.Incomparable _ structs.Incomparable
Version string // version number of IPN backend Version string // version number of IPN backend
ErrMessage *string // critical error message, if any ErrMessage *string // critical error message, if any; for InUseOtherUser, the details
LoginFinished *empty.Message // event: non-nil when login process succeeded LoginFinished *empty.Message // event: non-nil when login process succeeded
State *State // current IPN state has changed State *State // current IPN state has changed
Prefs *Prefs // preferences were changed Prefs *Prefs // preferences were changed

@ -10,6 +10,7 @@ import (
"errors" "errors"
"fmt" "fmt"
"io" "io"
"io/ioutil"
"log" "log"
"net" "net"
"net/http" "net/http"
@ -102,10 +103,11 @@ type server struct {
bs *ipn.BackendServer bs *ipn.BackendServer
mu sync.Mutex mu sync.Mutex
serverModeUser *user.User // or nil if not in server mode serverModeUser *user.User // or nil if not in server mode
lastUserID string // tracks last userid; on change, Reset state for paranoia lastUserID string // tracks last userid; on change, Reset state for paranoia
allClients map[net.Conn]connIdentity // HTTP or IPN allClients map[net.Conn]connIdentity // HTTP or IPN
clients map[net.Conn]bool // subset of allClients; only IPN protocol clients map[net.Conn]bool // subset of allClients; only IPN protocol
disconnectSub map[chan<- struct{}]struct{} // keys are subscribers of disconnects
} }
// connIdentity represents the owner of a localhost TCP connection. // connIdentity represents the owner of a localhost TCP connection.
@ -177,6 +179,42 @@ func (s *server) lookupUserFromID(uid string) (*user.User, error) {
return u, err return u, err
} }
// blockWhileInUse blocks while until either a Read from conn fails
// (i.e. it's closed) or until the server is able to accept ci as a
// user.
func (s *server) blockWhileInUse(conn io.Reader, ci connIdentity) {
s.logf("blocking client while server in use; connIdentity=%v", ci)
connDone := make(chan struct{})
go func() {
io.Copy(ioutil.Discard, conn)
close(connDone)
}()
ch := make(chan struct{}, 1)
s.registerDisconnectSub(ch, true)
defer s.registerDisconnectSub(ch, false)
for {
select {
case <-connDone:
s.logf("blocked client Read completed; connIdentity=%v", ci)
return
case <-ch:
s.mu.Lock()
err := s.checkConnIdentityLocked(ci)
s.mu.Unlock()
if err == nil {
s.logf("unblocking client, server is free; connIdentity=%v", ci)
// Server is now available again for a new user.
// TODO(bradfitz): keep this connection alive. But for
// now just return and have our caller close the connection
// (which unblocks the io.Copy goroutine we started above)
// and then the client (e.g. Windows) will reconnect and
// discover that it works.
return
}
}
}
}
func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) {
// First see if it's an HTTP request. // First see if it's an HTTP request.
br := bufio.NewReader(c) br := bufio.NewReader(c)
@ -195,8 +233,14 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) {
defer c.Close() defer c.Close()
serverToClient := func(b []byte) { ipn.WriteMsg(c, b) } serverToClient := func(b []byte) { ipn.WriteMsg(c, b) }
bs := ipn.NewBackendServer(logf, nil, serverToClient) bs := ipn.NewBackendServer(logf, nil, serverToClient)
bs.SendErrorMessage(err.Error()) _, occupied := err.(inUseOtherUserError)
time.Sleep(time.Second) if occupied {
bs.SendInUseOtherUserErrorMessage(err.Error())
s.blockWhileInUse(c, ci)
} else {
bs.SendErrorMessage(err.Error())
time.Sleep(time.Second)
}
return return
} }
@ -243,6 +287,58 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) {
} }
} }
// inUseOtherUserError is the error type for when the server is in use
// by a different local user.
type inUseOtherUserError struct{ error }
func (e inUseOtherUserError) Unwrap() error { return e.error }
// checkConnIdentityLocked checks whether the provided identity is
// allowed to connect to the server.
//
// The returned error, when non-nil, will be of type inUseOtherUserError.
//
// s.mu must be held.
func (s *server) checkConnIdentityLocked(ci connIdentity) error {
// If clients are already connected, verify they're the same user.
// This mostly matters on Windows at the moment.
if len(s.allClients) > 0 {
var active connIdentity
for _, active = range s.allClients {
break
}
if ci.UserID != active.UserID {
//lint:ignore ST1005 we want to capitalize Tailscale here
return inUseOtherUserError{fmt.Errorf("Tailscale already in use by %s, pid %d", active.User.Username, active.Pid)}
}
}
if su := s.serverModeUser; su != nil && ci.UserID != su.Uid {
//lint:ignore ST1005 we want to capitalize Tailscale here
return inUseOtherUserError{fmt.Errorf("Tailscale running as %s. Access denied.", su.Username)}
}
return nil
}
// registerDisconnectSub adds ch as a subscribe to connection disconnect
// events. If add is false, the subscriber is removed.
func (s *server) registerDisconnectSub(ch chan<- struct{}, add bool) {
s.mu.Lock()
defer s.mu.Unlock()
if add {
if s.disconnectSub == nil {
s.disconnectSub = make(map[chan<- struct{}]struct{})
}
s.disconnectSub[ch] = struct{}{}
} else {
delete(s.disconnectSub, ch)
}
}
// addConn adds c to the server's list of clients.
//
// If the returned error is of type inUseOtherUserError then the
// returned connIdentity is also valid.
func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) { func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) {
ci, err = s.getConnIdentity(c) ci, err = s.getConnIdentity(c)
if err != nil { if err != nil {
@ -271,21 +367,8 @@ func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) {
s.allClients = map[net.Conn]connIdentity{} s.allClients = map[net.Conn]connIdentity{}
} }
// If clients are already connected, verify they're the same user. if err := s.checkConnIdentityLocked(ci); err != nil {
// This mostly matters on Windows at the moment. return ci, err
if len(s.allClients) > 0 {
var active connIdentity
for _, active = range s.allClients {
break
}
if ci.UserID != active.UserID {
//lint:ignore ST1005 we want to capitalize Tailscale here
return ci, fmt.Errorf("Tailscale already in use by %s, pid %d", active.User.Username, active.Pid)
}
}
if su := s.serverModeUser; su != nil && ci.UserID != su.Uid {
//lint:ignore ST1005 we want to capitalize Tailscale here
return ci, fmt.Errorf("Tailscale running as %s. Access denied.", su.Username)
} }
if !isHTTP { if !isHTTP {
@ -307,10 +390,16 @@ func (s *server) removeAndCloseConn(c net.Conn) {
delete(s.clients, c) delete(s.clients, c)
delete(s.allClients, c) delete(s.allClients, c)
remain := len(s.allClients) remain := len(s.allClients)
for sub := range s.disconnectSub {
select {
case sub <- struct{}{}:
default:
}
}
s.mu.Unlock() s.mu.Unlock()
if remain == 0 && s.resetOnZero { if remain == 0 && s.resetOnZero {
if s.b.RunningAndDaemonForced() { if s.b.InServerMode() {
s.logf("client disconnected; staying alive in server mode") s.logf("client disconnected; staying alive in server mode")
} else { } else {
s.logf("client disconnected; stopping server") s.logf("client disconnected; stopping server")
@ -358,7 +447,7 @@ func (s *server) setServerModeUserLocked() {
} }
func (s *server) writeToClients(b []byte) { func (s *server) writeToClients(b []byte) {
inServerMode := s.b.RunningAndDaemonForced() inServerMode := s.b.InServerMode()
s.mu.Lock() s.mu.Lock()
defer s.mu.Unlock() defer s.mu.Unlock()
@ -456,7 +545,7 @@ func Run(ctx context.Context, logf logger.Logf, logid string, getEngine func() (
key := string(autoStartKey) key := string(autoStartKey)
if strings.HasPrefix(key, "user-") { if strings.HasPrefix(key, "user-") {
uid := strings.TrimPrefix(key, "user-") uid := strings.TrimPrefix(key, "user-")
u, err := user.LookupId(uid) u, err := server.lookupUserFromID(uid)
if err != nil { if err != nil {
logf("ipnserver: found server mode auto-start key %q; failed to load user: %v", key, err) logf("ipnserver: found server mode auto-start key %q; failed to load user: %v", key, err)
} else { } else {

@ -81,6 +81,7 @@ type LocalBackend struct {
stateKey StateKey // computed in part from user-provided value stateKey StateKey // computed in part from user-provided value
userID string // current controlling user ID (for Windows, primarily) userID string // current controlling user ID (for Windows, primarily)
prefs *Prefs prefs *Prefs
inServerMode bool
machinePrivKey wgcfg.PrivateKey machinePrivKey wgcfg.PrivateKey
state State state State
// hostinfo is mutated in-place while mu is held. // hostinfo is mutated in-place while mu is held.
@ -414,6 +415,7 @@ func (b *LocalBackend) Start(opts Options) error {
return fmt.Errorf("loading requested state: %v", err) return fmt.Errorf("loading requested state: %v", err)
} }
b.inServerMode = b.stateKey != ""
b.serverURL = b.prefs.ControlURL b.serverURL = b.prefs.ControlURL
hostinfo.RoutableIPs = append(hostinfo.RoutableIPs, b.prefs.AdvertiseRoutes...) hostinfo.RoutableIPs = append(hostinfo.RoutableIPs, b.prefs.AdvertiseRoutes...)
hostinfo.RequestTags = append(hostinfo.RequestTags, b.prefs.AdvertiseTags...) hostinfo.RequestTags = append(hostinfo.RequestTags, b.prefs.AdvertiseTags...)
@ -766,6 +768,35 @@ func (b *LocalBackend) initMachineKeyLocked() (err error) {
return nil return nil
} }
// writeServerModeStartState stores the ServerModeStartKey value based on the current
// user and prefs. If userID is blank or prefs is blank, no work is done.
//
// b.mu may either be held or not.
func (b *LocalBackend) writeServerModeStartState(userID string, prefs *Prefs) {
if userID == "" || prefs == nil {
return
}
if prefs.ForceDaemon {
stateKey := StateKey("user-" + userID)
if err := b.store.WriteState(ServerModeStartKey, []byte(stateKey)); err != nil {
b.logf("WriteState error: %v", err)
}
// It's important we do this here too, even if it looks
// redundant with the one in the 'if stateKey != ""'
// check block above. That one won't fire in the case
// where the Windows client started up in client mode.
// This happens when we transition into server mode:
if err := b.store.WriteState(stateKey, prefs.ToBytes()); err != nil {
b.logf("WriteState error: %v", err)
}
} else {
if err := b.store.WriteState(ServerModeStartKey, nil); err != nil {
b.logf("WriteState error: %v", err)
}
}
}
// loadStateLocked sets b.prefs and b.stateKey based on a complex // loadStateLocked sets b.prefs and b.stateKey based on a complex
// combination of key, prefs, and legacyPath. b.mu must be held when // combination of key, prefs, and legacyPath. b.mu must be held when
// calling. // calling.
@ -786,6 +817,7 @@ func (b *LocalBackend) loadStateLocked(key StateKey, prefs *Prefs, legacyPath st
return fmt.Errorf("initMachineKeyLocked: %w", err) return fmt.Errorf("initMachineKeyLocked: %w", err)
} }
b.stateKey = "" b.stateKey = ""
b.writeServerModeStartState(b.userID, b.prefs)
return nil return nil
} }
@ -843,13 +875,10 @@ func (b *LocalBackend) State() State {
return b.state return b.state
} }
// RunningAndDaemonForced reports whether the backend is currently func (b *LocalBackend) InServerMode() bool {
// running and the preferences say that Tailscale should run in
// "server mode" (ForceDaemon).
func (b *LocalBackend) RunningAndDaemonForced() bool {
b.mu.Lock() b.mu.Lock()
defer b.mu.Unlock() defer b.mu.Unlock()
return b.state == Running && b.prefs != nil && b.prefs.ForceDaemon return b.inServerMode
} }
// getEngineStatus returns a copy of b.engineStatus. // getEngineStatus returns a copy of b.engineStatus.
@ -1001,6 +1030,7 @@ func (b *LocalBackend) SetPrefs(newp *Prefs) {
oldp := b.prefs oldp := b.prefs
newp.Persist = oldp.Persist // caller isn't allowed to override this newp.Persist = oldp.Persist // caller isn't allowed to override this
b.prefs = newp b.prefs = newp
b.inServerMode = newp.ForceDaemon
// We do this to avoid holding the lock while doing everything else. // We do this to avoid holding the lock while doing everything else.
newp = b.prefs.Clone() newp = b.prefs.Clone()
@ -1019,26 +1049,7 @@ func (b *LocalBackend) SetPrefs(newp *Prefs) {
b.logf("Failed to save new controlclient state: %v", err) b.logf("Failed to save new controlclient state: %v", err)
} }
} }
if userID != "" { // e.g. on Windows b.writeServerModeStartState(userID, newp)
if newp.ForceDaemon {
stateKey := StateKey("user-" + userID)
if err := b.store.WriteState(ServerModeStartKey, []byte(stateKey)); err != nil {
b.logf("WriteState error: %v", err)
}
// It's important we do this here too, even if it looks
// redundant with the one in the 'if stateKey != ""'
// check block above. That one won't fire in the case
// where the Windows client started up in client mode.
// This happens when we transition into server mode:
if err := b.store.WriteState(stateKey, newp.ToBytes()); err != nil {
b.logf("WriteState error: %v", err)
}
} else {
if err := b.store.WriteState(ServerModeStartKey, nil); err != nil {
b.logf("WriteState error: %v", err)
}
}
}
// [GRINDER STATS LINE] - please don't remove (used for log parsing) // [GRINDER STATS LINE] - please don't remove (used for log parsing)
b.logf("SetPrefs: %v", newp.Pretty()) b.logf("SetPrefs: %v", newp.Pretty())

@ -92,6 +92,17 @@ func (bs *BackendServer) SendErrorMessage(msg string) {
bs.send(Notify{ErrMessage: &msg}) bs.send(Notify{ErrMessage: &msg})
} }
// SendInUseOtherUserErrorMessage sends a Notify message to the client that
// both sets the state to 'InUseOtherUser' and sets the associated reason
// to msg.
func (bs *BackendServer) SendInUseOtherUserErrorMessage(msg string) {
inUse := InUseOtherUser
bs.send(Notify{
State: &inUse,
ErrMessage: &msg,
})
}
// GotCommandMsg parses the incoming message b as a JSON Command and // GotCommandMsg parses the incoming message b as a JSON Command and
// calls GotCommand with it. // calls GotCommand with it.
func (bs *BackendServer) GotCommandMsg(b []byte) error { func (bs *BackendServer) GotCommandMsg(b []byte) error {

Loading…
Cancel
Save