unhashed#user#password in entry extension

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/402

• Created at 2012-06-29 03:44:51 by nhosoi (@nhosoi)
• Closed as Fixed
• Assigned to nhosoi (@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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-06-29 04:38:40

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-06-29 05:14:02

[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 */

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-02 23:31:15

git patch file (master)
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.2.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-02 23:32:43

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.

[389-ds-bot]

Comment from nkinder (@nkinder) at 2012-07-07 02:38:33

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.

[389-ds-bot]

Comment from nkinder (@nkinder) at 2012-07-07 03:03:32

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?

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-07 05:27:40

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-07 05:37:04

git patch file (master) - take 3 including autogen'ed files.
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.3.patch

[389-ds-bot]

Comment from nkinder (@nkinder) at 2012-07-11 00:27:56

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-11 03:27:17

git patch file (master) - take 4 including autogen'ed files.
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.4.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-11 03:33:12

git patch file (master) - take 4 including autogen'ed files.
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-11 03:39:48

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2012-07-11 03:55:52

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

[389-ds-bot]

Comment from nkinder (@nkinder) at 2012-08-28 04:14:44

Added initial screened field value.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2013-06-13 07:04:56

git patch file (master)
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.5.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2013-06-13 07:05:37

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2013-06-13 07:29:36

Reviewed by Nathan (Thank you!!)

Pushed to master: commit c4667c0

Pushed to 389-ds-base-1.3.1: commit 539419f

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2017-02-11 23:09:14

Metadata Update from @nhosoi:

• Issue assigned to nhosoi
• Issue set to the milestone: 1.3.1.1