From bf5007add8b0d47889b0d902299fed3a74ccdc1e Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 11:11:02 +0000 Subject: [PATCH] policy/v2: prefer the user owning an email over a username match --- docs/ref/oidc.md | 7 ++ .../policy/v2/tailscale_users_compat_test.go | 43 +------- hscontrol/policy/v2/types.go | 19 +++- hscontrol/policy/v2/types_test.go | 97 +++++++++++++++++++ 4 files changed, 123 insertions(+), 43 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/tailscale_users_compat_test.go b/hscontrol/policy/v2/tailscale_users_compat_test.go index 8db392f3d..372b8a2a0 100644 --- a/hscontrol/policy/v2/tailscale_users_compat_test.go +++ b/hscontrol/policy/v2/tailscale_users_compat_test.go @@ -99,33 +99,27 @@ var userTokenSkipReasons = map[string]string{ "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/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-owner-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-dst/name-is-owner-email/boot": 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-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, "users-unknown-direct/duplicate-unknown-email/verdict": ambiguousWrite, "users-unknown-member/duplicate-unknown-email/verdict": ambiguousWrite, @@ -133,32 +127,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, @@ -215,7 +183,6 @@ const ( 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` ) diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 3852bc963..3802d9605 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -434,13 +434,13 @@ 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 @@ -454,9 +454,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 { diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index a2996e96b..7ed5c6d71 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,98 @@ 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) + }) + } + } +}