From 6e33d2da2b64ff9a2dce7b83e2147d2fb4f4ab47 Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Thu, 24 Nov 2022 15:51:19 -0700 Subject: [PATCH] ipn/ipnauth, util/winutil: add temporary LookupPseudoUser workaround to address os/user.LookupId errors on Windows I added util/winutil/LookupPseudoUser, which essentially consists of the bits that I am in the process of adding to Go's standard library. We check the provided SID for "S-1-5-x" where 17 <= x <= 20 (which are the known pseudo-users) and then manually populate a os/user.User struct with the correct information. Fixes https://github.com/tailscale/tailscale/issues/869 Fixes https://github.com/tailscale/tailscale/issues/2894 Signed-off-by: Aaron Klotz --- cmd/derper/depaware.txt | 1 + ipn/ipnauth/ipnauth.go | 17 +++++++- util/winutil/winutil.go | 14 +++++++ util/winutil/winutil_notwindows.go | 10 +++++ util/winutil/winutil_windows.go | 60 ++++++++++++++++++++++++++++ util/winutil/winutil_windows_test.go | 31 ++++++++++++++ 6 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 util/winutil/winutil_windows_test.go diff --git a/cmd/derper/depaware.txt b/cmd/derper/depaware.txt index 286af04e7..45403489e 100644 --- a/cmd/derper/depaware.txt +++ b/cmd/derper/depaware.txt @@ -183,6 +183,7 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa net/url from crypto/x509+ os from crypto/rand+ os/exec from golang.zx2c4.com/wireguard/windows/tunnel/winipcfg+ + W os/user from tailscale.com/util/winutil path from golang.org/x/crypto/acme/autocert+ path/filepath from crypto/x509+ reflect from crypto/x509+ diff --git a/ipn/ipnauth/ipnauth.go b/ipn/ipnauth/ipnauth.go index 158f3a791..84de43ce9 100644 --- a/ipn/ipnauth/ipnauth.go +++ b/ipn/ipnauth/ipnauth.go @@ -14,7 +14,6 @@ import ( "os/user" "runtime" "strconv" - "syscall" "inet.af/peercred" "tailscale.com/envknob" @@ -22,6 +21,7 @@ import ( "tailscale.com/net/netstat" "tailscale.com/safesocket" "tailscale.com/types/logger" + "tailscale.com/util/clientmetric" "tailscale.com/util/groupmember" "tailscale.com/util/pidowner" "tailscale.com/util/winutil" @@ -122,11 +122,23 @@ func GetConnIdentity(logf logger.Logf, c net.Conn) (ci *ConnIdentity, err error) return ci, nil } +var metricIssue869Workaround = clientmetric.NewCounter("issue_869_workaround") + // LookupUserFromID is a wrapper around os/user.LookupId that works around some // issues on Windows. On non-Windows platforms it's identical to user.LookupId. func LookupUserFromID(logf logger.Logf, uid string) (*user.User, error) { u, err := user.LookupId(uid) - if err != nil && runtime.GOOS == "windows" && errors.Is(err, syscall.Errno(0x534)) { + if err != nil && runtime.GOOS == "windows" { + // See if uid resolves as a pseudo-user. Temporary workaround until + // https://github.com/golang/go/issues/49509 resolves and ships. + if u, err := winutil.LookupPseudoUser(uid); err == nil { + return u, nil + } + + // TODO(aaron): With LookupPseudoUser in place, I don't expect us to reach + // this point anymore. Leaving the below workaround in for now to confirm + // that pseudo-user resolution sufficiently handles this problem. + // The below workaround is only applicable when uid represents a // valid security principal. Omitting this check causes us to succeed // even when uid represents a deleted user. @@ -134,6 +146,7 @@ func LookupUserFromID(logf logger.Logf, uid string) (*user.User, error) { return nil, err } + metricIssue869Workaround.Add(1) logf("[warning] issue 869: os/user.LookupId failed; ignoring") // Work around https://github.com/tailscale/tailscale/issues/869 for // now. We don't strictly need the username. It's just a nice-to-have. diff --git a/util/winutil/winutil.go b/util/winutil/winutil.go index 0103598aa..324ed074d 100644 --- a/util/winutil/winutil.go +++ b/util/winutil/winutil.go @@ -5,6 +5,10 @@ // Package winutil contains misc Windows/Win32 helper functions. package winutil +import ( + "os/user" +) + // RegBase is the registry path inside HKEY_LOCAL_MACHINE where registry settings // are stored. This constant is a non-empty string only when GOOS=windows. const RegBase = regBase @@ -62,3 +66,13 @@ func GetRegInteger(name string, defval uint64) uint64 { func IsSIDValidPrincipal(uid string) bool { return isSIDValidPrincipal(uid) } + +// LookupPseudoUser attempts to resolve the user specified by uid by checking +// against well-known pseudo-users on Windows. This is a temporary workaround +// until https://github.com/golang/go/issues/49509 is resolved and shipped. +// +// This function will only work on GOOS=windows. Trying to run it on any other +// OS will always return an error. +func LookupPseudoUser(uid string) (*user.User, error) { + return lookupPseudoUser(uid) +} diff --git a/util/winutil/winutil_notwindows.go b/util/winutil/winutil_notwindows.go index 76c858e19..c859a9c63 100644 --- a/util/winutil/winutil_notwindows.go +++ b/util/winutil/winutil_notwindows.go @@ -6,6 +6,12 @@ package winutil +import ( + "fmt" + "os/user" + "runtime" +) + const regBase = `` func getPolicyString(name, defval string) string { return defval } @@ -17,3 +23,7 @@ func getRegString(name, defval string) string { return defval } func getRegInteger(name string, defval uint64) uint64 { return defval } func isSIDValidPrincipal(uid string) bool { return false } + +func lookupPseudoUser(uid string) (*user.User, error) { + return nil, fmt.Errorf("unimplemented on %v", runtime.GOOS) +} diff --git a/util/winutil/winutil_windows.go b/util/winutil/winutil_windows.go index 819104f76..547c5a260 100644 --- a/util/winutil/winutil_windows.go +++ b/util/winutil/winutil_windows.go @@ -9,6 +9,7 @@ import ( "fmt" "log" "os/exec" + "os/user" "runtime" "strings" "syscall" @@ -492,3 +493,62 @@ func OpenKeyWait(k registry.Key, path RegistryPath, access uint32) (registry.Key k = key } } + +func lookupPseudoUser(uid string) (*user.User, error) { + sid, err := windows.StringToSid(uid) + if err != nil { + return nil, err + } + + // We're looking for SIDs "S-1-5-x" where 17 <= x <= 20. + // This is checking for the the "5" + if sid.IdentifierAuthority() != windows.SECURITY_NT_AUTHORITY { + return nil, fmt.Errorf(`SID %q does not use "NT AUTHORITY"`, uid) + } + + // This is ensuring that there is only one sub-authority. + // In other words, only one value after the "5". + if sid.SubAuthorityCount() != 1 { + return nil, fmt.Errorf("SID %q should have only one subauthority", uid) + } + + // Get that sub-authority value (this is "x" above) and check it. + rid := sid.SubAuthority(0) + if rid < 17 || rid > 20 { + return nil, fmt.Errorf("SID %q does not represent a known pseudo-user", uid) + } + + // We've got one of the known pseudo-users. Look up the localized name of the + // account. + username, domain, _, err := sid.LookupAccount("") + if err != nil { + return nil, err + } + + // This call is best-effort. If it fails, homeDir will be empty. + homeDir, _ := findHomeDirInRegistry(uid) + + result := &user.User{ + Uid: uid, + Gid: uid, // Gid == Uid with these accounts. + Username: fmt.Sprintf(`%s\%s`, domain, username), + Name: username, + HomeDir: homeDir, + } + return result, nil +} + +// findHomeDirInRegistry finds the user home path based on the uid. +// This is borrowed from Go's std lib. +func findHomeDirInRegistry(uid string) (dir string, err error) { + k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList\`+uid, registry.QUERY_VALUE) + if err != nil { + return "", err + } + defer k.Close() + dir, _, err = k.GetStringValue("ProfileImagePath") + if err != nil { + return "", err + } + return dir, nil +} diff --git a/util/winutil/winutil_windows_test.go b/util/winutil/winutil_windows_test.go new file mode 100644 index 000000000..0620d1c8e --- /dev/null +++ b/util/winutil/winutil_windows_test.go @@ -0,0 +1,31 @@ +// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package winutil + +import ( + "testing" +) + +const ( + localSystemSID = "S-1-5-18" + networkSID = "S-1-5-2" +) + +func TestLookupPseudoUser(t *testing.T) { + localSystem, err := LookupPseudoUser(localSystemSID) + if err != nil { + t.Errorf("LookupPseudoUser(%q) error: %v", localSystemSID, err) + } + if localSystem.Gid != localSystemSID { + t.Errorf("incorrect Gid, got %q, want %q", localSystem.Gid, localSystemSID) + } + t.Logf("localSystem: %v", localSystem) + + // networkSID is a built-in known group but not a pseudo-user. + _, err = LookupPseudoUser(networkSID) + if err == nil { + t.Errorf("LookupPseudoUser(%q) unexpectedly succeeded", networkSID) + } +}