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. * Better document and restrict ACLChecklist::matches() outcomes; list the ones we actually support. Assert on unsupported outcomes (for now). TODO: * Remove 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. * Rename currentAnswer() to finalAnswer(). We probably never change the "current" answer any more. * Testing, trunk port, and polishing. * Detail all patch changes. === modified file 'src/acl/Acl.cc' --- src/acl/Acl.cc 2012-04-25 05:29:20 +0000 +++ src/acl/Acl.cc 2012-05-22 19:38:43 +0000 @@ -310,45 +310,45 @@ 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); 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-22 23:34:36 +0000 @@ -25,249 +25,263 @@ * 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 +/** 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. +*/ +bool ACLChecklist::matchAclList(const ACLList * head, bool const fast) { PROF_start(aclMatchAclList); - const ACLList *node = head; finished_ = false; - while (node) { - bool nodeMatched = node->matches(this); - - if (fast) - changeState(NullState::Instance()); + for (const ACLList *node = head; node; node = node->next) { + 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 (finished()) { - PROF_stop(aclMatchAclList); - return; + if (nodeMatched) { + // matches() should return false in all special cases + assert(!needsAsync && !matchFinished); + continue; } - if (!nodeMatched || state_ != NullState::Instance()) { - - bool async = state_ != NullState::Instance(); + if (matchFinished) { + // we cannot be done and need an async call at the same time + assert(!needsAsync); + debugs(28, 3, HERE << this << " exception: " << currentAnswer()); + PROF_stop(aclMatchAclList); + return false; + } - checkForAsync(); + if (!needsAsync) { + debugs(28, 3, HERE << this << " simple mismatch"); + PROF_stop(aclMatchAclList); + 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()); - - 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; - } + /* we need an async call */ - debugs(28, 3, "aclmatchAclList: " << this << " returning (AND list entry awaiting an async lookup)"); + if (fast) { + changeState(NullState::Instance()); // disable async checks + markFinished(ACCESS_DUNNO); // cannot continue if we do not know + debugs(28, 3, HERE << this << " DUNNO because cannot async"); PROF_stop(aclMatchAclList); - return; + return false; } - node = node->next; + // Ideally, this should be inside match() itself, but that requires + // prohibiting slow ACLs in options that do not support them + // TODO: rename to startAsync() + checkForAsync(); + assert(asyncInProgress()); // async operation started + assert(!finished()); // async operation is truly asynchronous + debugs(28, 3, HERE << this << " awaiting async operation"); + PROF_stop(aclMatchAclList); + return false; } - debugs(28, 3, "aclmatchAclList: " << this << " returning true (AND list satisfied)"); - - markFinished(); + debugs(28, 3, HERE << this << " success: all ACLs matched"); PROF_stop(aclMatchAclList); + return true; } 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 +314,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_DENIED) // 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-22 23:24:27 +0000 @@ -108,85 +108,84 @@ * 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); + bool matchAclList(const ACLList * list, 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/AclMaxUserIp.cc' --- src/auth/AclMaxUserIp.cc 2012-05-14 10:37:40 +0000 +++ src/auth/AclMaxUserIp.cc 2012-05-22 23:40:08 +0000 @@ -138,41 +138,42 @@ /* * 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); + checklist->markFinished(answer); + return -1; // other; TODO: simplify the "else" code below } 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: return -1; // other } === modified file 'src/auth/AclProxyAuth.cc' --- src/auth/AclProxyAuth.cc 2012-05-14 10:37:40 +0000 +++ src/auth/AclProxyAuth.cc 2012-05-22 23:41:25 +0000 @@ -67,41 +67,42 @@ 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); + checklist->markFinished(answer); + return -1; // other; TODO: simplify the "else" code below } // 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: return -1; // other } } wordlist * ACLProxyAuth::dump() const @@ -178,43 +179,42 @@ 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(); + checklist->markFinished(ACCESS_AUTH_REQUIRED); } 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) */ === modified file 'src/external_acl.cc' --- src/external_acl.cc 2012-05-14 10:37:40 +0000 +++ src/external_acl.cc 2012-05-22 23:42:51 +0000 @@ -848,41 +848,42 @@ } } } 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); + checklist->markFinished(answer); + return -1; // other; TODO: simplify the "else" code below } // 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: return -1; // other } } wordlist * ACLExternal::dump() const { === modified file 'src/ident/AclIdent.cc' --- src/ident/AclIdent.cc 2012-01-20 18:55:04 +0000 +++ src/ident/AclIdent.cc 2012-05-22 21:11:03 +0000 @@ -115,43 +115,44 @@ } 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)) { debugs(28, 3, HERE << "Doing ident lookup" ); checklist->asyncInProgress(true); Ident::Start(checklist->conn()->clientConnection, LookupDone, checklist); } else { +// XXX: this should be checked before we go async! debugs(28, DBG_IMPORTANT, "IdentLookup::checkForAsync: Can't start ident lookup. No client connection" ); - checklist->currentAnswer(ACCESS_DENIED); - checklist->markFinished(); + checklist->changeState(ACLChecklist::NullState::Instance()); + checklist->markFinished(ACCESS_DENIED); } } 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])