AUTH implementation

[thperret]

Hi,
I improved a little the AUTH implementation provided by @athoune in #10.
I added tests, coverage and documentation.

[warsaw]

We're starting to run out of time to release 1.0 before Pycon, so I'm going to defer AUTH support until after 1.0. I'd still like to pursue this PR and #10, resolve the conflicts, and get the code into shape to land. Are you still interesting in working on it?

[thperret]

Hi. Yes, I'm still interested in working on this PR. I can work on fixing tests and conflicts (do you prefer a rebase or a merge on master?).

[warsaw]

@thperret Awesome, thanks. Whatever is easiest for you is fine with me.

[thperret]

I rebased and the tests seems to be ok but there are several errors that I can't understand. Any ideas?

[warsaw]

I don't yet know what's causing the tracebacks, but the eof related ones only happen in the STARTTLS tests and are described in #83

The never-yielded-from errors are even more mysterious because they only seem to happen on Travis. Unlike the above, they never happen (for me) locally. I captured the problem in #88

Neither of these cause the test suite to fail, which in and of itself is mildly concerning. But for the purposes of your branch you can ignore then since they already exist.

I'll review the code a little later, but the main question I have is the relationship to this PR with #10 - is this built on top of #10 or do we need to reconcile the two approaches?

[Mortal]

@warsaw The PR10 commits are contained in the PR66 commits, so it appears that this PR builds on top of (and supersedes) #10.

[thperret]

Yes, I kept the commits from #10, fixed some issues and wrote documentation

[tlhackque]

It's poor practice to offer a plaintext AUTH method (e.g. PLAIN, LOGIN) over an insecure connection, since it exposes the password. In fact, it violates RFC4954 (see Page 5, last paragraph):

Note: A server implementation MUST implement a configuration in which
it does NOT permit any plaintext password mechanisms, unless either
the STARTTLS [SMTP-TLS] command has been negotiated or some other
mechanism that protects the session from password snooping has been
provided. Server sites SHOULD NOT use any configuration which
permits a plaintext password mechanism without such a protection
mechanism against password snooping.

Since you only support PLAIN, you shouldn't offer AUTH in response to EHLO/LHLO unless the connection is secure (TLS, as that's the only supported security layer).

If at some point you add support for authentication that doesn't transmit plaintext passwords (e.g. DIGEST-MD5, CRAM-MD5, GSSAPI, you can offer those on an insecure connection (subject to require_starttls). In that case, you would need different offerings for secure and insecure connections. I suggest coding to allow for this so as not to increase the tech debt...

Some servers have an option to allow plaintext authentication over an insecure connection - but I don't think that any new server implementation should do this. Exposing a password is worse than not using one. It's also a SHOULD NOT as noted above.

See also section 14 for the client considerations.

[ghost]

@tlhackque I have a bit different point of view, given my use case.

We use aiosmtpd to get data from embedded devices and support for all weird implementation of SMTP is a higher priority for us than security or strict RFC compliance. That's the reason why we actually use aiosmtpd - if we wanted full-blown SMTP server we would use postfix or something.

