The comment should use new naming like warn-process-safely

Except the minor comment the patch looks good to me. ACK

I also think that the patch is ok, but I am not fully happy with the naming of the options.

The warn-process-safely does change behavior by enforcing rfc compliance, that it logs some info in the access log is an additional feature, but it is not a "warning" option. I would prefer something like:

off (was off)
reject (was strict)
process-warn-only (new)
process-strict or process-rfc-safely or ... (was warn)

good point @lkrispen I also had mixed feeling with the naming but rather because I found them too long. An other suggestion could be

off (was off)
warn (new)
strict (was warn)
reject (was strict)

good point @lkrispen I also had mixed feeling with the naming but rather because I found them too long. An other suggestion could be
off (was off)
warn (new)
strict (was warn)
reject (was strict)

This could have been a valid setting, but since it would change the meaning of warn and strict it would no longer be possible to do a "legacy" processing, so the new names need to be different (except off)

good point @lkrispen I also had mixed feeling with the naming but rather because I found them too long. An other suggestion could be
off (was off)
warn (new)
strict (was warn)
reject (was strict)

This could have been a valid setting, but since it would change the meaning of warn and strict it would no longer be possible to do a "legacy" processing, so the new names need to be different (except off)

This hasn't really been released or documented yet. So we should be able to use whatever values we want even if it is conflicting from the first version of this fix. But I do like Thierry's names as I'm not a huge fan of the original settings use of the word "safe" in them. It's a bit misleading/alarming IMHO

There is a trend in a lot of apis and configurations now to have "hints" about configuration risks in the name. Having settings that open you to denial of service risks certainly speaks of "unsafe" to me.

As @lkrispen rightly points out, we can't reuse the warn/off/strict names to have new meanings, they have to stay with the old meanings. That's why I made all the new names more descriptive.

Having the value be "long" isn't necessarily a bad thing. We hint what is valid in the error if it's not set correctly, so it's easy to discover the valid settings. Having the settings be descriptive is good because it can hint to admins what's a good setting or what's risky, and they are then able to look at documentation about why. A lot of settings don't really indicate what's a good or bad state to be in, so having more descriptive values like this IMO does help.

So I'd tend to want to keep the names I put here, but I'll change them if it's really a problem to people. I'm not going to resist "too hard", I just wanted to state my case.

If I was to change them, I prefer @lkrispen's suggestion, as it's the "closest" to what I want in terms of communicating risks and positive/negative options:

off (was off)
reject (was strict)
process-warn-only (new)
process-strict or process-rfc-safely or ... (was warn)

I have a mixed feeling regarding DOS. There are several options for an attacker to trigger unindexed search even with valid attributetypes (e.g. "(objectclass=*)" or "(FTPQuotaMBytes=123)" where FTPQuotaMBytes is likely not indexed). In additional, even if the attribute is unknown in the schema, you can index it and use it to index a search. So if I understand correctly this RFE it is more oriented to RFC compliance than security.

Regarding the names, I agree with the legacy concern. I am not fan with 'process' header but I understand the will to give a "hint" with good option name. Could it be something like 'warn-only' and 'strict-rfc'. @mmuehlfeldrh any suggestion ?

This hasn't really been released or documented yet. So we should be able to use
whatever values we want even if it is conflicting from the first version of this fix.

ACK. If this was never seen by a user, I would not care about name conflicts with the old names.

But I do like Thierry's names as I'm not a huge fan of the original settings use
of the word "safe" in them. It's a bit misleading/alarming IMHO

I agree that "safe" can be misleading.

There is a trend in a lot of apis and configurations now to have "hints"
about configuration risks in the name.

I agree that this would be nice, but I'm unsure if the suggested names (reject-invalid, warn-process-safely, warn-allow-unsafe, off-allow-unsafe) are descriptive enough. According to the patch, the name of the attribute is "nsslapd-verify-filter-schema". Based on that:

• I can imagine what "reject-invalid" does.

• "warn-process-safely" sounds like only the valid filter schema part is processed and a filter that can cause trouble is ignored and a warning logged.

• I'm really unsure what the difference is between "warn-allow-unsafe" and "off-allow-unsafe". Both seem to allow something that is unsafe, but what is the difference between "warn" and "off" in the two value's names? Does one allow unsafe filters but doesn't log a warning (off)?

1) As someone who is not familiar with the topic, it took me a few minutes to think about these value names and their possible meaning. This means, they were not very self-explaining and it would be faster to look up the possible values and their meanings in the Config Reference Guide rather than guessing.

