Re: DUNNO state and implicit ACLs

On 18/05/2012 11:34 a.m., Alex Rousskov wrote:
> Hello,
>
> I am afraid ACLs are still broken in trunk. The problems appear to
> go deeper than the recently reverted extended ACCESS_AUTH states. Here
> is my current understanding and a sketch of a partial(?) fix.
>
> Squid trunk r11562 introduced ACCESS_DUNNO result (a good idea on its
> own) but broke fastCheck(). When there was no match, that function
> returned last line action (ALLOW or DENY) instead of its inversion.
>
> Squid trunk r11562 attempted to fix the above bug, but did it in a
> strange way. The change log seems to claim that the DUNNO state is now
> the replacement for the implicit ACL rules that invert the last seen
> keyword. Such a replacement is impossible because a single state,
> whatever it is, cannot be a replacement for the "opposite of the last
> state checked" value.
>
>
> 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.

>
>
> BTW, is finished() equivalent to "the rule matched"?

Yes. It means entire line matched.

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

Amos
Received on Fri May 18 2012 - 02:21:03 MDT