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

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

Merged
merged 57 commits into from Apr 25, 2019
Merged
Show file tree
Hide file tree
Changes from 54 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
dbb3319
Config option for verifying federation certificates
anoadragon453 Mar 28, 2019
5fd4cd0
Whitelist per domain
anoadragon453 Mar 29, 2019
9146059
Ability to specify list of custom CA certificates
anoadragon453 Apr 1, 2019
4d1002f
Documentation of new options
anoadragon453 Apr 1, 2019
904ea6c
Add changelog
anoadragon453 Apr 1, 2019
8e89fc5
fixes and lint
anoadragon453 Apr 1, 2019
6de7ab8
flake8 lied to me :c
anoadragon453 Apr 1, 2019
8b1c459
lint
anoadragon453 Apr 1, 2019
ffc9c10
punctuation
anoadragon453 Apr 1, 2019
da23aa2
Cleaner code logic
anoadragon453 Apr 1, 2019
2325928
consolidate logic
anoadragon453 Apr 1, 2019
0ce5b5b
words
anoadragon453 Apr 1, 2019
2851e64
Generate config and remove extra newline
anoadragon453 Apr 1, 2019
d95b4ef
No tls.
anoadragon453 Apr 1, 2019
0f07754
Fix documentation
anoadragon453 Apr 1, 2019
86dfaf4
Merge branch 'develop' into anoa/msc_1711
anoadragon453 Apr 1, 2019
9d27f8e
Cache config
anoadragon453 Apr 1, 2019
4f177c5
again
anoadragon453 Apr 1, 2019
5575f7a
again
anoadragon453 Apr 1, 2019
ee0c7e1
again
anoadragon453 Apr 1, 2019
a7d7c5a
Don't run validation code if validation is turned off
anoadragon453 Apr 2, 2019
fec0c9a
Remove TODO
anoadragon453 Apr 2, 2019
aeffa4d
Use platformTrust instead of verify=True
anoadragon453 Apr 2, 2019
821baa4
Give tests config with default config values
anoadragon453 Apr 2, 2019
40702b6
Check for type instead of not None
anoadragon453 Apr 2, 2019
cc149b1
Give MFA config access
anoadragon453 Apr 2, 2019
8647013
lint
anoadragon453 Apr 2, 2019
d194e5d
liiint
anoadragon453 Apr 2, 2019
3adf15d
Correct imports
anoadragon453 Apr 2, 2019
61a39a4
isort
anoadragon453 Apr 2, 2019
3e29d45
isort
anoadragon453 Apr 2, 2019
f38da61
Pass config
anoadragon453 Apr 2, 2019
3f1fe92
Shuffle config
anoadragon453 Apr 2, 2019
a8adde0
Restart build
anoadragon453 Apr 2, 2019
e083ae3
Address changes. Make federation_domain_whitelist not None
anoadragon453 Apr 2, 2019
a87b556
regen sample config
anoadragon453 Apr 2, 2019
983474d
Remove turning cert validation off from faq
anoadragon453 Apr 3, 2019
1fd5680
Don't laugh at my own jokes
anoadragon453 Apr 3, 2019
7c432de
Simplify with better exception handling
anoadragon453 Apr 3, 2019
4f03528
Raise config error if empty ca list
anoadragon453 Apr 3, 2019
892c71d
Change test defaults
anoadragon453 Apr 3, 2019
a5ab4af
None is very different from []
anoadragon453 Apr 3, 2019
999f7db
Don't break logic when refactoring
anoadragon453 Apr 3, 2019
507cdf2
fix domain whitelist
anoadragon453 Apr 3, 2019
9bd1432
regen config
anoadragon453 Apr 3, 2019
d3f926a
regen
anoadragon453 Apr 3, 2019
09f6622
provide an arg to default_config
anoadragon453 Apr 3, 2019
e337c2d
Addressed changes
anoadragon453 Apr 4, 2019
433db40
Address changes
anoadragon453 Apr 4, 2019
396eb64
Change federation whitelist stuff back
anoadragon453 Apr 4, 2019
3dbad06
lint
anoadragon453 Apr 4, 2019
595d6ec
fix domain_whitelist
anoadragon453 Apr 4, 2019
edf2dd4
sample confiiiiiiiiiiiiiiiiiiiiiiig
anoadragon453 Apr 4, 2019
93850f0
domain globbing
anoadragon453 Apr 4, 2019
6691998
Add comment and simplify diff
anoadragon453 Apr 8, 2019
92cc6b0
Heavier warning about disabling TLS verification
anoadragon453 Apr 8, 2019
bc4b148
regen sample config
anoadragon453 Apr 8, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/4967.feature
@@ -0,0 +1 @@
Implementation of [MSC1711](https://github.com/matrix-org/matrix-doc/pull/1711) including config options for requiring valid TLS certificates for federation traffic, the ability to disable TLS validation for specific domains, and the ability to specify your own list of CA certificates.
1 change: 0 additions & 1 deletion docs/MSC1711_certificates_FAQ.md
Expand Up @@ -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`.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit pointless, but ok

## FAQ

### Synapse 0.99.0 has just been released, what do I need to do right now?
Expand Down
33 changes: 33 additions & 0 deletions docs/sample_config.yaml
Expand Up @@ -257,6 +257,39 @@ listeners:
#
#tls_private_key_path: "CONFDIR/SERVERNAME.tls.key"

# Whether to verify TLS certificates when sending federation traffic.
#
# This currently defaults to `false`, however this will change in
# Synapse 1.0 when valid federation certificates will be required.
#
#federation_verify_certificates: true

# Skip federation certificate verification on the following whitelist
# of domains.
#
# This setting should only normally be used within a private network of
# homeservers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcgruenhage can you propose better wording then please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#
# Only effective if federation_verify_certicates is `true`.
#
#federation_certificate_verification_whitelist:
# - lon.example.com
# - *.domain.com
# - *.onion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# List of custom certificate authorities for federation traffic.
#
# This setting should only normally be used within a private network of
# homeservers.
#
# Note that this list will replace those that are provided by your
# operating environment. Certificates must be in PEM format.
#
#federation_custom_ca_list:
# - myCA1.pem
# - myCA2.pem
# - myCA3.pem

# ACME support: This will configure Synapse to request a valid TLS certificate
# for your configured `server_name` via Let's Encrypt.
#
Expand Down
8 changes: 5 additions & 3 deletions synapse/config/server.py
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this code being changed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

federation_domain_whitelist = config.get(
"federation_domain_whitelist", None
"federation_domain_whitelist", None,
)
# turn the whitelist into a hash for speed of lookup

self.federation_domain_whitelist = None
if federation_domain_whitelist is not None:
# turn the whitelist into a hash for speed of lookup
self.federation_domain_whitelist = {}

for domain in federation_domain_whitelist:
self.federation_domain_whitelist[domain] = True

Expand Down
91 changes: 85 additions & 6 deletions synapse/config/tls.py
Expand Up @@ -24,8 +24,10 @@
from unpaddedbase64 import encode_base64

from OpenSSL import crypto
from twisted.internet._sslverify import Certificate, trustRootFromCertificates

from synapse.config._base import Config, ConfigError
from synapse.util import glob_to_regex

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -70,6 +72,50 @@ def read_config(self, config):

self.tls_fingerprints = list(self._original_tls_fingerprints)

# Whether to verify certificates on outbound federation traffic
self.federation_verify_certificates = config.get(
"federation_verify_certificates", False,
)

# Whitelist of domains to not verify certificates for
fed_whitelist_entries = config.get(
"federation_certificate_verification_whitelist", [],
)

# Support globs (*) in whitelist values
self.federation_certificate_verification_whitelist = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for entry in fed_whitelist_entries:
# Convert globs to regex
entry_regex = glob_to_regex(entry)
self.federation_certificate_verification_whitelist.append(entry_regex)

# List of custom certificate authorities for federation traffic validation
custom_ca_list = config.get(
"federation_custom_ca_list", None,
)

# Read in and parse custom CA certificates
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 "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"any certificate files")

certs = []
for ca_file in custom_ca_list:
logger.debug("Reading custom CA certificate file: %s", ca_file)
content = self.read_file(ca_file)

# Parse the CA certificates
try:
cert_base = Certificate.loadPEM(content)
certs.append(cert_base)
except Exception as e:
raise ConfigError("Error parsing custom CA certificate file %s: %s"
% (ca_file, e))

self.federation_ca_trust_root = trustRootFromCertificates(certs)

# This config option applies to non-federation HTTP clients
# (e.g. for talking to recaptcha, identity servers, and such)
# It should never be used in production, and is intended for
Expand Down Expand Up @@ -99,15 +145,15 @@ def is_disk_cert_valid(self, allow_self_signed=True):
try:
with open(self.tls_certificate_file, 'rb') as f:
cert_pem = f.read()
except Exception:
logger.exception("Failed to read existing certificate off disk!")
raise
richvdh marked this conversation as resolved.
Show resolved Hide resolved
except Exception as e:
raise ConfigError("Failed to read existing certificate file %s: %s"
% (self.tls_certificate_file, e))

try:
tls_certificate = crypto.load_certificate(crypto.FILETYPE_PEM, cert_pem)
except Exception:
logger.exception("Failed to parse existing certificate off disk!")
raise
except Exception as e:
raise ConfigError("Failed to parse existing certificate file %s: %s"
% (self.tls_certificate_file, e))

if not allow_self_signed:
if tls_certificate.get_subject() == tls_certificate.get_issuer():
Expand Down Expand Up @@ -192,6 +238,39 @@ def default_config(self, config_dir_path, server_name, **kwargs):
#
#tls_private_key_path: "%(tls_private_key_path)s"

# Whether to verify TLS certificates when sending federation traffic.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
#
# This currently defaults to `false`, however this will change in
# Synapse 1.0 when valid federation certificates will be required.
#
#federation_verify_certificates: true

# Skip federation certificate verification on the following whitelist
# of domains.
#
# This setting should only normally be used within a private network of
# homeservers.
#
# Only effective if federation_verify_certicates is `true`.
#
#federation_certificate_verification_whitelist:
# - lon.example.com
# - *.domain.com
# - *.onion

# List of custom certificate authorities for federation traffic.
#
# This setting should only normally be used within a private network of
# homeservers.
#
# Note that this list will replace those that are provided by your
# operating environment. Certificates must be in PEM format.
#
#federation_custom_ca_list:
# - myCA1.pem
# - myCA2.pem
# - myCA3.pem

# ACME support: This will configure Synapse to request a valid TLS certificate
# for your configured `server_name` via Let's Encrypt.
#
Expand Down
33 changes: 27 additions & 6 deletions synapse/crypto/context_factory.py
Expand Up @@ -18,10 +18,10 @@
from zope.interface import implementer

from OpenSSL import SSL, crypto
from twisted.internet._sslverify import _defaultCurveName
from twisted.internet._sslverify import ClientTLSOptions, _defaultCurveName
from twisted.internet.abstract import isIPAddress, isIPv6Address
from twisted.internet.interfaces import IOpenSSLClientConnectionCreator
from twisted.internet.ssl import CertificateOptions, ContextFactory
from twisted.internet.ssl import CertificateOptions, ContextFactory, platformTrust
from twisted.python.failure import Failure

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -90,7 +90,7 @@ def infoCallback(connection, where, ret):


@implementer(IOpenSSLClientConnectionCreator)
class ClientTLSOptions(object):
class ClientTLSOptionsNoVerify(object):
"""
Client creator for TLS without certificate identity verification. This is a
copy of twisted.internet._sslverify.ClientTLSOptions with the identity
Expand Down Expand Up @@ -127,9 +127,30 @@ class ClientTLSOptionsFactory(object):
to remote servers for federation."""

def __init__(self, config):
# We don't use config options yet
self._options = CertificateOptions(verify=False)
self._config = config
self._options_noverify = CertificateOptions()

# Check if we're using a custom list of a CA certificates
trust_root = config.federation_ca_trust_root
if trust_root is None:
# Use CA root certs provided by OpenSSL
trust_root = platformTrust()

self._options_verify = CertificateOptions(trustRoot=trust_root)

def get_options(self, host):
# Use _makeContext so that we get a fresh OpenSSL CTX each time.
return ClientTLSOptions(host, self._options._makeContext())

# Check if certificate verification has been enabled
richvdh marked this conversation as resolved.
Show resolved Hide resolved
should_verify = self._config.federation_verify_certificates

# Check if we've disabled certificate verification for this host
if should_verify:
for regex in self._config.federation_certificate_verification_whitelist:
if regex.match(host):
should_verify = False
break

if should_verify:
return ClientTLSOptions(host, self._options_verify._makeContext())
return ClientTLSOptionsNoVerify(host, self._options_noverify._makeContext())
2 changes: 1 addition & 1 deletion synapse/http/federation/matrix_federation_agent.py
Expand Up @@ -149,7 +149,7 @@ def request(self, method, uri, headers=None, bodyProducer=None):
tls_options = None
else:
tls_options = self._tls_client_options_factory.get_options(
res.tls_server_name.decode("ascii")
res.tls_server_name.decode("ascii"),
)

# make sure that the Host header is set correctly
Expand Down
3 changes: 2 additions & 1 deletion tests/http/federation/test_matrix_federation_agent.py
Expand Up @@ -39,6 +39,7 @@
from tests.http import ServerTLSContext
from tests.server import FakeTransport, ThreadedMemoryReactorClock
from tests.unittest import TestCase
from tests.utils import default_config

logger = logging.getLogger(__name__)

Expand All @@ -53,7 +54,7 @@ def setUp(self):

self.agent = MatrixFederationAgent(
reactor=self.reactor,
tls_client_options_factory=ClientTLSOptionsFactory(None),
tls_client_options_factory=ClientTLSOptionsFactory(default_config("test")),
_well_known_tls_policy=TrustingTLSPolicyForHTTPS(),
_srv_resolver=self.mock_resolver,
_well_known_cache=self.well_known_cache,
Expand Down