The F29 build fails because of the test:

[  ERROR   ] --- validate_filter(invalid, FILTER_POLICY_WARNING) == FILTER_SCHEMA_WARNING
[   LINE   ] --- test/libslapd/schema/filter_validate.c:95: error: Failure!
[  FAILED  ] test_libslapd_schema_filter_validate_simple

@spichugi yes that's expected, @tbordaz has answered my question (This code is not in a mergeable state yet).

@tbordaz As a follow up, can you check this to see if my approach to checking if the filter attributes are in schema seems reasonable and correct? I've tried to make this a cmocka test so that we can be more confident in the code compare to just a lib389 test - it's also a bit easier to debug too. Would like to know your thoughts on if this seems reasonable so far. :)

Why not using attr_syntax_get_by_name_locking_optional(name, FALSE, 0)

I have a bit concern about locking the syntax tables here. I could increase contention on it.
Do we really need the lock for a lookup ?

The patch looks good but I am still not convinced of the need to check the filter against unknown attribute.
It is valid to have unknown attribute in a filter. ldap client can create unindexed search but if it is by mistake, client could check notes=U/A. If it is to prevent an attack why not relying on required-index param ?

@tbordaz We need the read lock on the lookup, yes. Because it can be dynamically changed by schema, we really do need the read lock. Given it's an rwlock, I don't think there is much risk here as schema tends to be very static as a structure, and we only take the read path. The risk would actually be on linux that we delay the writer to schema because linux RW locks are reader favouring, not writer favouring.

I think I laid out the justification pretty well for why this is needed:

-- flags U/A doesn't not indicate the cause of the error missing index, and FreeIPA have shown they will ignore these flags when going to production.

-- We should provide appropriate feedback when an erroring query is submitted. The RFC is patently wrong to think "silently allowing" it is a reasonable course of action. It violates reasonable human interaction and psychology principles.

-- it absolutely can cause a denial of service on larger sites, and I have seen this in production both at UofA and in working with GSS at RH.

-- FreeIPA has gone to production, with queries on attributes that do not exist, that collapse to full table scans - because we do not tell people when they are making mistakes. I don't think we can keep blaming people for "holding it wrong" when our server is just so difficult to use and get right.

So it really really is justified to have this IMO which is why I'm pursuing so to much - because this is a feature I would LOVE to have if I was a sys-admin over again :)

Anyway, I really do appreciate the review, thank you!

Why not using attr_syntax_get_by_name_locking_optional(name, FALSE, 0)

Because I want to avoid the reference count cycle that's in this function, because I only need to check the char* is in the map, I don't need to take a reference to it. Basically saving two atomics in the process because else I'd need to call release as well.

PR_ASSERT(0) ?

@tbordaz We need the read lock on the lookup, yes. Because it can be dynamically changed by schema, we really do need the read lock. Given it's an rwlock, I don't think there is much risk here as schema tends to be very static as a structure, and we only take the read path. The risk would actually be on linux that we delay the writer to schema because linux RW locks are reader favouring, not writer favouring.

