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

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

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

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);
 }

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

Regards,
   Christos

On 03/09/2012 10:21 PM, Alex Rousskov wrote:
> Hello,
>
> Am I going crazy here? While working on the bump-ssl-server-first
> project, we noticed that authentication does not seem to work right.
> Squid debugging shows that a denied user is authenticated but Squid
> allows access anyway. The attached patch is what I came up with. Please
> review as I am not an ACL expert, and it seems strange to me that such a
> big bug would remain unnoticed for so long!
>
> Technical/commit details from the patch preamble:
>
> When AuthenticateAcl() and aclMatchExternal() were converted to use
> extended authentication ACL states (r11644 and r11645 dated 2011-08-14),
> the result of those function calls was set as the current checklist
> answer. This was incorrect because those functions do not make
> allow/deny decisions. They only tell us whether the ACL part of the
> allow/deny rule matches. If there is a match, the
> ACCESS_ALLOWED/ACCESS_DENIED answer depends on whether it is an allow or
> deny rule.
>
> For example, "http_access deny BadGuys" should deny access when the
> BadGuys ACL matches, but it was allowing access instead.
>
>
> Thank you,
>
> Alex.
Received on Mon Mar 12 2012 - 15:16:32 MDT