From a6f7f845702c56dd7d1e7dfabd3f50a71f245cc1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 19 Nov 2021 10:55:09 +0000 Subject: [PATCH] Fix verification of objects signed with old local keys (#11379) 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 | 69 +++++++++++++++++++++--------------- tests/crypto/test_keyring.py | 56 +++++++++++++++++++++++++++-- 3 files changed, 95 insertions(+), 31 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..4cda439ad927 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 + found_keys: Dict[str, FetchKeyResult] = {} - # 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 - ) + # 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`