table-ldap: Strip {CRYPT} prefix from userPassword

[xwhatsit]

TL;DR - This allows easy use of the existing (and mandatory) userPassword LDAP field on OpenBSD using ldapd from base. Without something like this, I guess you would need an extra attribute for storing the hash "raw" without the prefix, which you'd need to keep in sync with the normal userPassword field.

Both OpenBSD's ldapd and OpenLDAP use RFC2307 passwords for the
userPassword field; these are either plaintext, or in the form of
"{scheme}hash". A scheme of CRYPT specifies the hash can be checked
using crypt(3). On OpenBSD at least, this will be in the same format as
OpenSMTPD's expected hash.

If the credentials lookup uses the well-known attribute userPassword to
retrieve the password hash, and the result is prefixed with {CRYPT},
remove this prefix to allow direct use of userPassword.

[poolpOrg]

sorry, I missed your pull request

the idea is ok and the diff looks ok but I haven't looked at this code in a while and I need to understand it again because I can decide if your diff is valid, most notably the bit that annoys me is if (strip_hash_prefix(q->attrs[1], res[1][0])) where I don't recall how q->attrs works and if you're guaranteed to have the password attribute in index 1

[xwhatsit]

On Sat, Apr 18 '20 at 20.59 NZST, Gilles Chehade wrote: sorry, I missed your pull request

No worries, I imagine this is a long way down the priority list :)

the idea is ok and the diff looks ok but I haven't looked at this code in a while and I need to understand it again because I can decide if your diff is valid, most notably the bit that annoys me is ```if (strip_hash_prefix(q->attrs[1], res[1][0]))``` where I don't recall how q->attrs works and if you're guaranteed to have the password attribute in index 1

I'd also forgotten the details of this. I believe `q->attrs` is guaranteed to have 2 members because of the `expect` parameter in `ldap_parse_attributes`. The order of course could be different if one chooses to enter e.g. `credentials_attributes userPassword,uid` instead of `uid,userPassword` in their config file, but this would also reorder the `res` array and given them a password:user result instead of user:password; this would break in the context of of a credentials table, unless I misunderstood. Maybe I've missed a more generic use of the credential table---in this case I can redo the pull request with a test against all indices?

[belzebubek]

Having this code in is a must. Whole LDAP authentication is broken without this!

[mgraves00]

Is it possible to get this change committed/merged to a new sub-point release? (6.7.2?)

[ichdasich]

IMHO, this change is not really a good idea; LDAP passwords may not only be prefixed with {CRYPT}; So, at the very least, the prefix to strip should be configurable in the table config (Mind, technically opensmtpd won't work with those schemes though; Still, the hardcoding feels iffy);

However, more generally, there are similar issues for, e.g., table-mysql depending on the tools used; For example, i noticed that Dovecot also kind-of prefers {BLF-CRYPT} prefixes when having a mixed password DB (needed this when doing the migration, i.e., when users did not have their password changed from {SSHA512} to {BLF-CRYPT} upon login).

Similarly, SOGo adds the wrong hash-prefix when updating a password ($2y$ vs $2b$) needing some additional mysql magic (see: https://doing-stupid-things.as59645.net/mail/nsfp/2022/04/14/send-it.html ).

So, it might make sense to have some more general password substitution control when reading passwords from a DB table.

[mgraves00]

I agree with the assessment that there can be multiple CRYPT/HASH schemes inside the curly brackets {}. I have a simpler diff that will "strip" the {scheme} off the password string. If interested I will submit a pull request for review.

[ichdasich]

Hm, not sure (also, keep in mind, i am no opensmtpd dev, i just randomly dropped here while debugging an opensmtpd and hit this issue before. ;-) )

What might be ideal would be stripping {CRYPT} and {BLF-CRYPT}, doing the $2y$/$2b$ replace and writing an 'unsupported password hash type' log message for everything != $2b$ and !({CRYPT}|{BLF-CRYPT}); That would probably be the diff I would try to write if i'd be less bad at coding. :-)

[omar-polo]

Hello,

just stripping {CRYPT} feels wrong. In the light of the new table protocol, there's now a mechanism to let tables handle the authentication. (well, technically the diff is still pending, but will be committed very soon.) With that, it will be possible for smtpd to offload the authentication to LDAP which will receive the plaintext password from the user and do the checking by itself. See this thread on tech@ https://marc.info/?l=openbsd-tech&m=171650123723389&w=2

of course, table-ldap will need some changes to take advantage of this new mechanism, but it seems better than just stripping {CRYPT}.

Also, this repository is about to be archived, as all the tables were moved to their own repositories, so I'm going to close this PR. I've opened OpenSMTPD/table-ldap#1 on the new table-ldap repo about this, and we can eventually move the discussion there.

Thanks,