diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 19a295c4..57beae59 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -498,7 +498,14 @@ func (pm *PolicyManager) SSHCheckParams( // Check if dst node matches any destination. for _, dst := range rule.Destinations { if ag, isAG := dst.(*AutoGroup); isAG && ag.Is(AutoGroupSelf) { + // User().Valid() guards the User().ID() dereference: the + // NodeStore can hold a non-tagged node with UserID set but + // the User association unhydrated (nil), and IsTagged() + // alone does not cover that. Mirrors filter.go's + // autogroup:self guard. Without it, a tailnet client on the + // Noise SSH-check path crashes the server (nil deref). if !srcNode.IsTagged() && !dstNode.IsTagged() && + srcNode.User().Valid() && dstNode.User().Valid() && srcNode.User().ID() == dstNode.User().ID() { return checkPeriodFromRule(rule), true } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index ef28bf36..9237f33f 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -292,6 +292,68 @@ func TestSetNodesAutogroupSelfUnhydratedUser(t *testing.T) { require.NoError(t, err) } +// TestSSHCheckParamsUnhydratedUserNoPanic proves that SSHCheckParams does +// not panic when a non-tagged node reaches the policy manager with its +// UserID set but its User association unhydrated (User pointer nil) — the +// same NodeStore shape that crashed /machine/map in commit 171fd7a3. The +// autogroup:self SSH branch dereferences node.User().ID() guarded only by +// !IsTagged(), not by User().Valid(); SSHCheckParams is reached from the +// Noise SSH-check path (noise.go), so a tailnet client triggers the panic +// and crashes the server (DoS) whenever an SSH check rule with an +// autogroup:self destination is active. +func TestSSHCheckParamsUnhydratedUserNoPanic(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "user1", Email: "user1@headscale.net"}, + } + + policy := `{ + "ssh": [ + { + "action": "check", + "src": ["user1@headscale.net"], + "dst": ["autogroup:self"], + "users": ["root"] + } + ] + }` + + initialNodes := types.Nodes{ + node("user1-src", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("user1-dst", "100.64.0.2", "fd7a:115c:a1e0::2", users[0]), + } + 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) + + // Simulate a node restarting tailscaled: the destination node is pushed + // back into the policy manager with no hydrated User association (UserID + // set, User pointer nil), the exact shape that crashed beta.1. + unhydratedDst := &types.Node{ + ID: 2, + Hostname: "user1-dst", + IPv4: ap("100.64.0.2"), + IPv6: ap("fd7a:115c:a1e0::2"), + UserID: new(users[0].ID), + User: nil, + } + require.False(t, unhydratedDst.IsTagged(), "dst node must be user-owned for autogroup:self") + + updatedNodes := types.Nodes{ + node("user1-src", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + unhydratedDst, + } + updatedNodes[0].ID = 1 + _, err = pm.SetNodes(updatedNodes.ViewSlice()) + require.NoError(t, err) + + require.NotPanics(t, func() { + pm.SSHCheckParams(types.NodeID(1), types.NodeID(2)) + }, "SSHCheckParams must not panic when a non-tagged node has an unhydrated User") +} + // TestInvalidateGlobalPolicyCache tests the cache invalidation logic for global policies. func TestInvalidateGlobalPolicyCache(t *testing.T) { mustIPPtr := func(s string) *netip.Addr {