ACME config cleanups (#4525)

* Handle listening for ACME requests on IPv6 addresses

the weird url-but-not-actually-a-url-string doesn't handle IPv6 addresses
without extra quoting. Building a string which you are about to parse again
seems like a weird choice. Let's just use listenTCP, which is consistent with
what we do elsewhere.

* Clean up the default ACME config

make it look a bit more consistent with everything else, and tweak the defaults
to listen on port 80.

* newsfile

4 files changed, 114 insertions, 60 deletions

diff --git a/synapse/app/__init__.py b/synapse/app/__init__.py
index b45adafdd3..f56f5fcc13 100644
--- a/synapse/app/__init__.py
+++ b/synapse/app/__init__.py
@@ -12,15 +12,38 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
+import logging
 import sys
 
 from synapse import python_dependencies  # noqa: E402
 
 sys.dont_write_bytecode = True
 
+logger = logging.getLogger(__name__)
+
 try:
     python_dependencies.check_requirements()
 except python_dependencies.DependencyException as e:
     sys.stderr.writelines(e.message)
     sys.exit(1)
+
+
+def check_bind_error(e, address, bind_addresses):
+    """
+    This method checks an exception occurred while binding on 0.0.0.0.
+    If :: is specified in the bind addresses a warning is shown.
+    The exception is still raised otherwise.
+
+    Binding on both 0.0.0.0 and :: causes an exception on Linux and macOS
+    because :: binds on both IPv4 and IPv6 (as per RFC 3493).
+    When binding on 0.0.0.0 after :: this can safely be ignored.
+
+    Args:
+        e (Exception): Exception that was caught.
+        address (str): Address on which binding was attempted.
+        bind_addresses (list): Addresses on which the service listens.
+    """
+    if address == '0.0.0.0' and '::' in bind_addresses:
+        logger.warn('Failed to listen on 0.0.0.0, continuing because listening on [::]')
+    else:
+        raise e
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index 3840c663ab..5b97a54d45 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -22,6 +22,7 @@ from daemonize import Daemonize
 
 from twisted.internet import error, reactor
 
+from synapse.app import check_bind_error
 from synapse.util import PreserveLoggingContext
 from synapse.util.rlimit import change_resource_limit
 
@@ -188,24 +189,3 @@ def listen_ssl(
 
     logger.info("Synapse now listening on port %d (TLS)", port)
     return r
-
-
-def check_bind_error(e, address, bind_addresses):
-    """
-    This method checks an exception occurred while binding on 0.0.0.0.
-    If :: is specified in the bind addresses a warning is shown.
-    The exception is still raised otherwise.
-
-    Binding on both 0.0.0.0 and :: causes an exception on Linux and macOS
-    because :: binds on both IPv4 and IPv6 (as per RFC 3493).
-    When binding on 0.0.0.0 after :: this can safely be ignored.
-
-    Args:
-        e (Exception): Exception that was caught.
-        address (str): Address on which binding was attempted.
-        bind_addresses (list): Addresses on which the service listens.
-    """
-    if address == '0.0.0.0' and '::' in bind_addresses:
-        logger.warn('Failed to listen on 0.0.0.0, continuing because listening on [::]')
-    else:
-        raise e
diff --git a/synapse/config/tls.py b/synapse/config/tls.py
index 734f612db7..5f63676d9c 100644
--- a/synapse/config/tls.py
+++ b/synapse/config/tls.py
@@ -31,13 +31,16 @@ logger = logging.getLogger()
 class TlsConfig(Config):
     def read_config(self, config):
 
-        acme_config = config.get("acme", {})
+        acme_config = config.get("acme", None)
+        if acme_config is None:
+            acme_config = {}
+
         self.acme_enabled = acme_config.get("enabled", False)
         self.acme_url = acme_config.get(
             "url", "https://acme-v01.api.letsencrypt.org/directory"
         )
-        self.acme_port = acme_config.get("port", 8449)
-        self.acme_bind_addresses = acme_config.get("bind_addresses", ["127.0.0.1"])
+        self.acme_port = acme_config.get("port", 80)
+        self.acme_bind_addresses = acme_config.get("bind_addresses", ['::', '0.0.0.0'])
         self.acme_reprovision_threshold = acme_config.get("reprovision_threshold", 30)
 
         self.tls_certificate_file = self.abspath(config.get("tls_certificate_path"))
@@ -126,21 +129,80 @@ class TlsConfig(Config):
         tls_certificate_path = base_key_name + ".tls.crt"
         tls_private_key_path = base_key_name + ".tls.key"
 
+        # this is to avoid the max line length. Sorrynotsorry
+        proxypassline = (
+            'ProxyPass /.well-known/acme-challenge '
+            'http://localhost:8009/.well-known/acme-challenge'
+        )
+
         return (
             """\
-        # PEM encoded X509 certificate for TLS.
-        # This certificate, as of Synapse 1.0, will need to be a valid
-        # and verifiable certificate, with a root that is available in
-        # the root store of other servers you wish to federate to. Any
-        # required intermediary certificates can be appended after the
-        # primary certificate in hierarchical order.
+        # 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.
+        #
+        # See 'ACME support' below to enable auto-provisioning this certificate via
+        # Let's Encrypt.
+        #
         tls_certificate_path: "%(tls_certificate_path)s"
 
-        # PEM encoded private key for TLS
+        # PEM-encoded private key for TLS
         tls_private_key_path: "%(tls_private_key_path)s"
 
-        # Don't bind to the https port
-        no_tls: False
+        # ACME support: This will configure Synapse to request a valid TLS certificate
+        # for your configured `server_name` via Let's Encrypt.
+        #
+        # Note that provisioning a certificate in this way requires port 80 to be
+        # routed to Synapse so that it can complete the http-01 ACME challenge.
+        # By default, if you enable ACME support, Synapse will attempt to listen on
+        # port 80 for incoming http-01 challenges - however, this will likely fail
+        # with 'Permission denied' or a similar error.
+        #
+        # There are a couple of potential solutions to this:
+        #
+        #  * If you already have an Apache, Nginx, or similar listening on port 80,
+        #    you can configure Synapse to use an alternate port, and have your web
+        #    server forward the requests. For example, assuming you set 'port: 8009'
+        #    below, on Apache, you would write:
+        #
+        #    %(proxypassline)s
+        #
+        #  * Alternatively, you can use something like `authbind` to give Synapse
+        #    permission to listen on port 80.
+        #
+        acme:
+            # ACME support is disabled by default. Uncomment the following line
+            # to enable it.
+            #
+            # enabled: true
+
+            # Endpoint to use to request certificates. If you only want to test,
+            # use Let's Encrypt's staging url:
+            #     https://acme-staging.api.letsencrypt.org/directory
+            #
+            # url: https://acme-v01.api.letsencrypt.org/directory
+
+            # Port number to listen on for the HTTP-01 challenge. Change this if
+            # you are forwarding connections through Apache/Nginx/etc.
+            #
+            # port: 80
+
+            # Local addresses to listen on for incoming connections.
+            # Again, you may want to change this if you are forwarding connections
+            # through Apache/Nginx/etc.
+            #
+            # bind_addresses: ['::', '0.0.0.0']
+
+            # How many days remaining on a certificate before it is renewed.
+            #
+            # 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 False will
+        # do so (and avoid the need to give synapse a TLS private key).
+        #
+        # no_tls: False
 
         # List of allowed TLS fingerprints for this server to publish along
         # with the signing keys for this server. Other matrix servers that
@@ -170,20 +232,6 @@ class TlsConfig(Config):
         tls_fingerprints: []
         # tls_fingerprints: [{"sha256": "<base64_encoded_sha256_fingerprint>"}]
 
-        ## Support for ACME certificate auto-provisioning.
-        # acme:
-        #    enabled: false
-        ##   ACME path.
-        ##   If you only want to test, use the staging url:
-        ##   https://acme-staging.api.letsencrypt.org/directory
-        #    url: 'https://acme-v01.api.letsencrypt.org/directory'
-        ##   Port number (to listen for the HTTP-01 challenge).
-        ##   Using port 80 requires utilising something like authbind, or proxying to it.
-        #    port: 8449
-        ##   Hosts to bind to.
-        #    bind_addresses: ['127.0.0.1']
-        ##   How many days remaining on a certificate before it is renewed.
-        #    reprovision_threshold: 30
         """
             % locals()
         )
diff --git a/synapse/handlers/acme.py b/synapse/handlers/acme.py
index 73ea7ed018..dd0b217965 100644
--- a/synapse/handlers/acme.py
+++ b/synapse/handlers/acme.py
@@ -18,13 +18,16 @@ import logging
 import attr
 from zope.interface import implementer
 
+import twisted
+import twisted.internet.error
 from twisted.internet import defer
-from twisted.internet.endpoints import serverFromString
 from twisted.python.filepath import FilePath
 from twisted.python.url import URL
 from twisted.web import server, static
 from twisted.web.resource import Resource
 
+from synapse.app import check_bind_error
+
 logger = logging.getLogger(__name__)
 
 try:
@@ -96,16 +99,19 @@ class AcmeHandler(object):
 
         srv = server.Site(responder_resource)
 
-        listeners = []
-
-        for host in self.hs.config.acme_bind_addresses:
+        bind_addresses = self.hs.config.acme_bind_addresses
+        for host in bind_addresses:
             logger.info(
-                "Listening for ACME requests on %s:%s", host, self.hs.config.acme_port
-            )
-            endpoint = serverFromString(
-                self.reactor, "tcp:%s:interface=%s" % (self.hs.config.acme_port, host)
+                "Listening for ACME requests on %s:%i", host, self.hs.config.acme_port,
             )
-            listeners.append(endpoint.listen(srv))
+            try:
+                self.reactor.listenTCP(
+                    self.hs.config.acme_port,
+                    srv,
+                    interface=host,
+                )
+            except twisted.internet.error.CannotListenError as e:
+                check_bind_error(e, host, bind_addresses)
 
         # Make sure we are registered to the ACME server. There's no public API
         # for this, it is usually triggered by startService, but since we don't
@@ -114,9 +120,6 @@ class AcmeHandler(object):
         self._issuer._registered = False
         yield self._issuer._ensure_registered()
 
-        # Return a Deferred that will fire when all the servers have started up.
-        yield defer.DeferredList(listeners, fireOnOneErrback=True, consumeErrors=True)
-
     @defer.inlineCallbacks
     def provision_certificate(self):