Issue 5170 - BUG - incorrect behaviour of filter test

[Firstyear]

Bug Description: In the filter test during access only
checks, OR conditions were not correctly evaluated. They
would have their access checked, but it was not confirmed
if this was the element that the entry matched. This mean
that queries could incorrectly reduce entries.

Fix Description: In the "access only" check mode, do not
handle it as a "special case", and apply the full OR check
algorithm to ensure that the component we are matching on
is indeed, the one we are accessing.

fixes: #5170

Author: William Brown william@blackhats.net.au

Review by: ???

[Firstyear]

This CONFLICTS with #5316

Only ONE of these PR's can be merged!

[Firstyear]

@mreynolds389 seem to fully pass the tests in suites/filter for me.

[mreynolds389]

Here is my revised test case that includes CVE test:

import ldap
import logging
import pytest
import os
from lib389._constants import *
from lib389.topologies import topology_st as topo
from lib389.idm.domain import Domain
from lib389.idm.organizationalunit import OrganizationalUnits
from lib389.idm.account import Anonymous

log = logging.getLogger(__name__)


def test_filter_access(topo):
    """Search that compound filters are correctly processed by access control

    :id: ad6a3ffc-2620-4e76-909b-926f94c1a920
    :setup: Standalone Instance
    :steps:
        1. Add anonymous aci
        2. Add ou
        2. Test good filters
        4. Test bad filters
    :expectedresults:
        1. Success
        2. Success
        3. The good filters return the OU entry
        4. The bad filters do not return the OU entry
    """

    # Add aci
    ACI_TEXT = ('(targetattr="objectclass || postalCode")(version 3.0; acl "Anonymous read access"; allow' +
                '(read, search, compare) userdn = "ldap:///anyone";)')
    domain = Domain(topo.standalone, DEFAULT_SUFFIX)
    domain.replace('aci', ACI_TEXT)

    # Cleanup existing OU's
    ous = OrganizationalUnits(topo.standalone, DEFAULT_SUFFIX)
    existing_ous = ous.list()
    for eou in existing_ous:
        eou.delete(recursive=True)

    # Create container
    OU_PROPS = {
        'ou': 'container',
    }
    OrganizationalUnits(topo.standalone, DEFAULT_SUFFIX).create(properties=OU_PROPS)
    OU_DN = "ou=container," + DEFAULT_SUFFIX

    # Create allowed entry
    OU_PROPS = {
        'ou': 'allowed',
        'postalCode': '19605'
    }
    OrganizationalUnits(topo.standalone, "ou=container," + DEFAULT_SUFFIX).create(properties=OU_PROPS)

    # Create restricted entry
    OU_PROPS = {
        'ou': 'restricted',
        'description': 'secret data'
    }
    OrganizationalUnits(topo.standalone, "ou=container," + DEFAULT_SUFFIX).create(properties=OU_PROPS)

    # Do anonymous search using different filters
    GOOD_FILTERS = [
        ("(|(postalcode=*)(description=secret data))", 1),
        ("(&(postalcode=*)(|(description=secret data)(objectClass=organizationalunit)))", 1),
        ("(|(postalcode=*)(&(objectClass=organizationalunit)(description=secret data)))", 1),
        ("(|(objectClass=organizationalunit)(&(objectClass=organizationalunit)(description=secret data)))", 3),
        ("(|(&(objectClass=organizationalunit)(description=secret data))(objectClass=top))", 3),
        ("(|(objectClass=organizationalunit)(description=secret data)(sn=*))", 3),
        ("(|(description=secret data)(objectClass=organizationalunit)(sn=*))", 3),
        ("(|(sn=*)(description=secret data)(objectClass=organizationalunit))", 3),
        ("(objectClass=organizationalunit)", 3),
    ]
    BAD_FILTERS = [
        "(|(objectClass=person)(&(objectClass=organizationalunit)(description=secret data)))",
        "(&(objectClass=top)(objectClass=organizationalunit)(description=secret data))",
        "(|(&(description=*)(objectClass=top))(objectClass=person))",
        "(description=secret data)",
        "(description=*)",
        "(ou=*)",
    ]
    conn = Anonymous(topo.standalone).bind()

    # These searches should return the OU
    for search_filter, nentries in GOOD_FILTERS:
        entries = conn.search_s(OU_DN, ldap.SCOPE_SUBTREE, search_filter, escapehatch='i am sure')
        log.debug(f"Testing good filter: {search_filter} result: {len(entries)}")
        assert len(entries) == nentries

    # These searches should not return the OU
    for search_filter in BAD_FILTERS:
        entries = conn.search_s(OU_DN, ldap.SCOPE_ONELEVEL, search_filter, escapehatch='i am sure')
        log.debug(f"Testing bad filter: {search_filter} result: {len(entries)}")
        assert len(entries) == 0

if __name__ == '__main__':
    # Run isolated
    # -s for DEBUG mode
    CURRENT_FILE = os.path.realpath(__file__)
    pytest.main(["-s", CURRENT_FILE])

So I'd like to see this testcase updated, but the reset looks good to me! Approving fix...

[mreynolds389]

A few minor comments, but ack

found regressions

[mreynolds389]

Actually there are several "filter suite" regressions with this change:

