Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle errors when fetching remote server keys #4722

Merged
merged 3 commits into from Feb 25, 2019
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+52 −21
Diff settings

Always

Just for now

@@ -0,0 +1 @@
Don't log exceptions when failing to fetch remote server keys
@@ -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 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 @@
run_in_background,
)
from synapse.util.metrics import Measure
from synapse.util.retryutils import NotRetryingDestination

logger = logging.getLogger(__name__)

@@ -367,13 +374,18 @@ def get_key(perspective_name, perspective_keys):
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 @@ def get_server_verify_key_v2_indirect(self, server_names_and_key_ids,
# 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(
destination=perspective_name,
path="/_matrix/key/v2/query",
data={
u"server_keys": {
server_name: {
key_id: {
u"minimum_valid_until_ts": 0
} for key_id in key_ids
try:
query_response = yield self.client.post_json(
destination=perspective_name,
path="/_matrix/key/v2/query",
data={
u"server_keys": {
server_name: {
key_id: {
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 raise_from(

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 25, 2019

Member

I think the raise is redundant here?

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Feb 25, 2019

Author Member

Aha, yes

KeyLookupError("Failed to connect to remote server"), e,
)
except HttpResponseException as e:
raise raise_from(
KeyLookupError("Remote server returned an error"), e,
)

keys = {}

@@ -502,11 +523,20 @@ def get_server_verify_key_v2_direct(self, server_name, key_ids):
if requested_key_id in keys:
continue

response = yield self.client.get_json(
destination=server_name,
path="/_matrix/key/v2/server/" + urllib.parse.quote(requested_key_id),
ignore_backoff=True,
)
try:
response = yield self.client.get_json(
destination=server_name,
path="/_matrix/key/v2/server/" + urllib.parse.quote(requested_key_id),
ignore_backoff=True,
)
except (NotRetryingDestination, RequestSendFailed) as e:
raise raise_from(
KeyLookupError("Failed to connect to remote server"), e,
)
except HttpResponseException as e:
raise raise_from(
KeyLookupError("Remote server returned an error"), e,
)

if (u"signatures" not in response
or server_name not in response[u"signatures"]):
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.