I agree that update of schema are rare and most of the time the RWLock will be available in read. Now a RWLock has a cost even if it is free (see https://pagure.io/389-ds-base/issue/49873). Do you expect the filter validation to be enabled by default ?

I think I laid out the justification pretty well for why this is needed:
-- flags U/A doesn't not indicate the cause of the error missing index, and FreeIPA have shown they will ignore these flags when going to production.

When performance comes on the table, U/A and etime are important. I agree it does not help much to find the RC but it shows where to investigate.

-- We should provide appropriate feedback when an erroring query is submitted. The RFC is patently wrong to think "silently allowing" it is a reasonable course of action. It violates reasonable human interaction and psychology principles.
-- it absolutely can cause a denial of service on larger sites, and I have seen this in production both at UofA and in working with GSS at RH.

Using unknown attribute can create DOS. But there are many valid ways to create unindexed filter (having NOT ('!'), improbable attr (e.g. 'nsServerMigrationClassname'), improbable MR (e.g. 'userSMIMECertificate=foo')... requiring use of index look more global approach for preventing unindexed DOS.

IMHO a benefit of the patch is a help on diagnostic when by mistake client use wrong attribute name

-- FreeIPA has gone to production, with queries on attributes that do not exist, that collapse to full table scans - because we do not tell people when they are making mistakes. I don't think we can keep blaming people for "holding it wrong" when our server is just so difficult to use and get right.

Right.

So it really really is justified to have this IMO which is why I'm pursuing so to much - because this is a feature I would LOVE to have if I was a sys-admin over again :)
Anyway, I really do appreciate the review, thank you!

The justification make sense and the patch looks good. My only concern is about potential contention. Have you run some SRCH load to verify filter verification has no contention on attribute syntax ?

@tbordaz We need the read lock on the lookup, yes. Because it can be dynamically changed by schema, we really do need the read lock. Given it's an rwlock, I don't think there is much risk here as schema tends to be very static as a structure, and we only take the read path. The risk would actually be on linux that we delay the writer to schema because linux RW locks are reader favouring, not writer favouring.

I agree that update of schema are rare and most of the time the RWLock will be available in read. Now a RWLock has a cost even if it is free (see https://pagure.io/389-ds-base/issue/49873). Do you expect the filter validation to be enabled by default ?

I would like it in warning by default (says that there are missing elements, but we don't reject bad filters).

We have plenty of atomics and locking through the codebase. We don't blink at adding another lock in a plugin or another atomic in libglobs.c. I think if we have a concern about locking, we should really rethink our concurrency strategy because it's a bit "un-planned". Saying this, I'm not worried too much about the read lock, read sides of those locks are very low cost. I would be more worried if this was a mutex .... (I'll still test performance though! I just think sometimes we misplace our concerns to single mutexs/locks when our issues are structural and how we use combinations of those constructions :) )

I think I laid out the justification pretty well for why this is needed:
-- flags U/A doesn't not indicate the cause of the error missing index, and FreeIPA have shown they will ignore these flags when going to production.

When performance comes on the table, U/A and etime are important. I agree it does not help much to find the RC but it shows where to investigate.

-- We should provide appropriate feedback when an erroring query is submitted. The RFC is patently wrong to think "silently allowing" it is a reasonable course of action. It violates reasonable human interaction and psychology principles.
-- it absolutely can cause a denial of service on larger sites, and I have seen this in production both at UofA and in working with GSS at RH.

Using unknown attribute can create DOS. But there are many valid ways to create unindexed filter (having NOT ('!'), improbable attr (e.g. 'nsServerMigrationClassname'), improbable MR (e.g. 'userSMIMECertificate=foo')... requiring use of index look more global approach for preventing unindexed DOS.
IMHO a benefit of the patch is a help on diagnostic when by mistake client use wrong attribute name

Yep, feedback certainly is a major element of this patch :) preventing dos in some cases is a nice side effect (especially if we can replace missing with idl of 0 len rather than allids).

This is absolutely true for the improbably attr, but the improbably MR would be rare I would think ... we should think about how to help in these cases too. :)

-- FreeIPA has gone to production, with queries on attributes that do not exist, that collapse to full table scans - because we do not tell people when they are making mistakes. I don't think we can keep blaming people for "holding it wrong" when our server is just so difficult to use and get right.

Right.

So it really really is justified to have this IMO which is why I'm pursuing so to much - because this is a feature I would LOVE to have if I was a sys-admin over again :)
Anyway, I really do appreciate the review, thank you!

The justification make sense and the patch looks good. My only concern is about potential contention. Have you run some SRCH load to verify filter verification has no contention on attribute syntax ?

Well, the patch at the moment is proof of concept that I'm looking up attrs and verifying filters correctly only - it's only cmocka tests! I wanted expert review to be sure I was approaching the problem the correct way before I went too deep into integration. I am still working on the integration to have it integrate with SRCH. I'll be sure to run some ldclt tests for you with good/bad filters to be sure :)

I'll probably have it integrated and working today I expect (I wrote the libglobs part yesterday, as well as lib389 tests), so we can have a better review shortly.

Thank you!

@tbordaz I have just pushed the rest of the "now integrated" feature, which works as expected and has a test case. I'd love your review of it (I plan to do a self-review soon to make sure I haven't missed anything).

In addition I'd like to wait for https://pagure.io/389-ds-base/pull-request/50252 to be merged before I merge this because I think they both touch similar areas, and I want the tests from 50252 to be merged so I can run them against this PR. So even if you ack this, I'll probably delay merge until the filter opt PR is merged too.

Thanks!

EDIT: PS - I'm about to setup and test performance of this now :)

