389-ds-base

Clone
Source Code
GIT

#51196 Issue 51192 - Add option to reject internal unindexed searches
Closed 3 years ago by spichugi. Opened 3 years ago by mreynolds.
mreynolds/389-ds-base issue51192  into  master

file modified
+95 -2 
@@ -1,5 +1,5 @@ 
 

  # --- BEGIN COPYRIGHT BLOCK ---
 

- # Copyright (C) 2016 Red Hat, Inc.
 

+ # Copyright (C) 2020 Red Hat, Inc.
 

  # All rights reserved.
 

  #
 

  # License: GPL (version 3 or any later version).
 
@@ -12,13 +12,15 @@ 
 

  from lib389.tasks import *
 

  from lib389.topologies import topology_m2, topology_st as topo
 

  from lib389.utils import *
 

- from lib389._constants import DN_CONFIG, DEFAULT_SUFFIX
 

+ from lib389._constants import DN_CONFIG, DEFAULT_SUFFIX, DEFAULT_BENAME
 

  from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES
 

+ from lib389.idm.group import Groups
 

  from lib389.backend import *
 

  from lib389.config import LDBMConfig, BDB_LDBMConfig
 

  from lib389.cos import CosPointerDefinitions, CosTemplates
 

  from lib389.backend import Backends
 

  from lib389.monitor import MonitorLDBM
 

+ from lib389.plugins import ReferentialIntegrityPlugin
 

  

  pytestmark = pytest.mark.tier0
 

  
@@ -458,6 +460,97 @@ 
 

          topo.standalone.config.set('nsslapd-ndn-cache-max-size', 'invalid_value')
 

  

  

+ def test_require_index(topo):
 

+     """Test nsslapd-ignore-virtual-attrs configuration attribute
 

+ 

+     :id: fb6e31f2-acc2-4e75-a195-5c356faeb803
 

+     :setup: Standalone instance
 

+     :steps:
 

+         1. Set "nsslapd-require-index" to "on"
 

+         2. Test an unindexed search is rejected
 

+     :expectedresults:
 

+         1. Success
 

+         2. Success
 

+     """
 

+ 

+     # Set the config
 

+     be_insts = Backends(topo.standalone).list()
 

+     for be in be_insts:
 

+         if be.get_attr_val_utf8_l('nsslapd-suffix') == DEFAULT_SUFFIX:
 

+             be.set('nsslapd-require-index', 'on')
 

+ 

+     db_cfg = DatabaseConfig(topo.standalone)
 

+     db_cfg.set([('nsslapd-idlistscanlimit', '100')])
 

+ 

+     users = UserAccounts(topo.standalone, DEFAULT_SUFFIX)
 

+     for i in range(101):
 

+         users.create_test_user(uid=i)
 

+ 

+     # Issue unindexed search,a nd make sure it is rejected
 

+     raw_objects = DSLdapObjects(topo.standalone, basedn=DEFAULT_SUFFIX)
 

+     with pytest.raises(ldap.UNWILLING_TO_PERFORM):
 

+         raw_objects.filter("(description=test*)")
 

+ 

+ 

+ 

+ @pytest.mark.skipif(ds_is_older('1.4.2'), reason="The config setting only exists in 1.4.2 and higher")
 

+ def test_require_internal_index(topo):
 

+     """Test nsslapd-ignore-virtual-attrs configuration attribute
 

+ 

+     :id: 22b94f30-59e3-4f27-89a1-c4f4be036f7f
 

+     :setup: Standalone instance
 

+     :steps:
 

+         1. Set "nsslapd-require-internalop-index" to "on"
 

+         2. Enable RI plugin, and configure it to use an attribute that is not indexed
 

+         3. Create a user and add it a group
 

+         4. Deleting user should be rejected as the RI plugin issues an
 

+         unindexed internal search
 

+     :expectedresults:
 

+         1. Success
 

+         2. Success
 

+         3. Success
 

+         4. Success
 

+     """
 

+     # Set the config
 

+     be_insts = Backends(topo.standalone).list()
 

+     for be in be_insts:
 

+         if be.get_attr_val_utf8_l('nsslapd-suffix') == DEFAULT_SUFFIX:
 

+             be.set('nsslapd-require-index', 'off')
 

+             be.set('nsslapd-require-internalop-index', 'on')
 

+ 

+     # Configure RI plugin
 

+     rip = ReferentialIntegrityPlugin(topo.standalone)
 

+     rip.set('referint-membership-attr', 'description')
 

+     rip.enable()
 

+ 

+     # Create a bunch of users
 

+     db_cfg = DatabaseConfig(topo.standalone)
 

+     db_cfg.set([('nsslapd-idlistscanlimit', '100')])
 

+     users = UserAccounts(topo.standalone, DEFAULT_SUFFIX)
 

+     for i in range(102, 202):
 

+         users.create_test_user(uid=i)
 

+ 

+     # Create user and group
 

+     user = users.create(properties={
 

+         'uid': 'indexuser',
 

+         'cn' : 'indexuser',
 

+         'sn' : 'user',
 

+         'uidNumber' : '1010',
 

+         'gidNumber' : '2010',
 

+         'homeDirectory' : '/home/indexuser'
 

+     })
 

+     groups = Groups(topo.standalone, DEFAULT_SUFFIX)
 

+     group = groups.create(properties={'cn': 'group',
 

+                                       'member': user.dn})
 

+ 

+     # Restart the server
 

+     topo.standalone.restart()
 

+ 

+     # Deletion of user should be rejected
 

+     with pytest.raises(ldap.UNWILLING_TO_PERFORM):
 

+         user.delete()
 

+ 

+ 

  if __name__ == '__main__':
 

      # Run isolated
 

      # -s for DEBUG mode

file modified
+16 -8 
@@ -49,7 +49,7 @@ 
 

  int referint_postop_modrdn(Slapi_PBlock *pb);
 

  int referint_postop_start(Slapi_PBlock *pb);
 

  int referint_postop_close(Slapi_PBlock *pb);
 

- int update_integrity(Slapi_DN *sDN, char *newrDN, Slapi_DN *newsuperior);
 

+ int update_integrity(Slapi_DN *sDN, char *newrDN, Slapi_DN *newsuperior, Slapi_PBlock *pb);
 

  int GetNextLine(char *dest, int size_dest, PRFileDesc *stream);
 

  int my_fgetc(PRFileDesc *stream);
 

  void referint_thread_func(void *arg);
 
@@ -611,7 +611,7 @@ 
 

      } else if (delay == 0) { /* no delay */
 

          /* call function to update references to entry */
 

          if (referint_sdn_in_entry_scope(sdn)) {
 

-             rc = update_integrity(sdn, NULL, NULL);
 

+             rc = update_integrity(sdn, NULL, NULL, pb);
 

          }
 

      } else {
 

          /* write the entry to integrity log */
 
@@ -663,7 +663,7 @@ 
 

          /* call function to update references to entry */
 

          if (!plugin_EntryScope && !plugin_ExcludeEntryScope) {
 

              /* no scope defined, default always process referint */
 

-             rc = update_integrity(sdn, newrdn, newsuperior);
 

+             rc = update_integrity(sdn, newrdn, newsuperior, pb);
 

          } else {
 

              const char *newsuperiordn = slapi_sdn_get_dn(newsuperior);
 

              if ((newsuperiordn == NULL && referint_sdn_in_entry_scope(sdn)) ||
 
@@ -672,10 +672,10 @@ 
 

                   * It is a modrdn inside the scope or into the scope,
 

                   * process normal modrdn
 

                   */
 

-                 rc = update_integrity(sdn, newrdn, newsuperior);
 

+                 rc = update_integrity(sdn, newrdn, newsuperior, pb);
 

              } else if (referint_sdn_in_entry_scope(sdn)) {
 

                  /* the entry is moved out of scope, treat as delete */
 

-                 rc = update_integrity(sdn, NULL, NULL);
 

+                 rc = update_integrity(sdn, NULL, NULL, pb);
 

              }
 

          }
 

      } else {
 
@@ -1067,7 +1067,8 @@ 
 

  int
 

  update_integrity(Slapi_DN *origSDN,
 

                   char *newrDN,
 

-                  Slapi_DN *newsuperior)
 

+                  Slapi_DN *newsuperior,
 

+                  Slapi_PBlock *pb)
 

  {
 

      Slapi_PBlock *search_result_pb = NULL;
 

      Slapi_PBlock *mod_pb = slapi_pblock_new();
 
@@ -1181,6 +1182,10 @@ 
 

                                           * We're using backend transactions,
 

                                           * so we need to stop on failure.
 

                                           */
 

+                                         if (pb) {
 

+                                             /* Set the error code of the failure */
 

+                                             slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);
 

+                                         }
 

                                          rc = SLAPI_PLUGIN_FAILURE;
 

                                          goto free_and_return;
 

                                      } else {
 
@@ -1196,8 +1201,11 @@ 
 

                                        "update_integrity - Search (base=%s filter=%s) returned "
 

                                        "error %d\n",
 

                                        search_base, filter, search_result);
 

-                         rc = SLAPI_PLUGIN_FAILURE;
 

                          slapi_free_search_results_internal(search_result_pb);
 

+                         if (pb) {
 

+                             slapi_pblock_set(pb, SLAPI_RESULT_CODE, &search_result);
 

+                         }
 

+                         rc = SLAPI_PLUGIN_FAILURE;
 

                          goto free_and_return;
 

                      }
 

                  }
 
@@ -1432,7 +1440,7 @@ 
 

                  }
 

              }
 

  

-             update_integrity(sdn, tmprdn, tmpsuperior);
 

+             update_integrity(sdn, tmprdn, tmpsuperior, NULL);
 

  

              slapi_sdn_free(&sdn);
 

              slapi_ch_free_string(&tmprdn);

file modified
+1 
@@ -755,6 +755,7 @@ 
 

      char *inst_dataversion;          /* The user data version tag.  Used by replication. */
 

      void *inst_db;                   /* implementation specific instance data */
 

      int require_index;               /* set to 1 to require an index be used in search */
 

+     int require_internalop_index;    /* set to 1 to require an index be used in an internal search */
 

      struct cache inst_dncache;       /* The dn cache for this instance. */
 

  } ldbm_instance;

file modified
+1 
@@ -135,6 +135,7 @@ 
 

  #define CONFIG_INSTANCE_DIR "nsslapd-directory"
 

  

  #define CONFIG_INSTANCE_REQUIRE_INDEX "nsslapd-require-index"
 

+ #define CONFIG_INSTANCE_REQUIRE_INTERNALOP_INDEX "nsslapd-require-internalop-index"
 

  

  #define CONFIG_USE_LEGACY_ERRORCODE "nsslapd-do-not-use-vlv-error"

file modified
+27 
@@ -247,6 +247,14 @@ 
 

      return (void *)((uintptr_t)inst->require_index);
 

  }
 

  

+ static void *
 

+ ldbm_instance_config_require_internalop_index_get(void *arg)
 

+ {
 

+     ldbm_instance *inst = (ldbm_instance *)arg;
 

+ 

+     return (void *)((uintptr_t)inst->require_internalop_index);
 

+ }
 

+ 

  static int
 

  ldbm_instance_config_readonly_set(void *arg,
 

                                    void *value,
 
@@ -299,6 +307,24 @@ 
 

  }
 

  

  

+ static int
 

+ ldbm_instance_config_require_internalop_index_set(void *arg,
 

+                                                   void *value,
 

+                                                   char *errorbuf __attribute__((unused)),
 

+                                                   int phase __attribute__((unused)),
 

+                                                   int apply)
 

+ {
 

+     ldbm_instance *inst = (ldbm_instance *)arg;
 

+ 

+     if (!apply) {
 

+         return LDAP_SUCCESS;
 

+     }
 

+ 

+     inst->require_internalop_index = (int)((uintptr_t)value);
 

+ 

+     return LDAP_SUCCESS;
 

+ }
 

+ 

  /*------------------------------------------------------------------------
 

   * ldbm instance configuration array
 

   *----------------------------------------------------------------------*/
 
@@ -307,6 +333,7 @@ 
 

      {CONFIG_INSTANCE_CACHEMEMSIZE, CONFIG_TYPE_UINT64, DEFAULT_CACHE_SIZE_STR, &ldbm_instance_config_cachememsize_get, &ldbm_instance_config_cachememsize_set, CONFIG_FLAG_ALWAYS_SHOW | CONFIG_FLAG_ALLOW_RUNNING_CHANGE},
 

      {CONFIG_INSTANCE_READONLY, CONFIG_TYPE_ONOFF, "off", &ldbm_instance_config_readonly_get, &ldbm_instance_config_readonly_set, CONFIG_FLAG_ALWAYS_SHOW | CONFIG_FLAG_ALLOW_RUNNING_CHANGE},
 

      {CONFIG_INSTANCE_REQUIRE_INDEX, CONFIG_TYPE_ONOFF, "off", &ldbm_instance_config_require_index_get, &ldbm_instance_config_require_index_set, CONFIG_FLAG_ALWAYS_SHOW | CONFIG_FLAG_ALLOW_RUNNING_CHANGE},
 

+ 	{CONFIG_INSTANCE_REQUIRE_INTERNALOP_INDEX, CONFIG_TYPE_ONOFF, "off", &ldbm_instance_config_require_internalop_index_get, &ldbm_instance_config_require_internalop_index_set, CONFIG_FLAG_ALWAYS_SHOW | CONFIG_FLAG_ALLOW_RUNNING_CHANGE},
 

      {CONFIG_INSTANCE_DNCACHEMEMSIZE, CONFIG_TYPE_UINT64, DEFAULT_DNCACHE_SIZE_STR, &ldbm_instance_config_dncachememsize_get, &ldbm_instance_config_dncachememsize_set, CONFIG_FLAG_ALWAYS_SHOW | CONFIG_FLAG_ALLOW_RUNNING_CHANGE},
 

      {NULL, 0, NULL, NULL, NULL, 0}};

file modified
+13 -15 
@@ -834,6 +834,7 @@ 
 

      if (NULL != candidates && ALLIDS(candidates)) {
 

          unsigned int opnote;
 

          int ri = 0;
 

+         int rii = 0;
 

          int pr_idx = -1;
 

          Connection *pb_conn = NULL;
 

          Operation *pb_op = NULL;
 
@@ -849,23 +850,20 @@ 
 

          int32_t op_nested_count;
 

  

          /*
 

-          * Return error if nsslapd-require-index is set and
 

-          * this is not an internal operation.
 

-          * We hope the plugins know what they are doing!
 

+          * Return error if require index is set
 

           */
 

-         if (!internal_op) {
 

- 

-             PR_Lock(inst->inst_config_mutex);
 

-             ri = inst->require_index;
 

-             PR_Unlock(inst->inst_config_mutex);
 

- 

-             if (ri) {
 

-                 idl_free(&candidates);
 

-                 candidates = idl_alloc(0);
 

-                 tmp_err = LDAP_UNWILLING_TO_PERFORM;
 

-                 tmp_desc = "Search is not indexed";
 

-             }
 

+         PR_Lock(inst->inst_config_mutex);
 

+         ri = inst->require_index;

Pretty minor, couldn't this be ri = inst->require_index | inst->require_internalop_index; since we don't distinguish the two in the if condition? Would make the if condition a bit clearer. Not major though. 

+         rii = inst->require_internalop_index;
 

+         PR_Unlock(inst->inst_config_mutex);
 

+ 

+         if ((internal_op && rii) || (!internal_op && ri)) {
 

+             idl_free(&candidates);
 

+             candidates = idl_alloc(0);
 

+             tmp_err = LDAP_UNWILLING_TO_PERFORM;
 

+             tmp_desc = "Search is not indexed";
 

          }
 

+ 

          /*
 

           * When an search is fully unindexed we need to log the
 

           * details as these kinds of searches can cause issues with bdb db

Bug Description:

Some plugins can perform unindexed searches, and under the right conditions this can cause problems like exhausting DB locks. The setting "nsslapd-require-index" does not apply to internal searches, so there is no way to prevent these searches from occurring.

Fix Description:

Add a new database setting "nsslapd-require-internalop-index" that rejects an internal unindexed searches.

              Also found during testing that when the RI plugin fails that
              it does not set the proper result error code.

relates: https://pagure.io/389-ds-base/issue/51192

Pretty minor, couldn't this be ri = inst->require_index | inst->require_internalop_index; since we don't distinguish the two in the if condition? Would make the if condition a bit clearer. Not major though.

Besides that one comment, to me this looks like a good change. Would it be worth a ticket for 1.5/2.0 to make this default to on? I think probably there would be some concern about defaulting to on in the middle of the current 1.4 series, but it would be really good to default this to on in the future.

Pretty minor, couldn't this be ri = inst->require_index | inst->require_internalop_index; since we don't distinguish the two in the if condition? Would make the if condition a bit clearer. Not major though.

Well we still have to determine if it's internal op or not. You could reject internal unindexed ops, but still allow client unindexed searches. Seems silly, but it's allowed since there are two separate config settings.

Ohh yeah you're right! Sorry about that :)

The patch look good to me as well. Ack

Just a minor remark. search return a ldap result while update_intergrity is a plugin function that returns a plugin result (SLAPI_PLUGIN_FAILURE or SLAPI_PLUGIN_SUCCESS). I agree that LDAP_SUCCESS=SLAPI_PLUGIN_SUCCESS=0 but here it think SLAPI_PLUGIN_FAILURE is more appropriate than a ldap result.

If we run both tests then this part fails because the users already exist.
Besides that, looks good to me.

rebased onto fd0b57cbc652eff1b9db481af9bba1ea356bcb6a
3 years ago

Just a minor remark. search return a ldap result while update_intergrity is a plugin function that returns a plugin result (SLAPI_PLUGIN_FAILURE or SLAPI_PLUGIN_SUCCESS). I agree that LDAP_SUCCESS=SLAPI_PLUGIN_SUCCESS=0 but here it think SLAPI_PLUGIN_FAILURE is more appropriate than a ldap result.

The problem is that we need to return the ldap error code so we know why it failed. In the calling code we still return SLAPI_PLUGIN_FAILURE, but we take the error code from update integrity and set that in the pblock RESULT_CODE. Since this is a bit confusing I now pass the parent pblock to update_integrity(), there we update the result code and we do not meddle with the result code.

Please review...

The python part looks good to me.

rebased onto 53f9218
3 years ago

Pull-Request has been merged by mreynolds
3 years ago

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4249

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 6
+95 -2
file changed
dirsrvtests/tests/suites/config/config_test.py
+16 -8
file changed
ldap/servers/plugins/referint/referint.c
+1 -0
file changed
ldap/servers/slapd/back-ldbm/back-ldbm.h
+1 -0
file changed
ldap/servers/slapd/back-ldbm/ldbm_config.h
+27 -0
file changed
ldap/servers/slapd/back-ldbm/ldbm_instance_config.c
+13 -15
file changed
ldap/servers/slapd/back-ldbm/ldbm_search.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.