policy: guard SSHCheckParams autogroup:self against nil User

The autogroup:self SSH-check branch dereferenced node.User().ID() guarded only by !IsTagged(); a non-tagged node with an unhydrated User (UserID set, association nil) crashed the server via the Noise SSH-check path. Gate on User().Valid() like filter.go, same shape as 171fd7a3.
This commit is contained in:
Kristoffer Dalby
2026-06-06 16:42:33 +00:00
committed by Kristoffer Dalby
parent 8f75ee5647
commit 56cd3eb24d
2 changed files with 69 additions and 0 deletions
+7
View File
@@ -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
}
+62
View File
@@ -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 {