From e002faee01615c1976437af28f66544c5f2eed84 Mon Sep 17 00:00:00 2001 From: Shay Date: Thu, 28 Oct 2021 10:27:17 -0700 Subject: [PATCH] Fetch verify key locally rather than trying to do so over federation if origin and host are the same. (#11129) * add tests for fetching key locally * add logic to check if origin server is same as host and fetch verify key locally rather than over federation * add changelog * slight refactor, add docstring, change changelog entry * Make changelog entry one line * remove verify_json_locally and push locality check to process_request, add function process_request_locally * remove leftover code reference * refactor to add common call to 'verify_json and associated handling code * add type hint to process_json * add some docstrings + very slight refactor --- changelog.d/11129.bugfix | 1 + synapse/crypto/keyring.py | 74 ++++++++++++++++++++++-------------- tests/crypto/test_keyring.py | 12 ++++++ 3 files changed, 58 insertions(+), 29 deletions(-) create mode 100644 changelog.d/11129.bugfix diff --git a/changelog.d/11129.bugfix b/changelog.d/11129.bugfix new file mode 100644 index 0000000000..5e9aa538ec --- /dev/null +++ b/changelog.d/11129.bugfix @@ -0,0 +1 @@ +Fix long-standing bug where verification requests could fail in certain cases if whitelist was in place but did not include your own homeserver. \ No newline at end of file diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 8628e951c4..f641ab7ef5 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -22,6 +22,7 @@ import attr from signedjson.key import ( decode_verify_key_bytes, encode_verify_key_base64, + get_verify_key, is_signing_algorithm_supported, ) from signedjson.sign import ( @@ -30,6 +31,7 @@ from signedjson.sign import ( signature_ids, verify_signed_json, ) +from signedjson.types import VerifyKey from unpaddedbase64 import decode_base64 from twisted.internet import defer @@ -177,6 +179,8 @@ class Keyring: 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 async def verify_json_for_server( self, @@ -196,6 +200,7 @@ class Keyring: validity_time: timestamp at which we require the signing key to be valid. (0 implies we don't care) """ + request = VerifyJsonRequest.from_json_object( server_name, json_object, @@ -262,6 +267,11 @@ class Keyring: 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. @@ -285,35 +295,8 @@ class Keyring: if key_result.valid_until_ts < verify_request.minimum_valid_until_ts: continue - verify_key = key_result.verify_key - json_object = verify_request.get_json_object() - try: - verify_signed_json( - json_object, - verify_request.server_name, - verify_key, - ) - verified = True - except SignatureVerifyException as e: - logger.debug( - "Error verifying signature for %s:%s:%s with key %s: %s", - verify_request.server_name, - verify_key.alg, - verify_key.version, - encode_verify_key_base64(verify_key), - str(e), - ) - raise SynapseError( - 401, - "Invalid signature for server %s with key %s:%s: %s" - % ( - verify_request.server_name, - verify_key.alg, - verify_key.version, - str(e), - ), - Codes.UNAUTHORIZED, - ) + await self._process_json(key_result.verify_key, verify_request) + verified = True if not verified: raise SynapseError( @@ -322,6 +305,39 @@ class Keyring: Codes.UNAUTHORIZED, ) + async def _process_json( + self, verify_key: VerifyKey, verify_request: VerifyJsonRequest + ) -> None: + """Processes the `VerifyJsonRequest`. Raises if the signature can't be + verified. + """ + try: + verify_signed_json( + verify_request.get_json_object(), + verify_request.server_name, + verify_key, + ) + except SignatureVerifyException as e: + logger.debug( + "Error verifying signature for %s:%s:%s with key %s: %s", + verify_request.server_name, + verify_key.alg, + verify_key.version, + encode_verify_key_base64(verify_key), + str(e), + ) + raise SynapseError( + 401, + "Invalid signature for server %s with key %s:%s: %s" + % ( + verify_request.server_name, + verify_key.alg, + verify_key.version, + str(e), + ), + Codes.UNAUTHORIZED, + ) + async def _inner_fetch_key_requests( self, requests: List[_FetchKeyRequest] ) -> Dict[str, Dict[str, FetchKeyResult]]: diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index 745c295d3b..cbecc1c20f 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -197,6 +197,18 @@ class KeyringTestCase(unittest.HomeserverTestCase): # self.assertFalse(d.called) self.get_success(d) + def test_verify_for_server_locally(self): + """Ensure that locally signed JSON can be verified without fetching keys + over federation + """ + kr = keyring.Keyring(self.hs) + json1 = {} + signedjson.sign.sign_json(json1, self.hs.hostname, self.hs.signing_key) + + # Test that verify_json_for_server succeeds on a object signed by ourselves + 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`