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