From bc9fb6d403f182e4441398371454edba27b088d0 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Tue, 28 Apr 2026 16:09:42 +0000 Subject: [PATCH] hscontrol/policy/v2: reject ambiguous user references at load time When a user@ token resolved to more than one DB row, ACL and SSH rules referencing it were silently dropped at compile time, leaving clients with SSHPolicy={rules: null} and no signal to the admin. Validate every Username reference in groups, tagOwners, autoApprovers, ACLs and SSH rules at NewPolicyManager and SetPolicy and return ErrMultipleUsersFound. Missing-user tokens stay tolerant per #2863. Updates #3160 --- hscontrol/policy/v2/policy.go | 96 +++++++++++++++++ hscontrol/policy/v2/policy_test.go | 161 +++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 355c8158..f11741fb 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -20,6 +20,7 @@ import ( "tailscale.com/tailcfg" "tailscale.com/types/views" "tailscale.com/util/deephash" + "tailscale.com/util/multierr" ) // ErrInvalidTagOwner is returned when a tag owner is not an Alias type. @@ -73,6 +74,91 @@ type filterAndPolicy struct { Policy *Policy } +// validateUserReferences surfaces ambiguous user@ tokens at policy load so +// duplicate DB rows fail loudly instead of silently dropping rules (#3160). +// Missing-user tokens stay tolerant (#2863). Empty users → no-op for +// syntax-only checks. +func validateUserReferences(pol *Policy, users types.Users) error { + if pol == nil || len(users) == 0 { + return nil + } + + var errs []error + + check := func(u *Username) { + if u == nil { + return + } + + _, err := u.resolveUser(users) + if err != nil && errors.Is(err, ErrMultipleUsersFound) { + errs = append(errs, err) + } + } + + checkAlias := func(a Alias) { + if u, ok := a.(*Username); ok { + check(u) + } + } + + checkOwner := func(o Owner) { + if u, ok := o.(*Username); ok { + check(u) + } + } + + checkAutoApprover := func(aa AutoApprover) { + if u, ok := aa.(*Username); ok { + check(u) + } + } + + for _, usernames := range pol.Groups { + for i := range usernames { + check(&usernames[i]) + } + } + + for _, owners := range pol.TagOwners { + for _, o := range owners { + checkOwner(o) + } + } + + for _, approvers := range pol.AutoApprovers.Routes { + for _, aa := range approvers { + checkAutoApprover(aa) + } + } + + for _, aa := range pol.AutoApprovers.ExitNode { + checkAutoApprover(aa) + } + + for _, acl := range pol.ACLs { + for _, src := range acl.Sources { + checkAlias(src) + } + + for _, dst := range acl.Destinations { + checkAlias(dst.Alias) + } + } + + for _, ssh := range pol.SSHs { + for _, src := range ssh.Sources { + checkAlias(src) + } + + for _, dst := range ssh.Destinations { + checkAlias(dst) + } + } + + return multierr.New(errs...) +} + // NewPolicyManager creates a new PolicyManager from a policy file and a list of users and nodes. // It returns an error if the policy file is invalid. // The policy manager will update the filter rules based on the users and nodes. @@ -82,6 +168,11 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node return nil, fmt.Errorf("parsing policy: %w", err) } + err = validateUserReferences(policy, users) + if err != nil { + return nil, fmt.Errorf("validating policy user references: %w", err) + } + pm := PolicyManager{ pol: policy, users: users, @@ -351,6 +442,11 @@ func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { pm.mu.Lock() defer pm.mu.Unlock() + err = validateUserReferences(pol, pm.users) + if err != nil { + return false, fmt.Errorf("validating policy user references: %w", err) + } + // Log policy metadata for debugging log.Debug(). Int("policy.bytes", len(polB)). diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 722799db..9495e110 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -1861,3 +1861,164 @@ func TestBuildPeerMap_AutogroupInternetMakesExitNodeVisible(t *testing.T) { require.True(t, aliceNode.View().CanAccess(matchers, exitNode.View()), "alice.CanAccess(exit) should be true via DestsIsTheInternet()+IsExitNode() (#3212)") } + +// Reproduction for #3160: ambiguous user@ used to silently drop rules. +func TestNewPolicyManager_DuplicateUsername(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 2}, Name: "yala"}, + {Model: gorm.Model{ID: 7}, Name: "yala", Email: "yala@yala.yala"}, + } + + polB := []byte(`{ + "groups": {"group:admins": ["yala@"]}, + "tagOwners": {"tag:ssh": ["group:admins"]}, + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}], + "ssh": [ + {"action":"accept","src":["group:admins"],"dst":["tag:ssh"],"users":["root"]} + ] +}`) + + _, err := NewPolicyManager(polB, users, types.Nodes{}.ViewSlice()) + require.Error(t, err, "NewPolicyManager must reject policy with ambiguous username") + require.ErrorIs(t, err, ErrMultipleUsersFound) + require.Contains(t, err.Error(), "yala@", + "error must name the offending token") +} + +// Missing-user tokens stay tolerant per #2863; only multi-match blocks load. +func TestNewPolicyManager_UnknownUsernameTolerant(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "alice"}, + } + + polB := []byte(`{ + "acls": [{"action":"accept","src":["ghost@"],"dst":["*:*"]}] +}`) + + _, err := NewPolicyManager(polB, users, types.Nodes{}.ViewSlice()) + require.NoError(t, err, "missing-user references must not block policy load (#2863)") +} + +// Rejected SetPolicy must keep the previous policy intact. +func TestSetPolicy_DuplicateUsername(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 2}, Name: "yala"}, + {Model: gorm.Model{ID: 7}, Name: "yala", Email: "yala@yala.yala"}, + } + + good := []byte(`{ + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}] +}`) + + pm, err := NewPolicyManager(good, users, types.Nodes{}.ViewSlice()) + require.NoError(t, err) + + bad := []byte(`{ + "groups": {"group:admins": ["yala@"]}, + "acls": [{"action":"accept","src":["group:admins"],"dst":["*:*"]}] +}`) + + _, err = pm.SetPolicy(bad) + require.Error(t, err) + require.ErrorIs(t, err, ErrMultipleUsersFound) + + filter, _ := pm.Filter() + require.NotNil(t, filter, "filter must remain populated after rejected SetPolicy") +} + +// Empty users → syntax-only check, used by `headscale policy check`. +func TestValidateUserReferences_EmptyUsersTolerant(t *testing.T) { + polB := []byte(`{ + "groups": {"group:admins": ["yala@"]}, + "tagOwners": {"tag:ssh": ["group:admins"]}, + "acls": [{"action":"accept","src":["yala@"],"dst":["*:*"]}], + "ssh": [ + {"action":"accept","src":["yala@"],"dst":["tag:ssh"],"users":["root"]} + ] +}`) + + _, err := NewPolicyManager(polB, nil, types.Nodes{}.ViewSlice()) + require.NoError(t, err, "nil users must skip user-reference validation") + + _, err = NewPolicyManager(polB, types.Users{}, types.Nodes{}.ViewSlice()) + require.NoError(t, err, "empty users must skip user-reference validation") +} + +// One case per AST site so a dropped walk fails the matching subtest. +func TestValidateUserReferences_AllSites(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "alice"}, + {Model: gorm.Model{ID: 2}, Name: "dup"}, + {Model: gorm.Model{ID: 3}, Name: "dup"}, + } + + tests := []struct { + name string + pol string + }{ + { + name: "groups", + pol: `{ + "groups": {"group:admins": ["dup@"]}, + "acls": [{"action":"accept","src":["group:admins"],"dst":["*:*"]}] +}`, + }, + { + name: "tagOwners", + pol: `{ + "tagOwners": {"tag:ssh": ["dup@"]}, + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}] +}`, + }, + { + name: "autoApprovers.routes", + pol: `{ + "autoApprovers": {"routes": {"10.0.0.0/8": ["dup@"]}}, + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}] +}`, + }, + { + name: "autoApprovers.exitNode", + pol: `{ + "autoApprovers": {"exitNode": ["dup@"]}, + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}] +}`, + }, + { + name: "acls.src", + pol: `{ + "acls": [{"action":"accept","src":["dup@"],"dst":["*:*"]}] +}`, + }, + { + name: "acls.dst", + pol: `{ + "acls": [{"action":"accept","src":["alice@"],"dst":["dup@:*"]}] +}`, + }, + { + name: "ssh.src", + pol: `{ + "tagOwners": {"tag:ssh": ["alice@"]}, + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}], + "ssh": [{"action":"accept","src":["dup@"],"dst":["tag:ssh"],"users":["root"]}] +}`, + }, + { + // ErrSSHUserDestRequiresSameUser forces src==dst when dst is a user. + name: "ssh.dst", + pol: `{ + "acls": [{"action":"accept","src":["*"],"dst":["*:*"]}], + "ssh": [{"action":"accept","src":["dup@"],"dst":["dup@"],"users":["root"]}] +}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewPolicyManager([]byte(tt.pol), users, types.Nodes{}.ViewSlice()) + require.Error(t, err, "site %q must surface duplicate-user errors", tt.name) + require.ErrorIs(t, err, ErrMultipleUsersFound) + }) + } +}