Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config option for verifying federation certificates (MSC 1711) #4967

Merged
merged 57 commits into from Apr 25, 2019

Conversation

3 participants
@anoadragon453
Copy link
Member

commented Mar 28, 2019

Implementation of MSC 1711 for Synapse - aka verifying federation certificates. Adds three new config values:

  • One for verifying certificates
  • One for domains to exempt from the above
  • One for the use of custom CAs

TODO:

  • Enable/disable certificate validation via config
  • Domain whitelist for certificate validation
  • Custom CA list for certificate validation
  • Append new option info to MSC1711 FAQ

Note that certificate validation is currently set to False. I assume we want to change this during the 1.0 release.

Closes #4366

anoadragon453 added some commits Mar 28, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 1, 2019

Codecov Report

Merging #4967 into develop will decrease coverage by 0.05%.
The diff coverage is 37.77%.

@@             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

anoadragon453 added some commits Apr 1, 2019

@anoadragon453 anoadragon453 requested a review from richvdh Apr 3, 2019

Addressed changes

Show resolved Hide resolved docs/MSC1711_certificates_FAQ.md Outdated
federation_domain_whitelist = config.get(
"federation_domain_whitelist", None
"federation_domain_whitelist", [],

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 4, 2019

Member

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.

Show resolved Hide resolved synapse/config/tls.py Outdated
Show resolved Hide resolved synapse/config/tls.py
Show resolved Hide resolved synapse/config/tls.py Outdated
Show resolved Hide resolved synapse/crypto/context_factory.py Outdated
Show resolved Hide resolved synapse/crypto/context_factory.py Outdated
Show resolved Hide resolved synapse/config/tls.py Outdated
Show resolved Hide resolved synapse/crypto/context_factory.py
@@ -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

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 4, 2019

Member

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)

anoadragon453 added some commits Apr 4, 2019

Addressed

@anoadragon453 anoadragon453 requested a review from richvdh Apr 4, 2019

# of domains.
#
# This setting should only normally be used within a private network of
# homeservers.

This comment has been minimized.

Copy link
@jcgruenhage

jcgruenhage Apr 6, 2019

Member

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.

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

@jcgruenhage can you propose better wording then please.

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Apr 8, 2019

Author Member

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."

This comment has been minimized.

Copy link
@jcgruenhage

jcgruenhage Apr 8, 2019

Member

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

This comment has been minimized.

Copy link
@jcgruenhage

jcgruenhage Apr 6, 2019

Member

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?

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

I'd quite like to get this PR landed. Let's descope netmasks for now.

This comment has been minimized.

Copy link
@jcgruenhage

jcgruenhage Apr 8, 2019

Member

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`.


This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

bit pointless, but ok

# of domains.
#
# This setting should only normally be used within a private network of
# homeservers.

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

@jcgruenhage can you propose better wording then please.

#federation_certificate_verification_whitelist:
# - lon.example.com
# - *.domain.com
# - *.onion

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

I'd quite like to get this PR landed. Let's descope netmasks for now.

@@ -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

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

why is this code being changed?

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Apr 8, 2019

Author Member

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 = []

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

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 "

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 8, 2019

Member

a comment as to why this is the right thing to do would be helpful.

Addressed concerns.

@anoadragon453 anoadragon453 requested a review from richvdh Apr 8, 2019

Updated config string

@anoadragon453 anoadragon453 requested a review from jcgruenhage Apr 8, 2019

@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Apr 15, 2019

@babolivier babolivier requested review from matrix-org/synapse-core and removed request for richvdh Apr 18, 2019

@richvdh
Copy link
Member

left a comment

lgtm!

@richvdh richvdh merged commit 6824ddd into develop Apr 25, 2019

24 checks passed

buildkite/synapse Build #891 passed (13 minutes, 25 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 10 seconds)
Details
buildkite/synapse/isort Passed (16 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (16 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pep-8 Passed (54 seconds)
Details
buildkite/synapse/pipeline Passed (2 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-postgres-9-dot-4 Passed (11 minutes, 23 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-postgres-9-dot-5 Passed (11 minutes, 30 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-sqlite Passed (7 minutes, 24 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-sqlite-slash-old-deps Passed (8 minutes, 7 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-4 Passed (12 minutes, 4 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (11 minutes, 46 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (7 minutes, 38 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (7 minutes, 35 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (12 minutes, 20 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (12 minutes, 2 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (7 minutes, 17 seconds)
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
codecov/patch 37.77% of diff hit (target 0%)
Details
codecov/project 60.58% (target 0%)
Details

Homeserver Task Board automation moved this from In progress to Done Apr 25, 2019

anoadragon453 added a commit that referenced this pull request Apr 30, 2019

Merge branch 'develop' into anoa/blacklist_ip_ranges
* 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
  ...

@anoadragon453 anoadragon453 referenced this pull request May 9, 2019

Closed

support for MSC1711 #4366

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.