remove trailing slashes as per #3622

Add docs for ACME setup

CHANGES.md

@@ -1,3 +1,35 @@
+Synapse 0.99.0rc4 (2019-02-01)
+==============================
+
+Internal Changes
+----------------
+
+- Update federation routing logic to check .well-known before SRV ([\#4539](https://github.com/matrix-org/synapse/issues/4539))
+- Improve performance of handling servers with invalid .well-known ([\#4542](https://github.com/matrix-org/synapse/issues/4542))
+- Treat an invalid .well-known file the same as an absent one ([\#4544](https://github.com/matrix-org/synapse/issues/4544))
+
+
+Synapse 0.99.0rc3 (2019-01-31)
+==============================
+
+Bugfixes
+--------
+
+- Fix infinite loop when an event is redacted in a v3 room ([\#4535](https://github.com/matrix-org/synapse/issues/4535))
+
+
+Improved Documentation
+----------------------
+
+- Update debian installation instructions ([\#4526](https://github.com/matrix-org/synapse/issues/4526))
+
+
+Internal Changes
+----------------
+
+- Add some debug for membership syncing issues ([\#4538](https://github.com/matrix-org/synapse/issues/4538))
+
+
 Synapse 0.99.0rc2 (2019-01-30)
 ==============================
 

README.rst

@@ -216,16 +216,83 @@ different. See `the spec`__ for more information on key management.)
 The default configuration exposes two HTTP ports: 8008 and 8448. Port 8008 is
 configured without TLS; it should be behind a reverse proxy for TLS/SSL
 termination on port 443 which in turn should be used for clients. Port 8448
-is configured to use TLS with a self-signed certificate. If you would like
-to do initial test with a client without having to setup a reverse proxy,
-you can temporarly use another certificate. (Note that a self-signed
-certificate is fine for `Federation`_). You can do so by changing
-``tls_certificate_path`` and ``tls_private_key_path``
-in ``homeserver.yaml``; alternatively, you can use a reverse-proxy, but be sure
-to read `Using a reverse proxy with Synapse`_ when doing so.
+is configured to use TLS for `Federation`_ with a self-signed or verified
+certificate, but please be aware that a valid certificate will be required in
+Synapse v1.0.
+
+If you would like to use your own certificates, you can do so by changing
+``tls_certificate_path`` and ``tls_private_key_path`` in ``homeserver.yaml``;
+alternatively, you can use a reverse-proxy. Apart from port 8448 using TLS,
+both ports are the same in the default configuration.
+
+
+ACME setup
+----------
+
+Synapse v1.0 will require valid TLS certificates for communication between servers
+(port ``8448`` by default) in addition to those that are client-facing (port
+``443``). In the case that your `server_name` config variable is the same as
+the hostname that the client connects to, then the same certificate can be
+used between client and federation ports without issue. Synapse v0.99.0+
+**will provision server-to-server certificates automatically for you for
+free** through `Let's Encrypt
+<https://letsencrypt.org/>`_ if you tell it to.
+
+In order for Synapse to complete the ACME challenge to provision a
+certificate, it needs access to port 80. Typically listening on port 80 is
+only granted to applications running as root. There are thus two solutions to
+this problem.
+
+**Using a reverse proxy**
+
+A reverse proxy such as Apache or nginx allows a single process (the web
+server) to listen on port 80 and proxy traffic to the appropriate program
+running on your server. It is the recommended method for setting up ACME as
+it allows you to use your existing webserver while also allowing Synapse to
+provision certificates as needed.
+
+For nginx users, add the following line to your existing ``server`` block::
+
+    location /.well-known/acme-challenge {
+        proxy_pass http://localhost:8009/;
+    }
+
+For Apache, add the following to your existing webserver config::
+
+    ProxyPass /.well-known/acme-challenge http://localhost:8009/.well-known/acme-challenge
+
+Make sure to restart/reload your webserver after making changes.
+
+
+**Authbind**
+
+``authbind`` allows a program which does not run as root to bind to
+low-numbered ports in a controlled way. The setup is simpler, but requires a
+webserver not to already be running on port 80. **This includes every time
+Synapse renews a certificate**, which may be cumbersome if you usually run a
+web server on port 80. Nevertheless, if you're sure port 80 is not being used
+for any other purpose then all that is necessary is the following:
+
+Install ``authbind``. For example, on Debian/Ubuntu::
+
+    sudo apt-get install authbind
+
+Allow ``authbind`` to bind port 80::
+
+    sudo touch /etc/authbind/byport/80
+    sudo chmod 777 /etc/authbind/byport/80
+
+When Synapse is started, use the following syntax::
+
+    authbind --deep <synapse start command>
+
+Finally, once Synapse's is able to listen on port 80 for ACME challenge
+requests, it must be told to perform ACME provisioning by setting ``enabled``
+to true under the ``acme`` section in ``homeserver.yaml``::
+
+    acme:
+        enabled: true
 
-Apart from port 8448 using TLS, both ports are the same in the default
-configuration.
 
 Registering a user
 ------------------
@@ -281,10 +348,11 @@ following the recommended setup, or ``https://localhost:8448`` - remember to spe
 port (``:8448``) if not ``:443`` unless you changed the configuration. (Leave the identity
 server as the default - see `Identity servers`_.)
 
-If using port 8448 you will run into errors until you accept the self-signed
-certificate. You can easily do this by going to ``https://localhost:8448``
+If using port 8448 you will run into errors if you are using a self-signed
+certificate. To overcome this, simply go to ``https://localhost:8448``
 directly with your browser and accept the presented certificate. You can then
-go back in your web client and proceed further.
+go back in your web client and proceed further. Valid federation certificates
+should not have this problem.
 
 If all goes well you should at least be able to log in, create a room, and
 start sending messages.
@@ -333,12 +401,38 @@ https://developer.github.com/changes/2014-04-25-user-content-security for more d
 Platform-Specific Instructions
 ==============================
 
-Debian
-------
+Debian/Ubuntu
+-------------
 
-Matrix provides official Debian packages via apt from https://matrix.org/packages/debian/.
-Note that these packages do not include a client - choose one from
-https://matrix.org/docs/projects/try-matrix-now.html (or build your own with one of our SDKs :)
+Matrix.org packages
+~~~~~~~~~~~~~~~~~~~
+
+Matrix.org provides Debian/Ubuntu packages of the latest stable version of
+Synapse via https://matrix.org/packages/debian/. To use them::
+
+    sudo apt install -y lsb-release curl apt-transport-https
+    echo "deb https://matrix.org/packages/debian `lsb_release -cs` main" |
+        sudo tee /etc/apt/sources.list.d/matrix-org.list
+    curl "https://matrix.org/packages/debian/repo-key.asc" |
+        sudo apt-key add -
+    sudo apt update
+    sudo apt install matrix-synapse-py3
+
+Downstream Debian/Ubuntu packages
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+For ``buster`` and ``sid``, Synapse is available in the Debian repositories and
+it should be possible to install it with simply::
+
+    sudo apt install matrix-synapse
+
+There is also a version of ``matrix-synapse`` in ``stretch-backports``. Please
+see the `Debian documentation on backports
+<https://backports.debian.org/Instructions/>`_ for information on how to use
+them.
+
+We do not recommend using the packages in downstream Ubuntu at this time, as
+they are old and suffer from known security vulnerabilities.
 
 Fedora
 ------
@@ -527,9 +621,7 @@ you to run your server on a machine that might not have the same name as your
 domain name. For example, you might want to run your server at
 ``synapse.example.com``, but have your Matrix user-ids look like
 ``@user:example.com``. (A SRV record also allows you to change the port from
-the default 8448. However, if you are thinking of using a reverse-proxy on the
-federation port, which is not recommended, be sure to read
-`Reverse-proxying the federation port`_ first.)
+the default 8448).
 
 To use a SRV record, first create your SRV record and publish it in DNS. This
 should have the format ``_matrix._tcp.<yourdomain.com> <ttl> IN SRV 10 0 <port>
@@ -567,6 +659,8 @@ Troubleshooting
 You can use the federation tester to check if your homeserver is all set:
 ``https://matrix.org/federationtester/api/report?server_name=<your_server_name>``
 If any of the attributes under "checks" is false, federation won't work.
+There is also a nicer interface available from a community member at
+`<https://neo.lain.haus/fed-tester>`_.
 
 The typical failure mode with federation is that when you try to join a room,
 it is rejected with "401: Unauthorized". Generally this means that other
@@ -575,8 +669,6 @@ complicated dance which requires connections in both directions).
 
 So, things to check are:
 
-* If you are trying to use a reverse-proxy, read `Reverse-proxying the
-  federation port`_.
 * If you are not using a SRV record, check that your ``server_name`` (the part
   of your user-id after the ``:``) matches your hostname, and that port 8448 on
   that hostname is reachable from outside your network.
@@ -631,14 +723,8 @@ port. Indeed, clients will use port 443 by default, whereas servers default to
 port 8448. Where these are different, we refer to the 'client port' and the
 'federation port'.
 
-The next most important thing to know is that using a reverse-proxy on the
-federation port has a number of pitfalls. It is possible, but be sure to read
-`Reverse-proxying the federation port`_.
-
-The recommended setup is therefore to configure your reverse-proxy on port 443
-to port 8008 of synapse for client connections, but to also directly expose port
-8448 for server-server connections. All the Matrix endpoints begin ``/_matrix``,
-so an example nginx configuration might look like::
+All Matrix endpoints begin with ``/_matrix``, so an example nginx
+configuration for forwarding client connections to Synapse might look like::
 
   server {
       listen 443 ssl;
@@ -679,64 +765,6 @@ Having done so, you can then use ``https://matrix.example.com`` (instead of
 ``https://matrix.example.com:8448``) as the "Custom server" when `Connecting to
 Synapse from a client`_.
 
-Reverse-proxying the federation port
-------------------------------------
-
-There are two issues to consider before using a reverse-proxy on the federation
-port:
-
-* Due to the way SSL certificates are managed in the Matrix federation protocol
-  (see `spec`__), Synapse needs to be configured with the path to the SSL
-  certificate, *even if you do not terminate SSL at Synapse*.
-
-  .. __: `key_management`_
-
-* Until v0.33.3, Synapse did not support SNI on the federation port
-  (`bug #1491 <https://github.com/matrix-org/synapse/issues/1491>`_). This bug
-  is now fixed, but means that federating with older servers can be unreliable
-  when using name-based virtual hosting.
-
-Furthermore, a number of the normal reasons for using a reverse-proxy do not
-apply:
-
-* Other servers will connect on port 8448 by default, so there is no need to
-  listen on port 443 (for federation, at least), which avoids the need for root
-  privileges and virtual hosting.
-
-* A self-signed SSL certificate is fine for federation, so there is no need to
-  automate renewals. (The certificate generated by ``--generate-config`` is
-  valid for 10 years.)
-
-If you want to set up a reverse-proxy on the federation port despite these
-caveats, you will need to do the following:
-
-* In ``homeserver.yaml``, set ``tls_certificate_path`` to the path to the SSL
-  certificate file used by your reverse-proxy, and set ``no_tls`` to ``True``.
-  (``tls_private_key_path`` will be ignored if ``no_tls`` is ``True``.)
-
-* In your reverse-proxy configuration:
-
-  * If there are other virtual hosts on the same port, make sure that the
-    *default* one uses the certificate configured above.
-
-  * Forward ``/_matrix`` to Synapse.
-
-* If your reverse-proxy is not listening on port 8448, publish a SRV record to
-  tell other servers how to find you. See `Setting up Federation`_.
-
-When updating the SSL certificate, just update the file pointed to by
-``tls_certificate_path`` and then restart Synapse. (You may like to use a symbolic link
-to help make this process atomic.)
-
-The most common mistake when setting up federation is not to tell Synapse about
-your SSL certificate. To check it, you can visit
-``https://matrix.org/federationtester/api/report?server_name=<your_server_name>``.
-Unfortunately, there is no UI for this yet, but, you should see
-``"MatchingTLSFingerprint": true``. If not, check that
-``Certificates[0].SHA256Fingerprint`` (the fingerprint of the certificate
-presented by your reverse-proxy) matches ``Keys.tls_fingerprints[0].sha256``
-(the fingerprint of the certificate Synapse is using).
-
 
 Identity Servers
 ================

changelog.d/4547.misc

@@ -0,0 +1 @@
+Add docs for ACME setup to README.

contrib/prometheus/README.md

@@ -6,8 +6,10 @@ To use it, first install prometheus by following the instructions at
   http://prometheus.io/
 
 ### for Prometheus v1
+
 Add a new job to the main prometheus.conf file:
 
+```yaml
   job: {
     name: "synapse"
 
@@ -15,10 +17,12 @@ Add a new job to the main prometheus.conf file:
       target: "http://SERVER.LOCATION.HERE:PORT/_synapse/metrics"
     }
   }
+```
 
 ### for Prometheus v2
 Add a new job to the main prometheus.yml file:
 
+```yaml
   - job_name: "synapse"
     metrics_path: "/_synapse/metrics"
     # when endpoint uses https:
@@ -26,11 +30,14 @@ Add a new job to the main prometheus.yml file:
 
     static_configs:
     - targets: ['SERVER.LOCATION:PORT']
+```
 
 To use `synapse.rules` add
 
+```yaml
     rule_files:
       - "/PATH/TO/synapse-v2.rules"
+```
 
 Metrics are disabled by default when running synapse; they must be enabled
 with the 'enable-metrics' option, either in the synapse config file or as a

synapse/__init__.py

@@ -27,4 +27,4 @@ try:
 except ImportError:
     pass
 
-__version__ = "0.99.0rc2"
+__version__ = "0.99.0rc4"

synapse/federation/transport/client.py

@@ -167,7 +167,7 @@ class TransportLayerClient(object):
         # generated by the json_data_callback.
         json_data = transaction.get_dict()
 
-        path = _create_v1_path("/send/%s/", transaction.transaction_id)
+        path = _create_v1_path("/send/%s", transaction.transaction_id)
 
         response = yield self.client.put_json(
             transaction.destination,

synapse/federation/transport/server.py

@@ -366,7 +366,7 @@ class FederationSendServlet(BaseFederationServlet):
 
 
 class FederationEventServlet(BaseFederationServlet):
-    PATH = "/event/(?P<event_id>[^/]*)/"
+    PATH = "/event/(?P<event_id>[^/]*)"
 
     # This is when someone asks for a data item for a given server data_id pair.
     def on_GET(self, origin, content, query, event_id):
@@ -374,7 +374,7 @@ class FederationEventServlet(BaseFederationServlet):
 
 
 class FederationStateServlet(BaseFederationServlet):
-    PATH = "/state/(?P<context>[^/]*)/"
+    PATH = "/state/(?P<context>[^/]*)"
 
     # This is when someone asks for all data for a given context.
     def on_GET(self, origin, content, query, context):
@@ -386,7 +386,7 @@ class FederationStateServlet(BaseFederationServlet):
 
 
 class FederationStateIdsServlet(BaseFederationServlet):
-    PATH = "/state_ids/(?P<room_id>[^/]*)/"
+    PATH = "/state_ids/(?P<room_id>[^/]*)"
 
     def on_GET(self, origin, content, query, room_id):
         return self.handler.on_state_ids_request(
@@ -397,7 +397,7 @@ class FederationStateIdsServlet(BaseFederationServlet):
 
 
 class FederationBackfillServlet(BaseFederationServlet):
-    PATH = "/backfill/(?P<context>[^/]*)/"
+    PATH = "/backfill/(?P<context>[^/]*)"
 
     def on_GET(self, origin, content, query, context):
         versions = [x.decode('ascii') for x in query[b"v"]]

synapse/handlers/sync.py

@@ -895,14 +895,17 @@ class SyncHandler(object):
         Returns:
             Deferred(SyncResult)
         """
-        logger.info("Calculating sync response for %r", sync_config.user)
-
         # NB: The now_token gets changed by some of the generate_sync_* methods,
         # this is due to some of the underlying streams not supporting the ability
         # to query up to a given point.
         # Always use the `now_token` in `SyncResultBuilder`
         now_token = yield self.event_sources.get_current_token()
 
+        logger.info(
+            "Calculating sync response for %r between %s and %s",
+            sync_config.user, since_token, now_token,
+        )
+
         user_id = sync_config.user.to_string()
         app_service = self.store.get_app_service_by_user_id(user_id)
         if app_service:
@@ -1390,6 +1393,12 @@ class SyncHandler(object):
         room_entries = []
         invited = []
         for room_id, events in iteritems(mem_change_events_by_room_id):
+            logger.info(
+                "Membership changes in %s: [%s]",
+                room_id,
+                ", ".join(("%s (%s)" % (e.event_id, e.membership) for e in events)),
+            )
+
             non_joins = [e for e in events if e.membership != Membership.JOIN]
             has_join = len(non_joins) != len(events)
 

synapse/http/federation/matrix_federation_agent.py

@@ -23,14 +23,17 @@ from zope.interface import implementer
 
 from twisted.internet import defer
 from twisted.internet.endpoints import HostnameEndpoint, wrapClientTLS
+from twisted.internet.interfaces import IStreamClientEndpoint
 from twisted.web.client import URI, Agent, HTTPConnectionPool, RedirectAgent, readBody
 from twisted.web.http import stringToDatetime
 from twisted.web.http_headers import Headers
 from twisted.web.iweb import IAgent
 
 from synapse.http.federation.srv_resolver import SrvResolver, pick_server_from_list
+from synapse.util import Clock
 from synapse.util.caches.ttlcache import TTLCache
 from synapse.util.logcontext import make_deferred_yieldable
+from synapse.util.metrics import Measure
 
 # period to cache .well-known results for by default
 WELL_KNOWN_DEFAULT_CACHE_PERIOD = 24 * 3600
@@ -44,7 +47,6 @@ WELL_KNOWN_INVALID_CACHE_PERIOD = 1 * 3600
 # cap for .well-known cache period
 WELL_KNOWN_MAX_CACHE_PERIOD = 48 * 3600
 
-
 logger = logging.getLogger(__name__)
 well_known_cache = TTLCache('well-known')
 
@@ -78,6 +80,8 @@ class MatrixFederationAgent(object):
         _well_known_cache=well_known_cache,
     ):
         self._reactor = reactor
+        self._clock = Clock(reactor)
+
         self._tls_client_options_factory = tls_client_options_factory
         if _srv_resolver is None:
             _srv_resolver = SrvResolver()
@@ -98,6 +102,10 @@ class MatrixFederationAgent(object):
         )
         self._well_known_agent = _well_known_agent
 
+        # our cache of .well-known lookup results, mapping from server name
+        # to delegated name. The values can be:
+        #   `bytes`:     a valid server-name
+        #   `None`:      there is no (valid) .well-known here
         self._well_known_cache = _well_known_cache
 
     @defer.inlineCallbacks
@@ -152,12 +160,9 @@ class MatrixFederationAgent(object):
         class EndpointFactory(object):
             @staticmethod
             def endpointForURI(_uri):
-                logger.info(
-                    "Connecting to %s:%i",
-                    res.target_host.decode("ascii"),
-                    res.target_port,
+                ep = LoggingHostnameEndpoint(
+                    self._reactor, res.target_host, res.target_port,
                 )
-                ep = HostnameEndpoint(self._reactor, res.target_host, res.target_port)
                 if tls_options is not None:
                     ep = wrapClientTLS(tls_options, ep)
                 return ep
@@ -210,11 +215,7 @@ class MatrixFederationAgent(object):
                 target_port=parsed_uri.port,
             ))
 
-        # try a SRV lookup
-        service_name = b"_matrix._tcp.%s" % (parsed_uri.host,)
-        server_list = yield self._srv_resolver.resolve_service(service_name)
-
-        if not server_list and lookup_well_known:
+        if lookup_well_known:
             # try a .well-known lookup
             well_known_server = yield self._get_well_known(parsed_uri.host)
 
@@ -250,6 +251,10 @@ class MatrixFederationAgent(object):
                 res = yield self._route_matrix_uri(new_uri, lookup_well_known=False)
                 defer.returnValue(res)
 
+        # try a SRV lookup
+        service_name = b"_matrix._tcp.%s" % (parsed_uri.host,)
+        server_list = yield self._srv_resolver.resolve_service(service_name)
+
         if not server_list:
             target_host = parsed_uri.host
             port = 8448
@@ -283,14 +288,32 @@ class MatrixFederationAgent(object):
                 None if there was no .well-known file.
         """
         try:
-            cached = self._well_known_cache[server_name]
-            defer.returnValue(cached)
+            result = self._well_known_cache[server_name]
         except KeyError:
-            pass
+            # TODO: should we linearise so that we don't end up doing two .well-known
+            # requests for the same server in parallel?
+            with Measure(self._clock, "get_well_known"):
+                result, cache_period = yield self._do_get_well_known(server_name)
 
-        # TODO: should we linearise so that we don't end up doing two .well-known requests
-        # for the same server in parallel?
+            if cache_period > 0:
+                self._well_known_cache.set(server_name, result, cache_period)
 
+        defer.returnValue(result)
+
+    @defer.inlineCallbacks
+    def _do_get_well_known(self, server_name):
+        """Actually fetch and parse a .well-known, without checking the cache
+
+        Args:
+            server_name (bytes): name of the server, from the requested url
+
+        Returns:
+            Deferred[Tuple[bytes|None|object],int]:
+                result, cache period, where result is one of:
+                 - the new server name from the .well-known (as a `bytes`)
+                 - None if there was no .well-known file.
+                 - INVALID_WELL_KNOWN if the .well-known was invalid
+        """
         uri = b"https://%s/.well-known/matrix/server" % (server_name, )
         uri_str = uri.decode("ascii")
         logger.info("Fetching %s", uri_str)
@@ -301,18 +324,7 @@ class MatrixFederationAgent(object):
             body = yield make_deferred_yieldable(readBody(response))
             if response.code != 200:
                 raise Exception("Non-200 response %s" % (response.code, ))
-        except Exception as e:
-            logger.info("Error fetching %s: %s", uri_str, e)
 
-            # add some randomness to the TTL to avoid a stampeding herd every hour
-            # after startup
-            cache_period = WELL_KNOWN_INVALID_CACHE_PERIOD
-            cache_period += random.uniform(0, WELL_KNOWN_DEFAULT_CACHE_PERIOD_JITTER)
-
-            self._well_known_cache.set(server_name, None, cache_period)
-            defer.returnValue(None)
-
-        try:
             parsed_body = json.loads(body.decode('utf-8'))
             logger.info("Response from .well-known: %s", parsed_body)
             if not isinstance(parsed_body, dict):
@@ -320,7 +332,13 @@ class MatrixFederationAgent(object):
             if "m.server" not in parsed_body:
                 raise Exception("Missing key 'm.server'")
         except Exception as e:
-            raise Exception("invalid .well-known response from %s: %s" % (uri_str, e,))
+            logger.info("Error fetching %s: %s", uri_str, e)
+
+            # add some randomness to the TTL to avoid a stampeding herd every hour
+            # after startup
+            cache_period = WELL_KNOWN_INVALID_CACHE_PERIOD
+            cache_period += random.uniform(0, WELL_KNOWN_DEFAULT_CACHE_PERIOD_JITTER)
+            defer.returnValue((None, cache_period))
 
         result = parsed_body["m.server"].encode("ascii")
 
@@ -336,10 +354,20 @@ class MatrixFederationAgent(object):
         else:
             cache_period = min(cache_period, WELL_KNOWN_MAX_CACHE_PERIOD)
 
-        if cache_period > 0:
-            self._well_known_cache.set(server_name, result, cache_period)
+        defer.returnValue((result, cache_period))
 
-        defer.returnValue(result)
+
+@implementer(IStreamClientEndpoint)
+class LoggingHostnameEndpoint(object):
+    """A wrapper for HostnameEndpint which logs when it connects"""
+    def __init__(self, reactor, host, port, *args, **kwargs):
+        self.host = host
+        self.port = port
+        self.ep = HostnameEndpoint(reactor, host, port, *args, **kwargs)
+
+    def connect(self, protocol_factory):
+        logger.info("Connecting to %s:%i", self.host.decode("ascii"), self.port)
+        return self.ep.connect(protocol_factory)
 
 
 def _cache_period_from_headers(headers, time_now=time.time):

synapse/rest/client/v2_alpha/groups.py

@@ -190,7 +190,7 @@ class GroupCategoriesServlet(RestServlet):
     """Get all group categories
     """
     PATTERNS = client_v2_patterns(
-        "/groups/(?P<group_id>[^/]*)/categories/$"
+        "/groups/(?P<group_id>[^/]*)/categories$"
     )
 
     def __init__(self, hs):

synapse/storage/events_worker.py

@@ -161,6 +161,12 @@ class EventsWorkerStore(SQLBaseStore):
             log_ctx = LoggingContext.current_context()
             log_ctx.record_event_fetch(len(missing_events_ids))
 
+            # Note that _enqueue_events is also responsible for turning db rows
+            # into FrozenEvents (via _get_event_from_row), which involves seeing if
+            # the events have been redacted, and if so pulling the redaction event out
+            # of the database to check it.
+            #
+            # _enqueue_events is a bit of a rubbish name but naming is hard.
             missing_events = yield self._enqueue_events(
                 missing_events_ids,
                 allow_rejected=allow_rejected,
@@ -179,14 +185,35 @@ class EventsWorkerStore(SQLBaseStore):
             # instead.
             if not allow_rejected and entry.event.type == EventTypes.Redaction:
                 if entry.event.internal_metadata.need_to_check_redaction():
-                    orig = yield self.get_event(
-                        entry.event.redacts,
+                    # XXX: we need to avoid calling get_event here.
+                    #
+                    # The problem is that we end up at this point when an event
+                    # which has been redacted is pulled out of the database by
+                    # _enqueue_events, because _enqueue_events needs to check the
+                    # redaction before it can cache the redacted event. So obviously,
+                    # calling get_event to get the redacted event out of the database
+                    # gives us an infinite loop.
+                    #
+                    # For now (quick hack to fix during 0.99 release cycle), we just
+                    # go and fetch the relevant row from the db, but it would be nice
+                    # to think about how we can cache this rather than hit the db
+                    # every time we access a redaction event.
+                    #
+                    # One thought on how to do this:
+                    #  1. split _get_events up so that it is divided into (a) get the
+                    #     rawish event from the db/cache, (b) do the redaction/rejection
+                    #     filtering
+                    #  2. have _get_event_from_row just call the first half of that
+
+                    orig_sender = yield self._simple_select_one_onecol(
+                        table="events",
+                        keyvalues={"event_id": entry.event.redacts},
+                        retcol="sender",
                         allow_none=True,
-                        allow_rejected=True,
-                        get_prev_content=False,
                     )
+
                     expected_domain = get_domain_from_id(entry.event.sender)
-                    if orig and get_domain_from_id(orig.sender) == expected_domain:
+                    if orig_sender and get_domain_from_id(orig_sender) == expected_domain:
                         # This redaction event is allowed. Mark as not needing a
                         # recheck.
                         entry.event.internal_metadata.recheck_redaction = False

tests/http/federation/test_matrix_federation_agent.py

@@ -124,7 +124,7 @@ class MatrixFederationAgentTests(TestCase):
                 _check_logcontext(context)
 
     def _handle_well_known_connection(
-        self, client_factory, expected_sni, target_server, response_headers={},
+        self, client_factory, expected_sni, content, response_headers={},
     ):
         """Handle an outgoing HTTPs connection: wire it up to a server, check that the
         request is for a .well-known, and send the response.
@@ -132,8 +132,7 @@ class MatrixFederationAgentTests(TestCase):
         Args:
             client_factory (IProtocolFactory): outgoing connection
             expected_sni (bytes): SNI that we expect the outgoing connection to send
-            target_server (bytes): target server that we should redirect to in the
-                .well-known response.
+            content (bytes): content to send back as the .well-known
         Returns:
             HTTPChannel: server impl
         """
@@ -145,10 +144,10 @@ class MatrixFederationAgentTests(TestCase):
         # check the .well-known request and send a response
         self.assertEqual(len(well_known_server.requests), 1)
         request = well_known_server.requests[0]
-        self._send_well_known_response(request, target_server, headers=response_headers)
+        self._send_well_known_response(request, content, headers=response_headers)
         return well_known_server
 
-    def _send_well_known_response(self, request, target_server, headers={}):
+    def _send_well_known_response(self, request, content, headers={}):
         """Check that an incoming request looks like a valid .well-known request, and
         send back the response.
         """
@@ -161,7 +160,7 @@ class MatrixFederationAgentTests(TestCase):
         # send back a response
         for k, v in headers.items():
             request.setHeader(k, v)
-        request.write(b'{ "m.server": "%s" }' % (target_server,))
+        request.write(content)
         request.finish()
 
         self.reactor.pump((0.1, ))
@@ -358,9 +357,8 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
+        # No SRV record lookup yet
+        self.mock_resolver.resolve_service.assert_not_called()
 
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
@@ -376,6 +374,11 @@ class MatrixFederationAgentTests(TestCase):
         # .well-known request fails.
         self.reactor.pump((0.4,))
 
+        # now there should be a SRV lookup
+        self.mock_resolver.resolve_service.assert_called_once_with(
+            b"_matrix._tcp.testserv",
+        )
+
         # we should fall back to a direct connection
         self.assertEqual(len(clients), 2)
         (host, port, client_factory, _timeout, _bindAddress) = clients[1]
@@ -403,8 +406,7 @@ class MatrixFederationAgentTests(TestCase):
         self.successResultOf(test_d)
 
     def test_get_well_known(self):
-        """Test the behaviour when the server name has no port and no SRV record, but
-        the .well-known redirects elsewhere
+        """Test the behaviour when the .well-known delegates elsewhere
         """
 
         self.mock_resolver.resolve_service.side_effect = lambda _: []
@@ -416,11 +418,6 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
-        self.mock_resolver.resolve_service.reset_mock()
-
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 1)
@@ -429,10 +426,11 @@ class MatrixFederationAgentTests(TestCase):
         self.assertEqual(port, 443)
 
         self._handle_well_known_connection(
-            client_factory, expected_sni=b"testserv", target_server=b"target-server",
+            client_factory, expected_sni=b"testserv",
+            content=b'{ "m.server": "target-server" }',
         )
 
-        # there should be another SRV lookup
+        # there should be a SRV lookup
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.target-server",
         )
@@ -483,11 +481,6 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
-        self.mock_resolver.resolve_service.reset_mock()
-
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 1)
@@ -529,7 +522,7 @@ class MatrixFederationAgentTests(TestCase):
 
         self.reactor.pump((0.1, ))
 
-        # there should be another SRV lookup
+        # there should be a SRV lookup
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.target-server",
         )
@@ -567,6 +560,64 @@ class MatrixFederationAgentTests(TestCase):
         self.well_known_cache.expire()
         self.assertNotIn(b"testserv", self.well_known_cache)
 
+    def test_get_invalid_well_known(self):
+        """
+        Test the behaviour when the server name has an *invalid* well-known (and no SRV)
+        """
+
+        self.mock_resolver.resolve_service.side_effect = lambda _: []
+        self.reactor.lookups["testserv"] = "1.2.3.4"
+
+        test_d = self._make_get_request(b"matrix://testserv/foo/bar")
+
+        # Nothing happened yet
+        self.assertNoResult(test_d)
+
+        # No SRV record lookup yet
+        self.mock_resolver.resolve_service.assert_not_called()
+
+        # there should be an attempt to connect on port 443 for the .well-known
+        clients = self.reactor.tcpClients
+        self.assertEqual(len(clients), 1)
+        (host, port, client_factory, _timeout, _bindAddress) = clients.pop()
+        self.assertEqual(host, '1.2.3.4')
+        self.assertEqual(port, 443)
+
+        self._handle_well_known_connection(
+            client_factory, expected_sni=b"testserv", content=b'NOT JSON',
+        )
+
+        # now there should be a SRV lookup
+        self.mock_resolver.resolve_service.assert_called_once_with(
+            b"_matrix._tcp.testserv",
+        )
+
+        # we should fall back to a direct connection
+        self.assertEqual(len(clients), 1)
+        (host, port, client_factory, _timeout, _bindAddress) = clients.pop()
+        self.assertEqual(host, '1.2.3.4')
+        self.assertEqual(port, 8448)
+
+        # make a test server, and wire up the client
+        http_server = self._make_connection(
+            client_factory,
+            expected_sni=b'testserv',
+        )
+
+        self.assertEqual(len(http_server.requests), 1)
+        request = http_server.requests[0]
+        self.assertEqual(request.method, b'GET')
+        self.assertEqual(request.path, b'/foo/bar')
+        self.assertEqual(
+            request.requestHeaders.getRawHeaders(b'host'),
+            [b'testserv'],
+        )
+
+        # finish the request
+        request.finish()
+        self.reactor.pump((0.1,))
+        self.successResultOf(test_d)
+
     def test_get_hostname_srv(self):
         """
         Test the behaviour when there is a single SRV record
@@ -581,6 +632,7 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
+        # the request for a .well-known will have failed with a DNS lookup error.
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.testserv",
         )
@@ -613,11 +665,9 @@ class MatrixFederationAgentTests(TestCase):
         self.successResultOf(test_d)
 
     def test_get_well_known_srv(self):
-        """Test the behaviour when the server name has no port and no SRV record, but
-        the .well-known redirects to a place where there is a SRV.
+        """Test the behaviour when the .well-known redirects to a place where there
+        is a SRV.
         """
-
-        self.mock_resolver.resolve_service.side_effect = lambda _: []
         self.reactor.lookups["testserv"] = "1.2.3.4"
         self.reactor.lookups["srvtarget"] = "5.6.7.8"
 
@@ -626,11 +676,6 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
-        self.mock_resolver.resolve_service.reset_mock()
-
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 1)
@@ -643,10 +688,11 @@ class MatrixFederationAgentTests(TestCase):
         ]
 
         self._handle_well_known_connection(
-            client_factory, expected_sni=b"testserv", target_server=b"target-server",
+            client_factory, expected_sni=b"testserv",
+            content=b'{ "m.server": "target-server" }',
         )
 
-        # there should be another SRV lookup
+        # there should be a SRV lookup
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.target-server",
         )
@@ -691,9 +737,8 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.xn--bcher-kva.com",
-        )
+        # No SRV record lookup yet
+        self.mock_resolver.resolve_service.assert_not_called()
 
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
@@ -709,6 +754,11 @@ class MatrixFederationAgentTests(TestCase):
         # .well-known request fails.
         self.reactor.pump((0.4,))
 
+        # now there should have been a SRV lookup
+        self.mock_resolver.resolve_service.assert_called_once_with(
+            b"_matrix._tcp.xn--bcher-kva.com",
+        )
+
         # We should fall back to port 8448
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 2)
@@ -806,7 +856,7 @@ class MatrixFederationAgentTests(TestCase):
             client_factory,
             expected_sni=b"testserv",
             response_headers={b'Cache-Control': b'max-age=10'},
-            target_server=b"target-server",
+            content=b'{ "m.server": "target-server" }',
         )
 
         r = self.successResultOf(fetch_d)
@@ -834,7 +884,7 @@ class MatrixFederationAgentTests(TestCase):
         self._handle_well_known_connection(
             client_factory,
             expected_sni=b"testserv",
-            target_server=b"other-server",
+            content=b'{ "m.server": "other-server" }',
         )
 
         r = self.successResultOf(fetch_d)