diff --git a/hscontrol/servertest/mapstream_compat_test.go b/hscontrol/servertest/mapstream_compat_test.go index 7f5523e31..12d69228e 100644 --- a/hscontrol/servertest/mapstream_compat_test.go +++ b/hscontrol/servertest/mapstream_compat_test.go @@ -127,11 +127,6 @@ var mapStreamSkipReasons = map[string]string{ "policy-generation/G4/thor-1/peer-owners": sameUserPeers, "policy-generation/G4/thor-2/peer-owners": sameUserPeers, - "policy-generation/G4/rt/enabled": withdrawnRouteEnabled, - "policy-generation/G5/rt/enabled": withdrawnRouteEnabled, - "policy-generation/G7/rt/enabled": withdrawnRouteEnabled, - "policy-generation/G8/rt/enabled": withdrawnRouteEnabled, - "user-role/S1-reconnect/own-a/frames": reconnectAnnounced, "user-role/S1-reconnect/own-b/frames": reconnectAnnounced, "user-role/S2-reconnect/own-a/frames": reconnectAnnounced, @@ -150,7 +145,6 @@ const ( peerHostinfoWhole = "PEER_HOSTINFO_WHOLE: headscale sends peers a node's whole reported Hostinfo; SaaS sends only Hostname, OS, Services, the SSH host keys and AppConnector" peerServicesWhole = "PEER_SERVICES_WHOLE: headscale sends peers every service a node reports; SaaS leaves out peerapi-dns-proxy" removalSentToAll = "REMOVAL_SENT_TO_ALL: headscale tells every connected node that a deleted node is gone; SaaS tells only the nodes that had it as a peer" - withdrawnRouteEnabled = "WITHDRAWN_ROUTE_ENABLED: headscale's routes API keeps a route the device stopped advertising in enabledRoutes; SaaS lists only advertised routes as enabled" taggedDeviceUser = "TAGGED_DEVICE_USER: SaaS shows a tagged device's user as the login that made its key; headscale shows tagged-devices" ) diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index f439e1dd4..8be1c0b22 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -39,14 +39,6 @@ import ( var reauthSkipReasons = map[string]string{ "owner-routes/r-expired-retag/expire/online": expiredPeerOffline, - "owner-routes/r-admin/owner/enabled": adminRetagNoApproval, - "owner-routes/r-admin/owner/served": adminRetagNoApproval, - "owner-routes/r-admin/owner/self-routes": adminRetagNoApproval, - - "owner-routes/r-retag/withdraw/enabled": withdrawKeepsAutoApproval, - "owner-routes/r-expired-retag/withdraw/enabled": withdrawKeepsAutoApproval, - "owner-routes/u-convert/withdraw/enabled": withdrawKeepsAutoApproval, - "identity/w-tagreq-relogin-tags/w-tagreq/owner": keyLoginTagsRefused, "identity/w-tagreq-relogin-tags/w-tagreq/outcome": keyLoginTagsRefused, "identity/w-tagreq-relogin-tags/w-tagreq/node-key": keyLoginTagsRefused, @@ -85,11 +77,6 @@ var reauthSkipReasons = map[string]string{ "ephemeral-lifecycle/late-expired/late-exp/legacy-text": refusalText, } -const ( - adminRetagNoApproval = "ADMIN_RETAG_NO_APPROVAL: an admin tag change does not auto-approve the routes the new tags may approve; SaaS approves them at once" - withdrawKeepsAutoApproval = "WITHDRAW_KEEPS_AUTO_APPROVAL: headscale keeps an auto-approved route once it is withdrawn; SaaS drops it and keeps only admin-set approvals" -) - const ( keyLoginTagsRefused = "KEY_LOGIN_TAGS_REFUSED: headscale refuses a user's key login that requests tags the user owns; SaaS tags the device, converting it in place or creating it tagged" taggedKeyOwnedTagRefused = "TAGGED_KEY_OWNED_TAG_REFUSED: headscale refuses a tagged key's login that requests a tag the key's tags own; SaaS gives the device the requested tags" diff --git a/hscontrol/state/autoapprove_test.go b/hscontrol/state/autoapprove_test.go index b0b75f1ce..2e87682c9 100644 --- a/hscontrol/state/autoapprove_test.go +++ b/hscontrol/state/autoapprove_test.go @@ -4,6 +4,7 @@ import ( "net/netip" "testing" + "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" @@ -214,3 +215,161 @@ func approvedRoutes(t *testing.T, s *State, id types.NodeID) []netip.Prefix { return nv.ApprovedRoutes().AsSlice() } + +// TestWithdrawnRouteKeepsOnlyAdminApprovals: a route only auto-approval +// granted is dropped once the router stops announcing it and approved again +// when it comes back; a route an admin approved stays, also when +// autoApprovers cover it, and so does every approval made before the upgrade. +func TestWithdrawnRouteKeepsOnlyAdminApprovals(t *testing.T) { + auto := netip.MustParsePrefix("10.0.1.0/24") + admin := netip.MustParsePrefix("10.0.2.0/24") + + for _, tt := range []struct { + name string + setup func(t *testing.T, s *State, id types.NodeID) // after announcing both routes + want []netip.Prefix // approved after the withdrawal + }{ + {name: "auto only", want: nil}, + { + name: "admin overlap", + setup: func(t *testing.T, s *State, id types.NodeID) { + t.Helper() + + _, _, err := s.SetApprovedRoutes(id, []netip.Prefix{auto, admin}) + require.NoError(t, err) + }, + want: []netip.Prefix{auto, admin}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + dbPath, s, id := persistTestSetup(t) + + // Stored, so the restart below keeps it. + _, err := s.db.SetPolicy(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["persist-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`) + require.NoError(t, err) + _, err = s.ReloadPolicy() + require.NoError(t, err) + + announce(t, s, id, auto, admin) + require.ElementsMatch(t, []netip.Prefix{auto, admin}, approvedRoutes(t, s, id)) + + if tt.setup != nil { + tt.setup(t, s, id) + } + + // The marks are stored, so a restart between keeps them. + require.NoError(t, s.Close()) + s = persistTestReopen(t, dbPath) + + announce(t, s, id) + assert.ElementsMatch(t, tt.want, approvedRoutes(t, s, id), "after the withdrawal") + + stored, err := s.db.GetNodeByID(id) + require.NoError(t, err) + assert.ElementsMatch(t, tt.want, stored.ApprovedRoutes.List()) + + announce(t, s, id, auto, admin) + assert.ElementsMatch(t, []netip.Prefix{auto, admin}, approvedRoutes(t, s, id), "announced again") + }) + } + + t.Run("approved before the upgrade", func(t *testing.T) { + dbPath, s, id := persistTestSetup(t) + require.NoError(t, s.Close()) + + // A row as the migration leaves it: approved, with no marks. + database, err := db.NewHeadscaleDatabase(persistTestConfig(dbPath)) + require.NoError(t, err) + require.NoError(t, database.DB.Exec( + `UPDATE nodes SET approved_routes = ?, auto_approved_routes = NULL, host_info = ? WHERE id = ?`, + `["10.0.1.0/24"]`, `{"RoutableIPs":["10.0.1.0/24"]}`, id).Error) + require.NoError(t, database.Close()) + + s = persistTestReopen(t, dbPath) + + _, err = s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["persist-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + announce(t, s, id) + assert.Equal(t, []netip.Prefix{auto}, approvedRoutes(t, s, id)) + }) +} + +// announce sends a map request in which node id announces routes. +func announce(t *testing.T, s *State, id types.NodeID, routes ...netip.Prefix) { + t.Helper() + + nv, ok := s.GetNodeByID(id) + require.True(t, ok) + + _, err := s.UpdateNodeFromMapRequest(id, tailcfg.MapRequest{ + NodeKey: nv.NodeKey(), + DiscoKey: nv.DiscoKey(), + Hostinfo: &tailcfg.Hostinfo{Hostname: nv.Hostname(), RoutableIPs: routes}, + }) + require.NoError(t, err) +} + +// TestRegistrationDropsWithdrawnAutoApprovals: a registration that stops +// announcing an auto-approved route drops its approval too. +func TestRegistrationDropsWithdrawnAutoApprovals(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("withdraw-user") + + _, err := s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["withdraw-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + kept := netip.MustParsePrefix("10.0.1.0/24") + withdrawn := netip.MustParsePrefix("10.0.2.0/24") + machineKey := key.NewMachine().Public() + req := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{kept, withdrawn}}, + } + + node, _, err := s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + require.Equal(t, []netip.Prefix{kept, withdrawn}, approvedRoutes(t, s, node.ID())) + + req.Hostinfo = &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{kept}} + _, _, err = s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + assert.Equal(t, []netip.Prefix{kept}, approvedRoutes(t, s, node.ID())) +} + +// TestAdminTagChangeApprovesRoutes: an admin tag change runs route +// auto-approval for the routes the new tags may approve. +func TestAdminTagChangeApprovesRoutes(t *testing.T) { + _, s, id := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + _, err := s.SetPolicy([]byte(`{ + "tagOwners": {"tag:router": ["persist-user@"]}, + "autoApprovers": {"routes": {"10.0.0.0/16": ["tag:router"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + route := netip.MustParsePrefix("10.0.1.0/24") + + announce(t, s, id, route) + require.Empty(t, approvedRoutes(t, s, id), "the user may not approve it") + + node, _, err := s.SetNodeTags(id, []string{"tag:router"}) + require.NoError(t, err) + assert.Equal(t, []netip.Prefix{route}, node.ApprovedRoutes().AsSlice()) + assert.Equal(t, []netip.Prefix{route}, approvedRoutes(t, s, id)) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 90ed3f890..a4eda1f1a 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -122,6 +122,7 @@ var nodeUpdateColumns = []string{ "Expiry", "LastSeen", "ApprovedRoutes", + "AutoApprovedRoutes", "UpdatedAt", } @@ -1263,6 +1264,20 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, // Setting OriginNode ensures the node gets a self-update with the new tags. c.OriginNode = nodeID + // The new tags may approve announced routes, as on Tailscale SaaS. This + // runs after the policy refresh above, which teaches the policy manager + // the node's new owner. + routesChange, err := s.AutoApproveRoutes(nodeView) + c = c.Merge(routesChange) + + if fresh, ok := s.nodeStore.GetNode(nodeID); ok { + nodeView = fresh + } + + if err != nil { + return nodeView, c, fmt.Errorf("auto approving routes: %w", err) + } + return nodeView, c, nil } @@ -1270,11 +1285,11 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, // It returns a PolicyChange when a primary moved, the policy manager saw a // policy input change, or the node's peers changed; otherwise NodeAdded. func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (types.NodeView, change.Change, error) { - // 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. + // Every route an admin sets is the admin's, even one autoApprovers + // cover, so a withdrawal keeps it. return s.updateApprovedRoutes(nodeID, func(node *types.Node) { node.ApprovedRoutes = routes + node.AutoApprovedRoutes = nil }) } @@ -1568,11 +1583,15 @@ func (s *State) SetPolicy(pol []byte) (bool, error) { return changed, nil } -// AutoApproveRoutes checks if a node's routes should be auto-approved. -// AutoApproveRoutes checks if any routes should be auto-approved for a node and updates them. +// AutoApproveRoutes approves the announced routes the policy lets a node +// approve, and drops the auto-approved routes it no longer announces. func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { approved, changed := policy.ApproveRoutesWithPolicy(s.polMan, nv, nv.ApprovedRoutes().AsSlice(), nv.AnnouncedRoutes()) - if changed { + withdrawn := slices.ContainsFunc(nv.AutoApprovedRoutes().AsSlice(), func(route netip.Prefix) bool { + return !slices.Contains(nv.AnnouncedRoutes(), route) + }) + + if changed || withdrawn { log.Debug(). EmbedObject(nv). Strs("routes.announced", util.PrefixesToString(nv.AnnouncedRoutes())). @@ -1581,7 +1600,10 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { Msg("Single node auto-approval detected route changes") // Recompute on the current node so an older view cannot undo an admin write. - _, c, err := s.updateApprovedRoutes(nv.ID(), s.approveAnnouncedRoutes) + _, c, err := s.updateApprovedRoutes(nv.ID(), func(n *types.Node) { + dropWithdrawnAutoApprovals(n) + s.approveAnnouncedRoutes(n) + }) if err != nil { log.Error(). EmbedObject(nv). @@ -1597,8 +1619,43 @@ 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()) +// approveAnnouncedRoutes approves the announced routes the policy lets n +// approve, and records the ones it adds as auto-approved. +func (s *State) approveAnnouncedRoutes(n *types.Node) bool { + approved, changed := policy.ApproveRoutesWithPolicy(s.polMan, n.View(), n.ApprovedRoutes, n.AnnouncedRoutes()) + if !changed { + return false + } + + for _, route := range approved { + if !slices.Contains(n.ApprovedRoutes, route) { + n.AutoApprovedRoutes = append(n.AutoApprovedRoutes, route) + } + } + + n.ApprovedRoutes = approved + + return true +} + +// dropWithdrawnAutoApprovals removes the auto-approved routes n no longer +// announces, as Tailscale SaaS does. Routes an admin approved stay. +func dropWithdrawnAutoApprovals(n *types.Node) bool { + announced := n.AnnouncedRoutes() + withdrawn := func(route netip.Prefix) bool { + return slices.Contains(n.AutoApprovedRoutes, route) && !slices.Contains(announced, route) + } + + if !slices.ContainsFunc(n.ApprovedRoutes, withdrawn) { + return false + } + + n.ApprovedRoutes = slices.DeleteFunc(slices.Clone(n.ApprovedRoutes), withdrawn) + n.AutoApprovedRoutes = slices.DeleteFunc(slices.Clone(n.AutoApprovedRoutes), func(route netip.Prefix) bool { + return !slices.Contains(announced, route) + }) + + return true } // GetPolicy retrieves the current policy from the database. @@ -3808,10 +3865,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest Msg("Processing MapRequest for node") var ( - delta mapRequestDelta - routeChange bool - autoApprovedRoutes []netip.Prefix - staleNodeKey bool + delta mapRequestDelta + routeChange bool + staleNodeKey bool ) // Snapshot the primary assignment so we can tell whether the // Hostinfo + auto-approval that follows shifted any prefix. @@ -3898,40 +3954,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest hostinfoToStore := delta.hostinfoChanged || delta.derpChanged - // 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 { - announcedRoutes = req.Hostinfo.RoutableIPs - } - - // Apply policy-based auto-approval if routes are announced - if len(announcedRoutes) > 0 { - autoApprovedRoutes, routeChange = policy.ApproveRoutesWithPolicy( - s.polMan, - currentNode.View(), - currentNode.ApprovedRoutes, - announcedRoutes, - ) - } - } - - // Log when routes change but approval doesn't - if delta.routesChanged && !routeChange { - if hi := req.Hostinfo; hi != nil { - log.Debug(). - Caller(). - Uint64(zf.NodeID, id.Uint64()). - Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())). - Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(hi.RoutableIPs)). - Strs(zf.ApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). - Bool(zf.RouteChanged, routeChange). - Msg("announced routes changed but approved routes did not") - } - } + oldAnnounced := currentNode.AnnouncedRoutes() currentNode.ApplyPeerChange(&delta.peerChange) @@ -3957,11 +3980,25 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest } } - if routeChange { - // Approving in this write keeps one request at one NodeStore - // write, one peer build and one row update. Persisting is - // already due: route approval only runs on a Hostinfo change. - currentNode.ApprovedRoutes = autoApprovedRoutes + // Approval follows the announced set, only when it changes: any + // other report would re-grant a route an admin revoked. Deciding in + // this write keeps one request at one NodeStore write, one peer + // build and one row update; persisting is already due, as the + // Hostinfo changed. + if delta.routesChanged { + oldApproved := currentNode.ApprovedRoutes + dropped := dropWithdrawnAutoApprovals(currentNode) + routeChange = s.approveAnnouncedRoutes(currentNode) || dropped + + log.Debug(). + Caller(). + Uint64(zf.NodeID, id.Uint64()). + Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(oldAnnounced)). + Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())). + Strs(zf.RoutesApprovedOld, util.PrefixesToString(oldApproved)). + Strs(zf.RoutesApprovedNew, util.PrefixesToString(currentNode.ApprovedRoutes)). + Bool(zf.RouteChanged, routeChange). + Msg("announced routes changed") } // AllApprovedRoutes is announced ∩ approved; a Hostinfo diff --git a/integration/route_test.go b/integration/route_test.go index 1ee041dd6..cf1762cdc 100644 --- a/integration/route_test.go +++ b/integration/route_test.go @@ -3007,7 +3007,9 @@ func TestAutoApproveMultiNetwork(t *testing.T) { nodes, err = headscale.ListNodes() assert.NoError(c, err) requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 1, 1, 1) - requireNodeRouteCountWithCollect(c, nodes[1], 1, 1, 0) + // The withdrawn sub-route was auto-approved, so it loses + // its approval. + requireNodeRouteCountWithCollect(c, nodes[1], 1, 0, 0) requireNodeRouteCountWithCollect(c, nodes[2], 0, 0, 0) }, assertTimeout, 500*time.Millisecond, "route state changes should propagate") @@ -3047,7 +3049,7 @@ func TestAutoApproveMultiNetwork(t *testing.T) { nodes, err = headscale.ListNodes() assert.NoError(c, err) requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 1, 1, 1) - requireNodeRouteCountWithCollect(c, nodes[1], 1, 1, 0) + requireNodeRouteCountWithCollect(c, nodes[1], 1, 0, 0) requireNodeRouteCountWithCollect(c, nodes[2], 2, 2, 2) }, assertTimeout, 500*time.Millisecond, "route state changes should propagate")