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

Added reject_authenticated_sender_login_mismatch #872

Merged
merged 7 commits into from
Mar 7, 2018

Conversation

17Halbe
Copy link
Contributor

@17Halbe 17Halbe commented Mar 4, 2018

Kudos to @TechnicLab who had the idea and found that "undesired behaviour"
This will deny authorized clients to send with a different than their owned mail-address.

So there remain some considerations:

  • With this in place people may not be sending mails under their aliases as well. (Except they configure an account for every alias(which they would have to do with a normal client program anyway))
  • Might their be use cases where someone wants to send mails as a different user?

See also #825
partially solves #524

@17Halbe 17Halbe changed the title Added reject_sender_login_mismatch Added reject_authenticated_sender_login_mismatch Mar 4, 2018
@johansmitsnl
Copy link
Contributor

johansmitsnl commented Mar 4, 2018

@17Halbe looks good!
Does this needs testing for ldap to or is this just the same behaviour?
Does it also work with an alias setup for your account and use it as a sender email?

@TechnicLab
Copy link
Contributor

TechnicLab commented Mar 4, 2018

@17Halbe Thank you! @johansmitsnl It is working for LDAP.

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 4, 2018

Yes I don't see a reason why this shouldn't be working with ldap.
It is comparing the logged in email address with the one sending. If they are identical, the request is granted. This shouldn't make a difference for ldap.

@johansmitsnl
Copy link
Contributor

And for the aliases?

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 4, 2018

Yes, as I said, if root@mailserver.com trys to send an email as (his alias) postmaster@mailserver.com, it would fail.
In that case one has to set a seperate mailaccount for postmaster@mailserver.com up to be able to send mail from postmaster@mailserver.com.
I don't know, does anyone do that?

Receiving mails for your alias of course is not a problem.

In my opinion the behaviour of not allowing someone to send a mail with a spoofed address (even non existing ones) should be default. If someone would need to send an email via an alias, they should actually consider setting this account up as a regular one. IMHO that's not what aliases are for. I always considered them as more restrictive "catch-all" addresses.
How do you use them?

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 4, 2018

I'm still thinking about this. we could probably work around the alias problem if you consider it neccessary. But in that case we would have to add every normal address to the virtual (alias) file as well.
And this would need a special handling for ldap.

@johansmitsnl
Copy link
Contributor

@17Halbe in my personal setup I use on the iPhone mail app and there I have setup some aliasses so that I can email with the same account back but maintain the alias in the reply.
Example:
main account: johan@domain.com
Alias: alias@domain.com
When I reply to a email that arrived on the alias it replies with the alias from and should be valid to me.
If not this might break more then desired on multiple setups.

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 4, 2018

Ok, we then need a setup differenciating between ldap and the standard setup.
I will provide a solution for the standard setup. I‘ll try my best for ldap.
Do we need an exception for mails containing delimiters as well?

@johansmitsnl
Copy link
Contributor

I don't use it but if you can include them yes.

@dmcgrandle
Copy link
Contributor

dmcgrandle commented Mar 4, 2018 via email

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 4, 2018

Just to clarify, this PR is just denying a logged in(!) user to send mails with another than his account address or (going to be implemented) his alias addresses.
Would this work for your setup?

@dmcgrandle
Copy link
Contributor

dmcgrandle commented Mar 5, 2018 via email

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 5, 2018

So I introduced a new env variable, which default value is deactivating reject_authenticated_sender_login_mismatch.

For a new upcoming release in the hopefully upcoming release channel system I would suggest to implement reject_authenticated_sender_login_mismatch as a default. It's really easy to delete out of main.cf if one really needs to work around it.
In the end I think most people would agree that sending mails for a different account is wrong. ;)

Tested ldap implementation only in the test.bats. Can someone please check if this would work with an alias in ldap. I just copy pasted the ldap config from #825

bind             = yes
bind_dn          = cn=admin,dc=domain,dc=com
bind_pw          = admin
query_filter     = (&(|(mail=%s)(mailalias=%s))(mailEnabled=TRUE))
result_attribute = mail
search_base      = ou=people,dc=domain,dc=com
server_host      = mail.domain.com
start_tls        = no
version          = 3

Does that look correct?

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 5, 2018

Nevermind. Just reverted the ldap lookup to the already existing tables and added test for aliases as well. This should now be a safe ldap implementation!

@johansmitsnl
Copy link
Contributor

This looks very good! Waiting for @tomav to let me know when the release branches are setup to I can merge it.

@17Halbe
Copy link
Contributor Author

17Halbe commented Mar 7, 2018

So if you're going to merge this to a new release I would suggest switching the default to enable SPOOF_PROTECTION. This PR can be merged without changing the current behaviour at all. It's disabled by default. You have to set SPOOF_PROTECTION to 1 to make it work.

What about merging it now and I'll provide a PR for the new release to make spoof protection the default setting?

@johansmitsnl
Copy link
Contributor

@17Halbe thats a good idea. Merged it so you can provide a new PR once the branching is done.

@johansmitsnl johansmitsnl merged commit a73692c into docker-mailserver:master Mar 7, 2018
@dmcgrandle
Copy link
Contributor

Good work on this @17Halbe!
:D

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

Successfully merging this pull request may close these issues.

None yet

4 participants