Merge pull request #4613 from matrix-org/rav/deprecate_no_tls

Infer no_tls from presence of TLS listeners

11 files changed, 82 insertions, 44 deletions

diff --git a/changelog.d/4613.feature b/changelog.d/4613.feature
new file mode 100644
index 0000000000..098f906af2
--- /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 0000000000..098f906af2
--- /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/4616.misc b/changelog.d/4616.misc
new file mode 100644
index 0000000000..ee79e208e3
--- /dev/null
+++ b/changelog.d/4616.misc
@@ -0,0 +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 0000000000..098f906af2
--- /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/synapse/app/_base.py b/synapse/app/_base.py
index e1fc1afd5b..50fd17c0be 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 b4476bf16e..dbd98d394f 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 5aad062c36..727fdc54d8 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 ce0458195c..767897c419 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 9fcc79816d..86e6eb80db 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 286ad80100..85f2848fb1 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(
diff --git a/tests/config/test_tls.py b/tests/config/test_tls.py
index 4ccaf35603..d8fd18a9cb 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()