Hi William,

Is it ok to close this ticket since we set an explicit path to auditfail path?

I think that I might update this ticket to be "cannot delete values in cn=config", as I need to be able to do this for another situation.

tentative patch
0001-auditfail-deletable.patch

The attached patch is a tentative one when I was working on the explicit path fix.

We don't need this, but just in case, I'm adding it to avoid the duplicated task.

William, if you don't want to close this ticket, is it ok to push it to FUTURE?

Well, future or not, I'll be working on this soon. We need this fixed so we can "reset" values in dse.ldif to defaults, which I think will be more important in the future. So I will do this "soon".

attachment
0001-Ticket-48961-Add-ability-to-delete-auditfaillog-from.patch

attachment
0002-Ticket-48961-Clean-up-libglobs.c-defaults.patch

attachment
0003-Ticket-48961-Minimise-template-dse.ldif.patch

This change really tackles three issues. I want to break those down and explain.

The first patch tackles reset of values to default. Previously, we limited the values in DS that could be deleted in cn=config by default. By changing this, we now allow any value that ships with a hardcoded default to be deleted. This means that admins can effectively do a reset-to-factory settings by doing a mod_delete. Consider mod_delete on nsslapd-passwordstoragescheme. Because of my recent change, this would upgrade the site to SSHA512 automatically.

There is a powerful side effect of this change. By deleting values from cn=config, they are removed from dse.ldif. This means the Administrator and their site are using defaults from slap.h / libglobs. Consider we enable nunc-stans, or we change from SSHA512 to PBKDF2. When we upgrade slap.h, the Admin running yum upgrade will automatically receive the change! By the same token, when they yum downgrade, it will go back to the old value too! This reduces our reliance on messy upgrade.pl scripts that need to be upgraded, and we can manage defaults correctly in slap.h.

The second patch tackles inconsistency in our defaults. Our values have to be a combination of ints/strings for the different parts of libglobs. Sadly, these were spread out, sometimes wrong, and other times missing. This change moves all our defaults to slap.h, and groups them by int/str pairs. When we update the defaults, we can be sure we are updating them correctly now, as they are all in one place.

Because sometimes the values in the string form (which goes to the cfg hash) and the int form (which goes to the struct) were not the same, it meant that you could have different values from dse.ldif, the header, and the libglobs. Consider something like maxthreads. You would have one value in dse.ldif, say 40. Then you remove that, and restart ds. Now you get the value 30, which is from the cfg struct. While running live, you reset this to default by mod_delete of maxthreads. Then you get 20 from slap.h. The quirk is that when you restart, you would re-read the value from the cfg struct, so it would reset back to 30 now!

The third patch removes as much as possible from dse.ldif that is not site specific. Infact, dse.ldif had a number of defaults that contradicted slap.h/libglobs. slap.h and libglobs were updated to reflect the values in dse.ldif, as these will be "what admins currently get".

The benefit to this, is now that every value in cn=config will be upgrade automatically on upgrade, and can also be removed on upgrade without needing to tamper with dse.ldif. We remove another location of inconsistency. If we were to try and upgrade a default value in dse.ldif, we would not have seen the change as a previously deployed instance would override it from the dse.ldif that they installed from.

Another benefit is migration. Previously, an admin could not distinguish a value we shipped in cn=config, vs a value that they set in cn=config. Now with this change, any value is dse.ldif cn=config must have been set by the admins at that site, and is site specific. This allows tracking customisations to be easier, and in the future will reduce the support workload as we can target broken settings faster.

In summary, this allows the idea of upgradable config items to really be realised, so we as a team can start to ship better defaults, and we can change them over time as the product evolves. It reduces administrator load, as they don't need to work out every single setting, and can run a minimal dse.ldif. It simplifies the product, without removing the power and granular configuration that we support.

Usage: sh cnconfig.sh <directory_manager_password>
cnconfig.sh

Please run the attached script against your server.
Check the error log, then try restarting the server.
Thanks.

That was a good test. I've adapted your script into my python test, and it's exposed some errors. Mainly around a few values with wrong defaults, some mishandled, some that can be deleted that shouldn't and some that behave in unique ways.

Will update the patches with these fixes.

