]> git.ipfire.org Git - thirdparty/squid.git/commit
Fixed several ACL-related bugs, including:
authorAlex Rousskov <rousskov@measurement-factory.com>
Fri, 1 Jun 2012 22:01:43 +0000 (16:01 -0600)
committerAlex Rousskov <rousskov@measurement-factory.com>
Fri, 1 Jun 2012 22:01:43 +0000 (16:01 -0600)
commitdcb1e4bbf8c0f051401d00ac7b6bc671c4274e3d
tree15b2f026363c4f2fe4f8c4b664dd47b46ae8bf0c
parent68715527ffbb6609937c0c03f37ae901628b38cd
Fixed several ACL-related bugs, including:

    # 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.
src/ExternalACL.h
src/acl/Acl.cc
src/acl/Checklist.cc
src/acl/Checklist.h
src/auth/Acl.cc
src/auth/AclMaxUserIp.cc
src/auth/AclProxyAuth.cc
src/auth/AclProxyAuth.h
src/external_acl.cc
src/http.cc
src/ident/AclIdent.cc