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

On 20/03/2012 6:10 p.m., Alex Rousskov wrote:
> 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).
>

I'm open to dropping the EXPIRED states temporarily until we can roll
the non-int API through the whole access system. Then re-aligning it to
include them as separate projects.

Although, that is for the overview problem. I have a patch which I'm
trying to test right now for the immediate issue of auth in ssl_bump.

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

The admin whose bug report triggered this all was using a config
equivalent to:

    http_access allow localnet
    ssl_bump allow login
    ssl_bump deny all

Do you think it reasonable to expect the LAN can do HTTP anywhere but
only get bumped if the user is logged in?

Additionally we document all over the place that "slow" group ACLs can
do helper lookups (including auth). ssl_bump was made a slow group
access list.

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

We need to propigate the auth failure for places where any failure is a
deny.
We also need to propigate the expired/stale/missing auth info around for
the Digest authentication headers.

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

I'm aware of need or requests for http_access, adapted_http_access
http_reply_access, ssl_bump, url_rewrite_access, log_access,
cache_peer_access, always_direct, never_direct, and miss_access to
handle auth in ways other than allow/deny based on the credentials being
existing+current+valid.

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

You are thinking of the access_log requests, where expired-but-known
credentials were handled as AUTH_REQUIRED in a fast-access check right?
all Squid suffer from that, no change through any of these alterations.

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

Amos
Received on Tue Mar 20 2012 - 11:24:24 MDT