PR - Ticket 50727 - correct mistaken options in filter validation patch

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50769

• Created at 2019-12-06 05:10:17 by firstyear (@Firstyear)
• Merged at 2019-12-17 00:42:43

---

Bug Description: Because William of the past missed (forgot) to make
some agreed upon changes, we shipped the feature for filter validation
in a state that was a bit unclear for users.

Fix Description: Fix the options to now be clearer in what is
expected/demaned from admins. We now have 4 possible states for
the value of the config:

• reject-invalid (prev on)
• warn-process-safely (prev warn)
• warn-allow-unsafe
• off-allow-unsafe (prev off)

These behave as:

• reject-invalid - reject queries that contain unknown attributes
• warn-process-safely - log a notes=F that an attr is missing, and idl_alloc(0)
  the missing attribute for RFC4511 compliance.
• warn-allow-unsafe - log a notes=F that an attr is missing, and process
  as ALLIDS (the legacy behaviour)
• off-allow-unsafe - process as ALLIDs (the legacy behaviour)

The default is "warn-process-safely".

Resolves: #3782

Author: William Brown william@blackhats.net.au

Review by: ???

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-06 09:33:08

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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-06 10:16:17

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

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-12-06 10:23:41

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)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-06 10:47:33

good point @elkris 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)

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-12-06 11:10:13

good point @elkris 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)

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-06 21:30:19

good point @elkris 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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-09 01:19:30

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 @elkris 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 @elkris'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)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-09 10:22:43

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 ?

[389-ds-bot]

Comment from mmuehlfeldrh at 2019-12-09 11:37:37

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. :-)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-10 00:25:03

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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-10 11:44:55

@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 ?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-12 00:13:46

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 =
authorityo=3Ddev.blackhats.net.au,cn=3Dipauser1)
=
(altsecurityidentities=3DX509:O=3DDEV.BLACKHATS.NET.AU,CN=3DCertificate=
AuthorityO=3DDEV.BLACKHATS.NET.AU,CN=3Dipauser1)
)
(objectClass=3Dposixaccount)
(uid=3D*)
(uidNumber=3D*)
(!
(uidNumber=3D0)
)
)
)

The "intent" was to get accounts where a cert name is , and SSSD was =
emiting a filter that would work on both ipa and ad.

However, in reality, when processed this would cause the inner OR =
condition (ipacertmapdata=3D)(altsecurityidentites=3D), due to =
the altsecurityidentiies (from active directory) being unknown to be =
ALLIDS, which makes the OR ALLIDS, so then the filter is bascially =
becoming this index (idl)

(|
(objectClass=3Dreferral)
(&
(objectClass=3D*) <<-- the all IDS
(objectClass=3Dposixaccount)
(uid=3D*)
(uidNumber=3D*)
(!
(uidNumber=3D0)
)
)
)

And then the apply filter has to happen "after" to do the actual =
assertion about the certname.

So in a test env they would get the right result, because =
objectClass=3Daccount would be say less than 20 entries, which isn't =
hitting any limits.

But on a production machine, this would become a filter that either:

• Uses a lot of resources as you have to filter test "up to" the filter =
  test limit of allids
• Or the limits are hit and this "magically" returns no entries, much to =
  users frustration and disappointment that their system silently breaks.

In the production case I saw it was similar - VMware would do an ldap =
search for users with both posix and AD attributes to find users, which =
again became ALLIDS and caused both production LDAP servers to grind to =
a halt when enabled. That's because by default our lookthrough limit (I =
think it is) for allids cases is actually really really high, probably =
much too high for secure operations.=20

``
=20
To reply, visit the link below or just reply to this email
#3824

=E2=80=94
Sincerely,

William Brown

Senior Software Engineer, 389 Directory Server
SUSE Labs

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-12 00:57:52

rebased onto 9104539a0f24bf15a0cb1818f356a950480d263d

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-12 00:58:25

@tbordaz Updated based on your feedback :)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-12 10:04:31

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 @elkris and @mreynolds389 feedback.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-12 14:33:04

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.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-12 23:10:41

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.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-12 23:17:48

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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-13 16:23:33

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

[389-ds-bot]

Comment from abiagion at 2019-12-13 22:55:19

I've built a custom box for testing with freeipa-pr-ci, here it is the test run:
freeipa-pr-ci2/freeipa#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.

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-16 10:16:17

@abiagion thank you very much for the run. It helps a lot.
@mreynolds389 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.

[389-ds-bot]

Comment from vashirov (@vashirov) at 2019-12-16 12:01:29

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?

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-12-16 12:16:28

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

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-16 16:45:01

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".

[389-ds-bot]

Comment from vashirov (@vashirov) at 2019-12-16 16:57:38

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

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-16 17:00:41

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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 00:38:52

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.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 00:39:26

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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 00:42:07

rebased onto 9327a33

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 00:42:43

Pull-Request has been merged by Firstyear

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 00:43:30

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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 00:48:02

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

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-17 03:52:58

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...

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 04:04:29

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 #3438 . 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.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-17 04:11:50

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 ...

[389-ds-bot]

Comment from vashirov (@vashirov) at 2019-12-17 12:29:05

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 #3438 . 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.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-18 01:13:28

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 #3782 and once again sorry for the confusion caused by my mistakes :(

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-18 16:39:39

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.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-12-19 06:29:39

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-bot]

Patch
50769.patch