iredmail / iRedAdmin

iRedMail Admin Panel (Open Source Edition)
https://www.iredmail.org/admin_panel.html
GNU General Public License v2.0
77 stars 28 forks source link

Check credentials against LDAP bind instead of performing local hash #5

Closed francescocarzaniga closed 4 years ago

francescocarzaniga commented 4 years ago

When using LDAP backend, use LDAP native resources instead of checking/generating hashes manually. It complies to the various RFCs and allows all hashes supported by LDAP (e.g. argon2). Checked with OpenLDAP.

iredmail commented 4 years ago

Dear @francescocarzaniga

Thanks for the contribution. But we still need to support servers which runs old OpenLDAP or don't have pw_sha2 module enabled. It may take 1-2 years to slowly retire them and drop support for them.

francescocarzaniga commented 4 years ago

In this commit I use three LDAP operations:

  1. BIND (which you already use)
  2. WHOAMI (RFC4532, introduced in 2006)
  3. PASSWORD MODIFY EXTENDED (RFC3062, introduced in 2001)

This has nothing to do with pw_sha2 or any other LDAP plugin, it simply makes use of native LDAP operations the way they are intended to be used. WHOAMI has been supported, even without an RFC, since OpenLDAP 2.1.6 (released in 2001).

I'd be hard pressed to find an LDAP server which iRedMail makes use of that predates these operations. Please reconsider your decision to close @iredmail

iredmail commented 4 years ago

I think there're some misunderstanding.

francescocarzaniga commented 4 years ago

Function verify_bind_dn_pw() is used to verify all (iRedAdmin) logins, not just cn=vmailadmin.

Indeed, my changes maintain this behaviour unless something's wrong with my code.

For cn=vmailadmin, we always use SSHA, but for mail users, we store the password hashes/schemes supported by Dovecot (https://doc.dovecot.org/configuration_manual/authentication/password_schemes/), it may be not supported by OpenLDAP for direct bind and auth.

If somebody's using LDAP then it is reasonable to store hashes supported by LDAP. Dovecot does an authenticated bind as well by default configuration (auth_bind = yes), and so do all other iRedMail services. This change does not break anything as far as I can tell. In fact iredadmin is the first service I come across that does not support authenticated bind against LDAP, which is exactly what prompted me to make this change.

Your change also drops support for setting settings.STORE_PASSWORD_IN_PLAIN_TEXT

This is not true, if LDAP is set to store cleartext passwords this functionality is retained.

The current way of creating hashes client side treats LDAP as no more than database, offers no advantage with respect to said database, is markedly less secure for a variety of reasons, and ends up breaking functionality as well (example of argon2 on LDAP, which iredadmin is not capable of understanding at the moment, even though it is supported by Dovecot). All in all I think that password management should be left to LDAP, since it is what it was born to do.

I do not want to come across as adversarial, but I think that at least having the possibility of doing an authenticated bind is essential.

iredmail commented 4 years ago

Function verify_bind_dn_pw() is used to verify all (iRedAdmin) logins, not just cn=vmailadmin.

Indeed, my changes maintain this behaviour unless something's wrong with my code.

If somebody's using LDAP then it is reasonable to store hashes supported by LDAP. Dovecot does an authenticated bind as well by default configuration (auth_bind = yes), and so do all other iRedMail services. This change does not break anything as far as I can tell. In fact iredadmin is the first service I come across that does not support authenticated bind against LDAP, which is exactly what prompted me to make this change.

Your change also drops support for setting settings.STORE_PASSWORD_IN_PLAIN_TEXT

This is not true, if LDAP is set to store cleartext passwords this functionality is retained.

This is not ideal. storePasswordInPlainText is a parameter submitted in form, it's per-user setting while creating new user. It allows storing plain password for some user, but not all users (with settings.STORE_PASSWORD_IN_PLAIN_TEXT).

The current way of creating hashes client side treats LDAP as no more than database, offers no advantage with respect to said database, is markedly less secure for a variety of reasons, and ends up breaking functionality as well (example of argon2 on LDAP, which iredadmin is not capable of understanding at the moment, even though it is supported by Dovecot). All in all I think that password management should be left to LDAP, since it is what it was born to do.

I agree, but you're free to store the hash supported by both OpenLDAP and Dovecot for this purpose.

I do not want to come across as adversarial ...

No worries about this. I'm open to tech discussions. :)