Re: DUNNO state and implicit ACLs

On 05/17/2012 08:20 PM, Amos Jeffries wrote:
> On 18/05/2012 11:34 a.m., Alex Rousskov wrote:
>> Currently, fastCheck() does this (the exact version is quoted at the end
>> of this email):
>>
>> allow_t const&
>> ACLChecklist::fastCheck()
>> {
>> for each ACL rule {
>> if (rule matches)
>> return the rule's keyword (either ALLOW or DENY)
>> }
>> return DUNNO;
>> }
>>
>>
>> The above logic breaks simple rules like
>>
>> foo_access deny !some
>>
>> because they will result in DUNNO outcome for some transactions while
>> they should result in ALLOW outcome for those transactions (because of
>> the implicit reversal of the explicit deny rule that did not match).
>>
>> Since code often treats DUNNO as "no match", things break badly.
>>
>> My understanding is that fastCheck() should be written like this instead:
>>
>>
>> allow_t const&
>> ACLChecklist::fastCheck()
>> {
>> last_keyword_seen = DUNNO
>> for each ACL rule {
>> last_keyword_seen = rule's keyword (either ALLOW or DENY)
>> if (rule matches)
>> return the rule's keyword (either ALLOW or DENY)
>> }
>>
>> // cannot reverse if there were no rules (should not happen?)
>> if last_keyword_seen is DUNNO
>> return DUNNO
>>
>> // reverse the last explicit rule's keyword
>> if last_keyword_seen is DENY
>> return ALLOW
>> else
>> return DENY
>> }
>>
>> Am I on the right track?

> I don't think so. BUt dont have time to debate right now. Will try to
> find time in a few days.
>
> The cases to consider are fastCheck() matches where there is no
> last_keyword_seen.

Hi Amos,

    Parameterless fastCheck() code makes it impossible for a rule to
match without seeing a keyword so I do not understand what cases you are
concerned about.

The proposed code above covers the following cases:

0. No rules: DUNNO answer (this may not be possible, not sure).
1. No matching rules: Negate the keyword of the last rule.
2. A matching rule: Use that rule's keyword.

where "matching" means finished(). Unfortunately, I suspect that the
meaning of finished() is not exactly "matching" (see below).

It is not clear to me what fastCheck() should return when it is applied
to [async] ACLs that can "finish" with a different answer than the rule
keyword. The current fastCheck() code simply ignores such answers. For
example, if you apply fastCheck() to an "allow" rule with an IdentLookup
ACL, the possible ACCESS_DENIED answer produced by that ACL will be
ignored by fastCheck(), and an ACCESS_ALLOWED result will be returned to
the fastCheck() caller. The fix for _that_ problem is discussed below.

>> BTW, is finished() equivalent to "the rule matched"?
>
> Yes. It means entire line matched.

I do not think that is always true because
ProxyAuthNeeded::checkForAsync() and IdentLookup::checkForAsync() may
call markFinished(). The checkForAsync() method is called when the rule
did NOT match (and in some other weird cases too, but those cases do not
matter here).

>> The current
>> ACLChecklist::fastCheck() code appears to treat it that way, but I am
>> thinking that it is possible that a rule did not match but finish()
>> became true because some ACL match check resulted in a DUNNO (and so we
>> cannot continue checking other rules and have to finish). Or is that
>> currently impossible because ACL matches always return either yes or no
>> answer?
>
> Yes maybe async matches can result in DUNNO and set finish(). I'm not
> sure there.

I have not found such cases yet. The checkForAsync() methods mentioned
above set the current answer to ACCESS_AUTH_REQUIRED and ACCESS_DENIED
(which is then ignored by fastCheck() as discussed above). There may be
other cases I have missed, of course.

My current understanding is that finished() means that the ACL
processing must stop or has been stopped. The
ACLChecklist::matchAclList() code does not set the currentAnswer() when
it calls markFinished() itself so the matchAclList() callers must set
the current answer to the rule keyword. fastCheck() does not do that,
which is another bug.

Cheers,

Alex.
Received on Fri May 18 2012 - 23:36:12 MDT