From 53f921816bde65797f0c7af776c943823c4d002a Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jul 01 2020 13:07:37 +0000 Subject: Issue 51192 - Add option to reject internal unindexed searches 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 occuring. 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 Reviewed by: firstyear, spichugi & tbordaz (Thanks!!!) --- diff --git a/dirsrvtests/tests/suites/config/config_test.py b/dirsrvtests/tests/suites/config/config_test.py index 567059b..38d1ed9 100644 --- a/dirsrvtests/tests/suites/config/config_test.py +++ b/dirsrvtests/tests/suites/config/config_test.py @@ -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 @@ import pytest 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 @@ def test_ndn_cache_enabled(topo): 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 diff --git a/ldap/servers/plugins/referint/referint.c b/ldap/servers/plugins/referint/referint.c index 7319976..eb4b089 100644 --- a/ldap/servers/plugins/referint/referint.c +++ b/ldap/servers/plugins/referint/referint.c @@ -49,7 +49,7 @@ int referint_postop_del(Slapi_PBlock *pb); 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 @@ referint_postop_del(Slapi_PBlock *pb) } 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 @@ referint_postop_modrdn(Slapi_PBlock *pb) /* 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 @@ referint_postop_modrdn(Slapi_PBlock *pb) * 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 @@ bail: 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 @@ update_integrity(Slapi_DN *origSDN, * 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(Slapi_DN *origSDN, "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 @@ referint_thread_func(void *arg __attribute__((unused))) } } - update_integrity(sdn, tmprdn, tmpsuperior); + update_integrity(sdn, tmprdn, tmpsuperior, NULL); slapi_sdn_free(&sdn); slapi_ch_free_string(&tmprdn); diff --git a/ldap/servers/slapd/back-ldbm/back-ldbm.h b/ldap/servers/slapd/back-ldbm/back-ldbm.h index 9d78ad7..e325d25 100644 --- a/ldap/servers/slapd/back-ldbm/back-ldbm.h +++ b/ldap/servers/slapd/back-ldbm/back-ldbm.h @@ -755,6 +755,7 @@ typedef struct ldbm_instance 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; diff --git a/ldap/servers/slapd/back-ldbm/ldbm_config.h b/ldap/servers/slapd/back-ldbm/ldbm_config.h index fdf8f6e..58e6479 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_config.h +++ b/ldap/servers/slapd/back-ldbm/ldbm_config.h @@ -135,6 +135,7 @@ struct config_info #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" diff --git a/ldap/servers/slapd/back-ldbm/ldbm_instance_config.c b/ldap/servers/slapd/back-ldbm/ldbm_instance_config.c index 628ac90..36ec112 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_instance_config.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_instance_config.c @@ -247,6 +247,14 @@ ldbm_instance_config_require_index_get(void *arg) 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 @@ ldbm_instance_config_require_index_set(void *arg, } +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 @@ static config_info ldbm_instance_config[] = { {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}}; diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c index 7235714..1a7b510 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_search.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c @@ -834,6 +834,7 @@ ldbm_back_search(Slapi_PBlock *pb) 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 @@ ldbm_back_search(Slapi_PBlock *pb) 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; + 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