2) Additionally: Do user see all these options somewhere (e. g. in the logs) so that they can easily find out, for example, what causes the warning and what other values they can set? If I only see the current value (e. g. "warn-process-safely"), then it gives me a glue why this warning is logged, but to find out what other values I can set to fix the problem, I still need the docs. It would be more helpful for me if the warning is descriptive and contains the attribute's name, so that I can look up the details in the docs.

Because of 1) and 2), I think that the long names are nice, but not really necessary. I would prefer the short option names and a good description in the docs. But that's just my opinion. :-)

I have a mixed feeling regarding DOS. There are several options for an attacker to trigger unindexed search even with valid attributetypes (e.g. "(objectclass=*)" or "(FTPQuotaMBytes=123)" where FTPQuotaMBytes is likely not indexed). In additional, even if the attribute is unknown in the schema, you can index it and use it to index a search. So if I understand correctly this RFE it is more oriented to RFC compliance than security.

It was about security and stability because FreeIPA was creating queries that caused huge query times, and I have seen this create a DOS situation when I was previously an Admin. So this is a real problem.

I agree that "safe" can be misleading.

It's not misleading in this case. I don't know why everyone seems to gloss over the fact I've seen this cause production outages when I was an admin ... at the time I didn't know how to solve it or express the problem verbally, but now everyone seems to be ignoring this experience and saying "ohh it's not so bad".

Because of 1) and 2), I think that the long names are nice, but not really necessary. I would prefer the short option names and a good description in the docs. But that's just my opinion. :-)

Yeah that's fair.

So I'm going to say this suggestion is what we go with, taking into account Marc's comment about how it appears next to the config name.

nsslapd-verify-filter-schema: off
nsslapd-verify-filter-schema: reject-invalid
nsslapd-verify-filter-schema: warn-invalid
nsslapd-verify-filter-schema: process-safe

@firstyear, I have no admin experience. Thanks for you explanations I have no doubt that it exists a risk (application using unknown attribute) that production can be severely impacted without immediate explanation/workaround.

So the proposed values set (off, reject-invalid...) looks good to me.

The default value (process-safe) looks the good one considering risk/impact.

FreeIPA used unknown attribute in their requests. I hope it is no longer the case else it will break. By any chance do you remember example of those bad filter ?

On 10 Dec 2019, at 20:44, thierry bordaz pagure@pagure.io wrote:
=20
=20
tbordaz commented on the pull-request: Ticket 50727 - correct = mistaken options in filter validation patch that you are following:
``
@firstyear, I have no admin experience. Thanks for you explanations I =
have no doubt that it exists a risk (application using unknown =
attribute) that production can be severely impacted without immediate =
explanation/workaround.
=20
So the proposed values set (off, reject-invalid...) looks good to me.
=20
The default value (process-safe) looks the good one considering =
risk/impact.
=20
FreeIPA used unknown attribute in their requests. I hope it is no =
longer the case else it will break. By any chance do you remember =
example of those bad filter ?

They were requesting something like:

(|
(objectClass=3Dreferral)
(&
(|
(usercertificate;binary=3D...)
=
(ipaCertMapData=3Dx509:o=3Ddev.blackhats.net.au,cn=3Dcertificate =
authority<s>o=3Ddev.blackhats.net.au,cn=3Dipauser1)
=
(altsecurityidentities=3DX509:O=3DDEV.BLACKHATS.NET.AU,CN=3DCertificate=
Authority<S>O=3DDEV.BLACKHATS.NET.AU,CN=3Dipauser1)
)
(objectClass=3Dposixaccount)
(uid=3D)
(uidNumber=3D)
(!
(uidNumber=3D0)
)
)
)

@tbordaz Updated based on your feedback :)

Thank for the info. ipaCertMapData and altSecurityIdentities are now part of FreeIPA 73certmap.ldif schema definition, so the use of those definitions will not hit the change of behavior.

You have my ACK. Please wait for @lkrispen and @mreynolds feedback.

Sorry I have not been following this closely as I am out of the office this week. The main question is are we changing the default behavior with this patch? If there is a risk of that I would like this PR to be run through the IPA tests to make sure we are not introducing any regressions.

We aren't changing the behaviour by default. It's the same as we have shipped for the last few months.

PS: if there are IPA regressions with this feature set to process-safe that indicates a bug in IPA, not DS.

