diff --git a/changelog.d/5392.bugfix b/changelog.d/5392.bugfix
new file mode 100644
index 0000000000..295a7cfce1
--- /dev/null
+++ b/changelog.d/5392.bugfix
@@ -0,0 +1 @@
+Remove redundant warning about key server response validation.
diff --git a/changelog.d/5415.bugfix b/changelog.d/5415.bugfix
new file mode 100644
index 0000000000..83629e193d
--- /dev/null
+++ b/changelog.d/5415.bugfix
@@ -0,0 +1 @@
+Fix bug where old keys stored in the database with a null valid until timestamp caused all verification requests for that key to fail.
diff --git a/synapse/config/key.py b/synapse/config/key.py
index aba7092ccd..424875feae 100644
--- a/synapse/config/key.py
+++ b/synapse/config/key.py
@@ -41,6 +41,15 @@ validation or TLS certificate validation. This is likely to be very insecure. If
you are *sure* you want to do this, set 'accept_keys_insecurely' on the
keyserver configuration."""
+RELYING_ON_MATRIX_KEY_ERROR = """\
+Your server is configured to accept key server responses without TLS certificate
+validation, and which are only signed by the old (possibly compromised)
+matrix.org signing key 'ed25519:auto'. This likely isn't what you want to do,
+and you should enable 'federation_verify_certificates' in your configuration.
+
+If you are *sure* you want to do this, set 'accept_keys_insecurely' on the
+trusted_key_server configuration."""
+
logger = logging.getLogger(__name__)
@@ -340,10 +349,20 @@ def _parse_key_servers(key_servers, federation_verify_certificates):
result.verify_keys[key_id] = verify_key
if (
- not verify_keys
- and not server.get("accept_keys_insecurely")
- and not federation_verify_certificates
+ not federation_verify_certificates and
+ not server.get("accept_keys_insecurely")
):
- raise ConfigError(INSECURE_NOTARY_ERROR)
+ _assert_keyserver_has_verify_keys(result)
yield result
+
+
+def _assert_keyserver_has_verify_keys(trusted_key_server):
+ if not trusted_key_server.verify_keys:
+ raise ConfigError(INSECURE_NOTARY_ERROR)
+
+ # also check that they are not blindly checking the old matrix.org key
+ if trusted_key_server.server_name == "matrix.org" and any(
+ key_id == "ed25519:auto" for key_id in trusted_key_server.verify_keys
+ ):
+ raise ConfigError(RELYING_ON_MATRIX_KEY_ERROR)
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index 96964b0d50..6f603f1961 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -750,13 +750,6 @@ class PerspectivesKeyFetcher(BaseV2KeyFetcher):
verify_signed_json(response, perspective_name, perspective_keys[key_id])
verified = True
- if perspective_name == "matrix.org" and key_id == "ed25519:auto":
- logger.warning(
- "Trusting trusted_key_server responses signed by the "
- "compromised matrix.org signing key 'ed25519:auto'. "
- "This is a placebo."
- )
-
if not verified:
raise KeyLookupError(
"Response not signed with a known key: signed with: %r, known keys: %r"
diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py
index 5300720dbb..e3655ad8d7 100644
--- a/synapse/storage/keys.py
+++ b/synapse/storage/keys.py
@@ -80,6 +80,14 @@ class KeyStore(SQLBaseStore):
for row in txn:
server_name, key_id, key_bytes, ts_valid_until_ms = row
+
+ if ts_valid_until_ms is None:
+ # Old keys may be stored with a ts_valid_until_ms of null,
+ # in which case we treat this as if it was set to `0`, i.e.
+ # it won't match key requests that define a minimum
+ # `ts_valid_until_ms`.
+ ts_valid_until_ms = 0
+
res = FetchKeyResult(
verify_key=decode_verify_key_bytes(key_id, bytes(key_bytes)),
valid_until_ts=ts_valid_until_ms,
diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py
index 4b1901ce31..5a355f00cc 100644
--- a/tests/crypto/test_keyring.py
+++ b/tests/crypto/test_keyring.py
@@ -25,7 +25,11 @@ from twisted.internet import defer
from synapse.api.errors import SynapseError
from synapse.crypto import keyring
-from synapse.crypto.keyring import PerspectivesKeyFetcher, ServerKeyFetcher
+from synapse.crypto.keyring import (
+ PerspectivesKeyFetcher,
+ ServerKeyFetcher,
+ StoreKeyFetcher,
+)
from synapse.storage.keys import FetchKeyResult
from synapse.util import logcontext
from synapse.util.logcontext import LoggingContext
@@ -219,6 +223,50 @@ class KeyringTestCase(unittest.HomeserverTestCase):
# self.assertFalse(d.called)
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`
+ """
+ mock_fetcher = keyring.KeyFetcher()
+ mock_fetcher.get_keys = Mock(return_value=defer.succeed({}))
+
+ kr = keyring.Keyring(
+ self.hs, key_fetchers=(StoreKeyFetcher(self.hs), mock_fetcher)
+ )
+
+ key1 = signedjson.key.generate_signing_key(1)
+ r = self.hs.datastore.store_server_verify_keys(
+ "server9",
+ time.time() * 1000,
+ [("server9", get_key_id(key1), FetchKeyResult(get_verify_key(key1), None))],
+ )
+ self.get_success(r)
+
+ json1 = {}
+ signedjson.sign.sign_json(json1, "server9", key1)
+
+ # should fail immediately on an unsigned object
+ d = _verify_json_for_server(kr, "server9", {}, 0, "test unsigned")
+ self.failureResultOf(d, SynapseError)
+
+ # should fail on a signed object with a non-zero minimum_valid_until_ms,
+ # as it tries to refetch the keys and fails.
+ d = _verify_json_for_server(
+ kr, "server9", json1, 500, "test signed non-zero min"
+ )
+ self.get_failure(d, SynapseError)
+
+ # We expect the keyring tried to refetch the key once.
+ mock_fetcher.get_keys.assert_called_once_with(
+ {"server9": {get_key_id(key1): 500}}
+ )
+
+ # should succeed on a signed object with a 0 minimum_valid_until_ms
+ d = _verify_json_for_server(
+ kr, "server9", json1, 0, "test signed with zero min"
+ )
+ self.get_success(d)
+
def test_verify_json_dedupes_key_requests(self):
"""Two requests for the same key should be deduped."""
key1 = signedjson.key.generate_signing_key(1)
|
