Ability to blacklist ip ranges for federation traffic by anoadragon453 · Pull Request #5043 · matrix-org/synapse

anoadragon453 · 2019-04-10T09:43:38Z

This PR adds a new config option that functions similar to url_preview_ip_range_blacklist, but provides a blacklist for federation traffic. It does this by making use of our IPBlacklistResolver and BlacklistingAgentWrapper classes.

Sytest PR: matrix-org/sytest#607

codecov · 2019-04-10T11:22:30Z

Codecov Report

Merging #5043 into develop will increase coverage by 0.01%.
The diff coverage is 86.36%.

@@             Coverage Diff             @@
##           develop    #5043      +/-   ##
===========================================
+ Coverage    61.75%   61.76%   +0.01%     
===========================================
  Files          336      336              
  Lines        34639    34658      +19     
  Branches      5692     5694       +2     
===========================================
+ Hits         21390    21407      +17     
- Misses       11721    11723       +2     
  Partials      1528     1528

richvdh

generally looks sane. A few suggestions

richvdh · 2019-04-30T10:25:52Z

synapse/config/server.py

        #  - syd.example.com

+        # Prevent federation requests from being sent to the following
+        # blacklist IP address CIDR ranges.


please could you explain what the default is (even if it's that nothing is blacklisted)?

richvdh · 2019-04-30T10:27:45Z

synapse/config/server.py

+        # Prevent federation requests from being sent to the following
+        # blacklist IP address CIDR ranges.
+        #
+        #federation_ip_range_blacklist:


do you think we should uncomment this so that we blacklist the RFC1918 addresses by default for new deployments?

Seeing as it would be a security benefit for the vast majority of use cases, I would argue yes.

so would I. Please can you make it so?

Done so, but requires having sytest override it so that tests don't fail.

richvdh · 2019-04-30T10:28:37Z

synapse/http/client.py

                        "Dropped %s from DNS resolution to %s" % (ip_address, hostname)
                    )
-                    raise SynapseError(403, "IP address blocked by IP blacklist entry")
+                    # Only raise a 403 if this request originated from a


why do we need to do this?

I'm struggling to remember now. I remember I worked with @erikjohnston on this.

Something will passing tests perhaps?

T'was actually due to special-casing for unit tests. We've now removed this, as the existing implementation didn't make sense anyways.

richvdh · 2019-04-30T10:32:07Z

synapse/config/server.py

                self.federation_domain_whitelist[domain] = True

+        self.federation_ip_range_blacklist = config.get(
+            "federation_ip_range_blacklist", None,


If we make the default [] rather than None we can save ourselves a bunch of special-casing?

I suppose so since this is a blacklist and an empty one is the same as not having one at all.

richvdh · 2019-04-30T10:33:05Z

tests/http/test_fedclient.py

        self.assertNoResult(test_d)

-        # Make sure treq is trying to connect
+        # Make sure the req is trying to connect


treq is correct

richvdh · 2019-04-30T10:33:26Z

tests/http/test_fedclient.py

+    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


import this at the top?

* develop: (34 commits) Add a default .m.rule.tombstone push rule (#4867) Fix infinite loop in presence handler changelog more logging improvements remove extraneous exception logging Clarify logging when PDU signature checking fails Changelog Add --no-pep-517 to README instructions set PIP_USE_PEP517 = False for tests Fix handling of SYNAPSE_NO_TLS in docker image (#5005) Config option for verifying federation certificates (MSC 1711) (#4967) Remove log error for .well-known/matrix/client (#4972) Prevent "producer not unregistered" message (#5009) add gpg key fingerprint Don't crash on lack of expiry templates Update debian install docs for new key and repo (#5074) Add management endpoints for account validity Send out emails with links to extend an account's validity period Make sure we're not registering the same 3pid twice Newsfile ...

richvdh · 2019-05-02T23:55:45Z

tests/http/test_fedclient.py

+        d = do_request()
+        self.pump()
+
+        # Nothing has happened yet


please remember to test that these deferreds eventually get resolved one way or the other, otherwise we'll end up with requests blocking forever.

Still not 100% sure on how to do this. yield'ing results in the function timing out (though maybe that's what we want to test?).

yield'ing results in the function timing out

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 Deferred-returning function, which only does part of its work synchronously, and leaves the rest to be done later (at which point, the Deferred will resolve).

Now, the way that we test the asynchronous code from a synchronous function is something like:

run a function that returns a deferred

satisfy the condition that the function is waiting for (eg: pretend that some time has passed; connect a TCP socket; spoof some results from the database).

Once we satisfy the awaited condition, the second half of the function should complete, and hence resolve its deferred.

Check that the deferred has now resolved with the expected result.

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 self.pump does.

[It may be helpful to note that this means that there are actually two reactors in play, since the test runner (trial) is a Twisted application which runs on a regular reactor: so we end up with the main reactor which is running trial and the test reactor which is running the code under test. But in practice, as long as your test functions are synchronous, you can forget about the trial reactor.]

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.successResultOf(d) or self.failureResultOf(d, cls) [iirc]. If that assertion fails, it means that the deferred isn't completing, and that is a bad thing!

old

richvdh

this is taking shape!

One thing I would say is that I think this PR is starting to sprawl a bit, and I would suggest splitting out the changes to the existing blacklisting code so that we can land them independently of the federation blacklist.

synapse/http/client.py

richvdh · 2019-05-08T08:41:28Z

synapse/http/client.py

            ):
                logger.info(
-                    "Blocking access to %s because of blacklist" % (ip_address,)
+                    "Blocking access to %s because of blacklist. Returning 0 results" %


Is it true to say we are returning 0 results here? It certainly doesn't match my way of thinking about it.

richvdh · 2019-05-08T08:42:14Z

synapse/http/client.py

+                    (ip_address,)
                )
-                e = SynapseError(403, "IP address blocked by IP blacklist entry")
+                e = SynapseError(404, "No results found")


I'm guessing this is changing to keep it in sync with https://github.com/matrix-org/synapse/pull/5043/files#diff-3c2efbd259756c1c1dc5dbdf467b5570R341, for which see my comments there, however:

On the one hand I see the value of keeping the behaviour consistent between literal IPs, and hostnames. On the other, since we have the ability to give a better response like a 403 here, I'm inclined to do so.

richvdh · 2019-05-08T08:42:52Z

synapse/rest/media/v1/preview_url_resource.py

                logger.warn("Error downloading %s: %r", url, e)
+
+                if isinstance(e, DNSLookupError):
+                    # DNS lookup returned no results


there are other reasons that we could get a DNSLookupError

richvdh · 2019-05-08T08:43:28Z

synapse/rest/media/v1/preview_url_resource.py

                # FIXME: pass through 404s and other error messages nicely
                logger.warn("Error downloading %s: %r", url, e)
+
+                if isinstance(e, DNSLookupError):


Think this would be better handled with a separate except DNSLookupError as e clause.

richvdh · 2019-05-08T08:52:22Z

synapse/rest/media/v1/preview_url_resource.py

+                    # Note: This will also be the case if the found IP address
+                    # is blacklisted
+                    raise SynapseError(
+                        404, "No results found", Codes.UNKNOWN


It's not obvious to me that this is the correct thing to return. "No results found" is a pretty useless error, and why a 404?

https://matrix.org/docs/spec/client_server/unstable.html#get-matrix-media-r0-preview-url is annoying unhelpful about how to handle this situation, which would be a nice thing to rectify, but I think a 502 is the normal way for a proxy-like thing (which we are) to say "I couldn't connect to the the thing I'm supposed to be proxying".

richvdh · 2019-05-08T09:17:38Z

synapse/http/matrixfederationclient.py

            tls_client_options_factory,
        )
+
+        # Prevent direct connections to blacklisted IP addresses


I don't find "direct connections" very clear here. How about:

"Use a BlacklistingAgentWrapper to prevent circumventing the IP blacklist via IP literals in server names"

richvdh · 2019-05-08T09:38:08Z

tests/http/test_fedclient.py

+        d = do_request()
+        self.pump()
+
+        # Nothing has happened yet


yield'ing results in the function timing out

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 Deferred-returning function, which only does part of its work synchronously, and leaves the rest to be done later (at which point, the Deferred will resolve).

Now, the way that we test the asynchronous code from a synchronous function is something like:

run a function that returns a deferred

satisfy the condition that the function is waiting for (eg: pretend that some time has passed; connect a TCP socket; spoof some results from the database).

Once we satisfy the awaited condition, the second half of the function should complete, and hence resolve its deferred.

Check that the deferred has now resolved with the expected result.

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 self.pump does.

[It may be helpful to note that this means that there are actually two reactors in play, since the test runner (trial) is a Twisted application which runs on a regular reactor: so we end up with the main reactor which is running trial and the test reactor which is running the code under test. But in practice, as long as your test functions are synchronous, you can forget about the trial reactor.]

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.successResultOf(d) or self.failureResultOf(d, cls) [iirc]. If that assertion fails, it means that the deferred isn't completing, and that is a bad thing!

old

richvdh · 2019-05-09T22:04:32Z

synapse/config/server.py

            for domain in federation_domain_whitelist:
                self.federation_domain_whitelist[domain] = True

