Treat an invalid .well-known the same as an absent one

... basically, carry on and fall back to SRV etc.

3 files changed, 77 insertions, 30 deletions

diff --git a/changelog.d/4544.misc b/changelog.d/4544.misc
new file mode 100644
index 0000000000..b29fc8575c
--- /dev/null
+++ b/changelog.d/4544.misc
@@ -0,0 +1 @@
+Treat an invalid .well-known file the same as an absent one
\ No newline at end of file
diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py
index 50baa8bf0a..c4312a3653 100644
--- a/synapse/http/federation/matrix_federation_agent.py
+++ b/synapse/http/federation/matrix_federation_agent.py
@@ -47,9 +47,6 @@ WELL_KNOWN_INVALID_CACHE_PERIOD = 1 * 3600
 # cap for .well-known cache period
 WELL_KNOWN_MAX_CACHE_PERIOD = 48 * 3600
 
-# magic value to mark an invalid well-known
-INVALID_WELL_KNOWN = object()
-
 logger = logging.getLogger(__name__)
 well_known_cache = TTLCache('well-known')
 
@@ -108,8 +105,7 @@ class MatrixFederationAgent(object):
         # 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 .well-known here
-        #   INVALID_WELL_KNWOWN: the .well-known here is invalid
+        #   `None`:      there is no (valid) .well-known here
         self._well_known_cache = _well_known_cache
 
     @defer.inlineCallbacks
@@ -302,9 +298,6 @@ class MatrixFederationAgent(object):
             if cache_period > 0:
                 self._well_known_cache.set(server_name, result, cache_period)
 
-        if result == INVALID_WELL_KNOWN:
-            raise Exception("invalid .well-known on this server")
-
         defer.returnValue(result)
 
     @defer.inlineCallbacks
@@ -331,16 +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)
-            defer.returnValue((None, cache_period))
 
-        try:
             parsed_body = json.loads(body.decode('utf-8'))
             logger.info("Response from .well-known: %s", parsed_body)
             if not isinstance(parsed_body, dict):
@@ -348,10 +332,13 @@ class MatrixFederationAgent(object):
             if "m.server" not in parsed_body:
                 raise Exception("Missing key 'm.server'")
         except Exception as e:
-            logger.info("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((INVALID_WELL_KNOWN, cache_period))
+            defer.returnValue((None, cache_period))
 
         result = parsed_body["m.server"].encode("ascii")
 
diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py
index 369e63ecc6..dcf184d3cf 100644
--- a/tests/http/federation/test_matrix_federation_agent.py
+++ b/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, ))
@@ -407,7 +406,7 @@ class MatrixFederationAgentTests(TestCase):
         self.successResultOf(test_d)
 
     def test_get_well_known(self):
-        """Test the behaviour when the .well-known redirects elsewhere
+        """Test the behaviour when the .well-known delegates elsewhere
         """
 
         self.mock_resolver.resolve_service.side_effect = lambda _: []
@@ -427,7 +426,8 @@ 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 a SRV lookup
@@ -560,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
@@ -630,7 +688,8 @@ 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 a SRV lookup
@@ -797,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)
@@ -825,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)