From 685f74623276770821f7c7a84c78ff6aaa6e3146 Mon Sep 17 00:00:00 2001 From: Cy Nhr Date: Thu, 17 Oct 2024 19:32:24 +0200 Subject: [PATCH 1/3] Add email.tlsname config option The existing `email.smtp_host` config option is used for two distinct purposes: it is resolved into the IP address to connect to, and used to (request via SNI and) validate the server's certificate if TLS is enabled. This new option allows specifying a different name for the second purpose. This is especially helpful, if `email.smtp_host` isn't a global FQDN, but something that resolves only locally (e.g. "localhost" to connect through the loopback interface, or some other internally routed name), that one cannot get a valid certificate for. --- changelog.d/17849.feature | 1 + docs/usage/configuration/config_documentation.md | 4 +++- synapse/config/emailconfig.py | 1 + synapse/handlers/send_email.py | 11 +++++++++-- 4 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 changelog.d/17849.feature diff --git a/changelog.d/17849.feature b/changelog.d/17849.feature new file mode 100644 index 00000000000..4de580f9edb --- /dev/null +++ b/changelog.d/17849.feature @@ -0,0 +1 @@ +Added the `email.tlsname` config option. This allows specifying the domain name used to validate the SMTP server's TLS certificate separately from the `email.smtp_host` to connect to. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 47e3ef12870..20595258419 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -673,8 +673,9 @@ This setting has the following sub-options: TLS via STARTTLS *if the SMTP server supports it*. If this option is set, Synapse will refuse to connect unless the server supports STARTTLS. * `enable_tls`: By default, if the server supports TLS, it will be used, and the server - must present a certificate that is valid for 'smtp_host'. If this option + must present a certificate that is valid for `tlsname`. If this option is set to false, TLS will not be used. +* `tlsname`: The domain name the SMTP server's TLS certificate must be valid for, defaulting to `smtp_host`. * `notif_from`: defines the "From" address to use when sending emails. It must be set if email sending is enabled. The placeholder '%(app)s' will be replaced by the application name, which is normally set in `app_name`, but may be overridden by the @@ -741,6 +742,7 @@ email: force_tls: true require_transport_security: true enable_tls: false + tlsname: mail.server.example.com notif_from: "Your Friendly %(app)s homeserver " app_name: my_branded_matrix_server enable_notifs: true diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index 8033fa2e526..c3a3e05a825 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -110,6 +110,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: raise ConfigError( "email.require_transport_security requires email.enable_tls to be true" ) + self.email_tlsname = email_config.get("tlsname", None) if "app_name" in email_config: self.email_app_name = email_config["app_name"] diff --git a/synapse/handlers/send_email.py b/synapse/handlers/send_email.py index 70cdb0721c9..bfb882daa78 100644 --- a/synapse/handlers/send_email.py +++ b/synapse/handlers/send_email.py @@ -71,6 +71,7 @@ async def _sendmail( require_tls: bool = False, enable_tls: bool = True, force_tls: bool = False, + tlsname: Optional[str] = None, ) -> None: """A simple wrapper around ESMTPSenderFactory, to allow substitution in tests @@ -88,9 +89,13 @@ async def _sendmail( enable_tls: True to enable STARTTLS. If this is False and require_tls is True, the request will fail. force_tls: True to enable Implicit TLS. + tlsname: the domain name expected as the TLS certificate's commonname, + defaults to smtphost. """ msg = BytesIO(msg_bytes) d: "Deferred[object]" = Deferred() + if tlsname is None: + tlsname = smtphost def build_sender_factory(**kwargs: Any) -> ESMTPSenderFactory: return ESMTPSenderFactory( @@ -117,10 +122,10 @@ def build_sender_factory(**kwargs: Any) -> ESMTPSenderFactory: else: # for twisted 21.2 and later, there is a 'hostname' parameter which we should # set to enable TLS. - factory = build_sender_factory(hostname=smtphost if enable_tls else None) + factory = build_sender_factory(hostname=tlsname if enable_tls else None) if force_tls: - factory = TLSMemoryBIOFactory(optionsForClientTLS(smtphost), True, factory) + factory = TLSMemoryBIOFactory(optionsForClientTLS(tlsname), True, factory) endpoint = HostnameEndpoint( reactor, smtphost, smtpport, timeout=30, bindAddress=None @@ -148,6 +153,7 @@ def __init__(self, hs: "HomeServer"): self._require_transport_security = hs.config.email.require_transport_security self._enable_tls = hs.config.email.enable_smtp_tls self._force_tls = hs.config.email.force_tls + self._tlsname = hs.config.email.email_tlsname self._sendmail = _sendmail @@ -227,4 +233,5 @@ async def send_email( require_tls=self._require_transport_security, enable_tls=self._enable_tls, force_tls=self._force_tls, + tlsname=self._tlsname, ) From 856843676c44551ac149c69f1a0041f46e285288 Mon Sep 17 00:00:00 2001 From: Cy Nhr Date: Mon, 16 Dec 2024 23:59:36 +0100 Subject: [PATCH 2/3] Support email.tlsname with old Twisted versions Old twisted versions' ESMTPSenderFactory doesn't support the `hostname` parameter used to implement the email.tlsname configuration option (and also the email.enable_tls option). This backports that parameter by extending ESMTPSenderFactory when twisted is old. Were shadowing the attribute `ESMTPSenderFactory.protocol` with the method `_BackportESMTPSenderFactory.protocol` here, that is on purpose. That attribute usually contains the `ESMTPSender` class. It is used by `ESMTPSenderFactory.buildProtocol` as a Callable to instantiate an `ESMTPSender`. By providing this Callable as a method, the previously set `_BackportESMTPSenderFactory.__hostname` can be provided to the newly instantiated `_BackportESMTPSender`. Note the `# type: ignore` annotation required to pass the mypy type checker. Mypy assigns the type `Type[ESMTPSender]` to `ESMTPSenderFactory.protocol`, which is valid (twisted does assign a `Type[ESMTPSender]`), but AFAICT it is only ever used to instantiate values of that type, i.e. as a `Callable[[...], ESMTPSender]`, so overriding it with a method with that signature should be fine. It is a public attribute though, so technically it could be accessed somewhere else, actually requiring a `Type[ESMTPSender]`. But we do need to get the `hostname` parameter to our `_BackportESMTPSender` somehow, and this seems neater compared to the two alternatives I considered: * Override `ESMTPSenderFactory.buildProtocol` instead of `ESMTPSenderFactory.protocol`. That would require copying the entire body of `ESMTPSenderFactory.buildProtocol` into `_BackportESMTPSenderFactory.buildProtocol`, most of which isn't related to the change we're trying to implement. That just seems unnecessarily brittle (e.g. maybe different old twisted versions could have different versions `buildProtocol` (I didn't check), picking one of them could lead to an inconsistent object). * Set `self.protocol` in `_BackportESMTPSenderFactory.__init__` to a class defined within `_BackportESMTPSenderFactory.__init__`, such that it can close over the `hostname` parameter---i.e. each instantiation of the `_BackportESMTPSenderFactory` would define a new class extending `ESMTPSender` specific to that instantiation's `hostname` parameter. That's a nice (functional-style) solution, but I think the solution implemented here might feel more natural to the average python programmer. This effectively retrofits modern twisted's [1] `hostname` parameter to old twisted's [2] classes. [1]: https://github.com/twisted/twisted/blob/twisted-24.11.0/src/twisted/mail/smtp.py#L1992 [2]: https://github.com/twisted/twisted/blob/twisted-21.2.0/src/twisted/mail/smtp.py#L2005 --- synapse/handlers/send_email.py | 84 +++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 32 deletions(-) diff --git a/synapse/handlers/send_email.py b/synapse/handlers/send_email.py index bfb882daa78..8cf8d2badab 100644 --- a/synapse/handlers/send_email.py +++ b/synapse/handlers/send_email.py @@ -47,15 +47,45 @@ _is_old_twisted = parse_version(twisted.__version__) < parse_version("21") -class _NoTLSESMTPSender(ESMTPSender): - """Extend ESMTPSender to disable TLS +class _BackportESMTPSender(ESMTPSender): + """Extend old versions of ESMTPSender to configure TLS. - Unfortunately, before Twisted 21.2, ESMTPSender doesn't give an easy way to disable - TLS, so we override its internal method which it uses to generate a context factory. + Unfortunately, before Twisted 21.2, ESMTPSender doesn't give an easy way to + disable TLS, or to configure the hostname used for TLS certificate validation. + This backports the `hostname` parameter for that functionality. """ + __hostname: Optional[str] + + def __init__(self, *args: Any, **kwargs: Any) -> None: + """""" + self.__hostname = kwargs.pop("hostname", None) + super().__init__(*args, **kwargs) + def _getContextFactory(self) -> Optional[IOpenSSLContextFactory]: - return None + if self.context is not None: + return self.context + elif self.__hostname is None: + return None # disable TLS if hostname is None + return optionsForClientTLS(self.__hostname) + + +class _BackportESMTPSenderFactory(ESMTPSenderFactory): + """An ESMTPSenderFactory for _BackportESMTPSender. + + This backports the `hostname` parameter, to disable or configure TLS. + """ + + __hostname: Optional[str] + + def __init__(self, *args: Any, **kwargs: Any) -> None: + self.__hostname = kwargs.pop("hostname", None) + super().__init__(*args, **kwargs) + + def protocol(self, *args: Any, **kwargs: Any) -> ESMTPSender: # type: ignore + # this overrides ESMTPSenderFactory's `protocol` attribute, with a Callable + # instantiating our _BackportESMTPSender, providing the hostname parameter + return _BackportESMTPSender(*args, **kwargs, hostname=self.__hostname) async def _sendmail( @@ -94,35 +124,25 @@ async def _sendmail( """ msg = BytesIO(msg_bytes) d: "Deferred[object]" = Deferred() - if tlsname is None: + if not enable_tls: + tlsname = None + elif tlsname is None: tlsname = smtphost - def build_sender_factory(**kwargs: Any) -> ESMTPSenderFactory: - return ESMTPSenderFactory( - username, - password, - from_addr, - to_addr, - msg, - d, - heloFallback=True, - requireAuthentication=require_auth, - requireTransportSecurity=require_tls, - **kwargs, - ) - - factory: IProtocolFactory - if _is_old_twisted: - # before twisted 21.2, we have to override the ESMTPSender protocol to disable - # TLS - factory = build_sender_factory() - - if not enable_tls: - factory.protocol = _NoTLSESMTPSender - else: - # for twisted 21.2 and later, there is a 'hostname' parameter which we should - # set to enable TLS. - factory = build_sender_factory(hostname=tlsname if enable_tls else None) + factory: IProtocolFactory = ( + _BackportESMTPSenderFactory if _is_old_twisted else ESMTPSenderFactory + )( + username, + password, + from_addr, + to_addr, + msg, + d, + heloFallback=True, + requireAuthentication=require_auth, + requireTransportSecurity=require_tls, + hostname=tlsname, + ) if force_tls: factory = TLSMemoryBIOFactory(optionsForClientTLS(tlsname), True, factory) From a21991159eb429636f777fae2a724e84a3fe9efc Mon Sep 17 00:00:00 2001 From: Cy Nhr Date: Tue, 17 Dec 2024 19:11:15 +0100 Subject: [PATCH 3/3] Add test for email.tlsname configuration option Ensure the email.tlsname parameter ends up in the `ClientTLSOptions._hostname` attribute, which is documented as "The hostname to verify". --- tests/handlers/test_send_email.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_send_email.py b/tests/handlers/test_send_email.py index cedcea27d93..5f7839c82c1 100644 --- a/tests/handlers/test_send_email.py +++ b/tests/handlers/test_send_email.py @@ -163,6 +163,7 @@ def test_send_email(self) -> None: "email": { "notif_from": "noreply@test", "force_tls": True, + "tlsname": "example.org", }, } ) @@ -186,10 +187,9 @@ def test_send_email_force_tls(self) -> None: self.assertEqual(host, self.reactor.lookups["localhost"]) self.assertEqual(port, 465) # We need to make sure that TLS is happenning - self.assertIsInstance( - client_factory._wrappedFactory._testingContextFactory, - ClientTLSOptions, - ) + context_factory = client_factory._wrappedFactory._testingContextFactory + self.assertIsInstance(context_factory, ClientTLSOptions) + self.assertEqual(context_factory._hostname, "example.org") # tlsname # And since we use endpoints, they go through reactor.connectTCP # which works differently to connectSSL on the testing reactor