From 78214699ad61e19f3d21b538e73bbff363025877 Mon Sep 17 00:00:00 2001
From: Kristoffer Dalby <kristoffer@tailscale.com>
Date: Fri, 4 Oct 2024 12:24:35 +0200
Subject: [PATCH] Harden OIDC migration and make optional

This commit hardens the migration part of the OIDC from
the old username based approach to the new sub based approach
and makes it possible for the operator to opt out entirely.

Fixes #1990

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
---
 config-example.yaml       | 18 ++++++++++++------
 hscontrol/oidc.go         | 13 ++++++++++++-
 hscontrol/types/config.go |  3 +++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/config-example.yaml b/config-example.yaml
index a22226a1..93204398 100644
--- a/config-example.yaml
+++ b/config-example.yaml
@@ -369,12 +369,18 @@ unix_socket_permission: "0770"
 #   allowed_users:
 #     - alice@example.com
 #
-#   # If `strip_email_domain` is set to `true`, the domain part of the username email address will be removed.
-#   # This will transform `first-name.last-name@example.com` to the user `first-name.last-name`
-#   # If `strip_email_domain` is set to `false` the domain part will NOT be removed resulting to the following
-#   user: `first-name.last-name.example.com`
-#
-#   strip_email_domain: true
+#   # Map legacy users from pre-0.24.0 versions of headscale to the new OIDC users
+#   # by taking the username from the legacy user and matching it with the username
+#   # provided by the OIDC. This is useful when migrating from legacy users to OIDC
+#   # to force them using the unique identifier from the OIDC and to give them a
+#   # proper display name and picture if available.
+#   # Note that this will only work if the username from the legacy user is the same
+#   # and ther is a posibility for account takeover should a username have changed
+#   # with the provider.
+#   # Disabling this feature will cause all new logins to be created as new users.
+#   # Note this option will be removed in the future and should be set to false
+#   # on all new installations, or when all users have logged in with OIDC once.
+#   map_legacy_users: true
 
 # Logtail configuration
 # Logtail is Tailscales logging and auditing infrastructure, it allows the control panel
diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go
index 10008e67..a4775ae8 100644
--- a/hscontrol/oidc.go
+++ b/hscontrol/oidc.go
@@ -443,7 +443,9 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim(
 
 	// This check is for legacy, if the user cannot be found by the OIDC identifier
 	// look it up by username. This should only be needed once.
-	if user == nil {
+	// This branch will presist for a number of versions after the OIDC migration and
+	// then be removed following a deprecation.
+	if a.cfg.MapLegacyUsers && user == nil {
 		user, err = a.db.GetUserByName(claims.Username)
 		if err != nil && !errors.Is(err, db.ErrUserNotFound) {
 			return nil, fmt.Errorf("creating or updating user: %w", err)
@@ -453,6 +455,15 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim(
 		if user == nil {
 			user = &types.User{}
 		}
+
+		// If the user exists, but it already has a provider identifier (OIDC sub), create a new user.
+		// This is to prevent users that have already been migrated to the new OIDC format
+		// to be updated with the new OIDC identifier inexplicitly which might be the cause of an
+		// account takeover.
+		if user.ProviderIdentifier != "" {
+			log.Info().Str("username", claims.Username).Str("sub", claims.Sub).Msg("user found by username, but has provider identifier, creating new user.")
+			user = &types.User{}
+		}
 	}
 
 	user.FromClaim(claims)
diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go
index 3d6abff7..07131a18 100644
--- a/hscontrol/types/config.go
+++ b/hscontrol/types/config.go
@@ -166,6 +166,7 @@ type OIDCConfig struct {
 	AllowedGroups              []string
 	Expiry                     time.Duration
 	UseExpiryFromToken         bool
+	MapLegacyUsers             bool
 }
 
 type DERPConfig struct {
@@ -278,6 +279,7 @@ func LoadConfig(path string, isFile bool) error {
 	viper.SetDefault("oidc.only_start_if_oidc_is_available", true)
 	viper.SetDefault("oidc.expiry", "180d")
 	viper.SetDefault("oidc.use_expiry_from_token", false)
+	viper.SetDefault("oidc.map_legacy_users", true)
 
 	viper.SetDefault("logtail.enabled", false)
 	viper.SetDefault("randomize_client_port", false)
@@ -900,6 +902,7 @@ func LoadServerConfig() (*Config, error) {
 				}
 			}(),
 			UseExpiryFromToken: viper.GetBool("oidc.use_expiry_from_token"),
+			MapLegacyUsers:     viper.GetBool("oidc.map_legacy_users"),
 		},
 
 		LogTail:             logTailConfig,