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 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
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 69ff83598d517bed84922b1c7dd67cab023b4d99 Pushed to 389-ds-base-1.3.1 branch: commit 36696aac383ea9de73ff15f255677dfe1f975acf
git patch file (master) -- coverity fix 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.4.patch
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...
git patch file (master) 0001-Ticket-529-dn-normalization-must-handle-multiple-spa.7.patch
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.
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)