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]:
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]:
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...
Revision #2, went back to using Slapi_DN's 0001-Ticket-458-RFE-Make-it-possible-for-privileges-to-be.patch
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
properly check the password admin dn 0001-Ticket-458-Need-to-properly-check-if-the-password-ad.patch
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
backport to 1.2.11 0001-Ticket-417-458-47522-Password-Administrator-Backport.patch
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1118007
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
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/458
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.