------------------------------------------------------------ revno: 12176 revision-id: rousskov@measurement-factory.com-20120616150346-fvbvash0et5pevvk parent: squid3@treenet.co.nz-20120616021714-o0oxb07ys7qetsqj fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3560 committer: Alex Rousskov branch nick: trunk timestamp: Sat 2012-06-16 09:03:46 -0600 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: rousskov@measurement-factory.com-20120616150346-\ # fvbvash0et5pevvk # target_branch: http://bzr.squid-cache.org/bzr/squid3/trunk/ # testament_sha1: bc84151a007f48a2b7f45b440831e88919573178 # timestamp: 2012-06-16 15:57:14 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/trunk/ # base_revision_id: squid3@treenet.co.nz-20120616021714-\ # o0oxb07ys7qetsqj # # Begin patch === modified file 'src/ExternalACL.h' --- src/ExternalACL.h 2009-03-08 19:34:36 +0000 +++ src/ExternalACL.h 2012-06-16 15:03:46 +0000 @@ -36,7 +36,8 @@ #include "acl/Checklist.h" -class external_acl; +/** \todo CLEANUP: kill this typedef. */ +typedef struct _external_acl_data external_acl_data; class ExternalACLLookup : public ACLChecklist::AsyncState { @@ -45,14 +46,15 @@ static ExternalACLLookup *Instance(); virtual void checkForAsync(ACLChecklist *)const; + // If possible, starts an asynchronous lookup of an external ACL. + // Otherwise, asserts (or bails if background refresh is requested). + static void Start(ACLChecklist *checklist, external_acl_data *acl, bool bg); + private: static ExternalACLLookup instance_; static void LookupDone(void *data, void *result); }; -/** \todo CLEANUP: kill this typedef. */ -typedef struct _external_acl_data external_acl_data; - #include "acl/Acl.h" class ACLExternal : public ACL @@ -61,7 +63,7 @@ public: MEMPROXY_CLASS(ACLExternal); - static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *, EAH * callback, void *callback_data); + static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *); ACLExternal(char const *); === modified file 'src/acl/Acl.cc' --- src/acl/Acl.cc 2012-04-25 05:29:20 +0000 +++ src/acl/Acl.cc 2012-06-16 15:03:46 +0000 @@ -322,16 +322,22 @@ ACLList::matches (ACLChecklist *checklist) const { assert (_acl); + // XXX: AclMatchedName does not contain a matched ACL name when the acl + // does not match (or contains stale name if no ACLs are checked). In + // either case, we get misleading debugging and possibly incorrect error + // messages. Unfortunately, deny_info's "when none http_access + // lines match" exception essentially requires this mess. + // TODO: Rework by using an acl-free deny_info for the no-match cases? AclMatchedName = _acl->name; debugs(28, 3, "ACLList::matches: checking " << (op ? null_string : "!") << _acl->name); if (_acl->checklistMatches(checklist) != op) { debugs(28, 4, "ACLList::matches: result is false"); - return checklist->lastACLResult(false); + return false; } debugs(28, 4, "ACLList::matches: result is true"); - return checklist->lastACLResult(true); + return true; } === modified file 'src/acl/Checklist.cc' --- src/acl/Checklist.cc 2012-01-20 18:55:04 +0000 +++ src/acl/Checklist.cc 2012-06-16 15:03:46 +0000 @@ -36,29 +36,14 @@ #include "squid-old.h" #include "acl/Checklist.h" -allow_t const & -ACLChecklist::currentAnswer() const -{ - return allow_; -} - -void -ACLChecklist::currentAnswer(allow_t const newAnswer) -{ - allow_ = newAnswer; -} - void ACLChecklist::matchNonBlocking() { if (checking()) return; - /** Deny if no rules present. */ - currentAnswer(ACCESS_DENIED); - if (callerGone()) { - checkCallback(currentAnswer()); + checkCallback(ACCESS_DUNNO); // the answer does not really matter return; } @@ -67,25 +52,25 @@ * We cannot select a sensible default for all callers here. */ if (accessList == NULL) { debugs(28, DBG_CRITICAL, "SECURITY ERROR: ACL " << this << " checked with nothing to match against!!"); - currentAnswer(ACCESS_DENIED); - checkCallback(currentAnswer()); + checkCallback(ACCESS_DUNNO); return; } + allow_t lastSeenKeyword = ACCESS_DUNNO; /* NOTE: This holds a cbdata reference to the current access_list * entry, not the whole list. */ while (accessList != NULL) { /** \par * If the _acl_access is no longer valid (i.e. its been - * freed because of a reconfigure), then bail on this - * access check. For now, return ACCESS_DENIED. + * freed because of a reconfigure), then bail with ACCESS_DUNNO. */ if (!cbdataReferenceValid(accessList)) { cbdataReferenceDone(accessList); debugs(28, 4, "ACLChecklist::check: " << this << " accessList is invalid"); - continue; + checkCallback(ACCESS_DUNNO); + return; } checking (true); @@ -107,6 +92,8 @@ return; } + lastSeenKeyword = accessList->allow; + /* * Reference the next access entry */ @@ -119,11 +106,14 @@ cbdataReferenceDone(A); } - /** If dropped off the end of the list return inversion of last line allow/deny action. */ - debugs(28, 3, HERE << this << " NO match found, returning " << - (currentAnswer() != ACCESS_DENIED ? ACCESS_DENIED : ACCESS_ALLOWED)); + calcImplicitAnswer(lastSeenKeyword); + checkCallback(currentAnswer()); +} - checkCallback(currentAnswer() != ACCESS_DENIED ? ACCESS_DENIED : ACCESS_ALLOWED); +bool +ACLChecklist::asyncNeeded() const +{ + return state_ != NullState::Instance(); } bool @@ -148,28 +138,32 @@ } void -ACLChecklist::markFinished() +ACLChecklist::markFinished(const allow_t &finalAnswer, const char *reason) { assert (!finished() && !asyncInProgress()); finished_ = true; - debugs(28, 3, "ACLChecklist::markFinished: " << this << - " checklist processing finished"); + allow_ = finalAnswer; + debugs(28, 3, HERE << this << " answer " << allow_ << " for " << reason); } +/// Called first (and once) by all checks to initialize their state void -ACLChecklist::preCheck() +ACLChecklist::preCheck(const char *what) { - debugs(28, 3, "ACLChecklist::preCheck: " << this << " checking '" << accessList->cfgline << "'"); - /* what is our result on a match? */ - currentAnswer(accessList->allow); + debugs(28, 3, HERE << this << " checking " << what); + finished_ = false; } void ACLChecklist::checkAccessList() { - preCheck(); + debugs(28, 3, HERE << this << " checking '" << accessList->cfgline << "'"); /* does the current AND clause match */ - matchAclList(accessList->aclList, false); + if (matchAclList(accessList->aclList, false)) + markFinished(accessList->allow, "first matching rule won"); + + // If we are not finished() here, the caller must distinguish between + // slow async calls and pure rule mismatches using asyncInProgress(). } void @@ -196,62 +190,128 @@ delete this; } -void +/// An ACLChecklist::matchNodes() wrapper to simplify profiling. +bool ACLChecklist::matchAclList(const ACLList * head, bool const fast) { + // TODO: remove by using object con/destruction-based PROF_* macros. PROF_start(aclMatchAclList); - const ACLList *node = head; - - finished_ = false; - - while (node) { - bool nodeMatched = node->matches(this); - - if (fast) - changeState(NullState::Instance()); - - if (finished()) { - PROF_stop(aclMatchAclList); - return; - } - - if (!nodeMatched || state_ != NullState::Instance()) { - - bool async = state_ != NullState::Instance(); - - checkForAsync(); - - bool async_in_progress = asyncInProgress(); - debugs(28, 3, "aclmatchAclList: async=" << (async ? 1 : 0) << - " nodeMatched=" << (nodeMatched ? 1 : 0) << - " async_in_progress=" << (async_in_progress ? 1 : 0) << - " lastACLResult() = " << (lastACLResult() ? 1 : 0) << - " finished() = " << finished()); - + const bool result = matchNodes(head, fast); + PROF_stop(aclMatchAclList); + return result; +} + +/** Returns true if and only if there was a match. If false is returned: + finished() indicates an error or exception of some kind, while + !finished() means there was a mismatch or an allowed slow async call. + If async calls are allowed (i.e. 'fast' was false), then those last + two cases can be distinguished using asyncInProgress(). +*/ +bool +ACLChecklist::matchNodes(const ACLList * head, bool const fast) +{ + assert(!finished()); + + for (const ACLList *node = head; node; node = node->next) { + + const NodeMatchingResult resultBeforeAsync = matchNode(*node, fast); + + if (resultBeforeAsync == nmrMatch) + continue; + + if (resultBeforeAsync == nmrMismatch || resultBeforeAsync == nmrFinished) + return false; + + assert(resultBeforeAsync == nmrNeedsAsync); + + // Ideally, this should be inside match() itself, but that requires + // prohibiting slow ACLs in options that do not support them. + // TODO: rename to maybeStartAsync()? + checkForAsync(); + + // Some match() code claims that an async lookup is needed, but then + // fails to start an async lookup when given a chance. We catch such + // cases here and call matchNode() again, hoping that some cached data + // prevents us from going async again. + // This is inefficient and ugly, but fixing all match() code, including + // the code it calls, such as ipcache_nbgethostbyname(), takes time. + if (!asyncInProgress()) { // failed to start an async operation + if (finished()) { - debugs(28, 3, "aclmatchAclList: " << this << " returning (AND list entry failed to match)"); - PROF_stop(aclMatchAclList); - return; + debugs(28, 3, HERE << this << " finished after failing to go async: " << currentAnswer()); + return false; // an exceptional case } - if (async && nodeMatched && !asyncInProgress() && lastACLResult()) { - // async acl, but using cached response, and it was a match - node = node->next; + const NodeMatchingResult resultAfterAsync = matchNode(*node, true); + // the second call disables slow checks so we cannot go async again + assert(resultAfterAsync != nmrNeedsAsync); + if (resultAfterAsync == nmrMatch) continue; - } - debugs(28, 3, "aclmatchAclList: " << this << " returning (AND list entry awaiting an async lookup)"); - PROF_stop(aclMatchAclList); - return; + assert(resultAfterAsync == nmrMismatch || resultAfterAsync == nmrFinished); + return false; } - node = node->next; - } - - debugs(28, 3, "aclmatchAclList: " << this << " returning true (AND list satisfied)"); - - markFinished(); - PROF_stop(aclMatchAclList); + assert(!finished()); // async operation is truly asynchronous + debugs(28, 3, HERE << this << " awaiting async operation"); + return false; + } + + debugs(28, 3, HERE << this << " success: all ACLs matched"); + return true; +} + + +/// Check whether a single ACL matches, returning NodeMatchingResult +ACLChecklist::NodeMatchingResult +ACLChecklist::matchNode(const ACLList &node, bool const fast) +{ + const bool nodeMatched = node.matches(this); + const bool needsAsync = asyncNeeded(); + const bool matchFinished = finished(); + + debugs(28, 3, HERE << this << + " matched=" << nodeMatched << + " async=" << needsAsync << + " finished=" << matchFinished); + + /* There are eight possible outcomes of the matches() call based on + (matched, async, finished) permutations. We support these four: + matched,!async,!finished: a match (must check next rule node) + !matched,!async,!finished: a mismatch (whole rule fails to match) + !matched,!async,finished: error or special condition (propagate) + !matched,async,!finished: ACL needs to make an async call (pause) + */ + + if (nodeMatched) { + // matches() should return false in all special cases + assert(!needsAsync && !matchFinished); + return nmrMatch; + } + + if (matchFinished) { + // we cannot be done and need an async call at the same time + assert(!needsAsync); + debugs(28, 3, HERE << this << " exception: " << currentAnswer()); + return nmrFinished; + } + + if (!needsAsync) { + debugs(28, 3, HERE << this << " simple mismatch"); + return nmrMismatch; + } + + /* we need an async call */ + + if (fast) { + changeState(NullState::Instance()); // disable async checks + markFinished(ACCESS_DUNNO, "async required but prohibited"); + debugs(28, 3, HERE << this << " DUNNO because cannot async"); + return nmrFinished; + } + + debugs(28, 3, HERE << this << " going async"); + return nmrNeedsAsync; } ACLChecklist::ACLChecklist() : @@ -261,8 +321,7 @@ async_(false), finished_(false), allow_(ACCESS_DENIED), - state_(NullState::Instance()), - lastACLResult_(false) + state_(NullState::Instance()) { } @@ -320,6 +379,7 @@ void ACLChecklist::nonBlockingCheck(ACLCB * callback_, void *callback_data_) { + preCheck("slow rules"); callback = callback_; callback_data = cbdataReference(callback_data_); matchNonBlocking(); @@ -329,11 +389,15 @@ ACLChecklist::fastCheck(const ACLList * list) { PROF_start(aclCheckFast); - currentAnswer(ACCESS_DUNNO); - matchAclList(list, true); - // assume ALLOWED on matches due to not having an acl_access object - if (finished()) - currentAnswer(ACCESS_ALLOWED); + + preCheck("fast ACLs"); + + // assume DENY/ALLOW on mis/matches due to not having acl_access object + if (matchAclList(list, true)) + markFinished(ACCESS_ALLOWED, "all ACLs matched"); + else + if (!finished()) + markFinished(ACCESS_DENIED, "ACL mismatched"); PROF_stop(aclCheckFast); return currentAnswer(); } @@ -345,33 +409,55 @@ ACLChecklist::fastCheck() { PROF_start(aclCheckFast); - currentAnswer(ACCESS_DUNNO); - + + preCheck("fast rules"); + + allow_t lastSeenKeyword = ACCESS_DUNNO; debugs(28, 5, "aclCheckFast: list: " << accessList); const acl_access *acl = cbdataReference(accessList); while (acl != NULL && cbdataReferenceValid(acl)) { - matchAclList(acl->aclList, true); + // on a match, finish + if (matchAclList(acl->aclList, true)) + markFinished(acl->allow, "first matching rule won"); + + // if finished (on a match or in exceptional cases), stop if (finished()) { - currentAnswer(acl->allow); + cbdataReferenceDone(acl); PROF_stop(aclCheckFast); - cbdataReferenceDone(acl); return currentAnswer(); } - /* - * Reference the next access entry - */ + // on a mismatch, try the next access rule + lastSeenKeyword = acl->allow; const acl_access *A = acl; acl = cbdataReference(acl->next); cbdataReferenceDone(A); } - debugs(28, 5, "aclCheckFast: no matches, returning: " << currentAnswer()); + // There were no rules to match or no rules matched + calcImplicitAnswer(lastSeenKeyword); PROF_stop(aclCheckFast); return currentAnswer(); } +/// When no rules matched, the answer is the inversion of the last seen rule +/// action (or ACCESS_DUNNO if the reversal is not possible). The caller +/// should set lastSeenAction to ACCESS_DUNNO if there were no rules to see. +void +ACLChecklist::calcImplicitAnswer(const allow_t &lastSeenAction) +{ + allow_t implicitRuleAnswer = ACCESS_DUNNO; + if (lastSeenAction == ACCESS_DENIED) // reverse last seen "deny" + implicitRuleAnswer = ACCESS_ALLOWED; + else if (lastSeenAction == ACCESS_ALLOWED) // reverse last seen "allow" + implicitRuleAnswer = ACCESS_DENIED; + // else we saw no rules and will respond with ACCESS_DUNNO + + debugs(28, 3, HERE << this << " NO match found, last action " << + lastSeenAction << " so returning " << implicitRuleAnswer); + markFinished(implicitRuleAnswer, "implicit rule won"); +} bool ACLChecklist::checking() const === modified file 'src/acl/Checklist.h' --- src/acl/Checklist.h 2011-07-16 15:21:48 +0000 +++ src/acl/Checklist.h 2012-06-16 15:03:46 +0000 @@ -92,47 +92,85 @@ virtual ~ACLChecklist(); /** - * Trigger off a non-blocking access check for a set of *_access options.. - * The callback specified will be called with true/false - * when the results of the ACL tests are known. + * Start a non-blocking (async) check for a list of allow/deny rules. + * Each rule comes with a list of ACLs. + * + * The callback specified will be called with the result of the check. + * + * The first rule where all ACLs match wins. If there is such a rule, + * the result becomes that rule keyword (ACCESS_ALLOWED or ACCESS_DENIED). + * + * If there are rules but all ACL lists mismatch, an implicit rule is used. + * Its result is the negation of the keyword of the last seen rule. + * + * Some ACLs may stop the check prematurely by setting an exceptional + * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a + * match or mismatch. + * + * If there are no rules to check at all, the result becomes ACCESS_DUNNO. + * Calling this method with no rules to check wastes a lot of CPU cycles + * and will result in a DBG_CRITICAL debugging message. */ void nonBlockingCheck(ACLCB * callback, void *callback_data); /** - * Trigger a blocking access check for a set of *_access options. - * - * ACLs which cannot be satisfied directly from available data are ignored. - * This means any proxy_auth, external_acl, DNS lookups, Ident lookups etc - * which have not already been performed and cached will not be checked. - * - * If there is no access list to check the default is to return ALLOWED. - * However callers should perform their own check and default based on local - * knowledge of the ACL usage rather than depend on this default. - * That will also save on work setting up ACLChecklist fields for a no-op. - * - * \retval ACCESS_DUNNO Unable to determine any result - * \retval ACCESS_ALLOWED Access Allowed - * \retval ACCESS_DENIED Access Denied + * Perform a blocking (immediate) check for a list of allow/deny rules. + * Each rule comes with a list of ACLs. + * + * The first rule where all ACLs match wins. If there is such a rule, + * the result becomes that rule keyword (ACCESS_ALLOWED or ACCESS_DENIED). + * + * If there are rules but all ACL lists mismatch, an implicit rule is used + * Its result is the negation of the keyword of the last seen rule. + * + * Some ACLs may stop the check prematurely by setting an exceptional + * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a + * match or mismatch. + * + * Some ACLs may require an async lookup which is prohibited by this + * method. In this case, the exceptional check result of ACCESS_DUNNO is + * immediately returned. + * + * If there are no rules to check at all, the result becomes ACCESS_DUNNO. */ allow_t const & fastCheck(); /** - * A version of fastCheck() for use when there is a one-line set of ACLs - * to be tested and a match determins the result action to be done. - * - * \retval ACCESS_DUNNO Unable to determine any result - * \retval ACCESS_ALLOWED ACLs all matched + * Perform a blocking (immediate) check whether a list of ACLs matches. + * This method is meant to be used with squid.conf ACL-driven options that + * lack allow/deny keywords and are tested one ACL list at a time. Whether + * the checks for other occurrences of the same option continue after this + * call is up to the caller and option semantics. + * + * If all ACLs match, the result becomes ACCESS_ALLOWED. + * + * If all ACLs mismatch, the result becomes ACCESS_DENIED. + * + * Some ACLs may stop the check prematurely by setting an exceptional + * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a + * match or mismatch. + * + * Some ACLs may require an async lookup which is prohibited by this + * method. In this case, the exceptional check result of ACCESS_DUNNO is + * immediately returned. + * + * If there are no ACLs to check at all, the result becomes ACCESS_ALLOWED. */ allow_t const & fastCheck(const ACLList * list); + // whether the last checked ACL of the current rule needs + // an async operation to determine whether there was a match + bool asyncNeeded() const; bool asyncInProgress() const; void asyncInProgress(bool const); + /// whether markFinished() was called bool finished() const; - void markFinished(); + /// called when no more ACLs should be checked; sets the final answer and + /// prints a debugging message explaining the reason for that answer + void markFinished(const allow_t &newAnswer, const char *reason); - allow_t const & currentAnswer() const; - void currentAnswer(allow_t const); + const allow_t ¤tAnswer() const { return allow_; } void changeState(AsyncState *); AsyncState *asyncState() const; @@ -156,20 +194,25 @@ void *callback_data; /** - * Attempt to check the current checklist against current data. - * This is the core routine behind all ACL test routines. - * As much as possible of current tests are performed immediately - * and the result is maybe delayed to wait for async lookups. - * - * When all tests are done callback is presented with one of: - * - ACCESS_ALLOWED Access explicitly Allowed - * - ACCESS_DENIED Access explicitly Denied + * Performs non-blocking check starting with the current rule. + * Used by nonBlockingCheck() to initiate the checks and by + * async operation callbacks to resume checks after the async + * operation updates the current Squid state. See nonBlockingCheck() + * for details on final result determination. */ void matchNonBlocking(); private: /* internal methods */ - void preCheck(); - void matchAclList(const ACLList * list, bool const fast); + /// possible outcomes when trying to match a single ACL node in a list + typedef enum { nmrMatch, nmrMismatch, nmrFinished, nmrNeedsAsync } + NodeMatchingResult; + + /// prepare for checking ACLs; called once per check + void preCheck(const char *what); + bool matchAclList(const ACLList * list, bool const fast); + bool matchNodes(const ACLList * head, bool const fast); + NodeMatchingResult matchNode(const ACLList &node, bool const fast); + void calcImplicitAnswer(const allow_t &lastSeenAction); bool async_; bool finished_; @@ -180,13 +223,7 @@ bool checking() const; void checking (bool const); - bool lastACLResult_; bool callerGone(); - -public: - bool lastACLResult(bool x) { return lastACLResult_ = x; } - - bool lastACLResult() const { return lastACLResult_; } }; #endif /* SQUID_ACLCHECKLIST_H */ === modified file 'src/auth/Acl.cc' --- src/auth/Acl.cc 2012-01-20 18:55:04 +0000 +++ src/auth/Acl.cc 2012-06-16 15:03:46 +0000 @@ -57,13 +57,16 @@ break; case AUTH_ACL_HELPER: - debugs(28, 4, HERE << "returning " << ACCESS_DENIED << " sending credentials to helper."); + debugs(28, 4, HERE << "returning " << ACCESS_DUNNO << " sending credentials to helper."); checklist->changeState(ProxyAuthLookup::Instance()); return ACCESS_DUNNO; // XXX: break this down into DUNNO, EXPIRED_OK, EXPIRED_BAD states case AUTH_ACL_CHALLENGE: - debugs(28, 4, HERE << "returning " << ACCESS_DENIED << " sending authentication challenge."); - checklist->changeState(ProxyAuthNeeded::Instance()); + debugs(28, 4, HERE << "returning " << ACCESS_AUTH_REQUIRED << " sending authentication challenge."); + /* Client is required to resend the request with correct authentication + * credentials. (This may be part of a stateful auth protocol.) + * The request is denied. + */ return ACCESS_AUTH_REQUIRED; default: === modified file 'src/auth/AclMaxUserIp.cc' --- src/auth/AclMaxUserIp.cc 2012-05-08 01:21:10 +0000 +++ src/auth/AclMaxUserIp.cc 2012-06-16 15:03:46 +0000 @@ -151,7 +151,6 @@ { ACLFilledChecklist *checklist = Filled(cl); allow_t answer = AuthenticateAcl(checklist); - checklist->currentAnswer(answer); int ti; // convert to tri-state ACL match 1,0,-1 @@ -168,6 +167,10 @@ case ACCESS_DUNNO: case ACCESS_AUTH_REQUIRED: default: + // If the answer is not allowed or denied (matches/not matches) and + // async authentication is not needed (asyncNeeded), then we are done. + if (!checklist->asyncNeeded()) + checklist->markFinished(answer, "AuthenticateAcl exception"); return -1; // other } } === modified file 'src/auth/AclProxyAuth.cc' --- src/auth/AclProxyAuth.cc 2012-05-08 01:21:10 +0000 +++ src/auth/AclProxyAuth.cc 2012-06-16 15:03:46 +0000 @@ -80,7 +80,6 @@ ACLProxyAuth::match(ACLChecklist *checklist) { allow_t answer = AuthenticateAcl(checklist); - checklist->currentAnswer(answer); // convert to tri-state ACL match 1,0,-1 switch (answer) { @@ -94,6 +93,10 @@ case ACCESS_DUNNO: case ACCESS_AUTH_REQUIRED: default: + // If the answer is not allowed or denied (matches/not matches) and + // async authentication is not needed (asyncNeeded), then we are done. + if (!checklist->asyncNeeded()) + checklist->markFinished(answer, "AuthenticateAcl exception"); return -1; // other } } @@ -126,14 +129,6 @@ return true; } -ProxyAuthNeeded ProxyAuthNeeded::instance_; - -ProxyAuthNeeded * -ProxyAuthNeeded::Instance() -{ - return &instance_; -} - ProxyAuthLookup ProxyAuthLookup::instance_; ProxyAuthLookup * @@ -182,19 +177,6 @@ checklist->matchNonBlocking(); } -void -ProxyAuthNeeded::checkForAsync(ACLChecklist *checklist) const -{ - /* Client is required to resend the request with correct authentication - * credentials. (This may be part of a stateful auth protocol.) - * The request is denied. - */ - debugs(28, 6, "ACLChecklist::checkForAsync: requiring Proxy Auth header."); - checklist->currentAnswer(ACCESS_AUTH_REQUIRED); - checklist->changeState (ACLChecklist::NullState::Instance()); - checklist->markFinished(); -} - ACL * ACLProxyAuth::clone() const { === modified file 'src/auth/AclProxyAuth.h' --- src/auth/AclProxyAuth.h 2011-02-07 10:27:53 +0000 +++ src/auth/AclProxyAuth.h 2012-06-16 15:03:46 +0000 @@ -53,17 +53,6 @@ static void LookupDone(void *data, char *result); }; -class ProxyAuthNeeded : public ACLChecklist::AsyncState -{ - -public: - static ProxyAuthNeeded *Instance(); - virtual void checkForAsync(ACLChecklist *)const; - -private: - static ProxyAuthNeeded instance_; -}; - class ACLProxyAuth : public ACL { === modified file 'src/external_acl.cc' --- src/external_acl.cc 2012-05-08 01:21:10 +0000 +++ src/external_acl.cc 2012-06-16 15:03:46 +0000 @@ -805,12 +805,12 @@ debugs(82, 9, HERE << "No helper entry available"); #if USE_AUTH if (acl->def->require_auth) { - int ti = AuthenticateAcl(ch); /* Make sure the user is authenticated */ debugs(82, 3, HERE << acl->def->name << " check user authenticated."); - if (ti != 1) { + const allow_t ti = AuthenticateAcl(ch); + if (ti != ACCESS_ALLOWED) { debugs(82, 2, HERE << acl->def->name << " user not authenticated (" << ti << ")"); - return ACCESS_AUTH_REQUIRED; + return ti; } debugs(82, 3, HERE << acl->def->name << " user is authenticated."); } @@ -824,7 +824,18 @@ entry = static_cast(hash_lookup(acl->def->cache, key)); - if (!entry || external_acl_grace_expired(acl->def, entry)) { + external_acl_entry *staleEntry = entry; + if (entry && external_acl_entry_expired(acl->def, entry)) + entry = NULL; + + if (entry && external_acl_grace_expired(acl->def, entry)) { + // refresh in the background + ExternalACLLookup::Start(ch, acl, true); + debugs(82, 4, HERE << "no need to wait for the refresh of '" << + key << "' in '" << acl->def->name << "' (ch=" << ch << ")."); + } + + if (!entry) { debugs(82, 2, HERE << acl->def->name << "(\"" << key << "\") = lookup needed"); debugs(82, 2, HERE << "\"" << key << "\": entry=@" << entry << ", age=" << (entry ? (long int) squid_curtime - entry->date : 0)); @@ -833,9 +844,9 @@ debugs(82, 2, HERE << "\"" << key << "\": queueing a call."); ch->changeState(ExternalACLLookup::Instance()); debugs(82, 2, HERE << "\"" << key << "\": return -1."); - return ACCESS_DUNNO; // to get here we have to have an expired cache entry. MUST not use. + return ACCESS_DUNNO; // expired cached or simply absent entry } else { - if (!entry) { + if (!staleEntry) { debugs(82, DBG_IMPORTANT, "WARNING: external ACL '" << acl->def->name << "' queue overload. Request rejected '" << key << "'."); external_acl_message = "SYSTEM TOO BUSY, TRY AGAIN LATER"; @@ -843,12 +854,22 @@ } else { debugs(82, DBG_IMPORTANT, "WARNING: external ACL '" << acl->def->name << "' queue overload. Using stale result. '" << key << "'."); + entry = staleEntry; /* Fall thru to processing below */ } } } } + debugs(82, 4, HERE << "entry = { date=" << + (long unsigned int) entry->date << + ", result=" << entry->result << + " tag=" << entry->tag << + " log=" << entry->log << " }"); +#if USE_AUTH + debugs(82, 4, HERE << "entry user=" << entry->user); +#endif + external_acl_cache_touch(acl->def, entry); external_acl_message = entry->message.termedBuf(); @@ -861,7 +882,6 @@ ACLExternal::match(ACLChecklist *checklist) { allow_t answer = aclMatchExternal(data, Filled(checklist)); - checklist->currentAnswer(answer); // convert to tri-state ACL match 1,0,-1 switch (answer) { @@ -874,6 +894,10 @@ case ACCESS_DUNNO: case ACCESS_AUTH_REQUIRED: default: + // If the answer is not allowed or denied (matches/not matches) and + // async authentication is not needed (asyncNeeded), then we are done. + if (!checklist->asyncNeeded()) + checklist->markFinished(answer, "aclMatchExternal exception"); return -1; // other } } @@ -1365,53 +1389,28 @@ } void -ACLExternal::ExternalAclLookup(ACLChecklist *checklist, ACLExternal * me, EAH * callback, void *callback_data) -{ - MemBuf buf; - external_acl_data *acl = me->data; +ACLExternal::ExternalAclLookup(ACLChecklist *checklist, ACLExternal * me) +{ + ExternalACLLookup::Start(checklist, me->data, false); +} + +void +ExternalACLLookup::Start(ACLChecklist *checklist, external_acl_data *acl, bool inBackground) +{ external_acl *def = acl->def; - externalAclState *state; - dlink_node *node; - externalAclState *oldstate = NULL; - bool graceful = 0; - + ACLFilledChecklist *ch = Filled(checklist); -#if USE_AUTH - if (acl->def->require_auth) { - int ti; - /* Make sure the user is authenticated */ - debugs(82, 3, HERE << acl->def->name << " check user authenticated."); - - if ((ti = AuthenticateAcl(ch)) != 1) { - debugs(82, DBG_IMPORTANT, "WARNING: " << acl->def->name << - " user authentication failure (" << ti << ", ch=" << ch << ")"); - callback(callback_data, NULL); - return; - } - debugs(82, 3, HERE << acl->def->name << " user is authenticated."); - } -#endif - const char *key = makeExternalAclKey(ch, acl); - - if (!key) { - debugs(82, 1, "externalAclLookup: lookup in '" << def->name << - "', prerequisit failure (ch=" << ch << ")"); - callback(callback_data, NULL); - return; - } - - debugs(82, 2, "externalAclLookup: lookup in '" << def->name << "' for '" << key << "'"); - - external_acl_entry *entry = static_cast(hash_lookup(def->cache, key)); - - if (entry && external_acl_entry_expired(def, entry)) - entry = NULL; + assert(key); + + debugs(82, 2, HERE << (inBackground ? "bg" : "fg") << " lookup in '" << + def->name << "' for '" << key << "'"); /* Check for a pending lookup to hook into */ // only possible if we are caching results. + externalAclState *oldstate = NULL; if (def->cache_size > 0) { - for (node = def->queue.head; node; node = node->next) { + for (dlink_node *node = def->queue.head; node; node = node->next) { externalAclState *oldstatetmp = static_cast(node->data); if (strcmp(key, oldstatetmp->key) == 0) { @@ -1421,35 +1420,21 @@ } } - if (entry && external_acl_grace_expired(def, entry)) { - if (oldstate) { - debugs(82, 4, "externalAclLookup: in grace period, but already pending lookup ('" << key << "', ch=" << ch << ")"); - callback(callback_data, entry); - return; - } else { - graceful = 1; // grace expired, (neg)ttl did not, and we must start a new lookup. - } - } - - // The entry is in the cache, grace_ttl did not expired. - if (!graceful && entry && !external_acl_grace_expired(def, entry)) { - /* Should not really happen, but why not.. */ - callback(callback_data, entry); - debugs(82, 4, "externalAclLookup: no lookup pending for '" << key << "', and grace not expired"); - debugs(82, 4, "externalAclLookup: (what tha' hell?)"); + // A background refresh has no need to piggiback on a pending request: + // When the pending request completes, the cache will be refreshed anyway. + if (oldstate && inBackground) { + debugs(82, 7, HERE << "'" << def->name << "' queue is already being refreshed (ch=" << ch << ")"); return; } - /* No pending lookup found. Sumbit to helper */ - state = cbdataAlloc(externalAclState); - + externalAclState *state = cbdataAlloc(externalAclState); state->def = cbdataReference(def); state->key = xstrdup(key); - if (!graceful) { - state->callback = callback; - state->callback_data = cbdataReference(callback_data); + if (!inBackground) { + state->callback = &ExternalACLLookup::LookupDone; + state->callback_data = cbdataReference(checklist); } if (oldstate) { @@ -1457,16 +1442,19 @@ state->queue = oldstate->queue; oldstate->queue = state; } else { + /* No pending lookup found. Sumbit to helper */ + /* Check for queue overload */ if (def->theHelper->stats.queue_size >= (int)def->theHelper->childs.n_running) { - debugs(82, 1, "externalAclLookup: '" << def->name << "' queue overload (ch=" << ch << ")"); + debugs(82, 7, HERE << "'" << def->name << "' queue is too long"); + assert(inBackground); // or the caller should have checked cbdataFree(state); - callback(callback_data, entry); return; } /* Send it off to the helper */ + MemBuf buf; buf.init(); buf.Printf("%s\n", key); @@ -1480,28 +1468,6 @@ buf.clean(); } - if (graceful) { - /* No need to wait during grace period */ - debugs(82, 4, "externalAclLookup: no need to wait for the result of '" << - key << "' in '" << def->name << "' (ch=" << ch << ")."); - debugs(82, 4, "externalAclLookup: using cached entry " << entry); - - if (entry != NULL) { - debugs(82, 4, "externalAclLookup: entry = { date=" << - (long unsigned int) entry->date << - ", result=" << entry->result << - " tag=" << entry->tag << - " log=" << entry->log << " }"); -#if USE_AUTH - debugs(82, 4, "externalAclLookup: user=" << entry->user); -#endif - copyResultsFromEntry(ch->request, entry); - } - - callback(callback_data, entry); - return; - } - debugs(82, 4, "externalAclLookup: will wait for the result of '" << key << "' in '" << def->name << "' (ch=" << ch << ")."); } @@ -1586,9 +1552,10 @@ ACLExternal *me = dynamic_cast (acl); assert (me); checklist->asyncInProgress(true); - ACLExternal::ExternalAclLookup(checklist, me, LookupDone, checklist); + ACLExternal::ExternalAclLookup(checklist, me); } +/// Called when an async lookup returns void ExternalACLLookup::LookupDone(void *data, void *result) { === modified file 'src/http.cc' --- src/http.cc 2012-04-11 09:10:15 +0000 +++ src/http.cc 2012-06-16 15:03:46 +0000 @@ -745,7 +745,7 @@ if (Config.accessList.reply) { ACLFilledChecklist ch(Config.accessList.reply, originalRequest(), NULL); ch.reply = HTTPMSGLOCK(reply); - if (!ch.fastCheck()) { // TODO: support slow lookups? + if (ch.fastCheck() != ACCESS_ALLOWED) { // TODO: support slow lookups? debugs(11, 3, HERE << "ignoring denied 1xx"); proceedAfter1xx(); return; @@ -2191,7 +2191,7 @@ } ACLFilledChecklist ch(Config.accessList.brokenPosts, originalRequest(), NULL); - if (!ch.fastCheck()) { + if (ch.fastCheck() != ACCESS_ALLOWED) { debugs(11, 5, HERE << "didn't match brokenPosts"); return false; } === modified file 'src/ident/AclIdent.cc' --- src/ident/AclIdent.cc 2012-01-20 18:55:04 +0000 +++ src/ident/AclIdent.cc 2012-06-16 15:03:46 +0000 @@ -89,10 +89,14 @@ return data->match(checklist->rfc931); } else if (checklist->conn() != NULL && checklist->conn()->clientConnection != NULL && checklist->conn()->clientConnection->rfc931[0]) { return data->match(checklist->conn()->clientConnection->rfc931); - } else { + } else if (checklist->conn() != NULL && Comm::IsConnOpen(checklist->conn()->clientConnection)) { debugs(28, 3, HERE << "switching to ident lookup state"); checklist->changeState(IdentLookup::Instance()); return 0; + } else { + debugs(28, DBG_IMPORTANT, HERE << "Can't start ident lookup. No client connection" ); + checklist->markFinished(ACCESS_DUNNO, "cannot start ident lookup"); + return -1; } } @@ -127,15 +131,12 @@ IdentLookup::checkForAsync(ACLChecklist *cl)const { ACLFilledChecklist *checklist = Filled(cl); - if (checklist->conn() != NULL && Comm::IsConnOpen(checklist->conn()->clientConnection)) { + const ConnStateData *conn = checklist->conn(); + // check that ACLIdent::match() tested this lookup precondition + assert(conn && Comm::IsConnOpen(conn->clientConnection)); debugs(28, 3, HERE << "Doing ident lookup" ); checklist->asyncInProgress(true); Ident::Start(checklist->conn()->clientConnection, LookupDone, checklist); - } else { - debugs(28, DBG_IMPORTANT, "IdentLookup::checkForAsync: Can't start ident lookup. No client connection" ); - checklist->currentAnswer(ACCESS_DENIED); - checklist->markFinished(); - } } void