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

Add LDAP_QUERY_FILTER_SENDERS setting for spoof protection with LDAP #1902

Merged

Conversation

moqmar
Copy link
Contributor

@moqmar moqmar commented Apr 12, 2021

Description

This adds an environment variable LDAP_QUERY_FILTER_SENDERS that determines if a user is allowed to use a specific identity to send an email - for example, we're using something like (|(mail=%s)(mail=noreply@example.com)), to allow users to send an email if they either have the mail address they want to send as or if they have the mail address noreply@example.com. This applies only if SPOOF_PROTECTION=1.

Also, as suggested in #1340, this sets smtpd_sasl_local_domain to an empty string - it seems like without that it won't work, but I couldn't figure out the implications of that, so that might be worth discussing. (that change has been reverted as the issue itself seems to have been fixed in the meantime)

If the environment variable is empty, the old sender smtpd_sender_login_maps behaviour will be used.

Fixes #1340

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or ENVIRONMENT.md or the documentation)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach
Copy link
Member

LGTM, but I'm not using LDAP myself. I will request a review.

@reneploetz
Copy link
Contributor

reneploetz commented Apr 13, 2021

Setting smtpd_sasl_local_domain to an empty string changes the lookup key for the authentication in case the SMTP client does not send an authentication with a domain name, e.g. if he tries to authenticate with "foo" and smtpd_sasl_local_domain = "bar.com", then he would authenticate with "foo@bar.com" against SASL - see check_sasl_access in postfix manual.
I think this might break existing configurations if the SASLAUTHD_LDAP_FILTER returns results without a domain name.

Note that SPOOF_PROTECTION=1 does already work on my system using LDAP aliases for my accounts (*), so I think this is more a configuration issue, especially given that you replace all other lookups with a new lookup. Removing the requirement for $mydomain in smtpd_sasl_local_domain leads me to believe that the underlying problem is that your initial lookup maps do not match your domain settings.

(*) I'm currently using LDAP with a Kopano-LDAP-schema like so:

LDAP_QUERY_FILTER_USER=(&(kopanoAccount=1)(mail=%s))
LDAP_QUERY_FILTER_GROUP=(&(objectclass=kopano-group)(mail=%s))
LDAP_QUERY_FILTER_ALIAS=(&(kopanoAccount=1)(kopanoAliases=%s))
LDAP_QUERY_FILTER_DOMAIN=(mail=*@%s)
SASLAUTHD_LDAP_FILTER=(&(kopanoAccount=1)(uid=%s))

In my setup, both mail and kopanoAliases attributes contain fully qualified email addresses of my current domain and domains I want to use for relay hosts. The uid is the local part without the domain name. I'm using LDAP_DOMAIN for the same LDAP container you are using with no subdomain.

@moqmar Given your original bug report, can you first check your SASLAUTHD_MECHANISMS as the default is "pam" and you either want to use ldap or rimap (with SASLAUTHD_MECH_OPTIONS containing the IP/FQDN of your dovecot). I'm using the latter which should ignore the SASLAUTHD_LDAP_FILTER settings.

@moqmar
Copy link
Contributor Author

moqmar commented Apr 13, 2021

Ah, your SASLAUTHD_LDAP_FILTER isn't being used at all because you're using rimap. According to https://blog.sys4.de/cyrus-sasl-saslauthdconf-man-page-en.html, %s is the service name: in my case it looks up (uid=smtp) if I set it that way. %D is empty; %u is just the part before the @, %d is always empty for some reason, even when specifying a domain. (mail=%u@example.org) is the only thing that works, but then mail@blah.com is treated the same as mail@example.org, although that error can then be caught later by the spoof protection when using the email address as an identifier.

When using that (mail=%u@example.org) filter, you're right that spoof protection seems to work correctly now, so it seems like the original #1340 issue has solved itself or always was a configuration issue?

TL;DR: SASLAUTHD_LDAP_FILTER doesn't really work reliably in general, and has not much to do with this; when leaving smtpd_sasl_local_domain as it was before it seems like everything still works as intended, and #1340 seems to be solved. I have changed it back, and thanks for the rimap tip.


