Update JWT login type to support JWKS, custom sub claim, and encode special chars in user ID by boti7 · Pull Request #9493 · matrix-org/synapse

boti7 · 2021-02-25T00:23:26Z

This PR adds the following optional fields to the JWT login type configuration:

jwks_uri allows specifying a JSON Web Key Set endpoint where RSA signing keys can be retrieved from, instead of using a secret or public key from the configuration.
subject_claim allows specifying a claim other than the default "sub" to use as localpart of the Matrix ID.
normalize_user_id enables mapping the localpart from other character sets using the map_username_to_mxid_localpart function.

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: Botond Horvath hello@boti7.com

Signed-off-by: Botond Horvath <hello@boti7.com>

… ID for JWT logins Signed-off-by: Botond Horvath <hello@boti7.com>

…logins Signed-off-by: Botond Horvath <hello@boti7.com>

Signed-off-by: Botond Horvath <hello@boti7.com>

clokep · 2021-02-25T12:08:34Z

Looks like reasonable improvements! 👍 This seems broken on Python 3.5 (which pyjwt technically doesn't support: jpadilla/pyjwt#585, Synapse will be dropping it shortly). Also could you see if any of the documentation under docs/jwt.md needs to be updated? (I don't know if we need to detail all the options there + the sample config, but at least mentioning some of the additional features likely makes sense.)

clokep · 2021-03-08T13:29:26Z

@boti7 It seems that some unrelated changes were pushed onto this branch for CI. If those are improvements that would be useful we should do them as a separate PR.

Did you see my comment above? Note that we'll be dropping support for Python 3.5 soon, so might make sense to just wait on that if possible.

worldofgeese · 2021-03-08T13:34:55Z

@clokep this was my fault. We'll roll it all back.

clokep · 2021-03-08T13:45:13Z

(Also merging in develop should fix the mypy issues.)

worldofgeese · 2021-03-08T13:48:35Z

@clokep I've hard reset to @boti7's last change. Let me know if there's anything else I can do.

clokep

Looks pretty reasonable overall. I suspect it would be a nice follow-up to move the JWT code into a separate handler instead of sprinkling it into LoginRestServlet.

synapse/config/jwt_config.py

synapse/rest/client/v1/login.py

clokep · 2021-03-08T16:35:33Z

synapse/rest/client/v1/login.py

+
+        if not key and self.jwt_jwks_uri:
+            jwks_client = PyJWKClient(self.jwt_jwks_uri)
+            signing_key = jwks_client.get_signing_key_from_jwt(token)


Does this make a synchronous HTTP call? If so we ideally would do this via a SimpleHttpClient or push this into the background.

Yes, seems like it makes a synchronous HTTP call. It is also possible to implement the loading and parsing of JWKS without PyJWT, like in OidcHandler, then we have more control over the request and caching.

Fetching this for every login seems quite inefficient. I wonder if we should do something on start-up (like OIDC). It looks like PyJWKClient is quite simple!

boti7 · 2021-03-09T14:08:47Z

I just made a small change to docs/jwt.md and I think the other options are clear from the sample config, but let me know if you want me to add anything else.

From my side it's totally fine to wait until you drop support for Python 3.5. Also merged in develop, but there are still some mypy issues.

anoadragon453 · 2021-04-09T11:05:03Z

We've now dropped Python 3.5 support 🎉

erikjohnston · 2021-05-20T10:10:27Z

I think we're still waiting for some changes here? If nothing else there is a merge conflict. @boti7 please nudge if/when its ready for review 🙂

e-001 · 2021-06-17T20:30:40Z

@boti7 I'll clean up this conflict and PR. Desperately need escaped sub claim. Auth0 lovingly uses a pipe to separate auth method and ID.

e-001 · 2021-06-17T22:01:23Z

a77ebc9 Caught up to develop and refactored for 683d6f7. Waiting on mqsdk/synapse#1.

Rebase JWT-auth changes onto upstream/develop

erikjohnston · 2021-06-18T08:57:04Z

Looks like the JWT tests are failing when using old-deps, possibly need to bump the minimum version of the library?

boti7 · 2021-06-19T16:00:58Z

@erikjohnston It's ready for review from my side.

clokep

Looks pretty good overall! I wonder if there's enough JWT logic here now that we should move this code to a separate JwtHandler. What do you think?

synapse/config/jwt.py

clokep · 2021-06-29T19:15:49Z

synapse/python_dependencies.py

    "sentry": ["sentry-sdk>=0.7.2"],
    "opentracing": ["jaeger-client>=4.0.0", "opentracing>=2.2.0"],
-    "jwt": ["pyjwt>=1.6.4"],
+    "jwt": ["pyjwt>=2.1.0"],


I asked the packages whether this change would be OK. Can you remind me why we're bumping this?

Looks like supported versions of Debian and Fedora are on 1.7.1 still. Would it be possible to use that?

PyJWKClient was added in v2.0.0 and caching of signing keys is supported since v2.1.0: https://github.com/jpadilla/pyjwt/blob/master/CHANGELOG.rst#v210

I'm not really sure what to do about this, although I do wonder if we should be using PyJWKClient since it bypasses the reactor / current methods for communicating with external resources.

clokep · 2021-06-29T19:18:49Z

synapse/rest/client/v1/login.py

+
+        if not key and self.jwt_jwks_uri:
+            jwks_client = PyJWKClient(self.jwt_jwks_uri)
+            signing_key = jwks_client.get_signing_key_from_jwt(token)


Fetching this for every login seems quite inefficient. I wonder if we should do something on start-up (like OIDC). It looks like PyJWKClient is quite simple!

