diff --git a/CHANGELOG.md b/CHANGELOG.md index bf7ae27b..13cc7fe0 100644 --- 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 diff --git a/docs/ref/oidc.md b/docs/ref/oidc.md index 9f8c3e59..7cd5e198 100644 --- 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 diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index 29c1141e..d6a6d59f 100644 --- 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 { diff --git a/hscontrol/policy/acls_test.go b/hscontrol/policy/acls_test.go index 750d7b53..87da4062 100644 --- 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, ) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index add5f0f2..0b69a1a4 100644 --- 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"), diff --git a/hscontrol/util/dns.go b/hscontrol/util/dns.go index 6c4e8a37..386e91e2 100644 --- 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 -} diff --git a/integration/auth_oidc_test.go b/integration/auth_oidc_test.go index 0c757a2d..a76220d8 100644 --- 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) @@ -242,10 +233,7 @@ func TestOIDC024UserCreation(t *testing.T) { want func(iss string) []*v1.User }{ { - name: "no-migration-verified-email", - config: map[string]string{ - "HEADSCALE_OIDC_MAP_LEGACY_USERS": "0", - }, + name: "no-migration-verified-email", emailVerified: true, cliUsers: []string{"user1", "user2"}, oidcUsers: []string{"user1", "user2"}, @@ -279,10 +267,7 @@ func TestOIDC024UserCreation(t *testing.T) { }, }, { - name: "no-migration-not-verified-email", - config: map[string]string{ - "HEADSCALE_OIDC_MAP_LEGACY_USERS": "0", - }, + name: "no-migration-not-verified-email", emailVerified: false, cliUsers: []string{"user1", "user2"}, oidcUsers: []string{"user1", "user2"}, @@ -314,105 +299,7 @@ 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", - }, + name: "migration-no-strip-domains-not-verified-email", 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(