search

axllent / mailpit

An email and SMTP testing tool with API for developers
https://mailpit.axllent.org
MIT License
5.7k stars 139 forks source link

[Feature] Always support authentication even without STARTTLS #56

Closed shizunge closed 1 year ago

shizunge commented 1 year ago

I am testing Vaultwarden which uses the library lettre sending emails. I set SMTP_SECURITY=off and use empty username and password for testing. (I will refer vaultwarden as client below)

Mailhog could receive the email. On the other hand, when connecting to mailpit, the client gives me No compatible authentication mechanism was found.

Digging into the code, it is due to mailpit does not send 250-AUTH in the EHLO response. We need the following condition to send 250-AUTH.

  1. tls is enabled. See here
  2. AuthHandler is set.

So could we

  1. Set smtpd.Server.AuthMechs to map[string]bool{"LOGIN": true, "PLAIN": true} Or set them to true via some CLI flags.
  2. Always set a AuthHandler even if there is no config.SMTPAuthFile provided. We can always return true in the new handler and log the username and password for debug purpose.

The above settings may not make sense in a real SMTP server. But it is nice to have these options in mailpit.

axllent commented 1 year ago

Hi there! If I understand you correctly, you are requesting an override to allow Mailpit to allow an insecure login? I don't believe that is a valid / allowed any more (authentication without TLS), it's even stated in that function's comment, RFC 4954 specifies that plaintext authentication mechanisms such as LOGIN and PLAIN require a TLS connection.

Can you please clarify what your reasoning is for not using TLS when authenticating, or why you are authenticating at all if insecure?

shizunge commented 1 year ago

For me, everything is running locally, that is why I don't need TLS. And I don't actually need authentication.

The problem shows up that my client works with mailhog but not mailpit. And I guess is due to missing 250-AUTH in EHLO. It is comfirmed by building mailpit, always returning 250-AUTH.

I hope I can have an option to use a simply setup, minimizing the files and steps needed, like TLS keys. What I proposed is based on how to let the the stmpd library always return 250-AUTH in EHLO to keep my client happy.

These setups do not make sence for real production stmp server. But for testing/verification, it could help us

  1. reduce setup cost.
  2. answer more "what if" questions.
axllent commented 1 year ago

@shizunge Fair enough. I don't generally believe that "just because it worked in Mailhog it should work in Mailpit too" is a valid reason, especially when it's a non-RFC-compliant feature, however I can see the potential use-case for this in testing some mail client configurations.

From what I can see there are actually two separate features required here to get this working:

  1. Allow insecure logins (eg --allow-insecure-logins) - being that TLS is not set but authentication is required
  2. Allow empty/blank logins (eg --allow-empty-logins) - to allow a blank username and password to be provided as a login

From what I understand, neither of these are standard (which surprises me that lettre would support this), so as you say, both of these features would need to be opt-in.

