Work in progress to fix various ACL bugs. 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. === modified file 'src/ExternalACL.h' --- src/ExternalACL.h 2009-03-08 19:34:36 +0000 +++ src/ExternalACL.h 2012-05-30 22:34:54 +0000 @@ -19,66 +19,68 @@ * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA. * * * Copyright (c) 2003, Robert Collins */ #ifndef SQUID_EXTERNALACL_H #define SQUID_EXTERNALACL_H #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 { public: 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 { public: MEMPROXY_CLASS(ACLExternal); - static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *, EAH * callback, void *callback_data); + static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *); ACLExternal(char const *); ACLExternal(ACLExternal const &); ~ACLExternal(); ACLExternal&operator=(ACLExternal const &); virtual ACL *clone()const; virtual char const *typeString() const; virtual void parse(); virtual int match(ACLChecklist *checklist); /* This really should be dynamic based on the external class defn */ virtual bool requiresRequest() const {return true;} /* when requiresRequest is made dynamic, review this too */ // virtual bool requiresReply() const {return true;} virtual bool isProxyAuth() const; virtual wordlist *dump() const; virtual bool valid () const; virtual bool empty () const; === modified file 'src/acl/Acl.cc' --- src/acl/Acl.cc 2012-04-25 05:29:20 +0000 +++ src/acl/Acl.cc 2012-05-30 22:45:20 +0000 @@ -305,50 +305,56 @@ if (!checklist->hasRequest() && requiresRequest()) { debugs(28, 1, "ACL::checklistMatches WARNING: '" << name << "' ACL is used but there is no HTTP request -- not matching."); return 0; } if (!checklist->hasReply() && requiresReply()) { debugs(28, 1, "ACL::checklistMatches WARNING: '" << name << "' ACL is used but there is no HTTP reply -- not matching."); return 0; } debugs(28, 3, "ACL::checklistMatches: checking '" << name << "'"); rv= match(checklist); debugs(28, 3, "ACL::ChecklistMatches: result for '" << name << "' is " << rv); return rv; } bool 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; } /*********************/ /* Destroy functions */ /*********************/ ACL::~ACL() { debugs(28, 3, "ACL::~ACL: '" << cfgline << "'"); safe_free(cfgline); } /* to be split into separate files in the future */ CBDATA_CLASS_INIT(acl_access); void * acl_access::operator new (size_t) { === modified file 'src/acl/Checklist.cc' --- src/acl/Checklist.cc 2012-05-04 22:20:07 +0000 +++ src/acl/Checklist.cc 2012-05-30 22:52:22 +0000 @@ -25,249 +25,312 @@ * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA. * * Copyright (c) 2003, Robert Collins */ #include "squid-old.h" #include "acl/Checklist.h" void ACLChecklist::matchNonBlocking() { if (checking()) return; - /** Deny if no rules present. */ - currentAnswer(ACCESS_DENIED); - if (callerGone()) { - checkCallback(currentAnswer()); + checkCallback(ACCESS_DENIED); // the answer does not really matter return; } /** The ACL List should NEVER be NULL when calling this method. * Always caller should check for NULL and handle appropriate to its needs first. * 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); checkAccessList(); checking (false); if (asyncInProgress()) { return; } if (finished()) { /** \par * Either the request is allowed, denied, requires authentication. */ debugs(28, 3, "ACLChecklist::check: " << this << " match found, calling back with " << currentAnswer()); cbdataReferenceDone(accessList); /* A */ checkCallback(currentAnswer()); /* From here on in, this may be invalid */ return; } + lastSeenKeyword = accessList->allow; + /* * Reference the next access entry */ const acl_access *A = accessList; assert (A); accessList = cbdataReference(A->next); 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 ACLChecklist::asyncInProgress() const { return async_; } void ACLChecklist::asyncInProgress(bool const newAsync) { assert (!finished() && !(asyncInProgress() && newAsync)); async_ = newAsync; debugs(28, 3, "ACLChecklist::asyncInProgress: " << this << " async set to " << async_); } bool ACLChecklist::finished() const { return finished_; } void -ACLChecklist::markFinished() +ACLChecklist::markFinished(const allow_t &finalAnswer) { assert (!finished() && !asyncInProgress()); finished_ = true; - debugs(28, 3, "ACLChecklist::markFinished: " << this << - " checklist processing finished"); -} - -void -ACLChecklist::preCheck() -{ - debugs(28, 3, "ACLChecklist::preCheck: " << this << " checking '" << accessList->cfgline << "'"); - /* what is our result on a match? */ - currentAnswer(accessList->allow); + currentAnswer(finalAnswer); + debugs(28, 3, HERE << this << " answer: " << finalAnswer); } 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); + + // If we are not finished() here, the caller must distinguish between + // slow async calls and pure rule mismatches using asyncInProgress(). } void ACLChecklist::checkForAsync() { asyncState()->checkForAsync(this); } // ACLFilledChecklist overwrites this to unclock something before we // "delete this" void ACLChecklist::checkCallback(allow_t answer) { ACLCB *callback_; void *cbdata_; debugs(28, 3, "ACLChecklist::checkCallback: " << this << " answer=" << answer); callback_ = callback; callback = NULL; if (cbdataReferenceValidDone(callback_data, &cbdata_)) callback_(answer, cbdata_); 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; + 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) +{ finished_ = false; - while (node) { - bool nodeMatched = node->matches(this); + for (const ACLList *node = head; node; node = node->next) { - if (fast) - changeState(NullState::Instance()); + const NodeMatchingResult resultBeforeAsync = matchNode(*node, fast); - if (finished()) { - PROF_stop(aclMatchAclList); - return; - } + 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, HERE << this << " finished after failing to go async: " << currentAnswer()); + return false; // an exceptional case + } - if (!nodeMatched || state_ != NullState::Instance()) { + 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; - bool async = state_ != NullState::Instance(); + assert(resultAfterAsync == nmrMismatch || resultAfterAsync == nmrFinished); + return false; + } - checkForAsync(); + assert(!finished()); // async operation is truly asynchronous + debugs(28, 3, HERE << this << " awaiting async operation"); + return false; + } - 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()); + debugs(28, 3, HERE << this << " success: all ACLs matched"); + return true; +} - if (finished()) { - debugs(28, 3, "aclmatchAclList: " << this << " returning (AND list entry failed to match)"); - PROF_stop(aclMatchAclList); - return; - } - if (async && nodeMatched && !asyncInProgress() && lastACLResult()) { - // async acl, but using cached response, and it was a match - node = node->next; - continue; - } +/// 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) + */ - debugs(28, 3, "aclmatchAclList: " << this << " returning (AND list entry awaiting an async lookup)"); - PROF_stop(aclMatchAclList); - return; - } + if (nodeMatched) { + // matches() should return false in all special cases + assert(!needsAsync && !matchFinished); + return nmrMatch; + } - node = node->next; + 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; } - debugs(28, 3, "aclmatchAclList: " << this << " returning true (AND list satisfied)"); + /* we need an async call */ - markFinished(); - PROF_stop(aclMatchAclList); + if (fast) { + changeState(NullState::Instance()); // disable async checks + markFinished(ACCESS_DUNNO); // cannot continue as that requires async + debugs(28, 3, HERE << this << " DUNNO because cannot async"); + return nmrFinished; + } + + debugs(28, 3, HERE << this << " going async"); + return nmrNeedsAsync; } ACLChecklist::ACLChecklist() : accessList (NULL), callback (NULL), callback_data (NULL), async_(false), finished_(false), allow_(ACCESS_DENIED), - state_(NullState::Instance()), - lastACLResult_(false) + state_(NullState::Instance()) { } ACLChecklist::~ACLChecklist() { assert (!asyncInProgress()); cbdataReferenceDone(accessList); debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this); } void ACLChecklist::AsyncState::changeState (ACLChecklist *checklist, AsyncState *newState) const { checklist->changeState(newState); } ACLChecklist::NullState * @@ -300,81 +363,102 @@ return state_; } /** * Kick off a non-blocking (slow) ACL access list test * * NP: this should probably be made Async now. */ void ACLChecklist::nonBlockingCheck(ACLCB * callback_, void *callback_data_) { callback = callback_; callback_data = cbdataReference(callback_data_); matchNonBlocking(); } allow_t const & 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); + // assume DENY/ALLOW on mis/matches due to not having acl_access object + if (matchAclList(list, true)) + markFinished(ACCESS_ALLOWED); + else + if (!finished()) + markFinished(ACCESS_DENIED); PROF_stop(aclCheckFast); return currentAnswer(); } /* Warning: do not cbdata lock this here - it * may be static or on the stack */ allow_t const & ACLChecklist::fastCheck() { PROF_start(aclCheckFast); - currentAnswer(ACCESS_DUNNO); + 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); + + // if finished (on a match or in exceptional cases), stop if (finished()) { - currentAnswer(acl->allow); - PROF_stop(aclCheckFast); cbdataReferenceDone(acl); + PROF_stop(aclCheckFast); 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); +} bool ACLChecklist::checking() const { return checking_; } void ACLChecklist::checking (bool const newValue) { checking_ = newValue; } bool ACLChecklist::callerGone() { return !cbdataReferenceValid(callback_data); } === modified file 'src/acl/Checklist.h' --- src/acl/Checklist.h 2012-05-04 22:20:07 +0000 +++ src/acl/Checklist.h 2012-05-30 22:59:32 +0000 @@ -108,85 +108,90 @@ * 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 */ 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 */ 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 matched; sets current answer + void markFinished(const allow_t &newAnswer); const allow_t ¤tAnswer() const { return allow_; } void currentAnswer(const allow_t &newAnswer) { allow_ = newAnswer; } void changeState(AsyncState *); AsyncState *asyncState() const; // XXX: ACLs that need request or reply have to use ACLFilledChecklist and // should do their own checks so that we do not have to povide these two // for ACL::checklistMatches to use virtual bool hasRequest() const = 0; virtual bool hasReply() const = 0; protected: virtual void checkCallback(allow_t answer); private: + void calcImplicitAnswer(const allow_t &lastSeenAction); void checkAccessList(); void checkForAsync(); public: const acl_access *accessList; ACLCB *callback; 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 */ 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; + + bool matchAclList(const ACLList * list, bool const fast); + bool matchNodes(const ACLList * head, bool const fast); + NodeMatchingResult matchNode(const ACLList &node, bool const fast); bool async_; bool finished_; allow_t allow_; AsyncState *state_; bool checking_; 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-05-23 21:30:22 +0000 @@ -40,34 +40,37 @@ /* Proxy authorization on proxy requests */ headertype = HDR_PROXY_AUTHORIZATION; } /* get authed here */ /* Note: this fills in auth_user_request when applicable */ const AuthAclState result = Auth::UserRequest::tryToAuthenticateAndSetAuthUser( &checklist->auth_user_request, headertype, request, checklist->conn(), checklist->src_addr); switch (result) { case AUTH_ACL_CANNOT_AUTHENTICATE: debugs(28, 4, HERE << "returning " << ACCESS_DENIED << " user authenticated but not authorised."); return ACCESS_DENIED; case AUTH_AUTHENTICATED: return ACCESS_ALLOWED; 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: fatal("unexpected authenticateAuthenticate reply\n"); return ACCESS_DENIED; } } === modified file 'src/auth/AclMaxUserIp.cc' --- src/auth/AclMaxUserIp.cc 2012-05-14 10:37:40 +0000 +++ src/auth/AclMaxUserIp.cc 2012-05-25 15:09:00 +0000 @@ -134,63 +134,60 @@ /* remove _this_ ip, as it is the culprit for going over the limit */ authenticateAuthUserRequestRemoveIp(auth_user_request, src_addr); debugs(28, 4, "aclMatchUserMaxIP: Denying access in strict mode"); } else { /* * non-strict - remove some/all of the cached entries * ie to allow the user to move machines easily */ authenticateAuthUserRequestClearIp(auth_user_request); debugs(28, 4, "aclMatchUserMaxIP: Denying access in non-strict mode - flushing the user ip cache"); } return 1; } int ACLMaxUserIP::match(ACLChecklist *cl) { ACLFilledChecklist *checklist = Filled(cl); allow_t answer = AuthenticateAcl(checklist); - if (answer != ACCESS_DENIED && answer != ACCESS_ALLOWED) { - // If the answer is not allowed or denied (matches/not matches), requires - // authentication (ACCESS_AUTH_*) or the authentication is in progress (ACCESS_DUNNO) - // so change the state in the related checklist. - checklist->currentAnswer(answer); - } - int ti; // convert to tri-state ACL match 1,0,-1 switch (answer) { case ACCESS_ALLOWED: // check for a match ti = match(checklist->auth_user_request, checklist->src_addr); checklist->auth_user_request = NULL; return ti; case ACCESS_DENIED: return 0; // non-match 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); return -1; // other } } wordlist * ACLMaxUserIP::dump() const { if (!maximum) return NULL; wordlist *W = NULL; if (flags.strict) wordlistAdd(&W, "-s"); char buf[128]; snprintf(buf, sizeof(buf), "%lu", (unsigned long int) maximum); wordlistAdd(&W, buf); === modified file 'src/auth/AclProxyAuth.cc' --- src/auth/AclProxyAuth.cc 2012-05-14 10:37:40 +0000 +++ src/auth/AclProxyAuth.cc 2012-05-25 15:07:23 +0000 @@ -63,99 +63,88 @@ type_ = rhs.type_; return *this; } char const * ACLProxyAuth::typeString() const { return type_; } void ACLProxyAuth::parse() { data->parse(); } int ACLProxyAuth::match(ACLChecklist *checklist) { allow_t answer = AuthenticateAcl(checklist); - if (answer != ACCESS_DENIED && answer != ACCESS_ALLOWED) { - // If the answer is not allowed or denied (matches/not matches), requires - // authentication (ACCESS_AUTH_*) or the authentication is in progress (ACCESS_DUNNO) - // so change the state in the related checklist. - checklist->currentAnswer(answer); - } - // convert to tri-state ACL match 1,0,-1 switch (answer) { case ACCESS_ALLOWED: // check for a match return matchProxyAuth(checklist); case ACCESS_DENIED: return 0; // non-match 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); return -1; // other } } wordlist * ACLProxyAuth::dump() const { return data->dump(); } bool ACLProxyAuth::empty () const { return data->empty(); } bool ACLProxyAuth::valid () const { if (authenticateSchemeCount() == 0) { debugs(28, 0, "Can't use proxy auth because no authentication schemes were compiled."); return false; } if (authenticateActiveSchemeCount() == 0) { debugs(28, 0, "Can't use proxy auth because no authentication schemes are fully configured."); return false; } return true; } -ProxyAuthNeeded ProxyAuthNeeded::instance_; - -ProxyAuthNeeded * -ProxyAuthNeeded::Instance() -{ - return &instance_; -} - ProxyAuthLookup ProxyAuthLookup::instance_; ProxyAuthLookup * ProxyAuthLookup::Instance() { return &instance_; } void ProxyAuthLookup::checkForAsync(ACLChecklist *cl)const { ACLFilledChecklist *checklist = Filled(cl); checklist->asyncInProgress(true); debugs(28, 3, HERE << "checking password via authenticator"); /* make sure someone created auth_user_request for us */ assert(checklist->auth_user_request != NULL); assert(checklist->auth_user_request->valid()); checklist->auth_user_request->start(LookupDone, checklist); @@ -170,53 +159,40 @@ if (result != NULL) fatal("AclLookupProxyAuthDone: Old code floating around somewhere.\nMake clean and if that doesn't work, report a bug to the squid developers.\n"); if (checklist->auth_user_request == NULL || !checklist->auth_user_request->valid() || checklist->conn() == NULL) { /* credentials could not be checked either way * restart the whole process */ /* OR the connection was closed, there's no way to continue */ checklist->auth_user_request = NULL; if (checklist->conn() != NULL) { checklist->conn()->auth_user_request = NULL; } } checklist->asyncInProgress(false); checklist->changeState (ACLChecklist::NullState::Instance()); 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 { return new ACLProxyAuth(*this); } int ACLProxyAuth::matchForCache(ACLChecklist *cl) { ACLFilledChecklist *checklist = Filled(cl); assert (checklist->auth_user_request != NULL); return data->match(checklist->auth_user_request->username()); } /* aclMatchProxyAuth can return two exit codes: * 0 : Authorisation for this ACL failed. (Did not match) * 1 : Authorisation OK. (Matched) */ int ACLProxyAuth::matchProxyAuth(ACLChecklist *cl) === modified file 'src/auth/AclProxyAuth.h' --- src/auth/AclProxyAuth.h 2011-02-07 10:27:53 +0000 +++ src/auth/AclProxyAuth.h 2012-05-23 21:30:52 +0000 @@ -36,51 +36,40 @@ #define SQUID_ACLPROXYAUTH_H #if USE_AUTH #include "acl/Acl.h" #include "acl/Data.h" #include "acl/Checklist.h" class ProxyAuthLookup : public ACLChecklist::AsyncState { public: static ProxyAuthLookup *Instance(); virtual void checkForAsync(ACLChecklist *)const; private: static ProxyAuthLookup instance_; 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 { public: MEMPROXY_CLASS(ACLProxyAuth); ~ACLProxyAuth(); ACLProxyAuth(ACLData *, char const *); ACLProxyAuth (ACLProxyAuth const &); ACLProxyAuth &operator= (ACLProxyAuth const &); virtual char const *typeString() const; virtual void parse(); virtual bool isProxyAuth() const {return true;} virtual int match(ACLChecklist *checklist); virtual wordlist *dump() const; virtual bool valid () const; virtual bool empty () const; virtual bool requiresRequest() const {return true;} === modified file 'src/external_acl.cc' --- src/external_acl.cc 2012-05-14 10:37:40 +0000 +++ src/external_acl.cc 2012-05-30 23:18:08 +0000 @@ -788,114 +788,132 @@ } } else { /* Not valid, or not ours.. get rid of it */ debugs(82, 9, HERE << "entry " << entry << " not valid or not ours. Discarded."); if (entry) { debugs(82, 9, HERE << "entry def=" << entry->def << ", our def=" << acl->def); key = makeExternalAclKey(ch, acl); debugs(82, 9, HERE << "entry key='" << (char *)entry->key << "', our key='" << key << "'"); } cbdataReferenceDone(ch->extacl_entry); entry = NULL; } } external_acl_message = "MISSING REQUIRED INFORMATION"; if (!entry) { 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."); } #endif key = makeExternalAclKey(ch, acl); if (!key) { /* Not sufficient data to process */ return ACCESS_DUNNO; } 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)); if (acl->def->theHelper->stats.queue_size <= (int)acl->def->theHelper->childs.n_active) { 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"; return ACCESS_DUNNO; } 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(); debugs(82, 2, HERE << acl->def->name << " = " << entry->result); copyResultsFromEntry(ch->request, entry); return entry->result; } int ACLExternal::match(ACLChecklist *checklist) { allow_t answer = aclMatchExternal(data, Filled(checklist)); - if (answer != ACCESS_DENIED && answer != ACCESS_ALLOWED) { - // If the answer is not allowed or denied (matches/not matches), requires - // authentication (ACCESS_AUTH_*) or the authentication is in progress (ACCESS_DUNNO) - // so change the state in the related checklist. - checklist->currentAnswer(answer); - } - // convert to tri-state ACL match 1,0,-1 switch (answer) { case ACCESS_ALLOWED: return 1; // match case ACCESS_DENIED: return 0; // non-match 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); return -1; // other } } wordlist * ACLExternal::dump() const { external_acl_data const *acl = data; wordlist *result = NULL; wordlist *arg; MemBuf mb; mb.init(); mb.Printf("%s", acl->def->name); for (arg = acl->arguments; arg; arg = arg->next) { mb.Printf(" %s", arg->key); } wordlistAdd(&result, mb.buf); mb.clean(); @@ -1353,177 +1371,118 @@ external_acl_cache_delete(state->def, oldentry); } } do { void *cbdata; cbdataReferenceDone(state->def); if (state->callback && cbdataReferenceValidDone(state->callback_data, &cbdata)) state->callback(cbdata, entry); next = state->queue; cbdataFree(state); state = next; } while (state); } void -ACLExternal::ExternalAclLookup(ACLChecklist *checklist, ACLExternal * me, EAH * callback, void *callback_data) +ACLExternal::ExternalAclLookup(ACLChecklist *checklist, ACLExternal * me) { - MemBuf buf; - external_acl_data *acl = me->data; - external_acl *def = acl->def; - externalAclState *state; - dlink_node *node; - externalAclState *oldstate = NULL; - bool graceful = 0; + ExternalACLLookup::Start(checklist, me->data, false); +} +void +ExternalACLLookup::Start(ACLChecklist *checklist, external_acl_data *acl, bool inBackground) +{ + external_acl *def = acl->def; + 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); + assert(key); - 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; + 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) { oldstate = oldstatetmp; break; } } } - 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?)"); + // no need to piggiback to a pending request if that request is sufficient + 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) { /* Hook into pending lookup */ 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); debugs(82, 4, "externalAclLookup: looking up for '" << key << "' in '" << def->name << "'."); helperSubmit(def->theHelper, buf.buf, externalAclHandleReply, state); dlinkAdd(state, &state->list, &def->queue); 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 << ")."); } static void externalAclStats(StoreEntry * sentry) { external_acl *p; for (p = Config.externalAclHelperList; p; p = p->next) { storeAppendPrintf(sentry, "External ACL Statistics: %s\n", p->name); storeAppendPrintf(sentry, "Cache size: %d\n", p->cache->count); helperStats(sentry, p->theHelper); storeAppendPrintf(sentry, "\n"); } } static void externalAclRegisterWithCacheManager(void) { @@ -1574,43 +1533,44 @@ } } ExternalACLLookup ExternalACLLookup::instance_; ExternalACLLookup * ExternalACLLookup::Instance() { return &instance_; } void ExternalACLLookup::checkForAsync(ACLChecklist *checklist)const { /* TODO: optimise this - we probably have a pointer to this * around somewhere */ ACL *acl = ACL::FindByName(AclMatchedName); assert(acl); 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) { ACLFilledChecklist *checklist = Filled(static_cast(data)); checklist->extacl_entry = cbdataReference((external_acl_entry *)result); checklist->asyncInProgress(false); checklist->changeState (ACLChecklist::NullState::Instance()); checklist->matchNonBlocking(); } /* This registers "external" in the registry. To do dynamic definitions * of external ACL's, rather than a static prototype, have a Prototype instance * prototype in the class that defines each external acl 'class'. * Then, then the external acl instance is created, it self registers under * it's name. * Be sure that clone is fully functional for that acl class though! */ ACL::Prototype ACLExternal::RegistryProtoype(&ACLExternal::RegistryEntry_, "external"); ACLExternal ACLExternal::RegistryEntry_("external"); === modified file 'src/ident/AclIdent.cc' --- src/ident/AclIdent.cc 2012-01-20 18:55:04 +0000 +++ src/ident/AclIdent.cc 2012-05-23 22:01:17 +0000 @@ -72,87 +72,88 @@ void ACLIdent::parse() { if (!data) { debugs(28, 3, HERE << "current is null. Creating"); data = new ACLUserData; } data->parse(); } int ACLIdent::match(ACLChecklist *cl) { ACLFilledChecklist *checklist = Filled(cl); if (checklist->rfc931[0]) { 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); + return -1; } } wordlist * ACLIdent::dump() const { return data->dump(); } bool ACLIdent::empty () const { return data->empty(); } ACL * ACLIdent::clone() const { return new ACLIdent(*this); } IdentLookup IdentLookup::instance_; IdentLookup * IdentLookup::Instance() { return &instance_; } void IdentLookup::checkForAsync(ACLChecklist *cl)const { ACLFilledChecklist *checklist = Filled(cl); - if (checklist->conn() != NULL && Comm::IsConnOpen(checklist->conn()->clientConnection)) { + 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 IdentLookup::LookupDone(const char *ident, void *data) { ACLFilledChecklist *checklist = Filled(static_cast(data)); assert(checklist->asyncState() == IdentLookup::Instance()); if (ident) { xstrncpy(checklist->rfc931, ident, USER_IDENT_SZ); } else { xstrncpy(checklist->rfc931, dash_str, USER_IDENT_SZ); } /* * Cache the ident result in the connection, to avoid redoing ident lookup * over and over on persistent connections */ if (checklist->conn() != NULL && checklist->conn()->clientConnection != NULL && !checklist->conn()->clientConnection->rfc931[0]) xstrncpy(checklist->conn()->clientConnection->rfc931, checklist->rfc931, USER_IDENT_SZ);