#47525 Allow memberOf to use an alternate config area
Closed: wontfix None Opened 10 years ago by nkinder.

The memberOf plug-in currently uses it's main plug-in config entry in cn=config for the memberOf configuration. This doesn't allow the memberOf configuration to be replicated across all masters in a replicated environment.

We should add support for using an alternate config area that is in a normal backend. We already do this for other plug-ins by using nsslapd-pluginConfigArea in the main plug-in config entry.


need some error checking here in case user did not specify a valid DN
{{{
Slapi_DN *config_sdn = slapi_sdn_new_dn_byval(config_dn);
}}}
and here too?
{{{
config_sdn = slapi_sdn_new_dn_byval(sharedcfg))){
}}}
otherwise, ack

Replying to [comment:6 rmeggins]:

need some error checking here in case user did not specify a valid DN
{{{
Slapi_DN *config_sdn = slapi_sdn_new_dn_byval(config_dn);
}}}
and here too?
{{{
config_sdn = slapi_sdn_new_dn_byval(sharedcfg))){
}}}
otherwise, ack

Added dn validation, even though the config attribute is set as DN syntax, syntax checking could be turned off, or the value could be edited while the server is offline.

New patch is attached.

git merge ticket47525
Updating 5e25530..f81ac81
Fast-forward
ldap/servers/plugins/memberof/memberof.c | 191 ++++++++++++++++++++---
ldap/servers/plugins/memberof/memberof.h | 9 +
ldap/servers/plugins/memberof/memberof_config.c | 167 ++++++++++++++++++--
ldap/servers/slapd/slapi-plugin.h | 3 +
4 files changed, 337 insertions(+), 33 deletions(-)

git push origin master
Counting objects: 21, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 4.43 KiB, done.
Total 11 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
5e25530..f81ac81 master -> master

commit f81ac81a475bcc92f54bffd2c6d9cb45dbf9a324
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Nov 22 15:48:59 2013 -0500

git merge ticket47525
Updating 5753df1..51b1b83
Fast-forward
ldap/servers/plugins/memberof/memberof_config.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 699 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
5753df1..51b1b83 master -> master

commit 51b1b836421b5d1a57ffacc98e6004dfa7238162
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Nov 25 09:24:35 2013 -0500

git merge ticket47525
Updating 085c6d4..7e21a4b
Fast-forward
ldap/servers/plugins/memberof/memberof.c | 28 ++++++++++++++++++++++++----
1 files changed, 24 insertions(+), 4 deletions(-)

git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 1005 bytes, done.
Total 7 (delta 4), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
085c6d4..7e21a4b master -> master

commit 7e21a4bddbdf53c342dbb9afa63c6c9dbc5d2920
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Nov 26 11:32:13 2013 -0500

The previous fixes are causing crashes when the memberOf plug-in is disabled when running the following in-tree test:

dirsrvtests/tickets/ticket47560_test.py

The problem is that we are modifying the preop entry we fetch from the pblock, but it should be treated as read-only. A patch will be attached shortly.

I'm curious... If the preop entry 'e' should be treated as read-only, is it okay to "free" it with slapi_entry_free?

The problem is that we are modifying the preop entry we fetch from the pblock, but it should be treated as read-only.
730 733 bail:
731 734 slapi_mods_free(&smods);
735 slapi_entry_free(resulting_e);
732 736 slapi_entry_free(e);

Replying to [comment:17 nhosoi]:

I'm curious... If the preop entry 'e' should be treated as read-only, is it okay to "free" it with slapi_entry_free?

No, we shouldn't! Good catch. I focused on the modifications to the entry and missed the fact that we're freeing it. It's interesting that is passed the test by just avoiding modifying the entry. A new patch is on it's way.

Attachment 0001-Ticket-47525-Don-t-modify-preop-entry-in-memberOf-co.patch​ added
ack.

Pushed patch to not modify preop entry to master:

38bda615b3d3ace52f4965efcc21b97b2b6899a1

Crash encountered when a entry with an invalid configuration is used for the nsslapd-pluginconfigarea

I think this comment is incorrect:

{{{
366 / config area does not exist! /
367 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,
368 "memberof_apply_config: "
369 "%s does not contain a valid DN (%s)\n",
370 SLAPI_PLUGIN_SHARED_CONFIG_AREA, sharedcfg);
}}}

Aside from that, this looks good to me.

Do not add the \n to the returntext. Only add it when using slapi_log_error. Otherwise, ack

Thanks for the feedback.

I also noticed that we are validating the plugin config area entry in post op, and not in preop. So it reports error 53, but the operation still succeeds - I'm sure this is what QE was trying to test. So I'm going to rework the fix to do the validation in preop.

Need to make sure to free configarea_dn, config_sdn, and config_entry in case there is an incorrect or malicious attempt to have a modify with more than one SLAPI_PLUGIN_SHARED_CONFIG_AREA in it. That is, you should not do another loop iteration until you free configarea_dn, config_sdn, and config_entry.

Fix crash with invalid shared config area, and move shared config area validation into preop
0001-Ticket-47525-Crash-if-setting-invalid-plugin-config-.patch

Replying to [comment:27 rmeggins]:

Need to make sure to free configarea_dn, config_sdn, and config_entry in case there is an incorrect or malicious attempt to have a modify with more than one SLAPI_PLUGIN_SHARED_CONFIG_AREA in it. That is, you should not do another loop iteration until you free configarea_dn, config_sdn, and config_entry.

Yup, okay new patch attached.

f6929c9..42f935a master -> master
commit 42f935ab7406802d522f357048db1e68d729d5e5
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Dec 3 16:47:59 2014 -0500

b6cd13f..d06b397 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit d06b39743ef3b016ac25a242821a5dfc1ec67cb4

Metadata Update from @mreynolds:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.3.3 - 12/13 (December)

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

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