URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: Dn with spaces Action: opened
PR body: """ This PR is push to upstream work of @thalman as he is on PTO now. Those 2 commits fixes issue with white space in DN reported by one of clients. Client already confirmed that this solves the issue for him. When @thalman will be back from PTO he will took over this. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: edited
Changed field: title Original value: """ Dn with spaces """
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Bugzilla
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """ @sumit-bose I see one more issue in the old code. If DN comes with escape characters the we escape them again and we get
cn=Doe, John => cn=Doe\5c\2c John
and it breaks the search. Lets address this too before merge. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-671967794
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """ Hi Tomas,
there is `ldap_dn_normalize()` in OpenLDAP. Can you check if something like
ldap_dn_normalize(dn_str, LDAP_DN_FORMAT_LDAP, &sanitized_dn_str, LDAP_DN_FORMAT_LDAPV3);
does remove the space as well? A wrapper function would still be nice so that we can get the result as talloc'ed string?
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-673413612
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """ I tried to use OpenLDAP function for sanitization as well as ldb. But both have some downsides so I agreed with @sumit-bose to proceed with our own function for DN normalization & sanitization. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-674949085
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ Those tests fails under valgrind: ``` FAIL: nss-srv-tests FAIL: nestedgroups-tests FAIL: test_sysdb_ts_cache FAIL: test_sysdb_views FAIL: ssh-srv-tests FAIL: test_ldap_id_cleanup FAIL: sysdb-tests ``` """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675018921
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """ Fixed. There was forgotten "\" string in calling escape function. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675285461
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """
@sumit-bose I see one more issue in the old code. If DN comes with escape characters the we escape them again and we get
cn=Doe\, John => cn=Doe\5c\2c Johnand it breaks the search. Lets address this too before merge.
Hi,
can you add such examples to the test as well to make sure we handle escaped characters, especially escaped spaces, correctly?
bye, Sumit
Unfortunately this issue is not addressed by this PR. Simple solution with skipping escapes doesn't work and breaks the cache (Or at least tests).
Considering that this is a bit different issue, I will address that separately in other PR.
"""
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675344723
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """
@sumit-bose I see one more issue in the old code. If DN comes with escape characters the we escape them again and we get
cn=Doe\, John => cn=Doe\5c\2c Johnand it breaks the search. Lets address this too before merge.
Hi, can you add such examples to the test as well to make sure we handle escaped characters, especially escaped spaces, correctly? bye, Sumit
Unfortunately this issue is not addressed by this PR. Simple solution with skipping escapes doesn't work and breaks the cache (Or at least tests).
Considering that this is a bit different issue, I will address that separately in other PR.
Yes, makes sense, as we discussed, this is an issue which would already happen in the old code using just `sss_filter_sanitize()` so it can be handled separately. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675452160
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """ Hi,
in general I agree with the patches.
I'm just wondering if the changes are needed in places where the DN was generated by `ldb_dn_get_linearized()` which are the changes in
- src/ldb_modules/memberof.c - src/providers/ldap/ldap_id_cleanup.c
I wouldn't expect `ldb_dn_get_linearized()` to return extra space and so we can safe the extra check and keep `sss_filter_sanitize()`. While the cleanup would not matter much, the memberof plugin is often called and saving processing time would be beneficial.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675459244
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """
I'm just wondering if the changes are needed in places where the DN was generated by `ldb_dn_get_linearized()` which are the changes in
* src/ldb_modules/memberof.c * src/providers/ldap/ldap_id_cleanup.cI wouldn't expect `ldb_dn_get_linearized()` to return extra space and so we can safe the extra check and keep `sss_filter_sanitize()`. While the cleanup would not matter much, the memberof plugin is often called and saving processing time would be beneficial.
bye, Sumit
Thank you, removed from those two files. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675465662
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """ Hi,
thanks, I do not expect any CI issues, so ACK.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-675470082
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """
Hi,
thanks, I do not expect any CI issues, so ACK.
bye, Sumit
Unfortunately I had to put it back, Tests show that `ldb_dn_get_linearized()` can return the spaces in DN too.
Please review. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-676364937
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """
Hi, thanks, I do not expect any CI issues, so ACK. bye, Sumit
Unfortunately I had to put it back, Tests show that `ldb_dn_get_linearized()` can return the spaces in DN too.
Please review.
Sorry for the misleading suggestion. The third patch restores the original state of your patch. ACK.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-676372476
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """ Hi,
the reason for the issue with the hash that on one hand the DN from the user object is used as key when the user is added to the ghost users table and on the other hand the DN from the member attribute is used as key to lookup a user in the hash. Those two DNs might differ for other reasons as well, e.g. different cases in the string as discussed in https://bugzilla.redhat.com/show_bug.cgi?id=1817122 (sorry, mostly private).
I was thinking if this issue can be avoided in general by using the DN from the member attribute as well when adding the user to the hash table and came up with https://github.com/sumit-bose/sssd/commits/ghost_hash_dn. Can you check if this will make the group lookup worked as well when you replace your 'CACHE: trim spaces in DN before hash lookup' and 'CACHE: use talloc_pool to avoid malloc' with this patch?
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-680898733
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """
I was thinking if this issue can be avoided in general by using the DN from the member attribute as well when adding the user to the hash table and came up with https://github.com/sumit-bose/sssd/commits/ghost_hash_dn. Can you check if this will make the group lookup worked as well when you replace your 'CACHE: trim spaces in DN before hash lookup' and 'CACHE: use talloc_pool to avoid malloc' with this patch?
bye, Sumit
Tested, works well """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-680977506
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """
I was thinking if this issue can be avoided in general by using the DN from the member attribute as well when adding the user to the hash table and came up with https://github.com/sumit-bose/sssd/commits/ghost_hash_dn. Can you check if this will make the group lookup worked as well when you replace your 'CACHE: trim spaces in DN before hash lookup' and 'CACHE: use talloc_pool to avoid malloc' with this patch? bye, Sumit
Tested, works well
Thanks, @alexey-tikhonov, which solution would you prefer? """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-681917021
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """
Thanks, @alexey-tikhonov, which solution would you prefer?
If I understood correctly, your approach is to use the same DN string as a table key everywhere instead of attempt to unify (transform to single state) two DN strings (with help of trimming/sanitization).
If I got it right, then in general I like your approach more.
But I must admit I don't understand sdap* code well enough to: - verify if you spotted all the places where this "new" attribute (SYSDB_DN_FOR_MEMBER_HASH_TABLE) must be stored/updated and read to be used as a key; - to estimate performance hit (shouldn't be dramatic though, because it's just one additional attr to store, and should have no impact during lookup; but I can't compare performance-wise with Tomas' patch).
"""
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-682111564
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """
Thanks, @alexey-tikhonov, which solution would you prefer?
If I understood correctly, your approach is to use the same DN string as a table key everywhere instead of attempt to unify (transform to single state) two DN strings (with help of trimming/sanitization).
Hi,
yes.
If I got it right, then in general I like your approach more.
But I must admit I don't understand sdap* code well enough to:
* verify if you spotted all the places where this "new" attribute (SYSDB_DN_FOR_MEMBER_HASH_TABLE) must be stored/updated and read to be used as a key;
Currently this is only about the ghost hash table where as far as I can see only when the entries were added the DN of the user object was used. Lookups are always done with the DN from the member attribute. So I think only the creation of the entries had to be updated.
* to estimate performance hit (shouldn't be dramatic though, because it's just one additional attr to store, and should have no impact during lookup; but I can't compare performance-wise with Tomas' patch).
Yes, this is always hard, but I agree, it is only about adding an additionally attribute, which would be a memory allocation and a copy of the string. Since there is no comparison for every single character it might be a bit faster than Tomas' patch but the trimming of spaces shouldn't take much time anyways.
The main reason I propose this solution is that it would cover cases as well, e.g. when the DN in the member attribute differs in case from the DN of the user object.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-682354104
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ Covscan is clear. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-682432819
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ Patches: ``` UTIL: DN sanitization UTIL: Use sss_sanitize_dn where we deal with DN UTIL: Use sss_sanitize_dn where we deal with DN 2 ``` were reviewed by @elkoniu and @sumit-bose
Patch ``` ldap: use member DN to create ghost user hash table ``` was reviewed by @thalman and me.
"""
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-688254202
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """ @alexey-tikhonov, @thalman, I've added a test to https://github.com/sumit-bose/sssd/commits/ghost_hash_dn, maybe you'd like to pull this as well? """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-688266601
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ That would be great. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-688268353
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ That would be great.
Removing "accepted" label until test is included. """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-688268353
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
ikerexxe commented: """ Just tested it and commit 80a996d29d395a5cc7d02463d1a2fa8fae868d18 also solves [bz1817122](https://bugzilla.redhat.com/show_bug.cgi?id=1817122). """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-688739899
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """ I have cherry-picked last changes from @sumit-bose including tests """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-689549733
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ ``` ./src/tests/intg/test_ldap.py:417:40: E128 continuation line under-indented for visual indent ./src/tests/intg/test_ldap.py:417:80: E501 line too long (80 > 79 characters) ``` """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-691967604
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
sumit-bose commented: """
./src/tests/intg/test_ldap.py:417:40: E128 continuation line under-indented for visual indent ./src/tests/intg/test_ldap.py:417:80: E501 line too long (80 > 79 characters)
ah, sorry, I fixed this in my branch, @thalman, can you pick the latest version of 'intg: allow member DN to have a different case'? Thanks.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-692145522
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
thalman commented: """ Cherry-picked latest test version """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-692778107
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
alexey-tikhonov commented: """ ACK """
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-702242946
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
pbrezina commented: """ Pushed PR: https://github.com/SSSD/sssd/pull/5262
* `master` * 88631392e9172ae4fa3e411398516a2f39f0060e - intg: allow member DN to have a different case * 50d0d154cedb6915ab321b47c40851c40e91cf41 - ldap: use member DN to create ghost user hash table * fe0f1e64e8a77dadde699495c7eb368ce61ac992 - UTIL: Use sss_sanitize_dn where we deal with DN 2 * 21b9417e14ce35a2548c309642325ac43103d51e - UTIL: Use sss_sanitize_dn where we deal with DN * 093061f553ab0a2c316794221e79779fb1bd40d2 - UTIL: DN sanitization
"""
See the full comment at https://github.com/SSSD/sssd/pull/5262#issuecomment-702649764
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5262 Title: #5262: DN with white spaces
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
elkoniu
-
ikerexxe
-
pbrezina
-
sumit-bose
-
thalman