From cacaaa8b4a767c0587427d6b52b56918b1a79bd7 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 9 Jan 2023 17:46:20 +0000 Subject: [PATCH 1/6] Keyring: add some comments and refactor code for readability --- synapse/crypto/keyring.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 69310d90351c..b9775f270105 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -154,8 +154,11 @@ def __init__( if key_fetchers is None: key_fetchers = ( + # Fetch keys from the database. StoreKeyFetcher(hs), + # Fetch keys from a configured Perspectives server. PerspectivesKeyFetcher(hs), + # Fetch keys from the origin server directly. ServerKeyFetcher(hs), ) self._key_fetchers = key_fetchers @@ -279,6 +282,11 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: key_ids_to_find = set(verify_request.key_ids) - found_keys.keys() if key_ids_to_find: + # We're still missing some keys. Consult each of our `KeyFetcher` instances + # (stored in `self._key_fetchers`) to try and find them. + # Key fetch attempts are queued via `self._server_queue` below, and carried + # out in `self._inner_fetch_key_requests`. + # 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. @@ -420,26 +428,22 @@ async def _inner_fetch_key_request( if not key: continue - # If we already have a result for the given key ID we keep the + # If we already have a result for the given key ID, we keep the # one with the highest `valid_until_ts`. existing_key = found_keys.get(key_id) - if existing_key: - if key.valid_until_ts <= existing_key.valid_until_ts: - continue + if existing_key and existing_key.valid_until_ts > key.valid_until_ts: + continue - # We always store the returned key even if it doesn't the + # Check if this key's expiry timestamp is valid for the verify request. + if key.valid_until_ts >= verify_request.minimum_valid_until_ts: + # Stop looking for this key from subsequent fetchers. + missing_key_ids.discard(key_id) + + # We always store the returned key even if it doesn't meet the # `minimum_valid_until_ts` requirement, as some verification # requests may still be able to be satisfied by it. - # - # We still keep looking for the key from other fetchers in that - # case though. found_keys[key_id] = key - if key.valid_until_ts < verify_request.minimum_valid_until_ts: - continue - - missing_key_ids.discard(key_id) - return found_keys From 6a4b75ace29796b6a80997fc594c5721ae38ceba Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 9 Jan 2023 18:18:05 +0000 Subject: [PATCH 2/6] changelog --- changelog.d/14804.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14804.misc diff --git a/changelog.d/14804.misc b/changelog.d/14804.misc new file mode 100644 index 000000000000..24302332bd64 --- /dev/null +++ b/changelog.d/14804.misc @@ -0,0 +1 @@ +Add some clarifying comments and refactor a portion of the `Keyring` class for readability. \ No newline at end of file From d4d322c7f75c3e3fc40ad8ce78758d4db5c01cfa Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 9 Jan 2023 18:42:23 +0000 Subject: [PATCH 3/6] Correct some logic that was accidentally changed --- 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 b9775f270105..d98535d92ae3 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -431,7 +431,7 @@ async def _inner_fetch_key_request( # If we already have a result for the given key ID, we keep the # one with the highest `valid_until_ts`. existing_key = found_keys.get(key_id) - if existing_key and existing_key.valid_until_ts > key.valid_until_ts: + if existing_key and existing_key.valid_until_ts >= key.valid_until_ts: continue # Check if this key's expiry timestamp is valid for the verify request. From 82560e91e4eb55a0ca55342013b3d6d5103e9ff2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 10 Jan 2023 10:44:23 +0000 Subject: [PATCH 4/6] Rename Keyring._server_queue -> Keyring._fetch_keys_queue --- synapse/crypto/keyring.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index d98535d92ae3..a1f2294a973b 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -163,7 +163,7 @@ def __init__( ) self._key_fetchers = key_fetchers - self._server_queue: BatchingQueue[ + self._fetch_keys_queue: BatchingQueue[ _FetchKeyRequest, Dict[str, Dict[str, FetchKeyResult]] ] = BatchingQueue( "keyring_server", @@ -284,7 +284,7 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: if key_ids_to_find: # We're still missing some keys. Consult each of our `KeyFetcher` instances # (stored in `self._key_fetchers`) to try and find them. - # Key fetch attempts are queued via `self._server_queue` below, and carried + # Key fetch attempts are queued via `self._fetch_keys_queue` below, and carried # out in `self._inner_fetch_key_requests`. # Add the keys we need to verify to the queue for retrieval. We queue @@ -295,7 +295,7 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: 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( + found_keys_by_server = await self._fetch_keys_queue.add_to_queue( key_request, key=verify_request.server_name ) From c5ba64f706816e8a8e2180aeada5b3545ee97ec9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 10 Jan 2023 11:10:04 +0000 Subject: [PATCH 5/6] Migrate to words in docstrings --- synapse/crypto/keyring.py | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index a1f2294a973b..77329559234c 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -168,6 +168,7 @@ def __init__( ] = BatchingQueue( "keyring_server", clock=hs.get_clock(), + # The method called to fetch each key process_batch_callback=self._inner_fetch_key_requests, ) @@ -282,11 +283,6 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None: key_ids_to_find = set(verify_request.key_ids) - found_keys.keys() if key_ids_to_find: - # We're still missing some keys. Consult each of our `KeyFetcher` instances - # (stored in `self._key_fetchers`) to try and find them. - # Key fetch attempts are queued via `self._fetch_keys_queue` below, and carried - # out in `self._inner_fetch_key_requests`. - # 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. @@ -360,7 +356,17 @@ async def _process_json( async def _inner_fetch_key_requests( self, requests: List[_FetchKeyRequest] ) -> Dict[str, Dict[str, FetchKeyResult]]: - """Processing function for the queue of `_FetchKeyRequest`.""" + """Processing function for the queue of `_FetchKeyRequest`. + + Takes a list of key fetch requests, de-duplicates them and then carries out + each request by invoking self._inner_fetch_key_request. + + Args: + requests: A list of requests for homeserver verify keys. + + Returns: + {server name: {key id: fetch key result}} + """ logger.debug("Starting fetch for %s", requests) @@ -405,8 +411,23 @@ async def _inner_fetch_key_requests( async def _inner_fetch_key_request( self, verify_request: _FetchKeyRequest ) -> Dict[str, FetchKeyResult]: - """Attempt to fetch the given key by calling each key fetcher one by - one. + """Attempt to fetch the given key by calling each key fetcher one by one. + + If a key is found, check whether its `valid_until_ts` attribute satisfies the + `minimum_valid_until_ts` attribute of the `verify_request`. If it does, we + refrain from asking subsequent fetchers for that key. + + Even if the above check fails, we still return the found key - the caller may + still find the invalid key result useful. In this case, we continue to ask + subsequent fetchers for the invalid key, in case they return a valid result + for it. This can happen when fetching a stale key result from the database, + before querying the origin server for an up-to-date result. + + Args: + verify_request: The request for a verify key. Can include multiple key IDs. + + Returns: + A map of {key_id: the key fetch result}. """ logger.debug("Starting fetch for %s", verify_request) From 357eef8982cedf2047db338112b60c8be3f94101 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 11 Jan 2023 11:40:11 +0000 Subject: [PATCH 6/6] Revert "Correct some logic that was accidentally changed" This reverts commit d4d322c7f75c3e3fc40ad8ce78758d4db5c01cfa. --- 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 77329559234c..86cd4af9bd5a 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -452,7 +452,7 @@ async def _inner_fetch_key_request( # If we already have a result for the given key ID, we keep the # one with the highest `valid_until_ts`. existing_key = found_keys.get(key_id) - if existing_key and existing_key.valid_until_ts >= key.valid_until_ts: + if existing_key and existing_key.valid_until_ts > key.valid_until_ts: continue # Check if this key's expiry timestamp is valid for the verify request.