Enable changing passwords in user_ldap #600
Conversation
|
Great stuff! 😃 Can you update the license header of the new files you added to this scheme https://docs.nextcloud.com/server/9/developer_manual/general/codingguidelines.html#license-headers Thanks! |
|
Could I ask you to remove the 6 merge commits. Those clutter the history. A rebase on our master is a lot cleaner (git history wise). That would be awesome :) |
|
The setPassword method lacks encryption. In addition, please check if the ldap entry is a sambaSamAccount and change that password accordingly if present. |
Password hashing/encryption had been considered but won't be in it because
|
|
@schiessle : as mentioned in #519 it's applied @MorrisJobke : as discussed, we found a way Alright, unit tests are committed 😊 , @blizzz would be great if you could have a first look when convenient 😅 |
|
The ldap_mod_replace function will set the password as plaintext by default. AFAIK the ldap passwd verb isn't supported in PHP, so you need to encrypt yourself. If some ldap installations need plaintext, please make it configurable. |
Please have a look at the second point 2. it can actually be done by the LDAP server itself This means that the LDAP server can be configured to use password storage schemes like SSHA, CRYPT etc. independently from the client. The client sends the plain text password and the server handles hashing by its own. Therefore it is generally not required for the client to hash passwords. If you have different requirements, it is suggested you come over to IRC and discuss it before opening a new PR. |
apps/user_ldap/lib/User_LDAP.php
Outdated
| * @param string $uid The username | ||
| * @param string $password The new password | ||
| * @return bool | ||
| * |
|
Thanks @LukasReschke for commenting, applied according. |
|
The ldap server can do the password hashing itself if the correct api is used, but only if the server is requested to do so. ldapmodify does NOT trigger this (modifyRequest), you need to use ldappasswd instead (extendedRequest with modifyPasswordOID, implementing RFC3062). You're currently using the wrong ldap call, bypassing the server password procedures. |
Besides the fact that exops like ldappasswd are not supported by PHP, ldapmodify is the correct way to go. Just a hint for you: google for "ppolicy_hash_cleartext". Btw, this PR has been tested with password hashing enabled on the LDAP server and works as expected. |
|
ppolicy is still marked experimental, and certainly only installed on a minority of ldap servers in use. Yes, sadly RFC3062 isn't supported in PHP. For extended password handling on the server side, modify is obviously NOT the recommended way. There's a reason for ldappasswd being a dedicated tool. |
This is not a discussion about ppolicy. This PR neither relies on it nor does it need it. But besides, even in older OpenLDAP 2.3 docs it states clearly that ppolicy "is production quality and considered stable enough for use in production environments".
LDAP passwords have always been plaintext in existing installations. And for the reasons lined out above, there will be no need to hash them. |
Since when have LDAP passwords been plaintext? A least for the installations I run, they are all hashed. As far as I know Microsoft uses some kind of encryption/hashes for AD. |
|
slapd has password-hash or OlcPasswordHash, but this is effective only if ldap_password is used. |
The context is about nextCloud which acts as the LDAP client. Existing nextCloud installations send plaintext passwords to the LDAP server for binding. It is up to the LDAP server to determine how passwords are stored. |
|
Binding and storing are non-related. Binding password may be transferred plaintext (hopefully secured by TLS), but advising the server to store plaintext passwords is outdated. Yes, may the server decide how to encrypt, see RFC3062. You are effectively bypassing default encryption mechanisms, so you' re responsible. |
Exactly, and it should stay like that! If you introduced client side hashing, then the binding would also become effected, as it would have to use the same hashing mechanism.
Don't know where you got that advice from, in production environments it's usually not recommended.
Hey, stay cool. Maybe you should educate yourself about why client side hashing is basically senseless, http://security.stackexchange.com/questions/53594/why-is-client-side-hashing-of-a-password-so-uncommon would be start. There are rare cases where it would make sense to hash passwords both at client and server sides, but that is utterly out of scope here. |
|
The server will happily bind with the non-encrypted password against any known hashed password. We're talking about STORING an unencrypted password only, which happens by your implementation. |
To repeat it for the third and last time: Not the client determines how passwords are stored, but the server. This PR is based on this assumption. If you have different requirements, then discuss it in IRC and/or try to open a PR by your own. |
|
... and you're BYPASSING server side encryption using ldap_modify. Read about ldap_password. |
|
My late feedback:
|
|
Not viable to require to reconfigure probably >90% of ldap servers around, especially if it requires a module that says "experimental not for production use" as of openldap 2.4.44 Requiring transport encryption isn't solving anything and is even misleading; this is about STORING unencrypted passwords unintentionally. Make hash algorithm configurable including PLAINTEXT for those exotic ppolicy users, everything solved. Hashing doesn't really hurt, does it? Apparently even the password attribute name must be configurable, e.g. for ActiveDirectory. |
Proof?
Nonsense, read http://www.openldap.org/faq/data/cache/1204.html . And again, not a discussion of thirdparty products.
Nope, changing passwords is disabled by default. Enabling it comes with intentions.
Feel free to implement it in an enhancing PR.
Logic to handle the default password attribute unicodePwd for Active Directory is in the works and will be added to this PR. |
|
ppolicy isn't active in debian by default, neither in freebsd, and afaics not when installing from sources, so the >90% is just a guess. If offering pwd change via nextcloud, it's only pragmatic to use nextcloud's password policy as well. Installing a server side module (if you can so at at all) to save some 50 lines of code is quite invasive. Password changing isn't defined in base LDAPV3, intentionally left to the server implementation, so offering password change is 3rd party specific anyway. |
GitHubUser4234
force-pushed
the
ldap_password_pr
branch
2 times, most recently
from
bd5a04d to
a6220d8
Compare
GitHubUser4234
force-pushed
the
ldap_password_pr
branch
from
a6220d8 to
f4dfd1f
Compare
|
@blizzz : the source code has been rebased and is ready for review, thanks a lot 😅 |
GitHubUser4234
force-pushed
the
ldap_password_pr
branch
14 times, most recently
from
a15f972 to
28fa495
Compare
blizzz
left a comment
blizzz
left a comment
There was a problem hiding this comment.
- Cannot enable the password change feature. The checkmark gets set on the checkbox, but it is never really saved. No save request to the server is sent.
| * @param LDAP $link | ||
| * @param string $userDN | ||
| * @param string $password | ||
| * @return bool |
There was a problem hiding this comment.
LDAP and (ILDAPWrapper) are supposed to just wrap around native PHP LDAP methods to have some better testing and logging abilities. To follow the same style, please call this method modReplace. Keep the logic within the calling method.
There was a problem hiding this comment.
Oh, the method names are related to the native PHP LDAP methods, now that you say it I'm seeing it, too 😆
apps/user_ldap/lib/Access.php
Outdated
| \OCP\Util::writeLog('user_ldap', | ||
| 'No LDAP Connector assigned, access impossible for setPassword.', | ||
| \OCP\Util::WARN); | ||
| return false; |
There was a problem hiding this comment.
Here and on the next return: do we really want to return false, or better throw an exception?
When a method of the ldap wrapper is called and the server is not available, an exception is thrown anyway. So, when dealing with Exceptions the latter check can be omitted. Dealing with exceptions also allows more appropriate error reporting, because when returning false, we cannot tell the user what happened (especially since latter log statement only appears on debug – the wrapper also takes care about writing log on missing LDAP links).
Please also do a check on whether the config switch has been set to allow password changes, especially since this is a public method. Bypassing the config should not be allowed so easily.
There was a problem hiding this comment.
Oh, I just copied that approach from the readAttribute method. If I understood correctly, I should instead replace the check with an "allow password changes check" which throws an exception.
| \OCP\Util::DEBUG); | ||
| if ($errorCode === 19) { // constraint violation | ||
| ldap_get_option($this->curArgs[0], LDAP_OPT_ERROR_STRING, $extended_error); | ||
| throw new HintException(!empty($extended_error)?$extended_error:$errorMsg, '', $errorCode); |
There was a problem hiding this comment.
HintExceptions will be shown directly to the end user, without any other information. Just passing a technical string here is confusing. Use a generic \Exception or \Error to gain proper output (which additional debug info if debug mode is enabled).
(cf. https://github.com/nextcloud/server/blob/master/index.php)
There was a problem hiding this comment.
Uhm, yes, the intention is to display the error message returned by the password policy module / AD straight to the user. For example it would say
Maybe this needs further discussion in IRC.
apps/user_ldap/lib/User_LDAP.php
Outdated
| try { | ||
| $ldapRecord = $this->getLDAPUserByLoginName($uid); | ||
| } catch(\Exception $e) { | ||
| \OC::$server->getLogger()->logException($e, ['app' => 'user_ldap']); |
There was a problem hiding this comment.
also here (and corresponding lines below), is returning false instead of Exception handling really desired?
There was a problem hiding this comment.
Also just copied from the checkPasswordmethod 😆 Ok, so if understood correctly, I just remove the try&catch and throw an exception in corresponding lines below.
There was a problem hiding this comment.
Yes. Per se it is correct to catch an Exception at some point, however I think it should not happen in this part. We learned from past the methods returning false or null in error cases are very bad to handle and haunt us at night 👻
There was a problem hiding this comment.
Hehe, I understand and the icon describes it well 😆
| '. Maybe the LDAP entry has no set display name attribute?'); | ||
| return false; | ||
| } | ||
| if($user->getUsername() !== false) { |
There was a problem hiding this comment.
$user->getDN() should be cheaper
| $access = $this->getAccessMock(); | ||
|
|
||
| $this->prepareAccessForSetPassword($access); | ||
| $backend = new UserLDAP($access, $this->getMock('\OCP\IConfig')); |
There was a problem hiding this comment.
getMock is deprecated, replace with createMock, please. Also, provide the argument as IConfig::class instead of a plain string. This makes it easier to find usages of that interface, do semi-automated changes, etc.
| $access = $this->getAccessMock(); | ||
|
|
||
| $this->prepareAccessForSetPassword($access); | ||
| $backend = new UserLDAP($access, $this->getMock('\OCP\IConfig')); |
| $backend = new UserLDAP($access, $this->getMock('\OCP\IConfig')); | ||
| \OC_User::useBackend($backend); | ||
|
|
||
| $result = $backend->setPassword('roland', 'dt'); |
There was a problem hiding this comment.
you can omit $result when it is unused.
| \OC_User::useBackend($backend); | ||
|
|
||
| $result = $backend->setPassword('roland', 'dt12234$'); | ||
| } |
There was a problem hiding this comment.
It'd also like to see a unit test against a case when setting the password is disabled.
| <p><label for="ldap_dynamic_group_member_url"><?php p($l->t('Dynamic Group Member URL'));?></label><input type="text" id="ldap_dynamic_group_member_url" name="ldap_dynamic_group_member_url" title="<?php p($l->t('The LDAP attribute that on group objects contains an LDAP search URL that determines what objects belong to the group. (An empty setting disables dynamic group membership functionality.)'));?>" data-default="<?php p($_['ldap_dynamic_group_member_url_default']); ?>" /></p> | ||
| <p><label for="ldap_nested_groups"><?php p($l->t('Nested Groups'));?></label><input type="checkbox" id="ldap_nested_groups" name="ldap_nested_groups" value="1" data-default="<?php p($_['ldap_nested_groups_default']); ?>" title="<?php p($l->t('When switched on, groups that contain groups are supported. (Only works if the group member attribute contains DNs.)'));?>" /></p> | ||
| <p><label for="ldap_paging_size"><?php p($l->t('Paging chunksize'));?></label><input type="number" id="ldap_paging_size" name="ldap_paging_size" title="<?php p($l->t('Chunksize used for paged LDAP searches that may return bulky results like user or group enumeration. (Setting it 0 disables paged LDAP searches in those situations.)'));?>" data-default="<?php p($_['ldap_paging_size_default']); ?>" /></p> | ||
| <p><label for="ldap_turn_on_pwd_change"><?php p($l->t('Enable LDAP password changes per user'));?></label><input type="checkbox" id="ldap_turn_on_pwd_change" name="ldap_turn_on_pwd_change" value="1" data-default="<?php p($_['ldap_turn_on_pwd_change_default']); ?>" title="<?php p($l->t('Allow LDAP users to change their password and allow Super Administrators and Group Administrators to change the password of their LDAP users. Only works when access control policies are configured accordingly on the LDAP server. As passwords are sent in plaintext to the LDAP server, transport encryption must be used and password hashing should be configured on the LDAP server.'));?>" /><br/></p> |
There was a problem hiding this comment.
I wonder whether we should have a brief attention message right to the checkbox. "New password is sent as plain text to LDAP" for instance. What do you think?
There was a problem hiding this comment.
Sure, that's fine.
|
Oh, and please do a commit with signing off (-s flag in git commit) so that the CI is happy. Needs a rebase, too. |
Uhm, could you please try to clear the browser cache (to refresh files like js)? I just retried and it's still working here...otherwise this might require some discussion in IRC to debug further 😜 |
GitHubUser4234
force-pushed
the
ldap_password_pr
branch
from
8c8c064 to
9ef6a12
Compare
|
ℹ️ Continuation on #1715 for reasons. |
…rn/pbkdf2-3.1.3 chore(deps-dev): Bump pbkdf2 from 3.1.2 to 3.1.3
9 participants
This is based on #519, it has been discussed at #25114 and covers the "Ability to change LDAP password" part that is described there. Password policy related errors should be able to be displayed for both LDAP and AD users. The committed source code is fully functional in this regard. Unit test will be added tomorrow.