Fix SMTPS / implicit TLS

[strongholdmedia]

What do these changes do?

Allow for implicit TLS usage (based solely on provided context data as, like @pepoluan mentioned in #281, it is not quite feasible to detect any wrapping (nor should it be done).

Are there changes in behavior for the user?

Should not be any, as the only change affects a feature that did not quite work, like, ever.

Related issue number

Fixes #274 and most likely also closes #281.

Checklist

• [✓] I think the code is well written
• [✓] Unit tests for the changes exist
• [✓] tox testenvs have been executed in the following environments:
  • [✓] Linux (Ubuntu 18.04, Ubuntu 20.04, Arch): {py36,py37,py38,py39}-{nocov,cov,diffcov}, qa, docs
  • [✓] Windows (7, 10): {py36,py37,py38,py39}-{nocov,cov,diffcov}
  • [✓] WSL 1.0 (Ubuntu 18.04): {py36,py37,py38,py39}-{nocov,cov,diffcov}, pypy3-{nocov,cov}, qa, docs
  • [✓] FreeBSD (12.2, 12.1, 11.4): {py36,pypy3}-{nocov,cov,diffcov}, qa
  • [✓] Cygwin: py36-{nocov,cov,diffcov}, qa, docs
• [✓] Documentation reflects the changes
• [✓] Add a news fragment into the NEWS.rst file
  • I cannot find "NEWS.rst" in this repository

[waynew]

I would like to see a test that can confirm this behavior - ideally one that we could run in the automated tests, but I'd even be OK with one that we could run out of band 🤔

@strongholdmedia are you able to do that in the next week or two?

[strongholdmedia]

@waynew well, my schedule is unusually overcrowded for the end of the year, but I can probably manage it in say, three weeks, if not two.
But I would like some encouragement about it getting processed in a shorter time frame – given that I created this PR just a bit over a year ago :)

EDIT: if this is to serve some release deadline purpose, I will try to adhere to some stricter timeline.

[waynew]

No worries - we've definitely seem some ebb in our activity 😅 obviously I'm getting back into the flow here.

If you can get it, that would be awesome - but I realize that life might be happening. Either way, I'm going to do what I can to get all these changes in before I cut a release.

[pepoluan]

Ohhh this is nice ... but a bit complex-y...

I really want to merge this, but let's do it after 1.4.3 goes final.

We're trying to ensure projects that depends on aiosmtpd to have time to test with 1.4.3 and make the Debian Freeze (see #322 )

[strongholdmedia]

We're trying to ensure projects that depends on aiosmtpd to have time to test with 1.4.3 and make the Debian Freeze (see #322 )

I still don't really understand this issue.
For once, this should be 100% backwards compatible (and, per the previous tests, it is).
On the other hand, it is 2022; we (and presumably wherever the providers don't have mandatory government intervention/rigging nobody else) are not using anything but implicit TLS for over 5-6 years.

[strongholdmedia]

@waynew I checked stuff and it seems that a very basic test case is provided historically for SMTPS.

I tried to extend it somewhat, for the mere sake of activity :) but I have a hard time figuring out tox's "internal error" that it throws on me during coverage testing (with the main branch as well, so nothing specific to the changes).

On the other hand, the basic test case may already suffice, as:

• as per the RFC, there is no way to distinguish between implicit TLS and plaintext protocol within the wrapped handler (here, smtpd itself)
• the SSL/TLS context already having tests in the STARTTLS test
• as such, if the SMTP test runs properly, and the STARTTLS as well (so that the SSL context handling is okay), the only thing that is left is to check whether the client can (still) communicate with the server when smtps options are provided; and that is covered by the skeletal test case being included since who knows when

As such, I am not quite sure what to test further – as you either have TLS wrapping, and everything appears as if it was plaintext, or you have no TLS wrapping when you expect it, and you get random "binary" garbage.

Meanwhile I merged upstream changes so that it is not outdated any further.

[strongholdmedia]

Ah, now I see that there should probably be a test case for #281 (that is, having AUTH in the EHLO response).

[strongholdmedia]

I checked smtplib docs and the test should work as it is. But I won't know until the workflow is authorized, as sadly, I must be missing something with the tox environments.
Will check tox sources for what could be causing the issue for me tomorrow.
Meanwhile, I hope the test will run :)

[pepoluan]

Whoops got this error:

aiosmtpd/tests/test_smtps.py:14:1: F401 'aiosmtpd.testing.statuscodes.SMTP_STATUS_CODES as S' imported but unused

If you can add another commit to your master to remove that line, we can start the workflow again.

[strongholdmedia]

I did, thanks. The previous test used the hardcoded response string for comparison, but the EHLO method returns a tuple IIRC.

[pepoluan]

I did, thanks. The previous test used the hardcoded response string for comparison, but the EHLO method returns a tuple IIRC.

The previous test actually compares the tuple returned to a well-known tuple 'constant' in another file. Just because I got tired of checking both the code and the string returned separately, every time 😄

[pepoluan]

Coming nearer to fixing this! Only 2 failed tests actually, though spread across all the OSes 😆

Fail No. 1

FAILED aiosmtpd/tests/test_smtp.py::TestAuthArgs::test_warn_authreqnotls - AssertionError: assert ('mail.log', ... using SMTPS') == ('mail.log', ...tls == False')
  At index 2 diff: 'auth_required == True but auth_require_tls == False and not using SMTPS' != 'auth_required == True but auth_require_tls == False'

Will need to change the expected string 😊

Fail No. 2

FAILED aiosmtpd/tests/test_smtps.py::TestSMTPS::test_smtps - AssertionError: assert False
 +  where False = <bound method SMTP.has_extn of <smtplib.SMTP_SSL object at 0x7fca56664c10>>('AUTH')

This might need a bit deeper digging; seems like has_extn('AUTH') is not returning True?

Edit: I think you need to drop line test_smtps.py#L45: The SMTP class being instantiated does NOT have an authenticator. After all, the test simply tests that SMTP works inside an implicit SSL tunnel.

[strongholdmedia]

Edit: I think you need to drop line test_smtps.py#L45: The SMTP class being instantiated does NOT have an authenticator. After all, the test simply tests that SMTP works inside an implicit SSL tunnel.

Well, the complaint in #281 was (duly so) that AUTH is not provided when using SMTPS. (I.e. handled as plaintext.)
That was definitely not optimal from any perspective.

This is now definitely fixed, I tested it manually yesterday (running the module directly).

Currently if a "ssl" prop (a TLS context) is provided to create_server, it will use SMTPS. (I think this was sort of historical behavior, it just never worked.)

So that I need to figure out how to pass a pre-wired TLS context to the constructor within the test (i.e. where to get key and cert from), then it should work.

EDIT: yes, this, with what you have stated already, should definitely mean the "SMTPS" test was always running in plain unsecured mode. :/

[waynew]

aiosmtpd/tests/certs/server.crt should be the cert you're looking for 👍

[pepoluan]

aiosmtpd/tests/certs/server.crt should be the cert you're looking for 👍

Aren't we supposed to refresh that cert every now and then?

[strongholdmedia]

Aren't we supposed to refresh that cert every now and then?

I see it is valid until May 8 13:55:26 2119 GMT.
So in theory yes, but I guess it will work for some more weeks coming :)

[strongholdmedia]

I missed the fact that all "extra" arguments are passed to the factory, then the controller, and finally to SMTP class.
This should be the proper approach – unsure if it really works though.

[pepoluan]

Wow, this time the change caused all jobs to fail. All jobs fail at exactly the same test case:

