#50751 Ticket 50727 - CI test plugins/test_acctpolicy failing because of filter containing unknown attribute
Closed by spichugi. Opened by tbordaz.
tbordaz/389-ds-base ticket_50727  into  master

Download 50751.patch

Bug Description:
When configured 'nsslapd-verify-filter-schema: warn' a filter component containing an unkwown
attribute is flagged SLAPI_FILTER_INVALID_ATTR. This is flagged in the early steps of the SRCH
before the SRCH lands into the backend callback and evaluates IDL.
Later in backend callback, flag SLAPI_FILTER_INVALID_ATTR returns an empty IDL.
This should not be an empty IDL because it is configured as WARNING only. So it should be
the normal result of the index (if it exists) lookup.

Fix Description:
If we are in WARNING mode, just flag the operation (in frontend) so that 'Note=F' will be logged.
Then let the operation going on normally, even if it leads to an unindexed search.
The fix also contains a part of the testcase because search with only one unindexed component
is logged with 'notes=U...'

https://pagure.io/389-ds-base/issue/50727

Reviewed by: ?

Platforms tested: F30

Flag Day: no

Doc impact: no

I think this fix is wrong. The levels are:

  • strict -> reject the filter
  • warn -> accept the filter, log notes=F, and anything invalid is set to idl_alloc(0) as per the LDAP RFC
  • off -> accept the filter, do nothing.

So this change actually breaks the key value of warn which is as a security defence again DOS

If you are searching something like a=a and a is not in the schema then setting this to idl_alloc(0) is the correct behaviour.
If a is in the schema, then we won't reject it.

So I think this is the wrong approach ....

Actually a bit more though, I think the issue is invalid, extensibleObject is a cursed item, and absolutely should break in this case. Yoursely ludwig and I discussed this that we should be rfc compliant by default (which is what warn does), and then if people want to use extensibleObject they have to set this to "off" to accept that risk of security issue via DOS.

So I think this issue and this fix are invalid, and the user should just set this to "off".

@firstyear this is likely my fault, I misunderstood the conclusion of our discussion.
In my mind we decided, but I am probably wrong,

  • off: accept the filter and do nothing
  • on: reject the filter
  • warn: accept the filter and do nothing except logging a warning. So accept the risk of DOS.

If I am wrong, I agree that the subject of the ticket is invalid.
Note that I opened this ticket, while testing "tests/suites/plugins//acceptance_test.py::test_acctpolicy" that fails because the 'warn' setting returns an empty candidate list to '(testlastlogintime=*)' (testlastlogintime being unknown attribute). So with 'warn' -> idl(0) the testcase should be modified and I will use this ticket to fix it.

Warn accepts the filter and sets missing components to empty set is what we agreed I think, but we did talk about a lot that week I seem to recall!

rebased onto 7a59bff0f83aa3201951f10cc0e08e56b06c81eb

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

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.

Pull-Request has been closed by spichugi

Metadata