policy/v2: address review on sshTests engine

Empty-dst-nodes fails loudly. Compat runner uses per-capture topology
so capture IPs match. Test rows tightened; stale godoc and goto fixed.

hscontrol/policy/v2/sshtest.go

@@ -264,7 +264,7 @@ func runSSHPolicyTest(
 		return res
 	}
 
-	dstNodes, err := resolveSSHTestDestNodes(test.Dst, pol, users, nodes, srcUserID)
+	dstNodes, emptyDsts, err := resolveSSHTestDestNodes(test.Dst, pol, users, nodes, srcUserID)
 	if err != nil {
 		res.Passed = false
 		res.Errors = append(res.Errors,
@@ -273,6 +273,20 @@ func runSSHPolicyTest(
 		return res
 	}
 
+	// SaaS treats a dst alias that resolves to no nodes as a failure
+	// when the entry has anything to assert. Without this branch, the
+	// per-assertion loops below run zero iterations and the test
+	// passes silently — wrong shape, missed regression.
+	for _, dst := range emptyDsts {
+		res.Passed = false
+		res.Errors = append(res.Errors,
+			fmt.Sprintf("dst alias %q resolved to no nodes", dst))
+	}
+
+	if len(dstNodes) == 0 {
+		return res
+	}
+
 	for _, user := range test.Accept {
 		evaluateAssertion(
 			pol, users, nodes, cache,
@@ -334,6 +348,7 @@ func evaluateAssertion(
 	kind sshAssertion,
 	res *SSHPolicyTestResult,
 ) {
+dstLoop:
 	for _, dst := range dstNodes {
 		dstPol, err := compiledSSHPolicy(pol, users, nodes, cache, dst)
 		if err != nil {
@@ -347,8 +362,8 @@ func evaluateAssertion(
 
 		dstLabel := dst.Hostname()
 
-		// reachableAccept covers "any matching accept-or-check rule";
-		// reachableCheck restricts to check-action matches only.
+		// acceptHit covers "any matching accept-or-check rule";
+		// checkHit restricts to check-action matches only.
 		acceptHit := false
 		checkHit := false
 
@@ -371,14 +386,14 @@ func evaluateAssertion(
 					res.Passed = false
 					res.AcceptFail = appendUserDst(res.AcceptFail, user, dstLabel)
 
-					goto nextDst
+					continue dstLoop
 				}
 			case assertDeny:
 				if a {
 					res.Passed = false
 					res.DenyFail = appendUserDst(res.DenyFail, user, dstLabel)
 
-					goto nextDst
+					continue dstLoop
 				}
 			case assertCheck:
 				if !c {
@@ -392,7 +407,7 @@ func evaluateAssertion(
 						res.AcceptOK = appendUserDst(res.AcceptOK, user, dstLabel)
 					}
 
-					goto nextDst
+					continue dstLoop
 				}
 			}
 		}
@@ -409,8 +424,6 @@ func evaluateAssertion(
 				res.CheckOK = appendUserDst(res.CheckOK, user, dstLabel)
 			}
 		}
-
-	nextDst:
 	}
 }
 
@@ -482,25 +495,32 @@ func resolveSSHTestDestNodes(
 	users []types.User,
 	nodes views.Slice[types.NodeView],
 	srcUserID uint,
-) ([]types.NodeView, error) {
+) ([]types.NodeView, []string, error) {
 	seen := make(map[types.NodeID]struct{})
 
-	var out []types.NodeView
+	var (
+		out       []types.NodeView
+		emptyDsts []string
+	)
 
 	for _, dst := range dsts {
 		alias, err := parseAlias(dst)
 		if err != nil {
-			return nil, fmt.Errorf("invalid destination %q: %w", dst, err)
+			return nil, nil, fmt.Errorf("invalid destination %q: %w", dst, err)
 		}
 
+		matched := false
+
 		if ag, ok := alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) {
 			// autogroup:self → destinations are the non-tagged
 			// nodes owned by the same user as src. A tagged or
 			// IP-only src has no user identity, so the dst set
-			// is empty (matches SaaS, which treats this as a
-			// no-op assertion that the engine then reports as a
-			// failure via the empty-dst-nodes branch below).
+			// is empty and the caller surfaces it as a failure
+			// (matches SaaS, which treats a no-node dst as a
+			// failing assertion).
 			if srcUserID == 0 {
+				emptyDsts = append(emptyDsts, dst)
+
 				continue
 			}
 
@@ -517,6 +537,8 @@ func resolveSSHTestDestNodes(
 					continue
 				}
 
+				matched = true
+
 				if _, dup := seen[n.ID()]; dup {
 					continue
 				}
@@ -525,15 +547,21 @@ func resolveSSHTestDestNodes(
 				out = append(out, n)
 			}
 
+			if !matched {
+				emptyDsts = append(emptyDsts, dst)
+			}
+
 			continue
 		}
 
 		ips, err := alias.Resolve(pol, users, nodes)
 		if err != nil {
-			return nil, fmt.Errorf("resolving destination %q: %w", dst, err)
+			return nil, nil, fmt.Errorf("resolving destination %q: %w", dst, err)
 		}
 
 		if ips == nil || ips.Empty() {
+			emptyDsts = append(emptyDsts, dst)
+
 			continue
 		}
 
@@ -542,7 +570,7 @@ func resolveSSHTestDestNodes(
 		// the resolved prefixes.
 		set, err := prefixesToIPSet(ips.Prefixes())
 		if err != nil {
-			return nil, fmt.Errorf("building IPSet for %q: %w", dst, err)
+			return nil, nil, fmt.Errorf("building IPSet for %q: %w", dst, err)
 		}
 
 		for _, n := range nodes.All() {
@@ -550,6 +578,8 @@ func resolveSSHTestDestNodes(
 				continue
 			}
 
+			matched = true
+
 			if _, dup := seen[n.ID()]; dup {
 				continue
 			}
@@ -557,9 +587,13 @@ func resolveSSHTestDestNodes(
 			seen[n.ID()] = struct{}{}
 			out = append(out, n)
 		}
+
+		if !matched {
+			emptyDsts = append(emptyDsts, dst)
+		}
 	}
 
-	return out, nil
+	return out, emptyDsts, nil
 }
 
 // prefixesToIPSet builds a netipx.IPSet from a slice of prefixes. The

hscontrol/policy/v2/sshtest_test.go

@@ -1,6 +1,7 @@
 package v2
 
 import (
+	"encoding/json"
 	"strings"
 	"testing"
 
@@ -8,7 +9,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gorm.io/gorm"
-	"tailscale.com/tailcfg"
 )
 
 // sshTestUsers/sshTestNodes are reused across the table below to keep
@@ -563,24 +563,6 @@ func TestRunSSHTests(t *testing.T) {
 			}`,
 			wantPass: true,
 		},
-		{
-			name: "tag-as-dst-single-node",
-			policy: `{
-				"tagOwners": { "tag:server": ["alice@headscale.net"] },
-				"ssh": [{
-					"action": "accept",
-					"src":    ["alice@headscale.net"],
-					"dst":    ["tag:server"],
-					"users":  ["root"]
-				}],
-				"sshTests": [{
-					"src":    "alice@headscale.net",
-					"dst":    ["tag:server"],
-					"accept": ["root"]
-				}]
-			}`,
-			wantPass: true,
-		},
 		{
 			name: "host-alias-as-dst",
 			policy: `{
@@ -618,9 +600,19 @@ func TestRunSSHTests(t *testing.T) {
 			wantErrSub: []string{"alice@headscale.net", "root", "expected ALLOWED"},
 		},
 		{
+			// ACL grants only TCP:80 to alice; no rule grants TCP:22.
+			// SSH rule independently allows root@tag:server. The
+			// sshTests assertion must pass on the SSH layer alone,
+			// proving the engine does not require an ACL packet-
+			// filter rule for the SSH port.
 			name: "acl-denies-tcp22-ssh-rule-allows",
 			policy: `{
 				"tagOwners": { "tag:server": ["alice@headscale.net"] },
+				"acls": [{
+					"action": "accept",
+					"src":    ["alice@headscale.net"],
+					"dst":    ["tag:server:80"]
+				}],
 				"ssh": [{
 					"action": "accept",
 					"src":    ["alice@headscale.net"],
@@ -704,6 +696,40 @@ func TestRunSSHTests(t *testing.T) {
 			wantPass:   false,
 			wantErrSub: []string{"alice@headscale.net", "expected ALLOWED"},
 		},
+		{
+			// tag:empty has an owner but no tagged nodes, so the dst
+			// alias resolves to no nodes. Without the empty-dst guard
+			// the per-assertion loop runs zero iterations and the
+			// test silently passes — exactly the regression the
+			// guard exists to catch.
+			name: "dst-tag-with-no-tagged-nodes-fails",
+			policy: `{
+				"tagOwners": { "tag:empty": ["alice@headscale.net"] },
+				"sshTests": [{
+					"src":    "alice@headscale.net",
+					"dst":    ["tag:empty"],
+					"accept": ["root"]
+				}]
+			}`,
+			wantPass:   false,
+			wantErrSub: []string{"tag:empty", "resolved to no nodes"},
+		},
+		{
+			// autogroup:self from a tag src has no user identity to
+			// scope to, so the dst alias resolves to no nodes. Same
+			// empty-dst guard, distinct trigger path.
+			name: "dst-autogroup-self-from-tag-src-fails",
+			policy: `{
+				"tagOwners": { "tag:prod": ["alice@headscale.net"] },
+				"sshTests": [{
+					"src":    "tag:prod",
+					"dst":    ["autogroup:self"],
+					"accept": ["root"]
+				}]
+			}`,
+			wantPass:   false,
+			wantErrSub: []string{"autogroup:self", "resolved to no nodes"},
+		},
 	}
 
 	for _, tt := range tests {
@@ -813,12 +839,19 @@ func TestSetPolicyRejectsFailingSSHTests(t *testing.T) {
 
 	// Snapshot SSHPolicy output for alice-laptop before the rejected
 	// write — the live PolicyManager state must still describe the
-	// previous (good) rules afterwards.
+	// previous (good) rules afterwards. JSON-marshal the snapshot so
+	// the comparison sees rule content, not just object identity: a
+	// hypothetical mutation that preserves the slice length but
+	// rewrites principals or SSHUsers would slip past a count-only
+	// assertion.
 	aliceView := nodes.ViewSlice().At(0)
 
 	beforePol, err := pm.SSHPolicy("", aliceView)
 	require.NoError(t, err)
 
+	beforeJSON, err := json.Marshal(beforePol)
+	require.NoError(t, err)
+
 	changed, err := pm.SetPolicy([]byte(bad))
 	require.Error(t, err, "SetPolicy must reject a policy whose sshTests fail")
 	require.False(t, changed, "SetPolicy must report no change when rejected")
@@ -827,7 +860,11 @@ func TestSetPolicyRejectsFailingSSHTests(t *testing.T) {
 
 	afterPol, err := pm.SSHPolicy("", aliceView)
 	require.NoError(t, err)
-	require.Equal(t, sshRuleCount(beforePol), sshRuleCount(afterPol),
+
+	afterJSON, err := json.Marshal(afterPol)
+	require.NoError(t, err)
+
+	require.JSONEq(t, string(beforeJSON), string(afterJSON),
 		"live SSH policy must not change after a rejected SetPolicy")
 }
 
@@ -903,6 +940,12 @@ func TestSetPolicyAggregatesACLAndSSHTestFailures(t *testing.T) {
 		"aggregated error must include the ACL failure message")
 	assert.Contains(t, body, "bob@headscale.net",
 		"aggregated error must include the bob src")
+	// The SSH renderer emits "src/user -> dst" form; the ACL renderer
+	// emits "src -> dst". Substring "/root -> " is unique to the SSH
+	// body, so finding it inside the aggregated error proves the SSH
+	// failure rendering was concatenated alongside the ACL body.
+	assert.Contains(t, body, "/root -> ",
+		"aggregated error must include the SSH-shape src/user -> dst rendering")
 }
 
 // TestNewPolicyManagerWarnsOnSSHTestsFailure asserts the boot path does
@@ -977,14 +1020,3 @@ func TestSSHPolicyTestResultsErrorsRendering(t *testing.T) {
 	assert.Equal(t, 3, strings.Count(rendered, "\n")+1,
 		"expected one line per failing assertion")
 }
-
-// sshRuleCount returns the number of rules in p, treating nil as 0.
-// Used by TestSetPolicyRejectsFailingSSHTests to verify a rejected
-// SetPolicy leaves the live SSH policy untouched.
-func sshRuleCount(p *tailcfg.SSHPolicy) int {
-	if p == nil {
-		return 0
-	}
-
-	return len(p.Rules)
-}

hscontrol/policy/v2/sshtester_compat_test.go

@@ -32,16 +32,6 @@ import (
 // disagree on whether a policy is accepted. Each entry should describe
 // the engine area a follow-up PR needs to touch.
 var knownSSHTesterDivergences = map[string]string{
-	// SaaS resolves an IP-literal sshTests src to the owning node's
-	// user identity and matches it against compiled SSH principals;
-	// headscale's resolveSSHTestSource returns srcUserID=0 for any
-	// non-Username alias and only consults principal NodeIPs, so an
-	// IP that names a user-owned node misses every accept rule keyed
-	// by that user. Fix in hscontrol/policy/v2/sshtest.go — extend
-	// resolveSSHTestSource to recover the owning user when the src
-	// resolves to exactly one user-owned node.
-	"sshtest-ip-literal-src": "headscale does not map an IP-literal sshTests src to the owning node's user; SaaS does",
-
 	// SaaS rejects `users: ["*"]` on an `ssh` rule at policy-parse
 	// time with `user "*" is not valid`; headscale accepts the
 	// wildcard and proceeds to evaluate sshTests against it. Fix in
@@ -61,7 +51,6 @@ func TestSSHTesterCompat(t *testing.T) {
 	}
 
 	users := setupSSHDataCompatUsers()
-	nodes := setupSSHDataCompatNodes(users)
 
 	for _, file := range files {
 		c, err := testcapture.Read(file)
@@ -74,6 +63,14 @@ func TestSSHTesterCompat(t *testing.T) {
 				t.Skip(reason)
 			}
 
+			// Per-capture nodes mean the topology IPs (which a
+			// policy `hosts` mapping references by literal IP)
+			// resolve to real nodes in the test fixture. Without
+			// this the static fixture's IPs do not overlap with
+			// the captures and host-alias dsts resolve to no
+			// nodes — that path is now a load-bearing failure.
+			nodes := buildGrantsNodesFromCapture(users, c)
+
 			policyJSON := []byte(c.Input.FullPolicy)
 
 			pm, parseErr := NewPolicyManager(policyJSON, users, nodes.ViewSlice())

hscontrol/policy/v2/tailscale_ssh_data_compat_test.go

@@ -61,61 +61,6 @@ func setupSSHDataCompatUsers() types.Users {
 	}
 }
 
-// setupSSHDataCompatNodes returns the test nodes for SSH data-driven
-// compatibility tests. Node GivenNames match the anonymized pokémon names:
-//   - bulbasaur (owned by odin)
-//   - ivysaur (owned by thor)
-//   - venusaur (owned by freya)
-//   - beedrill (tag:server)
-//   - kakuna (tag:prod)
-func setupSSHDataCompatNodes(users types.Users) types.Nodes {
-	return types.Nodes{
-		&types.Node{
-			ID:        1,
-			GivenName: "bulbasaur",
-			User:      &users[0],
-			UserID:    &users[0].ID,
-			IPv4:      ptrAddr("100.90.199.68"),
-			IPv6:      ptrAddr("fd7a:115c:a1e0::2d01:c747"),
-			Hostinfo:  &tailcfg.Hostinfo{},
-		},
-		&types.Node{
-			ID:        2,
-			GivenName: "ivysaur",
-			User:      &users[1],
-			UserID:    &users[1].ID,
-			IPv4:      ptrAddr("100.110.121.96"),
-			IPv6:      ptrAddr("fd7a:115c:a1e0::1737:7960"),
-			Hostinfo:  &tailcfg.Hostinfo{},
-		},
-		&types.Node{
-			ID:        3,
-			GivenName: "venusaur",
-			User:      &users[2],
-			UserID:    &users[2].ID,
-			IPv4:      ptrAddr("100.103.90.82"),
-			IPv6:      ptrAddr("fd7a:115c:a1e0::9e37:5a52"),
-			Hostinfo:  &tailcfg.Hostinfo{},
-		},
-		&types.Node{
-			ID:        4,
-			GivenName: "beedrill",
-			IPv4:      ptrAddr("100.108.74.26"),
-			IPv6:      ptrAddr("fd7a:115c:a1e0::b901:4a87"),
-			Tags:      []string{"tag:server"},
-			Hostinfo:  &tailcfg.Hostinfo{},
-		},
-		&types.Node{
-			ID:        5,
-			GivenName: "kakuna",
-			IPv4:      ptrAddr("100.103.8.15"),
-			IPv6:      ptrAddr("fd7a:115c:a1e0::5b37:80f"),
-			Tags:      []string{"tag:prod"},
-			Hostinfo:  &tailcfg.Hostinfo{},
-		},
-	}
-}
-
 // loadSSHTestFile loads and parses a single SSH capture HuJSON file.
 func loadSSHTestFile(t *testing.T, path string) *testcapture.Capture {
 	t.Helper()

hscontrol/policy/v2/test.go

@@ -82,10 +82,11 @@ type SSHPolicyTest struct {
 	// Src connects to each entry in Dst.
 	Deny []string `json:"deny,omitempty"`
 
-	// Check lists SSH login users that must be allowed by an action:check
-	// rule specifically. action:accept matches do not satisfy a check
-	// assertion. Engine evaluation is not implemented yet; parse-time
-	// validation accepts the field so policies can be authored ahead of it.
+	// Check lists SSH login users that must reach every dst via an
+	// action:check rule specifically (the HoldAndDelegate signal on the
+	// compiled SSH policy). An action:accept rule alone does not satisfy
+	// a check assertion — SaaS keeps the two categories distinct so
+	// policy authors can pin sensitive logins to check rules.
 	Check []string `json:"check,omitempty"`
 }