allow specifying https:// proxy

[Bubu]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[Bubu]

I'm trying to adapt the proxyagent tests for this... but it's not that easy to see what's going on and now having to nest two ssl connections inside each other isn't helping at all.

[clokep]

I'm trying to adapt the proxyagent tests for this... but it's not that easy to see what's going on and now having to nest two ssl connections inside each other isn't helping at all.

Please feel free to ask, probably best in #synapse-dev:matrix.org.

[Bubu]

Sorry for this being stale so long, I've rebased this on develop again (meanwhile #9372 has changed things up somewhat, so that required some cosmetic changes here as well).

The blocker preciously were missing tests, I think I found a good way to add tests for this now..

[clokep]

Looks reasonable overall! I left a few comments though.

[f1-outsourcing]

I have this issue[1], I assume it is related to half implemented proxy support. Is it possible to get some patch or so, that I can apply to the pip package matrix-synapse. I would like to try matrix chat.

Or is there some indication when proxy support will be fully implemented?

[1]
#9852

[clokep]

I think #9119 (comment) is still an oustanding comment, by the way.

[erikjohnston]

I think we're still waiting for #9119 (comment) to be dealt with, so removing review request for now. (There also seems to be some conflicts that need to be resolved)

[f1-outsourcing]

I tried this bubu https_proxy branch, but I am still getting these

matrix-synapse synapse.http.federation.matrix_federation_agent - 288 - INFO - GET-23 - Failed to connect to matrix-federation.matrix.org.cdn.cloudflare.net:8443: No route to host: 101: Network unreachable.

[anoadragon453]

Note that the context for @f1-outsourcing's setup is at #9852.

[f1-outsourcing]

@anoadragon453 @Bubu

Note that the context for @f1-outsourcing's setup is at #9852.

Will this merge make it possible to use matrix without setting a default gateway and only use a forward and reverse proxy? Because this is still not clear to me. Or is this changing just a little part of the code, and then we have to change another little part, and then we have to change another little part. etc.
The reason why I am asking is, is it worth waiting for this merge, or should I just try an find an xmpp solution?

[f1-outsourcing]

@anoadragon453 @Bubu

Note that the context for @f1-outsourcing's setup is at #9852.

Will this merge make it possible to use matrix without setting a default gateway and only use a forward and reverse proxy? Because this is still not clear to me. Or is this changing just a little part of the code, and then we have to change another little part, and then we have to change another little part. etc.
The reason why I am asking is, is it worth waiting for this merge, or should I just try an find an xmpp solution?

If you do not know, would you mind stating this then, because I do not know what to expect now.

stale review

[richvdh]

looks generally good to me! A few small comments here.

[richvdh]

this will break for https://username:password@host, I think? Probably best to call parse_proxy here

[dklimpel]

Is there a reason why only credentials were parsed for https proxy? Only security reason or anything else?

[dklimpel]

this will break for https://username:password@host, I think? Probably best to call parse_proxy here

I would suggest a solution like this.
This change can make parse_username_password compatible to https://username:password@host

diff --git a/synapse/http/proxyagent.py b/synapse/http/proxyagent.py
index c747b99..3c4fee6 100644
--- a/synapse/http/proxyagent.py
+++ b/synapse/http/proxyagent.py
@@ -293,7 +293,7 @@
 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.
+    username:password@hostname:port or https://username:password@hostname:port
 
     Args:
         proxy: The proxy connection string.
@@ -304,9 +304,15 @@
         ProxyCredentials instance is replaced with None.
     """
     if proxy and b"@" in proxy:
+        scheme, host, port = parse_proxy(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
+        credentials, proxy_without_credentials = host.rsplit(b"@", 1)
+        return (
+            ProxyCredentials(credentials),
+            b"".join(
+                [scheme, b"://", proxy_without_credentials, b":", str(port).encode()]
+            ),
+        )
 
     return None, proxy

[richvdh]

Is there a reason why only credentials were parsed for https proxy? Only security reason or anything else?

I don't think there is much reason at all. Support for credentials was added in #9657: perhaps http_proxy was just fogotten?

[richvdh]

I would suggest a solution like this.

seems sensible.

[dklimpel]

Is there a reason why only credentials were parsed for https proxy? Only security reason or anything else?

I don't think there is much reason at all. Support for credentials was added in #9657: perhaps http_proxy was just fogotten?

In addition the issue for this #9000

[richvdh]

needs support for <scheme>://<username>:<password>@<host>

[richvdh]

While I'm here, I'll just complain that it's a real shame that this code has been cut-and-pasted into sygnal, at https://github.com/matrix-org/sygnal/blob/main/sygnal/helper/proxy/proxyagent_twisted.py rather than factored out to a separate library. Now we have two versions to maintain :(.

[richvdh]

oh I hadn't realised that @Bubu had updated it since my last review. Let me stick it back in the queue for review.

[richvdh]

I think this just needs a couple of minor tweaks.

@Bubu are you still keen to work on this? It sounds like @dklimpel would be happy to finish it up if you're busy.

[richvdh]

this is a bit confusing. A hostname where?

... and yes, of course it supports http and https proxies - what other sorts of proxies would an http/https agent possibly care about?

Suggest removing these lines.

[richvdh]

Is there a reason why only credentials were parsed for https proxy? Only security reason or anything else?

I don't think there is much reason at all. Support for credentials was added in #9657: perhaps http_proxy was just fogotten?

[richvdh]

I would suggest a solution like this.

seems sensible.

[dklimpel]

I have thought about the problem with Python 3.9 and urlparse. See: #9119 (comment)

I would suggest to work with urlparse after all. For compatibility reasons we can check if there is a sheme and if not we add this. Somthing like this

Suggested change

	if b"://" in proxy:
	scheme, host = proxy.split(b"://", 1)
	if not b"://" in proxy:
	proxy = b"".join([default_scheme, b"://", proxy])

[richvdh]

ok, I'm going to close this in favour of #10411. Thanks @dklimpel for taking up the baton.

[Bubu]

Thanks for taking over! ❤️