#529 dn normalization must handle multiple space characters in attributes
Closed: wontfix Opened 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 86ceedbcb6eff76c52797bcbc8629091d4fc8ee0

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...

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=918707

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 69ff83598d517bed84922b1c7dd67cab023b4d99
Pushed to 389-ds-base-1.3.1 branch: commit 36696aac383ea9de73ff15f255677dfe1f975acf

Description: commit 69ff83598d517bed84922b1c7dd67cab023b4d99
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 77e61a80d56da6fe8a9dd56c6c29acb86032e131
Pushed to 389-ds-base-1.3.1 branch: commit 75c84e0a28e5483ae0dd85ce890779b4e86de79d

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 e53d6de87af2a370be1238a348769b19c5f429b6
Pushed to 389-ds-base-1.3.1 branch: commit 9da5558ef719b56845baf541515d46d54437e3c6

Bug Description: Commit 69ff83598d517bed84922b1c7dd67cab023b4d99
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 72f3f04e1ec6d3d552f3d0141768329dcc632fe1
Pushed to 389-ds-base-1.3.1: commit 77ca0bb587d38409916c2c130de17a61551b2499

Turned out it's broken...

Bug Description: Commit 69ff83598d517bed84922b1c7dd67cab023b4d99
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 a68952f9a8e6d3ff9fea1b4b485acc63f77b3ce6
Pushed to 389-ds-base-1.3.1: commit 286b09d62dd17cc3eef9dfa058521e4c863cdb80

git patch file (master) -- fixing compiler warnings.
0001-Ticket-529-dn-normalization-must-handle-multiple-spa.8.patch

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 5a0e057940bddd6e076186845a0cacf99c6c0143
Pushed to 389-ds-base-1.3.1: commit 3a7c7271a4972910a4250c140096cfd402e98566

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

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

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

Metadata