From f4fc27abe905ba758396b65af57d268ac51c8e52 Mon Sep 17 00:00:00 2001 From: tzyl Date: Wed, 10 Feb 2021 14:47:10 +0000 Subject: [PATCH 1/8] Add support for no_proxy and case insensitive env variables --- synapse/http/client.py | 2 + synapse/http/proxyagent.py | 29 ++++++- synapse/rest/media/v1/preview_url_resource.py | 7 +- synapse/server.py | 14 ++-- tests/http/test_proxyagent.py | 84 ++++++++++--------- 5 files changed, 86 insertions(+), 50 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 37ccf5ab98f7..51b0017e9244 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -291,6 +291,7 @@ def __init__( ip_blacklist: Optional[IPSet] = None, http_proxy: Optional[bytes] = None, https_proxy: Optional[bytes] = None, + no_proxy: Optional[bytes] = None, ): """ Args: @@ -302,6 +303,7 @@ def __init__( request if it were otherwise caught in a blacklist. http_proxy: proxy server to use for http connections. host[:port] https_proxy: proxy server to use for https connections. host[:port] + no_proxy: locations that should explicitly not use a proxy. """ self.hs = hs diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index b730d2c63452..2eb1a6b43247 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -14,6 +14,7 @@ # limitations under the License. import logging import re +from urllib.request import proxy_bypass_environment from zope.interface import implementer @@ -58,6 +59,12 @@ class ProxyAgent(_AgentBase): pool (HTTPConnectionPool|None): connection pool to be used. If None, a non-persistent pool instance will be created. + + http_proxy (bytes): Proxy server to use for http connections. host[:port] + + https_proxy (bytes): Proxy server to use for https connections. host[:port] + + no_proxy (bytes): Locations that should explicitly not use a proxy. """ def __init__( @@ -70,6 +77,7 @@ def __init__( pool=None, http_proxy=None, https_proxy=None, + no_proxy=None, ): _AgentBase.__init__(self, reactor, pool) @@ -92,6 +100,8 @@ def __init__( https_proxy, self.proxy_reactor, **self._endpoint_kwargs ) + self.no_proxy = no_proxy + self._policy_for_https = contextFactory self._reactor = reactor @@ -138,14 +148,29 @@ def request(self, method, uri, headers=None, bodyProducer=None): parsed_uri = URI.fromBytes(uri) pool_key = (parsed_uri.scheme, parsed_uri.host, parsed_uri.port) request_path = parsed_uri.originForm + should_skip_proxy = ( + proxy_bypass_environment( + parsed_uri.host.decode(), proxies={"no": self.no_proxy.decode()}, + ) + if self.no_proxy is not None + else False + ) - if parsed_uri.scheme == b"http" and self.http_proxy_endpoint: + if ( + parsed_uri.scheme == b"http" + and self.http_proxy_endpoint + and not should_skip_proxy + ): # Cache *all* connections under the same key, since we are only # connecting to a single destination, the proxy: pool_key = ("http-proxy", self.http_proxy_endpoint) endpoint = self.http_proxy_endpoint request_path = uri - elif parsed_uri.scheme == b"https" and self.https_proxy_endpoint: + elif ( + parsed_uri.scheme == b"https" + and self.https_proxy_endpoint + and not should_skip_proxy + ): endpoint = HTTPConnectProxyEndpoint( self.proxy_reactor, self.https_proxy_endpoint, diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index ae53b1d23ff7..27909b2a8684 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -25,6 +25,7 @@ import traceback from typing import TYPE_CHECKING, Any, Dict, Generator, Iterable, Optional, Union from urllib import parse as urlparse +from urllib.request import getproxies import attr @@ -144,13 +145,15 @@ def __init__( self.max_spider_size = hs.config.max_spider_size self.server_name = hs.hostname self.store = hs.get_datastore() + proxies = getproxies() self.client = SimpleHttpClient( hs, treq_args={"browser_like_redirects": True}, ip_whitelist=hs.config.url_preview_ip_range_whitelist, ip_blacklist=hs.config.url_preview_ip_range_blacklist, - http_proxy=os.getenvb(b"http_proxy"), - https_proxy=os.getenvb(b"HTTPS_PROXY"), + http_proxy=proxies["http"].encode() if "http" in proxies else None, + https_proxy=proxies["https"].encode() if "https" in proxies else None, + no_proxy=proxies["no"].encode() if "no" in proxies else None, ) self.media_repo = media_repo self.primary_base_path = media_repo.primary_base_path diff --git a/synapse/server.py b/synapse/server.py index 6b3892e3cdff..1dd15d12d353 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -24,7 +24,6 @@ import abc import functools import logging -import os from typing import ( TYPE_CHECKING, Any, @@ -36,6 +35,7 @@ Union, cast, ) +from urllib.request import getproxies import twisted.internet.base import twisted.internet.tcp @@ -370,10 +370,12 @@ def get_proxied_http_client(self) -> SimpleHttpClient: """ An HTTP client that uses configured HTTP(S) proxies. """ + proxies = getproxies() return SimpleHttpClient( self, - http_proxy=os.getenvb(b"http_proxy"), - https_proxy=os.getenvb(b"HTTPS_PROXY"), + http_proxy=proxies["http"].encode() if "http" in proxies else None, + https_proxy=proxies["https"].encode() if "https" in proxies else None, + no_proxy=proxies["no"].encode() if "no" in proxies else None, ) @cache_in_self @@ -382,12 +384,14 @@ def get_proxied_blacklisted_http_client(self) -> SimpleHttpClient: An HTTP client that uses configured HTTP(S) proxies and blacklists IPs based on the IP range blacklist/whitelist. """ + proxies = getproxies() return SimpleHttpClient( self, ip_whitelist=self.config.ip_range_whitelist, ip_blacklist=self.config.ip_range_blacklist, - http_proxy=os.getenvb(b"http_proxy"), - https_proxy=os.getenvb(b"HTTPS_PROXY"), + http_proxy=proxies["http"].encode() if "http" in proxies else None, + https_proxy=proxies["https"].encode() if "https" in proxies else None, + no_proxy=proxies["no"].encode() if "no" in proxies else None, ) @cache_in_self diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 9a56e1c14afc..1aca8a2dcccd 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -100,22 +100,25 @@ def _make_connection( return http_protocol - def test_http_request(self): - agent = ProxyAgent(self.reactor) + def _test_request_no_proxy(self, agent, scheme, hostname, path): + is_https = scheme == b"https" - self.reactor.lookups["test.com"] = "1.2.3.4" - d = agent.request(b"GET", b"http://test.com") + self.reactor.lookups[hostname.decode()] = "1.2.3.4" + d = agent.request(b"GET", scheme + b"://" + hostname + b"/" + path) # there should be a pending TCP connection clients = self.reactor.tcpClients self.assertEqual(len(clients), 1) (host, port, client_factory, _timeout, _bindAddress) = clients[0] self.assertEqual(host, "1.2.3.4") - self.assertEqual(port, 80) + self.assertEqual(port, 443 if is_https else 80) # make a test server, and wire up the client http_server = self._make_connection( - client_factory, _get_test_protocol_factory() + client_factory, + _get_test_protocol_factory(), + ssl=is_https, + expected_sni=hostname if is_https else None, ) # the FakeTransport is async, so we need to pump the reactor @@ -126,8 +129,8 @@ def test_http_request(self): request = http_server.requests[0] self.assertEqual(request.method, b"GET") - self.assertEqual(request.path, b"/") - self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [b"test.com"]) + self.assertEqual(request.path, b"/" + path) + self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [hostname]) request.write(b"result") request.finish() @@ -137,48 +140,46 @@ def test_http_request(self): body = self.successResultOf(treq.content(resp)) self.assertEqual(body, b"result") + def test_http_request(self): + agent = ProxyAgent(self.reactor) + self._test_request_no_proxy(agent, b"http", b"test.com", b"") + def test_https_request(self): agent = ProxyAgent(self.reactor, contextFactory=get_test_https_policy()) + self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") - self.reactor.lookups["test.com"] = "1.2.3.4" - d = agent.request(b"GET", b"https://test.com/abc") - - # there should be a pending TCP connection - clients = self.reactor.tcpClients - self.assertEqual(len(clients), 1) - (host, port, client_factory, _timeout, _bindAddress) = clients[0] - self.assertEqual(host, "1.2.3.4") - self.assertEqual(port, 443) - - # make a test server, and wire up the client - http_server = self._make_connection( - client_factory, - _get_test_protocol_factory(), - ssl=True, - expected_sni=b"test.com", + def test_http_request_via_no_proxy(self): + agent = ProxyAgent( + self.reactor, http_proxy=b"proxy.com:8888", no_proxy=b"test.com,unused.com" ) + self._test_request_no_proxy(agent, b"http", b"test.com", b"") - # the FakeTransport is async, so we need to pump the reactor - self.reactor.advance(0) - - # 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"/abc") - self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [b"test.com"]) - request.write(b"result") - request.finish() + def test_https_request_via_no_proxy(self): + agent = ProxyAgent( + self.reactor, + contextFactory=get_test_https_policy(), + https_proxy=b"proxy.com", + no_proxy=b"test.com,unused.com", + ) + self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") - self.reactor.advance(0) + def test_http_request_via_no_proxy_star(self): + agent = ProxyAgent(self.reactor, http_proxy=b"proxy.com:8888", no_proxy=b"*") + self._test_request_no_proxy(agent, b"http", b"test.com", b"") - resp = self.successResultOf(d) - body = self.successResultOf(treq.content(resp)) - self.assertEqual(body, b"result") + def test_https_request_via_no_proxy_star(self): + agent = ProxyAgent( + self.reactor, + contextFactory=get_test_https_policy(), + https_proxy=b"proxy.com", + no_proxy=b"*", + ) + self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") def test_http_request_via_proxy(self): - agent = ProxyAgent(self.reactor, http_proxy=b"proxy.com:8888") + agent = ProxyAgent( + self.reactor, http_proxy=b"proxy.com:8888", no_proxy=b"unused.com" + ) self.reactor.lookups["proxy.com"] = "1.2.3.5" d = agent.request(b"GET", b"http://test.com") @@ -219,6 +220,7 @@ def test_https_request_via_proxy(self): self.reactor, contextFactory=get_test_https_policy(), https_proxy=b"proxy.com", + no_proxy=b"unused.com", ) self.reactor.lookups["proxy.com"] = "1.2.3.5" From cac17edcd5f5a58fd37b902cfbba4e9e61ec8381 Mon Sep 17 00:00:00 2001 From: tzyl Date: Wed, 10 Feb 2021 15:20:09 +0000 Subject: [PATCH 2/8] Add changelog --- changelog.d/9372.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9372.feature diff --git a/changelog.d/9372.feature b/changelog.d/9372.feature new file mode 100644 index 000000000000..3cb01004c97b --- /dev/null +++ b/changelog.d/9372.feature @@ -0,0 +1 @@ +The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. From e2584975348c1285ebf99205ff3b53d8ffdedb0f Mon Sep 17 00:00:00 2001 From: tzyl Date: Wed, 10 Feb 2021 15:25:59 +0000 Subject: [PATCH 3/8] Actually pass it in from SimpleHttpClient --- synapse/http/client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/http/client.py b/synapse/http/client.py index 51b0017e9244..2f82e6e1066c 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -349,6 +349,7 @@ def __init__( pool=pool, http_proxy=http_proxy, https_proxy=https_proxy, + no_proxy=no_proxy, ) if self._ip_blacklist: From d84117ff4ba7671386fd314c52d022dd75d03dac Mon Sep 17 00:00:00 2001 From: tzyl Date: Wed, 10 Feb 2021 19:54:41 +0000 Subject: [PATCH 4/8] Use getproxies_environment directly --- synapse/rest/media/v1/preview_url_resource.py | 4 ++-- synapse/server.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 27909b2a8684..3fef7c3d7b63 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -25,7 +25,7 @@ import traceback from typing import TYPE_CHECKING, Any, Dict, Generator, Iterable, Optional, Union from urllib import parse as urlparse -from urllib.request import getproxies +from urllib.request import getproxies_environment import attr @@ -145,7 +145,7 @@ def __init__( self.max_spider_size = hs.config.max_spider_size self.server_name = hs.hostname self.store = hs.get_datastore() - proxies = getproxies() + proxies = getproxies_environment() self.client = SimpleHttpClient( hs, treq_args={"browser_like_redirects": True}, diff --git a/synapse/server.py b/synapse/server.py index 1dd15d12d353..813bdb7ae129 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -35,7 +35,7 @@ Union, cast, ) -from urllib.request import getproxies +from urllib.request import getproxies_environment import twisted.internet.base import twisted.internet.tcp @@ -370,7 +370,7 @@ def get_proxied_http_client(self) -> SimpleHttpClient: """ An HTTP client that uses configured HTTP(S) proxies. """ - proxies = getproxies() + proxies = getproxies_environment() return SimpleHttpClient( self, http_proxy=proxies["http"].encode() if "http" in proxies else None, @@ -384,7 +384,7 @@ def get_proxied_blacklisted_http_client(self) -> SimpleHttpClient: An HTTP client that uses configured HTTP(S) proxies and blacklists IPs based on the IP range blacklist/whitelist. """ - proxies = getproxies() + proxies = getproxies_environment() return SimpleHttpClient( self, ip_whitelist=self.config.ip_range_whitelist, From 18074738940c50288b75e8ec3b4a18126a00cd62 Mon Sep 17 00:00:00 2001 From: tzyl Date: Thu, 11 Feb 2021 10:12:59 +0000 Subject: [PATCH 5/8] Revert "Use getproxies_environment directly" This reverts commit d84117ff4ba7671386fd314c52d022dd75d03dac. --- synapse/rest/media/v1/preview_url_resource.py | 4 ++-- synapse/server.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 3fef7c3d7b63..27909b2a8684 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -25,7 +25,7 @@ import traceback from typing import TYPE_CHECKING, Any, Dict, Generator, Iterable, Optional, Union from urllib import parse as urlparse -from urllib.request import getproxies_environment +from urllib.request import getproxies import attr @@ -145,7 +145,7 @@ def __init__( self.max_spider_size = hs.config.max_spider_size self.server_name = hs.hostname self.store = hs.get_datastore() - proxies = getproxies_environment() + proxies = getproxies() self.client = SimpleHttpClient( hs, treq_args={"browser_like_redirects": True}, diff --git a/synapse/server.py b/synapse/server.py index 813bdb7ae129..1dd15d12d353 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -35,7 +35,7 @@ Union, cast, ) -from urllib.request import getproxies_environment +from urllib.request import getproxies import twisted.internet.base import twisted.internet.tcp @@ -370,7 +370,7 @@ def get_proxied_http_client(self) -> SimpleHttpClient: """ An HTTP client that uses configured HTTP(S) proxies. """ - proxies = getproxies_environment() + proxies = getproxies() return SimpleHttpClient( self, http_proxy=proxies["http"].encode() if "http" in proxies else None, @@ -384,7 +384,7 @@ def get_proxied_blacklisted_http_client(self) -> SimpleHttpClient: An HTTP client that uses configured HTTP(S) proxies and blacklists IPs based on the IP range blacklist/whitelist. """ - proxies = getproxies_environment() + proxies = getproxies() return SimpleHttpClient( self, ip_whitelist=self.config.ip_range_whitelist, From b2653d9a71e1154d7abbb46f6bea5c4dcbfe1433 Mon Sep 17 00:00:00 2001 From: tzyl Date: Thu, 25 Feb 2021 11:10:03 +0000 Subject: [PATCH 6/8] Address review --- synapse/http/client.py | 13 ++--- synapse/http/proxyagent.py | 31 ++++++----- synapse/rest/media/v1/preview_url_resource.py | 6 +- synapse/server.py | 14 +---- tests/http/test_proxyagent.py | 55 +++++++++++-------- 5 files changed, 57 insertions(+), 62 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 2f82e6e1066c..36d2a110c44a 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -289,9 +289,7 @@ def __init__( treq_args: Dict[str, Any] = {}, ip_whitelist: Optional[IPSet] = None, ip_blacklist: Optional[IPSet] = None, - http_proxy: Optional[bytes] = None, - https_proxy: Optional[bytes] = None, - no_proxy: Optional[bytes] = None, + use_proxy: bool = False, ): """ Args: @@ -301,9 +299,8 @@ def __init__( we may not request. ip_whitelist: The whitelisted IP addresses, that we can request if it were otherwise caught in a blacklist. - http_proxy: proxy server to use for http connections. host[:port] - https_proxy: proxy server to use for https connections. host[:port] - no_proxy: locations that should explicitly not use a proxy. + use_proxy: Whether proxy settings should be discovered and used + from conventional environment variables. Defaults to false. """ self.hs = hs @@ -347,9 +344,7 @@ def __init__( connectTimeout=15, contextFactory=self.hs.get_http_client_context_factory(), pool=pool, - http_proxy=http_proxy, - https_proxy=https_proxy, - no_proxy=no_proxy, + use_proxy=use_proxy, ) if self._ip_blacklist: diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 2eb1a6b43247..7585d63ab2de 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -14,7 +14,7 @@ # limitations under the License. import logging import re -from urllib.request import proxy_bypass_environment +from urllib.request import getproxies_environment, proxy_bypass_environment from zope.interface import implementer @@ -60,11 +60,8 @@ class ProxyAgent(_AgentBase): pool (HTTPConnectionPool|None): connection pool to be used. If None, a non-persistent pool instance will be created. - http_proxy (bytes): Proxy server to use for http connections. host[:port] - - https_proxy (bytes): Proxy server to use for https connections. host[:port] - - no_proxy (bytes): Locations that should explicitly not use a proxy. + use_proxy (bool): Whether proxy settings should be discovered and used + from conventional environment variables. Defaults to false. """ def __init__( @@ -75,9 +72,7 @@ def __init__( connectTimeout=None, bindAddress=None, pool=None, - http_proxy=None, - https_proxy=None, - no_proxy=None, + use_proxy=False, ): _AgentBase.__init__(self, reactor, pool) @@ -92,6 +87,15 @@ def __init__( if bindAddress is not None: self._endpoint_kwargs["bindAddress"] = bindAddress + http_proxy = None + https_proxy = None + no_proxy = None + if use_proxy: + proxies = getproxies_environment() + http_proxy = proxies["http"].encode() if "http" in proxies else None + https_proxy = proxies["https"].encode() if "https" in proxies else None + no_proxy = proxies["no"].encode() if "no" in proxies else None + self.http_proxy_endpoint = _http_proxy_endpoint( http_proxy, self.proxy_reactor, **self._endpoint_kwargs ) @@ -148,13 +152,12 @@ def request(self, method, uri, headers=None, bodyProducer=None): parsed_uri = URI.fromBytes(uri) pool_key = (parsed_uri.scheme, parsed_uri.host, parsed_uri.port) request_path = parsed_uri.originForm - should_skip_proxy = ( - proxy_bypass_environment( + + should_skip_proxy = False + if self.no_proxy is not None: + should_skip_proxy = proxy_bypass_environment( parsed_uri.host.decode(), proxies={"no": self.no_proxy.decode()}, ) - if self.no_proxy is not None - else False - ) if ( parsed_uri.scheme == b"http" diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 27909b2a8684..3e4566464bbc 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -25,7 +25,6 @@ import traceback from typing import TYPE_CHECKING, Any, Dict, Generator, Iterable, Optional, Union from urllib import parse as urlparse -from urllib.request import getproxies import attr @@ -145,15 +144,12 @@ def __init__( self.max_spider_size = hs.config.max_spider_size self.server_name = hs.hostname self.store = hs.get_datastore() - proxies = getproxies() self.client = SimpleHttpClient( hs, treq_args={"browser_like_redirects": True}, ip_whitelist=hs.config.url_preview_ip_range_whitelist, ip_blacklist=hs.config.url_preview_ip_range_blacklist, - http_proxy=proxies["http"].encode() if "http" in proxies else None, - https_proxy=proxies["https"].encode() if "https" in proxies else None, - no_proxy=proxies["no"].encode() if "no" in proxies else None, + use_proxy=True, ) self.media_repo = media_repo self.primary_base_path = media_repo.primary_base_path diff --git a/synapse/server.py b/synapse/server.py index 1dd15d12d353..91d59b755a0e 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -35,7 +35,6 @@ Union, cast, ) -from urllib.request import getproxies import twisted.internet.base import twisted.internet.tcp @@ -370,13 +369,7 @@ def get_proxied_http_client(self) -> SimpleHttpClient: """ An HTTP client that uses configured HTTP(S) proxies. """ - proxies = getproxies() - return SimpleHttpClient( - self, - http_proxy=proxies["http"].encode() if "http" in proxies else None, - https_proxy=proxies["https"].encode() if "https" in proxies else None, - no_proxy=proxies["no"].encode() if "no" in proxies else None, - ) + return SimpleHttpClient(self, use_proxy=True) @cache_in_self def get_proxied_blacklisted_http_client(self) -> SimpleHttpClient: @@ -384,14 +377,11 @@ def get_proxied_blacklisted_http_client(self) -> SimpleHttpClient: An HTTP client that uses configured HTTP(S) proxies and blacklists IPs based on the IP range blacklist/whitelist. """ - proxies = getproxies() return SimpleHttpClient( self, ip_whitelist=self.config.ip_range_whitelist, ip_blacklist=self.config.ip_range_blacklist, - http_proxy=proxies["http"].encode() if "http" in proxies else None, - https_proxy=proxies["https"].encode() if "https" in proxies else None, - no_proxy=proxies["no"].encode() if "no" in proxies else None, + use_proxy=True, ) @cache_in_self diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 1aca8a2dcccd..1d5627937006 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -13,6 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +import os +from unittest.mock import patch import treq from netaddr import IPSet @@ -148,38 +150,47 @@ def test_https_request(self): agent = ProxyAgent(self.reactor, contextFactory=get_test_https_policy()) self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") + @patch.dict(os.environ, {}) + def test_http_request_use_proxy_no_environment(self): + agent = ProxyAgent(self.reactor, use_proxy=True) + self._test_request_no_proxy(agent, b"http", b"test.com", b"") + + @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "NO_PROXY": "test.com"}) + def test_http_request_via_uppercase_no_proxy(self): + agent = ProxyAgent(self.reactor, use_proxy=True) + self._test_request_no_proxy(agent, b"http", b"test.com", b"") + + @patch.dict( + os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "test.com,unused.com"} + ) def test_http_request_via_no_proxy(self): - agent = ProxyAgent( - self.reactor, http_proxy=b"proxy.com:8888", no_proxy=b"test.com,unused.com" - ) + agent = ProxyAgent(self.reactor, use_proxy=True) self._test_request_no_proxy(agent, b"http", b"test.com", b"") + @patch.dict( + os.environ, {"https_proxy": "proxy.com", "no_proxy": "test.com,unused.com"} + ) def test_https_request_via_no_proxy(self): agent = ProxyAgent( - self.reactor, - contextFactory=get_test_https_policy(), - https_proxy=b"proxy.com", - no_proxy=b"test.com,unused.com", + self.reactor, contextFactory=get_test_https_policy(), use_proxy=True, ) self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") + @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "*"}) def test_http_request_via_no_proxy_star(self): - agent = ProxyAgent(self.reactor, http_proxy=b"proxy.com:8888", no_proxy=b"*") + agent = ProxyAgent(self.reactor, use_proxy=True) self._test_request_no_proxy(agent, b"http", b"test.com", b"") + @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "*"}) def test_https_request_via_no_proxy_star(self): agent = ProxyAgent( - self.reactor, - contextFactory=get_test_https_policy(), - https_proxy=b"proxy.com", - no_proxy=b"*", + self.reactor, contextFactory=get_test_https_policy(), use_proxy=True, ) self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") + @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "unused.com"}) def test_http_request_via_proxy(self): - agent = ProxyAgent( - self.reactor, http_proxy=b"proxy.com:8888", no_proxy=b"unused.com" - ) + agent = ProxyAgent(self.reactor, use_proxy=True) self.reactor.lookups["proxy.com"] = "1.2.3.5" d = agent.request(b"GET", b"http://test.com") @@ -215,12 +226,10 @@ def test_http_request_via_proxy(self): body = self.successResultOf(treq.content(resp)) self.assertEqual(body, b"result") + @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) def test_https_request_via_proxy(self): agent = ProxyAgent( - self.reactor, - contextFactory=get_test_https_policy(), - https_proxy=b"proxy.com", - no_proxy=b"unused.com", + self.reactor, contextFactory=get_test_https_policy(), use_proxy=True, ) self.reactor.lookups["proxy.com"] = "1.2.3.5" @@ -296,6 +305,7 @@ def test_https_request_via_proxy(self): body = self.successResultOf(treq.content(resp)) self.assertEqual(body, b"result") + @patch.dict(os.environ, {"http_proxy": "proxy.com:8888"}) def test_http_request_via_proxy_with_blacklist(self): # The blacklist includes the configured proxy IP. agent = ProxyAgent( @@ -303,7 +313,7 @@ def test_http_request_via_proxy_with_blacklist(self): self.reactor, ip_whitelist=None, ip_blacklist=IPSet(["1.0.0.0/8"]) ), self.reactor, - http_proxy=b"proxy.com:8888", + use_proxy=True, ) self.reactor.lookups["proxy.com"] = "1.2.3.5" @@ -340,7 +350,8 @@ def test_http_request_via_proxy_with_blacklist(self): body = self.successResultOf(treq.content(resp)) self.assertEqual(body, b"result") - def test_https_request_via_proxy_with_blacklist(self): + @patch.dict(os.environ, {"HTTPS_PROXY": "proxy.com"}) + def test_https_request_via_uppercase_proxy_with_blacklist(self): # The blacklist includes the configured proxy IP. agent = ProxyAgent( BlacklistingReactorWrapper( @@ -348,7 +359,7 @@ def test_https_request_via_proxy_with_blacklist(self): ), self.reactor, contextFactory=get_test_https_policy(), - https_proxy=b"proxy.com", + use_proxy=True, ) self.reactor.lookups["proxy.com"] = "1.2.3.5" From dc9a1b4685b0102402ae939e7255f50b7629dbe6 Mon Sep 17 00:00:00 2001 From: tzyl Date: Thu, 25 Feb 2021 11:33:04 +0000 Subject: [PATCH 7/8] Format --- synapse/http/proxyagent.py | 3 ++- tests/http/test_proxyagent.py | 12 +++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 7585d63ab2de..92851097f57f 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -156,7 +156,8 @@ def request(self, method, uri, headers=None, bodyProducer=None): should_skip_proxy = False if self.no_proxy is not None: should_skip_proxy = proxy_bypass_environment( - parsed_uri.host.decode(), proxies={"no": self.no_proxy.decode()}, + parsed_uri.host.decode(), + proxies={"no": self.no_proxy.decode()}, ) if ( diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 1d5627937006..1e338eca173e 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -172,7 +172,9 @@ def test_http_request_via_no_proxy(self): ) def test_https_request_via_no_proxy(self): agent = ProxyAgent( - self.reactor, contextFactory=get_test_https_policy(), use_proxy=True, + self.reactor, + contextFactory=get_test_https_policy(), + use_proxy=True, ) self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") @@ -184,7 +186,9 @@ def test_http_request_via_no_proxy_star(self): @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "*"}) def test_https_request_via_no_proxy_star(self): agent = ProxyAgent( - self.reactor, contextFactory=get_test_https_policy(), use_proxy=True, + self.reactor, + contextFactory=get_test_https_policy(), + use_proxy=True, ) self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") @@ -229,7 +233,9 @@ def test_http_request_via_proxy(self): @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "unused.com"}) def test_https_request_via_proxy(self): agent = ProxyAgent( - self.reactor, contextFactory=get_test_https_policy(), use_proxy=True, + self.reactor, + contextFactory=get_test_https_policy(), + use_proxy=True, ) self.reactor.lookups["proxy.com"] = "1.2.3.5" From b468bd863ae80e6b1dfc24b3cb5a91e708ddc496 Mon Sep 17 00:00:00 2001 From: tzyl Date: Fri, 26 Feb 2021 13:22:03 +0000 Subject: [PATCH 8/8] Address review --- synapse/http/client.py | 2 +- synapse/http/proxyagent.py | 6 +++--- tests/http/test_proxyagent.py | 32 +++++++++++++++++++++----------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 6b9229edb82a..a910548f1edb 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -300,7 +300,7 @@ def __init__( ip_whitelist: The whitelisted IP addresses, that we can request if it were otherwise caught in a blacklist. use_proxy: Whether proxy settings should be discovered and used - from conventional environment variables. Defaults to false. + from conventional environment variables. """ self.hs = hs diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 92851097f57f..3d553ae236cf 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -61,7 +61,7 @@ class ProxyAgent(_AgentBase): non-persistent pool instance will be created. use_proxy (bool): Whether proxy settings should be discovered and used - from conventional environment variables. Defaults to false. + from conventional environment variables. """ def __init__( @@ -94,7 +94,7 @@ def __init__( proxies = getproxies_environment() http_proxy = proxies["http"].encode() if "http" in proxies else None https_proxy = proxies["https"].encode() if "https" in proxies else None - no_proxy = proxies["no"].encode() if "no" in proxies else None + no_proxy = proxies["no"] if "no" in proxies else None self.http_proxy_endpoint = _http_proxy_endpoint( http_proxy, self.proxy_reactor, **self._endpoint_kwargs @@ -157,7 +157,7 @@ def request(self, method, uri, headers=None, bodyProducer=None): if self.no_proxy is not None: should_skip_proxy = proxy_bypass_environment( parsed_uri.host.decode(), - proxies={"no": self.no_proxy.decode()}, + proxies={"no": self.no_proxy}, ) if ( diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 1e338eca173e..505ffcd300fc 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -102,7 +102,18 @@ def _make_connection( return http_protocol - def _test_request_no_proxy(self, agent, scheme, hostname, path): + def _test_request_direct_connection(self, agent, scheme, hostname, path): + """Runs a test case for a direct connection not going through a proxy. + + Args: + agent (ProxyAgent): the proxy agent being tested + + scheme (bytes): expected to be either "http" or "https" + + hostname (bytes): the hostname to connect to in the test + + path (bytes): the path to connect to in the test + """ is_https = scheme == b"https" self.reactor.lookups[hostname.decode()] = "1.2.3.4" @@ -144,28 +155,27 @@ def _test_request_no_proxy(self, agent, scheme, hostname, path): def test_http_request(self): agent = ProxyAgent(self.reactor) - self._test_request_no_proxy(agent, b"http", b"test.com", b"") + self._test_request_direct_connection(agent, b"http", b"test.com", b"") def test_https_request(self): agent = ProxyAgent(self.reactor, contextFactory=get_test_https_policy()) - self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") + self._test_request_direct_connection(agent, b"https", b"test.com", b"abc") - @patch.dict(os.environ, {}) - def test_http_request_use_proxy_no_environment(self): + def test_http_request_use_proxy_empty_environment(self): agent = ProxyAgent(self.reactor, use_proxy=True) - self._test_request_no_proxy(agent, b"http", b"test.com", b"") + self._test_request_direct_connection(agent, b"http", b"test.com", b"") @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "NO_PROXY": "test.com"}) def test_http_request_via_uppercase_no_proxy(self): agent = ProxyAgent(self.reactor, use_proxy=True) - self._test_request_no_proxy(agent, b"http", b"test.com", b"") + self._test_request_direct_connection(agent, b"http", b"test.com", b"") @patch.dict( os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "test.com,unused.com"} ) def test_http_request_via_no_proxy(self): agent = ProxyAgent(self.reactor, use_proxy=True) - self._test_request_no_proxy(agent, b"http", b"test.com", b"") + self._test_request_direct_connection(agent, b"http", b"test.com", b"") @patch.dict( os.environ, {"https_proxy": "proxy.com", "no_proxy": "test.com,unused.com"} @@ -176,12 +186,12 @@ def test_https_request_via_no_proxy(self): contextFactory=get_test_https_policy(), use_proxy=True, ) - self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") + self._test_request_direct_connection(agent, b"https", b"test.com", b"abc") @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "*"}) def test_http_request_via_no_proxy_star(self): agent = ProxyAgent(self.reactor, use_proxy=True) - self._test_request_no_proxy(agent, b"http", b"test.com", b"") + self._test_request_direct_connection(agent, b"http", b"test.com", b"") @patch.dict(os.environ, {"https_proxy": "proxy.com", "no_proxy": "*"}) def test_https_request_via_no_proxy_star(self): @@ -190,7 +200,7 @@ def test_https_request_via_no_proxy_star(self): contextFactory=get_test_https_policy(), use_proxy=True, ) - self._test_request_no_proxy(agent, b"https", b"test.com", b"abc") + self._test_request_direct_connection(agent, b"https", b"test.com", b"abc") @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "unused.com"}) def test_http_request_via_proxy(self):