ipn/ipnlocal: better enforce system policies

Previously, policies affected the default prefs for a new profile, but
that does not affect existing profiles. This change ensures that
policies are applied whenever preferences are loaded or changed, so a
CLI or GUI client that does not respect the policies will still be
overridden.

Exit node IP is dropped from this PR as it was implemented elsewhere
in #10172.

Fixes tailscale/corp#15585

Change-Id: Ide4c3a4b00a64e43f506fa1fab70ef591407663f
Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>

ipn/ipnlocal/local.go

@@ -1119,6 +1119,9 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 	if setExitNodeID(prefs, st.NetMap) {
 		prefsChanged = true
 	}
+	if applySysPolicy(prefs) {
+		prefsChanged = true
+	}
 
 	// Until recently, we did not store the account's tailnet name. So check if this is the case,
 	// and backfill it on incoming status update.
@@ -1227,6 +1230,35 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 	b.authReconfig()
 }
 
+// applySysPolicy overwrites configured preferences with policies that may be
+// configured by the system administrator in an OS-specific way.
+func applySysPolicy(prefs *ipn.Prefs) (anyChange bool) {
+	if controlURL, err := syspolicy.GetString(syspolicy.ControlURL, prefs.ControlURL); err == nil && prefs.ControlURL != controlURL {
+		prefs.ControlURL = controlURL
+		anyChange = true
+	}
+
+	// Allow Incoming (used by the UI) is the negation of ShieldsUp (used by the
+	// backend), so this has to convert between the two conventions.
+	if shieldsUp, err := syspolicy.GetPreferenceOption(syspolicy.EnableIncomingConnections); err == nil {
+		newVal := !shieldsUp.ShouldEnable(!prefs.ShieldsUp)
+		if prefs.ShieldsUp != newVal {
+			prefs.ShieldsUp = newVal
+			anyChange = true
+		}
+	}
+
+	if forceDaemon, err := syspolicy.GetPreferenceOption(syspolicy.EnableServerMode); err == nil {
+		newVal := forceDaemon.ShouldEnable(prefs.ForceDaemon)
+		if prefs.ForceDaemon != newVal {
+			prefs.ForceDaemon = newVal
+			anyChange = true
+		}
+	}
+
+	return anyChange
+}
+
 var _ controlclient.NetmapDeltaUpdater = (*LocalBackend)(nil)
 
 // UpdateNetmapDelta implements controlclient.NetmapDeltaUpdater.
@@ -3051,10 +3083,12 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) ipn
 	if oldp.Valid() {
 		newp.Persist = oldp.Persist().AsStruct() // caller isn't allowed to override this
 	}
-	// findExitNodeIDLocked returns whether it updated b.prefs, but
+	// setExitNodeID returns whether it updated b.prefs, but
 	// everything in this function treats b.prefs as completely new
 	// anyway. No-op if no exit node resolution is needed.
 	setExitNodeID(newp, netMap)
+	// applySysPolicy does likewise so we can also ignore its return value.
+	applySysPolicy(newp)
 	// We do this to avoid holding the lock while doing everything else.
 
 	oldHi := b.hostinfo

ipn/ipnlocal/local_test.go

