Repository: matrix-org/synapse

Issue:
Synapse should not require TLS certificate files
Testing with 0.99rc4, it seems like:

- regardless of the `no_tls` setting's value (`False` or `True`), Synapse would still try to load `tls_certificate_path` or `tls_private_key_path` (I'm not sure which one now, but it does try to read it and fails with an exception if it's missing or not a valid file)

- the comment in `homeserver.yaml` about `no_tls` seems backward ("Setting no_tls to False will do so (and avoid the need to give synapse a TLS private key).") I guess this should say `True`, rather than `False`

----------

I wish to use Synapse with a reverse-proxy and this issue is a blocker for that. I'd rather not provide any certificate files to Synapse at all.

Now, looking at #2840, unless something changed, it seems like Synapse may really need to access the certificate file.

This leads to all sorts of pain, especially when automated renewal is involved.
Not only do we need to renew the certificate and reload the reverse-proxy (no downtime there), but we also need to **restart** Synapse at the same time (reloading doesn't seem possible as explained in #1180).

Discussion:

Patch:
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,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,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/4616.misc b/changelog.d/4616.misc
new file mode 100644
index 000000000..ee79e208e
--- /dev/null
+++ b/changelog.d/4616.misc
@@ -0,0 +1,1 @@
+Fail cleanly if listener config lacks a 'port'
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,1 @@
+There is no longer any need to specify `no_tls`: it is inferred from the absence of TLS listeners
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index e1fc1afd5..50fd17c0b 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -213,13 +213,16 @@ 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()
+
+    if not hs.config.has_tls_listener():
+        # nothing else to do here
+        return
+
     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 +237,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/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 ce0458195..767897c41 100644
--- a/synapse/config/server.py
+++ b/synapse/config/server.py
@@ -126,9 +126,22 @@ class ServerConfig(Config):
                 self.public_baseurl += '/'
         self.start_pushers = config.get("start_pushers", True)
 
-        self.listeners = config.get("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
 
-        for listener in self.listeners:
             bind_address = listener.pop("bind_address", None)
             bind_addresses = listener.setdefault("bind_addresses", [])
 
@@ -140,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)
 
@@ -147,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)
@@ -193,6 +211,7 @@ class ServerConfig(Config):
                 "port": manhole,
                 "bind_addresses": ["127.0.0.1"],
                 "type": "manhole",
+                "tls": False,
             })
 
         metrics_port = config.get("metrics_port")
@@ -218,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 9fcc79816..86e6eb80d 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):
@@ -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)
@@ -110,20 +109,10 @@ class TlsConfig(Config):
         """
         Read the certificates from disk.
         """
-        self.tls_certificate = self.read_tls_certificate(self.tls_certificate_file)
+        self.tls_certificate = self.read_tls_certificate()
 
-        # 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."
-                )
-            )
-
-        if not self.no_tls:
-            self.tls_private_key = self.read_tls_private_key(self.tls_private_key_file)
+        if self.has_tls_listener():
+            self.tls_private_key = self.read_tls_private_key()
 
         self.tls_fingerprints = list(self._original_tls_fingerprints)
 
@@ -151,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.
@@ -211,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
@@ -250,10 +234,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)
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(


Test patch:
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()