#529 dn normalization must handle multiple space characters in attributes
Closed: Fixed None Opened 6 years ago by lkrispen.

ack


It is possible to create entries, where their dn only differs in the number of spaces inside the value of the rdn attribute, eg:

ldapsearch ..... -b "dc=example,dc=com" "cn=user two" dn
dn: cn=user two,dc=example,dc=com
dn: cn=user two,dc=example,dc=com

Note the the search filter was did contain only one space, also attemping to add a new cn value differing in the number of spaces fails.

ldapmodify .....
dn: cn=user two,dc=example,dc=com
changetype: modify
add: cn
cn: user two

modifying entry "cn=user two,dc=example,dc=com"
ldap_modify: Type or value exists (20)

So on the attribute level, attribute value normalization maps all variants to a single space, which I think is correct. In my opinion this should also apply
to the use of an attribuet in the dn.

If you try to add the second entry above in OpenLDAP the result is:
ldap_add: already exists(68)
Also Sun/Oracle DSEE also does not allow adding these two entries.

See also: http://www.openldap.org/lists/openldap-software/200501/msg00102.html

and RFC4517, 4.2.15. distinguishedNameMatch can be read that the comparison has to be done by avas and so "dn: cn=user two,dc=example,dc=com" to be compared to "cn=user two,dc=example,dc=com" would require to normalize the two cn parts by applying the directory string match.

Bug description: If dn has a leaf rdn which contains space(s)
in the value (e.g., dn: cn=ABC DEF,...), it was allowed to add
entries which dn is identical but the number of the spaces
is different. (e.g., dn: cn=ABC DEF,...).

Fix description: Applying this patch, dn normalization changes
the behaviour so that multiple consecutive spaces are replaced
with one. But in case the attribute value pair in the rdn is
not found in the entry and being added, the original spaces are
kept in the attribute value. For instance, if an entry
dn: cn=ABC DEF,dc=example,dc=com
is added and the entry does not have
cn: ABC DEF
in it, then in the added entry, the dn would be normalized to
dn: cn=ABC DEF,dc=example,dc=com
with the original cn value
cn: ABC DEF

In addition:
1) In entryrdn_rename_subtree, an error message was suppressed,
which is issued when replacing an rdn with the one which is
identical except the number of spaces (e.g., cn=ABC DEF -->
cn=ABC DEF).
2) Fixed slapi_sdn_copy which used to trash the original dn
value. Now it copies the original correctly.
3) Fixed a compiler warning in str2entry_fast.

Reviewed by Ludwig (Thank you!!)

Pushed to master: commit 86ceedb

I have one concern, how is backward compatibility addressed ?
Without the fix a database coould have two entries cn=A B,o=suffix and cn=A B,o=suffix
, which would now be duplicates. If any of the is used as a search base, will it work.
Also there are attributes with dn syntax, eg uniqemember, like
uniquemember: cn=A B,o=suffix
which are normalized and index with the old normalization code.

Replying to [comment:9 lkrispen]:

I have one concern, how is backward compatibility addressed ?
Without the fix a database coould have two entries cn=A B,o=suffix and cn=A B,o=suffix
, which would now be duplicates. If any of the is used as a search base, will it work.
Also there are attributes with dn syntax, eg uniqemember, like
uniquemember: cn=A B,o=suffix
which are normalized and index with the old normalization code.
You are right, Ludwig. I think we need to provide an upgrade script for the issues. Reopening this ticket...

Bug description: This is the second half of the fix for #529.
The first half fixed the DN normalization which used to allow
DNs where only the number of spaces are different. Now it is
rejucted as expected. But it breaks the backward compatibility.

Fix description: The upgrade script 80upgradednformat.pl called
from "setup-ds.pl -u" checks the duplicated DNs and rename them
if necessary.

For instance, if there are 2 DNs:
cn=test user0,dc=example,dc=com (entryid: N)
cn=test user0,dc=example,dc=com (entryid: M)
then the upgrade script/tool modifies the second one as follows:
cn=test user0 M,dc=example,dc=com (entryid: M)
and the original "cn: test user0" is kept in the attribute.
The modified result is reported in "setup-ds.pl -u" as follows:
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Duplicated DN(s) were found and renamed.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Renamed entry IDs are listed in /var/lib/dirsrv/slapd-ID/ldif/userRoot_conflict.txt.
Contents of the conflict.txt:
prinary entry ID: duplicated entry IDs
13:16 18
14:17

