Re: DUNNO state and implicit ACLs

On 19/05/2012 11:35 a.m., Alex Rousskov wrote:
> 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).

Good examples. Line matched and currentAnswer() is set to whatever the
line state is.

The proxy_auth finished() is line finished and credentials needed. I
agree the fastCheck() handling of AUTH_REQUIRED as being the line match
is a bit wrong there.

I'm not sure why IDENT is setting currentAnswer() instead of just
failing the match. That would seem to be a bug. I think it should be
setting the match to fail, not the line to deny.

>>> 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.

Okay good.

>
> 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.

fastCheck() does so on return from matchAclList() inside "if
(finished()) ...". Ignoring the proxy_auth special state and simply
assuming the line state is probably the main bug here.

Amos
Received on Sun May 20 2012 - 09:25:02 MDT