Re: DUNNO state and implicit ACLs

On 05/24/2012 04:30 AM, Amos Jeffries wrote:
> 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.

IMHO, if badGuys ACL cannot tell whether the client is legit,
http_access check should return DUNNO. No other return value would be
correct in all cases. If badGuys relies on working IDENT but the admin
wants to allow client with broken IDENT, a different set of access rules
should be used (we may need to add support for that special use case).

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

If DUNNO result is returned, then Squid should not emit the error
message you are quoting above because FOO did not match badGuys.
Instead, the error message should be something like "The request for FOO
is denied because access credentials could not be obtained or verified".

Which error message is actually emitted when an ACL check ends with
DUNNO is a separate question. There could be more bugs or missing
features in that code, of course. However, those bugs do not justify
returning the wrong result from the check (and both ALLOW and DENY would
be wrong in this case), IMO.

> 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?

Again, if IDENT is broken and badGuys relies on IDENT, FOO will not
match badGuys after the changes I propose. FOO will not mismatch badGuys
either. Instead, access checking will stop with a "cannot determine
result" error. The caller will need to treat that error (the DUNNO
answer) appropriately, of course. Most callers already do.

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

I disagree that ignoring the access check rule is the best action: In
some configurations, skipping a rule allows bad guys in when things do
not work as expected. This is exactly what the bad guys want us to do
so, if nothing else, we are just giving them an additional incentive to
break things so that they can get in.

Frankly, I am surprised this is something we have to argue about. Should
not access controls always err on the side of being
conservative/restrictive rather than permissive? If yes, only DUNNO
answer is appropriate here (and no rule skipping).

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

IMO, DUNNO during rule checking should equate to stopping all checks and
returning the "I cannot tell whether this request matches or mismatches
the configured rules" error to the caller.

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

That is possible, but I am discussing the case where IDENT (or any other
ACL) is broken -- it cannot give a match/mismatch answer for some
reason, including internal Squid bugs, external script bugs, etc.

> 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 think we agree on this point.

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

I am not sure this is something we can fix unless we add an initially
_required_ option to specify whether Squid should use implicit rules.
Changing keywords would also work to detect old configs, but "allow" and
"deny" are difficult to rename in some contexts.

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

Sounds good to me. I already posted a preview patch that implements most
of the fixes. AFAIK, these bugs are specific to v3.2/trunk so no
backporting of these changes to v3.1 will be needed.

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

Yes, but that is a different issue, I think. Currently, the fastCheck or
"slowCheck" caller is responsible for determining what to do when there
are no rules at all (the caller code should not call fast/slowCheck in
those cases). I think we would have to keep it that way because
fast/slowCheck implementation cannot make that determination on its own.
We could move the _detection_ of that condition into fast/slowCheck
itself, but in the slowCheck case it would have a noticeable affect on
performance, so I would not do it.

Thank you,

Alex.
Received on Thu May 24 2012 - 18:48:01 MDT