-
Notifications
You must be signed in to change notification settings - Fork 622
feat: add DangerousSSOAutoLinking config flag for automatic SSO ident... #2351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,11 +14,21 @@ import ( | |
| // _should_ generally fall under the same User entity. It's just a runtime | ||
| // string, and is not typically persisted in the database. This value can vary | ||
| // across time. | ||
| func GetAccountLinkingDomain(provider string, ownLinkingDomains []string) string { | ||
| if strings.HasPrefix(provider, "sso:") || slices.Contains(ownLinkingDomains, provider) { | ||
| // when the provider ID is a SSO provider, then the linking | ||
| // domain is the provider itself i.e. there can only be one | ||
| // user + identity per identity provider | ||
| func GetAccountLinkingDomain(provider string, ownLinkingDomains []string, dangerousSSOAutoLinking bool) string { | ||
| if strings.HasPrefix(provider, "sso:") { | ||
| if dangerousSSOAutoLinking { | ||
| // When dangerous SSO auto-linking is enabled, treat SSO providers | ||
| // like OAuth providers - they join the default linking domain and | ||
| // will be linked to existing accounts based on email matching. | ||
| return "default" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Severity: HIGH Authentication Bypass via Malicious SSO IdP: When 💡 Fix SuggestionSuggestion: This is a complex architectural security issue that requires multiple complementary mitigations:
These changes span multiple files (linking.go, configuration.go, samlacs.go, audit logging system, and database migrations) and require careful consideration of backward compatibility and migration paths for existing deployments. |
||
| } | ||
| // Default behavior: SSO providers get their own isolated linking domain, | ||
| // meaning they will always create new user accounts. | ||
| return provider | ||
| } | ||
|
|
||
| if slices.Contains(ownLinkingDomains, provider) { | ||
| // Providers explicitly configured to have their own linking domain | ||
| return provider | ||
| } | ||
|
|
||
|
|
@@ -65,7 +75,7 @@ func DetermineAccountLinking(tx *storage.Connection, config *conf.GlobalConfigur | |
| } | ||
|
|
||
| // this is the linking domain for the new identity | ||
| candidateLinkingDomain := GetAccountLinkingDomain(providerName, config.Experimental.ProvidersWithOwnLinkingDomain) | ||
| candidateLinkingDomain := GetAccountLinkingDomain(providerName, config.Experimental.ProvidersWithOwnLinkingDomain, config.Security.DangerousSSOAutoLinking) | ||
|
|
||
| if identity, terr := FindIdentityByIdAndProvider(tx, sub, providerName); terr == nil { | ||
| // account exists | ||
|
|
@@ -93,7 +103,7 @@ func DetermineAccountLinking(tx *storage.Connection, config *conf.GlobalConfigur | |
| // or link to an existing one | ||
| if len(verifiedEmails) == 0 { | ||
| // if there are no verified emails, we always decide to create a new account | ||
| user, terr := IsDuplicatedEmail(tx, candidateEmail.Email, aud, nil, config.Experimental.ProvidersWithOwnLinkingDomain) | ||
| user, terr := IsDuplicatedEmail(tx, candidateEmail.Email, aud, nil, config.Experimental.ProvidersWithOwnLinkingDomain, config.Security.DangerousSSOAutoLinking) | ||
| if terr != nil { | ||
| return AccountLinkingResult{}, terr | ||
| } | ||
|
|
@@ -131,7 +141,7 @@ func DetermineAccountLinking(tx *storage.Connection, config *conf.GlobalConfigur | |
| // now let's see if there are any existing and similar identities in | ||
| // the same linking domain | ||
| for _, identity := range similarIdentities { | ||
| if GetAccountLinkingDomain(identity.Provider, config.Experimental.ProvidersWithOwnLinkingDomain) == candidateLinkingDomain { | ||
| if GetAccountLinkingDomain(identity.Provider, config.Experimental.ProvidersWithOwnLinkingDomain, config.Security.DangerousSSOAutoLinking) == candidateLinkingDomain { | ||
| linkingIdentities = append(linkingIdentities, identity) | ||
| } | ||
| } | ||
|
|
||