#47526 Allow memberOf suffixes to be configurable
Closed: wontfix None Opened 10 years ago by nkinder.

The memberOf plug-in currently doesn't allow you to restrict the suffixes it applies to. It would be nice to be able to list the suffixes to include or exclude for memberOf operations. There are a few scenarios to consider:

  • If a user that is outside of one of the configured suffixes is added as a member to a group that is within one of the configured suffixes, no memberOf attribute should be added to the user. This is the same behavior that happens today when you add a non-existent user DN as a member in a group.

  • If a user entry is moved outside of one of the configured suffixes, any groups that reference the member would need to have the member attribute removed for that user, and the memberOf values would also need to be removed from the user.

  • If a group is moved outside of one of the configured suffixes, any memberOf attributes that reference that group inside of user entries within the configured suffixes should be removed. We need to decide if the member values should also be removed from the group or left as is.

This functionality is needed by FreeIPA for a user provisioning feature that is being designed.


The ticket description mixes two different requests:
1] to apply scoping to memberof
2] to mainatain references in groups if entries are moved in or out of scope

I think 2] should be the task of referential integrity plugin (when extended to handle scoping) and the memberof plugin should be contained to handle group references in entries

Replying to [comment:5 lkrispen]:

The ticket description mixes two different requests:
1] to apply scoping to memberof
2] to mainatain references in groups if entries are moved in or out of scope

I think 2] should be the task of referential integrity plugin (when extended to handle scoping) and the memberof plugin should be contained to handle group references in entries

There is already some precedent for having the memberOf plug-in maintain references in group entries.

The memberof_postop_modrdn() function currently handles the case where a member entry is renamed. When a member entry is renamed, the memberOf plug-in fixes up any references that groups have to the member.

The memberof_postop_del() function will removes any "member" attribute references in groups when a member entry is deleted. This is the exact same logic that needs to occur for the MODRDN case mentioned in #2 above.

side track: the memberof attribute requires the inetuser objectclass. If an entry doesn't have this oc and is added to a group, no memberof is displayed.
When adding the inetuser oc to the entry it still doesn't show up, only after a fixup run - shouldn't this happen in the modify postop when the oc is added?

Replying to [comment:7 lkrispen]:

side track: the memberof attribute requires the inetuser objectclass. If an entry doesn't have this oc and is added to a group, no memberof is displayed.
When adding the inetuser oc to the entry it still doesn't show up, only after a fixup run - shouldn't this happen in the modify postop when the oc is added?

We could check, but it wasn't done to try to avoid parsing through any modify operation that touches the objectclass attribute. The idea was to have the plug-in only be triggered when an actual membership attribute is touched, and to avoid going through other write operations as much as possible.

One more side track and clarification about how to apply scope

Side track: The memberof is an attribute in the user entry and maintains a 1:1 relationship of users and group membership. Now as a real attribute, memberof can directly be modified in the user entry: adding or deleting values or deleting the attribute. It would be an option to keep the group membership in sync in these cases by modifying the memberof groups – so mebership could be maintained either via user or group entries. But again this could be too costly.

The request of this ticket is to narrow the scope of users/groups in memberof handling, but there also exists the memberofAllBackends attribute, which extends the scope of memberof handling, so there could be conflicting configurations. I would suggest that in case of memberofAllBackends: on the definition of memberof scope is ignored and only a warning logged.

For the application of memberof scope assume the following scenario:
Backend: dc=com
subtrees:
ou=in,dc=com
ou=out,dc=com
ou=groups,ou=in,dc=com
ou=people,ou=in,dc=com
ou=groups,ou=out,dc=com
ou=people,ou=out,dc=com and let the memberof scope be defined as: “ou=in,dc=com”

So there are four different types of member operations:
1] add user=x,ou=people,ou=in,dc=com to cn=g1,ou=groups,ou=in,dc=com
2] add user=x,ou=people,ou=out,dc=com to cn=g1,ou=groups,ou=in,dc=com
3] add user=x,ou=people,ou=in,dc=com to cn=g1,ou=groups,ou=out,dc=com
4] add user=x,ou=people,ou=out,dc=com to cn=g1,ou=groups,ou=out,dc=com

which of these should create a memberof attribute.
1]- yes and 4]- no are clear, but I am unsure about 2] and 3]
Do we require the user and the group to be in scope or does one of them in scope trigger the creation ?
Or do we need two scope definitions, one for groups and one for users ?

Replying to [comment:9 lkrispen]:

One more side track and clarification about how to apply scope

Side track: The memberof is an attribute in the user entry and maintains a 1:1 relationship of users and group membership. Now as a real attribute, memberof can directly be modified in the user entry: adding or deleting values or deleting the attribute. It would be an option to keep the group membership in sync in these cases by modifying the memberof groups – so mebership could be maintained either via user or group entries. But again this could be too costly.

This issue came up early in the memberOf plug-in design, and it was decided that membership must only be maintained from the group side to avoid complexity. That is, a user should only ever update "member" attributes when using the memberOf plugin. The "memberOf" attributes should be considered as owned by the server, and setting an ACI to restrict access is a good idea. We didn't want to hardcode the access restriction in case an administrator ever needed to manually clean up a "memberOf" attribute in case of a bug or inconsistency.

