From b43d9a920b434180b0ae12516b19d09011f37c59 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 18:54:03 +0100 Subject: [PATCH 1/6] Fix docstring on get_server_keys_json --- synapse/storage/keys.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py index 030cd1e5a34d..f24ab3eedda1 100644 --- a/synapse/storage/keys.py +++ b/synapse/storage/keys.py @@ -188,8 +188,8 @@ def get_server_keys_json(self, server_keys): Args: server_keys (list): List of (server_name, key_id, source) triplets. Returns: - Dict mapping (server_name, key_id, source) triplets to dicts with - "ts_valid_until_ms" and "key_json" keys. + Deferred[dict[Tuple[str, str, str|None], list[dict]]]: + Dict mapping (server_name, key_id, source) triplets to lists of dicts """ def _get_server_keys_json_txn(txn): From 30805237fa90d399164d08577f782b7bc7631640 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 19:12:54 +0100 Subject: [PATCH 2/6] add a test for get_keys_from_server --- tests/crypto/test_keyring.py | 63 ++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index c30a1a69e719..1c70cc3f8d31 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -23,6 +23,7 @@ from synapse.api.errors import SynapseError from synapse.crypto import keyring +from synapse.crypto.keyring import KeyLookupError from synapse.util import logcontext from synapse.util.logcontext import LoggingContext @@ -202,6 +203,68 @@ def test_verify_json_for_server(self): self.assertFalse(d.called) self.get_success(d) + def test_get_keys_from_server(self): + # arbitrarily advance the clock a bit + self.reactor.advance(100) + + SERVER_NAME = "server2" + kr = keyring.Keyring(self.hs) + testkey = signedjson.key.generate_signing_key("ver1") + testverifykey = signedjson.key.get_verify_key(testkey) + testverifykey_id = "ed25519:ver1" + VALID_UNTIL_TS = 1000 + + # valid response + response = { + "server_name": SERVER_NAME, + "old_verify_keys": {}, + "valid_until_ts": VALID_UNTIL_TS, + "verify_keys": { + testverifykey_id: { + "key": signedjson.key.encode_verify_key_base64(testverifykey) + } + }, + } + signedjson.sign.sign_json(response, SERVER_NAME, testkey) + + def get_json(destination, path, **kwargs): + self.assertEqual(destination, SERVER_NAME) + self.assertEqual(path, "/_matrix/key/v2/server/key1") + return response + + self.http_client.get_json.side_effect = get_json + + server_name_and_key_ids = [(SERVER_NAME, ("key1",))] + keys = self.get_success(kr.get_keys_from_server(server_name_and_key_ids)) + k = keys[SERVER_NAME][testverifykey_id] + self.assertEqual(k, testverifykey) + self.assertEqual(k.alg, "ed25519") + self.assertEqual(k.version, "ver1") + + # check that the perspectives store is correctly updated + lookup_triplet = (SERVER_NAME, testverifykey_id, None) + key_json = self.get_success( + self.hs.get_datastore().get_server_keys_json([lookup_triplet]) + ) + res = key_json[lookup_triplet] + self.assertEqual(len(res), 1) + res = res[0] + self.assertEqual(res["key_id"], testverifykey_id) + self.assertEqual(res["from_server"], SERVER_NAME) + self.assertEqual(res["ts_added_ms"], self.reactor.seconds() * 1000) + self.assertEqual(res["ts_valid_until_ms"], VALID_UNTIL_TS) + + # we expect it to be encoded as canonical json *before* it hits the db + self.assertEqual( + bytes(res["key_json"]), canonicaljson.encode_canonical_json(response) + ) + + # change the server name: it should cause a rejection + response["server_name"] = "OTHER_SERVER" + self.get_failure( + kr.get_keys_from_server(server_name_and_key_ids), KeyLookupError + ) + @defer.inlineCallbacks def run_in_context(f, *args, **kwargs): From b2d574f1266e726b24458b6d46168f224e944478 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 19:12:54 +0100 Subject: [PATCH 3/6] test for get_keys_from_perspectives --- tests/crypto/test_keyring.py | 67 ++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index 1c70cc3f8d31..b224fdb23a2b 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -16,6 +16,7 @@ from mock import Mock +import canonicaljson import signedjson.key import signedjson.sign @@ -49,6 +50,9 @@ def get_signed_key(self, server_name, verify_key): key_id: {"key": signedjson.key.encode_verify_key_base64(verify_key)} }, } + return self.get_signed_response(res) + + def get_signed_response(self, res): signedjson.sign.sign_json(res, self.server_name, self.key) return res @@ -265,6 +269,69 @@ def get_json(destination, path, **kwargs): kr.get_keys_from_server(server_name_and_key_ids), KeyLookupError ) + def test_get_keys_from_perspectives(self): + # arbitrarily advance the clock a bit + self.reactor.advance(100) + + SERVER_NAME = "server2" + kr = keyring.Keyring(self.hs) + testkey = signedjson.key.generate_signing_key("ver1") + testverifykey = signedjson.key.get_verify_key(testkey) + testverifykey_id = "ed25519:ver1" + VALID_UNTIL_TS = 200 * 1000 + + # valid response + response = { + "server_name": SERVER_NAME, + "old_verify_keys": {}, + "valid_until_ts": VALID_UNTIL_TS, + "verify_keys": { + testverifykey_id: { + "key": signedjson.key.encode_verify_key_base64(testverifykey) + } + }, + } + + persp_resp = { + "server_keys": [self.mock_perspective_server.get_signed_response(response)] + } + + 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 key + q = data["server_keys"] + self.assertEqual(list(q[SERVER_NAME].keys()), ["key1"]) + return persp_resp + + self.http_client.post_json.side_effect = post_json + + server_name_and_key_ids = [(SERVER_NAME, ("key1",))] + keys = self.get_success(kr.get_keys_from_perspectives(server_name_and_key_ids)) + self.assertIn(SERVER_NAME, keys) + k = keys[SERVER_NAME][testverifykey_id] + self.assertEqual(k, testverifykey) + self.assertEqual(k.alg, "ed25519") + self.assertEqual(k.version, "ver1") + + # check that the perspectives store is correctly updated + lookup_triplet = (SERVER_NAME, testverifykey_id, None) + key_json = self.get_success( + self.hs.get_datastore().get_server_keys_json([lookup_triplet]) + ) + res = key_json[lookup_triplet] + self.assertEqual(len(res), 1) + res = res[0] + self.assertEqual(res["key_id"], testverifykey_id) + self.assertEqual(res["ts_added_ms"], self.reactor.seconds() * 1000) + self.assertEqual(res["ts_valid_until_ms"], VALID_UNTIL_TS) + + self.assertEqual( + bytes(res["key_json"]), + canonicaljson.encode_canonical_json(persp_resp["server_keys"][0]), + ) + @defer.inlineCallbacks def run_in_context(f, *args, **kwargs): From ef27d434d109732bd9624738c3278863412139ad Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 19:12:54 +0100 Subject: [PATCH 4/6] Clean up Keyring.process_v2_response Make this just return the key dict, rather than a single-entry dict mapping the server name to the key dict. It's easy for the caller to get the server name from from the response object anyway. --- synapse/crypto/keyring.py | 69 ++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 0207cd989a2d..98b8b15680c1 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -20,6 +20,7 @@ from six import raise_from from six.moves import urllib +import nacl.signing from signedjson.key import ( decode_verify_key_bytes, encode_verify_key_base64, @@ -496,9 +497,9 @@ def get_server_verify_key_v2_indirect(self, server_names_and_key_ids, processed_response = yield self.process_v2_response( perspective_name, response, only_from_server=False ) + server_name = response["server_name"] - for server_name, response_keys in processed_response.items(): - keys.setdefault(server_name, {}).update(response_keys) + keys.setdefault(server_name, {}).update(processed_response) yield logcontext.make_deferred_yieldable(defer.gatherResults( [ @@ -517,7 +518,7 @@ def get_server_verify_key_v2_indirect(self, server_names_and_key_ids, @defer.inlineCallbacks def get_server_verify_key_v2_direct(self, server_name, key_ids): - keys = {} + keys = {} # type: dict[str, nacl.signing.VerifyKey] for requested_key_id in key_ids: if requested_key_id in keys: @@ -550,24 +551,49 @@ def get_server_verify_key_v2_direct(self, server_name, key_ids): keys.update(response_keys) - yield logcontext.make_deferred_yieldable(defer.gatherResults( - [ - run_in_background( - self.store_keys, - server_name=key_server_name, - from_server=server_name, - verify_keys=verify_keys, - ) - for key_server_name, verify_keys in keys.items() - ], - consumeErrors=True - ).addErrback(unwrapFirstError)) - - defer.returnValue(keys) + yield self.store_keys( + server_name=server_name, + from_server=server_name, + verify_keys=keys, + ) + defer.returnValue({server_name: keys}) @defer.inlineCallbacks - def process_v2_response(self, from_server, response_json, - requested_ids=[], only_from_server=True): + def process_v2_response( + self, from_server, response_json, requested_ids=[], only_from_server=True + ): + """Parse a 'Server Keys' structure from the result of a /key request + + This is used to parse either the entirety of the response from + GET /_matrix/key/v2/server, or a single entry from the list returned by + POST /_matrix/key/v2/query. + + Checks that each signature in the response that claims to come from the origin + server is valid. (Does not check that there actually is such a signature, for + some reason.) + + Stores the json in server_keys_json so that it can be used for future responses + to /_matrix/key/v2/query. + + Args: + from_server (str): the name of the server producing this result: either + the origin server for a /_matrix/key/v2/server request, or the notary + for a /_matrix/key/v2/query. + + response_json (dict): the json-decoded Server Keys response object + + requested_ids (iterable[str]): a list of the key IDs that were requested. + We will store the json for these key ids as well as any that are + actually in the response + + only_from_server (bool): if True, we will check that the server_name in the + the response (ie, the server which originated the key) matches + from_server. + + Returns: + Deferred[dict[str, nacl.signing.VerifyKey]]: + map from key_id to key object + """ time_now_ms = self.clock.time_msec() response_keys = {} verify_keys = {} @@ -589,7 +615,6 @@ def process_v2_response(self, from_server, response_json, verify_key.time_added = time_now_ms old_verify_keys[key_id] = verify_key - results = {} server_name = response_json["server_name"] if only_from_server: if server_name != from_server: @@ -643,9 +668,7 @@ def process_v2_response(self, from_server, response_json, consumeErrors=True, ).addErrback(unwrapFirstError)) - results[server_name] = response_keys - - defer.returnValue(results) + defer.returnValue(response_keys) def store_keys(self, server_name, from_server, verify_keys): """Store a collection of verify keys for a given server From 6ae9361510eb033d6a4dd9172c5e75bb4d0039dd Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 19:12:54 +0100 Subject: [PATCH 5/6] Hoist server_name check out of process_v2_response It's easier to check it in the caller than to complicate the interface with an extra param. --- synapse/crypto/keyring.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 98b8b15680c1..54af60d71169 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -495,7 +495,7 @@ def get_server_verify_key_v2_indirect(self, server_names_and_key_ids, ) processed_response = yield self.process_v2_response( - perspective_name, response, only_from_server=False + perspective_name, response ) server_name = response["server_name"] @@ -543,6 +543,11 @@ def get_server_verify_key_v2_direct(self, server_name, key_ids): or server_name not in response[u"signatures"]): raise KeyLookupError("Key response not signed by remote server") + if response["server_name"] != server_name: + raise KeyLookupError("Expected a response for server %r not %r" % ( + server_name, response["server_name"] + )) + response_keys = yield self.process_v2_response( from_server=server_name, requested_ids=[requested_key_id], @@ -560,7 +565,7 @@ def get_server_verify_key_v2_direct(self, server_name, key_ids): @defer.inlineCallbacks def process_v2_response( - self, from_server, response_json, requested_ids=[], only_from_server=True + self, from_server, response_json, requested_ids=[], ): """Parse a 'Server Keys' structure from the result of a /key request @@ -586,10 +591,6 @@ def process_v2_response( We will store the json for these key ids as well as any that are actually in the response - only_from_server (bool): if True, we will check that the server_name in the - the response (ie, the server which originated the key) matches - from_server. - Returns: Deferred[dict[str, nacl.signing.VerifyKey]]: map from key_id to key object @@ -616,13 +617,6 @@ def process_v2_response( old_verify_keys[key_id] = verify_key server_name = response_json["server_name"] - if only_from_server: - if server_name != from_server: - raise KeyLookupError( - "Expected a response for server %r not %r" % ( - from_server, server_name - ) - ) for key_id in response_json["signatures"].get(server_name, {}): if key_id not in response_json["verify_keys"]: raise KeyLookupError( From b78aac5582da10131d6a2a69c25dcf4a36c528d8 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 19:12:54 +0100 Subject: [PATCH 6/6] changelog --- changelog.d/5001.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5001.misc diff --git a/changelog.d/5001.misc b/changelog.d/5001.misc new file mode 100644 index 000000000000..bf590a016d05 --- /dev/null +++ b/changelog.d/5001.misc @@ -0,0 +1 @@ +Clean up some code in the server-key Keyring. \ No newline at end of file