One of the devices we tested insists on using AUTH forcing us to implement it (we will not validate the password anyway but we need to implement the command). So having the option to support AUTH over insecure connection would be handy for us (it's OK if a flag or something similar is required to enable it).

[tlhackque]

An implementation can have a flag to support this (as I noted, it's a SHOULD NOT) - but it should default to secure. And it's a bad idea.

Mailman is using this to implement its (LMTP) input port; it needs to be RFC-compliant.

Insecure embedded devices are the current frontier of security breaches - you should be pushing back on the device supplier for security. You don't want to be in the news as the gateway to the next headline.

Think stuxnet; the router-based breaches; etc. And yes, mail can be the transmission vector. And no, an isolated network doesn't provide adequate protection from many threats.

TLS is implementable on embedded devices; even an 8-bit microprocessor has the necessary compute power, and memory is cheap.

(Former embedded systems architect.)

[athoune]

Hum, STARTTLS is implemented, why not AUTH? PLAIN AUTH over SARTTLS is enough. SASL is boring and outdated. Right now, I want to do some transactional mail, with DKIM and other modern security stuff. I used (and patch) Cuttlefish, and this is painful, I want to do similar things, with a framework, not a monolithic application.

[waynew]

FWIW @vrehak if you haven't already, I've actually done the weird thing and implemented the AUTH command in my own code. It may violate the mess out of the RFC, but I'm literally using aiosmtpd because Postfix doesn't support the weird behavior I want that I'm pretty sure is literally a violation of the RFC.

But that's OK, because I understand precisely why I want to violate the RFC (i.e. I don't want my relay server sending spam).

I'd be OK with requiring a setting to turn on AUTH over plain text, since it's not hard to flip a switch if you need it - though as an aside, with the rise of Let's Encrypt it's reasonably easy to get SSL setup on your server.

[waynew]

@thperret are you interested in continuing this PR?

[EvyBongers]

Any idea when this might be implemented?

[waynew]

@evypb just waiting on someone to complete the work. If you're capable and interested, we haven't seen any responses from anyone else, so it seems like you'd be more than welcome to pick up where this left off!

[thperret]

Hi, sorry I didn't finished this feature, I end up working on something totally different. I don't have time right now to improve the code and by completely RFC compliant so if anyone wants to pick it up, be my guest. I will maybe have time to do it in a month or two.

[EvyBongers]

@waynew Sorry for my late response. I'm currently in no position to work on this.

[balki]

Can we have the code merged please? I took a look at the traceback and I think they are harmless. It is just the tester thread is faster in travis and kills the server before it could fully await loop.create_server i.e it was at this line when the tester thread killed it. It doesn't always occur because usually the process is at this line, which explicitly ignores the exception. Basically the tests do not shutdown properly. Tests can be improved to do something similar to the Controller class and we won't have this traceback. I think we can merge the code.

[waynew]

@thperret are you interested in finishing this up?

[thperret]

@waynew sorry for my late reply (again). I'm really sorry but I wont have time to work on that anytime soon and as I left this for too long, I prefer to say that I give up on this. If anyone wants to take over, please do so

[pepoluan]

What's blocking getting this PR merged?

[waynew]

@pepoluan just someone to do the work - looks like we've still got some conflicts. It's been a while since I've looked at this implementation, but IIRC it was pretty reasonable.

[pepoluan]

I can pick this up. As it is right now, I have to 'wrap' aiosmtpd's SMTP class into an SMTP_with_AUTH class that I derive from this PR. It would be nice to not doing some acrobatics to have AUTH support :-)

[pepoluan]

@waynew just to be clear: since aiosmtpd is using aysnc def and await, we're targeting Python >= 3.5 right?

EDIT: Wait ... we're requiring atpublic, and atpublic requires Python >= 3.6 ... I'll target Python >= 3.6 then.

[pepoluan]

Quick Update: Conflicts have been resolved in my branch: https://github.com/pepoluan/aiosmtpd/tree/auth_base

However, I still need to update the documentation for I took the easy way out of just overwriting all .rst files with the latest from upstream. I'll spend the next few days updating them docs.

When all is done, I'll create a new PR (for I have no idea how to 'takeover' this PR).

Update 2: While I'm at it, I implemented AUTH LOGIN as well, to help solve #102 . I'll need to write tests for AUTH LOGIN, though 😞 ... if anyone can help me, that would be swell.

Update 3: Fixing test. It's a doozy. After tests are fixed (hopefully with coverage > 90%), I'll fix docs and submit a PR.

[pepoluan]

My PR is live!

#192

[sirk390]

It seems this PR is obsolete. See #192

[pepoluan]

It seems this PR is obsolete. See #192

Indeed. So I'm closing this PR.