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 to use an alternate config area #862

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

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.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2013-11-23 02:00:04

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

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-11-23 02:49:30

revision - check DN syntax 0001-Ticket-47525-Allow-memberOf-to-use-an-alternate-conf.patch

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-11-23 02:51:52

Replying to [comment:6 richm]:

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.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-11-23 03:27:44

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 mreynolds389@redhat.com Date: Fri Nov 22 15:48:59 2013 -0500

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-11-25 20:26:12

Fix memory leak 0001-Ticket-47525-Fix-memory-leak.patch

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-11-25 20:45:13

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 mreynolds389@redhat.com Date: Mon Nov 25 09:24:35 2013 -0500

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-11-26 22:34:10

Added rwlock for config area 0001-Ticket-47525-Need-to-add-locking-around-config-area-.patch

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2013-12-03 01:55:57

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 mreynolds389@redhat.com Date: Tue Nov 26 11:32:13 2013 -0500

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-12-13 06:05:31

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.

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2013-12-13 06:19:34

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);

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-12-13 06:25:57

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.

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-12-13 06:28:09

attachment 0001-Ticket-47525-Don-t-modify-preop-entry-in-memberOf-co.patch

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2013-12-13 06:30:10

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

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-12-13 08:48:11

Pushed patch to not modify preop entry to master:

38bda615b3d3ace52f4965efcc21b97b2b6899a1

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2013-12-18 04:05:31

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

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2014-11-26 04:24:41

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

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2014-11-26 04:45:44

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.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2014-11-26 05:05:45

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

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2014-11-26 05:39:29

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.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2014-12-02 03:49:43

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.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2014-12-04 03:49:42

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

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2014-12-04 03:50:44

Replying to [comment:27 richm]:

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.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2014-12-05 03:46:35

f6929c9..42f935a master -> master commit 42f935ab7406802d522f357048db1e68d729d5e5 Author: Mark Reynolds mreynolds389@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

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2017-02-11 23:12:37

Metadata Update from @mreynolds389: