diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index 9ad30f525..f59626131 100644 --- a/hscontrol/policy/pm.go +++ b/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. diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index d9070dde9..dbefc5bd0 100644 --- a/hscontrol/policy/v2/policy.go +++ b/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) } } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index cc13857a0..dacfcfe65 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/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, }, { diff --git a/hscontrol/policy/v2/tagowner_compat_test.go b/hscontrol/policy/v2/tagowner_compat_test.go index 4bab77581..af11b423c 100644 --- a/hscontrol/policy/v2/tagowner_compat_test.go +++ b/hscontrol/policy/v2/tagowner_compat_test.go @@ -26,18 +26,10 @@ import ( // //. 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: diff --git a/hscontrol/policy/v2/tailscale_acl_data_compat_test.go b/hscontrol/policy/v2/tailscale_acl_data_compat_test.go index c89ccf2ca..c4a4b32c8 100644 --- a/hscontrol/policy/v2/tailscale_acl_data_compat_test.go +++ b/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 diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 10819e8bf..7ff9f564a 100644 --- a/hscontrol/policy/v2/types.go +++ b/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) { diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 624769cdb..057307598 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/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