search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 91 forks source link

Allow memberOf suffixes to be configurable #863

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/47526

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:

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

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2013-11-22 19:21:16

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

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-11-22 21:29:31

Replying to [comment:5 elkris]:

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.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2013-11-25 21:21:41

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?

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-11-25 21:40:15

Replying to [comment:7 elkris]:

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.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2013-11-26 20:27:22

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 ?

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-11-26 22:06:26

Replying to [comment:9 elkris]:

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.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2013-12-03 15:20:40

attachment 0001-Ticket-47526-Allow-memberof-suffixes-to-be-configura.patch

389-ds-bot commented 4 years ago

Comment from pspacek at 2013-12-03 20:38:51

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.

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2013-12-03 22:01:35

Changes look good to me. ack

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2013-12-04 00:18:59

me too

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2013-12-04 00:21:21

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         }
389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2013-12-04 18:15:52

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

@@ -470,6 +470,8 @@ memberof_apply_config (Slapi_PBlock pb, Slapi_Entry entryBefore, Slapi_Entry* } else { theConfig.allBackends = 0; } +

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2013-12-04 23:28:08

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!

389-ds-bot commented 4 years ago

Comment from pspacek at 2013-12-05 19:01:20

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.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2013-12-05 19:55:42

$ 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

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-12-18 03:39:20

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1044170

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-12-16 01:05:31

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

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-16 14:43:31

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

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-16 14:49:08

attachment 0001-Additional-fix-for-ticket-47526.patch

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-12-17 00:38:07

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.


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         {
389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-17 16:07:04

attachment 0001-Additional-fix-for-ticket-47526-v2.patch

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-17 16:12:06

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

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2014-12-17 19:56:07

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

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-17 20:22:22

Replying to [comment:26 tbordaz]:

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

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-17 23:15:28

attachment 0001-Additional-fix-for-ticket-47526-v3.patch

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2014-12-17 23:16:10

added new patch for the latest issue.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2015-01-22 16:41:05

commit in master: New commits: commit c46cceae924459118588b61e3f3aa988be81e71c commit 31f4f341b8f1d2ae04ae1606f0822169201d2737

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2015-01-22 16:51:19

commits in 1.3.3:

commit c8951669b3a74cb9fda013ffe914031b76e2e452 commit c8951669b3a74cb9fda013ffe914031b76e2e452

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2015-01-22 16:52:53

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

commit 4760dc6c66249a793c8485a23c147954265f6fd3

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2015-06-16 17:39:23

attachment ticket47833_test.py

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2015-06-16 17:42:13

The last patch fixes the tickets

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.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2017-02-11 22:57:06

Metadata Update from @elkris: