From 086f6f27d409520e71556cad4707cb2f70476e20 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 11 Feb 2019 21:00:41 +0000 Subject: [PATCH 1/5] Logging improvements around TLS certs Log which file we're reading keys and certs from, and refactor the code a bit in preparation for other work --- changelog.d/4615.misc | 1 + synapse/app/_base.py | 6 ++--- synapse/config/tls.py | 54 ++++++++++++++++++++++++++++--------------- 3 files changed, 39 insertions(+), 22 deletions(-) create mode 100644 changelog.d/4615.misc diff --git a/changelog.d/4615.misc b/changelog.d/4615.misc new file mode 100644 index 000000000..c7266fcfc --- /dev/null +++ b/changelog.d/4615.misc @@ -0,0 +1 @@ +Logging improvements around TLS certs diff --git a/synapse/app/_base.py b/synapse/app/_base.py index e1fc1afd5..6d72de1da 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -213,13 +213,11 @@ def refresh_certificate(hs): Refresh the TLS certificates that Synapse is using by re-reading them from disk and updating the TLS context factories to use them. """ - logging.info("Loading certificate from disk...") hs.config.read_certificate_from_disk() hs.tls_server_context_factory = context_factory.ServerContextFactory(hs.config) - logging.info("Certificate loaded.") if hs._listening_services: - logging.info("Updating context factories...") + logger.info("Updating context factories...") for i in hs._listening_services: # When you listenSSL, it doesn't make an SSL port but a TCP one with # a TLS wrapping factory around the factory you actually want to get @@ -234,7 +232,7 @@ def refresh_certificate(hs): False, i.factory.wrappedFactory ) - logging.info("Context factories updated.") + logger.info("Context factories updated.") def start(hs, listeners=None): diff --git a/synapse/config/tls.py b/synapse/config/tls.py index 9fcc79816..76d2add4f 100644 --- a/synapse/config/tls.py +++ b/synapse/config/tls.py @@ -25,7 +25,7 @@ from OpenSSL import crypto from synapse.config._base import Config -logger = logging.getLogger() +logger = logging.getLogger(__name__) class TlsConfig(Config): @@ -110,20 +110,10 @@ class TlsConfig(Config): """ Read the certificates from disk. """ - self.tls_certificate = self.read_tls_certificate(self.tls_certificate_file) - - # Check if it is self-signed, and issue a warning if so. - if self.tls_certificate.get_issuer() == self.tls_certificate.get_subject(): - warnings.warn( - ( - "Self-signed TLS certificates will not be accepted by Synapse 1.0. " - "Please either provide a valid certificate, or use Synapse's ACME " - "support to provision one." - ) - ) + self.tls_certificate = self.read_tls_certificate() if not self.no_tls: - self.tls_private_key = self.read_tls_private_key(self.tls_private_key_file) + self.tls_private_key = self.read_tls_private_key() self.tls_fingerprints = list(self._original_tls_fingerprints) @@ -250,10 +240,38 @@ class TlsConfig(Config): % locals() ) - def read_tls_certificate(self, cert_path): - cert_pem = self.read_file(cert_path, "tls_certificate") - return crypto.load_certificate(crypto.FILETYPE_PEM, cert_pem) + def read_tls_certificate(self): + """Reads the TLS certificate from the configured file, and returns it - def read_tls_private_key(self, private_key_path): - private_key_pem = self.read_file(private_key_path, "tls_private_key") + Also checks if it is self-signed, and warns if so + + Returns: + OpenSSL.crypto.X509: the certificate + """ + cert_path = self.tls_certificate_file + logger.info("Loading TLS certificate from %s", cert_path) + cert_pem = self.read_file(cert_path, "tls_certificate_path") + cert = crypto.load_certificate(crypto.FILETYPE_PEM, cert_pem) + + # Check if it is self-signed, and issue a warning if so. + if cert.get_issuer() == cert.get_subject(): + warnings.warn( + ( + "Self-signed TLS certificates will not be accepted by Synapse 1.0. " + "Please either provide a valid certificate, or use Synapse's ACME " + "support to provision one." + ) + ) + + return cert + + def read_tls_private_key(self): + """Reads the TLS private key from the configured file, and returns it + + Returns: + OpenSSL.crypto.PKey: the private key + """ + private_key_path = self.tls_private_key_file + logger.info("Loading TLS key from %s", private_key_path) + private_key_pem = self.read_file(private_key_path, "tls_private_key_path") return crypto.load_privatekey(crypto.FILETYPE_PEM, private_key_pem) From 2129dd1a023d1e221dab8753be3fbd7024963634 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 11 Feb 2019 21:13:53 +0000 Subject: [PATCH 2/5] Fail cleanly if listener config lacks a 'port' ... otherwise we would fail with a mysterious KeyError or something later. --- changelog.d/4616.misc | 1 + synapse/config/server.py | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 changelog.d/4616.misc diff --git a/changelog.d/4616.misc b/changelog.d/4616.misc new file mode 100644 index 000000000..ee79e208e --- /dev/null +++ b/changelog.d/4616.misc @@ -0,0 +1 @@ +Fail cleanly if listener config lacks a 'port' diff --git a/synapse/config/server.py b/synapse/config/server.py index ce0458195..eed9d7c81 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -129,6 +129,11 @@ class ServerConfig(Config): self.listeners = config.get("listeners", []) for listener in self.listeners: + if not isinstance(listener.get("port", None), int): + raise ConfigError( + "Listener configuration is lacking a valid 'port' option" + ) + bind_address = listener.pop("bind_address", None) bind_addresses = listener.setdefault("bind_addresses", []) From 9645728619828fda050fa08aaa25628f5db5d775 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 11 Feb 2019 21:30:59 +0000 Subject: [PATCH 3/5] Don't create server contexts when TLS is disabled we aren't going to use them anyway. --- changelog.d/4617.misc | 1 + synapse/app/_base.py | 5 +++++ synapse/crypto/context_factory.py | 4 +--- 3 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 changelog.d/4617.misc diff --git a/changelog.d/4617.misc b/changelog.d/4617.misc new file mode 100644 index 000000000..6d751865c --- /dev/null +++ b/changelog.d/4617.misc @@ -0,0 +1 @@ +Don't create server contexts when TLS is disabled diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 6d72de1da..6b3cb61ae 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -214,6 +214,11 @@ def refresh_certificate(hs): disk and updating the TLS context factories to use them. """ hs.config.read_certificate_from_disk() + + if hs.config.no_tls: + # nothing else to do here + return + hs.tls_server_context_factory = context_factory.ServerContextFactory(hs.config) if hs._listening_services: diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 286ad8010..85f2848fb 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -43,9 +43,7 @@ class ServerContextFactory(ContextFactory): logger.exception("Failed to enable elliptic curve for TLS") context.set_options(SSL.OP_NO_SSLv2 | SSL.OP_NO_SSLv3) context.use_certificate_chain_file(config.tls_certificate_file) - - if not config.no_tls: - context.use_privatekey(config.tls_private_key) + context.use_privatekey(config.tls_private_key) # https://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/ context.set_cipher_list( From 4fddf8fc77496d9bb3b5fa8835f0e5ba9a5a9926 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 11 Feb 2019 17:57:58 +0000 Subject: [PATCH 4/5] Infer no_tls from presence of TLS listeners Rather than have to specify `no_tls` explicitly, infer whether we need to load the TLS keys etc from whether we have any TLS-enabled listeners. --- changelog.d/4613.feature | 1 + changelog.d/4615.feature | 1 + changelog.d/4615.misc | 1 - changelog.d/4617.feature | 1 + changelog.d/4617.misc | 1 - synapse/app/_base.py | 2 +- synapse/app/homeserver.py | 5 ----- synapse/config/homeserver.py | 2 +- synapse/config/server.py | 23 ++++++++++++++++++++--- synapse/config/tls.py | 10 ++-------- 10 files changed, 27 insertions(+), 20 deletions(-) create mode 100644 changelog.d/4613.feature create mode 100644 changelog.d/4615.feature delete mode 100644 changelog.d/4615.misc create mode 100644 changelog.d/4617.feature delete mode 100644 changelog.d/4617.misc diff --git a/changelog.d/4613.feature b/changelog.d/4613.feature new file mode 100644 index 000000000..098f906af --- /dev/null +++ b/changelog.d/4613.feature @@ -0,0 +1 @@ +There is no longer any need to specify `no_tls`: it is inferred from the absence of TLS listeners diff --git a/changelog.d/4615.feature b/changelog.d/4615.feature new file mode 100644 index 000000000..098f906af --- /dev/null +++ b/changelog.d/4615.feature @@ -0,0 +1 @@ +There is no longer any need to specify `no_tls`: it is inferred from the absence of TLS listeners diff --git a/changelog.d/4615.misc b/changelog.d/4615.misc deleted file mode 100644 index c7266fcfc..000000000 --- a/changelog.d/4615.misc +++ /dev/null @@ -1 +0,0 @@ -Logging improvements around TLS certs diff --git a/changelog.d/4617.feature b/changelog.d/4617.feature new file mode 100644 index 000000000..098f906af --- /dev/null +++ b/changelog.d/4617.feature @@ -0,0 +1 @@ +There is no longer any need to specify `no_tls`: it is inferred from the absence of TLS listeners diff --git a/changelog.d/4617.misc b/changelog.d/4617.misc deleted file mode 100644 index 6d751865c..000000000 --- a/changelog.d/4617.misc +++ /dev/null @@ -1 +0,0 @@ -Don't create server contexts when TLS is disabled diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 6b3cb61ae..50fd17c0b 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -215,7 +215,7 @@ def refresh_certificate(hs): """ hs.config.read_certificate_from_disk() - if hs.config.no_tls: + if not hs.config.has_tls_listener(): # nothing else to do here return diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index b4476bf16..dbd98d394 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -90,11 +90,6 @@ class SynapseHomeServer(HomeServer): tls = listener_config.get("tls", False) site_tag = listener_config.get("tag", port) - if tls and config.no_tls: - raise ConfigError( - "Listener on port %i has TLS enabled, but no_tls is set" % (port,), - ) - resources = {} for res in listener_config["resources"]: for name in res["names"]: diff --git a/synapse/config/homeserver.py b/synapse/config/homeserver.py index 5aad062c3..727fdc54d 100644 --- a/synapse/config/homeserver.py +++ b/synapse/config/homeserver.py @@ -42,7 +42,7 @@ from .voip import VoipConfig from .workers import WorkerConfig -class HomeServerConfig(TlsConfig, ServerConfig, DatabaseConfig, LoggingConfig, +class HomeServerConfig(ServerConfig, TlsConfig, DatabaseConfig, LoggingConfig, RatelimitConfig, ContentRepositoryConfig, CaptchaConfig, VoipConfig, RegistrationConfig, MetricsConfig, ApiConfig, AppServiceConfig, KeyConfig, SAML2Config, CasConfig, diff --git a/synapse/config/server.py b/synapse/config/server.py index eed9d7c81..767897c41 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -126,14 +126,22 @@ class ServerConfig(Config): self.public_baseurl += '/' self.start_pushers = config.get("start_pushers", True) - self.listeners = config.get("listeners", []) - - for listener in self.listeners: + self.listeners = [] + for listener in config.get("listeners", []): if not isinstance(listener.get("port", None), int): raise ConfigError( "Listener configuration is lacking a valid 'port' option" ) + if listener.setdefault("tls", False): + # no_tls is not really supported any more, but let's grandfather it in + # here. + if config.get("no_tls", False): + logger.info( + "Ignoring TLS-enabled listener on port %i due to no_tls" + ) + continue + bind_address = listener.pop("bind_address", None) bind_addresses = listener.setdefault("bind_addresses", []) @@ -145,6 +153,8 @@ class ServerConfig(Config): if not bind_addresses: bind_addresses.extend(DEFAULT_BIND_ADDRESSES) + self.listeners.append(listener) + if not self.web_client_location: _warn_if_webclient_configured(self.listeners) @@ -152,6 +162,9 @@ class ServerConfig(Config): bind_port = config.get("bind_port") if bind_port: + if config.get("no_tls", False): + raise ConfigError("no_tls is incompatible with bind_port") + self.listeners = [] bind_host = config.get("bind_host", "") gzip_responses = config.get("gzip_responses", True) @@ -198,6 +211,7 @@ class ServerConfig(Config): "port": manhole, "bind_addresses": ["127.0.0.1"], "type": "manhole", + "tls": False, }) metrics_port = config.get("metrics_port") @@ -223,6 +237,9 @@ class ServerConfig(Config): _check_resource_config(self.listeners) + def has_tls_listener(self): + return any(l["tls"] for l in self.listeners) + def default_config(self, server_name, data_dir_path, **kwargs): _, bind_port = parse_and_validate_server_name(server_name) if bind_port is not None: diff --git a/synapse/config/tls.py b/synapse/config/tls.py index 76d2add4f..e37a41eff 100644 --- a/synapse/config/tls.py +++ b/synapse/config/tls.py @@ -51,7 +51,6 @@ class TlsConfig(Config): self._original_tls_fingerprints = [] self.tls_fingerprints = list(self._original_tls_fingerprints) - self.no_tls = config.get("no_tls", False) # This config option applies to non-federation HTTP clients # (e.g. for talking to recaptcha, identity servers, and such) @@ -141,6 +140,8 @@ class TlsConfig(Config): return ( """\ + ## TLS ## + # PEM-encoded X509 certificate for TLS. # This certificate, as of Synapse 1.0, will need to be a valid and verifiable # certificate, signed by a recognised Certificate Authority. @@ -201,13 +202,6 @@ class TlsConfig(Config): # # reprovision_threshold: 30 - # If your server runs behind a reverse-proxy which terminates TLS connections - # (for both client and federation connections), it may be useful to disable - # All TLS support for incoming connections. Setting no_tls to True will - # do so (and avoid the need to give synapse a TLS private key). - # - # no_tls: True - # List of allowed TLS fingerprints for this server to publish along # with the signing keys for this server. Other matrix servers that # make HTTPS requests to this server will check that the TLS From 0ca290865350212e1834730c918973162a3067f4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 11 Feb 2019 22:01:27 +0000 Subject: [PATCH 5/5] fix tests --- synapse/config/tls.py | 2 +- tests/config/test_tls.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/config/tls.py b/synapse/config/tls.py index e37a41eff..86e6eb80d 100644 --- a/synapse/config/tls.py +++ b/synapse/config/tls.py @@ -111,7 +111,7 @@ class TlsConfig(Config): """ self.tls_certificate = self.read_tls_certificate() - if not self.no_tls: + if self.has_tls_listener(): self.tls_private_key = self.read_tls_private_key() self.tls_fingerprints = list(self._original_tls_fingerprints) diff --git a/tests/config/test_tls.py b/tests/config/test_tls.py index 4ccaf3560..d8fd18a9c 100644 --- a/tests/config/test_tls.py +++ b/tests/config/test_tls.py @@ -20,6 +20,11 @@ from synapse.config.tls import TlsConfig from tests.unittest import TestCase +class TestConfig(TlsConfig): + def has_tls_listener(self): + return False + + class TLSConfigTests(TestCase): def test_warn_self_signed(self): @@ -55,11 +60,10 @@ s4niecZKPBizL6aucT59CsunNmmb5Glq8rlAcU+1ZTZZzGYqVYhF6axB9Qg= config = { "tls_certificate_path": os.path.join(config_dir, "cert.pem"), - "no_tls": True, "tls_fingerprints": [] } - t = TlsConfig() + t = TestConfig() t.read_config(config) t.read_certificate_from_disk()