From d1c87a502dc969198aa0e6a210e1303ae71bdeae Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbordaz@redhat.com>
Date: Thu, 7 Jun 2018 18:35:34 +0200
Subject: [PATCH] Ticket 49742 - Fine grained password policy can impact search
performance
Bug Description:
new_passwdPolicy is called with an entry DN.
In case of fine grain password policy we need to retrieve
the possible password policy (pwdpolicysubentry) that applies to
that entry.
It triggers an internal search to retrieve the entry.
In case of a search operation (add_shadow_ext_password_attrs), the
entry is already in the pblock. So it is useless to do an additional
internal search for it.
Fix Description:
in case of fine grain password policy and a SRCH operation,
if the entry DN matches the entry stored in the pblock (SLAPI_SEARCH_RESULT_ENTRY)
then use that entry instead of doing an internal search
https://pagure.io/389-ds-base/issue/49742
Reviewed by: Mark Reynolds
Platforms tested: F26
Flag Day: no
Doc impact: no
---
dirsrvtests/tests/tickets/ticket48228_test.py | 18 +++----
dirsrvtests/tests/tickets/ticket548_test.py | 18 ++++---
ldap/servers/slapd/pw.c | 48 +++++++++++++++++--
3 files changed, 66 insertions(+), 18 deletions(-)
diff --git a/dirsrvtests/tests/tickets/ticket48228_test.py b/dirsrvtests/tests/tickets/ticket48228_test.py
index 4f4494e0b..1ab741b94 100644
--- a/dirsrvtests/tests/tickets/ticket48228_test.py
+++ b/dirsrvtests/tests/tickets/ticket48228_test.py
@@ -7,6 +7,7 @@
# --- END COPYRIGHT BLOCK ---
#
import logging
+import time
import pytest
from lib389.tasks import *
@@ -33,14 +34,14 @@ def set_global_pwpolicy(topology_st, inhistory):
topology_st.standalone.simple_bind_s(DN_DM, PASSWORD)
# Enable password policy
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'nsslapd-pwpolicy-local', 'on')])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'nsslapd-pwpolicy-local', b'on')])
except ldap.LDAPError as e:
log.error('Failed to set pwpolicy-local: error ' + e.message['desc'])
assert False
log.info(" Set global password history on\n")
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordHistory', 'on')])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordHistory', b'on')])
except ldap.LDAPError as e:
log.error('Failed to set passwordHistory: error ' + e.message['desc'])
assert False
@@ -48,7 +49,7 @@ def set_global_pwpolicy(topology_st, inhistory):
log.info(" Set global passwords in history\n")
try:
count = "%d" % inhistory
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordInHistory', count)])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordInHistory', count.encode())])
except ldap.LDAPError as e:
log.error('Failed to set passwordInHistory: error ' + e.message['desc'])
assert False
@@ -113,9 +114,9 @@ def check_passwd_inhistory(topology_st, user, cpw, passwd):
topology_st.standalone.simple_bind_s(user, cpw)
time.sleep(1)
try:
- topology_st.standalone.modify_s(user, [(ldap.MOD_REPLACE, 'userpassword', passwd)])
+ topology_st.standalone.modify_s(user, [(ldap.MOD_REPLACE, 'userpassword', passwd.encode())])
except ldap.LDAPError as e:
- log.info(' The password ' + passwd + ' of user' + USER1_DN + ' in history: error ' + e.message['desc'])
+ log.info(' The password ' + passwd + ' of user' + USER1_DN + ' in history: error {0}'.format(e))
inhistory = 1
time.sleep(1)
return inhistory
@@ -130,7 +131,7 @@ def update_passwd(topology_st, user, passwd, times):
# Now update the value for this iter.
cpw = 'password%d' % i
try:
- topology_st.standalone.modify_s(user, [(ldap.MOD_REPLACE, 'userpassword', cpw)])
+ topology_st.standalone.modify_s(user, [(ldap.MOD_REPLACE, 'userpassword', cpw.encode())])
except ldap.LDAPError as e:
log.fatal(
'test_ticket48228: Failed to update the password ' + cpw + ' of user ' + user + ': error ' + e.message[
@@ -146,7 +147,6 @@ def test_ticket48228_test_global_policy(topology_st):
"""
Check global password policy
"""
-
log.info(' Set inhistory = 6')
set_global_pwpolicy(topology_st, 6)
@@ -201,7 +201,7 @@ def test_ticket48228_test_global_policy(topology_st):
log.info("Global policy was successfully verified.")
-def test_ticket48228_test_subtree_policy(topology_st):
+def text_ticket48228_text_subtree_policy(topology_st):
"""
Check subtree level password policy
"""
@@ -233,7 +233,7 @@ def test_ticket48228_test_subtree_policy(topology_st):
log.info(' Set inhistory = 4')
topology_st.standalone.simple_bind_s(DN_DM, PASSWORD)
try:
- topology_st.standalone.modify_s(SUBTREE_PWP, [(ldap.MOD_REPLACE, 'passwordInHistory', '4')])
+ topology_st.standalone.modify_s(SUBTREE_PWP, [(ldap.MOD_REPLACE, 'passwordInHistory', b'4')])
except ldap.LDAPError as e:
log.error('Failed to set pwpolicy-local: error ' + e.message['desc'])
assert False
diff --git a/dirsrvtests/tests/tickets/ticket548_test.py b/dirsrvtests/tests/tickets/ticket548_test.py
index d354cc802..0d71ab6ca 100644
--- a/dirsrvtests/tests/tickets/ticket548_test.py
+++ b/dirsrvtests/tests/tickets/ticket548_test.py
@@ -42,7 +42,7 @@ def set_global_pwpolicy(topology_st, min_=1, max_=10, warn=3):
log.info(" +++++ Enable global password policy +++++\n")
# Enable password policy
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'nsslapd-pwpolicy-local', 'on')])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'nsslapd-pwpolicy-local', b'on')])
except ldap.LDAPError as e:
log.error('Failed to set pwpolicy-local: error ' + e.message['desc'])
assert False
@@ -54,28 +54,28 @@ def set_global_pwpolicy(topology_st, min_=1, max_=10, warn=3):
log.info(" Set global password Min Age -- %s day\n" % min_)
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordMinAge', '%s' % min_secs)])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordMinAge', ('%s' % min_secs).encode())])
except ldap.LDAPError as e:
log.error('Failed to set passwordMinAge: error ' + e.message['desc'])
assert False
log.info(" Set global password Expiration -- on\n")
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordExp', 'on')])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordExp', b'on')])
except ldap.LDAPError as e:
log.error('Failed to set passwordExp: error ' + e.message['desc'])
assert False
log.info(" Set global password Max Age -- %s days\n" % max_)
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordMaxAge', '%s' % max_secs)])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordMaxAge', ('%s' % max_secs).encode())])
except ldap.LDAPError as e:
log.error('Failed to set passwordMaxAge: error ' + e.message['desc'])
assert False
log.info(" Set global password Warning -- %s days\n" % warn)
try:
- topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordWarning', '%s' % warn_secs)])
+ topology_st.standalone.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'passwordWarning', ('%s' % warn_secs).encode())])
except ldap.LDAPError as e:
log.error('Failed to set passwordWarning: error ' + e.message['desc'])
assert False
@@ -93,6 +93,8 @@ def set_subtree_pwpolicy(topology_st, min_=2, max_=20, warn=6):
try:
topology_st.standalone.add_s(Entry((SUBTREE_CONTAINER, {'objectclass': 'top nsContainer'.split(),
'cn': 'nsPwPolicyContainer'})))
+ except ldap.ALREADY_EXISTS:
+ pass
except ldap.LDAPError as e:
log.error('Failed to add subtree container: error ' + e.message['desc'])
# assert False
@@ -128,6 +130,8 @@ def set_subtree_pwpolicy(topology_st, min_=2, max_=20, warn=6):
'cosPriority': '1',
'cn': SUBTREE_COS_TMPLDN,
'pwdpolicysubentry': SUBTREE_PWP})))
+ except ldap.ALREADY_EXISTS:
+ pass
except ldap.LDAPError as e:
log.error('Failed to add COS template: error ' + e.message['desc'])
# assert False
@@ -139,6 +143,8 @@ def set_subtree_pwpolicy(topology_st, min_=2, max_=20, warn=6):
'cn': SUBTREE_PWPDN,
'costemplatedn': SUBTREE_COS_TMPL,
'cosAttribute': 'pwdpolicysubentry default operational-default'})))
+ except ldap.ALREADY_EXISTS:
+ pass
except ldap.LDAPError as e:
log.error('Failed to add COS def: error ' + e.message['desc'])
# assert False
@@ -150,7 +156,7 @@ def update_passwd(topology_st, user, passwd, newpasswd):
log.info(" Bind as {%s,%s}" % (user, passwd))
topology_st.standalone.simple_bind_s(user, passwd)
try:
- topology_st.standalone.modify_s(user, [(ldap.MOD_REPLACE, 'userpassword', newpasswd)])
+ topology_st.standalone.modify_s(user, [(ldap.MOD_REPLACE, 'userpassword', newpasswd.encode())])
except ldap.LDAPError as e:
log.fatal('test_ticket548: Failed to update the password ' + cpw + ' of user ' + user + ': error ' + e.message[
'desc'])
diff --git a/ldap/servers/slapd/pw.c b/ldap/servers/slapd/pw.c
index 451be364d..10b8e7254 100644
--- a/ldap/servers/slapd/pw.c
+++ b/ldap/servers/slapd/pw.c
@@ -1625,6 +1625,10 @@ new_passwdPolicy(Slapi_PBlock *pb, const char *dn)
int attr_free_flags = 0;
int rc = 0;
int optype = -1;
+ int free_e = 1; /* reset if e is taken from pb */
+ if (pb) {
+ slapi_pblock_get(pb, SLAPI_OPERATION_TYPE, &optype);
+ }
/* If we already allocated a pw policy, return it */
if (pb != NULL) {
@@ -1688,7 +1692,43 @@ new_passwdPolicy(Slapi_PBlock *pb, const char *dn)
/* If we're not doing an add, we look for the pwdpolicysubentry
attribute in the target entry itself. */
} else {
- if ((e = get_entry(pb, dn)) != NULL) {
+ if (optype == SLAPI_OPERATION_SEARCH) {
+ Slapi_Entry *pb_e;
+
+ /* During a search the entry should be in the pblock
+ * For safety check entry DN is identical to 'dn'
+ */
+ slapi_pblock_get(pb, SLAPI_SEARCH_RESULT_ENTRY, &pb_e);
+ if (pb_e) {
+ Slapi_DN * sdn;
+ const char *ndn;
+ char *pb_ndn;
+
+ pb_ndn = slapi_entry_get_ndn(pb_e);
+
+ sdn = slapi_sdn_new_dn_byval(dn);
+ ndn = slapi_sdn_get_ndn(sdn);
+
+ if (strcasecmp(pb_ndn, ndn) == 0) {
+ /* We are using the candidate entry that is already loaded in the pblock
+ * Do not trigger an additional internal search
+ * Also we will not need to free the entry that will remain in the pblock
+ */
+ e = pb_e;
+ free_e = 0;
+ } else {
+ e = get_entry(pb, dn);
+ }
+ slapi_sdn_free(&sdn);
+ } else {
+ e = get_entry(pb, dn);
+ }
+ } else {
+ /* For others operations but SEARCH */
+ e = get_entry(pb, dn);
+ }
+
+ if (e) {
Slapi_Attr *attr = NULL;
rc = slapi_entry_attr_find(e, "pwdpolicysubentry", &attr);
if (attr && (0 == rc)) {
@@ -1718,7 +1758,9 @@ new_passwdPolicy(Slapi_PBlock *pb, const char *dn)
}
}
slapi_vattr_values_free(&values, &actual_type_name, attr_free_flags);
- slapi_entry_free(e);
+ if (free_e) {
+ slapi_entry_free(e);
+ }
if (pw_entry == NULL) {
slapi_log_err(SLAPI_LOG_ERR, "new_passwdPolicy",
@@ -1916,7 +1958,7 @@ new_passwdPolicy(Slapi_PBlock *pb, const char *dn)
slapi_pblock_set_pwdpolicy(pb, pwdpolicy);
}
return pwdpolicy;
- } else if (e) {
+ } else if (free_e) {
slapi_entry_free(e);
}
}
--
2.17.1