Add OAuth2 support #7059

maxklenk · 2020-03-10T17:28:42Z

It is now possible to register and login in using the OAuth2 Authorization Code Flow.

Things to discuss:

add support for custom mapping providers similar to the ones used in the SAML integration
use of an oAuth library

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)

Signed-off-by: Max Klenk max@klenk.biz

richvdh

this looks really great - thank you! A few stylistic bits and pieces below.

I'm not really familiar with OAuth2, so I'm largely taking it on trust that this does what it is supposed to do. Are there any well-known services (google? github?) that this will work with?

A document in the docs folder giving a few notes on how to configure OAuth2 integration might be helpful?

richvdh · 2020-03-30T17:44:47Z

synapse/handlers/oauth2_handler.py

+# limitations under the License.
+import logging
+
+from six.moves import urllib


we don't need to support python 2 any more; please use urllib directly rather than using six.

richvdh · 2020-03-30T17:45:48Z

synapse/handlers/oauth2_handler.py

+        return userinfo
+
+    def get_server_redirect_url(self):
+        return self.public_baseurl + b"_matrix/client/r0/login/oauth/response"


can we not store this in a field in the constructor, rather than having to call a method?

richvdh · 2020-03-30T17:45:50Z

synapse/handlers/oauth2_handler.py

+        )
+        return result
+
+    async def get_access_token(self, oauth2_code):


for internal methods like this, please could you prefix them with _ to show that they are internal.

(alternatively, if they form part of the public API of the class, they need docstrings.)

(tbh it could do with a docstring anyway: in this case it should clarify that this method is fetching the OAuth2 access token rather than a matrix access token.)

richvdh · 2020-03-30T17:46:35Z

synapse/handlers/oauth2_handler.py

+        self.oauth2_server_userinfo_url = hs.config.oauth2_server_userinfo_url
+        self.oauth2_client_id = hs.config.oauth2_client_id
+        self.oauth2_client_secret = hs.config.oauth2_client_secret
+        self.oauth2_scope = "openid"


why are these object-level fields rather than file-level constants?

The response_type and response_mode should be hardcoded as the code flow is the only one implemented yet. The scope on the other hand should be configurable.

richvdh · 2020-03-30T17:49:12Z

synapse/handlers/oauth2_handler.py

+        self.oauth2_response_type = "code"
+        self.oauth2_response_mode = "query"
+
+        # state


please can you document the structure of this. what does it map from and to?

richvdh · 2020-03-30T17:50:46Z

synapse/config/oauth2.py

@@ -0,0 +1,57 @@
+# -*- coding: utf-8 -*-
+# Copyright 2015, 2016 OpenMarket Ltd


/me checks calendar.

Also please don't assign copyright for new code to OpenMarket unless you work for them? You can either put your own name here or omit the copyright line altogether.

richvdh · 2020-03-30T17:50:54Z

synapse/handlers/oauth2_handler.py

@@ -0,0 +1,154 @@
+# -*- coding: utf-8 -*-
+# Copyright 2019 The Matrix.org Foundation C.I.C.


likewise, it's not really sensible to put Matrix.org here unless you work for them.

richvdh · 2020-03-30T17:53:18Z

synapse/rest/client/v1/login.py

@@ -600,5 +624,8 @@ def register_servlets(hs, http_server):
    if hs.config.cas_enabled:
        CasRedirectServlet(hs).register(http_server)
        CasTicketServlet(hs).register(http_server)
+    elif hs.config.oauth2_enabled:


we probably need to add a check somewhere that will make synapse refuse to start if you try to enable more than one of CAS/OAuth/SAML2.

richvdh · 2020-03-30T17:56:51Z

docs/sample_config.yaml

@@ -1367,6 +1367,18 @@ saml2_config:
 #   #    name: value


+
+# Enable OAuth2 for registration and login.


please don't follow the poor example of the CAS config block: there are conventions defined for the config file at https://github.com/matrix-org/synapse/blob/master/docs/code_style.md#configuration-file-format.

richvdh · 2020-03-30T17:57:45Z

synapse/rest/client/v1/login.py

+
+
+class OAuth2ResponseServlet(RestServlet):
+    PATTERNS = client_patterns("/login/oauth/response", v1=True)


once again, the CAS code sets a poor example here. This doesn't form part of the matrix client-server API, so it should be on a different path. Have a look at the way the SAML2ResponseResource works.

richvdh

I pressed the wrong button ^

maxklenk · 2020-03-31T07:41:32Z

Thank you for the review. I will improve my code and come back to you once I finished the fixes or have additional questions.

sandhose · 2020-04-08T08:12:46Z

Hey @maxklenk. I started working on kinda the same thing this week (implementing OpenID Connect, which is ~basically OAuth2) and did not saw this PR before working on it. Do you mind if continue my work and submit a PR by the end of the week? It should also cover your use case, but be a lot more flexible

sandhose · 2020-04-08T08:27:31Z

The main things I don't really know what the right approach is, is how to carry state throughout the login flow.
If we want the implementation to be secure, when we redirect the user to the provider, we need to generate a random state and a random nonce field, save them client-side, and verify them when the users comes back. Usually this is done with cookies.

@richvdh I'd like your input on this. Would it be OK to store a cookie through the flow to save some infos about it? Maybe a macaroon token with those infos in them might be a good candidate for that

Also, there are multiple approach to get the user infos:

via the id_token, exchanged through the token endpoint
via the id_token with the implicit flow
via the user_info endpoint

Ideally, all of those should be supported.

The scopes should be configurable, as well as the claims (~user attributes) are mapped.

The OpenID Connect spec also specifies a way to logout from Relying Parties (in this case Synapse), initiated by the provider. This implies that we store the token we get from the provider so we can revoke the matrix access token when the user logs out from the OIDC Provider

