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

[jiggle]

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

[argonemyth]

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

[argonemyth]

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!

[argonemyth]

first attempt to fix the bug!

[exarkun]

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.

[argonemyth]

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"!

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.

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.

[exarkun]

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

[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.

[exarkun]

 Build results

[exarkun]

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

[tom.prince]

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).

[exarkun]

(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.