From 33aa40d54c355294de2e6ee7e2ce61551e6d164d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 26 Nov 2021 17:29:12 +0000 Subject: [PATCH 1/4] Fix perspectives requests for multiple keys for the same server If we tried to request multiple keys for the same server, we would end up dropping some of those requests. --- changelog.d/11440.bugfix | 1 + synapse/crypto/keyring.py | 30 ++++++++++------ tests/crypto/test_keyring.py | 68 ++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 11 deletions(-) create mode 100644 changelog.d/11440.bugfix diff --git a/changelog.d/11440.bugfix b/changelog.d/11440.bugfix new file mode 100644 index 000000000000..02ce2e428fe8 --- /dev/null +++ b/changelog.d/11440.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.36 which could cause problems fetching event-signing keys from trusted key servers. diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 4cda439ad927..4efdc8ef12fa 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -667,21 +667,25 @@ async def get_server_verify_key_v2_indirect( perspective_name, ) + request = {} + for queue_value in keys_to_fetch: + # there may be multiple requests for each server, so we have to merge + # them intelligently. + request_for_server = { + key_id: { + "minimum_valid_until_ts": queue_value.minimum_valid_until_ts, + } + for key_id in queue_value.key_ids + } + request.setdefault(queue_value.server_name, {}).update(request_for_server) + + logger.debug("Request to notary server %s: %s", perspective_name, request) + try: query_response = await self.client.post_json( destination=perspective_name, path="/_matrix/key/v2/query", - data={ - "server_keys": { - queue_value.server_name: { - key_id: { - "minimum_valid_until_ts": queue_value.minimum_valid_until_ts, - } - for key_id in queue_value.key_ids - } - for queue_value in keys_to_fetch - } - }, + data={"server_keys": request}, ) except (NotRetryingDestination, RequestSendFailed) as e: # these both have str() representations which we can't really improve upon @@ -689,6 +693,10 @@ async def get_server_verify_key_v2_indirect( except HttpResponseException as e: raise KeyLookupError("Remote server returned an error: %s" % (e,)) + logger.debug( + "Response from notary server %s: %s", perspective_name, query_response + ) + keys: Dict[str, Dict[str, FetchKeyResult]] = {} added_keys: List[Tuple[str, str, FetchKeyResult]] = [] diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index 4d1e154578c1..25d246586175 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -22,6 +22,7 @@ from nacl.signing import SigningKey from signedjson.key import encode_verify_key_base64, get_verify_key +from twisted.internet import defer from twisted.internet.defer import Deferred, ensureDeferred from synapse.api.errors import SynapseError @@ -577,6 +578,73 @@ def test_get_keys_from_perspectives(self): bytes(res["key_json"]), canonicaljson.encode_canonical_json(response) ) + def test_get_multiple_keys_from_perspectives(self): + """Check that we can correctly request multiple keys for the same server""" + + fetcher = PerspectivesKeyFetcher(self.hs) + + SERVER_NAME = "server2" + + testkey1 = signedjson.key.generate_signing_key("ver1") + testverifykey1 = signedjson.key.get_verify_key(testkey1) + testverifykey1_id = "ed25519:ver1" + + testkey2 = signedjson.key.generate_signing_key("ver2") + testverifykey2 = signedjson.key.get_verify_key(testkey2) + testverifykey2_id = "ed25519:ver2" + + VALID_UNTIL_TS = 200 * 1000 + + response1 = self.build_perspectives_response( + SERVER_NAME, + testkey1, + VALID_UNTIL_TS, + ) + response2 = self.build_perspectives_response( + SERVER_NAME, + testkey2, + VALID_UNTIL_TS, + ) + + async def post_json(destination, path, data, **kwargs): + self.assertEqual(destination, self.mock_perspective_server.server_name) + self.assertEqual(path, "/_matrix/key/v2/query") + + # check that the request is for the expected keys + q = data["server_keys"] + + self.assertEqual( + list(q[SERVER_NAME].keys()), [testverifykey1_id, testverifykey2_id] + ) + return {"server_keys": [response1, response2]} + + self.http_client.post_json.side_effect = post_json + + # fire off two separate requests; they should get merged together into a + # single HTTP hit. + request1_d = defer.ensureDeferred( + fetcher.get_keys(SERVER_NAME, [testverifykey1_id], 0) + ) + request2_d = defer.ensureDeferred( + fetcher.get_keys(SERVER_NAME, [testverifykey2_id], 0) + ) + + keys1 = self.get_success(request1_d) + self.assertIn(testverifykey1_id, keys1) + k = keys1[testverifykey1_id] + self.assertEqual(k.valid_until_ts, VALID_UNTIL_TS) + self.assertEqual(k.verify_key, testverifykey1) + self.assertEqual(k.verify_key.alg, "ed25519") + self.assertEqual(k.verify_key.version, "ver1") + + keys2 = self.get_success(request2_d) + self.assertIn(testverifykey2_id, keys2) + k = keys2[testverifykey2_id] + self.assertEqual(k.valid_until_ts, VALID_UNTIL_TS) + self.assertEqual(k.verify_key, testverifykey2) + self.assertEqual(k.verify_key.alg, "ed25519") + self.assertEqual(k.verify_key.version, "ver2") + def test_get_perspectives_own_key(self): """Check that we can get the perspectives server's own keys From a633000c7989164d1b4c8c82dbcc18368ce811fe Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 28 Nov 2021 20:36:00 +0000 Subject: [PATCH 2/4] fix types --- synapse/crypto/keyring.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 4efdc8ef12fa..993b04099e28 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -667,7 +667,7 @@ async def get_server_verify_key_v2_indirect( perspective_name, ) - request = {} + request: JsonDict = {} for queue_value in keys_to_fetch: # there may be multiple requests for each server, so we have to merge # them intelligently. From 93d0911806f8d684415c1a687f974cfef780b0f7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 29 Nov 2021 12:25:34 +0000 Subject: [PATCH 3/4] add an assertion that only one request is sent --- tests/crypto/test_keyring.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index 25d246586175..8b171032dfd7 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -645,6 +645,9 @@ async def post_json(destination, path, data, **kwargs): self.assertEqual(k.verify_key.alg, "ed25519") self.assertEqual(k.verify_key.version, "ver2") + # finally, ensured that only one request was sent + self.assertEqual(self.http_client.post_json.call_count, 1) + def test_get_perspectives_own_key(self): """Check that we can get the perspectives server's own keys From 2b788a962844d690332bc90deeb073f2d427d819 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 29 Nov 2021 12:27:57 +0000 Subject: [PATCH 4/4] Update tests/crypto/test_keyring.py Co-authored-by: David Robertson --- tests/crypto/test_keyring.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index 8b171032dfd7..17a9fb63a176 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -645,7 +645,7 @@ async def post_json(destination, path, data, **kwargs): self.assertEqual(k.verify_key.alg, "ed25519") self.assertEqual(k.verify_key.version, "ver2") - # finally, ensured that only one request was sent + # finally, ensure that only one request was sent self.assertEqual(self.http_client.post_json.call_count, 1) def test_get_perspectives_own_key(self):