Performed on 6core i7 w_ 32Gb of ram + SSD
OFF (mail=xxx@) lt + idlscan 20k
DEBUG:lib389:b'ldclt version 4.23\nldclt[30924]: Starting at Tue May 21 16:15:32 2019\n\nldclt[30924]: Average rate: 5569.20/thr  (5569.20/sec), total:  55692\nldclt[30924]: Average rate: 5347.30/thr  (5347.30/sec), total:  53473\nldclt[30924]: Average rate: 5492.10/thr  (5492.10/sec), total:  54921\nldclt[30924]: Average rate: 5397.30/thr  (5397.30/sec), total:  53973\nldclt[30924]: Average rate: 5413.00/thr  (5413.00/sec), total:  54130\nldclt[30924]: Average rate: 5469.10/thr  (5469.10/sec), total:  54691\nldclt[30924]: Average rate: 5461.30/thr  (5461.30/sec), total:  54613\nldclt[30924]: Average rate: 4843.30/thr  (4843.30/sec), total:  48433\nldclt[30924]: Average rate: 5037.10/thr  (5037.10/sec), total:  50371\nldclt[30924]: Average rate: 5248.30/thr  (5248.30/sec), total:  52483\nldclt[30924]: Number of samples achieved. Bye-bye...\nldclt[30924]: All threads are dead - exit.\nldclt[30924]: Global average rate: 53278.00/thr  (5327.80/sec), total: 532780\nldclt[30924]: Global number times "no activity" reports: never\nldclt[30924]: Global no error occurs during this session.\nldclt[30924]: Ending at Tue May 21 16:17:12 2019\nldclt[30924]: Exit status 0 - No problem during execution.\nldclt[30924]: T005: thread is dead.\nldclt[30924]: T000: thread is dead.\nldclt[30924]: T004: thread is dead.\n'
WARN (mail=xxx@) lt + idlscan 20k
DEBUG:lib389:b'ldclt version 4.23\nldclt[30777]: Starting at Tue May 21 16:12:52 2019\n\nldclt[30777]: Average rate: 5612.70/thr  (5612.70/sec), total:  56127\nldclt[30777]: Average rate: 5586.00/thr  (5586.00/sec), total:  55860\nldclt[30777]: Average rate: 5380.90/thr  (5380.90/sec), total:  53809\nldclt[30777]: Average rate: 5370.50/thr  (5370.50/sec), total:  53705\nldclt[30777]: Average rate: 5100.10/thr  (5100.10/sec), total:  51001\nldclt[30777]: Average rate: 5368.10/thr  (5368.10/sec), total:  53681\nldclt[30777]: Average rate: 5183.30/thr  (5183.30/sec), total:  51833\nldclt[30777]: Average rate: 5373.20/thr  (5373.20/sec), total:  53732\nldclt[30777]: Average rate: 5188.20/thr  (5188.20/sec), total:  51882\nldclt[30777]: Average rate: 5012.70/thr  (5012.70/sec), total:  50127\nldclt[30777]: Number of samples achieved. Bye-bye...\nldclt[30777]: All threads are dead - exit.\nldclt[30777]: Global average rate: 53175.70/thr  (5317.57/sec), total: 531757\nldclt[30777]: Global number times "no activity" reports: never\nldclt[30777]: Global no error occurs during this session.\nldclt[30777]: T005: thread is dead.\nldclt[30777]: Ending at Tue May 21 16:14:32 2019\nldclt[30777]: Exit status 0 - No problem during execution.\nldclt[30777]: T007: thread is dead.\nldclt[30777]: T003: thread is dead.\nldclt[30777]: T004: thread is dead.\nldclt[30777]: T008: thread is dead.\n'
OFF (|(mail=xxx@)(nonexist=foo)) lt + idlscan 20k
DEBUG:lib389:b'ldclt version 4.23\nldclt[30483]: Starting at Tue May 21 16:07:38 2019\n\nldclt[30483]: Average rate:    7.40/thr  (   7.40/sec), total:     74\nldclt[30483]: Average rate:    8.30/thr  (   8.30/sec), total:     83\nldclt[30483]: Average rate:    8.80/thr  (   8.80/sec), total:     88\nldclt[30483]: Average rate:    9.00/thr  (   9.00/sec), total:     90\nldclt[30483]: Average rate:    8.90/thr  (   8.90/sec), total:     89\nldclt[30483]: Average rate:    8.70/thr  (   8.70/sec), total:     87\nldclt[30483]: Average rate:    8.70/thr  (   8.70/sec), total:     87\nldclt[30483]: Average rate:    9.10/thr  (   9.10/sec), total:     91\nldclt[30483]: Average rate:    8.70/thr  (   8.70/sec), total:     87\nldclt[30483]: Average rate:    8.70/thr  (   8.70/sec), total:     87\nldclt[30483]: Number of samples achieved. Bye-bye...\nldclt[30483]: All threads are dead - exit.\nldclt[30483]: Global average rate:   86.30/thr  (  8.63/sec), total:    863\nldclt[30483]: Global number times "no activity" reports: never\nldclt[30483]: Global no error occurs during this session.\nldclt[30483]: Ending at Tue May 21 16:09:18 2019\nldclt[30483]: Exit status 0 - No problem during execution.\n'
WARN (|(mail=xxx@)(nonexist=foo)) lt + idlscan 20k
DEBUG:lib389:b'ldclt version 4.23\nldclt[30630]: Starting at Tue May 21 16:10:11 2019\n\nldclt[30630]: Average rate: 4808.70/thr  (4808.70/sec), total:  48087\nldclt[30630]: Average rate: 4734.90/thr  (4734.90/sec), total:  47349\nldclt[30630]: Average rate: 4846.30/thr  (4846.30/sec), total:  48463\nldclt[30630]: Average rate: 4819.70/thr  (4819.70/sec), total:  48197\nldclt[30630]: Average rate: 4529.80/thr  (4529.80/sec), total:  45298\nldclt[30630]: Average rate: 4489.10/thr  (4489.10/sec), total:  44891\nldclt[30630]: Average rate: 4327.60/thr  (4327.60/sec), total:  43276\nldclt[30630]: Average rate: 4730.40/thr  (4730.40/sec), total:  47304\nldclt[30630]: Average rate: 4686.30/thr  (4686.30/sec), total:  46863\nldclt[30630]: Average rate: 4575.80/thr  (4575.80/sec), total:  45758\nldclt[30630]: Number of samples achieved. Bye-bye...\nldclt[30630]: All threads are dead - exit.\nldclt[30630]: Global average rate: 46548.60/thr  (4654.86/sec), total: 465486\nldclt[30630]: Global number times "no activity" reports: never\nldclt[30630]: Global no error occurs during this session.\nldclt[30630]: Ending at Tue May 21 16:11:51 2019\nldclt[30630]: Exit status 0 - No problem during execution.\nldclt[30630]: T002: thread is dead.\nldclt[30630]: T007: thread is dead.\nldclt[30630]: T003: thread is dead.\nldclt[30630]: T006: thread is dead.\nldclt[30630]: T000: thread is dead.\nldclt[30630]: T005: thread is dead.\n'