Also thanks for the ack @tbordaz I'll give them a few days to comment then I'll probably look to merge early next week :)

@mreynolds, @abiagion kindly accepted to run NR IPA tests on a test build. @firstyear please hold on the PR until those tests complete.

I've built a custom box for testing with freeipa-pr-ci, here it is the test run:
https://github.com/freeipa-pr-ci2/freeipa/pull/95

It's executing the same set of tests we run weekly with copr repo @389ds/389-ds-base-nightly enabled.

Possible failures in test_caless_* are known and already reported.

@abiagion thank you very much for the run. It helps a lot.
@mreynolds the PR does not trigger any IPA regression (NR run) and original problematic definitions (ipaCertMapData and altSecurityIdentities) are now part of IPA. IMHO the change of behavior (default=process-safe) may create concerns for some customers but it is expected so to protect the production and at the same time do our best to answer requests.

The default value (process-safe) looks the good one considering risk/impact.

IMHO, there should be a grace period when we introduce a new breaking change. Debugging missing search results can be frustrating.
warn-invalid should be a default for now. This change should be communicated in the release notes and support should be aware of the upcoming change (KCS article). I would also add a warning in the errors log (sans dragons) on a startup about this just to be on a safe side.
And in the next release (1.4.3?) we can switch it to process-safe, again highlighting it in the release notes. WDYT?

WDIT ?. I think that you are a wise advocate of users :)

At the moment, there is a warning logged in access logs (notes=F + explanation). Adding one warning in error logs (and KCS) is a good idea as we expect to change default behavior in upcoming version. Do you think we should log only at startup ? Making it noisy at each will help detection.

Regarding IPA, we may open a ticket so that it is configured with process-safe sooner

WDIT ?. I think that you are a wise advocate of users :)
At the moment, there is a warning logged in access logs (notes=F + explanation). Adding one warning in error logs (and KCS) is a good idea as we expect to change default behavior in upcoming version. Do you think we should log only at startup ? Making it noisy at each will help detection.
Regarding IPA, we may open a ticket so that it is configured with process-safe sooner

I think this is best way forward. For 1.4.2 (upstream) & 1.4.3 we use warn-invalid, then in 1.4.4 we change it to: process-safe.

For elaborate on this time line, 1.4.2 is what is going into RHEL 8.2, but we can no longer add enhancements to 8.2. So as this is an enhancement it's going to land downstream in 1.4.3 (RHEL 8.3), then we can release note the initial change/enhancement, and in 8.4 (1.4.4) we set the default to "process-safe".

nsslapd-verify-filter-schema feature is already in RHEL 8.2 (was part of the rebase to 1.4.2) and changes the default behavior. So I'd like to see this fix land also in 8.2 (we still have time for bug fixes).
And then we can switch to process-safe in 1.4.3 (RHEL 8.3).

nsslapd-verify-filter-schema feature is already in RHEL 8.2 (was part of the rebase to 1.4.2) and changes the default behavior. So I'd like to see this fix land also in 8.2 (we still have time for bug fixes).
And then we can switch to process-safe in 1.4.3 (RHEL 8.3).

Sorry I didn't know it was already in 8.2 Cool, so yeah we change the default in 8.2 and release note it, and then change it again in 8.3 (1.4.3).

The default value (process-safe) looks the good one considering risk/impact.

IMHO, there should be a grace period when we introduce a new breaking change. Debugging missing search results can be frustrating.
warn-invalid should be a default for now. This change should be communicated in the release notes and support should be aware of the upcoming change (KCS article). I would also add a warning in the errors log (sans dragons) on a startup about this just to be on a safe side.
And in the next release (1.4.3?) we can switch it to process-safe, again highlighting it in the release notes. WDYT?

There are two issues with this:
Everyone becomes afraid/hesitant to make the further step to make it enforce or change the behaviour as we'll forever cite "breaking" applications as the reason to stall meaningful improvements
Users don't "prepare" their applications because they aren't looking through access log for notes= fields (this goes for IPA too), so they will not notice the upcoming change.
People read release notes after* something goes wrong. Not before.

So I actually disagree, I think that when we make changes like this, we ship with the default that affects behaviour, people notice the behavioural change and then can make an informed decision to disable the feature OR to fix their applications. People should be testing in staging or dev first, and most ops people are well disciplined to do this and find issues before production.

Anyway, I'm going to merge this to 1.4.3 and 1.4.2, and then we can follow up to change the default if there is a strong case for it (merging will keep the exsiting behaviour of process-safe).

