Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add email.tlsname config option #17849

Merged
merged 4 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17849.feature
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 3 additions & 1 deletion docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 <[email protected]>"
app_name: my_branded_matrix_server
enable_notifs: true
Expand Down
1 change: 1 addition & 0 deletions synapse/config/emailconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
11 changes: 9 additions & 2 deletions synapse/handlers/send_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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(
Expand All @@ -117,10 +122,10 @@ def build_sender_factory(**kwargs: Any) -> ESMTPSenderFactory:
else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not touch the branch for Twisted < v21. Should I try to implement this for old Twisted versions as well? That would basically entail backporting the hostname parameter introduced in Twisted v21 into our _NoTLSESMTPSender class, where that class would also need to close over the tlsname parameter, as it's value isn't provided through the v21 ESTMPSenderFactory. I don't have a system with old Twisted readily available to test that such an implementation.
Or maybe just log an error and refuse to send mail if Twisted < v21 and tlsname != smtphost?

Thanks for bringing this up!

If we're not going to support this option for Twisted < v21, it would be ideal fail early and not start up at all (error when parsing the config). Along with updating the documentation to state the version requirement. But I'm not sure of another spot where we make a similar constraint. Perhaps it's important that we support Twisted < v21 but I don't have that context.

I also don't have a sense for how big or complex the hostname backport would be but seems like it's do-able to you.

I don't have a system with old Twisted readily available to test that such an implementation.

It looks like the trial-olddeps CI job uses the minimum versions from our pyproject.toml which will be Twisted 18.9.0. So as long as we have some tests for this new functionality in tests/handlers/test_send_email.py, we should be covered.

I can run the CI when you want but should be also possible emulate the same thing that the CI job is doing to test it locally as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should now also work with Twisted < v21. I also added a check to the tests that we pass the tlsname to Twisted, and successfully ran that with both old and new Twisted. I did not verify that old Twisted actually does with this parameter what we want it to do, but it is at least documented as such. (With new Twisted I mentioned before I tested it with an actual SMTP server. I did repeat that test for the happy path, a cert with commonname = tlsnamesmtp_host is still being accepted.)

I also was not able to reproduce the sytest failure for commit 685f746 locally ($ podman run --rm -it -v "$PWD":/src:ro -v "$PWD"/logs\:/logs -e POSTGRES=1 -e MULTI_POSTGRES=1 -e WORKERS=1 -e REDIS=1 docker.io/matrixdotorg/sytest-synapse:focal completes with run-tests PASSED). The failures also appear to affect parts I didn't really touch. If you nonetheless think it's related to my changes, I can try looking into it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the diligence @cynhr!

The CI is all green ✅ and the previous failure was probably just a flakey test (unfortunately).

# 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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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,
)
Loading