Re: [PREVIEW] Various ACL fixes around matchAclList

On 31.05.2012 11:59, Alex Rousskov wrote:
> 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.

Thank you. Just done a quick once-over to get my head around the
changes, there is probably more.

in src/acl/Checklist.cc:

  * You removed the currentAnswer(ACCESS_DUNNO) state reset at the start
of parametered fachCheck(). This allows the answer from one check to
potentially bleed over into following checks.

The parameterized and non-parameterised fastCheck() need to be kept in
sync semantically. The only difference being that one uses the Checklist
aclList and its result value, and the other uses the parameter and a
hard-coded ACCESS_ALLOW result value on full match.

To set the final states cleanly and get some useful debugging around
these tests, I think it should be:

  allow_t const &
  ACLChecklist::fastCheck(const ACLList * list)
  {
      PROF_start(aclCheckFast);
+ // assume DENY/ALLOW on mis/matches due to not having acl_access
object
+ if (matchAclList(list, true))
+ markFinished(ACCESS_ALLOWED);
+ else
+ calcImplicitAnswer(ACCESS_ALLOWED);
      PROF_stop(aclCheckFast);
      return currentAnswer();
  }

in src/external_acl.cc
* looks to be missing cbdataReferenceDone(ch->extacl_entry); when
before the new entry=NULL

* comment "no need to piggiback to a pending request if that request is
sufficient" would be better written as:
    "background requests have no need to piggiback on a pending request"

in src/ident/AclIdent.cc:
* IdentLookup::checkForAsync local variable ConnStateData *conn should
probably be a const.

* The new assert() seems like not such a good idea. Perhapse a "BUG"
debugs() message and broken-lookup result instead of killing Squid.

Amos
Received on Thu May 31 2012 - 03:19:36 MDT