policy/v2: match SaaS wording for group nesting rejection

SaaS rejects any group-in-group reference (chain, cycle, or
self-reference) with `groups["X"]: "Y": group members cannot be
recursive`. headscale rejected the same input but the message
surfaced as a generic JSON parse error from the unmarshal layer.
Groups.UnmarshalJSON now scans the raw map in descending
alphabetical order and reports the first group whose member is
itself a group, mirroring the (X, Y) pair SaaS picks (deepest
non-leaf parent first). Drop the now-unused ErrNestedGroups
sentinel and update the existing group-in-group test row plus add
chain, cycle, and self-cycle rows.
This commit is contained in:
Kristoffer Dalby
2026-05-13 10:19:09 +00:00
parent 079dca8924
commit 2b61b26772
3 changed files with 66 additions and 22 deletions
@@ -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
+22 -5
View File
@@ -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:
@@ -124,7 +125,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")
@@ -1408,6 +1408,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 {
@@ -1420,10 +1441,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
}
+44 -2
View File
@@ -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",