Re: DUNNO state and implicit ACLs

On 23/05/2012 5:19 a.m., Alex Rousskov wrote:
> On 05/20/2012 03:24 AM, Amos Jeffries wrote:
>
>> I'm not sure why IDENT is setting currentAnswer() instead of just
>> failing the match. That would seem to be a bug. I think it should be
>> setting the match to fail, not the line to deny.
> "Failing the match" (i.e., returning a regular "there was no match"
> answer) is equivalent to deleting the rule from squid.conf for that
> fastCheck() execution. The next rule will be tried. Ignoring that
> "failing" rule is not necessarily what the admin meant, IMHO:
>
> http_access deny badGuys
> http_access allow all

... but the PC was a legit client with broken or disabled IDENT.

   foo_access deny badGuys
   foo_access allow all

    "The request for FOO is denied because is matched badGuys "

... when the client is NOT blacklisted -> ouch, tricky talking to
explain why we do this.

   foo_access allow badGuys
   foo_access deny all

  "The request for FOO is denied because it matched badGuys "

... is broken -> complaints. In what world is it legit to deny a request
based on an explicit whitelist?

Skipping the test and complaining loudly in the log is the best action
here. Trouble is there are cases when quiet is wanted, so complaints
happen all around.

> If badGuys is an IDENT ACL and your "just fail the match" logic is
> implemented, then the above will allow everybody when IDENT is broken.
> Would not that be the wrong outcome from many admins point of view? I
> think the bug is in fastCheck(). It should return DUNNO if a rule cannot
> determine whether there is a match (for any reason). It should not just
> ignore that rule as if there was a mismatch (your proposal) and it
> should not return the rule's keyword (current code).

DUNNO is what equates to skip the line.

IDENT needs a state similar to AUTH_REQUIRED but not triggering auth.

ACL is still a trilean -1/0/+1 output ... so IDENT plays with
currentAnswer() [sets DENIED, not DUNNO] and returns (-1? 0?).

I'm arguing that it doing DENIED is broken. Agreed on DUNNO being what
it should produce.

>
> I do not know whether my interpretation matches what Squid documentation
> promises (and/or most admins expect) in cases where ACLs fail (i.e.,
> they can declare neither a match or mismatch). I could not find this
> topic covered in squid.conf.documented. That is one of the reasons I
> hesitate fixing this. I do not want to create new semantics if there is
> already a consistent rule interpretation semantics.

I don't think its documented well if at all. The inversion is mentinoed
in the FAQ and tutorials, along with years of texts explaining that to
"unbreak" configs every access list should end with an explicit "
allow/deny all" just to be sure the inversion does not do something
unexpected.

These days someone dropping "http_access deny foo" into an obscure
included sub-config can convert the whole http_access permission set to
an open-proxy implicit "allow all". Same goes for any of the other
access line types, and those others are particularly vulnerable to this
since they do not explicitly have an entry in squid.conf default like
"http_access deny all" does.

>
>
> Note that this is a secondary bug discussed in this thread. The tread
> primary focus is fastCheck() incorrect handling of a case where no rules
> finished(). That primary bug is even worse because it is not specific to
> authentication and failure cases.
>
>
> In summary, I believe these test cases are broken now:
>
> # broken for some transactions (i.e., when "some" matches)
> http_access deny !some
>
> and
>
> # broken if badGuys requires authentication or fails
> http_access allow !badGuys
>
>
> And the following test case will be broken if "just declare a mismatch"
> logic is implemented:
>
> # broken if badGuys requires authentication or fails
> http_access deny badGuys
>

Specific to that bug. Okay, I'm happy for it to be fixed back to the
default-implicit-invert behaviour for 3.2 while we argue out the
pros/cons of having a fixed and deterministic policy as the default
foo_access action. see my last paragrah above (this mail) for a major
PRO to the fixed policy change.

Note that we *have* to implement such a default policy anyway for when
no access lines at all are entered in squid.conf. (When there is no
"last" rule to invert.)

Amos
Received on Thu May 24 2012 - 10:31:13 MDT