[PREVIEW] Various ACL fixes around matchAclList

From: Alex Rousskov <rousskov_at_measurement-factory.com>
Date: Wed, 30 May 2012 17:59:54 -0600

Hello,

    Attached is the fourth version of the work-in-progress patch that
attempts to fix most of the bugs discussed in recent ACL-related
squid-dev threads ("DUNNO state and implicit ACLs" and "Handle ACLs that
are neither denied nor allowed") and other ACL bugs.

This patch did pass a few tests but I would like to give it more testing
before proposing to merge these changes into trunk. I do not expect to
make more changes (other than synchronization with the current trunk)
unless testing exposes new bugs.

Comments welcome!

Done:

 * Removed ACLChecklist::lastACLResult(). It was doing nothing but
   duplicating nodeMatched value as far as I could tell.

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

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

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

TODO:

 * Rename currentAnswer() to finalAnswer(). We probably never change the
   "current" answer any more.

 * More testing.

 * Trunk port.

Cheers,

Alex.

Received on Wed May 30 2012 - 23:59:58 MDT

This archive was generated by hypermail 2.2.0 : Thu May 31 2012 - 12:00:11 MDT