URL: https://github.com/SSSD/sssd/pull/300 Author: justin-stephenson Title: #300: LDAP: Fix nesting level comparison Action: opened
PR body: """ Very(very) basic fix to correct an issue with nesting level comparison of option `ldap_group_nesting_level` to ensure that setting nesting level 0 will avoid parent group of group searches.
This was tested and confirmed as fixed downstream, but if needed can be tested with the LDAP provider and `ldap_schema = rfc2307bis` with nesting level set to 0.
Resolves: https://pagure.io/SSSD/sssd/issue/3425 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/300/head:pr300 git checkout pr300
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-306943980
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-306944001
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
fidencio commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-307020012
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
fidencio commented: """ @justin-stephenson, the code itself looks good.
I'm running some of our tests here and I will get back to you with the results. Would be really nice if you could also provide some step-by-step on how to test the patch.
Last but not least, AFAIR we should already have tests for "no nesting" in our code base (please, take a look on `src/tests/intg/test_ldap.py` and, most likely, improving the test to catch the issue you're fixing would be a really good (and desired to merged with this patch).
@lslebodn, please, correct me if I'm mistaken about the "no nesting" tests.
For now setting the label to "Changes requested" as per the tests comment. @lslebodn, again, please, feel free to remove the label if I'm mistaken about the tests. """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-307023176
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
fidencio commented: """ CI: http://sssd-ci.duckdns.org/logs/job/71/12/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-307038219
URL: https://github.com/SSSD/sssd/pull/300 Author: justin-stephenson Title: #300: LDAP: Fix nesting level comparison Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/300/head:pr300 git checkout pr300
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
justin-stephenson commented: """ @fidencio to test this you would need to configure SSSD with a basic LDAP provider configuration and using the options `ldap_group_nesting_level = 0` and `ldap_schema = rfc2307bis` then test a basic user lookup.
Before the patch you would see: `[rfc2307bis_nested_groups_next_base] (0x0400): Searching for parent groups of group...`
After the patch only parent groups of the user should be searched, not parent groups of groups.
I added to the **test_zero_nesting_level** integration test, I don't really think the original test was checking nesting level properly so the extra commit is what I propose to fix it. """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-308877157
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
justin-stephenson commented: """ @fidencio to test this you would need to configure SSSD with a basic LDAP provider configuration and using the options `ldap_group_nesting_level = 0` and `ldap_schema = rfc2307bis` then test a basic user lookup.
Before the patch you would see: `[rfc2307bis_nested_groups_next_base] (0x0400): Searching for parent groups of group...`
After the patch only parent groups of the user should be searched, not parent groups of groups.
I added to the **test_zero_nesting_level** integration test, I don't really think the original test was checking nesting level properly so the extra commit is what I propose to fix it. """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-308877157
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
fidencio commented: """ @justin-stephenson: I'm not sure if I understood properly the changed you proposed for the tests.
I was expecting some changes that would make our test_zero_nesting_level failed without your patch applied, but it doesn't happen at all.
Am I missing something here? Does make sense to expect some changes in order to have the tests failing in case your patch is not applied? """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-309386815
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
lslebodn commented: """
@justin-stephenson: I'm not sure if I understood properly the changed you proposed for the tests.
I was expecting some changes that would make our test_zero_nesting_level failed without your patch applied, but it doesn't happen at all.
I would expect that core developers would give a hint to external contributors what is missing and not ask what is missing :-). I usually try to write test if external contributor didn't write it how to write a test f8d34835b4b97cff751677e911f26eae6a6d7381.
Am I missing something here? Does make sense to expect some changes in order to have the tests failing in case your patch is not applied?
The missing part is that bug is not triggered by `getgrnam/getgrgid` ("getent group") but by `initgroups` (`id -G username`). So you need to test with zero nested level and `sssd_id.get_user_gids`
"""
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-309422976
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
justin-stephenson commented: """ The reason why the integration test succeeds even without the patch in this PR applied is that the `id` and `id -G` commands are filtering out the nested group properly before the patch. Note the missing **nestedgrp** below:
-- Before the patch with **ldap_group_nesting_level = 0**
``` [root@sssd-f25-ad ~]# id posixuser@LDAP uid=10000(posixuser) gid=10001(posixgrp) groups=10001(posixgrp)
``` -- After the patch with **ldap_group_nesting_level = 0**
``` [root@sssd-f25-ad ~]# id posixuser@LDAP uid=10000(posixuser) gid=10001(posixgrp) groups=10001(posixgrp) ```
-- With **ldap_group_nesting_level = 2** ```
[root@sssd-f25-ad ~]# id posixuser@LDAP uid=10000(posixuser) gid=10001(posixgrp) groups=10001(posixgrp),10002(nestedgrp)
``` Here we see that **nestedgrp** is not actually visible even before the patch in this PR so at first it does not seem the patch makes a difference but in fact there is a slight difference. In the non-patched code the nestedgrp is actually discovered with LDAP search and processed but never gets stored in the cache because it is filtered elsewhere in the code. With the patch applied the code to search for this nested group is circumvented and we save some LDAP search operations skipping rfc2307bis_nested_groups_step().
-- Without patch, nesting level 0 ```
[root@sssd-f25-ad ~]# egrep -irn 'nesting_level|rfc2307bis_nested_group' /var/log/sssd/sssd_LDAP.log [sssd[be[LDAP]]] [dp_get_options] (0x0400): Option ldap_group_nesting_level has value 0 [sssd[be[LDAP]]] [rfc2307bis_nested_groups_send] (0x2000): About to process 1 groups in nesting level 0 [sssd[be[LDAP]]] [rfc2307bis_nested_groups_step] (0x1000): Processing group [posixgrp@ldap] [sssd[be[LDAP]]] [rfc2307bis_nested_groups_next_base] (0x0400): Searching for parent groups of group [CN=posixgrp,CN=Users,DC=AD,DC=JSTEPHEN] with base [dc=ad,dc=jstephen] [sssd[be[LDAP]]] [rfc2307bis_nested_groups_process] (0x1000): Found 1 parent groups of [CN=posixgrp,CN=Users,DC=AD,DC=JSTEPHEN] [sssd[be[LDAP]]] [rfc2307bis_nested_groups_process] (0x2000): Total of 1 direct parents after this iteration [sssd[be[LDAP]]] [rfc2307bis_nested_groups_send] (0x2000): About to process 1 groups in nesting level 1
``` -- After patch, nesting level 0
``` [root@sssd-f25-ad ~]# egrep 'nesting_level|rfc2307bis_nested' /var/log/sssd/sssd_LDAP.log [sssd[be[LDAP]]] [dp_get_options] (0x0400): Option ldap_group_nesting_level has value 0 [sssd[be[LDAP]]] [rfc2307bis_nested_groups_send] (0x2000): About to process 1 groups in nesting level 0
```
As the patch leads to skipping some parts of the code and not affecting the initgroups list output, I don't know if it is possible to write a test for this.
I did not drop my changes to the `test_zero_nesting_level` test because I don't see how the original test was performing a sufficient nesting level test.
``` ent_list.add_user("user1", 1001, 2001) ent_list.add_group_bis("group1", 20001, member_uids=["user1"])
ent.assert_group_by_name("group1", dict(mem=ent.contains_only("user1"))) ```
Even with nesting level 0 parent groups should be searched and I believe my suggested changes will improve the test.
To summarize: This `Fix nesting level comparison` patch does not affect the group list in `id` command output but it causes the backend to skip unnecessary nested group LDAP searches and processing. The changes to the `test_zero_nesting_level` test improve the behavior of this test however the test will succeed even without this patch applied. """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-311691402
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
justin-stephenson commented: """ @fidencio to test this I used Active Directory as a basic LDAP server and created a user(posixuser), a parent group(posixgrp), and a nested group(nestedgrp). posixuser is a member of posixgrp and posixgrp is a member of nestedgrp.
I manually added uid/gid attributes to the user and each group and used the following SSSD configuration:
``` [domain/LDAP] id_provider = ldap auth_provider = ldap ldap_uri = ldap://justin-ad2012r2.ad.jstephen ldap_search_base = dc=ad,dc=jstephen ldap_schema = rfc2307bis ldap_tls_reqcert = never cache_credentials = true ldap_group_nesting_level = 0 ldap_user_object_class = person ldap_group_object_class = group ldap_default_bind_dn = CN=Administrator,CN=Users,DC=AD,DC=JSTEPHEN ldap_default_authtok = mypassword timeout = 3600 debug_level = 9
``` After the patch, the parent groups of posixgrp should not be searched - this line should **not** be in the logs:
`[sssd[be[LDAP]]] [rfc2307bis_nested_groups_next_base] (0x0400): Searching for parent groups of group [CN=posixgrp,CN=Users,DC=AD,DC=JSTEPHEN] with base [dc=ad,dc=jstephen]` """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-311692281
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
fidencio commented: """ @justin-stephenson: Thanks a lot for the really detailed explanation.
So, patches look good. There's just one nitpick about the commit message on the patch touching the tests themselves.
TESTS: Update zero nesting level test - You can drop the "Resolves: ..." part of the commit message - Would be really nice to add a comment to the tests (like the one in https://github.com/SSSD/sssd/pull/300#issuecomment-311691402) just to have explicitly written there what exactly the test covers, what is not covered and why (again, your comment is really good for it). """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-312657951
URL: https://github.com/SSSD/sssd/pull/300 Author: justin-stephenson Title: #300: LDAP: Fix nesting level comparison Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/300/head:pr300 git checkout pr300
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
justin-stephenson commented: """ @fidencio thanks for the review, changes have been made. """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-312715606
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
fidencio commented: """ ACK! """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-314126336
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
jhrozek commented: """ * master: * 6d57cd501c28aa52731c56cd751bbc404f991ae0 * 925a14d50edf0e3b800ce659b10b771ae1cde293 """
See the full comment at https://github.com/SSSD/sssd/pull/300#issuecomment-314396978
URL: https://github.com/SSSD/sssd/pull/300 Author: justin-stephenson Title: #300: LDAP: Fix nesting level comparison Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/300/head:pr300 git checkout pr300
URL: https://github.com/SSSD/sssd/pull/300 Title: #300: LDAP: Fix nesting level comparison
Label: +Pushed
sssd-devel@lists.fedorahosted.org
-
centos-ci -
fidencio
-
jhrozek
-
justin-stephenson
-
lslebodn