There are two tests in here:

• To show that there is no regression with this on warning compared to off.
• To show the effect of the warning mode which replaces invalid types with idl_alloc(0).

Summary:

• There is neglible change between off and warning (warning was infact slightly better, but I think this could be test noise as it's a low % change).
• When warn is on and invalid types are used, the server is orders of magnitudes better at handling this as a DoS case (this is dependent on sites setting lookthrough to a high number). Even without this, if look through was low, you still are "shortcutting" by returning faster due to the empty or member so we aren't doing work and throwing it away before hitting admin limit.

Hope this helps!

@firstyear I like this change to use slapi_pblock_set_flag_operation_notes but it is a different change from checking the attribute against the schema. IMHO it makes review more complex and I would prefer a separate ticket.
In addition I think it is a wrong use here. slapi_pblock_set does set the value while slapi_pblock_set_flag_operation_notes do a OR.
Better to check the others places where you used slapi_pblock_set_flag_operation_notes that the OR was intended

I have a doubt here. A component with an unknown attribute results in an idl_allids rather than an empty list.
I have the same doubt at many places of the patch that contain idl_alloc(0)

Just realized that you also created slapi_pblock_set_operation_notes that can be used here

@firstyear, thanks for your performance tests. How many threads did you use for the load (10) ?
Do you mind to rerun the test (with call to slapi_filter_schema_check and without) with more threads than workers and check (pstack) if there are some of them in contention on asi lock during slapi_filter_schema_check.

Just forgot to say... it is very nice patch ;)

I have a doubt here. A component with an unknown attribute results in an idl_allids rather than an empty list.
I have the same doubt at many places of the patch that contain idl_alloc(0)

I can see that this could be a more discussion heavy part of the change. IMO idl_alloc(0) is the correct thing to return because:

-- If the attribute is not in schema, it is not possible to validly index it due to missing equality, so the only reasonable result is to say nothing matches it.

