Implement OpenID Connect-based login #7256

sandhose · 2020-04-09T15:26:11Z

This PR implements OIDC based login through the m.login.sso mechanism.

OpenID Connect is based on OAuth2, so this PR is kinda redundant with #7059. For now, it only supports OpenID Connect-compliant providers with the authorization code flow. This could be expanded to support generic OAuth2 provider, as long as they either support the openid scope or the userinfo endpoint to fetch user infos.

This is a work in progress, here are a few things that need to be done:

make the authorization, token and jwks endpoints configurable. Right now they are discovered from the discovery document.
refresh the jwks periodically. The keys can be rotated by the OIDC Provider
~~support implicit and hybrid flows~~ – edit: implicit and hybrid flows are for single page apps essentially & give the token in the fragment part of the URL, which can't be retrieved server-side
make the user attributes mapping configurable (maybe the same way the SAML provider does)
handle errors more nicely
write some tests
make mypy happy, add typings where it makes sense
maybe handle logouts: the OIDC specs support frontchannel (initiated by the app) and backchannel (initiated by the provider) logouts. For that we would need to save the id_token we get from the provider

For the flow to be secure (and prevent CSRF attacks for example), I needed to have a way to carry state through the flow. I chose to use a macaroon token with that state in it.

I needed an URL for the authorization flow callback. As it is not part of the spec, I chose to have that endpoint under /_synapse/oidc/callback.

I used OIDC as a name pretty much everywhere, but it would make sense to use oauth2 instead ; haven't really decided.

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

sandhose · 2020-04-17T19:55:35Z

I've polished this PR quite a lot the last few days. CI is passing, and I started writing some tests.

Even though it is not ready for merge, mainly because of error handling, I'd love to have a first review of it.

Here are a few things I'd like to know:

are the names I chose alright in general? I used OIDC pretty much everywhere, but it could be named OAuth2, as it can handle generic OAuth2 providers
I used /_synapse/oidc/callback as a redirect_uri ; should that be something else?
to pass data through the OAuth2 flow, I save a cookie with some session informations encoded in a macaroon token. Is that the right approach?
I used authlib to handle to handle some of the OAuth2/OIDC parts. I added it in synapse/python_dependencies.py. Is there somewhere else I should mention it?
I'm not sure how to handle errors properly. Raising SynapseError in an async function does not seem to do anything.
to map the user infos from to provider to a mxid/display name, I used jinja templates that are configurable through the config file. Is that a good approach, or should I opt for something more similar than what is done in the SAML2 module?

A few things that I'd like to do, but that I might do later in other PRs:

maybe refactor a bit with the login via JWT module. This would make the JWT login a lot more flexible, like being able to validate tokens through a public/private keypair instead of a shared secret and map something else than the sub claim for the localpart.
sync somehow the display name after the initial registration ; maybe periodically, maybe after each login
get the user picture from the provider
handle logouts. This implies that we save the access/id/refresh tokens

sandhose · 2020-04-21T10:27:02Z

I've added some docs to test some providers and handled errors more nicely. It should be ready for review.

ptman · 2020-04-21T11:07:40Z

Excellect. I hope this will get merged soon. Not being familiar with hydra or dex, would testing with keycloak be useful? Keycloak can act as a translator between saml and oidc, which can be useful.

sandhose · 2020-04-21T11:28:10Z

Excellect. I hope this will get merged soon. Not being familiar with hydra or dex, would testing with keycloak be useful? Keycloak can act as a translator between saml and oidc, which can be useful.

Sure. I don't have a keycloak instance available for that, but it should work without problem.
Feel free add instructions on how to try it with Keycloak in docs/dev/oidc.md.

Conan-Kudo · 2020-04-22T13:46:08Z

@sandhose OIDC is perfectly fine. Ipsilon supports OIDC and OAuth2 as separate backends, and calling it OIDC makes it easier to understand what to use it with.

OvermindDL1 · 2020-04-23T02:48:40Z

