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:
codecov-io: # [Codecov](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=h1) Report
> Merging [#4618](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=desc) into [develop](https://codecov.io/gh/matrix-org/synapse/commit/46b8a79b3a418ad6795e903588d1f8e12f547e2c?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `40.9%`.

[![Impacted file tree graph](https://codecov.io/gh/matrix-org/synapse/pull/4618/graphs/tree.svg?width=650&token=or46CYFl0Y&height=150&src=pr)](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           develop    #4618      +/-   ##
===========================================
- Coverage     75.3%   75.25%   -0.05%     
===========================================
  Files          338      338              
  Lines        34552    34564      +12     
  Branches      5647     5652       +5     
===========================================
- Hits         26018    26010       -8     
- Misses        6946     6964      +18     
- Partials      1588     1590       +2
```


| [Impacted Files](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [synapse/config/tls.py](https://codecov.io/gh/matrix-org/synapse/pull/4618/diff?src=pr&el=tree#diff-c3luYXBzZS9jb25maWcvdGxzLnB5) | `59.55% <40%> (-8.4%)` | :arrow_down: |
| [synapse/app/\_base.py](https://codecov.io/gh/matrix-org/synapse/pull/4618/diff?src=pr&el=tree#diff-c3luYXBzZS9hcHAvX2Jhc2UucHk=) | `54.9% <50%> (-0.55%)` | :arrow_down: |
| [synapse/storage/\_\_init\_\_.py](https://codecov.io/gh/matrix-org/synapse/pull/4618/diff?src=pr&el=tree#diff-c3luYXBzZS9zdG9yYWdlL19faW5pdF9fLnB5) | `70.88% <0%> (-5.7%)` | :arrow_down: |
| [synapse/app/homeserver.py](https://codecov.io/gh/matrix-org/synapse/pull/4618/diff?src=pr&el=tree#diff-c3luYXBzZS9hcHAvaG9tZXNlcnZlci5weQ==) | `54.62% <0%> (-0.31%)` | :arrow_down: |
| [synapse/config/\_base.py](https://codecov.io/gh/matrix-org/synapse/pull/4618/diff?src=pr&el=tree#diff-c3luYXBzZS9jb25maWcvX2Jhc2UucHk=) | `75.69% <0%> (+1.1%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=footer). Last update [46b8a79...863144f](https://codecov.io/gh/matrix-org/synapse/pull/4618?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/changelog.d/4618.bugfix b/changelog.d/4618.bugfix
new file mode 100644
index 000000000..22115a020
--- /dev/null
+++ b/changelog.d/4618.bugfix
@@ -0,0 +1,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 50fd17c0b..5b0ca312e 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 86e6eb80d..57f117a14 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)


Test patch:
diff --git a/tests/config/test_tls.py b/tests/config/test_tls.py
index d8fd18a9c..c260d3359 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)