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.
git patch file (master) 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.patch
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...
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=918707
git patch file (master) 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.2.patch
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:
--- a/ldap/admin/src/scripts/80upgradednformat.pl +++ b/ldap/admin/src/scripts/80upgradednformat.pl @@ -84,8 +84,11 @@ sub runinst { $mtentry = $conn->nextEntry(); }
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.
git patch file (master -- take 2) 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.3.patch
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
git patch file (master) -- coverity fix 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.4.patch
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...
git patch file (master) 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.7.patch
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.
Pushed to master: commit a68952f Pushed to 389-ds-base-1.3.1: commit 286b09d
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 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
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.