multi-ingress: cross-IdP SSO by supersven · Pull Request #5212 · wireapp/wire-server

supersven · 2026-05-01T10:00:28Z

I've covered this usecase in the documentation. To avoid duplication, please look at the docs in this PR.

Ticket: https://wearezeta.atlassian.net/browse/WPB-25161

Security notes:

This requires a final sign off by @wireapp/security
The algorithm for multi-ingress SSO cross-IdP logins has been discussed with @emil-wire
This PR also lowers the condition to be found by /sso/get-by-email from SCIM SSO to SSO user

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

Copilot

Pull request overview

Adds support for multi-ingress “cross-IdP” SSO logins in Spar by falling back to a team-wide lookup (by email NameID) when the primary (issuer, NameID) lookup fails, and updates /sso/get-by-email discovery to work for any SSO user (not only SCIM-managed).

Changes:

Extend Spar SAML finalize-login handling to support cross-IdP migration in multi-ingress mode (and add a dedicated config error for non-email NameIDs).
Relax /sso/get-by-email lookup condition from “SCIM+SSO” to “SSO user”.
Add integration and unit tests plus documentation/changelog updates covering the new behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
services/spar/test-integration/Test/Spar/AppSpec.hs	Updates integration test helper to call updated `verdictHandler` signature with SAML config.
services/spar/src/Spar/Error.hs	Adds and renders a new error for invalid multi-ingress cross-IdP configuration (non-email NameID).
services/spar/src/Spar/App.hs	Implements multi-ingress cross-IdP fallback flow + structured logging changes.
services/spar/src/Spar/API.hs	Plumbs SAML config/host info into finalize-login handling and updates logger constraints.
libs/wire-subsystems/test/unit/Wire/IdPSubsystem/InterpreterSpec.hs	Adjusts unit test expectation to “non SSO user” behavior for get-by-email.
libs/wire-subsystems/src/Wire/IdPSubsystem/Interpreter.hs	Changes `/sso/get-by-email` eligibility check from SCIM+SSO to SSO.
integration/test/Test/Spar/MultiIngressCrossIdpSso.hs	Adds new integration test suite for cross-IdP migration and related error cases.
integration/integration.cabal	Registers new integration test module.
docs/src/developer/reference/config-options.md	Documents cross-IdP fallback algorithm and updates get-by-email criteria.
changelog.d/2-features/multi-ingress-cross-IdP-SSO	Adds changelog entry describing cross-IdP SSO behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

supersven · 2026-05-12T15:23:38Z

