From 7dd299b683d702acdb8b495d9a2de121497730d7 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 16 Jan 2026 14:40:28 +0000 Subject: [PATCH] policy,integration: update tests for SSH src/dst validation Update v1 policy tests and integration tests to use valid SSH src/dst combinations following the new validation rules from #3010. V1 policy tests (policy_test.go): - Add taggedServer node for valid group->tag SSH scenarios - Change test destinations from user@ to tag:server for validity - Tests now use group->tag instead of group->user Integration tests (ssh_test.go): - TestSSHMultipleUsersAllToAll: Use autogroup:self instead of cross-user - TestSSHIsBlockedInACL: Use autogroup:member->autogroup:self - TestSSHUserOnlyIsolation: Use user@->user@ rules (same user) - Add TestSSHInvalidPolicySrcDstValidation: Verify policy rejection The new integration test validates that invalid SSH policies are rejected at policy load time, covering tag->user, group->user, and different-user->user combinations. Updates #3010 --- hscontrol/policy/policy_test.go | 55 ++++++-- integration/ssh_test.go | 217 +++++++++++++++++++++++++++++--- 2 files changed, 243 insertions(+), 29 deletions(-) diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index 87142dd9..8891342a 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -1092,6 +1092,15 @@ func TestSSHPolicyRules(t *testing.T) { Tags: []string{"tag:client"}, } + // Tagged server for valid group->tag SSH rules + taggedServer := types.Node{ + Hostname: "tagged-server", + IPv4: ap("100.64.0.3"), + UserID: ptr.To(uint(1)), + User: ptr.To(users[0]), + Tags: []string{"tag:server"}, + } + tests := []struct { name string targetNode types.Node @@ -1102,18 +1111,22 @@ func TestSSHPolicyRules(t *testing.T) { errorMessage string }{ { - name: "group-to-user", - targetNode: nodeUser1, + // Test group->tag SSH (valid combination) + name: "group-to-tag", + targetNode: taggedServer, peers: types.Nodes{&nodeUser2}, policy: `{ "groups": { "group:admins": ["user2@"] }, + "tagOwners": { + "tag:server": ["user1@"] + }, "ssh": [ { "action": "accept", "src": ["group:admins"], - "dst": ["user1@"], + "dst": ["tag:server"], "users": ["autogroup:nonroot"] } ] @@ -1137,19 +1150,21 @@ func TestSSHPolicyRules(t *testing.T) { }}, }, { + // Test check action with tag->tag SSH (valid combination) name: "check-period-specified", - targetNode: nodeUser1, + targetNode: taggedServer, peers: types.Nodes{&taggedClient}, policy: `{ "tagOwners": { "tag:client": ["user1@"], + "tag:server": ["user1@"] }, "ssh": [ { "action": "check", "checkPeriod": "24h", "src": ["tag:client"], - "dst": ["user1@"], + "dst": ["tag:server"], "users": ["autogroup:nonroot"] } ] @@ -1174,18 +1189,20 @@ func TestSSHPolicyRules(t *testing.T) { }}, }, { + // Test no-matching-rules: target (nodeUser2) doesn't have tag:server name: "no-matching-rules", targetNode: nodeUser2, - peers: types.Nodes{&nodeUser1}, + peers: types.Nodes{&taggedClient}, policy: `{ "tagOwners": { "tag:client": ["user1@"], + "tag:server": ["user1@"] }, "ssh": [ { "action": "accept", "src": ["tag:client"], - "dst": ["user1@"], + "dst": ["tag:server"], "users": ["autogroup:nonroot"] } ] @@ -1245,18 +1262,22 @@ func TestSSHPolicyRules(t *testing.T) { errorMessage: "autogroup \"autogroup:invalid\" is not supported", }, { + // Test autogroup:nonroot user mapping with group->tag SSH name: "autogroup-nonroot-should-use-wildcard-with-root-excluded", - targetNode: nodeUser1, + targetNode: taggedServer, peers: types.Nodes{&nodeUser2}, policy: `{ "groups": { "group:admins": ["user2@"] }, + "tagOwners": { + "tag:server": ["user1@"] + }, "ssh": [ { "action": "accept", "src": ["group:admins"], - "dst": ["user1@"], + "dst": ["tag:server"], "users": ["autogroup:nonroot"] } ] @@ -1281,18 +1302,22 @@ func TestSSHPolicyRules(t *testing.T) { }}, }, { + // Test autogroup:nonroot + root user mapping with group->tag SSH name: "autogroup-nonroot-plus-root-should-use-wildcard-with-root-mapped", - targetNode: nodeUser1, + targetNode: taggedServer, peers: types.Nodes{&nodeUser2}, policy: `{ "groups": { "group:admins": ["user2@"] }, + "tagOwners": { + "tag:server": ["user1@"] + }, "ssh": [ { "action": "accept", "src": ["group:admins"], - "dst": ["user1@"], + "dst": ["tag:server"], "users": ["autogroup:nonroot", "root"] } ] @@ -1317,18 +1342,22 @@ func TestSSHPolicyRules(t *testing.T) { }}, }, { + // Test specific user mapping with group->tag SSH name: "specific-users-should-map-to-themselves-not-equals", - targetNode: nodeUser1, + targetNode: taggedServer, peers: types.Nodes{&nodeUser2}, policy: `{ "groups": { "group:admins": ["user2@"] }, + "tagOwners": { + "tag:server": ["user1@"] + }, "ssh": [ { "action": "accept", "src": ["group:admins"], - "dst": ["user1@"], + "dst": ["tag:server"], "users": ["ubuntu", "root"] } ] diff --git a/integration/ssh_test.go b/integration/ssh_test.go index 1ca291c0..fd7f5cc3 100644 --- a/integration/ssh_test.go +++ b/integration/ssh_test.go @@ -8,6 +8,7 @@ import ( "time" policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" + "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/integration/hsic" "github.com/juanfont/headscale/integration/tsic" "github.com/stretchr/testify/assert" @@ -127,6 +128,11 @@ func TestSSHOneUserToAll(t *testing.T) { } } +// TestSSHMultipleUsersAllToAll tests that users in the same group can SSH to each other's devices. +// Per Tailscale rules, user->user SSH requires separate rules for each user: +// - user1@ can SSH to user1@ devices (same user) +// - user2@ can SSH to user2@ devices (same user) +// Note: Cross-user SSH (user1@ -> user2@) requires using tags as destinations. func TestSSHMultipleUsersAllToAll(t *testing.T) { IntegrationSkip(t) @@ -145,12 +151,18 @@ func TestSSHMultipleUsersAllToAll(t *testing.T) { }, }, }, + // Per Tailscale rules: when dst is a username, src must be only the same username. + // Use autogroup:self to allow same-user SSH across all users. SSHs: []policyv2.SSH{ { - Action: "accept", - Sources: policyv2.SSHSrcAliases{groupp("group:integration-test")}, - Destinations: policyv2.SSHDstAliases{usernamep("user1@"), usernamep("user2@")}, - Users: []policyv2.SSHUser{policyv2.SSHUser("ssh-it-user")}, + Action: "accept", + Sources: policyv2.SSHSrcAliases{ + ptr.To(policyv2.AutoGroupMember), + }, + Destinations: policyv2.SSHDstAliases{ + ptr.To(policyv2.AutoGroupSelf), + }, + Users: []policyv2.SSHUser{policyv2.SSHUser("ssh-it-user")}, }, }, }, @@ -170,16 +182,40 @@ func TestSSHMultipleUsersAllToAll(t *testing.T) { _, err = scenario.ListTailscaleClientsFQDNs() requireNoErrListFQDN(t, err) - testInterUserSSH := func(sourceClients []TailscaleClient, targetClients []TailscaleClient) { - for _, client := range sourceClients { - for _, peer := range targetClients { - assertSSHHostname(t, client, peer) + // With autogroup:self, users can only SSH to their own devices + // Test same-user SSH works + for _, client := range nsOneClients { + for _, peer := range nsOneClients { + if client.Hostname() == peer.Hostname() { + continue } + + assertSSHHostname(t, client, peer) } } - testInterUserSSH(nsOneClients, nsTwoClients) - testInterUserSSH(nsTwoClients, nsOneClients) + for _, client := range nsTwoClients { + for _, peer := range nsTwoClients { + if client.Hostname() == peer.Hostname() { + continue + } + + assertSSHHostname(t, client, peer) + } + } + + // Test cross-user SSH is denied (autogroup:self restricts to same user) + for _, client := range nsOneClients { + for _, peer := range nsTwoClients { + assertSSHPermissionDenied(t, client, peer) + } + } + + for _, client := range nsTwoClients { + for _, peer := range nsOneClients { + assertSSHPermissionDenied(t, client, peer) + } + } } func TestSSHNoSSHConfigured(t *testing.T) { @@ -226,6 +262,8 @@ func TestSSHNoSSHConfigured(t *testing.T) { } } +// TestSSHIsBlockedInACL tests that SSH connections timeout when ACL doesn't allow port 22. +// Uses autogroup:self for SSH policy (valid), but ACL only allows port 80. func TestSSHIsBlockedInACL(t *testing.T) { IntegrationSkip(t) @@ -244,12 +282,17 @@ func TestSSHIsBlockedInACL(t *testing.T) { }, }, }, + // Use autogroup:self which is a valid SSH src/dst combination SSHs: []policyv2.SSH{ { - Action: "accept", - Sources: policyv2.SSHSrcAliases{groupp("group:integration-test")}, - Destinations: policyv2.SSHDstAliases{usernamep("user1@")}, - Users: []policyv2.SSHUser{policyv2.SSHUser("ssh-it-user")}, + Action: "accept", + Sources: policyv2.SSHSrcAliases{ + ptr.To(policyv2.AutoGroupMember), + }, + Destinations: policyv2.SSHDstAliases{ + ptr.To(policyv2.AutoGroupSelf), + }, + Users: []policyv2.SSHUser{policyv2.SSHUser("ssh-it-user")}, }, }, }, @@ -277,6 +320,8 @@ func TestSSHIsBlockedInACL(t *testing.T) { } } +// TestSSHUserOnlyIsolation tests that users can only SSH to their own devices. +// Uses user@->user@ rules (valid: same user as src and dst) to achieve isolation. func TestSSHUserOnlyIsolation(t *testing.T) { IntegrationSkip(t) @@ -296,16 +341,18 @@ func TestSSHUserOnlyIsolation(t *testing.T) { }, }, }, + // Per Tailscale rules: when dst is a username, src must be only the same username. + // Valid: user1@ -> user1@, user2@ -> user2@ SSHs: []policyv2.SSH{ { Action: "accept", - Sources: policyv2.SSHSrcAliases{groupp("group:ssh1")}, + Sources: policyv2.SSHSrcAliases{usernamep("user1@")}, Destinations: policyv2.SSHDstAliases{usernamep("user1@")}, Users: []policyv2.SSHUser{policyv2.SSHUser("ssh-it-user")}, }, { Action: "accept", - Sources: policyv2.SSHSrcAliases{groupp("group:ssh2")}, + Sources: policyv2.SSHSrcAliases{usernamep("user2@")}, Destinations: policyv2.SSHDstAliases{usernamep("user2@")}, Users: []policyv2.SSHUser{policyv2.SSHUser("ssh-it-user")}, }, @@ -540,3 +587,141 @@ func TestSSHAutogroupSelf(t *testing.T) { } } } + +// TestSSHInvalidPolicySrcDstValidation tests that invalid SSH src/dst combinations +// are rejected at policy validation time. +// Per Tailscale docs: when dst contains a username, src must contain only the same username. +// See: https://tailscale.com/kb/1337/policy-syntax#dst-1 +// This test verifies #3010 is fixed: SSH from tagged device to user device should be denied. +func TestSSHInvalidPolicySrcDstValidation(t *testing.T) { + // This test doesn't need Docker - it validates policy parsing via NewPolicyManager + // which unmarshals and validates the policy JSON + tests := []struct { + name string + policy string + expectErr bool + errMsg string + }{ + { + name: "tag-src-user-dst-rejected", + policy: `{ + "tagOwners": { + "tag:server": ["user1@"] + }, + "ssh": [{ + "action": "accept", + "src": ["tag:server"], + "dst": ["user1@"], + "users": ["root"] + }] + }`, + expectErr: true, + errMsg: "tags in src cannot SSH to user-owned devices", + }, + { + name: "group-src-user-dst-rejected", + policy: `{ + "groups": { + "group:admins": ["user1@"] + }, + "ssh": [{ + "action": "accept", + "src": ["group:admins"], + "dst": ["user1@"], + "users": ["root"] + }] + }`, + expectErr: true, + errMsg: "groups in src cannot SSH to user-owned devices", + }, + { + name: "different-user-src-dst-rejected", + policy: `{ + "ssh": [{ + "action": "accept", + "src": ["user2@"], + "dst": ["user1@"], + "users": ["root"] + }] + }`, + expectErr: true, + errMsg: "users in dst are only allowed from the same user", + }, + { + name: "same-user-src-dst-allowed", + policy: `{ + "ssh": [{ + "action": "accept", + "src": ["user1@"], + "dst": ["user1@"], + "users": ["root"] + }] + }`, + expectErr: false, + }, + { + name: "tag-src-tag-dst-allowed", + policy: `{ + "tagOwners": { + "tag:client": ["user1@"], + "tag:server": ["user1@"] + }, + "ssh": [{ + "action": "accept", + "src": ["tag:client"], + "dst": ["tag:server"], + "users": ["root"] + }] + }`, + expectErr: false, + }, + { + name: "group-src-tag-dst-allowed", + policy: `{ + "groups": { + "group:admins": ["user1@"] + }, + "tagOwners": { + "tag:server": ["user1@"] + }, + "ssh": [{ + "action": "accept", + "src": ["group:admins"], + "dst": ["tag:server"], + "users": ["root"] + }] + }`, + expectErr: false, + }, + { + name: "autogroup-self-allowed", + policy: `{ + "ssh": [{ + "action": "accept", + "src": ["autogroup:member"], + "dst": ["autogroup:self"], + "users": ["root"] + }] + }`, + expectErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Validate the policy using NewPolicyManager which parses and validates JSON + _, err := policyv2.NewPolicyManager( + []byte(tt.policy), + nil, // no users needed for validation + types.Nodes{}.ViewSlice(), // empty nodes slice + ) + + if tt.expectErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errMsg) + } else { + require.NoError(t, err) + } + }) + } +}