diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 5fbb0bd98..6120c52c6 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2026,7 +2026,20 @@ func mutationsAreWorthyOfTellingIPNBus(muts []netmap.NodeMutation) bool { // or resolve ExitNodeIP to an ID and use that. It returns whether prefs was mutated. func setExitNodeID(prefs *ipn.Prefs, suggestedExitNodeID tailcfg.StableNodeID, nm *netmap.NetworkMap) (prefsChanged bool) { if prefs.AutoExitNode.IsSet() { - newExitNodeID := cmp.Or(suggestedExitNodeID, unresolvedExitNodeID) + var newExitNodeID tailcfg.StableNodeID + if !suggestedExitNodeID.IsZero() { + // If we have a suggested exit node, use it. + newExitNodeID = suggestedExitNodeID + } else if isAllowedAutoExitNodeID(prefs.ExitNodeID) { + // If we don't have a suggested exit node, but the prefs already + // specify an allowed auto exit node ID, retain it. + newExitNodeID = prefs.ExitNodeID + } else { + // Otherwise, use [unresolvedExitNodeID] to install a blackhole route, + // preventing traffic from leaking to the local network until an actual + // exit node is selected. + newExitNodeID = unresolvedExitNodeID + } if prefs.ExitNodeID != newExitNodeID { prefs.ExitNodeID = newExitNodeID prefsChanged = true diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 5c9adfb5f..c9bad838e 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -620,6 +620,7 @@ func TestConfigureExitNode(t *testing.T) { useExitNodeEnabled *bool exitNodeIDPolicy *tailcfg.StableNodeID exitNodeIPPolicy *netip.Addr + exitNodeAllowedIDs []tailcfg.StableNodeID // nil if all IDs are allowed for auto exit nodes wantPrefs ipn.Prefs }{ { @@ -894,6 +895,91 @@ func TestConfigureExitNode(t *testing.T) { AutoExitNode: "any", }, }, + { + name: "auto-any-via-policy/no-netmap/with-existing", // set auto exit node via syspolicy without a netmap, but with a previously set exit node ID + prefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), // should be retained + }, + netMap: nil, + report: report, + exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), + exitNodeAllowedIDs: nil, // not configured, so all exit node IDs are implicitly allowed + wantPrefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), + AutoExitNode: "any", + }, + }, + { + name: "auto-any-via-policy/no-netmap/with-allowed-existing", // same, but now with a syspolicy setting that explicitly allows the existing exit node ID + prefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), // should be retained + }, + netMap: nil, + report: report, + exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), + exitNodeAllowedIDs: []tailcfg.StableNodeID{ + exitNode2.StableID(), // the current exit node ID is allowed + }, + wantPrefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), + AutoExitNode: "any", + }, + }, + { + name: "auto-any-via-policy/no-netmap/with-disallowed-existing", // same, but now with a syspolicy setting that does not allow the existing exit node ID + prefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), // not allowed by [syspolicy.AllowedSuggestedExitNodes] + }, + netMap: nil, + report: report, + exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), + exitNodeAllowedIDs: []tailcfg.StableNodeID{ + exitNode1.StableID(), // a different exit node ID; the current one is not allowed + }, + wantPrefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: unresolvedExitNodeID, // we don't have a netmap yet, and the current exit node ID is not allowed; block traffic + AutoExitNode: "any", + }, + }, + { + name: "auto-any-via-policy/with-netmap/with-allowed-existing", // same, but now with a syspolicy setting that does not allow the existing exit node ID + prefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode1.StableID(), // not allowed by [syspolicy.AllowedSuggestedExitNodes] + }, + netMap: clientNetmap, + report: report, + exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), + exitNodeAllowedIDs: []tailcfg.StableNodeID{ + exitNode2.StableID(), // a different exit node ID; the current one is not allowed + }, + wantPrefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), // we have a netmap; switch to the best allowed exit node + AutoExitNode: "any", + }, + }, + { + name: "auto-any-via-policy/with-netmap/switch-to-better", // if all exit nodes are allowed, switch to the best one once we have a netmap + prefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode2.StableID(), + }, + netMap: clientNetmap, + report: report, + exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), + wantPrefs: ipn.Prefs{ + ControlURL: controlURL, + ExitNodeID: exitNode1.StableID(), // switch to the best exit node + AutoExitNode: "any", + }, + }, { name: "auto-foo-via-policy", // set auto exit node via syspolicy with an unknown/unsupported expression prefs: ipn.Prefs{ @@ -929,19 +1015,23 @@ func TestConfigureExitNode(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Configure policy settings, if any. - var settings []source.TestSetting[string] + store := source.NewTestStore(t) if tt.exitNodeIDPolicy != nil { - settings = append(settings, source.TestSettingOf(syspolicy.ExitNodeID, string(*tt.exitNodeIDPolicy))) + store.SetStrings(source.TestSettingOf(syspolicy.ExitNodeID, string(*tt.exitNodeIDPolicy))) } if tt.exitNodeIPPolicy != nil { - settings = append(settings, source.TestSettingOf(syspolicy.ExitNodeIP, tt.exitNodeIPPolicy.String())) + store.SetStrings(source.TestSettingOf(syspolicy.ExitNodeIP, tt.exitNodeIPPolicy.String())) } - if settings != nil { - syspolicy.MustRegisterStoreForTest(t, "TestStore", setting.DeviceScope, source.NewTestStoreOf(t, settings...)) - } else { + if tt.exitNodeAllowedIDs != nil { + store.SetStringLists(source.TestSettingOf(syspolicy.AllowedSuggestedExitNodes, toStrings(tt.exitNodeAllowedIDs))) + } + if store.IsEmpty() { // No syspolicy settings, so don't register a store. // This allows the test to run in parallel with other tests. t.Parallel() + } else { + // Register the store for syspolicy settings to make them available to the LocalBackend. + syspolicy.MustRegisterStoreForTest(t, "TestStore", setting.DeviceScope, store) } // Create a new LocalBackend with the given prefs. @@ -6127,3 +6217,11 @@ func TestDisplayMessageIPNBus(t *testing.T) { }) } } + +func toStrings[T ~string](in []T) []string { + out := make([]string, len(in)) + for i, v := range in { + out[i] = string(v) + } + return out +} diff --git a/util/syspolicy/source/test_store.go b/util/syspolicy/source/test_store.go index 4b175611f..efaf4cd5a 100644 --- a/util/syspolicy/source/test_store.go +++ b/util/syspolicy/source/test_store.go @@ -154,6 +154,13 @@ func (s *TestStore) RegisterChangeCallback(callback func()) (unregister func(), }, nil } +// IsEmpty reports whether the store does not contain any settings. +func (s *TestStore) IsEmpty() bool { + s.mu.RLock() + defer s.mu.RUnlock() + return len(s.mr) == 0 +} + // ReadString implements [Store]. func (s *TestStore) ReadString(key setting.Key) (string, error) { defer s.recordRead(key, setting.StringValue)