From 3f7cbcef3611dc22460dd4dc2919ff933f79433a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 27 Aug 2021 12:51:52 +0100 Subject: [PATCH 1/3] Fix incompatibility with Twisted < 21. Turns out that the functionality added in #10546 to skip TLS was incompatible with older Twisted versions, so we need to be a bit more inventive. Also, add a test to (hopefully) not break this in future. Sadly, testing TLS is really hard. --- changelog.d/10713.bugfix | 1 + mypy.ini | 1 + synapse/handlers/send_email.py | 37 ++++++++-- tests/handlers/test_send_email.py | 112 ++++++++++++++++++++++++++++++ tests/server.py | 15 +++- 5 files changed, 157 insertions(+), 9 deletions(-) create mode 100644 changelog.d/10713.bugfix create mode 100644 tests/handlers/test_send_email.py diff --git a/changelog.d/10713.bugfix b/changelog.d/10713.bugfix new file mode 100644 index 000000000000..e8caf3d23aaa --- /dev/null +++ b/changelog.d/10713.bugfix @@ -0,0 +1 @@ +Fix a regression introduced in Synapse 1.41 which broke email transmission on Systems using older versions of the Twisted library. diff --git a/mypy.ini b/mypy.ini index e1b9405daa85..349efe37bbc4 100644 --- a/mypy.ini +++ b/mypy.ini @@ -87,6 +87,7 @@ files = tests/test_utils, tests/handlers/test_password_providers.py, tests/handlers/test_room_summary.py, + tests/handlers/test_send_email.py, tests/rest/client/v1/test_login.py, tests/rest/client/v2_alpha/test_auth.py, tests/util/test_itertools.py, diff --git a/synapse/handlers/send_email.py b/synapse/handlers/send_email.py index dda9659c11c2..11ff18418c62 100644 --- a/synapse/handlers/send_email.py +++ b/synapse/handlers/send_email.py @@ -13,6 +13,7 @@ # limitations under the License. import email.utils +import inspect import logging from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText @@ -20,8 +21,8 @@ from typing import TYPE_CHECKING, Optional from twisted.internet.defer import Deferred -from twisted.internet.interfaces import IReactorTCP -from twisted.mail.smtp import ESMTPSenderFactory +from twisted.internet.interfaces import IOpenSSLContextFactory, IReactorTCP +from twisted.mail.smtp import ESMTPSender, ESMTPSenderFactory from synapse.logging.context import make_deferred_yieldable @@ -31,6 +32,21 @@ logger = logging.getLogger(__name__) +class _NoTLSESMTPSender(ESMTPSender): + """Extend ESMTPSender to disable TLS + + Unfortunatlely ESMTPSender doesn't give an easy way to disable TLS, so we override + its internal method which it uses to generate a context factory. + + As of Twisted 21.2, one alternative is to set the `hostname` param of + ESMTPSenderFactory to `None`, so if in future we drop support for earlier versions, + that is a possibility. + """ + + def _getContextFactory(self) -> Optional[IOpenSSLContextFactory]: + return None + + async def _sendmail( reactor: IReactorTCP, smtphost: str, @@ -42,7 +58,7 @@ async def _sendmail( password: Optional[bytes] = None, require_auth: bool = False, require_tls: bool = False, - tls_hostname: Optional[str] = None, + enable_tls: bool = True, ) -> None: """A simple wrapper around ESMTPSenderFactory, to allow substitution in tests @@ -57,12 +73,19 @@ async def _sendmail( password: password to give when authenticating require_auth: if auth is not offered, fail the request require_tls: if TLS is not offered, fail the reqest - tls_hostname: TLS hostname to check for. None to disable TLS. + enable_tls: True to enable TLS. If this is False and require_tls is True, + the request will fail. """ msg = BytesIO(msg_bytes) d: "Deferred[object]" = Deferred() + # Twisted 21.2 introduced a 'hostname' parameter to ESMTPSenderFactory, which we + # need to set to enable TLS. + kwargs = {} + sig = inspect.signature(ESMTPSenderFactory) + if "hostname" in sig.parameters: + kwargs["hostname"] = smtphost factory = ESMTPSenderFactory( username, password, @@ -73,8 +96,10 @@ async def _sendmail( heloFallback=True, requireAuthentication=require_auth, requireTransportSecurity=require_tls, - hostname=tls_hostname, + **kwargs, ) + if not enable_tls: + factory.protocol = _NoTLSESMTPSender # the IReactorTCP interface claims host has to be a bytes, which seems to be wrong reactor.connectTCP(smtphost, smtpport, factory, timeout=30, bindAddress=None) # type: ignore[arg-type] @@ -154,5 +179,5 @@ async def send_email( password=self._smtp_pass, require_auth=self._smtp_user is not None, require_tls=self._require_transport_security, - tls_hostname=self._smtp_host if self._enable_tls else None, + enable_tls=self._enable_tls, ) diff --git a/tests/handlers/test_send_email.py b/tests/handlers/test_send_email.py new file mode 100644 index 000000000000..752762da27e5 --- /dev/null +++ b/tests/handlers/test_send_email.py @@ -0,0 +1,112 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# 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. + + +from typing import List, Tuple + +from zope.interface import implementer + +from twisted.internet import defer +from twisted.internet.address import IPv4Address +from twisted.internet.defer import ensureDeferred +from twisted.mail import interfaces, smtp + +from tests.server import FakeTransport +from tests.unittest import HomeserverTestCase + + +@implementer(interfaces.IMessageDelivery) +class _DummyMessageDelivery: + def __init__(self): + # (recipient, message) tuples + self.messages: List[Tuple[smtp.Address, bytes]] = [] + + def receivedHeader(self, helo, origin, recipients): + return None + + def validateFrom(self, helo, origin): + return origin + + def record_message(self, recipient: smtp.Address, message: bytes): + self.messages.append((recipient, message)) + + def validateTo(self, user: smtp.User): + return lambda: _DummyMessage(self, user) + + +@implementer(interfaces.IMessageSMTP) +class _DummyMessage: + """IMessageSMTP implementation which saves the message delivered to it + to the _DummyMessageDelivery object. + """ + + def __init__(self, delivery: _DummyMessageDelivery, user: smtp.User): + self._delivery = delivery + self._user = user + self._buffer: List[bytes] = [] + + def lineReceived(self, line): + self._buffer.append(line) + + def eomReceived(self): + message = b"\n".join(self._buffer) + b"\n" + self._delivery.record_message(self._user.dest, message) + return defer.succeed(b"saved") + + def connectionLost(self): + pass + + +class SendEmailHandlerTestCase(HomeserverTestCase): + def test_send_email(self): + """Happy-path test that we can send email to a non-TLS server.""" + h = self.hs.get_send_email_handler() + d = ensureDeferred( + h.send_email( + "foo@bar.com", "test subject", "Tests", "HTML content", "Text content" + ) + ) + # there should be an attempt to connect to localhost:25 + self.assertEqual(len(self.reactor.tcpClients), 1) + (host, port, client_factory, _timeout, _bindAddress) = self.reactor.tcpClients[ + 0 + ] + self.assertEqual(host, "localhost") + self.assertEqual(port, 25) + + # wire it up to an SMTP server + message_delivery = _DummyMessageDelivery() + server_protocol = smtp.ESMTP() + server_protocol.delivery = message_delivery + # make sure that the server uses the test reactor to set timeouts + server_protocol.callLater = self.reactor.callLater + + client_protocol = client_factory.buildProtocol(None) + client_protocol.makeConnection(FakeTransport(server_protocol, self.reactor)) + server_protocol.makeConnection( + FakeTransport( + client_protocol, + self.reactor, + peer_address=IPv4Address("TCP", "127.0.0.1", 1234), + ) + ) + + # the message should now get delivered + self.get_success(d, by=0.1) + + # check it arrived + self.assertEqual(len(message_delivery.messages), 1) + user, msg = message_delivery.messages.pop() + self.assertEqual(str(user), "foo@bar.com") + self.assertIn(b"Subject: test subject", msg) diff --git a/tests/server.py b/tests/server.py index 6fddd3b30558..b861c7b866f8 100644 --- a/tests/server.py +++ b/tests/server.py @@ -10,9 +10,10 @@ from twisted.internet import address, threads, udp from twisted.internet._resolver import SimpleResolverComplexifier -from twisted.internet.defer import Deferred, fail, succeed +from twisted.internet.defer import Deferred, fail, maybeDeferred, succeed from twisted.internet.error import DNSLookupError from twisted.internet.interfaces import ( + IAddress, IHostnameResolver, IProtocol, IPullProducer, @@ -511,6 +512,9 @@ class FakeTransport: will get called back for connectionLost() notifications etc. """ + _peer_address: Optional[IAddress] = attr.ib(default=None) + """The value to be returend by getPeer""" + disconnecting = False disconnected = False connected = True @@ -519,7 +523,7 @@ class FakeTransport: autoflush = attr.ib(default=True) def getPeer(self): - return None + return self._peer_address def getHost(self): return None @@ -572,7 +576,12 @@ def registerProducer(self, producer, streaming): self.producerStreaming = streaming def _produce(): - d = self.producer.resumeProducing() + if not self.producer: + # we've been unregistered + return + # some implementations of IProducer (for example, FileSender) + # don't return a deferred. + d = maybeDeferred(self.producer.resumeProducing) d.addCallback(lambda x: self._reactor.callLater(0.1, _produce)) if not streaming: From b29e3f545c5c097781dd87f18ef377342c20ff75 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 27 Aug 2021 13:28:34 +0100 Subject: [PATCH 2/3] pacify mypy --- tests/handlers/test_send_email.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_send_email.py b/tests/handlers/test_send_email.py index 752762da27e5..6f77b1237c97 100644 --- a/tests/handlers/test_send_email.py +++ b/tests/handlers/test_send_email.py @@ -90,7 +90,7 @@ def test_send_email(self): server_protocol = smtp.ESMTP() server_protocol.delivery = message_delivery # make sure that the server uses the test reactor to set timeouts - server_protocol.callLater = self.reactor.callLater + server_protocol.callLater = self.reactor.callLater # type: ignore[assignment] client_protocol = client_factory.buildProtocol(None) client_protocol.makeConnection(FakeTransport(server_protocol, self.reactor)) From 14badc080d19a413a3615db5db086e68873daae0 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 27 Aug 2021 15:00:55 +0100 Subject: [PATCH 3/3] Replace introspection with a check on `twisted.__version__`. --- synapse/handlers/send_email.py | 60 ++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/synapse/handlers/send_email.py b/synapse/handlers/send_email.py index 11ff18418c62..a31fe3e3c7ef 100644 --- a/synapse/handlers/send_email.py +++ b/synapse/handlers/send_email.py @@ -13,13 +13,15 @@ # limitations under the License. import email.utils -import inspect import logging from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from io import BytesIO from typing import TYPE_CHECKING, Optional +from pkg_resources import parse_version + +import twisted from twisted.internet.defer import Deferred from twisted.internet.interfaces import IOpenSSLContextFactory, IReactorTCP from twisted.mail.smtp import ESMTPSender, ESMTPSenderFactory @@ -31,16 +33,14 @@ logger = logging.getLogger(__name__) +_is_old_twisted = parse_version(twisted.__version__) < parse_version("21") + class _NoTLSESMTPSender(ESMTPSender): """Extend ESMTPSender to disable TLS - Unfortunatlely ESMTPSender doesn't give an easy way to disable TLS, so we override - its internal method which it uses to generate a context factory. - - As of Twisted 21.2, one alternative is to set the `hostname` param of - ESMTPSenderFactory to `None`, so if in future we drop support for earlier versions, - that is a possibility. + 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. """ def _getContextFactory(self) -> Optional[IOpenSSLContextFactory]: @@ -77,29 +77,33 @@ async def _sendmail( the request will fail. """ msg = BytesIO(msg_bytes) - d: "Deferred[object]" = Deferred() - # Twisted 21.2 introduced a 'hostname' parameter to ESMTPSenderFactory, which we - # need to set to enable TLS. - kwargs = {} - sig = inspect.signature(ESMTPSenderFactory) - if "hostname" in sig.parameters: - kwargs["hostname"] = smtphost - factory = ESMTPSenderFactory( - username, - password, - from_addr, - to_addr, - msg, - d, - heloFallback=True, - requireAuthentication=require_auth, - requireTransportSecurity=require_tls, - **kwargs, - ) - if not enable_tls: - factory.protocol = _NoTLSESMTPSender + def build_sender_factory(**kwargs) -> ESMTPSenderFactory: + return ESMTPSenderFactory( + username, + password, + from_addr, + to_addr, + msg, + d, + heloFallback=True, + requireAuthentication=require_auth, + requireTransportSecurity=require_tls, + **kwargs, + ) + + 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=smtphost if enable_tls else None) # the IReactorTCP interface claims host has to be a bytes, which seems to be wrong reactor.connectTCP(smtphost, smtpport, factory, timeout=30, bindAddress=None) # type: ignore[arg-type]