rpms/389-ds-base
6
0
Fork 0
Code Issues Pull Requests Releases Activity
b4fc291a94
389-ds-base/SOURCES/0043-Issue-6571-Nested-group-does-not-receive-memberOf-at.patch

292 lines
13 KiB
Diff
Raw Blame History

From 8d62124fb4d0700378b6f0669cc9d47338a8151c Mon Sep 17 00:00:00 2001
From: tbordaz <tbordaz@redhat.com>
Date: Tue, 25 Mar 2025 09:20:50 +0100
Subject: [PATCH] Issue 6571 - Nested group does not receive memberOf attribute
(#6679)
Bug description:
There is a risk to create a loop in group membership.
For example G2 is member of G1 and G1 is member of G2.
Memberof plugins iterates from a node to its ancestors
to update the 'memberof' values of the node.
The plugin uses a valueset ('already_seen_ndn_vals')
to keep the track of the nodes it already visited.
It uses this valueset to detect a possible loop and
in that case it does not add the ancestor as the
memberof value of the node.
This is an error in case there are multiples paths
up to an ancestor.
Fix description:
The ancestor should be added to the node systematically,
just in case the ancestor is in 'already_seen_ndn_vals'
it skips the final recursion
fixes: #6571
Reviewed by: Pierre Rogier, Mark Reynolds (Thanks !!!)
---
.../suites/memberof_plugin/regression_test.py | 109 ++++++++++++++++++
.../tests/suites/plugins/memberof_test.py | 5 +
ldap/servers/plugins/memberof/memberof.c | 52 ++++-----
3 files changed, 137 insertions(+), 29 deletions(-)
diff --git a/dirsrvtests/tests/suites/memberof_plugin/regression_test.py b/dirsrvtests/tests/suites/memberof_plugin/regression_test.py
index 4c681a909..dba908975 100644
--- a/dirsrvtests/tests/suites/memberof_plugin/regression_test.py
+++ b/dirsrvtests/tests/suites/memberof_plugin/regression_test.py
@@ -467,6 +467,21 @@ def _find_memberof_ext(server, user_dn=None, group_dn=None, find_result=True):
else:
assert (not found)
+def _check_membership(server, entry, expected_members, expected_memberof):
+ assert server
+ assert entry
+
+ memberof = entry.get_attr_vals('memberof')
+ member = entry.get_attr_vals('member')
+ assert len(member) == len(expected_members)
+ assert len(memberof) == len(expected_memberof)
+ for e in expected_members:
+ server.log.info("Checking %s has member %s" % (entry.dn, e.dn))
+ assert e.dn.encode() in member
+ for e in expected_memberof:
+ server.log.info("Checking %s is member of %s" % (entry.dn, e.dn))
+ assert e.dn.encode() in memberof
+
@pytest.mark.ds49161
def test_memberof_group(topology_st):
@@ -535,6 +550,100 @@ def test_memberof_group(topology_st):
_find_memberof_ext(inst, dn1, g2n, True)
_find_memberof_ext(inst, dn2, g2n, True)
+def test_multipaths(topology_st, request):
+ """Test memberof succeeds to update memberof when
+ there are multiple paths from a leaf to an intermediate node
+
+ :id: 35aa704a-b895-4153-9dcb-1e8a13612ebf
+
+ :setup: Single instance
+
+ :steps:
+ 1. Create a graph G1->U1, G2->G21->U1
+ 2. Add G2 as member of G1: G1->U1, G1->G2->G21->U1
+ 3. Check members and memberof in entries G1,G2,G21,User1
+
+ :expectedresults:
+ 1. Graph should be created
+ 2. succeed
+ 3. Membership is okay
+ """
+
+ inst = topology_st.standalone
+ memberof = MemberOfPlugin(inst)
+ memberof.enable()
+ memberof.replace('memberOfEntryScope', SUFFIX)
+ if (memberof.get_memberofdeferredupdate() and memberof.get_memberofdeferredupdate().lower() == "on"):
+ delay = 3
+ else:
+ delay = 0
+ inst.restart()
+
+ #
+ # Create the hierarchy
+ #
+ #
+ # Grp1 ---------------> User1
+ # ^
+ # /
+ # Grp2 ----> Grp21 ------/
+ #
+ users = UserAccounts(inst, SUFFIX, rdn=None)
+ user1 = users.create(properties={'uid': "user1",
+ 'cn': "user1",
+ 'sn': 'SN',
+ 'description': 'leaf',
+ 'uidNumber': '1000',
+ 'gidNumber': '2000',
+ 'homeDirectory': '/home/user1'
+ })
+ group = Groups(inst, SUFFIX, rdn=None)
+ g1 = group.create(properties={'cn': 'group1',
+ 'member': user1.dn,
+ 'description': 'group1'})
+ g21 = group.create(properties={'cn': 'group21',
+ 'member': user1.dn,
+ 'description': 'group21'})
+ g2 = group.create(properties={'cn': 'group2',
+ 'member': [g21.dn],
+ 'description': 'group2'})
+
+ # Enable debug logs if necessary
+ #inst.config.replace('nsslapd-errorlog-level', '65536')
+ #inst.config.set('nsslapd-accesslog-level','260')
+ #inst.config.set('nsslapd-plugin-logging', 'on')
+ #inst.config.set('nsslapd-auditlog-logging-enabled','on')
+ #inst.config.set('nsslapd-auditfaillog-logging-enabled','on')
+
+ #
+ # Update the hierarchy
+ #
+ #
+ # Grp1 ----------------> User1
+ # \ ^
+ # \ /
+ # --> Grp2 --> Grp21 --
+ #
+ g1.add_member(g2.dn)
+ time.sleep(delay)
+
+ #
+ # Check G1, G2, G21 and User1 members and memberof
+ #
+ _check_membership(inst, g1, expected_members=[g2, user1], expected_memberof=[])
+ _check_membership(inst, g2, expected_members=[g21], expected_memberof=[g1])
+ _check_membership(inst, g21, expected_members=[user1], expected_memberof=[g2, g1])
+ _check_membership(inst, user1, expected_members=[], expected_memberof=[g21, g2, g1])
+
+ def fin():
+ try:
+ user1.delete()
+ g1.delete()
+ g2.delete()
+ g21.delete()
+ except:
+ pass
+ request.addfinalizer(fin)
def _config_memberof_entrycache_on_modrdn_failure(server):
diff --git a/dirsrvtests/tests/suites/plugins/memberof_test.py b/dirsrvtests/tests/suites/plugins/memberof_test.py
index 2de1389fd..621c45daf 100644
--- a/dirsrvtests/tests/suites/plugins/memberof_test.py
+++ b/dirsrvtests/tests/suites/plugins/memberof_test.py
@@ -2168,9 +2168,14 @@ def test_complex_group_scenario_6(topology_st):
# add Grp[1-4] (uniqueMember) to grp5
# it creates a membership loop !!!
+ topology_st.standalone.config.replace('nsslapd-errorlog-level', '65536')
mods = [(ldap.MOD_ADD, 'uniqueMember', memofegrp020_5)]
for grp in [memofegrp020_1, memofegrp020_2, memofegrp020_3, memofegrp020_4]:
topology_st.standalone.modify_s(ensure_str(grp), mods)
+ topology_st.standalone.config.replace('nsslapd-errorlog-level', '0')
+
+ results = topology_st.standalone.ds_error_log.match('.*detecting a loop in group.*')
+ assert results
time.sleep(5)
# assert user[1-4] are member of grp20_[1-4]
diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
index e75b99b14..32bdcf3f1 100644
--- a/ldap/servers/plugins/memberof/memberof.c
+++ b/ldap/servers/plugins/memberof/memberof.c
@@ -1592,7 +1592,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
ht_grp = ancestors_cache_lookup(config, (const void *)ndn);
if (ht_grp) {
#if MEMBEROF_CACHE_DEBUG
- slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);
+ slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%lx)\n", ndn, (ulong) ht_grp);
#endif
add_ancestors_cbdata(ht_grp, callback_data);
*cached = 1;
@@ -1600,7 +1600,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
}
}
#if MEMBEROF_CACHE_DEBUG
- slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s not cached\n", ndn);
+ slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s not cached\n", slapi_sdn_get_ndn(sdn));
#endif
/* Escape the dn, and build the search filter. */
@@ -3233,7 +3233,8 @@ cache_ancestors(MemberOfConfig *config, Slapi_Value **member_ndn_val, memberof_g
return;
}
#if MEMBEROF_CACHE_DEBUG
- if (double_check = ancestors_cache_lookup(config, (const void*) key)) {
+ double_check = ancestors_cache_lookup(config, (const void*) key);
+ if (double_check) {
dump_cache_entry(double_check, "read back");
}
#endif
@@ -3263,13 +3264,13 @@ merge_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *v1, memb
sval_dn = slapi_value_new_string(slapi_value_get_string(sval));
if (sval_dn) {
/* Use the normalized dn from v1 to search it
- * in v2
- */
+ * in v2
+ */
val_sdn = slapi_sdn_new_dn_byval(slapi_value_get_string(sval_dn));
sval_ndn = slapi_value_new_string(slapi_sdn_get_ndn(val_sdn));
if (!slapi_valueset_find(
((memberof_get_groups_data *)v2)->config->group_slapiattrs[0], v2_group_norm_vals, sval_ndn)) {
-/* This ancestor was not already present in v2 => Add it
+ /* This ancestor was not already present in v2 => Add it
* Using slapi_valueset_add_value it consumes val
* so do not free sval
*/
@@ -3318,7 +3319,7 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn, memberof_get
merge_ancestors(&member_ndn_val, &member_data, data);
if (!cached && member_data.use_cache)
- cache_ancestors(config, &member_ndn_val, &member_data);
+ cache_ancestors(config, &member_ndn_val, data);
slapi_value_free(&member_ndn_val);
slapi_valueset_free(groupvals);
@@ -3379,25 +3380,6 @@ memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
goto bail;
}
- /* Have we been here before? Note that we don't loop through all of the group_slapiattrs
- * in config. We only need this attribute for it's syntax so the comparison can be
- * performed. Since all of the grouping attributes are validated to use the Dinstinguished
- * Name syntax, we can safely just use the first group_slapiattr. */
- if (slapi_valueset_find(
- ((memberof_get_groups_data *)callback_data)->config->group_slapiattrs[0], already_seen_ndn_vals, group_ndn_val)) {
- /* we either hit a recursive grouping, or an entry is
- * a member of a group through multiple paths. Either
- * way, we can just skip processing this entry since we've
- * already gone through this part of the grouping hierarchy. */
- slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM,
- "memberof_get_groups_callback - Possible group recursion"
- " detected in %s\n",
- group_ndn);
- slapi_value_free(&group_ndn_val);
- ((memberof_get_groups_data *)callback_data)->use_cache = PR_FALSE;
- goto bail;
- }
-
/* if the group does not belong to an excluded subtree, adds it to the valueset */
if (memberof_entry_in_scope(config, group_sdn)) {
/* Push group_dn_val into the valueset. This memory is now owned
@@ -3407,9 +3389,21 @@ memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
group_dn_val = slapi_value_new_string(group_dn);
slapi_valueset_add_value_ext(groupvals, group_dn_val, SLAPI_VALUE_FLAG_PASSIN);
- /* push this ndn to detect group recursion */
- already_seen_ndn_val = slapi_value_new_string(group_ndn);
- slapi_valueset_add_value_ext(already_seen_ndn_vals, already_seen_ndn_val, SLAPI_VALUE_FLAG_PASSIN);
+ if (slapi_valueset_find(
+ ((memberof_get_groups_data *)callback_data)->config->group_slapiattrs[0], already_seen_ndn_vals, group_ndn_val)) {
+ /* The group group_ndn_val has already been processed
+ * skip the final recursion to prevent infinite loop
+ */
+ slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM,
+ "memberof_get_groups_callback - detecting a loop in group %s (stop building memberof)\n",
+ group_ndn);
+ ((memberof_get_groups_data *)callback_data)->use_cache = PR_FALSE;
+ goto bail;
+ } else {
+ /* keep this ndn to detect a possible group recursion */
+ already_seen_ndn_val = slapi_value_new_string(group_ndn);
+ slapi_valueset_add_value_ext(already_seen_ndn_vals, already_seen_ndn_val, SLAPI_VALUE_FLAG_PASSIN);
+ }
}
if (!config->skip_nested || config->fixup_task) {
/* now recurse to find ancestors groups of e */
--
2.49.0
Reference in New Issue View Git Blame Copy Permalink