DUNNO state and implicit ACLs

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?

BTW, is finished() equivalent to "the rule 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?

Thank you,

Alex.

> allow_t const &
> ACLChecklist::fastCheck()
> {
> PROF_start(aclCheckFast);
> currentAnswer(ACCESS_DUNNO);
>
> debugs(28, 5, "aclCheckFast: list: " << accessList);
> const acl_access *acl = cbdataReference(accessList);
> while (acl != NULL && cbdataReferenceValid(acl)) {
> matchAclList(acl->aclList, true);
> if (finished()) {
> currentAnswer(acl->allow);
> PROF_stop(aclCheckFast);
> cbdataReferenceDone(acl);
> return currentAnswer();
> }
>
> /*
> * Reference the next access entry
> */
> const acl_access *A = acl;
> acl = cbdataReference(acl->next);
> cbdataReferenceDone(A);
> }
>
> debugs(28, 5, "aclCheckFast: no matches, returning: " << currentAnswer());
> PROF_stop(aclCheckFast);
>
> return currentAnswer();
> }
Received on Thu May 17 2012 - 23:35:21 MDT