clokep · 2021-06-29T19:20:06Z

synapse/rest/client/v1/login.py

+        subject_claim = self.jwt_subject_claim or "sub"
+        user = payload.get(subject_claim, None)


We already did the fallback to sub in the config code, no need to do it again.

Suggested change

subject_claim = self.jwt_subject_claim or "sub"

user = payload.get(subject_claim, None)

user = payload.get(self.jwt_subject_claim, None)

The issue is that tests don't use the config code but set hs.config directly, so without specifying a fallback here, many tests in JWTTestCase would fail. What do you suggest to solve this?

The config code should get run during tests, see:

synapse/tests/unittest.py

Lines 469 to 472 in fe604a0

# Parse the config from a config dict into a HomeServerConfig

config_obj = HomeServerConfig()

config_obj.parse_config_dict(config, "", "")

kwargs["config"] = config_obj

Were you able to try this again? It should run fine during tests.

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

clokep · 2021-07-26T14:11:31Z

Removing my review -- this has a couple of open questions still that are waiting for responses!

anoadragon453 · 2021-08-18T10:32:23Z

Hey @boti7, are you able to continue working on this? 🙂

Also note that there is now a conflict in synapse/rest/client/v1/login.py. You'll need to simply merge the develop branch into yours to resolve it.

anoadragon453 · 2021-09-14T15:43:27Z

Given the lack of activity, I'm going to close this. @boti7 feel free to reopen if you'd like to continue working on this!

Anyone else is free to pick it back up in a new PR as well 🙂

boti7 added 4 commits February 24, 2021 15:34

Add option to normalize user ID for JWT logins

7bd41fa

Signed-off-by: Botond Horvath <hello@boti7.com>

Add option to customize the claim used for the local part of the user…

4ab669d

… ID for JWT logins Signed-off-by: Botond Horvath <hello@boti7.com>

Add option to retrieve RSA signing keys from a JWKS endpoint for JWT …

da2bfb1

…logins Signed-off-by: Botond Horvath <hello@boti7.com>

Add changelog entry

d8920fd

Signed-off-by: Botond Horvath <hello@boti7.com>

clokep requested a review from a team March 8, 2021 13:54

clokep reviewed Mar 8, 2021

View reviewed changes

boti7 added 2 commits March 9, 2021 12:50

Update docs/jwt.md

3d43b53

Merge remote-tracking branch 'upstream/develop' into feature/jwt-auth

a1ebec5

erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label May 20, 2021

e-001 added 2 commits June 17, 2021 16:46

Merge branch 'feature/jwt-auth' into develop

22577bc

Rename jwt_config.py as done in 683d6f

a77ebc9

Merge pull request #1 from Enigma-Labs/feature/jwt-auth-develop

6a52891

Rebase JWT-auth changes onto upstream/develop

clokep mentioned this pull request Jun 18, 2021

Add option to normalize user ID for JWT logins #10203

Closed

4 tasks

boti7 added 3 commits June 19, 2021 14:49

Check if either a secret or a jwks uri is provided in jwt_config

66f12d0

Default JWT subject claim field to "sub"

e706eb4

Increase minimum version of PyJWT library to 2.1.0

987431a

richvdh requested a review from a team June 21, 2021 10:25

erikjohnston removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 22, 2021

clokep suggested changes Jun 29, 2021

View reviewed changes

boti7 and others added 2 commits June 29, 2021 21:39

Change spelling in sample config

93acad2

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

Update docs/sample_config.yaml

c8c4a11

erikjohnston requested a review from clokep July 23, 2021 16:16

clokep removed their request for review July 26, 2021 14:11

clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 26, 2021

anoadragon453 closed this Sep 14, 2021

vrinek mentioned this pull request Nov 16, 2021

Customize claim for local part of JWT logins #11361

Merged

4 tasks

clokep mentioned this pull request May 24, 2022

New login flow: Use OAuth2 JWT as external identifier #12830

Closed

4 tasks

		subject_claim = self.jwt_subject_claim or "sub"
		user = payload.get(subject_claim, None)

	subject_claim = self.jwt_subject_claim or "sub"
	user = payload.get(subject_claim, None)
	user = payload.get(self.jwt_subject_claim, None)

	# Parse the config from a config dict into a HomeServerConfig
	config_obj = HomeServerConfig()
	config_obj.parse_config_dict(config, "", "")
	kwargs["config"] = config_obj

Uh oh!

Conversation

boti7 commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

clokep commented Feb 25, 2021

Uh oh!

clokep commented Mar 8, 2021

Uh oh!

worldofgeese commented Mar 8, 2021

Uh oh!

clokep commented Mar 8, 2021

Uh oh!

worldofgeese commented Mar 8, 2021

Uh oh!

clokep left a comment

Choose a reason for hiding this comment

Uh oh!

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!

boti7 commented Mar 9, 2021

Uh oh!

anoadragon453 commented Apr 9, 2021

Uh oh!

erikjohnston commented May 20, 2021

Uh oh!

e-001 commented Jun 17, 2021

Uh oh!

e-001 commented Jun 17, 2021

Uh oh!

erikjohnston commented Jun 18, 2021

Uh oh!

boti7 commented Jun 19, 2021

Uh oh!

clokep 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!

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!

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!

clokep commented Jul 26, 2021

Uh oh!

anoadragon453 commented Aug 18, 2021

Uh oh!

anoadragon453 commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

boti7 commented Feb 25, 2021 •

edited

Loading

anoadragon453 commented Sep 14, 2021 •

edited

Loading