This fix is a hack. Ticket #378 (closed enhancement: fixed) unhashed#user#password field It treats unhashe#user#password specially all over the code, which is not preferable.
Opening a new ticket to enhance the unhashed password handling.
Fix description: This patch stashes unhashed password in the entry extension instead of the ordinary attribute value. The entry extension was implemented using the factory framework. Taking the advantage of the framework, the setter and getter functions (slapi_pw_set/get_entry_ext) are added.
[Example change for unhashed password users]
{{{ $ diff -twU4 ./daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_prepost.c /tmp/ipapwd_prepost.c --- ./daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_prepost.c 2012-06-28 14:54:57.809671891 -0700 +++ /tmp/ipapwd_prepost.c 2012-06-28 15:54:47.998631419 -0700 @@ -204,11 +204,26 @@ } slapi_ch_free_string(&userpw); userpw = tmp; } else if (slapi_is_encoded(userpw)) { + char userpw_clear = NULL; +#if 0 / check if we have access to the unhashed user password / - char userpw_clear = + userpw_clear = slapi_entry_attr_get_charptr(e, "unhashed#user#password"); +#else + Slapi_Value pwvals = NULL; + rc = slapi_pw_get_entry_ext(e, &pwvals); + if (LDAP_SUCCESS == rc) { + Slapi_ValueSet vset; + Slapi_Value value = NULL; + / pwvals is passed in to vset; + * thus no need to free vset nor userpw_clear. */ + valueset_set_valuearray_passin(&vset, pwvals); + slapi_valueset_first_value(&vset, &value); + userpw_clear = slapi_value_get_string(value); + } +#endif
/* unhashed#user#password doesn't always contain the clear text * password, therefore we need to check if its value isn't the same * as userPassword to make sure */
@@ -217,11 +232,11 @@ slapi_ch_free_string(&userpw); } else { userpw = slapi_ch_strdup(userpw_clear); } - +#if 0 slapi_ch_free_string(&userpw_clear); - +#endif if (rc != LDAP_SUCCESS) { / we don't have access to the clear text password; * let it slide if migration is enabled, but don't * generate kerberos keys /
}}}
git patch file (master) 0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.2.patch
Fix description: This patch adds the method to use entry extension to stash the unhashed password in addition to the existing one which uses the ordinary attribute.
It introduces the definition "USE_OLD_UNHASHED" in configure.ac to keep the old method to use the attribute.
Once all the plugins' migration is done, the old method can be disabled by removing the definition. We could also remove the code in "#if defined(USE_OLD_UNHASHED)" then.
There appears to have been a problem merging your changes in ldap/servers/slapd/ldaputil.c with the patch for ticket #399. Your patch reverts the fix made to always get the bind result, which will cause a regression.
It looks odd that the slapi_entry_apply_mod_extension() and entry_apply_mod_wsi() functions expect SLAPI_PW_* specific extensions since these are generic functions. Is this a safe assumption?
If pw_entry_constructor() fails to allocate a lock, should we just return NULL without allocating and returning pw_extp? The slapi_rwlock_*() functions called by the extension get/set functions do check if the lock is NULL before attempting to use them, but this will behave with no protection from the lock.
Does pw_copy_entry_ext() need to use the rwlock from the source and destination extensions?
Replying to [comment:5 nkinder]:
Sorry, I don't know why the file was reverted. I recreated the patch which has no change on ldaputil.c.
Yes, you are right! I replaced SLAPI_PW_SET_ADD with SLAPI_EXT_SET_ADD and SLAPI_PW_SET_REPLACE with SLAPI_EXT_SET_REPLACE.
A good point. Thanks for pointing it out. I've modified constructor to return NULL when slapi_new_rwlock fails. If it happens, "struct slapi_pw_entry_ext" won't be allocated. And the following unhashed password set/get fails with ""pw_entry_extension is not set".
Another good point! I added to call slapi_rwlock_wrlock to protect valuearray_add_valuearray.
git patch file (master) - take 3 including autogen'ed files. 0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.3.patch
Does pw_copy_entry_ext() also need to obtain a reader lock on src_extp? I would think it should hold a reader lock before it attempts to copy src_extp->pw_entry_values.
Aside from that, the patch looks good.
git patch file (master) - take 4 including autogen'ed files. 0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.4.patch
git patch file (master) - take 4 including autogen'ed files. 0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.patch
Replying to [comment:8 nkinder]:
Does pw_copy_entry_ext() also need to obtain a reader lock on src_extp? I would think it should hold a reader lock before it attempts to copy src_extp->pw_entry_values. Aside from that, the patch looks good.
Thanks a lot, Nathan! I added the read lock and the new patch is attached to this ticket.
Reviewed by Nathan (Thank you!!!)
Pushed to master.
$ git merge trac402 Updating 6ba24eb..091c749 Fast-forward Makefile.in | 7 +- aclocal.m4 | 40 +- config.h.in | 9 + configure |18950 ++++++++------------ configure.ac | 1 + ldap/servers/plugins/acl/acleffectiverights.c | 10 +- ldap/servers/plugins/deref/deref.c | 5 +- .../plugins/replication/windows_protocol_util.c | 139 +- ldap/servers/slapd/add.c | 114 +- ldap/servers/slapd/attr.c | 4 + ldap/servers/slapd/entry.c | 181 +- ldap/servers/slapd/entrywsi.c | 47 +- ldap/servers/slapd/opshared.c | 2 + ldap/servers/slapd/pblock.c | 2 + ldap/servers/slapd/proto-slap.h | 3 +- ldap/servers/slapd/pw.c | 238 + ldap/servers/slapd/pw_mgmt.c | 10 +- ldap/servers/slapd/pw_retry.c | 2 +- ldap/servers/slapd/schema.c | 4 + ldap/servers/slapd/slap.h | 9 + ldap/servers/slapd/slapi-plugin.h | 88 + ldap/servers/slapd/slapi-private.h | 4 + ldap/servers/slapd/util.c | 32 + ldap/servers/slapd/valueset.c | 32 + ltmain.sh | 3968 +++-- 25 files changed, 10982 insertions(+), 12919 deletions(-)
$ git push Counting objects: 67, done. Delta compression using up to 4 threads. Compressing objects: 100% (33/33), done. Writing objects: 100% (34/34), 74.75 KiB, done. Total 34 (delta 31), reused 1 (delta 1) To ssh://git.fedorahosted.org/git/389/ds.git 6ba24eb..091c749 master -> master
Added initial screened field value.
git patch file (master) 0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.5.patch
Description: commit 091c749 had a logic flaw: entry_apply_mod_wsi checks whether modify candidate attribute is to be stored in an entry extension or not. If it is supposed to be in the entry extension, it removes the attribute from the entry attribute list (e_attrs), and put it into the entry extension. The steps have to be done under any condition, but entry_apply_mod_wsi used to check if the entry extension was configured properly and the attribute existed in the extension, first. If both were not satisfied, the attribute was not removed from the attribute list.
This patch eliminated the check and the attribute to be stored in the entry extension is always removed from the attribute list in the entry.
Reviewed by Nathan (Thank you!!)
Pushed to master: commit c4667c0
Pushed to 389-ds-base-1.3.1: commit 539419f
Metadata Update from @nhosoi: - Issue assigned to nhosoi - Issue set to the milestone: 1.3.1.1
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/402
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Log in to comment on this ticket.