From 51958df766667a75236764c9df57cfd60d57bd5e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 27 Jan 2019 23:24:17 +0000 Subject: [PATCH 1/4] MatrixFederationAgent: factor out routing logic This is going to get too big and unmanageable. --- .../federation/matrix_federation_agent.py | 80 ++++++++++++++----- 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 9526f39cc..2a8bceed9 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -14,6 +14,7 @@ # limitations under the License. import logging +import attr from zope.interface import implementer from twisted.internet import defer @@ -85,9 +86,11 @@ class MatrixFederationAgent(object): response from being received (including problems that prevent the request from being sent). """ - parsed_uri = URI.fromBytes(uri, defaultPort=-1) + res = yield self._route_matrix_uri(parsed_uri) + # set up the TLS connection params + # # XXX disabling TLS is really only supported here for the benefit of the # unit tests. We should make the UTs cope with TLS rather than having to make # the code support the unit tests. @@ -95,22 +98,9 @@ class MatrixFederationAgent(object): tls_options = None else: tls_options = self._tls_client_options_factory.get_options( - parsed_uri.host.decode("ascii") + res.tls_server_name.decode("ascii") ) - if parsed_uri.port != -1: - # there was an explicit port in the URI - target = parsed_uri.host, parsed_uri.port - else: - service_name = b"_matrix._tcp.%s" % (parsed_uri.host, ) - server_list = yield self._srv_resolver.resolve_service(service_name) - if not server_list: - target = (parsed_uri.host, 8448) - logger.debug( - "No SRV record for %s, using %s", service_name, target) - else: - target = pick_server_from_list(server_list) - # make sure that the Host header is set correctly if headers is None: headers = Headers() @@ -118,13 +108,13 @@ class MatrixFederationAgent(object): headers = headers.copy() if not headers.hasHeader(b'host'): - headers.addRawHeader(b'host', parsed_uri.netloc) + headers.addRawHeader(b'host', res.host_header) class EndpointFactory(object): @staticmethod def endpointForURI(_uri): - logger.info("Connecting to %s:%s", target[0], target[1]) - ep = HostnameEndpoint(self._reactor, host=target[0], port=target[1]) + logger.info("Connecting to %s:%s", 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 @@ -134,3 +124,57 @@ class MatrixFederationAgent(object): agent.request(method, uri, headers, bodyProducer) ) defer.returnValue(res) + + @defer.inlineCallbacks + def _route_matrix_uri(self, parsed_uri): + """Helper for `request`: determine the routing for a Matrix URI + + Args: + parsed_uri (twisted.web.client.URI): uri to route. Note that it should be + parsed with URI.fromBytes(uri, defaultPort=-1) to set the `port` to -1 + if there is no explicit port given. + + Returns: + Deferred[_RoutingResult] + """ + if parsed_uri.port != -1: + # there is an explicit port + defer.returnValue(_RoutingResult( + host_header=parsed_uri.netloc, + tls_server_name=parsed_uri.host, + target_host=parsed_uri.host, + 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: + target_host = parsed_uri.host + port = 8448 + logger.debug( + "No SRV record for %s, using %s:%i", + parsed_uri.host.decode("ascii"), target_host.decode("ascii"), port, + ) + else: + target_host, port = pick_server_from_list(server_list) + logger.debug( + "Picked %s:%i from SRV records for %s", + target_host.decode("ascii"), port, parsed_uri.host.decode("ascii"), + ) + + defer.returnValue(_RoutingResult( + host_header=parsed_uri.netloc, + tls_server_name=parsed_uri.host, + target_host=target_host, + target_port=port, + )) + + +@attr.s +class _RoutingResult(object): + host_header = attr.ib() + tls_server_name = attr.ib() + target_host = attr.ib() + target_port = attr.ib() From 0fd5b3b53e312a48d98afec27ff3686d8ffce199 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sat, 26 Jan 2019 21:48:50 +0000 Subject: [PATCH 2/4] Handle IP literals explicitly We don't want to be doing .well-known lookups on these guys. --- .../federation/matrix_federation_agent.py | 19 +++++++++++++++++++ .../test_matrix_federation_agent.py | 19 ++----------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 2a8bceed9..4d674cdb9 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -15,6 +15,7 @@ import logging import attr +from netaddr import IPAddress from zope.interface import implementer from twisted.internet import defer @@ -137,6 +138,24 @@ class MatrixFederationAgent(object): Returns: Deferred[_RoutingResult] """ + # check for an IP literal + try: + ip_address = IPAddress(parsed_uri.host.decode("ascii")) + except Exception: + # not an IP address + ip_address = None + + if ip_address: + port = parsed_uri.port + if port == -1: + port = 8448 + defer.returnValue(_RoutingResult( + host_header=parsed_uri.netloc, + tls_server_name=parsed_uri.host, + target_host=parsed_uri.host, + target_port=port, + )) + if parsed_uri.port != -1: # there is an explicit port defer.returnValue(_RoutingResult( diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index f144092a5..8257594fb 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -166,11 +166,7 @@ class MatrixFederationAgentTests(TestCase): """ Test the behaviour when the server name contains an explicit IP (with no port) """ - - # the SRV lookup will return an empty list (XXX: why do we even do an SRV lookup?) - self.mock_resolver.resolve_service.side_effect = lambda _: [] - - # then there will be a getaddrinfo on the IP + # there will be a getaddrinfo on the IP self.reactor.lookups["1.2.3.4"] = "1.2.3.4" test_d = self._make_get_request(b"matrix://1.2.3.4/foo/bar") @@ -178,10 +174,6 @@ class MatrixFederationAgentTests(TestCase): # Nothing happened yet self.assertNoResult(test_d) - self.mock_resolver.resolve_service.assert_called_once_with( - b"_matrix._tcp.1.2.3.4", - ) - # Make sure treq is trying to connect clients = self.reactor.tcpClients self.assertEqual(len(clients), 1) @@ -215,10 +207,7 @@ class MatrixFederationAgentTests(TestCase): (with no port) """ - # the SRV lookup will return an empty list (XXX: why do we even do an SRV lookup?) - self.mock_resolver.resolve_service.side_effect = lambda _: [] - - # then there will be a getaddrinfo on the IP + # there will be a getaddrinfo on the IP self.reactor.lookups["::1"] = "::1" test_d = self._make_get_request(b"matrix://[::1]/foo/bar") @@ -226,10 +215,6 @@ class MatrixFederationAgentTests(TestCase): # Nothing happened yet self.assertNoResult(test_d) - self.mock_resolver.resolve_service.assert_called_once_with( - b"_matrix._tcp.::1", - ) - # Make sure treq is trying to connect clients = self.reactor.tcpClients self.assertEqual(len(clients), 1) From ff05ad147a2b62da9a4b7f00c8e8c298da06fa52 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 27 Jan 2019 23:54:50 +0000 Subject: [PATCH 3/4] changelog --- changelog.d/4488.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4488.feature diff --git a/changelog.d/4488.feature b/changelog.d/4488.feature new file mode 100644 index 000000000..bda713adf --- /dev/null +++ b/changelog.d/4488.feature @@ -0,0 +1 @@ +Implement MSC1708 (.well-known routing for server-server federation) \ No newline at end of file From 3bd0f1a4a3b735794f4a19d352b36c2e86e86dc0 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 28 Jan 2019 12:43:09 +0000 Subject: [PATCH 4/4] docstrings for _RoutingResult --- .../federation/matrix_federation_agent.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 4d674cdb9..4a6f634c8 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -193,7 +193,43 @@ class MatrixFederationAgent(object): @attr.s class _RoutingResult(object): + """The result returned by `_route_matrix_uri`. + + Contains the parameters needed to direct a federation connection to a particular + server. + + Where a SRV record points to several servers, this object contains a single server + chosen from the list. + """ + host_header = attr.ib() + """ + The value we should assign to the Host header (host:port from the matrix + URI, or .well-known). + + :type: bytes + """ + tls_server_name = attr.ib() + """ + The server name we should set in the SNI (typically host, without port, from the + matrix URI or .well-known) + + :type: bytes + """ + target_host = attr.ib() + """ + The hostname (or IP literal) we should route the TCP connection to (the target of the + SRV record, or the hostname from the URL/.well-known) + + :type: bytes + """ + target_port = attr.ib() + """ + The port we should route the TCP connection to (the target of the SRV record, or + the port from the URL/.well-known, or 8448) + + :type: int + """