Re: [PATCH] Honor the "deny" part of "foobar deny ACL" options

On 03/12/2012 09:16 AM, Tsantilas Christos wrote:

> I believe the patch is in the right direction but is not correct.
>
> Just removing the checklist->currentAnswer(asnwer) from the *::match
> methods will has as result the ACCESS_AUTH_EXPIRED_OK and
> ACCESS_AUTH_EXPIRED_BAD never returned as answers (The
> ACCESS_AUTH_REQUIRED returned because it is set as currentAnswer inside
> ProxyAuthNeeded::checkForAsync method).

I agree that ACCESS_AUTH_EXPIRED* will not be returned to main Squid as
answers after this patch. And I understand the desire to return more
information to main Squid than just ACCESS_ALLOWED/DENIED. However,
calling currentAnswer() is not how such additional information should be
returned, IMHO. I do not think currentAnswer() users are currently
capable of coping with such additional information, for two primary
reasons discussed below.

> But there is code inside squid which check for such answers eg the
> ClientRequestContext::maybeSendAuthChallenge method.

Agreed, although maybeSendAuthChallenge() does not distinguish
ACCESS_AUTH_EXPIRED_OK from ACCESS_ALLOWED; ACCESS_AUTH_EXPIRED_BAD from
ACCESS_AUTH_REQUIRED; and ACCESS_DUNNO from ACCESS_DENIED.

Curiously, ACLMaxUserIP::match(), ACLProxyAuth::match(), and
ACLExternal::match() also pair those ACCESS_* answers but use different
pairings! And peerCheckNeverDirectDone() groups these answers in its own
unique way.

To make matter even worse, while most of Squid code compares the ACL
match result with ACCESS_ALLOWED (and treating everything else as a
denial case), there is code that compares with ACCESS_DENIED (and
treating everything else as allowed case). See FwdState::fwdStart() and
tunnelStart() for examples of the latter.

What a mess!

> I believe instead of removing the checklist->currentAnswer() lines,
> using a code like the following will be better:
>
> if (answer != ACCESS_DENIED && answer != ACCESS_ALLOWED) {
> checklist->currentAnswer(answer);
> }

I think this will fix some problems but also expose other problems due
to how answers are used by current callers. In other words, there is no
simple fix. Below, you have found at least one problem that the above
adjustment may not address, but I think there are more problems.

> But in any case looks that it is better to change currentAnswer only
> inside Checklist (make currentAnswer(allow_t) private) and make new
> methods and members to report to Checklist that authentication required...
>
> An other problem I am seeing is that the ACLProxyAuth::match returns
> tri-state value but the callers are checking for 1 and 0.
> Inside ACLList::matches (ACLChecklist *checklist) const method:
>
> if (_acl->checklistMatches(checklist) != op) {
> debugs(28, 4, "ACLList::matches: result is false");
> return checklist->lastACLResult(false);
> }
>
> But the op is 0 or 1 (negate or not negate the ACLlist).

Right, adding more to the mess discussed above.

I will propose a way to resolve this in a separate message because it
goes beyond adjusting this particular patch.

Thank you,

Alex.
Received on Mon Mar 12 2012 - 16:53:59 MDT