Add email.tlsname config option #17849
Conversation
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.
cynhr
force-pushed
the
cynhr-email-tlsname
branch
from
9cc3545 to
685f746
Compare
I did test it in one configuration ( The 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 |
synapse/handlers/send_email.py
Outdated
| @@ -117,10 +122,10 @@ def build_sender_factory(**kwargs: Any) -> ESMTPSenderFactory: | |||
| else: | |||
There was a problem hiding this comment.
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
hostnameparameter introduced in Twisted v21 into our_NoTLSESMTPSenderclass, where that class would also need to close over thetlsnameparameter, as it's value isn't provided through the v21ESTMPSenderFactory. 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 andtlsname != 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.
There was a problem hiding this comment.
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 = tlsname ≠ smtp_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.
There was a problem hiding this comment.
Thanks for the diligence @cynhr!
The CI is all green ✅ and the previous failure was probably just a flakey test (unfortunately).
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
cynhr
force-pushed
the
cynhr-email-tlsname
branch
from
b8c3ad7 to
beeddf2
Compare
Ensure the email.tlsname parameter ends up in the `ClientTLSOptions._hostname` attribute, which is documented as "The hostname to verify".
The existing
email.smtp_hostconfig 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_hostisn'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.Alternatives would of course be to specify a global FQDN as
email.smtp_host, or to disable TLS entirely, both of which might be undesirable, depending on the SMTP server configuration.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)