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>
duplicate values for nsslapd-pluginarg fix 0001-Ticket-47431-Duplicate-values-for-the-attribute-nssl.patch
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:
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:
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:
nsslapd-pluginarg0: uid nsslapd-pluginarg2: cn nsslapd-pluginarg3: , nsslapd-pluginarg4: dc=test,dc=com
Output: The config entry is modified to
nsslapd-pluginarg0: uid nsslapd-pluginarg1: cn nsslapd-pluginarg2: , nsslapd-pluginarg3: dc=test,dc=com
TestCase5:
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
done
duplicate values for nsslapd-pluginarg modified fix 0001-Ticket-47431-Duplicate-values-for-the-attribute-nssl.2.patch
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.
git patch file (master) 0001-Ticket-47431-Duplicate-values-for-the-attribute-nssl.3.patch
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]:
Thank you, Rich... In addtion, there were some more errors in the comments... :p
Replacing it with the corrected version.
git patch file (master) -- CI test 0002-Ticket-47431-CI-test-added-test-cases-for-ticket-474.patch
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
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.