-- It prevents the DOS case. (|(nonexist=foo)(uid=1000)). uid=1000 should return an IDL of length one, but if we did ALLIDS, this would become an ALLIDS search. This then causes us to do a fulltable scan, hitting the lookthrough limit (which could be in the thousands in some sites - admins do not understand this value and how it applies to the search process from what I've seen of deployments ...). By returning idl_alloc(0), we add a security feature which is that the search is never going to satisfy, preventing ALLIDS.

EDIT: This also is a problem for freeipa where a search or feature in development looks like it is functional, but in production is performing poorly or erroring with admin limits. So "fail fast" means the issue would be resolved by IPA in dev rather than in production (with a lot less finger pointing between 389 and freeipa to say who's at fault).

-- Returning idl_alloc(0) here would be RFC compliant (as much as I think the rfc is wrong here ....). From @abbra's comment:

 I don't think it is realistically possible to enforce that all attributes of a filter are present in schema and reject the filter otherwise. RFC 4511 section 4.5.1.7. states that
     Servers MUST NOT return errors if attribute descriptions or matching rule ids are not recognized, assertion values are invalid, or the assertion syntax is not supported. More details of filter processing are given in Clause 7.8 of [X.511].
and Clause 7.8 of https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-X.511-201610-I!!PDF-E&type=items has this language:
    Any assertion about the values of such an attribute is only defined if the AttributeType is known by the evaluating mechanism, the purported AttributeValue(s) conforms to the attribute syntax defined for that attribute type, the implied or indicated matching rule is applicable to that attribute type, and (when used) a presented matchValue conforms to the syntax defined for the indicated matching rules. When these conditions are not met, the FilterItem shall evaluate to the logical value UNDEFINED.
    An assertion which is defined by these conditions additionally evaluates to UNDEFINED if it relates to an attribute value and the attribute type is not present in an attribute against which the assertion is being tested. An assertion which is defined by these conditions and relates to the presence of an attribute type evaluates to FALSE.

The tl;dr being - if the attribute type is not known then it is undefined, and evals to false - idl_alloc(0) in our case because no candidate can satisfy a false requirement.

For sites relying on this behaviour for extensibleObject, they can set this feature to "off", which of course disables the warning and the idl_alloc(0) behaviour. Remember, the "on" behaviour is strict, which rejects the filter outright (default is warn).

IMO the rfc is wrong because the filter should never be accepted in the first place - in William's perfect world I would default to "strict" - but we do not live in my perfect world, so I chose to default to warn, which is RFC compliant.

So I think these are the reasons and justifications for the idl_alloc(0) rather than ALLIDS here.

@firstyear I like this change to use slapi_pblock_set_flag_operation_notes but it is a different change from checking the attribute against the schema. IMHO it makes review more complex and I would prefer a separate ticket.
In addition I think it is a wrong use here. slapi_pblock_set does set the value while slapi_pblock_set_flag_operation_notes do a OR.
Better to check the others places where you used slapi_pblock_set_flag_operation_notes that the OR was intended

This is actually really important to use the set_flag instead - if the filter was:

((mail=foo)(nonexist=bar))

Assume mail is unindexed, and nonexist is not in schema. As a result, after ava_candidates opnotes is OP_NOTE_UNINDEXED due to mail.

If we used opnote set, we would CLEAR the unindexed flag, then replace with OP_NOTE_FILTER_INVALID, losing the notes=U.

By using setflag, we keep both flags in the opnotes, building the opnotes to a set of flags instead. So this is actually a really key change to the api and process here.

Maybe it's also my perfection-ist nature, but the get/set I wrote is better to avoid a set of derferences and case-switch that is 'costly' (it probably isn't costly at all, but even 0.01% of a million is 100. It adds up :)

Thank you for your awesome review, I have addressed your other comments in the code, and I'm going to expand a few code comments to help make some other questions clearer too. Your other points have not been ignored!

I have a doubt here. A component with an unknown attribute results in an idl_allids rather than an empty list.
I have the same doubt at many places of the patch that contain idl_alloc(0)

I can see that this could be a more discussion heavy part of the change. IMO idl_alloc(0) is the correct thing to return because:
-- If the attribute is not in schema, it is not possible to validly index it due to missing equality, so the only reasonable result is to say nothing matches it.
-- It prevents the DOS case. (|(nonexist=foo)(uid=1000)). uid=1000 should return an IDL of length one, but if we did ALLIDS, this would become an ALLIDS search. This then causes us to do a fulltable scan, hitting the lookthrough limit (which could be in the thousands in some sites - admins do not understand this value and how it applies to the search process from what I've seen of deployments ...). By returning idl_alloc(0), we add a security feature which is that the search is never going to satisfy, preventing ALLIDS.

Okay but if with a filter like (&(nonexist=foo)(uid=1000)), because of idl_alloc(0) this filter will return no candidate although it exists one.

EDIT: This also is a problem for freeipa where a search or feature in development looks like it is functional, but in production is performing poorly or erroring with admin limits. So "fail fast" means the issue would be resolved by IPA in dev rather than in production (with a lot less finger pointing between 389 and freeipa to say who's at fault).
-- Returning idl_alloc(0) here would be RFC compliant (as much as I think the rfc is wrong here ....). From @abbra's comment:
I don't think it is realistically possible to enforce that all attributes of a filter are present in schema and reject the filter otherwise. RFC 4511 section 4.5.1.7. states that

 Servers MUST NOT return errors if attribute descriptions or matching rule ids are not recognized, assertion values are invalid, or the assertion syntax is not supported. More details of filter processing are given in Clause 7.8 of [X.511].

and Clause 7.8 of https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-X.511-201610-I!!PDF-E&type=items has this language:

Any assertion about the values of such an attribute is only defined if the AttributeType is known by the evaluating mechanism, the purported AttributeValue(s) conforms to the attribute syntax defined for that attribute type, the implied or indicated matching rule is applicable to that attribute type, and (when used) a presented matchValue conforms to the syntax defined for the indicated matching rules. When these conditions are not met, the FilterItem shall evaluate to the logical value UNDEFINED.
An assertion which is defined by these conditions additionally evaluates to UNDEFINED if it relates to an attribute value and the attribute type is not present in an attribute against which the assertion is being tested. An assertion which is defined by these conditions and relates to the presence of an attribute type evaluates to FALSE.

The tl;dr being - if the attribute type is not known then it is undefined, and evals to false - idl_alloc(0) in our case because no candidate can satisfy a false requirement.

I think it is where I am confuse. The filter is used to build a candidate list and then to match a candidate before returning it.
To build a candidate list there is no undefined. There are only lists to merge/intersect.
Later during entry matching I agree that unknown attribute may land in 'undefined'.

For sites relying on this behaviour for extensibleObject, they can set this feature to "off", which of course disables the warning and the idl_alloc(0) behaviour. Remember, the "on" behaviour is strict, which rejects the filter outright (default is warn).
IMO the rfc is wrong because the filter should never be accepted in the first place - in William's perfect world I would default to "strict" - but we do not live in my perfect world, so I chose to default to warn, which is RFC compliant.
So I think these are the reasons and justifications for the idl_alloc(0) rather than ALLIDS here.

@firstyear I like this change to use slapi_pblock_set_flag_operation_notes but it is a different change from checking the attribute against the schema. IMHO it makes review more complex and I would prefer a separate ticket.
In addition I think it is a wrong use here. slapi_pblock_set does set the value while slapi_pblock_set_flag_operation_notes do a OR.
Better to check the others places where you used slapi_pblock_set_flag_operation_notes that the OR was intended

This is actually really important to use the set_flag instead - if the filter was:
((mail=foo)(nonexist=bar))

Assume mail is unindexed, and nonexist is not in schema. As a result, after ava_candidates opnotes is OP_NOTE_UNINDEXED due to mail.
If we used opnote set, we would CLEAR the unindexed flag, then replace with OP_NOTE_FILTER_INVALID, losing the notes=U.
By using setflag, we keep both flags in the opnotes, building the opnotes to a set of flags instead. So this is actually a really key change to the api and process here.
Maybe it's also my perfection-ist nature, but the get/set I wrote is better to avoid a set of derferences and case-switch that is 'costly' (it probably isn't costly at all, but even 0.01% of a million is 100. It adds up :)
Thank you for your awesome review, I have addressed your other comments in the code, and I'm going to expand a few code comments to help make some other questions clearer too. Your other points have not been ignored!

Okay but if with a filter like (&(nonexist=foo)(uid=1000)), because of idl_alloc(0) this filter will return no candidate although it exists one.

But it can't exist! nonexist is not in schema so it would never be valid to add to an object, so this query can never match.

And if it is on the object you must have extensibleObject then, so you turn this setting "off".

The tl;dr being - if the attribute type is not known then it is undefined, and evals to false - idl_alloc(0) in our case because no candidate can satisfy a false requirement.

I think it is where I am confuse. The filter is used to build a candidate list and then to match a candidate before returning it.
To build a candidate list there is no undefined. There are only lists to merge/intersect.
Later during entry matching I agree that unknown attribute may land in 'undefined'.

The filter evaluates to undefined if the attribute requested is not in schema - undefined causing the filter to eval to false. Therefore the candidate list that filter yields must be idl_alloc(0), because false can never be satisfied. That's what the RFC is saying.

So as a result (|(nonexist=foo)(uid=william)) would yield only "uid=william", becuause the condition nonexist=foo is always evaluated to false, aka empty set. If this was an & condition, nothing could possibly match, because it's not possible for uid=william to have nonexist as an attribute as it's not valid on schema.

Of course, extensibleObject ruins and trashes all of this, but that's why there is a tunable to disable it for those people who chose to use EO on their main backend.

EDIT: Go back to the freeipa certmap case. Because their filter was:

(&
  (|
    (non-existant-ad-attribute=...)
    (certSubjectDn=something...)
  )
  (objectClass=ipaaccount)
)

Currently, this would ALWAYS evaluate to the candidate set of "objectClass=ipaaccount", which then must all be filter tested. That's because the non-existant-ad-attribute becomes ALLIDS, and ALLIDS in an OR must return ALLIDS. So then objectClass=ipaaccount is the only filter/index matching! For a large enough site, if the lookthroughlimit was low, this would cause all certmap queries to suddenly fail.

Instead, if you have non-existant-ad return idl_alloc(0), the certSubjectDn query can now be satisfied, which then can be ANDed with the objectClass=ipaaccount. We get the proper result, always, regardless of the database scale. (Throw in filter optimisation/test threshold, we may not even load the objectClass=ipaaccount index at all!).

So I stand by the idl_alloc(0) being the right decision here as part of filter validation, both in practical aspects to resolve active FreeIPA issues, and to conform to the RFC.

@firstyear thanks for your explanations. So if I understand correctly, 'unknown attribute' filter check can fail if some entries are extensibleobject and contain unknown attributes. Correct ?

In that case, it looks important to me that the default config value is SLAPI_WARN (like it is currently).

So my last concern is the contention on attribute syntax lock. If your tests (with many threads) show no contention on it. I am okay with patch.

thanks

@firstyear thanks for your explanations. So if I understand correctly, 'unknown attribute' filter check can fail if some entries are extensibleobject and contain unknown attributes. Correct ?

Yes - if you have an extensibleObject with attribute "foo=bar", and you did a filter of (foo=bar), this would yield no results as foo is not part of schema, so must evaluate to false (undefined).

In that case, it looks important to me that the default config value is SLAPI_WARN (like it is currently).

To be 100% clear:

• off -> do nothing
• warn -> warn that you have a schema-missing filter component AND evaluate that component to undefined
• strict -> reject the filter if a schema-missing filter component is detected

To be sure there is no confusion about warn vs strict here :)

So my last concern is the contention on attribute syntax lock. If your tests (with many threads) show no contention on it. I am okay with patch.
thanks

I need to re-run the test with the pstacks to be 100% sure, but the initial results did not seem to display any contention. But I'll run again with pstacks for you :)

Yes - if you have an extensibleObject with attribute "foo=bar", and you did a filter of (foo=bar), this would yield no results as foo is not part of schema, so must evaluate to false (undefined).

In that case, it looks important to me that the default config value is SLAPI_WARN (like it is currently).

To be 100% clear:

off -> do nothing
warn -> warn that you have a schema-missing filter component AND evaluate that component to undefined

An admin may ignore if the DB contains extensibleObject entries with invalid attribute.
I see two options in WARN mode:

• log a warning that the filter contains unknown attribute and that the set of returned entries may be incomplete
• log a warning that the filter contains unknown attribute and that it could be the reason of unindexed search (perf issue).

So I think you are suggesting:

off -> do nothing
warn -> warn that there is an unknown attr that may cause a perf issue
warn-strict -> warn there is unknown, and also set it to undefined so some results may be missing
strict -> reject the filter

Is that correct?

I still think that warn-strict should be the default to be rfc compliant, and because without a "feedback" such as missing results, people won't be inclined to look and work otu "why" something is happening.

We agreed that @tbordaz would contact some people internal to RH to ask about the correct default value for this, but discussing between @tbordaz, @lkrispen and I, we agreed that warn-strict (current warn), is the correct option as most current situations work this way, and we have a security benefit. Any disruption to users would be very odd situations.

In a meeting I noted from @mreynolds suggestion:

• Add logconv.pl support
• possible to add extended text from note=F to describe the problem. It could be worth adding extra text for A, U at the same time!

@mreynolds I may need your help for logconv.pl :S I can't seem to work out where the line for notes=U/A/F is matched in this :( my perl skills are basically non-existant. Where should I look in the file?

Okay, and a follow up - looking at result.c, I can see the note2str and notemap, but the mechanism for injection of extra messages to the access log is not obvious. Is it perhaps the PB_RESULT_TEXT you mean? I think that also may not really be correct to use here ....

Okay, and a follow up - looking at result.c, I can see the note2str and notemap, but the mechanism for injection of extra messages to the access log is not obvious. Is it perhaps the PB_RESULT_TEXT you mean? I think that also may not really be correct to use here ....

What I was thinking of is what we do in bind.c --> log_bind_access() So I guess it's not "built" into the logging framework.

@mreynolds Ahhh okay! I see. So in that case, something like

if notes & SLAPI_OP_NOTE_FILTER_INVALID 
{ log_bind_access(" .... your filter in invalid to schema"); }

Is this what you had in mind?

@mreynolds Ahhh okay! I see. So in that case, something like
if notes & SLAPI_OP_NOTE_FILTER_INVALID
{ log_bind_access(" .... your filter in invalid to schema"); }

Is this what you had in mind?

Actually something like:

{ log_bind_access("... notes=F - filter attribute (marksInvalidAttribute) is not in the schema"); }

Not sure how easy it would be to pass that information, but it would be nice :-)

