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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions synapse/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,16 @@ def read_config(self, config):
"block_non_admin_invites", False,
)

# FIXME: federation_domain_whitelist needs sytests
self.federation_domain_whitelist = None
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.

"federation_domain_whitelist", []
"federation_domain_whitelist", None
)
# turn the whitelist into a hash for speed of lookup
self.federation_domain_whitelist = {}
for domain in federation_domain_whitelist:
self.federation_domain_whitelist[domain] = True
# FIXME: federation_domain_whitelist needs sytests
if federation_domain_whitelist is not None:
self.federation_domain_whitelist = {}
for domain in federation_domain_whitelist:
self.federation_domain_whitelist[domain] = True

if self.public_baseurl is not None:
if self.public_baseurl[-1] != '/':
Expand Down Expand Up @@ -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?

#
# federation_domain_whitelist:
# - lon.example.com
Expand Down
2 changes: 1 addition & 1 deletion synapse/federation/federation_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ def get_pdu(self, destinations, event_id, outlier=False, timeout=None):
logger.info(e.message)
continue
except FederationDeniedError as e:
logger.debug(e.message)
logger.info(e.message)
continue
except Exception as e:
pdu_attempts[destination] = now
Expand Down
2 changes: 1 addition & 1 deletion synapse/federation/transaction_queue.py
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ def _transaction_transmission_loop(self, destination):
),
)
except FederationDeniedError as e:
logger.debug(e)
logger.info(e)
except Exception as e:
logger.warn(
"TX [%s] Failed to send transaction: %s",
Expand Down
2 changes: 1 addition & 1 deletion synapse/federation/transport/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def authenticate_request(self, request, content):
}

if (
self.federation_domain_whitelist and
self.federation_domain_whitelist is not None and
self.server_name not in self.federation_domain_whitelist
):
raise FederationDeniedError(self.server_name)
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/device.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ def _handle_device_updates(self, user_id):
# eventually become consistent.
return
except FederationDeniedError as e:
logger.debug(e)
logger.info(e)
return
except Exception:
# TODO: Remember that we are now out of sync and try again
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ def try_backfill(domains):
logger.info(e.message)
continue
except FederationDeniedError as e:
logger.debug(e)
logger.info(e)
continue
except Exception as e:
logger.exception(
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/key/v2/remote_key_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def query_keys(self, request, query, query_remote_on_cache_miss=False):
store_queries = []
for server_name, key_ids in query.items():
if (
self.federation_domain_whitelist and
self.federation_domain_whitelist is not None and
server_name not in self.federation_domain_whitelist
):
logger.debug("Federation denied with %s", server_name)
Expand Down
8 changes: 4 additions & 4 deletions synapse/rest/media/v1/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ def get_remote_media(self, request, server_name, media_id, name):
to request
"""
if (
self.federation_domain_whitelist and
self.federation_domain_whitelist is not None and
server_name not in self.federation_domain_whitelist
):
raise FederationDeniedError(server_name)
Expand Down Expand Up @@ -266,7 +266,7 @@ def get_remote_media_info(self, server_name, media_id):
Deferred[dict]: The media_info of the file
"""
if (
self.federation_domain_whitelist and
self.federation_domain_whitelist is not None and
server_name not in self.federation_domain_whitelist
):
raise FederationDeniedError(server_name)
Expand Down Expand Up @@ -387,8 +387,8 @@ def _download_remote_file(self, server_name, media_id, file_id):
logger.warn("Not retrying destination %r", server_name)
raise SynapseError(502, "Failed to fetch remote media")
except FederationDeniedError as e:
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

except Exception:
logger.exception("Failed to fetch remote media %s/%s",
server_name, media_id)
Expand Down