From 8049053f86a0b7cc0eca2aef9e14032de8c28d35 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 27 Nov 2022 09:04:00 -0800 Subject: [PATCH] ipn/*: make new WindowsUserID type to consolidate docs The "userID is empty everywhere but Windows" docs on lots of places but not everywhere while using just a string type was getting confusing. This makes a new type to wrap up those rules, however weird/historical they might be. Change-Id: I142e85a8e38760988d6c0c91d0efecedade81b9b Signed-off-by: Brad Fitzpatrick --- ipn/ipnauth/ipnauth.go | 30 ++++++++++++++++++++++-------- ipn/ipnlocal/local.go | 6 +++--- ipn/ipnlocal/profiles.go | 12 ++++++------ ipn/ipnserver/server.go | 10 ++++------ ipn/prefs.go | 8 +++++++- 5 files changed, 42 insertions(+), 24 deletions(-) diff --git a/ipn/ipnauth/ipnauth.go b/ipn/ipnauth/ipnauth.go index fb82a08aa..158f3a791 100644 --- a/ipn/ipnauth/ipnauth.go +++ b/ipn/ipnauth/ipnauth.go @@ -17,6 +17,8 @@ import ( "syscall" "inet.af/peercred" + "tailscale.com/envknob" + "tailscale.com/ipn" "tailscale.com/net/netstat" "tailscale.com/safesocket" "tailscale.com/types/logger" @@ -40,19 +42,31 @@ type ConnIdentity struct { // TODO(bradfitz): merge these into the peercreds package and // use that for all. pid int - userID string + userID ipn.WindowsUserID user *user.User } -// UserID returns the local machine's userid of the connection. +// WindowsUserID returns the local machine's userid of the connection +// if it's on Windows. Otherwise it returns the empty string. // // It's suitable for passing to LookupUserFromID (os/user.LookupId) on any // operating system. -// -// TODO(bradfitz): it currently returns an empty string on everything -// but Windows. We should make it return the actual uid also on all supported -// peercred platforms from the creds if non-nil. -func (ci *ConnIdentity) UserID() string { return ci.userID } +func (ci *ConnIdentity) WindowsUserID() ipn.WindowsUserID { + if envknob.GOOS() != "windows" { + return "" + } + if ci.userID != "" { + return ci.userID + } + // For Linux tests running as Windows: + const isBroken = true // TODO(bradfitz,maisem): fix tests; this doesn't work yet + if ci.creds != nil && !isBroken { + if uid, ok := ci.creds.UserID(); ok { + return ipn.WindowsUserID(uid) + } + } + return "" +} func (ci *ConnIdentity) User() *user.User { return ci.user } func (ci *ConnIdentity) Pid() int { return ci.pid } @@ -99,7 +113,7 @@ func GetConnIdentity(logf logger.Logf, c net.Conn) (ci *ConnIdentity, err error) } return ci, fmt.Errorf("failed to map connection's pid to a user%s: %w", hint, err) } - ci.userID = uid + ci.userID = ipn.WindowsUserID(uid) u, err := LookupUserFromID(logf, uid) if err != nil { return ci, fmt.Errorf("failed to look up user from userid: %w", err) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 01c7ea436..be8b645b0 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2076,12 +2076,12 @@ func (b *LocalBackend) CheckIPNConnectionAllowed(ci *ipnauth.ConnIdentity) error if !b.pm.CurrentPrefs().ForceDaemon() { return nil } - uid := ci.UserID() + uid := ci.WindowsUserID() if uid == "" { return errors.New("empty user uid in connection identity") } if uid != serverModeUid { - return fmt.Errorf("Tailscale running in server mode (%q); connection from %q not allowed", b.tryLookupUserName(serverModeUid), b.tryLookupUserName(uid)) + return fmt.Errorf("Tailscale running in server mode (%q); connection from %q not allowed", b.tryLookupUserName(string(serverModeUid)), b.tryLookupUserName(string(uid))) } return nil } @@ -2257,7 +2257,7 @@ func (b *LocalBackend) shouldUploadServices() bool { // changed. // // On non-multi-user systems, the uid should be set to empty string. -func (b *LocalBackend) SetCurrentUserID(uid string) { +func (b *LocalBackend) SetCurrentUserID(uid ipn.WindowsUserID) { b.mu.Lock() if b.pm.CurrentUserID() == uid { b.mu.Unlock() diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index 0cefa00ba..cd6d64168 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -28,7 +28,7 @@ type profileManager struct { store ipn.StateStore logf logger.Logf - currentUserID string // only used on Windows + currentUserID ipn.WindowsUserID knownProfiles map[ipn.ProfileID]*ipn.LoginProfile currentProfile *ipn.LoginProfile // always non-nil prefs ipn.PrefsView // always Valid. @@ -42,13 +42,13 @@ type profileManager struct { // CurrentUserID returns the current user ID. It is only non-empty on // Windows where we have a multi-user system. -func (pm *profileManager) CurrentUserID() string { +func (pm *profileManager) CurrentUserID() ipn.WindowsUserID { return pm.currentUserID } // SetCurrentUserID sets the current user ID. The uid is only non-empty // on Windows where we have a multi-user system. -func (pm *profileManager) SetCurrentUserID(uid string) error { +func (pm *profileManager) SetCurrentUserID(uid ipn.WindowsUserID) error { if pm.currentUserID == uid { return nil } @@ -63,7 +63,7 @@ func (pm *profileManager) SetCurrentUserID(uid string) error { // Read the CurrentProfileKey from the store which stores // the selected profile for the current user. - b, err := pm.store.ReadState(ipn.CurrentProfileKey(uid)) + b, err := pm.store.ReadState(ipn.CurrentProfileKey(string(uid))) if err == ipn.ErrStateNotExist || len(b) == 0 { pm.NewProfile() return nil @@ -310,7 +310,7 @@ func (pm *profileManager) SwitchProfile(id ipn.ProfileID) error { } func (pm *profileManager) setAsUserSelectedProfileLocked() error { - k := ipn.CurrentProfileKey(pm.currentUserID) + k := ipn.CurrentProfileKey(string(pm.currentUserID)) return pm.store.WriteState(k, []byte(pm.currentProfile.Key)) } @@ -487,7 +487,7 @@ func newProfileManagerWithGOOS(store ipn.StateStore, logf logger.Logf, stateKey } if pm.currentProfile == nil { if suf, ok := strs.CutPrefix(string(stateKey), "user-"); ok { - pm.currentUserID = suf + pm.currentUserID = ipn.WindowsUserID(suf) } pm.NewProfile() } else { diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 3e770ef44..ab22d0482 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -14,7 +14,6 @@ import ( "os" "os/user" "path/filepath" - "runtime" "strings" "sync" "time" @@ -87,7 +86,7 @@ type Server struct { // mu guards the fields that follow. // lock order: mu, then LocalBackend.mu mu sync.Mutex - lastUserID string // tracks last userid; on change, Reset state for paranoia + lastUserID ipn.WindowsUserID // tracks last userid; on change, Reset state for paranoia activeReqs map[*http.Request]*ipnauth.ConnIdentity } @@ -168,7 +167,7 @@ func (s *Server) checkConnIdentityLocked(ci *ipnauth.ConnIdentity) error { for _, active = range s.activeReqs { break } - if active != nil && ci.UserID() != active.UserID() { + if active != nil && ci.WindowsUserID() != active.WindowsUserID() { return inUseOtherUserError{fmt.Errorf("Tailscale already in use by %s, pid %d", active.User().Username, active.Pid())} } } @@ -183,7 +182,7 @@ func (s *Server) checkConnIdentityLocked(ci *ipnauth.ConnIdentity) error { // // s.mu must not be held. func (s *Server) localAPIPermissions(ci *ipnauth.ConnIdentity) (read, write bool) { - switch runtime.GOOS { + switch envknob.GOOS() { case "windows": s.mu.Lock() defer s.mu.Unlock() @@ -272,8 +271,7 @@ func (s *Server) addActiveHTTPRequest(req *http.Request, ci *ipnauth.ConnIdentit mak.Set(&s.activeReqs, req, ci) - if envknob.GOOS() == "windows" && len(s.activeReqs) == 1 { - uid := ci.UserID() + if uid := ci.WindowsUserID(); uid != "" && len(s.activeReqs) == 1 { // Tell the LocalBackend about the identity we're now running as. s.b.SetCurrentUserID(uid) if s.lastUserID != uid { diff --git a/ipn/prefs.go b/ipn/prefs.go index eb1d48497..466f20b15 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -702,6 +702,12 @@ func SavePrefs(filename string, p *Prefs) { // profile. It is a 4 character hex string like "1ab3". type ProfileID string +// WindowsUserID is a userid (suitable for passing to ipnauth.LookupUserFromID +// or os/user.LookupId) but only set on Windows. It's empty on all other +// platforms, unless envknob.GOOS is in used, making Linux act like Windows for +// tests. +type WindowsUserID string + // LoginProfile represents a single login profile as managed // by the ProfileManager. type LoginProfile struct { @@ -734,5 +740,5 @@ type LoginProfile struct { // LocalUserID is the user ID of the user who created this profile. // It is only relevant on Windows where we have a multi-user system. // It is assigned once at profile creation time and never changes. - LocalUserID string + LocalUserID WindowsUserID }