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

Fetch verify key locally rather than trying to do so over federation if origin and host are the same. #11129

Merged
merged 10 commits into from
Oct 28, 2021
1 change: 1 addition & 0 deletions changelog.d/11129.bugfix
Original file line number Diff line number Diff line change
@@ -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.
74 changes: 45 additions & 29 deletions synapse/crypto/keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -30,6 +31,7 @@
signature_ids,
verify_signed_json,
)
from signedjson.types import VerifyKey
from unpaddedbase64 import decode_base64

from twisted.internet import defer
Expand Down Expand Up @@ -177,6 +179,8 @@ def __init__(
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,
Expand All @@ -196,6 +200,7 @@ 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)
"""

request = VerifyJsonRequest.from_json_object(
server_name,
json_object,
Expand Down Expand Up @@ -262,6 +267,11 @@ 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:
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.
Expand All @@ -285,35 +295,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(
Expand All @@ -322,6 +305,39 @@ async def process_request(self, verify_request: VerifyJsonRequest) -> None:
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]]:
Expand Down
12 changes: 12 additions & 0 deletions tests/crypto/test_keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,18 @@ def test_verify_json_for_server(self):
# self.assertFalse(d.called)
self.get_success(d)

def test_verify_for_server_locally(self):
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""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)
clokep marked this conversation as resolved.
Show resolved Hide resolved

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`
Expand Down