-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Update JWT login type to support JWKS, custom sub claim, and encode special chars in user ID #9493
Changes from all commits
7bd41fa
4ab669d
da2bfb1
d8920fd
3d43b53
a1ebec5
22577bc
a77ebc9
6a52891
66f12d0
e706eb4
987431a
93acad2
c8c4a11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Update JWT login type to support JSON Web Key Sets (JWKS), custom sub claim, and option to encode unsupported characters in user ID. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |||||||||||||||
| from synapse.http.site import SynapseRequest | ||||||||||||||||
| from synapse.rest.client.v2_alpha._base import client_patterns | ||||||||||||||||
| from synapse.rest.well_known import WellKnownBuilder | ||||||||||||||||
| from synapse.types import JsonDict, UserID | ||||||||||||||||
| from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart | ||||||||||||||||
|
|
||||||||||||||||
| if TYPE_CHECKING: | ||||||||||||||||
| from synapse.server import HomeServer | ||||||||||||||||
|
|
@@ -56,6 +56,9 @@ def __init__(self, hs: "HomeServer"): | |||||||||||||||
| # JWT configuration variables. | ||||||||||||||||
| self.jwt_enabled = hs.config.jwt_enabled | ||||||||||||||||
| self.jwt_secret = hs.config.jwt_secret | ||||||||||||||||
| self.jwt_jwks_uri = hs.config.jwt_jwks_uri | ||||||||||||||||
| self.jwt_subject_claim = hs.config.jwt_subject_claim | ||||||||||||||||
| self.jwt_normalize_user_id = hs.config.jwt_normalize_user_id | ||||||||||||||||
| self.jwt_algorithm = hs.config.jwt_algorithm | ||||||||||||||||
| self.jwt_issuer = hs.config.jwt_issuer | ||||||||||||||||
| self.jwt_audiences = hs.config.jwt_audiences | ||||||||||||||||
|
|
@@ -319,11 +322,19 @@ async def _do_jwt_login(self, login_submission: JsonDict) -> Dict[str, str]: | |||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| import jwt | ||||||||||||||||
| from jwt import PyJWKClient | ||||||||||||||||
|
|
||||||||||||||||
| key = self.jwt_secret | ||||||||||||||||
|
|
||||||||||||||||
| 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) | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this make a synchronous HTTP call? If so we ideally would do this via a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fetching this for every login seems quite inefficient. I wonder if we should do something on start-up (like OIDC). It looks like |
||||||||||||||||
| key = signing_key.key | ||||||||||||||||
|
boti7 marked this conversation as resolved.
|
||||||||||||||||
|
|
||||||||||||||||
| try: | ||||||||||||||||
| payload = jwt.decode( | ||||||||||||||||
| token, | ||||||||||||||||
| self.jwt_secret, | ||||||||||||||||
| key, | ||||||||||||||||
| algorithms=[self.jwt_algorithm], | ||||||||||||||||
| issuer=self.jwt_issuer, | ||||||||||||||||
| audience=self.jwt_audiences, | ||||||||||||||||
|
|
@@ -336,10 +347,14 @@ async def _do_jwt_login(self, login_submission: JsonDict) -> Dict[str, str]: | |||||||||||||||
| errcode=Codes.FORBIDDEN, | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| user = payload.get("sub", None) | ||||||||||||||||
| subject_claim = self.jwt_subject_claim or "sub" | ||||||||||||||||
| user = payload.get(subject_claim, None) | ||||||||||||||||
|
Comment on lines
+350
to
+351
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already did the fallback to
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that tests don't use the config code but set
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The config code should get run during tests, see: Lines 469 to 472 in fe604a0
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were you able to try this again? It should run fine during tests. |
||||||||||||||||
| if user is None: | ||||||||||||||||
| raise LoginError(403, "Invalid JWT", errcode=Codes.FORBIDDEN) | ||||||||||||||||
|
|
||||||||||||||||
| if self.jwt_normalize_user_id: | ||||||||||||||||
| user = map_username_to_mxid_localpart(user) | ||||||||||||||||
|
|
||||||||||||||||
| user_id = UserID(user, self.hs.hostname).to_string() | ||||||||||||||||
| result = await self._complete_login( | ||||||||||||||||
| user_id, login_submission, create_non_existent_users=True | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked the packages whether this change would be OK. Can you remind me why we're bumping this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like supported versions of Debian and Fedora are on 1.7.1 still. Would it be possible to use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyJWKClientwas 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#v210There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what to do about this, although I do wonder if we should be using
PyJWKClientsince it bypasses the reactor / current methods for communicating with external resources.