diff --git a/CHANGELOG.md b/CHANGELOG.md index 79f67b07..9bb65002 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index fdb8ca0d..f24ce5ff 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/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[] == 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[] == for every named user. func sshUserMapAllows(m map[string]string, user string) bool { if user == "" { return false diff --git a/hscontrol/policy/v2/sshtester_compat_test.go b/hscontrol/policy/v2/sshtester_compat_test.go index 747e7092..568de951 100644 --- a/hscontrol/policy/v2/sshtester_compat_test.go +++ b/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) diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 6b27f068..8e0f906a 100644 --- a/hscontrol/policy/v2/test.go +++ b/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"` diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 8051ccd4..da088b59 100644 --- a/hscontrol/policy/v2/types.go +++ b/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:*@` form. Non-canonical strings that merely start -// with `localpart:` (e.g. `localpart:`, `localpart:foo`) are treated as -// literal user names per SaaS behaviour. +// `localpart:*@` 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:*@`. 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:*@`). 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 } diff --git a/integration/cli_policy_test.go b/integration/cli_policy_test.go index 4ec3a33e..9773b895 100644 --- a/integration/cli_policy_test.go +++ b/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{ {