[PATCH] Various ACL fixes around matchAclList

On 05/31/2012 12:20 PM, Alex Rousskov wrote:

> Attached is take5 of the patch with all the changes promised so far.
> AFAICT, there are at lest two areas were we may not be in agreement:
>
> 1) Whether current state initialization before each check is sufficient.
> I hope take5 changes and the above comments will convince you that it is.
>
> 2) Whether parameterized fastCheck() declaration comments are correct.

Hello,

    The attached patch is for trunk. It should be functionally identical
to the last [PREVIEW] patch posted earlier (which received a little more
testing since then). Thus, the above comments from our conversation with
Amos still apply.

Please review for trunk inclusion.

Thank you,

Alex.
----------- proposed commit message --------------
> Fix several ACL-related bugs including broken default rules and ACCESS_DUNNO.
>
> For example:
>
> # broken when "goodGuys" matches (denies good guys)
> acl_driven_option deny !goodGuys
>
> and
>
> # broken if badGuys fails to match or mismatch (allows bad guys)
> acl_driven_option allow !badGuys
>
> Fixing the above resulted in significant changes (and more fixes!)
> detailed below.
>
> * Revised ACLChecklist::fastCheck() and nonBlockingCheck() APIs to
> clarify all possible outcomes and to specify that exceptional ACL
> check outcomes (not ALLOW or DENIED) are not ignored/skipped but
> result in the same exceptional final answer. I believe this is the
> right behavior even if it is going to break some [already broken
> IMHO] existing configurations. Skipping failed ACLs is insecure and
> may lead to confusing results.
>
> * Correctly handle cases where no rules were matched and, hence, the
> keyword/action of the last seen rule (if any) has to be "reversed".
>
> * Do not ignore non-allow/deny outcomes of rules in fastCheck().
>
> * Move away from setting the "default" (and usually wrong) "current"
> answer and then sometimes adjusting it. Set the answer only when we
> know what it is. This is done using markFinished() call which now
> accepts the [final] answer value and debugging reason for selecting
> that answer.
>
> * Streamline and better document ACLChecklist::matchAclList()
> interface. Use it in a more consistent fashion.
>
> * Rewrote ACLChecklist::matchAclList() implementation when it comes to
> handling ACLList::matches() outcomes. Better document and restrict
> supported outcomes. Assert on unsupported outcomes (for now).
>
> * Removed ACLChecklist::lastACLResult(). It was doing nothing but
> duplicating nodeMatched value as far as I could tell.
>
> * Removed ProxyAuthNeeded class. It is an async state that does not
> perform async operations and, hence, is not needed.
>
> * Move IdentLookup::checkForAsync() connection check into
> ACLIdent::match() to avoid creating an async state that is not
> needed.
>
> * Polished aclMatchExternal() and greatly simplified
> ACLExternal::ExternalAclLookup() to avoid creating async state under
> non-async conditions, to avoid checking for the same conditions
> twice, to fix wrong debugging messages, and to simplify (and possibly
> fix) the overall algorithm.
>
> The aclMatchExternal() call now checks most of the corner cases,
> discards stale cached entries, and schedules either a background
> cache update or a regular external lookup as needed.
>
> ACLExternal::ExternalAclLookup() code is now
> ExternalACLLookup::Start(). It initiates an external lookup. It does
> not deal with the cached entry at all. It relies on
> aclMatchExternal() to check various preconditions.
>
> Some of the old code made little sense to me, and this is the biggest
> ACL-specific change in this project, with the highest probability of
> new bugs or unintended side-effects.
>
> My goal here was to prevent aclMatchExternal() from creating an async
> state where none was needed because new ACLChecklist::matchAclList()
> code prohibited such self-contradictory outcomes. However, I later
> discovered that it is not possible to prohibit them without rewriting
> how Squid DNS cache lookups are working -- ipcache_nbgethostbyname()
> and similar code may call back immediately if the item is in the
> cache. Since I did not want to rewrite DNS code as well, I ended up
> relaxing the ACLChecklist::matchAclList() code requirements, going a
> step back to where we sometimes call ACLList::matches() twice for the
> same ACL node.
>
> Thus, it is probably possible to undo most of the external_acl.cc
> changes. I left them in because I think they improve the quality of
> the code and possibly fix a bug or two.
>
> * Adjusted ACLMaxUserIP::match(), ACLProxyAuth::match(), and
> ACLExternal::match() to explicitly stop ACL processing when an
> exceptional state is discovered instead of just setting the current
> answer and proceeding as if more ACLs could be checked. On the other
> hand, we now do not set the answer if the corresponding internal
> matching code (e.g., AuthenticateAcl()) needs an async operation
> because we do not know the answer yet.
>
> * Fixed HttpStateData::handle1xx() and
> HttpStateData::finishingBrokenPost() to correctly handle
> fastCheck(void) return values. They were assuming that there are only
> two possible return values (ACCESS_DENIED/ALLOWED), potentially
> subjecting more messages to invasive adaptations than necessary.
>
> TODO:
>
> * Rename currentAnswer() to finalAnswer(). We probably never change the
> "current" answer any more.