The request of this ticket is to narrow the scope of users/groups in memberof handling, but there also exists the memberofAllBackends attribute, which extends the scope of memberof handling, so there could be conflicting configurations. I would suggest that in case of memberofAllBackends: on the definition of memberof scope is ignored and only a warning logged.

Initially, memberOf references would not work when the group was in one backend and the user was in another. The "memberofAllBackends" setting was added to allow these cross-backend membership references to work.

I am not sure that the "memberOfScope" and "memberOfAllBackends" settings should be mutually exclusive. Consider the following case with nested backends:

{{{
backend1 - "dc=example,dc=com"
backend2 - "cn=users,dc=example,dc=com"
backend3 - "dc=other,dc=com"
}}}

In this case, I may want to restrict my "memberOfScope" to "dc=example,dc=com". I also might have my users in "cn=users,dc=example,dc=com" (backend2), and my groups in "cn=groups,dc=example,dc=com" (backend1). For these cross-backend membership references to work, I would need to set "memberOfAllBackends" to on.

For the application of memberof scope assume the following scenario:
Backend: dc=com
subtrees:
ou=in,dc=com
ou=out,dc=com
ou=groups,ou=in,dc=com
ou=people,ou=in,dc=com
ou=groups,ou=out,dc=com
ou=people,ou=out,dc=com and let the memberof scope be defined as: “ou=in,dc=com”

Let me make sure I understand the scenario correctly. We have a single backend, and the new "memberOfScope" setting is set to "ou=in,dc=com"? My answers below will assume that my understanding is correct.

So there are four different types of member operations:
1] add user=x,ou=people,ou=in,dc=com to cn=g1,ou=groups,ou=in,dc=com
2] add user=x,ou=people,ou=out,dc=com to cn=g1,ou=groups,ou=in,dc=com
3] add user=x,ou=people,ou=in,dc=com to cn=g1,ou=groups,ou=out,dc=com
4] add user=x,ou=people,ou=out,dc=com to cn=g1,ou=groups,ou=out,dc=com

which of these should create a memberof attribute.
1]- yes and 4]- no are clear, but I am unsure about 2] and 3]
Do we require the user and the group to be in scope or does one of them in scope trigger the creation ?
Or do we need two scope definitions, one for groups and one for users ?

1] The memberOf attribute should be added to "user=x,ou=people,ou=in,dc=com"
2] No memberOf operation is performed.
3] No memberOf operation is performed.
4] No memberof operation is performed.

The behavior is that the group entry and the member entry must both exist within the defined scope. In cases 2 and 3, only one entry is within the scope, so no operation should be performed.

Guys, could you push that to 1.3.2.x in Fedora 20? FreeIPA would need this for performance reasons. Retro changelog trimming is pretty slow without this.

Changes look good to me. ack