+        self.federation_ip_range_blacklist = config.get(


we ought to follow the example of #5134 and include 0.0.0.0 and ::, whether they were explicitly listed or not.

richvdh · 2019-05-09T22:06:17Z

synapse/http/client.py

    def resolveHostName(self, recv, hostname, portNumber=0):

        r = recv()
-        d = defer.Deferred()


can you merge #5155 so that we can get this stuff out of the diff?

richvdh · 2019-05-09T22:07:57Z

tests/http/test_fedclient.py

+        # Nothing happened yet
+        self.assertNoResult(d)
+
+        self.pump(120)


hrm, having to wait 120 seconds for this doesn't sound right. Why does it take so long?

Oh, it doesn't. I've just selected that value arbitrarily. Have decreased.

old

* develop: (45 commits) URL preview blacklisting fixes (#5155) Revert 085ae34 Add a DUMMY stage to captcha-only registration flow Make Prometheus snippet less confusing on the metrics collection doc (#4288) Set syslog identifiers in systemd units (#5023) Run Black on the tests again (#5170) Add AllowEncodedSlashes to apache (#5068) remove instructions for jessie installation (#5164) Run `black` on per_destination_queue Limit the number of EDUs in transactions to 100 as expected by receiver (#5138) Fix bogus imports in tests (#5154) add options to require an access_token to GET /profile and /publicRooms on CS API (#5083) Do checks on aliases for incoming m.room.aliases events (#5128) Remove the requirement to authenticate for /admin/server_version. (#5122) Fix spelling in server notices admin API docs (#5142) Fix sample config 0.99.3.2 include disco in deb build target list changelog Debian: we now need libpq-dev. ...

richvdh

lgtm!

richvdh · 2019-05-13T18:04:00Z

synapse/config/server.py

this could really have done with being outside the try/catch, but nm

anoadragon453 added 7 commits April 10, 2019 10:35

tests fail

1b8532b

tests pass

4501489

lint

0200c86

lint and changelog

9f1f03f

actually add changelog

6631485

sample config

25c99dc

Don't raise an exception if coming from federation

9795344

lint

1b3989b

anoadragon453 marked this pull request as ready for review April 10, 2019 11:31

anoadragon453 requested a review from a team April 10, 2019 11:32

Add some notes

0e2f8ca

richvdh previously requested changes Apr 30, 2019

View reviewed changes

anoadragon453 and others added 5 commits April 30, 2019 15:01

Use an empty list as default

6479cd5

Testing

968ddca

We can't throw exceptions in an IResolutionReceiver

e1feb45

Remove different behaviour for fed vs. nonfed

152d7a8

richvdh reviewed May 2, 2019

View reviewed changes

anoadragon453 added 7 commits May 2, 2019 16:55

Import at the top

6592691

isort locally didn't have a problem >:(

517794e

lint

15d1802

yield deferred

131b9c0

Same behavior for no result and result blacklisted

13f430c

lint

e2bc9af

Remove yield

ec67848

anoadragon453 requested a review from richvdh May 4, 2019 01:01

Enable federation blacklisting by default

43ffe47

richvdh previously requested changes May 8, 2019

View reviewed changes

anoadragon453 mentioned this pull request May 8, 2019

Disable ip range federation blacklist under Sytest matrix-org/sytest#607

Merged

Fix tests and various small review issues

aee810a

anoadragon453 mentioned this pull request May 8, 2019

URL preview blacklisting fixes #5155

Merged

anoadragon453 added 2 commits May 8, 2019 15:42

Update tests

a30a778

lint

ede582f

anoadragon453 requested a review from richvdh May 9, 2019 19:37

richvdh previously requested changes May 9, 2019

View reviewed changes

anoadragon453 added 2 commits May 10, 2019 09:50

always blacklist 0.0.0.0, ::

4ba420f

lower pump value

358777d

anoadragon453 added 3 commits May 10, 2019 09:57

lint

7f15dd7

regen config

6b29f7e

anoadragon453 requested a review from richvdh May 10, 2019 18:30

richvdh approved these changes May 13, 2019

View reviewed changes

synapse/config/server.py

Copy link

Member

richvdh May 13, 2019

Choose a reason for hiding this comment

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

this could really have done with being outside the try/catch, but nm

richvdh merged commit 5a4b328 into develop May 13, 2019

richvdh mentioned this pull request May 13, 2019

It should be possible to configure a list of IPs synapse will not make outbound connections to #3953

Closed

Uh oh!

Conversation

anoadragon453 commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

anoadragon453 commented Apr 10, 2019 •

edited

Loading

codecov bot commented Apr 10, 2019 •

edited

Loading

anoadragon453 May 2, 2019 •

edited

Loading

anoadragon453 May 10, 2019 •

edited

Loading