policy/v2: resolve tag owners one hop deep

A user claims only the tags it owns directly, and a tag owns only the
tags that list it, as on Tailscale SaaS. With no walk left, validate
names an undefined owner once, with the tag that lists it.

hscontrol/policy/pm.go

@@ -47,9 +47,9 @@ type PolicyManager interface {
 	TagExists(tag string) bool
 
 	// TagOwnedByTags reports whether a credential holding ownerTags may apply
-	// tag: true if tag is one of ownerTags, or tag's tag-to-tag ownership chain
-	// transitively includes one of ownerTags. Authorises the tags an OAuth
-	// access token may set on the auth keys it mints.
+	// tag: true if tag is one of ownerTags, or tag's tagOwners list one of
+	// them (one hop). Authorises the tags an OAuth access token may set on the
+	// auth keys it mints.
 	TagOwnedByTags(tag string, ownerTags []string) bool
 
 	// NodeCanApproveRoute reports whether the given node can approve the given route.

hscontrol/policy/v2/policy.go

@@ -1273,63 +1273,35 @@ func (pm *PolicyManager) UserCanHaveTag(user types.UserView, tag string) bool {
 }
 
 // TagOwnedByTags reports whether a credential holding ownerTags is authorised to
-// apply tag. It is true when tag is one of ownerTags, or when tag's tagOwners
-// chain (tag-to-tag ownership) transitively includes one of ownerTags. This is
-// the tag-level check used when an OAuth access token mints an auth key: the
-// requested tags must each be owned by the token's tags, so an operator token
-// tagged tag:k8s-operator may mint tag:k8s keys when the policy declares
-// "tag:k8s": ["tag:k8s-operator"]. It is purely tag-relational and does not
-// consult node IPs.
+// apply tag: tag is one of ownerTags, or tag's tagOwners list one of them.
+// Ownership reaches one hop, as on Tailscale SaaS: a tag owned by a tag that
+// ownerTags own is not reached. This is the tag-level check used when an
+// OAuth access token mints an auth key: the requested tags must each be owned
+// by the token's tags, so an operator token tagged tag:k8s-operator may mint
+// tag:k8s keys when the policy declares "tag:k8s": ["tag:k8s-operator"]. It is
+// purely tag-relational and does not consult node IPs.
 func (pm *PolicyManager) TagOwnedByTags(tag string, ownerTags []string) bool {
 	if pm == nil {
 		return false
 	}
 
-	owns := make(map[string]bool, len(ownerTags))
-	for _, t := range ownerTags {
-		owns[t] = true
-	}
-
 	// A credential may always apply a tag it directly holds; this needs no policy.
-	if owns[tag] {
+	if slices.Contains(ownerTags, tag) {
 		return true
 	}
 
 	pm.mu.RLock()
 	defer pm.mu.RUnlock()
 
-	// Owned-by delegation requires the policy's tagOwners.
 	if pm.pol == nil {
 		return false
 	}
 
-	// Walk tag-to-tag ownership transitively, guarding against cycles.
-	visited := make(map[Tag]bool)
+	return slices.ContainsFunc(pm.pol.TagOwners[Tag(tag)], func(owner Owner) bool {
+		ot, ok := owner.(*Tag)
 
-	var walk func(t Tag) bool
-
-	walk = func(t Tag) bool {
-		if visited[t] {
-			return false
-		}
-
-		visited[t] = true
-
-		for _, owner := range pm.pol.TagOwners[t] {
-			ot, ok := owner.(*Tag)
-			if !ok {
-				continue
-			}
-
-			if owns[string(*ot)] || walk(*ot) {
-				return true
-			}
-		}
-
-		return false
-	}
-
-	return walk(Tag(tag))
+		return ok && slices.Contains(ownerTags, string(*ot))
+	})
 }
 
 // userMatchesOwner checks if a user matches a tag owner entry.
@@ -1985,93 +1957,31 @@ func (pm *PolicyManager) invalidateGlobalPolicyCache(newNodes views.Slice[types.
 	})
 }
 
-// flattenTags resolves nested tag-owner references. Cycles
-// (tag:a -> tag:b -> tag:a, or tag:a -> tag:a) drop the cycle-causing
-// edge and contribute 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] {
-		return nil, nil
-	}
-
-	visiting[tag] = true
-
-	chain = append(chain, tag)
-	defer delete(visiting, tag)
-
-	var result Owners
-
-	for _, owner := range tagOwners[tag] {
-		switch o := owner.(type) {
-		case *Tag:
-			if _, ok := tagOwners[*o]; !ok {
-				return nil, fmt.Errorf("tag %q %w %q", tag, ErrUndefinedTagReference, *o)
-			}
-
-			nested, err := flattenTags(tagOwners, *o, visiting, chain)
-			if err != nil {
-				return nil, err
-			}
-
-			result = append(result, nested...)
-		default:
-			result = append(result, owner)
-		}
-	}
-
-	return result, nil
-}
-
-// flattenTagOwners flattens all [TagOwners] by resolving nested tags and detecting cycles.
-// It will return a new [TagOwners] map where all the [Tag] types have been resolved to their underlying [Owners].
-func flattenTagOwners(tagOwners TagOwners) (TagOwners, error) {
-	ret := make(TagOwners)
-
-	for tag := range tagOwners {
-		flattened, err := flattenTags(tagOwners, tag, make(map[Tag]bool), nil)
-		if err != nil {
-			return nil, err
-		}
-
-		slices.SortFunc(flattened, func(a, b Owner) int {
-			return cmp.Compare(a.String(), b.String())
-		})
-		ret[tag] = slices.CompactFunc(flattened, func(a, b Owner) bool {
-			return a.String() == b.String()
-		})
-	}
-
-	return ret, nil
-}
-
 // resolveTagOwners resolves the [TagOwners] to a map of [Tag] to [netipx.IPSet].
 // The resulting map can be used to quickly look up the IPSet for a given [Tag].
 // It is intended for internal use in a [PolicyManager].