2 minor questions...
1) Before theConfig.entryScope is overwritten at the line 479, 482, and 485, the previous value (if any) needs to be freed by slapi_sdn_free?
2) Why slapi_sdn_new_dn_byref is called instead of slapi_sdn_new_dn_passin? If entryScope is updated, slapi_sdn_free does not free if it's from "_byref"... Could the entryScope string be leaked?
{{{
… … memberof_apply_config (Slapi_PBlock pb, Slapi_Entry entryBefore, Slapi_Entry*
473 if (entryScope)
474 {
475 if (slapi_dn_syntax_check(NULL, entryScope, 1) == 1) {
476 slapi_log_error(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM,
477 "Error: Ignoring invalid DN used as plugin entry scope: [%s]\n",
478 entryScope);
479 theConfig.entryScope = NULL;
480 slapi_ch_free_string(&entryScope);
481 } else {
482 theConfig.entryScope = slapi_sdn_new_dn_byref(entryScope);
483 }
484 } else {
485 theConfig.entryScope = NULL;
486 }
}}}

Noriko, I think you are right with passin if we want to free the sdn, and it is probably safer to free theConfig.entryScope before reassigning - I just had thought that this would be only called at startup.
But the other members of th econfig struct are also freed, so lets do it for entryscope as well.
But I didn't find that theConfig was initialized, so just freeing could be harmful, I added an initialization.

Interestingly a coverty scan did not report any of these problems.

Should I add the changes below ?

--- a/ldap/servers/plugins/memberof/memberof_config.c
+++ b/ldap/servers/plugins/memberof/memberof_config.c
@@ -77,7 +77,7 @@ static int memberof_search (Slapi_PBlock pb, Slapi_Entry entryBefore, Slapi_En
/ This is the main configuration which is updated from dse.ldif. The
* config will be copied when it is used by the plug-in to prevent it
* being changed out from under a running memberOf operation.
/
-static MemberOfConfig theConfig;
+static MemberOfConfig theConfig = {NULL, NULL,0, NULL, NULL, NULL};
static Slapi_RWLock *memberof_config_lock = 0;
static int inited = 0;

@@ -470,6 +470,8 @@ memberof_apply_config (Slapi_PBlock pb, Slapi_Entry entryBefore, Slapi_Entry
} else {
theConfig.allBackends = 0;
}
+
+ slapi_sdn_free(&theConfig.entryScope);
if (entryScope)
{
if (slapi_dn_syntax_check(NULL, entryScope, 1) == 1) {
@@ -479,7 +481,7 @@ memberof_apply_config (Slapi_PBlock
pb, Slapi_Entry entryBefore, Slapi_Entry
theConfig.entryScope = NULL;
slapi_ch_free_string(&entryScope);
} else {
- theConfig.entryScope = slapi_sdn_new_dn_byref(entryScope);
+ theConfig.entryScope = slapi_sdn_new_dn_passin(entryScope);
}
} else {
theConfig.entryScope = NULL;

But I didn't find that theConfig was initialized, so just freeing could be harmful, I added an initialization. -static MemberOfConfig? theConfig;
+static MemberOfConfig? theConfig = {NULL, NULL,0, NULL, NULL, NULL};

Since theConfig is static, we could safely assume it's initialized to NULL. But I also think having the explicit initialization makes the code clearer.
{{{
Static-duration variables (global and static local) are guaranteed to be initialized to 0 if you do not use an explicit initializer in the definition.
}}}

You have my ack. Thank you, Ludwig!

By the way, I have tested backported patch on top of 1.3.2.7 in Fedora 20 and it solved by performance problems with retro change log trimming.

I have added following configuration:
{{{
memberofentryscope: dc=ipa,dc=test
}}}

I hope that the configuration is valid :-) My intention was to limit all plugin operations to dc=ipa,dc=test subtree so any change in cn=changelog will not trigger the plugin.

$ git merge ticket47526
Updating 5d60dab..11898c3
Fast-forward
ldap/servers/plugins/memberof/memberof.c | 29 ++++++++++++++++++++++++++++-
ldap/servers/plugins/memberof/memberof.h | 3 +++
ldap/servers/plugins/memberof/memberof_config.c | 32 +++++++++++++++++++++++++++++++-
3 files changed, 62 insertions(+), 2 deletions(-)
$ git push origin master
Counting objects: 17, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.88 KiB, done.
Total 9 (delta 6), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
5d60dab..11898c3 master -> master

Hi Ludwig, is it okay to cherry-pick the commit on 389-ds-base-1.3.3 and 1.3.2?

Replying to [comment:21 nhosoi]:

Hi Ludwig, is it okay to cherry-pick the commit on 389-ds-base-1.3.3 and 1.3.2?

it will first need to be reviewed and committed

1) Considering the original code, the if-clause (line 907 ~ 922) is supposed to be run only when "ret == LDAP_SUCCESS"? Or is it intentional? Please note if it is a non-group, it's checked at the line 925.

2) Might be a minor issue, but slapi_filter_test_simple returns these 3 kinds of return value.
* returns 0 filter matched
* -1 filter did not match
* >0 an ldap error code
If it fails due to some error (>0), it goes to the non-group case. Is it okay?

{{{

899 if(ret == LDAP_SUCCESS && (ret = memberof_del_dn_type_callback(post_e, &del_data))){
900 slapi_log_error( SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM,
901 "memberof_postop_modrdn - delete dn callback failed for (%s), error (%d)\n",
902 slapi_entry_get_dn(post_e), ret);
904 / is the entry of interest as a group? /
905 if(pre_e && configCopy.group_filter && !slapi_filter_test_simple(pre_e, configCopy.group_filter))
906 {
}}}

ad 2]
you're right,I had just copied the code. Fixed calls to filter test in the memberof plugin code, but there are other places also not correctly checking the return code of slapi_filter_test_simple(), but did not handle in this ticket

ad 1] I think the code would be ok, as in the group case it is checked in the for loop, but it could be more clear.

new patch attached

Ludwig,

The post_e is going out of the scope (or exclude) of memberof plugin. My understanding is that we remove post_e 'memberof' attribute values only if the entry is not a group. Am I missing something ?
Shouldn't we do both: if the entry is a group then update the entries belonging to the group. Then in all case update memberof of post_e (whether it is a group or not).

thanks
thierry

Replying to [comment:26 tbordaz]:

you're right. need a new version of the patch

added new patch for the latest issue.

commit in master:
New commits:
commit c46ccea
commit 31f4f34

commits in 1.3.3:

commit c895166
commit c895166

correction, latest update containe the same commit twice, second one should be:

commit 4760dc6

The last patch fixes the tickets
https://fedorahosted.org/389/ticket/48012
https://fedorahosted.org/389/ticket/47833

They are both closed as duplicate of this ticket 47526.

Adding the test case of 47833 as it is the only available test case for these tickets.
This test case verify 48012 and 47833 but only part of 47526.

Metadata Update from @lkrispen:
- Issue assigned to lkrispen
- Issue set to the milestone: 1.3.3.6

7 years ago

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/863

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata