#458 [RFE] Make it possible for privileges to be provided to an admin user to import an LDIF file containing hashed passwords
Closed: Fixed None Opened 6 years ago by rmeggins.

https://bugzilla.redhat.com/show_bug.cgi?id=856577 (Red Hat Directory Server)

3. What is the nature and description of the request?

For automated password reset procedures the hashed password has to be imported
via ldif file into LDAP server. If password syntax check is enabled only the
cn=directory manager is allowed to do it. but due to permission separation the
import user should only have access to a certain tree in LDAP.


4. Why does the customer need this? (List the business requirements here)

To support the following scenario:
a) A password reset is triggered from another server.
b) No interactive access is granted to the support personnel. They have to
create an ldif file which is loaded into LDAP.
c) LDAP is also used for the authentication of non-user sessions and all
changes have to be done via ldif files.
d) Due to security restrictions only hashed passwords are allowed in ldif
files.


5. How would the customer like to achieve this? (List the functional
requirements here)

Add a special role for the admin user of LDAP subtree which allows them to
bypass the hashed password check when loading an ldif file with hashed
passwords.


6. For each functional requirement listed in question 5, specify how Red Hat
and the customer can test to confirm the requirement is successfully
implemented.

a) setup LDAP subtree
b) create admin user
c) activate password sytax check on subtree
d) import ldif file with hashed passord by admin user -> should fail
e) add special role to admin user
d) import ldif file with hashed passord by admin user -> should work


7. Is there already an existing RFE upstream or in Red Hat bugzilla?

Bug 843576, "RFE - forcing passwordmustchange attribute by non-cn=directory
manager" / upstream <https://fedorahosted.org/389/ticket/417> is in a closely
related are; perhaps this RFE can be merged with it.


8. Does the customer have any specific timeline dependencies?

No.


9. Is the sales team involved in this request and do they have any additional
input?

The sales team is actively involved in this account, but not in this request
specifically.


10. List any affected packages or components.

389-ds-base


11. Would the customer be able to assist in testing this functionality if
implemented?

Yes.

Hi Mark, your design and implementation looks nice! I have a couple of requests...

{{{
ldap/servers/slapd/entry.c
2738 slapi_entry_attr_get_charray_ext( const Slapi_Entry e, const char type, int numVals)
2761
numVals = count;
}}}
It may be safer to check if numVals is not NULL, then assign count...
{{{
ldap/servers/slapd/pw.c
1509 pw_is_pwp_admin(Slapi_PBlock pb, passwdPolicy pwp){
1523 bind_sdn = slapi_sdn_new_dn_byval(bind_dn);
}}}
1) bind_sdn is leaking?
2) if bind_sdn is just for slapi_sdn_compare, passin is good enough? (you could eliminate one malloc...)
3) SLAPI_REQUESTOR_DN returns a normalized DN. So, instead of new_dn
, you could use new_normdn_... Combining (2) and (3), you could use slapi_sdn_new_normdn_passin, which skips malloc of "bind_dn" and dn_normalization...
{{{
1534 pw_get_admin_users(passwdPolicy *pwp)
1573 member_vals = slapi_entry_attr_get_charray_ext(entries[0], "member", &member_count);
}}}
I'm afraid member_vals could be also leaking, too...
I guess you may want to run valgrind to see how they are processed...

Thanks Noriko, I missed those leaks. So I think instead of changing to the sdn passin stuff, I'm going to store the normalized dn strings instead. There will just be a lot less processing and mallocing.

Replying to [comment:7 mreynolds]:

Thanks Noriko, I missed those leaks. So I think instead of changing to the sdn passin stuff, I'm going to store the normalized dn strings instead. There will just be a lot less processing and mallocing.

Sounds like a good plan! Thanks!

I think it would be better if pw_admin_user was Slapi_DN instead of char , then you could also use

    slapi_pblock_get(pb, SLAPI_REQUESTOR_SDN, &bind_sdn);

and use slapi_sdn_compare.

Replying to [comment:9 rmeggins]:

I think it would be better if pw_admin_user was Slapi_DN instead of char , then you could also use

    slapi_pblock_get(pb, SLAPI_REQUESTOR_SDN, &bind_sdn);

and use slapi_sdn_compare.

Ugh, that's what I originally had(with a few leaks), and then I rewrote the whole thing to use a char array. Anyway, no problem I'll re-work it.

The latest patch does pass the valgrind tests, sending out for review...

Hmmm, I thought there are 2 minor issues (but I could be wrong). uniquemember_vals[i] and member_vals[i] are consumed in slapi_sdn_new_dn_passin. But, we still need to free uniquemember_vals and member_vals by slapi_ch_free (note: not by charray_free)...
{{{
1572 uniquemember_vals = slapi_entry_attr_get_charray_ext(entries[0], "uniquemember", &uniquemember_count);
1573 member_vals = slapi_entry_attr_get_charray_ext(entries[0], "member", &member_count);
1574 pwp->pw_admin_user = (Slapi_DN *)slapi_ch_calloc((uniquemember_count + member_count + 1), sizeof(Slapi_DN ));
1575 if(uniquemember_count > 0){
1576 for(i = 0; i < uniquemember_count; i++){
1577 pwp->pw_admin_user[count++] = slapi_sdn_new_dn_passin(uniquemember_vals[i]);
1578 }
1579 }
1580 if(member_count > 0){
1581 for(i = 0; i < member_count; i++){
1582 pwp->pw_admin_user[count++] = slapi_sdn_new_dn_passin(member_vals[i]);
1583 }
}}}

Replying to [comment:12 nhosoi]:

Hmmm, I thought there are 2 minor issues (but I could be wrong). uniquemember_vals[i] and member_vals[i] are consumed in slapi_sdn_new_dn_passin. But, we still need to free uniquemember_vals and member_vals by slapi_ch_free (note: not by charray_free)...
{{{
1572 uniquemember_vals = slapi_entry_attr_get_charray_ext(entries[0], "uniquemember", &uniquemember_count);
1573 member_vals = slapi_entry_attr_get_charray_ext(entries[0], "member", &member_count);
1574 pwp->pw_admin_user = (Slapi_DN *)slapi_ch_calloc((uniquemember_count + member_count + 1), sizeof(Slapi_DN ));
1575 if(uniquemember_count > 0){
1576 for(i = 0; i < uniquemember_count; i++){
1577 pwp->pw_admin_user[count++] = slapi_sdn_new_dn_passin(uniquemember_vals[i]);
1578 }
1579 }
1580 if(member_count > 0){
1581 for(i = 0; i < member_count; i++){
1582 pwp->pw_admin_user[count++] = slapi_sdn_new_dn_passin(member_vals[i]);
1583 }
}}}

They are freed in delete_passwdPolicy(). Also valgrind did not complain about any leaks.

I doubled checked my valgrind tests - still no leaks reported. But I still freed the pointers. Valgrind did not complain about that either. New patch attached (revision 3)

Looks good - why do you need to cast the arguments to the slapi_sdn_free calls to (Slapi_DN) in delete_passwdPolicy? They should already be Slapi_DN - casting could hide other problems.

Looks good to me, too. Thanks for adding the free code. (And good to hear adding free did not introduce Invalid read/write. :)

I also think Rich is right about the cast.

I'm setting "ack" since you can push your patch with one line fix...

Thanks for the reviews - removed the castings in delete_passwdPolicy()

git merge ticket458
Updating 1d4f3ca..3ea0476
Fast-forward
ldap/schema/02common.ldif | 3 +-
ldap/servers/slapd/entry.c | 53 ++++++++++++-----
ldap/servers/slapd/libglobs.c | 20 +++++++
ldap/servers/slapd/modify.c | 22 +++-----
ldap/servers/slapd/pblock.c | 7 ++-
ldap/servers/slapd/proto-slap.h | 1 +
ldap/servers/slapd/pw.c | 113 ++++++++++++++++++++++++++++++++++++-
ldap/servers/slapd/pw.h | 1 +
ldap/servers/slapd/slap.h | 3 +
ldap/servers/slapd/slapi-plugin.h | 31 ++++++++++
10 files changed, 219 insertions(+), 35 deletions(-)

git push origin master
Counting objects: 31, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (16/16), done.
Writing objects: 100% (16/16), 3.82 KiB, done.
Total 16 (delta 14), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
1d4f3ca..3ea0476 master -> master

git merge ticket458
Updating 4c4cb3b..ab46f87
Fast-forward
ldap/servers/slapd/pw.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

git push origin master
Counting objects: 11, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (6/6), done.
Writing objects: 100% (6/6), 824 bytes, done.
Total 6 (delta 4), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
4c4cb3b..ab46f87 master -> master

Bug description: coverity reported "13160 - Resource leak"

Fix description: generate pblock after checking NULl check for binddn.

Pushed to master: commit a3c14c2 (Ticket 458)

Backported tickets 417, 458, 47522 to 1.2.11:

git merge passwordAdmin
Updating 8252d02..096d895
Fast-forward
ldap/schema/02common.ldif | 3 +-
ldap/servers/slapd/entry.c | 50 +++++++++++----
ldap/servers/slapd/libglobs.c | 24 ++++++-
ldap/servers/slapd/modify.c | 25 +++++---
ldap/servers/slapd/pblock.c | 6 ++
ldap/servers/slapd/proto-slap.h | 1 +
ldap/servers/slapd/pw.c | 123 +++++++++++++++++++++++++++++++++++--
ldap/servers/slapd/pw.h | 1 +
ldap/servers/slapd/slap.h | 3 +
ldap/servers/slapd/slapi-plugin.h | 32 ++++++++++
10 files changed, 236 insertions(+), 32 deletions(-)

git push origin 389-ds-base-1.2.11
Counting objects: 31, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (16/16), done.
Writing objects: 100% (16/16), 3.75 KiB, done.
Total 16 (delta 14), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
8252d02..096d895 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

commit 096d895
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Mar 5 15:55:31 2014 -0500

Unable to remove password administrators from config, error 53, reopening ticket

Going to track this issue in ticket 47900

Metadata Update from @mreynolds:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.3.1

2 years ago

Login to comment on this ticket.

Metadata