Merge pull request #4722 from matrix-org/erikj/correctly_handle_keyring_exceptions

Handle errors when fetching remote server keys

changelog.d/4722.misc

@@ -0,0 +1 @@
+Don't log exceptions when failing to fetch remote server keys

synapse/crypto/keyring.py

@@ -17,6 +17,7 @@
 import logging
 from collections import namedtuple
 
+from six import raise_from
 from six.moves import urllib
 
 from signedjson.key import (
@@ -35,7 +36,12 @@ from unpaddedbase64 import decode_base64
 
 from twisted.internet import defer
 
-from synapse.api.errors import Codes, RequestSendFailed, SynapseError
+from synapse.api.errors import (
+    Codes,
+    HttpResponseException,
+    RequestSendFailed,
+    SynapseError,
+)
 from synapse.util import logcontext, unwrapFirstError
 from synapse.util.logcontext import (
     LoggingContext,
@@ -44,6 +50,7 @@ from synapse.util.logcontext import (
     run_in_background,
 )
 from synapse.util.metrics import Measure
+from synapse.util.retryutils import NotRetryingDestination
 
 logger = logging.getLogger(__name__)
 
@@ -367,13 +374,18 @@ class Keyring(object):
                     server_name_and_key_ids, perspective_name, perspective_keys
                 )
                 defer.returnValue(result)
+            except KeyLookupError as e:
+                logger.warning(
+                    "Key lookup failed from %r: %s", perspective_name, e,
+                )
             except Exception as e:
                 logger.exception(
                     "Unable to get key from %r: %s %s",
                     perspective_name,
                     type(e).__name__, str(e),
                 )
-                defer.returnValue({})
+
+            defer.returnValue({})
 
         results = yield logcontext.make_deferred_yieldable(defer.gatherResults(
             [
@@ -421,21 +433,30 @@ class Keyring(object):
         # TODO(mark): Set the minimum_valid_until_ts to that needed by
         # the events being validated or the current time if validating
         # an incoming request.
-        query_response = yield self.client.post_json(
+        try:
-            destination=perspective_name,
+            query_response = yield self.client.post_json(
-            path="/_matrix/key/v2/query",
+                destination=perspective_name,
-            data={
+                path="/_matrix/key/v2/query",
-                u"server_keys": {
+                data={
-                    server_name: {
+                    u"server_keys": {
-                        key_id: {
+                        server_name: {
-                            u"minimum_valid_until_ts": 0
+                            key_id: {
-                        } for key_id in key_ids
+                                u"minimum_valid_until_ts": 0
+                            } for key_id in key_ids
+                        }
+                        for server_name, key_ids in server_names_and_key_ids
                     }
-                    for server_name, key_ids in server_names_and_key_ids
+                },
-                }
+                long_retries=True,
-            },
+            )
-            long_retries=True,
+        except (NotRetryingDestination, RequestSendFailed) as e:
-        )
+            raise_from(
+                KeyLookupError("Failed to connect to remote server"), e,
+            )
+        except HttpResponseException as e:
+            raise_from(
+                KeyLookupError("Remote server returned an error"), e,
+            )
 
         keys = {}
 
@@ -502,11 +523,20 @@ class Keyring(object):
             if requested_key_id in keys:
                 continue
 
-            response = yield self.client.get_json(
+            try:
-                destination=server_name,
+                response = yield self.client.get_json(
-                path="/_matrix/key/v2/server/" + urllib.parse.quote(requested_key_id),
+                    destination=server_name,
-                ignore_backoff=True,
+                    path="/_matrix/key/v2/server/" + urllib.parse.quote(requested_key_id),
-            )
+                    ignore_backoff=True,
+                )
+            except (NotRetryingDestination, RequestSendFailed) as e:
+                raise_from(
+                    KeyLookupError("Failed to connect to remote server"), e,
+                )
+            except HttpResponseException as e:
+                raise_from(
+                    KeyLookupError("Remote server returned an error"), e,
+                )
 
             if (u"signatures" not in response
                     or server_name not in response[u"signatures"]):