[RFC] Handle ACLs that are neither denied nor allowed

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.

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 ]

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

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?

Thank you,

Alex.
Received on Mon Mar 12 2012 - 17:41:00 MDT