support federation queries through http connect proxy #9306

Bubu · 2021-02-02T22:33:55Z

Can be specified by HTTPS_PROXY env var.

pass unfiltered reactor to federation agent for proxy support

Signed-off-by: Marcus Hoffmann bubu@bubu1.eu

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)

Fixes #8660

Bubu · 2021-02-02T23:21:54Z

@clokep I'd like your feedback here, some notes:

This has been running in production on an instance of synapse that only supports outgoing connections via a proxy. So I'm somewhat certain that in general it does the correct thing.
It obviously still needs tests. I didn't actually look into that yet.
this doesn't attempt to tackle the fact that the blacklisting should probably be only happening at the proxy instead of in synapse. I think this can be solved independently though.
From the will not be used for list from Support for routing outbound HTTP requests via a proxy #6239 this only adds fed traffic as there was no immediate need for the other things on that list (in fact adding some of those would need Support for proxy exceptions (e.g. NO_PROXY env var) #9088 to be solved)
This might be solving Support proxy server for outbound federation #8859, though the reasoning in that issue is currently not clear to me (ssl offloading vs. us needed that just for connectivity to other servers)
The thing I'm unsure about is how to best introduce this: It changes existing functionality if you use a proxy server for anything... but on the other hand introducing more (nonstandard) env vars or config options for this doesn't seem satisfactory either. Maybe add this together with Support for proxy exceptions (e.g. NO_PROXY env var) #9088 and pointing it out in the upgrading notes would be enough?

clokep · 2021-02-03T12:30:49Z

I didn't look in detail yet, but a quick look makes it look reasonable. Is this only HTTPS proxy support? (No HTTP proxy support?)

I've set the review flag so that someone can provide feedback!

Bubu · 2021-02-03T12:34:36Z

I didn't look in detail yet, but a quick look makes it look reasonable. Is this only HTTPS proxy support? (No HTTP proxy support?)

