search

aio-libs / aiosmtpd

A reimplementation of the Python stdlib smtpd.py based on asyncio.
https://aiosmtpd.aio-libs.org
Apache License 2.0
319 stars 96 forks source link

Docs and example suggest running aiosmtpd INSECURELY #374

Open remram44 opened 1 year ago

remram44 commented 1 year ago

The authenticated relayed example does NOT actually required authentication to relay.

The docs suggest a similar setup that is INSECURE.

This is a major issue, people will implement their system incorrectly. It will look like it works as using wrong login/password will correctly reject, but using no auth at all will incorrectly accept incoming mail.

This can be fixed for example by checking session.authenticated in handle_RCPT or handle_DATA.

waynew commented 1 year ago

Thanks for the report! Is this a documentation change that you would feel comfortable submitting a PR for?

remram44 commented 1 year ago

To be honest I think the behavior is wrong, but this can probably be patched in the docs if we are not changing behavior.

waynew commented 1 year ago

If there's broken behavior we need to get a test to exhibit the broken behavior, and then fix that - but if it's just an incorrect documentation of behavior then we should fix that.

I know in my personal project I just implemented my own AUTH handler. At the time, I don't think we handled AUTH at all within aiosmtpd and it was left as an exercise to the reader.

remram44 commented 1 year ago

I think the fact that your own example got this wrong is a sign that the behavior expected by everybody is not what is implemented.

I prepared a pull request at #375, but I think auth_required should default to True if authenticator is set.

pepoluan commented 11 months ago

I think the fact that your own example got this wrong is a sign that the behavior expected by everybody is not what is implemented.

I prepared a pull request at #375, but I think auth_required should default to True if authenticator is set.

Good point.

But then again we maintain the same behavior as the original implementation.

Changing the default behavior means breaking backward compatibility.

So ... maybe for the next major version.

For now we'll make do with -- maybe -- a Warning both in the docs and when the library is invoked insecurely.