From 7e4be35cd2ecd002cd882a75bb82dec069e20418 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 13 May 2026 09:57:54 +0000 Subject: [PATCH] policy/v2: tolerate tag-owner cycles by resolving to empty SaaS accepts tagOwners with circular references (tag:a -> tag:b -> tag:a, tag:a -> tag:a) by dropping the cycle edge instead of failing the policy. Each tag whose only path back is via the cycle resolves to the empty owner set; a sibling non-tag owner survives. headscale previously rejected the policy with ErrCircularReference at parse time. Drop the sentinel, change flattenTags to return an empty owner list on revisit, and update flattenTagOwners tests to capture the new behaviour including the sibling-survives edge case. --- hscontrol/policy/v2/policy.go | 29 ++++------ .../v2/tailscale_ssh_data_compat_test.go | 12 ----- hscontrol/policy/v2/types.go | 2 - hscontrol/policy/v2/types_test.go | 53 +++++++++++++++++-- 4 files changed, 58 insertions(+), 38 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 4d33397c..679b4397 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1456,27 +1456,18 @@ func (pm *PolicyManager) invalidateGlobalPolicyCache(newNodes views.Slice[types. } } -// flattenTags flattens the TagOwners by resolving nested tags and detecting cycles. -// It will return a Owners list where all the Tag types have been resolved to their underlying Owners. +// flattenTags flattens the TagOwners by resolving nested tags. Cycles +// in the ownership graph (tag:a -> tag:b -> tag:a, or tag:a -> tag:a) +// are tolerated to match SaaS: the cycle-causing edge is dropped, the +// remaining owners propagate, and the cycle itself contributes no +// addresses. Non-cycle owners on the cycled tags still resolve. +// Undefined-tag references remain a hard error. func flattenTags(tagOwners TagOwners, tag Tag, visiting map[Tag]bool, chain []Tag) (Owners, error) { if visiting[tag] { - cycleStart := 0 - - for i, t := range chain { - if t == tag { - cycleStart = i - break - } - } - - cycleTags := make([]string, len(chain[cycleStart:])) - for i, t := range chain[cycleStart:] { - cycleTags[i] = string(t) - } - - slices.Sort(cycleTags) - - return nil, fmt.Errorf("%w: %s", ErrCircularReference, strings.Join(cycleTags, " -> ")) + // Cycle: this tag is already on the resolution stack. SaaS + // drops the edge instead of failing, so we return an empty + // owner set and let the caller continue with any siblings. + return nil, nil } visiting[tag] = true diff --git a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go index 615b044b..92521940 100644 --- a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go @@ -87,18 +87,6 @@ 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", - - // TAG_OWNER_CYCLE_TOLERATED (2 tests) - // - // SaaS accepts tagOwners with circular references (tag:a -> tag:b, - // tag:b -> tag:a or tag:a -> tag:a) and resolves them as the empty - // owner set. headscale's TagOwnerSet resolver eagerly detects - // circular tag references and rejects the policy at parse time. - // Pick a side and adjust: either match SaaS by treating cycles as - // "no owners" instead of an error, or document that headscale is - // intentionally stricter and remove these scenarios. - "ssh-tag-owner-cycle": "headscale rejects tagOwner cycles that SaaS accepts as empty owner set", - "ssh-tag-owner-self-reference": "headscale rejects self-referencing tagOwner that SaaS accepts as empty owner set", } // sshRejectSkipReasons documents APIResponseCode != 200 captures where diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index b2e746fa..379d6ba0 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -34,8 +34,6 @@ const Wildcard = Asterix(0) var ErrAutogroupSelfRequiresPerNodeResolution = errors.New("autogroup:self requires per-node resolution and cannot be resolved in this context") -var ErrCircularReference = errors.New("circular reference detected") - var ErrUndefinedTagReference = errors.New("references undefined tag") // SSH validation errors. diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 28161bac..24a14ec3 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -4153,13 +4153,45 @@ func TestFlattenTagOwners(t *testing.T) { wantErr: "", }, { - name: "circular-reference", + // SaaS tolerates tag:a <-> tag:b cycles by dropping the + // cycle edge; both tags resolve to an empty owner set + // because neither chain reaches a non-tag owner. + name: "circular-reference-resolves-to-empty", input: TagOwners{ Tag("tag:a"): Owners{new(Tag("tag:b"))}, Tag("tag:b"): Owners{new(Tag("tag:a"))}, }, - want: nil, - wantErr: "circular reference detected: tag:a -> tag:b", + want: TagOwners{ + Tag("tag:a"): nil, + Tag("tag:b"): nil, + }, + wantErr: "", + }, + { + // tag:a -> tag:a self-reference: the only owner is the + // cycle edge itself; result is empty. + name: "self-reference-resolves-to-empty", + input: TagOwners{ + Tag("tag:a"): Owners{new(Tag("tag:a"))}, + }, + want: TagOwners{ + Tag("tag:a"): nil, + }, + wantErr: "", + }, + { + // Cycle plus a sibling non-tag owner: the cycle edge + // drops out, the sibling owner survives. + name: "cycle-plus-sibling-keeps-sibling", + input: TagOwners{ + Tag("tag:a"): Owners{new(Tag("tag:b")), new(Username("alice@example.com"))}, + Tag("tag:b"): Owners{new(Tag("tag:a"))}, + }, + want: TagOwners{ + Tag("tag:a"): Owners{new(Username("alice@example.com"))}, + Tag("tag:b"): Owners{new(Username("alice@example.com"))}, + }, + wantErr: "", }, { name: "mixed-owners", @@ -4218,7 +4250,9 @@ func TestFlattenTagOwners(t *testing.T) { wantErr: "", }, { - name: "tag-long-circular-chain", + // Long cycle: every tag eventually points back to itself. + // Each tag resolves to the empty owner set. + name: "tag-long-circular-chain-resolves-to-empty", input: TagOwners{ Tag("tag:a"): Owners{new(Tag("tag:g"))}, Tag("tag:b"): Owners{new(Tag("tag:a"))}, @@ -4228,7 +4262,16 @@ func TestFlattenTagOwners(t *testing.T) { Tag("tag:f"): Owners{new(Tag("tag:e"))}, Tag("tag:g"): Owners{new(Tag("tag:f"))}, }, - wantErr: "circular reference detected: tag:a -> tag:b -> tag:c -> tag:d -> tag:e -> tag:f -> tag:g", + want: TagOwners{ + Tag("tag:a"): nil, + Tag("tag:b"): nil, + Tag("tag:c"): nil, + Tag("tag:d"): nil, + Tag("tag:e"): nil, + Tag("tag:f"): nil, + Tag("tag:g"): nil, + }, + wantErr: "", }, { name: "undefined-tag-reference",