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
2 changes: 2 additions & 0 deletions changelog.d/11129.bugfix
Original file line number Diff line number Diff line change
@@ -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.
H-Shay marked this conversation as resolved.
Show resolved Hide resolved
45 changes: 45 additions & 0 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 Down Expand Up @@ -177,6 +178,46 @@ 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

def verify_json_locally(self, server_name: str, json_object: JsonDict) -> None:
H-Shay marked this conversation as resolved.
Show resolved Hide resolved
"""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,
Expand All @@ -196,6 +237,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
H-Shay marked this conversation as resolved.
Show resolved Hide resolved
if server_name == self.hostname:
return self.verify_json_locally(server_name, json_object)
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

request = VerifyJsonRequest.from_json_object(
server_name,
json_object,
Expand Down
24 changes: 24 additions & 0 deletions tests/crypto/test_keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

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