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

[9740] ESMTPSender: dont't force TLSv1.0 by default #1225

Merged
merged 8 commits into from Mar 28, 2020

Conversation

@mmilata
Copy link
Contributor

mmilata commented Mar 3, 2020

Hello, I've also been bitten by matrix-org/synapse#6211 and would like to see if the fix is acceptable for inclusion in Twisted.

Contributor Checklist:

@mmilata mmilata changed the title [9470] ESMTPSender: dont't force TLSv1.0 by default [9740] ESMTPSender: dont't force TLSv1.0 by default Mar 3, 2020
Copy link
Member

glyph left a comment

Hi Martin!

Thanks so much for your contribution to Twisted.

The correct fix, here, if you want proper TLS security on twisted.mail connections, would be to drop ClientContextFactory entirely, and use optionsForClientTLS. Dropping TLS 1.0 as a protocol version is good, but security-wise this change is almost meaningless, as ssl.ClientContextFactory doesn't verify certificates, or hostnames, or really provide ''any'' proper security properties. (Separate to this change, it should be deprecated and removed from Twisted entirely.)

Dropping in optionsForClientTLS here should not be much harder that just fixing the protocol version, and it would have the added benefit that it would use a whole slew of ''other'' better security attributes as well. Would you mind modifying your patch to do that?

Thanks!

@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Mar 10, 2020

@glyph thanks for quick review. I tried to replace the ClientContextFactory ctor call with optionsForClientTLS, however it requires the server hostname which I'm not quite sure how to pass to it without breaking the public interface. PTAL

Anyway the TLS negotiation goes further now before rejecting my server's self-signed cert😆. Oh well, probably a good time to replace it with LetsEncrypt one.

mmilata added 2 commits Mar 10, 2020
@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Mar 12, 2020

Installed LE certificate & now matrix-synapse can send emails through postfix configured to only use TLS1.1 or higher with this patch.

src/twisted/mail/smtp.py Outdated Show resolved Hide resolved
src/twisted/mail/smtp.py Outdated Show resolved Hide resolved
src/twisted/mail/smtp.py Outdated Show resolved Hide resolved
src/twisted/mail/smtp.py Outdated Show resolved Hide resolved
@glyph

This comment has been minimized.

Copy link
Member

glyph commented Mar 17, 2020

Thanks again for your contribution! I think the compatibility break is acceptable given the security needs here. Please do re-add the review keyword so we notice it on twistedmatrix.com when you've addressed feedback, so we find it again.

Copy link
Contributor

rodrigc left a comment

I think @mmilata has addressed the review feedback, and this PR now looks OK.

@rodrigc rodrigc merged commit d329445 into twisted:trunk Mar 28, 2020
7 checks passed
7 checks passed
Tests #1520 succeeded
Details
ci/circleci: documentation Your tests passed on CircleCI!
Details
ci/circleci: linux-wheels Your tests passed on CircleCI!
Details
ci/circleci: osx-wheels Your tests passed on CircleCI!
Details
ci/circleci: static_checkers Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 90.728%
Details
@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Mar 28, 2020

Thanks @glyph & @rodrigc!

@mmilata mmilata deleted the mmilata:9470-esmtp-sender-tls-version branch Mar 28, 2020
@rodrigc

This comment has been minimized.

Copy link
Contributor

rodrigc commented Mar 28, 2020

@mmilata thank you for contributing to Twisted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.