policy/v2: tighten comments and CHANGELOG style on sshTests

Drop redundant function doc-comments; trim block comments to short paragraphs or lists; remove SaaS-body citations. Drop the HuJSON example from the CHANGELOG entry and split SSH rule validation into its own subsection.

CHANGELOG.md

@@ -46,40 +46,27 @@ This feature is **beta** while behavioural coverage against Tailscale SaaS broad
 
 ### SSH policy tests (beta)
 
-Headscale now evaluates the `sshTests` block in a policy file. Tests assert which SSH login users
-can connect from a named source to named destinations against the same SSH rules clients receive.
-They run on `headscale policy set`, on SIGHUP reload (`systemctl reload headscale` /
-`kill -HUP $(pidof headscale)`), and on `headscale policy check`. A failing test rejects the write
-before it is applied, with the same error message Tailscale SaaS would return for the same policy.
-
-An entry has the shape:
-
-```hujson
-"sshTests": [
-    {
-        "src":    "alice@example.com",
-        "dst":    ["tag:server"],
-        "accept": ["root"],
-        "deny":   ["alice"],
-        "check":  ["ubuntu"]
-    }
-]
-```
-
-`accept` asserts the listed login users reach every dst via an accept- or check-action SSH rule,
-`deny` asserts none of them reach any dst, and `check` requires reachability specifically via a
-check-action rule.
+Headscale now evaluates the `sshTests` block in a policy file. Each entry names a source, one or
+more destination hosts, and three optional user lists: `accept` asserts the listed login users
+reach every destination via an accept- or check-action SSH rule, `deny` asserts none of them
+reach any destination, and `check` requires reachability specifically through a check-action
+rule. Tests run on `headscale policy set`, on SIGHUP reload (`systemctl reload headscale` /
+`kill -HUP $(pidof headscale)`), and on `headscale policy check`. A failing test rejects the
+write before it is applied, with the same error message Tailscale SaaS would return for the same
+policy.
 
 At boot a stored policy whose sshTests no longer pass — for example because a referenced user was
-deleted while the server was offline — logs a warning and the server keeps running. Fix the policy
-and reload.
+deleted while the server was offline — logs a warning and the server keeps running. Fix the
+policy and reload.
 
 This feature is **beta** while behavioural coverage against Tailscale SaaS broadens.
 
-SSH rule validation now trims whitespace on `action`, `users`, `src`, and `dst`, rejects empty or
-wildcard entries in `users`, rejects empty `acceptEnv` and negative `checkPeriod`, rejects
-`hosts:` aliases as SSH dst, rejects non-ASCII tag names, and matches the rejection wording for
-group-nesting cycles.
+### SSH rule validation
+
+SSH rule parsing now trims surrounding whitespace on `action`, `users`, `src`, and `dst`,
+rejects empty or wildcard entries in `users`, rejects empty `acceptEnv`, and rejects negative
+`checkPeriod`. `hosts:` aliases are rejected as SSH destinations, non-ASCII tag names are
+rejected at parse time, and the wording for group-nesting cycles matches Tailscale SaaS.
 
 ### Grants
 

hscontrol/policy/v2/sshtest.go

@@ -12,26 +12,17 @@ import (
 	"tailscale.com/types/views"
 )
 
-// Each sshTests entry asserts that a source identity attempting SSH to
-// one or more destination hosts can (or cannot) log in as the named
-// users. Evaluation runs at user-initiated writes (SetPolicy, policy
-// check, file reload); boot reload skips evaluation so a stale
-// reference does not block startup.
+// sshTests assertions evaluate on user-initiated writes; boot reload
+// skips them so a stale reference does not block startup. Each entry
+// names a src and one or more dst, and uses:
 //
-// Three assertion kinds:
-//
-//   - accept[user]: every (src, dst) must reach via an accept- or
-//     check-action rule. Both actions resolve to "session permitted"
-//     at the wire layer, so check counts as reachable for accept.
-//   - deny[user]: no (src, dst) reaches. Passes when no rule allows
-//     the user or every matching rule's SSHUsers map blocks them.
-//   - check[user]: every (src, dst) must reach via a check-action
-//     rule (HoldAndDelegate set; see filter.go sshCheck). An
-//     accept-only match fails — the two categories are kept distinct
-//     so policy authors can pin sensitive logins to check rules.
+//   - accept: every listed user reaches every dst via an accept- or
+//     check-action rule.
+//   - deny: no listed user reaches any dst.
+//   - check: every listed user reaches every dst via a check-action
+//     rule specifically (accept-only matches fail the assertion).
 
