#402 unhashed#user#password in entry extension
Closed: wontfix None Opened 11 years ago by nhosoi.

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
/

}}}

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]:

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.

Sorry, I don't know why the file was reverted. I recreated the patch which has no change on ldaputil.c.

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?

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.

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.

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".

Does pw_copy_entry_ext() need to use the rwlock from the source and destination extensions?

Another good point! I added to call slapi_rwlock_wrlock to protect valuearray_add_valuearray.

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.

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.

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

7 years ago

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.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata