#47431 Duplicate values for the attribute nsslapd-pluginarg are not handled correctly
Closed: wontfix None Opened 10 years ago by anjain.

Duplicate values and multiple values for the attribute nsslapd-pluginarg are not handled correctly.

Steps to reproduce:

1) Enter duplicate values for nsslapd-pluginarg2

dn: cn=7-bit check,cn=plugins,cn=config
...
nsslapd-pluginarg0: uid
nsslapd-pluginarg1: mail
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg3: ,
nsslapd-pluginarg4: dc=test,dc=com

Out of 27 duplicate values, only 14 duplicate values are detected and removed

2) Enter multiple values for nsslapd-pluginarg2

dn: cn=7-bit check,cn=plugins,cn=config
...
nsslapd-pluginarg0: uid
nsslapd-pluginarg1: mail
nsslapd-pluginarg2: userpassword
nsslapd-pluginarg2: cn
nsslapd-pluginarg2: sn
nsslapd-pluginarg3: ,
nsslapd-pluginarg4: dc=test,dc=com

In this case, only the first value of nsslapd-pluginarg2 is picked up and rest of the values remain in the config entry. The rest of them should be removed from the config entry with warning/error.


Please investigate this area, as well.
{{{
2085 plugin_setup(Slapi_Entry plugin_entry, struct slapi_componentid group,
2086 slapi_plugin_init_fnptr p_initfunc, int add_entry)
...
2280 / add the plugin arguments /
2281 value = 0;
2282 ii = 0;
2283 PR_snprintf(attrname, sizeof(attrname), "%s%d", ATTR_PLUGIN_ARG, ii);
2284 while ((value = slapi_entry_attr_get_charptr(plugin_entry, attrname)) != NULL)
2285 {
2286 charray_add(&plugin->plg_argv, value);
2287 plugin->plg_argc++;
2288 ++ii;
2289 PR_snprintf(attrname, sizeof(attrname), "%s%d", ATTR_PLUGIN_ARG, ii);
2290 }
}}}
I'd like you to fix this case if possible.
{{{
nsslapd-pluginarg0: ...
nsslapd-pluginarg1: ...
nsslapd-pluginarg3: ...
}}}
to
{{{
nsslapd-pluginarg0: ...
nsslapd-pluginarg1: ...
nsslapd-pluginarg2: ...
}}}
Currently, this is cut at arg1 and arg3 is dropped.
{{{
nsslapd-pluginarg0: ...
nsslapd-pluginarg1: ...
nsslapd-pluginarg3: ...
}}}

Sorry Anupam, this question has come up now... :)

What happens if your plugin has this line? The type starts with "nsslapd-pluginarg", but not end with digits?
nsslapd-pluginargA: <value>

Tests Executed:

TestCase1:

dn: cn=7-bit check,cn=plugins,cn=config

...

nsslapd-pluginarg0: uid
nsslapd-pluginarg0: uid
nsslapd-pluginarg1: ,
nsslapd-pluginarg2: dc=test,dc=com

Output: Duplicate value is removed from the entry. Following message is logged in the server error log:

[16/Jul/2013:17:31:11 -0700] - str2entry_dupcheck: Duplicate value for attribute type nsslapd-pluginArg0 detected in entry cn=7-bit check,cn=plugins,cn=config. Extra value ignored.

TestCase2:

dn: cn=7-bit check,cn=plugins,cn=config

...

nsslapd-pluginarg0: uid
nsslapd-pluginarg0: cn
nsslapd-pluginarg1: ,
nsslapd-pluginarg2: dc=test,dc=com

Output: The server does not start. Following message is logged in the server error log:

[16/Jul/2013:17:35:59 -0700] - Entry "cn=7-bit check,cn=plugins,cn=config" single-valued attribute "nsslapd-pluginArg0" has multiple values
[16/Jul/2013:17:35:59 -0700] dse - Could not load config file [dse.ldif]
[16/Jul/2013:17:35:59 -0700] dse - Please edit the file to correct the reported problems and then restart the server.

TestCase3:

dn: cn=7-bit check,cn=plugins,cn=config

...

nsslapd-pluginarg0: uid
nsslapd-pluginargabcd: cn
nsslapd-pluginarg1: ,
nsslapd-pluginarg2: dc=test,dc=com

Output: The server is started but the following message is logged in the server error log:

[16/Jul/2013:17:39:24 -0700] cn=7-bit check,cn=plugins,cn=config - Invalid Plugin argument: nsslapd-pluginargabcd. Argument ignored

TestCase4:

dn: cn=7-bit check,cn=plugins,cn=config

...

nsslapd-pluginarg0: uid
nsslapd-pluginarg2: cn
nsslapd-pluginarg3: ,
nsslapd-pluginarg4: dc=test,dc=com

Output: The config entry is modified to

dn: cn=7-bit check,cn=plugins,cn=config

...

nsslapd-pluginarg0: uid
nsslapd-pluginarg1: cn
nsslapd-pluginarg2: ,
nsslapd-pluginarg3: dc=test,dc=com

TestCase5:

dn: cn=7-bit check,cn=plugins,cn=config

...

nsslapd-pluginarg0: uid
nsslapd-pluginarg123: cn
nsslapd-pluginarg2: ,
nsslapd-pluginarg3: dc=test,dc=com

Output: The server is not started. Following message is logged in the server error log:

[16/Jul/2013:17:46:52 -0700] cn=7-bit check,cn=plugins,cn=config - Plugin argument value nsslapd-pluginArg123 exceeded maximum allowed value nsslapd-pluginArg15
[16/Jul/2013:17:46:52 -0700] - Unable to load plugin "cn=7-bit check,cn=plugins,cn=config"

Very nice! Probably, one last small thing...
{{{
This could be an integer instead of a string?
60 #define MAX_PLUGINARG_NUM "15"
=> #define MAX_PLUGINARG_NUM 15

Then, you don't have to pass it to atoi?
2327 if ((numdigits < 3) && (atoi(ptr_num) <= atoi(MAX_PLUGINARG_NUM)))
===> if ((numdigits < 3) && (atoi(ptr_num) <= MAX_PLUGINARG_NUM))
2328 num_args++;
2329 else
2330 {
2331 slapi_log_error( SLAPI_LOG_FATAL, plugin->plg_dn, "Plugin argument value nsslapd-pluginArg%s exceeded maximum allowed value nsslapd-pluginArg%s\n", ptr_num, MAX_PLUGINARG_NUM);
===> slapi_log_error( SLAPI_LOG_FATAL, plugin->plg_dn, "Plugin argument value nsslapd-pluginArg%s exceeded maximum allowed value nsslapd-pluginArg%d\n", ptr_num, MAX_PLUGINARG_NUM);
2332 status = -1;
2333 goto PLUGIN_CLEANUP;
2334 }
}}}

Replying to [comment:6 nhosoi]:

Very nice! Probably, one last small thing...
{{{
This could be an integer instead of a string?
60 #define MAX_PLUGINARG_NUM "15"
=> #define MAX_PLUGINARG_NUM 15

Then, you don't have to pass it to atoi?
2327 if ((numdigits < 3) && (atoi(ptr_num) <= atoi(MAX_PLUGINARG_NUM)))
===> if ((numdigits < 3) && (atoi(ptr_num) <= MAX_PLUGINARG_NUM))
2328 num_args++;
2329 else
2330 {
2331 slapi_log_error( SLAPI_LOG_FATAL, plugin->plg_dn, "Plugin argument value nsslapd-pluginArg%s exceeded maximum allowed value nsslapd-pluginArg%s\n", ptr_num, MAX_PLUGINARG_NUM);
===> slapi_log_error( SLAPI_LOG_FATAL, plugin->plg_dn, "Plugin argument value nsslapd-pluginArg%s exceeded maximum allowed value nsslapd-pluginArg%d\n", ptr_num, MAX_PLUGINARG_NUM);
2332 status = -1;
2333 goto PLUGIN_CLEANUP;
2334 }
}}}

done

On behalf of Anupam, pushed to master: commit d2c5b35

Sorry, but this fix breaks FreeIPA 3.1+.

Is there any particular reason why a limit of plugin arguments was introduced? Previously there were no limits at all and 15 looks like absolutely out of blue. For example, referential integrity plugin in FreeIPA uses 17 arguments and may use more if more attributes need to be tracked for an entry.

In particular, nothing in the code that actually sets the attributes relies on the limit. Why cannot the code simply calculate the number of attributes and process them all rather than limiting them?

To enforce that each nsslapd-pluginargX attribute is single-valued, we require the attribute to be defined in the schema. This is why a limitation on the number of pluginarg attributes was introduced. This style of plug-in config is really old and not very flexible. The ideal way to configure plug-ins is to have real named config attributes.

If we want to keep duplicate value checking, we should increase the limit for old style plug-in args, deprecate this old stype of plug-in config, and modify these older plugins like 7-bit and attribute uniqueness to have more sane named configuration attributes. The difficultly here is choosing an appropriate limit, as we need to add all of these numbered plug-in attributes to the schema, which requires using up OIDs and creates a larger schema.

One alternative is to simply back out the duplicate checking changes since they haven't been in a release yet, then implement support for named configuration attributes instead. The old style config would still be prone to issues with duplicates, but it is a corner-case where these plug-ins are mis-configured.

I have reverted the commit for this issue in master:

Counting objects: 15, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (8/8), done.
Writing objects: 100% (8/8), 963 bytes, done.
Total 8 (delta 6), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
9093f58..33da858 master -> master

New nightly builds should remove the limitation on the number of plug-in config arguments. This ticket will be re-triaged so a different approach can be taken that will not limit the number of plug-in config arguments.

In the CI test - most of the comments have
[..] NS7bitAttr_Init - 3: member
but you don't appear to be using the member attribute in the tests

Replying to [comment:22 rmeggins]:

In the CI test - most of the comments have
[..] NS7bitAttr_Init - 3: member
but you don't appear to be using the member attribute in the tests

Thank you, Rich... In addtion, there were some more errors in the comments... :p

Replacing it with the corrected version.

Replaced: Attachment 0002-Ticket-47431-CI-test-added-test-cases-for-ticket-474.patch​ added

git patch file (master) -- CI test

Reviewed by Rich (Thank you!!)

Pushed to master:
308c1bb..2802f36 master -> master
commit 6aa9fdc
commit 9576982

Pushed to 389-ds-base-1.3.3:
748054f..06a5cc4 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit b0c5a38
commit f298e2b

Metadata Update from @nhosoi:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.3.3.9

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

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