Fix issues found by Noriko's script.
0003-Ticket-48961-Minimise-template-dse.ldif.2.patch

Replaced only patch 3 with fixes for the delete every attribute case. Would you prefer me to squash the set of three into 1 patch for review?

lib389: Fix issues related to 48961 tests.
0001-Ticket-48961-Fix-lib389-minor-issues-shown-by-48961-.patch

Hi William,
please, replace DEBUGGING to False in the test case.

The rest looks good, both the test case and lib389 fixes. I haven't reviewed 389-ds changes though.

Squashed the patches together, updated libglobs.c with detailed usage.

attachment
0001-Ticket-48961-Allow-reset-of-configuration-values-to-.patch

lib389 fixs, per spichugi's ack

commit 619dabd931ccc9af7a24553812fd98690b5330be
Writing objects: 100% (6/6), 813 bytes | 0 bytes/s, done.
Total 6 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/lib389.git
d578672..619dabd master -> master

1) Broken indentation...
{{{
… … modify_config_dse(Slapi_PBlock pb, Slapi_Entry entryBefore, Slapi_Entry e, in
368 if (SLAPI_IS_MOD_ADD(mods[i]->mod_op)) {
376 if (reject_attr_type(config_attr)) {
377 slapi_log_err(SLAPI_LOG_WARNING, "modify_config_dse",
378 "Modification of attribute \"%s\" is not allowed, REJECTING!\n",
379 config_attr);
380 rc = LDAP_UNWILLING_TO_PERFORM;
381 } else if (ignore_attr_type(config_attr)) {
382 slapi_log_err(SLAPI_LOG_WARNING, "modify_config_dse",
383 "Modification of attribute \"%s\" is not allowed, ignoring!\n",
384 config_attr);
385 } else if (SLAPI_IS_MOD_ADD(mods[i]->mod_op)) {
369 386 if (apply_mods) { / log warning once */
}}}

2) Why reject_attr_type needs to be introduced? Why updating "cn" needs to return LDAP_UNWILLING_TO_PERFORM while the others in the ignore list do not have to? And ignore_attr_type list still contains "cn"...
{{{
77 / these attr types are ignored for the purposes of configuration search/modify /
79 ignore_attr_type(const char *attr_type)

96 / These trigger rejections for config modify! /
98 reject_attr_type(const char *attr_type)
}}}

3) Thank you for adding the detailed instructions to libglobs.c, which is excellent. Now, I have one question... If you upgrade a pre-1.3.6 server to 1.3.6, the existing dse.ldif won't be touched, will it? I found one upgrade script 50updateconfig.ldif, in which nsslapd-ndn-cache-enabled gets enabled (that is done against the dse.ldif with the server down, I think). I believe such an operation in upgrading the server does not do any harm to your enhancement, but I want to double check...

4) This means you can delete listenhost and the default value is an empty string? I guess you wanted to remove the comment?
{{{
734 731 {CONFIG_LISTENHOST_ATTRIBUTE, config_set_listenhost,
735 732 NULL, 0,
736 733 (void)&global_slapdFrontendConfig.listenhost,
737 CONFIG_STRING, NULL, NULL/ NULL value is allowed /},
734 CONFIG_STRING, NULL, "" / NULL value is allowed /},
}}}
Ditto.
{{{
797 794 {CONFIG_SECURELISTENHOST_ATTRIBUTE, config_set_securelistenhost,
798 795 NULL, 0,
799 796 (void)&global_slapdFrontendConfig.securelistenhost,
800 CONFIG_STRING, NULL, NULL/ NULL value is allowed /},
797 CONFIG_STRING, NULL, "" / NULL value is allowed /},
}}}

5) This config param is no longer needed?
{{{
982 979 {CONFIG_ALLOWED_TO_DELETE_ATTRIBUTE, config_set_allowed_to_delete_attrs,
983 980 NULL, 0,
984 981 (void**)&global_slapdFrontendConfig.allowed_to_delete_attrs,
985 982 CONFIG_STRING, (ConfigGetFunc)config_get_allowed_to_delete_attrs,
986 DEFAULT_ALLOWED_TO_DELETE_ATTRS },
983 SLAPD_DEFAULT_ALLOWED_TO_DELETE_ATTRS },
}}}

