Opened 5 years ago

Closed 19 months ago

#3989 defect closed fixed (fixed)

t.mail.smtp.ESMTPSenderFactory requireTransportSecurity fails to recognize when SSL is in use

Reported by: jiggle Owned by: exarkun
Priority: normal Milestone:
Component: mail Keywords: ESMTP SSL TLS
Cc: fei@… Branch: branches/esmtp-ssl-3989
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

If one uses t.reactor.connectSSL() to establish an SSL connection to a SMTP server, and requireTransportSecurity=True, it still attempts to use TLS (over SSL) and usually fails (the server won't allow it).

It should recognize that TLS is already present...

Current work-around is to set requireTransportSecurity=False when creating an instance of the factory.

Failure in this case (from the server, with an SSL connection already present is):
Failure: twisted.mail.smtp.TLSRequiredError: 502 Server does not support secure communication via TLS / SSL

Attachments (2)

3989-ESMTP_SSL-draft1.patch (5.5 KB) - added by argonemyth 2 years ago.
first attempt to fix the bug!
3989-ESMTP_SSL-draft2.patch (5.0 KB) - added by argonemyth 2 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by <automation>

  • Owner exarkun deleted

comment:2 Changed 2 years ago by argonemyth

  • Cc fei@… added
  • Keywords SMTP SSL TLS added
  • Owner set to argonemyth

Going to make connectSSL work without manually change the default value of requireTransportSecurity.

comment:3 Changed 2 years ago by argonemyth

  • Keywords ESMTP review added; SMTP removed
  • Owner argonemyth deleted

Fixing the bug is fairly easy, as I followed exarkun's suggestions. But, I spent pretty long time to write the proper test case for it.

Exarkun asked me to deprecate the tlsMode in ESMTPClient, but I am not sure how to deprecate a variable properly. Since the chance of someone setting the value to True is rare, I removed all the references of tlsMode in smtp.py. But, I don't think it's a good an idea to just remove them all, let me know the proper way of handling this.

As for the test case, I tried different approaches to simulate reactor.connectSSL (tried to use existing classes and helpers), but still, the file ended up with 4 new classes. Please review the draft 1 of the patch and let me know if you have better idea for the test case.

Thank you very much!

Changed 2 years ago by argonemyth

first attempt to fix the bug!

comment:4 Changed 2 years ago by exarkun

  • Owner set to exarkun

comment:5 follow-up: Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to argonemyth

Thanks. Two main comments:

  1. Don't delete tlsMode. CompatibilityPolicy describes how we make incompatible changes. This will involve deprecating the attribute, releasing that a couple times, and then removing it. Please file a new ticket for introducting that deprecation. For now, for this branch, leave the tlsMode attribute in place, but we don't have to use it anymore.
  2. The test says it is going to "simulate" reactor.connectSSL. That sounds excellent. However, what the test actually does is use the real reactor.connectSSL. Any time it's possible to avoid creating real TCP connections (SSL runs over TCP), we want to avoid it. Real TCP connections are subject to too many external influences to be reliably used in unit tests.
    1. See twisted.test.proto_helpers.MemoryReactor for a bare-bones simulation of connectSSL that would help out with this test. You'll probably have to write one or two more similar "memory" simulations to complete the test, though - for example, a simulation of the transport object (although we have twisted/test/iosim.py, it isn't documented and does some weird extra things that might not be helpful in this case).
    2. Also, test methods need docstrings too.

Please feel free to find me on IRC or post to twisted-python if you'd like to discuss how to approach either of these points.

comment:6 in reply to: ↑ 5 Changed 2 years ago by argonemyth

  • Keywords review added
  • Owner argonemyth deleted

Replying to exarkun:

Uploaded the second version of the patch...

  1. Don't delete tlsMode. CompatibilityPolicy describes how we make incompatible changes. This will involve deprecating the attribute, releasing that a couple times, and then removing it. Please file a new ticket for introducting that deprecation. For now, for this branch, leave the tlsMode attribute in place, but we don't have to use it anymore.

Kept the tlsMode as the class attribute, but removed any other appearances of tlsMode in the class. Hope that's what you mean by "we don't have to use it anymore"!

  1. The test says it is going to "simulate" reactor.connectSSL. That sounds excellent. However, what the test actually does is use the real reactor.connectSSL. Any time it's possible to avoid creating real TCP connections (SSL runs over TCP), we want to avoid it. Real TCP connections are subject to too many external influences to be reliably used in unit tests.

Well, In the second patch, I used the MemoryReactor, but I made the connectSSL method returning a protocol object which connected with a fake SSL StringTransport. I don't know if it's a good solution, but it's the simplest I can come up with right now, and it did the job of testing ESMTPClient.tryTLS method.

Changed 2 years ago by argonemyth

comment:7 Changed 19 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/esmtp-ssl-3989

(In [36672]) Branching to 'esmtp-ssl-3989'

comment:8 Changed 19 months ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks.

  1. The whole MemoryReactor.connectSSL process is unnecessary. All you need to do is instantiate the protocol (ESMTPClient) being tested, connect it to a transport (call makeConnection on it), and then deliver data to it.
  2. There is some missing test coverage for some of the code paths through tryTLS.

comment:10 Changed 19 months ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

I addressed those points, made the tests properly skip when SSL dependencies are unavailable, and added a news fragment.

comment:11 Changed 19 months ago by tom.prince

  • Keywords review removed
  • Owner set to exarkun

The behavior and use of {{{tryTLS}} has changed, and isn't entirely clear. It would be good to add a docstring explaining what it does.

Please commit after adding a docstring for tryTLS (and optionally esmtpState_serverConfig and esmtpState_starttls).

comment:12 Changed 19 months ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [36695]) Merge esmtp-ssl-3989

Author: argonemyth, exarkun
Reviewer: tom.prince
Fixes: #3989

Teach ESMTPClient about running over SSL connections - specifically, that it does
not need to try to negotiate a new SSL session in this case.

Note: See TracTickets for help on using tickets.