From 85081fa7159c9cf53e91aab2ffb2e319d6f8f627 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 11 Jan 2021 19:21:46 +0100 Subject: [PATCH 01/28] support federation queries through http connect proxy Can be specified by HTTPS_PROXY env var. pass unfiltered reactor to federation agent for proxy support Signed-off-by: Marcus Hoffmann --- changelog.d/9306.feature | 1 + .../federation/matrix_federation_agent.py | 48 +++++++- synapse/http/matrixfederationclient.py | 1 + synapse/http/proxyagent.py | 6 +- .../test_matrix_federation_agent.py | 106 ++++++++++++++---- 5 files changed, 136 insertions(+), 26 deletions(-) create mode 100644 changelog.d/9306.feature diff --git a/changelog.d/9306.feature b/changelog.d/9306.feature new file mode 100644 index 000000000000..cf4d811b1cdc --- /dev/null +++ b/changelog.d/9306.feature @@ -0,0 +1 @@ +Add support for sending federation requests through a proxy. Contributed by @Bubu. diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 950770201a79..0e0ce98c1b05 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -14,6 +14,7 @@ import logging import urllib.parse from typing import Any, Generator, List, Optional +from urllib.request import getproxies, proxy_bypass from netaddr import AddrFormatError, IPAddress, IPSet from zope.interface import implementer @@ -30,9 +31,12 @@ from twisted.web.iweb import IAgent, IAgentEndpointFactory, IBodyProducer from synapse.crypto.context_factory import FederationPolicyForHTTPS +from synapse.http import proxyagent from synapse.http.client import BlacklistingAgentWrapper +from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint from synapse.http.federation.srv_resolver import Server, SrvResolver from synapse.http.federation.well_known_resolver import WellKnownResolver +from synapse.http.proxyagent import ProxyAgent from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.types import ISynapseReactor from synapse.util import Clock @@ -72,6 +76,7 @@ def __init__( tls_client_options_factory: Optional[FederationPolicyForHTTPS], user_agent: bytes, ip_blacklist: IPSet, + proxy_reactor: IReactorCore = None, _srv_resolver: Optional[SrvResolver] = None, _well_known_resolver: Optional[WellKnownResolver] = None, ): @@ -82,10 +87,22 @@ def __init__( self._pool.maxPersistentPerHost = 5 self._pool.cachedConnectionTimeout = 2 * 60 + if proxy_reactor is None: + self.proxy_reactor = reactor + else: + self.proxy_reactor = proxy_reactor + + proxies = getproxies() + https_proxy = proxies["https"].encode() if "https" in proxies else None + self._agent = Agent.usingEndpointFactory( self._reactor, MatrixHostnameEndpointFactory( - reactor, tls_client_options_factory, _srv_resolver + reactor, + self.proxy_reactor, + tls_client_options_factory, + _srv_resolver, + https_proxy, ), pool=self._pool, ) @@ -97,10 +114,12 @@ def __init__( _well_known_resolver = WellKnownResolver( self._reactor, agent=BlacklistingAgentWrapper( - Agent( + ProxyAgent( self._reactor, + self.proxy_reactor, pool=self._pool, contextFactory=tls_client_options_factory, + use_proxy=True, ), ip_blacklist=ip_blacklist, ), @@ -200,8 +219,10 @@ class MatrixHostnameEndpointFactory: def __init__( self, reactor: IReactorCore, + proxy_reactor: IReactorCore, tls_client_options_factory: Optional[FederationPolicyForHTTPS], srv_resolver: Optional[SrvResolver], + https_proxy: Optional[bytes] = None, ): self._reactor = reactor self._tls_client_options_factory = tls_client_options_factory @@ -210,6 +231,9 @@ def __init__( srv_resolver = SrvResolver() self._srv_resolver = srv_resolver + self.https_proxy_endpoint = proxyagent.http_proxy_endpoint( + https_proxy, proxy_reactor + ) def endpointForURI(self, parsed_uri): return MatrixHostnameEndpoint( @@ -217,6 +241,7 @@ def endpointForURI(self, parsed_uri): self._tls_client_options_factory, self._srv_resolver, parsed_uri, + self.https_proxy_endpoint, ) @@ -239,11 +264,14 @@ def __init__( tls_client_options_factory: Optional[FederationPolicyForHTTPS], srv_resolver: SrvResolver, parsed_uri: URI, + https_proxy_endpoint: Optional[IStreamClientEndpoint], ): self._reactor = reactor self._parsed_uri = parsed_uri + self.https_proxy_endpoint = https_proxy_endpoint + # set up the TLS connection params # # XXX disabling TLS is really only supported here for the benefit of the @@ -274,8 +302,20 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: port = server.port try: - logger.debug("Connecting to %s:%i", host.decode("ascii"), port) - endpoint = HostnameEndpoint(self._reactor, host, port) + if self.https_proxy_endpoint and not proxy_bypass(host.decode()): + logger.debug( + "Connecting to %s:%i via %s", + host.decode("ascii"), + port, + self.https_proxy_endpoint, + ) + endpoint = HTTPConnectProxyEndpoint( + self._reactor, self.https_proxy_endpoint, host, port + ) + else: + logger.debug("Connecting to %s:%i", host.decode("ascii"), port) + # not using a proxy + endpoint = HostnameEndpoint(self._reactor, host, port) if self._tls_options: endpoint = wrapClientTLS(self._tls_options, endpoint) result = await make_deferred_yieldable( diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index bb837b7b1979..f17868ddb35e 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -279,6 +279,7 @@ def __init__(self, hs, tls_client_options_factory): tls_client_options_factory, user_agent, hs.config.federation_ip_range_blacklist, + proxy_reactor=hs.get_reactor(), ) # Use a BlacklistingAgentWrapper to prevent circumventing the IP diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 7dfae8b786b9..47c3a1d24e87 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -120,11 +120,11 @@ def __init__( # Parse credentials from https proxy connection string if present self.https_proxy_creds, https_proxy = parse_username_password(https_proxy) - self.http_proxy_endpoint = _http_proxy_endpoint( + self.http_proxy_endpoint = http_proxy_endpoint( http_proxy, self.proxy_reactor, **self._endpoint_kwargs ) - self.https_proxy_endpoint = _http_proxy_endpoint( + self.https_proxy_endpoint = http_proxy_endpoint( https_proxy, self.proxy_reactor, **self._endpoint_kwargs ) @@ -243,7 +243,7 @@ def request(self, method, uri, headers=None, bodyProducer=None): ) -def _http_proxy_endpoint(proxy: Optional[bytes], reactor, **kwargs): +def http_proxy_endpoint(proxy: Optional[bytes], reactor, **kwargs): """Parses an http proxy setting and returns an endpoint for the proxy Args: diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index e45980316b6d..719320e0e5d9 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- # Copyright 2019 New Vector Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -12,8 +13,10 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import Optional -from unittest.mock import Mock +import os +from unittest.mock import patch + +from mock import Mock import treq from netaddr import IPSet @@ -109,7 +112,7 @@ def setUp(self): _well_known_resolver=self.well_known_resolver, ) - def _make_connection(self, client_factory, expected_sni): + def _make_connection(self, client_factory, expected_sni=None): """Builds a test server, and completes the outgoing client connection Returns: @@ -146,12 +149,13 @@ def _make_connection(self, client_factory, expected_sni): self.reactor.pump((0.1,)) # check the SNI - server_name = server_tls_connection.get_servername() - self.assertEqual( - server_name, - expected_sni, - "Expected SNI %s but got %s" % (expected_sni, server_name), - ) + if expected_sni is not None: + server_name = server_tls_connection.get_servername() + self.assertEqual( + server_name, + expected_sni, + "Expected SNI %s but got %s" % (expected_sni, server_name), + ) return http_protocol @@ -179,11 +183,7 @@ def _make_get_request(self, uri): _check_logcontext(context) def _handle_well_known_connection( - self, - client_factory, - expected_sni, - content, - response_headers: Optional[dict] = None, + 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. @@ -205,12 +205,10 @@ def _handle_well_known_connection( self.assertEqual( request.requestHeaders.getRawHeaders(b"user-agent"), [b"test-agent"] ) - self._send_well_known_response(request, content, headers=response_headers or {}) + self._send_well_known_response(request, content, headers=response_headers) return well_known_server - def _send_well_known_response( - self, request, content, headers: Optional[dict] = None - ): + 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. """ @@ -218,7 +216,7 @@ def _send_well_known_response( self.assertEqual(request.path, b"/.well-known/matrix/server") self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [b"testserv"]) # send back a response - for k, v in (headers or {}).items(): + for k, v in headers.items(): request.setHeader(k, v) request.write(content) request.finish() @@ -282,6 +280,76 @@ def test_get(self): json = self.successResultOf(treq.json_content(response)) self.assertEqual(json, {"a": 1}) + @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) + def test_get_via_proxy(self): + """ + test for federation request through proxy + """ + # recreate the agent with patched env + self.agent = MatrixFederationAgent( + reactor=self.reactor, + tls_client_options_factory=self.tls_factory, + user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. + ip_blacklist=IPSet(), + _srv_resolver=self.mock_resolver, + _well_known_resolver=self.well_known_resolver, + ) + + self.reactor.lookups["testserv"] = "1.2.3.4" + self.reactor.lookups["proxy.com"] = "9.9.9.9" + test_d = self._make_get_request(b"matrix://testserv:8448/foo/bar") + + # Nothing happened yet + self.assertNoResult(test_d) + + # Make sure treq is trying to connect + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (host, port, client_factory, _timeout, _bindAddress) = clients[0] + # make sure we are connecting to the proxy + self.assertEqual(host, "9.9.9.9") + self.assertEqual(port, 1080) + + # make a test server, and wire up the client + http_server = self._make_connection(client_factory) + + 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:8448"] + ) + self.assertEqual( + request.requestHeaders.getRawHeaders(b"user-agent"), [b"test-agent"] + ) + content = request.content.read() + self.assertEqual(content, b"") + + # Deferred is still without a result + self.assertNoResult(test_d) + + # send the headers + request.responseHeaders.setRawHeaders(b"Content-Type", [b"application/json"]) + request.write("") + + self.reactor.pump((0.1,)) + + response = self.successResultOf(test_d) + + # that should give us a Response object + self.assertEqual(response.code, 200) + + # Send the body + request.write('{ "a": 1 }'.encode("ascii")) + request.finish() + + self.reactor.pump((0.1,)) + + # check it can be read + json = self.successResultOf(treq.json_content(response)) + self.assertEqual(json, {"a": 1}) + def test_get_ip_address(self): """ Test the behaviour when the server name contains an explicit IP (with no port) From 71155676f4d661a52b038be1ad724b401ae2f099 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 26 Apr 2021 21:14:48 +0200 Subject: [PATCH 02/28] HTTPConnectProxyEndpoint now want a headers parameter This is used for proxy authentication, we just continue our "solution" of copying code from proxyagent -> matrix_federation_agent. --- .../federation/matrix_federation_agent.py | 21 ++++++++++++++++--- synapse/http/proxyagent.py | 2 +- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 0e0ce98c1b05..222ab273ca03 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -36,7 +36,7 @@ from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint from synapse.http.federation.srv_resolver import Server, SrvResolver from synapse.http.federation.well_known_resolver import WellKnownResolver -from synapse.http.proxyagent import ProxyAgent +from synapse.http.proxyagent import ProxyAgent, parse_username_password from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.types import ISynapseReactor from synapse.util import Clock @@ -76,7 +76,7 @@ def __init__( tls_client_options_factory: Optional[FederationPolicyForHTTPS], user_agent: bytes, ip_blacklist: IPSet, - proxy_reactor: IReactorCore = None, + proxy_reactor: Optional[ISynapseReactor] = None, _srv_resolver: Optional[SrvResolver] = None, _well_known_resolver: Optional[WellKnownResolver] = None, ): @@ -231,6 +231,8 @@ def __init__( srv_resolver = SrvResolver() self._srv_resolver = srv_resolver + self.https_proxy_creds, https_proxy = parse_username_password(https_proxy) + self.https_proxy_endpoint = proxyagent.http_proxy_endpoint( https_proxy, proxy_reactor ) @@ -242,6 +244,7 @@ def endpointForURI(self, parsed_uri): self._srv_resolver, parsed_uri, self.https_proxy_endpoint, + self.https_proxy_creds, ) @@ -265,6 +268,7 @@ def __init__( srv_resolver: SrvResolver, parsed_uri: URI, https_proxy_endpoint: Optional[IStreamClientEndpoint], + https_proxy_creds, ): self._reactor = reactor @@ -272,6 +276,8 @@ def __init__( self.https_proxy_endpoint = https_proxy_endpoint + self.https_proxy_creds = https_proxy_creds + # set up the TLS connection params # # XXX disabling TLS is really only supported here for the benefit of the @@ -309,8 +315,17 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: port, self.https_proxy_endpoint, ) + connect_headers = Headers() + # Determine whether we need to set Proxy-Authorization headers + if self.https_proxy_creds: + # Set a Proxy-Authorization header + connect_headers.addRawHeader( + b"Proxy-Authorization", + self.https_proxy_creds.as_proxy_authorization_value(), + ) + endpoint = HTTPConnectProxyEndpoint( - self._reactor, self.https_proxy_endpoint, host, port + self._reactor, self.https_proxy_endpoint, host, port, headers=connect_headers ) else: logger.debug("Connecting to %s:%i", host.decode("ascii"), port) diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 47c3a1d24e87..e56eabea40f7 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -267,7 +267,7 @@ def http_proxy_endpoint(proxy: Optional[bytes], reactor, **kwargs): return HostnameEndpoint(reactor, host, port, **kwargs) -def parse_username_password(proxy: bytes) -> Tuple[Optional[ProxyCredentials], bytes]: +def parse_username_password(proxy: Optional[bytes]) -> Tuple[Optional[ProxyCredentials], bytes]: """ Parses the username and password from a proxy declaration e.g username:password@hostname:port. From ef8792a750076a4d74fb61820ad9480e4e5620d9 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Tue, 27 Apr 2021 18:49:51 +0200 Subject: [PATCH 03/28] make mypy happy --- synapse/http/federation/matrix_federation_agent.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 222ab273ca03..ddb58be5da08 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -307,6 +307,7 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: host = server.host port = server.port + endpoint: IStreamClientEndpoint try: if self.https_proxy_endpoint and not proxy_bypass(host.decode()): logger.debug( From 781da361a1c2e41aca5b1ce137c6239b9cd0c7d4 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Tue, 27 Apr 2021 18:53:23 +0200 Subject: [PATCH 04/28] also make black happy :) --- synapse/http/federation/matrix_federation_agent.py | 6 +++++- synapse/http/proxyagent.py | 4 +++- tests/http/federation/test_matrix_federation_agent.py | 3 +-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index ddb58be5da08..2872cdb98d3d 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -326,7 +326,11 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: ) endpoint = HTTPConnectProxyEndpoint( - self._reactor, self.https_proxy_endpoint, host, port, headers=connect_headers + self._reactor, + self.https_proxy_endpoint, + host, + port, + headers=connect_headers, ) else: logger.debug("Connecting to %s:%i", host.decode("ascii"), port) diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index e56eabea40f7..3dadbadc3c01 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -267,7 +267,9 @@ def http_proxy_endpoint(proxy: Optional[bytes], reactor, **kwargs): return HostnameEndpoint(reactor, host, port, **kwargs) -def parse_username_password(proxy: Optional[bytes]) -> Tuple[Optional[ProxyCredentials], bytes]: +def parse_username_password( + proxy: Optional[bytes], +) -> Tuple[Optional[ProxyCredentials], bytes]: """ Parses the username and password from a proxy declaration e.g username:password@hostname:port. diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 719320e0e5d9..0fdb624068b5 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -16,9 +16,8 @@ import os from unittest.mock import patch -from mock import Mock - import treq +from mock import Mock from netaddr import IPSet from service_identity import VerificationError from zope.interface import implementer From 86b36232996e2b37edc2b6dd7cc3784e9e56c140 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 17:39:55 +0200 Subject: [PATCH 05/28] add type hints and comments to tests --- .../test_matrix_federation_agent.py | 56 ++++++++++++------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 25ea1e9e2c70..cee09a3af8fa 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- # Copyright 2019 New Vector Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -13,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from typing import Optional import os from unittest.mock import patch @@ -24,11 +24,12 @@ from twisted.internet import defer from twisted.internet._sslverify import ClientTLSOptions, OpenSSLCertificateOptions +from twisted.internet.interfaces import IProtocolFactory from twisted.internet.protocol import Factory from twisted.protocols.tls import TLSMemoryBIOFactory from twisted.web._newclient import ResponseNeverReceived from twisted.web.client import Agent -from twisted.web.http import HTTPChannel +from twisted.web.http import HTTPChannel, Request from twisted.web.http_headers import Headers from twisted.web.iweb import IPolicyForHTTPS @@ -111,11 +112,19 @@ def setUp(self): _well_known_resolver=self.well_known_resolver, ) - def _make_connection(self, client_factory, expected_sni=None): + def _make_connection( + self, client_factory: IProtocolFactory, expected_sni: bytes = None + ) -> HTTPChannel: """Builds a test server, and completes the outgoing client connection + Args: + client_factory: the the factory that the + application is trying to use to make the outbound connection. We will + invoke it to build the client Protocol + + expected_sni: the expected SNI value Returns: - HTTPChannel: the test server + the server Protocol returned by server_factory """ # build the test server @@ -144,8 +153,8 @@ def _make_connection(self, client_factory, expected_sni=None): # fish the test server back out of the server-side TLS protocol. http_protocol = server_tls_protocol.wrappedProtocol - # give the reactor a pump to get the TLS juices flowing. - self.reactor.pump((0.1,)) + # give the reactor a pump to get the TLS juices flowing (if needed) + self.reactor.advance(0) # check the SNI if expected_sni is not None: @@ -153,13 +162,13 @@ def _make_connection(self, client_factory, expected_sni=None): self.assertEqual( server_name, expected_sni, - "Expected SNI %s but got %s" % (expected_sni, server_name), + f"Expected SNI {expected_sni!s} but got {server_name!s}", ) return http_protocol @defer.inlineCallbacks - def _make_get_request(self, uri): + def _make_get_request(self, uri: bytes): """ Sends a simple GET request via the agent, and checks its logcontext management """ @@ -182,17 +191,21 @@ def _make_get_request(self, uri): _check_logcontext(context) def _handle_well_known_connection( - self, client_factory, expected_sni, content, response_headers={} - ): + self, + client_factory: IProtocolFactory, + expected_sni: bytes, + content: bytes, + response_headers: Optional[dict] = None, + ) -> HTTPChannel: """Handle an outgoing HTTPs connection: wire it up to a server, check that the request is for a .well-known, and send the response. Args: - client_factory (IProtocolFactory): outgoing connection - expected_sni (bytes): SNI that we expect the outgoing connection to send - content (bytes): content to send back as the .well-known + client_factory: outgoing connection + expected_sni: SNI that we expect the outgoing connection to send + content: content to send back as the .well-known Returns: - HTTPChannel: server impl + server impl """ # make the connection for .well-known well_known_server = self._make_connection( @@ -204,10 +217,15 @@ def _handle_well_known_connection( self.assertEqual( request.requestHeaders.getRawHeaders(b"user-agent"), [b"test-agent"] ) - self._send_well_known_response(request, content, headers=response_headers) + self._send_well_known_response(request, content, headers=response_headers or {}) return well_known_server - def _send_well_known_response(self, request, content, headers={}): + def _send_well_known_response( + self, + request: Request, + content: bytes, + headers: Optional[dict] = None, + ): """Check that an incoming request looks like a valid .well-known request, and send back the response. """ @@ -215,7 +233,7 @@ def _send_well_known_response(self, request, content, headers={}): self.assertEqual(request.path, b"/.well-known/matrix/server") self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [b"testserv"]) # send back a response - for k, v in headers.items(): + for k, v in (headers or {}).items(): request.setHeader(k, v) request.write(content) request.finish() @@ -1358,9 +1376,9 @@ def _build_test_server(connection_creator): return server_tls_factory.buildProtocol(None) -def _log_request(request): +def _log_request(request: str): """Implements Factory.log, which is expected by Request.finish""" - logger.info("Completed request %s", request) + logger.info(f"Completed request {request}") @implementer(IPolicyForHTTPS) From 3c7c3b6895c9517af58d49ec3061306c65baa80a Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 19:11:12 +0200 Subject: [PATCH 06/28] change logic of test to the same like in `test_proxyagent` As a result, each test creates its own certificates now. A little bit more overhead --- .../test_matrix_federation_agent.py | 68 +++++++++---------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index cee09a3af8fa..f13a0d1b01c2 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -12,8 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import Optional import os +from typing import Iterable, Optional from unittest.mock import patch import treq @@ -52,24 +52,6 @@ logger = logging.getLogger(__name__) -test_server_connection_factory = None - - -def get_connection_factory(): - # this needs to happen once, but not until we are ready to run the first test - global test_server_connection_factory - if test_server_connection_factory is None: - test_server_connection_factory = TestServerTLSConnectionFactory( - sanlist=[ - b"DNS:testserv", - b"DNS:target-server", - b"DNS:xn--bcher-kva.com", - b"IP:1.2.3.4", - b"IP:::1", - ] - ) - return test_server_connection_factory - # Once Async Mocks or lambdas are supported this can go away. def generate_resolve_service(result): @@ -128,7 +110,8 @@ def _make_connection( """ # build the test server - server_tls_protocol = _build_test_server(get_connection_factory()) + server_factory = _wrap_server_factory_for_tls(_get_test_protocol_factory()) + server_tls_protocol = server_factory.buildProtocol(None) # now, tell the client protocol factory to build the client protocol (it will be a # _WrappingProtocol, around a TLSMemoryBIOProtocol, around an @@ -1351,29 +1334,44 @@ def _check_logcontext(context): raise AssertionError("Expected logcontext %s but was %s" % (context, current)) -def _build_test_server(connection_creator): - """Construct a test server - - This builds an HTTP channel, wrapped with a TLSMemoryBIOProtocol - +def _wrap_server_factory_for_tls( + factory: IProtocolFactory, sanlist: Iterable[bytes] = None +) -> IProtocolFactory: + """Wrap an existing Protocol Factory with a test TLSMemoryBIOFactory + The resultant factory will create a TLS server which presents a certificate + signed by our test CA, valid for the domains in `sanlist` Args: - connection_creator (IOpenSSLServerConnectionCreator): thing to build - SSL connections - sanlist (list[bytes]): list of the SAN entries for the cert returned - by the server + factory: protocol factory to wrap + sanlist: list of domains the cert should be valid for + Returns: + interfaces.IProtocolFactory + """ + if sanlist is None: + sanlist = [ + b"DNS:testserv", + b"DNS:target-server", + b"DNS:xn--bcher-kva.com", + b"IP:1.2.3.4", + b"IP:::1", + ] + + connection_creator = TestServerTLSConnectionFactory(sanlist=sanlist) + return TLSMemoryBIOFactory( + connection_creator, isClient=False, wrappedFactory=factory + ) + +def _get_test_protocol_factory() -> IProtocolFactory: + """Get a protocol Factory which will build an HTTPChannel Returns: - TLSMemoryBIOProtocol + interfaces.IProtocolFactory """ server_factory = Factory.forProtocol(HTTPChannel) + # Request.finish expects the factory to have a 'log' method. server_factory.log = _log_request - server_tls_factory = TLSMemoryBIOFactory( - connection_creator, isClient=False, wrappedFactory=server_factory - ) - - return server_tls_factory.buildProtocol(None) + return server_factory def _log_request(request: str): From cee7f65ea57567be957294e1947a42fb076cc300 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 19:30:53 +0200 Subject: [PATCH 07/28] Add http connections to `_make_connection` for http proxies similar to `test_proxyagent` --- .../test_matrix_federation_agent.py | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index f13a0d1b01c2..03d149ed38e2 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -95,7 +95,11 @@ def setUp(self): ) def _make_connection( - self, client_factory: IProtocolFactory, expected_sni: bytes = None + self, + client_factory: IProtocolFactory, + ssl: bool = True, + expected_sni: bytes = None, + tls_sanlist: Optional[Iterable[bytes]] = None, ) -> HTTPChannel: """Builds a test server, and completes the outgoing client connection Args: @@ -103,15 +107,24 @@ def _make_connection( application is trying to use to make the outbound connection. We will invoke it to build the client Protocol + ssl: If true, we will expect an ssl connection and wrap + server_factory with a TLSMemoryBIOFactory + False is set only for connection via http_proxy + expected_sni: the expected SNI value + tls_sanlist: list of SAN entries for the TLS cert presented by the server. + Returns: the server Protocol returned by server_factory """ # build the test server - server_factory = _wrap_server_factory_for_tls(_get_test_protocol_factory()) - server_tls_protocol = server_factory.buildProtocol(None) + server_factory = _get_test_protocol_factory() + if ssl: + server_factory = _wrap_server_factory_for_tls(server_factory, tls_sanlist) + + server_protocol = server_factory.buildProtocol(None) # now, tell the client protocol factory to build the client protocol (it will be a # _WrappingProtocol, around a TLSMemoryBIOProtocol, around an @@ -122,26 +135,29 @@ def _make_connection( # stubbing that out here. client_protocol = client_factory.buildProtocol(None) client_protocol.makeConnection( - FakeTransport(server_tls_protocol, self.reactor, client_protocol) + FakeTransport(server_protocol, self.reactor, client_protocol) ) - # tell the server tls protocol to send its stuff back to the client, too - server_tls_protocol.makeConnection( - FakeTransport(client_protocol, self.reactor, server_tls_protocol) + # tell the server protocol to send its stuff back to the client, too + server_protocol.makeConnection( + FakeTransport(client_protocol, self.reactor, server_protocol) ) - # grab a hold of the TLS connection, in case it gets torn down - server_tls_connection = server_tls_protocol._tlsConnection - - # fish the test server back out of the server-side TLS protocol. - http_protocol = server_tls_protocol.wrappedProtocol + if ssl: + # fish the test server back out of the server-side TLS protocol. + http_protocol = server_protocol.wrappedProtocol + # grab a hold of the TLS connection, in case it gets torn down + tls_connection = server_protocol._tlsConnection + else: + http_protocol = server_protocol + tls_connection = None # give the reactor a pump to get the TLS juices flowing (if needed) self.reactor.advance(0) # check the SNI if expected_sni is not None: - server_name = server_tls_connection.get_servername() + server_name = tls_connection.get_servername() self.assertEqual( server_name, expected_sni, From e8cb0879cdcf3f7af5681036837db5baa5aff6bd Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 20:51:05 +0200 Subject: [PATCH 08/28] add tests for proxy --- .../test_matrix_federation_agent.py | 117 +++++++++++++++++- 1 file changed, 113 insertions(+), 4 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 03d149ed38e2..9ed1bc972003 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import base64 import logging import os from typing import Iterable, Optional @@ -297,9 +298,50 @@ def test_get(self): self.assertEqual(json, {"a": 1}) @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) - def test_get_via_proxy(self): + def test_get_via_http_proxy(self): """ - test for federation request through proxy + test for federation request through a http proxy + """ + self._do_get_via_proxy(ssl=False, auth_credentials=None) + + @patch.dict( + os.environ, {"https_proxy": "user:pass@proxy.com", "no_proxy": "unused.com"} + ) + def test_get_via_http_proxy_with_auth(self): + """ + test for federation request through a http proxy with authentication + """ + self._do_get_via_proxy(ssl=False, auth_credentials=b"user:pass") + + @patch.dict( + os.environ, {"https_proxy": "https://proxy.com", "no_proxy": "unused.com"} + ) + def test_get_via_https_proxy(self): + """ + test for federation request through a https proxy + """ + self._do_get_via_proxy(ssl=True, auth_credentials=None) + + @patch.dict( + os.environ, + {"https_proxy": "https://user:pass@proxy.com", "no_proxy": "unused.com"}, + ) + def test_get_via_https_proxy_with_auth(self): + """ + test for federation request through a https proxy with authentication + """ + self._do_get_via_proxy(ssl=True, auth_credentials=b"user:pass") + + def _do_get_via_proxy( + self, + ssl: bool = False, + auth_credentials: Optional[bytes] = None, + ): + """Send a https federation request via an agent and check that it is correctly + received at the proxy and client. The proxy can use either http or https. + Args: + ssl: True if we expect the request to connect via https to proxy + auth_credentials: credentials to authenticate at proxy """ # recreate the agent with patched env self.agent = MatrixFederationAgent( @@ -326,10 +368,75 @@ def test_get_via_proxy(self): self.assertEqual(host, "9.9.9.9") self.assertEqual(port, 1080) - # make a test server, and wire up the client - http_server = self._make_connection(client_factory) + # make a test HTTP proxy server, and wire up the client + proxy_server = self._make_connection( + client_factory, + ssl=ssl, + tls_sanlist=[b"DNS:proxy.com"] if ssl else None, + expected_sni=b"proxy.com" if ssl else None, + ) + + # fish the transports back out so that we can do the old switcheroo + s2c_transport = proxy_server.transport + client_protocol = s2c_transport.other + c2s_transport = client_protocol.transport + # the FakeTransport is async, so we need to pump the reactor + self.reactor.advance(0) + + # now there should be a pending CONNECT request + self.assertEqual(len(proxy_server.requests), 1) + + request = proxy_server.requests[0] + self.assertEqual(request.method, b"CONNECT") + self.assertEqual(request.path, b"testserv:8448") + + # Check whether auth credentials have been supplied to the proxy + proxy_auth_header_values = request.requestHeaders.getRawHeaders( + b"Proxy-Authorization" + ) + + if auth_credentials is not None: + # Compute the correct header value for Proxy-Authorization + encoded_credentials = base64.b64encode(auth_credentials) + expected_header_value = b"Basic " + encoded_credentials + + # Validate the header's value + self.assertIn(expected_header_value, proxy_auth_header_values) + else: + # Check that the Proxy-Authorization header has not been supplied to the proxy + self.assertIsNone(proxy_auth_header_values) + + # tell the proxy server not to close the connection + proxy_server.persistent = True + + # this just stops the http Request trying to do a chunked response + # request.setHeader(b"Content-Length", b"0") + request.finish() + + # now we can replace the proxy channel with a new, SSL-wrapped HTTP channel + ssl_factory = _wrap_server_factory_for_tls(_get_test_protocol_factory()) + ssl_protocol = ssl_factory.buildProtocol(None) + http_server = ssl_protocol.wrappedProtocol + + ssl_protocol.makeConnection( + FakeTransport(client_protocol, self.reactor, ssl_protocol) + ) + c2s_transport.other = ssl_protocol + + self.reactor.advance(0) + + server_name = ssl_protocol._tlsConnection.get_servername() + expected_sni = b"testserv" + self.assertEqual( + server_name, + expected_sni, + f"Expected SNI {expected_sni!s} but got {server_name!s}", + ) + + # now there should be a pending request 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") @@ -339,6 +446,8 @@ def test_get_via_proxy(self): self.assertEqual( request.requestHeaders.getRawHeaders(b"user-agent"), [b"test-agent"] ) + # Check that the destination server DID NOT receive proxy credentials + self.assertIsNone(request.requestHeaders.getRawHeaders(b"Proxy-Authorization")) content = request.content.read() self.assertEqual(content, b"") From 73c5ff5f80f5b2dceda8d5be595d1af72f7dc15d Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 21:03:06 +0200 Subject: [PATCH 09/28] add test for bypass proxy --- .../test_matrix_federation_agent.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 9ed1bc972003..fce88074b753 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -244,6 +244,32 @@ def test_get(self): """ happy-path test of a GET request with an explicit port """ + self._do_get() + + @patch.dict( + os.environ, + {"https_proxy": "proxy.com", "no_proxy": "testserv"}, + ) + def test_get_bypass_proxy(self): + """ + test of a GET request with an explicit port and bypass proxy + """ + self._do_get() + + def _do_get(self): + """ + test of a GET request with an explicit port + """ + # recreate the agent with patched env + self.agent = MatrixFederationAgent( + reactor=self.reactor, + tls_client_options_factory=self.tls_factory, + user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. + ip_blacklist=IPSet(), + _srv_resolver=self.mock_resolver, + _well_known_resolver=self.well_known_resolver, + ) + self.reactor.lookups["testserv"] = "1.2.3.4" test_d = self._make_get_request(b"matrix://testserv:8448/foo/bar") From 4cd4a488458e24483fb6405e8b4004fe0cb1575b Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 21:20:48 +0200 Subject: [PATCH 10/28] put creation of `MatrixFederationAgent` in own function --- .../test_matrix_federation_agent.py | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index fce88074b753..db2b88b4a968 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -86,14 +86,7 @@ def setUp(self): had_well_known_cache=self.had_well_known_cache, ) - self.agent = MatrixFederationAgent( - reactor=self.reactor, - tls_client_options_factory=self.tls_factory, - user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. - ip_blacklist=IPSet(), - _srv_resolver=self.mock_resolver, - _well_known_resolver=self.well_known_resolver, - ) + self.agent = self._make_agent() def _make_connection( self, @@ -240,6 +233,20 @@ def _send_well_known_response( self.reactor.pump((0.1,)) + def _make_agent(self) -> MatrixFederationAgent: + """ + If a proxy server is set, the MatrixFederationAgent must be created again + because it is created too early during setUp + """ + return MatrixFederationAgent( + reactor=self.reactor, + tls_client_options_factory=self.tls_factory, + user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. + ip_blacklist=IPSet(), + _srv_resolver=self.mock_resolver, + _well_known_resolver=self.well_known_resolver, + ) + def test_get(self): """ happy-path test of a GET request with an explicit port @@ -261,14 +268,7 @@ def _do_get(self): test of a GET request with an explicit port """ # recreate the agent with patched env - self.agent = MatrixFederationAgent( - reactor=self.reactor, - tls_client_options_factory=self.tls_factory, - user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. - ip_blacklist=IPSet(), - _srv_resolver=self.mock_resolver, - _well_known_resolver=self.well_known_resolver, - ) + self.agent = self._make_agent() self.reactor.lookups["testserv"] = "1.2.3.4" test_d = self._make_get_request(b"matrix://testserv:8448/foo/bar") @@ -370,14 +370,7 @@ def _do_get_via_proxy( auth_credentials: credentials to authenticate at proxy """ # recreate the agent with patched env - self.agent = MatrixFederationAgent( - reactor=self.reactor, - tls_client_options_factory=self.tls_factory, - user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. - ip_blacklist=IPSet(), - _srv_resolver=self.mock_resolver, - _well_known_resolver=self.well_known_resolver, - ) + self.agent = self._make_agent() self.reactor.lookups["testserv"] = "1.2.3.4" self.reactor.lookups["proxy.com"] = "9.9.9.9" From 1595da4df8a41f65095f7ee75231805611212405 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 21:40:49 +0200 Subject: [PATCH 11/28] comments and type hints to federation_agent --- .../federation/matrix_federation_agent.py | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 2872cdb98d3d..529ef54b7a39 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -14,7 +14,10 @@ import logging import urllib.parse from typing import Any, Generator, List, Optional -from urllib.request import getproxies, proxy_bypass +from urllib.request import ( # type: ignore[attr-defined] + getproxies_environment, + proxy_bypass, +) from netaddr import AddrFormatError, IPAddress, IPSet from zope.interface import implementer @@ -36,7 +39,11 @@ from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint from synapse.http.federation.srv_resolver import Server, SrvResolver from synapse.http.federation.well_known_resolver import WellKnownResolver -from synapse.http.proxyagent import ProxyAgent, parse_username_password +from synapse.http.proxyagent import ( + ProxyAgent, + ProxyCredentials, + parse_username_password, +) from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.types import ISynapseReactor from synapse.util import Clock @@ -61,6 +68,12 @@ class MatrixFederationAgent: user_agent: The user agent header to use for federation requests. + ip_blacklist: Disallowed IP addresses. + + proxy_reactor: twisted reactor to use for connections to the proxy server + reactor might have some blacklisting applied (i.e. for DNS queries), + but we need unblocked access to the proxy. + _srv_resolver: SrvResolver implementation to use for looking up SRV records. None to use a default implementation. @@ -92,7 +105,8 @@ def __init__( else: self.proxy_reactor = proxy_reactor - proxies = getproxies() + # http_proxy is not needed because federation is always over TLS + proxies = getproxies_environment() https_proxy = proxies["https"].encode() if "https" in proxies else None self._agent = Agent.usingEndpointFactory( @@ -237,7 +251,7 @@ def __init__( https_proxy, proxy_reactor ) - def endpointForURI(self, parsed_uri): + def endpointForURI(self, parsed_uri: URI): return MatrixHostnameEndpoint( self._reactor, self._tls_client_options_factory, @@ -259,6 +273,8 @@ class MatrixHostnameEndpoint: factory to use for fetching client tls options, or none to disable TLS. srv_resolver: The SRV resolver to use parsed_uri: The parsed URI that we're wanting to connect to. + https_proxy_endpoint: Endpoint to use to connect to the proxy, + https_proxy_creds: Credentials to authenticate at proxy. """ def __init__( @@ -268,14 +284,12 @@ def __init__( srv_resolver: SrvResolver, parsed_uri: URI, https_proxy_endpoint: Optional[IStreamClientEndpoint], - https_proxy_creds, + https_proxy_creds: Optional[ProxyCredentials], ): self._reactor = reactor - self._parsed_uri = parsed_uri self.https_proxy_endpoint = https_proxy_endpoint - self.https_proxy_creds = https_proxy_creds # set up the TLS connection params From 5acd7ced324d853918e644e42038527cc2fefd84 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 22:20:54 +0200 Subject: [PATCH 12/28] move proxy handling to `MatrixHostnameEndpoint` similar to `ProxyAgent` --- .../federation/matrix_federation_agent.py | 46 ++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 529ef54b7a39..232d54abab3e 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -16,7 +16,7 @@ from typing import Any, Generator, List, Optional from urllib.request import ( # type: ignore[attr-defined] getproxies_environment, - proxy_bypass, + proxy_bypass_environment, ) from netaddr import AddrFormatError, IPAddress, IPSet @@ -105,10 +105,6 @@ def __init__( else: self.proxy_reactor = proxy_reactor - # http_proxy is not needed because federation is always over TLS - proxies = getproxies_environment() - https_proxy = proxies["https"].encode() if "https" in proxies else None - self._agent = Agent.usingEndpointFactory( self._reactor, MatrixHostnameEndpointFactory( @@ -116,7 +112,6 @@ def __init__( self.proxy_reactor, tls_client_options_factory, _srv_resolver, - https_proxy, ), pool=self._pool, ) @@ -236,29 +231,23 @@ def __init__( proxy_reactor: IReactorCore, tls_client_options_factory: Optional[FederationPolicyForHTTPS], srv_resolver: Optional[SrvResolver], - https_proxy: Optional[bytes] = None, ): self._reactor = reactor + self._proxy_reactor = proxy_reactor self._tls_client_options_factory = tls_client_options_factory if srv_resolver is None: srv_resolver = SrvResolver() self._srv_resolver = srv_resolver - self.https_proxy_creds, https_proxy = parse_username_password(https_proxy) - - self.https_proxy_endpoint = proxyagent.http_proxy_endpoint( - https_proxy, proxy_reactor - ) def endpointForURI(self, parsed_uri: URI): return MatrixHostnameEndpoint( self._reactor, + self._proxy_reactor, self._tls_client_options_factory, self._srv_resolver, parsed_uri, - self.https_proxy_endpoint, - self.https_proxy_creds, ) @@ -273,24 +262,32 @@ class MatrixHostnameEndpoint: factory to use for fetching client tls options, or none to disable TLS. srv_resolver: The SRV resolver to use parsed_uri: The parsed URI that we're wanting to connect to. - https_proxy_endpoint: Endpoint to use to connect to the proxy, - https_proxy_creds: Credentials to authenticate at proxy. + proxy_reactor: twisted reactor to use for connections to the proxy server + reactor might have some blacklisting applied (i.e. for DNS queries), + but we need unblocked access to the proxy. """ def __init__( self, reactor: IReactorCore, + proxy_reactor: IReactorCore, tls_client_options_factory: Optional[FederationPolicyForHTTPS], srv_resolver: SrvResolver, parsed_uri: URI, - https_proxy_endpoint: Optional[IStreamClientEndpoint], - https_proxy_creds: Optional[ProxyCredentials], ): self._reactor = reactor self._parsed_uri = parsed_uri - self.https_proxy_endpoint = https_proxy_endpoint - self.https_proxy_creds = https_proxy_creds + # http_proxy is not needed because federation is always over TLS + proxies = getproxies_environment() + https_proxy = proxies["https"].encode() if "https" in proxies else None + self.no_proxy = proxies["no"] if "no" in proxies else None + + self.https_proxy_creds, https_proxy = parse_username_password(https_proxy) + + self.https_proxy_endpoint = proxyagent.http_proxy_endpoint( + https_proxy, proxy_reactor + ) # set up the TLS connection params # @@ -321,9 +318,16 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: host = server.host port = server.port + should_skip_proxy = False + if self.no_proxy is not None: + should_skip_proxy = proxy_bypass_environment( + host.decode(), + proxies={"no": self.no_proxy}, + ) + endpoint: IStreamClientEndpoint try: - if self.https_proxy_endpoint and not proxy_bypass(host.decode()): + if self.https_proxy_endpoint and not should_skip_proxy: logger.debug( "Connecting to %s:%i via %s", host.decode("ascii"), From 3342bdae720c39852288a318ab89ef3d33f4b2db Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 22:46:56 +0200 Subject: [PATCH 13/28] newsfile --- changelog.d/{9306.feature => 10475.feature} | 2 +- synapse/http/federation/matrix_federation_agent.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) rename changelog.d/{9306.feature => 10475.feature} (66%) diff --git a/changelog.d/9306.feature b/changelog.d/10475.feature similarity index 66% rename from changelog.d/9306.feature rename to changelog.d/10475.feature index cf4d811b1cdc..52eab11b0305 100644 --- a/changelog.d/9306.feature +++ b/changelog.d/10475.feature @@ -1 +1 @@ -Add support for sending federation requests through a proxy. Contributed by @Bubu. +Add support for sending federation requests through a proxy. Contributed by @Bubu and @dklimpel. \ 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 232d54abab3e..882b5908767a 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -258,13 +258,13 @@ class MatrixHostnameEndpoint: Args: reactor: twisted reactor to use for underlying requests + proxy_reactor: twisted reactor to use for connections to the proxy server + reactor might have some blacklisting applied (i.e. for DNS queries), + but we need unblocked access to the proxy. tls_client_options_factory: factory to use for fetching client tls options, or none to disable TLS. srv_resolver: The SRV resolver to use parsed_uri: The parsed URI that we're wanting to connect to. - proxy_reactor: twisted reactor to use for connections to the proxy server - reactor might have some blacklisting applied (i.e. for DNS queries), - but we need unblocked access to the proxy. """ def __init__( From 0bf5ac8cf8ced6d019bbb0507a1f10e881f50458 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 24 Jul 2021 23:28:56 +0200 Subject: [PATCH 14/28] fix imports --- synapse/http/federation/matrix_federation_agent.py | 6 +----- tests/http/federation/test_matrix_federation_agent.py | 3 +-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 882b5908767a..235b4ba9a813 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -39,11 +39,7 @@ from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint from synapse.http.federation.srv_resolver import Server, SrvResolver from synapse.http.federation.well_known_resolver import WellKnownResolver -from synapse.http.proxyagent import ( - ProxyAgent, - ProxyCredentials, - parse_username_password, -) +from synapse.http.proxyagent import ProxyAgent, parse_username_password from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.types import ISynapseReactor from synapse.util import Clock diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index db2b88b4a968..8bcb9aa76c80 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -15,10 +15,9 @@ import logging import os from typing import Iterable, Optional -from unittest.mock import patch +from unittest.mock import Mock, patch import treq -from mock import Mock from netaddr import IPSet from service_identity import VerificationError from zope.interface import implementer From b81eaafbeafcc5374adbd3aafae8503ad6e9a9b1 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 26 Jul 2021 22:24:54 +0200 Subject: [PATCH 15/28] add upgrade notes --- docs/upgrade.md | 24 ++++++++++++++++ .../test_matrix_federation_agent.py | 28 +++++-------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index c8f4a2c17172..21e0d1933c0e 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -86,6 +86,30 @@ process, for example: ``` +# Upgrading to v1.xx.0 + +## Add support for routing outbound HTTP requests via a proxy for federation + +Since Synapse 1.6.0 (2019-11-26) you can set a proxy for outbound HTTP requests via +http_proxy/https_proxy environment variables. This proxy was set for: +- push +- url previews +- phone-home stats +- recaptcha validation +- CAS auth validation +- OpenID Connect + +In this version we have added support for outbound requests for: +- federation +- download remote media +- fetch keys from other servers +- Identity servers + +These requests use the same proxy configuration. If you have a proxy configuration we +recommend to verify the configuration. It may be necessary to adjust the `no_proxy` +environment variable. + + # Upgrading to v1.39.0 ## Deprecation of the current third-party rules module interface diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 8bcb9aa76c80..b28607d68f62 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -247,9 +247,7 @@ def _make_agent(self) -> MatrixFederationAgent: ) def test_get(self): - """ - happy-path test of a GET request with an explicit port - """ + """happy-path test of a GET request with an explicit port""" self._do_get() @patch.dict( @@ -257,15 +255,11 @@ def test_get(self): {"https_proxy": "proxy.com", "no_proxy": "testserv"}, ) def test_get_bypass_proxy(self): - """ - test of a GET request with an explicit port and bypass proxy - """ + """test of a GET request with an explicit port and bypass proxy""" self._do_get() def _do_get(self): - """ - test of a GET request with an explicit port - """ + """test of a GET request with an explicit port""" # recreate the agent with patched env self.agent = self._make_agent() @@ -324,27 +318,21 @@ def _do_get(self): @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) def test_get_via_http_proxy(self): - """ - test for federation request through a http proxy - """ + """test for federation request through a http proxy""" self._do_get_via_proxy(ssl=False, auth_credentials=None) @patch.dict( os.environ, {"https_proxy": "user:pass@proxy.com", "no_proxy": "unused.com"} ) def test_get_via_http_proxy_with_auth(self): - """ - test for federation request through a http proxy with authentication - """ + """test for federation request through a http proxy with authentication""" self._do_get_via_proxy(ssl=False, auth_credentials=b"user:pass") @patch.dict( os.environ, {"https_proxy": "https://proxy.com", "no_proxy": "unused.com"} ) def test_get_via_https_proxy(self): - """ - test for federation request through a https proxy - """ + """test for federation request through a https proxy""" self._do_get_via_proxy(ssl=True, auth_credentials=None) @patch.dict( @@ -352,9 +340,7 @@ def test_get_via_https_proxy(self): {"https_proxy": "https://user:pass@proxy.com", "no_proxy": "unused.com"}, ) def test_get_via_https_proxy_with_auth(self): - """ - test for federation request through a https proxy with authentication - """ + """test for federation request through a https proxy with authentication""" self._do_get_via_proxy(ssl=True, auth_credentials=b"user:pass") def _do_get_via_proxy( From 70a8b1120ecd9723b1bfc97a45af259675aff648 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 27 Jul 2021 21:16:05 +0200 Subject: [PATCH 16/28] fix merge error and update tests --- .../federation/matrix_federation_agent.py | 4 +- .../test_matrix_federation_agent.py | 47 +++++++++++-------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 95e68ebae174..a2824f63bbfe 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -44,7 +44,7 @@ from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint from synapse.http.federation.srv_resolver import Server, SrvResolver from synapse.http.federation.well_known_resolver import WellKnownResolver -from synapse.http.proxyagent import ProxyAgent, parse_username_password +from synapse.http.proxyagent import ProxyAgent from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.types import ISynapseReactor from synapse.util import Clock @@ -290,7 +290,7 @@ def __init__( ) = proxyagent.http_proxy_endpoint( https_proxy, proxy_reactor, - _tls_client_options_factory or BrowserLikePolicyForHTTPS(), + tls_client_options_factory or BrowserLikePolicyForHTTPS(), ) # set up the TLS connection params diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index e91702c0d66f..db09d3e6a65f 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -26,7 +26,7 @@ from twisted.internet._sslverify import ClientTLSOptions, OpenSSLCertificateOptions from twisted.internet.interfaces import IProtocolFactory from twisted.internet.protocol import Factory -from twisted.protocols.tls import TLSMemoryBIOFactory +from twisted.protocols.tls import TLSMemoryBIOFactory, TLSMemoryBIOProtocol from twisted.web._newclient import ResponseNeverReceived from twisted.web.client import Agent from twisted.web.http import HTTPChannel, Request @@ -375,7 +375,7 @@ def _do_get_via_proxy( self.assertEqual(host, "9.9.9.9") self.assertEqual(port, 1080) - # make a test HTTP proxy server, and wire up the client + # make a test server to act as the proxy, and wire up the client proxy_server = self._make_connection( client_factory, ssl=ssl, @@ -383,13 +383,7 @@ def _do_get_via_proxy( expected_sni=b"proxy.com" if ssl else None, ) - # fish the transports back out so that we can do the old switcheroo - s2c_transport = proxy_server.transport - client_protocol = s2c_transport.other - c2s_transport = client_protocol.transport - - # the FakeTransport is async, so we need to pump the reactor - self.reactor.advance(0) + assert isinstance(proxy_server, HTTPChannel) # now there should be a pending CONNECT request self.assertEqual(len(proxy_server.requests), 1) @@ -417,23 +411,35 @@ def _do_get_via_proxy( # tell the proxy server not to close the connection proxy_server.persistent = True - # this just stops the http Request trying to do a chunked response - # request.setHeader(b"Content-Length", b"0") request.finish() - # now we can replace the proxy channel with a new, SSL-wrapped HTTP channel - ssl_factory = _wrap_server_factory_for_tls(_get_test_protocol_factory()) - ssl_protocol = ssl_factory.buildProtocol(None) - http_server = ssl_protocol.wrappedProtocol + # now we make another test server to act as the upstream HTTP server. + server_ssl_protocol = _wrap_server_factory_for_tls( + _get_test_protocol_factory() + ).buildProtocol(None) - ssl_protocol.makeConnection( - FakeTransport(client_protocol, self.reactor, ssl_protocol) - ) - c2s_transport.other = ssl_protocol + # Tell the HTTP server to send outgoing traffic back via the proxy's transport. + proxy_server_transport = proxy_server.transport + server_ssl_protocol.makeConnection(proxy_server_transport) + + # ... and replace the protocol on the proxy's transport with the + # TLSMemoryBIOProtocol for the test server, so that incoming traffic + # to the proxy gets sent over to the HTTP(s) server. + + # See also comment at `_do_https_request_via_proxy` + # in ../test_proxyagent.py for more details + if ssl: + assert isinstance(proxy_server_transport, TLSMemoryBIOProtocol) + proxy_server_transport.wrappedProtocol = server_ssl_protocol + else: + assert isinstance(proxy_server_transport, FakeTransport) + client_protocol = proxy_server_transport.other + c2s_transport = client_protocol.transport + c2s_transport.other = server_ssl_protocol self.reactor.advance(0) - server_name = ssl_protocol._tlsConnection.get_servername() + server_name = server_ssl_protocol._tlsConnection.get_servername() expected_sni = b"testserv" self.assertEqual( server_name, @@ -442,6 +448,7 @@ def _do_get_via_proxy( ) # now there should be a pending request + http_server = server_ssl_protocol.wrappedProtocol self.assertEqual(len(http_server.requests), 1) request = http_server.requests[0] From b68a14de8d9f66772fa381547eca4ff5de4ade95 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 27 Jul 2021 21:22:57 +0200 Subject: [PATCH 17/28] fix lint --- synapse/http/federation/matrix_federation_agent.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index a2824f63bbfe..641f5f0e1f8c 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -29,12 +29,7 @@ IReactorCore, IStreamClientEndpoint, ) -from twisted.web.client import ( - URI, - Agent, - BrowserLikePolicyForHTTPS, - HTTPConnectionPool, -) +from twisted.web.client import URI, Agent, BrowserLikePolicyForHTTPS, HTTPConnectionPool from twisted.web.http_headers import Headers from twisted.web.iweb import IAgent, IAgentEndpointFactory, IBodyProducer, IResponse From eab6c74f59fc2f7b2399f31c7f66ca137cf6e71a Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 5 Aug 2021 13:57:14 +0200 Subject: [PATCH 18/28] update docs --- docs/setup/forward_proxy.md | 6 +++--- docs/upgrade.md | 11 +++++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/setup/forward_proxy.md b/docs/setup/forward_proxy.md index a0720ab342e7..494c14893b26 100644 --- a/docs/setup/forward_proxy.md +++ b/docs/setup/forward_proxy.md @@ -45,18 +45,18 @@ The proxy will be **used** for: - recaptcha validation - CAS auth validation - OpenID Connect +- Outbound federation - Federation (checking public key revocation) +- Fetching public keys of other servers +- Downloading remote media It will **not be used** for: - Application Services - Identity servers -- Outbound federation - In worker configurations - connections between workers - connections from workers to Redis -- Fetching public keys of other servers -- Downloading remote media ## Troubleshooting diff --git a/docs/upgrade.md b/docs/upgrade.md index 21e0d1933c0e..346c2996c6d6 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -98,17 +98,20 @@ http_proxy/https_proxy environment variables. This proxy was set for: - recaptcha validation - CAS auth validation - OpenID Connect +- Federation (checking public key revocation) In this version we have added support for outbound requests for: -- federation -- download remote media -- fetch keys from other servers -- Identity servers +- Outbound federation +- Downloading remote media +- Fetching public keys of other servers These requests use the same proxy configuration. If you have a proxy configuration we recommend to verify the configuration. It may be necessary to adjust the `no_proxy` environment variable. +See [using a forward proxy with Synapse documentation](setup/forward_proxy.md) for +details. + # Upgrading to v1.39.0 From 73a73809ed37e7da3620c8bf1657f12e4947c11d Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 13:50:37 +0200 Subject: [PATCH 19/28] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/http/federation/matrix_federation_agent.py | 9 +++++---- tests/http/federation/test_matrix_federation_agent.py | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 641f5f0e1f8c..0c9fbc99ce39 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -254,8 +254,8 @@ class MatrixHostnameEndpoint: Args: reactor: twisted reactor to use for underlying requests - proxy_reactor: twisted reactor to use for connections to the proxy server - reactor might have some blacklisting applied (i.e. for DNS queries), + proxy_reactor: twisted reactor to use for connections to the proxy server. + 'reactor' might have some blacklisting applied (i.e. for DNS queries), but we need unblocked access to the proxy. tls_client_options_factory: factory to use for fetching client tls options, or none to disable TLS. @@ -279,9 +279,10 @@ def __init__( https_proxy = proxies["https"].encode() if "https" in proxies else None self.no_proxy = proxies["no"] if "no" in proxies else None + # endpoint and credentials to use to connect to the outbound https proxy, if any. ( - self.https_proxy_endpoint, - self.https_proxy_creds, + self._https_proxy_endpoint, + self._https_proxy_creds, ) = proxyagent.http_proxy_endpoint( https_proxy, proxy_reactor, diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index db09d3e6a65f..d476cc47d8c2 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -354,8 +354,8 @@ def _do_get_via_proxy( """Send a https federation request via an agent and check that it is correctly received at the proxy and client. The proxy can use either http or https. Args: - ssl: True if we expect the request to connect via https to proxy - auth_credentials: credentials to authenticate at proxy + ssl: True if we expect the request to connect to the proxy via https. + expected_auth_credentials: credentials we expect to be presented to authenticate at the proxy """ # recreate the agent with patched env self.agent = self._make_agent() From 007ea8a01d2a6c1dfe6453038f16c1460bbd88dd Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 13:54:22 +0200 Subject: [PATCH 20/28] add to internal attributes underscore prefixes --- synapse/http/federation/matrix_federation_agent.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 0c9fbc99ce39..f9025324c7cd 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -327,25 +327,25 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: endpoint: IStreamClientEndpoint try: - if self.https_proxy_endpoint and not should_skip_proxy: + if self._https_proxy_endpoint and not should_skip_proxy: logger.debug( "Connecting to %s:%i via %s", host.decode("ascii"), port, - self.https_proxy_endpoint, + self._https_proxy_endpoint, ) connect_headers = Headers() # Determine whether we need to set Proxy-Authorization headers - if self.https_proxy_creds: + if self._https_proxy_creds: # Set a Proxy-Authorization header connect_headers.addRawHeader( b"Proxy-Authorization", - self.https_proxy_creds.as_proxy_authorization_value(), + self._https_proxy_creds.as_proxy_authorization_value(), ) endpoint = HTTPConnectProxyEndpoint( self._reactor, - self.https_proxy_endpoint, + self._https_proxy_endpoint, host, port, headers=connect_headers, From 258fab8aeee3e32476c4c59e7714074d6b7ae88a Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 14:08:08 +0200 Subject: [PATCH 21/28] rename param `ssl` to `expect_proxy_ssl` --- .../test_matrix_federation_agent.py | 23 +++++----- tests/http/test_proxyagent.py | 46 +++++++++++-------- 2 files changed, 38 insertions(+), 31 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index d476cc47d8c2..a08ba6091ab2 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -102,7 +102,8 @@ def _make_connection( ssl: If true, we will expect an ssl connection and wrap server_factory with a TLSMemoryBIOFactory - False is set only for connection via http_proxy + False is set only for when proxy expect http connection. + Otherwise federation requests use always https. expected_sni: the expected SNI value @@ -321,7 +322,7 @@ def _do_get(self): ) def test_get_via_http_proxy(self): """test for federation request through a http proxy""" - self._do_get_via_proxy(ssl=False, auth_credentials=None) + self._do_get_via_proxy(expect_proxy_ssl=False, auth_credentials=None) @patch.dict( os.environ, @@ -329,14 +330,14 @@ def test_get_via_http_proxy(self): ) def test_get_via_http_proxy_with_auth(self): """test for federation request through a http proxy with authentication""" - self._do_get_via_proxy(ssl=False, auth_credentials=b"user:pass") + self._do_get_via_proxy(expect_proxy_ssl=False, auth_credentials=b"user:pass") @patch.dict( os.environ, {"https_proxy": "https://proxy.com", "no_proxy": "unused.com"} ) def test_get_via_https_proxy(self): """test for federation request through a https proxy""" - self._do_get_via_proxy(ssl=True, auth_credentials=None) + self._do_get_via_proxy(expect_proxy_ssl=True, auth_credentials=None) @patch.dict( os.environ, @@ -344,17 +345,17 @@ def test_get_via_https_proxy(self): ) def test_get_via_https_proxy_with_auth(self): """test for federation request through a https proxy with authentication""" - self._do_get_via_proxy(ssl=True, auth_credentials=b"user:pass") + self._do_get_via_proxy(expect_proxy_ssl=True, auth_credentials=b"user:pass") def _do_get_via_proxy( self, - ssl: bool = False, + expect_proxy_ssl: bool = False, auth_credentials: Optional[bytes] = None, ): """Send a https federation request via an agent and check that it is correctly received at the proxy and client. The proxy can use either http or https. Args: - ssl: True if we expect the request to connect to the proxy via https. + expect_proxy_ssl: True if we expect the request to connect to the proxy via https. expected_auth_credentials: credentials we expect to be presented to authenticate at the proxy """ # recreate the agent with patched env @@ -378,9 +379,9 @@ def _do_get_via_proxy( # make a test server to act as the proxy, and wire up the client proxy_server = self._make_connection( client_factory, - ssl=ssl, - tls_sanlist=[b"DNS:proxy.com"] if ssl else None, - expected_sni=b"proxy.com" if ssl else None, + ssl=expect_proxy_ssl, + tls_sanlist=[b"DNS:proxy.com"] if expect_proxy_ssl else None, + expected_sni=b"proxy.com" if expect_proxy_ssl else None, ) assert isinstance(proxy_server, HTTPChannel) @@ -428,7 +429,7 @@ def _do_get_via_proxy( # See also comment at `_do_https_request_via_proxy` # in ../test_proxyagent.py for more details - if ssl: + if expect_proxy_ssl: assert isinstance(proxy_server_transport, TLSMemoryBIOProtocol) proxy_server_transport.wrappedProtocol = server_ssl_protocol else: diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index e5865c161d5e..57ec3895d268 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -392,7 +392,7 @@ def test_http_request_via_proxy(self): """ Tests that requests can be made through a proxy. """ - self._do_http_request_via_proxy(ssl=False, auth_credentials=None) + self._do_http_request_via_proxy(expect_proxy_ssl=False, auth_credentials=None) @patch.dict( os.environ, @@ -402,13 +402,13 @@ def test_http_request_via_proxy_with_auth(self): """ Tests that authenticated requests can be made through a proxy. """ - self._do_http_request_via_proxy(ssl=False, auth_credentials=b"bob:pinkponies") + self._do_http_request_via_proxy(expect_proxy_ssl=False, auth_credentials=b"bob:pinkponies") @patch.dict( os.environ, {"http_proxy": "https://proxy.com:8888", "no_proxy": "unused.com"} ) def test_http_request_via_https_proxy(self): - self._do_http_request_via_proxy(ssl=True, auth_credentials=None) + self._do_http_request_via_proxy(expect_proxy_ssl=True, auth_credentials=None) @patch.dict( os.environ, @@ -418,12 +418,14 @@ def test_http_request_via_https_proxy(self): }, ) def test_http_request_via_https_proxy_with_auth(self): - self._do_http_request_via_proxy(ssl=True, auth_credentials=b"bob:pinkponies") + self._do_http_request_via_proxy( + expect_proxy_ssl=True, auth_credentials=b"bob:pinkponies" + ) @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) def test_https_request_via_proxy(self): """Tests that TLS-encrypted requests can be made through a proxy""" - self._do_https_request_via_proxy(ssl=False, auth_credentials=None) + self._do_https_request_via_proxy(expect_proxy_ssl=False, auth_credentials=None) @patch.dict( os.environ, @@ -431,14 +433,16 @@ def test_https_request_via_proxy(self): ) def test_https_request_via_proxy_with_auth(self): """Tests that authenticated, TLS-encrypted requests can be made through a proxy""" - self._do_https_request_via_proxy(ssl=False, auth_credentials=b"bob:pinkponies") + self._do_https_request_via_proxy( + expect_proxy_ssl=False, auth_credentials=b"bob:pinkponies" + ) @patch.dict( os.environ, {"https_proxy": "https://proxy.com", "no_proxy": "unused.com"} ) def test_https_request_via_https_proxy(self): """Tests that TLS-encrypted requests can be made through a proxy""" - self._do_https_request_via_proxy(ssl=True, auth_credentials=None) + self._do_https_request_via_proxy(expect_proxy_ssl=True, auth_credentials=None) @patch.dict( os.environ, @@ -446,20 +450,22 @@ def test_https_request_via_https_proxy(self): ) def test_https_request_via_https_proxy_with_auth(self): """Tests that authenticated, TLS-encrypted requests can be made through a proxy""" - self._do_https_request_via_proxy(ssl=True, auth_credentials=b"bob:pinkponies") + self._do_https_request_via_proxy( + expect_proxy_ssl=True, auth_credentials=b"bob:pinkponies" + ) def _do_http_request_via_proxy( self, - ssl: bool = False, + expect_proxy_ssl: bool = False, auth_credentials: Optional[bytes] = None, ): """Send a http request via an agent and check that it is correctly received at the proxy. The proxy can use either http or https. Args: - ssl: True if we expect the request to connect via https to proxy + expect_proxy_ssl: True if we expect the request to connect via https to proxy auth_credentials: credentials to authenticate at proxy """ - if ssl: + if expect_proxy_ssl: agent = ProxyAgent( self.reactor, use_proxy=True, contextFactory=get_test_https_policy() ) @@ -480,9 +486,9 @@ def _do_http_request_via_proxy( http_server = self._make_connection( client_factory, _get_test_protocol_factory(), - ssl=ssl, - tls_sanlist=[b"DNS:proxy.com"] if ssl else None, - expected_sni=b"proxy.com" if ssl else None, + ssl=expect_proxy_ssl, + tls_sanlist=[b"DNS:proxy.com"] if expect_proxy_ssl else None, + expected_sni=b"proxy.com" if expect_proxy_ssl else None, ) # the FakeTransport is async, so we need to pump the reactor @@ -523,13 +529,13 @@ def _do_http_request_via_proxy( def _do_https_request_via_proxy( self, - ssl: bool = False, + expect_proxy_ssl: bool = False, auth_credentials: Optional[bytes] = None, ): """Send a https request via an agent and check that it is correctly received at the proxy and client. The proxy can use either http or https. Args: - ssl: True if we expect the request to connect via https to proxy + expect_proxy_ssl: True if we expect the request to connect via https to proxy auth_credentials: credentials to authenticate at proxy """ agent = ProxyAgent( @@ -552,9 +558,9 @@ def _do_https_request_via_proxy( proxy_server = self._make_connection( client_factory, _get_test_protocol_factory(), - ssl=ssl, - tls_sanlist=[b"DNS:proxy.com"] if ssl else None, - expected_sni=b"proxy.com" if ssl else None, + ssl=expect_proxy_ssl, + tls_sanlist=[b"DNS:proxy.com"] if expect_proxy_ssl else None, + expected_sni=b"proxy.com" if expect_proxy_ssl else None, ) assert isinstance(proxy_server, HTTPChannel) @@ -606,7 +612,7 @@ def _do_https_request_via_proxy( # Protocol to implement the proxy, which starts out by forwarding to an # HTTPChannel (to implement the CONNECT command) and can then be switched # into a mode where it forwards its traffic to another Protocol.) - if ssl: + if expect_proxy_ssl: assert isinstance(proxy_server_transport, TLSMemoryBIOProtocol) proxy_server_transport.wrappedProtocol = server_ssl_protocol else: From 08823be16020280d948cffb427abc4c94ab2e19e Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 14:25:45 +0200 Subject: [PATCH 22/28] remove `_make_agent()` from `setUp` in tests --- .../test_matrix_federation_agent.py | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index a08ba6091ab2..cc719398a916 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -85,8 +85,6 @@ def setUp(self): had_well_known_cache=self.had_well_known_cache, ) - self.agent = self._make_agent() - def _make_connection( self, client_factory: IProtocolFactory, @@ -261,7 +259,6 @@ def test_get_bypass_proxy(self): def _do_get(self): """test of a GET request with an explicit port""" - # recreate the agent with patched env self.agent = self._make_agent() self.reactor.lookups["testserv"] = "1.2.3.4" @@ -358,7 +355,6 @@ def _do_get_via_proxy( expect_proxy_ssl: True if we expect the request to connect to the proxy via https. expected_auth_credentials: credentials we expect to be presented to authenticate at the proxy """ - # recreate the agent with patched env self.agent = self._make_agent() self.reactor.lookups["testserv"] = "1.2.3.4" @@ -494,6 +490,8 @@ def test_get_ip_address(self): """ Test the behaviour when the server name contains an explicit IP (with no port) """ + self.agent = self._make_agent() + # there will be a getaddrinfo on the IP self.reactor.lookups["1.2.3.4"] = "1.2.3.4" @@ -528,6 +526,7 @@ def test_get_ipv6_address(self): Test the behaviour when the server name contains an explicit IPv6 address (with no port) """ + self.agent = self._make_agent() # there will be a getaddrinfo on the IP self.reactor.lookups["::1"] = "::1" @@ -563,6 +562,7 @@ def test_get_ipv6_address_with_port(self): Test the behaviour when the server name contains an explicit IPv6 address (with explicit port) """ + self.agent = self._make_agent() # there will be a getaddrinfo on the IP self.reactor.lookups["::1"] = "::1" @@ -597,6 +597,8 @@ def test_get_hostname_bad_cert(self): """ Test the behaviour when the certificate on the server doesn't match the hostname """ + self.agent = self._make_agent() + self.mock_resolver.resolve_service.side_effect = generate_resolve_service([]) self.reactor.lookups["testserv1"] = "1.2.3.4" @@ -649,6 +651,8 @@ def test_get_ip_address_bad_cert(self): Test the behaviour when the server name contains an explicit IP, but the server cert doesn't cover it """ + self.agent = self._make_agent() + # there will be a getaddrinfo on the IP self.reactor.lookups["1.2.3.5"] = "1.2.3.5" @@ -679,6 +683,7 @@ def test_get_no_srv_no_well_known(self): """ Test the behaviour when the server name has no port, no SRV, and no well-known """ + self.agent = self._make_agent() self.mock_resolver.resolve_service.side_effect = generate_resolve_service([]) self.reactor.lookups["testserv"] = "1.2.3.4" @@ -732,6 +737,7 @@ def test_get_no_srv_no_well_known(self): def test_get_well_known(self): """Test the behaviour when the .well-known delegates elsewhere""" + self.agent = self._make_agent() self.mock_resolver.resolve_service.side_effect = generate_resolve_service([]) self.reactor.lookups["testserv"] = "1.2.3.4" @@ -795,6 +801,8 @@ def test_get_well_known_redirect(self): """Test the behaviour when the server name has no port and no SRV record, but the .well-known has a 300 redirect """ + self.agent = self._make_agent() + self.mock_resolver.resolve_service.side_effect = generate_resolve_service([]) self.reactor.lookups["testserv"] = "1.2.3.4" self.reactor.lookups["target-server"] = "1::f" @@ -883,6 +891,7 @@ def test_get_invalid_well_known(self): """ Test the behaviour when the server name has an *invalid* well-known (and no SRV) """ + self.agent = self._make_agent() self.mock_resolver.resolve_service.side_effect = generate_resolve_service([]) self.reactor.lookups["testserv"] = "1.2.3.4" @@ -988,6 +997,8 @@ def test_get_hostname_srv(self): """ Test the behaviour when there is a single SRV record """ + self.agent = self._make_agent() + self.mock_resolver.resolve_service.side_effect = generate_resolve_service( [Server(host=b"srvtarget", port=8443)] ) @@ -1028,6 +1039,8 @@ def test_get_well_known_srv(self): """Test the behaviour when the .well-known redirects to a place where there is a SRV. """ + self.agent = self._make_agent() + self.reactor.lookups["testserv"] = "1.2.3.4" self.reactor.lookups["srvtarget"] = "5.6.7.8" @@ -1084,6 +1097,7 @@ def test_get_well_known_srv(self): def test_idna_servername(self): """test the behaviour when the server name has idna chars in""" + self.agent = self._make_agent() self.mock_resolver.resolve_service.side_effect = generate_resolve_service([]) @@ -1145,6 +1159,7 @@ def test_idna_servername(self): def test_idna_srv_target(self): """test the behaviour when the target of a SRV record has idna chars""" + self.agent = self._make_agent() self.mock_resolver.resolve_service.side_effect = generate_resolve_service( [Server(host=b"xn--trget-3qa.com", port=8443)] # târget.com @@ -1348,6 +1363,8 @@ def test_well_known_too_large(self): def test_srv_fallbacks(self): """Test that other SRV results are tried if the first one fails.""" + self.agent = self._make_agent() + self.mock_resolver.resolve_service.side_effect = generate_resolve_service( [ Server(host=b"target.com", port=8443), From 67522c3281ce9681335ca1f955f2d58beee9d343 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 14:32:55 +0200 Subject: [PATCH 23/28] rename in test `auth_credentials` to `expected_auth_credentials` --- .../test_matrix_federation_agent.py | 18 ++++---- tests/http/test_proxyagent.py | 42 ++++++++++++------- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index cc719398a916..88cc8cd4d4f8 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -319,7 +319,7 @@ def _do_get(self): ) def test_get_via_http_proxy(self): """test for federation request through a http proxy""" - self._do_get_via_proxy(expect_proxy_ssl=False, auth_credentials=None) + self._do_get_via_proxy(expect_proxy_ssl=False, expected_auth_credentials=None) @patch.dict( os.environ, @@ -327,14 +327,16 @@ def test_get_via_http_proxy(self): ) def test_get_via_http_proxy_with_auth(self): """test for federation request through a http proxy with authentication""" - self._do_get_via_proxy(expect_proxy_ssl=False, auth_credentials=b"user:pass") + self._do_get_via_proxy( + expect_proxy_ssl=False, expected_auth_credentials=b"user:pass" + ) @patch.dict( os.environ, {"https_proxy": "https://proxy.com", "no_proxy": "unused.com"} ) def test_get_via_https_proxy(self): """test for federation request through a https proxy""" - self._do_get_via_proxy(expect_proxy_ssl=True, auth_credentials=None) + self._do_get_via_proxy(expect_proxy_ssl=True, expected_auth_credentials=None) @patch.dict( os.environ, @@ -342,12 +344,14 @@ def test_get_via_https_proxy(self): ) def test_get_via_https_proxy_with_auth(self): """test for federation request through a https proxy with authentication""" - self._do_get_via_proxy(expect_proxy_ssl=True, auth_credentials=b"user:pass") + self._do_get_via_proxy( + expect_proxy_ssl=True, expected_auth_credentials=b"user:pass" + ) def _do_get_via_proxy( self, expect_proxy_ssl: bool = False, - auth_credentials: Optional[bytes] = None, + expected_auth_credentials: Optional[bytes] = None, ): """Send a https federation request via an agent and check that it is correctly received at the proxy and client. The proxy can use either http or https. @@ -394,9 +398,9 @@ def _do_get_via_proxy( b"Proxy-Authorization" ) - if auth_credentials is not None: + if expected_auth_credentials is not None: # Compute the correct header value for Proxy-Authorization - encoded_credentials = base64.b64encode(auth_credentials) + encoded_credentials = base64.b64encode(expected_auth_credentials) expected_header_value = b"Basic " + encoded_credentials # Validate the header's value diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 57ec3895d268..433ae1db0356 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -392,7 +392,9 @@ def test_http_request_via_proxy(self): """ Tests that requests can be made through a proxy. """ - self._do_http_request_via_proxy(expect_proxy_ssl=False, auth_credentials=None) + self._do_http_request_via_proxy( + expect_proxy_ssl=False, expected_auth_credentials=None + ) @patch.dict( os.environ, @@ -402,13 +404,17 @@ def test_http_request_via_proxy_with_auth(self): """ Tests that authenticated requests can be made through a proxy. """ - self._do_http_request_via_proxy(expect_proxy_ssl=False, auth_credentials=b"bob:pinkponies") + self._do_http_request_via_proxy( + expect_proxy_ssl=False, expected_auth_credentials=b"bob:pinkponies" + ) @patch.dict( os.environ, {"http_proxy": "https://proxy.com:8888", "no_proxy": "unused.com"} ) def test_http_request_via_https_proxy(self): - self._do_http_request_via_proxy(expect_proxy_ssl=True, auth_credentials=None) + self._do_http_request_via_proxy( + expect_proxy_ssl=True, expected_auth_credentials=None + ) @patch.dict( os.environ, @@ -419,13 +425,15 @@ def test_http_request_via_https_proxy(self): ) def test_http_request_via_https_proxy_with_auth(self): self._do_http_request_via_proxy( - expect_proxy_ssl=True, auth_credentials=b"bob:pinkponies" + expect_proxy_ssl=True, expected_auth_credentials=b"bob:pinkponies" ) @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) def test_https_request_via_proxy(self): """Tests that TLS-encrypted requests can be made through a proxy""" - self._do_https_request_via_proxy(expect_proxy_ssl=False, auth_credentials=None) + self._do_https_request_via_proxy( + expect_proxy_ssl=False, expected_auth_credentials=None + ) @patch.dict( os.environ, @@ -434,7 +442,7 @@ def test_https_request_via_proxy(self): def test_https_request_via_proxy_with_auth(self): """Tests that authenticated, TLS-encrypted requests can be made through a proxy""" self._do_https_request_via_proxy( - expect_proxy_ssl=False, auth_credentials=b"bob:pinkponies" + expect_proxy_ssl=False, expected_auth_credentials=b"bob:pinkponies" ) @patch.dict( @@ -442,7 +450,9 @@ def test_https_request_via_proxy_with_auth(self): ) def test_https_request_via_https_proxy(self): """Tests that TLS-encrypted requests can be made through a proxy""" - self._do_https_request_via_proxy(expect_proxy_ssl=True, auth_credentials=None) + self._do_https_request_via_proxy( + expect_proxy_ssl=True, expected_auth_credentials=None + ) @patch.dict( os.environ, @@ -451,19 +461,19 @@ def test_https_request_via_https_proxy(self): def test_https_request_via_https_proxy_with_auth(self): """Tests that authenticated, TLS-encrypted requests can be made through a proxy""" self._do_https_request_via_proxy( - expect_proxy_ssl=True, auth_credentials=b"bob:pinkponies" + expect_proxy_ssl=True, expected_auth_credentials=b"bob:pinkponies" ) def _do_http_request_via_proxy( self, expect_proxy_ssl: bool = False, - auth_credentials: Optional[bytes] = None, + expected_auth_credentials: Optional[bytes] = None, ): """Send a http request via an agent and check that it is correctly received at the proxy. The proxy can use either http or https. Args: expect_proxy_ssl: True if we expect the request to connect via https to proxy - auth_credentials: credentials to authenticate at proxy + expected_auth_credentials: credentials to authenticate at proxy """ if expect_proxy_ssl: agent = ProxyAgent( @@ -504,9 +514,9 @@ def _do_http_request_via_proxy( b"Proxy-Authorization" ) - if auth_credentials is not None: + if expected_auth_credentials is not None: # Compute the correct header value for Proxy-Authorization - encoded_credentials = base64.b64encode(auth_credentials) + encoded_credentials = base64.b64encode(expected_auth_credentials) expected_header_value = b"Basic " + encoded_credentials # Validate the header's value @@ -530,13 +540,13 @@ def _do_http_request_via_proxy( def _do_https_request_via_proxy( self, expect_proxy_ssl: bool = False, - auth_credentials: Optional[bytes] = None, + expected_auth_credentials: Optional[bytes] = None, ): """Send a https request via an agent and check that it is correctly received at the proxy and client. The proxy can use either http or https. Args: expect_proxy_ssl: True if we expect the request to connect via https to proxy - auth_credentials: credentials to authenticate at proxy + expected_auth_credentials: credentials to authenticate at proxy """ agent = ProxyAgent( self.reactor, @@ -576,9 +586,9 @@ def _do_https_request_via_proxy( b"Proxy-Authorization" ) - if auth_credentials is not None: + if expected_auth_credentials is not None: # Compute the correct header value for Proxy-Authorization - encoded_credentials = base64.b64encode(auth_credentials) + encoded_credentials = base64.b64encode(expected_auth_credentials) expected_header_value = b"Basic " + encoded_credentials # Validate the header's value From 512166b8bddf91fd5af2a6644a10119b80e15ae3 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 15:13:55 +0200 Subject: [PATCH 24/28] remove default TLS policy --- synapse/http/federation/matrix_federation_agent.py | 8 ++++++-- synapse/http/proxyagent.py | 13 ++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index f9025324c7cd..522fcf79dee8 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -29,7 +29,7 @@ IReactorCore, IStreamClientEndpoint, ) -from twisted.web.client import URI, Agent, BrowserLikePolicyForHTTPS, HTTPConnectionPool +from twisted.web.client import URI, Agent, HTTPConnectionPool from twisted.web.http_headers import Headers from twisted.web.iweb import IAgent, IAgentEndpointFactory, IBodyProducer, IResponse @@ -261,6 +261,10 @@ class MatrixHostnameEndpoint: factory to use for fetching client tls options, or none to disable TLS. srv_resolver: The SRV resolver to use parsed_uri: The parsed URI that we're wanting to connect to. + + Raises: + ValueError if the environment variables contain an invalid proxy specification. + RuntimeError if no tls_options_factory is given for a https connection """ def __init__( @@ -286,7 +290,7 @@ def __init__( ) = proxyagent.http_proxy_endpoint( https_proxy, proxy_reactor, - tls_client_options_factory or BrowserLikePolicyForHTTPS(), + tls_client_options_factory, ) # set up the TLS connection params diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 8cb9ea30f721..9bb8d9a9359b 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -95,6 +95,7 @@ class ProxyAgent(_AgentBase): Raises: ValueError if use_proxy is set and the environment variables contain an invalid proxy specification. + RuntimeError if no tls_options_factory is given for a https connection """ def __init__( @@ -271,7 +272,7 @@ def request( def http_proxy_endpoint( proxy: Optional[bytes], reactor: IReactorCore, - tls_options_factory: IPolicyForHTTPS, + tls_options_factory: Optional[IPolicyForHTTPS], **kwargs, ) -> Tuple[Optional[IStreamClientEndpoint], Optional[ProxyCredentials]]: """Parses an http proxy setting and returns an endpoint for the proxy @@ -294,6 +295,7 @@ def http_proxy_endpoint( Raise: ValueError if proxy has no hostname or unsupported scheme. + RuntimeError if no tls_options_factory is given for a https connection """ if proxy is None: return None, None @@ -305,8 +307,13 @@ def http_proxy_endpoint( proxy_endpoint = HostnameEndpoint(reactor, host, port, **kwargs) if scheme == b"https": - tls_options = tls_options_factory.creatorForNetloc(host, port) - proxy_endpoint = wrapClientTLS(tls_options, proxy_endpoint) + if tls_options_factory: + tls_options = tls_options_factory.creatorForNetloc(host, port) + proxy_endpoint = wrapClientTLS(tls_options, proxy_endpoint) + else: + raise RuntimeError( + f"No TLS options for a https connection via proxy {proxy!s}" + ) return proxy_endpoint, credentials From ab2067d34c48389686e93bd6f96e00d0a24a1e9f Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 16:53:28 +0200 Subject: [PATCH 25/28] bring `ProxyCredentials` to `HTTPConnectProxyEndpoint` --- synapse/http/connectproxyclient.py | 68 +++++++++++++------ .../federation/matrix_federation_agent.py | 11 +-- synapse/http/proxyagent.py | 32 +-------- tests/http/test_proxyagent.py | 3 +- 4 files changed, 52 insertions(+), 62 deletions(-) diff --git a/synapse/http/connectproxyclient.py b/synapse/http/connectproxyclient.py index 17e1c5abb13d..c577142268c5 100644 --- a/synapse/http/connectproxyclient.py +++ b/synapse/http/connectproxyclient.py @@ -12,8 +12,11 @@ # See the License for the specific language governing permissions and # limitations under the License. +import base64 import logging +from typing import Optional +import attr from zope.interface import implementer from twisted.internet import defer, protocol @@ -21,7 +24,6 @@ from twisted.internet.interfaces import IReactorCore, IStreamClientEndpoint from twisted.internet.protocol import ClientFactory, Protocol, connectionDone from twisted.web import http -from twisted.web.http_headers import Headers logger = logging.getLogger(__name__) @@ -30,6 +32,22 @@ class ProxyConnectError(ConnectError): pass +@attr.s +class ProxyCredentials: + username_password = attr.ib(type=bytes) + + def as_proxy_authorization_value(self) -> bytes: + """ + Return the value for a Proxy-Authorization header (i.e. 'Basic abdef=='). + + Returns: + A transformation of the authentication string the encoded value for + a Proxy-Authorization header. + """ + # Encode as base64 and prepend the authorization type + return b"Basic " + base64.encodebytes(self.username_password) + + @implementer(IStreamClientEndpoint) class HTTPConnectProxyEndpoint: """An Endpoint implementation which will send a CONNECT request to an http proxy @@ -46,7 +64,7 @@ class HTTPConnectProxyEndpoint: proxy_endpoint: the endpoint to use to connect to the proxy host: hostname that we want to CONNECT to port: port that we want to connect to - headers: Extra HTTP headers to include in the CONNECT request + proxy_creds: credentials to authenticate at proxy """ def __init__( @@ -55,20 +73,20 @@ def __init__( proxy_endpoint: IStreamClientEndpoint, host: bytes, port: int, - headers: Headers, + proxy_creds: Optional[ProxyCredentials], ): self._reactor = reactor self._proxy_endpoint = proxy_endpoint self._host = host self._port = port - self._headers = headers + self._proxy_creds = proxy_creds def __repr__(self): return "" % (self._proxy_endpoint,) def connect(self, protocolFactory: ClientFactory): f = HTTPProxiedClientFactory( - self._host, self._port, protocolFactory, self._headers + self._host, self._port, protocolFactory, self._proxy_creds ) d = self._proxy_endpoint.connect(f) # once the tcp socket connects successfully, we need to wait for the @@ -87,7 +105,7 @@ class HTTPProxiedClientFactory(protocol.ClientFactory): dst_host: hostname that we want to CONNECT to dst_port: port that we want to connect to wrapped_factory: The original Factory - headers: Extra HTTP headers to include in the CONNECT request + proxy_creds: credentials to authenticate at proxy """ def __init__( @@ -95,12 +113,12 @@ def __init__( dst_host: bytes, dst_port: int, wrapped_factory: ClientFactory, - headers: Headers, + proxy_creds: Optional[ProxyCredentials], ): self.dst_host = dst_host self.dst_port = dst_port self.wrapped_factory = wrapped_factory - self.headers = headers + self.proxy_creds = proxy_creds self.on_connection = defer.Deferred() def startedConnecting(self, connector): @@ -114,7 +132,7 @@ def buildProtocol(self, addr): self.dst_port, wrapped_protocol, self.on_connection, - self.headers, + self.proxy_creds, ) def clientConnectionFailed(self, connector, reason): @@ -145,7 +163,7 @@ class HTTPConnectProtocol(protocol.Protocol): connected_deferred: a Deferred which will be callbacked with wrapped_protocol when the CONNECT completes - headers: Extra HTTP headers to include in the CONNECT request + proxy_creds: credentials to authenticate at proxy """ def __init__( @@ -154,16 +172,16 @@ def __init__( port: int, wrapped_protocol: Protocol, connected_deferred: defer.Deferred, - headers: Headers, + proxy_creds: Optional[ProxyCredentials], ): self.host = host self.port = port self.wrapped_protocol = wrapped_protocol self.connected_deferred = connected_deferred - self.headers = headers + self.proxy_creds = proxy_creds self.http_setup_client = HTTPConnectSetupClient( - self.host, self.port, self.headers + self.host, self.port, self.proxy_creds ) self.http_setup_client.on_connected.addCallback(self.proxyConnected) @@ -205,30 +223,38 @@ class HTTPConnectSetupClient(http.HTTPClient): Args: host: The hostname to send in the CONNECT message port: The port to send in the CONNECT message - headers: Extra headers to send with the CONNECT message + proxy_creds: credentials to authenticate at proxy """ - def __init__(self, host: bytes, port: int, headers: Headers): + def __init__( + self, + host: bytes, + port: int, + proxy_creds: Optional[ProxyCredentials], + ): self.host = host self.port = port - self.headers = headers + self.proxy_creds = proxy_creds self.on_connected = defer.Deferred() def connectionMade(self): logger.debug("Connected to proxy, sending CONNECT") self.sendCommand(b"CONNECT", b"%s:%d" % (self.host, self.port)) - # Send any additional specified headers - for name, values in self.headers.getAllRawHeaders(): - for value in values: - self.sendHeader(name, value) + # Determine whether we need to set Proxy-Authorization headers + if self.proxy_creds: + # Set a Proxy-Authorization header + self.sendHeader( + b"Proxy-Authorization", + self.proxy_creds.as_proxy_authorization_value(), + ) self.endHeaders() def handleStatus(self, version: bytes, status: bytes, message: bytes): logger.debug("Got Status: %s %s %s", status, message, version) if status != b"200": - raise ProxyConnectError("Unexpected status on CONNECT: %s" % status) + raise ProxyConnectError(f"Unexpected status on CONNECT: {status!s}") def handleEndHeaders(self): logger.debug("End Headers") diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 522fcf79dee8..300c89834e12 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -338,21 +338,12 @@ async def _do_connect(self, protocol_factory: IProtocolFactory) -> None: port, self._https_proxy_endpoint, ) - connect_headers = Headers() - # Determine whether we need to set Proxy-Authorization headers - if self._https_proxy_creds: - # Set a Proxy-Authorization header - connect_headers.addRawHeader( - b"Proxy-Authorization", - self._https_proxy_creds.as_proxy_authorization_value(), - ) - endpoint = HTTPConnectProxyEndpoint( self._reactor, self._https_proxy_endpoint, host, port, - headers=connect_headers, + proxy_creds=self._https_proxy_creds, ) else: logger.debug("Connecting to %s:%i", host.decode("ascii"), port) diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 9bb8d9a9359b..a3f31452d0cc 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -11,7 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import base64 import logging import re from typing import Any, Dict, Optional, Tuple @@ -21,7 +20,6 @@ proxy_bypass_environment, ) -import attr from zope.interface import implementer from twisted.internet import defer @@ -38,7 +36,7 @@ from twisted.web.http_headers import Headers from twisted.web.iweb import IAgent, IBodyProducer, IPolicyForHTTPS -from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint +from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint, ProxyCredentials from synapse.types import ISynapseReactor logger = logging.getLogger(__name__) @@ -46,22 +44,6 @@ _VALID_URI = re.compile(br"\A[\x21-\x7e]+\Z") -@attr.s -class ProxyCredentials: - username_password = attr.ib(type=bytes) - - def as_proxy_authorization_value(self) -> bytes: - """ - Return the value for a Proxy-Authorization header (i.e. 'Basic abdef=='). - - Returns: - A transformation of the authentication string the encoded value for - a Proxy-Authorization header. - """ - # Encode as base64 and prepend the authorization type - return b"Basic " + base64.encodebytes(self.username_password) - - @implementer(IAgent) class ProxyAgent(_AgentBase): """An Agent implementation which will use an HTTP proxy if one was requested @@ -225,22 +207,12 @@ def request( and self.https_proxy_endpoint and not should_skip_proxy ): - connect_headers = Headers() - - # Determine whether we need to set Proxy-Authorization headers - if self.https_proxy_creds: - # Set a Proxy-Authorization header - connect_headers.addRawHeader( - b"Proxy-Authorization", - self.https_proxy_creds.as_proxy_authorization_value(), - ) - endpoint = HTTPConnectProxyEndpoint( self.proxy_reactor, self.https_proxy_endpoint, parsed_uri.host, parsed_uri.port, - headers=connect_headers, + self.https_proxy_creds, ) else: # not using a proxy diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 433ae1db0356..2db77c6a7345 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -29,7 +29,8 @@ from twisted.web.http import HTTPChannel from synapse.http.client import BlacklistingReactorWrapper -from synapse.http.proxyagent import ProxyAgent, ProxyCredentials, parse_proxy +from synapse.http.connectproxyclient import ProxyCredentials +from synapse.http.proxyagent import ProxyAgent, parse_proxy from tests.http import TestServerTLSConnectionFactory, get_test_https_policy from tests.server import FakeTransport, ThreadedMemoryReactorClock From e8fd305142b5285825c43e4a08b94d3b5300dcb2 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 18:02:23 +0200 Subject: [PATCH 26/28] move build `BlacklistingReactorWrapper` into `MatrixFederationAgent` --- .../http/federation/matrix_federation_agent.py | 18 +++++++++++------- synapse/http/matrixfederationclient.py | 11 ++--------- .../federation/test_matrix_federation_agent.py | 2 ++ 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 300c89834e12..442c5bdbbf2f 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -35,7 +35,7 @@ from synapse.crypto.context_factory import FederationPolicyForHTTPS from synapse.http import proxyagent -from synapse.http.client import BlacklistingAgentWrapper +from synapse.http.client import BlacklistingAgentWrapper, BlacklistingReactorWrapper from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint from synapse.http.federation.srv_resolver import Server, SrvResolver from synapse.http.federation.well_known_resolver import WellKnownResolver @@ -64,6 +64,8 @@ class MatrixFederationAgent: user_agent: The user agent header to use for federation requests. + ip_whitelist: Allowed IP addresses. + ip_blacklist: Disallowed IP addresses. proxy_reactor: twisted reactor to use for connections to the proxy server @@ -84,11 +86,18 @@ def __init__( reactor: ISynapseReactor, tls_client_options_factory: Optional[FederationPolicyForHTTPS], user_agent: bytes, + ip_whitelist: IPSet, ip_blacklist: IPSet, - proxy_reactor: Optional[ISynapseReactor] = None, _srv_resolver: Optional[SrvResolver] = None, _well_known_resolver: Optional[WellKnownResolver] = None, ): + # proxy_reactor is not blacklisted + self.proxy_reactor = reactor + + # We need to use a DNS resolver which filters out blacklisted IP + # addresses, to prevent DNS rebinding. + reactor = BlacklistingReactorWrapper(reactor, ip_whitelist, ip_blacklist) + self._reactor = reactor self._clock = Clock(reactor) self._pool = HTTPConnectionPool(reactor) @@ -96,11 +105,6 @@ def __init__( self._pool.maxPersistentPerHost = 5 self._pool.cachedConnectionTimeout = 2 * 60 - if proxy_reactor is None: - self.proxy_reactor = reactor - else: - self.proxy_reactor = proxy_reactor - self._agent = Agent.usingEndpointFactory( self._reactor, MatrixHostnameEndpointFactory( diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index af1852a8fc7f..de2eea7d693c 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -59,7 +59,6 @@ from synapse.http import QuieterFileBodyProducer from synapse.http.client import ( BlacklistingAgentWrapper, - BlacklistingReactorWrapper, BodyExceededMaxSize, ByteWriteable, encode_query_args, @@ -325,13 +324,7 @@ def __init__(self, hs, tls_client_options_factory): self.signing_key = hs.signing_key self.server_name = hs.hostname - # We need to use a DNS resolver which filters out blacklisted IP - # addresses, to prevent DNS rebinding. - self.reactor: ISynapseReactor = BlacklistingReactorWrapper( - hs.get_reactor(), - hs.config.federation_ip_range_whitelist, - hs.config.federation_ip_range_blacklist, - ) + self.reactor = hs.get_reactor() user_agent = hs.version_string if hs.config.user_agent_suffix: @@ -342,8 +335,8 @@ def __init__(self, hs, tls_client_options_factory): self.reactor, tls_client_options_factory, user_agent, + hs.config.federation_ip_range_whitelist, hs.config.federation_ip_range_blacklist, - proxy_reactor=hs.get_reactor(), ) # Use a BlacklistingAgentWrapper to prevent circumventing the IP diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 88cc8cd4d4f8..992d8f94fd70 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -240,6 +240,7 @@ def _make_agent(self) -> MatrixFederationAgent: reactor=self.reactor, tls_client_options_factory=self.tls_factory, user_agent="test-agent", # Note that this is unused since _well_known_resolver is provided. + ip_whitelist=IPSet(), ip_blacklist=IPSet(), _srv_resolver=self.mock_resolver, _well_known_resolver=self.well_known_resolver, @@ -964,6 +965,7 @@ def test_get_well_known_unsigned_cert(self): reactor=self.reactor, tls_client_options_factory=tls_factory, user_agent=b"test-agent", # This is unused since _well_known_resolver is passed below. + ip_whitelist=IPSet(), ip_blacklist=IPSet(), _srv_resolver=self.mock_resolver, _well_known_resolver=WellKnownResolver( From d0933e5a6dce942b1ed6b205b5485ff32fa2f480 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 7 Aug 2021 18:05:27 +0200 Subject: [PATCH 27/28] lint --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index de2eea7d693c..2e9898997c4c 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -68,7 +68,7 @@ from synapse.logging import opentracing from synapse.logging.context import make_deferred_yieldable from synapse.logging.opentracing import set_tag, start_active_span, tags -from synapse.types import ISynapseReactor, JsonDict +from synapse.types import JsonDict from synapse.util import json_decoder from synapse.util.async_helpers import timeout_deferred from synapse.util.metrics import Measure From c953242f2bff91c7f84313aa4b0245cf6a9e9702 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 9 Aug 2021 15:19:15 +0200 Subject: [PATCH 28/28] remove not needed reactor --- .../http/federation/matrix_federation_agent.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 442c5bdbbf2f..1238bfd28726 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -92,13 +92,12 @@ def __init__( _well_known_resolver: Optional[WellKnownResolver] = None, ): # proxy_reactor is not blacklisted - self.proxy_reactor = reactor + proxy_reactor = reactor # We need to use a DNS resolver which filters out blacklisted IP # addresses, to prevent DNS rebinding. reactor = BlacklistingReactorWrapper(reactor, ip_whitelist, ip_blacklist) - self._reactor = reactor self._clock = Clock(reactor) self._pool = HTTPConnectionPool(reactor) self._pool.retryAutomatically = False @@ -106,10 +105,10 @@ def __init__( self._pool.cachedConnectionTimeout = 2 * 60 self._agent = Agent.usingEndpointFactory( - self._reactor, + reactor, MatrixHostnameEndpointFactory( reactor, - self.proxy_reactor, + proxy_reactor, tls_client_options_factory, _srv_resolver, ), @@ -118,14 +117,12 @@ def __init__( self.user_agent = user_agent if _well_known_resolver is None: - # Note that the name resolver has already been wrapped in a - # IPBlacklistingResolver by MatrixFederationHttpClient. _well_known_resolver = WellKnownResolver( - self._reactor, + reactor, agent=BlacklistingAgentWrapper( ProxyAgent( - self._reactor, - self.proxy_reactor, + reactor, + proxy_reactor, pool=self._pool, contextFactory=tls_client_options_factory, use_proxy=True,