-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Ability to blacklist ip ranges for federation traffic #5043
Changes from 9 commits
1b8532b
4501489
0200c86
9f1f03f
6631485
25c99dc
9795344
1b3989b
0e2f8ca
6479cd5
3f4f931
968ddca
e1feb45
152d7a8
6592691
517794e
15d1802
131b9c0
13f430c
e2bc9af
ec67848
43ffe47
aee810a
a30a778
ede582f
4ba420f
358777d
7f15dd7
6b29f7e
e0715d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add ability to blacklist IP ranges for the federation client. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,6 +122,30 @@ def read_config(self, config): | |
for domain in federation_domain_whitelist: | ||
self.federation_domain_whitelist[domain] = True | ||
|
||
self.federation_ip_range_blacklist = config.get( | ||
"federation_ip_range_blacklist", None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose so since this is a blacklist and an empty one is the same as not having one at all. |
||
) | ||
if self.federation_ip_range_blacklist is not None: | ||
# Import IPSet | ||
try: | ||
from netaddr import IPSet | ||
except ImportError: | ||
raise ConfigError( | ||
"Missing netaddr library. This is required to use " | ||
"federation_ip_range_blacklist" | ||
) | ||
|
||
# Attempt to create an IPSet from the given ranges | ||
try: | ||
self.federation_ip_range_blacklist = IPSet( | ||
self.federation_ip_range_blacklist | ||
) | ||
except Exception as e: | ||
raise ConfigError( | ||
"Invalid range(s) provided in " | ||
"federation_ip_range_blacklist: %s" % e | ||
) | ||
|
||
if self.public_baseurl is not None: | ||
if self.public_baseurl[-1] != '/': | ||
self.public_baseurl += '/' | ||
|
@@ -351,6 +375,20 @@ def default_config(self, server_name, data_dir_path, **kwargs): | |
# - nyc.example.com | ||
# - syd.example.com | ||
|
||
# Prevent federation requests from being sent to the following | ||
# blacklist IP address CIDR ranges. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please could you explain what the default is (even if it's that nothing is blacklisted)? |
||
# | ||
#federation_ip_range_blacklist: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think we should uncomment this so that we blacklist the RFC1918 addresses by default for new deployments? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing as it would be a security benefit for the vast majority of use cases, I would argue yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so would I. Please can you make it so? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done so, but requires having sytest override it so that tests don't fail. |
||
# - '127.0.0.0/8' | ||
# - '10.0.0.0/8' | ||
# - '172.16.0.0/12' | ||
# - '192.168.0.0/16' | ||
# - '100.64.0.0/10' | ||
# - '169.254.0.0/16' | ||
# - '::1/128' | ||
# - 'fe80::/64' | ||
# - 'fc00::/7' | ||
|
||
# List of ports that Synapse should listen on, their purpose and their | ||
# configuration. | ||
# | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,16 +76,18 @@ class IPBlacklistingResolver(object): | |
addresses, preventing DNS rebinding attacks on URL preview. | ||
""" | ||
|
||
def __init__(self, reactor, ip_whitelist, ip_blacklist): | ||
def __init__(self, reactor, ip_whitelist, ip_blacklist, federation=False): | ||
""" | ||
Args: | ||
reactor (twisted.internet.reactor) | ||
ip_whitelist (netaddr.IPSet) | ||
ip_blacklist (netaddr.IPSet) | ||
federation (bool): this resolver is for federation traffic | ||
""" | ||
self._reactor = reactor | ||
self._ip_whitelist = ip_whitelist | ||
self._ip_blacklist = ip_blacklist | ||
self._from_federation = federation | ||
|
||
def resolveHostName(self, recv, hostname, portNumber=0): | ||
|
||
|
@@ -109,7 +111,12 @@ def addressResolved(address): | |
logger.info( | ||
"Dropped %s from DNS resolution to %s" % (ip_address, hostname) | ||
) | ||
raise SynapseError(403, "IP address blocked by IP blacklist entry") | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Only raise a 403 if this request originated from a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm struggling to remember now. I remember I worked with @erikjohnston on this. Something will passing tests perhaps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. T'was actually due to special-casing for unit tests. We've now removed this, as the existing implementation didn't make sense anyways. |
||
# client-server call | ||
if not self._from_federation: | ||
raise SynapseError(403, | ||
"IP address blocked by IP blacklist entry") | ||
return | ||
|
||
addresses.append(address) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,7 +78,7 @@ def do_request(): | |
# Nothing happened yet | ||
self.assertNoResult(test_d) | ||
|
||
# Make sure treq is trying to connect | ||
# Make sure the req is trying to connect | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
clients = self.reactor.tcpClients | ||
self.assertEqual(len(clients), 1) | ||
(host, port, factory, _timeout, _bindAddress) = clients[0] | ||
|
@@ -211,6 +211,110 @@ def test_client_connect_no_response(self): | |
self.assertIsInstance(f.value, RequestSendFailed) | ||
self.assertIsInstance(f.value.inner_exception, ResponseNeverReceived) | ||
|
||
def test_client_ip_range_blacklist(self): | ||
"""Ensure that Synapse does not try to connect to blacklisted IPs""" | ||
# Set up the ip_range blacklist | ||
from netaddr import IPSet | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. import this at the top? |
||
|
||
self.hs.config.federation_ip_range_blacklist = IPSet([ | ||
"127.0.0.0/8", | ||
"fe80::/64", | ||
]) | ||
self.reactor.lookups["internal"] = "127.0.0.1" | ||
self.reactor.lookups["internalv6"] = "fe80:0:0:0:0:8a2e:370:7337" | ||
self.reactor.lookups["fine"] = "10.20.30.40" | ||
cl = MatrixFederationHttpClient(self.hs, None) | ||
|
||
# Try making a GET request to a blacklisted IPv4 address | ||
# ------------------------------------------------------ | ||
@defer.inlineCallbacks | ||
def do_request(): | ||
with LoggingContext("one") as context: | ||
fetch_d = cl.get_json("internal:8008", "foo/bar") | ||
|
||
# Nothing happened yet | ||
self.assertNoResult(fetch_d) | ||
|
||
# should have reset logcontext to the sentinel | ||
check_logcontext(LoggingContext.sentinel) | ||
|
||
try: | ||
fetch_res = yield fetch_d | ||
defer.returnValue(fetch_res) | ||
finally: | ||
check_logcontext(context) | ||
|
||
# Make the request | ||
d = do_request() | ||
self.pump() | ||
|
||
# Nothing has happened yet | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remember to test that these deferreds eventually get resolved one way or the other, otherwise we'll end up with requests blocking forever. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still not 100% sure on how to do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes. The thing to note here is that the test function itself is running synchronously: it starts at the beginning, and doesn't return until it is finished. Note that is different to a Now, the way that we test the asynchronous code from a synchronous function is something like:
Often, all that is needed is to "pretend some time has passed". We do this by using a fake Reactor where we can manipulate its idea of the current time. "Pretending time has passed" is then called "advancing the reactor", and is what [It may be helpful to note that this means that there are actually two reactors in play, since the test runner ( So, long story short: how do we test that the deferreds eventually get resolved? well, somewhere in the test we ought to be able to call |
||
self.assertNoResult(d) | ||
|
||
# Check that it was unable to resolve the address | ||
clients = self.reactor.tcpClients | ||
self.assertEqual(len(clients), 0) | ||
|
||
# Try making a POST request to a blacklisted IPv6 address | ||
# ------------------------------------------------------- | ||
@defer.inlineCallbacks | ||
def do_request(): | ||
with LoggingContext("one") as context: | ||
fetch_d = cl.post_json("internalv6:8008", "foo/bar") | ||
|
||
# Nothing happened yet | ||
self.assertNoResult(fetch_d) | ||
|
||
# should have reset logcontext to the sentinel | ||
check_logcontext(LoggingContext.sentinel) | ||
|
||
try: | ||
fetch_res = yield fetch_d | ||
defer.returnValue(fetch_res) | ||
finally: | ||
check_logcontext(context) | ||
|
||
# Make the request | ||
d = do_request() | ||
self.pump() | ||
|
||
# Nothing has happened yet | ||
self.assertNoResult(d) | ||
|
||
# Check that it was unable to resolve the address | ||
clients = self.reactor.tcpClients | ||
self.assertEqual(len(clients), 0) | ||
|
||
# Try making a GET request to a non-blacklisted IPv4 address | ||
# ---------------------------------------------------------- | ||
@defer.inlineCallbacks | ||
def do_request(): | ||
with LoggingContext("one") as context: | ||
fetch_d = cl.post_json("fine:8008", "foo/bar") | ||
|
||
# Nothing happened yet | ||
self.assertNoResult(fetch_d) | ||
|
||
# should have reset logcontext to the sentinel | ||
check_logcontext(LoggingContext.sentinel) | ||
|
||
try: | ||
fetch_res = yield fetch_d | ||
defer.returnValue(fetch_res) | ||
finally: | ||
check_logcontext(context) | ||
|
||
# Make the request | ||
d = do_request() | ||
self.pump() | ||
|
||
# Nothing has happened yet | ||
self.assertNoResult(d) | ||
|
||
# Check that it was able to resolve the address | ||
clients = self.reactor.tcpClients | ||
self.assertEqual(len(clients), 1) | ||
|
||
def test_client_gets_headers(self): | ||
""" | ||
Once the client gets the headers, _request returns successfully. | ||
|
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 ought to follow the example of #5134 and include 0.0.0.0 and
::
, whether they were explicitly listed or not.