So, it doesn't seem like this solves #1340 (as it seems to be working now in general), but I'd still like to see this merged. The big advantage of the sender filter (even if it works without this PR) is that it just provides a lot more flexibility. I'm using basically the following filter for example:

(&
  (memberOf=cn=employee,ou=groups,dc=example,dc=org)
  (|
    (&(mail=%u@example.org)(mail=%s))
    (mail=noreply@example.org)
    (memberOf=cn=admin,ou=groups,dc=example,dc=org)
  )
)

This means that only employee members can send any emails (even if others might have an IMAP inbox and can receive emails), only from their email address @example.org (even if they also have an additional @blah.com in the LDAP, and maybe even can receive emails on that address). Also, noreply@example.org and members of the admin group can send emails from any address.

That kind of flexibility would be impossible to achieve without an additional sender filter table.

@moqmar moqmar force-pushed the bugfix/ldap-spoof-protection branch from 33c1f47 to 5e1ec62 Compare April 13, 2021 10:18
Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻 Thanks for your contribution.
I just want someone else to look over it before approving this

Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please write another test that shows that your changes work both ways?

This should be the correct file:
https://github.com/docker-mailserver/docker-mailserver/blob/master/test/mail_with_ldap.bats

Marking this with low priority as the mailserver works without your change as well.

@moqmar
Copy link
Contributor Author

moqmar commented Apr 16, 2021

Test has been added - I set the variable so that additionally to the original behaviour, the user some.user.id (from "04_user-email-different-uid.ldif") can use any address for sending.

The "rejects sender forging" test then checks if spoofing fails for some.user (from "02_user-email.ldif"), and the "uses senders filter" checks if some.user.id can spoof its address due to the exception in the filter.

Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your changes. Really appreciate your contribution and the fact that you wrote some tests!

I will approve this PR BUT I would really like to see the nice explanation you already provided in the documentation.

Can you add this:

The big advantage of the sender filter (even if it works without this PR) is that it just provides

a lot more flexibility. I'm using basically the following filter for example:

(&
(memberOf=cn=employee,ou=groups,dc=example,dc=org)
(|
(&(mail=%u@example.org)(mail=%s))
(mail=noreply@example.org)
(memberOf=cn=admin,ou=groups,dc=example,dc=org)
)
)
This means that only employee members can send any emails (even if others might have an IMAP inbox and can receive emails), only from their email address @example.org (even if they also have an additional @blah.com in the LDAP, and maybe even can receive emails on that address). Also, noreply@example.org and members of the admin group can send emails from any address.

That kind of flexibility would be impossible to achieve without an additional sender filter table.

to probably this page (or another one if you find a better spot) before this gets merged?

ENVIRONMENT.md Outdated Show resolved Hide resolved
target/postfix/ldap-senders.cf Show resolved Hide resolved
test/mail_with_ldap.bats Show resolved Hide resolved
Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay after finishing my review i realized that there are more changes so i will swith to request changes but in general i like your changes and will approve based on your next action ;)

@moqmar
Copy link
Contributor Author

moqmar commented Apr 16, 2021

For the LDAP documentation, I'd like to generally create a separate PR as the other variables aren't really explained yet either, is that fine for you?

@moqmar moqmar requested a review from wernerfred April 16, 2021 12:20
@wernerfred
Copy link
Member

For the LDAP documentation, I'd like to generally create a separate PR as the other variables aren't really explained yet either, is that fine for you?

Absolutely fine. Would be wonderful if you could give the LDAP documentation some love. Feel free to open the PR as WIP so we can keep track of ongoing changes and support you :)

Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aendeavor wanna still wait for #1901 ? ;) I let merging this up to you.

In #1901 I want at least another one with LDAP experience to review it.

@georglauterbach
Copy link
Member

@aendeavor wanna still wait for #1901 ? ;) I let merging this up to you.

Fine with me, there's no need to wait :D I will merge this.

In #1901 I want at least another one with LDAP experience to review it.

Yes, that's a good idea. I don't have experience with LDAP either.

@georglauterbach georglauterbach merged commit 271d94a into docker-mailserver:master Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot send emails with SPOOF_PROTECTION=1
4 participants