From eb3cd3291106dc603316e4df65ad85cc0d3b3e6b Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Fri, 22 Nov 2024 08:45:53 -0600 Subject: [PATCH] ipn/ipnlocal: update ipn.Prefs when there's a change in syspolicy settings In this PR, we update ipnlocal.NewLocalBackend to subscribe to policy change notifications and reapply syspolicy settings to the current profile's ipn.Prefs whenever a change occurs. Updates #12687 Signed-off-by: Nick Khyl --- ipn/ipnlocal/local.go | 102 ++++++++++++++++++++++-------- ipn/ipnlocal/local_test.go | 123 +++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 26 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 7c0ddc90c..8763581f1 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -106,6 +106,7 @@ "tailscale.com/util/rands" "tailscale.com/util/set" "tailscale.com/util/syspolicy" + "tailscale.com/util/syspolicy/rsop" "tailscale.com/util/systemd" "tailscale.com/util/testenv" "tailscale.com/util/uniq" @@ -178,27 +179,28 @@ type watchSession struct { // state machine generates events back out to zero or more components. type LocalBackend struct { // Elements that are thread-safe or constant after construction. - ctx context.Context // canceled by Close - ctxCancel context.CancelFunc // cancels ctx - logf logger.Logf // general logging - keyLogf logger.Logf // for printing list of peers on change - statsLogf logger.Logf // for printing peers stats on change - sys *tsd.System - health *health.Tracker // always non-nil - metrics metrics - e wgengine.Engine // non-nil; TODO(bradfitz): remove; use sys - store ipn.StateStore // non-nil; TODO(bradfitz): remove; use sys - dialer *tsdial.Dialer // non-nil; TODO(bradfitz): remove; use sys - pushDeviceToken syncs.AtomicValue[string] - backendLogID logid.PublicID - unregisterNetMon func() - unregisterHealthWatch func() - portpoll *portlist.Poller // may be nil - portpollOnce sync.Once // guards starting readPoller - varRoot string // or empty if SetVarRoot never called - logFlushFunc func() // or nil if SetLogFlusher wasn't called - em *expiryManager // non-nil - sshAtomicBool atomic.Bool + ctx context.Context // canceled by Close + ctxCancel context.CancelFunc // cancels ctx + logf logger.Logf // general logging + keyLogf logger.Logf // for printing list of peers on change + statsLogf logger.Logf // for printing peers stats on change + sys *tsd.System + health *health.Tracker // always non-nil + metrics metrics + e wgengine.Engine // non-nil; TODO(bradfitz): remove; use sys + store ipn.StateStore // non-nil; TODO(bradfitz): remove; use sys + dialer *tsdial.Dialer // non-nil; TODO(bradfitz): remove; use sys + pushDeviceToken syncs.AtomicValue[string] + backendLogID logid.PublicID + unregisterNetMon func() + unregisterHealthWatch func() + unregisterSysPolicyWatch func() + portpoll *portlist.Poller // may be nil + portpollOnce sync.Once // guards starting readPoller + varRoot string // or empty if SetVarRoot never called + logFlushFunc func() // or nil if SetLogFlusher wasn't called + em *expiryManager // non-nil + sshAtomicBool atomic.Bool // webClientAtomicBool controls whether the web client is running. This should // be true unless the disable-web-client node attribute has been set. webClientAtomicBool atomic.Bool @@ -410,7 +412,7 @@ type metrics struct { // but is not actually running. // // If dialer is nil, a new one is made. -func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, loginFlags controlclient.LoginFlags) (*LocalBackend, error) { +func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, loginFlags controlclient.LoginFlags) (_ *LocalBackend, err error) { e := sys.Engine.Get() store := sys.StateStore.Get() dialer := sys.Dialer.Get() @@ -485,6 +487,15 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo } } + if b.unregisterSysPolicyWatch, err = b.registerSysPolicyWatch(); err != nil { + return nil, err + } + defer func() { + if err != nil { + b.unregisterSysPolicyWatch() + } + }() + netMon := sys.NetMon.Get() b.sockstatLogger, err = sockstatlog.NewLogger(logpolicy.LogsDir(logf), logf, logID, netMon, sys.HealthTracker()) if err != nil { @@ -981,6 +992,7 @@ func (b *LocalBackend) Shutdown() { b.unregisterNetMon() b.unregisterHealthWatch() + b.unregisterSysPolicyWatch() if cc != nil { cc.Shutdown() } @@ -1703,6 +1715,40 @@ func applySysPolicy(prefs *ipn.Prefs, lastSuggestedExitNode tailcfg.StableNodeID return anyChange } +// registerSysPolicyWatch subscribes to syspolicy change notifications +// and immediately applies the effective syspolicy settings to the current profile. +func (b *LocalBackend) registerSysPolicyWatch() (unregister func(), err error) { + if unregister, err = syspolicy.RegisterChangeCallback(b.sysPolicyChanged); err != nil { + return nil, fmt.Errorf("syspolicy: LocalBacked failed to register policy change callback: %v", err) + } + if prefs, anyChange := b.applySysPolicy(); anyChange { + b.logf("syspolicy: changed initial profile prefs: %v", prefs.Pretty()) + } + return unregister, nil +} + +// applySysPolicy overwrites the current profile's preferences with policies +// that may be configured by the system administrator in an OS-specific way. +// +// b.mu must not be held. +func (b *LocalBackend) applySysPolicy() (_ ipn.PrefsView, anyChange bool) { + unlock := b.lockAndGetUnlock() + prefs := b.pm.CurrentPrefs().AsStruct() + if !applySysPolicy(prefs, b.lastSuggestedExitNode) { + unlock.UnlockEarly() + return prefs.View(), false + } + return b.setPrefsLockedOnEntry(prefs, unlock), true +} + +// sysPolicyChanged is a callback triggered by syspolicy when it detects +// a change in one or more syspolicy settings. +func (b *LocalBackend) sysPolicyChanged(*rsop.PolicyChange) { + if prefs, anyChange := b.applySysPolicy(); anyChange { + b.logf("syspolicy: changed profile prefs: %v", prefs.Pretty()) + } +} + var _ controlclient.NetmapDeltaUpdater = (*LocalBackend)(nil) // UpdateNetmapDelta implements controlclient.NetmapDeltaUpdater. @@ -3889,10 +3935,14 @@ func (b *LocalBackend) setPrefsLockedOnEntry(newp *ipn.Prefs, unlock unlockOnce) } prefs := newp.View() - if err := b.pm.SetPrefs(prefs, ipn.NetworkProfile{ - MagicDNSName: b.netMap.MagicDNSSuffix(), - DomainName: b.netMap.DomainName(), - }); err != nil { + np := b.pm.CurrentProfile().NetworkProfile + if netMap != nil { + np = ipn.NetworkProfile{ + MagicDNSName: b.netMap.MagicDNSSuffix(), + DomainName: b.netMap.DomainName(), + } + } + if err := b.pm.SetPrefs(prefs, np); err != nil { b.logf("failed to save new controlclient state: %v", err) } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index c5bd51265..b1be86392 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -4562,3 +4562,126 @@ func TestGetVIPServices(t *testing.T) { }) } } + +func TestUpdatePrefsOnSysPolicyChange(t *testing.T) { + const enableLogging = false + + type fieldChange struct { + name string + want any + } + + wantPrefsChanges := func(want ...fieldChange) *wantedNotification { + return &wantedNotification{ + name: "Prefs", + cond: func(t testing.TB, actor ipnauth.Actor, n *ipn.Notify) bool { + if n.Prefs != nil { + prefs := reflect.Indirect(reflect.ValueOf(n.Prefs.AsStruct())) + for _, f := range want { + got := prefs.FieldByName(f.name).Interface() + if !reflect.DeepEqual(got, f.want) { + t.Errorf("%v: got %v; want %v", f.name, got, f.want) + } + } + } + return n.Prefs != nil + }, + } + } + + unexpectedPrefsChange := func(t testing.TB, _ ipnauth.Actor, n *ipn.Notify) bool { + if n.Prefs != nil { + t.Errorf("Unexpected Prefs: %v", n.Prefs.Pretty()) + return true + } + return false + } + + tests := []struct { + name string + initialPrefs *ipn.Prefs + stringSettings []source.TestSetting[string] + want *wantedNotification + }{ + { + name: "ShieldsUp/True", + stringSettings: []source.TestSetting[string]{source.TestSettingOf(syspolicy.EnableIncomingConnections, "never")}, + want: wantPrefsChanges(fieldChange{"ShieldsUp", true}), + }, + { + name: "ShieldsUp/False", + initialPrefs: &ipn.Prefs{ShieldsUp: true}, + stringSettings: []source.TestSetting[string]{source.TestSettingOf(syspolicy.EnableIncomingConnections, "always")}, + want: wantPrefsChanges(fieldChange{"ShieldsUp", false}), + }, + { + name: "ExitNodeID", + stringSettings: []source.TestSetting[string]{source.TestSettingOf(syspolicy.ExitNodeID, "foo")}, + want: wantPrefsChanges(fieldChange{"ExitNodeID", tailcfg.StableNodeID("foo")}), + }, + { + name: "EnableRunExitNode", + stringSettings: []source.TestSetting[string]{source.TestSettingOf(syspolicy.EnableRunExitNode, "always")}, + want: wantPrefsChanges(fieldChange{"AdvertiseRoutes", []netip.Prefix{tsaddr.AllIPv4(), tsaddr.AllIPv6()}}), + }, + { + name: "Multiple", + initialPrefs: &ipn.Prefs{ + ExitNodeAllowLANAccess: true, + }, + stringSettings: []source.TestSetting[string]{ + source.TestSettingOf(syspolicy.EnableServerMode, "always"), + source.TestSettingOf(syspolicy.ExitNodeAllowLANAccess, "never"), + source.TestSettingOf(syspolicy.ExitNodeIP, "127.0.0.1"), + }, + want: wantPrefsChanges( + fieldChange{"ForceDaemon", true}, + fieldChange{"ExitNodeAllowLANAccess", false}, + fieldChange{"ExitNodeIP", netip.MustParseAddr("127.0.0.1")}, + ), + }, + { + name: "NoChange", + initialPrefs: &ipn.Prefs{ + CorpDNS: true, + ExitNodeID: "foo", + AdvertiseRoutes: []netip.Prefix{tsaddr.AllIPv4(), tsaddr.AllIPv6()}, + }, + stringSettings: []source.TestSetting[string]{ + source.TestSettingOf(syspolicy.EnableTailscaleDNS, "always"), + source.TestSettingOf(syspolicy.ExitNodeID, "foo"), + source.TestSettingOf(syspolicy.EnableRunExitNode, "always"), + }, + want: nil, // syspolicy settings match the preferences; no change notification is expected. + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + syspolicy.RegisterWellKnownSettingsForTest(t) + store := source.NewTestStoreOf[string](t) + syspolicy.MustRegisterStoreForTest(t, "TestSource", setting.DeviceScope, store) + + lb := newLocalBackendWithTestControl(t, enableLogging, func(tb testing.TB, opts controlclient.Options) controlclient.Client { + return newClient(tb, opts) + }) + if tt.initialPrefs != nil { + lb.SetPrefsForTest(tt.initialPrefs) + } + if err := lb.Start(ipn.Options{}); err != nil { + t.Fatalf("(*LocalBackend).Start(): %v", err) + } + + nw := newNotificationWatcher(t, lb, &ipnauth.TestActor{}) + if tt.want != nil { + nw.watch(0, []wantedNotification{*tt.want}) + } else { + nw.watch(0, nil, unexpectedPrefsChange) + } + + store.SetStrings(tt.stringSettings...) + + nw.check() + }) + } +}