From 3433e1e78ff42b29b2752769abb6fe3b99246974 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 19 Mar 2021 18:32:50 +0000 Subject: [PATCH 1/7] Handle proxy credentials for HTTPS_PROXY env vars --- synapse/http/connectproxyclient.py | 41 ++++++++++++++--- synapse/http/proxyagent.py | 73 +++++++++++++++++++++++++++--- 2 files changed, 101 insertions(+), 13 deletions(-) diff --git a/synapse/http/connectproxyclient.py b/synapse/http/connectproxyclient.py index 856e28454fad..1b6b6f67549a 100644 --- a/synapse/http/connectproxyclient.py +++ b/synapse/http/connectproxyclient.py @@ -22,6 +22,7 @@ from twisted.internet.interfaces import IStreamClientEndpoint from twisted.internet.protocol import connectionDone from twisted.web import http +from twisted.web.http_headers import Headers logger = logging.getLogger(__name__) @@ -47,19 +48,23 @@ class HTTPConnectProxyEndpoint: proxy host (bytes): hostname that we want to CONNECT to port (int): port that we want to connect to + headers: Extra HTTP headers to include in the CONNECT request """ - def __init__(self, reactor, proxy_endpoint, host, port): + def __init__(self, reactor, proxy_endpoint, host, port, headers): self._reactor = reactor self._proxy_endpoint = proxy_endpoint self._host = host self._port = port + self._headers = headers def __repr__(self): return "" % (self._proxy_endpoint,) def connect(self, protocolFactory): - f = HTTPProxiedClientFactory(self._host, self._port, protocolFactory) + f = HTTPProxiedClientFactory( + self._host, self._port, protocolFactory, self._headers + ) d = self._proxy_endpoint.connect(f) # once the tcp socket connects successfully, we need to wait for the # CONNECT to complete. @@ -77,12 +82,14 @@ class HTTPProxiedClientFactory(protocol.ClientFactory): dst_host (bytes): hostname that we want to CONNECT to dst_port (int): port that we want to connect to wrapped_factory (protocol.ClientFactory): The original Factory + headers: Extra HTTP headers to include in the CONNECT request """ - def __init__(self, dst_host, dst_port, wrapped_factory): + def __init__(self, dst_host, dst_port, wrapped_factory, headers): self.dst_host = dst_host self.dst_port = dst_port self.wrapped_factory = wrapped_factory + self.headers = headers self.on_connection = defer.Deferred() def startedConnecting(self, connector): @@ -92,7 +99,11 @@ def buildProtocol(self, addr): wrapped_protocol = self.wrapped_factory.buildProtocol(addr) return HTTPConnectProtocol( - self.dst_host, self.dst_port, wrapped_protocol, self.on_connection + self.dst_host, + self.dst_port, + wrapped_protocol, + self.on_connection, + self.headers, ) def clientConnectionFailed(self, connector, reason): @@ -122,14 +133,22 @@ class HTTPConnectProtocol(protocol.Protocol): connected_deferred (Deferred): a Deferred which will be callbacked with wrapped_protocol when the CONNECT completes + + headers: Extra HTTP headers to include in the CONNECT request """ - def __init__(self, host, port, wrapped_protocol, connected_deferred): + def __init__( + self, host, port, wrapped_protocol, connected_deferred, headers: Headers + ): self.host = host self.port = port self.wrapped_protocol = wrapped_protocol self.connected_deferred = connected_deferred - self.http_setup_client = HTTPConnectSetupClient(self.host, self.port) + self.headers = headers + + self.http_setup_client = HTTPConnectSetupClient( + self.host, self.port, self.headers + ) self.http_setup_client.on_connected.addCallback(self.proxyConnected) def connectionMade(self): @@ -170,16 +189,24 @@ class HTTPConnectSetupClient(http.HTTPClient): Args: host (bytes): The hostname to send in the CONNECT message port (int): The port to send in the CONNECT message + headers: Extra headers to send with the CONNECT message """ - def __init__(self, host, port): + def __init__(self, host, port, headers: Headers): self.host = host self.port = port + self.headers = headers 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) + self.endHeaders() def handleStatus(self, version, status, message): diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 3d553ae236cf..814e0218ed81 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -12,10 +12,13 @@ # 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 Optional, Tuple from urllib.request import getproxies_environment, proxy_bypass_environment +import attr from zope.interface import implementer from twisted.internet import defer @@ -23,6 +26,7 @@ from twisted.python.failure import Failure from twisted.web.client import URI, BrowserLikePolicyForHTTPS, _AgentBase from twisted.web.error import SchemeNotSupported +from twisted.web.http_headers import Headers from twisted.web.iweb import IAgent from synapse.http.connectproxyclient import HTTPConnectProxyEndpoint @@ -32,6 +36,22 @@ _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 @@ -96,6 +116,9 @@ def __init__( https_proxy = proxies["https"].encode() if "https" in proxies else None no_proxy = proxies["no"] if "no" in proxies else None + # 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( http_proxy, self.proxy_reactor, **self._endpoint_kwargs ) @@ -175,11 +198,22 @@ def request(self, method, uri, headers=None, bodyProducer=None): 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, ) else: # not using a proxy @@ -223,16 +257,43 @@ def _http_proxy_endpoint(proxy, reactor, **kwargs): if proxy is None: return None - # currently we only support hostname:port. Some apps also support - # protocol://[:port], which allows a way of requiring a TLS connection to the - # proxy. - + # Parse the connection string, supporting the form: [:@][:port] host, port = parse_host_port(proxy, default_port=1080) return HostnameEndpoint(reactor, host, port, **kwargs) -def parse_host_port(hostport, default_port=None): - # could have sworn we had one of these somewhere else... +def parse_username_password(proxy: bytes) -> Tuple[Optional[ProxyCredentials], bytes]: + """ + Parses the username and password from a proxy declaration e.g + username:password@hostname:port. + + Args: + proxy: The proxy connection string. + + Returns + An instance of ProxyCredentials and the proxy connection string with any credentials + stripped, i.e u:p@host:port -> host:port. If no credentials were found, the + ProxyCredentials instance is replaced with None. + """ + if proxy and b"@" in proxy: + # We use rsplit here as the password could contain an @ character + credentials, proxy_without_credentials = proxy.rsplit(b"@", 1) + return ProxyCredentials(credentials), proxy_without_credentials + + return None, proxy + + +def parse_host_port(hostport: bytes, default_port: int = None) -> Tuple[bytes, int]: + """ + Parse the hostname and port from a proxy connection byte string. + + Args: + hostport: The proxy connection string. Must be in the form 'host[:port]'. + default_port: The default port to return if one is not found in `hostport`. + + Returns: + A tuple containing the hostname and port. Uses `default_port` if one was not found. + """ if b":" in hostport: host, port = hostport.rsplit(b":", 1) try: From 4a3190160043a3afabd499af3068d14279cab7d7 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 19 Mar 2021 18:33:07 +0000 Subject: [PATCH 2/7] Test proxy credentials in HTTPS_PROXY --- tests/http/test_proxyagent.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index 505ffcd300fc..b8120513f382 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -12,6 +12,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 unittest.mock import patch @@ -240,7 +241,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"}) + @patch.dict( + os.environ, + {"https_proxy": "bob:pinkponies@proxy.com", "no_proxy": "unused.com"}, + ) def test_https_request_via_proxy(self): agent = ProxyAgent( self.reactor, @@ -278,6 +282,16 @@ def test_https_request_via_proxy(self): self.assertEqual(request.method, b"CONNECT") self.assertEqual(request.path, b"test.com:443") + # Check that credentials have been supplied for the proxy + proxy_auth_header_values = request.requestHeaders.getRawHeaders( + b"Proxy-Authorization" + ) + + # Compute the correct header value for Proxy-Authorization + encoded_credentials = base64.b64encode(b"bob:pinkponies") + expected_header_value = b"Basic " + encoded_credentials + self.assertIn(expected_header_value, proxy_auth_header_values) + # tell the proxy server not to close the connection proxy_server.persistent = True @@ -312,6 +326,13 @@ def test_https_request_via_proxy(self): self.assertEqual(request.method, b"GET") self.assertEqual(request.path, b"/abc") self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [b"test.com"]) + + # Check that the destination server DID NOT receive proxy credentials + proxy_auth_header_values = request.requestHeaders.getRawHeaders( + b"Proxy-Authorization" + ) + self.assertIsNone(proxy_auth_header_values) + request.write(b"result") request.finish() From 21a334a5188d2992fd6c8049fa5d3fb7f3befb2c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 19 Mar 2021 18:48:13 +0000 Subject: [PATCH 3/7] Add additional type hints --- synapse/http/connectproxyclient.py | 63 +++++++++++++++++++----------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/synapse/http/connectproxyclient.py b/synapse/http/connectproxyclient.py index 1b6b6f67549a..b797e3ce80b3 100644 --- a/synapse/http/connectproxyclient.py +++ b/synapse/http/connectproxyclient.py @@ -19,8 +19,8 @@ from twisted.internet import defer, protocol from twisted.internet.error import ConnectError -from twisted.internet.interfaces import IStreamClientEndpoint -from twisted.internet.protocol import connectionDone +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 @@ -44,14 +44,20 @@ class HTTPConnectProxyEndpoint: Args: reactor: the Twisted reactor to use for the connection - proxy_endpoint (IStreamClientEndpoint): the endpoint to use to connect to the - proxy - host (bytes): hostname that we want to CONNECT to - port (int): port that we want to connect to + 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 """ - def __init__(self, reactor, proxy_endpoint, host, port, headers): + def __init__( + self, + reactor: IReactorCore, + proxy_endpoint: IStreamClientEndpoint, + host: bytes, + port: int, + headers: Headers, + ): self._reactor = reactor self._proxy_endpoint = proxy_endpoint self._host = host @@ -61,7 +67,7 @@ def __init__(self, reactor, proxy_endpoint, host, port, headers): def __repr__(self): return "" % (self._proxy_endpoint,) - def connect(self, protocolFactory): + def connect(self, protocolFactory: ClientFactory): f = HTTPProxiedClientFactory( self._host, self._port, protocolFactory, self._headers ) @@ -79,13 +85,19 @@ class HTTPProxiedClientFactory(protocol.ClientFactory): HTTP Protocol object and run the rest of the connection. Args: - dst_host (bytes): hostname that we want to CONNECT to - dst_port (int): port that we want to connect to - wrapped_factory (protocol.ClientFactory): The original Factory + 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 """ - def __init__(self, dst_host, dst_port, wrapped_factory, headers): + def __init__( + self, + dst_host: bytes, + dst_port: int, + wrapped_factory: ClientFactory, + headers: Headers, + ): self.dst_host = dst_host self.dst_port = dst_port self.wrapped_factory = wrapped_factory @@ -123,22 +135,27 @@ class HTTPConnectProtocol(protocol.Protocol): """Protocol that wraps an existing Protocol to do a CONNECT handshake at connect Args: - host (bytes): The original HTTP(s) hostname or IPv4 or IPv6 address literal + host: The original HTTP(s) hostname or IPv4 or IPv6 address literal to put in the CONNECT request - port (int): The original HTTP(s) port to put in the CONNECT request + port: The original HTTP(s) port to put in the CONNECT request - wrapped_protocol (interfaces.IProtocol): the original protocol (probably - HTTPChannel or TLSMemoryBIOProtocol, but could be anything really) + wrapped_protocol: the original protocol (probably HTTPChannel or + TLSMemoryBIOProtocol, but could be anything really) - connected_deferred (Deferred): a Deferred which will be callbacked with + 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 """ def __init__( - self, host, port, wrapped_protocol, connected_deferred, headers: Headers + self, + host: bytes, + port: int, + wrapped_protocol: Protocol, + connected_deferred: defer.Deferred, + headers: Headers, ): self.host = host self.port = port @@ -173,7 +190,7 @@ def proxyConnected(self, _): if buf: self.wrapped_protocol.dataReceived(buf) - def dataReceived(self, data): + def dataReceived(self, data: bytes): # if we've set up the HTTP protocol, we can send the data there if self.wrapped_protocol.connected: return self.wrapped_protocol.dataReceived(data) @@ -187,12 +204,12 @@ class HTTPConnectSetupClient(http.HTTPClient): """HTTPClient protocol to send a CONNECT message for proxies and read the response. Args: - host (bytes): The hostname to send in the CONNECT message - port (int): The port to send in the CONNECT message + 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 """ - def __init__(self, host, port, headers: Headers): + def __init__(self, host: bytes, port: int, headers: Headers): self.host = host self.port = port self.headers = headers @@ -209,7 +226,7 @@ def connectionMade(self): self.endHeaders() - def handleStatus(self, version, status, message): + 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) From 740ef5402f2179e1a2d77a804e0664734dcfad8c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Sat, 20 Mar 2021 00:52:54 +0000 Subject: [PATCH 4/7] Changelog --- changelog.d/9657.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9657.feature diff --git a/changelog.d/9657.feature b/changelog.d/9657.feature new file mode 100644 index 000000000000..976b4bee6cc7 --- /dev/null +++ b/changelog.d/9657.feature @@ -0,0 +1 @@ +Support credentials for proxy authentication in the `HTTPS_PROXY` environment variable. \ No newline at end of file From ed1b12a47274fe31f93c17ec2fe6d077706e4710 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 22 Mar 2021 16:15:59 +0000 Subject: [PATCH 5/7] Test HTTPS proxy with and without auth credentials --- tests/http/test_proxyagent.py | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py index b8120513f382..3ea8b5bec7ec 100644 --- a/tests/http/test_proxyagent.py +++ b/tests/http/test_proxyagent.py @@ -15,6 +15,7 @@ import base64 import logging import os +from typing import Optional from unittest.mock import patch import treq @@ -241,11 +242,23 @@ 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): + """Tests that TLS-encrypted requests can be made through a proxy""" + self._do_https_request_via_proxy(auth_credentials=None) + @patch.dict( os.environ, {"https_proxy": "bob:pinkponies@proxy.com", "no_proxy": "unused.com"}, ) - 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(auth_credentials="bob:pinkponies") + + def _do_https_request_via_proxy( + self, + auth_credentials: Optional[str] = None, + ): agent = ProxyAgent( self.reactor, contextFactory=get_test_https_policy(), @@ -282,15 +295,21 @@ def test_https_request_via_proxy(self): self.assertEqual(request.method, b"CONNECT") self.assertEqual(request.path, b"test.com:443") - # Check that credentials have been supplied for the proxy + # Check whether auth credentials have been supplied to the proxy proxy_auth_header_values = request.requestHeaders.getRawHeaders( b"Proxy-Authorization" ) - # Compute the correct header value for Proxy-Authorization - encoded_credentials = base64.b64encode(b"bob:pinkponies") - expected_header_value = b"Basic " + encoded_credentials - self.assertIn(expected_header_value, proxy_auth_header_values) + if auth_credentials is not None: + # Compute the correct header value for Proxy-Authorization + encoded_credentials = base64.b64encode(b"bob:pinkponies") + 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 From 0c5a5b9ede58acee6fa7f9062fd35f4ebddc5e32 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 22 Mar 2021 16:24:37 +0000 Subject: [PATCH 6/7] Clarify supported proxy connection string; clean up docstring --- synapse/http/proxyagent.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py index 814e0218ed81..16ec850064dd 100644 --- a/synapse/http/proxyagent.py +++ b/synapse/http/proxyagent.py @@ -242,12 +242,16 @@ def request(self, method, uri, headers=None, bodyProducer=None): ) -def _http_proxy_endpoint(proxy, reactor, **kwargs): +def _http_proxy_endpoint(proxy: Optional[bytes], reactor, **kwargs): """Parses an http proxy setting and returns an endpoint for the proxy Args: - proxy (bytes|None): the proxy setting + proxy: the proxy setting in the form: [:@][:] + Note that compared to other apps, this function currently lacks support + for specifying a protocol schema (i.e. protocol://...). + reactor: reactor to be used to connect to the proxy + kwargs: other args to be passed to HostnameEndpoint Returns: @@ -257,7 +261,7 @@ def _http_proxy_endpoint(proxy, reactor, **kwargs): if proxy is None: return None - # Parse the connection string, supporting the form: [:@][:port] + # Parse the connection string host, port = parse_host_port(proxy, default_port=1080) return HostnameEndpoint(reactor, host, port, **kwargs) From a9a11e751bb6a3ab48d8ba7c65b5c9249bacdae0 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 22 Mar 2021 16:41:37 +0000 Subject: [PATCH 7/7] Update changelog.d/9657.feature Co-authored-by: Erik Johnston --- changelog.d/9657.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/9657.feature b/changelog.d/9657.feature index 976b4bee6cc7..c56a615a8b19 100644 --- a/changelog.d/9657.feature +++ b/changelog.d/9657.feature @@ -1 +1 @@ -Support credentials for proxy authentication in the `HTTPS_PROXY` environment variable. \ No newline at end of file +Add support for credentials for proxy authentication in the `HTTPS_PROXY` environment variable.