Re: [RFC] Handle ACLs that are neither denied nor allowed

On 03/13/2012 01:11 AM, Amos Jeffries wrote:
> On 13/03/2012 6:40 a.m., Alex Rousskov wrote:
>> Hello,
>>
>> This started as the
>>
>> [PATCH] Honor the "deny" part of "foobar deny ACL" options
>>
>> email thread but it looks like more fundamental changes are required
>> than my insufficient fix posted there. I think we need to step back for
>> a minute before adding more hacks to this messy code.
>>
>> AFAICT, all main Squid code uses of ACLs are essentially dealing with
>> the following kind of squid.conf option (which could be expressed
>> differently but boils down to the same overall design):
>>
>> foobar allow/deny ACL
>>
>> There are three ACL processing outcomes that main Squid code deals with:
>>
>> ACCESS_ALLOWED: An allow match with no caveats.
>> ACCESS_DENIED: A deny match with no caveats.
>> ACCESS_DUNNO: Insufficient information or an error of some kind. The
>> code must consult some other member of the ACL check result to figure
>> out what happened. It could be that the user has not been authenticated
>> yet, that an external ACL helper has failed, that Squid failed to supply
>> an HttpRequest structure, etc.
>
> There is three levels to the system (in this order of evaluation and
> result inheritance):
> - ACL results
> - line results
> - checklist results
>
> Do you mean line results or checklist results? you have said "main
> code", which would be checklist results, but described only the three
> states processed at line results level.

Main Squid code is what you call "checklist results", I think. It is
code that does not know anything about ACL internals. It just needs a
yes/no/error answer.

I suspect ACL line processing needs are not that different from the main
code though: match/no-match/error.

> checklist results has always included AUTH_REQUIRED. The EXPIRED are new
> and mostly non-existent, so if you want to get to them later fine.

If the checklist results has always included AUTH_REQUIRED, then I bet
that some of the main code did not process it correctly at some point of
time or another. It is very easy to make a mistake of writing "result !=
denied" and think that is the same as writing "result == allowed".

The changes outlined about are meant to make such mistakes less likely,
whether there is just AUTH_REQUIRED or ten more future special states to
worry about. I am not sure these changes are the right ones (as
discussed below) but I am sure some changes are needed here.

> ACL-results produces tri-state, line results level maps that to a
> quad-state for handling at checklist results. See below...

>> ACCESS_ALLOWED and ACCESS_DENIED cases are clear. We probably handled
>> them more-or-less correctly before the code was broken by the changes
>> my patch was trying to fix.
>>
>> The handling of ACCESS_DUNNO case depends on the caller:
>>
>> * Low-level callers such as ACL combining code: All processing should
>> stop. We should set the "additional information" field to some code or
>> more detailed information to explain the ultimate caller what went
>> wrong. The final result must be ACCESS_DUNNO regardless of whether the
>> foobar rule had used an allow or deny keyword. [ If there are callers
>> that need to know which keyword was used then the lower level code would
>> have to be made smarter to bypass some DUNNO results instead
>> of stopping at the first DUNNO result ]
>
> Note: In ACL level DUNNO=-1 state, ALLOWED=1, DENIED=0. That level does
> not (yet) use the new enums, these are int return values from match().

If the above proposed higher level changes are implemented, I would
argue that ACL level code should be converted to use a similar approach
at the same time. The primary difference is that there is no allow/deny
at the lower level. There is match/no-match. The third
"other/error/dunno" case is common to all levels, of course.

> This is where your description overlooks the line-result handlers (one
> in fastCheck loop, one in nonBlockingCheck callback).

I just wanted to get the high-level stuff right first (on paper). We can
discuss lower levels later, I think. Both should probably be changed at
the same time though.

> The line-results level takes these int tri-state values and maps to a
> N-state output:
> ACCESS_ALLOWED (1 and "allow" in config file)
> ACCESS_DENIED (1 and "deny" in config file)
> ACCESS_AUTH_REQUIRED (0 with last ACL having authMatch flag set, -1
> with last checked ACL requiresAuth() being true)
> ACCESS_DUNNO (-1 if no special ACL flags)
>
> There is a mess in external ACL and proxy_auth match() which take the 5
> auth states output (match, no-match, challenge needed, expired, expired
> and bad), then maps them to the tri-state to be re-mapped by the above.
> All other ACLs produce just the tri-state.
>
> Note how the other new auth states are not mapped to yet by the line
> result handlers.

As far as I can tell from the bug reports, the current and recent
handling of authentication is not very good when it comes to "not
allowed and not denied" cases. I suspect that the above mapping and
flagging mess is at least partially to blame that Squid ACLs do not do
what reasonable users want/need.

Again, I am not an ACL expert, but just the above description alone
tells me that the current design is asking for trouble. Adding even more
authentication states to that mess is unlikely to improve the situation.
What I propose may not be sufficient and may be flat wrong, but I hope
this discussion leads us to a more sane state than the current ACL code
(which is not only insane but buggy or I would not be talking about it).