Well, in that case we could just change the notemap to have "F" -> "F - filter attribute not found"

I think it may be hard to pass the filterattribute that wasn't found to this, because they could be 1 to N so perhaps thats a V2? Or we wait for the richer logging to be available and then log which attributes are invalid inside of the filter processing/application to idl? I'm just worried it could be a bit hacky to try and get the filter elements that are invalid here, but I also could be overthinking it ... your intent to provide more clarify about what is invalid is an awesome idea of course!

Actually something like:
{ log_bind_access("... notes=F - filter attribute (marksInvalidAttribute) is not in the schema"); }

And/or state that the filter component is ignore?

Would you accept loging to the access log in the filter processing/validation step and having a unique log line to declare which elements will be ignored, rather than in the results line?

Well, in that case we could just change the notemap to have "F" -> "F - filter attribute not found"
I think it may be hard to pass the filterattribute that wasn't found to this, because they could be 1 to N so perhaps thats a V2? Or we wait for the richer logging to be available and then log which attributes are invalid inside of the filter processing/application to idl?

Yeah makes sense to do it in the new format. Otherwise you would have to use thread storage or like you said something hacky to pass that info along.

@mreynolds I may need your help for logconv.pl :S I can't seem to work out where the line for notes=U/A/F is matched in this :( my perl skills are basically non-existant. Where should I look in the file?