In perl code, you need to check the return value of system() to see if the program crashed for our programs such as upgradednformat, db2index, etc. - see http://perldoc.perl.org/functions/system.html

otherwise, looks good

Minor issue, but should probably be addressed:

Index: ldap/admin/src/scripts/80upgradednformat.pl

--- a/ldap/admin/src/scripts/80upgradednformat.pl
+++ b/ldap/admin/src/scripts/80upgradednformat.pl
@@ -84,8 +84,11 @@ sub runinst {
$mtentry = $conn->nextEntry();
}

  • my $ldifdir = $config_entry->{"nsslapd-ldifdir"}[0];
    my $instancedir = $config_entry->{"nsslapd-instancedir"}[0];
  • my $upgradednformat = $instancedir . "/upgradednformat";
  • my ($slapd, $serverID) = split(/-/, $instancedir);
  • my $upgradednformat = "/usr/sbin/upgradednformat";
  • my $reindex = "/usr/sbin/db2index";

You are hard coding the path's, but if you compile 389 with a prefix, then this will be broken. You should be able to use "@sbindir@" for all the /usr/sbin directory paths.

Thank you for your comments, Rich and Mark!

1) I've added the child process crash handling.
{{{
+ $cmd = "$upgradednformat -n $backend -a $dbinstdir -N";
+ $rc = system("$cmd");
+ if ($rc & 127) {
+ push @errs, [ 'error_running_command', $cmd, $rc, $! ];
+ return @errs;
+ }
}}}
2) and renamed 80upgradednformat.pl to 80upgradednformat.pl.in to take an advantage to use "@sbindir@".

Thanks, looks good. Ack.

Reviewed by Rich and Mark (Thank you, both!!)

Pushed to master: commit 69ff835
Pushed to 389-ds-base-1.3.1 branch: commit 36696aa

Description: commit 69ff835
introduced 2 coverity defects:
13162: Resource leak
Free allocated strings: newrdn and parentdn
13163: Unused pointer value
Removed unused pointer "p"

Reviewed by Rich (Thank you!!)
Pushed to master: commit 77e61a8
Pushed to 389-ds-base-1.3.1 branch: commit 75c84e0

git patch file (master) -- fixing a compiler warning introduced by the previous patch
0001-Ticket-529-dn-normalization-must-handle-multiple-spa.5.patch

Thanks to Mark for finding it out!

Pushed to master: commit e53d6de
Pushed to 389-ds-base-1.3.1 branch: commit 9da5558

Bug Description: Commit 69ff835
had a flaw -- When multiple server instances exist, upgrade dn
format fails due to missing server ID.

Fix Description: The upgrade script always passes server ID.

revised git patch file (master) -- fixing a bug in the previous commit
0001-Ticket-529-dn-normalization-must-handle-multiple-spa.6.patch

Reviewed by Rich (Thank you!!)

Pushed to master: commit 72f3f04
Pushed to 389-ds-base-1.3.1: commit 77ca0bb

Turned out it's broken...

Bug Description: Commit 69ff835
had a flaw -- handling normdn in upgradedn_producer had a
problem. The string was passed to the Slapi_DN in the entry
using slapi_sdn_init_dn_passin, while the string was reffered
at other places and even could be modified.

Fix Description: This patch manages the normdn string more
carefully.

Reviewed by Rich (Thank you!!)

Pushed to master: commit a68952f
Pushed to 389-ds-base-1.3.1: commit 286b09d

Thanks to Mark for pointing out the warnings:

Looks like you introduced a compiler warning
../ds/ldap/servers/slapd/back-ldbm/import-threads.c: In function 'upgradedn_producer':
../ds/ldap/servers/slapd/back-ldbm/import-threads.c:1817: warning: passing argument 1 of 'slapi_ch_free_string' from incompatible pointer type
../ds/ldap/servers/slapd/slapi-plugin.h:5491: note: expected 'char ' but argument is of type 'const char '

Pushed to master: commit 5a0e057
Pushed to 389-ds-base-1.3.1: commit 3a7c727

Metadata Update from @nhosoi:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.3.1.1

2 years ago

Login to comment on this ticket.

Metadata