-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Conflicts: synapse/http/matrixfederationclient.py
Conflicts: synapse/http/matrixfederationclient.py
Maybe fix #15773 (comment) ``` Failure: twisted.trial.util.DirtyReactorAggregateError: Reactor was unclean. ```
Conflicts: tests/http/test_matrixfederationclient.py
It'd be useful if someone else could take a quick pass through this to make sure I'm not being a crank. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
Only point that seemed dubious to me was that we ignore the federation control lists when using a federation proxy.
I guess we may be expecting the federation proxy to enforce that for us, but even if this is the case I think it would be worth spelling it out.
hs.config.server.federation_ip_range_whitelist, | ||
hs.config.server.federation_ip_range_blacklist, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want to assert that these options are not in use, if we are using the proxy, since it seems like we do not respect them (right now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those options will be respected. Workers will delegate outbound federation federation traffic to whatever worker is configured for outbound_federation_restricted_to
. And that outbound_federation_restricted_to
worker will use the federation_ip_range_allowlist
/federation_ip_range_blocklist
options.
You have to trust that whatever worker you proxy through but in the Synapse case, I think that's a given (other workers behave as expected).
Thanks for the review @erikjohnston, @H-Shay, and @reivilibre 🦒 |
|
||
if federation_proxy.tls: | ||
tls_connection_creator = self._policy_for_https.creatorForNetloc( | ||
federation_proxy.host, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@realtyem mentioned that we probably need to copy what https://github.com/matrix-org/synapse/pull/15746/files did for TLS to work properly. host
comes in as a str
and creatorForNetloc(...)
is expecting bytes
.
federation_proxy.host, | |
federation_proxy.host.encode("utf-8"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #15886
This reverts commit b07b14b.
Allow configuring the set of workers to proxy outbound federation traffic through (`outbound_federation_restricted_to`). This is useful when you have a worker setup with `federation_sender` instances responsible for sending outbound federation requests and want to make sure *all* outbound federation traffic goes through those instances. Before this change, the generic workers would still contact federation themselves for things like profile lookups, backfill, etc. This PR allows you to set more strict access controls/firewall for all workers and only allow the `federation_sender`'s to contact the outside world. The original code is from @erikjohnston's branches which I've gotten in-shape to merge.
**Before:** ``` Error retrieving alias ``` **After:** ``` Error retrieving alias #foo:bar -> 401 Unauthorized ``` *Spawning from creating the [manual testing strategy for the outbound federation proxy](#15773
**Before:** ``` Error retrieving alias ``` **After:** ``` Error retrieving alias #foo:bar -> 401 Unauthorized ``` *Spawning from creating the [manual testing strategy for the outbound federation proxy](matrix-org#15773
**Before:** ``` Error retrieving alias ``` **After:** ``` Error retrieving alias #foo:bar -> 401 Unauthorized ``` *Spawning from creating the [manual testing strategy for the outbound federation proxy](matrix-org#15773
Federation outbound proxy. Allow configuring the set of workers to proxy outbound federation traffic through (
outbound_federation_restricted_to
).This is useful when you have a worker setup with
federation_sender
instances responsible for sending outbound federation requests and want to make sure all outbound federation traffic goes through those instances. Before this change, the generic workers would still contact federation themselves for things like profile lookups, backfill, etc. This PR allows you to set more strict access controls/firewall for all workers and only allow thefederation_sender
's to contact the outside world.The original code is from @erikjohnston's branches which I've gotten in-shape to merge.
Switched from
matrix://
tomatrix-federation://
becausematrix://
is actually a registered scheme nowadays and doesn't make sense for our internal to Synapse use case anymore. (discussion)Testing strategy
Testing with Complement
MatrixFederationAgent.request(...)
andProxyAgent.request(...)
to verify requests flying arounddocker/configure_workers_and_start.py#L399-L400
so it looks like the following:MatrixFederationAgent.request(...)
only happens on thefederation_sender
Testing manually
Setup a Redis instance for inter-worker communication:
docker run -d -p 6379:6379 --name redis redis
To see if Redis is working, run
exec 3<>/dev/tcp/127.0.0.1/6379 && echo -e "PING\r\n" >&3 && head -c 7 <&3
(via https://stackoverflow.com/a/39214806/796832) and you should see+PONG
Modify your
homeserver.yaml
as instructed on https://matrix-org.github.io/synapse/latest/workers.html#shared-configurationAdditionally add the following to your
homeserver.yaml
Create a new
homeserver_generic_worker1.yaml
Copy your existing logging config
cp my.synapse.linux.server.config my.synapse.linux.server_generic_worker1.log.config
and adjustfilename
log path so they don't conflictAdd some logging to
MatrixFederationAgent.request(...)
andProxyAgent.request(...)
to verify requests flying aroundModify
MatrixFederationAgent.request(...)
to always overridemethod
anduri
with our own dummy request (add the following at the top of the function):Setup a dummy server that responds to
https://localhost:4444/foo.json
(note that it needs to support HTTPS/TLS):node server.js
Start the main process
poetry run synapse_homeserver --config-path homeserver.yaml
Start the worker process
poetry run synapse_worker --config-path homeserver.yaml --config-path homeserver_generic_worker1.yaml
POST http://localhost:8008/_matrix/client/v3/join/%23matrix%3Amatrix.org
Check the logs to verify that
MatrixFederationAgent.request(...)
only happens on thefederation_sender
Check the logs to verify that the main process still got the dummy response (if you really want to be sure, you can modify
synapse/handlers/directory.py#L270
) to log thefed_result
.Todo
Dev notes
Related tests:
tests/http/test_matrixfederationclient.py
tests/http/test_proxyagent.py
->MatrixFederationAgentTests
tests/federation/test_federation_client.py
Twisted docs:
Twisted source:
src/twisted/web/http.py
src/twisted/web/server.py
What headers to copy over (hop-by-hop vs end-to-end headers):
Manually messing
matrix-federation://
against the workers:Pull Request Checklist
Pull request includes a sign off(run the linters)