FAILED filter_with_non_root_user_test.py::test_all_positive[(&(!(l=Cupertino))(!(|(uid=wal)(&(sn~=tiller) (roomNumber=2295)))))] - AssertionError: assert []
FAILED filter_with_non_root_user_test.py::test_all_positive[(&(l=Cupertino)(!(|(uid=wal)(&(sn~=tiller) (roomNumber=2295)))))] - AssertionError: assert []
FAILED filter_with_non_root_user_test.py::test_all_positive[(!(|(!(l=))(!(l=sunnyvale))))] - AssertionError: assert []
FAILED filter_with_non_root_user_test.py::test_all_positive[(&(!(l=Cupertino))(!(mail=exam))(!(|(uid=wal) (l=))))] - AssertionError: assert []
FAILED large_filter_test.py::test_large_filter[(&(objectClass=person)(|(manager=uid=fmcdonnagh )(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_0,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_1,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_2,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_3,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_4,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_5,)(manager=uid=jvedder,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_6,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_7,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_8,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_9,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_10,)(manager=uid=cnewport,)))]
FAILED vfilter_simple_test.py::test_param_positive[(&(!(l=Cupertino))(!(|(uid=wal)(&(nsRole~=cn=new managed) (nsRole=cn=vaddr)))))] - AssertionError: assert []
FAILED vfilter_simple_test.py::test_param_positive[(&(l=Cupertino)(!(|(uid=wal)(&(nsRole~=cn=new managed) (nsRole=cn=vaddr)))))] - AssertionError: assert []
FAILED vfilter_simple_test.py::test_param_positive[(!(|(!(l=))(!(l=sunnyvale))))] - AssertionError: assert []
FAILED vfilter_simple_test.py::test_param_positive[(&(!(l=Cupertino))(!(emailclass=emai))(!(|(nsRole=cn=vaddr) (l=))))] - AssertionError: assert []

[Firstyear]

Actually there are several "filter suite" regressions with this change:

FAILED filter_with_non_root_user_test.py::test_all_positive[(&(!(l=Cupertino))(!(|(uid=wal)(&(sn~=tiller) (roomNumber=2295)))))] - AssertionError: assert [] FAILED filter_with_non_root_user_test.py::test_all_positive[(&(l=Cupertino)(!(|(uid=wal)(&(sn~=tiller) (roomNumber=2295)))))] - AssertionError: assert [] FAILED filter_with_non_root_user_test.py::test_all_positive[(!(|(!(l=))(!(l=sunnyvale))))] - AssertionError: assert [] FAILED filter_with_non_root_user_test.py::test_all_positive[(&(!(l=Cupertino))(!(mail=exam))(!(|(uid=wal) (l=))))] - AssertionError: assert [] FAILED large_filter_test.py::test_large_filter[(&(objectClass=person)(|(manager=uid=fmcdonnagh )(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_0,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_1,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_2,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_3,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_4,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_5,)(manager=uid=jvedder,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_6,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_7,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_8,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_9,)(manager=cn=no_such_entry_with_a_really_long_dn_component_to_stress_the_filter_handling_code_10,)(manager=uid=cnewport,)))] FAILED vfilter_simple_test.py::test_param_positive[(&(!(l=Cupertino))(!(|(uid=wal)(&(nsRole~=cn=new managed) (nsRole=cn=vaddr)))))] - AssertionError: assert [] FAILED vfilter_simple_test.py::test_param_positive[(&(l=Cupertino)(!(|(uid=wal)(&(nsRole~=cn=new managed) (nsRole=cn=vaddr)))))] - AssertionError: assert [] FAILED vfilter_simple_test.py::test_param_positive[(!(|(!(l=))(!(l=sunnyvale))))] - AssertionError: assert [] FAILED vfilter_simple_test.py::test_param_positive[(&(!(l=Cupertino))(!(emailclass=emai))(!(|(nsRole=cn=vaddr) (l=))))] - AssertionError: assert []

Going to be fun to find out why these were failing probably because they were relying on some kind of bug ....

[Firstyear]

The problem appears to be that in "only check access mode" of the filter test, that testing is completely busted because the logic is just a complete mess. The whole notion of "just check access divorced from what actually matched on this attribute" is a pretty broken concept anyway. So I think the fix is that we always have to filter test when we acl check.

[Firstyear]

The issue is that because so many of these attributes aren't indexed, this is forcing the ALLIDS case, which means we go to the filter test which now has "acl check only" path.

IF THESE WERE INDEXED then this would have broken on the server "as is" because the skip of the filter test ALSO takes the busted "access check only" route. So I think if this had of indexed sn/roomnumber/l then this test would fail too.

Yet again, another bug ...

Anyway, the fix is to strictly and always check access AND the filter matching because that's the only way that actually works correctly. I'll update the PR shortly once the tests finish.

[Firstyear]

Worth pointing out, this was the only part of the code that used the broken access check only mode, everything else either does full access tests, or it's doing no access tests.

[Firstyear]

That's the no root test with my fix for the broken acl check mode.

 134 passed, 136 warnings in 675.69s (0:11:15)

[mreynolds389]

Request for changes... Fix indentation, update CI test case (although I might have a new one at some point), and I think for all the new(and existing?) "filter" logging, we should add more info to the logging lines, like the entry DN, etc. It would just make things easier to debug.

[Firstyear]

Will do the changes today. :)

[Firstyear]

done, not sure what you wanted done with the test case though ....

[mreynolds389]

Besides improving the logging, this looks good to me, and it passed the IPA tests.

[Firstyear]

Besides improving the logging, this looks good to me, and it passed the IPA tests.

exhausted hoorayyyyyy

I'll improve the log and will merge.

[Firstyear]

   4838b4ab1..7651a4ba3  389-ds-base-2.1 -> 389-ds-base-2.1
   031152f11..00f044f12  389-ds-base-2.0 -> 389-ds-base-2.0