sandhose · 2020-04-08T09:08:58Z

synapse/handlers/oauth2_handler.py

+        ).encode("ascii")
+        return b"%s?%s" % (self.oauth2_server_authorization_url, service_param)
+
+    async def handle_oauth2_response(self, request):


This should also handle oauth2 errors

sandhose · 2020-04-08T09:11:20Z

synapse/handlers/oauth2_handler.py

+        """
+
+        oauth2_nonce = random_string(12)
+        self.nonces[oauth2_nonce] = {"redirectUrl": client_redirect_url}


the nonce and the state fields should be two different things and should be checked somehow when the clients return (usually with a cookie). Also, I'm not sure if storing that in memory is a good idea, as it means this servlet can't be scaled, and restarting it will make all the running auth flows fail.

sandhose · 2020-04-08T09:12:12Z

synapse/handlers/oauth2_handler.py

+        userinfo = await self.get_userinfo(access_token)
+
+        user = "sso_" + userinfo.get("sub")
+        displayname = userinfo.get("preferred_username")


That should not be hard-coded. Also, only the sub field is guaranteed to be here.

richvdh · 2020-04-08T10:23:20Z

If we want the implementation to be secure, when we redirect the user to the provider, we need to generate a random state and a random nonce field, save them client-side, and verify them when the users comes back. Usually this is done with cookies.

Interesting. We've not needed this for CAS or SAML (for SAML there is an in_response_to field which allows us to track sessions in the server; for CAS we stick an extra parameter on the service param which tells the CAS server where to redirect back to). I wonder if either of those techniques could be used here?

In principle though, there is no problem with storing a cookie for the duration of the login process.

sandhose · 2020-04-08T10:46:55Z

Interesting. We've not needed this for CAS or SAML (for SAML there is an in_response_to field which allows us to track sessions in the server; for CAS we stick an extra parameter on the service param which tells the CAS server where to redirect back to). I wonder if either of those techniques could be used here?

The state field is given in the initial authorization redirect by the Relying Party (synapse) and then restituted by the OpenID Provider on the callback URI. This was designed to indeed carry state throughout the flow, but also prevent CSRF attacks.

See https://auth0.com/docs/protocols/oauth2/oauth-state

In principle though, there is no problem with storing a cookie for the duration of the login process.

It is also recommended to have the cookie saving the state signed to prevent it from being forged. What would you think of using a macaroon token, with the state, the nonce and the client_redirect_url in it stored as a cookie? That way we don't have to store any state server-side

clokep · 2020-05-04T19:40:18Z

@maxklenk Thanks for your contribution, it seems that this PR has been superseded by #7256, which implements a more general approach for OpenID Connect, not just OAuth2. Will that PR fulfill all your requirements? If so, I think we can close this.

maxklenk · 2020-05-04T21:54:54Z

Yes, I stopped working on the PR and instead waited for #7256. I have tested @sandhose PR with additional OAuth providers and are confident that it contains everything I was aiming at with my PR. This PR can be closed.

clokep · 2020-05-05T11:14:13Z

Thanks for doing some additional testing! 👍

maxklenk added 5 commits March 10, 2020 15:48

add oauth2 code flow handling

f294e32

add oauth state validation

9a8fc87

add changelog file

8474b32

generate sample_config

bbd0395

Merge branch 'develop' into feature/oauth2

02f736e

babolivier requested a review from a team March 10, 2020 18:53

richvdh reviewed Mar 30, 2020

View reviewed changes

richvdh suggested changes Mar 30, 2020

View reviewed changes

sandhose reviewed Apr 8, 2020

View reviewed changes

sandhose mentioned this pull request Apr 9, 2020

Implement OpenID Connect-based login #7256

Merged

11 tasks

ptman mentioned this pull request Apr 29, 2020

There is no OAuth2 password provider #2376

Closed

maxklenk closed this May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OAuth2 support #7059

Add OAuth2 support #7059

maxklenk commented Mar 10, 2020 •

edited

Loading

richvdh left a comment

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

sandhose Apr 8, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh Mar 30, 2020

richvdh left a comment

maxklenk commented Mar 31, 2020

sandhose commented Apr 8, 2020

sandhose commented Apr 8, 2020

sandhose Apr 8, 2020

sandhose Apr 8, 2020

sandhose Apr 8, 2020

richvdh commented Apr 8, 2020

sandhose commented Apr 8, 2020

clokep commented May 4, 2020

maxklenk commented May 4, 2020

clokep commented May 5, 2020

		@@ -0,0 +1,57 @@
		# -- coding: utf-8 --
		# Copyright 2015, 2016 OpenMarket Ltd

		@@ -0,0 +1,154 @@
		# -- coding: utf-8 --
		# Copyright 2019 The Matrix.org Foundation C.I.C.

		@@ -1367,6 +1367,18 @@ saml2_config:
		# # name: value



		# Enable OAuth2 for registration and login.



		class OAuth2ResponseServlet(RestServlet):
		PATTERNS = client_patterns("/login/oauth/response", v1=True)

Add OAuth2 support #7059

Add OAuth2 support #7059

Conversation

maxklenk commented Mar 10, 2020 • edited Loading

Pull Request Checklist

richvdh 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

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

richvdh left a comment

Choose a reason for hiding this comment

maxklenk commented Mar 31, 2020

sandhose commented Apr 8, 2020

sandhose commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Apr 8, 2020

sandhose commented Apr 8, 2020

clokep commented May 4, 2020

maxklenk commented May 4, 2020

clokep commented May 5, 2020

maxklenk commented Mar 10, 2020 •

edited

Loading