From 5bcccfde6c8e32b0186a072074c13fbeb51c0d04 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 14:45:11 +0100 Subject: [PATCH 1/8] Don't include html comments in description --- synapse/rest/media/v1/preview_url_resource.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 4060593f7f..bdd0e60c5b 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -345,7 +345,8 @@ class PreviewUrlResource(Resource): # lines) text_nodes = ( re.sub(r'\s+', '\n', el.text).strip() - for el in cloned_tree.iter() if el.text + for el in cloned_tree.iter() + if el.text and isinstance(el.tag, basestring) # Removes comments ) og['og:description'] = summarize_paragraphs(text_nodes) From a8b946decbc8d43fe51313235460076da400f720 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 15:31:02 +0100 Subject: [PATCH 2/8] Raise 404 when couldn't find event --- synapse/storage/events.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index e4dbaa3547..d2feee8dbb 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -350,7 +350,7 @@ class EventsStore(SQLBaseStore): ) if not events and not allow_none: - raise RuntimeError("Could not find event %s" % (event_id,)) + raise SynapseError(404, "Could not find event %s" % (event_id,)) defer.returnValue(events[0] if events else None) From 597c79be1001f748d467c0c64acfe85262ee00f9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 16:17:04 +0100 Subject: [PATCH 3/8] Change the way we specify if we require auth or not --- synapse/federation/transport/server.py | 97 +++++++++++++++----------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 0bc6e0801d..ee8f94e340 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -18,7 +18,7 @@ from twisted.internet import defer from synapse.api.urls import FEDERATION_PREFIX as PREFIX from synapse.api.errors import Codes, SynapseError from synapse.http.server import JsonResource -from synapse.http.servlet import parse_json_object_from_request, parse_string +from synapse.http.servlet import parse_json_object_from_request from synapse.util.ratelimitutils import FederationRateLimiter import functools @@ -60,6 +60,16 @@ class TransportLayerServer(JsonResource): ) +class AuthenticationError(SynapseError): + """There was a problem authenticating the request""" + pass + + +class NoAuthenticationError(AuthenticationError): + """The request had no authentication information""" + pass + + class Authenticator(object): def __init__(self, hs): self.keyring = hs.get_keyring() @@ -67,7 +77,7 @@ class Authenticator(object): # A method just so we can pass 'self' as the authenticator to the Servlets @defer.inlineCallbacks - def authenticate_request(self, request): + def authenticate_request(self, request, content): json_request = { "method": request.method, "uri": request.uri, @@ -75,17 +85,10 @@ class Authenticator(object): "signatures": {}, } - content = None - origin = None + if content is not None: + json_request["content"] = content - if request.method in ["PUT", "POST"]: - # TODO: Handle other method types? other content types? - try: - content_bytes = request.content.read() - content = json.loads(content_bytes) - json_request["content"] = content - except: - raise SynapseError(400, "Unable to parse JSON", Codes.BAD_JSON) + origin = None def parse_auth_header(header_str): try: @@ -103,14 +106,14 @@ class Authenticator(object): sig = strip_quotes(param_dict["sig"]) return (origin, key, sig) except: - raise SynapseError( + raise AuthenticationError( 400, "Malformed Authorization header", Codes.UNAUTHORIZED ) auth_headers = request.requestHeaders.getRawHeaders(b"Authorization") if not auth_headers: - raise SynapseError( + raise NoAuthenticationError( 401, "Missing Authorization headers", Codes.UNAUTHORIZED, ) @@ -121,7 +124,7 @@ class Authenticator(object): json_request["signatures"].setdefault(origin, {})[key] = sig if not json_request["signatures"]: - raise SynapseError( + raise NoAuthenticationError( 401, "Missing Authorization headers", Codes.UNAUTHORIZED, ) @@ -130,10 +133,12 @@ class Authenticator(object): logger.info("Request from %s", origin) request.authenticated_entity = origin - defer.returnValue((origin, content)) + defer.returnValue(origin) class BaseFederationServlet(object): + REQUIRE_AUTH = True + def __init__(self, handler, authenticator, ratelimiter, server_name, room_list_handler): self.handler = handler @@ -141,29 +146,46 @@ class BaseFederationServlet(object): self.ratelimiter = ratelimiter self.room_list_handler = room_list_handler - def _wrap(self, code): + def _wrap(self, func): authenticator = self.authenticator ratelimiter = self.ratelimiter @defer.inlineCallbacks - @functools.wraps(code) - def new_code(request, *args, **kwargs): + @functools.wraps(func) + def new_func(request, *args, **kwargs): + content = None + if request.method in ["PUT", "POST"]: + # TODO: Handle other method types? other content types? + content = parse_json_object_from_request(request) + try: - (origin, content) = yield authenticator.authenticate_request(request) - with ratelimiter.ratelimit(origin) as d: - yield d - response = yield code( - origin, content, request.args, *args, **kwargs - ) + origin = yield authenticator.authenticate_request(request, content) + except NoAuthenticationError: + origin = None + if self.REQUIRE_AUTH: + logger.exception("authenticate_request failed") + raise except: logger.exception("authenticate_request failed") raise + + if origin: + with ratelimiter.ratelimit(origin) as d: + yield d + response = yield func( + origin, content, request.args, *args, **kwargs + ) + else: + response = yield func( + origin, content, request.args, *args, **kwargs + ) + defer.returnValue(response) # Extra logic that functools.wraps() doesn't finish - new_code.__self__ = code.__self__ + new_func.__self__ = func.__self__ - return new_code + return new_func def register(self, server): pattern = re.compile("^" + PREFIX + self.PATH + "$") @@ -429,9 +451,10 @@ class FederationGetMissingEventsServlet(BaseFederationServlet): class On3pidBindServlet(BaseFederationServlet): PATH = "/3pid/onbind" + REQUIRE_AUTH = False + @defer.inlineCallbacks - def on_POST(self, request): - content = parse_json_object_from_request(request) + def on_POST(self, origin, content, query): if "invites" in content: last_exception = None for invite in content["invites"]: @@ -453,11 +476,6 @@ class On3pidBindServlet(BaseFederationServlet): raise last_exception defer.returnValue((200, {})) - # Avoid doing remote HS authorization checks which are done by default by - # BaseFederationServlet. - def _wrap(self, code): - return code - class OpenIdUserInfo(BaseFederationServlet): """ @@ -478,9 +496,11 @@ class OpenIdUserInfo(BaseFederationServlet): PATH = "/openid/userinfo" + REQUIRE_AUTH = False + @defer.inlineCallbacks - def on_GET(self, request): - token = parse_string(request, "access_token") + def on_GET(self, origin, content, query): + token = query.get("access_token", [None])[0] if token is None: defer.returnValue((401, { "errcode": "M_MISSING_TOKEN", "error": "Access Token required" @@ -497,11 +517,6 @@ class OpenIdUserInfo(BaseFederationServlet): defer.returnValue((200, {"sub": user_id})) - # Avoid doing remote HS authorization checks which are done by default by - # BaseFederationServlet. - def _wrap(self, code): - return code - class PublicRoomList(BaseFederationServlet): """ From 24f36469bc5c634ff49c87e49e32579d6ac43d7c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 16:36:07 +0100 Subject: [PATCH 4/8] Add federation /version API --- synapse/app/federation_reader.py | 2 +- synapse/app/homeserver.py | 2 +- synapse/app/pusher.py | 2 +- synapse/app/synchrotron.py | 2 +- synapse/federation/transport/server.py | 18 +++++++++++++++++- synapse/util/versionstring.py | 8 ++++---- 6 files changed, 25 insertions(+), 9 deletions(-) diff --git a/synapse/app/federation_reader.py b/synapse/app/federation_reader.py index 58d425f9ac..7355499ae2 100644 --- a/synapse/app/federation_reader.py +++ b/synapse/app/federation_reader.py @@ -165,7 +165,7 @@ def start(config_options): db_config=config.database_config, tls_server_context_factory=tls_server_context_factory, config=config, - version_string=get_version_string("Synapse", synapse), + version_string="Synapse/" + get_version_string(synapse), database_engine=database_engine, ) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index fe68ceb07c..40e6f65236 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -285,7 +285,7 @@ def setup(config_options): # check any extra requirements we have now we have a config check_requirements(config) - version_string = get_version_string("Synapse", synapse) + version_string = "Synapse/" + get_version_string(synapse) logger.info("Server hostname: %s", config.server_name) logger.info("Server version: %s", version_string) diff --git a/synapse/app/pusher.py b/synapse/app/pusher.py index 4f1d18ab5f..c8dde0fcb8 100644 --- a/synapse/app/pusher.py +++ b/synapse/app/pusher.py @@ -273,7 +273,7 @@ def start(config_options): config.server_name, db_config=config.database_config, config=config, - version_string=get_version_string("Synapse", synapse), + version_string="Synapse/" + get_version_string(synapse), database_engine=database_engine, ) diff --git a/synapse/app/synchrotron.py b/synapse/app/synchrotron.py index 8cf5bbbb6d..215ccfd522 100644 --- a/synapse/app/synchrotron.py +++ b/synapse/app/synchrotron.py @@ -424,7 +424,7 @@ def start(config_options): config.server_name, db_config=config.database_config, config=config, - version_string=get_version_string("Synapse", synapse), + version_string="Synapse/" + get_version_string(synapse), database_engine=database_engine, application_service_handler=SynchrotronApplicationService(), ) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index ee8f94e340..37c0d4fbc4 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -20,11 +20,12 @@ from synapse.api.errors import Codes, SynapseError from synapse.http.server import JsonResource from synapse.http.servlet import parse_json_object_from_request from synapse.util.ratelimitutils import FederationRateLimiter +from synapse.util.versionstring import get_version_string import functools import logging -import simplejson as json import re +import synapse logger = logging.getLogger(__name__) @@ -557,6 +558,20 @@ class PublicRoomList(BaseFederationServlet): defer.returnValue((200, data)) +class FederationVersionServlet(BaseFederationServlet): + PATH = "/version" + + REQUIRE_AUTH = False + + def on_GET(self, origin, content, query): + return defer.succeed((200, { + "server": { + "name": "Synapse", + "version": get_version_string(synapse) + }, + })) + + SERVLET_CLASSES = ( FederationSendServlet, FederationPullServlet, @@ -580,6 +595,7 @@ SERVLET_CLASSES = ( On3pidBindServlet, OpenIdUserInfo, PublicRoomList, + FederationVersionServlet, ) diff --git a/synapse/util/versionstring.py b/synapse/util/versionstring.py index a4f156cb3b..52086df465 100644 --- a/synapse/util/versionstring.py +++ b/synapse/util/versionstring.py @@ -21,7 +21,7 @@ import logging logger = logging.getLogger(__name__) -def get_version_string(name, module): +def get_version_string(module): try: null = open(os.devnull, 'w') cwd = os.path.dirname(os.path.abspath(module.__file__)) @@ -74,11 +74,11 @@ def get_version_string(name, module): ) return ( - "%s/%s (%s)" % ( - name, module.__version__, git_version, + "%s (%s)" % ( + module.__version__, git_version, ) ).encode("ascii") except Exception as e: logger.info("Failed to check for git repository: %s", e) - return ("%s/%s" % (name, module.__version__,)).encode("ascii") + return module.__version__.encode("ascii") From f45be053058f4275527f3a850eba43a4f01748a1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 16:46:14 +0100 Subject: [PATCH 5/8] Print newline after result in federation_client script --- scripts-dev/federation_client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts-dev/federation_client.py b/scripts-dev/federation_client.py index 59c3dce3d7..d1ab42d3af 100644 --- a/scripts-dev/federation_client.py +++ b/scripts-dev/federation_client.py @@ -143,6 +143,7 @@ def main(): ) json.dump(result, sys.stdout) + print "" if __name__ == "__main__": main() From 46453bfc2f203bae8ac7845f04ef64c04e172c92 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 18:02:03 +0100 Subject: [PATCH 6/8] Retry joining via other servers if first one failed --- synapse/federation/federation_client.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 65778fd4ee..92332cfdcf 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -523,14 +523,19 @@ class FederationClient(FederationBase): (destination, self.event_from_pdu_json(pdu_dict)) ) break - except CodeMessageException: - raise + except CodeMessageException as e: + if not 500 <= e.code < 600: + raise + else: + logger.warn( + "Failed to make_%s via %s: %s", + membership, destination, e.message + ) except Exception as e: logger.warn( "Failed to make_%s via %s: %s", membership, destination, e.message ) - raise raise RuntimeError("Failed to send to any server.") @@ -602,8 +607,14 @@ class FederationClient(FederationBase): "auth_chain": signed_auth, "origin": destination, }) - except CodeMessageException: - raise + except CodeMessageException as e: + if not 500 <= e.code < 600: + raise + else: + logger.exception( + "Failed to send_join via %s: %s", + destination, e.message + ) except Exception as e: logger.exception( "Failed to send_join via %s: %s", From 5f360182c65d1ef24417ee284c5a66239beca9fe Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Aug 2016 18:08:32 +0100 Subject: [PATCH 7/8] Fix a couple of python bugs --- synapse/federation/federation_client.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 92332cfdcf..da95c2ad6d 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -236,9 +236,9 @@ class FederationClient(FederationBase): # TODO: Rate limit the number of times we try and get the same event. if self._get_pdu_cache: - e = self._get_pdu_cache.get(event_id) - if e: - defer.returnValue(e) + ev = self._get_pdu_cache.get(event_id) + if ev: + defer.returnValue(ev) pdu = None for destination in destinations: @@ -269,7 +269,7 @@ class FederationClient(FederationBase): break - except SynapseError: + except SynapseError as e: logger.info( "Failed to get PDU %s from %s because %s", event_id, destination, e, @@ -336,8 +336,10 @@ class FederationClient(FederationBase): ev.event_id: ev for ev in fetched_events } - pdus = [event_map[e_id] for e_id in state_event_ids] - auth_chain = [event_map[e_id] for e_id in auth_event_ids] + pdus = [event_map[e_id] for e_id in state_event_ids if e_id in event_map] + auth_chain = [ + event_map[e_id] for e_id in auth_event_ids if e_id in event_map + ] auth_chain.sort(key=lambda e: e.depth) From 7c1a92274c720f125e8c5bb96b85f05734a58943 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 8 Aug 2016 11:12:21 +0100 Subject: [PATCH 8/8] Make psutil optional --- synapse/metrics/__init__.py | 13 +++++++++++-- synapse/metrics/metric.py | 5 ++--- synapse/python_dependencies.py | 4 +++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index cce3dba47c..76d5998d75 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -68,9 +68,18 @@ class Metrics(object): def register_memory_metrics(hs): - metric = MemoryUsageMetric(hs) + try: + import psutil + process = psutil.Process() + process.memory_info().rss + except (ImportError, AttributeError): + logger.warn( + "psutil is not installed or incorrect version." + " Disabling memory metrics." + ) + return + metric = MemoryUsageMetric(hs, psutil) all_metrics.append(metric) - return metric def get_metrics_for(pkg_name): diff --git a/synapse/metrics/metric.py b/synapse/metrics/metric.py index 7becbe0491..e81af29895 100644 --- a/synapse/metrics/metric.py +++ b/synapse/metrics/metric.py @@ -16,8 +16,6 @@ from itertools import chain -import psutil - # TODO(paul): I can't believe Python doesn't have one of these def map_concat(func, items): @@ -167,9 +165,10 @@ class MemoryUsageMetric(object): UPDATE_HZ = 2 # number of times to get memory per second WINDOW_SIZE_SEC = 30 # the size of the window in seconds - def __init__(self, hs): + def __init__(self, hs, psutil): clock = hs.get_clock() self.memory_snapshots = [] + self.process = psutil.Process() clock.looping_call(self._update_curr_values, 1000 / self.UPDATE_HZ) diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index 799d35da5e..86e3d89154 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -36,7 +36,6 @@ REQUIREMENTS = { "blist": ["blist"], "pysaml2>=3.0.0,<4.0.0": ["saml2>=3.0.0,<4.0.0"], "pymacaroons-pynacl": ["pymacaroons"], - "psutil>=2.0.0": ["psutil>=2.0.0"], } CONDITIONAL_REQUIREMENTS = { "web_client": { @@ -52,6 +51,9 @@ CONDITIONAL_REQUIREMENTS = { "ldap": { "ldap3>=1.0": ["ldap3>=1.0"], }, + "psutil": { + "psutil>=2.0.0": ["psutil>=2.0.0"], + }, }