Handling forced logout when the OIDC server mandates a logout would definitely be a wanted feature, too many security issues nowadays to not have that. ^.^;

richvdh · 2020-04-23T08:53:42Z

Handling forced logout when the OIDC server mandates a logout would definitely be a wanted feature, too many security issues nowadays to not have that. ^.^;

for the avoidance of doubt, this is probably true, but let's keep it as a follow up rather than adding further complexity to this PR.

clokep

First of all, thanks for the PR! It's long, so I expect it will take a few back and forth to get it into a state we're both happy with! I left some comments (and will try to answer the questions below), overall it looks pretty good though!

are the names I chose alright in general? I used OIDC pretty much everywhere, but it could be named OAuth2, as it can handle generic OAuth2 providers

I think OIDC is fine, putting it into a search engine / Wikipedia brings up pretty quickly how it is. It seems that OIDC is more generic than OAuth2? It might be nice to include (in the newsfragement?) that this includes OAuth2 providers though.

I used /_synapse/oidc/callback as a redirect_uri ; should that be something else?

I think that's reasonable.

to pass data through the OAuth2 flow, I save a cookie with some session informations encoded in a macaroon token. Is that the right approach?

I'm not sure on this, I'm guessing that unlike SAML / CAS the data can't be pushed through with the client redirect?

I used authlib to handle to handle some of the OAuth2/OIDC parts. I added it in synapse/python_dependencies.py. Is there somewhere else I should mention it?

I think that's enough!

I'm not sure how to handle errors properly. Raising SynapseError in an async function does not seem to do anything.

This should work fine, we do this frequently. What was not working about it?

to map the user infos from to provider to a mxid/display name, I used jinja templates that are configurable through the config file. Is that a good approach, or should I opt for something more similar than what is done in the SAML2 module?

I suspect being consistent with the SAML implementation is beneficial. The SAML implementation (of a plug-in) probably provides more flexibility too.

sync somehow the display name after the initial registration ; maybe periodically, maybe after each login

get the user picture from the provider

I don't think we do this with SAML / CAS currently?

handle logouts. This implies that we save the access/id/refresh tokens

I think it was mentioned above that this seems like a good follow-up.

One piece I see missing in here is handling the UI authentication process that was added in #5667. I think we'll definitely want this, but it might be OK to be added as a follow-up.

Also, do you have any good resources for the flow diagram of OpenID Connect? I think it'd help me understand the various stages as I'm reviewing this! Thanks!

synapse/rest/client/v1/login.py

docs/sample_config.yaml

synapse/app/homeserver.py

synapse/handlers/auth.py

synapse/handlers/oidc_handler.py

sandhose · 2020-04-24T08:24:12Z

I think OIDC is fine, putting it into a search engine / Wikipedia brings up pretty quickly how it is. It seems that OIDC is more generic than OAuth2? It might be nice to include (in the newsfragement?) that this includes OAuth2 providers though.

OAuth2 is a bunch of RFCs (mainly 6749) that define the authorization flows, but not where the endpoint should be, nor how user infos should work. OIDC defines that. For example, take a look at Google's OIDC discovery document, you'll see all the OAuth2 endpoints they have: https://accounts.google.com/.well-known/openid-configuration

I'm not sure on this, I'm guessing that unlike SAML / CAS the data can't be pushed through with the client redirect?

Well it can, that's the purpose of the state parameter, to persist data throughout the flow. The signed cookie is here to be sure that the request is not intercepted by another malicious app/browser and prevent CSRF attacks. See Auth0 docs about that, they say “The cookie in which the state value is stored should be signed to prevent attackers to forge it.”.

This should work fine, we do this frequently. What was not working about it?

I forget to decorate the handlers with @wrap_html_request_handler, that's why it was not working. :)

I suspect being consistent with the SAML implementation is beneficial. The SAML implementation (of a plug-in) probably provides more flexibility too.

sync somehow the display name after the initial registration ; maybe periodically, maybe after each login

get the user picture from the provider

I don't think we do this with SAML / CAS currently?

