From ac6a84818fb9535ca4634c9b6c8325e234386ae8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 14 Apr 2020 10:09:58 +0100 Subject: [PATCH 01/12] Only register devices edu handler on the master process (#7255) --- changelog.d/7255.bugfix | 1 + synapse/handlers/e2e_keys.py | 18 +++++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) create mode 100644 changelog.d/7255.bugfix diff --git a/changelog.d/7255.bugfix b/changelog.d/7255.bugfix new file mode 100644 index 000000000..a96d52256 --- /dev/null +++ b/changelog.d/7255.bugfix @@ -0,0 +1 @@ +Fix a bug that prevented cross-signing with users on worker-mode synapses. \ No newline at end of file diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 95a9d71f4..8d7075f2e 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -54,19 +54,23 @@ class E2eKeysHandler(object): self._edu_updater = SigningKeyEduUpdater(hs, self) + federation_registry = hs.get_federation_registry() + self._is_master = hs.config.worker_app is None if not self._is_master: self._user_device_resync_client = ReplicationUserDevicesResyncRestServlet.make_client( hs ) + else: + # Only register this edu handler on master as it requires writing + # device updates to the db + # + # FIXME: switch to m.signing_key_update when MSC1756 is merged into the spec + federation_registry.register_edu_handler( + "org.matrix.signing_key_update", + self._edu_updater.incoming_signing_key_update, + ) - federation_registry = hs.get_federation_registry() - - # FIXME: switch to m.signing_key_update when MSC1756 is merged into the spec - federation_registry.register_edu_handler( - "org.matrix.signing_key_update", - self._edu_updater.incoming_signing_key_update, - ) # doesn't really work as part of the generic query API, because the # query request requires an object POST, but we abuse the # "query handler" interface. From 72fe2affb6ac86d433b80b6452da57052365aa26 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 12:36:01 +0100 Subject: [PATCH 02/12] Query missing cross-signing keys on local sig upload Add changelog Save retrieved keys to the db lint Fix and de-brittle remote result dict processing Use query_user_devices instead, assume only master, self_signing key types Make changelog more useful Remove very specific exception handling Wrap get_verify_key_from_cross_signing_key in a try/except Note that _get_e2e_cross_signing_verify_key can raise a SynapseError lint Add comment explaining why this is useful Only fetch master and self_signing key types Fix log statements, docstrings Remove extraneous items from remote query try/except lint Factor key retrieval out into a separate function Send device updates, modeled after SigningKeyEduUpdater._handle_signing_key_updates Update method docstring --- changelog.d/7289.bugfix | 1 + synapse/federation/transport/client.py | 14 ++- synapse/handlers/e2e_keys.py | 138 +++++++++++++++++++++++-- 3 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 changelog.d/7289.bugfix diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix new file mode 100644 index 000000000..5b4fbd77a --- /dev/null +++ b/changelog.d/7289.bugfix @@ -0,0 +1 @@ +Fix an edge-case where it was not possible to cross-sign a user which did not share a room with any user on your homeserver. The bug only affected Synapse deployments in worker mode. diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index dc563538d..c35637a57 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -406,13 +406,19 @@ class TransportLayerClient(object): "device_keys": { "": { "": {...} + } } + "master_keys": { + "": {...} + } } + "self_signing_keys": { + "": {...} } } } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containg the device keys. + A dict containing device and cross-signing keys. """ path = _create_v1_path("/user/keys/query") @@ -429,14 +435,16 @@ class TransportLayerClient(object): Response: { "stream_id": "...", - "devices": [ { ... } ] + "devices": [ { ... } ], + "master_key": { ... }, + "self_signing_key: { ... } } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containg the device keys. + A dict containing device and cross-signing keys. """ path = _create_v1_path("/user/devices/%s", user_id) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 8d7075f2e..afc173ab2 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -16,6 +16,7 @@ # limitations under the License. import logging +from typing import Dict, Optional, Tuple from six import iteritems @@ -23,6 +24,7 @@ import attr from canonicaljson import encode_canonical_json, json from signedjson.key import decode_verify_key_bytes from signedjson.sign import SignatureVerifyException, verify_signed_json +from signedjson.types import VerifyKey from unpaddedbase64 import decode_base64 from twisted.internet import defer @@ -174,8 +176,8 @@ class E2eKeysHandler(object): """This is called when we are querying the device list of a user on a remote homeserver and their device list is not in the device list cache. If we share a room with this user and we're not querying for - specific user we will update the cache - with their device list.""" + specific user we will update the cache with their device list. + """ destination_query = remote_queries_not_in_cache[destination] @@ -961,13 +963,19 @@ class E2eKeysHandler(object): return signature_list, failures @defer.inlineCallbacks - def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None): - """Fetch the cross-signing public key from storage and interpret it. + def _get_e2e_cross_signing_verify_key( + self, user_id: str, key_type: str, from_user_id: str = None + ): + """Fetch locally or remotely query for a cross-signing public key. + + First, attempt to fetch the cross-signing public key from storage. + If that fails, query the keys from the homeserver they belong to + and update our local copy. Args: - user_id (str): the user whose key should be fetched - key_type (str): the type of key to fetch - from_user_id (str): the user that we are fetching the keys for. + user_id: the user whose key should be fetched + key_type: the type of key to fetch + from_user_id: the user that we are fetching the keys for. This affects what signatures are fetched. Returns: @@ -976,16 +984,128 @@ class E2eKeysHandler(object): Raises: NotFoundError: if the key is not found + SynapseError: if `user_id` is invalid """ + user = UserID.from_string(user_id) + key_id = None + verify_key = None + key = yield self.store.get_e2e_cross_signing_key( user_id, key_type, from_user_id ) + + # If we couldn't find the key locally, and we're looking for keys of + # another user then attempt to fetch the missing key from the remote + # user's server. + # + # We may run into this in possible edge cases where a user tries to + # cross-sign a remote user, but does not share any rooms with them yet. + # Thus, we would not have their key list yet. We fetch the key here, + # store it and notify clients of new, associated device IDs. + if ( + key is None + and not self.is_mine(user) + # We only get "master" and "self_signing" keys from remote servers + and key_type in ["master", "self_signing"] + ): + key = yield self._retrieve_cross_signing_keys_for_remote_user( + user, key_type + ) + if key is None: - logger.debug("no %s key found for %s", key_type, user_id) + logger.debug("No %s key found for %s", key_type, user_id) raise NotFoundError("No %s key found for %s" % (key_type, user_id)) - key_id, verify_key = get_verify_key_from_cross_signing_key(key) + + # If we retrieved the keys remotely, these values will already be set + if key_id is None or verify_key is None: + try: + key_id, verify_key = get_verify_key_from_cross_signing_key(key) + except ValueError as e: + logger.debug( + "Invalid %s key retrieved: %s - %s %s", key_type, key, type(e), e, + ) + raise SynapseError( + 502, "Invalid %s key retrieved from remote server", key_type + ) + return key, key_id, verify_key + @defer.inlineCallbacks + def _retrieve_cross_signing_keys_for_remote_user( + self, user: UserID, desired_key_type: str, + ) -> Tuple[Optional[Dict], Optional[str], Optional[VerifyKey]]: + """Queries cross-signing keys for a remote user and saves them to the database + + Only the key specified by `key_type` will be returned, while all retrieved keys + will be saved regardless + + Args: + user: The user to query remote keys for + desired_key_type: The type of key to receive. One of "master", "self_signing" + + Returns: + A tuple of the retrieved key content, the key's ID and the matching VerifyKey. + If the key cannot be retrieved, all values in the tuple will instead be None. + """ + try: + remote_result = yield self.federation.query_user_devices( + user.domain, user.to_string() + ) + except Exception as e: + logger.warning( + "Unable to query %s for cross-signing keys of user %s: %s %s", + user.domain, + user.to_string(), + type(e), + e, + ) + return None + + # Process each of the retrieved cross-signing keys + final_key = None + final_key_id = None + final_verify_key = None + device_ids = [] + for key_type in ["master", "self_signing"]: + key_content = remote_result.get(key_type + "_key") + if not key_content: + continue + + # At the same time, store this key in the db for + # subsequent queries + yield self.store.set_e2e_cross_signing_key( + user.to_string(), key_type, key_content + ) + + # Note down the device ID attached to this key + try: + # verify_key is a VerifyKey from signedjson, which uses + # .version to denote the portion of the key ID after the + # algorithm and colon, which is the device ID + key_id, verify_key = get_verify_key_from_cross_signing_key(key_content) + except ValueError as e: + logger.debug( + "Invalid %s key retrieved: %s - %s %s", + key_type, + key_content, + type(e), + e, + ) + continue + device_ids.append(verify_key.version) + + # If this is the desired key type, save it and it's ID/VerifyKey + if key_type == desired_key_type: + final_key = key_content + final_verify_key = verify_key + final_key_id = key_id + + # Notify clients that new devices for this user have been discovered + if device_ids: + yield self.device_handler.notify_device_update(user.to_string(), device_ids) + + return final_key, final_key_id, final_verify_key + def _check_cross_signing_key(key, user_id, key_type, signing_key=None): """Check a cross-signing key uploaded by a user. Performs some basic sanity From 40f79f58bfab1724ca8ca93ba8e53815d8dac301 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 30 Mar 2020 14:34:28 +0100 Subject: [PATCH 03/12] Always send the user updates to their own device list (#7160) --- changelog.d/7160.feature | 1 + synapse/handlers/device.py | 14 ++++++++++++-- synapse/handlers/sync.py | 7 ++++++- 3 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 changelog.d/7160.feature diff --git a/changelog.d/7160.feature b/changelog.d/7160.feature new file mode 100644 index 000000000..c1205969a --- /dev/null +++ b/changelog.d/7160.feature @@ -0,0 +1 @@ +Always send users their own device updates. diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index a514c3071..993499f44 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -125,8 +125,14 @@ class DeviceWorkerHandler(BaseHandler): users_who_share_room = yield self.store.get_users_who_share_room_with_user( user_id ) + + tracked_users = set(users_who_share_room) + + # Always tell the user about their own devices + tracked_users.add(user_id) + changed = yield self.store.get_users_whose_devices_changed( - from_token.device_list_key, users_who_share_room + from_token.device_list_key, tracked_users ) # Then work out if any users have since joined @@ -456,7 +462,11 @@ class DeviceHandler(DeviceWorkerHandler): room_ids = yield self.store.get_rooms_for_user(user_id) - yield self.notifier.on_new_event("device_list_key", position, rooms=room_ids) + # specify the user ID too since the user should always get their own device list + # updates, even if they aren't in any rooms. + yield self.notifier.on_new_event( + "device_list_key", position, users=[user_id], rooms=room_ids + ) if hosts: logger.info( diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 669dbc8a4..cfd5dfc9e 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1143,9 +1143,14 @@ class SyncHandler(object): user_id ) + tracked_users = set(users_who_share_room) + + # Always tell the user about their own devices + tracked_users.add(user_id) + # Step 1a, check for changes in devices of users we share a room with users_that_have_changed = await self.store.get_users_whose_devices_changed( - since_token.device_list_key, users_who_share_room + since_token.device_list_key, tracked_users ) # Step 1b, check for newly joined rooms From d41c8f6d4ddc647b91eb17ee7d410b039101e442 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 20 Apr 2020 17:54:35 +0100 Subject: [PATCH 04/12] Revert "Query missing cross-signing keys on local sig upload" This was incorrectly merged to the release branch before it was ready. This reverts commit 72fe2affb6ac86d433b80b6452da57052365aa26. --- changelog.d/7289.bugfix | 1 - synapse/federation/transport/client.py | 14 +-- synapse/handlers/e2e_keys.py | 138 ++----------------------- 3 files changed, 12 insertions(+), 141 deletions(-) delete mode 100644 changelog.d/7289.bugfix diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix deleted file mode 100644 index 5b4fbd77a..000000000 --- a/changelog.d/7289.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix an edge-case where it was not possible to cross-sign a user which did not share a room with any user on your homeserver. The bug only affected Synapse deployments in worker mode. diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index c35637a57..dc563538d 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -406,19 +406,13 @@ class TransportLayerClient(object): "device_keys": { "": { "": {...} - } } - "master_keys": { - "": {...} - } } - "self_signing_keys": { - "": {...} } } } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containing device and cross-signing keys. + A dict containg the device keys. """ path = _create_v1_path("/user/keys/query") @@ -435,16 +429,14 @@ class TransportLayerClient(object): Response: { "stream_id": "...", - "devices": [ { ... } ], - "master_key": { ... }, - "self_signing_key: { ... } + "devices": [ { ... } ] } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containing device and cross-signing keys. + A dict containg the device keys. """ path = _create_v1_path("/user/devices/%s", user_id) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index afc173ab2..8d7075f2e 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -16,7 +16,6 @@ # limitations under the License. import logging -from typing import Dict, Optional, Tuple from six import iteritems @@ -24,7 +23,6 @@ import attr from canonicaljson import encode_canonical_json, json from signedjson.key import decode_verify_key_bytes from signedjson.sign import SignatureVerifyException, verify_signed_json -from signedjson.types import VerifyKey from unpaddedbase64 import decode_base64 from twisted.internet import defer @@ -176,8 +174,8 @@ class E2eKeysHandler(object): """This is called when we are querying the device list of a user on a remote homeserver and their device list is not in the device list cache. If we share a room with this user and we're not querying for - specific user we will update the cache with their device list. - """ + specific user we will update the cache + with their device list.""" destination_query = remote_queries_not_in_cache[destination] @@ -963,19 +961,13 @@ class E2eKeysHandler(object): return signature_list, failures @defer.inlineCallbacks - def _get_e2e_cross_signing_verify_key( - self, user_id: str, key_type: str, from_user_id: str = None - ): - """Fetch locally or remotely query for a cross-signing public key. - - First, attempt to fetch the cross-signing public key from storage. - If that fails, query the keys from the homeserver they belong to - and update our local copy. + def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None): + """Fetch the cross-signing public key from storage and interpret it. Args: - user_id: the user whose key should be fetched - key_type: the type of key to fetch - from_user_id: the user that we are fetching the keys for. + user_id (str): the user whose key should be fetched + key_type (str): the type of key to fetch + from_user_id (str): the user that we are fetching the keys for. This affects what signatures are fetched. Returns: @@ -984,128 +976,16 @@ class E2eKeysHandler(object): Raises: NotFoundError: if the key is not found - SynapseError: if `user_id` is invalid """ - user = UserID.from_string(user_id) - key_id = None - verify_key = None - key = yield self.store.get_e2e_cross_signing_key( user_id, key_type, from_user_id ) - - # If we couldn't find the key locally, and we're looking for keys of - # another user then attempt to fetch the missing key from the remote - # user's server. - # - # We may run into this in possible edge cases where a user tries to - # cross-sign a remote user, but does not share any rooms with them yet. - # Thus, we would not have their key list yet. We fetch the key here, - # store it and notify clients of new, associated device IDs. - if ( - key is None - and not self.is_mine(user) - # We only get "master" and "self_signing" keys from remote servers - and key_type in ["master", "self_signing"] - ): - key = yield self._retrieve_cross_signing_keys_for_remote_user( - user, key_type - ) - if key is None: - logger.debug("No %s key found for %s", key_type, user_id) + logger.debug("no %s key found for %s", key_type, user_id) raise NotFoundError("No %s key found for %s" % (key_type, user_id)) - - # If we retrieved the keys remotely, these values will already be set - if key_id is None or verify_key is None: - try: - key_id, verify_key = get_verify_key_from_cross_signing_key(key) - except ValueError as e: - logger.debug( - "Invalid %s key retrieved: %s - %s %s", key_type, key, type(e), e, - ) - raise SynapseError( - 502, "Invalid %s key retrieved from remote server", key_type - ) - + key_id, verify_key = get_verify_key_from_cross_signing_key(key) return key, key_id, verify_key - @defer.inlineCallbacks - def _retrieve_cross_signing_keys_for_remote_user( - self, user: UserID, desired_key_type: str, - ) -> Tuple[Optional[Dict], Optional[str], Optional[VerifyKey]]: - """Queries cross-signing keys for a remote user and saves them to the database - - Only the key specified by `key_type` will be returned, while all retrieved keys - will be saved regardless - - Args: - user: The user to query remote keys for - desired_key_type: The type of key to receive. One of "master", "self_signing" - - Returns: - A tuple of the retrieved key content, the key's ID and the matching VerifyKey. - If the key cannot be retrieved, all values in the tuple will instead be None. - """ - try: - remote_result = yield self.federation.query_user_devices( - user.domain, user.to_string() - ) - except Exception as e: - logger.warning( - "Unable to query %s for cross-signing keys of user %s: %s %s", - user.domain, - user.to_string(), - type(e), - e, - ) - return None - - # Process each of the retrieved cross-signing keys - final_key = None - final_key_id = None - final_verify_key = None - device_ids = [] - for key_type in ["master", "self_signing"]: - key_content = remote_result.get(key_type + "_key") - if not key_content: - continue - - # At the same time, store this key in the db for - # subsequent queries - yield self.store.set_e2e_cross_signing_key( - user.to_string(), key_type, key_content - ) - - # Note down the device ID attached to this key - try: - # verify_key is a VerifyKey from signedjson, which uses - # .version to denote the portion of the key ID after the - # algorithm and colon, which is the device ID - key_id, verify_key = get_verify_key_from_cross_signing_key(key_content) - except ValueError as e: - logger.debug( - "Invalid %s key retrieved: %s - %s %s", - key_type, - key_content, - type(e), - e, - ) - continue - device_ids.append(verify_key.version) - - # If this is the desired key type, save it and it's ID/VerifyKey - if key_type == desired_key_type: - final_key = key_content - final_verify_key = verify_key - final_key_id = key_id - - # Notify clients that new devices for this user have been discovered - if device_ids: - yield self.device_handler.notify_device_update(user.to_string(), device_ids) - - return final_key, final_key_id, final_verify_key - def _check_cross_signing_key(key, user_id, key_type, signing_key=None): """Check a cross-signing key uploaded by a user. Performs some basic sanity From 974c0d726add8d81aef251946282ad19bae6c365 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 21 Apr 2020 10:46:30 +0100 Subject: [PATCH 05/12] Support GET account_data requests on a worker (#7311) --- changelog.d/7311.doc | 1 + docs/workers.md | 2 ++ synapse/app/generic_worker.py | 6 ++++++ synapse/rest/client/v2_alpha/account_data.py | 8 ++++++++ 4 files changed, 17 insertions(+) create mode 100644 changelog.d/7311.doc diff --git a/changelog.d/7311.doc b/changelog.d/7311.doc new file mode 100644 index 000000000..cecb31c15 --- /dev/null +++ b/changelog.d/7311.doc @@ -0,0 +1 @@ +Document that account_data get requests can be routed to a worker. diff --git a/docs/workers.md b/docs/workers.md index cf460283d..cb3b9f8e6 100644 --- a/docs/workers.md +++ b/docs/workers.md @@ -268,6 +268,8 @@ Additionally, the following REST endpoints can be handled for GET requests: ^/_matrix/client/(api/v1|r0|unstable)/pushrules/.*$ ^/_matrix/client/(api/v1|r0|unstable)/groups/.*$ + ^/_matrix/client/(api/v1|r0|unstable)/user/[^/]*/account_data/ + ^/_matrix/client/(api/v1|r0|unstable)/user/[^/]*/rooms/[^/]*/account_data/ Additionally, the following REST endpoints can be handled, but all requests must be routed to the same instance: diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index 5363642d6..66be6ea2e 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -98,6 +98,10 @@ from synapse.rest.client.v1.voip import VoipRestServlet from synapse.rest.client.v2_alpha import groups, sync, user_directory from synapse.rest.client.v2_alpha._base import client_patterns from synapse.rest.client.v2_alpha.account import ThreepidRestServlet +from synapse.rest.client.v2_alpha.account_data import ( + AccountDataServlet, + RoomAccountDataServlet, +) from synapse.rest.client.v2_alpha.keys import KeyChangesServlet, KeyQueryServlet from synapse.rest.client.v2_alpha.register import RegisterRestServlet from synapse.rest.client.versions import VersionsRestServlet @@ -475,6 +479,8 @@ class GenericWorkerServer(HomeServer): ProfileDisplaynameRestServlet(self).register(resource) ProfileRestServlet(self).register(resource) KeyUploadServlet(self).register(resource) + AccountDataServlet(self).register(resource) + RoomAccountDataServlet(self).register(resource) sync.register_servlets(self, resource) events.register_servlets(self, resource) diff --git a/synapse/rest/client/v2_alpha/account_data.py b/synapse/rest/client/v2_alpha/account_data.py index 64eb7fec3..c1d4cd0ca 100644 --- a/synapse/rest/client/v2_alpha/account_data.py +++ b/synapse/rest/client/v2_alpha/account_data.py @@ -38,8 +38,12 @@ class AccountDataServlet(RestServlet): self.auth = hs.get_auth() self.store = hs.get_datastore() self.notifier = hs.get_notifier() + self._is_worker = hs.config.worker_app is not None async def on_PUT(self, request, user_id, account_data_type): + if self._is_worker: + raise Exception("Cannot handle PUT /account_data on worker") + requester = await self.auth.get_user_by_req(request) if user_id != requester.user.to_string(): raise AuthError(403, "Cannot add account data for other users.") @@ -86,8 +90,12 @@ class RoomAccountDataServlet(RestServlet): self.auth = hs.get_auth() self.store = hs.get_datastore() self.notifier = hs.get_notifier() + self._is_worker = hs.config.worker_app is not None async def on_PUT(self, request, user_id, room_id, account_data_type): + if self._is_worker: + raise Exception("Cannot handle PUT /account_data on worker") + requester = await self.auth.get_user_by_req(request) if user_id != requester.user.to_string(): raise AuthError(403, "Cannot add account data for other users.") From 556566f0b8141102c629591cc3ea09279511fcba Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 21 Apr 2020 13:20:16 +0100 Subject: [PATCH 06/12] Fix changelog file I updated the PR and forgot to update the changelog. --- changelog.d/7311.doc | 1 - changelog.d/7311.feature | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/7311.doc create mode 100644 changelog.d/7311.feature diff --git a/changelog.d/7311.doc b/changelog.d/7311.doc deleted file mode 100644 index cecb31c15..000000000 --- a/changelog.d/7311.doc +++ /dev/null @@ -1 +0,0 @@ -Document that account_data get requests can be routed to a worker. diff --git a/changelog.d/7311.feature b/changelog.d/7311.feature new file mode 100644 index 000000000..c3adc1d6e --- /dev/null +++ b/changelog.d/7311.feature @@ -0,0 +1 @@ +Add support for handling GET requests for account_data on a worker. From f89ad3b6dfccbe33ff563ec5523723f94cc912ff Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Wed, 22 Apr 2020 12:29:36 +0100 Subject: [PATCH 07/12] Query missing cross-signing keys on local sig upload (#7289) --- changelog.d/7289.bugfix | 1 + synapse/federation/transport/client.py | 49 ++++++-- synapse/handlers/e2e_keys.py | 150 +++++++++++++++++++++++-- 3 files changed, 181 insertions(+), 19 deletions(-) create mode 100644 changelog.d/7289.bugfix diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix new file mode 100644 index 000000000..84699e50a --- /dev/null +++ b/changelog.d/7289.bugfix @@ -0,0 +1 @@ +Fix a bug with cross-signing devices belonging to remote users who did not share a room with any user on the local homeserver. diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index dc563538d..383e3fdc8 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -399,20 +399,30 @@ class TransportLayerClient(object): { "device_keys": { "": [""] - } } + } + } Response: { "device_keys": { "": { "": {...} - } } } + } + }, + "master_key": { + "": {...} + } + }, + "self_signing_key": { + "": {...} + } + } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containg the device keys. + A dict containing device and cross-signing keys. """ path = _create_v1_path("/user/keys/query") @@ -429,14 +439,30 @@ class TransportLayerClient(object): Response: { "stream_id": "...", - "devices": [ { ... } ] + "devices": [ { ... } ], + "master_key": { + "user_id": "", + "usage": [...], + "keys": {...}, + "signatures": { + "": {...} + } + }, + "self_signing_key": { + "user_id": "", + "usage": [...], + "keys": {...}, + "signatures": { + "": {...} + } + } } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containg the device keys. + A dict containing device and cross-signing keys. """ path = _create_v1_path("/user/devices/%s", user_id) @@ -454,8 +480,10 @@ class TransportLayerClient(object): { "one_time_keys": { "": { - "": "" - } } } + "": "" + } + } + } Response: { @@ -463,13 +491,16 @@ class TransportLayerClient(object): "": { "": { ":": "" - } } } } + } + } + } + } Args: destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containg the one-time keys. + A dict containing the one-time keys. """ path = _create_v1_path("/user/keys/claim") diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 8d7075f2e..8f1bc0323 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -174,8 +174,8 @@ class E2eKeysHandler(object): """This is called when we are querying the device list of a user on a remote homeserver and their device list is not in the device list cache. If we share a room with this user and we're not querying for - specific user we will update the cache - with their device list.""" + specific user we will update the cache with their device list. + """ destination_query = remote_queries_not_in_cache[destination] @@ -961,13 +961,19 @@ class E2eKeysHandler(object): return signature_list, failures @defer.inlineCallbacks - def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None): - """Fetch the cross-signing public key from storage and interpret it. + def _get_e2e_cross_signing_verify_key( + self, user_id: str, key_type: str, from_user_id: str = None + ): + """Fetch locally or remotely query for a cross-signing public key. + + First, attempt to fetch the cross-signing public key from storage. + If that fails, query the keys from the homeserver they belong to + and update our local copy. Args: - user_id (str): the user whose key should be fetched - key_type (str): the type of key to fetch - from_user_id (str): the user that we are fetching the keys for. + user_id: the user whose key should be fetched + key_type: the type of key to fetch + from_user_id: the user that we are fetching the keys for. This affects what signatures are fetched. Returns: @@ -976,16 +982,140 @@ class E2eKeysHandler(object): Raises: NotFoundError: if the key is not found + SynapseError: if `user_id` is invalid """ + user = UserID.from_string(user_id) key = yield self.store.get_e2e_cross_signing_key( user_id, key_type, from_user_id ) - if key is None: - logger.debug("no %s key found for %s", key_type, user_id) + + if key: + # We found a copy of this key in our database. Decode and return it + key_id, verify_key = get_verify_key_from_cross_signing_key(key) + return key, key_id, verify_key + + # If we couldn't find the key locally, and we're looking for keys of + # another user then attempt to fetch the missing key from the remote + # user's server. + # + # We may run into this in possible edge cases where a user tries to + # cross-sign a remote user, but does not share any rooms with them yet. + # Thus, we would not have their key list yet. We instead fetch the key, + # store it and notify clients of new, associated device IDs. + if self.is_mine(user) or key_type not in ["master", "self_signing"]: + # Note that master and self_signing keys are the only cross-signing keys we + # can request over federation raise NotFoundError("No %s key found for %s" % (key_type, user_id)) - key_id, verify_key = get_verify_key_from_cross_signing_key(key) + + ( + key, + key_id, + verify_key, + ) = yield self._retrieve_cross_signing_keys_for_remote_user(user, key_type) + + if key is None: + raise NotFoundError("No %s key found for %s" % (key_type, user_id)) + return key, key_id, verify_key + @defer.inlineCallbacks + def _retrieve_cross_signing_keys_for_remote_user( + self, user: UserID, desired_key_type: str, + ): + """Queries cross-signing keys for a remote user and saves them to the database + + Only the key specified by `key_type` will be returned, while all retrieved keys + will be saved regardless + + Args: + user: The user to query remote keys for + desired_key_type: The type of key to receive. One of "master", "self_signing" + + Returns: + Deferred[Tuple[Optional[Dict], Optional[str], Optional[VerifyKey]]]: A tuple + of the retrieved key content, the key's ID and the matching VerifyKey. + If the key cannot be retrieved, all values in the tuple will instead be None. + """ + try: + remote_result = yield self.federation.query_user_devices( + user.domain, user.to_string() + ) + except Exception as e: + logger.warning( + "Unable to query %s for cross-signing keys of user %s: %s %s", + user.domain, + user.to_string(), + type(e), + e, + ) + return None, None, None + + # Process each of the retrieved cross-signing keys + desired_key = None + desired_key_id = None + desired_verify_key = None + retrieved_device_ids = [] + for key_type in ["master", "self_signing"]: + key_content = remote_result.get(key_type + "_key") + if not key_content: + continue + + # Ensure these keys belong to the correct user + if "user_id" not in key_content: + logger.warning( + "Invalid %s key retrieved, missing user_id field: %s", + key_type, + key_content, + ) + continue + if user.to_string() != key_content["user_id"]: + logger.warning( + "Found %s key of user %s when querying for keys of user %s", + key_type, + key_content["user_id"], + user.to_string(), + ) + continue + + # Validate the key contents + try: + # verify_key is a VerifyKey from signedjson, which uses + # .version to denote the portion of the key ID after the + # algorithm and colon, which is the device ID + key_id, verify_key = get_verify_key_from_cross_signing_key(key_content) + except ValueError as e: + logger.warning( + "Invalid %s key retrieved: %s - %s %s", + key_type, + key_content, + type(e), + e, + ) + continue + + # Note down the device ID attached to this key + retrieved_device_ids.append(verify_key.version) + + # If this is the desired key type, save it and its ID/VerifyKey + if key_type == desired_key_type: + desired_key = key_content + desired_verify_key = verify_key + desired_key_id = key_id + + # At the same time, store this key in the db for subsequent queries + yield self.store.set_e2e_cross_signing_key( + user.to_string(), key_type, key_content + ) + + # Notify clients that new devices for this user have been discovered + if retrieved_device_ids: + # XXX is this necessary? + yield self.device_handler.notify_device_update( + user.to_string(), retrieved_device_ids + ) + + return desired_key, desired_key_id, desired_verify_key + def _check_cross_signing_key(key, user_id, key_type, signing_key=None): """Check a cross-signing key uploaded by a user. Performs some basic sanity From 51f358e2fe4b568009de46c0130bd6843ed8215b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 16 Apr 2020 10:52:55 -0400 Subject: [PATCH 08/12] Do not treat display names as globs for push rules. (#7271) --- changelog.d/7271.bugfix | 1 + synapse/push/push_rule_evaluator.py | 69 +++++++++++++++----------- tests/push/test_push_rule_evaluator.py | 65 ++++++++++++++++++++++++ tox.ini | 1 + 4 files changed, 106 insertions(+), 30 deletions(-) create mode 100644 changelog.d/7271.bugfix create mode 100644 tests/push/test_push_rule_evaluator.py diff --git a/changelog.d/7271.bugfix b/changelog.d/7271.bugfix new file mode 100644 index 000000000..e8315e4ce --- /dev/null +++ b/changelog.d/7271.bugfix @@ -0,0 +1 @@ +Do not treat display names as globs in push rules. diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index b1587183a..4cd702b5f 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -16,9 +16,11 @@ import logging import re +from typing import Pattern from six import string_types +from synapse.events import EventBase from synapse.types import UserID from synapse.util.caches import CACHE_SIZE_FACTOR, register_cache from synapse.util.caches.lrucache import LruCache @@ -56,18 +58,18 @@ def _test_ineq_condition(condition, number): rhs = m.group(2) if not rhs.isdigit(): return False - rhs = int(rhs) + rhs_int = int(rhs) if ineq == "" or ineq == "==": - return number == rhs + return number == rhs_int elif ineq == "<": - return number < rhs + return number < rhs_int elif ineq == ">": - return number > rhs + return number > rhs_int elif ineq == ">=": - return number >= rhs + return number >= rhs_int elif ineq == "<=": - return number <= rhs + return number <= rhs_int else: return False @@ -83,7 +85,13 @@ def tweaks_for_actions(actions): class PushRuleEvaluatorForEvent(object): - def __init__(self, event, room_member_count, sender_power_level, power_levels): + def __init__( + self, + event: EventBase, + room_member_count: int, + sender_power_level: int, + power_levels: dict, + ): self._event = event self._room_member_count = room_member_count self._sender_power_level = sender_power_level @@ -92,7 +100,7 @@ class PushRuleEvaluatorForEvent(object): # Maps strings of e.g. 'content.body' -> event["content"]["body"] self._value_cache = _flatten_dict(event) - def matches(self, condition, user_id, display_name): + def matches(self, condition: dict, user_id: str, display_name: str) -> bool: if condition["kind"] == "event_match": return self._event_match(condition, user_id) elif condition["kind"] == "contains_display_name": @@ -106,7 +114,7 @@ class PushRuleEvaluatorForEvent(object): else: return True - def _event_match(self, condition, user_id): + def _event_match(self, condition: dict, user_id: str) -> bool: pattern = condition.get("pattern", None) if not pattern: @@ -134,7 +142,7 @@ class PushRuleEvaluatorForEvent(object): return _glob_matches(pattern, haystack) - def _contains_display_name(self, display_name): + def _contains_display_name(self, display_name: str) -> bool: if not display_name: return False @@ -142,51 +150,52 @@ class PushRuleEvaluatorForEvent(object): if not body: return False - return _glob_matches(display_name, body, word_boundary=True) + # Similar to _glob_matches, but do not treat display_name as a glob. + r = regex_cache.get((display_name, False, True), None) + if not r: + r = re.escape(display_name) + r = _re_word_boundary(r) + r = re.compile(r, flags=re.IGNORECASE) + regex_cache[(display_name, False, True)] = r - def _get_value(self, dotted_key): + return r.search(body) + + def _get_value(self, dotted_key: str) -> str: return self._value_cache.get(dotted_key, None) -# Caches (glob, word_boundary) -> regex for push. See _glob_matches +# Caches (string, is_glob, word_boundary) -> regex for push. See _glob_matches regex_cache = LruCache(50000 * CACHE_SIZE_FACTOR) register_cache("cache", "regex_push_cache", regex_cache) -def _glob_matches(glob, value, word_boundary=False): +def _glob_matches(glob: str, value: str, word_boundary: bool = False) -> bool: """Tests if value matches glob. Args: - glob (string) - value (string): String to test against glob. - word_boundary (bool): Whether to match against word boundaries or entire + glob + value: String to test against glob. + word_boundary: Whether to match against word boundaries or entire string. Defaults to False. - - Returns: - bool """ try: - r = regex_cache.get((glob, word_boundary), None) + r = regex_cache.get((glob, True, word_boundary), None) if not r: r = _glob_to_re(glob, word_boundary) - regex_cache[(glob, word_boundary)] = r + regex_cache[(glob, True, word_boundary)] = r return r.search(value) except re.error: logger.warning("Failed to parse glob to regex: %r", glob) return False -def _glob_to_re(glob, word_boundary): +def _glob_to_re(glob: str, word_boundary: bool) -> Pattern: """Generates regex for a given glob. Args: - glob (string) - word_boundary (bool): Whether to match against word boundaries or entire - string. Defaults to False. - - Returns: - regex object + glob + word_boundary: Whether to match against word boundaries or entire string. """ if IS_GLOB.search(glob): r = re.escape(glob) @@ -219,7 +228,7 @@ def _glob_to_re(glob, word_boundary): return re.compile(r, flags=re.IGNORECASE) -def _re_word_boundary(r): +def _re_word_boundary(r: str) -> str: """ Adds word boundary characters to the start and end of an expression to require that the match occur as a whole word, diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py new file mode 100644 index 000000000..9ae6a87d7 --- /dev/null +++ b/tests/push/test_push_rule_evaluator.py @@ -0,0 +1,65 @@ +# -*- coding: utf-8 -*- +# Copyright 2020 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from synapse.api.room_versions import RoomVersions +from synapse.events import FrozenEvent +from synapse.push.push_rule_evaluator import PushRuleEvaluatorForEvent + +from tests import unittest + + +class PushRuleEvaluatorTestCase(unittest.TestCase): + def setUp(self): + event = FrozenEvent( + { + "event_id": "$event_id", + "type": "m.room.history_visibility", + "sender": "@user:test", + "state_key": "", + "room_id": "@room:test", + "content": {"body": "foo bar baz"}, + }, + RoomVersions.V1, + ) + room_member_count = 0 + sender_power_level = 0 + power_levels = {} + self.evaluator = PushRuleEvaluatorForEvent( + event, room_member_count, sender_power_level, power_levels + ) + + def test_display_name(self): + """Check for a matching display name in the body of the event.""" + condition = { + "kind": "contains_display_name", + } + + # Blank names are skipped. + self.assertFalse(self.evaluator.matches(condition, "@user:test", "")) + + # Check a display name that doesn't match. + self.assertFalse(self.evaluator.matches(condition, "@user:test", "not found")) + + # Check a display name which matches. + self.assertTrue(self.evaluator.matches(condition, "@user:test", "foo")) + + # A display name that matches, but not a full word does not result in a match. + self.assertFalse(self.evaluator.matches(condition, "@user:test", "ba")) + + # A display name should not be interpreted as a regular expression. + self.assertFalse(self.evaluator.matches(condition, "@user:test", "ba[rz]")) + + # A display name with spaces should work fine. + self.assertTrue(self.evaluator.matches(condition, "@user:test", "foo bar")) diff --git a/tox.ini b/tox.ini index 8e3f09e63..34d6322c4 100644 --- a/tox.ini +++ b/tox.ini @@ -194,6 +194,7 @@ commands = mypy \ synapse/metrics \ synapse/module_api \ synapse/push/pusherpool.py \ + synapse/push/push_rule_evaluator.py \ synapse/replication \ synapse/rest \ synapse/spam_checker_api \ From 83af1079d6a175eec0bb364a2e573b6acd0ee2b7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Apr 2020 14:28:23 +0100 Subject: [PATCH 09/12] 1.12.4rc1 --- synapse/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/__init__.py b/synapse/__init__.py index 3bf2d0245..fff9d311f 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -36,7 +36,7 @@ try: except ImportError: pass -__version__ = "1.12.3" +__version__ = "1.12.4rc1" if bool(os.environ.get("SYNAPSE_TEST_PATCH_LOG_CONTEXTS", False)): # We import here so that we don't have to install a bunch of deps when From dc8003f9216efa76133677ddaa32d2861753a91e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Apr 2020 14:29:39 +0100 Subject: [PATCH 10/12] 1.12.4rc1 --- CHANGES.md | 18 ++++++++++++++++++ changelog.d/7160.feature | 1 - changelog.d/7255.bugfix | 1 - changelog.d/7271.bugfix | 1 - changelog.d/7289.bugfix | 1 - changelog.d/7311.feature | 1 - 6 files changed, 18 insertions(+), 5 deletions(-) delete mode 100644 changelog.d/7160.feature delete mode 100644 changelog.d/7255.bugfix delete mode 100644 changelog.d/7271.bugfix delete mode 100644 changelog.d/7289.bugfix delete mode 100644 changelog.d/7311.feature diff --git a/CHANGES.md b/CHANGES.md index e78c8e48e..1ccc39f4a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,21 @@ +Synapse 1.12.4rc1 (2020-04-22) +============================== + +Features +-------- + +- Always send users their own device updates. ([\#7160](https://github.com/matrix-org/synapse/issues/7160)) +- Add support for handling GET requests for account_data on a worker. ([\#7311](https://github.com/matrix-org/synapse/issues/7311)) + + +Bugfixes +-------- + +- Fix a bug that prevented cross-signing with users on worker-mode synapses. ([\#7255](https://github.com/matrix-org/synapse/issues/7255)) +- Do not treat display names as globs in push rules. ([\#7271](https://github.com/matrix-org/synapse/issues/7271)) +- Fix a bug with cross-signing devices belonging to remote users who did not share a room with any user on the local homeserver. ([\#7289](https://github.com/matrix-org/synapse/issues/7289)) + + Synapse 1.12.3 (2020-04-03) =========================== diff --git a/changelog.d/7160.feature b/changelog.d/7160.feature deleted file mode 100644 index c1205969a..000000000 --- a/changelog.d/7160.feature +++ /dev/null @@ -1 +0,0 @@ -Always send users their own device updates. diff --git a/changelog.d/7255.bugfix b/changelog.d/7255.bugfix deleted file mode 100644 index a96d52256..000000000 --- a/changelog.d/7255.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix a bug that prevented cross-signing with users on worker-mode synapses. \ No newline at end of file diff --git a/changelog.d/7271.bugfix b/changelog.d/7271.bugfix deleted file mode 100644 index e8315e4ce..000000000 --- a/changelog.d/7271.bugfix +++ /dev/null @@ -1 +0,0 @@ -Do not treat display names as globs in push rules. diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix deleted file mode 100644 index 84699e50a..000000000 --- a/changelog.d/7289.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix a bug with cross-signing devices belonging to remote users who did not share a room with any user on the local homeserver. diff --git a/changelog.d/7311.feature b/changelog.d/7311.feature deleted file mode 100644 index c3adc1d6e..000000000 --- a/changelog.d/7311.feature +++ /dev/null @@ -1 +0,0 @@ -Add support for handling GET requests for account_data on a worker. From ba0aac5e44bf98837d1f099d9638f2fd4d4deaca Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Apr 2020 14:50:51 +0100 Subject: [PATCH 11/12] formatting for the changelog --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 1ccc39f4a..7f47011a3 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,7 +5,7 @@ Features -------- - Always send users their own device updates. ([\#7160](https://github.com/matrix-org/synapse/issues/7160)) -- Add support for handling GET requests for account_data on a worker. ([\#7311](https://github.com/matrix-org/synapse/issues/7311)) +- Add support for handling GET requests for `account_data` on a worker. ([\#7311](https://github.com/matrix-org/synapse/issues/7311)) Bugfixes From ce9b62e13f7d653e5099ae4a159c0e5cde419cf8 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 23 Apr 2020 10:59:10 -0400 Subject: [PATCH 12/12] 1.12.4 --- CHANGES.md | 6 ++++++ debian/changelog | 6 ++++++ synapse/__init__.py | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 7f47011a3..342768c8c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,9 @@ +Synapse 1.12.4 (2020-04-23) +=========================== + +No significant changes. + + Synapse 1.12.4rc1 (2020-04-22) ============================== diff --git a/debian/changelog b/debian/changelog index 642115fc5..8eaca8523 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +matrix-synapse-py3 (1.12.4) stable; urgency=medium + + * New synapse release 1.12.4. + + -- Synapse Packaging team Thu, 23 Apr 2020 10:58:14 -0400 + matrix-synapse-py3 (1.12.3) stable; urgency=medium [ Richard van der Hoff ] diff --git a/synapse/__init__.py b/synapse/__init__.py index fff9d311f..d8d340f42 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -36,7 +36,7 @@ try: except ImportError: pass -__version__ = "1.12.4rc1" +__version__ = "1.12.4" if bool(os.environ.get("SYNAPSE_TEST_PATCH_LOG_CONTEXTS", False)): # We import here so that we don't have to install a bunch of deps when