LDAP: Fix nesting level comparison

[justin-stephenson]

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

[centos-ci]

Can one of the admins verify this patch?

[centos-ci]

Can one of the admins verify this patch?

[fidencio]

ok to test

[fidencio]

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

[fidencio]

CI: http://sssd-ci.duckdns.org/logs/job/71/12/summary.html

[lslebodn]

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

We have a single test for nesting level zero. But we check level on few places (3 IIRC) so there might and this codepath was not covered by existing test.

[justin-stephenson]

@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 for a user that is a member of nested groups.

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(nesting level 0), 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.

[fidencio]

@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?

[lslebodn]

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

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

[justin-stephenson]

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.

[justin-stephenson]

@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]

[fidencio]

@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 LDAP: Fix nesting level comparison #300 (comment)) 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).

[justin-stephenson]

@fidencio thanks for the review, changes have been made.

[fidencio]

ACK!

[jhrozek]

• master:
• 6d57cd5
• 925a14d