From ca934035d680e14fb61197f67ac9ed6b14412033 Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Tue, 16 Nov 2021 19:06:12 +0100 Subject: [PATCH 01/12] Customize claim for local part of JWT logins --- changelog.d/9493.feature | 1 + docs/jwt.md | 5 +++-- docs/sample_config.yaml | 6 ++++++ synapse/config/jwt.py | 8 ++++++++ synapse/rest/client/login.py | 4 +++- tests/rest/client/test_login.py | 22 ++++++++++++++++++++++ 6 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 changelog.d/9493.feature diff --git a/changelog.d/9493.feature b/changelog.d/9493.feature new file mode 100644 index 000000000000..5cf6f146f85b --- /dev/null +++ b/changelog.d/9493.feature @@ -0,0 +1 @@ +Update JWT login type to support custom sub claim. diff --git a/docs/jwt.md b/docs/jwt.md index 5be9fd26e331..32f58cc0cbbf 100644 --- a/docs/jwt.md +++ b/docs/jwt.md @@ -22,8 +22,9 @@ will be removed in a future version of Synapse. The `token` field should include the JSON web token with the following claims: -* The `sub` (subject) claim is required and should encode the local part of the - user ID. +* A claim that encodes the local part of the user ID is required. By default, + the `sub` (subject) claim is used, or a custom claim can be set in the + configuration file. * The expiration time (`exp`), not before time (`nbf`), and issued at (`iat`) claims are optional, but validated if present. * The issuer (`iss`) claim is optional, but required and validated if configured. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index d48c08f1d95f..2e6f7cfbb86a 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -2030,6 +2030,12 @@ sso: # #secret: "provided-by-your-issuer" + # Name of the claim containing a unique identifier for the user. + # + # Optional, defaults to `sub`. + # + #subject_claim: "sub" + # The algorithm used to sign the JSON web token. # # Supported algorithms are listed at diff --git a/synapse/config/jwt.py b/synapse/config/jwt.py index 9d295f5856e6..502f44c1744c 100644 --- a/synapse/config/jwt.py +++ b/synapse/config/jwt.py @@ -33,6 +33,7 @@ def read_config(self, config, **kwargs): # The issuer and audiences are optional, if provided, it is asserted # that the claims exist on the JWT. + self.jwt_subject_claim = jwt_config.get("subject_claim", "sub") self.jwt_issuer = jwt_config.get("issuer") self.jwt_audiences = jwt_config.get("audiences") @@ -48,6 +49,7 @@ def read_config(self, config, **kwargs): self.jwt_algorithm = None self.jwt_issuer = None self.jwt_audiences = None + self.jwt_subject_claim = None def generate_config_section(self, **kwargs): return """\ @@ -79,6 +81,12 @@ def generate_config_section(self, **kwargs): # #secret: "provided-by-your-issuer" + # Name of the claim containing a unique identifier for the user. + # + # Optional, defaults to `sub`. + # + #subject_claim: "sub" + # The algorithm used to sign the JSON web token. # # Supported algorithms are listed at diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 467444a04135..3f91450e5f12 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -72,6 +72,7 @@ def __init__(self, hs: "HomeServer"): # JWT configuration variables. self.jwt_enabled = hs.config.jwt.jwt_enabled self.jwt_secret = hs.config.jwt.jwt_secret + self.jwt_subject_claim = hs.config.jwt.jwt_subject_claim self.jwt_algorithm = hs.config.jwt.jwt_algorithm self.jwt_issuer = hs.config.jwt.jwt_issuer self.jwt_audiences = hs.config.jwt.jwt_audiences @@ -413,7 +414,8 @@ async def _do_jwt_login( errcode=Codes.FORBIDDEN, ) - user = payload.get("sub", None) + subject_claim = self.jwt_subject_claim or "sub" + user = payload.get(subject_claim, None) if user is None: raise LoginError(403, "Invalid JWT", errcode=Codes.FORBIDDEN) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 0b90e3f80323..b8fa2ceed251 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -962,6 +962,28 @@ def test_login_aud_no_config(self): channel.json_body["error"], "JWT validation failed: Invalid audience" ) + def test_login_default_sub(self): + """Test reading user ID from the default subject claim.""" + channel = self.jwt_login({"sub": "kermit"}) + self.assertEqual(channel.result["code"], b"200", channel.result) + self.assertEqual(channel.json_body["user_id"], "@kermit:test") + + @override_config( + { + "jwt_config": { + "jwt_enabled": True, + "secret": jwt_secret, + "algorithm": jwt_algorithm, + "subject_claim": "username", + } + } + ) + def test_login_custom_sub(self): + """Test reading user ID from a custom subject claim.""" + channel = self.jwt_login({"username": "frog"}) + self.assertEqual(channel.result["code"], b"200", channel.result) + self.assertEqual(channel.json_body["user_id"], "@frog:test") + def test_login_no_token(self): params = {"type": "org.matrix.login.jwt"} channel = self.make_request(b"POST", LOGIN_URL, params) From 1ff87641192d61a05756728c7be2503808442e06 Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Wed, 17 Nov 2021 16:55:21 +0100 Subject: [PATCH 02/12] Move changelog according to new PR number --- changelog.d/{9493.feature => 11361.feature} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{9493.feature => 11361.feature} (100%) diff --git a/changelog.d/9493.feature b/changelog.d/11361.feature similarity index 100% rename from changelog.d/9493.feature rename to changelog.d/11361.feature From 636cb684a54a2efb4dc54d99c2edcd783dacc3e8 Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Thu, 18 Nov 2021 11:56:08 +0100 Subject: [PATCH 03/12] Avoid repeating the default value --- synapse/rest/client/login.py | 2 +- tests/rest/client/test_login.py | 36 +++++++++++++++++++-------------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 3f91450e5f12..67e9a5b7ef62 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -414,7 +414,7 @@ async def _do_jwt_login( errcode=Codes.FORBIDDEN, ) - subject_claim = self.jwt_subject_claim or "sub" + subject_claim = self.jwt_subject_claim user = payload.get(subject_claim, None) if user is None: raise LoginError(403, "Invalid JWT", errcode=Codes.FORBIDDEN) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index b8fa2ceed251..dd561d7ce1c3 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -816,12 +816,16 @@ class JWTTestCase(unittest.HomeserverTestCase): jwt_secret = "secret" jwt_algorithm = "HS256" - def make_homeserver(self, reactor, clock): - self.hs = self.setup_test_homeserver() - self.hs.config.jwt.jwt_enabled = True - self.hs.config.jwt.jwt_secret = self.jwt_secret - self.hs.config.jwt.jwt_algorithm = self.jwt_algorithm - return self.hs + def default_config(self): + config = super().default_config() + if config.get("jwt_config", None) is None: + config["jwt_config"] = { + "enabled": True, + "secret": self.jwt_secret, + "algorithm": self.jwt_algorithm, + } + + return config def jwt_encode(self, payload: Dict[str, Any], secret: str = jwt_secret) -> str: # PyJWT 2.0.0 changed the return type of jwt.encode from bytes to str. @@ -882,7 +886,7 @@ def test_login_no_sub(self): @override_config( { "jwt_config": { - "jwt_enabled": True, + "enabled": True, "secret": jwt_secret, "algorithm": jwt_algorithm, "issuer": "test-issuer", @@ -922,7 +926,7 @@ def test_login_iss_no_config(self): @override_config( { "jwt_config": { - "jwt_enabled": True, + "enabled": True, "secret": jwt_secret, "algorithm": jwt_algorithm, "audiences": ["test-audience"], @@ -971,7 +975,7 @@ def test_login_default_sub(self): @override_config( { "jwt_config": { - "jwt_enabled": True, + "enabled": True, "secret": jwt_secret, "algorithm": jwt_algorithm, "subject_claim": "username", @@ -1046,12 +1050,14 @@ class JWTPubKeyTestCase(unittest.HomeserverTestCase): ] ) - def make_homeserver(self, reactor, clock): - self.hs = self.setup_test_homeserver() - self.hs.config.jwt.jwt_enabled = True - self.hs.config.jwt.jwt_secret = self.jwt_pubkey - self.hs.config.jwt.jwt_algorithm = "RS256" - return self.hs + def default_config(self): + config = super().default_config() + config["jwt_config"] = { + "enabled": True, + "secret": self.jwt_pubkey, + "algorithm": "RS256", + } + return config def jwt_encode(self, payload: Dict[str, Any], secret: str = jwt_privatekey) -> str: # PyJWT 2.0.0 changed the return type of jwt.encode from bytes to str. From e6023b86279f638191cd0df350c86e039777833f Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Thu, 18 Nov 2021 12:01:33 +0100 Subject: [PATCH 04/12] DRY up config overrides --- tests/rest/client/test_login.py | 47 ++++++++------------------------- 1 file changed, 11 insertions(+), 36 deletions(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index dd561d7ce1c3..a884ec6810bb 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -815,15 +815,17 @@ class JWTTestCase(unittest.HomeserverTestCase): jwt_secret = "secret" jwt_algorithm = "HS256" + base_config = { + "enabled": True, + "secret": jwt_secret, + "algorithm": jwt_algorithm, + } def default_config(self): config = super().default_config() - if config.get("jwt_config", None) is None: - config["jwt_config"] = { - "enabled": True, - "secret": self.jwt_secret, - "algorithm": self.jwt_algorithm, - } + + if config.get("jwt_config") is None: + config["jwt_config"] = self.base_config return config @@ -883,16 +885,7 @@ def test_login_no_sub(self): self.assertEqual(channel.json_body["errcode"], "M_FORBIDDEN") self.assertEqual(channel.json_body["error"], "Invalid JWT") - @override_config( - { - "jwt_config": { - "enabled": True, - "secret": jwt_secret, - "algorithm": jwt_algorithm, - "issuer": "test-issuer", - } - } - ) + @override_config({"jwt_config": {**base_config, "issuer": "test-issuer"}}) def test_login_iss(self): """Test validating the issuer claim.""" # A valid issuer. @@ -923,16 +916,7 @@ def test_login_iss_no_config(self): self.assertEqual(channel.result["code"], b"200", channel.result) self.assertEqual(channel.json_body["user_id"], "@kermit:test") - @override_config( - { - "jwt_config": { - "enabled": True, - "secret": jwt_secret, - "algorithm": jwt_algorithm, - "audiences": ["test-audience"], - } - } - ) + @override_config({"jwt_config": {**base_config, "audiences": ["test-audience"]}}) def test_login_aud(self): """Test validating the audience claim.""" # A valid audience. @@ -972,16 +956,7 @@ def test_login_default_sub(self): self.assertEqual(channel.result["code"], b"200", channel.result) self.assertEqual(channel.json_body["user_id"], "@kermit:test") - @override_config( - { - "jwt_config": { - "enabled": True, - "secret": jwt_secret, - "algorithm": jwt_algorithm, - "subject_claim": "username", - } - } - ) + @override_config({"jwt_config": {**base_config, "subject_claim": "username"}}) def test_login_custom_sub(self): """Test reading user ID from a custom subject claim.""" channel = self.jwt_login({"username": "frog"}) From b8e19cf4af643e10b340552c35135c0fa91e9bfd Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Thu, 18 Nov 2021 12:04:31 +0100 Subject: [PATCH 05/12] Remove needless variable --- synapse/rest/client/login.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 67e9a5b7ef62..00e65c66acbb 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -414,8 +414,7 @@ async def _do_jwt_login( errcode=Codes.FORBIDDEN, ) - subject_claim = self.jwt_subject_claim - user = payload.get(subject_claim, None) + user = payload.get(self.jwt_subject_claim, None) if user is None: raise LoginError(403, "Invalid JWT", errcode=Codes.FORBIDDEN) From be2a749a35c48067dd707f065e52f0c893e11db8 Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Thu, 18 Nov 2021 12:09:18 +0100 Subject: [PATCH 06/12] Explain potentially confusing code --- tests/rest/client/test_login.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index a884ec6810bb..19f5e465372b 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -824,6 +824,7 @@ class JWTTestCase(unittest.HomeserverTestCase): def default_config(self): config = super().default_config() + # If jwt_config has been defined (eg via @override_config), don't replace it. if config.get("jwt_config") is None: config["jwt_config"] = self.base_config From e9e2831f101f9a66d6c86bd021769b9cd4b0d70b Mon Sep 17 00:00:00 2001 From: Kostas Date: Thu, 18 Nov 2021 14:44:10 +0100 Subject: [PATCH 07/12] Update changelog.d/11361.feature Co-authored-by: Patrick Cloke --- changelog.d/11361.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11361.feature b/changelog.d/11361.feature index 5cf6f146f85b..6838f35d15c8 100644 --- a/changelog.d/11361.feature +++ b/changelog.d/11361.feature @@ -1 +1 @@ -Update JWT login type to support custom sub claim. +Update JWT login type to support custom `sub` claim. From 168f9299cf1ef5e68695850215352fddd1f79fb1 Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Fri, 19 Nov 2021 14:00:58 +0100 Subject: [PATCH 08/12] Group optional settings together in config --- synapse/config/jwt.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/config/jwt.py b/synapse/config/jwt.py index 502f44c1744c..dc12966ba8ec 100644 --- a/synapse/config/jwt.py +++ b/synapse/config/jwt.py @@ -81,12 +81,6 @@ def generate_config_section(self, **kwargs): # #secret: "provided-by-your-issuer" - # Name of the claim containing a unique identifier for the user. - # - # Optional, defaults to `sub`. - # - #subject_claim: "sub" - # The algorithm used to sign the JSON web token. # # Supported algorithms are listed at @@ -96,6 +90,12 @@ def generate_config_section(self, **kwargs): # #algorithm: "provided-by-your-issuer" + # Name of the claim containing a unique identifier for the user. + # + # Optional, defaults to `sub`. + # + #subject_claim: "sub" + # The issuer to validate the "iss" claim against. # # Optional, if provided the "iss" claim will be required and From 9772cd23107db17e15c4ac894142f657ea1056db Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Fri, 19 Nov 2021 14:01:49 +0100 Subject: [PATCH 09/12] Rearrange lines for comments to make sense --- synapse/config/jwt.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/config/jwt.py b/synapse/config/jwt.py index dc12966ba8ec..f5b5c2bfcdc6 100644 --- a/synapse/config/jwt.py +++ b/synapse/config/jwt.py @@ -31,9 +31,10 @@ def read_config(self, config, **kwargs): self.jwt_secret = jwt_config["secret"] self.jwt_algorithm = jwt_config["algorithm"] + self.jwt_subject_claim = jwt_config.get("subject_claim", "sub") + # The issuer and audiences are optional, if provided, it is asserted # that the claims exist on the JWT. - self.jwt_subject_claim = jwt_config.get("subject_claim", "sub") self.jwt_issuer = jwt_config.get("issuer") self.jwt_audiences = jwt_config.get("audiences") From 3a9c5b3c94253449060e373dbff2e43029dca317 Mon Sep 17 00:00:00 2001 From: Kostas Date: Mon, 22 Nov 2021 12:16:34 +0100 Subject: [PATCH 10/12] Update changelog.d/11361.feature Co-authored-by: Patrick Cloke --- changelog.d/11361.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11361.feature b/changelog.d/11361.feature index 6838f35d15c8..24c9244887fa 100644 --- a/changelog.d/11361.feature +++ b/changelog.d/11361.feature @@ -1 +1 @@ -Update JWT login type to support custom `sub` claim. +Update the JWT login type to support custom a `sub` claim. From 5f581ab538cacfce13ce6b17dab1c4cef52e78ad Mon Sep 17 00:00:00 2001 From: Kostas Date: Mon, 22 Nov 2021 12:16:52 +0100 Subject: [PATCH 11/12] Update synapse/config/jwt.py Co-authored-by: Patrick Cloke --- synapse/config/jwt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/jwt.py b/synapse/config/jwt.py index f5b5c2bfcdc6..24c3ef01fc4c 100644 --- a/synapse/config/jwt.py +++ b/synapse/config/jwt.py @@ -48,9 +48,9 @@ def read_config(self, config, **kwargs): self.jwt_enabled = False self.jwt_secret = None self.jwt_algorithm = None + self.jwt_subject_claim = None self.jwt_issuer = None self.jwt_audiences = None - self.jwt_subject_claim = None def generate_config_section(self, **kwargs): return """\ From 7f2ad4d4ee1629a0210e7214c1be51923db68f3d Mon Sep 17 00:00:00 2001 From: Kostas Karachalios Date: Mon, 22 Nov 2021 15:22:59 +0100 Subject: [PATCH 12/12] Regenerate sample config --- docs/sample_config.yaml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 2e6f7cfbb86a..02e9d14767a6 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -2030,12 +2030,6 @@ sso: # #secret: "provided-by-your-issuer" - # Name of the claim containing a unique identifier for the user. - # - # Optional, defaults to `sub`. - # - #subject_claim: "sub" - # The algorithm used to sign the JSON web token. # # Supported algorithms are listed at @@ -2045,6 +2039,12 @@ sso: # #algorithm: "provided-by-your-issuer" + # Name of the claim containing a unique identifier for the user. + # + # Optional, defaults to `sub`. + # + #subject_claim: "sub" + # The issuer to validate the "iss" claim against. # # Optional, if provided the "iss" claim will be required and