net/dns, util/winutil: improve detection of group policy affecting NRPT

Due to a customer issue, I investigated the Windows Dnscache service more
intensively. I learned that the only time it attempts to read the NRPT
from group policy is in response to a group policy change notification.

Under the hypothesis that policy refresh is not effectively delivering GP
notifications due to its dependency on reaching a DC, I replaced our use
of the RefreshPolicyEx with the quasi-documented GenerateGPNotification API.

Tests have been updated to ensure they check that they are running as
LocalSystem, which is required for GenerateGPNotification.

Fixes #20187

Signed-off-by: Aaron Klotz <aaron@tailscale.com>

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()
 	}

net/dns/manager_windows_test.go

@@ -85,23 +85,23 @@ func TestHostFileChanged(t *testing.T) {
 }
 
 func TestManagerWindowsLocal(t *testing.T) {
-	if !isWindows10OrBetter() || !winutil.IsCurrentProcessElevated() {
+	if !winutil.IsCurrentProcessLocalSystem() {
-		t.Skipf("test requires running as elevated user on Windows 10+")
+		t.Skipf("test requires running as LocalSystem on Windows 10+")
 	}
 
 	runTest(t, true)
 }
 
 func TestManagerWindowsGP(t *testing.T) {
-	if !isWindows10OrBetter() || !winutil.IsCurrentProcessElevated() {
+	if !winutil.IsCurrentProcessLocalSystem() {
-		t.Skipf("test requires running as elevated user on Windows 10+")
+		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() {
+	if !winutil.IsCurrentProcessLocalSystem() {
-		t.Skipf("test requires running as elevated user on Windows 10+")
+		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) {
+		if !isLocal && !trk.DidGroupPolicyChange(true) {
-			t.Fatalf("DidRefresh false, want true\n")
+			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 @@ var (
 )
 
 // 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

net/dns/nrpt_windows.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"strings"
 	"sync"
-	"sync/atomic"
 
 	"golang.org/x/sys/windows"
 	"golang.org/x/sys/windows/registry"
@@ -19,7 +18,6 @@ import (
 )
 
 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 @@ const (
 // nrptRuleDatabase encapsulates access to the Windows Name Resolution Policy
 // Table (NRPT).
 type nrptRuleDatabase struct {
-	logf               logger.Logf
+	logf      logger.Logf
-	watcher            *gp.ChangeWatcher
+	watcher   *gp.ChangeWatcher
-	isGPRefreshPending atomic.Bool
+	mu        sync.Mutex // protects the fields below
-	mu                 sync.Mutex // protects the fields below
+	ruleIDs   []string
-	ruleIDs            []string
+	isGPDirty bool
-	isGPDirty          bool
+	writeAsGP 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
+// isPolicyConfigSubkeyEmpty returns whether the nrptBaseGP exists and does not
-// and does not contain any values or subkeys.
+// 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.
+// NotifyPolicyChanged notifies the Windows group policy engine when policies have changed.
-func (db *nrptRuleDatabase) Refresh() {
+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.
+	db.logf("Calling NotifyMachinePolicyChange")
-	// (*nrptRuleDatabase).watchForGPChanges() checks this value to avoid false
+	if err := gp.NotifyMachinePolicyChange(); err != nil {
-	// positives.
+		db.logf("NotifyMachinePolicyChange failed: %v", err)
-	db.isGPRefreshPending.Store(true)
-
-	if err := gp.RefreshMachinePolicy(true); err != nil {
-		db.logf("RefreshMachinePolicy 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
+		db.logf("Received machine group policy change notification. Reconfiguring NRPT rule database.")
-		// initiating the group policy refresh.
-		if db.isGPRefreshPending.CompareAndSwap(true, false) {
-			return
-		}
-		db.logf("Computer group policies refreshed, 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.
+	// to notify once this updateGroupPoliciesLocked is done.
-	defer db.refreshLocked()
+	defer db.notifyPolicyChangedLocked()
 
 	for _, id := range db.ruleIDs {
 		if writeAsGP {

util/winutil/gp/gp_windows.go

@@ -9,6 +9,7 @@ package gp
 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)
+}

util/winutil/gp/mksyscall.go

@@ -6,6 +6,7 @@ package gp
 //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

util/winutil/gp/zsyscall_windows.go

@@ -43,6 +43,7 @@ var (
 
 	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 {

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)
+}