diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 7a914224..299a2e2f 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1299,13 +1299,18 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S // Tagged nodes don't participate in autogroup:self (identity is tag-based), // so we skip them when collecting affected users, except when tag status changes // (which affects the user's device set). - affectedUsers := make(map[uint]struct{}) + // + // Ownership is keyed on TypedUserID (the UserID field), not the User + // association view: the NodeStore holds nodes by value with User as a + // *User pointer, and not every write path hydrates that association. A + // non-tagged node always has UserID set, so it is the reliable owner key. + affectedUsers := make(map[types.UserID]struct{}) // Check for removed nodes (only non-tagged nodes affect autogroup:self) for nodeID, oldNode := range oldNodeMap { if _, exists := newNodeMap[nodeID]; !exists { if !oldNode.IsTagged() { - affectedUsers[oldNode.User().ID()] = struct{}{} + affectedUsers[oldNode.TypedUserID()] = struct{}{} } } } @@ -1314,7 +1319,7 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S for nodeID, newNode := range newNodeMap { if _, exists := oldNodeMap[nodeID]; !exists { if !newNode.IsTagged() { - affectedUsers[newNode.User().ID()] = struct{}{} + affectedUsers[newNode.TypedUserID()] = struct{}{} } } } @@ -1327,10 +1332,10 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S if oldNode.IsTagged() != newNode.IsTagged() { if !oldNode.IsTagged() { // Was untagged, now tagged: user lost a device - affectedUsers[oldNode.User().ID()] = struct{}{} + affectedUsers[oldNode.TypedUserID()] = struct{}{} } else { // Was tagged, now untagged: user gained a device - affectedUsers[newNode.User().ID()] = struct{}{} + affectedUsers[newNode.TypedUserID()] = struct{}{} } continue @@ -1342,9 +1347,9 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S } // Check if user changed (both versions are non-tagged here) - if oldNode.User().ID() != newNode.User().ID() { - affectedUsers[oldNode.User().ID()] = struct{}{} - affectedUsers[newNode.User().ID()] = struct{}{} + if oldNode.TypedUserID() != newNode.TypedUserID() { + affectedUsers[oldNode.TypedUserID()] = struct{}{} + affectedUsers[newNode.TypedUserID()] = struct{}{} } // Check if IPs changed (simple check - could be more sophisticated) @@ -1352,12 +1357,12 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S newIPs := newNode.IPs() if len(oldIPs) != len(newIPs) { - affectedUsers[newNode.User().ID()] = struct{}{} + affectedUsers[newNode.TypedUserID()] = struct{}{} } else { // Check if any IPs are different for i, oldIP := range oldIPs { if i >= len(newIPs) || oldIP != newIPs[i] { - affectedUsers[newNode.User().ID()] = struct{}{} + affectedUsers[newNode.TypedUserID()] = struct{}{} break } } @@ -1370,7 +1375,7 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S // because autogroup:self rules depend on the entire user's device set. for nodeID := range pm.filterRulesMap { // Find the user for this cached node - var nodeUserID uint + var nodeUserID types.UserID found := false @@ -1384,7 +1389,7 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S break } - nodeUserID = node.User().ID() + nodeUserID = node.TypedUserID() found = true break @@ -1400,7 +1405,7 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S break } - nodeUserID = node.User().ID() + nodeUserID = node.TypedUserID() found = true break diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index bb90afc1..ef28bf36 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -226,6 +226,72 @@ func TestInvalidateAutogroupSelfCache(t *testing.T) { } } +// TestSetNodesAutogroupSelfUnhydratedUser reproduces the panic seen on +// /machine/map when an autogroup:self policy is active and a non-tagged +// node reaches the policy manager with its UserID set but the User +// association left unhydrated (User pointer nil). The NodeStore stores +// nodes by value with User as a *User; not every write path hydrates the +// association, so the autogroup:self cache invalidation must derive the +// owning user from UserID, not from the User view. +func TestSetNodesAutogroupSelfUnhydratedUser(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "user1", Email: "user1@headscale.net"}, + {Model: gorm.Model{ID: 2}, Name: "user2", Email: "user2@headscale.net"}, + } + + policy := `{ + "acls": [ + { + "action": "accept", + "src": ["autogroup:member"], + "dst": ["autogroup:self:*"] + } + ] + }` + + // unhydratedNode mirrors a NodeStore snapshot entry whose UserID is + // set (so it is unambiguously user-owned, not tagged) but whose User + // association was never loaded. + unhydratedNode := func(name, ipv4, ipv6 string, userID uint) *types.Node { + return &types.Node{ + Hostname: name, + IPv4: ap(ipv4), + IPv6: ap(ipv6), + UserID: new(userID), + User: nil, + } + } + + initialNodes := types.Nodes{ + node("user1-node1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("user2-node1", "100.64.0.2", "fd7a:115c:a1e0::2", users[1]), + } + for i, n := range initialNodes { + n.ID = types.NodeID(i + 1) //nolint:gosec // safe conversion in test + } + + pm, err := NewPolicyManager([]byte(policy), users, initialNodes.ViewSlice()) + require.NoError(t, err) + + require.False(t, initialNodes[0].IsTagged(), "node must be user-owned for autogroup:self") + + // Simulate a node restarting tailscaled: the same node is pushed back + // into the policy manager, but the snapshot version has no hydrated + // User association. This is the exact shape that crashed beta.1. + updatedNodes := types.Nodes{ + unhydratedNode("user1-node1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0].ID), + node("user2-node1", "100.64.0.2", "fd7a:115c:a1e0::2", users[1]), + } + for i, n := range updatedNodes { + n.ID = types.NodeID(i + 1) //nolint:gosec // safe conversion in test + } + + require.NotPanics(t, func() { + _, err = pm.SetNodes(updatedNodes.ViewSlice()) + }, "SetNodes must not panic when a non-tagged node has an unhydrated User") + require.NoError(t, err) +} + // TestInvalidateGlobalPolicyCache tests the cache invalidation logic for global policies. func TestInvalidateGlobalPolicyCache(t *testing.T) { mustIPPtr := func(s string) *netip.Addr {