From 9cd33d2f4bc14a165f693e2d3dfe231179e968e8 Mon Sep 17 00:00:00 2001
From: Amber Brown <hawkowl@atleastfornow.net>
Date: Fri, 8 Feb 2019 17:25:57 +0000
Subject: Deduplicate some code in synapse.app (#4567)

---
 synapse/app/_base.py | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

(limited to 'synapse/app/_base.py')

diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index 5b97a54d45..3cbb003035 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -15,7 +15,9 @@
 
 import gc
 import logging
+import signal
 import sys
+import traceback
 
 import psutil
 from daemonize import Daemonize
@@ -23,11 +25,25 @@ from daemonize import Daemonize
 from twisted.internet import error, reactor
 
 from synapse.app import check_bind_error
+from synapse.crypto import context_factory
 from synapse.util import PreserveLoggingContext
 from synapse.util.rlimit import change_resource_limit
 
 logger = logging.getLogger(__name__)
 
+_sighup_callbacks = []
+
+
+def register_sighup(func):
+    """
+    Register a function to be called when a SIGHUP occurs.
+
+    Args:
+        func (function): Function to be called when sent a SIGHUP signal.
+            Will be called with a single argument, the homeserver.
+    """
+    _sighup_callbacks.append(func)
+
 
 def start_worker_reactor(appname, config):
     """ Run the reactor in the main process
@@ -189,3 +205,50 @@ def listen_ssl(
 
     logger.info("Synapse now listening on port %d (TLS)", port)
     return r
+
+
+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)
+    hs.tls_client_options_factory = context_factory.ClientTLSOptionsFactory(
+        hs.config
+    )
+    logging.info("Certificate loaded.")
+
+
+def start(hs, listeners=None):
+    """
+    Start a Synapse server or worker.
+
+    Args:
+        hs (synapse.server.HomeServer)
+        listeners (list[dict]): Listener configuration ('listeners' in homeserver.yaml)
+    """
+    try:
+        # Set up the SIGHUP machinery.
+        if hasattr(signal, "SIGHUP"):
+            def handle_sighup(*args, **kwargs):
+                for i in _sighup_callbacks:
+                    i(hs)
+
+            signal.signal(signal.SIGHUP, handle_sighup)
+
+            register_sighup(refresh_certificate)
+
+        # Load the certificate from disk.
+        refresh_certificate(hs)
+
+        # It is now safe to start your Synapse.
+        hs.start_listening(listeners)
+        hs.get_datastore().start_profiling()
+    except Exception:
+        traceback.print_exc(file=sys.stderr)
+        reactor = hs.get_reactor()
+        if reactor.running:
+            reactor.stop()
+        sys.exit(1)
-- 
cgit 1.5.1


From 6e2a5aa050fc132a7dee6b3e33a7a368207d7e5a Mon Sep 17 00:00:00 2001
From: Amber Brown <hawkowl@atleastfornow.net>
Date: Mon, 11 Feb 2019 21:36:26 +1100
Subject: ACME Reprovisioning (#4522)

---
 changelog.d/4522.feature  |  1 +
 synapse/app/_base.py      | 19 ++++++++++++
 synapse/app/homeserver.py | 79 +++++++++++++++++++++++++++++++++--------------
 synapse/config/tls.py     | 12 ++++++-
 synapse/server.py         |  3 ++
 5 files changed, 89 insertions(+), 25 deletions(-)
 create mode 100644 changelog.d/4522.feature

(limited to 'synapse/app/_base.py')

diff --git a/changelog.d/4522.feature b/changelog.d/4522.feature
new file mode 100644
index 0000000000..ef18daf60b
--- /dev/null
+++ b/changelog.d/4522.feature
@@ -0,0 +1 @@
+Synapse's ACME support will now correctly reprovision a certificate that approaches its expiry while Synapse is running.
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index 3cbb003035..62c633146f 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -23,6 +23,7 @@ import psutil
 from daemonize import Daemonize
 
 from twisted.internet import error, reactor
+from twisted.protocols.tls import TLSMemoryBIOFactory
 
 from synapse.app import check_bind_error
 from synapse.crypto import context_factory
@@ -220,6 +221,24 @@ def refresh_certificate(hs):
     )
     logging.info("Certificate loaded.")
 
+    if hs._listening_services:
+        logging.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
+            # requests. This factory attribute is public but missing from
+            # Twisted's documentation.
+            if isinstance(i.factory, TLSMemoryBIOFactory):
+                # We want to replace TLS factories with a new one, with the new
+                # TLS configuration. We do this by reaching in and pulling out
+                # the wrappedFactory, and then re-wrapping it.
+                i.factory = TLSMemoryBIOFactory(
+                    hs.tls_server_context_factory,
+                    False,
+                    i.factory.wrappedFactory
+                )
+        logging.info("Context factories updated.")
+
 
 def start(hs, listeners=None):
     """
diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py
index d1cab07bb6..b4476bf16e 100755
--- a/synapse/app/homeserver.py
+++ b/synapse/app/homeserver.py
@@ -83,7 +83,6 @@ def gz_wrap(r):
 
 class SynapseHomeServer(HomeServer):
     DATASTORE_CLASS = DataStore
-    _listening_services = []
 
     def _listener_http(self, config, listener_config):
         port = listener_config["port"]
@@ -376,42 +375,73 @@ def setup(config_options):
 
     hs.setup()
 
+    @defer.inlineCallbacks
+    def do_acme():
+        """
+        Reprovision an ACME certificate, if it's required.
+
+        Returns:
+            Deferred[bool]: Whether the cert has been updated.
+        """
+        acme = hs.get_acme_handler()
+
+        # Check how long the certificate is active for.
+        cert_days_remaining = hs.config.is_disk_cert_valid(
+            allow_self_signed=False
+        )
+
+        # We want to reprovision if cert_days_remaining is None (meaning no
+        # certificate exists), or the days remaining number it returns
+        # is less than our re-registration threshold.
+        provision = False
+
+        if (cert_days_remaining is None):
+            provision = True
+
+        if cert_days_remaining > hs.config.acme_reprovision_threshold:
+            provision = True
+
+        if provision:
+            yield acme.provision_certificate()
+
+        defer.returnValue(provision)
+
+    @defer.inlineCallbacks
+    def reprovision_acme():
+        """
+        Provision a certificate from ACME, if required, and reload the TLS
+        certificate if it's renewed.
+        """
+        reprovisioned = yield do_acme()
+        if reprovisioned:
+            _base.refresh_certificate(hs)
+
     @defer.inlineCallbacks
     def start():
         try:
-            # Check if the certificate is still valid.
-            cert_days_remaining = hs.config.is_disk_cert_valid()
-
+            # Run the ACME provisioning code, if it's enabled.
             if hs.config.acme_enabled:
-                # If ACME is enabled, we might need to provision a certificate
-                # before starting.
                 acme = hs.get_acme_handler()
-
                 # Start up the webservices which we will respond to ACME
-                # challenges with.
+                # challenges with, and then provision.
                 yield acme.start_listening()
+                yield do_acme()
 
-                # We want to reprovision if cert_days_remaining is None (meaning no
-                # certificate exists), or the days remaining number it returns
-                # is less than our re-registration threshold.
-                if (cert_days_remaining is None) or (
-                    not cert_days_remaining > hs.config.acme_reprovision_threshold
-                ):
-                    yield acme.provision_certificate()
+                # Check if it needs to be reprovisioned every day.
+                hs.get_clock().looping_call(
+                    reprovision_acme,
+                    24 * 60 * 60 * 1000
+                )
 
             _base.start(hs, config.listeners)
 
             hs.get_pusherpool().start()
             hs.get_datastore().start_doing_background_updates()
-        except Exception as e:
-            # If a DeferredList failed (like in listening on the ACME listener),
-            # we need to print the subfailure explicitly.
-            if isinstance(e, defer.FirstError):
-                e.subFailure.printTraceback(sys.stderr)
-                sys.exit(1)
-
-            # Something else went wrong when starting. Print it and bail out.
+        except Exception:
+            # Print the exception and bail out.
             traceback.print_exc(file=sys.stderr)
+            if reactor.running:
+                reactor.stop()
             sys.exit(1)
 
     reactor.callWhenRunning(start)
@@ -420,7 +450,8 @@ def setup(config_options):
 
 
 class SynapseService(service.Service):
-    """A twisted Service class that will start synapse. Used to run synapse
+    """
+    A twisted Service class that will start synapse. Used to run synapse
     via twistd and a .tac.
     """
     def __init__(self, config):
diff --git a/synapse/config/tls.py b/synapse/config/tls.py
index 81b3a659fe..9fcc79816d 100644
--- a/synapse/config/tls.py
+++ b/synapse/config/tls.py
@@ -64,10 +64,14 @@ class TlsConfig(Config):
         self.tls_certificate = None
         self.tls_private_key = None
 
-    def is_disk_cert_valid(self):
+    def is_disk_cert_valid(self, allow_self_signed=True):
         """
         Is the certificate we have on disk valid, and if so, for how long?
 
+        Args:
+            allow_self_signed (bool): Should we allow the certificate we
+                read to be self signed?
+
         Returns:
             int: Days remaining of certificate validity.
             None: No certificate exists.
@@ -88,6 +92,12 @@ class TlsConfig(Config):
             logger.exception("Failed to parse existing certificate off disk!")
             raise
 
+        if not allow_self_signed:
+            if tls_certificate.get_subject() == tls_certificate.get_issuer():
+                raise ValueError(
+                    "TLS Certificate is self signed, and this is not permitted"
+                )
+
         # YYYYMMDDhhmmssZ -- in UTC
         expires_on = datetime.strptime(
             tls_certificate.get_notAfter().decode('ascii'), "%Y%m%d%H%M%SZ"
diff --git a/synapse/server.py b/synapse/server.py
index 6c52101616..a2cf8a91cd 100644
--- a/synapse/server.py
+++ b/synapse/server.py
@@ -112,6 +112,8 @@ class HomeServer(object):
 
     Attributes:
         config (synapse.config.homeserver.HomeserverConfig):
+        _listening_services (list[twisted.internet.tcp.Port]): TCP ports that
+            we are listening on to provide HTTP services.
     """
 
     __metaclass__ = abc.ABCMeta
@@ -196,6 +198,7 @@ class HomeServer(object):
         self._reactor = reactor
         self.hostname = hostname
         self._building = {}
+        self._listening_services = []
 
         self.clock = Clock(reactor)
         self.distributor = Distributor()
-- 
cgit 1.5.1


From 5d27730a73d01bd3ff7eb42a21f2f024493c5b89 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Mon, 11 Feb 2019 18:03:30 +0000
Subject: Move ClientTLSOptionsFactory init out of refresh_certificates (#4611)

It's nothing to do with refreshing the certificates. No idea why it was here.
---
 changelog.d/4611.misc                  | 1 +
 synapse/app/_base.py                   | 3 ---
 synapse/http/matrixfederationclient.py | 4 ++--
 synapse/server.py                      | 6 +++++-
 tests/http/test_fedclient.py           | 4 +---
 5 files changed, 9 insertions(+), 9 deletions(-)
 create mode 100644 changelog.d/4611.misc

(limited to 'synapse/app/_base.py')

diff --git a/changelog.d/4611.misc b/changelog.d/4611.misc
new file mode 100644
index 0000000000..d2e0a05daa
--- /dev/null
+++ b/changelog.d/4611.misc
@@ -0,0 +1 @@
+Move ClientTLSOptionsFactory init out of refresh_certificates
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index 62c633146f..e1fc1afd5b 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -216,9 +216,6 @@ def refresh_certificate(hs):
     logging.info("Loading certificate from disk...")
     hs.config.read_certificate_from_disk()
     hs.tls_server_context_factory = context_factory.ServerContextFactory(hs.config)
-    hs.tls_client_options_factory = context_factory.ClientTLSOptionsFactory(
-        hs.config
-    )
     logging.info("Certificate loaded.")
 
     if hs._listening_services:
diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 5ee4d528d2..3c24bf3805 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -168,7 +168,7 @@ class MatrixFederationHttpClient(object):
             requests.
     """
 
-    def __init__(self, hs):
+    def __init__(self, hs, tls_client_options_factory):
         self.hs = hs
         self.signing_key = hs.config.signing_key[0]
         self.server_name = hs.hostname
@@ -176,7 +176,7 @@ class MatrixFederationHttpClient(object):
 
         self.agent = MatrixFederationAgent(
             hs.get_reactor(),
-            hs.tls_client_options_factory,
+            tls_client_options_factory,
         )
         self.clock = hs.get_clock()
         self._store = hs.get_datastore()
diff --git a/synapse/server.py b/synapse/server.py
index a2cf8a91cd..8615b67ad4 100644
--- a/synapse/server.py
+++ b/synapse/server.py
@@ -31,6 +31,7 @@ from synapse.api.filtering import Filtering
 from synapse.api.ratelimiting import Ratelimiter
 from synapse.appservice.api import ApplicationServiceApi
 from synapse.appservice.scheduler import ApplicationServiceScheduler
+from synapse.crypto import context_factory
 from synapse.crypto.keyring import Keyring
 from synapse.events.builder import EventBuilderFactory
 from synapse.events.spamcheck import SpamChecker
@@ -367,7 +368,10 @@ class HomeServer(object):
         return PusherPool(self)
 
     def build_http_client(self):
-        return MatrixFederationHttpClient(self)
+        tls_client_options_factory = context_factory.ClientTLSOptionsFactory(
+            self.config
+        )
+        return MatrixFederationHttpClient(self, tls_client_options_factory)
 
     def build_db_pool(self):
         name = self.db_config["name"]
diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py
index 018c77ebcd..b03b37affe 100644
--- a/tests/http/test_fedclient.py
+++ b/tests/http/test_fedclient.py
@@ -43,13 +43,11 @@ def check_logcontext(context):
 
 class FederationClientTests(HomeserverTestCase):
     def make_homeserver(self, reactor, clock):
-
         hs = self.setup_test_homeserver(reactor=reactor, clock=clock)
-        hs.tls_client_options_factory = None
         return hs
 
     def prepare(self, reactor, clock, homeserver):
-        self.cl = MatrixFederationHttpClient(self.hs)
+        self.cl = MatrixFederationHttpClient(self.hs, None)
         self.reactor.lookups["testserv"] = "1.2.3.4"
 
     def test_client_get(self):
-- 
cgit 1.5.1


From 086f6f27d409520e71556cad4707cb2f70476e20 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Mon, 11 Feb 2019 21:00:41 +0000
Subject: 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

(limited to 'synapse/app/_base.py')

diff --git a/changelog.d/4615.misc b/changelog.d/4615.misc
new file mode 100644
index 0000000000..c7266fcfc7
--- /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 e1fc1afd5b..6d72de1daa 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 9fcc79816d..76d2add4fe 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
+
+        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, private_key_path):
-        private_key_pem = self.read_file(private_key_path, "tls_private_key")
+    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)
-- 
cgit 1.5.1


