remove oidc migration (#2411)

* remove oidc migration Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> * update changelog Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> --------- Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
2025-02-10 16:48:05 -05:00 · 2025-02-07 13:49:45 +01:00 · 2025-02-07 13:49:45 +01:00 · b92bd3d27e
commit b92bd3d27e
parent 3bf7d5a9c9
7 changed files with 12 additions and 231 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -3,6 +3,12 @@
 ## Next


+### Changes
+
+- `oidc.map_legacy_users` and `oidc.strip_email_domain` has been removed
+  [#2411](https://github.com/juanfont/headscale/pull/2411)
+
+
 ## 0.25.0 (2025-02-xx)

 ### BREAKING
--- a/docs/ref/oidc.md
+++ b/docs/ref/oidc.md
@ -56,12 +56,6 @@ oidc:
    # - plain: Use plain code verifier
    # - S256: Use SHA256 hashed code verifier (default, recommended)
    method: S256
-
-  # 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
 ```

 ## Azure AD example
--- a/hscontrol/oidc.go
+++ b/hscontrol/oidc.go
@ -442,32 +442,6 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim(
 		return nil, fmt.Errorf("creating or updating user: %w", err)
 	}

-	// 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.
-	// This branch will persist for a number of versions after the OIDC migration and
-	// then be removed following a deprecation.
-	// TODO(kradalby): Remove when strip_email_domain and migration is removed
-	// after #2170 is cleaned up.
-	if a.cfg.MapLegacyUsers && user == nil {
-		log.Trace().Str("username", claims.Username).Str("sub", claims.Sub).Msg("user not found by OIDC identifier, looking up by username")
-		if oldUsername, err := getUserName(claims, a.cfg.StripEmaildomain); err == nil {
-			log.Trace().Str("old_username", oldUsername).Str("sub", claims.Sub).Msg("found username")
-			user, err = a.db.GetUserByName(oldUsername)
-			if err != nil && !errors.Is(err, db.ErrUserNotFound) {
-				return nil, fmt.Errorf("getting user: %w", err)
-			}
-
-			// 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 != nil && user.ProviderIdentifier.Valid {
-				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{}
-			}
-		}
-	}
-
 	// if the user is still not found, create a new empty user.
 	if user == nil {
 		user = &types.User{}
@ -548,27 +522,6 @@ func renderOIDCCallbackTemplate(
 	return &content, nil
 }

-// TODO(kradalby): Reintroduce when strip_email_domain is removed
-// after #2170 is cleaned up
-// DEPRECATED: DO NOT USE.
-func getUserName(
-	claims *types.OIDCClaims,
-	stripEmaildomain bool,
-) (string, error) {
-	if !claims.EmailVerified {
-		return "", fmt.Errorf("email not verified")
-	}
-	userName, err := util.NormalizeToFQDNRules(
-		claims.Email,
-		stripEmaildomain,
-	)
-	if err != nil {
-		return "", err
-	}
-
-	return userName, nil
-}
-
 func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string) (string, error) {
 	val, err := util.GenerateRandomStringURLSafe(64)
 	if err != nil {
--- a/hscontrol/policy/acls_test.go
+++ b/hscontrol/policy/acls_test.go
@ -13,7 +13,6 @@ import (
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/util"
 	"github.com/rs/zerolog/log"
-	"github.com/spf13/viper"
 	"github.com/stretchr/testify/require"
 	"go4.org/netipx"
 	"gopkg.in/check.v1"
@ -681,8 +680,6 @@ func Test_expandGroup(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			viper.Set("oidc.strip_email_domain", test.args.stripEmail)
-
 			got, err := test.field.pol.expandUsersFromGroup(
 				test.args.group,
 			)
--- a/hscontrol/types/config.go
+++ b/hscontrol/types/config.go
@ -180,10 +180,8 @@ type OIDCConfig struct {
 	AllowedDomains             []string
 	AllowedUsers               []string
 	AllowedGroups              []string
-	StripEmaildomain           bool
 	Expiry                     time.Duration
 	UseExpiryFromToken         bool
-	MapLegacyUsers             bool
 	PKCE                       PKCEConfig
 }

@ -315,11 +313,9 @@ func LoadConfig(path string, isFile bool) error {
 	viper.SetDefault("database.sqlite.wal_autocheckpoint", 1000) // SQLite default

 	viper.SetDefault("oidc.scope", []string{oidc.ScopeOpenID, "profile", "email"})
-	viper.SetDefault("oidc.strip_email_domain", true)
 	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", false)
 	viper.SetDefault("oidc.pkce.enabled", false)
 	viper.SetDefault("oidc.pkce.method", "S256")

@ -365,9 +361,9 @@ func validateServerConfig() error {
 	depr.fatal("dns.use_username_in_magic_dns")
 	depr.fatal("dns_config.use_username_in_magic_dns")

-	// TODO(kradalby): Reintroduce when strip_email_domain is removed
-	// after #2170 is cleaned up
-	// depr.fatal("oidc.strip_email_domain")
+	// Removed since version v0.26.0
+	depr.fatal("oidc.strip_email_domain")
+	depr.fatal("oidc.map_legacy_users")

 	if viper.GetBool("oidc.enabled") {
 		if err := validatePKCEMethod(viper.GetString("oidc.pkce.method")); err != nil {
@ -377,19 +373,6 @@ func validateServerConfig() error {

 	depr.Log()

-	for _, removed := range []string{
-		// TODO(kradalby): Reintroduce when strip_email_domain is removed
-		// after #2170 is cleaned up
-		// "oidc.strip_email_domain",
-		"dns.use_username_in_magic_dns",
-		"dns_config.use_username_in_magic_dns",
-	} {
-		if viper.IsSet(removed) {
-			log.Fatal().
-				Msgf("Fatal config error: %s has been removed. Please remove it from your config file", removed)
-		}
-	}
-
 	if viper.IsSet("dns.extra_records") && viper.IsSet("dns.extra_records_path") {
 		log.Fatal().Msg("Fatal config error: dns.extra_records and dns.extra_records_path are mutually exclusive. Please remove one of them from your config file")
 	}
@ -959,10 +942,6 @@ func LoadServerConfig() (*Config, error) {
 				}
 			}(),
 			UseExpiryFromToken: viper.GetBool("oidc.use_expiry_from_token"),
-			// TODO(kradalby): Remove when strip_email_domain is removed
-			// after #2170 is cleaned up
-			StripEmaildomain: viper.GetBool("oidc.strip_email_domain"),
-			MapLegacyUsers:   viper.GetBool("oidc.map_legacy_users"),
 			PKCE: PKCEConfig{
 				Enabled: viper.GetBool("oidc.pkce.enabled"),
 				Method:  viper.GetString("oidc.pkce.method"),
--- a/hscontrol/util/dns.go
+++ b/hscontrol/util/dns.go
@ -227,32 +227,3 @@ func GenerateIPv6DNSRootDomain(ipPrefix netip.Prefix) []dnsname.FQDN {

 	return fqdns
 }
-
-// TODO(kradalby): Reintroduce when strip_email_domain is removed
-// after #2170 is cleaned up
-// DEPRECATED: DO NOT USE
-// NormalizeToFQDNRules will replace forbidden chars in user
-// it can also return an error if the user doesn't respect RFC 952 and 1123.
-func NormalizeToFQDNRules(name string, stripEmailDomain bool) (string, error) {
-	name = strings.ToLower(name)
-	name = strings.ReplaceAll(name, "'", "")
-	atIdx := strings.Index(name, "@")
-	if stripEmailDomain && atIdx > 0 {
-		name = name[:atIdx]
-	} else {
-		name = strings.ReplaceAll(name, "@", ".")
-	}
-	name = invalidDNSRegex.ReplaceAllString(name, "-")
-
-	for _, elt := range strings.Split(name, ".") {
-		if len(elt) > LabelHostnameLength {
-			return "", fmt.Errorf(
-				"label %v is more than 63 chars: %w",
-				elt,
-				ErrInvalidUserName,
-			)
-		}
-	}
-
-	return name, nil
-}
--- a/integration/auth_oidc_test.go
+++ b/integration/auth_oidc_test.go
@ -80,10 +80,6 @@ func TestOIDCAuthenticationPingAll(t *testing.T) {
 		"HEADSCALE_OIDC_CLIENT_ID":          oidcConfig.ClientID,
 		"CREDENTIALS_DIRECTORY_TEST":        "/tmp",
 		"HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret",
-		// TODO(kradalby): Remove when strip_email_domain is removed
-		// after #2170 is cleaned up
-		"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "0",
-		"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "0",
 	}

 	err = scenario.CreateHeadscaleEnv(
@ -225,11 +221,6 @@ func TestOIDCExpireNodesBasedOnTokenExpiry(t *testing.T) {
 	assertTailscaleNodesLogout(t, allClients)
 }

-// TODO(kradalby):
-// - Test that creates a new user when one exists when migration is turned off
-// - Test that takes over a user when one exists when migration is turned on
-//   - But email is not verified
-//   - stripped email domain on/off
 func TestOIDC024UserCreation(t *testing.T) {
 	IntegrationSkip(t)

@ -243,9 +234,6 @@ func TestOIDC024UserCreation(t *testing.T) {
 	}{
 		{
 			name:          "no-migration-verified-email",
-			config: map[string]string{
-				"HEADSCALE_OIDC_MAP_LEGACY_USERS": "0",
-			},
 			emailVerified: true,
 			cliUsers:      []string{"user1", "user2"},
 			oidcUsers:     []string{"user1", "user2"},
@ -280,9 +268,6 @@ func TestOIDC024UserCreation(t *testing.T) {
 		},
 		{
 			name:          "no-migration-not-verified-email",
-			config: map[string]string{
-				"HEADSCALE_OIDC_MAP_LEGACY_USERS": "0",
-			},
 			emailVerified: false,
 			cliUsers:      []string{"user1", "user2"},
 			oidcUsers:     []string{"user1", "user2"},
@ -313,106 +298,8 @@ func TestOIDC024UserCreation(t *testing.T) {
 				}
 			},
 		},
-		{
-			name: "migration-strip-domains-verified-email",
-			config: map[string]string{
-				"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "1",
-				"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "1",
-			},
-			emailVerified: true,
-			cliUsers:      []string{"user1", "user2"},
-			oidcUsers:     []string{"user1", "user2"},
-			want: func(iss string) []*v1.User {
-				return []*v1.User{
-					{
-						Id:         1,
-						Name:       "user1",
-						Email:      "user1@headscale.net",
-						Provider:   "oidc",
-						ProviderId: iss + "/user1",
-					},
-					{
-						Id:         2,
-						Name:       "user2",
-						Email:      "user2@headscale.net",
-						Provider:   "oidc",
-						ProviderId: iss + "/user2",
-					},
-				}
-			},
-		},
-		{
-			name: "migration-strip-domains-not-verified-email",
-			config: map[string]string{
-				"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "1",
-				"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "1",
-			},
-			emailVerified: false,
-			cliUsers:      []string{"user1", "user2"},
-			oidcUsers:     []string{"user1", "user2"},
-			want: func(iss string) []*v1.User {
-				return []*v1.User{
-					{
-						Id:    1,
-						Name:  "user1",
-						Email: "user1@test.no",
-					},
-					{
-						Id:         2,
-						Name:       "user1",
-						Provider:   "oidc",
-						ProviderId: iss + "/user1",
-					},
-					{
-						Id:    3,
-						Name:  "user2",
-						Email: "user2@test.no",
-					},
-					{
-						Id:         4,
-						Name:       "user2",
-						Provider:   "oidc",
-						ProviderId: iss + "/user2",
-					},
-				}
-			},
-		},
-		{
-			name: "migration-no-strip-domains-verified-email",
-			config: map[string]string{
-				"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "1",
-				"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "0",
-			},
-			emailVerified: true,
-			cliUsers:      []string{"user1.headscale.net", "user2.headscale.net"},
-			oidcUsers:     []string{"user1", "user2"},
-			want: func(iss string) []*v1.User {
-				return []*v1.User{
-					// Hmm I think we will have to overwrite the initial name here
-					// createuser with "user1.headscale.net", but oidc with "user1"
-					{
-						Id:         1,
-						Name:       "user1",
-						Email:      "user1@headscale.net",
-						Provider:   "oidc",
-						ProviderId: iss + "/user1",
-					},
-					{
-						Id:         2,
-						Name:       "user2",
-						Email:      "user2@headscale.net",
-						Provider:   "oidc",
-						ProviderId: iss + "/user2",
-					},
-				}
-			},
-		},
 		{
 			name:          "migration-no-strip-domains-not-verified-email",
-			config: map[string]string{
-				"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "1",
-				"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "0",
-			},
 			emailVerified: false,
 			cliUsers:      []string{"user1.headscale.net", "user2.headscale.net"},
 			oidcUsers:     []string{"user1", "user2"},
@ -544,8 +431,6 @@ func TestOIDCAuthenticationWithPKCE(t *testing.T) {
 		"HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret",
 		"CREDENTIALS_DIRECTORY_TEST":        "/tmp",
 		"HEADSCALE_OIDC_PKCE_ENABLED":       "1", // Enable PKCE
-		"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "0",
-		"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "0",
 	}

 	err = scenario.CreateHeadscaleEnv(
@ -608,10 +493,6 @@ func TestOIDCReloginSameNodeNewUser(t *testing.T) {
 		"HEADSCALE_OIDC_CLIENT_ID":          oidcConfig.ClientID,
 		"CREDENTIALS_DIRECTORY_TEST":        "/tmp",
 		"HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret",
-		// TODO(kradalby): Remove when strip_email_domain is removed
-		// after #2170 is cleaned up
-		"HEADSCALE_OIDC_MAP_LEGACY_USERS":   "0",
-		"HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": "0",
 	}

 	err = scenario.CreateHeadscaleEnv(