-// SSHPolicyTestResult is the outcome of a single SSHPolicyTest. Each
-// map is keyed by login user with the per-dst breakdown.
+// SSHPolicyTestResult is the outcome of a single SSHPolicyTest.
 type SSHPolicyTestResult struct {
 	Src    string   `json:"src"`
 	Passed bool     `json:"passed"`
@@ -52,8 +43,6 @@ type SSHPolicyTestResults struct {
 }
 
 // Errors renders the per-test failure breakdown joined by newlines.
-// Operators invoking SetPolicy from the CLI or file reload have no
-// separate audit channel, so the rendered body is their only signal.
 func (r SSHPolicyTestResults) Errors() string {
 	if r.AllPassed {
 		return ""
@@ -102,8 +91,6 @@ func (r SSHPolicyTestResults) Errors() string {
 	return strings.Join(lines, "\n")
 }
 
-// sortedUsers returns the keys of m sorted by user name so error
-// rendering is deterministic across runs.
 func sortedUsers(m map[string][]string) []string {
 	keys := make([]string, 0, len(m))
 	for k := range m {
@@ -115,9 +102,7 @@ func sortedUsers(m map[string][]string) []string {
 	return keys
 }
 
-// displayUser formats a login user for the rendered error. An empty
-// string is shown as `""` so the operator can see that the assertion
-// referenced an empty username (which is itself a failure case).
+// displayUser shows an empty username as `""` rather than blank.
 func displayUser(u string) string {
 	if u == "" {
 		return `""`
@@ -126,9 +111,8 @@ func displayUser(u string) string {
 	return u
 }
 
-// checkFailReason annotates a check-fail line with whether the user
-// reached the dst via an accept rule (so the operator knows to flip the
-// rule to action:check) or did not reach the dst at all.
+// checkFailReason annotates a check-fail with whether the user reached
+// the dst via an accept rule or did not reach at all.
 func checkFailReason(res SSHPolicyTestResult, user, dst string) string {
 	if slices.Contains(res.AcceptOK[user], dst) {
 		return "ALLOWED via accept"
@@ -137,10 +121,8 @@ func checkFailReason(res SSHPolicyTestResult, user, dst string) string {
 	return "DENIED"
 }
 
-// RunSSHTests evaluates the policy's sshTests block against the live
-// users and nodes and returns a wrapped error when any assertion fails.
-// Callers that need the per-test breakdown can call runSSHPolicyTests
-// directly with their own compile cache.
+// RunSSHTests evaluates the live policy's sshTests block and wraps any
+// failure in errSSHPolicyTestsFailed.
 func (pm *PolicyManager) RunSSHTests() error {
 	if pm == nil || pm.pol == nil || len(pm.pol.SSHTests) == 0 {
 		return nil
@@ -159,9 +141,7 @@ func (pm *PolicyManager) RunSSHTests() error {
 	return fmt.Errorf("%w:\n%s", errSSHPolicyTestsFailed, results.Errors())
 }
 
-// evaluateSSHTests is the user-write sandbox: run sshTests against pol
-// + current users/nodes without mutating any live state. It mirrors
-// evaluateTests for the ACL block.
+// evaluateSSHTests runs the block against pol without mutating live state.
 func evaluateSSHTests(
 	pol *Policy,
 	users []types.User,
@@ -181,9 +161,8 @@ func evaluateSSHTests(
 	return fmt.Errorf("%w:\n%s", errSSHPolicyTestsFailed, results.Errors())
 }
 
-// runSSHPolicyTests evaluates every sshTests entry against pol. The
-// cache is keyed by destination node ID and reused across entries so a
-// 10-entry block hitting 4 dst nodes pays 4 compiles, not 40.
+// runSSHPolicyTests evaluates every sshTests entry. The cache is keyed
+// by dst NodeID so repeat destinations only compile once per pass.
 func runSSHPolicyTests(
 	pol *Policy,
 	users []types.User,
@@ -207,11 +186,8 @@ func runSSHPolicyTests(
 	return results
 }
 
-// runSSHPolicyTest evaluates one SSHPolicyTest entry against pol.
-//
-// Order of operations: resolve src → resolve dst nodes → reject empty
-// assertion blocks → walk accept/deny/check arrays, asking the per-dst
-// compiled SSH policy whether the user can reach the dst.
+// runSSHPolicyTest evaluates one entry: resolve src → resolve dst →
+// walk accept/deny/check arrays against each dst's compiled SSH policy.
 func runSSHPolicyTest(
 	test SSHPolicyTest,
 	pol *Policy,
@@ -246,8 +222,7 @@ func runSSHPolicyTest(
 		return res
 	}
 
-	// An entry with no accept/deny/check arrays asserts nothing — flag
-	// it explicitly so a silent pass cannot mask misconfiguration.
+	// An entry with no assertion arrays would silently pass.
 	if len(test.Accept) == 0 && len(test.Deny) == 0 && len(test.Check) == 0 {
 		res.Passed = false
 		res.Errors = append(res.Errors,
@@ -265,9 +240,7 @@ func runSSHPolicyTest(
 		return res
 	}
 
-	// A dst alias resolving to no nodes makes the per-assertion loops
-	// below run zero iterations and the test pass silently — surface
-	// it as a failure instead.
+	// A dst resolving to zero nodes would silently pass.
 	for _, dst := range emptyDsts {
 		res.Passed = false
 		res.Errors = append(res.Errors,
@@ -305,8 +278,6 @@ func runSSHPolicyTest(
 	return res
 }
 
-// sshAssertion is the kind of assertion being evaluated for a single
-// (src, dst, user) triple.
 type sshAssertion int
 
 const (
@@ -316,11 +287,8 @@ const (
 )
 
 // evaluateAssertion walks every (srcAddr, dstNode) pair for one user
-// and records the outcome in res. accept passes iff every pair reaches
-// via an accept- or check-action rule; deny passes iff no pair
-// reaches; check requires HoldAndDelegate on the matching rule.
-// Empty username is parse-accepted but fails here because SSH login
-// users cannot be empty.
+// and records the outcome. Empty username fails — SSH login users
+// cannot be empty even when parse accepted it.
 func evaluateAssertion(
 	pol *Policy,
 	users []types.User,
@@ -346,8 +314,6 @@ dstLoop:
 
 		dstLabel := dst.Hostname()
 
-		// acceptHit covers "any matching accept-or-check rule";
-		// checkHit restricts to check-action matches only.
 		acceptHit := false
 		checkHit := false
 
@@ -361,9 +327,8 @@ dstLoop:
 				checkHit = true
 			}
 
-			// accept and deny require ALL src IPs to reach (or all
-			// to be blocked). A single counter-example fails the
-			// assertion.
+			// All src IPs must agree; one counter-example fails
+			// the whole (user, dst) pair.
 			switch kind {
 			case assertAccept:
 				if !a {
@@ -411,7 +376,7 @@ dstLoop:
 	}
 }
 
-// appendUserDst appends dst to m[user], lazily allocating m.
+// appendUserDst appends dst to m[user], allocating m on first use.
 func appendUserDst(m map[string][]string, user, dst string) map[string][]string {
 	if m == nil {
 		m = make(map[string][]string)
@@ -422,11 +387,9 @@ func appendUserDst(m map[string][]string, user, dst string) map[string][]string
 	return m
 }
 
-// resolveSSHTestSource resolves the typed src alias into a list of
-// netip.Addr (one per principal address the SSH compiler would emit
-// for the same source). For user-shaped sources, srcUserID returns the
-// resolved user's ID so autogroup:self destinations can scope to the
-// same user. Returns ID 0 when the source is a tag, host, or IP.
+// resolveSSHTestSource returns the src's principal addresses and, for
+// user-shaped sources, the user ID (so autogroup:self can scope to it).
+// Tag, host, and IP sources return userID 0.
 func resolveSSHTestSource(
 	src Alias,
 	pol *Policy,
@@ -464,10 +427,10 @@ func resolveSSHTestSource(
 	return out, userID, nil
 }
 
-// resolveSSHTestDestNodes resolves every dst alias to its destination
-// NodeViews. autogroup:self is handled separately because it cannot
-// resolve outside a per-node context. For every other alias, the
-// resolved IPSet is matched against each node's IPs via InIPSet.
+// resolveSSHTestDestNodes maps each dst alias to its destination
+// NodeViews. autogroup:self needs special handling: it cannot resolve
+// without per-node context, so it walks the node set keyed on src's
+// owning user. Other aliases resolve to an IPSet and match via InIPSet.
 func resolveSSHTestDestNodes(
 	dsts SSHTestDestinations,
 	pol *Policy,
@@ -487,10 +450,8 @@ func resolveSSHTestDestNodes(
 		matched := false
 
 		if ag, ok := alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) {
-			// autogroup:self → 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 and the caller
-			// surfaces a failing assertion.
+			// autogroup:self resolves to non-tagged nodes owned by
+			// the same user as src; tagged/IP sources have no user.
 			if srcUserID == 0 {
 				emptyDsts = append(emptyDsts, dstLabel)
 
@@ -538,9 +499,6 @@ func resolveSSHTestDestNodes(
 			continue
 		}
 
-		// Compile to an IPSet for the InIPSet primitive. ResolvedAddresses
-		// already wraps one; expose it via the IPSet builder by walking
-		// the resolved prefixes.
 		set, err := prefixesToIPSet(ips.Prefixes())
 		if err != nil {
 			return nil, nil, fmt.Errorf("building IPSet for %q: %w", dstLabel, err)
@@ -581,10 +539,9 @@ func prefixesToIPSet(prefixes []netip.Prefix) (*netipx.IPSet, error) {
 	return b.IPSet()
 }
 
-// compiledSSHPolicy returns the per-node compiled SSH policy, populating
-// cache on miss. baseURL is empty because the engine only needs the
-// "is this rule a check rule" signal (HoldAndDelegate non-empty), not
-// the actual URL contents.
+// compiledSSHPolicy returns the per-node compiled SSH policy, caching
+// on miss. baseURL is empty because reachability only checks for the
+// presence of HoldAndDelegate, not its value.
 func compiledSSHPolicy(
 	pol *Policy,
 	users []types.User,
@@ -606,15 +563,10 @@ func compiledSSHPolicy(
 	return sshPol, nil
 }
 
-// reachability walks dstPolicy.Rules and reports whether srcAddr is
-// allowed to log in as user via:
+// reachability reports whether srcAddr can log in as user via:
 //
-//   - any rule (first return) — satisfies accept assertions
-//   - a check rule specifically (second return) — satisfies check assertions
-//
-// A nil policy is treated as "no rule matches", which is the right
-// answer for both accept (DENIED) and check (DENIED) and for deny
-// (PASS, because the deny assertion inverts).
+//   - any matching rule (acceptHit, satisfies accept assertions)
+//   - a check-action rule (checkHit, satisfies check assertions)
 func reachability(
 	dstPolicy *tailcfg.SSHPolicy,
 	srcAddr netip.Addr,
@@ -645,8 +597,8 @@ func reachability(
 			checkHit = true
 		}
 
-		// Early-out only when both bits are set; a rule that
-		// satisfies one assertion may not satisfy the other.
+		// Early-out only when both bits are set: a rule satisfying
+		// accept does not always satisfy check.
 		if acceptHit && checkHit {
 			return acceptHit, checkHit
 		}
@@ -655,9 +607,8 @@ func reachability(
 	return acceptHit, checkHit
 }
 
-// principalContainsAddr reports whether any principal has a NodeIP
-// matching srcAddr. The SSH compiler emits one principal per source
-// IP, so an exact-match comparison is correct.
+// principalContainsAddr reports whether any principal's NodeIP matches
+// srcAddr exactly (the SSH compiler emits one principal per source IP).
 func principalContainsAddr(
 	principals []*tailcfg.SSHPrincipal,
 	srcAddr netip.Addr,
@@ -684,16 +635,13 @@ func principalContainsAddr(
 	return false
 }
 
-// sshUserMapAllows reports whether SSHUsers permits user. The wire
-// shape (see filter.go compileSSHPolicy):
+// sshUserMapAllows reports whether SSHUsers permits user. The SSHUsers
+// wire shape (see filter.go compileSSHPolicy):
 //
-//   - SSHUsers["root"] == "root" when the rule lists "root"; == ""
-//     means root is explicitly disallowed.
-//   - SSHUsers["*"] == "=" when the rule lists autogroup:nonroot —
-//     wildcard fallback for any non-root user.
-//   - SSHUsers[<literal>] == <literal> for every named SSH user.
-//
-// Empty user input (parse-accepted as a failure case) matches nothing.
+//   - SSHUsers["root"] == "root" allows root; == "" disallows it.
+//   - SSHUsers["*"] == "=" is the wildcard fallback for non-root users
+//     (set when the rule lists autogroup:nonroot).
+//   - SSHUsers[<literal>] == <literal> for every named user.
 func sshUserMapAllows(m map[string]string, user string) bool {
 	if user == "" {
 		return false

hscontrol/policy/v2/sshtester_compat_test.go

@@ -15,9 +15,8 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// knownSSHTesterDivergences documents captures where headscale and the
-// upstream control plane disagree. Each entry names the engine area a
-// follow-up needs to touch.
+// knownSSHTesterDivergences names the engine gap for each capture where
+// headscale and upstream disagree.
 var knownSSHTesterDivergences = map[string]string{
 	"sshtest-malformed-dst-bare-ipv6": "bare-IPv6 sshTests dst: upstream parse-accepts then engine-rejects; headscale accepts (IPv4 mirror passes both sides)",
 }
@@ -45,11 +44,8 @@ func TestSSHTesterCompat(t *testing.T) {
 				t.Skip(reason)
 			}
 
-			// Per-capture nodes: each capture pins its own
-			// topology IPs, and policy `hosts` aliases reference
-			// them by literal IP. A shared fixture would leave
-			// host-alias dsts resolving to no nodes — surface
-			// the path as a load-bearing failure instead.
+			// Each capture pins its own topology IPs; build nodes
+			// from the capture so host-alias dsts resolve.
 			nodes := buildGrantsNodesFromCapture(users, c)
 
 			policyJSON := []byte(c.Input.FullPolicy)

hscontrol/policy/v2/test.go

@@ -25,16 +25,9 @@ import (
 // The tests evaluate against the compiled global filter rules, which fold in
 // both `acls` and `grants`, so the `tests` block validates the whole policy.
 
-// errPolicyTestsFailed wraps the rendered failure body so callers can
-// type-assert when they need to react differently to test failures vs. parse
-// errors. The Error() prefix is "test(s) failed", the same string Tailscale
-// SaaS returns in the api_response_body.message — see
-// hscontrol/policy/v2/testdata/policytest_results/.
-//
-// errSSHPolicyTestsFailed wraps sshTests failures. Tailscale SaaS returns the
-// same literal "test(s) failed" body for both ACL tests and SSH tests, but
-// the two sentinels are kept as distinct values so callers can use errors.Is
-// to tell them apart while still matching the SaaS body byte-for-byte.
+// errPolicyTestsFailed and errSSHPolicyTestsFailed share the
+// "test(s) failed" prefix but stay distinct so callers can use
+// errors.Is to tell ACL-test and SSH-test failures apart.
 var (
 	errPolicyTestsFailed    = errors.New("test(s) failed")
 	errSSHPolicyTestsFailed = errors.New("test(s) failed")
@@ -60,47 +53,36 @@ type PolicyTest struct {
 	Deny []string `json:"deny,omitempty"`
 }
 
-// SSHPolicyTest is one entry in the policy's `sshTests` block. Unlike the
-// ACL `tests` block, sshTests describe SSH login attempts: a source alias
-// connects to each destination host and tries each named login user. The
-// accept / deny / check arrays carry usernames, not destinations — every
-// listed user is asserted against every entry in dst.
+// SSHPolicyTest is one entry in the policy's `sshTests` block. The
+// accept/deny/check arrays carry usernames, not destinations — every
+// listed user is asserted against every entry in Dst.
 type SSHPolicyTest struct {
-	// Src is a single source alias (user, group, tag, host, or IP). Same
-	// shape as PolicyTest.Src — Tailscale only supports one src per entry.
+	// Src is a single source alias (user, group, tag, host, or IP).
 	Src Alias `json:"src"`
 
-	// Dst lists destination host aliases the test exercises. Tags, hosts,
-	// and the SSH-compatible autogroups are valid; ports, CIDR ranges, and
-	// autogroup:internet are rejected at parse time.
+	// Dst lists destinations the test exercises (tag, host, or SSH-
+	// compatible autogroup). Ports, CIDRs, and autogroup:internet are
+	// rejected at parse time.
 	Dst SSHTestDestinations `json:"dst"`
 
-	// Accept lists SSH login users that must be allowed by an action:accept
-	// or action:check rule when Src connects to each entry in Dst.
+	// Accept lists users that must reach every Dst via an accept- or
+	// check-action rule.
 	Accept []SSHUser `json:"accept,omitempty"`
 
-	// Deny lists SSH login users that must NOT be allowed by any rule when
-	// Src connects to each entry in Dst.
+	// Deny lists users that must NOT reach any Dst.
 	Deny []SSHUser `json:"deny,omitempty"`
 
-	// 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 lists users that must reach every Dst via a check-action
+	// rule specifically; an accept-action rule does not satisfy this.
 	Check []SSHUser `json:"check,omitempty"`
 }
 
-// SSHTestDestinations is the list of destination aliases an sshTests entry
-// targets. Unmarshalling reuses the same alias parser the rest of the
-// policy engine drives so each element lands as a typed Alias; the parse-
-// time shape rules in validateSSHTestDestination continue to enforce the
-// SSH-specific restrictions (no :port, no CIDR, no autogroup:internet,
+// SSHTestDestinations is the typed list of destination aliases an
+// sshTests entry targets. validateSSHTestDestination enforces the
+// SSH-specific shape rules (no :port, no CIDR, no autogroup:internet,
 // known tag).
 type SSHTestDestinations []Alias
 
-// UnmarshalJSON walks the JSON array, dispatching each element through
-// AliasEnc so trimming and prefix detection match the rest of the parser.
 func (d *SSHTestDestinations) UnmarshalJSON(b []byte) error {
 	var aliases []AliasEnc
 
@@ -117,13 +99,9 @@ func (d *SSHTestDestinations) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
-// UnmarshalJSON drives the typed shape of SSHPolicyTest. The wire format
-// is unchanged: src is a JSON string parsed through parseAlias; dst is an
-// array of strings handled by SSHTestDestinations; accept/deny/check are
-// arrays of strings handled per element by SSHUser.UnmarshalJSON. An
-// empty src string lands as a nil Alias so the empty-src case stays a
-// validation-time error with the SaaS-aligned ErrSSHTestEmptySrc body
-// rather than a raw parser failure.
+// UnmarshalJSON parses each typed field. An empty src lands as a nil
+// Alias so validation surfaces ErrSSHTestEmptySrc rather than a parser
+// failure.
 func (t *SSHPolicyTest) UnmarshalJSON(b []byte) error {
 	var raw struct {
 		Src    string              `json:"src"`

hscontrol/policy/v2/types.go

@@ -1414,11 +1414,9 @@ func (g *Groups) UnmarshalJSON(b []byte) error {
 		}
 	}
 
-	// SaaS rejects any group-in-group reference (cycle, chain,
-	// self-cycle) with `groups["X"]: "Y": group members cannot be
-	// recursive`. Iterate keys in descending alphabetical order so
-	// the reported (X, Y) pair matches the SaaS engine, which
-	// reports the deepest non-leaf parent first.
+	// Reject group-in-group references. Reverse-sort the keys so the
+	// reported (parent, child) pair names the deepest non-leaf parent
+	// first.
 	keys := make([]string, 0, len(rawGroups))
 	for k := range rawGroups {
 		keys = append(keys, k)
@@ -1698,15 +1696,9 @@ func (a *SSHAction) String() string {
 	return string(*a)
 }
 
-// UnmarshalJSON implements JSON unmarshaling for SSHAction.
-//
-// Empty strings are accepted at parse time; the per-rule validate()
-// pass surfaces them with `action must be specified` to match SaaS.
-// Non-empty unknown values fail here with `"foo" is not a valid action`.
-//
-// SaaS trims surrounding whitespace before comparing, then complains
-// about the trimmed content; the resulting error quotes the trimmed
-// value (e.g. `" Accept "` → `"Accept" is not a valid action`).
+// UnmarshalJSON trims surrounding whitespace before matching, lets the
+// empty string through (per-rule Validate() surfaces it later), and
+// rejects every other unknown value here.
 func (a *SSHAction) UnmarshalJSON(b []byte) error {
 	str := strings.TrimSpace(strings.Trim(string(b), `"`))
 	switch str {
@@ -2529,34 +2521,26 @@ func (p *Policy) validate() error {
 	}
 
 	for _, ssh := range p.SSHs {
-		// SaaS rejects empty/missing `action` with `action must be
-		// specified`; an empty SSHAction survives parse intentionally
-		// so this validate pass surfaces the SaaS-aligned wording.
+		// Empty action and users survive parse; surface them here.
 		if ssh.Action == "" {
 			errs = append(errs, ErrSSHActionMustBeSpecified)
 		}
 
-		// SaaS rejects empty/missing `users` with `users must be
-		// specified`; non-canonical user strings (autogroup:*, group:,
-		// tag:, malformed localpart:) are accepted and flow through to
-		// compileSSHPolicy as literals — matching SaaS compile output.
 		if len(ssh.Users) == 0 {
 			errs = append(errs, ErrSSHUsersMustBeSpecified)
 		}
 
+		// "" and "*" are not valid login users; any other string
+		// (including autogroup, group, tag, malformed localpart) is
+		// treated as a literal user name.
 		for _, user := range ssh.Users {
-			// SaaS rejects `""` and `"*"` as user logins; everything
-			// else (including autogroup:*, group:, tag:, malformed
-			// localpart:) is accepted and treated as a literal.
 			switch user {
 			case "", "*":
 				errs = append(errs, fmt.Errorf("user %q %w", user, ErrSSHUserInvalid))
 			}
 		}
 
-		// SaaS rejects empty entries in `acceptEnv` with
-		// `acceptEnv values cannot be empty`. The wildcard `*` and
-		// double-glob `**` are accepted (only empty string is invalid).
+		// acceptEnv entries cannot be empty; "*" and "**" are valid.
 		for _, env := range ssh.AcceptEnv {
 			if env == "" {
 				errs = append(errs, ErrSSHAcceptEnvEmpty)
@@ -2620,11 +2604,8 @@ func (p *Policy) validate() error {
 					errs = append(errs, err)
 				}
 			case *Host:
-				// SaaS rejects every hosts-table alias on an SSH
-				// dst with `invalid dst "alias"`, whether the
-				// alias resolves to a single IP or a CIDR. The
-				// equivalent ACL rule accepts the same aliases,
-				// so reject here rather than at parse time.
+				// Hosts-table aliases are valid on ACL dst but
+				// rejected here for SSH dst.
 				errs = append(errs, fmt.Errorf("%w %q", ErrSSHDestinationHostAlias, string(*dst)))
 			}
 		}
@@ -2971,22 +2952,18 @@ func (p SSHCheckPeriod) MarshalJSON() ([]byte, error) {
 	return fmt.Appendf(nil, "%q", p.Duration.String()), nil
 }
 
-// Validate checks that the SSHCheckPeriod is within allowed bounds.
-// SaaS rejects negative durations with `must be a positive duration`
-// and anything above 168h with `is above the max (168h)`; the 168h
-// upper bound is inclusive.
+// Validate rejects negative durations and anything above the inclusive
+// 168h max.
 func (p *SSHCheckPeriod) Validate() error {
 	if p.Always {
 		return nil
 	}
 
 	if p.Duration < 0 {
-		// SaaS body: `checkPeriod -1m0s must be a positive duration`.
 		return fmt.Errorf("checkPeriod %s %w", p.Duration, ErrSSHCheckPeriodNegative)
 	}
 
 	if p.Duration > SSHCheckPeriodMax {
-		// SaaS body: `checkPeriod 200h0m0s is above the max (168h)`.
 		return fmt.Errorf("checkPeriod %s %w", p.Duration, ErrSSHCheckPeriodAboveMax)
 	}
 
@@ -3166,20 +3143,18 @@ func (u SSHUsers) ContainsNonRoot() bool {
 }
 
 // ContainsLocalpart returns true if any entry is a canonical
-// `localpart:*@<domain>` form. Non-canonical strings that merely start
-// with `localpart:` (e.g. `localpart:`, `localpart:foo`) are treated as
-// literal user names per SaaS behaviour.
+// `localpart:*@<domain>` form. Non-canonical strings starting with
+// `localpart:` are treated as literal usernames.
 func (u SSHUsers) ContainsLocalpart() bool {
 	return slices.ContainsFunc(u, func(user SSHUser) bool {
 		return user.IsCanonicalLocalpart()
 	})
 }
 
-// NormalUsers returns SSH users handled by the literal user map: every
-// entry except root, autogroup:nonroot, and canonical
-// `localpart:*@<domain>`. Malformed `localpart:` strings flow through
-// here so they end up in the compiled SSHUsers map literally — matching
-// SaaS, which also keeps them verbatim.
+// NormalUsers returns users that land in the compiled literal user map
+// (everything except root, autogroup:nonroot, and canonical
+// `localpart:*@<domain>`). Malformed `localpart:` strings stay here as
+// literals.
 func (u SSHUsers) NormalUsers() []SSHUser {
 	return slicesx.Filter(nil, u, func(user SSHUser) bool {
 		return user != "root" && user != SSHUser(AutoGroupNonRoot) && !user.IsCanonicalLocalpart()
@@ -3255,12 +3230,9 @@ func (u SSHUser) MarshalJSON() ([]byte, error) {
 	return json.Marshal(string(u))
 }
 
-// UnmarshalJSON trims surrounding whitespace per element so a policy
-// like `"users": [" root"]` stores `"root"` and compiles to the same
-// `sshUsers: {"root": "root"}` map SaaS produces. A whitespace-only
-// entry like `[" "]` collapses to `""` and falls through to the
-// per-rule validate() pass, which surfaces the SaaS-aligned
-// `user "" is not valid`.
+// UnmarshalJSON trims surrounding whitespace per element. A whitespace-
+// only entry collapses to `""` and surfaces as `user "" is not valid` in
+// the per-rule Validate() pass.
 func (u *SSHUser) UnmarshalJSON(b []byte) error {
 	var s string
 	if err := json.Unmarshal(b, &s); err != nil { //nolint:noinlineerr
@@ -3299,14 +3271,13 @@ func unmarshalPolicy(b []byte) (*Policy, error) {
 			}
 
 			// Non-tag entries in grant.via surface as type errors on
-			// []Tag; match SaaS wording instead of Go's JSON diagnostic.
+			// []Tag; rephrase to the wire-compatible body.
 			if strings.Contains(string(serr.JSONPointer), "/via/") {
 				return nil, ErrGrantViaNotATag
 			}
 
 			// Non-ASCII tag-name failures surface from Tag.Validate
-			// at unmarshal time. Reshape the error to mirror SaaS
-			// (`tagOwners["tag:X"]: …`).
+			// at unmarshal time. Reshape to `tagOwners["tag:X"]: …`.
 			if errors.Is(serr.Err, ErrTagNameMustStartWithLetter) {
 				ptr := serr.JSONPointer
 				name := ptr.LastToken()
@@ -3390,16 +3361,15 @@ func validateTests(pol *Policy, tests []PolicyTest) error {
 	return nil
 }
 
-// validateTestDestination enforces that a tests-block dst describes one
-// connection attempt to one specific host on one specific port. SaaS
-// rejects three shapes that violate the rule: autogroup:internet (routed
-// by exit-node AllowedIPs, not the packet filter); multi-port
-// (range/list/wildcard, no single allow/deny answer); and CIDR ranges
-// — both raw `/N` syntax and `hosts:`-table aliases whose RHS is a
-// multi-host prefix. Bare IP literals reach this function as *Prefix
-// /32 or /128 just like explicit `/32` / `/128` does, so the CIDR
-// check inspects the raw input string for `/` rather than the parsed
-// alias type.
+// validateTestDestination rejects tests-block dst shapes that cannot
+// describe one connection to one host on one port:
+//
+//   - autogroup:internet (routed by exit-node AllowedIPs),
+//   - multi-port ranges, lists, or wildcards,
+//   - CIDR ranges (raw `/N` or hosts: aliases that resolve wider).
+//
+// Bare IPs parse as /32 or /128 like explicit forms, so the CIDR
+// check inspects the raw input rather than the parsed alias type.
 func validateTestDestination(pol *Policy, dst string) error {
 	awp, err := parseDestinationAlias(dst)
 	if err != nil {
@@ -3430,15 +3400,10 @@ func validateTestDestination(pol *Policy, dst string) error {
 	return nil
 }
 
-// validateSSHTests enforces the parse-time shape rules an sshTests entry
-// must satisfy: a non-empty src alias, at least one dst, and a dst list
-// whose entries each describe a single SSH-reachable host. Login-user
-// assertions (accept/deny/check) are not validated here — SaaS reports
-// empty assertion arrays and empty user strings through the same
-// "test(s) failed" body it uses for true evaluation failures, so those
-// cases stay with the engine. Both the parse-time errors and the
-// engine-time failures share the errSSHPolicyTestsFailed wrapper so
-// callers see one consistent body.
+// validateSSHTests enforces parse-time shape on every sshTests entry:
+// non-empty src, at least one dst, and each dst describing a single
+// SSH-reachable host. Login-user assertions land with the engine so
+// failures surface through the same errSSHPolicyTestsFailed wrapper.
 func validateSSHTests(pol *Policy, tests []SSHPolicyTest) error {
 	var errs []error
 
@@ -3466,44 +3431,38 @@ func validateSSHTests(pol *Policy, tests []SSHPolicyTest) error {
 	return nil
 }
 
-// validateSSHTestDestination enforces that an sshTests dst entry names a
-// single SSH-reachable host. Tailscale SaaS rejects three shapes at parse
-// time: a `:port` suffix (read by the parser as an unknown tag, hence the
-// "unknown tag" error wording); a multi-host CIDR (raw `/N` narrower than
-// the address width, or a `hosts:` entry whose RHS is a multi-host prefix);
-// and autogroup:internet (only valid in ACL destinations, not SSH ones).
-// A bare IP literal — which parses to a `/BitLen` prefix — names a single
-// host and is accepted. Tag entries must reference a tag that exists in
-// tagOwners; bare hosts must resolve to a single-address prefix.
+// validateSSHTestDestination rejects sshTests dst shapes that cannot
+// name a single SSH-reachable host:
+//
+//   - `host:port` suffixes (parsed as an unknown tag),
+//   - multi-host CIDRs (raw `/N` or a hosts: entry resolving wider),
+//   - autogroup:internet (valid as ACL dst only).
+//
+// A bare IP literal (single-host /BitLen prefix) is accepted. Tag
+// entries must exist in tagOwners.
 func validateSSHTestDestination(pol *Policy, alias Alias) error {
 	dst := alias.String()
 
 	switch a := alias.(type) {
 	case *AutoGroup:
-		// autogroup:internet is the only autogroup SaaS rejects at parse.
-		// Other autogroups (member, tagged, self, nonroot) are valid SSH
-		// dst aliases and pass through to engine evaluation.
+		// autogroup:internet is the only autogroup not valid here;
+		// member/tagged/self/nonroot pass to engine evaluation.
 		if *a == AutoGroupInternet {
 			return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst)
 		}
 
 	case *Prefix:
-		// SaaS accepts a bare IP (parsed to a `/BitLen` prefix) as a
-		// single SSH-reachable host but rejects a narrower CIDR like
-		// `10.0.0.0/24`. Distinguish the two by mask width: a prefix
-		// whose Bits() matches the address BitLen() is a single host
-		// and passes; anything narrower is a multi-host range and is
-		// rejected the same way as raw `/N`.
+		// A bare IP parses as `/BitLen` and is a valid single-host dst;
+		// any narrower CIDR is a multi-host range and is rejected.
 		p := netip.Prefix(*a)
 		if p.Bits() < p.Addr().BitLen() {
 			return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst)
 		}
 
 	case *Tag:
-		// A tag must be declared in tagOwners. The `tag:server:22` shape
-		// reaches this branch because isTag only checks the `tag:` prefix
-		// — the colon-port suffix is preserved in the Tag string and the
-		// tagOwners lookup misses, reproducing the SaaS error wording.
+		// A tag must be declared in tagOwners. `tag:server:22` lands
+		// here too because isTag only checks the prefix, so the lookup
+		// misses and the colon-port suffix surfaces as unknown-tag.
 		if pol == nil {
 			return fmt.Errorf("%w %q", ErrSSHTestDstUnknownTag, string(*a))
 		}
@@ -3514,8 +3473,8 @@ func validateSSHTestDestination(pol *Policy, alias Alias) error {
 		}
 
 	case *Host:
-		// A hosts: entry that resolves to a multi-host prefix is a CIDR
-		// in disguise — reject it the same way as raw `/N`.
+		// A hosts: alias that resolves to multiple addresses is a CIDR
+		// in disguise.
 		if pol == nil {
 			return nil
 		}

integration/cli_policy_test.go

@@ -166,14 +166,10 @@ func TestPolicyCheckCommand(t *testing.T) {
 	}
 }
 
-// TestSSHTestsRejectFailingPolicy asserts that `headscale policy set`
-// rejects a policy whose sshTests evaluate to a failure, surfaces the
-// engine's "test(s) failed" sentinel on stderr, and leaves the previously
-// stored policy untouched. The good policy admits user1@ → autogroup:member
-// as root; the bad policy reuses the same SSH rule but asserts user2@ can
-// SSH, which the rule denies. autogroup:member as dst means every
-// scenario node is a member, so dst resolution finds real nodes without
-// requiring a separately-tagged node.
+// TestSSHTestsRejectFailingPolicy asserts `headscale policy set` rejects
+// a policy whose sshTests fail, surfaces the engine's "test(s) failed"
+// sentinel, and leaves the stored policy unchanged. autogroup:member as
+// dst lets every scenario node count, so no tagged node is needed.
 func TestSSHTestsRejectFailingPolicy(t *testing.T) {
 	IntegrationSkip(t)
 
@@ -182,9 +178,7 @@ func TestSSHTestsRejectFailingPolicy(t *testing.T) {
 		user2 = "user2@"
 	)
 
-	// Good policy: SSH rule and sshTests agree — user1@ may SSH as root
-	// to any autogroup:member node, and the sshTests entry asserts exactly
-	// that.
+	// Good policy: user1@ may SSH as root, and the sshTests asserts it.
 	goodPolicy := policyv2.Policy{
 		SSHs: []policyv2.SSH{
 			{
@@ -205,11 +199,8 @@ func TestSSHTestsRejectFailingPolicy(t *testing.T) {
 		},
 	}
 
-	// Bad policy: same SSH rule, but the sshTests block asserts that
-	// user2@ can SSH as root to autogroup:member. The rule only admits
-	// user1@, so the assertion must fail and the write must be rejected.
-	// SSHTests is a slice (reference type), so reassigning the field
-	// rather than mutating in place preserves goodPolicy.SSHTests.
+	// Bad policy: same SSH rule, but the sshTests asserts user2@ — who
+	// the rule does not admit — can SSH. Must be rejected.
 	badPolicy := goodPolicy
 	badPolicy.SSHTests = []policyv2.SSHPolicyTest{
 		{