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

The interaction between url_preview_ip_range_blacklist and outbound HTTP proxies is confusing #9812

Closed
anoadragon453 opened this issue Apr 14, 2021 · 7 comments
Labels
good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@anoadragon453
Copy link
Member

Context: #9417 (comment)

The Synapse config option url_preview_ip_range_blacklist is intended to restrict access to previewing certain URLs based on their resolved IP address. However, when using a proxy with Synapse (via the HTTP_PROXY and HTTPS_PROXY environment variables), url preview requests are forwarded to the proxy, which then resolves the IP address of the URL. This completely bypasses any IP ranges set in the url_preview_ip_range_blacklist option.

Ideally we'd do a couple things that would help reduce confusion for sysadmins:

  • We should document in the sample config file that the value of url_preview_ip_range_blacklist is ignored when using an HTTP proxy, as resolving URLs to preview is done by the proxy.
  • Currently it is required to set url_preview_ip_range_blacklist if URL previews are enabled. We shouldn't require url_preview_ip_range_blacklist to be set when using a proxy, and we should log a warning when url_preview_ip_range_blacklist is set and a proxy is in use so that the sysadmin isn't under the wrong impression regarding security of the system.

To be clear, if you're using a proxy for URL previews, you should do your ip blacklisting in the proxy config. Also note that url_preview_url_blacklist is still enforced whether a proxy is in use or not.

@anoadragon453 anoadragon453 added good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. labels Apr 14, 2021
@clokep
Copy link
Member

clokep commented Apr 14, 2021

We should also ensure that ip_blacklist says that it is ignored when a proxy is in use too. 👍 I'm not sure if it already has docs around that or not.

@anoadragon453
Copy link
Member Author

It does not!

# Prevent outgoing requests from being sent to the following blacklisted IP address
# CIDR ranges. If this option is not specified then it defaults to private IP
# address ranges (see the example below).
#
# The blacklist applies to the outbound requests for federation, identity servers,
# push servers, and for checking key validity for third-party invite events.
#
# (0.0.0.0 and :: are always blacklisted, whether or not they are explicitly
# listed here, since they correspond to unroutable addresses.)
#
# This option replaces federation_ip_range_blacklist in Synapse v1.25.0.
#
#ip_range_blacklist:

@richvdh
Copy link
Member

richvdh commented Jun 16, 2021

We should document in the sample config file that the value of url_preview_ip_range_blacklist is ignored when using an HTTP proxy, as resolving URLs to preview is done by the proxy.

We should also ensure that ip_blacklist says that it is ignored when a proxy is in use too.

I don't think these are quite accurate, are they? IIRC they are applied to the connection to the proxy. If we say that they are ignored, that is also confusing.

@clokep
Copy link
Member

clokep commented Jun 16, 2021

IIRC they are applied to the connection to the proxy. If we say that they are ignored, that is also confusing.

They are not. See #9084.

@richvdh
Copy link
Member

richvdh commented Jun 16, 2021

oh! sorry!

richvdh pushed a commit that referenced this issue Aug 3, 2021
Per issue #9812 using `url_preview_ip_range_blacklist` with a proxy via `HTTPS_PROXY` or `HTTP_PROXY` environment variables has some inconsistent bahavior than mentioned. This PR changes the following:

- Changes the Sample Config file to include a note mentioning that `url_preview_ip_range_blacklist` and `ip_range_blacklist` is ignored when using a proxy
- Changes some logic in synapse/config/repository.py to send a warning when both `*ip_range_blacklist` configs and a proxy environment variable are set and but no longer throws an error.

Signed-off-by: Kento Okamoto <[email protected]>
@clokep
Copy link
Member

clokep commented Aug 3, 2021

Can this be closed now that #10129 was merged?

@anoadragon453
Copy link
Member Author

I think so - thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

3 participants