From 28b62847fb3c5596807840c400864aca9334bb83 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 17 Oct 2024 06:22:44 -0600 Subject: [PATCH] restore strip_email_domain for migration Signed-off-by: Kristoffer Dalby --- hscontrol/oidc.go | 52 ++++++++++++++++++++++++++++----------- hscontrol/types/config.go | 10 ++++++-- hscontrol/util/dns.go | 30 ++++++++++++++++++++++ 3 files changed, 75 insertions(+), 17 deletions(-) diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index e7ce0acb..08681f80 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -445,25 +445,29 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim( // look it up by username. This should only be needed once. // This branch will presist 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 { - 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) - } + if oldUsername, err := getUserName(claims, a.cfg.StripEmaildomain); err == nil { + user, err = a.db.GetUserByName(oldUsername) + if err != nil && !errors.Is(err, db.ErrUserNotFound) { + return nil, fmt.Errorf("creating or updating user: %w", err) + } - // if the user is still not found, create a new empty user. - 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 != nil && 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{} + } } + } - // 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{} - } + // if the user is still not found, create a new empty user. + if user == nil { + user = &types.User{} } user.FromClaim(claims) @@ -513,3 +517,21 @@ 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) { + userName, err := util.NormalizeToFQDNRules( + claims.Email, + stripEmaildomain, + ) + if err != nil { + return "", err + } + + return userName, nil +} diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 9ebbbb96..1a47231a 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -162,6 +162,7 @@ type OIDCConfig struct { AllowedDomains []string AllowedUsers []string AllowedGroups []string + StripEmaildomain bool Expiry time.Duration UseExpiryFromToken bool MapLegacyUsers bool @@ -273,6 +274,7 @@ func LoadConfig(path string, isFile bool) error { viper.SetDefault("database.sqlite.write_ahead_log", true) 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) @@ -320,14 +322,18 @@ func validateServerConfig() error { depr.warn("dns_config.use_username_in_magic_dns") depr.warn("dns.use_username_in_magic_dns") - depr.fatal("oidc.strip_email_domain") + // TODO(kradalby): Reintroduce when strip_email_domain is removed + // after #2170 is cleaned up + // depr.fatal("oidc.strip_email_domain") depr.fatal("dns.use_username_in_musername_in_magic_dns") depr.fatal("dns_config.use_username_in_musername_in_magic_dns") depr.Log() for _, removed := range []string{ - "oidc.strip_email_domain", + // TODO(kradalby): Reintroduce when strip_email_domain is removed + // after #2170 is cleaned up + // "oidc.strip_email_domain", "dns_config.use_username_in_musername_in_magic_dns", } { if viper.IsSet(removed) { diff --git a/hscontrol/util/dns.go b/hscontrol/util/dns.go index f57576f4..bf43eb50 100644 --- a/hscontrol/util/dns.go +++ b/hscontrol/util/dns.go @@ -182,3 +182,33 @@ 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 = invalidCharsInUserRegex.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 +}