From 9645728619828fda050fa08aaa25628f5db5d775 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Mon, 11 Feb 2019 21:30:59 +0000
Subject: 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

(limited to 'synapse/app/_base.py')

diff --git a/changelog.d/4617.misc b/changelog.d/4617.misc
new file mode 100644
index 0000000000..6d751865c9
--- /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 6d72de1daa..6b3cb61ae9 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 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(
-- 
cgit 1.5.1


From 4fddf8fc77496d9bb3b5fa8835f0e5ba9a5a9926 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Mon, 11 Feb 2019 17:57:58 +0000
Subject: 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

(limited to 'synapse/app/_base.py')

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/4615.misc b/changelog.d/4615.misc
deleted file mode 100644
index c7266fcfc7..0000000000
--- 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 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/changelog.d/4617.misc b/changelog.d/4617.misc
deleted file mode 100644
index 6d751865c9..0000000000
--- 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 6b3cb61ae9..50fd17c0be 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 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 eed9d7c81e..767897c419 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 76d2add4fe..e37a41eff4 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
-- 
cgit 1.5.1


From 32b781bfe20034052d71558c0ed9b0df511a1f77 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Tue, 12 Feb 2019 10:51:31 +0000
Subject: Fix error when loading cert if tls is disabled (#4618)

If TLS is disabled, it should not be an error if no cert is given.

Fixes #4554.
---
 changelog.d/4618.bugfix  |  1 +
 synapse/app/_base.py     |  5 +++--
 synapse/config/tls.py    | 57 +++++++++++++++++++++++++++++++++++-------------
 tests/config/test_tls.py |  2 +-
 4 files changed, 47 insertions(+), 18 deletions(-)
 create mode 100644 changelog.d/4618.bugfix

(limited to 'synapse/app/_base.py')

diff --git a/changelog.d/4618.bugfix b/changelog.d/4618.bugfix
new file mode 100644
index 0000000000..22115a020e
--- /dev/null
+++ b/changelog.d/4618.bugfix
@@ -0,0 +1 @@
+Fix failure to start when not TLS certificate was given even if TLS was disabled.
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index 50fd17c0be..5b0ca312e2 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -213,12 +213,13 @@ 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.
     """
-    hs.config.read_certificate_from_disk()
 
     if not hs.config.has_tls_listener():
-        # nothing else to do here
+        # attempt to reload the certs for the good of the tls_fingerprints
+        hs.config.read_certificate_from_disk(require_cert_and_key=False)
         return
 
+    hs.config.read_certificate_from_disk(require_cert_and_key=True)
     hs.tls_server_context_factory = context_factory.ServerContextFactory(hs.config)
 
     if hs._listening_services:
diff --git a/synapse/config/tls.py b/synapse/config/tls.py
index 86e6eb80db..57f117a14d 100644
--- a/synapse/config/tls.py
+++ b/synapse/config/tls.py
@@ -23,7 +23,7 @@ from unpaddedbase64 import encode_base64
 
 from OpenSSL import crypto
 
-from synapse.config._base import Config
+from synapse.config._base import Config, ConfigError
 
 logger = logging.getLogger(__name__)
 
@@ -45,6 +45,19 @@ class TlsConfig(Config):
 
         self.tls_certificate_file = self.abspath(config.get("tls_certificate_path"))
         self.tls_private_key_file = self.abspath(config.get("tls_private_key_path"))
+
+        if self.has_tls_listener():
+            if not self.tls_certificate_file:
+                raise ConfigError(
+                    "tls_certificate_path must be specified if TLS-enabled listeners are "
+                    "configured."
+                )
+            if not self.tls_private_key_file:
+                raise ConfigError(
+                    "tls_certificate_path must be specified if TLS-enabled listeners are "
+                    "configured."
+                )
+
         self._original_tls_fingerprints = config.get("tls_fingerprints", [])
 
         if self._original_tls_fingerprints is None:
@@ -105,26 +118,40 @@ class TlsConfig(Config):
         days_remaining = (expires_on - now).days
         return days_remaining
 
-    def read_certificate_from_disk(self):
-        """
-        Read the certificates from disk.
+    def read_certificate_from_disk(self, require_cert_and_key):
         """
-        self.tls_certificate = self.read_tls_certificate()
+        Read the certificates and private key from disk.
 
-        if self.has_tls_listener():
+        Args:
+            require_cert_and_key (bool): set to True to throw an error if the certificate
+                and key file are not given
+        """
+        if require_cert_and_key:
             self.tls_private_key = self.read_tls_private_key()
+            self.tls_certificate = self.read_tls_certificate()
+        elif self.tls_certificate_file:
+            # we only need the certificate for the tls_fingerprints. Reload it if we
+            # can, but it's not a fatal error if we can't.
+            try:
+                self.tls_certificate = self.read_tls_certificate()
+            except Exception as e:
+                logger.info(
+                    "Unable to read TLS certificate (%s). Ignoring as no "
+                    "tls listeners enabled.", e,
+                )
 
         self.tls_fingerprints = list(self._original_tls_fingerprints)
 
-        # Check that our own certificate is included in the list of fingerprints
-        # and include it if it is not.
-        x509_certificate_bytes = crypto.dump_certificate(
-            crypto.FILETYPE_ASN1, self.tls_certificate
-        )
-        sha256_fingerprint = encode_base64(sha256(x509_certificate_bytes).digest())
-        sha256_fingerprints = set(f["sha256"] for f in self.tls_fingerprints)
-        if sha256_fingerprint not in sha256_fingerprints:
-            self.tls_fingerprints.append({u"sha256": sha256_fingerprint})
+        if self.tls_certificate:
+            # Check that our own certificate is included in the list of fingerprints
+            # and include it if it is not.
+            x509_certificate_bytes = crypto.dump_certificate(
+                crypto.FILETYPE_ASN1, self.tls_certificate
+            )
+            sha256_fingerprint = encode_base64(sha256(x509_certificate_bytes).digest())
+            sha256_fingerprints = set(f["sha256"] for f in self.tls_fingerprints)
+            if sha256_fingerprint not in sha256_fingerprints:
+                self.tls_fingerprints.append({u"sha256": sha256_fingerprint})
 
     def default_config(self, config_dir_path, server_name, **kwargs):
         base_key_name = os.path.join(config_dir_path, server_name)
diff --git a/tests/config/test_tls.py b/tests/config/test_tls.py
index d8fd18a9cb..c260d3359f 100644
--- a/tests/config/test_tls.py
+++ b/tests/config/test_tls.py
@@ -65,7 +65,7 @@ s4niecZKPBizL6aucT59CsunNmmb5Glq8rlAcU+1ZTZZzGYqVYhF6axB9Qg=
 
         t = TestConfig()
         t.read_config(config)
-        t.read_certificate_from_disk()
+        t.read_certificate_from_disk(require_cert_and_key=False)
 
         warnings = self.flushWarnings()
         self.assertEqual(len(warnings), 1)
-- 
cgit 1.5.1