diff --git a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go index 74258ff5..03d9dd24 100644 --- a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go @@ -106,21 +106,6 @@ var sshRejectSkipReasons = map[string]string{ "ssh-e1": "domain validation: headscale has no 'associated tailnet domains' concept", "ssh-e2": "domain validation: headscale has no 'associated tailnet domains' concept", "ssh-malformed-user-localpart-multi-glob": "domain validation: headscale has no 'associated tailnet domains' concept (same gap as ssh-b4/d1/e1/e2)", - - // GROUP_NESTING_ERROR_BODY (3 tests) - // - // SaaS rejects any group-in-group reference (cycle, chain, - // self-cycle) with the structured message - // `groups["X"]: "Y": group members cannot be recursive`. - // headscale rejects too but the error surfaces as a generic - // `parsing policy: parsing policy from bytes: json: unable to - // unmarshal …` because the group resolver fails before the - // validation phase that would emit a specific message. Wire the - // resolver's "group references a group" detection to produce the - // SaaS-style structured error so the body matches. - "ssh-group-nested-cycle": "group nesting rejected with different error body (parse error vs structured 'group members cannot be recursive')", - "ssh-group-nested-three-deep": "group nesting rejected with different error body (parse error vs structured 'group members cannot be recursive')", - "ssh-group-nested-two-deep": "group nesting rejected with different error body (parse error vs structured 'group members cannot be recursive')", } // TestSSHDataCompat is a data-driven test that loads all ssh-*.hujson test diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 4e4c3ecd..7a12188f 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -54,6 +54,7 @@ var ( ErrSSHActionInvalid = errors.New("is not a valid action") ErrSSHDestinationHostAlias = errors.New("invalid dst") ErrTagNameMustStartWithLetter = errors.New("tag names must start with a letter, after 'tag:'") + ErrGroupMembersCannotBeRecursive = errors.New("group members cannot be recursive") ) // SSH check period constants per Tailscale docs: @@ -104,7 +105,6 @@ var ( ErrGroupNotDefined = errors.New("group not defined in policy") ErrInvalidGroupMember = errors.New("invalid group member type") ErrGroupValueNotArray = errors.New("group value must be an array of users") - ErrNestedGroups = errors.New("nested groups are not allowed") ErrInvalidHostIP = errors.New("hostname contains invalid IP address") ErrTagNotDefined = errors.New("tag not found") ErrAutoApproverNotAlias = errors.New("auto approver is not an alias") @@ -1388,6 +1388,27 @@ 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. + keys := make([]string, 0, len(rawGroups)) + for k := range rawGroups { + keys = append(keys, k) + } + + slices.Sort(keys) + slices.Reverse(keys) + + for _, key := range keys { + for _, u := range rawGroups[key] { + if isGroup(u) { + return fmt.Errorf("groups[%q]: %q: %w", key, u, ErrGroupMembersCannotBeRecursive) + } + } + } + *g = make(Groups) for key, value := range rawGroups { @@ -1400,10 +1421,6 @@ func (g *Groups) UnmarshalJSON(b []byte) error { err := username.Validate() if err != nil { - if isGroup(u) { - return fmt.Errorf("%w: found %q inside %q", ErrNestedGroups, u, group) - } - return err } diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index a46738b7..8dd06464 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -407,8 +407,50 @@ func TestUnmarshalPolicy(t *testing.T) { }, } `, - // wantErr: `username must contain @, got: "group:inner"`, - wantErr: `nested groups are not allowed: found "group:inner" inside "group:example"`, + wantErr: `groups["group:example"]: "group:inner": group members cannot be recursive`, + }, + { + // SaaS reports the deepest non-leaf parent first: for + // the three-deep chain `a -> b -> c -> user`, the + // reported pair is `b -> c` rather than `a -> b`. + name: "group-nested-three-deep", + input: ` +{ + "groups": { + "group:a": ["group:b"], + "group:b": ["group:c"], + "group:c": ["thor@example.org"], + }, +} +`, + wantErr: `groups["group:b"]: "group:c": group members cannot be recursive`, + }, + { + // Cycle `a <-> b`: reported as `b -> a` so the body + // matches SaaS exactly. + name: "group-nested-cycle", + input: ` +{ + "groups": { + "group:a": ["group:b"], + "group:b": ["group:a"], + }, +} +`, + wantErr: `groups["group:b"]: "group:a": group members cannot be recursive`, + }, + { + // Self-cycle: the same group appears as its own + // member. Same wording. + name: "group-nested-self-cycle", + input: ` +{ + "groups": { + "group:a": ["group:a"], + }, +} +`, + wantErr: `groups["group:a"]: "group:a": group members cannot be recursive`, }, { name: "invalid-addr",