>> * High-level callers in main Squid code: Each such caller code must deal
>> with ACCESS_DUNNO depending on its internal logic. For example,
>> http_access may sometimes trigger authentication while ssl_bump will
>> just not bump connections. All such code should probably log a warning
>> if some DUNNO reason is not handled explicitly as it would indicate a
>> possible Squid misconfiguration or coding bug.
>
> At this checklist result level things are simpler:
> - ALLOWED - one "allow" line matched.
> - DENIED - one "deny" line matched.
> - DUNNO - the entire list did not produce any matching line.
> - AUTH_REQUIRED - testing halted on a line needing auth.
> - AUTH_EXPIRED_OK - testing halted on a line needing auth, known
> credentials are okay.
> - AUTH_EXPIRED_BAD - testing halted on a line needing auth, known
> credentials are bad.

It is wrong to force code like ssl_bump to figure out what to do with a
result like AUTH_EXPIRED_OK. Bumping code does not and should not know
anything about authentication. If we must propagate AUTH_EXPIRED_OK as a
top-level result to the ssl_bump code, we are doing something wrong.

We either need a way to tell low-level ACL code that the checklist
caller is not authentication-aware or we need a general way for an
authentication unaware caller code to handle authentication-specific
failures. I suspect the former is better because users are complaining
that authenticated clients are not matching ACLs that are checked later
(like ssl_bump although there are better examples) because their
credentials have just expired. This should not happen, IMHO. If nothing
else, this should not happen because there is no way that "later code"
can trigger client re-authentication.

>> Since the mess with inconsistent ACL check/match result usage has spread
>> around the code already, I suggest that we change allow_t from an enum
>> into a simple structure while moving ACCESS_* constants around a little:
>>
>> /// Possible ACL check outcomes
>> typedef enum {
>> ACCESS_DENIED,
>> ACCESS_ALLOWED,
>> ACCESS_DUNNO, // must consult Acl::Answer::reason
>> } AclResultCode;
>>
>> /// AclResultCode reasons
>> typedef enum {
>> /* general purpose reasons for ACCESS_DENIED / ACCESS_ALLOWED */
>> reasonUnknown, // should not be present in final result
>> reasonAllowMatched, // e.g., accepted valid credentials
>> reasonDenyMatched, // e.g., rejected valid credentials
>>
>> /* authentication ACLs failures */
>> reasonAuthMissing, // Missing Credentials
>> reasonAuthExpiredOk,// Expired now but were accepted
>> reasonAuthExpiredKo // Expired now and were rejected
>> } AclResultCodeReadon;
>>
>>
>> /// intermediate or final ACL check outcome
>> // TODO: rename to Acl::Answer eventually
>> class allow_t {
>> public:
>> allow_t(): code(ACCESS_DUNNO), reason(reasonUnknown) {}
>>
>> /// allow ACL matched; Warning: !allow() does not imply denial
>> bool allowed() const { return code == ACCESS_ALLOWED; }
>>
>> /// allowed or denied with no errors
>> bool allowedOrDenied(bool&allowed) const;
>>
>> ...
>>
>> public:
>> AclResultCode code; ///< ACL matching outcome
>> AclResultReason reason; ///< outcome reason
>> };
>>
>> and go through the code to adjust all callers to handle ACCESS_DUNNO
>> correctly, including the currentAnswer() callers. Most caller/callback
>> profiles will not have to be changed.
>>
>> Is this the best way to get rid of this mess?
>
> This was the original plan. It fell quickly when I discovered that the
> majority of handlers had to do two function lookups in long
> if-elseif-else statements to figure out which permutation of the
> (AclResultCode,AclResultReason) tuplet was being received in each high
> level handler. It was simpler to pass one code for each tuplet and
> switch handle the permutations everywhere.
>
> The mess is in:
> - mapping the 5-state auth meaning into tri-state ACL match (0/1/-1).
> - bad choices of how to handle each states in the top level. A lot of
> this bad choices is the result of keeping existing behaviour (incl.
> problems) as-is for 3.2, except where the semantics had an open bug
> about the behaviour.

Except for the main code that checks http[s]_access rules, what other
main code needs to be authentication-aware? If authentication is an
exception (i.e., the vast majority of checkers do not know or should not
know about it), then we should restructure the code so that
authentication issues are localized to http_access and friends. The
design I proposed may not be the best solution for that, but I would
like to confirm that we agree what the high-level problem is before
adjusting it.

> The patch which triggerd this discussion was one case of new choices
> being bad (choice to use a challenge-or-error semantics on the ssl_bump
> list, which should have been challenge or DENIED).

The latest ssl_bump bug is just the last(?) drop, IMO. There were other
complaints that ACLs which are checked after http_access are failing for
seemingly irrelevant authentication reasons. IIRC, those complaints
prompted AUTH_EXPIRED_OK and other "new" states additions. I think we
need to revisit that step because it made things worse, not better.

Whether this is fixed in v3.2 or v3.3 is less important for now so we
can make that decision later (but we will need some kind of a fix for
v3.2 anyway).

Thank you,

Alex.
Received on Tue Mar 20 2012 - 05:11:05 MDT