Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix perspectives requests for multiple keys for the same server #11440

Merged
merged 4 commits into from
Nov 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11440.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in Synapse 1.36 which could cause problems fetching event-signing keys from trusted key servers.
30 changes: 19 additions & 11 deletions synapse/crypto/keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -667,28 +667,36 @@ async def get_server_verify_key_v2_indirect(
perspective_name,
)

request: JsonDict = {}
for queue_value in keys_to_fetch:
# there may be multiple requests for each server, so we have to merge
# them intelligently.
request_for_server = {
key_id: {
"minimum_valid_until_ts": queue_value.minimum_valid_until_ts,
}
for key_id in queue_value.key_ids
}
request.setdefault(queue_value.server_name, {}).update(request_for_server)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen this idiom used much (instead of e.g. defaultdict(dict)), but it a) makes sense and b) seems to be used across Synapse.


logger.debug("Request to notary server %s: %s", perspective_name, request)

try:
query_response = await self.client.post_json(
destination=perspective_name,
path="/_matrix/key/v2/query",
data={
"server_keys": {
queue_value.server_name: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's the problem: we'd overwrite the value associated given to a given server's name if we saw that name a second time.

key_id: {
"minimum_valid_until_ts": queue_value.minimum_valid_until_ts,
}
for key_id in queue_value.key_ids
}
for queue_value in keys_to_fetch
}
},
data={"server_keys": request},
)
except (NotRetryingDestination, RequestSendFailed) as e:
# these both have str() representations which we can't really improve upon
raise KeyLookupError(str(e))
except HttpResponseException as e:
raise KeyLookupError("Remote server returned an error: %s" % (e,))

logger.debug(
"Response from notary server %s: %s", perspective_name, query_response
)

keys: Dict[str, Dict[str, FetchKeyResult]] = {}
added_keys: List[Tuple[str, str, FetchKeyResult]] = []

Expand Down
71 changes: 71 additions & 0 deletions tests/crypto/test_keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from nacl.signing import SigningKey
from signedjson.key import encode_verify_key_base64, get_verify_key

from twisted.internet import defer
from twisted.internet.defer import Deferred, ensureDeferred

from synapse.api.errors import SynapseError
Expand Down Expand Up @@ -577,6 +578,76 @@ def test_get_keys_from_perspectives(self):
bytes(res["key_json"]), canonicaljson.encode_canonical_json(response)
)

def test_get_multiple_keys_from_perspectives(self):
"""Check that we can correctly request multiple keys for the same server"""

fetcher = PerspectivesKeyFetcher(self.hs)

SERVER_NAME = "server2"

testkey1 = signedjson.key.generate_signing_key("ver1")
testverifykey1 = signedjson.key.get_verify_key(testkey1)
testverifykey1_id = "ed25519:ver1"

testkey2 = signedjson.key.generate_signing_key("ver2")
testverifykey2 = signedjson.key.get_verify_key(testkey2)
testverifykey2_id = "ed25519:ver2"

VALID_UNTIL_TS = 200 * 1000

response1 = self.build_perspectives_response(
SERVER_NAME,
testkey1,
VALID_UNTIL_TS,
)
response2 = self.build_perspectives_response(
SERVER_NAME,
testkey2,
VALID_UNTIL_TS,
)

async def post_json(destination, path, data, **kwargs):
self.assertEqual(destination, self.mock_perspective_server.server_name)
self.assertEqual(path, "/_matrix/key/v2/query")

# check that the request is for the expected keys
q = data["server_keys"]

self.assertEqual(
list(q[SERVER_NAME].keys()), [testverifykey1_id, testverifykey2_id]
)
return {"server_keys": [response1, response2]}

self.http_client.post_json.side_effect = post_json

# fire off two separate requests; they should get merged together into a
# single HTTP hit.
Comment on lines +623 to +624
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they should get merged together into a single HTTP hit.

I'm not sure how the test enforces this. Would a self.http_client.post_json.assert_called_once() help?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would!

Currently the test enforces it by checking that both key ids are present at line 617.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes. I think this test wouldn't spot us making an identical (correctly merged) request twice. But that's a property of the queue handling, not the merging.

request1_d = defer.ensureDeferred(
fetcher.get_keys(SERVER_NAME, [testverifykey1_id], 0)
)
request2_d = defer.ensureDeferred(
fetcher.get_keys(SERVER_NAME, [testverifykey2_id], 0)
)

keys1 = self.get_success(request1_d)
self.assertIn(testverifykey1_id, keys1)
k = keys1[testverifykey1_id]
self.assertEqual(k.valid_until_ts, VALID_UNTIL_TS)
self.assertEqual(k.verify_key, testverifykey1)
self.assertEqual(k.verify_key.alg, "ed25519")
self.assertEqual(k.verify_key.version, "ver1")

keys2 = self.get_success(request2_d)
self.assertIn(testverifykey2_id, keys2)
k = keys2[testverifykey2_id]
self.assertEqual(k.valid_until_ts, VALID_UNTIL_TS)
self.assertEqual(k.verify_key, testverifykey2)
self.assertEqual(k.verify_key.alg, "ed25519")
self.assertEqual(k.verify_key.version, "ver2")

# finally, ensure that only one request was sent
self.assertEqual(self.http_client.post_json.call_count, 1)

def test_get_perspectives_own_key(self):
"""Check that we can get the perspectives server's own keys

Expand Down