+//
+// Only a tag's own users and groups count: owning a tag does not give the
+// tags it owns, as on Tailscale SaaS. A tag listed as owner is answered by
+// [PolicyManager.TagOwnedByTags].
 func resolveTagOwners(p *Policy, users types.Users, nodes views.Slice[types.NodeView]) (map[Tag]*netipx.IPSet, error) {
-	if p == nil {
-		return make(map[Tag]*netipx.IPSet), nil
-	}
-
-	if len(p.TagOwners) == 0 {
-		return make(map[Tag]*netipx.IPSet), nil
-	}
-
 	ret := make(map[Tag]*netipx.IPSet)
-
-	tagOwners, err := flattenTagOwners(p.TagOwners)
-	if err != nil {
-		return nil, err
+	if p == nil {
+		return ret, nil
 	}
 
-	for tag, owners := range tagOwners {
+	for tag, owners := range p.TagOwners {
 		var ips netipx.IPSetBuilder
 
 		for _, owner := range owners {
 			switch o := owner.(type) {
 			case *Tag:
-				// After flattening, Tag types should not appear in the owners list.
-				// If they do, skip them as they represent already-resolved references.
-
+				// validate refuses this; a policy built in code can still
+				// name one.
+				if _, ok := p.TagOwners[*o]; !ok {
+					return nil, fmt.Errorf("tagOwners[%q]: %q: %w",
+						string(tag), string(*o), ErrUndefinedTagReference)
+				}
 			case Alias:
 				// If it does not resolve, that means the tag is not associated with any IP addresses.
 				resolved, _ := o.Resolve(p, users, nodes)
@@ -2082,7 +1992,6 @@ func resolveTagOwners(p *Policy, users types.Users, nodes views.Slice[types.Node
 				}
 
 			default:
-				// Should never happen - after flattening, all owners should be Alias types
 				return nil, fmt.Errorf("%w: %v", ErrInvalidTagOwner, owner)
 			}
 		}

hscontrol/policy/v2/policy_test.go

@@ -3439,9 +3439,15 @@ func TestTagOwnedByTags(t *testing.T) {
 			want:      true,
 		},
 		{
-			name:      "transitive chain root owns leaf",
+			name:      "two hops are not reached",
 			tag:       "tag:leaf",
 			ownerTags: []string{"tag:root"},
+			want:      false,
+		},
+		{
+			name:      "one of several held tags owns it",
+			tag:       "tag:leaf",
+			ownerTags: []string{"tag:lone", "tag:mid"},
 			want:      true,
 		},
 		{

hscontrol/policy/v2/tagowner_compat_test.go

@@ -26,18 +26,10 @@ import (
 // <test_id>/<step>/<function>. A listed check that matches fails, so a fix
 // must delete its entry.
 var tagOwnerSkipReasons = map[string]string{
-	"tag-owner-chain/u-re-relogin/NodeCanHaveTag": tagOwnedThroughTag,
-
-	"tag-owner-chain/C-base-mint-top/TagOwnedByTags":   tagReachedTwoHops,
-	"tag-owner-chain/C-base-mint-dtop/TagOwnedByTags":  tagReachedTwoHops,
 	"tag-owner-chain/C-two-mint-subset/TagOwnedByTags": clientTagSubset,
 }
 
-const (
-	tagOwnedThroughTag = "TAG_OWNED_THROUGH_TAG: headscale lets a user's node apply a tag whose owner is a tag the user owns; SaaS gives a user only the tags it owns directly"
-	tagReachedTwoHops  = "TAG_REACHED_TWO_HOPS: headscale lets a tagged credential apply a tag any number of ownership steps away; SaaS allows only tags its own tags own directly"
-	clientTagSubset    = "CLIENT_TAG_SUBSET: headscale lets an OAuth client mint a key with a subset of its tags; SaaS needs that subset owned by the client's tags"
-)
+const clientTagSubset = "CLIENT_TAG_SUBSET: headscale lets an OAuth client mint a key with a subset of its tags; SaaS needs that subset owned by the client's tags"
 
 // TestTagOwnerChainCompat compares each SaaS verdict on a tag request with
 // the policy function headscale asks for that request:

hscontrol/policy/v2/tailscale_acl_data_compat_test.go

@@ -207,9 +207,6 @@ var aclSkipReasons = map[string]string{
 	"acl-err10/refused": routeHostBits,
 	"acl-err11/refused": routeHostBits,
 
-	"acl-tagowner-undefined-nested/message":      tagOwnerUndefinedText,
-	"acl-tagowner-undefined-nested-deep/message": tagOwnerUndefinedText,
-
 	"acl-n01/beedrill/filter":   allowAllForm,
 	"acl-n01/bulbasaur/filter":  allowAllForm,
 	"acl-n01/charmander/filter": allowAllForm,
@@ -235,11 +232,10 @@ var aclSkipReasons = map[string]string{
 }
 
 const (
-	routeHostBits         = "ROUTE_HOST_BITS: SaaS refuses an autoApprovers route with host bits; headscale accepts it"
-	tagOwnerUndefinedText = "TAG_OWNER_UNDEFINED_TEXT: SaaS names the owning tag and its undefined owner in one line; headscale words it differently"
-	allowAllForm          = "ALLOW_ALL_FORM: SaaS writes the allow-all default's sources as the tailnet ranges, headscale as \"*\""
-	aclDstDedupe          = "ACL_DST_DEDUPE: SaaS writes a destination repeated within one ACL once, headscale once per mention"
-	srcAddressForm        = "FILTER_ADDRESS_FORM: SaaS lists contiguous source addresses one by one, headscale as a range"
+	routeHostBits  = "ROUTE_HOST_BITS: SaaS refuses an autoApprovers route with host bits; headscale accepts it"
+	allowAllForm   = "ALLOW_ALL_FORM: SaaS writes the allow-all default's sources as the tailnet ranges, headscale as \"*\""
+	aclDstDedupe   = "ACL_DST_DEDUPE: SaaS writes a destination repeated within one ACL once, headscale once per mention"
+	srcAddressForm = "FILTER_ADDRESS_FORM: SaaS lists contiguous source addresses one by one, headscale as a range"
 )
 
 // TestACLCompat is a data-driven test that loads all ACL-*.json test files

hscontrol/policy/v2/types.go

@@ -38,7 +38,7 @@ const Wildcard = Asterix(0)
 
 var ErrAutogroupSelfRequiresPerNodeResolution = errors.New("autogroup:self requires per-node resolution and cannot be resolved in this context")
 
-var ErrUndefinedTagReference = errors.New("references undefined tag")
+var ErrUndefinedTagReference = errors.New("does not exist")
 
 // SSH validation errors.
 var (
@@ -2782,7 +2782,7 @@ func (p *Policy) validate() error {
 		}
 	}
 
-	for _, tagOwners := range p.TagOwners {
+	for tag, tagOwners := range p.TagOwners {
 		for _, tagOwner := range tagOwners {
 			switch tagOwner := tagOwner.(type) {
 			case *Group:
@@ -2793,22 +2793,14 @@ func (p *Policy) validate() error {
 					errs = append(errs, err)
 				}
 			case *Tag:
-				t := tagOwner
-
-				err := p.TagOwners.Contains(t)
-				if err != nil {
-					errs = append(errs, err)
+				if _, ok := p.TagOwners[*tagOwner]; !ok {
+					errs = append(errs, fmt.Errorf("tagOwners[%q]: %q: %w",
+						string(tag), string(*tagOwner), ErrUndefinedTagReference))
 				}
 			}
 		}
 	}
 
-	// Validate tag ownership chains for circular references and undefined tags.
-	_, err := flattenTagOwners(p.TagOwners)
-	if err != nil {
-		errs = append(errs, err)
-	}
-
 	for _, approvers := range p.AutoApprovers.Routes {
 		for _, approver := range approvers {
 			switch approver := approver.(type) {

hscontrol/policy/v2/types_test.go

@@ -1544,7 +1544,7 @@ func TestUnmarshalPolicy(t *testing.T) {
   },
 }
 `,
-			wantErr: `tag "tag:child" references undefined tag "tag:nonexistent"`,
+			wantErr: `tagOwners["tag:child"]: "tag:nonexistent": does not exist`,
 		},
 		// SSH source/destination validation tests (#3009, #3010)
 		{
@@ -3526,10 +3526,38 @@ func TestResolveTagOwners(t *testing.T) {
 			},
 			want: map[Tag]*netipx.IPSet{
 				Tag("tag:bigbrother"):   mustIPSet("100.64.0.1/32"),
-				Tag("tag:smallbrother"): mustIPSet("100.64.0.1/32"),
+				Tag("tag:smallbrother"): mustIPSet(),
 			},
 			wantErr: false,
 		},
+		{
+			name: "tag-owner-chain-keeps-direct-owners",
+			policy: &Policy{
+				TagOwners: TagOwners{
+					Tag("tag:a"): Owners{new(Username("user1@"))},
+					Tag("tag:b"): Owners{new(Tag("tag:a"))},
+					Tag("tag:c"): Owners{new(Tag("tag:b")), new(Username("user2@"))},
+				},
+			},
+			want: map[Tag]*netipx.IPSet{
+				Tag("tag:a"): mustIPSet("100.64.0.1/32"),
+				Tag("tag:b"): mustIPSet(),
+				Tag("tag:c"): mustIPSet("100.64.0.2/32"),
+			},
+		},
+		{
+			name: "tag-owner-cycle",
+			policy: &Policy{
+				TagOwners: TagOwners{
+					Tag("tag:a"): Owners{new(Tag("tag:b")), new(Username("user1@"))},
+					Tag("tag:b"): Owners{new(Tag("tag:a"))},
+				},
+			},
+			want: map[Tag]*netipx.IPSet{
+				Tag("tag:a"): mustIPSet("100.64.0.1/32"),
+				Tag("tag:b"): mustIPSet(),
+			},
+		},
 	}
 
 	cmps := append(util.Comparers, cmp.Comparer(ipSetComparer))
@@ -4231,193 +4259,6 @@ func mustParseAlias(s string) Alias {
 	return alias
 }
 
-func TestFlattenTagOwners(t *testing.T) {
-	tests := []struct {
-		name    string
-		input   TagOwners
-		want    TagOwners
-		wantErr string
-	}{
-		{
-			name: "tag-owns-tag",
-			input: TagOwners{
-				Tag("tag:bigbrother"):   Owners{new(Group("group:user1"))},
-				Tag("tag:smallbrother"): Owners{new(Tag("tag:bigbrother"))},
-			},
-			want: TagOwners{
-				Tag("tag:bigbrother"):   Owners{new(Group("group:user1"))},
-				Tag("tag:smallbrother"): Owners{new(Group("group:user1"))},
-			},
-			wantErr: "",
-		},
-		{
-			// 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: 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",
-			input: TagOwners{
-				Tag("tag:x"): Owners{new(Username("user1@")), new(Tag("tag:y"))},
-				Tag("tag:y"): Owners{new(Username("user2@"))},
-			},
-			want: TagOwners{
-				Tag("tag:x"): Owners{new(Username("user1@")), new(Username("user2@"))},
-				Tag("tag:y"): Owners{new(Username("user2@"))},
-			},
-			wantErr: "",
-		},
-		{
-			name: "mixed-dupe-owners",
-			input: TagOwners{
-				Tag("tag:x"): Owners{new(Username("user1@")), new(Tag("tag:y"))},
-				Tag("tag:y"): Owners{new(Username("user1@"))},
-			},
-			want: TagOwners{
-				Tag("tag:x"): Owners{new(Username("user1@"))},
-				Tag("tag:y"): Owners{new(Username("user1@"))},
-			},
-			wantErr: "",
-		},
-		{
-			name: "no-tag-owners",
-			input: TagOwners{
-				Tag("tag:solo"): Owners{new(Username("user1@"))},
-			},
-			want: TagOwners{
-				Tag("tag:solo"): Owners{new(Username("user1@"))},
-			},
-			wantErr: "",
-		},
-		{
-			name: "tag-long-owner-chain",
-			input: TagOwners{
-				Tag("tag:a"): Owners{new(Group("group:user1"))},
-				Tag("tag:b"): Owners{new(Tag("tag:a"))},
-				Tag("tag:c"): Owners{new(Tag("tag:b"))},
-				Tag("tag:d"): Owners{new(Tag("tag:c"))},
-				Tag("tag:e"): Owners{new(Tag("tag:d"))},
-				Tag("tag:f"): Owners{new(Tag("tag:e"))},
-				Tag("tag:g"): Owners{new(Tag("tag:f"))},
-			},
-			want: TagOwners{
-				Tag("tag:a"): Owners{new(Group("group:user1"))},
-				Tag("tag:b"): Owners{new(Group("group:user1"))},
-				Tag("tag:c"): Owners{new(Group("group:user1"))},
-				Tag("tag:d"): Owners{new(Group("group:user1"))},
-				Tag("tag:e"): Owners{new(Group("group:user1"))},
-				Tag("tag:f"): Owners{new(Group("group:user1"))},
-				Tag("tag:g"): Owners{new(Group("group:user1"))},
-			},
-			wantErr: "",
-		},
-		{
-			// 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"))},
-				Tag("tag:c"): Owners{new(Tag("tag:b"))},
-				Tag("tag:d"): Owners{new(Tag("tag:c"))},
-				Tag("tag:e"): Owners{new(Tag("tag:d"))},
-				Tag("tag:f"): Owners{new(Tag("tag:e"))},
-				Tag("tag:g"): Owners{new(Tag("tag:f"))},
-			},
-			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",
-			input: TagOwners{
-				Tag("tag:a"): Owners{new(Tag("tag:nonexistent"))},
-			},
-			wantErr: `tag "tag:a" references undefined tag "tag:nonexistent"`,
-		},
-		{
-			name: "tag-with-empty-owners-is-valid",
-			input: TagOwners{
-				Tag("tag:a"): Owners{new(Tag("tag:b"))},
-				Tag("tag:b"): Owners{}, // empty owners but exists
-			},
-			want: TagOwners{
-				Tag("tag:a"): nil,
-				Tag("tag:b"): nil,
-			},
-			wantErr: "",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := flattenTagOwners(tt.input)
-			if tt.wantErr != "" {
-				if err == nil {
-					t.Fatalf("flattenTagOwners() expected error %q, got nil", tt.wantErr)
-				}
-
-				if err.Error() != tt.wantErr {
-					t.Fatalf("flattenTagOwners() expected error %q, got %q", tt.wantErr, err.Error())
-				}
-
-				return
-			}
-
-			if err != nil {
-				t.Fatalf("flattenTagOwners() unexpected error: %v", err)
-			}
-
-			if diff := cmp.Diff(tt.want, got); diff != "" {
-				t.Errorf("flattenTagOwners() mismatch (-want +got):\n%s", diff)
-			}
-		})
-	}
-}
-
 func TestSSHCheckPeriodUnmarshal(t *testing.T) {
 	tests := []struct {
 		name    string