From 8348481b7e0e37d34642af97dce49e3f6d288476 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 12:36:01 +0100 Subject: [PATCH 01/32] Query missing cross-signing keys on local sig upload --- synapse/federation/transport/client.py | 2 +- synapse/handlers/e2e_keys.py | 45 +++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index dc563538deb6..d25db5daae2f 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -412,7 +412,7 @@ def query_client_keys(self, destination, query_content, timeout): destination(str): The server to query. query_content(dict): The user ids to query. Returns: - A dict containg the device keys. + A dict containing the device keys. """ path = _create_v1_path("/user/keys/query") diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 8d7075f2eb2a..1111c1795181 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -27,7 +27,15 @@ from twisted.internet import defer -from synapse.api.errors import CodeMessageException, Codes, NotFoundError, SynapseError +from synapse.api.errors import ( + CodeMessageException, + Codes, + FederationDeniedError, + HttpResponseException, + NotFoundError, + RequestSendFailed, + SynapseError, +) from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.logging.opentracing import log_kv, set_tag, tag_args, trace from synapse.replication.http.devices import ReplicationUserDevicesResyncRestServlet @@ -174,8 +182,8 @@ def do_remote_query(destination): """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] @@ -981,8 +989,37 @@ def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None user_id, key_type, from_user_id ) if key is None: - logger.debug("no %s key found for %s", key_type, user_id) + # Attempt to fetch the missing key from the remote user's server + user = UserID.from_string(user_id) + try: + remote_result = yield self.federation.query_client_keys( + user.domain, {"device_keys": {user_id: []}}, timeout=10 * 1000 + ) + + # The key_type variable passed to this function is in the form + # "self_signing","master" etc. Whereas the results returned from + # the remote server use "self_signing_keys", "master_keys" etc. + # Translate that here. + remote_key_type = key_type + "_keys" + + key = remote_result.get(remote_key_type, {}).get(user_id) + except ( + HttpResponseException, + NotRetryingDestination, + FederationDeniedError, + RequestSendFailed, + ) as e: + logger.warning( + "Unable to query %s for cross-signing keys of user %s: %s", + user.domain, + user_id, + e, + ) + + if key is None: + 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) return key, key_id, verify_key From 106349532b3c4817ad87fe54b2fbca0ca4548328 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 12:50:34 +0100 Subject: [PATCH 02/32] Add changelog --- changelog.d/7289.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7289.bugfix diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix new file mode 100644 index 000000000000..e3c96dba9504 --- /dev/null +++ b/changelog.d/7289.bugfix @@ -0,0 +1 @@ +Query remote user's cross-signing keys on local user cross-signing signature upload if necessary. \ No newline at end of file From cc864574f739ae075af50ba3f9af3b5ca22a7899 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 13:54:59 +0100 Subject: [PATCH 03/32] Save retrieved keys to the db --- synapse/handlers/e2e_keys.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 1111c1795181..9c1fbaa9b558 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -996,6 +996,15 @@ def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None user.domain, {"device_keys": {user_id: []}}, timeout=10 * 1000 ) + # Save these keys to the database for subsequent queries + for key_type, remote_user_id in remote_result.items(): + if remote_user_id != user_id: + continue + key_contents = remote_result[key_type][remote_user_id] + key_type = key_type[:-5] # Remove the "_keys" from the key type + + yield self.store.set_e2e_cross_signing_key(user_id, key_type, key_contents) + # The key_type variable passed to this function is in the form # "self_signing","master" etc. Whereas the results returned from # the remote server use "self_signing_keys", "master_keys" etc. From c265bc7d9a48cbb8d33f636cbe16ba76ebd896b4 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 13:58:29 +0100 Subject: [PATCH 04/32] 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 9c1fbaa9b558..3fbb0d104af5 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1003,7 +1003,9 @@ def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None key_contents = remote_result[key_type][remote_user_id] key_type = key_type[:-5] # Remove the "_keys" from the key type - yield self.store.set_e2e_cross_signing_key(user_id, key_type, key_contents) + yield self.store.set_e2e_cross_signing_key( + user_id, key_type, key_contents + ) # The key_type variable passed to this function is in the form # "self_signing","master" etc. Whereas the results returned from From 39ed9f6938ed398dadd475d4354484bb11a58785 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 16:14:01 +0100 Subject: [PATCH 05/32] Fix and de-brittle remote result dict processing --- synapse/handlers/e2e_keys.py | 54 +++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 3fbb0d104af5..c10eec8baa9e 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -985,35 +985,49 @@ def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None Raises: NotFoundError: if the key is not found """ + 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: + + if key is None and self.is_mine(user): # Attempt to fetch the missing key from the remote user's server - user = UserID.from_string(user_id) try: remote_result = yield self.federation.query_client_keys( user.domain, {"device_keys": {user_id: []}}, timeout=10 * 1000 ) - # Save these keys to the database for subsequent queries - for key_type, remote_user_id in remote_result.items(): - if remote_user_id != user_id: - continue - key_contents = remote_result[key_type][remote_user_id] - key_type = key_type[:-5] # Remove the "_keys" from the key type - - yield self.store.set_e2e_cross_signing_key( - user_id, key_type, key_contents - ) - - # The key_type variable passed to this function is in the form - # "self_signing","master" etc. Whereas the results returned from - # the remote server use "self_signing_keys", "master_keys" etc. - # Translate that here. - remote_key_type = key_type + "_keys" - - key = remote_result.get(remote_key_type, {}).get(user_id) + # Process the result + for remote_key_type, remote_user_dict in remote_result.items(): + # The key_type variable passed to this function is in the form + # "self_signing","master" etc. whereas the results returned from + # the remote server use "self_signing_keys", "master_keys" etc. + # Remove the "_keys" from the key type + if remote_key_type.endswith("_keys"): + remote_key_type = remote_key_type[:-5] + + # remote_user_dict is a dictionary in the form of + # { + # "user_id": { + # "master_keys": ... + # }, + # ... + # } + + # Only extract the keys that pertain to the requested user + key_content_list = remote_user_dict.get(user_id, {}).values() + + for key_content in key_content_list: + # If the key_type here matches the key we're requesting, + # then this is the key we want to return + if remote_key_type == key_type: + key = key_content + + # At the same time, save the key to the database for subsequent + # queries + yield self.store.set_e2e_cross_signing_key( + user_id, remote_key_type, key_content + ) except ( HttpResponseException, NotRetryingDestination, From fd8d154e6da6830b1cc828b7e6dfcddf53b9cad2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:13:32 +0100 Subject: [PATCH 06/32] Use query_user_devices instead, assume only master, self_signing key types --- synapse/federation/transport/client.py | 14 ++++-- synapse/handlers/e2e_keys.py | 69 ++++++++++++-------------- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index d25db5daae2f..c35637a571dd 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -406,13 +406,19 @@ def query_client_keys(self, destination, query_content, timeout): "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 the device keys. + A dict containing device and cross-signing keys. """ path = _create_v1_path("/user/keys/query") @@ -429,14 +435,16 @@ def query_user_devices(self, destination, user_id, timeout): 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 c10eec8baa9e..f92544c7316f 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -969,12 +969,14 @@ def _process_other_signatures(self, user_id, signatures): return signature_list, failures @defer.inlineCallbacks - def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None): + def _get_e2e_cross_signing_verify_key( + self, user_id, desired_key_type, from_user_id=None + ): """Fetch the cross-signing public key from storage and interpret it. Args: user_id (str): the user whose key should be fetched - key_type (str): the type of key to fetch + desired_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. @@ -987,47 +989,38 @@ def _get_e2e_cross_signing_verify_key(self, user_id, key_type, from_user_id=None """ user = UserID.from_string(user_id) key = yield self.store.get_e2e_cross_signing_key( - user_id, key_type, from_user_id + user_id, desired_key_type, from_user_id ) - if key is None and self.is_mine(user): - # Attempt to fetch the missing key from the remote user's server + # If we still can't find the key, and we're looking for keys of another user, + # then attempt to fetch the missing key from the remote user's server. + # + # We don't get "user_signing" keys from remote servers, so disallow that here + if ( + key is None + and not self.is_mine(user) + and desired_key_type != "user_signing" + ): try: - remote_result = yield self.federation.query_client_keys( - user.domain, {"device_keys": {user_id: []}}, timeout=10 * 1000 + remote_result = yield self.federation.query_user_devices( + user.domain, user_id ) - # Process the result - for remote_key_type, remote_user_dict in remote_result.items(): - # The key_type variable passed to this function is in the form - # "self_signing","master" etc. whereas the results returned from - # the remote server use "self_signing_keys", "master_keys" etc. - # Remove the "_keys" from the key type - if remote_key_type.endswith("_keys"): - remote_key_type = remote_key_type[:-5] - - # remote_user_dict is a dictionary in the form of - # { - # "user_id": { - # "master_keys": ... - # }, - # ... - # } - - # Only extract the keys that pertain to the requested user - key_content_list = remote_user_dict.get(user_id, {}).values() - - for key_content in key_content_list: - # If the key_type here matches the key we're requesting, - # then this is the key we want to return - if remote_key_type == key_type: - key = key_content - - # At the same time, save the key to the database for subsequent - # queries - yield self.store.set_e2e_cross_signing_key( - user_id, remote_key_type, key_content - ) + # Process each of the retrieved cross-signing keys + for key_type in ["master", "self_signing"]: + key_content = remote_result.get(key_type + "_key") + if not key_content: + continue + + # If this is the desired key type, return it + if key_type == desired_key_type: + key = key_content + + # At the same time, store this key in the db for + # subsequent queries + yield self.store.set_e2e_cross_signing_key( + user_id, key_type, key_content + ) except ( HttpResponseException, NotRetryingDestination, From 759b6b04554b79fafedaa0ddb58ad760889e6586 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:23:28 +0100 Subject: [PATCH 07/32] Make changelog more useful --- changelog.d/7289.bugfix | 2 +- synapse/handlers/e2e_keys.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix index e3c96dba9504..5b4fbd77ac25 100644 --- a/changelog.d/7289.bugfix +++ b/changelog.d/7289.bugfix @@ -1 +1 @@ -Query remote user's cross-signing keys on local user cross-signing signature upload if necessary. \ No newline at end of file +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/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index f92544c7316f..fee1f9d5097b 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -973,6 +973,8 @@ def _get_e2e_cross_signing_verify_key( self, user_id, desired_key_type, from_user_id=None ): """Fetch the cross-signing public key from storage and interpret it. + If we cannot find the public key locally, we query the keys from the + homeserver they belong to, then update our local copy. Args: user_id (str): the user whose key should be fetched @@ -994,11 +996,10 @@ def _get_e2e_cross_signing_verify_key( # If we still can't find the key, and we're looking for keys of another user, # then attempt to fetch the missing key from the remote user's server. - # - # We don't get "user_signing" keys from remote servers, so disallow that here if ( key is None and not self.is_mine(user) + # We don't get "user_signing" keys from remote servers, so disallow that here and desired_key_type != "user_signing" ): try: From 03d2c8c65b52a5162a0c811ab92521dc93992a2e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:48:52 +0100 Subject: [PATCH 08/32] Remove very specific exception handling --- synapse/handlers/e2e_keys.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index fee1f9d5097b..3cd71d2e6d03 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -30,10 +30,7 @@ from synapse.api.errors import ( CodeMessageException, Codes, - FederationDeniedError, - HttpResponseException, NotFoundError, - RequestSendFailed, SynapseError, ) from synapse.logging.context import make_deferred_yieldable, run_in_background @@ -1022,12 +1019,7 @@ def _get_e2e_cross_signing_verify_key( yield self.store.set_e2e_cross_signing_key( user_id, key_type, key_content ) - except ( - HttpResponseException, - NotRetryingDestination, - FederationDeniedError, - RequestSendFailed, - ) as e: + except Exception as e: logger.warning( "Unable to query %s for cross-signing keys of user %s: %s", user.domain, From b386658d28eac83cb69cdec6cb17e132b01541db Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:51:00 +0100 Subject: [PATCH 09/32] Wrap get_verify_key_from_cross_signing_key in a try/except --- synapse/handlers/e2e_keys.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 3cd71d2e6d03..1315093af230 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1031,8 +1031,11 @@ def _get_e2e_cross_signing_verify_key( 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) - return key, key_id, verify_key + try: + key_id, verify_key = get_verify_key_from_cross_signing_key(key) + return key, key_id, verify_key + except ValueError: + raise SynapseError(502, "Invalid %s key retrieved from remote server") def _check_cross_signing_key(key, user_id, key_type, signing_key=None): From bd9a6712ae8655c9017c11a8c206c810268aa2bd Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:54:34 +0100 Subject: [PATCH 10/32] Note that _get_e2e_cross_signing_verify_key can raise a SynapseError --- synapse/handlers/e2e_keys.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 1315093af230..e1775a52633e 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -985,6 +985,7 @@ def _get_e2e_cross_signing_verify_key( Raises: NotFoundError: if the key is not found + SynapseError: if the user_id is invalid """ user = UserID.from_string(user_id) key = yield self.store.get_e2e_cross_signing_key( From 745e6538a3984d209d414e1589d616a06b8bb163 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:55:59 +0100 Subject: [PATCH 11/32] lint --- synapse/handlers/e2e_keys.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index e1775a52633e..1df1f85c4ea2 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -27,12 +27,7 @@ from twisted.internet import defer -from synapse.api.errors import ( - CodeMessageException, - Codes, - NotFoundError, - SynapseError, -) +from synapse.api.errors import CodeMessageException, Codes, NotFoundError, SynapseError from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.logging.opentracing import log_kv, set_tag, tag_args, trace from synapse.replication.http.devices import ReplicationUserDevicesResyncRestServlet From f8b6f14ae0943b774a4cc2e74bc0ab29db8feeb2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 17:59:47 +0100 Subject: [PATCH 12/32] Add comment explaining why this is useful --- synapse/handlers/e2e_keys.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 1df1f85c4ea2..afeb6121f9e2 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -980,7 +980,7 @@ def _get_e2e_cross_signing_verify_key( Raises: NotFoundError: if the key is not found - SynapseError: if the user_id is invalid + SynapseError: if `user_id` is invalid """ user = UserID.from_string(user_id) key = yield self.store.get_e2e_cross_signing_key( @@ -989,6 +989,11 @@ def _get_e2e_cross_signing_verify_key( # If we still can't find the key, 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 and + # store it just in case. if ( key is None and not self.is_mine(user) From 37ae6430f2a6f39ece79b7a1ee0851cdaf033b7b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Apr 2020 20:01:34 +0100 Subject: [PATCH 13/32] Only fetch master and self_signing key types --- synapse/handlers/e2e_keys.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index afeb6121f9e2..97aedb369e56 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -997,8 +997,8 @@ def _get_e2e_cross_signing_verify_key( if ( key is None and not self.is_mine(user) - # We don't get "user_signing" keys from remote servers, so disallow that here - and desired_key_type != "user_signing" + # We only get "master" and "self_signing" keys from remote servers + and desired_key_type in ["master", "self_signing"] ): try: remote_result = yield self.federation.query_user_devices( From 83861c3a2557e26ce96bf1597e58802f0d99a807 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Apr 2020 11:33:43 +0100 Subject: [PATCH 14/32] Fix log statements, docstrings --- synapse/handlers/e2e_keys.py | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 97aedb369e56..6094c6e259a3 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -962,16 +962,18 @@ def _process_other_signatures(self, user_id, signatures): @defer.inlineCallbacks def _get_e2e_cross_signing_verify_key( - self, user_id, desired_key_type, from_user_id=None + self, user_id: str, desired_key_type: str, from_user_id:str = None ): - """Fetch the cross-signing public key from storage and interpret it. - If we cannot find the public key locally, we query the keys from the - homeserver they belong to, then update our local copy. + """Fetch or request the given 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 - desired_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 + desired_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: @@ -1022,21 +1024,28 @@ def _get_e2e_cross_signing_verify_key( ) except Exception as e: logger.warning( - "Unable to query %s for cross-signing keys of user %s: %s", + "Unable to query %s for cross-signing keys of user %s: %s %s", user.domain, user_id, + type(e), e, ) if key is None: - logger.debug("No %s key found for %s", key_type, user_id) - raise NotFoundError("No %s key found for %s" % (key_type, user_id)) + logger.debug("No %s key found for %s", desired_key_type, user_id) + raise NotFoundError("No %s key found for %s" % (desired_key_type, user_id)) try: key_id, verify_key = get_verify_key_from_cross_signing_key(key) - return key, key_id, verify_key - except ValueError: - raise SynapseError(502, "Invalid %s key retrieved from remote server") + except ValueError as e: + logger.debug( + "Invalid %s key retrieved: %s - %s %s", desired_key_type, key, type(e), e + ) + raise SynapseError( + 502, "Invalid %s key retrieved from remote server", desired_key_type + ) + + return key, key_id, verify_key def _check_cross_signing_key(key, user_id, key_type, signing_key=None): From 671178b028e36e18ca3fd923034284f495025629 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Apr 2020 11:36:08 +0100 Subject: [PATCH 15/32] Remove extraneous items from remote query try/except --- synapse/handlers/e2e_keys.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 6094c6e259a3..863898cdf4f4 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -989,6 +989,7 @@ def _get_e2e_cross_signing_verify_key( user_id, desired_key_type, from_user_id ) + # If we still can't find the key, and we're looking for keys of another user, # then attempt to fetch the missing key from the remote user's server. # @@ -996,19 +997,30 @@ def _get_e2e_cross_signing_verify_key( # 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 and # store it just in case. + supported_remote_key_types = ["master", "self_signing"] if ( key is None and not self.is_mine(user) # We only get "master" and "self_signing" keys from remote servers - and desired_key_type in ["master", "self_signing"] + and desired_key_type in supported_remote_key_types ): + remote_result = None try: remote_result = yield self.federation.query_user_devices( user.domain, user_id ) + except Exception as e: + logger.warning( + "Unable to query %s for cross-signing keys of user %s: %s %s", + user.domain, + user_id, + type(e), + e, + ) + if remote_result is not None: # Process each of the retrieved cross-signing keys - for key_type in ["master", "self_signing"]: + for key_type in supported_remote_key_types: key_content = remote_result.get(key_type + "_key") if not key_content: continue @@ -1022,14 +1034,6 @@ def _get_e2e_cross_signing_verify_key( yield self.store.set_e2e_cross_signing_key( user_id, key_type, key_content ) - except Exception as e: - logger.warning( - "Unable to query %s for cross-signing keys of user %s: %s %s", - user.domain, - user_id, - type(e), - e, - ) if key is None: logger.debug("No %s key found for %s", desired_key_type, user_id) From 2d88b5d39dd451079205577d778fac89abded96b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Apr 2020 11:39:30 +0100 Subject: [PATCH 16/32] lint --- synapse/handlers/e2e_keys.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 863898cdf4f4..f80f0188c7ff 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -962,7 +962,7 @@ def _process_other_signatures(self, user_id, signatures): @defer.inlineCallbacks def _get_e2e_cross_signing_verify_key( - self, user_id: str, desired_key_type: str, from_user_id:str = None + self, user_id: str, desired_key_type: str, from_user_id: str = None ): """Fetch or request the given cross-signing public key. @@ -989,8 +989,7 @@ def _get_e2e_cross_signing_verify_key( user_id, desired_key_type, from_user_id ) - - # If we still can't find the key, and we're looking for keys of another user, + # If we still can't find the key, 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 @@ -1043,7 +1042,11 @@ def _get_e2e_cross_signing_verify_key( 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", desired_key_type, key, type(e), e + "Invalid %s key retrieved: %s - %s %s", + desired_key_type, + key, + type(e), + e, ) raise SynapseError( 502, "Invalid %s key retrieved from remote server", desired_key_type From f41730078ed14270ca69634b07cb9885934e299b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Apr 2020 12:07:19 +0100 Subject: [PATCH 17/32] Factor key retrieval out into a separate function --- synapse/handlers/e2e_keys.py | 104 ++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 43 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index f80f0188c7ff..493fcb4d9db6 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 from six import iteritems @@ -962,7 +963,7 @@ def _process_other_signatures(self, user_id, signatures): @defer.inlineCallbacks def _get_e2e_cross_signing_verify_key( - self, user_id: str, desired_key_type: str, from_user_id: str = None + self, user_id: str, key_type: str, from_user_id: str = None ): """Fetch or request the given cross-signing public key. @@ -972,7 +973,7 @@ def _get_e2e_cross_signing_verify_key( Args: user_id: the user whose key should be fetched - desired_key_type: the type of key to fetch + 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. @@ -986,7 +987,7 @@ def _get_e2e_cross_signing_verify_key( """ user = UserID.from_string(user_id) key = yield self.store.get_e2e_cross_signing_key( - user_id, desired_key_type, from_user_id + user_id, key_type, from_user_id ) # If we still can't find the key, and we're looking for keys of another user @@ -996,64 +997,81 @@ def _get_e2e_cross_signing_verify_key( # 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 and # store it just in case. - supported_remote_key_types = ["master", "self_signing"] if ( key is None and not self.is_mine(user) # We only get "master" and "self_signing" keys from remote servers - and desired_key_type in supported_remote_key_types + and key_type in ["master", "self_signing"] ): - remote_result = None - try: - remote_result = yield self.federation.query_user_devices( - user.domain, user_id - ) - except Exception as e: - logger.warning( - "Unable to query %s for cross-signing keys of user %s: %s %s", - user.domain, - user_id, - type(e), - e, - ) - - if remote_result is not None: - # Process each of the retrieved cross-signing keys - for key_type in supported_remote_key_types: - key_content = remote_result.get(key_type + "_key") - if not key_content: - continue - - # If this is the desired key type, return it - if key_type == desired_key_type: - key = key_content - - # At the same time, store this key in the db for - # subsequent queries - yield self.store.set_e2e_cross_signing_key( - user_id, key_type, key_content - ) + 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", desired_key_type, user_id) - raise NotFoundError("No %s key found for %s" % (desired_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)) 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", - desired_key_type, - key, - type(e), - e, + "Invalid %s key retrieved: %s - %s %s", key_type, key, type(e), e, ) raise SynapseError( - 502, "Invalid %s key retrieved from remote server", desired_key_type + 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, + ) -> Optional[Dict]: + """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: + The retrieved key content, or None if the key could not be retrieved + """ + 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 + key = None + for key_type in ["master", "self_signing"]: + key_content = remote_result.get(key_type + "_key") + if not key_content: + continue + + # If this is the desired key type, return it + if key_type == desired_key_type: + key = key_content + + # 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 + ) + + return 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 2f8705143ffeceb8c14414ddd829f347eb7dcd8c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Apr 2020 12:30:54 +0100 Subject: [PATCH 18/32] Send device updates, modeled after SigningKeyEduUpdater._handle_signing_key_updates --- synapse/handlers/e2e_keys.py | 78 ++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 22 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 493fcb4d9db6..be0d37fd4103 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -16,7 +16,7 @@ # limitations under the License. import logging -from typing import Dict, Optional +from typing import Dict, Optional, Tuple from six import iteritems @@ -24,6 +24,7 @@ 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 @@ -986,17 +987,21 @@ def _get_e2e_cross_signing_verify_key( 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 still can't find the key, and we're looking for keys of another user - # then attempt to fetch the missing key from the remote user's server. + # 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 and - # store it just in case. + # 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) @@ -1011,22 +1016,24 @@ def _get_e2e_cross_signing_verify_key( logger.debug("No %s key found for %s", key_type, user_id) raise NotFoundError("No %s key found for %s" % (key_type, user_id)) - 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 - ) + # 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, - ) -> Optional[Dict]: + ) -> 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 @@ -1037,7 +1044,8 @@ def _retrieve_cross_signing_keys_for_remote_user( desired_key_type: The type of key to receive. One of "master", "self_signing" Returns: - The retrieved key content, or None if the key could not be retrieved + 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( @@ -1054,23 +1062,49 @@ def _retrieve_cross_signing_keys_for_remote_user( return None # Process each of the retrieved cross-signing keys - key = None + 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 - # If this is the desired key type, return it - if key_type == desired_key_type: - key = key_content - # 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 ) - return key + # 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): From 5990d1c9c658ae8893045d1d7620c45f355d2532 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Apr 2020 12:54:55 +0100 Subject: [PATCH 19/32] Update method docstring --- synapse/handlers/e2e_keys.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index be0d37fd4103..afc173ab2f66 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -966,7 +966,7 @@ def _process_other_signatures(self, user_id, signatures): def _get_e2e_cross_signing_verify_key( self, user_id: str, key_type: str, from_user_id: str = None ): - """Fetch or request the given cross-signing public key. + """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 From 4f8ba5c7b75203c28dd718b90e582eab77fd4d4c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Apr 2020 11:57:13 +0100 Subject: [PATCH 20/32] Remove extraneous key_id and verify_key --- synapse/handlers/e2e_keys.py | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index afc173ab2f66..d036d5585402 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -987,9 +987,6 @@ def _get_e2e_cross_signing_verify_key( 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 ) @@ -1016,17 +1013,15 @@ def _get_e2e_cross_signing_verify_key( 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 - ) + 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 @@ -1094,7 +1089,7 @@ def _retrieve_cross_signing_keys_for_remote_user( continue device_ids.append(verify_key.version) - # If this is the desired key type, save it and it's ID/VerifyKey + # If this is the desired key type, save it and its ID/VerifyKey if key_type == desired_key_type: final_key = key_content final_verify_key = verify_key From 9240abccfb006a853ef53a2b1a53cb80887a79e6 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Apr 2020 11:58:35 +0100 Subject: [PATCH 21/32] Update changelog --- changelog.d/7289.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix index 5b4fbd77ac25..1568e1569b30 100644 --- a/changelog.d/7289.bugfix +++ b/changelog.d/7289.bugfix @@ -1 +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. +Fix a bug with cross-signing devices of users on other homeservers while in worker mode. \ No newline at end of file From 328242364bd1201fcc82ef7f879f645b56d2f974 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Apr 2020 15:35:22 +0100 Subject: [PATCH 22/32] Update changelog --- changelog.d/7289.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix index 1568e1569b30..1073f910d994 100644 --- a/changelog.d/7289.bugfix +++ b/changelog.d/7289.bugfix @@ -1 +1 @@ -Fix a bug with cross-signing devices of users on other homeservers while in worker mode. \ No newline at end of file +Fix a bug with cross-signing devices with remote users when they did not share a room with any user on the local homeserver. \ No newline at end of file From 95dd9d55b7d8af99ca35474799d13217056a9e4f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Apr 2020 16:05:44 +0100 Subject: [PATCH 23/32] Resolve review comments --- changelog.d/7289.bugfix | 2 +- synapse/handlers/e2e_keys.py | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix index 1073f910d994..b6f74a47417e 100644 --- a/changelog.d/7289.bugfix +++ b/changelog.d/7289.bugfix @@ -1 +1 @@ -Fix a bug with cross-signing devices with remote users when they did not share a room with any user on the local homeserver. \ No newline at end of file +Fix a bug with cross-signing devices with remote users when they did not share a room with any user on the local homeserver. diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index d036d5585402..862933210bd0 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1005,22 +1005,22 @@ def _get_e2e_cross_signing_verify_key( # 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( + key, key_id, verify_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.warning("No %s key found for %s", key_type, user_id) raise NotFoundError("No %s key found for %s" % (key_type, user_id)) try: key_id, verify_key = get_verify_key_from_cross_signing_key(key) except ValueError as e: - logger.debug( + logger.warning( "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 + 502, "Invalid %s key retrieved from remote server" % (key_type,) ) return key, key_id, verify_key @@ -1028,7 +1028,7 @@ def _get_e2e_cross_signing_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 @@ -1039,7 +1039,8 @@ def _retrieve_cross_signing_keys_for_remote_user( 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. + 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: @@ -1054,7 +1055,7 @@ def _retrieve_cross_signing_keys_for_remote_user( type(e), e, ) - return None + return None, None, None # Process each of the retrieved cross-signing keys final_key = None @@ -1079,7 +1080,7 @@ def _retrieve_cross_signing_keys_for_remote_user( # 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( + logger.warning( "Invalid %s key retrieved: %s - %s %s", key_type, key_content, From 4f41f3730961d4f8440d69b05444631e2fb3a379 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Apr 2020 16:07:00 +0100 Subject: [PATCH 24/32] lint --- synapse/handlers/e2e_keys.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 862933210bd0..6ea7a42c05be 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 @@ 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 @@ -1005,9 +1003,11 @@ def _get_e2e_cross_signing_verify_key( # We only get "master" and "self_signing" keys from remote servers and key_type in ["master", "self_signing"] ): - key, key_id, verify_key = yield self._retrieve_cross_signing_keys_for_remote_user( - user, key_type - ) + ( + key, + key_id, + verify_key, + ) = yield self._retrieve_cross_signing_keys_for_remote_user(user, key_type) if key is None: logger.warning("No %s key found for %s", key_type, user_id) From 6d559bab03da1bcc6196532a015f9727bc03f052 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 21 Apr 2020 12:23:29 +0100 Subject: [PATCH 25/32] Update changelog.d/7289.bugfix Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/7289.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7289.bugfix b/changelog.d/7289.bugfix index b6f74a47417e..84699e50a9ad 100644 --- a/changelog.d/7289.bugfix +++ b/changelog.d/7289.bugfix @@ -1 +1 @@ -Fix a bug with cross-signing devices with remote users when they did not share a room with any user on the local homeserver. +Fix a bug with cross-signing devices belonging to remote users who did not share a room with any user on the local homeserver. From 1b4dda5a8dd6d248205edb09076a233c111ac642 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Apr 2020 14:37:26 +0100 Subject: [PATCH 26/32] Refactor _get_e2e_cross_signing_verify_key --- synapse/handlers/e2e_keys.py | 39 +++++++++++++++--------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 6ea7a42c05be..633dc2ca6478 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -989,40 +989,33 @@ def _get_e2e_cross_signing_verify_key( user_id, key_type, from_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 fetch the key here, + # 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 ( - 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, - key_id, - verify_key, - ) = yield self._retrieve_cross_signing_keys_for_remote_user(user, key_type) + 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 + return + + ( + key, + key_id, + verify_key, + ) = yield self._retrieve_cross_signing_keys_for_remote_user(user, key_type) if key is None: - logger.warning("No %s key found for %s", key_type, user_id) raise NotFoundError("No %s key found for %s" % (key_type, user_id)) - try: - key_id, verify_key = get_verify_key_from_cross_signing_key(key) - except ValueError as e: - logger.warning( - "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 From 7cb1e4846a314dd900d758f574b4cd9c116b4a31 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Apr 2020 15:35:01 +0100 Subject: [PATCH 27/32] Refactor and add validation to _retrieve_cross_signing_keys_for_remote_user --- synapse/handlers/e2e_keys.py | 52 ++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 633dc2ca6478..a98548b674dd 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1051,22 +1051,33 @@ def _retrieve_cross_signing_keys_for_remote_user( return None, None, None # Process each of the retrieved cross-signing keys - final_key = None - final_key_id = None - final_verify_key = None - device_ids = [] + 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 - # 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 - ) + # 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 - # Note down the device ID attached to this key + # 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 @@ -1081,19 +1092,26 @@ def _retrieve_cross_signing_keys_for_remote_user( e, ) continue - device_ids.append(verify_key.version) + + # 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: - final_key = key_content - final_verify_key = verify_key - final_key_id = key_id + 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 device_ids: - yield self.device_handler.notify_device_update(user.to_string(), device_ids) + if retrieved_device_ids: + yield self.device_handler.notify_device_update(user.to_string(), retrieved_device_ids) - return final_key, final_key_id, final_verify_key + return desired_key, desired_key_id, desired_verify_key def _check_cross_signing_key(key, user_id, key_type, signing_key=None): From 74eaac0052fdaae1453661902a22a24e316160d8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Apr 2020 15:45:57 +0100 Subject: [PATCH 28/32] Improve details of query_client_keys and query_user_devices docstrings --- synapse/federation/transport/client.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index c35637a571dd..7f25c2016ece 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -407,12 +407,12 @@ def query_client_keys(self, destination, query_content, timeout): "": { "": {...} } } - "master_keys": { + "master_key": { "": {...} } } - "self_signing_keys": { + "self_signing_key": { "": {...} - } } } + } } Args: destination(str): The server to query. @@ -436,9 +436,22 @@ def query_user_devices(self, destination, user_id, timeout): { "stream_id": "...", "devices": [ { ... } ], - "master_key": { ... }, - "self_signing_key: { ... } - } + "master_key": { + "user_id": "", + "usage": [...], + "keys": {...}, + "signatures": { + "": {...} + } + }, + "self_signing_key": { + "user_id": "", + "usage": [...], + "keys": {...}, + "signatures": { + "": {...} + } + } } Args: destination(str): The server to query. From b08b7c71fc27f1cf43e88b921eeb4720ddc280fc Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Apr 2020 15:46:59 +0100 Subject: [PATCH 29/32] lint --- synapse/handlers/e2e_keys.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index a98548b674dd..119a2d0ce4d6 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1065,7 +1065,7 @@ def _retrieve_cross_signing_keys_for_remote_user( logger.warning( "Invalid %s key retrieved, missing user_id field: %s", key_type, - key_content + key_content, ) continue if user.to_string() != key_content["user_id"]: @@ -1109,7 +1109,9 @@ def _retrieve_cross_signing_keys_for_remote_user( # Notify clients that new devices for this user have been discovered if retrieved_device_ids: - yield self.device_handler.notify_device_update(user.to_string(), retrieved_device_ids) + yield self.device_handler.notify_device_update( + user.to_string(), retrieved_device_ids + ) return desired_key, desired_key_id, desired_verify_key From 8484a72f59e813393e039bf36e41a9018f56b089 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 22 Apr 2020 11:20:45 +0100 Subject: [PATCH 30/32] Address review comments --- synapse/federation/transport/client.py | 9 ++++----- synapse/handlers/e2e_keys.py | 3 ++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 7f25c2016ece..80c1433e7a09 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -406,10 +406,10 @@ def query_client_keys(self, destination, query_content, timeout): "device_keys": { "": { "": {...} - } } + } }, "master_key": { "": {...} - } } + } }, "self_signing_key": { "": {...} } } @@ -442,8 +442,7 @@ def query_user_devices(self, destination, user_id, timeout): "keys": {...}, "signatures": { "": {...} - } - }, + } }, "self_signing_key": { "user_id": "", "usage": [...], @@ -490,7 +489,7 @@ def claim_client_keys(self, destination, query_content, timeout): 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 119a2d0ce4d6..8f1bc0323c24 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1005,7 +1005,7 @@ def _get_e2e_cross_signing_verify_key( 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 - return + raise NotFoundError("No %s key found for %s" % (key_type, user_id)) ( key, @@ -1109,6 +1109,7 @@ def _retrieve_cross_signing_keys_for_remote_user( # 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 ) From 2932b9b224607c9dd4764e1ca3e0aae67474d196 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 22 Apr 2020 11:27:22 +0100 Subject: [PATCH 31/32] JSON brace endings on separate lines --- synapse/federation/transport/client.py | 29 ++++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 80c1433e7a09..dd3ea3e3f128 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -399,20 +399,24 @@ def query_client_keys(self, destination, query_content, timeout): { "device_keys": { "": [""] - } } + } + } Response: { "device_keys": { "": { "": {...} - } }, + } + }, "master_key": { "": {...} - } }, + } + }, "self_signing_key": { "": {...} - } } + } + } Args: destination(str): The server to query. @@ -442,7 +446,8 @@ def query_user_devices(self, destination, user_id, timeout): "keys": {...}, "signatures": { "": {...} - } }, + } + }, "self_signing_key": { "user_id": "", "usage": [...], @@ -450,7 +455,8 @@ def query_user_devices(self, destination, user_id, timeout): "signatures": { "": {...} } - } } + } + } Args: destination(str): The server to query. @@ -474,8 +480,10 @@ def claim_client_keys(self, destination, query_content, timeout): { "one_time_keys": { "": { - "": "" - } } } + "": "" + } + } + } Response: { @@ -483,7 +491,10 @@ def claim_client_keys(self, destination, query_content, timeout): "": { "": { ":": "" - } } } } + } + } + } + } Args: destination(str): The server to query. From ebea2ee83559a440c6b4f16cd5a69e60550645d1 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 22 Apr 2020 11:33:18 +0100 Subject: [PATCH 32/32] Spaces and braces --- synapse/federation/transport/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index dd3ea3e3f128..383e3fdc8bef 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -446,8 +446,8 @@ def query_user_devices(self, destination, user_id, timeout): "keys": {...}, "signatures": { "": {...} - } - }, + } + }, "self_signing_key": { "user_id": "", "usage": [...],