Nope, but that's definitely a thing we'd like to do in my company.

One piece I see missing in here is handling the UI authentication process that was added in #5667. I think we'll definitely want this, but it might be OK to be added as a follow-up.

I'll look into it.

Also, do you have any good resources for the flow diagram of OpenID Connect? I think it'd help me understand the various stages as I'm reviewing this! Thanks!

For the flow we are using (the authorization code flow), I think this is a good resource to understand it. For some of the OIDC specific parts, maybe look at this page.

Thanks for this first review, I think I have a lot of comments/docstrings to add in various functions :D
I'll work on it this afternoon/week-end.

clokep · 2020-04-24T10:59:47Z

Awesome! A couple more things after re-reading the above:

Doing display name / user picture sync sounds to me like a follow-up -- this PR is already getting big.
Let me know if you have questions on the UI authentication process bits, it also could probably be done as a follow-up, but we should at least make sure we don't back ourselves into a corner with the design.

ptman · 2020-04-24T11:26:38Z

I think displayname / avatar sync should maybe only be done on user creation or at least make it possible to not sync from the auth source. Often central auth can have a policy of only providing the name on official government ID, while people may want to be called a nickname in chat.

sandhose · 2020-04-24T22:18:15Z

@clokep I've resolved some of the remarks you did and added a lot of comments. Those should help you understand how the OIDC flow works. I think it's ready for another review.

synapse/handlers/auth.py

synapse/handlers/oidc_handler.py

clokep

Thanks for the extensive comments! I took another look and left some more comments, many are minor nits. There's also a few outstanding comments from the last review -- I've resolved all the ones that I see as done.

I also think the following was missed from above:

to map the user infos from to provider to a mxid/display name, I used jinja templates that are configurable through the config file. Is that a good approach, or should I opt for something more similar than what is done in the SAML2 module?

I suspect being consistent with the SAML implementation is beneficial. The SAML implementation (of a plug-in) probably provides more flexibility too.

synapse/handlers/oidc_handler.py

clokep · 2020-04-27T18:49:34Z

synapse/handlers/oidc_handler.py

+        if self._provider_needs_discovery:
+            url = get_well_known_url(self._provider_metadata["issuer"], external=True)
+            metadata_response = await self._http_client.get_json(url)
+            # TODO: maybe update the other way around to let user override some values?


Does this need additional thought?

I think that doing it the other way around was a lot harder when I wrote this comment, because there were a bunch of metadatas that were set not from the config. I'll see if I can do something there.

If this isn't easy we can just remove the comment. This could always be a future improvement if it is necessary.

synapse/handlers/oidc_handler.py

sandhose · 2020-05-03T11:25:42Z

I've extracted out the user mapping logic into a module dynamically loaded from the config. One thing I'm not entirely sure is if the map_user_attributes method should be async? It might make sense to want to fetch some user attributes somewhere asynchronously, maybe using the Token we just got from the provider.

I resolved most of the conversations in this PR, thanks @auscompgeek and @clokep for the reviews!

It might soon be time I rebase this PR since there are few conflicting files with develop. I'll wait the next review for that.

I should be able to do another round on Tuesday, so it would be awesome to have another review before that :)

sandhose · 2020-05-03T11:38:48Z

I had to rebase on develop and force-push because tests would fail because of merging conflicts, sorry about that.

The interesting commits start at 2fa9958 | OIDC login: custom modules for user mappings.

auscompgeek · 2020-05-03T13:30:21Z

synapse/handlers/oidc_handler.py