I will see how I get on and try add this at some point (I'm very busy at the moment so requests like this will have to wait unfortunately).

shizunge commented 1 year ago

I drafted a pull request. If you had a time, please take a look.

axllent commented 1 year ago

Thank you! I will definitely have a look at this in the next few days. I appreciate the effort.

axllent commented 1 year ago

I am busy reviewing your PR, and trying to align it with my naming structure (and brain!). I believe there is some overlapping functionality between these two options, so I thought I would just check to see if I understand your intention correctly.

You introduce two new flags, allow-logins-insecure and allow-logins-any.

allow-logins-insecure

This should, if I understand correctly, allow username/password logins via a non-TLS connection? If I am I right, then I do not understand the logic of srv.AuthMechs = map[string]bool{"LOGIN": true, "PLAIN": true} which sends the auth header (if not using TLS), but does not actually require auth. This logic seems horribly broken.

Shouldn't this option simply (by default) require authentication (via --smtp-auth-file handled via authHandler()), but just not require TLS? The way you have implemented it is that it sends a authentication mechanisms header, but doesn't require a login (which sounds like the purpose of allow-logins-any).

allow-logins-any

I guess as the name implies, it just allows no authentication and authentication (accepts anything). This, when used in conjunction with --allow-logins-insecure would allow any non-encrypted SMTP request whether it contain a username/password or not.

So what I think it should do here (and you will need to correct me if I am wrong) is this (and please don't change your code, I have imported your branch into a local testing branch and already made huge modifications, so there is no point):

Wow, I hope I didn't confuse you there! Does this make sense, and is it what you are after? Based on what I understand, you require an unencrypted connection that accepts a blank username and password.

shizunge commented 1 year ago

I don't think --allow-logins-insecure conflicts with --smtp-ssl-cert and --smtp-ssl-key. Setting all three --allow-logins-insecure --smtp-ssl-cert and --smtp-ssl-key is same as just setting --smtp-ssl-cert and --smtp-ssl-key.

And Yes --allow-logins-insecure requires either --smtp-auth-file or --allow-logins-any.

I let --smtp-auth-file take precedence. To me, "allow" defines a less-restrictive constraint, and --smtp-auth-file is more restrictive. When both appear, the more restrictive one wins. Same applies to --allow-logins-insecure v.s. --smtp-ssl-cert and --smtp-ssl-key.

-- Shizun GE (蓋世尊)

On Mon, Mar 6, 2023 at 8:34 PM Ralph Slooten @.***> wrote:

I am busy reviewing your PR, and trying to align it with my naming structure (and brain!). I believe there is some overlapping functionality between these two options, so I thought I would just check to see if I understand your intention correctly.

You introduce two new flags, allow-logins-insecure and allow-logins-any. allow-logins-insecure

This should, if I understand correctly, allow username/password logins via a non-TLS connection? If I am I right, then I do not understand the logic of srv.AuthMechs = map[string]bool{"LOGIN": true, "PLAIN": true} which sends the auth header (if not using TLS), but does not actually require auth. This logic seems horribly broken.

Shouldn't this option simply (by default) require authentication (via --smtp-auth-file handled via authHandler()), but just not require TLS? The way you have implemented it is that it sends a authentication mechanisms header, but doesn't require a login (which sounds like the purpose of allow-logins-any). allow-logins-any

I guess as the name implies, it just allows no authentication and authentication (accepts anything). This, when used in conjunction with --allow-logins-insecure would allow any non-encrypted SMTP request whether it contain a username/password or not.

So what I think it should do here (and you will need to correct me if I am wrong) is this (and please don't change your code, I have imported your branch into a local testing branch and already made huge modifications, so there is no point):

  • just --allow-logins-insecure will require a --smtp-auth-file, and block using --smtp-ssl-cert and --smtp-ssl-key ~ running SMTP unencrypted
  • just --allow-logins-any will block --smtp-auth-file and require --smtp-ssl-cert and --smtp-ssl-key ~ running SMTP over TLS
  • combining --allow-logins-insecure --allow-logins-any will block --smtp-auth-file, --smtp-ssl-cert and --smtp-ssl-key ~ running SMTP unencrypted and allow any logins

Wow, I hope I didn't confuse you there! Does this make sense, and is it what you are after? Based on what I understand, you require an unencrypted connection that accepts a blank username and password.

— Reply to this email directly, view it on GitHub https://github.com/axllent/mailpit/issues/56#issuecomment-1457514742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJY3UZ7XK2HBHZ5XTFUFQLW223G7ANCNFSM6AAAAAAVI5A3DQ . You are receiving this because you were mentioned.Message ID: @.***>

axllent commented 1 year ago

Sorry, I believe I completely misunderstood the Authentication mechanisms & your question here, please bare with me here while I think aloud and ask for your opinion/confirmation here. I am just trying to ensure that I fully understand everything before I make any changes.

Re-reading RFC 4954 specifies that plaintext authentication mechanisms such as LOGIN and PLAIN require a TLS connection, what this is actually saying is that both LOGIN and PLAIN are perfectly fine, provided it negotiates over TLS.

So if I am to implement your request properly, as well as correct my existing misunderstanding of authentication requirements in SMTP, I would need to make the following changes to Mailpit:

  1. Add LOGIN and PLAIN to the existing CRAM-MD5 authentication methods when using TLS.
  2. Allow authentication to be used without TLS, but it would only support (by default) CRAM-MD5 and not allow LOGIN and PLAIN (as per the RFC). LOGIN and PLAIN could then be added via --allow-logins-insecure. Currently Mailpit only allows authentication if TLS is used which I now understand is incorrect.

If my understanding is correct then, in order for Mailpit to send the 250-AUTH ... header, then SMTP authentication should be required (shouldn't it?). I'm having great difficulty finding a clear explanation as to whether this should be sent when the server doesn't require authentication (all I know is mhale/smtpd doesn't send this when no auth is required).

This brings me to your other feature (--allow-logins-any). I believe that if used, this should require authentication (srv.AuthRequired = true), however will always return true (ie: any username/password), using one of the allowed authentication methods of course.

Does it sound like I have a good understanding of the issue now? Please feel free to correct me, and I apologize for all the questions.

shizunge commented 1 year ago

I think your understanding aligns with me.

Add LOGIN and PLAIN to the existing CRAM-MD5 authentication methods when using TLS.

I think this is already done.

axllent commented 1 year ago

Thanks again for all your feedback & suggestions. In the end I must have created about 5 different variations while trying to integrate this in a manner I was happy with, but I think I got there in the end. I have just released v1.4.0 which contains both features (though different flags, --smtp-auth-accept-any & --smtp-auth-allow-insecure). Although very different to your PR (code-wise) , it does follow the similar principals.

When you get a chance, please have a play and let me know if this does what you expect it to? Thank you.

shizunge commented 1 year ago

It works. Thank you.

axllent commented 1 year ago

Awesome, and thank you for all the feedback & testing! I'll close your PR now as it's no longer needed :+1: