From bf067513a88156f5b3882dd4098a61110ced4caf Mon Sep 17 00:00:00 2001 From: tbordaz Date: Fri, 2 Oct 2020 12:03:12 +0200 Subject: [PATCH 4/5] Issue 4297- On ADD replication URP issue internal searches with filter containing unescaped chars (#4355) Bug description: In MMR a consumer receiving a ADD has to do some checking based on basedn. It checks if the entry was a tombstone or if the conflicting parent entry was a tombstone. To do this checking, URP does internal searches using basedn. A '*' (ASTERISK) is valid in a RDN and in a DN. But using a DN in an assertionvalue of a filter, the ASTERISK needs to be escaped else the server will interprete the filtertype to be a substring. (see https://tools.ietf.org/html/rfc4515#section-3) The problem is that if a added entry contains an ASTERISK in the DN, it will not be escaped in internal search and trigger substring search (likely unindexed). Fix description: escape the DN before doing internal search in URP Fixes: #4297 Reviewed by: Mark Reynolds, William Brown, Simon Pichugi (thanks !) Platforms tested: F31 --- .../suites/replication/acceptance_test.py | 167 ++++++++++++++++++ ldap/servers/plugins/replication/urp.c | 10 +- ldap/servers/slapd/filter.c | 21 +++ ldap/servers/slapd/slapi-plugin.h | 1 + 4 files changed, 196 insertions(+), 3 deletions(-) diff --git a/dirsrvtests/tests/suites/replication/acceptance_test.py b/dirsrvtests/tests/suites/replication/acceptance_test.py index 2897726f5..be432bea4 100644 --- a/dirsrvtests/tests/suites/replication/acceptance_test.py +++ b/dirsrvtests/tests/suites/replication/acceptance_test.py @@ -7,6 +7,8 @@ # --- END COPYRIGHT BLOCK --- # import pytest +import logging +from lib389.replica import Replicas from lib389.tasks import * from lib389.utils import * from lib389.topologies import topology_m4 as topo_m4 @@ -524,6 +526,171 @@ def test_invalid_agmt(topo_m4): assert False +def test_warining_for_invalid_replica(topo_m4): + """Testing logs to indicate the inconsistency when configuration is performed. + + :id: dd689d03-69b8-4bf9-a06e-2acd19d5e2c8 + :setup: MMR with four masters + :steps: + 1. Setup nsds5ReplicaBackoffMin to 20 + 2. Setup nsds5ReplicaBackoffMax to 10 + :expectedresults: + 1. nsds5ReplicaBackoffMin should set to 20 + 2. An error should be generated and also logged in the error logs. + """ + replicas = Replicas(topo_m4.ms["master1"]) + replica = replicas.list()[0] + log.info('Set nsds5ReplicaBackoffMin to 20') + replica.set('nsds5ReplicaBackoffMin', '20') + with pytest.raises(ldap.UNWILLING_TO_PERFORM): + log.info('Set nsds5ReplicaBackoffMax to 10') + replica.set('nsds5ReplicaBackoffMax', '10') + log.info('Resetting configuration: nsds5ReplicaBackoffMin') + replica.remove_all('nsds5ReplicaBackoffMin') + log.info('Check the error log for the error') + assert topo_m4.ms["master1"].ds_error_log.match('.*nsds5ReplicaBackoffMax.*10.*invalid.*') + +@pytest.mark.skipif(ds_is_older('1.4.4'), reason="Not implemented") +def test_csngen_task(topo_m2): + """Test csn generator test + + :id: b976849f-dbed-447e-91a7-c877d5d71fd0 + :setup: MMR with 2 masters + :steps: + 1. Create a csngen_test task + 2. Check that debug messages "_csngen_gen_tester_main" are in errors logs + :expectedresults: + 1. Should succeeds + 2. Should succeeds + """ + m1 = topo_m2.ms["master1"] + csngen_task = csngenTestTask(m1) + csngen_task.create(properties={ + 'ttl': '300' + }) + time.sleep(10) + log.info('Check the error log contains strings showing csn generator is tested') + assert m1.searchErrorsLog("_csngen_gen_tester_main") + +@pytest.mark.ds51082 +def test_csnpurge_large_valueset(topo_m2): + """Test csn generator test + + :id: 63e2bdb2-0a8f-4660-9465-7b80a9f72a74 + :setup: MMR with 2 masters + :steps: + 1. Create a test_user + 2. add a large set of values (more than 10) + 3. delete all the values (more than 10) + 4. configure the replica to purge those values (purgedelay=5s) + 5. Waiting for 6 second + 6. do a series of update + :expectedresults: + 1. Should succeeds + 2. Should succeeds + 3. Should succeeds + 4. Should succeeds + 5. Should succeeds + 6. Should not crash + """ + m1 = topo_m2.ms["master2"] + + test_user = UserAccount(m1, TEST_ENTRY_DN) + if test_user.exists(): + log.info('Deleting entry {}'.format(TEST_ENTRY_DN)) + test_user.delete() + test_user.create(properties={ + 'uid': TEST_ENTRY_NAME, + 'cn': TEST_ENTRY_NAME, + 'sn': TEST_ENTRY_NAME, + 'userPassword': TEST_ENTRY_NAME, + 'uidNumber' : '1000', + 'gidNumber' : '2000', + 'homeDirectory' : '/home/mmrepl_test', + }) + + # create a large value set so that it is sorted + for i in range(1,20): + test_user.add('description', 'value {}'.format(str(i))) + + # delete all values of the valueset + for i in range(1,20): + test_user.remove('description', 'value {}'.format(str(i))) + + # set purging delay to 5 second and wait more that 5second + replicas = Replicas(m1) + replica = replicas.list()[0] + log.info('nsds5ReplicaPurgeDelay to 5') + replica.set('nsds5ReplicaPurgeDelay', '5') + time.sleep(6) + + # add some new values to the valueset containing entries that should be purged + for i in range(21,25): + test_user.add('description', 'value {}'.format(str(i))) + +@pytest.mark.ds51244 +def test_urp_trigger_substring_search(topo_m2): + """Test that a ADD of a entry with a '*' in its DN, triggers + an internal search with a escaped DN + + :id: 9869bb39-419f-42c3-a44b-c93eb0b77667 + :setup: MMR with 2 masters + :steps: + 1. enable internal operation loggging for plugins + 2. Create on M1 a test_user with a '*' in its DN + 3. Check the test_user is replicated + 4. Check in access logs that the internal search does not contain '*' + :expectedresults: + 1. Should succeeds + 2. Should succeeds + 3. Should succeeds + 4. Should succeeds + """ + m1 = topo_m2.ms["master1"] + m2 = topo_m2.ms["master2"] + + # Enable loggging of internal operation logging to capture URP intop + log.info('Set nsslapd-plugin-logging to on') + for inst in (m1, m2): + inst.config.loglevel([AccessLog.DEFAULT, AccessLog.INTERNAL], service='access') + inst.config.set('nsslapd-plugin-logging', 'on') + inst.restart() + + # add a user with a DN containing '*' + test_asterisk_uid = 'asterisk_*_in_value' + test_asterisk_dn = 'uid={},{}'.format(test_asterisk_uid, DEFAULT_SUFFIX) + + test_user = UserAccount(m1, test_asterisk_dn) + if test_user.exists(): + log.info('Deleting entry {}'.format(test_asterisk_dn)) + test_user.delete() + test_user.create(properties={ + 'uid': test_asterisk_uid, + 'cn': test_asterisk_uid, + 'sn': test_asterisk_uid, + 'userPassword': test_asterisk_uid, + 'uidNumber' : '1000', + 'gidNumber' : '2000', + 'homeDirectory' : '/home/asterisk', + }) + + # check that the ADD was replicated on M2 + test_user_m2 = UserAccount(m2, test_asterisk_dn) + for i in range(1,5): + if test_user_m2.exists(): + break + else: + log.info('Entry not yet replicated on M2, wait a bit') + time.sleep(2) + + # check that M2 access logs does not "(&(objectclass=nstombstone)(nscpentrydn=uid=asterisk_*_in_value,dc=example,dc=com))" + log.info('Check that on M2, URP as not triggered such internal search') + pattern = ".*\(Internal\).*SRCH.*\(&\(objectclass=nstombstone\)\(nscpentrydn=uid=asterisk_\*_in_value,dc=example,dc=com.*" + found = m2.ds_access_log.match(pattern) + log.info("found line: %s" % found) + assert not found + + if __name__ == '__main__': # Run isolated # -s for DEBUG mode diff --git a/ldap/servers/plugins/replication/urp.c b/ldap/servers/plugins/replication/urp.c index 37fe77379..2c2d83c6c 100644 --- a/ldap/servers/plugins/replication/urp.c +++ b/ldap/servers/plugins/replication/urp.c @@ -1379,9 +1379,12 @@ urp_add_check_tombstone (Slapi_PBlock *pb, char *sessionid, Slapi_Entry *entry, Slapi_Entry **entries = NULL; Slapi_PBlock *newpb; char *basedn = slapi_entry_get_ndn(entry); + char *escaped_basedn; const Slapi_DN *suffix = slapi_get_suffix_by_dn(slapi_entry_get_sdn (entry)); + escaped_basedn = slapi_filter_escape_filter_value("nscpentrydn", basedn); - char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", basedn); + char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", escaped_basedn); + slapi_ch_free((void **)&escaped_basedn); newpb = slapi_pblock_new(); slapi_search_internal_set_pb(newpb, slapi_sdn_get_dn(suffix), /* Base DN */ @@ -1574,11 +1577,12 @@ urp_find_tombstone_for_glue (Slapi_PBlock *pb, char *sessionid, const Slapi_Entr Slapi_PBlock *newpb; const char *basedn = slapi_sdn_get_dn(parentdn); char *conflict_csnstr = slapi_entry_attr_get_charptr(entry, "conflictcsn"); + char *escaped_basedn = slapi_filter_escape_filter_value("nscpentrydn", basedn); CSN *conflict_csn = csn_new_by_string(conflict_csnstr); slapi_ch_free_string(&conflict_csnstr); CSN *tombstone_csn = NULL; - - char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", basedn); + char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", escaped_basedn); + slapi_ch_free((void **)&escaped_basedn); newpb = slapi_pblock_new(); char *parent_dn = slapi_dn_parent (basedn); slapi_search_internal_set_pb(newpb, diff --git a/ldap/servers/slapd/filter.c b/ldap/servers/slapd/filter.c index 393a4dcee..8e21b34c3 100644 --- a/ldap/servers/slapd/filter.c +++ b/ldap/servers/slapd/filter.c @@ -127,6 +127,27 @@ filter_escape_filter_value(struct slapi_filter *f, const char *fmt, size_t len _ return ptr; } +/* Escaped an equality filter value (assertionValue) of a given attribute + * Caller must free allocated escaped filter value + */ +char * +slapi_filter_escape_filter_value(char* filter_attr, char *filter_value) +{ + char *result; + struct slapi_filter *f; + + if ((filter_attr == NULL) || (filter_value == NULL)) { + return NULL; + } + f = (struct slapi_filter *)slapi_ch_calloc(1, sizeof(struct slapi_filter)); + f->f_choice = LDAP_FILTER_EQUALITY; + f->f_un.f_un_ava.ava_type = filter_attr; + f->f_un.f_un_ava.ava_value.bv_len = strlen(filter_value); + f->f_un.f_un_ava.ava_value.bv_val = filter_value; + result = filter_escape_filter_value(f, FILTER_EQ_FMT, FILTER_EQ_LEN); + slapi_ch_free((void**) &f); + return result; +} /* * get_filter_internal(): extract an LDAP filter from a BerElement and create diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index ea9bcf87b..a8b563dd1 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -5284,6 +5284,7 @@ int slapi_vattr_filter_test_ext(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Filter * int slapi_filter_compare(struct slapi_filter *f1, struct slapi_filter *f2); Slapi_Filter *slapi_filter_dup(Slapi_Filter *f); int slapi_filter_changetype(Slapi_Filter *f, const char *newtype); +char *slapi_filter_escape_filter_value(char* filter_attr, char *filter_value); int slapi_attr_is_last_mod(char *attr); -- 2.26.2