@@ -1333,32 +1333,41 @@ func (rc *routeCollector) AdvertiseRoute(pfx netip.Prefix) error {
 }
 
 type mockSyspolicyHandler struct {
-	t             *testing.T
-	key           syspolicy.Key
-	s             string
-	exitNodeIDKey bool
-	exitNodeIPKey bool
-	exitNodeID    string
-	exitNodeIP    string
+	t *testing.T
+	// stringPolicies is the collection of policies that we expect to see
+	// queried by the current test. If the policy is expected but unset, then
+	// use nil, otherwise use a string equal to the policy's desired value.
+	stringPolicies map[syspolicy.Key]*string
+	// failUnknownPolicies is set if policies other than those in stringPolicies
+	// (uint64 or bool policies are not supported by mockSyspolicyHandler yet)
+	// should be considered a test failure if they are queried.
+	failUnknownPolicies bool
 }
 
 func (h *mockSyspolicyHandler) ReadString(key string) (string, error) {
-	if h.exitNodeIDKey && key == string(syspolicy.ExitNodeID) {
-		return h.exitNodeID, nil
+	if s, ok := h.stringPolicies[syspolicy.Key(key)]; ok {
+		if s == nil {
+			return "", syspolicy.ErrNoSuchKey
+		}
+		return *s, nil
 	}
-	if h.exitNodeIPKey && key == string(syspolicy.ExitNodeIP) {
-		return h.exitNodeIP, nil
+	if h.failUnknownPolicies {
+		h.t.Errorf("ReadString(%q) unexpectedly called", key)
 	}
 	return "", syspolicy.ErrNoSuchKey
 }
 
 func (h *mockSyspolicyHandler) ReadUInt64(key string) (uint64, error) {
-	h.t.Errorf("ReadUInt64(%q) unexpectedly called", key)
+	if h.failUnknownPolicies {
+		h.t.Errorf("ReadUInt64(%q) unexpectedly called", key)
+	}
 	return 0, syspolicy.ErrNoSuchKey
 }
 
 func (h *mockSyspolicyHandler) ReadBoolean(key string) (bool, error) {
-	h.t.Errorf("ReadBoolean(%q) unexpectedly called", key)
+	if h.failUnknownPolicies {
+		h.t.Errorf("ReadBoolean(%q) unexpectedly called", key)
+	}
 	return false, syspolicy.ErrNoSuchKey
 }
 
@@ -1558,13 +1567,20 @@ func TestSetExitNodeIDPolicy(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			b := newTestBackend(t)
-			syspolicy.SetHandlerForTest(t, &mockSyspolicyHandler{
-				t:             t,
-				exitNodeID:    test.exitNodeID,
-				exitNodeIP:    test.exitNodeIP,
-				exitNodeIDKey: test.exitNodeIDKey,
-				exitNodeIPKey: test.exitNodeIPKey,
-			})
+			msh := &mockSyspolicyHandler{
+				t: t,
+				stringPolicies: map[syspolicy.Key]*string{
+					syspolicy.ExitNodeID: nil,
+					syspolicy.ExitNodeIP: nil,
+				},
+			}
+			if test.exitNodeIDKey {
+				msh.stringPolicies[syspolicy.ExitNodeID] = &test.exitNodeID
+			}
+			if test.exitNodeIPKey {
+				msh.stringPolicies[syspolicy.ExitNodeIP] = &test.exitNodeIP
+			}
+			syspolicy.SetHandlerForTest(t, msh)
 			if test.nm == nil {
 				test.nm = new(netmap.NetworkMap)
 			}
@@ -1577,21 +1593,15 @@ func TestSetExitNodeIDPolicy(t *testing.T) {
 			b.pm = pm
 			changed := setExitNodeID(b.pm.prefs.AsStruct(), test.nm)
 			b.SetPrefs(pm.CurrentPrefs().AsStruct())
-			if test.exitNodeIDKey {
-				got := b.pm.prefs.ExitNodeID()
-				if got != tailcfg.StableNodeID(test.exitNodeIDWant) {
-					t.Errorf("got %v want %v", got, test.exitNodeIDWant)
-				}
+			if got := b.pm.prefs.ExitNodeID(); got != tailcfg.StableNodeID(test.exitNodeIDWant) {
+				t.Errorf("got %v want %v", got, test.exitNodeIDWant)
 			}
-			if test.exitNodeIPKey {
-				got := b.pm.prefs.ExitNodeIP()
-				if test.exitNodeIPWant == "" {
-					if got.String() != "invalid IP" {
-						t.Errorf("got %v want invalid IP", got)
-					}
-				} else if got.String() != test.exitNodeIPWant {
-					t.Errorf("got %v want %v", got, test.exitNodeIPWant)
+			if got := b.pm.prefs.ExitNodeIP(); test.exitNodeIPWant == "" {
+				if got.String() != "invalid IP" {
+					t.Errorf("got %v want invalid IP", got)
 				}
+			} else if got.String() != test.exitNodeIPWant {
+				t.Errorf("got %v want %v", got, test.exitNodeIPWant)
 			}
 
 			if changed != test.prefsChanged {
@@ -1600,3 +1610,244 @@ func TestSetExitNodeIDPolicy(t *testing.T) {
 		})
 	}
 }
+
+func TestApplySysPolicy(t *testing.T) {
+	tests := []struct {
+		name           string
+		prefs          ipn.Prefs
+		wantPrefs      ipn.Prefs
+		wantAnyChange  bool
+		stringPolicies map[syspolicy.Key]string
+	}{
+		{
+			name: "empty prefs without policies",
+		},
+		{
+			name: "prefs set without policies",
+			prefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			wantPrefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+		},
+		{
+			name: "empty prefs with policies",
+			wantPrefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.ControlURL:                "1",
+				syspolicy.EnableIncomingConnections: "never",
+				syspolicy.EnableServerMode:          "always",
+			},
+		},
+		{
+			name: "prefs set with matching policies",
+			prefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			wantPrefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.ControlURL:                "1",
+				syspolicy.EnableIncomingConnections: "never",
+				syspolicy.EnableServerMode:          "always",
+			},
+		},
+		{
+			name: "prefs set with conflicting policies",
+			prefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			wantPrefs: ipn.Prefs{
+				ControlURL:  "2",
+				ShieldsUp:   false,
+				ForceDaemon: false,
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.ControlURL:                "2",
+				syspolicy.EnableIncomingConnections: "always",
+				syspolicy.EnableServerMode:          "never",
+			},
+		},
+		{
+			name: "prefs set with neutral policies",
+			prefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			wantPrefs: ipn.Prefs{
+				ControlURL:  "1",
+				ShieldsUp:   true,
+				ForceDaemon: true,
+			},
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.EnableIncomingConnections: "user-decides",
+				syspolicy.EnableServerMode:          "user-decides",
+			},
+		},
+		{
+			name: "ControlURL",
+			wantPrefs: ipn.Prefs{
+				ControlURL: "set",
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.ControlURL: "set",
+			},
+		},
+		{
+			name: "always incoming connections",
+			prefs: ipn.Prefs{
+				ShieldsUp: true,
+			},
+			wantPrefs: ipn.Prefs{
+				ShieldsUp: false,
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.EnableIncomingConnections: "always",
+			},
+		},
+		{
+			name: "never incoming connections",
+			prefs: ipn.Prefs{
+				ShieldsUp: false,
+			},
+			wantPrefs: ipn.Prefs{
+				ShieldsUp: true,
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.EnableIncomingConnections: "never",
+			},
+		},
+		{
+			name: "always server mode",
+			prefs: ipn.Prefs{
+				ForceDaemon: false,
+			},
+			wantPrefs: ipn.Prefs{
+				ForceDaemon: true,
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.EnableServerMode: "always",
+			},
+		},
+		{
+			name: "never server mode",
+			prefs: ipn.Prefs{
+				ForceDaemon: true,
+			},
+			wantPrefs: ipn.Prefs{
+				ForceDaemon: false,
+			},
+			wantAnyChange: true,
+			stringPolicies: map[syspolicy.Key]string{
+				syspolicy.EnableServerMode: "never",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Run("unit", func(t *testing.T) {
+				// Be strict when testing the func directly.
+				msh := &mockSyspolicyHandler{
+					t: t,
+					stringPolicies: map[syspolicy.Key]*string{
+						syspolicy.ControlURL:                nil,
+						syspolicy.EnableIncomingConnections: nil,
+						syspolicy.EnableServerMode:          nil,
+					},
+					failUnknownPolicies: true,
+				}
+				for p, v := range tt.stringPolicies {
+					v := v // construct a unique pointer for each policy value
+					msh.stringPolicies[p] = &v
+				}
+				syspolicy.SetHandlerForTest(t, msh)
+
+				prefs := tt.prefs.Clone()
+
+				gotAnyChange := applySysPolicy(prefs)
+
+				if gotAnyChange && prefs.Equals(&tt.prefs) {
+					t.Errorf("anyChange but prefs is unchanged: %v", prefs.Pretty())
+				}
+				if !gotAnyChange && !prefs.Equals(&tt.prefs) {
+					t.Errorf("!anyChange but prefs changed from %v to %v", tt.prefs.Pretty(), prefs.Pretty())
+				}
+				if gotAnyChange != tt.wantAnyChange {
+					t.Errorf("anyChange=%v, want %v", gotAnyChange, tt.wantAnyChange)
+				}
+				if !prefs.Equals(&tt.wantPrefs) {
+					t.Errorf("prefs=%v, want %v", prefs.Pretty(), tt.wantPrefs.Pretty())
+				}
+			})
+			// Create a more relaxed syspolicy for integration tests.
+			msh := &mockSyspolicyHandler{
+				t:              t,
+				stringPolicies: make(map[syspolicy.Key]*string, len(tt.stringPolicies)),
+			}
+			for p, v := range tt.stringPolicies {
+				v := v // construct a unique pointer for each policy value
+				msh.stringPolicies[p] = &v
+			}
+			syspolicy.SetHandlerForTest(t, msh)
+
+			t.Run("set prefs", func(t *testing.T) {
+
+				b := newTestBackend(t)
+				b.SetPrefs(tt.prefs.Clone())
+				if !b.Prefs().Equals(tt.wantPrefs.View()) {
+					t.Errorf("prefs=%v, want %v", b.Prefs().Pretty(), tt.wantPrefs.Pretty())
+				}
+			})
+
+			t.Run("status update", func(t *testing.T) {
+				// Profile manager fills in blank ControlURL but it's not set
+				// in most test cases to avoid cluttering them, so adjust for
+				// that.
+				usePrefs := tt.prefs.Clone()
+				if usePrefs.ControlURL == "" {
+					usePrefs.ControlURL = ipn.DefaultControlURL
+				}
+				wantPrefs := tt.wantPrefs.Clone()
+				if wantPrefs.ControlURL == "" {
+					wantPrefs.ControlURL = ipn.DefaultControlURL
+				}
+
+				pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+				pm.prefs = usePrefs.View()
+
+				b := newTestBackend(t)
+				b.mu.Lock()
+				b.pm = pm
+				b.mu.Unlock()
+
+				b.SetControlClientStatus(b.cc, controlclient.Status{})
+				if !b.Prefs().Equals(wantPrefs.View()) {
+					t.Errorf("prefs=%v, want %v", b.Prefs().Pretty(), wantPrefs.Pretty())
+				}
+			})
+		})
+	}
+}

ipn/ipnlocal/profiles.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
-	"net/netip"
 	"runtime"
 	"slices"
 	"strings"
@@ -19,7 +18,6 @@ import (
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
 	"tailscale.com/util/cmpx"
-	"tailscale.com/util/winutil"
 )
 
 var errAlreadyMigrated = errors.New("profile migration already completed")
@@ -443,37 +441,18 @@ func (pm *profileManager) NewProfile() {
 	pm.currentProfile = &ipn.LoginProfile{}
 }
 
-// defaultPrefs is the default prefs for a new profile.
+// defaultPrefs is the default prefs for a new profile. This initializes before
+// even this package's init() so do not rely on other parts of the system being
+// fully initialized here (for example, syspolicy will not be available on
+// Apple platforms).
 var defaultPrefs = func() ipn.PrefsView {
 	prefs := ipn.NewPrefs()
 	prefs.LoggedOut = true
 	prefs.WantRunning = false
 
-	controlURL, _ := winutil.GetPolicyString("LoginURL")
-	prefs.ControlURL = controlURL
-
-	prefs.ExitNodeIP = resolveExitNodeIP(netip.Addr{})
-
-	// Allow Incoming (used by the UI) is the negation of ShieldsUp (used by the
-	// backend), so this has to convert between the two conventions.
-	shieldsUp, _ := winutil.GetPolicyString("AllowIncomingConnections")
-	prefs.ShieldsUp = shieldsUp == "never"
-	forceDaemon, _ := winutil.GetPolicyString("UnattendedMode")
-	prefs.ForceDaemon = forceDaemon == "always"
-
 	return prefs.View()
 }()
 
-func resolveExitNodeIP(defIP netip.Addr) (ret netip.Addr) {
-	ret = defIP
-	if exitNode, _ := winutil.GetPolicyString("ExitNodeIP"); exitNode != "" {
-		if ip, err := netip.ParseAddr(exitNode); err == nil {
-			ret = ip
-		}
-	}
-	return ret
-}
-
 // Store returns the StateStore used by the ProfileManager.
 func (pm *profileManager) Store() ipn.StateStore {
 	return pm.store

ipn/ipnlocal/profiles_windows.go

@@ -67,9 +67,6 @@ func (pm *profileManager) loadLegacyPrefs() (string, ipn.PrefsView, error) {
 	}
 
 	prefs.ControlURL = policy.SelectControlURL(defaultPrefs.ControlURL(), prefs.ControlURL)
-	prefs.ExitNodeIP = resolveExitNodeIP(prefs.ExitNodeIP)
-	prefs.ShieldsUp = resolveShieldsUp(prefs.ShieldsUp)
-	prefs.ForceDaemon = resolveForceDaemon(prefs.ForceDaemon)
 
 	pm.logf("migrating Windows profile to new format")
 	return migrationSentinel, prefs.View(), nil
@@ -78,13 +75,3 @@ func (pm *profileManager) loadLegacyPrefs() (string, ipn.PrefsView, error) {
 func (pm *profileManager) completeMigration(migrationSentinel string) {
 	atomicfile.WriteFile(migrationSentinel, []byte{}, 0600)
 }
-
-func resolveShieldsUp(defval bool) bool {
-	pol := policy.GetPreferenceOptionPolicy("AllowIncomingConnections")
-	return !pol.ShouldEnable(!defval)
-}
-
-func resolveForceDaemon(defval bool) bool {
-	pol := policy.GetPreferenceOptionPolicy("UnattendedMode")
-	return pol.ShouldEnable(defval)
-}

util/syspolicy/policy_keys.go

@@ -17,7 +17,8 @@ const (
 	ExitNodeIP Key = "ExitNodeIP" // default ""; if blank, no exit node is forced. Value is exit node IP.
 
 	// Keys with a string value that specifies an option: "always", "never", "user-decides".
-	// The default is "user-decides" unless otherwise stated.
+	// The default is "user-decides" unless otherwise stated. Enforcement of
+	// these policies is typically performed in ipnlocal.applySysPolicy().
 	EnableIncomingConnections Key = "AllowIncomingConnections"
 	EnableServerMode          Key = "UnattendedMode"
 	ExitNodeAllowLANAccess    Key = "ExitNodeAllowLANAccess"
@@ -25,7 +26,9 @@ const (
 	EnableTailscaleSubnets    Key = "UseTailscaleSubnets"
 
 	// Keys with a string value that controls visibility: "show", "hide".
-	// The default is "show" unless otherwise stated.
+	// The default is "show" unless otherwise stated. Enforcement of these
+	// policies is typically performed by the UI code for the relevant operating
+	// system.
 	AdminConsoleVisibility    Key = "AdminConsole"
 	NetworkDevicesVisibility  Key = "NetworkDevices"
 	TestMenuVisibility        Key = "TestMenu"

util/syspolicy/syspolicy.go

@@ -68,6 +68,12 @@ func (p PreferenceOption) ShouldEnable(userChoice bool) bool {
 	}
 }
 
+// WillOverride checks if the choice administered by the policy is different
+// from the user's choice.
+func (p PreferenceOption) WillOverride(userChoice bool) bool {
+	return p.ShouldEnable(userChoice) != userChoice
+}
+
 // GetPreferenceOption loads a policy from the registry that can be
 // managed by an enterprise policy management system and allows administrative
 // overrides of users' choices in a way that we do not want tailcontrol to have