diff --git a/net/dns/manager_windows.go b/net/dns/manager_windows.go index 1973f8d49..71f7011c3 100644 --- a/net/dns/manager_windows.go +++ b/net/dns/manager_windows.go @@ -147,7 +147,7 @@ func (m *windowsManager) setSplitDNS(resolvers []netip.Addr, domains []dnsname.F return fmt.Errorf("Split DNS unsupported on this Windows version") } - defer m.nrptDB.Refresh() + defer m.nrptDB.NotifyPolicyChanged() if len(resolvers) == 0 { return m.nrptDB.DelAllRuleKeys() } diff --git a/net/dns/manager_windows_test.go b/net/dns/manager_windows_test.go index d1c65ed2b..ed994c30c 100644 --- a/net/dns/manager_windows_test.go +++ b/net/dns/manager_windows_test.go @@ -85,23 +85,23 @@ func TestHostFileChanged(t *testing.T) { } func TestManagerWindowsLocal(t *testing.T) { - if !isWindows10OrBetter() || !winutil.IsCurrentProcessElevated() { - t.Skipf("test requires running as elevated user on Windows 10+") + if !winutil.IsCurrentProcessLocalSystem() { + t.Skipf("test requires running as LocalSystem on Windows 10+") } runTest(t, true) } func TestManagerWindowsGP(t *testing.T) { - if !isWindows10OrBetter() || !winutil.IsCurrentProcessElevated() { - t.Skipf("test requires running as elevated user on Windows 10+") + if !winutil.IsCurrentProcessLocalSystem() { + t.Skipf("test requires running as LocalSystem on Windows 10+") } checkGPNotificationsWork(t) - // Make sure group policy is refreshed before this test exits but after we've + // Make sure group policy is current before this test exits but after we've // cleaned everything else up. - defer gp.RefreshMachinePolicy(true) + defer gp.NotifyMachinePolicyChange() err := createFakeGPKey() if err != nil { @@ -113,8 +113,8 @@ func TestManagerWindowsGP(t *testing.T) { } func TestManagerWindowsGPCopy(t *testing.T) { - if !isWindows10OrBetter() || !winutil.IsCurrentProcessElevated() { - t.Skipf("test requires running as elevated user on Windows 10+") + if !winutil.IsCurrentProcessLocalSystem() { + t.Skipf("test requires running as LocalSystem on Windows 10+") } checkGPNotificationsWork(t) @@ -179,9 +179,9 @@ func TestManagerWindowsGPCopy(t *testing.T) { t.Fatalf("regWatcher.watch: %v\n", err) } - err = gp.RefreshMachinePolicy(true) + err = gp.NotifyMachinePolicyChange() if err != nil { - t.Fatalf("testDoRefresh: %v\n", err) + t.Fatalf("NotifyMachinePolicyChange: %v\n", err) } err = regWatcher.wait() @@ -203,9 +203,9 @@ func TestManagerWindowsGPCopy(t *testing.T) { t.Fatalf("regWatcher.watch: %v\n", err) } - err = gp.RefreshMachinePolicy(true) + err = gp.NotifyMachinePolicyChange() if err != nil { - t.Fatalf("testDoRefresh: %v\n", err) + t.Fatalf("NotifyMachinePolicyChange: %v\n", err) } err = regWatcher.wait() @@ -236,13 +236,13 @@ func checkGPNotificationsWork(t *testing.T) { } defer trk.Close() - err = gp.RefreshMachinePolicy(true) + err = gp.NotifyMachinePolicyChange() if err != nil { - t.Fatalf("RefreshPolicyEx error: %v\n", err) + t.Fatalf("NotifyMachinePolicyChange error: %v\n", err) } timeout := uint32(10000) // Milliseconds - if !trk.DidRefreshTimeout(timeout) { + if !trk.DidGroupPolicyChangeTimeout(timeout) { t.Skipf("GP notifications are not working on this machine\n") } } @@ -338,8 +338,8 @@ func runTest(t *testing.T, isLocal bool) { } validateRegistry(t, regBaseValidate, caseDomains) ensureNoRulesInSubkey(t, regBaseEnsure) - if !isLocal && !trk.DidRefresh(true) { - t.Fatalf("DidRefresh false, want true\n") + if !isLocal && !trk.DidGroupPolicyChange(true) { + t.Fatalf("DidGroupPolicyChange false, want true\n") } } @@ -347,10 +347,6 @@ func runTest(t *testing.T, isLocal bool) { runCase(n) } - if isLocal && trk.DidRefresh(false) { - t.Errorf("DidRefresh true, want false\n") - } - t.Logf("Test case: nil resolver\n") err = mgr.setSplitDNS(nil, domains) if err != nil { @@ -573,7 +569,7 @@ func genRandomSubdomains(t *testing.T, n int) []dnsname.FQDN { ) // gpNotificationTracker registers with the Windows policy engine and receives -// notifications when policy refreshes occur. +// notifications when policy change notifications occur. type gpNotificationTracker struct { event windows.Handle } @@ -602,7 +598,7 @@ func newGPNotificationTracker() (*gpNotificationTracker, error) { return &gpNotificationTracker{evt}, nil } -func (trk *gpNotificationTracker) DidRefresh(isExpected bool) bool { +func (trk *gpNotificationTracker) DidGroupPolicyChange(isExpected bool) bool { // If we're not expecting a refresh event, then we need to use a timeout. timeout := uint32(1000) // 1 second (in milliseconds) if isExpected { @@ -610,10 +606,10 @@ func (trk *gpNotificationTracker) DidRefresh(isExpected bool) bool { timeout = windows.INFINITE } - return trk.DidRefreshTimeout(timeout) + return trk.DidGroupPolicyChangeTimeout(timeout) } -func (trk *gpNotificationTracker) DidRefreshTimeout(timeout uint32) bool { +func (trk *gpNotificationTracker) DidGroupPolicyChangeTimeout(timeout uint32) bool { waitCode, _ := windows.WaitForSingleObject(trk.event, timeout) return waitCode == windows.WAIT_OBJECT_0 } @@ -625,6 +621,8 @@ func (trk *gpNotificationTracker) Close() error { return nil } +const dnsBaseGP = `SOFTWARE\Policies\Microsoft\Windows NT\DNSClient` + type regKeyWatcher struct { keyGP registry.Key evtGP windows.Handle diff --git a/net/dns/nrpt_windows.go b/net/dns/nrpt_windows.go index 6d9cf71b7..2ad404485 100644 --- a/net/dns/nrpt_windows.go +++ b/net/dns/nrpt_windows.go @@ -7,7 +7,6 @@ "fmt" "strings" "sync" - "sync/atomic" "golang.org/x/sys/windows" "golang.org/x/sys/windows/registry" @@ -19,7 +18,6 @@ ) const ( - dnsBaseGP = `SOFTWARE\Policies\Microsoft\Windows NT\DNSClient` nrptBaseLocal = `SYSTEM\CurrentControlSet\Services\Dnscache\Parameters\DnsPolicyConfig` nrptBaseGP = `SOFTWARE\Policies\Microsoft\Windows NT\DNSClient\DnsPolicyConfig` @@ -53,13 +51,12 @@ // nrptRuleDatabase encapsulates access to the Windows Name Resolution Policy // Table (NRPT). type nrptRuleDatabase struct { - logf logger.Logf - watcher *gp.ChangeWatcher - isGPRefreshPending atomic.Bool - mu sync.Mutex // protects the fields below - ruleIDs []string - isGPDirty bool - writeAsGP bool + logf logger.Logf + watcher *gp.ChangeWatcher + mu sync.Mutex // protects the fields below + ruleIDs []string + isGPDirty bool + writeAsGP bool } func newNRPTRuleDatabase(logf logger.Logf) *nrptRuleDatabase { @@ -141,6 +138,12 @@ func (db *nrptRuleDatabase) detectWriteAsGP() { writeAsGP = len(gpSubkeyMap) > 0 } +func regKeyHasNoValues(ki *registry.KeyInfo) bool { + // Either the key has no values, or there is one value: an empty default value. + return ki.ValueCount == 0 || + ki.ValueCount == 1 && ki.MaxValueNameLen == 0 && ki.MaxValueLen == 0 +} + // DelAllRuleKeys removes any and all NRPT rules that are owned by Tailscale. func (db *nrptRuleDatabase) DelAllRuleKeys() error { db.mu.Lock() @@ -171,7 +174,7 @@ func (db *nrptRuleDatabase) delRuleKeys(nrptRuleIDs []string) error { keyNameGP := nrptBaseGP + `\` + rid err := registry.DeleteKey(registry.LOCAL_MACHINE, keyNameGP) if err == nil { - // If this deleted subkey existed under the GP key, we will need to refresh. + // If this deleted subkey existed under the GP key, we will need to notify. db.isGPDirty = true } else if err != registry.ErrNotExist { db.logf("Error deleting NRPT rule key %q: %v", keyNameGP, err) @@ -193,8 +196,8 @@ func (db *nrptRuleDatabase) delRuleKeys(nrptRuleIDs []string) error { return registry.DeleteKey(registry.LOCAL_MACHINE, nrptBaseGP) } -// isPolicyConfigSubkeyEmpty returns true if and only if the nrptBaseGP exists -// and does not contain any values or subkeys. +// isPolicyConfigSubkeyEmpty returns whether the nrptBaseGP exists and does not +// contain any values or subkeys. func isPolicyConfigSubkeyEmpty() (bool, error) { subKey, err := registry.OpenKey(registry.LOCAL_MACHINE, nrptBaseGP, registry.READ) if err != nil { @@ -210,7 +213,7 @@ func isPolicyConfigSubkeyEmpty() (bool, error) { return false, err } - return (ki.ValueCount == 0 && ki.SubKeyCount == 0), nil + return regKeyHasNoValues(ki) && ki.SubKeyCount == 0, nil } func (db *nrptRuleDatabase) WriteSplitDNSConfig(servers []string, domains []dnsname.FQDN) error { @@ -277,26 +280,22 @@ func (db *nrptRuleDatabase) WriteSplitDNSConfig(servers []string, domains []dnsn return nil } -// Refresh notifies the Windows group policy engine when policies have changed. -func (db *nrptRuleDatabase) Refresh() { +// NotifyPolicyChanged notifies the Windows group policy engine when policies have changed. +func (db *nrptRuleDatabase) NotifyPolicyChanged() { db.mu.Lock() defer db.mu.Unlock() - db.refreshLocked() + db.notifyPolicyChangedLocked() } -func (db *nrptRuleDatabase) refreshLocked() { +func (db *nrptRuleDatabase) notifyPolicyChangedLocked() { if !db.isGPDirty { return } - // Record that we are about to initiate a refresh. - // (*nrptRuleDatabase).watchForGPChanges() checks this value to avoid false - // positives. - db.isGPRefreshPending.Store(true) - - if err := gp.RefreshMachinePolicy(true); err != nil { - db.logf("RefreshMachinePolicy failed: %v", err) + db.logf("Calling NotifyMachinePolicyChange") + if err := gp.NotifyMachinePolicyChange(); err != nil { + db.logf("NotifyMachinePolicyChange failed: %v", err) return } @@ -355,12 +354,7 @@ func writeNRPTValues(key registry.Key, servers string, doms []string) error { func (db *nrptRuleDatabase) watchForGPChanges() { watchHandler := func() { - // Do not invoke detectWriteAsGP when we ourselves were responsible for - // initiating the group policy refresh. - if db.isGPRefreshPending.CompareAndSwap(true, false) { - return - } - db.logf("Computer group policies refreshed, reconfiguring NRPT rule database.") + db.logf("Received machine group policy change notification. Reconfiguring NRPT rule database.") db.detectWriteAsGP() } @@ -380,8 +374,8 @@ func (db *nrptRuleDatabase) watchForGPChanges() { // db.mu must already be locked. func (db *nrptRuleDatabase) updateGroupPoliciesLocked(writeAsGP bool) { // Since we're updating the group policy NRPT table, we need - // to refresh once this updateGroupPoliciesLocked is done. - defer db.refreshLocked() + // to notify once this updateGroupPoliciesLocked is done. + defer db.notifyPolicyChangedLocked() for _, id := range db.ruleIDs { if writeAsGP { diff --git a/util/winutil/gp/gp_windows.go b/util/winutil/gp/gp_windows.go index dd13a2701..df2052eff 100644 --- a/util/winutil/gp/gp_windows.go +++ b/util/winutil/gp/gp_windows.go @@ -9,6 +9,7 @@ import ( "fmt" "runtime" + "unsafe" "golang.org/x/sys/windows" ) @@ -77,3 +78,14 @@ func toRefreshPolicyFlags(force bool) uint32 { } return 0 } + +var mgmtProductTailscale = unsafe.SliceData([]uint16{'T', 'a', 'i', 'l', 's', 'c', 'a', 'l', 'e', 0}) + +// NotifyMachinePolicyChange sends a machine-scoped group policy change +// notification which Windows broadcasts to group policy change subscribers. +// The caller must be running as LocalSystem. +func NotifyMachinePolicyChange() error { + // GenerateGPNotification is quasi-documented. Note that its implementation + // contains an access check ensuring that the calling user is LocalSystem! + return generateGPNotification(true, mgmtProductTailscale, 0) +} diff --git a/util/winutil/gp/mksyscall.go b/util/winutil/gp/mksyscall.go index 22fd0c137..1ddbebb37 100644 --- a/util/winutil/gp/mksyscall.go +++ b/util/winutil/gp/mksyscall.go @@ -6,6 +6,7 @@ //go:generate go run golang.org/x/sys/windows/mkwinsyscall -output zsyscall_windows.go mksyscall.go //sys enterCriticalPolicySection(machine bool) (handle policyLockHandle, err error) [int32(failretval)==0] = userenv.EnterCriticalPolicySection +//sys generateGPNotification(machine bool, mgmtProduct *uint16, mgmtProductOptions uint32) (ret error) = userenv.GenerateGPNotification? //sys impersonateLoggedOnUser(token windows.Token) (err error) [int32(failretval)==0] = advapi32.ImpersonateLoggedOnUser //sys leaveCriticalPolicySection(handle policyLockHandle) (err error) [int32(failretval)==0] = userenv.LeaveCriticalPolicySection //sys registerGPNotification(event windows.Handle, machine bool) (err error) [int32(failretval)==0] = userenv.RegisterGPNotification diff --git a/util/winutil/gp/zsyscall_windows.go b/util/winutil/gp/zsyscall_windows.go index 41c240c26..f0ba9a1c4 100644 --- a/util/winutil/gp/zsyscall_windows.go +++ b/util/winutil/gp/zsyscall_windows.go @@ -43,6 +43,7 @@ func errnoErr(e syscall.Errno) error { procImpersonateLoggedOnUser = modadvapi32.NewProc("ImpersonateLoggedOnUser") procEnterCriticalPolicySection = moduserenv.NewProc("EnterCriticalPolicySection") + procGenerateGPNotification = moduserenv.NewProc("GenerateGPNotification") procLeaveCriticalPolicySection = moduserenv.NewProc("LeaveCriticalPolicySection") procRefreshPolicyEx = moduserenv.NewProc("RefreshPolicyEx") procRegisterGPNotification = moduserenv.NewProc("RegisterGPNotification") @@ -70,6 +71,22 @@ func enterCriticalPolicySection(machine bool) (handle policyLockHandle, err erro return } +func generateGPNotification(machine bool, mgmtProduct *uint16, mgmtProductOptions uint32) (ret error) { + ret = procGenerateGPNotification.Find() + if ret != nil { + return + } + var _p0 uint32 + if machine { + _p0 = 1 + } + r0, _, _ := syscall.SyscallN(procGenerateGPNotification.Addr(), uintptr(_p0), uintptr(unsafe.Pointer(mgmtProduct)), uintptr(mgmtProductOptions)) + if r0 != 0 { + ret = syscall.Errno(r0) + } + return +} + func leaveCriticalPolicySection(handle policyLockHandle) (err error) { r1, _, e1 := syscall.SyscallN(procLeaveCriticalPolicySection.Addr(), uintptr(handle)) if int32(r1) == 0 { diff --git a/util/winutil/winutil_windows.go b/util/winutil/winutil_windows.go index 671a2a1bc..1e2488f15 100644 --- a/util/winutil/winutil_windows.go +++ b/util/winutil/winutil_windows.go @@ -972,3 +972,25 @@ func GUIPathFromReg() (string, error) { return regPath, nil } + +// IsCurrentProcessLocalSystem checks whether the current process is running +// as LocalSystem. +func IsCurrentProcessLocalSystem() bool { + localSystem, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + return false + } + + token := windows.GetCurrentProcessToken() + // The current process token is a pseudo-handle so we don't need to close it. + if ok, err := token.IsMember(localSystem); err != nil || !ok { + return false + } + + u, err := token.GetTokenUser() + if err != nil { + return false + } + + return u.User.Sid.Equals(localSystem) +}