From 2180380fc1974db078bb93229c96d5794fba3ed9 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 13 May 2026 08:17:01 +0000 Subject: [PATCH] policy/v2: align SSH rule validation with Tailscale SaaS Drop autogroup/localpart strictness on SSH users. Drop checkPeriod minimum. Reject empty/wildcard users and empty acceptEnv at parse. Match SaaS wording for action and checkPeriod errors. 28 captures under testdata/ssh_results/ssh-malformed-*.hujson lock the surface. --- hscontrol/policy/policy_test.go | 30 ++- hscontrol/policy/v2/sshtester_compat_test.go | 9 +- .../v2/tailscale_ssh_data_compat_test.go | 63 ------ hscontrol/policy/v2/types.go | 139 +++++++----- hscontrol/policy/v2/types_test.go | 212 +++++++++++++++--- 5 files changed, 285 insertions(+), 168 deletions(-) diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index ad7dc162..40895977 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -1317,7 +1317,7 @@ func TestSSHPolicyRules(t *testing.T) { ] }`, expectErr: true, - errorMessage: `invalid SSH action: "invalid", must be one of: accept, check`, + errorMessage: `"invalid" is not a valid action`, }, { name: "invalid-check-period", @@ -1341,10 +1341,15 @@ func TestSSHPolicyRules(t *testing.T) { ] }`, expectErr: true, - errorMessage: "not a valid duration string", + errorMessage: `time: invalid duration "invalid"`, }, + // `autogroup:invalid` as an SSH user is no longer rejected: + // SaaS treats every `autogroup:*` user-string as a literal + // label and compiles it into the SSHUsers map. The compat + // suite covers this via ssh-malformed-user-autogroup-* — no + // dedicated case is needed here. { - name: "unsupported-autogroup", + name: "ssh-user-unknown-autogroup-as-literal", targetNode: taggedClient, peers: types.Nodes{&nodeUser2}, policy: `{ @@ -1363,8 +1368,23 @@ func TestSSHPolicyRules(t *testing.T) { } ] }`, - expectErr: true, - errorMessage: "autogroup not supported for SSH user", + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.2"}, + }, + SSHUsers: map[string]string{ + "autogroup:invalid": "autogroup:invalid", + "root": "", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + AllowRemotePortForwarding: true, + }, + }, + }}, }, { name: "autogroup-nonroot-should-use-wildcard-with-root-excluded", diff --git a/hscontrol/policy/v2/sshtester_compat_test.go b/hscontrol/policy/v2/sshtester_compat_test.go index 63558a04..f545844c 100644 --- a/hscontrol/policy/v2/sshtester_compat_test.go +++ b/hscontrol/policy/v2/sshtester_compat_test.go @@ -31,14 +31,7 @@ import ( // knownSSHTesterDivergences tracks scenarios where headscale and SaaS // 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 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 - // hscontrol/policy/v2/types.go — reject `*` as an SSH login user - // during SSH-rule validation. - "sshtest-user-wildcard": "headscale accepts `users: [\"*\"]` on ssh rules; SaaS rejects with `user \"*\" is not valid`", -} +var knownSSHTesterDivergences = map[string]string{} func TestSSHTesterCompat(t *testing.T) { t.Parallel() diff --git a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go index f58be9b0..111e9e1c 100644 --- a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go @@ -87,75 +87,12 @@ var sshSkipReasons = map[string]string{ // equivalent for the user:*@passkey wildcard pattern. "ssh-b5": "user:*@passkey wildcard not supported in headscale", "ssh-d10": "user:*@passkey wildcard not supported in headscale", - - // AUTOGROUP_AS_SSH_USER (4 tests) - // - // SaaS accepts `users: ["autogroup:X"]` on an `ssh` rule for - // every autogroup name (member, self, tagged, internet); the - // resolution then produces zero principals at compile time for - // the ones that don't map to actual logins. headscale's - // validate() restricts users-side autogroups to just - // `autogroup:nonroot` via autogroupForSSHUser, so the policy - // fails to parse. Either widen autogroupForSSHUser to mirror - // SaaS (and let compilation drop unsupported entries) or - // document that headscale is intentionally stricter. - "ssh-malformed-user-autogroup-internet": "headscale rejects `users: [autogroup:internet]`; SaaS accepts and compiles to zero principals", - "ssh-malformed-user-autogroup-member": "headscale rejects `users: [autogroup:member]`; SaaS accepts and compiles to zero principals", - "ssh-malformed-user-autogroup-self": "headscale rejects `users: [autogroup:self]`; SaaS accepts and compiles to zero principals", - "ssh-malformed-user-autogroup-tagged": "headscale rejects `users: [autogroup:tagged]`; SaaS accepts and compiles to zero principals", - - // LOCALPART_SHAPE (4 tests) - // - // SaaS accepts every shape under the `localpart:` prefix - // (`localpart:`, `localpart:foo`, `localpart:*@`, and - // `localpart:foo@example.com`) and compiles each to zero - // principals. headscale's SSHUser.ParseLocalpart enforces the - // strict `localpart:*@` shape and returns errors for - // every other form — see hscontrol/policy/v2/types.go - // ParseLocalpart and the call site in validate(). The strict - // path is arguably correct (these strings can never produce a - // useful principal) but it surfaces as a parse error rather - // than the SaaS behaviour of silently producing zero - // principals. - "ssh-malformed-user-localpart-empty": "headscale rejects `localpart:` (missing @); SaaS accepts and compiles to zero principals", - "ssh-malformed-user-localpart-no-at": "headscale rejects `localpart:foo` (missing @); SaaS accepts and compiles to zero principals", - "ssh-malformed-user-localpart-no-domain": "headscale rejects `localpart:*@` (empty domain); SaaS accepts and compiles to zero principals", - "ssh-malformed-user-localpart-no-glob": "headscale rejects `localpart:foo@example.com` (local part not *); SaaS accepts and compiles to zero principals", - - // CHECK_PERIOD_MIN (2 tests) - // - // SaaS allows `checkPeriod: "0s"` and any sub-minute value; - // headscale's SSHCheckPeriodMin = 1 minute rejects both. The - // minimum may be a deliberate hardening, but it diverges from - // SaaS — needs an explicit decision. - "ssh-malformed-checkperiod-zero": "headscale rejects checkPeriod `0s`; SaaS accepts", - "ssh-malformed-checkperiod-too-short": "headscale rejects checkPeriod `30s`; SaaS accepts (no minimum)", } // sshRejectSkipReasons documents APIResponseCode != 200 captures where // headscale and SaaS legitimately disagree on whether the policy should // be rejected (or where headscale rejects with different wording). var sshRejectSkipReasons = map[string]string{ - // WORDING_DIFFERS (4 tests) - // - // headscale rejects these inputs but with different error - // text, so the SaaS message is not a substring of headscale's. - "ssh-malformed-action-deny": `headscale rejects "deny" with 'invalid SSH action: "deny", must be one of: accept, check' vs SaaS '"deny" is not a valid action'`, - "ssh-malformed-action-empty": `headscale rejects empty action with 'invalid SSH action: "", must be one of: accept, check' vs SaaS 'action must be specified'`, - "ssh-malformed-checkperiod-malformed": `headscale rejects malformed duration with 'not a valid duration string: "abc"' vs SaaS 'time: invalid duration "abc"'`, - "ssh-malformed-checkperiod-too-long": "headscale rejects 200h with 'checkPeriod above maximum of 168 hours (1 week)' vs SaaS 'checkPeriod 200h0m0s is above the max (168h)'", - - // MISSING_VALIDATIONS (5 tests) - // - // SaaS rejects these inputs; headscale accepts them today. - // Fixes belong in hscontrol/policy/v2/types.go validate(). - "ssh-malformed-action-missing": "headscale accepts missing `action`; SaaS rejects with `action must be specified`", - "ssh-malformed-acceptenv-empty": "headscale accepts `acceptEnv: [\"\"]`; SaaS rejects with `acceptEnv values cannot be empty`", - "ssh-malformed-users-empty-array": "headscale accepts empty `users: []`; SaaS rejects with `users must be specified`", - "ssh-malformed-users-missing": "headscale accepts missing `users`; SaaS rejects with `users must be specified`", - "ssh-malformed-user-empty": `headscale accepts empty username ""; SaaS rejects with 'user "" is not valid'`, - "ssh-malformed-user-wildcard": `headscale accepts wildcard "*"; SaaS rejects with 'user "*" is not valid' (same gap as sshtester_compat_test.go's sshtest-user-wildcard)`, - // DOMAIN_NOT_ASSOCIATED (5 tests) // // SaaS validates that email domains in user:*@domain and diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 499e42eb..fca65e7a 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -14,7 +14,6 @@ import ( "github.com/go-json-experiment/json" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" - "github.com/prometheus/common/model" "github.com/tailscale/hujson" "go4.org/netipx" "tailscale.com/net/tsaddr" @@ -46,17 +45,21 @@ var ( ErrSSHAutogroupSelfRequiresUserSource = errors.New("autogroup:self destination requires source to contain only users or groups, not tags or autogroup:tagged") ErrSSHTagSourceToAutogroupMember = errors.New("tags in SSH source cannot access autogroup:member (user-owned devices)") ErrSSHWildcardDestination = errors.New("wildcard (*) is not supported as SSH destination") - ErrSSHCheckPeriodBelowMin = errors.New("checkPeriod below minimum of 1 minute") - ErrSSHCheckPeriodAboveMax = errors.New("checkPeriod above maximum of 168 hours (1 week)") + ErrSSHCheckPeriodAboveMax = errors.New("is above the max (168h)") ErrSSHCheckPeriodOnNonCheck = errors.New("checkPeriod is only valid with action \"check\"") ErrInvalidLocalpart = errors.New("invalid localpart format, must be localpart:*@") + ErrSSHUsersMustBeSpecified = errors.New("users must be specified") + ErrSSHUserInvalid = errors.New("is not valid") + ErrSSHAcceptEnvEmpty = errors.New("acceptEnv values cannot be empty") + ErrSSHActionMustBeSpecified = errors.New("action must be specified") + ErrSSHActionInvalid = errors.New("is not a valid action") ) // SSH check period constants per Tailscale docs: // https://tailscale.com/docs/features/tailscale-ssh#checkperiod +// SaaS imposes no minimum (0s is accepted) so headscale matches. const ( SSHCheckPeriodDefault = 12 * time.Hour - SSHCheckPeriodMin = time.Minute SSHCheckPeriodMax = 168 * time.Hour ) @@ -137,7 +140,6 @@ var ( ErrAutogroupDangerAllDst = errors.New("cannot use autogroup:danger-all as a dst") ErrAutogroupNotSupportedSSHSrc = errors.New("autogroup not supported for SSH sources") ErrAutogroupNotSupportedSSHDst = errors.New("autogroup not supported for SSH destinations") - ErrAutogroupNotSupportedSSHUsr = errors.New("autogroup not supported for SSH user") ErrHostNotDefined = errors.New("host not defined in policy") ErrSSHSourceAliasNotSupported = errors.New("alias not supported for SSH source") ErrSSHDestAliasNotSupported = errors.New("alias not supported for SSH destination") @@ -1651,15 +1653,21 @@ func (a *SSHAction) String() string { } // 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`. func (a *SSHAction) UnmarshalJSON(b []byte) error { str := strings.Trim(string(b), `"`) switch str { + case "": + *a = SSHAction("") case "accept": *a = SSHActionAccept case "check": *a = SSHActionCheck default: - return fmt.Errorf("%w: %q, must be one of: accept, check", ErrInvalidSSHAction, str) + return fmt.Errorf("%q %w", str, ErrSSHActionInvalid) } return nil @@ -2057,7 +2065,6 @@ var ( autogroupForDst = []AutoGroup{AutoGroupInternet, AutoGroupMember, AutoGroupTagged, AutoGroupSelf} autogroupForSSHSrc = []AutoGroup{AutoGroupMember, AutoGroupTagged} autogroupForSSHDst = []AutoGroup{AutoGroupMember, AutoGroupTagged, AutoGroupSelf} - autogroupForSSHUser = []AutoGroup{AutoGroupNonRoot} autogroupForNodeAttrs = []AutoGroup{AutoGroupMember, AutoGroupTagged} autogroupNotSupported = []AutoGroup{} @@ -2197,18 +2204,6 @@ func validateAutogroupForSSHDst(dst *AutoGroup) error { return nil } -func validateAutogroupForSSHUser(user *AutoGroup) error { - if user == nil { - return nil - } - - if !slices.Contains(autogroupForSSHUser, *user) { - return fmt.Errorf("%w: %q, can be %v", ErrAutogroupNotSupportedSSHUsr, *user, autogroupForSSHUser) - } - - return nil -} - // validateSSHSrcDstCombination validates that SSH source/destination combinations // follow Tailscale's security model: // - Destination can be: tags, autogroup:self (if source is users/groups), or same-user @@ -2484,23 +2479,37 @@ 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. + 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) + } + for _, user := range ssh.Users { - if strings.HasPrefix(string(user), "autogroup:") { - maybeAuto := AutoGroup(user) - - err := validateAutogroupForSSHUser(&maybeAuto) - if err != nil { - errs = append(errs, err) - continue - } + // 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)) } + } - if user.IsLocalpart() { - _, err := user.ParseLocalpart() - if err != nil { - errs = append(errs, err) - continue - } + // SaaS rejects empty entries in `acceptEnv` with + // `acceptEnv values cannot be empty`. The wildcard `*` and + // double-glob `**` are accepted (only empty string is invalid). + for _, env := range ssh.AcceptEnv { + if env == "" { + errs = append(errs, ErrSSHAcceptEnvEmpty) } } @@ -2886,12 +2895,16 @@ func (p *SSHCheckPeriod) UnmarshalJSON(b []byte) error { return nil } - d, err := model.ParseDuration(str) + // time.ParseDuration produces error strings like + // `time: invalid duration "abc"` which match SaaS body wording + // exactly; model.ParseDuration wraps the same parse with custom + // phrasing and would diverge. + d, err := time.ParseDuration(str) if err != nil { - return fmt.Errorf("parsing checkPeriod %q: %w", str, err) + return err } - p.Duration = time.Duration(d) + p.Duration = d return nil } @@ -2906,25 +2919,15 @@ func (p SSHCheckPeriod) MarshalJSON() ([]byte, error) { } // Validate checks that the SSHCheckPeriod is within allowed bounds. +// SaaS imposes no minimum; the only ceiling is 168h. func (p *SSHCheckPeriod) Validate() error { if p.Always { return nil } - if p.Duration < SSHCheckPeriodMin { - return fmt.Errorf( - "%w: got %s", - ErrSSHCheckPeriodBelowMin, - p.Duration, - ) - } - if p.Duration > SSHCheckPeriodMax { - return fmt.Errorf( - "%w: got %s", - ErrSSHCheckPeriodAboveMax, - p.Duration, - ) + // SaaS body: `checkPeriod 200h0m0s is above the max (168h)`. + return fmt.Errorf("checkPeriod %s %w", p.Duration, ErrSSHCheckPeriodAboveMax) } return nil @@ -3102,25 +3105,33 @@ func (u SSHUsers) ContainsNonRoot() bool { return slices.Contains(u, SSHUser(AutoGroupNonRoot)) } -// ContainsLocalpart returns true if any entry has the localpart: prefix. +// 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. func (u SSHUsers) ContainsLocalpart() bool { return slices.ContainsFunc(u, func(user SSHUser) bool { - return user.IsLocalpart() + return user.IsCanonicalLocalpart() }) } -// NormalUsers returns all SSH users that are not root, autogroup:nonroot, -// or localpart: entries. +// 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. func (u SSHUsers) NormalUsers() []SSHUser { return slicesx.Filter(nil, u, func(user SSHUser) bool { - return user != "root" && user != SSHUser(AutoGroupNonRoot) && !user.IsLocalpart() + return user != "root" && user != SSHUser(AutoGroupNonRoot) && !user.IsCanonicalLocalpart() }) } -// LocalpartEntries returns only the localpart: prefixed entries. +// LocalpartEntries returns only canonical `localpart:*@` entries. +// Non-canonical localpart strings are excluded so they do not trigger +// the resolution path; they are emitted literally by NormalUsers. func (u SSHUsers) LocalpartEntries() []SSHUser { return slicesx.Filter(nil, u, func(user SSHUser) bool { - return user.IsLocalpart() + return user.IsCanonicalLocalpart() }) } @@ -3130,11 +3141,25 @@ func (u SSHUser) String() string { return string(u) } -// IsLocalpart returns true if the SSHUser has the localpart: prefix. +// IsLocalpart returns true if the SSHUser has the literal `localpart:` +// prefix. It is a syntactic check only — non-canonical shapes still +// pass. func (u SSHUser) IsLocalpart() bool { return strings.HasPrefix(string(u), SSHUserLocalpartPrefix) } +// IsCanonicalLocalpart reports whether the SSHUser parses as the +// canonical `localpart:*@` form that resolution acts on. +func (u SSHUser) IsCanonicalLocalpart() bool { + if !u.IsLocalpart() { + return false + } + + _, err := u.ParseLocalpart() + + return err == nil +} + // ParseLocalpart validates and extracts the domain from a localpart: entry. // The expected format is localpart:*@. // Returns the domain part or an error if the format is invalid. diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 93b286e5..f5d2269c 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -660,7 +660,7 @@ func TestUnmarshalPolicy(t *testing.T) { }, }, { - name: "ssh-with-tag-and-user", + name: "ssh-with-tag-and-wildcard-user", input: ` { "tagOwners": { @@ -681,26 +681,7 @@ func TestUnmarshalPolicy(t *testing.T) { ] } `, - want: &Policy{ - TagOwners: TagOwners{ - Tag("tag:web"): Owners{new(Username("admin@example.com"))}, - Tag("tag:server"): Owners{new(Username("admin@example.com"))}, - }, - SSHs: []SSH{ - { - Action: "accept", - Sources: SSHSrcAliases{ - tp("tag:web"), - }, - Destinations: SSHDstAliases{ - tp("tag:server"), - }, - Users: []SSHUser{ - SSHUser("*"), - }, - }, - }, - }, + wantErr: `user "*" is not valid`, }, { name: "ssh-with-check-period", @@ -2006,8 +1987,11 @@ func TestUnmarshalPolicy(t *testing.T) { `, wantErr: "square brackets are only valid around IPv6 addresses", }, + // Non-canonical `localpart:` strings flow through as literal + // user names per SaaS behaviour — captured in + // ssh-malformed-user-localpart-{no-at,no-glob,no-domain}. { - name: "ssh-localpart-invalid-no-at-sign", + name: "ssh-localpart-non-canonical-no-at-sign", input: ` { "tagOwners": {"tag:prod": ["admin@"]}, @@ -2019,10 +2003,22 @@ func TestUnmarshalPolicy(t *testing.T) { }] } `, - wantErr: "invalid localpart format", + want: &Policy{ + TagOwners: TagOwners{ + Tag("tag:prod"): Owners{new(Username("admin@"))}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{agp("autogroup:member")}, + Destinations: SSHDstAliases{tp("tag:prod")}, + Users: []SSHUser{SSHUser("localpart:foo")}, + }, + }, + }, }, { - name: "ssh-localpart-invalid-non-wildcard", + name: "ssh-localpart-non-canonical-non-wildcard", input: ` { "tagOwners": {"tag:prod": ["admin@"]}, @@ -2034,10 +2030,22 @@ func TestUnmarshalPolicy(t *testing.T) { }] } `, - wantErr: "invalid localpart format", + want: &Policy{ + TagOwners: TagOwners{ + Tag("tag:prod"): Owners{new(Username("admin@"))}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{agp("autogroup:member")}, + Destinations: SSHDstAliases{tp("tag:prod")}, + Users: []SSHUser{SSHUser("localpart:alice@example.com")}, + }, + }, + }, }, { - name: "ssh-localpart-invalid-empty-domain", + name: "ssh-localpart-non-canonical-empty-domain", input: ` { "tagOwners": {"tag:prod": ["admin@"]}, @@ -2049,7 +2057,19 @@ func TestUnmarshalPolicy(t *testing.T) { }] } `, - wantErr: "invalid localpart format", + want: &Policy{ + TagOwners: TagOwners{ + Tag("tag:prod"): Owners{new(Username("admin@"))}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{agp("autogroup:member")}, + Destinations: SSHDstAliases{tp("tag:prod")}, + Users: []SSHUser{SSHUser("localpart:*@")}, + }, + }, + }, }, // A test entry with neither accept nor deny asserts nothing // and is silently accepted today. Tailscale rejects the policy. @@ -4364,18 +4384,21 @@ func TestSSHCheckPeriodValidate(t *testing.T) { period: SSHCheckPeriod{Always: true}, }, { - name: "1m minimum valid", + name: "zero duration is valid", + period: SSHCheckPeriod{Duration: 0}, + }, + { + name: "30s below previous minimum is valid (matches SaaS)", + period: SSHCheckPeriod{Duration: 30 * time.Second}, + }, + { + name: "1m valid", period: SSHCheckPeriod{Duration: time.Minute}, }, { name: "168h maximum valid", period: SSHCheckPeriod{Duration: 168 * time.Hour}, }, - { - name: "30s below minimum", - period: SSHCheckPeriod{Duration: 30 * time.Second}, - wantErr: ErrSSHCheckPeriodBelowMin, - }, { name: "169h above maximum", period: SSHCheckPeriod{Duration: 169 * time.Hour}, @@ -4444,7 +4467,7 @@ func TestSSHCheckPeriodPolicyValidation(t *testing.T) { wantErr: ErrSSHCheckPeriodOnNonCheck, }, { - name: "check with 30s is invalid", + name: "check with 30s is valid (matches SaaS, no minimum)", ssh: SSH{ Action: SSHActionCheck, Sources: SSHSrcAliases{up("user@")}, @@ -4452,7 +4475,17 @@ func TestSSHCheckPeriodPolicyValidation(t *testing.T) { Users: SSHUsers{"root"}, CheckPeriod: &SSHCheckPeriod{Duration: 30 * time.Second}, }, - wantErr: ErrSSHCheckPeriodBelowMin, + }, + { + name: "check with 200h above max is invalid", + ssh: SSH{ + Action: SSHActionCheck, + Sources: SSHSrcAliases{up("user@")}, + Destinations: SSHDstAliases{agp("autogroup:member")}, + Users: SSHUsers{"root"}, + CheckPeriod: &SSHCheckPeriod{Duration: 200 * time.Hour}, + }, + wantErr: ErrSSHCheckPeriodAboveMax, }, } @@ -4472,6 +4505,115 @@ func TestSSHCheckPeriodPolicyValidation(t *testing.T) { } } +// TestSSHRuleSaaSValidation exercises the SaaS-aligned rejections +// added to match the API body strings exactly. +func TestSSHRuleSaaSValidation(t *testing.T) { + baseSSH := func(modify func(*SSH)) SSH { + ssh := SSH{ + Action: SSHActionAccept, + Sources: SSHSrcAliases{up("user@")}, + Destinations: SSHDstAliases{agp("autogroup:member")}, + Users: SSHUsers{"root"}, + } + if modify != nil { + modify(&ssh) + } + + return ssh + } + + tests := []struct { + name string + ssh SSH + wantErr error + }{ + { + name: "users empty rejected", + ssh: baseSSH(func(s *SSH) { s.Users = nil }), + wantErr: ErrSSHUsersMustBeSpecified, + }, + { + name: "users empty array rejected", + ssh: baseSSH(func(s *SSH) { s.Users = SSHUsers{} }), + wantErr: ErrSSHUsersMustBeSpecified, + }, + { + name: "user empty string rejected", + ssh: baseSSH(func(s *SSH) { s.Users = SSHUsers{""} }), + wantErr: ErrSSHUserInvalid, + }, + { + name: "user wildcard rejected", + ssh: baseSSH(func(s *SSH) { s.Users = SSHUsers{"*"} }), + wantErr: ErrSSHUserInvalid, + }, + { + name: "acceptEnv empty entry rejected", + ssh: baseSSH(func(s *SSH) { s.AcceptEnv = []string{"FOO", ""} }), + wantErr: ErrSSHAcceptEnvEmpty, + }, + { + name: "action empty rejected", + ssh: baseSSH(func(s *SSH) { s.Action = "" }), + wantErr: ErrSSHActionMustBeSpecified, + }, + { + name: "user autogroup non-nonroot accepted (literal)", + ssh: baseSSH(func(s *SSH) { + s.Users = SSHUsers{"autogroup:internet"} + }), + }, + { + name: "user malformed localpart accepted (literal)", + ssh: baseSSH(func(s *SSH) { + s.Users = SSHUsers{"localpart:foo"} + }), + }, + { + name: "acceptEnv double-glob accepted", + ssh: baseSSH(func(s *SSH) { + s.AcceptEnv = []string{"**"} + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pol := &Policy{SSHs: []SSH{tt.ssh}} + err := pol.validate() + + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + + return + } + + require.NoError(t, err) + }) + } +} + +// TestSSHActionInvalidUnmarshal verifies the SaaS-aligned wording for +// non-empty unknown actions surfaces at JSON parse time. +func TestSSHActionInvalidUnmarshal(t *testing.T) { + var a SSHAction + + err := json.Unmarshal([]byte(`"deny"`), &a) + require.Error(t, err) + require.ErrorIs(t, err, ErrSSHActionInvalid) + require.Contains(t, err.Error(), `"deny" is not a valid action`) +} + +// TestSSHCheckPeriodInvalidDuration verifies the SaaS body for the +// malformed-duration case (`time: invalid duration "abc"`). +func TestSSHCheckPeriodInvalidDuration(t *testing.T) { + var p SSHCheckPeriod + + err := json.Unmarshal([]byte(`"abc"`), &p) + require.Error(t, err) + require.Contains(t, err.Error(), `time: invalid duration "abc"`) +} + func TestUnmarshalGrants(t *testing.T) { tests := []struct { name string