6) Found the same definitions in 2 header files. Is it intentional?
{{{
ldap/servers/slapd/slap.h
2172 2317 #ifndef DAEMON_LISTEN_SIZE
2173 2318 #define DAEMON_LISTEN_SIZE 128
2319 #define DAEMON_LISTEN_SIZE_STR "128"
2174 2320 #endif
include/base/systems.h
157 157 #ifndef DAEMON_LISTEN_SIZE
158 158 #define DAEMON_LISTEN_SIZE 128
159 #define DAEMON_LISTEN_SIZE_STR "128"
159 160 #endif / !DAEMON_LISTEN_SIZE /
}}}

7) Broken indentation...
{{{
7641 7642 }
7642 values[ii]->bv_len = strlen((char *)values[ii]->bv_val);
7643 }
7643 } else {
7644 retval = LDAP_UNWILLING_TO_PERFORM;
7645 }
7644 7646 break;
7645 7647 }

1002            if ((max_logsize > mdiskspace) && (mdiskspace != -1)) {

1002 1003 rv = 2;
1004 }
}}}

8) Compiler warning?
{{{
815 void g_log_init(int log_enabled);
814 void g_log_init();

245 void g_log_init(int log_enabled)
245 void g_log_init()
}}}
It should be:
{{{
void g_log_init(void) ???
}}}

9) Why are you changing this setting? I think it was chosen for the performance reason. RWLock is much slower.
{{{
2185 #define SLAPI_CFG_USE_RWLOCK 0
2331 #define SLAPI_CFG_USE_RWLOCK 1
}}}

attachment
0001-Ticket-48961-Allow-reset-of-configuration-values-to-.2.patch

Fixed the issues raised by Noriko, added some comments. The only exception was 6, where I didn't completely investigate this, so I decided to leave it alone.

Built and tested the new patch.

3) Thank you for adding the detailed instructions to libglobs.c, which is excellent. Now, I have one
question... If you upgrade a pre-1.3.6 server to 1.3.6, the existing dse.ldif won't be touched, will
it? I found one upgrade script 50updateconfig.ldif, in which nsslapd-ndn-cache-enabled gets enabled
(that is done against the dse.ldif with the server down, I think). I believe such an operation in
upgrading the server does not do any harm to your enhancement, but I want to double check...

The existing dse.ldif will not be altered or affected.

In fact, as a hint, had this change been made in the past, not upgrade script would be needed. Just set the value to "on" in libglobs.c, and it will take effect on all instances.

Replying to [comment:16 firstyear]:

3) Thank you for adding the detailed instructions to libglobs.c, which is excellent. Now, I have one
question... If you upgrade a pre-1.3.6 server to 1.3.6, the existing dse.ldif won't be touched, will
it? I found one upgrade script 50updateconfig.ldif, in which nsslapd-ndn-cache-enabled gets enabled
(that is done against the dse.ldif with the server down, I think). I believe such an operation in
upgrading the server does not do any harm to your enhancement, but I want to double check...

The existing dse.ldif will not be altered or affected.
Well, if some customer explicitely set it to "off", you have to delete it, right? ;)
In fact, as a hint, had this change been made in the past, not upgrade script would be needed. Just set the value to "on" in libglobs.c, and it will take effect on all instances.

If a customer explicitly set it to off, I would hope that they know what they are doing. But yes, you would have to explicitly delete this. Customer intent in dse.ldif always "takes precedence".

Close, but did you check the compiler warnings? :)

I think this line was complained???
{{{
ldap/servers/slapd/proto-slap.h
542 542 char *config_get_allowed_to_delete_attrs(void);
}}}

I didn't get that warning: I have disabled some of the warnings because NSS triggers false alerts :(

I'll fix that once up as well.

Fix compiler warnings.
0001-Ticket-48961-Allow-reset-of-configuration-values-to-.3.patch

Fix compiler warnings.
Thanks!

commit 54431ba
Writing objects: 100% (21/21), 12.89 KiB | 0 bytes/s, done.
Total 21 (delta 18), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
e31b324..54431ba master -> master

Thank you so much for all the reviews, I'm really happy to have this ack!

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

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.