From 381fdcc3f17f406bb8c5a711b562a23aaef6c98f Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Thu, 3 Jul 2025 20:32:30 -0500 Subject: [PATCH] ipn/ipnlocal,util/syspolicy/source: retain existing exit node when using auto exit node, if it's allowed by policy In this PR, we update setExitNodeID to retain the existing exit node if auto exit node is enabled, the current exit node is allowed by policy, and no suggested exit node is available yet. Updates tailscale/corp#29969 Signed-off-by: Nick Khyl --- ipn/ipnlocal/local.go | 15 +++- ipn/ipnlocal/local_test.go | 110 ++++++++++++++++++++++++++-- util/syspolicy/source/test_store.go | 7 ++ 3 files changed, 125 insertions(+), 7 deletions(-) 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)