From 88d7182adaef8711bf3cc80ff604e566e517b6e6 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Mon, 10 Jun 2019 10:33:00 +0100
Subject: [PATCH] Improve startup checks for insecure notary configs (#5392)

It's not really a problem to trust notary responses signed by the old key so
long as we are also doing TLS validation.

This commit adds a check to the config parsing code at startup to check that
we do not have the insecure matrix.org key without tls validation, and refuses
to start without it.

This allows us to remove the rather alarming-looking warning which happens at
runtime.
---
 changelog.d/5392.bugfix   |  1 +
 synapse/config/key.py     | 27 +++++++++++++++++++++++----
 synapse/crypto/keyring.py |  7 -------
 3 files changed, 24 insertions(+), 11 deletions(-)
 create mode 100644 changelog.d/5392.bugfix

diff --git a/changelog.d/5392.bugfix b/changelog.d/5392.bugfix
new file mode 100644
index 000000000..295a7cfce
--- /dev/null
+++ b/changelog.d/5392.bugfix
@@ -0,0 +1 @@
+Remove redundant warning about key server response validation.
diff --git a/synapse/config/key.py b/synapse/config/key.py
index aba7092cc..424875fea 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 96964b0d5..6f603f196 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"