Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add federation_domain_whitelist option #2820

Merged
merged 7 commits into from
Jan 22, 2018
Merged

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Jan 22, 2018

Gives a way to restrict which domains your HS is allowed to federate with.
Useful mainly for gracefully preventing a private but internet-connected HS from trying to federate outbound to the wider public Matrix network.
For symmetry, we also try to block inbound federation from non-whitelisted domains, but this should also be done at the IP or HTTP level rather than relying purely on Synapse's application-level filtering at this point.

gives a way to restrict which domains your HS is allowed to federate with.
useful mainly for gracefully preventing a private but internet-connected HS from trying to federate to the wider public Matrix network
@@ -266,6 +266,9 @@ def get_pdu(self, destinations, event_id, outlier=False, timeout=None):
except NotRetryingDestination as e:
logger.info(e.message)
continue
except FederationDeniedError as e:
logger.debug(e.message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to log these at debug rather than info? Debug logging is normally disabled, so I'm worried that people will get obscure failure modes due to lack of logging

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a thinko which i forgot to fully fix - thanks.

@@ -55,6 +55,15 @@ def read_config(self, config):
"block_non_admin_invites", False,
)

federation_domain_whitelist = config.get(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels very odd that you can whitelist everything by having an empty whitelist. I wonder if it would be better to make an empty whitelist actually mean that nothing is allowed, and have the default be *

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point; fixed.

@@ -41,15 +43,17 @@ def setUp(self):
self.event_id = 0

server_factory = ReplicationStreamProtocolFactory(self.hs)
listener = reactor.listenUNIX("\0xxx", server_factory)
# XXX: mktemp is unsafe and should never be used. but we're just a test.
path = tempfile.mktemp(prefix="base_slaved_store_test_case_socket")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks unrelated. Can you make it a separate PR please

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is unrelated, other than being pulled in to fix the tests whilst doing this PR. given it's trivial, i'd prefer to not burn the time splitting it out at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping unrelated things in separate PRs makes it easier to review them, helps with bisecting, and makes it easier to back things out separately if they turn out to be ill-advised. It being trivial means it's all the easier to separate out.

I've done it for you: c776c52 backs out the changes; #2821 is the new PR.

@ara4n
Copy link
Member Author

ara4n commented Jan 22, 2018

@erikjohnston (or @richvdh, but i'd have thought rich would rather snowboard) can you PTAL again?

@@ -222,7 +224,8 @@ def default_config(self, server_name, **kwargs):
# Restrict federation to the following whitelist of domains.
# N.B. we recommend also firewalling your federation listener to limit
# inbound federation traffic as early as possible, rather than relying
# purely on this application-layer restriction.
# purely on this application-layer restriction. If not specified, the
# default is to whitelist nothing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/nothing/everything?

richvdh added a commit that referenced this pull request Jan 22, 2018
logger.debug(e)
raise SynapseError(403, e.message)
logger.info(e)
raise e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be raise rather than raise e, unless there is a good reason to throw a new exception annd hence swallow the original stack trace.

Although, looking at it, I don't think you'll ever get here, because FederationDeniedError is a subclass of SynapseError so the clause at line 382 will get taken instead. I think you might as well remove the whole thing

@@ -41,15 +43,17 @@ def setUp(self):
self.event_id = 0

server_factory = ReplicationStreamProtocolFactory(self.hs)
listener = reactor.listenUNIX("\0xxx", server_factory)
# XXX: mktemp is unsafe and should never be used. but we're just a test.
path = tempfile.mktemp(prefix="base_slaved_store_test_case_socket")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping unrelated things in separate PRs makes it easier to review them, helps with bisecting, and makes it easier to back things out separately if they turn out to be ill-advised. It being trivial means it's all the easier to separate out.

I've done it for you: c776c52 backs out the changes; #2821 is the new PR.

@richvdh richvdh assigned ara4n and unassigned richvdh and erikjohnston Jan 22, 2018
@ara4n
Copy link
Member Author

ara4n commented Jan 22, 2018

ok. have pushed remaining PR fixes.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Please squash before/during merging so that we don't have all the bad commits confusing things for future bisecting

@andrewgdunn
Copy link

It'd be really nice to have some logging for the 403 (deny) so that homeserver operators can add inbound addresses that are constantly retrying.

@krombel
Copy link
Contributor

krombel commented Mar 29, 2018

@storrgie That should be possible when you have your server logs set to INFO.
Just grep the logs for "Federation denied with "

@ShellCode33
Copy link

Is there a way to wildcard name servers in order to disable the federation entirely ?

@turt2live
Copy link
Member

There doesn't appear to be wildcard support, but setting it to an empty array looks to be a way to disable it. As per the sample configuration, it's probably best to firewall it though (both inbound and outbound).

@krombel
Copy link
Contributor

krombel commented Apr 3, 2018

AFAIK when the federation whitelist is unset all federation is allowed.
So you might set the whitelist to your server_name and/or disable the federation listeners (so the server does not listen for federation requests entirely.

In short: Add

federation_domain_whitelist:
- <yourServerName>

to your config and remove your federation listener do disable all federation

//EDIT: Extended with more explicit HowTo

@hawkowl hawkowl deleted the matthew/limit-federation branch September 20, 2018 13:59
@flumpt
Copy link

flumpt commented Jan 11, 2019

Is there a way to wildcard name servers in order to disable the federation entirely ?

I would love to be able to use a wildcard to build a closed matrix subnet. Is there any way to do it without every server knowing all other participating servers?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants