From 010a806526faf44fba5d10db89f272acdcf3ea0d Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 11:03:12 +0000 Subject: [PATCH] policy/v2: resolve user references consistently across updates Prefer email owners over usernames and validate grants and nodeAttrs on write. Reject ambiguous writes; log and match nobody on reload. Bare @ matches nobody. --- docs/ref/oidc.md | 7 + hscontrol/policy/v2/compiled_test.go | 5 +- hscontrol/policy/v2/policy.go | 26 ++- hscontrol/policy/v2/policy_test.go | 143 ++++++++++------ .../policy/v2/tailscale_users_compat_test.go | 103 ++--------- hscontrol/policy/v2/types.go | 41 +++-- hscontrol/policy/v2/types_test.go | 160 ++++++++++++++++++ hscontrol/state/persist_test.go | 46 +++++ 8 files changed, 375 insertions(+), 156 deletions(-) diff --git a/docs/ref/oidc.md b/docs/ref/oidc.md index c3013dc6c..32f3f5681 100644 --- a/docs/ref/oidc.md +++ b/docs/ref/oidc.md @@ -185,6 +185,11 @@ You may refer to users in the Headscale policy via: - Provider identifier (this value is currently only available from the [API](api.md), database or directly from your identity provider) +References match a provider identifier first, then a stored email address, then a username. A token shaped like an +email address also matches a username with that value when no user has that email. If one user has the email and +another has the username, the email owner wins. Multiple matching emails, or multiple matching usernames when no email +matches, remain ambiguous. + !!! note "A user identifier in the policy must contain a single `@`" The Headscale policy requires a single `@` to reference a user. If the username or provider identifier doesn't @@ -200,6 +205,8 @@ You may refer to users in the Headscale policy via: consequences for Headscale where a policy might no longer work or a user might obtain more access by hijacking an existing username or email address. + Use provider identifiers in policies when your identity provider lets users change their own usernames. + !!! tip "Howto use the provider identifier in the policy" The provider identifier uniquely identifies an OIDC user and a well-behaving identity provider guarantees that this 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 1faa25781..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 { @@ -207,6 +206,22 @@ func validateUserReferences(pol *Policy, users types.Users) error { } } + for _, grant := range pol.Grants { + for _, src := range grant.Sources { + checkAlias(src) + } + + for _, dst := range grant.Destinations { + checkAlias(dst) + } + } + + for _, attr := range pol.NodeAttrs { + for _, target := range attr.Targets { + checkAlias(target) + } + } + for _, ssh := range pol.SSHs { for _, src := range ssh.Sources { checkAlias(src) @@ -231,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{ @@ -1106,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 ccf644d68..f1fd53f2c 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -96,44 +96,6 @@ func TestPolicyManager(t *testing.T) { } } -// TestSetPolicyRejectedKeepsLiveFilter pins that a policy SetPolicy rejects -// does not take effect. A nodeAttrs target naming an ambiguous user passes -// validation but fails to compile after the filter is already compiled; keeping that -// filter would run the rejected policy while the stored one is unchanged. -func TestSetPolicyRejectedKeepsLiveFilter(t *testing.T) { - // IDs are assigned after construction so the test also builds where - // types.User embeds gorm.Model. - users := types.Users{{Name: "user1"}, {Name: "user2"}, {Name: "dup"}, {Name: "dup"}} - users[0].ID, users[1].ID, users[2].ID, users[3].ID = 1, 2, 3, 4 - nodes := types.Nodes{ - node("n1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), - node("n2", "100.64.0.2", "fd7a:115c:a1e0::2", users[1]), - } - nodes[0].ID, nodes[1].ID = 1, 2 - - pm, err := NewPolicyManager([]byte(`{ - "acls": [{"action": "accept", "src": ["user1@"], "dst": ["user1@:*"]}] - }`), users, nodes.ViewSlice()) - require.NoError(t, err) - - before, _ := pm.Filter() - beforeRules, err := pm.FilterForNode(nodes[1].View()) - require.NoError(t, err) - - _, err = pm.SetPolicy([]byte(`{ - "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}], - "nodeAttrs": [{"target": ["dup@"], "attr": ["randomize-client-port"]}] - }`)) - require.Error(t, err) - - after, _ := pm.Filter() - require.Equal(t, before, after, "a rejected policy must not replace the live filter") - - afterRules, err := pm.FilterForNode(nodes[1].View()) - require.NoError(t, err) - require.Equal(t, beforeRules, afterRules) -} - func TestInvalidateAutogroupSelfCache(t *testing.T) { users := types.Users{ {ID: 1, Name: "user1", Email: "user1@headscale.net"}, @@ -2900,14 +2862,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"}, @@ -3211,6 +3175,8 @@ func TestValidateUserReferences_AllSites(t *testing.T) { {ID: 2, Name: "dup"}, {ID: 3, Name: "dup"}, } + nodes := types.Nodes{node("alice", "100.64.0.1", "fd7a:115c:a1e0::1", users[0])} + nodes[0].ID = 1 tests := []struct { name string @@ -3254,6 +3220,18 @@ func TestValidateUserReferences_AllSites(t *testing.T) { name: "acls.dst", pol: `{ "acls": [{"action":"accept","src":["alice@"],"dst":["dup@:*"]}] +}`, + }, + { + name: "grants.src", + pol: `{ + "grants": [{"src":["dup@"],"dst":["*"],"ip":["*"]}] +}`, + }, + { + name: "grants.dst", + pol: `{ + "grants": [{"src":["alice@"],"dst":["dup@"],"ip":["*"]}] }`, }, { @@ -3283,9 +3261,25 @@ func TestValidateUserReferences_AllSites(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := NewPolicyManager([]byte(tt.pol), users, types.Nodes{}.ViewSlice()) + pol, err := unmarshalPolicy([]byte(tt.pol)) + require.NoError(t, err) + + err = validateUserReferences(pol, users) require.Error(t, err, "site %q must surface duplicate-user errors", tt.name) require.ErrorIs(t, err, ErrMultipleUsersFound) + + pm, err := NewPolicyManager([]byte(`{ + "acls": [{"action":"accept","src":["alice@"],"dst":["alice@:*"]}] +}`), users, nodes.ViewSlice()) + require.NoError(t, err) + + before, _ := pm.Filter() + + _, err = pm.SetPolicy([]byte(tt.pol)) + require.ErrorIs(t, err, ErrMultipleUsersFound) + + after, _ := pm.Filter() + require.Equal(t, before, after, "a rejected policy must not replace the live filter") }) } } @@ -4021,9 +4015,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) { @@ -4071,9 +4065,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") @@ -4642,8 +4640,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) @@ -4811,3 +4809,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 8db392f3d..a4154da3c 100644 --- a/hscontrol/policy/v2/tailscale_users_compat_test.go +++ b/hscontrol/policy/v2/tailscale_users_compat_test.go @@ -97,35 +97,25 @@ var userTokenOverlays = []userTokenOverlay{ var userTokenSkipReasons = map[string]string{ "users-short-name/okta-name-only/server/filter": shortUsernameRow, - "users-bare-at-test-dst/breakdown": testUnknownAlias, - "users-bare-at-test-dst/duplicate-unknown-email/breakdown": testUnknownAlias, - "users-bare-at-test-dst/name-is-unknown-email/breakdown": testUnknownAlias, - "users-bare-at-test-dst/no-email-two/breakdown": testUnknownAlias, - "users-bare-at-test-src/breakdown": testUnknownAlias, - "users-bare-at-test-src/duplicate-unknown-email/breakdown": testUnknownAlias, - "users-bare-at-test-src/name-is-unknown-email/breakdown": testUnknownAlias, - "users-bare-at-test-src/no-email-two/breakdown": testUnknownAlias, + "users-bare-at-test-dst/breakdown": testUnknownAlias, + "users-bare-at-test-dst/duplicate-unknown-email/breakdown": testUnknownAlias, + "users-bare-at-test-dst/name-is-owner-email/breakdown": testUnknownAlias, + "users-bare-at-test-dst/name-is-unknown-email-unverified/breakdown": testUnknownAlias, + "users-bare-at-test-dst/name-is-unknown-email/breakdown": testUnknownAlias, + "users-bare-at-test-dst/no-email-one/breakdown": testUnknownAlias, + "users-bare-at-test-dst/no-email-two/breakdown": testUnknownAlias, + "users-bare-at-test-dst/okta-name-only/breakdown": testUnknownAlias, + "users-bare-at-test-src/breakdown": testUnknownAlias, + "users-bare-at-test-src/duplicate-unknown-email/breakdown": testUnknownAlias, + "users-bare-at-test-src/name-is-owner-email/breakdown": testUnknownAlias, + "users-bare-at-test-src/name-is-unknown-email-unverified/breakdown": testUnknownAlias, + "users-bare-at-test-src/name-is-unknown-email/breakdown": testUnknownAlias, + "users-bare-at-test-src/no-email-one/breakdown": testUnknownAlias, + "users-bare-at-test-src/no-email-two/breakdown": testUnknownAlias, + "users-bare-at-test-src/okta-name-only/breakdown": testUnknownAlias, - "users-bare-at-dst/name-is-owner-email/boot": ambiguousLoad, - "users-bare-at-dst/no-email-two/boot": ambiguousLoad, - "users-bare-at-group/name-is-owner-email/boot": ambiguousLoad, - "users-bare-at-group/no-email-two/boot": ambiguousLoad, - "users-bare-at-nodeattrs/name-is-owner-email/boot": ambiguousLoad, - "users-bare-at-nodeattrs/no-email-two/boot": ambiguousLoad, - "users-bare-at-nodeattrs/no-email-two/runtime": ambiguousLoad, - "users-bare-at-src/name-is-owner-email/boot": ambiguousLoad, - "users-bare-at-src/no-email-two/boot": ambiguousLoad, - "users-bare-at-tagowner/name-is-owner-email/boot": ambiguousLoad, - "users-bare-at-tagowner/no-email-two/boot": ambiguousLoad, - "users-bare-at-test-dst/name-is-owner-email/boot": ambiguousLoad, - "users-bare-at-test-src/name-is-owner-email/boot": ambiguousLoad, - "users-short-name/name-is-owner-email/boot": ambiguousLoad, - "users-unknown-direct/duplicate-unknown-email/boot": ambiguousLoad, - "users-unknown-direct/name-is-owner-email/boot": ambiguousLoad, - "users-unknown-member/duplicate-unknown-email/boot": ambiguousLoad, - "users-unknown-member/duplicate-unknown-email/runtime": ambiguousLoad, - "users-unknown-member/name-is-owner-email/boot": ambiguousLoad, - "users-unknown-member/name-is-owner-email/runtime": ambiguousLoad, + // 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-unknown-direct/duplicate-unknown-email/verdict": ambiguousWrite, "users-unknown-member/duplicate-unknown-email/verdict": ambiguousWrite, @@ -133,32 +123,6 @@ var userTokenSkipReasons = map[string]string{ "users-unknown-direct/duplicate-unknown-email/user-list": ambiguityListsUsers, "users-unknown-member/duplicate-unknown-email/user-list": ambiguityListsUsers, - "users-bare-at-dst/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-bare-at-group/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-bare-at-nodeattrs/name-is-owner-email/server/filter": emailOwnerAmbiguous, - "users-bare-at-nodeattrs/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-bare-at-src/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-bare-at-tagowner/name-is-owner-email/server/filter": emailOwnerAmbiguous, - "users-bare-at-tagowner/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-bare-at-test-dst/name-is-owner-email/breakdown": emailOwnerAmbiguous, - "users-bare-at-test-dst/name-is-owner-email/message": emailOwnerAmbiguous, - "users-bare-at-test-src/name-is-owner-email/breakdown": emailOwnerAmbiguous, - "users-bare-at-test-src/name-is-owner-email/message": emailOwnerAmbiguous, - "users-short-name/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-unknown-direct/name-is-owner-email/verdict": emailOwnerAmbiguous, - "users-unknown-member/name-is-owner-email/verdict": emailOwnerAmbiguous, - - "users-bare-at-dst/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-bare-at-group/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-bare-at-nodeattrs/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-bare-at-src/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-bare-at-tagowner/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-bare-at-test-dst/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-bare-at-test-src/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-short-name/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-unknown-direct/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-unknown-member/name-is-owner-email/user-list": emailOwnerAmbiguous, - "users-unknown-direct/name-is-unknown-email-unverified/overlay-1/filter": usernameAnswersEmail, "users-unknown-direct/name-is-unknown-email-unverified/overlay-1/route-approver": usernameAnswersEmail, "users-unknown-direct/name-is-unknown-email-unverified/overlay-1/tag-owner": usernameAnswersEmail, @@ -178,46 +142,15 @@ var userTokenSkipReasons = map[string]string{ "users-unknown-member/name-is-unknown-email/server/filter": usernameAnswersEmail, "users-unknown-member/name-is-unknown-email/server/ssh": usernameAnswersEmail, - "users-bare-at-dst/name-is-unknown-email-unverified/overlay-1/filter": bareAtNoEmail, - "users-bare-at-dst/no-email-one/overlay-1/filter": bareAtNoEmail, - "users-bare-at-dst/no-email-two/user-list": bareAtNoEmail, - "users-bare-at-dst/no-email-two/verdict": bareAtNoEmail, - "users-bare-at-dst/okta-name-only/user-tid/filter": bareAtNoEmail, - "users-bare-at-group/name-is-unknown-email-unverified/server/filter": bareAtNoEmail, - "users-bare-at-group/no-email-one/server/filter": bareAtNoEmail, - "users-bare-at-group/no-email-two/user-list": bareAtNoEmail, - "users-bare-at-group/no-email-two/verdict": bareAtNoEmail, - "users-bare-at-group/okta-name-only/server/filter": bareAtNoEmail, - "users-bare-at-nodeattrs/name-is-unknown-email-unverified/overlay-1/capmap": bareAtNoEmail, - "users-bare-at-nodeattrs/no-email-one/overlay-1/capmap": bareAtNoEmail, - "users-bare-at-nodeattrs/no-email-two/user-list": bareAtNoEmail, - "users-bare-at-nodeattrs/no-email-two/verdict": bareAtNoEmail, - "users-bare-at-nodeattrs/okta-name-only/user-tid/capmap": bareAtNoEmail, - "users-bare-at-src/name-is-unknown-email-unverified/server/filter": bareAtNoEmail, - "users-bare-at-src/no-email-one/server/filter": bareAtNoEmail, - "users-bare-at-src/no-email-two/user-list": bareAtNoEmail, - "users-bare-at-src/no-email-two/verdict": bareAtNoEmail, - "users-bare-at-src/okta-name-only/server/filter": bareAtNoEmail, - "users-bare-at-tagowner/name-is-unknown-email-unverified/overlay-1/tag-owner": bareAtNoEmail, - "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, - "users-short-name/server/filter": shortUsername, } 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" - emailOwnerAmbiguous = "EMAIL_OWNER_FIRST: a username equal to another user's e-mail makes that e-mail match two users" usernameAnswersEmail = "USERNAME_ANSWERS_EMAIL_TOKEN: a username shaped like an unclaimed address takes that address's rights" - bareAtNoEmail = `BARE_AT_NO_EMAIL: "@" matches the users without an e-mail; SaaS matches it to no one` ) // TestUserTokenCompat replays every users capture as captured and under diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 3852bc963..9bbdcaf94 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -434,19 +434,22 @@ func (u *Username) CanBeAutoApprover() bool { } // resolveUser attempts to find a user in the provided [types.Users] slice that matches the [Username]. -// It prioritizes matching the [types.User.ProviderIdentifier], and if not found, it falls back to matching -// the [types.User.Email] or [types.User.Name]. +// It prioritizes matching the [types.User.ProviderIdentifier], then the [types.User.Email], +// and falls back to the [types.User.Name] only when no email matches. // If no matching user is found, it returns an error indicating no user matching. // If multiple matching users are found, it returns an error indicating multiple users matching. // It returns the matched [types.User] and a nil error if exactly one match is found. func (u *Username) resolveUser(users types.Users) (types.User, error) { - var potentialUsers types.Users + var emailMatches, nameMatches types.Users // At parsetime, we require all usernames to contain an "@" character, if the // username token does not naturally do so (like email), the user have to // add it to the end of the username. We strip it here as we do not expect the // usernames to be stored with the "@". uTrimmed := strings.TrimSuffix(u.String(), "@") + if uTrimmed == "" { + return types.User{}, fmt.Errorf("%w: token %q", ErrUserNotFound, u.String()) + } for _, user := range users { if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == uTrimmed { @@ -454,9 +457,18 @@ func (u *Username) resolveUser(users types.Users) (types.User, error) { return user, nil } - if user.Email == uTrimmed || user.Name == uTrimmed { - potentialUsers = append(potentialUsers, user) + if user.Email == uTrimmed { + emailMatches = append(emailMatches, user) } + + if user.Name == uTrimmed { + nameMatches = append(nameMatches, user) + } + } + + potentialUsers := emailMatches + if len(potentialUsers) == 0 { + potentialUsers = nameMatches } if len(potentialUsers) == 0 { @@ -475,21 +487,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() { @@ -508,7 +513,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/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index a2996e96b..747b641cc 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -2,9 +2,11 @@ package v2 import ( "bytes" + "database/sql" "encoding/json" "fmt" "net/netip" + "slices" "strings" "testing" "time" @@ -6485,3 +6487,161 @@ func TestUnmarshalPolicyMaxDstPorts(t *testing.T) { }) } } + +func TestResolveUserPrecedence(t *testing.T) { + const ( + address = "alice@example.com" + providerID = "https://idp.example/alice" + ) + + owner := types.User{ID: 1, Name: "alice", Email: address} + oidcName := types.User{ID: 2, Name: address, Provider: "oidc"} + localName := types.User{ID: 3, Name: address} + both := types.User{ID: 4, Name: address, Email: address} + provider := types.User{ + ID: 5, + Name: "provider-user", + ProviderIdentifier: sql.NullString{String: providerID, Valid: true}, + } + + tests := []struct { + name string + token Username + users types.Users + want types.User + wantErr error + }{ + { + name: "email owner beats OIDC username", + token: Username(address), + users: types.Users{owner, oidcName}, + want: owner, + }, + { + name: "email owner beats local username", + token: Username(address), + users: types.Users{owner, localName}, + want: owner, + }, + { + name: "address-shaped username without email owner", + token: Username(address), + users: types.Users{oidcName, {ID: 6, Name: "other", Email: "other@example.com"}}, + want: oidcName, + }, + { + name: "email and username on the same user count once", + token: Username(address), + users: types.Users{both, oidcName}, + want: both, + }, + { + name: "duplicate usernames remain ambiguous", + token: Username(address), + users: types.Users{oidcName, localName}, + wantErr: ErrMultipleUsersFound, + }, + { + name: "duplicate emails remain ambiguous despite a unique username", + token: Username(address), + users: types.Users{owner, {ID: 7, Name: "second-owner", Email: address}, oidcName}, + wantErr: ErrMultipleUsersFound, + }, + { + name: "provider identifier wins", + token: Username(providerID + "@"), + users: types.Users{provider, {ID: 8, Name: providerID}}, + want: provider, + }, + { + name: "unknown user", + token: "missing@", + users: types.Users{owner, oidcName, localName}, + wantErr: ErrUserNotFound, + }, + } + + for _, tt := range tests { + for _, reverse := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/reverse=%t", tt.name, reverse), func(t *testing.T) { + users := slices.Clone(tt.users) + if reverse { + slices.Reverse(users) + } + + got, err := tt.token.resolveUser(users) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + + return + } + + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } + } +} + +func TestBareAtTokenMatchesNobody(t *testing.T) { + users := types.Users{ + {ID: 1, Name: "first-private-user"}, + {ID: 2, Name: "second-private-user"}, + } + + for count := range 3 { + t.Run(fmt.Sprintf("users=%d", count), func(t *testing.T) { + token := Username("@") + _, err := token.resolveUser(users[:count]) + require.ErrorIs(t, err, ErrUserNotFound) + + for _, user := range users { + require.NotContains(t, err.Error(), user.Name) + } + + pm, err := NewPolicyManager([]byte(`{ + "grants": [{"src":["@"], "dst":["*"], "ip":["*"]}] + }`), users[:count], types.Nodes{}.ViewSlice()) + require.NoError(t, err, "stored policies with a bare @ must still load") + + filter, _ := pm.Filter() + require.Empty(t, filter) + }) + } +} + +func TestSetPolicyBareAtToken(t *testing.T) { + users := types.Users{ + {ID: 1, Name: "first-private-user"}, + {ID: 2, Name: "second-private-user"}, + } + + nodes := types.Nodes{node("first-node", "100.64.0.1", "fd7a:115c:a1e0::1", users[0])} + nodes[0].ID = 1 + + const allowAll = `"acls": [{"action":"accept", "src":["*"], "dst":["*:*"]}]` + + for _, count := range []int{1, 2} { + for _, tt := range []struct { + name string + src string + dst string + }{ + {name: "source", src: "@", dst: "100.64.0.1:22"}, + {name: "destination", src: "100.64.0.1", dst: "@:22"}, + } { + t.Run(fmt.Sprintf("%s/users=%d", tt.name, count), func(t *testing.T) { + pm, err := NewPolicyManager([]byte(`{`+allowAll+`}`), users[:count], nodes.ViewSlice()) + require.NoError(t, err) + + candidate := fmt.Sprintf(`{%s, "tests":[{"src":%q, "accept":[%q]}]}`, allowAll, tt.src, tt.dst) + _, err = pm.SetPolicy([]byte(candidate)) + require.ErrorIs(t, err, errPolicyTestsFailed) + + for _, user := range users { + require.NotContains(t, err.Error(), user.Name) + } + }) + } + } +} diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 57c06adbc..b4fb04631 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" @@ -2775,3 +2776,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)) +}