From e2fd68e48ef7131e4cf6652dd516de6c5ce0c21f Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 11:08:01 +0000 Subject: [PATCH] policy/v2: match no one for an ambiguous user outside policy writes A change to user identifiers must not block startup or later logins. Keep policy writes strict; log ambiguous references on load and refresh. --- hscontrol/policy/v2/compiled_test.go | 5 +- hscontrol/policy/v2/policy.go | 10 ++- hscontrol/policy/v2/policy_test.go | 73 ++++++++++++++++--- .../policy/v2/tailscale_users_compat_test.go | 12 --- hscontrol/policy/v2/types.go | 19 ++--- hscontrol/state/persist_test.go | 46 ++++++++++++ 6 files changed, 126 insertions(+), 39 deletions(-) diff --git a/hscontrol/policy/v2/compiled_test.go b/hscontrol/policy/v2/compiled_test.go index 067d6f3eb..f26fc1c72 100644 --- a/hscontrol/policy/v2/compiled_test.go +++ b/hscontrol/policy/v2/compiled_test.go @@ -320,14 +320,15 @@ func TestCompileOtherDestsMatchesReference(t *testing.T) { hp("undefined"), } case "partial-group": + // An ambiguous member matches no one; the others stay. users = append(users, types.User{ID: 100, Name: users[0].Name}) ips, err := grant.Destinations[0].Resolve(pol, users, nodes.ViewSlice()) - require.Error(t, err) + require.NoError(t, err) require.NotNil(t, ips) require.False( t, ips.Empty(), - "exercise a partial result alongside the error", + "exercise a partial result", ) } diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 17b63fda3..dd2ae2fe1 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -135,8 +135,7 @@ type filterAndPolicy struct { Policy *Policy } -// validateUserReferences surfaces ambiguous user@ tokens at policy load so -// duplicate DB rows fail loudly instead of silently dropping rules. +// validateUserReferences reports ambiguous user@ tokens so writes can refuse them. // Missing-user tokens stay tolerant. Empty users → no-op for // syntax-only checks. func validateUserReferences(pol *Policy, users types.Users) error { @@ -247,7 +246,7 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node err = validateUserReferences(policy, users) if err != nil { - return nil, fmt.Errorf("validating policy user references: %w", err) + log.Error().Err(err).Msg("ambiguous policy users at boot") } pm := PolicyManager{ @@ -1122,6 +1121,11 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) { return false, false, err } + err = validateUserReferences(pm.pol, users) + if err != nil { + log.Error().Err(err).Msg("ambiguous policy users after updating users") + } + // SSH rules and per-node filters embed user identity outside the filter // hash, so a user change needs a client refresh even when it did not move. if hadPerNodeFilter || pm.needsPerNodeFilter || diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 09feaf549..fc547ae76 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -2860,14 +2860,16 @@ func TestNewPolicyManager_DuplicateUsername(t *testing.T) { ] }`) - _, err := NewPolicyManager(polB, users, types.Nodes{}.ViewSlice()) - require.Error(t, err, "NewPolicyManager must reject policy with ambiguous username") + pm, err := NewPolicyManager(polB, users, types.Nodes{}.ViewSlice()) + require.NoError(t, err, "ambiguous usernames must not block loading a stored policy") + + _, err = pm.SetPolicy(polB) 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. +// Missing-user tokens stay tolerant per #2863. func TestNewPolicyManager_UnknownUsernameTolerant(t *testing.T) { users := types.Users{ {ID: 1, Name: "alice"}, @@ -4011,9 +4013,9 @@ func TestSetNodesCachedResultsMatchFresh(t *testing.T) { } } -// TestFailedSetUsersKeepsCompiledPolicy renames alice to charlie and adds -// a second bob, so the recompile resolves charlie's grant and then fails on -// the ambiguous nodeAttrs target. A failed Set* must leave every compiled result as it was: +// TestFailedSetUsersKeepsCompiledPolicy renames alice to charlie while +// an unresolvable tag owner prevents the recompile from succeeding. +// A failed Set* must leave every compiled result as it was: // a partial compile would hand alice's nodes charlie's port 22, and the // reverse rename, which SetUsers sees as no change, would not clear it. func TestFailedSetUsersKeepsCompiledPolicy(t *testing.T) { @@ -4061,9 +4063,13 @@ func TestFailedSetUsersKeepsCompiledPolicy(t *testing.T) { renamed := slices.Clone(users) renamed[0].Name = "charlie" - renamed = append(renamed, types.User{ID: 3, Name: "bob"}) + good := pm.pol.TagOwners + missing := Tag("tag:missing") + pm.pol.TagOwners = TagOwners{"tag:srv": Owners{&missing}} _, _, err = pm.SetUsers(renamed) + pm.pol.TagOwners = good + require.Error(t, err) require.Equal(t, before, read(pm, nodes[0]), "a failed SetUsers must not change compiled results") @@ -4545,8 +4551,8 @@ func TestWithResolvedGroups(t *testing.T) { } } - require.ErrorIs(t, - resolved.resolvedGroups["group:ambiguous"].err, ErrMultipleUsersFound) + // An ambiguous member matches no one, as outside a policy write. + require.NoError(t, resolved.resolvedGroups["group:ambiguous"].err) unknown := Group("group:unknown") got, err := unknown.resolve(resolved, users, nodes) @@ -4714,3 +4720,52 @@ func TestSetNodesUnapprovedRoutesKeepGeneration(t *testing.T) { }) } } + +func TestAmbiguousUserKeepsOtherGroupMembers(t *testing.T) { + users := types.Users{ + {ID: 1, Name: "alice"}, + {ID: 2, Name: "dup"}, + {ID: 3, Name: "dup"}, + } + + nodes := types.Nodes{ + node("alice", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("first-dup", "100.64.0.2", "fd7a:115c:a1e0::2", users[1]), + node("second-dup", "100.64.0.3", "fd7a:115c:a1e0::3", users[2]), + } + for i, n := range nodes { + n.ID = types.NodeID(i + 1) //nolint:gosec + } + + pol := []byte(`{ + "groups": {"group:members": ["alice@", "dup@"]}, + "acls": [{"action":"accept", "src":["group:members"], "dst":["*:*"]}], + "nodeAttrs": [{"target":["group:members"], "attr":["randomize-client-port"]}] + }`) + pm, err := NewPolicyManager(pol, users[:2], nodes.ViewSlice()) + require.NoError(t, err) + require.NotEmpty(t, pm.NodeCapMap(nodes[0].ID)) + require.NotEmpty(t, pm.NodeCapMap(nodes[1].ID)) + require.Empty(t, pm.NodeCapMap(nodes[2].ID)) + + _, usersChanged, err := pm.SetUsers(users) + require.NoError(t, err) + require.True(t, usersChanged) + + filter, _ := pm.Filter() + require.Len(t, filter, 1) + require.ElementsMatch(t, []string{"100.64.0.1", "fd7a:115c:a1e0::1"}, filter[0].SrcIPs) + require.Equal(t, tailcfg.NodeCapMap{"randomize-client-port": nil}, pm.NodeCapMap(nodes[0].ID)) + require.Empty(t, pm.NodeCapMap(nodes[1].ID)) + require.Empty(t, pm.NodeCapMap(nodes[2].ID)) + + fresh, err := NewPolicyManager(pol, users, nodes.ViewSlice()) + require.NoError(t, err) + + freshFilter, _ := fresh.Filter() + require.Equal(t, filter, freshFilter) + require.Equal(t, pm.NodeCapMaps(), fresh.NodeCapMaps()) + + _, err = pm.SetPolicy(pol) + require.ErrorIs(t, err, ErrMultipleUsersFound) +} diff --git a/hscontrol/policy/v2/tailscale_users_compat_test.go b/hscontrol/policy/v2/tailscale_users_compat_test.go index 372b8a2a0..c3a0a5202 100644 --- a/hscontrol/policy/v2/tailscale_users_compat_test.go +++ b/hscontrol/policy/v2/tailscale_users_compat_test.go @@ -111,16 +111,6 @@ var userTokenSkipReasons = map[string]string{ // The evaluation error repeats the test's source unquoted, here a user's name. "users-bare-at-test-dst/name-is-owner-email/user-list": testUnknownAlias, - "users-bare-at-dst/no-email-two/boot": ambiguousLoad, - "users-bare-at-group/no-email-two/boot": ambiguousLoad, - "users-bare-at-nodeattrs/no-email-two/boot": ambiguousLoad, - "users-bare-at-nodeattrs/no-email-two/runtime": ambiguousLoad, - "users-bare-at-src/no-email-two/boot": ambiguousLoad, - "users-bare-at-tagowner/no-email-two/boot": ambiguousLoad, - "users-unknown-direct/duplicate-unknown-email/boot": ambiguousLoad, - "users-unknown-member/duplicate-unknown-email/boot": ambiguousLoad, - "users-unknown-member/duplicate-unknown-email/runtime": ambiguousLoad, - "users-unknown-direct/duplicate-unknown-email/verdict": ambiguousWrite, "users-unknown-member/duplicate-unknown-email/verdict": ambiguousWrite, @@ -170,7 +160,6 @@ var userTokenSkipReasons = map[string]string{ "users-bare-at-tagowner/no-email-one/overlay-1/tag-owner": bareAtNoEmail, "users-bare-at-tagowner/no-email-two/user-list": bareAtNoEmail, "users-bare-at-tagowner/no-email-two/verdict": bareAtNoEmail, - "users-bare-at-test-dst/no-email-two/user-list": bareAtNoEmail, "users-bare-at-test-src/no-email-two/user-list": bareAtNoEmail, "users-bare-at-test-src/okta-name-only/verdict": bareAtNoEmail, @@ -180,7 +169,6 @@ var userTokenSkipReasons = map[string]string{ const ( shortUsername = "HEADSCALE_SHORT_USERNAME: headscale lets odin@ name the user called odin; SaaS matches it to no one" shortUsernameRow = "HEADSCALE_SHORT_USERNAME: a row storing the address as its name leaves odin@ nothing to match" - ambiguousLoad = "AMBIGUOUS_USER_LOAD: a user token matching two users stops the policy loading; SaaS keeps serving it" ambiguousWrite = "AMBIGUOUS_USER_WRITE: headscale refuses to store a policy whose user token matches two users (#3160)" ambiguityListsUsers = "AMBIGUITY_LISTS_USERS: headscale's refusal of a token matching two users lists both users by ID and name" usernameAnswersEmail = "USERNAME_ANSWERS_EMAIL_TOKEN: a username shaped like an unclaimed address takes that address's rights" diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 3802d9605..6ac28c700 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -484,21 +484,14 @@ func (u *Username) Resolve(_ *Policy, users types.Users, nodes views.Slice[types } func (u *Username) resolve(_ *Policy, users types.Users, nodes views.Slice[types.NodeView]) (*netipx.IPSet, error) { - var ( - ips netipx.IPSetBuilder - errs []error - ) + var ips netipx.IPSetBuilder user, err := u.resolveUser(users) - if errors.Is(err, ErrUserNotFound) { - // A policy may name users before they register (#2863). Report it - // as an empty set, not an error: callers that bail on error would - // otherwise drop a whole group for one missing member (#3513). - return &netipx.IPSet{}, nil - } - if err != nil { - errs = append(errs, err) + // Users may be missing before registration (#2863), or become + // ambiguous after a login. An empty set preserves other group + // members (#3513); the policy write boundary rejects ambiguity. + return &netipx.IPSet{}, nil //nolint:nilerr // Unresolved users intentionally match no nodes. } for _, node := range nodes.All() { @@ -517,7 +510,7 @@ func (u *Username) resolve(_ *Policy, users types.Users, nodes views.Slice[types } } - return buildIPSetMultiErr(&ips, errs) + return ips.IPSet() } // Group is a special string which is always prefixed with `group:`. diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 9cdde85cd..3580eb404 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1,6 +1,7 @@ package state import ( + "database/sql" "encoding/json" "errors" "maps" @@ -2611,3 +2612,48 @@ func TestUpdateUserRefreshesNodeCopiesWhenPolicyUpdateFails(t *testing.T) { assert.Equal(t, "renamed-user", updated.User().Name(), "the committed row and its cached copy must agree") } + +func TestUserNameCollisionDoesNotBlockCreationOrRestart(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("dup") + node := database.CreateRegisteredNodeForTest(user, "duplicate-name-node") + _, err = database.SetPolicy(`{ + "acls": [], + "nodeAttrs": [{"target":["dup@"], "attr":["randomize-client-port"]}] + }`) + require.NoError(t, err) + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { + if s != nil { + _ = s.Close() + } + }) + require.NotEmpty(t, s.polMan.NodeCapMap(node.ID)) + + created, _, err := s.CreateUser(types.User{ + Name: "dup", + Provider: "oidc", + ProviderIdentifier: sql.NullString{String: "https://idp.example/second", Valid: true}, + }) + require.NoError(t, err, "a colliding user must not prevent user creation") + require.NotNil(t, created) + + _, _, err = s.CreateUser(types.User{Name: "later"}) + require.NoError(t, err, "a previous collision must not prevent later user creation") + assert.Empty(t, s.polMan.NodeCapMap(node.ID)) + require.NoError(t, s.Close()) + s = nil + + reopened := persistTestReopen(t, dbPath) + users, err := reopened.ListAllUsers() + require.NoError(t, err) + require.Len(t, users, 3) + require.Empty(t, reopened.polMan.NodeCapMap(node.ID)) +}