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.
This commit is contained in:
Richard van der Hoff 2021-11-19 10:55:09 +00:00 committed by GitHub
parent 7ffddd819c
commit a6f7f84570
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 95 additions and 31 deletions

1
changelog.d/11379.bugfix Normal file
View File

@ -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.

View File

@ -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 @@ class VerifyJsonRequest:
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 @@ 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
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 @@ 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
found_keys: Dict[str, FetchKeyResult] = {}
# 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 = verify_request.to_fetch_key_request()
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, {})
# 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.

View File

@ -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 synapse.storage.keys import FetchKeyResult
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 @@ class KeyringTestCase(unittest.HomeserverTestCase):
# 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 @@ class KeyringTestCase(unittest.HomeserverTestCase):
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`