This is https only (target server is on https, not the proxy server like #9090) because I was under the impression that federation traffic requires a tls connection anyway. (I should be easy enough to pass through the http_proxy as well, if that is necessary for some reason.)

clokep

This has been running in production on an instance of synapse that only supports outgoing connections via a proxy. So I'm somewhat certain that in general it does the correct thing.

Awesome news! I know that there's interest by others in running with this too!

It obviously still needs tests. I didn't actually look into that yet.

Yeah...it is rather unfortunate that the federation client is separate from the other HTTP client, but I don't think fixing that is trivial.

this doesn't attempt to tackle the fact that the blacklisting should probably be only happening at the proxy instead of in synapse. I think this can be solved independently though.

I think non-federation requests somewhat have the same issue right now, so I don't see this as a problem to solve here.

From the will not be used for list from Support for routing outbound HTTP requests via a proxy #6239 this only adds fed traffic as there was no immediate need for the other things on that list (in fact adding some of those would need Support for proxy exceptions (e.g. NO_PROXY env var) #9088 to be solved)

Ideally we would eventually add proxy support for all of these locations, I think. But not 100% sure. This fixes #8660, might be worth filing an issue with the remaining places we don't use a proxy?

This might be solving Support proxy server for outbound federation #8859, though the reasoning in that issue is currently not clear to me (ssl offloading vs. us needed that just for connectivity to other servers)

This solves #8660, which I've added to the description (#8859 was marked as duplicate of #8660).

The thing I'm unsure about is how to best introduce this: It changes existing functionality if you use a proxy server for anything... but on the other hand introducing more (nonstandard) env vars or config options for this doesn't seem satisfactory either. Maybe add this together with Support for proxy exceptions (e.g. NO_PROXY env var) #9088 and pointing it out in the upgrading notes would be enough?

For #6239 it seems we just turned it on, I don't think we would do something different here. I suspect documenting it in the upgrade notes should be enough.

synapse/server.py

synapse/http/federation/matrix_federation_agent.py

tzyl · 2021-02-09T10:29:32Z

Thanks for starting this @Bubu! This would also be really useful for me and I'm running a similar patch to support a setup where outbound traffic flows through an egress proxy host. A follow up that I would also want would be to support the NO_PROXY convention environment variable (as urllib and other non-python software does) which I'm happy to contribute following this.

clokep · 2021-02-09T17:47:46Z

@Bubu based on the conversation above...do you think this is ready for more feedback or what's the next step?

### Changes proposed in this PR - Add support for the `no_proxy` and `NO_PROXY` environment variables - Internally rely on urllib's [`proxy_bypass_environment`](https://github.com/python/cpython/blob/bdb941be423bde8b02a5695ccf51c303d6204bed/Lib/urllib/request.py#L2519) - Extract env variables using urllib's `getproxies`/[`getproxies_environment`](https://github.com/python/cpython/blob/bdb941be423bde8b02a5695ccf51c303d6204bed/Lib/urllib/request.py#L2488) which supports lowercase + uppercase, preferring lowercase, except for `HTTP_PROXY` in a CGI environment This does contain behaviour changes for consumers so making sure these are called out: - `no_proxy`/`NO_PROXY` is now respected - lowercase `https_proxy` is now allowed and taken over `HTTPS_PROXY` Related to #9306 which also uses `ProxyAgent` Signed-off-by: Timothy Leung tim95@hotmail.co.uk

richvdh · 2021-04-14T09:26:00Z

@Bubu is this ready for review, or are you still working on it? It looks like a couple of conflicts have cropped up.

f1-outsourcing · 2021-04-21T15:15:49Z

I also have a problem with the federation traffic not going through the proxy. Can anyone point me to a patch that I can apply to 1.31.0 ?

Can be specified by HTTPS_PROXY env var. pass unfiltered reactor to federation agent for proxy support Signed-off-by: Marcus Hoffmann <bubu@bubu1.eu>

This is used for proxy authentication, we just continue our "solution" of copying code from proxyagent -> matrix_federation_agent.

Bubu · 2021-04-27T16:55:01Z

@richvdh I resolved the conflicts. In general review is appreciated though it still lacks functioning tests.

(I currently don't have time to look into writing those, though potentially @chrisguida wanted to look at that.)

chrisguida · 2021-05-03T18:40:24Z

@richvdh I resolved the conflicts. In general review is appreciated though it still lacks functioning tests.

(I currently don't have time to look into writing those, though potentially @chrisguida wanted to look at that.)

If anyone else wants to take on writing the test so that this can be merged, please go ahead. I've run this code and it works, but figuring out how to write the test properly will most likely take me a while since I'm not too familiar with the testing framework. Otherwise I'll get to it when time permits.

richvdh

ok, I don't really understand what's going on in this code. Please update the docstrings to help me follow it.

richvdh · 2021-05-05T14:11:40Z

synapse/http/federation/matrix_federation_agent.py

@@ -72,6 +76,7 @@ def __init__(
 tls_client_options_factory: Optional[FederationPolicyForHTTPS],
 user_agent: bytes,
 ip_blacklist: IPSet,
+ proxy_reactor: Optional[ISynapseReactor] = None,


please add new params to the docstring (on the class, in this case). As well as making the code maintainable in future, it's important to help with review.

richvdh · 2021-05-05T14:19:39Z

synapse/http/matrixfederationclient.py

@@ -279,6 +279,7 @@ def __init__(self, hs, tls_client_options_factory):
 tls_client_options_factory,
 user_agent,
 hs.config.federation_ip_range_blacklist,
+ proxy_reactor=hs.get_reactor(),


given that we already pass the ip blacklist into MatrixFederationAgent, why not move the construction of BlacklistingReactorWrapper down to it, rather than having to pass in two reactors?

dklimpel · 2021-07-09T09:23:06Z

tests/http/federation/test_matrix_federation_agent.py

@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-


Suggested change

# -*- coding: utf-8 -*-

This is no longer needed. See #9786

dklimpel · 2021-07-11T07:25:40Z

(I currently don't have time to look into writing those, though potentially @chrisguida wanted to look at that.)

If anyone else wants to take on writing the test so that this can be merged, please go ahead. I've run this code and it works, but figuring out how to write the test properly will most likely take me a while since I'm not too familiar with the testing framework. Otherwise I'll get to it when time permits.

I will work on it after #10360 and #9119 is merged.
Some of this code and tests is needed to keep it simpler and avoid conflicts.

ShadowJonathan · 2021-07-11T11:39:46Z

I'm seeing uses of urllib here, is it possible to use urllib3 for this?

My main gripe with it is that urllib3 is newer, but also that urllib produces bytes instead of str, which speaks of python 2 behaviour. Those bytes may not be UTF8, or properly parsed from the environment, so for compatibility i'd suggest trying to see if you can get urllib3 to work with this.

dklimpel · 2021-07-11T12:27:56Z

I'm seeing uses of urllib here, is it possible to use urllib3 for this?

My main gripe with it is that urllib3 is newer, but also that urllib produces bytes instead of str, which speaks of python 2 behaviour. Those bytes may not be UTF8, or properly parsed from the environment, so for compatibility i'd suggest trying to see if you can get urllib3 to work with this.

I am not sure if it is useful to use the library only here.
If would have to check them certainly in whole Synapse.

There is also a discussion about it there: #9119 (comment)

clokep · 2021-08-05T12:29:19Z

I'm going to close this in lieu of #10475 where @dklimpel picked up this work. Hopefully that's OK! 🎉

Bubu · 2021-08-05T13:17:47Z

Sure, sorry for not having the bandwidth to continue here :-(.

And thanks for picking this up @dklimpel.

Bubu marked this pull request as draft February 2, 2021 22:35

clokep requested a review from a team February 3, 2021 12:29

clokep reviewed Feb 5, 2021

View reviewed changes

synapse/server.py Outdated Show resolved Hide resolved

synapse/http/federation/matrix_federation_agent.py Show resolved Hide resolved

tzyl mentioned this pull request Feb 10, 2021

Add support for no_proxy and case insensitive env variables #9372

Merged

4 tasks

Bubu force-pushed the federation_through_proxy branch from 1fb936c to 960e8a9 Compare March 12, 2021 11:49

dr-bonez mentioned this pull request Apr 14, 2021

Anonymous Homeservers (Tor/I2P) #7088

Open

anoadragon453 mentioned this pull request Apr 21, 2021

NoRouteError('Network unreachable') in PROXY setup #9852

Closed

Bubu added 3 commits April 27, 2021 18:33

support federation queries through http connect proxy

85081fa

Can be specified by HTTPS_PROXY env var. pass unfiltered reactor to federation agent for proxy support Signed-off-by: Marcus Hoffmann <bubu@bubu1.eu>

HTTPConnectProxyEndpoint now want a headers parameter

7115567

This is used for proxy authentication, we just continue our "solution" of copying code from proxyagent -> matrix_federation_agent.

make mypy happy

ef8792a

Bubu force-pushed the federation_through_proxy branch from 17d183c to ef8792a Compare April 27, 2021 16:50

also make black happy :)

781da36

richvdh requested a review from a team April 29, 2021 12:25

richvdh suggested changes May 5, 2021

View reviewed changes

erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label May 20, 2021

dklimpel mentioned this pull request Jul 7, 2021

allow specifying https:// proxy #9119

Closed

4 tasks

dklimpel reviewed Jul 9, 2021

View reviewed changes

dklimpel mentioned this pull request Jul 24, 2021

support federation queries through http connect proxy #10475

Merged

8 tasks

clokep closed this Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support federation queries through http connect proxy #9306

support federation queries through http connect proxy #9306

Bubu commented Feb 2, 2021 •

edited by clokep

Loading

Bubu commented Feb 2, 2021

clokep commented Feb 3, 2021

Bubu commented Feb 3, 2021

clokep left a comment

tzyl commented Feb 9, 2021

clokep commented Feb 9, 2021

richvdh commented Apr 14, 2021

f1-outsourcing commented Apr 21, 2021

Bubu commented Apr 27, 2021

chrisguida commented May 3, 2021

richvdh left a comment

richvdh May 5, 2021

richvdh May 5, 2021

dklimpel Jul 9, 2021

dklimpel commented Jul 11, 2021

ShadowJonathan commented Jul 11, 2021 •

edited

Loading

dklimpel commented Jul 11, 2021

clokep commented Aug 5, 2021

Bubu commented Aug 5, 2021 •

edited

Loading

support federation queries through http connect proxy #9306

support federation queries through http connect proxy #9306

Conversation

Bubu commented Feb 2, 2021 • edited by clokep Loading

Pull Request Checklist

Bubu commented Feb 2, 2021

clokep commented Feb 3, 2021

Bubu commented Feb 3, 2021

clokep left a comment

Choose a reason for hiding this comment

tzyl commented Feb 9, 2021

clokep commented Feb 9, 2021

richvdh commented Apr 14, 2021

f1-outsourcing commented Apr 21, 2021

Bubu commented Apr 27, 2021

chrisguida commented May 3, 2021

richvdh left a comment

Choose a reason for hiding this comment

richvdh May 5, 2021

Choose a reason for hiding this comment

richvdh May 5, 2021

Choose a reason for hiding this comment

dklimpel Jul 9, 2021

Choose a reason for hiding this comment

dklimpel commented Jul 11, 2021

ShadowJonathan commented Jul 11, 2021 • edited Loading

dklimpel commented Jul 11, 2021

clokep commented Aug 5, 2021

Bubu commented Aug 5, 2021 • edited Loading

Bubu commented Feb 2, 2021 •

edited by clokep

Loading

ShadowJonathan commented Jul 11, 2021 •

edited

Loading

Bubu commented Aug 5, 2021 •

edited

Loading