From c7603af1d06d65932c420ae76002b6ed94dbf23c Mon Sep 17 00:00:00 2001
From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 15 Jul 2021 11:37:08 +0200
Subject: [PATCH] Allow providing credentials to `http_proxy` (#10360)

---
 changelog.d/10360.feature     |  1 +
 synapse/http/proxyagent.py    | 12 ++++++-
 tests/http/test_proxyagent.py | 65 ++++++++++++++++++++++++++++-------
 3 files changed, 64 insertions(+), 14 deletions(-)
 create mode 100644 changelog.d/10360.feature

diff --git a/changelog.d/10360.feature b/changelog.d/10360.feature
new file mode 100644
index 0000000000..904221cb6d
--- /dev/null
+++ b/changelog.d/10360.feature
@@ -0,0 +1 @@
+Allow providing credentials to `http_proxy`.
\ No newline at end of file
diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py
index 7dfae8b786..7a6a1717de 100644
--- a/synapse/http/proxyagent.py
+++ b/synapse/http/proxyagent.py
@@ -117,7 +117,8 @@ class ProxyAgent(_AgentBase):
             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
+        # Parse credentials from http and https proxy connection string if present
+        self.http_proxy_creds, http_proxy = parse_username_password(http_proxy)
         self.https_proxy_creds, https_proxy = parse_username_password(https_proxy)
 
         self.http_proxy_endpoint = _http_proxy_endpoint(
@@ -189,6 +190,15 @@ class ProxyAgent(_AgentBase):
             and self.http_proxy_endpoint
             and not should_skip_proxy
         ):
+            # Determine whether we need to set Proxy-Authorization headers
+            if self.http_proxy_creds:
+                # Set a Proxy-Authorization header
+                if headers is None:
+                    headers = Headers()
+                headers.addRawHeader(
+                    b"Proxy-Authorization",
+                    self.http_proxy_creds.as_proxy_authorization_value(),
+                )
             # 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)
diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py
index fefc8099c9..437113929a 100644
--- a/tests/http/test_proxyagent.py
+++ b/tests/http/test_proxyagent.py
@@ -205,6 +205,41 @@ class MatrixFederationAgentTests(TestCase):
 
     @patch.dict(os.environ, {"http_proxy": "proxy.com:8888", "no_proxy": "unused.com"})
     def test_http_request_via_proxy(self):
+        """
+        Tests that requests can be made through a proxy.
+        """
+        self._do_http_request_via_proxy(auth_credentials=None)
+
+    @patch.dict(
+        os.environ,
+        {"http_proxy": "bob:pinkponies@proxy.com:8888", "no_proxy": "unused.com"},
+    )
+    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(auth_credentials="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(auth_credentials=None)
+
+    @patch.dict(
+        os.environ,
+        {"https_proxy": "bob:pinkponies@proxy.com", "no_proxy": "unused.com"},
+    )
+    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_http_request_via_proxy(
+        self,
+        auth_credentials: Optional[str] = None,
+    ):
+        """
+        Tests that requests can be made through a proxy.
+        """
         agent = ProxyAgent(self.reactor, use_proxy=True)
 
         self.reactor.lookups["proxy.com"] = "1.2.3.5"
@@ -229,6 +264,23 @@ class MatrixFederationAgentTests(TestCase):
         self.assertEqual(len(http_server.requests), 1)
 
         request = http_server.requests[0]
+
+        # 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(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)
+
         self.assertEqual(request.method, b"GET")
         self.assertEqual(request.path, b"http://test.com")
         self.assertEqual(request.requestHeaders.getRawHeaders(b"host"), [b"test.com"])
@@ -241,19 +293,6 @@ class MatrixFederationAgentTests(TestCase):
         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_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,