From 610a061c4ca6dc2a6c01cfbcf38add284828cf11 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Mar 2021 15:06:17 -0500 Subject: [PATCH 1/6] Add tests for blacklisting reactor/agent. --- tests/http/test_client.py | 126 +++++++++++++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 2 deletions(-) diff --git a/tests/http/test_client.py b/tests/http/test_client.py index 21ecb81c9926..0ce181a51e94 100644 --- a/tests/http/test_client.py +++ b/tests/http/test_client.py @@ -16,12 +16,23 @@ from mock import Mock +from netaddr import IPSet + +from twisted.internet.error import DNSLookupError from twisted.python.failure import Failure -from twisted.web.client import ResponseDone +from twisted.test.proto_helpers import AccumulatingProtocol +from twisted.web.client import Agent, ResponseDone from twisted.web.iweb import UNKNOWN_LENGTH -from synapse.http.client import BodyExceededMaxSize, read_body_with_max_size +from synapse.api.errors import SynapseError +from synapse.http.client import ( + BlacklistingAgentWrapper, + BlacklistingReactorWrapper, + BodyExceededMaxSize, + read_body_with_max_size, +) +from tests.server import FakeTransport, get_clock from tests.unittest import TestCase @@ -119,3 +130,114 @@ def test_content_length(self): # The data is never consumed. self.assertEqual(result.getvalue(), b"") + + +class BlacklistingAgentTest(TestCase): + def setUp(self): + self.reactor, self.clock = get_clock() + + self.safe_domain, self.safe_ip = b"safe.test", b"1.2.3.4" + self.unsafe_domain, self.unsafe_ip = b"danger.test", b"5.6.7.8" + self.allowed_domain, self.allowed_ip = b"allowed.test", b"5.1.1.1" + + # Configure the reactor's DNS resolver. + for (domain, ip) in ( + (self.safe_domain, self.safe_ip), + (self.unsafe_domain, self.unsafe_ip), + (self.allowed_domain, self.allowed_ip), + ): + self.reactor.lookups[domain.decode()] = ip.decode() + self.reactor.lookups[ip.decode()] = ip.decode() + + self.ip_whitelist = IPSet([self.allowed_ip.decode()]) + self.ip_blacklist = IPSet(["5.0.0.0/8"]) + + def test_reactor(self): + """Apply the blacklisting reactor and ensure it properly blocks connections to particular domains and IPs.""" + agent = Agent( + BlacklistingReactorWrapper( + self.reactor, + ip_whitelist=self.ip_whitelist, + ip_blacklist=self.ip_blacklist, + ), + ) + + # The unsafe domains and IPs should be rejected. + for domain in (self.unsafe_domain, self.unsafe_ip): + self.failureResultOf( + agent.request(b"GET", b"http://" + domain), DNSLookupError + ) + + # The safe domains IPs should be accepted. + for domain in ( + self.safe_domain, + self.allowed_domain, + self.safe_ip, + self.allowed_ip, + ): + d = agent.request(b"GET", b"http://" + domain) + + # Grab the latest TCP connection. + ( + host, + port, + client_factory, + _timeout, + _bindAddress, + ) = self.reactor.tcpClients[-1] + + # Make the connection and pump data through it. + client = client_factory.buildProtocol(None) + server = AccumulatingProtocol() + server.makeConnection(FakeTransport(client, self.reactor)) + client.makeConnection(FakeTransport(server, self.reactor)) + client.dataReceived( + b"HTTP/1.0 200 OK\r\nContent-Length: 0\r\nContent-Type: text/html\r\n\r\n" + ) + + response = self.successResultOf(d) + self.assertEqual(response.code, 200) + + def test_agent(self): + """Apply the blacklisting agent and ensure it properly blocks connections to particular IPs.""" + agent = BlacklistingAgentWrapper( + Agent(self.reactor), + ip_whitelist=self.ip_whitelist, + ip_blacklist=self.ip_blacklist, + ) + + # The unsafe IPs should be rejected. + self.failureResultOf( + agent.request(b"GET", b"http://" + self.unsafe_ip), SynapseError + ) + + # The safe and unsafe domains and safe IPs should be accepted. + for domain in ( + self.safe_domain, + self.unsafe_domain, + self.allowed_domain, + self.safe_ip, + self.allowed_ip, + ): + d = agent.request(b"GET", b"http://" + domain) + + # Grab the latest TCP connection. + ( + host, + port, + client_factory, + _timeout, + _bindAddress, + ) = self.reactor.tcpClients[-1] + + # Make the connection and pump data through it. + client = client_factory.buildProtocol(None) + server = AccumulatingProtocol() + server.makeConnection(FakeTransport(client, self.reactor)) + client.makeConnection(FakeTransport(server, self.reactor)) + client.dataReceived( + b"HTTP/1.0 200 OK\r\nContent-Length: 0\r\nContent-Type: text/html\r\n\r\n" + ) + + response = self.successResultOf(d) + self.assertEqual(response.code, 200) From b35eb1c4ef0cc5ff6536fe6d61e047025593681d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Mar 2021 15:08:42 -0500 Subject: [PATCH 2/6] Do not instantiate recv, it is already an instance. This just happens to work since the default implementation (via the HostnameEndpoint) happens to create a class that only has static methods, then passes the class (without instantiating it), presumedly to save memory. --- synapse/http/client.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index af34d583ad88..3ccfb0d62773 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -151,12 +151,10 @@ def __init__( def resolveHostName( self, recv: IResolutionReceiver, hostname: str, portNumber: int = 0 ) -> IResolutionReceiver: - - r = recv() addresses = [] # type: List[IAddress] def _callback() -> None: - r.resolutionBegan(None) + recv.resolutionBegan(None) has_bad_ip = False for i in addresses: @@ -176,8 +174,8 @@ def _callback() -> None: # valid results. if not has_bad_ip: for i in addresses: - r.addressResolved(i) - r.resolutionComplete() + recv.addressResolved(i) + recv.resolutionComplete() @provider(IResolutionReceiver) class EndpointReceiver: @@ -197,7 +195,7 @@ def resolutionComplete() -> None: EndpointReceiver, hostname, portNumber=portNumber ) - return r + return recv @implementer(ISynapseReactor) From edfa14327ba18fd32c7711f97061e062be4a5f3b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Mar 2021 15:11:29 -0500 Subject: [PATCH 3/6] Guard against other IAddress implementations. --- synapse/http/client.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 3ccfb0d62773..997975984cd5 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -39,6 +39,7 @@ from OpenSSL import SSL from OpenSSL.SSL import VERIFY_NONE from twisted.internet import defer, error as twisted_error, protocol, ssl +from twisted.internet.address import IPv4Address, IPv6Address from twisted.internet.interfaces import ( IAddress, IHostResolution, @@ -157,8 +158,13 @@ def _callback() -> None: recv.resolutionBegan(None) has_bad_ip = False - for i in addresses: - ip_address = IPAddress(i.host) + for address in addresses: + # We only expect IPv4 and IPv6 addresses since only A/AAAA lookups + # should go through this path. + if not isinstance(address, (IPv4Address, IPv6Address)): + continue + + ip_address = IPAddress(address.host) if check_against_blacklist( ip_address, self._ip_whitelist, self._ip_blacklist @@ -173,8 +179,8 @@ def _callback() -> None: # request, but all we can really do from here is claim that there were no # valid results. if not has_bad_ip: - for i in addresses: - recv.addressResolved(i) + for address in addresses: + recv.addressResolved(address) recv.resolutionComplete() @provider(IResolutionReceiver) From f8e9dc1857f8df4e88027e55c772218338d0d2cb Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Mar 2021 15:19:33 -0500 Subject: [PATCH 4/6] Properly call resolutionBegan. --- synapse/http/client.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 997975984cd5..df405b3f4e42 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -155,8 +155,6 @@ def resolveHostName( addresses = [] # type: List[IAddress] def _callback() -> None: - recv.resolutionBegan(None) - has_bad_ip = False for address in addresses: # We only expect IPv4 and IPv6 addresses since only A/AAAA lookups @@ -187,7 +185,7 @@ def _callback() -> None: class EndpointReceiver: @staticmethod def resolutionBegan(resolutionInProgress: IHostResolution) -> None: - pass + recv.resolutionBegan(resolutionInProgress) @staticmethod def addressResolved(address: IAddress) -> None: From e73c4119251c17e1dad18f40db6cf6ffd91c2441 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Mar 2021 15:23:57 -0500 Subject: [PATCH 5/6] Add a type hint for agent. --- synapse/http/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index df405b3f4e42..8f3da486b3a8 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -348,7 +348,7 @@ def __init__( contextFactory=self.hs.get_http_client_context_factory(), pool=pool, use_proxy=use_proxy, - ) + ) # type: IAgent if self._ip_blacklist: # If we have an IP blacklist, we then install the blacklisting Agent From 09a194abcadbe19e55e2a39f6bbe7aeb11c58320 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Mar 2021 15:26:15 -0500 Subject: [PATCH 6/6] Newsfragment --- changelog.d/9563.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9563.misc diff --git a/changelog.d/9563.misc b/changelog.d/9563.misc new file mode 100644 index 000000000000..7a3493e4a166 --- /dev/null +++ b/changelog.d/9563.misc @@ -0,0 +1 @@ +Fix type hints and tests for BlacklistingAgentWrapper and BlacklistingReactorWrapper.