Config option for verifying federation certificates (MSC 1711) #4967
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4967 +/- ##
===========================================
- Coverage 60.63% 60.58% -0.06%
===========================================
Files 332 332
Lines 34255 34269 +14
Branches 5658 5664 +6
===========================================
- Hits 20772 20762 -10
- Misses 12005 12025 +20
- Partials 1478 1482 +4 |
synapse/config/server.py
Outdated
| federation_domain_whitelist = config.get( | ||
| "federation_domain_whitelist", None | ||
| "federation_domain_whitelist", [], |
There was a problem hiding this comment.
hrm, could we keep changes to federation_domain_whitelist in a separate PR? there's already quite a lot going on here and it's hard to keep track of an extra dimension of changes.
tests/utils.py
Outdated
| @@ -137,6 +137,9 @@ def default_config(name): | |||
| config.email_enable_notifs = False | |||
| config.block_non_admin_invites = False | |||
| config.federation_domain_whitelist = None | |||
| config.federation_certificate_verification_whitelist = None | |||
There was a problem hiding this comment.
See the comment at line 126. These lines should be unnecessary. If you want to set the default config for all tests, update the config dict above (and add a comment to justify it)
docs/sample_config.yaml
Outdated
| # of domains. | ||
| # | ||
| # This setting should only normally be used within a private network of | ||
| # homeservers. |
There was a problem hiding this comment.
I disagree with this warning, there's a bunch of networks where this is useful that aren't private where this is useful. For private networks of homeservers, custom CAs are the right tool, not disabling TLS verification.
There was a problem hiding this comment.
@jcgruenhage can you propose better wording then please.
There was a problem hiding this comment.
How about "This setting should only normally be used within a private network of homeservers, and even then using a private CA should be taken into consideration before disabling TLS verification entirely."
There was a problem hiding this comment.
I'd say "This setting should only be used in very specific cases, such as federation over Tor hidden services and similar. For private networks of homeservers, you likely want to use a private CA instead."
| #federation_certificate_verification_whitelist: | ||
| # - lon.example.com | ||
| # - *.domain.com | ||
| # - *.onion |
There was a problem hiding this comment.
Small nitpicky thing about this whitelist, MSC 1711 also spoke about excluding net masks, not only domains. This seems to not be dealt with at all in this PR?
There was a problem hiding this comment.
I'd quite like to get this PR landed. Let's descope netmasks for now.
There was a problem hiding this comment.
I'm fine with that as long as they are still in scope for Synapse 1.0
| @@ -177,7 +177,6 @@ You can do this with a `.well-known` file as follows: | |||
| on `customer.example.net:8000` it correctly handles HTTP requests with | |||
| Host header set to `customer.example.net:8000`. | |||
|
|
|||
|
|
|||
docs/sample_config.yaml
Outdated
| # of domains. | ||
| # | ||
| # This setting should only normally be used within a private network of | ||
| # homeservers. |
There was a problem hiding this comment.
@jcgruenhage can you propose better wording then please.
| #federation_certificate_verification_whitelist: | ||
| # - lon.example.com | ||
| # - *.domain.com | ||
| # - *.onion |
There was a problem hiding this comment.
I'd quite like to get this PR landed. Let's descope netmasks for now.
synapse/config/server.py
Outdated
| @@ -111,13 +111,15 @@ def read_config(self, config): | |||
| self.admin_contact = config.get("admin_contact", None) | |||
|
|
|||
| # FIXME: federation_domain_whitelist needs sytests | |||
| self.federation_domain_whitelist = None | |||
There was a problem hiding this comment.
Left over from previous refactor that was reverted. Have changed code back so there's less diff now.
| ) | ||
|
|
||
| # Support globs (*) in whitelist values | ||
| self.federation_certificate_verification_whitelist = [] |
There was a problem hiding this comment.
for future reference, you could write this:
self.federation_certificate_verification_whitelist = [glob_to_regex(entry) for entry in fed_whitelist_entries]
what you have is fine though.
| self.federation_ca_trust_root = None | ||
| if custom_ca_list is not None: | ||
| if len(custom_ca_list) == 0: | ||
| raise ConfigError("federation_custom_ca_list specified without " |
There was a problem hiding this comment.
a comment as to why this is the right thing to do would be helpful.
* 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 ...
Implementation of MSC 1711 for Synapse - aka verifying federation certificates. Adds three new config values:
TODO:
Note that certificate validation is currently set to
False. I assume we want to change this during the 1.0 release.Closes #4366