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

On 13.03.2012 05:53, Alex Rousskov wrote:
> 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!

Don't panic. It is intentional.

The comparisons of the cases right out at the top level of callers is
relative to the semantics of the *_access list being tested. For example
miss_access in fwdStart only matters if outright denial happens, so only
tests for that. Similar for always_direct/never_direct, but with some
"silent" auth handling semantics on top for them since there are async.
Which are different from the auth handling semantics of http_access
which needs to challenge on expired results (non-"silent").

The major disjunct is that the match() function is still presenting
tri-state and being treated as boolean. Which is the old code. It still
maps to AUTH_REQUIRED in -1 cases which are ambiguous to retain the old
behaviour for those, sometimes 'incorrectly' going by the new expired
handling. But the re-write for that is big and out of scope for this.

Amos
Received on Tue Mar 13 2012 - 03:36:42 MDT