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.
This archive was generated by hypermail 2.2.0 : Thu May 31 2012 - 12:00:11 MDT