Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs and example suggest running aiosmtpd INSECURELY #374

Open
remram44 opened this issue Apr 13, 2023 · 5 comments
Open

Docs and example suggest running aiosmtpd INSECURELY #374

remram44 opened this issue Apr 13, 2023 · 5 comments

Comments

@remram44
Copy link

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
Copy link
Collaborator

waynew commented Apr 13, 2023

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

@remram44
Copy link
Author

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
Copy link
Collaborator

waynew commented Apr 13, 2023

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 remram44 mentioned this issue Apr 13, 2023
9 tasks
@remram44
Copy link
Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants