From e052bc19079c929f5aebdf6edf07376baba49fd7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 17 Nov 2021 20:40:37 +0000 Subject: [PATCH 1/2] Fix verification of objects signed with old local keys Fixes a bug introduced in #11129: objects signed by the local server, but with keys other than the current one, could not be successfully verified. We need to check the key id in the signature, and track down the right key. --- changelog.d/11379.bugfix | 1 + synapse/crypto/keyring.py | 71 +++++++++++++++++++++--------------- tests/crypto/test_keyring.py | 56 ++++++++++++++++++++++++++-- 3 files changed, 96 insertions(+), 32 deletions(-) create mode 100644 changelog.d/11379.bugfix diff --git a/changelog.d/11379.bugfix b/changelog.d/11379.bugfix new file mode 100644 index 000000000000..a49d4eb7766f --- /dev/null +++ b/changelog.d/11379.bugfix @@ -0,0 +1 @@ +Fix an issue introduced in v1.47.0 which prevented servers re-joining rooms they had previously left, if their signing keys were replaced. diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index f641ab7ef510..a16188a01946 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -1,5 +1,4 @@ -# Copyright 2014-2016 OpenMarket Ltd -# Copyright 2017, 2018 New Vector Ltd +# Copyright 2014-2021 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. @@ -120,16 +119,6 @@ def from_event( key_ids=key_ids, ) - def to_fetch_key_request(self) -> "_FetchKeyRequest": - """Create a key fetch request for all keys needed to satisfy the - verification request. - """ - return _FetchKeyRequest( - server_name=self.server_name, - minimum_valid_until_ts=self.minimum_valid_until_ts, - key_ids=self.key_ids, - ) - class KeyLookupError(ValueError): pass @@ -179,8 +168,22 @@ def __init__( clock=hs.get_clock(), process_batch_callback=self._inner_fetch_key_requests, ) - self.verify_key = get_verify_key(hs.signing_key) - self.hostname = hs.hostname + + self._hostname = hs.hostname + + # build a FetchKeyResult for each of our own keys, to shortcircuit the + # fetcher. + self._local_verify_keys: Dict[str, FetchKeyResult] = {} + for key_id, key in hs.config.key.old_signing_keys.items(): + self._local_verify_keys[key_id] = FetchKeyResult( + verify_key=key, valid_until_ts=key.expired_ts + ) + + vk = get_verify_key(hs.signing_key) + self._local_verify_keys[f"{vk.alg}:{vk.version}"] = FetchKeyResult( + verify_key=vk, + valid_until_ts=2 ** 63, # fake future timestamp + ) async def verify_json_for_server( self, @@ -267,22 +270,32 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: Codes.UNAUTHORIZED, ) - # If we are the originating server don't fetch verify key for self over federation - if verify_request.server_name == self.hostname: - await self._process_json(self.verify_key, verify_request) - return - - # Add the keys we need to verify to the queue for retrieval. We queue - # up requests for the same server so we don't end up with many in flight - # requests for the same keys. - key_request = verify_request.to_fetch_key_request() - found_keys_by_server = await self._server_queue.add_to_queue( - key_request, key=verify_request.server_name - ) + found_keys: Dict[str:FetchKeyResult] = {} + + # If we are the originating server, short-circuit the key-fetch for any keys + # we already have + if verify_request.server_name == self._hostname: + for key_id in verify_request.key_ids: + if key_id in self._local_verify_keys: + found_keys[key_id] = self._local_verify_keys[key_id] + + key_ids_to_find = set(verify_request.key_ids) - found_keys.keys() + if key_ids_to_find: + # Add the keys we need to verify to the queue for retrieval. We queue + # up requests for the same server so we don't end up with many in flight + # requests for the same keys. + key_request = _FetchKeyRequest( + server_name=verify_request.server_name, + minimum_valid_until_ts=verify_request.minimum_valid_until_ts, + key_ids=list(key_ids_to_find), + ) + found_keys_by_server = await self._server_queue.add_to_queue( + key_request, key=verify_request.server_name + ) - # Since we batch up requests the returned set of keys may contain keys - # from other servers, so we pull out only the ones we care about.s - found_keys = found_keys_by_server.get(verify_request.server_name, {}) + # Since we batch up requests the returned set of keys may contain keys + # from other servers, so we pull out only the ones we care about. + found_keys.update(found_keys_by_server.get(verify_request.server_name, {})) # Verify each signature we got valid keys for, raising if we can't # verify any of them. diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index cbecc1c20f3d..4d1e154578c1 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -1,4 +1,4 @@ -# Copyright 2017 New Vector Ltd +# Copyright 2017-2021 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. @@ -40,7 +40,7 @@ from tests import unittest from tests.test_utils import make_awaitable -from tests.unittest import logcontext_clean +from tests.unittest import logcontext_clean, override_config class MockPerspectiveServer: @@ -197,7 +197,7 @@ def test_verify_json_for_server(self): # self.assertFalse(d.called) self.get_success(d) - def test_verify_for_server_locally(self): + def test_verify_for_local_server(self): """Ensure that locally signed JSON can be verified without fetching keys over federation """ @@ -209,6 +209,56 @@ def test_verify_for_server_locally(self): d = kr.verify_json_for_server(self.hs.hostname, json1, 0) self.get_success(d) + OLD_KEY = signedjson.key.generate_signing_key("old") + + @override_config( + { + "old_signing_keys": { + f"{OLD_KEY.alg}:{OLD_KEY.version}": { + "key": encode_verify_key_base64(OLD_KEY.verify_key), + "expired_ts": 1000, + } + } + } + ) + def test_verify_for_local_server_old_key(self): + """Can also use keys in old_signing_keys for verification""" + json1 = {} + signedjson.sign.sign_json(json1, self.hs.hostname, self.OLD_KEY) + + kr = keyring.Keyring(self.hs) + d = kr.verify_json_for_server(self.hs.hostname, json1, 0) + self.get_success(d) + + def test_verify_for_local_server_unknown_key(self): + """Local keys that we no longer have should be fetched via the fetcher""" + + # the key we'll sign things with (nb, not known to the Keyring) + key2 = signedjson.key.generate_signing_key("2") + + # set up a mock fetcher which will return the key + async def get_keys( + server_name: str, key_ids: List[str], minimum_valid_until_ts: int + ) -> Dict[str, FetchKeyResult]: + self.assertEqual(server_name, self.hs.hostname) + self.assertEqual(key_ids, [get_key_id(key2)]) + + return {get_key_id(key2): FetchKeyResult(get_verify_key(key2), 1200)} + + mock_fetcher = Mock() + mock_fetcher.get_keys = Mock(side_effect=get_keys) + kr = keyring.Keyring( + self.hs, key_fetchers=(StoreKeyFetcher(self.hs), mock_fetcher) + ) + + # sign the json + json1 = {} + signedjson.sign.sign_json(json1, self.hs.hostname, key2) + + # ... and check we can verify it. + d = kr.verify_json_for_server(self.hs.hostname, json1, 0) + self.get_success(d) + def test_verify_json_for_server_with_null_valid_until_ms(self): """Tests that we correctly handle key requests for keys we've stored with a null `ts_valid_until_ms` From f7cfafda4725df8da257e97f88d2c473a59cbb7b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 17 Nov 2021 21:12:59 +0000 Subject: [PATCH 2/2] fix type annotation --- 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 a16188a01946..4cda439ad927 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -270,7 +270,7 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: Codes.UNAUTHORIZED, ) - found_keys: Dict[str:FetchKeyResult] = {} + found_keys: Dict[str, FetchKeyResult] = {} # If we are the originating server, short-circuit the key-fetch for any keys # we already have