389-ds-base

Clone
Source Code
GIT

#51245 Ticket 51244 - On ADD replication URP issue internal searches with filter containing unescaped chars
Closed 3 years ago by spichugi. Opened 3 years ago by tbordaz.
tbordaz/389-ds-base ticket_51244  into  master

file modified
+63 
@@ -7,6 +7,7 @@ 
 

  # --- END COPYRIGHT BLOCK ---
 

  #
 

  import pytest
 

+ import logging
 

  from lib389.replica import Replicas
 

  from lib389.tasks import *
 

  from lib389.utils import *
 
@@ -578,6 +579,68 @@ 
 

      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

file modified
+8 -2 
@@ -1411,9 +1411,12 @@ 
 

      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);

please use slapi_ch_free_string() here too! Thanks!

 
      newpb = slapi_pblock_new();
 

      slapi_search_internal_set_pb(newpb,
 

                                   slapi_sdn_get_dn(suffix), /* Base DN */
 
@@ -1602,12 +1605,15 @@ 
 

      Slapi_Entry **entries = NULL;
 

      Slapi_PBlock *newpb;
 

      const char *basedn = slapi_sdn_get_dn(parentdn);
 

+     char *escaped_basedn;
 

+     escaped_basedn = slapi_filter_escape_filter_value("nscpentrydn", basedn);
 

  

      char *conflict_csnstr = (char*)slapi_entry_attr_get_ref((Slapi_Entry *)entry, "conflictcsn");
 

      CSN *conflict_csn = csn_new_by_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);

please use slapi_ch_free_string()

 
      newpb = slapi_pblock_new();
 

      char *parent_dn = slapi_dn_parent (basedn);
 

      slapi_search_internal_set_pb(newpb,

file modified
+21 
@@ -130,6 +130,27 @@ 
 

      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

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

  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);

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

https://pagure.io/389-ds-base/issue/51244

Reviewed by: ?

Platforms tested: F31

Code looks good, but I thought you could have used: filter_escape_filter_value() Maybe not?

rebased onto d7c5cc5
3 years ago

@mreynolds thanks for reviewing.

Thanks for the tip. It took me some time to make it work using filter_escape_filter_value but I think it is the right approach. I updated the patch.

rebased onto fa025b8
3 years ago

@mreynolds, I rebase the PR. do you mind to review it again :)

This seems clearer, good job. I'll leave it to @mreynolds to finish review though :)

please use slapi_ch_free_string()

please use slapi_ch_free_string() here too! Thanks!

Two minor issues, but ack!! Yeah this code looks a lot better using the filter escape functions, thanks!

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/4298

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 4
+63 -0
file changed
dirsrvtests/tests/suites/replication/acceptance_test.py
+8 -2
file changed
ldap/servers/plugins/replication/urp.c
+21 -0
file changed
ldap/servers/slapd/filter.c
+1 -0
file changed
ldap/servers/slapd/slapi-plugin.h
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.