From 975b079e343c591015ab89c8e57e472e9fc472f2 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 15 Feb 2023 17:11:17 +0000 Subject: [PATCH 1/6] Don't require UIA for initial upload of cross signing keys --- synapse/config/experimental.py | 3 +++ synapse/rest/client/keys.py | 32 +++++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index d2d0270dddb1..20475f10e230 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -173,3 +173,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.msc3952_intentional_mentions = experimental.get( "msc3952_intentional_mentions", False ) + + # MSC3967: Do not require UIA when first uploading cross signing keys + self.msc3967_enabled = experimental.get("msc3967_enabled", False) diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 7873b363c06c..3a77120c8cfd 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -312,15 +312,29 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id = requester.user.to_string() body = parse_json_object_from_request(request) - await self.auth_handler.validate_user_via_ui_auth( - requester, - request, - body, - "add a device signing key to your account", - # Allow skipping of UI auth since this is frequently called directly - # after login and it is silly to ask users to re-auth immediately. - can_skip_ui_auth=True, - ) + if self.hs.config.experimental.msc3967_enabled: + existing_master_key = await self.e2e_keys_handler.store.get_e2e_cross_signing_key(user_id, "master") + if existing_master_key: + # If we already have a master key then cross signing is set up and we require UIA to reset + await self.auth_handler.validate_user_via_ui_auth( + requester, + request, + body, + "reset the device signing key on your account", + # Do not allow skipping of UIA auth. + can_skip_ui_auth=False, + ) + else: + # Previous behaviour is to always require UIA but allow it to be skipped + await self.auth_handler.validate_user_via_ui_auth( + requester, + request, + body, + "add a device signing key to your account", + # Allow skipping of UI auth since this is frequently called directly + # after login and it is silly to ask users to re-auth immediately. + can_skip_ui_auth=True, + ) result = await self.e2e_keys_handler.upload_signing_keys_for_user(user_id, body) return 200, result From a1006a195bc68e603e1d392b63b8900fa5c3ca61 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Thu, 16 Feb 2023 12:20:53 +0000 Subject: [PATCH 2/6] Add test cases --- synapse/rest/client/keys.py | 6 +- tests/rest/client/test_keys.py | 142 +++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 3a77120c8cfd..6c55aa740165 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -313,7 +313,11 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: body = parse_json_object_from_request(request) if self.hs.config.experimental.msc3967_enabled: - existing_master_key = await self.e2e_keys_handler.store.get_e2e_cross_signing_key(user_id, "master") + existing_master_key = ( + await self.e2e_keys_handler.store.get_e2e_cross_signing_key( + user_id, "master" + ) + ) if existing_master_key: # If we already have a master key then cross signing is set up and we require UIA to reset await self.auth_handler.validate_user_via_ui_auth( diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index 741fecea7713..c4ab2866f4ce 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -14,12 +14,22 @@ from http import HTTPStatus +from signedjson.key import ( + encode_verify_key_base64, + generate_signing_key, + get_verify_key, +) +from signedjson.sign import sign_json +from signedjson.types import SigningKey + from synapse.api.errors import Codes from synapse.rest import admin from synapse.rest.client import keys, login +from synapse.types import JsonDict from tests import unittest from tests.http.server._base import make_request_with_cancellation_test +from tests.unittest import override_config class KeyQueryTestCase(unittest.HomeserverTestCase): @@ -118,3 +128,135 @@ def test_key_query_cancellation(self) -> None: self.assertEqual(200, channel.code, msg=channel.result["body"]) self.assertIn(bob, channel.json_body["device_keys"]) + + def make_device_keys(self, user_id: str, device_id: str) -> JsonDict: + # We only generate a master key to simplify the test. + master_signing_key = generate_signing_key(device_id) + master_verify_key = encode_verify_key_base64(get_verify_key(master_signing_key)) + + return { + "master_key": sign_json( + { + "user_id": user_id, + "usage": ["master"], + "keys": {"ed25519:" + master_verify_key: master_verify_key}, + }, + user_id, + master_signing_key, + ), + } + + def test_device_signing_with_uia(self) -> None: + """Device signing key upload requires UIA.""" + password = "wonderland" + device_id = "ABCDEFGHI" + alice_id = self.register_user("alice", password) + alice_token = self.login("alice", password, device_id=device_id) + + content = self.make_device_keys(alice_id, device_id) + + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + content, + alice_token, + ) + + self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, channel.result) + # Grab the session + session = channel.json_body["session"] + # Ensure that flows are what is expected. + self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) + + # add UI auth + content["auth"] = { + "type": "m.login.password", + "identifier": {"type": "m.id.user", "user": alice_id}, + "password": password, + "session": session, + } + + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + content, + alice_token, + ) + + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + @override_config({"ui_auth": {"session_timeout": "15m"}}) + def test_device_signing_with_uia_session_timeout(self) -> None: + """Device signing key upload requires UIA buy passes with grace period.""" + password = "wonderland" + device_id = "ABCDEFGHI" + alice_id = self.register_user("alice", password) + alice_token = self.login("alice", password, device_id=device_id) + + content = self.make_device_keys(alice_id, device_id) + + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + content, + alice_token, + ) + + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + @override_config( + { + "experimental_features": {"msc3967_enabled": True}, + "ui_auth": {"session_timeout": "15s"}, + } + ) + def test_device_signing_with_msc3967(self) -> None: + """Device signing key follows MSC3967 behaviour when enabled.""" + password = "wonderland" + device_id = "ABCDEFGHI" + alice_id = self.register_user("alice", password) + alice_token = self.login("alice", password, device_id=device_id) + + keys1 = self.make_device_keys(alice_id, device_id) + + # Initial request should succeed as no existing keys are present. + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + keys1, + alice_token, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + keys2 = self.make_device_keys(alice_id, device_id) + + # Subsequent request should require UIA as keys already exist even though session_timeout is set. + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + keys2, + alice_token, + ) + self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, channel.result) + + # Grab the session + session = channel.json_body["session"] + # Ensure that flows are what is expected. + self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) + + # add UI auth + keys2["auth"] = { + "type": "m.login.password", + "identifier": {"type": "m.id.user", "user": alice_id}, + "password": password, + "session": session, + } + + # Request should complete + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + keys2, + alice_token, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) From a92f93fc5e7a5d99f761a49bbb15ce58c657c0c7 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Thu, 16 Feb 2023 13:38:56 +0000 Subject: [PATCH 3/6] Lint --- tests/rest/client/test_keys.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index c4ab2866f4ce..8ee548905704 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -20,7 +20,6 @@ get_verify_key, ) from signedjson.sign import sign_json -from signedjson.types import SigningKey from synapse.api.errors import Codes from synapse.rest import admin From fe0cc703ea46b548e00750284f82271ef24bee38 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Thu, 16 Feb 2023 13:41:25 +0000 Subject: [PATCH 4/6] Changelog --- changelog.d/15077.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15077.feature diff --git a/changelog.d/15077.feature b/changelog.d/15077.feature new file mode 100644 index 000000000000..384e751056b7 --- /dev/null +++ b/changelog.d/15077.feature @@ -0,0 +1 @@ +Experimental support for MSC3967 to not require UIA for setting up cross-signing on first use. From 84e4422225024cc9ca989fc907370e03a320325a Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Thu, 16 Feb 2023 13:56:01 +0000 Subject: [PATCH 5/6] Refactor to hopefully improve typings --- synapse/handlers/e2e_keys.py | 12 ++++++++++++ synapse/rest/client/keys.py | 8 ++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 43cbece21b69..5d9b94866e63 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1301,6 +1301,18 @@ async def _retrieve_cross_signing_keys_for_remote_user( return desired_key_data + async def is_cross_signing_set_up_for_user(self, user_id: str) -> bool: + """Checks if the user has cross-signing set up + + Args: + user_id: The user to check + + Returns: + True if the user has cross-signing set up, False otherwise + """ + existing_master_key = await self.store.get_e2e_cross_signing_key(user_id, "master") + return existing_master_key is not None + def _check_cross_signing_key( key: JsonDict, user_id: str, key_type: str, signing_key: Optional[VerifyKey] = None diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 6c55aa740165..32bb8b9a91a6 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -313,12 +313,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: body = parse_json_object_from_request(request) if self.hs.config.experimental.msc3967_enabled: - existing_master_key = ( - await self.e2e_keys_handler.store.get_e2e_cross_signing_key( - user_id, "master" - ) - ) - if existing_master_key: + if await self.e2e_keys_handler.is_cross_signing_set_up_for_user(user_id): # If we already have a master key then cross signing is set up and we require UIA to reset await self.auth_handler.validate_user_via_ui_auth( requester, @@ -328,6 +323,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # Do not allow skipping of UIA auth. can_skip_ui_auth=False, ) + # Otherwise we don't require UIA since we are setting up cross signing for first time else: # Previous behaviour is to always require UIA but allow it to be skipped await self.auth_handler.validate_user_via_ui_auth( From 2ff19b2cf9c343015d5b17921d2ea38ac874ff4f Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Thu, 16 Feb 2023 13:57:22 +0000 Subject: [PATCH 6/6] Lint --- synapse/handlers/e2e_keys.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 5d9b94866e63..4e9c8d8db0c7 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1310,7 +1310,9 @@ async def is_cross_signing_set_up_for_user(self, user_id: str) -> bool: Returns: True if the user has cross-signing set up, False otherwise """ - existing_master_key = await self.store.get_e2e_cross_signing_key(user_id, "master") + existing_master_key = await self.store.get_e2e_cross_signing_key( + user_id, "master" + ) return existing_master_key is not None