+    -- This used to check if the user is SCIM AND SSO! The RFC ("2025-05-12
+    -- RFC: Default SSO flow for team by host domain") is not really
+    -- unambiguous about this. The customer currently provisions non-SCIM.
+    isSsoUser :: User -> Bool
+    isSsoUser = isJust . userSSOId


This might be subject to change in case the customer decides to go for SCIM. Then, we could make this condition stricter (requiring SSO AND SCIM).

fisx

I read the docs and have some questions, more feedback coming up!

fisx · 2026-05-13T06:30:23Z

+1. **NameID must be an email address.** Username-based `NameID`s are rejected
+   to avoid ambiguity across authenticating IdPs.


[nit] admins need to manually guarantee that email addresses are unique, so why can't they do the same thing here?

@emil-wire and I were uncertain about this as well. I made the hand-wavy decision, because in my experience with other web services, my email address is much less likely to conflict than my desired username when I register somewhere.

However, the customer is not much into emails as fetching those may reveal relationships as well AND @julialongtin promised that all data will be located in one database (used by all IdPs)

So, this could actually be subject to change. 🤔

I think the best approach is to allow nicks / usernames, but put big red warnings stickers in all the right places and emphasize that emails are best here, and nicks with mandatory, instance-specific pre- or suffixes are second-best.

If you decide against my intuition that's also fine, if anything supporting only emails would make it safer.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

fisx

I skimmed through the integration tests and thoroughly reviewed the rest, looks intact! I've hidden a few possibly valuable change requests in a large number of nit-picks to make processing the review more fun. :-)

I'm pre-approving because none of the change requests affect correctness and security IMO.

fisx · 2026-07-13T10:05:40Z

+1. **NameID must be an email address.** Username-based `NameID`s are rejected
+   to avoid ambiguity across authenticating IdPs.


I think the best approach is to allow nicks / usernames, but put big red warnings stickers in all the right places and emphasize that emails are best here, and nicks with mandatory, instance-specific pre- or suffixes are second-best.

If you decide against my intuition that's also fine, if anything supporting only emails would make it safer.

fisx · 2026-07-13T10:13:10Z

+It must be ensured that email `NameID`s are unique across IdPs by IdP
+administrators. Otherwise, users may be logged into other users' accounts!


It's probably worth adding this warning to the UI in team management, at the point where multi-ingress is enabled. Has this happened? Is there a ticket for this?

I've agreed with @emil-wire that bold warnings in the docs are good enough; given that multi-ingress IdPs will most likely be configured in close co-operation with customer delivery engineering and IdPs will certainly configured via API anyways.

emil-wire · 2026-07-15T12:53:54Z

Not something this PR introduced, but its close enough to it that I'd rather raise it here than lose track of it and forget about it.

I don't think the rate limit on /sso/get-by-email is actually in force. The generic - path: /sso entry sits above the - path: /sso/get-by-email$ one, and both get rendered as regex locations by this template. nginx picks the first regex location that matches rather than the most specific one, so /v15/sso/get-by-email gets served by the /sso block and never reaches the block with the tighter regex.

I rendered the chart to check (helm template charts/nginz), both blocks land in the same server block in this order:

location ~* ^(/v[0-9]+)?/sso {                  ->  limit_req zone=reqs_per_addr_sso burst=10 nodelay;
location ~* ^(/v[0-9]+)?/sso/get-by-email$ {    ->  limit_req zone=reqs_per_addr_sso_get_by_email burst=15 nodelay;

So the endpoint ends up on 50r/s instead of the intended 5r/m. The $ doesn't do anything because the regex above already matched.

initiate-login and finalize-login are already above /sso, which suggests this was understood at the time and get-by-email$ just got appended at the end when it came in with #5024. Moving it above the /sso entry should be all it needs.

raisin it here because the two things interact: this PR widens which users the endpoint answers for (SCIM+SSO -> any SSO user), and config sets enableIdPByEmailDiscovery: true for every multi-ingress deployment, so more accounts become reachable through an endpoint whose brute force protection isn't running. Fixing the ordering doesn't close the endpoint as an oracle for a known address, but it does make enumeration slower.

Allow users to login with all team IdPs. This is required because there's usually one IdP per ingress in a team and users should be able to login on all ingresses.

emil-wire · 2026-08-03T13:10:36Z

 It must be ensured that email `NameID`s are unique across IdPs by IdP
 administrators. Otherwise, users may be logged into other users' accounts!

+!!! danger


zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label May 1, 2026

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch 3 times, most recently from 70c63ab to 63c5b7d Compare May 5, 2026 17:18

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch 3 times, most recently from c70220c to e7f3bf2 Compare May 12, 2026 08:33

supersven changed the title ~~POC: multi ingress sso cross idp~~ multi-ingress: cross-IdP SSO May 12, 2026

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from e7f3bf2 to d27326c Compare May 12, 2026 09:37

supersven requested review from Copilot and sanojwr May 12, 2026 10:06

Copilot started reviewing on behalf of supersven May 12, 2026 10:07 View session

supersven marked this pull request as ready for review May 12, 2026 10:07

supersven requested review from a team as code owners May 12, 2026 10:07

supersven requested a review from emil-wire May 12, 2026 10:08

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread services/spar/src/Spar/App.hs

Comment thread docs/src/developer/reference/config-options.md Outdated

Comment thread docs/src/developer/reference/config-options.md Outdated

Comment thread changelog.d/2-features/multi-ingress-cross-IdP-SSO Outdated

supersven commented May 12, 2026

View reviewed changes

fisx reviewed May 13, 2026

View reviewed changes

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch 2 times, most recently from a149340 to 4cce453 Compare June 9, 2026 06:17

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch 2 times, most recently from 82b25a8 to f9fea47 Compare July 6, 2026 16:15

supersven mentioned this pull request Jul 8, 2026

idpCertFingerprintAllowlist mandatory for multi-ingress SSO #5327

Merged

2 tasks

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from f9fea47 to c134d5d Compare July 9, 2026 09:55

supersven commented Jul 9, 2026

View reviewed changes

Comment thread services/spar/src/Spar/App.hs

supersven commented Jul 9, 2026

View reviewed changes

Comment thread services/spar/src/Spar/App.hs

supersven commented Jul 9, 2026

View reviewed changes

Comment thread services/spar/src/Spar/App.hs

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from 4113c24 to 9a88a59 Compare July 9, 2026 15:27

supersven requested a review from Copilot July 9, 2026 15:28

Copilot started reviewing on behalf of supersven July 9, 2026 15:29 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread services/spar/src/Spar/App.hs

Comment thread integration/test/Test/Spar/MultiIngressCrossIdpSso.hs

Comment thread changelog.d/2-features/multi-ingress-cross-IdP-SSO Outdated

Comment thread docs/src/developer/reference/config-options.md Outdated

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from 9a88a59 to cc9b20e Compare July 9, 2026 16:10

supersven requested a review from Copilot July 9, 2026 16:16

Copilot started reviewing on behalf of supersven July 9, 2026 16:16 View session

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from e54062a to 9e877f6 Compare July 9, 2026 16:20

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread services/spar/src/Spar/App.hs Outdated

Comment thread services/spar/src/Spar/App.hs

supersven requested a review from fisx July 10, 2026 05:49

fisx approved these changes Jul 13, 2026

View reviewed changes

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch 2 times, most recently from 700aa26 to 028e41e Compare July 15, 2026 06:38

emil-wire requested changes Jul 15, 2026

View reviewed changes

Comment thread docs/src/developer/reference/config-options.md

supersven mentioned this pull request Jul 15, 2026

fix /sso/get-by-email rate limiting #5341

Merged

2 tasks

multi-ingress: Cross-IdP migration

94c5438

Allow users to login with all team IdPs. This is required because there's usually one IdP per ingress in a team and users should be able to login on all ingresses.

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from 028e41e to 64b38e7 Compare August 3, 2026 09:32

supersven added 2 commits August 3, 2026 13:45

Add security warning notes

ea0dda5

Fix typo

74412fa

supersven force-pushed the sventennie/multi-ingress-sso_cross_idp branch from 64b38e7 to 74412fa Compare August 3, 2026 11:46

emil-wire approved these changes Aug 3, 2026

View reviewed changes

supersven merged commit ca95062 into develop Aug 3, 2026
10 checks passed

supersven deleted the sventennie/multi-ingress-sso_cross_idp branch August 3, 2026 15:07

This was referenced Aug 21, 2026

Release 2026-08-21 - (expected chart version 5.35.0) #5470

Closed

Release 2026-08-27 - (expected chart version 5.35.0) #5483

Closed

Release 2026-08-27 - (expected chart version 5.35.0) #5489

Merged

		1. NameID must be an email address. Username-based `NameID`s are rejected
		to avoid ambiguity across authenticating IdPs.

		It must be ensured that email `NameID`s are unique across IdPs by IdP
		administrators. Otherwise, users may be logged into other users' accounts!

Uh oh!

Conversation

supersven commented May 1, 2026 • edited by fisx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emil-wire commented Jul 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

supersven commented May 1, 2026 •

edited by fisx

Loading