Fix using reactor as ip whitelist

[tzyl]

Currently the reactor in MatrixFederationAgent is being passed as the ip_whitelist in BlacklistingAgentWrapper (src). This PR removes this in keeping with how BlacklistingAgentWrapper is instantiated in MatrixFederationHttpClient:

synapse/synapse/http/matrixfederationclient.py

Lines 254 to 258 in aa4d8c1

	# Use a BlacklistingAgentWrapper to prevent circumventing the IP
	# blacklist via IP literals in server names
	self.agent = BlacklistingAgentWrapper(
	self.agent, ip_blacklist=hs.config.federation_ip_range_blacklist,
	)

Signed-off-by: tzyl tim95@hotmail.co.uk

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[richvdh]

@tyzl thanks for this! For the record, please can you make it clear what problems this was causing, with the text of any exceptions you see?

[tzyl]

Hey @richvdh!

This was not causing any runtime problems to me but during reading the code stood out to me as something that looked fishy and possibly wrong, please correct me if I misunderstood and this was intentional. Based on my understanding this shouldn't really cause runtime problems due to the check_against_blacklist code having the same effect as no whitelist when looking for and never finding ip_address in the reactor.

synapse/synapse/http/client.py

Lines 107 to 109 in aa4d8c1

	if ip_address in ip_blacklist:
	if ip_whitelist is None or ip_address not in ip_whitelist:
	return True

[tzyl]

However, maybe it's possible that check would cause runtime errors if we actually do go through the ip_blacklist:

>>> from twisted.internet import reactor
>>> "x" in reactor
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: argument of type 'SelectReactor' is not iterable

[clokep]

This is a regression from the combination of #8812 and #8821.

[clokep]

@tzyl Any chance you'd be willing to add a test for this? I think we would want to create a test in test_matrix_federation_agent which uses a blacklist (it seems that it is not configured to right now).

[tzyl]

Hey @clokep looking into this more, this BlacklistingAgentWrapper is only used for the WellKnownResolver which only gets called if we have a non-IP literal:

synapse/synapse/http/federation/matrix_federation_agent.py

Lines 152 to 158 in ebd534b

	if (
	parsed_uri.scheme == b"matrix"
	and not _is_ip_literal(parsed_uri.hostname)
	and not parsed_uri.port
	):
	well_known_result = yield defer.ensureDeferred(
	self._well_known_resolver.get_well_known(parsed_uri.hostname)

In this case is it correct that I can't actually hit (and test) the blacklist currently because we only ever pass non-IP literals into the BlacklistingAgentWrapper?

synapse/synapse/http/client.py

Lines 262 to 273 in ebd534b

	try:
	ip_address = IPAddress(h.hostname)
	if check_against_blacklist(
	ip_address, self._ip_whitelist, self._ip_blacklist
	):
	logger.info("Blocking access to %s due to blacklist" % (ip_address,))
	e = SynapseError(403, "IP address blocked by IP blacklist entry")
	return defer.fail(Failure(e))
	except Exception:
	# Not an IP
	pass

[clokep]

@tzyl I just found the same bit of code! So I think what this means is that this PR is correct and we should fix this, but in reality the code won't be run for an IP literal due to the block you found! 👍

[clokep]

I think we should rename this to misc since there's no outward facing bug that comes from this.

Suggested change

	Fix reactor being passsed as ip_whitelist to BlacklistingAgentWrapper from MatrixFederationAgent.
	Fix the wrong arguments being passed to `BlacklistingAgentWrapper` from `MatrixFederationAgent`.

You can add Contributed by <your name or handle>. if you'd like!

[tzyl]

Updated the changelog and renamed to 9098.misc!

[tzyl]

Awesome, thanks for confirming @clokep! Is it expected that this does not get flagged by the type hints?

[clokep]

Is it expected that this does not get flagged by the type hints?

We had this discussion internally and it seems to be because mypy doesn't have type hints for netaddr so it falls back to Any for those parameters.

[clokep]

Thank you very much! 🎉