In IPA we configure memberof to work on the member attribute. This works for the majority of our group types (for users, hosts, etc). Our configuration looks like this: dn: cn=MemberOf Plugin,cn=plugins,cn=config objectClass: top objectClass: nsSlapdPlugin objectClass: extensibleObject cn: MemberOf Plugin nsslapd-pluginPath: libmemberof-plugin nsslapd-pluginInitfunc: memberof_postop_init nsslapd-pluginType: postoperation nsslapd-pluginEnabled: on nsslapd-plugin-depends-on-type: database memberofgroupattr: member memberofattr: memberOf nsslapd-pluginId: memberof nsslapd-pluginVersion: 1.2.6.rc3 nsslapd-pluginVendor: 389 Project nsslapd-pluginDescription: memberof plugin For netgroups we define two additional member attributes: memberHost and memberUser. We would like memberOf to operate on these attributes as well. I would be fine to modify the plugin to allow only a single memberOf config entry with multiple memberOfGroupAttr settings.
I put some design notes about this enhancement on the 389 wiki: http://directory.fedoraproject.org/wiki/MemberOf_Multiple_Grouping_Enhancements
Created attachment 439237 [details] Proposed Patch
I have some minor requests and questions... The rest looks good. 1. https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec8 It might not matter at all, but just want to have a confirmation... memberof_del_dn_from_groups returns the rc from the last memberof_call_foreach_dn, where the error return is ignored in the loop. Is it okay? 393 rc = memberof_call_foreach_dn(pb, dn, groupattrs, 394 memberof_del_dn_type_callback, &data); Same for this section: https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec10 2. https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec5 Forgive me. Now my eyes are contaminated by coverity... :) It'd be nice if you could add checking NULL types here... 469 for (num_types = 0; types[num_types]; num_types++) ==> 469 for (num_types = 0; types && types[num_types]; num_types++) Same for this section: https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2 294 for (num_groupattrs = 0; theConfig.groupattrs[num_groupattrs]; num_groupattrs++) ==> 294 for (num_groupattrs = 0; theConfig.groupattrs && theConfig.groupattrs[num_groupattrs]; num_groupattrs++) 309 for (i = 0; theConfig.groupattrs[i]; i++) ==> 309 for (i = 0; theConfig.groupattrs && theConfig.groupattrs[i]; i++) 3. https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec1 This is just an RFE. It'd be nice if this message tells which attribute is violating the dn syntax requirement... 202 if (not_dn_syntax) 203 { 204 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, 205 "The %s configuration attribute must be set to " 206 "an attribute defined to use the Distinguished 207 "Name syntax.", 208 MEMBEROF_GROUP_ATTR); 209 } And can we move slapi_attr_new out from the loop to reduce the allocation count? 187 test_attr = slapi_attr_new(); 4. https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2 I wonder what happens if slapi_str2filter returns NULL for some reason (I guess it will not happen, tho...) 346 theConfig.group_filter = slapi_str2filter(filter_str);
(In reply to comment #5) > I have some minor requests and questions... The rest looks good. > > 1. > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec8 > It might not matter at all, but just want to have a confirmation... > memberof_del_dn_from_groups returns the rc from the last > memberof_call_foreach_dn, where the error return is ignored in the loop. Is it > okay? > > 393 rc = memberof_call_foreach_dn(pb, dn, groupattrs, > 394 memberof_del_dn_type_callback, &data); The return value of memberof_del_dn_from_groups is never used. I will change the return type to void for this function and remove the rc variable. > > Same for this section: > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec10 I'll change this return type to void and remove the usage of rc as well. > > 2. > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec5 > Forgive me. Now my eyes are contaminated by coverity... :) It'd be nice if > you could add checking NULL types here... > 469 for (num_types = 0; types[num_types]; num_types++) > ==> > 469 for (num_types = 0; types && types[num_types]; num_types++) Sure. > > Same for this section: > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2 > 294 for (num_groupattrs = 0; theConfig.groupattrs[num_groupattrs]; > num_groupattrs++) > ==> > 294 for (num_groupattrs = 0; theConfig.groupattrs && > theConfig.groupattrs[num_groupattrs]; num_groupattrs++) > > 309 for (i = 0; theConfig.groupattrs[i]; i++) > ==> > 309 for (i = 0; theConfig.groupattrs && theConfig.groupattrs[i]; i++) Sure. > > 3. > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec1 > This is just an RFE. It'd be nice if this message tells which attribute is > violating the dn syntax requirement... > 202 if (not_dn_syntax) > 203 { > 204 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, > 205 "The %s configuration attribute must be set to " > 206 "an attribute defined to use the Distinguished 207 "Name syntax.", > 208 MEMBEROF_GROUP_ATTR); > 209 } Good idea. We should probably do this for the MEMBEROF_ATTR check as well, even though it only works with a single value. > > And can we move slapi_attr_new out from the loop to reduce the allocation > count? > 187 test_attr = slapi_attr_new(); I had looked into doing this initially, but slapi_attr_init() is for initializing an empty Slapi_Attr that has been created by slapi_attr_new(). The contents of an existing Slapi_Attr are not free'd first. There is no public SLAPI function to clear out the contents of an existing Slapi_Attr for reuse. There is a private attr_done() function which we could expose, but I thought that it wasn't worth doing for this issue since we only execute this code when the server is started or when the memberOf plug-in configuration is modified. If you feel that it is worthwhile to make these changes, I can. > > 4. > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2 > I wonder what happens if slapi_str2filter returns NULL for some reason (I guess > it will not happen, tho...) > 346 theConfig.group_filter = slapi_str2filter(filter_str); I'm not sure how/if that could happen since we validate the attribute names used to build the filter and build the filter ourselves in the plug-in. If it can happen, we would end up passing a NULL filter to slapi_filter_test_simple() when performing group checks in the memberOf plug-in, which would cause a server crash. The return value of memberof_apply_config() is not checked. It is assumed that memberof_validate_config() already caught any config problems that would cause issues, but memberof_validate_config is not responsible for building a Slapi_Filter. I can make memberof_apply_config log an error message if it is unable to build the filter and add NULL checking in the 4 places we use the filter to perform group checks. The plug-in will not work properly in this case as it will be unable to detect if an entry is a group, but that is better than crashing. One would see the error message and be able to fix their memberOf configuration to get things working again. Does this approach sound OK?
(In reply to comment #6) > (In reply to comment #5) > > 3. > I had looked into doing this initially, but slapi_attr_init() is for > initializing an empty Slapi_Attr that has been created by slapi_attr_new(). > The contents of an existing Slapi_Attr are not free'd first. There is no > public SLAPI function to clear out the contents of an existing Slapi_Attr for > reuse. There is a private attr_done() function which we could expose, but I > thought that it wasn't worth doing for this issue since we only execute this > code when the server is started or when the memberOf plug-in configuration is > modified. If you feel that it is worthwhile to make these changes, I can. Thank you for the details. Your analysis makes sense. Since it's not executed very often, the current code is efficient enough. > > 4. > > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2 > > I wonder what happens if slapi_str2filter returns NULL for some reason (I guess > > it will not happen, tho...) > > 346 theConfig.group_filter = slapi_str2filter(filter_str); > > I'm not sure how/if that could happen since we validate the attribute names > used to build the filter and build the filter ourselves in the plug-in. If it > can happen, we would end up passing a NULL filter to slapi_filter_test_simple() > when performing group checks in the memberOf plug-in, which would cause a > server crash. > > The return value of memberof_apply_config() is not checked. It is assumed that > memberof_validate_config() already caught any config problems that would cause > issues, but memberof_validate_config is not responsible for building a > Slapi_Filter. I can make memberof_apply_config log an error message if it is > unable to build the filter and add NULL checking in the 4 places we use the > filter to perform group checks. The plug-in will not work properly in this > case as it will be unable to detect if an entry is a group, but that is better > than crashing. One would see the error message and be able to fix their > memberOf configuration to get things working again. Does this approach sound > OK? Yes, I think so. Thank you!!
Created attachment 442000 [details] Revised patch This addresses the issues raised in Noriko's review (thanks!).
Thanks to Noriko for her reviews! Pushed to master. Counting objects: 23, done. Delta compression using 2 threads. Compressing objects: 100% (12/12), done. Writing objects: 100% (12/12), 5.78 KiB, done. Total 12 (delta 9), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git 8db7b8a..a2bcd81 master -> master
Verified 389-ds-base-1.2.8.2-1.el6.x86_64 Red Hat Enterprise Linux Server release 6.1 (Santiago) # ldapsearch -D "cn=Directory Manager" -w Secret123 -b "cn=MemberOf Plugin,cn=plugins,cn=config" # extended LDIF # # LDAPv3 # base <cn=MemberOf Plugin,cn=plugins,cn=config> with scope subtree # filter: (objectclass=*) # requesting: ALL # # MemberOf Plugin, plugins, config dn: cn=MemberOf Plugin,cn=plugins,cn=config objectClass: top objectClass: nsSlapdPlugin objectClass: extensibleObject cn: MemberOf Plugin nsslapd-pluginPath: libmemberof-plugin nsslapd-pluginInitfunc: memberof_postop_init nsslapd-pluginType: postoperation nsslapd-pluginEnabled: on nsslapd-plugin-depends-on-type: database memberofgroupattr: member memberofgroupattr: memberUser memberofgroupattr: memberHost memberofattr: memberOf nsslapd-pluginId: memberof nsslapd-pluginVersion: 1.2.8.2 nsslapd-pluginVendor: 389 Project nsslapd-pluginDescription: memberof plugin # search result search: 2 result: 0 Success # numResponses: 2 # numEntries: 1