Config option for verifying federation certificates (MSC 1711) by anoadragon453 · Pull Request #4967 · matrix-org/synapse

anoadragon453 · 2019-03-28T18:10:55Z

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

codecov · 2019-04-01T13:59:54Z

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

Addressed changes

docs/MSC1711_certificates_FAQ.md

richvdh · 2019-04-04T11:42:13Z

synapse/config/server.py

-        self.federation_domain_whitelist = None
        federation_domain_whitelist = config.get(
-            "federation_domain_whitelist", None
+            "federation_domain_whitelist", [],


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.

synapse/config/tls.py

synapse/crypto/context_factory.py

synapse/config/tls.py

synapse/crypto/context_factory.py

richvdh · 2019-04-04T12:05:51Z

tests/utils.py

    config.email_enable_notifs = False
    config.block_non_admin_invites = False
    config.federation_domain_whitelist = None
+    config.federation_certificate_verification_whitelist = None


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)

Addressed

jcgruenhage · 2019-04-06T09:10:52Z

docs/sample_config.yaml

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


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.

@jcgruenhage can you propose better wording then please.

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

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

jcgruenhage · 2019-04-06T09:14:06Z

docs/sample_config.yaml

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


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?

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

I'm fine with that as long as they are still in scope for Synapse 1.0

richvdh · 2019-04-08T11:25:59Z

docs/MSC1711_certificates_FAQ.md

       on `customer.example.net:8000` it correctly handles HTTP requests with
       Host header set to `customer.example.net:8000`.

-


bit pointless, but ok

richvdh · 2019-04-08T11:26:25Z

docs/sample_config.yaml

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


@jcgruenhage can you propose better wording then please.

richvdh · 2019-04-08T11:26:47Z

docs/sample_config.yaml

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


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

richvdh · 2019-04-08T11:29:05Z

synapse/config/server.py

        self.admin_contact = config.get("admin_contact", None)

        # FIXME: federation_domain_whitelist needs sytests
-        self.federation_domain_whitelist = None


why is this code being changed?

Left over from previous refactor that was reverted. Have changed code back so there's less diff now.

richvdh · 2019-04-08T11:30:31Z

synapse/config/tls.py

+        )
+
+        # Support globs (*) in whitelist values
+        self.federation_certificate_verification_whitelist = []


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.

richvdh · 2019-04-08T11:30:54Z

synapse/config/tls.py

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


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

Addressed concerns.

Updated config string

richvdh

lgtm!

* 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 added 11 commits March 28, 2019 18:08

Config option for verifying federation certificates

dbb3319

Whitelist per domain

5fd4cd0

Ability to specify list of custom CA certificates

9146059

Documentation of new options

4d1002f

Add changelog

904ea6c

fixes and lint

8e89fc5

flake8 lied to me :c

6de7ab8

lint

8b1c459

punctuation

ffc9c10

Cleaner code logic

da23aa2

consolidate logic

2325928

anoadragon453 added 18 commits April 1, 2019 15:01

words

0ce5b5b

Generate config and remove extra newline

2851e64

No tls.

d95b4ef

Fix documentation

0f07754

Merge branch 'develop' into anoa/msc_1711

86dfaf4

Cache config

9d27f8e

again

4f177c5

again

5575f7a

again

ee0c7e1

Don't run validation code if validation is turned off

a7d7c5a

Remove TODO

fec0c9a

Use platformTrust instead of verify=True

aeffa4d

Give tests config with default config values

821baa4

Check for type instead of not None

40702b6

Give MFA config access

cc149b1

lint

8647013

liiint

d194e5d

Correct imports

3adf15d

anoadragon453 added 2 commits April 3, 2019 13:27

regen

d3f926a

provide an arg to default_config

09f6622

anoadragon453 requested a review from richvdh April 3, 2019 12:40

richvdh previously requested changes Apr 4, 2019

View reviewed changes

anoadragon453 added 7 commits April 4, 2019 15:27

Addressed changes

e337c2d

Address changes

433db40

Change federation whitelist stuff back

396eb64

lint

3dbad06

fix domain_whitelist

595d6ec

sample confiiiiiiiiiiiiiiiiiiiiiiig

edf2dd4

domain globbing

93850f0

anoadragon453 requested a review from richvdh April 4, 2019 15:45

jcgruenhage previously requested changes Apr 6, 2019

View reviewed changes

richvdh previously requested changes Apr 8, 2019

View reviewed changes

Add comment and simplify diff

6691998

anoadragon453 requested a review from richvdh April 8, 2019 12:43

Heavier warning about disabling TLS verification

92cc6b0

anoadragon453 requested a review from jcgruenhage April 8, 2019 16:59

regen sample config

bc4b148

babolivier requested review from a team and removed request for richvdh April 18, 2019 09:49

richvdh approved these changes Apr 25, 2019

View reviewed changes

richvdh merged commit 6824ddd into develop Apr 25, 2019

anoadragon453 mentioned this pull request May 9, 2019

support for MSC1711 #4366

Closed

		on `customer.example.net:8000` it correctly handles HTTP requests with
		Host header set to `customer.example.net:8000`.

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anoadragon453 commented Mar 28, 2019 •

edited

Loading

codecov bot commented Apr 1, 2019 •

edited

Loading