commit 60c1831 (HEAD -> 389-ds-base-1.4.2, origin/389-ds-base-1.4.2)

To ssh://pagure.io/389-ds-base.git
35ae7f9..60c1831 389-ds-base-1.4.2 -> 389-ds-base-1.4.2

https://github.com/marcus2376/389wiki/pull/21 <<-- updates the design doc with the finalised names.

Anyway, I'm going to merge this to 1.4.3 and 1.4.2, and then we can follow up to change the default if there is a strong case for it (merging will keep the exsiting behaviour of process-safe).

Our strong case is that this can break our customers as it is unexpectedly changing the current behavior. You do not have to deal with these "regressions" (which is what they will call it when they open escalations), but we do. Waiting one release to flip the switch should have been perfectly acceptable. We all agreed we need to document it first, but you went rogue and merged it anyway. I'm a bit speechless to be honest :-( We'll discuss this later...

This patch is to fix the variable names, it's not about the default value of what this configuration should be - changing the default is one line, and despite the merge, that doesn't mean this patch is going to customers yet ... so the default option can still be discussed, and changed.

I misinterpretted something thierry had commented and the passing ipa tests as ack so I apologise for that. :( That was my fault and I shouldn't have merged it - but as mentioned - it's not changing any behaviour at all.

Finally - this behaviour that is being discussed already has gone to customers, and has gone to production, it was reviewed months ago, and we discussed the changes. It's PR https://pagure.io/389-ds-base/pull-request/50379 . It was merged 4 months ago. The default in that PR is "warn" which is now named "process-safe", so nothing changes by this PR.

This ticket is not changing that functionality, it's not "breaking things". It's simply renaming the config options. The "breaking" happened 4 months ago when we all accepted this and merged it.

Anyway, the change if you want to disable by default will be:

https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/libglobs.c#_1786

Change that one line to

    init_verify_filter_schema = cfg->verify_filter_schema = SLAPI_WARN_UNSAFE;

That will give "warn-invalid" without the "breaking" behaviour. I wasn't kidding when I said oneline change ...

So I actually disagree, I think that when we make changes like this, we ship with the default that affects behaviour, people notice the behavioural change and then can make an informed decision to disable the feature OR to fix their applications. People should be testing in staging or dev first, and most ops people are well disciplined to do this and find issues before production.

We have to be sure to cover our bases and do our due diligence before we introduce a change in behavior to save us from unnecessary escalations (which cost developers time that they could've spend on new features). Especially if it can be easily avoided by having a contingency plan: feature documentation, release notes and a grace period before we flip the switch. And if users still complain about "sudden" change after we exhausted our ways to communicate it, the onus would be on them.

Finally - this behaviour that is being discussed already has gone to customers, and has gone to production, it was reviewed months ago, and we discussed the changes. It's PR https://pagure.io/389-ds-base/pull-request/50379 . It was merged 4 months ago. The default in that PR is "warn" which is now named "process-safe", so nothing changes by this PR.

It was not shipped in RHEL or RHDS builds, so it didn't affect our paying customers.
It was in Fedora and a regression was found: https://bugzilla.redhat.com/show_bug.cgi?id=1759709. People rightfully complained about missing documentation for this change. So as we're moving towards the next releases of RHEL and RHDS, we need to do our best to not repeat the same mistake as we did in Fedora.

As mentioned last night - I'm not really going to debate more on this, I'll defer to your experience. I'm going to make the changes as discussed to 1.4.2/1.4.3 to the default settings, and look at other improvements to communicate the change.

I'll follow up on https://pagure.io/389-ds-base/issue/50727 and once again sorry for the confusion caused by my mistakes :(

As mentioned last night - I'm not really going to debate more on this, I'll defer to your experience.

I would just like to say that it is okay to disagree on issues. Everyone here had valid points about why their approach was better. In the end the team decided to go in the direction of being cautious of breaking existing customers with a potentially disruptive change to search filter behaviour. So sometimes we all just have to agree to disagree, as long as we do it as a team.

As mentioned last night - I'm not really going to debate more on this, I'll defer to your experience.

I would just like to say that it is okay to disagree on issues. Everyone here had valid points about why their approach was better. In the end the team decided to go in the direction of being cautious of breaking existing customers with a potentially disruptive change to search filter behaviour. So sometimes we all just have to agree to disagree, as long as we do it as a team.

My choice of words could have been better, but basically, I said poorly what you said here :)

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3824

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.