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

On 13.03.2012 04:16, 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).
>
> 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
>

I'm thinking split that ClientRequestContext::maybeSendAuthChallenge in
half. Making it call the second stage to delivery the error page only if
auth is required. the else condition of that if statement can just
return false, not set any error at all. The handlers which call
maybeSendAuthChallenge can call the second stage if they need to display
an error on other states.

Yes the deeper level match() functions and their handlers need to be
converted to the enum instead of the tri-state 'boolean'. That was
supposed to be the next stage for 3.3 since it will take a lot more
change and testing.

Amos

>
> 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 - 21:28:56 MDT