]> git.ipfire.org Git - thirdparty/squid.git/commit - src/http.cc
Fix several ACL-related bugs including broken default rules and ACCESS_DUNNO.
authorAlex Rousskov <rousskov@measurement-factory.com>
Sat, 16 Jun 2012 15:03:46 +0000 (09:03 -0600)
committerAlex Rousskov <rousskov@measurement-factory.com>
Sat, 16 Jun 2012 15:03:46 +0000 (09:03 -0600)
commite0f7153c051220e63a512fb4e20271e505ec2c52
treed562fae95dabe0c4a918bf586de1f29ff1cda5a4
parentccfc0ee9c700c6d7126ae4863eccb8f3e4ff0ecc
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.
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