#50727 CI test plugins/test_acctpolicy failing because of filter containing unknown attribute
Closed: fixed 5 months ago by mreynolds. Opened 9 months ago by tbordaz.

Issue Description

This bug is related to checking of filter component that they are known attribute (https://pagure.io/389-ds-base/issue/50349)

The tests plugin/test_acctpolicy creates an extensible object with an undefined attribute that is used in a search filter. By default (https://pagure.io/389-ds-base/issue/50349) is 'warn-strict' an the search returns not entry

Package Version and Platform

1.4.2

Steps to reproduce

# Configure schema check as warn (default)
dn: cn=config
nsslapd-verify-filter-schema: warn

# create an entry with unknown attribute (testlastlogintime)
dn: uid=test_user_1000,ou=People,dc=example,dc=com
objectClass: top
objectClass: account
objectClass: posixaccount
objectClass: inetOrgPerson
objectClass: organizationalPerson
objectClass: nsMemberOf
objectClass: nsAccount
objectClass: person
objectClass: extensibleObject
uid: test_user_1000
cn: test_user_1000
sn: test_user_1000
uidNumber: 1000
gidNumber: 2000
homeDirectory: /home/test_user_1000
testlastlogintime: 20191119145921Z

# ldapsearch -LLL ... -b "dc=example,dc=com" "(testlastlogintime=*)"
<no entry>

[19/Nov/2019:11:22:19.399045681 -0500] conn=9 op=0 BIND dn="cn=directory manager" method=128 version=3
[19/Nov/2019:11:22:19.492110741 -0500] conn=9 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(testlastlogintime=*)" attrs=ALL
[19/Nov/2019:11:22:19.492546261 -0500] conn=9 op=1 RESULT err=0 tag=101 nentries=0 etime=0.000592210 notes=F details="Filter Element Missing From Schema"

Actual results

Does not return the entry

Expected results

Should return the entry


You have a filter with a fully unindexed search, maybe you are hitting another server limit?

If you did this search with (&(testlastlogintime=*)(uid=foo)) it would probably work (there is a detailed filter test for this in the feature ....).

Metadata Update from @firstyear:
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None

9 months ago

@firstyear, yes there is a note=F plus details

But the filter with a '&' does not return the entry as well,

[20/Nov/2019:09:07:48.423016534 -0500] conn=11 op=0 BIND dn="cn=directory manager" method=128 version=3
[20/Nov/2019:09:07:48.531409401 -0500] conn=11 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(&(testlastlogintime=*)(uid=test_user_1000))" attrs=ALL
[20/Nov/2019:09:07:48.531812711 -0500] conn=11 op=1 RESULT err=0 tag=101 nentries=0 etime=0.000687936 notes=F details="Filter Element Missing From Schema"
...
[20/Nov/2019:09:08:29.612122270 -0500] conn=12 op=0 BIND dn="cn=directory manager" method=128 version=3
[20/Nov/2019:09:08:29.718679887 -0500] conn=12 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(uid=test_user_1000)" attrs=ALL
[20/Nov/2019:09:08:29.719161546 -0500] conn=12 op=1 RESULT err=0 tag=101 nentries=1 etime=0.000639975

If you set this setting to "nsslapd-verify-filter-schema: off" does it work?

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.4.2

9 months ago

@firstyear, sorry to back late on you question. Yes it works with 'nsslapd-verify-filter-schema: off'

PR https://pagure.io/389-ds-base/pull-request/50751

See my comments in the PR - I think this issue is invalid because after some memory searching and reflection, the whole point of this is RFC compliance that "an unknown attribute yields an empty idl", so extensibleObject here and using an attribute that isn't in schema, would cause exactly this problem and is the expected - and correct - behaviour.

@firstyear thanks for your review, indeed if 'warn' setting returns an empty IDL then the ticket will fix the testcase (see below) rather than changing the behavior of DS on 'warn'.

diff --git a/dirsrvtests/tests/suites/plugins/acceptance_test.py b/dirsrvtests/tests/suites/plugins/acceptance_test.py
index cdb629eef..f971be702 100644
--- a/dirsrvtests/tests/suites/plugins/acceptance_test.py
+++ b/dirsrvtests/tests/suites/plugins/acceptance_test.py
@@ -104,6 +104,10 @@ def test_acctpolicy(topo, args=None):
     """

     inst = topo[0]
+    # The testcase uses an unknown attribute: testLastLoginTime
+    # since https://pagure.io/389-ds-base/issue/50349, we need to disable
+    # schema checking on filter else '(testLastLoginTime=*)' will return no entry
+    inst.config.set("nsslapd-verify-filter-schema", "off")

     # stop the plugin, and start it
     plugin = AccountPolicyPlugin(inst)

I failed to retrieve the "an unknown attribute yields an empty idl" in the RFCs, do you mind to give me a pointer to it.

https://pagure.io/389-ds-base/pull-request/50252#comment-85228

This comment here has the section of text and a link. Hope that helps,

Thanks @firstyear for the pointer.

RFC 4511 4.5.1.7

A filter item evaluates to Undefined when the server would not be
able to determine whether the assertion value matches an entry.
...
For example, if a server did not recognize the attribute type
shoeSize, the filters (shoeSize=*), (shoeSize=12), (shoeSize>=12),
and (shoeSize<=12) would each evaluate to Undefined.

A server MUST evaluate filters according to the three-valued logic of
[X.511] (1993), Clause 7.8.1.  In summary, a filter is evaluated to
TRUE", "FALSE", or "Undefined".  
...
If the filter evaluates to FALSE or Undefined, then the entry is ignored for the Search.

So the fix to make a invalid filter component=idl(0) conforms the RFC.

However, because 389-ds was less strict than the RFC requires, I believe I remember that
we decided the default behavior (i.e. 'warn') should not change the existing user experience. Am I wrong ?

No, we agreed that we would become rfc compliant with the warn setting because the majority of users should NOT be using extensibleObject like this. We decided that the strict setting (rejecting the filter) was too strong, and that if we set it to "off" by default there was no point in having the feature at all.

So in my understanding we have three options:
- off: keep behaviour as is
- warn: set filtercomponent to 0 and become rfc compliant
- strict: reject the search totally

With the default of warn we are rfc compliant and do the right thing, but the naming is a bit confusing. I would expect from a 'warning' level to get a warning, but not to get a different behaviour. And the warning is only logged as "notes= " in the access log, the client does not get any clue why behaviour has changed.
Changing the "warn" name could cause new confusions, but I think that we should at least return the warning to the client in an "additional message" to the result code, stating that the search has been modified because of non schema attributes

Given this attribute is nowhere documented, yet (try to google it), we are more-or-less good changing the behaviour.

No one would expect a warn value to do something else than do the actual warning in the sense of throwing a message at a person.

If we want people to have these logged it should be explicitly settable (or at least we should have clear product options, so e.g. adding an strict-and-warn option or something).

Additionally, we shouldn't do breaking changes in a minor release without even documenting it. We should at least announce it in advance of a few minor releases and then change the behaviour.

No, we agreed that we would become rfc compliant with the warn setting because the majority of users should NOT be using extensibleObject like this. We decided that the strict setting (rejecting the filter) was too strong, and that if we set it to "off" by default there was no point in having the feature at all.

well, the agreement was in these two comments:
https://pagure.io/389-ds-base/pull-request/50379#comment-87214
https://pagure.io/389-ds-base/pull-request/50379#comment-89449

so the agreement had four values: off, warn, warn-strict, strict - looks like we have warn with the behavior of warn strict.

And I agree with Matus, we should not change behavior with an undocumented parameter

Given this attribute is nowhere documented, yet (try to google it), we are more-or-less good changing the behaviour.
No one would expect a warn value to do something else than do the actual warning in the sense of throwing a message at a person.
If we want people to have these logged it should be explicitly settable (or at least we should have clear product options, so e.g. adding an strict-and-warn option or something).
Additionally, we shouldn't do breaking changes in a minor release without even documenting it. We should at least announce it in advance of a few minor releases and then change the behaviour.

It should have been documented as part of redhat's release handover, as we were aware of it as a feature. It is absolutely my fault that there was no design document provided upstream though, so I will correct that now. The missing change to the config was also my fault, I think there was a lot going on in that discussion and I overlooked it (but so did everyone reviewing it, so I think we all have to share that one ...). So I think this issue is just to fix the config setting to have the 4th "warn-strict" option, and to default to it.

Changing values to improve the standard of our server is something we should do, and in discussion with @lkrispen I do remember that we agreed that enforcing the rfc compliant behaviour was the correct solution (default to warn-strict), even at the risk of affecting a minor number of deployments that use extensibleObject incorrectly.

I would say that a key point here is also that if we had of shipped this as "warn" by default, then the feature has almost no value because unless there are side effects admins aren't going to look at the 'notes=' field, and to be prompted that "hey, you have illegal filters".

So does this seem like an agreeable set of action items:

  • William to make design doc on port389.org
  • Someone to coordinate with @mmuehlfeldrh (?) about updating the RH command and configuration guid
  • PR for adding warn-strict, and setting it as the default
  • Communicate to GSS the design doc and inform them of symptoms and what do if it's seen

Is that reasonable?

@firstyear I agree with this actions items. Two comments

  • Regarding adding 'warn-strict' option, Would it be possible to add, like @lkrispen suggested, in send_ldap_result a message like 'warning because of use of unknown attribute the result set can be incomplete'.
  • I will use this ticket to fix the 'plugin/test_acctpolicy'

William to make design doc on port389.org
Someone to coordinate with @mmuehlfeldrh (?) about updating the RH command and configuration guid
PR for adding warn-strict, and setting it as the default
Communicate to GSS the design doc and inform them of symptoms and what do if it's seen

Is that reasonable?

yes

@firstyear I agree with this actions items. Two comments

Regarding adding 'warn-strict' option, Would it be possible to add, like @lkrispen suggested, in send_ldap_result a message like 'warning because of use of unknown attribute the result set can be incomplete'.

I'd need to think about where to add this message, but I don't see "why not"? My concern would be accidentally stomping on an already set result message if one existed.

I will use this ticket to fix the 'plugin/test_acctpolicy'

Cool. I'll put in a PR to fix the warn / warn-strict shortly.

This does NOT include @lkrispen's suggestion for the send_ldap_result message.

Metadata Update from @tbordaz:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1781120

8 months ago

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

We agreed to change the default value and to postpone the change of behavior to next release.
This PR does this change and also log some noisy warnings (in error logs) https://pagure.io/389-ds-base/pull-request/50751

Here are follow up issues to capture all the ideas we floated in the meeting.

And the PR that changes the default for 1.4.2:

1.4.3 as it currently stands will have the behaviour for process-safe.

for @mmuehlfeldrh : Here is the wiki page describing the change. https://github.com/marcus2376/389wiki/blob/master/docs/389ds/design/filter-syntax-verification.md

50792 was merged

commit 32be56b5139a3e84d58c19cf3ee2ee38030fc67f (HEAD -> 389-ds-base-1.4.2, origin/389-ds-base-1.4.2)

I'll be working on the three listed follow ups shortly.

6297b13..62432a2 389-ds-base-1.4.1 -> 389-ds-base-1.4.1

Part 2 for 1.4.1

3b2b6dd (HEAD -> 389-ds-base-1.4.1) Ticket 50727 - correct mistaken options in filter validation patch

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

5 months ago

Login to comment on this ticket.

Metadata