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

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.

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.

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

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

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.

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

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

Amos
Received on Tue Mar 13 2012 - 07:11:54 MDT