From af32d1c12003af0878361dcce60563bd28520299 Mon Sep 17 00:00:00 2001 From: Adrian Dewhurst Date: Wed, 1 Nov 2023 17:20:25 -0400 Subject: [PATCH] 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 --- ipn/ipnlocal/local.go | 36 +++- ipn/ipnlocal/local_test.go | 317 +++++++++++++++++++++++++++---- ipn/ipnlocal/profiles.go | 29 +-- ipn/ipnlocal/profiles_windows.go | 13 -- util/syspolicy/policy_keys.go | 7 +- util/syspolicy/syspolicy.go | 6 + 6 files changed, 334 insertions(+), 74 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 1fe42840b..0bfef49bc 100644 --- a/ipn/ipnlocal/local.go +++ b/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 diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index e41a818a4..18166b65a 100644 --- a/ipn/ipnlocal/local_test.go +++ b/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()) + } + }) + }) + } +} diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index 55bb6a1a1..ad2482178 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -8,7 +8,6 @@ "errors" "fmt" "math/rand" - "net/netip" "runtime" "slices" "strings" @@ -19,7 +18,6 @@ "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 diff --git a/ipn/ipnlocal/profiles_windows.go b/ipn/ipnlocal/profiles_windows.go index 22da3ff69..276b20ba9 100644 --- a/ipn/ipnlocal/profiles_windows.go +++ b/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) -} diff --git a/util/syspolicy/policy_keys.go b/util/syspolicy/policy_keys.go index 75393f9b2..60a87aa14 100644 --- a/util/syspolicy/policy_keys.go +++ b/util/syspolicy/policy_keys.go @@ -17,7 +17,8 @@ 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 @@ 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" diff --git a/util/syspolicy/syspolicy.go b/util/syspolicy/syspolicy.go index 2f8cf9dcb..6d44d4d74 100644 --- a/util/syspolicy/syspolicy.go +++ b/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