Support trying multiple localparts for OpenID Connect. #8801

clokep · 2020-11-23T19:04:58Z

This abstracts code between the SAML and OpenID Connect code to support re-trying to generate the matrix ID localpart. This is useful if the attribute from SSO that is being used to generate the matrix ID is unique (e.g. if you're using the localpart of an email address you might have bob@foo and bob@bar, both would make to bob. This allows the second one to end up with bob1 as a matrix ID.

This is done in a a similar matter to SAML, by passing a failures count to the mapping provider. (Note that this is only done for a mapping provider which supports it, i.e. it is backwards compatible.) The code changes look a bit larger than they are -- really most of the code is copied from SamlHandler to SsoHandler and then abstracted.

This should generally be reviewable commit by commit.

Fixes #8711

…art-gen

clokep · 2020-11-23T19:36:13Z

synapse/handlers/sso.py

+            # Check if this mxid already exists
+            user_id = UserID(attributes.localpart, self.server_name).to_string()
+            users = await self.store.get_users_by_id_case_insensitive(user_id)
+            if users and allow_existing_users:


I think that this means that if allow_existing_users is true we'll either match on the first iteration or error. I couldn't come up with a situation where we wouldn't do this.

Do we even want to retry if allow_existing_users is set to true?

I had the same debate internally and initially had the same though, but then convinced myself it didn't matter. I believe what my above comment is trying to say is that no matter what this will end on the first loop if allow_existing_users is true:

If users is empty we've found an unused ID and can claim it (the elif not users branch).

If users is truthy, we will either use one of those IDs (if there's only one, or an exact case match) or raise an error (the MappingException branch).

So I think the answer is "yes we should not retry", but we essentially already do that.

I added some of this info to the docstring for this method too.

Ok, I think I see. Can you just add a quick comment here to say something like "Note, if allow_existing_users is true we won't loop"?

See 6d11ff8

synapse/handlers/oidc_handler.py

clokep · 2020-11-23T19:37:45Z

I think there's a couple of other things we could do to merge these implementations, namely the mapping providers are almost identical, but not quite. Which is frustrating, but out of scope for this.

clokep · 2020-11-23T19:39:31Z

synapse/handlers/sso.py

+            RedirectException: some mapping providers may raise this if they need
+                to redirect to an interstitial page.


I think redirecting will magically just work for OIDC, but I'm not 100% sure. I don't see any other special support in SAML for it to be honest.

The HTTP server will magically convert RedirectExceptions, so it should always be fine (so as long as it gets called on the HTTP path)

erikjohnston

Mostly looks good I think, just a few bits and bobs that I think is worth clearing up

synapse/handlers/oidc_handler.py

erikjohnston · 2020-11-24T10:28:46Z

synapse/handlers/oidc_handler.py

@@ -93,6 +94,9 @@ class OidcHandler(BaseHandler):
    """Handles requests related to the OpenID Connect login flow.
    """

+    # The number of attempts to ask the mapping provider for when generating an MXID.
+    _MAP_USERNAME_RETRIES = 1000


Hmm, this seems a bit excessive and like we're trying to sweep the problem under the carpet. I can foresee people just increment an integer on the end, which would end up failing for the 1000th user.

Not sure what to do instead though :/

This is what we do for SAML currently, but it did seem rather excessive to me too.

I guess that's fine for now then.

See #8813 also.

synapse/handlers/oidc_handler.py

synapse/handlers/sso.py

erikjohnston · 2020-11-24T10:34:29Z

synapse/handlers/sso.py

+            RedirectException: some mapping providers may raise this if they need
+                to redirect to an interstitial page.


The HTTP server will magically convert RedirectExceptions, so it should always be fine (so as long as it gets called on the HTTP path)

synapse/handlers/oidc_handler.py

erikjohnston · 2020-11-24T10:39:36Z

synapse/handlers/sso.py

+            # Check if this mxid already exists
+            user_id = UserID(attributes.localpart, self.server_name).to_string()
+            users = await self.store.get_users_by_id_case_insensitive(user_id)
+            if users and allow_existing_users:


Do we even want to retry if allow_existing_users is set to true?

changelog.d/8801.feature

Co-authored-by: Erik Johnston <erik@matrix.org>

clokep · 2020-11-24T13:19:38Z

@erikjohnston Thanks for the review! I think I've handled or replied to all your comments!

erikjohnston · 2020-11-25T10:51:31Z

synapse/handlers/oidc_handler.py

@@ -1066,6 +1066,10 @@ async def map_user_attributes(
        # Ensure only valid characters are included in the MXID.
        localpart = map_username_to_mxid_localpart(localpart)

+        # Append suffix integer if last call to this function failed to produce
+        # a usable mxid.
+        localpart += str(failures) if failures else ""


I am quite hesitant about this as a) it'll explode if we have more users with same localpart than the number of retries, and b) there isn't a way of writing this particular logic correctly with this API? I know that this is what we do for other SSO, but it feels like a footgun that will one day hit us (especially if we decided to reduce the number of retries to a sensible number). I don't suggest we fix this here, but might be worth at least making an issue for it.

Filed #8813

erikjohnston

Looks good!

This was broken in #8801 when abstracting code shared with OIDC. After this change both SAML and OIDC have a concept of grandfathering users, but with different implementations.

…ers. (#8878) This was broken in #8801.

@angdraug

Synapse 1.24.0 (2020-12-09) =========================== Due to the two security issues highlighted below, server administrators are encouraged to update Synapse. We are not aware of these vulnerabilities being exploited in the wild. Security advisory ----------------- The following issues are fixed in v1.23.1 and v1.24.0. - There is a denial of service attack ([CVE-2020-26257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26257)) against the federation APIs in which future events will not be correctly sent to other servers over federation. This affects all servers that participate in open federation. (Fixed in [#8776](matrix-org/synapse#8776)). - Synapse may be affected by OpenSSL [CVE-2020-1971](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1971). Synapse administrators should ensure that they have the latest versions of the cryptography Python package installed. To upgrade Synapse along with the cryptography package: * Administrators using the [`matrix.org` Docker image](https://hub.docker.com/r/matrixdotorg/synapse/) or the [Debian/Ubuntu packages from `matrix.org`](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#matrixorg-packages) should ensure that they have version 1.24.0 or 1.23.1 installed: these images include the updated packages. * Administrators who have [installed Synapse from source](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#installing-from-source) should upgrade the cryptography package within their virtualenv by running: ```sh <path_to_virtualenv>/bin/pip install 'cryptography>=3.3' ``` * Administrators who have installed Synapse from distribution packages should consult the information from their distributions. Internal Changes ---------------- - Add a maximum version for pysaml2 on Python 3.5. ([\#8898](matrix-org/synapse#8898)) Synapse 1.24.0rc2 (2020-12-04) ============================== Bugfixes -------- - Fix a regression in v1.24.0rc1 which failed to allow SAML mapping providers which were unable to redirect users to an additional page. ([\#8878](matrix-org/synapse#8878)) Internal Changes ---------------- - Add support for the `prometheus_client` newer than 0.9.0. Contributed by Jordan Bancino. ([\#8875](matrix-org/synapse#8875)) Synapse 1.24.0rc1 (2020-12-02) ============================== Features -------- - Add admin API for logging in as a user. ([\#8617](matrix-org/synapse#8617)) - Allow specification of the SAML IdP if the metadata returns multiple IdPs. ([\#8630](matrix-org/synapse#8630)) - Add support for re-trying generation of a localpart for OpenID Connect mapping providers. ([\#8801](matrix-org/synapse#8801), [\#8855](matrix-org/synapse#8855)) - Allow the `Date` header through CORS. Contributed by Nicolas Chamo. ([\#8804](matrix-org/synapse#8804)) - Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "the number of rooms with unread messages" or "total unread messages". ([\#8820](matrix-org/synapse#8820)) - Add `force_purge` option to delete-room admin api. ([\#8843](matrix-org/synapse#8843)) Bugfixes -------- - Fix a bug where appservices may be sent an excessive amount of read receipts and presence. Broke in v1.22.0. ([\#8744](matrix-org/synapse#8744)) - Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. ([\#8776](matrix-org/synapse#8776)) - Fix a bug where synctl could spawn duplicate copies of a worker. Contributed by Waylon Cude. ([\#8798](matrix-org/synapse#8798)) - Allow per-room profiles to be used for the server notice user. ([\#8799](matrix-org/synapse#8799)) - Fix a bug where logging could break after a call to SIGHUP. ([\#8817](matrix-org/synapse#8817)) - Fix `register_new_matrix_user` failing with "Bad Request" when trailing slash is included in server URL. Contributed by @angdraug. ([\#8823](matrix-org/synapse#8823)) - Fix a minor long-standing bug in login, where we would offer the `password` login type if a custom auth provider supported it, even if password login was disabled. ([\#8835](matrix-org/synapse#8835)) - Fix a long-standing bug which caused Synapse to require unspecified parameters during user-interactive authentication. ([\#8848](matrix-org/synapse#8848)) - Fix a bug introduced in v1.20.0 where the user-agent and IP address reported during user registration for CAS, OpenID Connect, and SAML were of the wrong form. ([\#8784](matrix-org/synapse#8784)) Improved Documentation ---------------------- - Clarify the usecase for a msisdn delegate. Contributed by Adrian Wannenmacher. ([\#8734](matrix-org/synapse#8734)) - Remove extraneous comma from JSON example in User Admin API docs. ([\#8771](matrix-org/synapse#8771)) - Update `turn-howto.md` with troubleshooting notes. ([\#8779](matrix-org/synapse#8779)) - Fix the example on how to set the `Content-Type` header in nginx for the Client Well-Known URI. ([\#8793](matrix-org/synapse#8793)) - Improve the documentation for the admin API to list all media in a room with respect to encrypted events. ([\#8795](matrix-org/synapse#8795)) - Update the formatting of the `push` section of the homeserver config file to better align with the [code style guidelines](https://github.com/matrix-org/synapse/blob/develop/docs/code_style.md#configuration-file-format). ([\#8818](matrix-org/synapse#8818)) - Improve documentation how to configure prometheus for workers. ([\#8822](matrix-org/synapse#8822)) - Update example prometheus console. ([\#8824](matrix-org/synapse#8824)) Deprecations and Removals ------------------------- - Remove old `/_matrix/client/*/admin` endpoints which were deprecated since Synapse 1.20.0. ([\#8785](matrix-org/synapse#8785)) - Disable pretty printing JSON responses for curl. Users who want pretty-printed output should use [jq](https://stedolan.github.io/jq/) in combination with curl. Contributed by @tulir. ([\#8833](matrix-org/synapse#8833)) Internal Changes ---------------- - Simplify the way the `HomeServer` object caches its internal attributes. ([\#8565](matrix-org/synapse#8565), [\#8851](matrix-org/synapse#8851)) - Add an example and documentation for clock skew to the SAML2 sample configuration to allow for clock/time difference between the homserver and IdP. Contributed by @localguru. ([\#8731](matrix-org/synapse#8731)) - Generalise `RoomMemberHandler._locally_reject_invite` to apply to more flows than just invite. ([\#8751](matrix-org/synapse#8751)) - Generalise `RoomStore.maybe_store_room_on_invite` to handle other, non-invite membership events. ([\#8754](matrix-org/synapse#8754)) - Refactor test utilities for injecting HTTP requests. ([\#8757](matrix-org/synapse#8757), [\#8758](matrix-org/synapse#8758), [\#8759](matrix-org/synapse#8759), [\#8760](matrix-org/synapse#8760), [\#8761](matrix-org/synapse#8761), [\#8777](matrix-org/synapse#8777)) - Consolidate logic between the OpenID Connect and SAML code. ([\#8765](matrix-org/synapse#8765)) - Use `TYPE_CHECKING` instead of magic `MYPY` variable. ([\#8770](matrix-org/synapse#8770)) - Add a commandline script to sign arbitrary json objects. ([\#8772](matrix-org/synapse#8772)) - Minor log line improvements for the SSO mapping code used to generate Matrix IDs from SSO IDs. ([\#8773](matrix-org/synapse#8773)) - Add additional error checking for OpenID Connect and SAML mapping providers. ([\#8774](matrix-org/synapse#8774), [\#8800](matrix-org/synapse#8800)) - Add type hints to HTTP abstractions. ([\#8806](matrix-org/synapse#8806), [\#8812](matrix-org/synapse#8812)) - Remove unnecessary function arguments and add typing to several membership replication classes. ([\#8809](matrix-org/synapse#8809)) - Optimise the lookup for an invite from another homeserver when trying to reject it. ([\#8815](matrix-org/synapse#8815)) - Add tests for `password_auth_provider`s. ([\#8819](matrix-org/synapse#8819)) - Drop redundant database index on `event_json`. ([\#8845](matrix-org/synapse#8845)) - Simplify `uk.half-shot.msc2778.login.application_service` login handler. ([\#8847](matrix-org/synapse#8847)) - Refactor `password_auth_provider` support code. ([\#8849](matrix-org/synapse#8849)) - Add missing `ordering` to background database updates. ([\#8850](matrix-org/synapse#8850)) - Allow for specifying a room version when creating a room in unit tests via `RestHelper.create_room_as`. ([\#8854](matrix-org/synapse#8854))

clokep added 11 commits November 23, 2020 10:23

Support trying multiple localparts for OpenID Connect.

24265fa

Add an additional test.

f4bdc2f

Abstract the shared user mapping code from SAML and OIDC.

f419ff5

Do not pass through parameters to the mapper.

7e6a549

Do not retry Matrix IDs which will keep failing.

c34e973

Support matching an existing username for OIDC.

1f64e02

Clarify some of the type hints.

eda3e50

Merge remote-tracking branch 'origin/develop' into clokep/oidc-localp…

c0ae560

…art-gen

Add a newsfragment.

d3c4e04

Fix some comments.

0ded8b1

Remove a reference to SAML in the abstract code.

c29d8de

clokep requested a review from a team November 23, 2020 19:34

clokep commented Nov 23, 2020

View reviewed changes

synapse/handlers/oidc_handler.py Outdated Show resolved Hide resolved

clokep commented Nov 23, 2020

View reviewed changes

erikjohnston suggested changes Nov 24, 2020

View reviewed changes

clokep and others added 4 commits November 24, 2020 07:40

Update the changelog based on feedback.

512529f

Co-authored-by: Erik Johnston <erik@matrix.org>

Expand comments and add a return type.

9c5d0f6

Append the number of failures by default when mapping OIDC IDs.

1934c10

Do not use a deprecated function.

89f1869

clokep requested a review from erikjohnston November 24, 2020 13:19

erikjohnston reviewed Nov 25, 2020

View reviewed changes

erikjohnston approved these changes Nov 25, 2020

View reviewed changes

clokep mentioned this pull request Nov 25, 2020

Logic for generating unique matrix IDs for SSO users could fail #8813

Open

clokep added 2 commits November 25, 2020 08:40

Add an additional comment.

6d11ff8

Clarify another comment.

18fcaee

clokep merged commit 4fd222a into develop Nov 25, 2020

clokep deleted the clokep/oidc-localpart-gen branch November 25, 2020 15:04

This was referenced Dec 1, 2020

Fix grandfathering of SAML users #8855

Merged

Use the abstract registration code for CAS #8856

Merged

clokep mentioned this pull request Dec 4, 2020

Fix a regression that mapping providers should be able to redirect users. #8878

Merged

clokep added a commit that referenced this pull request Dec 4, 2020

Fix a regression that mapping providers should be able to redirect us…

22c6c19

…ers. (#8878) This was broken in #8801.

matrixbot mentioned this pull request Dec 21, 2023

Logic for generating unique matrix IDs for SSO users could fail element-hq/synapse#8813

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support trying multiple localparts for OpenID Connect. #8801

Support trying multiple localparts for OpenID Connect. #8801

clokep commented Nov 23, 2020

clokep Nov 23, 2020

erikjohnston Nov 24, 2020

clokep Nov 24, 2020

clokep Nov 24, 2020

erikjohnston Nov 25, 2020

clokep Nov 25, 2020

clokep Nov 25, 2020

clokep commented Nov 23, 2020

clokep Nov 23, 2020 •

edited

Loading

erikjohnston Nov 24, 2020

erikjohnston left a comment

erikjohnston Nov 24, 2020

clokep Nov 24, 2020

erikjohnston Nov 25, 2020

clokep Nov 25, 2020

erikjohnston Nov 24, 2020

erikjohnston Nov 24, 2020

clokep commented Nov 24, 2020

erikjohnston Nov 25, 2020 •

edited

Loading

clokep Nov 25, 2020

erikjohnston left a comment

		RedirectException: some mapping providers may raise this if they need
		to redirect to an interstitial page.

Support trying multiple localparts for OpenID Connect. #8801

Support trying multiple localparts for OpenID Connect. #8801

Conversation

clokep commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Nov 23, 2020

clokep Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Nov 24, 2020

erikjohnston Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

clokep Nov 23, 2020 •

edited

Loading

erikjohnston Nov 25, 2020 •

edited

Loading