diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index cbbea32aa..7c0ddc90c 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1489,10 +1489,10 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control b.logf("SetControlClientStatus failed to select auto exit node: %v", err) } } - if setExitNodeID(prefs, curNetMap, b.lastSuggestedExitNode) { + if applySysPolicy(prefs, b.lastSuggestedExitNode) { prefsChanged = true } - if applySysPolicy(prefs) { + if setExitNodeID(prefs, curNetMap) { prefsChanged = true } @@ -1658,12 +1658,37 @@ type preferencePolicyInfo struct { // 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) { +func applySysPolicy(prefs *ipn.Prefs, lastSuggestedExitNode tailcfg.StableNodeID) (anyChange bool) { if controlURL, err := syspolicy.GetString(syspolicy.ControlURL, prefs.ControlURL); err == nil && prefs.ControlURL != controlURL { prefs.ControlURL = controlURL anyChange = true } + if exitNodeIDStr, _ := syspolicy.GetString(syspolicy.ExitNodeID, ""); exitNodeIDStr != "" { + exitNodeID := tailcfg.StableNodeID(exitNodeIDStr) + if shouldAutoExitNode() && lastSuggestedExitNode != "" { + exitNodeID = lastSuggestedExitNode + } + // Note: when exitNodeIDStr == "auto" && lastSuggestedExitNode == "", + // then exitNodeID is now "auto" which will never match a peer's node ID. + // When there is no a peer matching the node ID, traffic will blackhole, + // preventing accidental non-exit-node usage when a policy is in effect that requires an exit node. + if prefs.ExitNodeID != exitNodeID || prefs.ExitNodeIP.IsValid() { + anyChange = true + } + prefs.ExitNodeID = exitNodeID + prefs.ExitNodeIP = netip.Addr{} + } else if exitNodeIPStr, _ := syspolicy.GetString(syspolicy.ExitNodeIP, ""); exitNodeIPStr != "" { + exitNodeIP, err := netip.ParseAddr(exitNodeIPStr) + if exitNodeIP.IsValid() && err == nil { + if prefs.ExitNodeID != "" || prefs.ExitNodeIP != exitNodeIP { + anyChange = true + } + prefs.ExitNodeID = "" + prefs.ExitNodeIP = exitNodeIP + } + } + for _, opt := range preferencePolicies { if po, err := syspolicy.GetPreferenceOption(opt.key); err == nil { curVal := opt.get(prefs.View()) @@ -1770,30 +1795,7 @@ func (b *LocalBackend) updateNetmapDeltaLocked(muts []netmap.NodeMutation) (hand // setExitNodeID updates prefs to reference an exit node by ID, rather // than by IP. It returns whether prefs was mutated. -func setExitNodeID(prefs *ipn.Prefs, nm *netmap.NetworkMap, lastSuggestedExitNode tailcfg.StableNodeID) (prefsChanged bool) { - if exitNodeIDStr, _ := syspolicy.GetString(syspolicy.ExitNodeID, ""); exitNodeIDStr != "" { - exitNodeID := tailcfg.StableNodeID(exitNodeIDStr) - if shouldAutoExitNode() && lastSuggestedExitNode != "" { - exitNodeID = lastSuggestedExitNode - } - // Note: when exitNodeIDStr == "auto" && lastSuggestedExitNode == "", then exitNodeID is now "auto" which will never match a peer's node ID. - // When there is no a peer matching the node ID, traffic will blackhole, preventing accidental non-exit-node usage when a policy is in effect that requires an exit node. - changed := prefs.ExitNodeID != exitNodeID || prefs.ExitNodeIP.IsValid() - prefs.ExitNodeID = exitNodeID - prefs.ExitNodeIP = netip.Addr{} - return changed - } - - oldExitNodeID := prefs.ExitNodeID - if exitNodeIPStr, _ := syspolicy.GetString(syspolicy.ExitNodeIP, ""); exitNodeIPStr != "" { - exitNodeIP, err := netip.ParseAddr(exitNodeIPStr) - if exitNodeIP.IsValid() && err == nil { - prefsChanged = prefs.ExitNodeID != "" || prefs.ExitNodeIP != exitNodeIP - prefs.ExitNodeID = "" - prefs.ExitNodeIP = exitNodeIP - } - } - +func setExitNodeID(prefs *ipn.Prefs, nm *netmap.NetworkMap) (prefsChanged bool) { if nm == nil { // No netmap, can't resolve anything. return false @@ -1811,6 +1813,7 @@ func setExitNodeID(prefs *ipn.Prefs, nm *netmap.NetworkMap, lastSuggestedExitNod prefsChanged = true } + oldExitNodeID := prefs.ExitNodeID for _, peer := range nm.Peers { for _, addr := range peer.Addresses().All() { if !addr.IsSingleIP() || addr.Addr() != prefs.ExitNodeIP { @@ -1820,7 +1823,7 @@ func setExitNodeID(prefs *ipn.Prefs, nm *netmap.NetworkMap, lastSuggestedExitNod // reference it directly for next time. prefs.ExitNodeID = peer.StableID() prefs.ExitNodeIP = netip.Addr{} - return oldExitNodeID != prefs.ExitNodeID + return prefsChanged || oldExitNodeID != prefs.ExitNodeID } } @@ -3844,12 +3847,12 @@ func (b *LocalBackend) setPrefsLockedOnEntry(newp *ipn.Prefs, unlock unlockOnce) if oldp.Valid() { newp.Persist = oldp.Persist().AsStruct() // caller isn't allowed to override this } - // 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, b.lastSuggestedExitNode) - // applySysPolicy does likewise so we can also ignore its return value. - applySysPolicy(newp) + // applySysPolicyToPrefsLocked returns whether it updated newp, + // but everything in this function treats b.prefs as completely new + // anyway, so its return value can be ignored here. + applySysPolicy(newp, b.lastSuggestedExitNode) + // setExitNodeID does likewise. No-op if no exit node resolution is needed. + setExitNodeID(newp, netMap) // 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 f30ff6adb..c5bd51265 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -1789,10 +1789,13 @@ func TestSetExitNodeIDPolicy(t *testing.T) { t.Run(test.name, func(t *testing.T) { b := newTestBackend(t) - policyStore := source.NewTestStoreOf(t, - source.TestSettingOf(syspolicy.ExitNodeID, test.exitNodeID), - source.TestSettingOf(syspolicy.ExitNodeIP, test.exitNodeIP), - ) + policyStore := source.NewTestStore(t) + if test.exitNodeIDKey { + policyStore.SetStrings(source.TestSettingOf(syspolicy.ExitNodeID, test.exitNodeID)) + } + if test.exitNodeIPKey { + policyStore.SetStrings(source.TestSettingOf(syspolicy.ExitNodeIP, test.exitNodeIP)) + } syspolicy.MustRegisterStoreForTest(t, "TestStore", setting.DeviceScope, policyStore) if test.nm == nil { @@ -1806,7 +1809,16 @@ func TestSetExitNodeIDPolicy(t *testing.T) { b.netMap = test.nm b.pm = pm b.lastSuggestedExitNode = test.lastSuggestedExitNode - changed := setExitNodeID(b.pm.prefs.AsStruct(), test.nm, tailcfg.StableNodeID(test.lastSuggestedExitNode)) + + prefs := b.pm.prefs.AsStruct() + if changed := applySysPolicy(prefs, test.lastSuggestedExitNode) || setExitNodeID(prefs, test.nm); changed != test.prefsChanged { + t.Errorf("wanted prefs changed %v, got prefs changed %v", test.prefsChanged, changed) + } + + // Both [LocalBackend.SetPrefsForTest] and [LocalBackend.EditPrefs] + // apply syspolicy settings to the current profile's preferences. Therefore, + // we pass the current, unmodified preferences and expect the effective + // preferences to change. b.SetPrefsForTest(pm.CurrentPrefs().AsStruct()) if got := b.pm.prefs.ExitNodeID(); got != tailcfg.StableNodeID(test.exitNodeIDWant) { @@ -1819,10 +1831,6 @@ func TestSetExitNodeIDPolicy(t *testing.T) { } else if got.String() != test.exitNodeIPWant { t.Errorf("got %v want %v", got, test.exitNodeIPWant) } - - if changed != test.prefsChanged { - t.Errorf("wanted prefs changed %v, got prefs changed %v", test.prefsChanged, changed) - } }) } } @@ -2332,7 +2340,7 @@ func TestApplySysPolicy(t *testing.T) { t.Run("unit", func(t *testing.T) { prefs := tt.prefs.Clone() - gotAnyChange := applySysPolicy(prefs) + gotAnyChange := applySysPolicy(prefs, "") if gotAnyChange && prefs.Equals(&tt.prefs) { t.Errorf("anyChange but prefs is unchanged: %v", prefs.Pretty()) @@ -2480,7 +2488,7 @@ func TestPreferencePolicyInfo(t *testing.T) { prefs := defaultPrefs.AsStruct() pp.set(prefs, tt.initialValue) - gotAnyChange := applySysPolicy(prefs) + gotAnyChange := applySysPolicy(prefs, "") if gotAnyChange != tt.wantChange { t.Errorf("anyChange=%v, want %v", gotAnyChange, tt.wantChange)