+
+env = Environment(finalize=jinja_finalize)
+
+JinjaOidcMappingConfig = TypedDict(


Is there a particular reason this is a TypedDict rather than a class?

Not really. Would it be better? I'm not really a python developer, so if you think that would make more sense, I can change that.

The SAML code gives a nicer way of doing this IMO:

synapse/synapse/handlers/saml_handler.py

Lines 292 to 295 in 207b173

@attr.s

class SamlConfig(object):

mxid_source_attribute = attr.ib()

mxid_mapper = attr.ib()

And then parse_config returns an instance of this class.

auscompgeek · 2020-05-03T13:30:46Z

synapse/http/client.py

@@ -434,6 +436,10 @@ def get_json(self, uri, args={}, headers=None):

            ValueError: if the response was not JSON
        """
+        actual_headers = {b"Accept": [b"application/json"]}


Hmm. I wonder whether this should also be setting the User-Agent.

That was a discussion on #synapse-dev:matrix.org. I initially had those in the handler because some providers expected to have that header and I thought it would make sense to have them by default on all *_json methods in the http client.

Oh, nevermind. Did not read your comment properly. That's actually done by SimpleHttpClient.get_raw.

docs/sample_config.yaml

clokep · 2020-05-04T13:22:09Z

docs/sample_config.yaml

+        # Jinja2 template for the localpart of the MXID
+        #
+        localpart: "{ user.preferred_username }"
+
+        # Jinja2 template for the display name to set on first login. Optional.
+        #
+        #display_name: "{ user.given_name } { user.last_name }"


It seems these had _template added to the end of them in the code, but not in the sample configs or the documentation.

I do have a fear that using jinja here is an added layer of complexity that is unnecessary. Is being able to specify a different module enough here?

I think that Jinja templates make sense because

it's already used in the HTML/email templates

it makes it easy to express some complex attributes combination ; given + family name is a good example for that

we could easily provide a bunch of builtin filters : hexencode, dotreplace (as seen in the SAML handler), a filter to extract the local part of a mxid/email address, etc.

I think the Jinja mapping provider is flexible enough to cover most of the use cases, but a simpler default provider would cover a lot less cases, meaning admins would have to write a module (with all the maintenance this applies) almost all the time

A solution between the two would be to have two mappers builtin: a simpler one that just extracts claims (as the SAML default one does), probably as default, and a Jinja-based one.

synapse/handlers/oidc_handler.py

This ensures the OP is behaving correctly and returns valid HTTP codes Signed-off-by: Quentin Gliech <[email protected]>

Signed-off-by: Quentin Gliech <[email protected]>

Co-authored-by: Patrick Cloke <[email protected]> Signed-off-by: Quentin Gliech <[email protected]>

Signed-off-by: Quentin Gliech <[email protected]>

Also passes the token as parameter of the mapping provider Signed-off-by: Quentin Gliech <[email protected]>

This allows to simplify the metadata edit code in tests and leverage unittest.mock.patch.dict Signed-off-by: Quentin Gliech <[email protected]>

Signed-off-by: Quentin Gliech <[email protected]>

sandhose · 2020-05-07T14:57:54Z

Phew, that should be it.

I rebased on develop

for the packaging issue in CI
change the commit author with my work email
edit the changelog entry (please tell me if it's OK)

So the only commit I really changed is OIDC login: add changelog entry, 7dccd63

Anyway, the new commits start at OIDC login: make the user attribute mapping async, 9bd40d1.

What was done (in commit order):

making the attribute mapper async, and add the token as parameter. I think it could make sense in some providers that might want to do authenticated requests to fetch some user attributes (9bd40d1)
changed the metadata validation a bit. Went from if "foo" in metadata to metadata.get("foo") is not None, which also simplified the tests a bit. (eace065)
refactored macaroon generation and verification ; so the MacaroonGenerator in auth_handler is now untouched by this PR (b3e7b6c)
added docstrings to tests. Not sure about the format, because I had trouble to find tests with docstrings (cfa177c)
some misc fixes based on the last review (0f5b4fd)

clokep

I think this is ready! 🥳 🎈

Thanks for working with us through the reviews! I'm going to give it another quick spin and merge this!

ptman · 2020-05-08T12:17:20Z

Excellent! Will this make it to 1.13 or will it wait for 1.14?

clokep · 2020-05-08T12:24:33Z

@ptman This will wait for 1.14, we'll be cutting an RC for 1.13 soon™️.

clokep · 2020-05-08T12:33:44Z

Thanks again for all your work on this! I believe we had mentioned a couple of follow-ups above, hopefully we have a good basis now to implement those!

…cache-config-without-synctl * 'develop' of github.com:matrix-org/synapse: (43 commits) Remove unused store method get_hosts_in_room (#7448) Don't UPGRADE database rows RST indenting Put rollback instructions in upgrade notes Fix changelog typo Oh yeah, RST Absolute URL it is then Fix upgrade notes link Provide summary of upgrade issues in changelog. Fix ) Move next version notes from changelog to upgrade notes Changelog fixes 1.13.0rc1 Documentation on setting up redis (#7446) Rework UI Auth session validation for registration (#7455) Extend spam checker to allow for multiple modules (#7435) Implement OpenID Connect-based login (#7256) Add room details admin endpoint (#7317) Fix errors from malformed log line (#7454) Drop support for redis.dbid (#7450) Fixes typo (bellow -> below) (#7449) ...

roderikelgodo · 2020-05-27T17:58:43Z

Hi. Thank you very much for such a great and hard work. I'm waiting the final release of this "OpenId connect-base login" implementation. Meanwhile I'm testing the branch develop with Dex, using Riot as a native ubuntu client. Everything seems to be working fine: authentication using an internal ldap or github works great (creating users in the database, etc). But the very last part of the process gets stuck in "Go to your browser to complete Sign In" (Riot part), and the "I trust this address" (web part) link that opens the Riot app with the "Go to your browser ...". So it keeps in a loop.

If I can be of any help for testing, count on that!

clokep · 2020-05-27T18:17:19Z

@roderikelgodo Are you using Riot Desktop? I think you're hitting element-hq/element-web#13719. If it isn't that, please open a new issue or join #synapse:matrix.org.

roderikelgodo · 2020-05-28T10:56:47Z

@clokep thank you very much for the answer. Yes, it is vector-im/riot-web#13719. After doing other test with other users, passing through Riot Web (and not through Riot Desktop), everything works fine.

mlwalsh1228 · 2020-05-29T00:50:30Z

Hello! This feature works so well! I'm using the standard Auth0 authorization code flow. There are a few things I had to modify to get it working, so please correct me if I've not configured something correctly!

_synapse/oidc was not able to be reached, and I've scanned much documentation to see if I needed to enable something to get the _synapse rest api working. I've modified the few places in code to be _matrix/oidc and this feature works perfect, but I must be doing something wrong?
Setting up the encryption key backup through Riot will require you to enter your password as one of the steps, which there isn't one in this case. I've checked upcoming changes to Riot, but did not see the handling for that? EDIT: Forgot to disable password login 🤦 Works like a charm.

So sorry to waste your time if I've done something incorrect! Let me know if I should file an issue

clokep · 2020-05-29T11:06:39Z

@mlwalsh1228 Do you have a reverse proxy that is blocking access to things not at _matrix?

Anyway, I'm going to lock this issue, please don't use closed PRs for questions. If you believe there's a bug please open a separate issue, for support requests please use #synapse:matrix.org.

clokep requested a review from a team April 17, 2020 20:00

sandhose marked this pull request as ready for review April 21, 2020 10:27

ara4n mentioned this pull request Apr 21, 2020

openid connect #6182

Closed

clokep self-assigned this Apr 23, 2020

clokep suggested changes Apr 23, 2020

View reviewed changes

sandhose requested a review from clokep April 24, 2020 22:19

auscompgeek reviewed Apr 25, 2020

View reviewed changes

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

auscompgeek reviewed Apr 25, 2020

View reviewed changes

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

auscompgeek reviewed Apr 25, 2020

View reviewed changes

synapse/handlers/oidc_handler.py Show resolved Hide resolved

clokep suggested changes Apr 27, 2020

View reviewed changes

sandhose requested a review from clokep May 3, 2020 11:25

auscompgeek reviewed May 3, 2020

View reviewed changes

clokep reviewed May 4, 2020

View reviewed changes

docs/sample_config.yaml Outdated Show resolved Hide resolved

clokep reviewed May 4, 2020

View reviewed changes

docs/sample_config.yaml Outdated Show resolved Hide resolved

clokep reviewed May 4, 2020

View reviewed changes

synapse/handlers/oidc_handler.py Show resolved Hide resolved

clokep reviewed May 4, 2020

View reviewed changes

synapse/handlers/oidc_handler.py Show resolved Hide resolved

sandhose and others added 12 commits May 7, 2020 16:40

OIDC login: proper error handling in code exchange

543e046

This ensures the OP is behaving correctly and returns valid HTTP codes Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: more tests

ea5d71d

Signed-off-by: Quentin Gliech <[email protected]>

Apply suggestions from code review

2181fac

Co-authored-by: Patrick Cloke <[email protected]> Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: fix sample config templates

02f8dc0

Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: log throughout the process

a6b6bd9

Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: check the validity of the auth method

be7b732

Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: fix dev docs & add Twitch example

c358f72

Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: make the user attribute mapping async

9bd40d1

Also passes the token as parameter of the mapping provider Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: check for None values in metadata

eace065

This allows to simplify the metadata edit code in tests and leverage unittest.mock.patch.dict Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: refactor macaroon generation/verification

b3e7b6c

Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: add docstrings to tests

cfa177c

Signed-off-by: Quentin Gliech <[email protected]>

OIDC login: various fixes from PR review

0f5b4fd

Signed-off-by: Quentin Gliech <[email protected]>

sandhose requested a review from clokep May 7, 2020 14:58

clokep approved these changes May 8, 2020

View reviewed changes

clokep merged commit 616af44 into matrix-org:develop May 8, 2020

clokep mentioned this pull request May 8, 2020

There is no OAuth2 password provider #2376

Closed

This was referenced May 8, 2020

Support UI Authentication for OpenID Connect accounts #7457

Merged

Update documentation about SSO mapping providers #7458

Merged

hungrymonkey mentioned this pull request May 27, 2020

Keycloak/SSO together with a Matrix - mxisd or matrix-synapse-rest-auth REST authentication password provider module? spantaleev/matrix-docker-ansible-deploy#96

Closed

matrix-org locked as resolved and limited conversation to collaborators May 29, 2020


		env = Environment(finalize=jinja_finalize)

		JinjaOidcMappingConfig = TypedDict(

	@attr.s
	class SamlConfig(object):
	mxid_source_attribute = attr.ib()
	mxid_mapper = attr.ib()

Implement OpenID Connect-based login #7256

Implement OpenID Connect-based login #7256

Conversation

sandhose commented Apr 9, 2020 • edited Loading

Pull Request Checklist

sandhose commented Apr 17, 2020

sandhose commented Apr 21, 2020

ptman commented Apr 21, 2020

sandhose commented Apr 21, 2020

Conan-Kudo commented Apr 22, 2020

OvermindDL1 commented Apr 23, 2020

richvdh commented Apr 23, 2020

clokep left a comment • edited Loading

Choose a reason for hiding this comment

sandhose commented Apr 24, 2020

clokep commented Apr 24, 2020

ptman commented Apr 24, 2020

sandhose commented Apr 24, 2020

clokep 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

sandhose commented May 3, 2020

sandhose commented May 3, 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

sandhose May 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandhose commented May 7, 2020

clokep left a comment

Choose a reason for hiding this comment

ptman commented May 8, 2020

clokep commented May 8, 2020

clokep commented May 8, 2020

roderikelgodo commented May 27, 2020

clokep commented May 27, 2020

roderikelgodo commented May 28, 2020

mlwalsh1228 commented May 29, 2020 • edited Loading

clokep commented May 29, 2020

sandhose commented Apr 9, 2020 •

edited

Loading

clokep left a comment •

edited

Loading

sandhose May 3, 2020 •

edited

Loading

mlwalsh1228 commented May 29, 2020 •

edited

Loading