From b0347ccccc265e3e274853e188b094dbfe7a237a Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 09:04:41 +0000 Subject: [PATCH] state: auto-approve routes in the NodeStore writer, not on reload Registration approval runs on the node the writer replaces. A policy reload approves nothing, and a map request only when its announced routes change, so neither re-grants a route an admin revoked. --- hscontrol/servertest/issues_test.go | 34 +--- hscontrol/servertest/lifecycle_compat_test.go | 13 -- hscontrol/state/autoapprove_test.go | 105 ++++++++++-- hscontrol/state/persist_test.go | 41 ----- hscontrol/state/state.go | 153 ++++-------------- integration/route_test.go | 24 ++- 6 files changed, 139 insertions(+), 231 deletions(-) diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index 618789234..0a51c83ea 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -282,40 +282,8 @@ func TestIssuesRoutes(t *testing.T) { selfOffersExit) }) - // A policy reload that auto-approves an advertised exit route goes - // through autoApproveNodes, a third producer of the same change. - t.Run("exit_route_auto_approved_on_reload_reaches_self", func(t *testing.T) { - t.Parallel() - - srv := servertest.NewServer(t) - user := srv.CreateUser(t, "reloadexit-user") - - exit := servertest.NewClient(t, srv, "reloadexit-node", - servertest.WithUser(user)) - obs := servertest.NewClient(t, srv, "reloadexit-obs", - servertest.WithUser(user)) - - exit.WaitForPeers(t, 1, 10*time.Second) - advertiseRoutes(t, exit, obs, tsaddr.ExitRoutes()) - - _, err := srv.State().SetPolicyInDB(`{ - "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}], - "autoApprovers": {"exitNode": ["reloadexit-user@"]} - }`) - require.NoError(t, err) - - changes, err := srv.State().ReloadPolicy() - require.NoError(t, err) - srv.App.Change(changes...) - - obs.WaitForCondition(t, "peer offers exit node", 10*time.Second, - peerOffersExit("reloadexit-node")) - exit.WaitForCondition(t, "self offers exit node", 10*time.Second, - selfOffersExit) - }) - // An auto-approver approves the route inside the advertising map - // request, a fourth producer of the same change. + // request, a third producer of the same change. t.Run("exit_route_auto_approved_on_advertise_reaches_self", func(t *testing.T) { t.Parallel() diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index 715cc22c1..9ccb50250 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -36,17 +36,6 @@ import ( // the step of what killed nodes leave behind. A listed check that matches // fails, so a fix must delete its entry. var lifecycleSkipReasons = map[string]string{ - "route-approval/b6/enabled": approvalOnHostinfo, - "route-approval/b6/served": approvalOnHostinfo, - "route-approval/b6/self-routes": approvalOnHostinfo, - - "route-approval/b7/enabled": approvalOnPolicy, - "route-approval/b7/served": approvalOnPolicy, - "route-approval/b7/self-routes": approvalOnPolicy, - "route-approval/b8/enabled": approvalOnPolicy, - "route-approval/b8/served": approvalOnPolicy, - "route-approval/b8/self-routes": approvalOnPolicy, - "route-approval/b9/enabled": approvalOnSameKeyReauth, "route-approval/b9/served": approvalOnSameKeyReauth, "route-approval/b9/self-routes": approvalOnSameKeyReauth, @@ -76,8 +65,6 @@ var lifecycleSkipReasons = map[string]string{ // SaaS runs auto-approval when a router's announced routes change and on a // re-auth that rotates its node key, over every announced route. const ( - approvalOnHostinfo = "APPROVAL_ON_HOSTINFO: headscale auto-approves when a router with no approved routes sends a Hostinfo change that keeps its routes; SaaS does not" - approvalOnPolicy = "APPROVAL_ON_POLICY: headscale auto-approves announced routes when a policy is set; SaaS waits for the router's next route change or key rotation" approvalOnSameKeyReauth = "APPROVAL_ON_SAME_KEY_REAUTH: headscale auto-approves on a re-auth that keeps the node key; SaaS does not" ) diff --git a/hscontrol/state/autoapprove_test.go b/hscontrol/state/autoapprove_test.go index 444a04764..f4b637d5a 100644 --- a/hscontrol/state/autoapprove_test.go +++ b/hscontrol/state/autoapprove_test.go @@ -10,33 +10,106 @@ import ( "tailscale.com/tailcfg" ) -// TestAutoApproveBatchApprovesRoutes verifies the batched autoApproveNodes still -// approves a node's advertised route when policy auto-approvers permit it. The -// batching collapses the per-node SetApprovedRoutes calls into one NodeStore -// update and one policy rebuild; this guards that correctness is preserved. -func TestAutoApproveBatchApprovesRoutes(t *testing.T) { +// TestRouteApprovalWaitsForAnnouncement: approvers run when the announced +// set changes, over every announced route, and neither on a policy reload +// nor on a report that keeps the routes. +func TestRouteApprovalWaitsForAnnouncement(t *testing.T) { _, s, nodeID := persistTestSetup(t) t.Cleanup(func() { _ = s.Close() }) route := netip.MustParsePrefix("10.0.0.0/24") + added := netip.MustParsePrefix("10.0.1.0/24") - _, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { - n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}} - }) + nv, ok := s.GetNodeByID(nodeID) require.True(t, ok) - pol := `{ - "autoApprovers": {"routes": {"10.0.0.0/24": ["persist-user@"]}}, + report := func(hostname string, routes ...netip.Prefix) { + t.Helper() + + _, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: nv.NodeKey(), + DiscoKey: nv.DiscoKey(), + Hostinfo: &tailcfg.Hostinfo{Hostname: hostname, RoutableIPs: routes}, + }) + require.NoError(t, err) + } + approved := func() []netip.Prefix { + t.Helper() + + current, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + return current.ApprovedRoutes().AsSlice() + } + + report("router", route) + require.Empty(t, approved()) + + _, err := s.db.SetPolicy(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["persist-user@"]}}, "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] - }` - _, err := s.SetPolicy([]byte(pol)) + }`) require.NoError(t, err) _, err = s.ReloadPolicy() require.NoError(t, err) + assert.Empty(t, approved(), "a reload approves nothing") - nv, ok := s.GetNodeByID(nodeID) - require.True(t, ok) - assert.Contains(t, nv.ApprovedRoutes().AsSlice(), route, - "auto-approver should have approved the advertised route") + report("router-renamed", route) + assert.Empty(t, approved(), "a report that keeps the routes approves nothing") + + report("router-renamed", route, added) + assert.ElementsMatch(t, []netip.Prefix{route, added}, approved(), + "a new announcement runs approvers over every announced route") +} + +// TestAutoApproveKeepsCurrentManualRoutes: registration approval runs on +// the node the writer replaces, so an admin write that lands after the +// caller read its view survives. +func TestAutoApproveKeepsCurrentManualRoutes(t *testing.T) { + manual := netip.MustParsePrefix("10.0.0.0/24") + automatic := netip.MustParsePrefix("10.1.0.0/24") + addition := netip.MustParsePrefix("10.2.0.0/24") + + for _, tt := range []struct { + name string + replacement []netip.Prefix + want []netip.Prefix + }{ + {name: "revoke", want: []netip.Prefix{automatic}}, + {name: "add", replacement: []netip.Prefix{manual, addition}, want: []netip.Prefix{manual, automatic, addition}}, + } { + t.Run(tt.name, func(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + _, err := s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.1.0.0/24": ["persist-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + _, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{manual, automatic}} + }) + require.True(t, ok) + + stale, _, err := s.SetApprovedRoutes(nodeID, []netip.Prefix{manual}) + require.NoError(t, err) + + _, _, err = s.SetApprovedRoutes(nodeID, tt.replacement) + require.NoError(t, err) + + _, err = s.AutoApproveRoutes(stale) + require.NoError(t, err) + + current, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.ElementsMatch(t, tt.want, current.ApprovedRoutes().AsSlice()) + + stored, err := s.db.GetNodeByID(nodeID) + require.NoError(t, err) + assert.ElementsMatch(t, tt.want, stored.ApprovedRoutes.List()) + }) + } } diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 976917723..c6124890e 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1215,47 +1215,6 @@ func TestFailedPersistPolicyChangeSurvivesDroppedChange(t *testing.T) { failedC.Type(), retryC.Type()) } -// TestReloadPolicyReturnsChangesOnAutoApproveFailure covers a policy reload -// whose route auto-approval fails to persist. The NodeStore write already -// fed the approved routes to the policy manager, so no later write sees the -// policy move again; the reload must return its changes with the error for -// the caller to publish. -func TestReloadPolicyReturnsChangesOnAutoApproveFailure(t *testing.T) { - _, s, nodeID := persistTestSetup(t) - t.Cleanup(func() { _ = s.Close() }) - - route := netip.MustParsePrefix("10.9.0.0/24") - _, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { - n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}} - }) - require.True(t, ok) - - _, err := s.db.SetPolicy(`{ - "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}], - "autoApprovers": {"routes": {"10.9.0.0/24": ["persist-user@"]}} - }`) - require.NoError(t, err) - - require.NoError(t, s.db.DB.Callback().Update().Before("gorm:update"). - Register("fail_node_update", func(tx *gorm.DB) { - if tx.Statement.Table == "nodes" { - _ = tx.AddError(errInjectedNodeUpdate) - } - })) - t.Cleanup(func() { _ = s.db.DB.Callback().Update().Remove("fail_node_update") }) - - cs, err := s.ReloadPolicy() - require.ErrorIs(t, err, errInjectedNodeUpdate) - - approved, ok := s.GetNodeByID(nodeID) - require.True(t, ok) - require.Contains(t, approved.ApprovedRoutes().AsSlice(), route, - "the NodeStore holds the approval the database write lost") - - assert.True(t, slices.ContainsFunc(cs, func(c change.Change) bool { return c.IncludePolicy }), - "the reload must return its policy change with the error: %v", cs) -} - // TestReloadPolicyOverlappingWrites has a second policy write complete after // the reload of the first one loaded its document and before it installs it. // The first reload must not put the older document back over the newer one. diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 6c8ee8cf8..14d2f8cde 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -468,9 +468,11 @@ func (s *State) DERPMap() tailcfg.DERPMapView { return s.derpMap.Load().View() } -// ReloadPolicy reloads the access control policy and triggers auto-approval if changed. -// Returns the resulting [change.Change] slice when the policy or routes changed, -// also alongside an error once the policy is swapped. +// ReloadPolicy reloads the access control policy. +// +// It approves no routes: auto-approval waits for a node's next change to +// its announced routes or its next node key rotation, as on Tailscale SaaS, +// so a reload cannot undo an admin's revoke. func (s *State) ReloadPolicy() ([]change.Change, error) { s.policyMu.Lock() defer s.policyMu.Unlock() @@ -514,36 +516,13 @@ func (s *State) reloadPolicyLocked() ([]change.Change, error) { s.nodeStore.RebuildPeerMaps() } + if policyChanged { + log.Info().Msg("Policy reload completed with changes") + } + // Nodes whose CapMap shifted get their self refresh from // [State.DrainSelfRefreshes] when these changes are dispatched. - //nolint:prealloc // cs starts with one element and may grow - cs := []change.Change{change.PolicyChange()} - - // Always call autoApproveNodes during policy reload, regardless of whether - // the policy content has changed. This ensures that routes are re-evaluated - // when they might have been manually disabled but could now be auto-approved - // with the current policy. - rcs, err := s.autoApproveNodes() - if err != nil { - // The policy is already swapped and the approvals already in the - // NodeStore; callers publish these before handling the error. - return append(cs, rcs...), fmt.Errorf("auto approving nodes: %w", err) - } - - // TODO(kradalby): These changes can probably be safely ignored. - // If the PolicyChange is happening, that will lead to a full update - // meaning that we do not need to send individual route changes. - cs = append(cs, rcs...) - - if len(rcs) > 0 || policyChanged { - log.Info(). - Bool("policy.changed", policyChanged). - Int("route.changes", len(rcs)). - Int("total.changes", len(cs)). - Msg("Policy reload completed with changes") - } - - return cs, nil + return []change.Change{change.PolicyChange()}, nil } // CreateUser creates a new user and updates the policy manager. @@ -672,8 +651,6 @@ func (s *State) ListAllUsers() ([]types.User, error) { // persistNode writes the node's database row, re-reading the // authoritative copy from [NodeStore], without touching the policy manager. -// Batch callers (e.g. autoApproveNodes) use it to write many rows and then -// trigger a single policy rebuild instead of one per node. func (s *State) persistNode(node types.NodeView) (types.NodeView, error) { return s.persistNodeWith(node, nodeUpdateColumns, nil) } @@ -1231,12 +1208,18 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t // TODO(kradalby): In principle we should call the AutoApprove logic here // because even if the CLI removes an auto-approved route, it will be added // back automatically. + return s.updateApprovedRoutes(nodeID, func(node *types.Node) { + node.ApprovedRoutes = routes + }) +} + +func (s *State) updateApprovedRoutes(nodeID types.NodeID, set UpdateNodeFunc) (types.NodeView, change.Change, error) { prevRoutes := s.nodeStore.PrimaryRoutes() prevPeers := s.nodeStore.ListPeerIDs(nodeID) genBefore := s.polMan.NodesGeneration() n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) { - node.ApprovedRoutes = routes + set(node) // A node with no approved routes is no longer an HA // candidate; drop any stale Unhealthy bit (mirrors the // legacy routes.SetRoutes(empty) auto-clear). @@ -1532,10 +1515,8 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { Strs("routes.approved.new", util.PrefixesToString(approved)). Msg("Single node auto-approval detected route changes") - // Persist the auto-approved routes to database and [NodeStore] via - // [State.SetApprovedRoutes]. This ensures consistency between database - // and [NodeStore]. - _, c, err := s.SetApprovedRoutes(nv.ID(), approved) + // Recompute on the current node so an older view cannot undo an admin write. + _, c, err := s.updateApprovedRoutes(nv.ID(), s.approveAnnouncedRoutes) if err != nil { log.Error(). EmbedObject(nv). @@ -1551,6 +1532,10 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { return change.Change{}, nil } +func (s *State) approveAnnouncedRoutes(n *types.Node) { + n.ApprovedRoutes, _ = policy.ApproveRoutesWithPolicy(s.polMan, n.View(), n.ApprovedRoutes, n.AnnouncedRoutes()) +} + // GetPolicy retrieves the current policy from the database. func (s *State) GetPolicy() (*types.Policy, error) { return s.db.GetPolicy() @@ -3634,83 +3619,6 @@ func (s *State) PingDB(ctx context.Context) error { return s.db.PingDB(ctx) } -// autoApproveNodes mass approves routes on all nodes. It is _only_ intended for -// use when the policy is replaced. It is not sending or reporting any changes -// or updates as we send full updates after replacing the policy. -// TODO(kradalby): This is kind of messy, maybe this is another +1 -// for an event bus. See example comments here. -// autoApproveNodes automatically approves nodes based on policy rules. -func (s *State) autoApproveNodes() ([]change.Change, error) { - nodes := s.ListNodes() - - // Compute every node's approval first, then apply them all in a single - // NodeStore batch and a single policy/peer-map rebuild. One - // SetApprovedRoutes per node would otherwise drive an O(n) policy SetNodes - // and O(n^2) peer-map rebuild for each changed node, i.e. O(m*n^2) per - // policy reload. - approvedByID := make(map[types.NodeID][]netip.Prefix) - - for _, nv := range nodes.All() { - approved, changed := policy.ApproveRoutesWithPolicy(s.polMan, nv, nv.ApprovedRoutes().AsSlice(), nv.AnnouncedRoutes()) - if !changed { - continue - } - - log.Debug(). - Uint64(zf.NodeID, nv.ID().Uint64()). - Str(zf.NodeName, nv.Hostname()). - Strs(zf.RoutesApprovedOld, util.PrefixesToString(nv.ApprovedRoutes().AsSlice())). - Strs(zf.RoutesApprovedNew, util.PrefixesToString(approved)). - Msg("Routes auto-approved by policy") - - approvedByID[nv.ID()] = approved - } - - if len(approvedByID) == 0 { - return nil, nil - } - - genBefore := s.polMan.NodesGeneration() - - updates := make(map[types.NodeID]UpdateNodeFunc, len(approvedByID)) - for id, approved := range approvedByID { - updates[id] = func(n *types.Node) { - n.ApprovedRoutes = approved - - // A node with no approved routes is no longer an HA candidate; - // drop any stale Unhealthy bit (mirrors SetApprovedRoutes). - if len(n.AllApprovedRoutes()) == 0 { - n.Unhealthy = false - } - } - } - - s.nodeStore.UpdateNodes(updates) - - for id := range approvedByID { - fresh, ok := s.nodeStore.GetNode(id) - if !ok { - continue - } - - _, err := s.persistNode(fresh) - if err != nil { - return []change.Change{s.policyChangeSince(genBefore)}, err - } - } - - c, err := s.updatePolicyManagerNodes(genBefore) - if err != nil { - return []change.Change{c}, err - } - - if c.IsEmpty() { - c = change.PolicyChange() - } - - return []change.Change{c}, nil -} - // isAutoDerivedGivenName reports whether given matches what // dnsname.SanitizeHostname(hostname) would produce, optionally with a // [NodeStore] collision-bump "-N" suffix. It is used to detect whether a @@ -3754,7 +3662,6 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest var ( delta mapRequestDelta routeChange bool - needsRouteApproval bool autoApprovedRoutes []netip.Prefix staleNodeKey bool ) @@ -3843,16 +3750,10 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest hostinfoToStore := delta.hostinfoChanged || delta.derpChanged - // Calculate route approval before [NodeStore] update to avoid - // calling View() inside callback - var hasNewRoutes bool - if hi := req.Hostinfo; hi != nil { - hasNewRoutes = len(hi.RoutableIPs) > 0 - } - - needsRouteApproval = delta.hostinfoChanged && - (delta.routesChanged || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0)) - if needsRouteApproval { + // Approvers run over every announced route, but only when the + // announced set changes: any other report would re-grant a route + // an admin revoked. + if delta.routesChanged { // Extract announced routes from request var announcedRoutes []netip.Prefix if req.Hostinfo != nil { diff --git a/integration/route_test.go b/integration/route_test.go index 0c0aed931..1ee041dd6 100644 --- a/integration/route_test.go +++ b/integration/route_test.go @@ -2857,8 +2857,9 @@ func TestAutoApproveMultiNetwork(t *testing.T) { } }, assertTimeout, 200*time.Millisecond, "Verifying routes disabled after route removal") - // Add the route back to the auto approver in the policy, the route should - // now become available again. + // Add the route back to the auto approver in the policy. The + // policy alone approves nothing; the route comes back once the + // router announces it again. var newApprovers policyv2.AutoApprovers switch { @@ -2875,6 +2876,25 @@ func TestAutoApproveMultiNetwork(t *testing.T) { err = headscale.SetPolicy(pol) require.NoError(t, err) + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err = headscale.ListNodes() + assert.NoError(c, err) + requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 1, 0, 0) + }, assertTimeout, 500*time.Millisecond, "a policy change alone approves no route") + + // Withdraw and wait, so the new announcement is a change. + _, _, err = routerUsernet1.Execute([]string{"tailscale", "set", "--advertise-routes="}) + require.NoErrorf(t, err, "failed to withdraw route: %s", err) + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err = headscale.ListNodes() + assert.NoError(c, err) + requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 0, 0, 0) + }, assertTimeout, 500*time.Millisecond, "route withdrawn") + + _, _, err = routerUsernet1.Execute([]string{"tailscale", "set", "--advertise-routes=" + route.String()}) + require.NoErrorf(t, err, "failed to advertise route: %s", err) + // Wait for route state changes to propagate assert.EventuallyWithT(t, func(c *assert.CollectT) { // These route should auto approve, so the node is expected to have a route