FAILED aiosmtpd/tests/test_smtps.py::TestSMTPS::test_smtps - AssertionError: assert 'smtps' in {'8bitmime': '', 'auth': ' LOGIN PLAIN', 'help': '', 'size': '33554432', ...}
 +  where {'8bitmime': '', 'auth': ' LOGIN PLAIN', 'help': '', 'size': '33554432', ...} = <smtplib.SMTP_SSL object at 0x7f5325683510>.esmtp_features

[strongholdmedia]

Well, my bad, it was not a change now but 2 days ago, which was some sort of leftover attempt. :/ But removed it now.
To up the challenge, added a line that tests that STARTTLS is not available, since that would make no sense to wrap the already encrypted communication.

@pepoluan all jobs failed previously as well, just – probably due to system congestion – they took longer to fail :)
But anyway, from this failure, we learned that AUTH is now indeed available with SMTPS.

[codecov]

Codecov Report

Merging #292 (5625213) into master (330e9ef) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #292   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1696      1697    +1     
  Branches       310       310           
=========================================
+ Hits          1696      1697    +1     

Impacted Files	Coverage Δ
aiosmtpd/main.py	100.00% <100.00%> (ø)
aiosmtpd/smtp.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[waynew]

aiosmtpd/tests/certs/server.crt should be the cert you're looking for 👍

Aren't we supposed to refresh that cert every now and then?

It expired a year or two ago, breaking all the tests. So I went ahead and generated one that was good for 100 years, since it's only used in testing.

Next time it needs help it won't be my problem

[strongholdmedia]

@pepoluan @waynew any news here? :) Not to be impatient or something, but I suggest the "in progress" tag be removed, and also the "enhancement" is debatable, as this is actually for a core feature that never worked according to the RFC (even the test run blank for it).

[pepoluan]

@pepoluan @waynew any news here? :) Not to be impatient or something, but I suggest the "in progress" tag be removed, and also the "enhancement" is debatable, as this is actually for a core feature that never worked according to the RFC (even the test run blank for it).

At the moment I'm really concerned with the deprecation of get_event_loop() which had passed under my radar. If not satisfactorily handled, this will cause everything relying on aiosmtpd to grind to a halt.

So I'm prioritizing #355 first.

[pepoluan]

Some thoughts, discussions welcome.

[pepoluan]

After doing some tracing, a thought occurred to me:

Implicit TLS (SMTPS) is triggered if tls_context is set, right?

Can we just set is_using_smtps attribute from that? So self._is_using_smtps = (tls_context is not None)?

I might've overlooked something, so I welcome a discussion here.

[strongholdmedia]

Well, yes and no.
For once, tls_context is for STARTTLS; SMTPS uses ssl_context. (This was already such before, I did not change it.)

Otherwise we could set that, yep, but this option communicates clear intent.

As you've noted in #281, there is no possible means for detecting whether or not we are using implicit TLS (SMTPS) within the SMTP class.

This goes to the extent that ssl_context is actually not used by the SMTP class at all.
It is still kept by me, as if later authentication methods will require e.g. client re-validation or something, it could happen.

But unlike is_using_smtps, ssl_context may or may not be an explicit intent.
ssl_context being None could equally mean "should not use SMTPS", as well as "hey, we/they/... failed to handle a key / cert error / expiry / ... somewhere".

But as per the above, there is no means to detect whichever from inside the SMTP class; as such, if the described scenario happens, the client/user would just note that "hey, we don't have AUTH", or even "got SMTP error, pls help, it worked yesterday".

Currently, if is_using_smtps is set with no ssl_context, it should present an error.
I understand your point and will change it if that'd be the agreement, but I feel that would be a technically less correct approach.

[pepoluan]

Moved this to 1.4.4 milestone because it doesn't seem to cause any b0rkages.

However, refraining from merging until the 'discussion' above has been resolved.

[ThiefMaster]

Could this please be merged and a fixed version released? :)

As a user I do not really care if you can pass stupid combinations of arguments or set attributes that would silence the warnings without actually using TLS. What I do care about is not having to use auth_require_tls=False (which triggers annoying warnings and even logs a warning message) in order to use implicit TLS...