Its there, and it won't be trivial to add. Just look for:

    if (m/ notes=[A-Z,]*A/){

You would need matching code for:

    if (m/ notes=[A-Z,]*F/){

It would require adding new database hashes, and other little things. If you want I can do this for you once we get the logging format agreed upon.

Well, at that point we have the pblock, so we can access the filter, and that has the flags associated to the filter elements? So we could do something like "filter2undefined(slapi_pblock pb, *char buf) {}" and then have it such that:

if (notes & SLAPI_OF_NOTES_FILTER) {
    char *buf = NULL;
    filter2undefined(pb, &buf);
    log_access("undefined by schema -> %s", buf)
    slapi_free(&buf);
}

But I think still that I would log the "undefined" elements into a seperate log line, rather than reusing the results line? So if we would use the seperate log line, we could easily just have inside of filter.c as we process the filter, calls to slapi_log_access to say "warning, op/conn this filter has undefined elements".

Thoughts?

I think I would appreciate the help with logconv, but as you say, lets get the format sorted. :)

What would the new access log lines look like? I think it might be best to add a new access log level to write these fine grained details. So have a more generic message for "notes=F - filter has invalid schema", then we get to the details with the new log level. Thoughts?

I'd say "notes=FilterContainsUndefined", and then the new log level with the details about it?

@tbordaz would like to keep this as notes=F, so maybe instead we can extend notesmap to have a third field like details, and then we can just put those into the buffer as well?

Something like "notes=F,U details="Filter Contains Undefined, Unindexed""

@firstyear, yes I like the idea of details field.

[04/Jul/2019:12:04:58.255243133 +1000] conn=1 op=12 RESULT err=0 tag=101 nentries=13 etime=0.0002102208 notes=U details="Partially Unindexed Filter"
[04/Jul/2019:12:05:06.783574304 +1000] conn=1 op=6 RESULT err=0 tag=101 nentries=0 etime=0.0001999146 notes=F details="Filter Element Missing From Schema"

Here is an example of what it looks like. I've pushed the changes to the branch too.

details are looking good.
If you manage to add detail msg for notes=U would it be possible to also add it to notes=A "Fully Unindexed Filter"

details are looking good.
If you manage to add detail msg for notes=U would it be possible to also add it to notes=A "Fully Unindexed Filter"

https://pagure.io/389-ds-base/pull-request/50379#_16__12

See this section of the diff :)

@tbordaz Any other comments? I did put in the notes= and details as requested :)

@firstyear, no other comment on my side Thanks. You have my ACK.

Thanks @tbordaz for your great help and reviews on this, and the very thoughtful and helpful discussions! I really appreciate it.

There is a regression, even with the default "warn" we hit this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1759709

The BZ contains an IPA test case and a simple testcase in comment #3
Setting nsslapd-verify-filter-schema: off returns the result

Okay, so we need to fix this to handle tagged attributes then.

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

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.