From 89e3fbfa9f4291bca83dd40ef7e7334110b417f9 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 19 Oct 2021 15:00:09 -0700 Subject: [PATCH 01/10] add tests for fetching key locally --- tests/crypto/test_keyring.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index 745c295d3ba1..ec6f8726b4ed 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -17,10 +17,12 @@ import attr import canonicaljson +import nacl import signedjson.key import signedjson.sign from nacl.signing import SigningKey from signedjson.key import encode_verify_key_base64, get_verify_key +from unpaddedbase64 import decode_base64 from twisted.internet.defer import Deferred, ensureDeferred @@ -197,6 +199,28 @@ def test_verify_json_for_server(self): # self.assertFalse(d.called) self.get_success(d) + def test_verify_json_locally(self): + kr = keyring.Keyring(self.hs) + json1 = {} + signedjson.sign.sign_json(json1, self.hs.hostname, self.hs.signing_key) + + # Test that verify_json_locally fails on an unsigned object + with self.assertRaises(SynapseError): + kr.verify_json_locally(self.hs.hostname, {}) + + # Test that verify_json_locally succeeds on a object signed by ourselves + kr.verify_json_locally(self.hs.hostname, json1) + + # Test that verify_json_locally fails on object not signed by origin + json2 = {"fake": "json"} + fake_sign_seed = decode_base64("YJDBA9Xnr2sVqXD9Vj7XVUnmFZcZrlw8Md7kMW+3XA1") + other_key = nacl.signing.SigningKey(fake_sign_seed) + other_key.alg = "ed25519" + other_key.version = "a_lPym" + signedjson.sign.sign_json(json2, "other_server", other_key) + with self.assertRaises(SynapseError): + kr.verify_json_locally("other_server", json2) + 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 7673fd83ff3172917c0a2aeff38229721460d640 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 19 Oct 2021 15:01:18 -0700 Subject: [PATCH 02/10] add logic to check if origin server is same as host and fetch verify key locally rather than over federation --- synapse/crypto/keyring.py | 47 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index e1e13a2412b4..9992d9bbcee4 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -22,6 +22,7 @@ from signedjson.key import ( decode_verify_key_bytes, encode_verify_key_base64, + get_verify_key, is_signing_algorithm_supported, ) from signedjson.sign import ( @@ -177,6 +178,48 @@ def __init__( clock=hs.get_clock(), process_batch_callback=self._inner_fetch_key_requests, ) + self.signing_key = hs.signing_key + self.hostname = hs.hostname + + def verify_json_locally(self, server_name: str, json_object: JsonDict): + verify_key = get_verify_key(self.signing_key) + verified = False + + try: + verify_signed_json( + json_object, + server_name, + verify_key, + ) + verified = True + + except SignatureVerifyException as e: + logger.debug( + "Error verifying signature for %s:%s:%s with key %s: %s", + 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" + % ( + server_name, + verify_key.alg, + verify_key.version, + str(e), + ), + Codes.UNAUTHORIZED, + ) + + if not verified: + raise SynapseError( + 401, + "Unable to verify request", + Codes.UNAUTHORIZED, + ) async def verify_json_for_server( self, @@ -196,6 +239,10 @@ async def verify_json_for_server( validity_time: timestamp at which we require the signing key to be valid. (0 implies we don't care) """ + # if we are the originating server don't fetch verify key for self over federation + if server_name == self.hostname: + return self.verify_json_locally(server_name, json_object) + request = VerifyJsonRequest.from_json_object( server_name, json_object, From 955bae7f6c64fd81bbba277ecfbe838f239b8b09 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 19 Oct 2021 15:09:42 -0700 Subject: [PATCH 03/10] add changelog --- changelog.d/11129.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11129.misc diff --git a/changelog.d/11129.misc b/changelog.d/11129.misc new file mode 100644 index 000000000000..396115feab3c --- /dev/null +++ b/changelog.d/11129.misc @@ -0,0 +1 @@ +Fetch verify key locally rather than trying to do so over federation if host and origin are the same. \ No newline at end of file From 44f4a94066a5aed22e926358b37adf172db8f561 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 20 Oct 2021 12:59:08 -0700 Subject: [PATCH 04/10] slight refactor, add docstring, change changelog entry --- changelog.d/11129.bugfix | 2 ++ changelog.d/11129.misc | 1 - synapse/crypto/keyring.py | 36 +++++++++++++++++------------------- 3 files changed, 19 insertions(+), 20 deletions(-) create mode 100644 changelog.d/11129.bugfix delete mode 100644 changelog.d/11129.misc diff --git a/changelog.d/11129.bugfix b/changelog.d/11129.bugfix new file mode 100644 index 000000000000..2adef79ad4ab --- /dev/null +++ b/changelog.d/11129.bugfix @@ -0,0 +1,2 @@ +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/changelog.d/11129.misc b/changelog.d/11129.misc deleted file mode 100644 index 396115feab3c..000000000000 --- a/changelog.d/11129.misc +++ /dev/null @@ -1 +0,0 @@ -Fetch verify key locally rather than trying to do so over federation if host and origin are the same. \ No newline at end of file diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 9992d9bbcee4..ac764b4d76fe 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -178,28 +178,33 @@ def __init__( clock=hs.get_clock(), process_batch_callback=self._inner_fetch_key_requests, ) - self.signing_key = hs.signing_key + self.verify_key = get_verify_key(hs.signing_key) self.hostname = hs.hostname - def verify_json_locally(self, server_name: str, json_object: JsonDict): - verify_key = get_verify_key(self.signing_key) - verified = False + def verify_json_locally(self, server_name: str, json_object: JsonDict) -> None: + """Verify that a JSON object has been signed by this homeserver + + Completes if the the object was correctly signed, otherwise raises. + Args: + server_name: name of the server which must have signed this object + + json_object: object to be checked + """ try: verify_signed_json( json_object, server_name, - verify_key, + self.verify_key, ) - verified = True - except SignatureVerifyException as e: + except Exception as e: logger.debug( "Error verifying signature for %s:%s:%s with key %s: %s", server_name, - verify_key.alg, - verify_key.version, - encode_verify_key_base64(verify_key), + self.verify_key.alg, + self.verify_key.version, + encode_verify_key_base64(self.verify_key), str(e), ) raise SynapseError( @@ -207,20 +212,13 @@ def verify_json_locally(self, server_name: str, json_object: JsonDict): "Invalid signature for server %s with key %s:%s: %s" % ( server_name, - verify_key.alg, - verify_key.version, + self.verify_key.alg, + self.verify_key.version, str(e), ), Codes.UNAUTHORIZED, ) - if not verified: - raise SynapseError( - 401, - "Unable to verify request", - Codes.UNAUTHORIZED, - ) - async def verify_json_for_server( self, server_name: str, From 1f45f4c5c6134103091ef99f0198025cee40fe46 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 25 Oct 2021 14:30:35 -0700 Subject: [PATCH 05/10] Make changelog entry one line --- changelog.d/11129.bugfix | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/changelog.d/11129.bugfix b/changelog.d/11129.bugfix index 2adef79ad4ab..5e9aa538ec88 100644 --- a/changelog.d/11129.bugfix +++ b/changelog.d/11129.bugfix @@ -1,2 +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 +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 From c1a2c73576797a7d8f41e4034d26f178d15d22c4 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 25 Oct 2021 14:33:46 -0700 Subject: [PATCH 06/10] remove verify_json_locally and push locality check to process_request, add function process_request_locally --- synapse/crypto/keyring.py | 76 +++++++++++++++++------------------- tests/crypto/test_keyring.py | 23 ++--------- 2 files changed, 40 insertions(+), 59 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index ac764b4d76fe..3478cd2ce0f2 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -181,44 +181,6 @@ def __init__( self.verify_key = get_verify_key(hs.signing_key) self.hostname = hs.hostname - def verify_json_locally(self, server_name: str, json_object: JsonDict) -> None: - """Verify that a JSON object has been signed by this homeserver - - Completes if the the object was correctly signed, otherwise raises. - - Args: - server_name: name of the server which must have signed this object - - json_object: object to be checked - """ - try: - verify_signed_json( - json_object, - server_name, - self.verify_key, - ) - - except Exception as e: - logger.debug( - "Error verifying signature for %s:%s:%s with key %s: %s", - server_name, - self.verify_key.alg, - self.verify_key.version, - encode_verify_key_base64(self.verify_key), - str(e), - ) - raise SynapseError( - 401, - "Invalid signature for server %s with key %s:%s: %s" - % ( - server_name, - self.verify_key.alg, - self.verify_key.version, - str(e), - ), - Codes.UNAUTHORIZED, - ) - async def verify_json_for_server( self, server_name: str, @@ -238,8 +200,8 @@ async def verify_json_for_server( be valid. (0 implies we don't care) """ # if we are the originating server don't fetch verify key for self over federation - if server_name == self.hostname: - return self.verify_json_locally(server_name, json_object) + # if server_name == self.hostname: + # return self.verify_json_locally(server_name, json_object) request = VerifyJsonRequest.from_json_object( server_name, @@ -307,6 +269,10 @@ 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: + return await self.process_request_locally(verify_request) + # 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. @@ -367,6 +333,36 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: Codes.UNAUTHORIZED, ) + async def process_request_locally(self, verify_request: VerifyJsonRequest) -> None: + verify_key = self.verify_key + json_object = verify_request.get_json_object() + try: + verify_signed_json( + 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 ec6f8726b4ed..faeb3feb2521 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -17,12 +17,10 @@ import attr import canonicaljson -import nacl import signedjson.key import signedjson.sign from nacl.signing import SigningKey from signedjson.key import encode_verify_key_base64, get_verify_key -from unpaddedbase64 import decode_base64 from twisted.internet.defer import Deferred, ensureDeferred @@ -199,27 +197,14 @@ def test_verify_json_for_server(self): # self.assertFalse(d.called) self.get_success(d) - def test_verify_json_locally(self): + def test_verify_for_server_locally(self): kr = keyring.Keyring(self.hs) json1 = {} signedjson.sign.sign_json(json1, self.hs.hostname, self.hs.signing_key) - # Test that verify_json_locally fails on an unsigned object - with self.assertRaises(SynapseError): - kr.verify_json_locally(self.hs.hostname, {}) - - # Test that verify_json_locally succeeds on a object signed by ourselves - kr.verify_json_locally(self.hs.hostname, json1) - - # Test that verify_json_locally fails on object not signed by origin - json2 = {"fake": "json"} - fake_sign_seed = decode_base64("YJDBA9Xnr2sVqXD9Vj7XVUnmFZcZrlw8Md7kMW+3XA1") - other_key = nacl.signing.SigningKey(fake_sign_seed) - other_key.alg = "ed25519" - other_key.version = "a_lPym" - signedjson.sign.sign_json(json2, "other_server", other_key) - with self.assertRaises(SynapseError): - kr.verify_json_locally("other_server", json2) + # 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 From d1c4bc40433730953d0d0e8204026032cb1749a3 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 25 Oct 2021 15:10:06 -0700 Subject: [PATCH 07/10] remove leftover code reference --- synapse/crypto/keyring.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 3478cd2ce0f2..26a818df9459 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -199,9 +199,6 @@ async def verify_json_for_server( validity_time: timestamp at which we require the signing key to be valid. (0 implies we don't care) """ - # if we are the originating server don't fetch verify key for self over federation - # if server_name == self.hostname: - # return self.verify_json_locally(server_name, json_object) request = VerifyJsonRequest.from_json_object( server_name, From 939c07b6a0d8b6a2dd6523b7e6e6bbaad7d9f3f6 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 27 Oct 2021 10:46:41 -0700 Subject: [PATCH 08/10] refactor to add common call to 'verify_json and associated handling code --- synapse/crypto/keyring.py | 39 +++++---------------------------------- 1 file changed, 5 insertions(+), 34 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 26a818df9459..de0e0a2f2b88 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -268,7 +268,7 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: # If we are the originating server don't fetch verify key for self over federation if verify_request.server_name == self.hostname: - return await self.process_request_locally(verify_request) + return await self.process_json(self.verify_key, verify_request) # 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 @@ -293,35 +293,8 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: 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( @@ -330,12 +303,10 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: Codes.UNAUTHORIZED, ) - async def process_request_locally(self, verify_request: VerifyJsonRequest) -> None: - verify_key = self.verify_key - json_object = verify_request.get_json_object() + async def process_json(self, verify_key, verify_request): try: verify_signed_json( - json_object, + verify_request.get_json_object(), verify_request.server_name, verify_key, ) From bc75c2b1166987b3cdbe7e8fc95a86c9e20f7b37 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 27 Oct 2021 11:04:35 -0700 Subject: [PATCH 09/10] add type hint to process_json --- synapse/crypto/keyring.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index de0e0a2f2b88..43313b87c031 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -31,6 +31,7 @@ signature_ids, verify_signed_json, ) +from signedjson.types import VerifyKey from unpaddedbase64 import decode_base64 from twisted.internet import defer @@ -303,7 +304,9 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: Codes.UNAUTHORIZED, ) - async def process_json(self, verify_key, verify_request): + async def process_json( + self, verify_key: VerifyKey, verify_request: VerifyJsonRequest + ) -> None: try: verify_signed_json( verify_request.get_json_object(), From 2834a719ca101a03218b836936dbcec0a522bc1f Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Thu, 28 Oct 2021 09:35:25 -0700 Subject: [PATCH 10/10] add some docstrings + very slight refactor --- synapse/crypto/keyring.py | 10 +++++++--- tests/crypto/test_keyring.py | 3 +++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 43313b87c031..2ce5c471d6c3 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -269,7 +269,8 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: # If we are the originating server don't fetch verify key for self over federation if verify_request.server_name == self.hostname: - return await self.process_json(self.verify_key, verify_request) + 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 @@ -294,7 +295,7 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: if key_result.valid_until_ts < verify_request.minimum_valid_until_ts: continue - await self.process_json(key_result.verify_key, verify_request) + await self._process_json(key_result.verify_key, verify_request) verified = True if not verified: @@ -304,9 +305,12 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: Codes.UNAUTHORIZED, ) - async def process_json( + 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(), diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index faeb3feb2521..cbecc1c20f3d 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -198,6 +198,9 @@ def test_verify_json_for_server(self): 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)