From e083ae39cd451b42f75c1ca5f24505b663227718 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 2 Apr 2019 18:10:13 +0100 Subject: [PATCH] Address changes. Make federation_domain_whitelist not None --- synapse/config/server.py | 10 +++--- synapse/config/tls.py | 35 ++++++++++--------- synapse/crypto/context_factory.py | 9 +++-- synapse/federation/transport/server.py | 5 +-- synapse/http/matrixfederationclient.py | 6 +--- synapse/rest/key/v2/remote_key_resource.py | 5 +-- synapse/rest/media/v1/media_repository.py | 10 ++---- .../test_matrix_federation_agent.py | 8 ++--- tests/utils.py | 5 ++- 9 files changed, 40 insertions(+), 53 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 08e4e45482..aa2bb0d040 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -111,15 +111,13 @@ class ServerConfig(Config): self.admin_contact = config.get("admin_contact", None) # FIXME: federation_domain_whitelist needs sytests - self.federation_domain_whitelist = None + self.federation_domain_whitelist = {} federation_domain_whitelist = config.get( - "federation_domain_whitelist", None + "federation_domain_whitelist", [], ) # turn the whitelist into a hash for speed of lookup - if federation_domain_whitelist is not None: - self.federation_domain_whitelist = {} - for domain in federation_domain_whitelist: - self.federation_domain_whitelist[domain] = True + for domain in federation_domain_whitelist: + self.federation_domain_whitelist[domain] = True if self.public_baseurl is not None: if self.public_baseurl[-1] != '/': diff --git a/synapse/config/tls.py b/synapse/config/tls.py index 4e0f2d9d75..05b6a6165f 100644 --- a/synapse/config/tls.py +++ b/synapse/config/tls.py @@ -77,16 +77,14 @@ class TlsConfig(Config): ) # Whitelist of domains to not verify certificates for - self.federation_certificate_verification_whitelist = None + self.federation_certificate_verification_whitelist = {} federation_certificate_verification_whitelist = config.get( - "federation_certificate_verification_whitelist", None + "federation_certificate_verification_whitelist", [], ) # Store whitelisted domains in a hash for fast lookup - if federation_certificate_verification_whitelist is not None: - self.federation_certificate_verification_whitelist = {} - for domain in federation_certificate_verification_whitelist: - self.federation_certificate_verification_whitelist[domain] = True + for domain in federation_certificate_verification_whitelist: + self.federation_certificate_verification_whitelist[domain] = True # List of custom certificate authorities for federation traffic validation self.federation_custom_ca_list = config.get( @@ -102,7 +100,7 @@ class TlsConfig(Config): with open(ca_file, 'rb') as f: content = f.read() except Exception: - logger.exception("Failed to read custom CA certificate off disk!") + logger.fatal("Failed to read custom CA certificate off disk!") raise # Parse the CA certificates @@ -110,11 +108,10 @@ class TlsConfig(Config): cert_base = Certificate.loadPEM(content) certs.append(cert_base) except Exception: - logger.exception("Failed to parse custom CA certificate off disk!") + logger.fatal("Failed to parse custom CA certificate off disk!") raise - if len(certs) > 0: - self.federation_custom_ca_list = trustRootFromCertificates(certs) + self.federation_custom_ca_list = trustRootFromCertificates(certs) # This config option applies to non-federation HTTP clients # (e.g. for talking to recaptcha, identity servers, and such) @@ -146,14 +143,12 @@ class TlsConfig(Config): 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 + logger.fatal("Failed to read existing certificate off disk!") try: tls_certificate = crypto.load_certificate(crypto.FILETYPE_PEM, cert_pem) except Exception: - logger.exception("Failed to parse existing certificate off disk!") - raise + logger.fatal("Failed to parse existing certificate off disk!") if not allow_self_signed: if tls_certificate.get_subject() == tls_certificate.get_issuer(): @@ -240,10 +235,18 @@ class TlsConfig(Config): # 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 - # Prevent federation certificate validation on the following whitelist - # of domains. Only effective if federation_verify_certicates is true. + # Skip federation certificate validation on the following whitelist of + # domains. + # + # Note that this should only be used within the context of private + # federation as it will otherwise break things. + # + # Only effective if federation_verify_certicates is `true`. # #federation_certificate_validation_whitelist: # - lon.example.com diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index db2fbb4097..d566e1bf23 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -18,7 +18,10 @@ import logging from zope.interface import implementer from OpenSSL import SSL, crypto -from twisted.internet._sslverify import OpenSSLCertificateAuthorities, _defaultCurveName +from twisted.internet._sslverify import ( + ClientTLSOptions as ClientTLSOptionsVerify, + _defaultCurveName, +) from twisted.internet.abstract import isIPAddress, isIPv6Address from twisted.internet.interfaces import IOpenSSLClientConnectionCreator from twisted.internet.ssl import CertificateOptions, ContextFactory, platformTrust @@ -132,7 +135,7 @@ class ClientTLSOptionsFactory(object): self._options_novalidate = CertificateOptions() # Check if we're using a custom list of a CA certificates - if isinstance(config.federation_custom_ca_list, OpenSSLCertificateAuthorities): + if config.federation_custom_ca_list is not None: self._options_validate = CertificateOptions( # Use custom CA trusted root certs trustRoot=config.federation_custom_ca_list, @@ -152,7 +155,7 @@ class ClientTLSOptionsFactory(object): if (self._config.federation_verify_certificates and host not in self._config.federation_certificate_validation_whitelist): # Require verification - return ClientTLSOptions(host, self._options_validate._makeContext()) + return ClientTLSOptionsVerify(host, self._options_validate._makeContext()) # Otherwise don't require verification return ClientTLSOptions(host, self._options_novalidate._makeContext()) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 452599e1a1..f28672f7e2 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -127,10 +127,7 @@ class Authenticator(object): json_request["origin"] = origin json_request["signatures"].setdefault(origin, {})[key] = sig - if ( - self.federation_domain_whitelist is not None and - origin not in self.federation_domain_whitelist - ): + if (origin not in self.federation_domain_whitelist): raise FederationDeniedError(origin) if not json_request["signatures"]: diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index cd17b48b71..c4fdcd0524 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -177,7 +177,6 @@ class MatrixFederationHttpClient(object): self.agent = MatrixFederationAgent( hs.get_reactor(), tls_client_options_factory, - hs.config, ) self.clock = hs.get_clock() self._store = hs.get_datastore() @@ -284,10 +283,7 @@ class MatrixFederationHttpClient(object): else: _sec_timeout = self.default_timeout - if ( - self.hs.config.federation_domain_whitelist is not None and - request.destination not in self.hs.config.federation_domain_whitelist - ): + if (request.destination not in self.hs.config.federation_domain_whitelist): raise FederationDeniedError(request.destination) limiter = yield synapse.util.retryutils.get_retry_limiter( diff --git a/synapse/rest/key/v2/remote_key_resource.py b/synapse/rest/key/v2/remote_key_resource.py index eb8782aa6e..dbd4512b74 100644 --- a/synapse/rest/key/v2/remote_key_resource.py +++ b/synapse/rest/key/v2/remote_key_resource.py @@ -139,10 +139,7 @@ class RemoteKey(Resource): store_queries = [] for server_name, key_ids in query.items(): - if ( - self.federation_domain_whitelist is not None and - server_name not in self.federation_domain_whitelist - ): + if (server_name not in self.federation_domain_whitelist): logger.debug("Federation denied with %s", server_name) continue diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index bdffa97805..dec1206e39 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -231,10 +231,7 @@ class MediaRepository(object): Deferred: Resolves once a response has successfully been written to request """ - if ( - self.federation_domain_whitelist is not None and - server_name not in self.federation_domain_whitelist - ): + if (server_name not in self.federation_domain_whitelist): raise FederationDeniedError(server_name) self.mark_recently_accessed(server_name, media_id) @@ -271,10 +268,7 @@ class MediaRepository(object): Returns: Deferred[dict]: The media_info of the file """ - if ( - self.federation_domain_whitelist is not None and - server_name not in self.federation_domain_whitelist - ): + if (server_name not in self.federation_domain_whitelist): raise FederationDeniedError(server_name) # We linearize here to ensure that we don't try and download remote diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 0e732580cc..dec91e86a3 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -39,6 +39,7 @@ from synapse.util.logcontext import LoggingContext 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__) @@ -49,16 +50,11 @@ class MatrixFederationAgentTests(TestCase): self.mock_resolver = Mock() - config = Mock() - config.federation_custom_ca_list = None - config.federation_verify_certificates = False - config.federation_certificate_validation_whitelist = [] - self.well_known_cache = TTLCache("test_cache", timer=self.reactor.seconds) self.agent = MatrixFederationAgent( reactor=self.reactor, - tls_client_options_factory=ClientTLSOptionsFactory(config), + tls_client_options_factory=ClientTLSOptionsFactory(default_config()), _well_known_tls_policy=TrustingTLSPolicyForHTTPS(), _srv_resolver=self.mock_resolver, _well_known_cache=self.well_known_cache, diff --git a/tests/utils.py b/tests/utils.py index cb75514851..c6952938d0 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -136,7 +136,10 @@ def default_config(name): config.worker_app = None config.email_enable_notifs = False config.block_non_admin_invites = False - config.federation_domain_whitelist = None + config.federation_domain_whitelist = [] + config.federation_certificate_verification_whitelist = [] + config.federation_custom_ca_list = [] + config.federation_verify_certificates = False config.federation_rc_reject_limit = 10 config.federation_rc_sleep_limit = 10 config.federation_rc_sleep_delay = 100