From: Amos Jeffries Date: Sat, 16 Jul 2011 15:21:48 +0000 (+1200) Subject: Acess Control API cleanup X-Git-Tag: take08~55^2~61 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2efeb0b7697c0d38f9e79f25510ffe411db864f9;p=thirdparty%2Fsquid.git Acess Control API cleanup In summary: * use nonBlockingCheck() or fastCheck() to test ACLs. * be prepared to handle any allow_t in the result. ACL testing functions publicly available from ACLChecklist are: - nonBlockingCheck (public), fastCheck public), check (public but not to be used) - matchAclListFast (public), matchAclListSlow (private), matchAclList (private). Given that there are only two types of test performed, this array of API methods has been causing confusion and mistakes for some developers. This patch seeks to clarify that API by correcting a flaw in the naming of check() and matchAclListFast(). Due to "Fast" ACLs coming in two types there are two overloaded fastCheck() functions. Now with identical output behaviour. Both return the allow_t result of the lookup. This is expected to _usually_ be ACCESS_ALLOWED / ACCESS_DENIED but that is not always the case. Callers need to be written with consideration that the set of enum results may change. - fastCheck(), no parameters, when a full set of "Fast" *_access lines are to be scanned. The checklist constructor accepts the list to be scanned. This is the old fastCheck(), with the new ALLOWED / DENIED / DUNNO result. - fastCheck(list), one parameter, when a single-line set of ACLs is to be scanned. This is the old matchAclListFast(), with the new ALLOWED / DENIED / DUNNO result. Will return ALLOWED whenever the whole set of ACLs matches. Other results may vary. - nonBlockingCheck() - for "Slow" non-blocking lookups with asynchronous callback handler. NP: not touched by this patch. The output change from boolean to allow_t is due to the fastCheck() callers mixed set of needs allow/deny/other which boolean cannot meet. Mapping that tri-state need to a boolean result has led to inconsistent cases of fastCheck() producing unusual values for "true". Sometimes wrongly for the caller. Added result lookup type ACCESS_DUNNO, to indicate a test was unable to be completed BUT there was no allow/deny/auth-required resulting. Alters all previous calling code to use the new fastCheck() API output. Some have been polished up to boolean where appropriate instead of relying on integer values. Removes matchAclListFast/matchAclListSlow, Renames check() to matchNonBlocking; all match*() functions are internal operations during ACL testing. --- diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index 006b028a22..cef5ef0630 100644 --- a/src/ClientRequestContext.h +++ b/src/ClientRequestContext.h @@ -26,11 +26,11 @@ public: bool httpStateIsValid(); void clientAccessCheck(); void clientAccessCheck2(); - void clientAccessCheckDone(int answer); + void clientAccessCheckDone(const allow_t &answer); void clientRedirectStart(); void clientRedirectDone(char *result); void checkNoCache(); - void checkNoCacheDone(int answer); + void checkNoCacheDone(const allow_t &answer); #if USE_ADAPTATION void adaptationAccessCheck(); diff --git a/src/CommCalls.cc b/src/CommCalls.cc index bd7e8c4afd..cad43d5410 100644 --- a/src/CommCalls.cc +++ b/src/CommCalls.cc @@ -79,7 +79,6 @@ CommIoCbParams::syncWithComm() if (conn->fd >= 0 && fd_table[conn->fd].closing() && flag != COMM_ERR_CLOSING) { debugs(5, 3, HERE << "converting late call to COMM_ERR_CLOSING: " << conn); flag = COMM_ERR_CLOSING; - size = 0; } return true; // now we are in sync and can handle the call } diff --git a/src/DelayId.cc b/src/DelayId.cc index 3f47fd327d..08dba5dd18 100644 --- a/src/DelayId.cc +++ b/src/DelayId.cc @@ -126,7 +126,7 @@ DelayId::DelayClient(ClientHttpRequest * http) if (http->getConn() != NULL) ch.conn(http->getConn()); - if (DelayPools::delay_data[pool].theComposite().getRaw() && ch.fastCheck()) { + if (DelayPools::delay_data[pool].theComposite().getRaw() && ch.fastCheck() == ACCESS_ALLOWED) { DelayId result (pool + 1); CompositePoolNode::CompositeSelectionDetails details; diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc index b5137c8647..4612fc2563 100644 --- a/src/HttpHeaderTools.cc +++ b/src/HttpHeaderTools.cc @@ -433,7 +433,7 @@ httpHdrMangle(HttpHeaderEntry * e, HttpRequest * request, int req_or_rep) ACLFilledChecklist checklist(hm->access_list, request, NULL); - if (checklist.fastCheck()) { + if (checklist.fastCheck() == ACCESS_ALLOWED) { /* aclCheckFast returns true for allow. */ retval = 1; } else if (NULL == hm->replacement) { diff --git a/src/HttpReply.cc b/src/HttpReply.cc index bfa84dbf22..f5124f454d 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -604,7 +604,7 @@ HttpReply::calcMaxBodySize(HttpRequest& request) ch.reply = HTTPMSGLOCK(this); // XXX: this lock makes method non-const for (acl_size_t *l = Config.ReplyBodySize; l; l = l -> next) { /* if there is no ACL list or if the ACLs listed match use this size value */ - if (!l->aclList || ch.matchAclListFast(l->aclList)) { + if (!l->aclList || ch.fastCheck(l->aclList) == ACCESS_ALLOWED) { debugs(58, 4, HERE << "bodySizeMax=" << bodySizeMax); bodySizeMax = l->size; // may be -1 break; diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index c73af9c6c0..6fb9f410c6 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -666,7 +666,7 @@ HttpRequest::getRangeOffsetLimit() for (acl_size_t *l = Config.rangeOffsetLimit; l; l = l -> next) { /* if there is no ACL list or if the ACLs listed match use this limit value */ - if (!l->aclList || ch.matchAclListFast(l->aclList)) { + if (!l->aclList || ch.fastCheck(l->aclList) == ACCESS_ALLOWED) { debugs(58, 4, HERE << "rangeOffsetLimit=" << rangeOffsetLimit); rangeOffsetLimit = l->size; // may be -1 break; diff --git a/src/ICP.h b/src/ICP.h index befd800fd1..fad0356ba9 100644 --- a/src/ICP.h +++ b/src/ICP.h @@ -131,7 +131,7 @@ extern Ip::Address theIcpPublicHostID; HttpRequest* icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from); /// \ingroup ServerProtocolICPAPI -int icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request); +bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request); /// \ingroup ServerProtocolICPAPI SQUIDCEXTERN void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from); diff --git a/src/PeerSelectState.h b/src/PeerSelectState.h index b312464450..7a323ac9c0 100644 --- a/src/PeerSelectState.h +++ b/src/PeerSelectState.h @@ -33,6 +33,7 @@ #ifndef SQUID_PEERSELECTSTATE_H #define SQUID_PEERSELECTSTATE_H +#include "acl/Checklist.h" #include "Array.h" #include "cbdata.h" #include "comm/forward.h" @@ -73,9 +74,9 @@ public: ps_state(); HttpRequest *request; StoreEntry *entry; - int always_direct; - int never_direct; - int direct; + allow_t always_direct; + allow_t never_direct; + int direct; // TODO: fold always_direct/never_direct/prefer_direct into this now that ACL can do a multi-state result. PSC *callback; void *callback_data; diff --git a/src/acl/Acl.h b/src/acl/Acl.h index cd5834aa33..9f10b32f6c 100644 --- a/src/acl/Acl.h +++ b/src/acl/Acl.h @@ -107,6 +107,7 @@ public: typedef enum { ACCESS_DENIED, ACCESS_ALLOWED, + ACCESS_DUNNO, ACCESS_REQ_PROXY_AUTH } allow_t; diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc index 99310c2d75..b558f18eb9 100644 --- a/src/acl/Checklist.cc +++ b/src/acl/Checklist.cc @@ -49,7 +49,7 @@ ACLChecklist::currentAnswer(allow_t const newAnswer) } void -ACLChecklist::check() +ACLChecklist::matchNonBlocking() { if (checking()) return; @@ -169,7 +169,7 @@ ACLChecklist::checkAccessList() { preCheck(); /* does the current AND clause match */ - matchAclListSlow(accessList->aclList); + matchAclList(accessList->aclList, false); } void @@ -183,7 +183,7 @@ ACLChecklist::checkForAsync() void ACLChecklist::checkCallback(allow_t answer) { - PF *callback_; + ACLCB *callback_; void *cbdata_; debugs(28, 3, "ACLChecklist::checkCallback: " << this << " answer=" << answer); @@ -196,12 +196,6 @@ ACLChecklist::checkCallback(allow_t answer) delete this; } -void -ACLChecklist::matchAclListSlow(const ACLList * list) -{ - matchAclList(list, false); -} - void ACLChecklist::matchAclList(const ACLList * head, bool const fast) { @@ -324,29 +318,44 @@ ACLChecklist::asyncState() const * NP: this should probably be made Async now. */ void -ACLChecklist::nonBlockingCheck(PF * callback_, void *callback_data_) +ACLChecklist::nonBlockingCheck(ACLCB * callback_, void *callback_data_) { callback = callback_; callback_data = cbdataReference(callback_data_); - check(); + 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); + PROF_stop(aclCheckFast); + return currentAnswer(); } /* Warning: do not cbdata lock this here - it * may be static or on the stack */ -int +allow_t const & ACLChecklist::fastCheck() { PROF_start(aclCheckFast); - currentAnswer(ACCESS_DENIED); + currentAnswer(ACCESS_DUNNO); + debugs(28, 5, "aclCheckFast: list: " << accessList); const acl_access *acl = cbdataReference(accessList); while (acl != NULL && cbdataReferenceValid(acl)) { currentAnswer(acl->allow); - if (matchAclListFast(acl->aclList)) { + matchAclList(acl->aclList, true); + if (finished()) { PROF_stop(aclCheckFast); cbdataReferenceDone(acl); - return currentAnswer() == ACCESS_ALLOWED; + return currentAnswer(); } /* @@ -357,10 +366,10 @@ ACLChecklist::fastCheck() cbdataReferenceDone(A); } - debugs(28, 5, "aclCheckFast: no matches, returning: " << (currentAnswer() == ACCESS_DENIED)); - + debugs(28, 5, "aclCheckFast: no matches, returning: " << currentAnswer()); PROF_stop(aclCheckFast); - return currentAnswer() == ACCESS_DENIED; + + return currentAnswer(); } @@ -381,12 +390,3 @@ ACLChecklist::callerGone() { return !cbdataReferenceValid(callback_data); } - -bool -ACLChecklist::matchAclListFast(const ACLList * list) -{ - matchAclList(list, true); - return finished(); -} - - diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 8b5702b511..33b81e0141 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -35,6 +35,9 @@ #include "acl/Acl.h" +/// ACL checklist callback +typedef void ACLCB(allow_t, void *); + /** \ingroup ACLAPI Base class for maintaining Squid and transaction state for access checks. Provides basic ACL checking methods. Its only child, ACLFilledChecklist, @@ -93,7 +96,7 @@ public: * The callback specified will be called with true/false * when the results of the ACL tests are known. */ - void nonBlockingCheck(PF * callback, void *callback_data); + void nonBlockingCheck(ACLCB * callback, void *callback_data); /** * Trigger a blocking access check for a set of *_access options. @@ -107,34 +110,20 @@ public: * 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 1/true Access Allowed - * \retval 0/false Access Denied + * \retval ACCESS_DUNNO Unable to determine any result + * \retval ACCESS_ALLOWED Access Allowed + * \retval ACCESS_DENIED Access Denied */ - int fastCheck(); + allow_t const & fastCheck(); /** - * Trigger a blocking access check for a single ACL line (a AND b AND c). + * 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. * - * 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. - * - * \retval 1/true Access Allowed - * \retval 0/false Access Denied + * \retval ACCESS_DUNNO Unable to determine any result + * \retval ACCESS_ALLOWED ACLs all matched */ - bool matchAclListFast(const ACLList * list); - - /** - * 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 check(); + allow_t const & fastCheck(const ACLList * list); bool asyncInProgress() const; void asyncInProgress(bool const); @@ -163,13 +152,24 @@ private: public: const acl_access *accessList; - PF *callback; + 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); - void matchAclListSlow(const ACLList * list); bool async_; bool finished_; diff --git a/src/acl/DestinationDomain.cc b/src/acl/DestinationDomain.cc index 6527deb1bd..c5836db5aa 100644 --- a/src/acl/DestinationDomain.cc +++ b/src/acl/DestinationDomain.cc @@ -68,7 +68,7 @@ DestinationDomainLookup::LookupDone(const char *fqdn, const DnsLookupDetails &de checklist->changeState (ACLChecklist::NullState::Instance()); checklist->markDestinationDomainChecked(); checklist->request->recordLookup(details); - checklist->check(); + checklist->matchNonBlocking(); } diff --git a/src/acl/DestinationIp.cc b/src/acl/DestinationIp.cc index 39d3bf9fc6..35d3b8186e 100644 --- a/src/acl/DestinationIp.cc +++ b/src/acl/DestinationIp.cc @@ -94,7 +94,7 @@ DestinationIPLookup::LookupDone(const ipcache_addrs *, const DnsLookupDetails &d checklist->request->recordLookup(details); checklist->asyncInProgress(false); checklist->changeState (ACLChecklist::NullState::Instance()); - checklist->check(); + checklist->matchNonBlocking(); } diff --git a/src/acl/Gadgets.cc b/src/acl/Gadgets.cc index 692285262f..ec11433f5d 100644 --- a/src/acl/Gadgets.cc +++ b/src/acl/Gadgets.cc @@ -196,7 +196,7 @@ aclParseAccessLine(ConfigParser &parser, acl_access ** head) for (B = *head, T = head; B; T = &B->next, B = B->next); *T = A; - /* We lock _acl_access structures in ACLChecklist::check() */ + /* We lock _acl_access structures in ACLChecklist::matchNonBlocking() */ } void diff --git a/src/acl/SourceDomain.cc b/src/acl/SourceDomain.cc index 44e71c6c63..322c4f7efc 100644 --- a/src/acl/SourceDomain.cc +++ b/src/acl/SourceDomain.cc @@ -66,7 +66,7 @@ SourceDomainLookup::LookupDone(const char *fqdn, const DnsLookupDetails &details checklist->changeState (ACLChecklist::NullState::Instance()); checklist->markSourceDomainChecked(); checklist->request->recordLookup(details); - checklist->check(); + checklist->matchNonBlocking(); } diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index d63199e01f..088914204f 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -139,7 +139,7 @@ Adaptation::AccessCheck::checkCandidates() } void -Adaptation::AccessCheck::AccessCheckCallbackWrapper(int answer, void *data) +Adaptation::AccessCheck::AccessCheckCallbackWrapper(allow_t answer, void *data) { debugs(93, 8, HERE << "callback answer=" << answer); AccessCheck *ac = (AccessCheck*)data; @@ -150,23 +150,22 @@ Adaptation::AccessCheck::AccessCheckCallbackWrapper(int answer, void *data) */ // convert to async call to get async call protections and features - typedef UnaryMemFunT MyDialer; + typedef UnaryMemFunT MyDialer; AsyncCall::Pointer call = asyncCall(93,7, "Adaptation::AccessCheck::noteAnswer", - MyDialer(ac, &Adaptation::AccessCheck::noteAnswer, - answer==ACCESS_ALLOWED)); + MyDialer(ac, &Adaptation::AccessCheck::noteAnswer, answer)); ScheduleCallHere(call); } /// process the results of the ACL check void -Adaptation::AccessCheck::noteAnswer(int answer) +Adaptation::AccessCheck::noteAnswer(allow_t answer) { Must(!candidates.empty()); // the candidate we were checking must be there debugs(93,5, HERE << topCandidate() << " answer=" << answer); - if (answer) { // the rule matched + if (answer == ACCESS_ALLOWED) { // the rule matched ServiceGroupPointer g = topGroup(); if (g != NULL) { // the corresponding group found callBack(g); diff --git a/src/adaptation/AccessCheck.h b/src/adaptation/AccessCheck.h index e0e4793daf..11097a6f6b 100644 --- a/src/adaptation/AccessCheck.h +++ b/src/adaptation/AccessCheck.h @@ -1,6 +1,7 @@ #ifndef SQUID_ADAPTATION__ACCESS_CHECK_H #define SQUID_ADAPTATION__ACCESS_CHECK_H +#include "acl/Acl.h" #include "base/AsyncJob.h" #include "adaptation/Elements.h" #include "adaptation/forward.h" @@ -47,8 +48,8 @@ private: public: void checkCandidates(); - static void AccessCheckCallbackWrapper(int, void*); - void noteAnswer(int answer); + static void AccessCheckCallbackWrapper(allow_t, void*); + void noteAnswer(allow_t answer); protected: // AsyncJob API diff --git a/src/adaptation/icap/Launcher.cc b/src/adaptation/icap/Launcher.cc index 18a094edb5..bf5482ddcf 100644 --- a/src/adaptation/icap/Launcher.cc +++ b/src/adaptation/icap/Launcher.cc @@ -136,7 +136,7 @@ bool Adaptation::Icap::Launcher::canRepeat(Adaptation::Icap::XactAbortInfo &info new ACLFilledChecklist(TheConfig.repeat, info.icapRequest, dash_str); cl->reply = HTTPMSGLOCK(info.icapReply); - const bool result = cl->fastCheck(); + bool result = cl->fastCheck() == ACCESS_ALLOWED; delete cl; return result; } diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index a719326fa6..883d4c2fb9 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -534,7 +534,7 @@ void Adaptation::Icap::Xaction::maybeLog() { if (IcapLogfileStatus == LOG_ENABLE) { ACLChecklist *checklist = new ACLFilledChecklist(::Config.accessList.icap, al.request, dash_str); - if (!::Config.accessList.icap || checklist->fastCheck()) { + if (!::Config.accessList.icap || checklist->fastCheck() == ACCESS_ALLOWED) { finalizeLogInfo(); icapLogLog(&al, checklist); } diff --git a/src/auth/AclProxyAuth.cc b/src/auth/AclProxyAuth.cc index 38ce3a195b..f5dc92f559 100644 --- a/src/auth/AclProxyAuth.cc +++ b/src/auth/AclProxyAuth.cc @@ -170,7 +170,7 @@ ProxyAuthLookup::LookupDone(void *data, char *result) checklist->asyncInProgress(false); checklist->changeState (ACLChecklist::NullState::Instance()); - checklist->check(); + checklist->matchNonBlocking(); } void diff --git a/src/client_side.cc b/src/client_side.cc index 85d545befb..8c41af18ca 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -672,7 +672,7 @@ ClientHttpRequest::logRequest() if (al.reply) checklist->reply = HTTPMSGLOCK(al.reply); - if (!Config.accessList.log || checklist->fastCheck()) { + if (!Config.accessList.log || checklist->fastCheck() == ACCESS_ALLOWED) { if (request) al.adapted_request = HTTPMSGLOCK(request); accessLogLog(&al, checklist); @@ -3128,7 +3128,7 @@ connStateCreate(const Comm::ConnectionPointer &client, http_port_list *port) ACLFilledChecklist identChecklist(Ident::TheConfig.identLookup, NULL, NULL); identChecklist.src_addr = client->remote; identChecklist.my_addr = client->local; - if (identChecklist.fastCheck()) + if (identChecklist.fastCheck() == ACCESS_ALLOWED) Ident::Start(client, clientIdentDone, result); } #endif @@ -3187,35 +3187,38 @@ httpAccept(int, const Comm::ConnectionPointer &details, comm_err_t flag, int xer /* it was said several times that client write limiter does not work if client_db is disabled */ ClientDelayPools& pools(Config.ClientDelay.pools); - for (unsigned int pool = 0; pool < pools.size(); pool++) { - - /* pools require explicit 'allow' to assign a client into them */ - if (!pools[pool].access) - continue; // warned in ClientDelayConfig::Finalize() - - ACLFilledChecklist ch(pools[pool].access, NULL, NULL); - - // TODO: we check early to limit error response bandwith but we - // should recheck when we can honor delay_pool_uses_indirect - - ch.src_addr = details->remote; - ch.my_addr = details->local; + ACLFilledChecklist ch(NULL, NULL, NULL); - if (ch.fastCheck()) { + // TODO: we check early to limit error response bandwith but we + // should recheck when we can honor delay_pool_uses_indirect + // TODO: we should also pass the port details for myportname here. + ch.src_addr = details->remote; + ch.my_addr = details->local; - /* request client information from db after we did all checks - this will save hash lookup if client failed checks */ - ClientInfo * cli = clientdbGetInfo(details->remote); - assert(cli); - - /* put client info in FDE */ - fd_table[details->fd].clientInfo = cli; + for (unsigned int pool = 0; pool < pools.size(); pool++) { - /* setup write limiter for this request */ - const double burst = floor(0.5 + - (pools[pool].highwatermark * Config.ClientDelay.initial)/100.0); - cli->setWriteLimiter(pools[pool].rate, burst, pools[pool].highwatermark); - break; + /* pools require explicit 'allow' to assign a client into them */ + if (pools[pool].access) { + ch.accessList = pools[pool].access; + allow_t answer = ch.fastCheck(); + if (answer == ACCESS_ALLOWED) { + + /* request client information from db after we did all checks + this will save hash lookup if client failed checks */ + ClientInfo * cli = clientdbGetInfo(details->remote); + assert(cli); + + /* put client info in FDE */ + fd_table[details->fd].clientInfo = cli; + + /* setup write limiter for this request */ + const double burst = floor(0.5 + + (pools[pool].highwatermark * Config.ClientDelay.initial)/100.0); + cli->setWriteLimiter(pools[pool].rate, burst, pools[pool].highwatermark); + break; + } else { + debugs(83, 4, HERE << "Delay pool " << pool << " skipped because ACL " << answer); + } } } } diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 8412a01334..a69864dd9e 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1920,7 +1920,7 @@ clientReplyContext::processReplyAccess () http->logType == LOG_TCP_DENIED_REPLY || alwaysAllowResponse(reply->sline.status)) { headers_sz = reply->hdr_sz; - processReplyAccessResult(1); + processReplyAccessResult(ACCESS_ALLOWED); return; } @@ -1934,7 +1934,7 @@ clientReplyContext::processReplyAccess () /** check for absent access controls (permit by default) */ if (!Config.accessList.reply) { - processReplyAccessResult(1); + processReplyAccessResult(ACCESS_ALLOWED); return; } @@ -1946,22 +1946,20 @@ clientReplyContext::processReplyAccess () } void -clientReplyContext::ProcessReplyAccessResult (int rv, void *voidMe) +clientReplyContext::ProcessReplyAccessResult(allow_t rv, void *voidMe) { clientReplyContext *me = static_cast(voidMe); me->processReplyAccessResult(rv); } void -clientReplyContext::processReplyAccessResult(bool accessAllowed) +clientReplyContext::processReplyAccessResult(const allow_t &accessAllowed) { debugs(88, 2, "The reply for " << RequestMethodStr(http->request->method) - << " " << http->uri << " is " - << ( accessAllowed ? "ALLOWED" : "DENIED") - << ", because it matched '" + << " " << http->uri << " is " << accessAllowed << ", because it matched '" << (AclMatchedName ? AclMatchedName : "NO ACL's") << "'" ); - if (!accessAllowed) { + if (accessAllowed != ACCESS_ALLOWED) { ErrorState *err; err_type page_id; page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); diff --git a/src/client_side_reply.h b/src/client_side_reply.h index 6ff0dd920a..c7e2b34be2 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -125,8 +125,8 @@ private: StoreIOBuffer holdingBuffer; HttpReply *reply; void processReplyAccess(); - static PF ProcessReplyAccessResult; - void processReplyAccessResult(bool accessAllowed); + static ACLCB ProcessReplyAccessResult; + void processReplyAccessResult(const allow_t &accessAllowed); void cloneReply(); void buildReplyHeader (); bool alwaysAllowResponse(http_status sline) const; diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 0aee4f10fc..2139479107 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -88,8 +88,7 @@ static const char *const crlf = "\r\n"; #if FOLLOW_X_FORWARDED_FOR -static void -clientFollowXForwardedForCheck(int answer, void *data); +static void clientFollowXForwardedForCheck(allow_t answer, void *data); #endif /* FOLLOW_X_FORWARDED_FOR */ CBDATA_CLASS_INIT(ClientRequestContext); @@ -112,14 +111,14 @@ ClientRequestContext::operator delete (void *address) /* Local functions */ /* other */ -static void clientAccessCheckDoneWrapper(int, void *); +static void clientAccessCheckDoneWrapper(allow_t, void *); #if USE_SSL -static void sslBumpAccessCheckDoneWrapper(int, void *); +static void sslBumpAccessCheckDoneWrapper(allow_t, void *); #endif static int clientHierarchical(ClientHttpRequest * http); static void clientInterpretRequestHeaders(ClientHttpRequest * http); static RH clientRedirectDoneWrapper; -static PF checkNoCacheDoneWrapper; +static void checkNoCacheDoneWrapper(allow_t, void *); extern "C" CSR clientGetMoreData; extern "C" CSS clientReplyStatus; extern "C" CSD clientReplyDetach; @@ -438,7 +437,7 @@ ClientRequestContext::httpStateIsValid() * ++ indirect_client_addr contains the remote direct client from the trusted peers viewpoint. */ static void -clientFollowXForwardedForCheck(int answer, void *data) +clientFollowXForwardedForCheck(allow_t answer, void *data) { ClientRequestContext *calloutContext = (ClientRequestContext *) data; @@ -570,7 +569,7 @@ ClientRequestContext::clientAccessCheck2() } void -clientAccessCheckDoneWrapper(int answer, void *data) +clientAccessCheckDoneWrapper(allow_t answer, void *data) { ClientRequestContext *calloutContext = (ClientRequestContext *) data; @@ -581,15 +580,14 @@ clientAccessCheckDoneWrapper(int answer, void *data) } void -ClientRequestContext::clientAccessCheckDone(int answer) +ClientRequestContext::clientAccessCheckDone(const allow_t &answer) { acl_checklist = NULL; err_type page_id; http_status status; debugs(85, 2, "The request " << RequestMethodStr(http->request->method) << " " << - http->uri << " is " << - (answer == ACCESS_ALLOWED ? "ALLOWED" : "DENIED") << + http->uri << " is " << answer << ", because it matched '" << (AclMatchedName ? AclMatchedName : "NO ACL's") << "'" ); @@ -717,7 +715,7 @@ ClientRequestContext::adaptationAclCheckDone(Adaptation::ServiceGroupPointer g) #endif static void -clientRedirectAccessCheckDone(int answer, void *data) +clientRedirectAccessCheckDone(allow_t answer, void *data) { ClientRequestContext *context = (ClientRequestContext *)data; ClientHttpRequest *http = context->http; @@ -1096,12 +1094,12 @@ ClientRequestContext::checkNoCache() acl_checklist->nonBlockingCheck(checkNoCacheDoneWrapper, this); } else { /* unless otherwise specified, we try to cache. */ - checkNoCacheDone(1); + checkNoCacheDone(ACCESS_ALLOWED); } } static void -checkNoCacheDoneWrapper(int answer, void *data) +checkNoCacheDoneWrapper(allow_t answer, void *data) { ClientRequestContext *calloutContext = (ClientRequestContext *) data; @@ -1112,10 +1110,10 @@ checkNoCacheDoneWrapper(int answer, void *data) } void -ClientRequestContext::checkNoCacheDone(int answer) +ClientRequestContext::checkNoCacheDone(const allow_t &answer) { acl_checklist = NULL; - http->request->flags.cachable = answer; + http->request->flags.cachable = (answer == ACCESS_ALLOWED); http->doCallouts(); } @@ -1141,7 +1139,7 @@ ClientRequestContext::sslBumpAccessCheck() * as ACLFilledChecklist callback */ static void -sslBumpAccessCheckDoneWrapper(int answer, void *data) +sslBumpAccessCheckDoneWrapper(allow_t answer, void *data) { ClientRequestContext *calloutContext = static_cast(data); diff --git a/src/external_acl.cc b/src/external_acl.cc index 9e962acf95..042293c49b 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -1562,7 +1562,7 @@ ExternalACLLookup::LookupDone(void *data, void *result) checklist->extacl_entry = cbdataReference((external_acl_entry *)result); checklist->asyncInProgress(false); checklist->changeState (ACLChecklist::NullState::Instance()); - checklist->check(); + checklist->matchNonBlocking(); } /* This registers "external" in the registry. To do dynamic definitions diff --git a/src/forward.cc b/src/forward.cc index 8e71279c02..53be5cb97c 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -209,9 +209,7 @@ FwdState::fwdStart(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, ACLFilledChecklist ch(Config.accessList.miss, request, NULL); ch.src_addr = request->client_addr; ch.my_addr = request->my_addr; - int answer = ch.fastCheck(); - - if (answer == 0) { + if (ch.fastCheck() == ACCESS_DENIED) { err_type page_id; page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); @@ -804,7 +802,9 @@ FwdState::connectStart() return; } - request->flags.pinned = 0; + request->flags.pinned = 0; // XXX: what if the ConnStateData set this to flag existing credentials? + // XXX: answer: the peer selection *should* catch it and give us only the pinned peer. so we reverse the =0 step below. + // XXX: also, logs will now lie if pinning is broken and leads to an error message. if (serverDestinations[0]->peerType == PINNED) { ConnStateData *pinned_connection = request->pinnedConnection(); assert(pinned_connection); @@ -1212,7 +1212,7 @@ aclMapTOS(acl_tos * head, ACLChecklist * ch) acl_tos *l; for (l = head; l; l = l->next) { - if (!l->aclList || ch->matchAclListFast(l->aclList)) + if (!l->aclList || ch->fastCheck(l->aclList) == ACCESS_ALLOWED) return l->tos; } @@ -1226,7 +1226,7 @@ aclMapNfmark(acl_nfmark * head, ACLChecklist * ch) acl_nfmark *l; for (l = head; l; l = l->next) { - if (!l->aclList || ch->matchAclListFast(l->aclList)) + if (!l->aclList || ch->fastCheck(l->aclList) == ACCESS_ALLOWED) return l->nfmark; } @@ -1284,7 +1284,7 @@ getOutgoingAddress(HttpRequest * request, Comm::ConnectionPointer conn) if (conn->remote.IsIPv4() != l->addr.IsIPv4()) continue; /* check ACLs for this outgoing address */ - if (!l->aclList || ch.matchAclListFast(l->aclList)) { + if (!l->aclList || ch.fastCheck(l->aclList) == ACCESS_ALLOWED) { conn->local = l->addr; return; } diff --git a/src/htcp.cc b/src/htcp.cc index af8d649059..96c67fc1a7 100644 --- a/src/htcp.cc +++ b/src/htcp.cc @@ -848,18 +848,17 @@ htcpUnpackDetail(char *buf, int sz) return d; } -static int -htcpAccessCheck(acl_access * acl, htcpSpecifier * s, Ip::Address &from) +static bool +htcpAccessAllowed(acl_access * acl, htcpSpecifier * s, Ip::Address &from) { /* default deny if no access list present */ if (!acl) - return 0; + return false; ACLFilledChecklist checklist(acl, s->request, NULL); checklist.src_addr = from; checklist.my_addr.SetNoAddr(); - int result = checklist.fastCheck(); - return result; + return (checklist.fastCheck() == ACCESS_ALLOWED); } static void @@ -1206,7 +1205,7 @@ htcpHandleTstRequest(htcpDataHeader * dhdr, char *buf, int sz, Ip::Address &from return; } - if (!htcpAccessCheck(Config.accessList.htcp, s, from)) { + if (!htcpAccessAllowed(Config.accessList.htcp, s, from)) { debugs(31, 2, "htcpHandleTstRequest: Access denied"); htcpLogHtcp(from, dhdr->opcode, LOG_UDP_DENIED, s->uri); htcpFreeSpecifier(s); @@ -1279,7 +1278,7 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from) return; } - if (!htcpAccessCheck(Config.accessList.htcp_clr, s, from)) { + if (!htcpAccessAllowed(Config.accessList.htcp_clr, s, from)) { debugs(31, 2, "htcpHandleClr: Access denied"); htcpLogHtcp(from, hdr->opcode, LOG_UDP_DENIED, s->uri); htcpFreeSpecifier(s); diff --git a/src/icp_v2.cc b/src/icp_v2.cc index d62baea373..4e4cf2f62b 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -445,18 +445,17 @@ icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) } } -int +bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) { /* absent an explicit allow, we deny all */ if (!Config.accessList.icp) - return 0; + return true; ACLFilledChecklist checklist(Config.accessList.icp, icp_request, NULL); checklist.src_addr = from; checklist.my_addr.SetNoAddr(); - int result = checklist.fastCheck(); - return result; + return (checklist.fastCheck() == ACCESS_ALLOWED); } char const * diff --git a/src/ident/AclIdent.cc b/src/ident/AclIdent.cc index c5ddb176d0..47a6885ece 100644 --- a/src/ident/AclIdent.cc +++ b/src/ident/AclIdent.cc @@ -159,7 +159,7 @@ IdentLookup::LookupDone(const char *ident, void *data) checklist->asyncInProgress(false); checklist->changeState(ACLChecklist::NullState::Instance()); - checklist->check(); + checklist->matchNonBlocking(); } #endif /* USE_IDENT */ diff --git a/src/log/access_log.cc b/src/log/access_log.cc index 9869232f83..e2f75ddffb 100644 --- a/src/log/access_log.cc +++ b/src/log/access_log.cc @@ -111,7 +111,7 @@ accessLogLogTo(customlog* log, AccessLogEntry * al, ACLChecklist * checklist) xstrncpy(al->hier.host, dash_str, SQUIDHOSTNAMELEN); for (; log; log = log->next) { - if (checklist && log->aclList && !checklist->matchAclListFast(log->aclList)) + if (log->aclList && checklist && checklist->fastCheck(log->aclList) != ACCESS_ALLOWED) continue; if (log->logfile) { diff --git a/src/neighbors.cc b/src/neighbors.cc index a4cf9db51c..eaea4f5f18 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -55,7 +55,7 @@ /* count mcast group peers every 15 minutes */ #define MCAST_COUNT_RATE 900 -int peerAllowedToUse(const peer *, HttpRequest *); +bool peerAllowedToUse(const peer *, HttpRequest *); static int peerWouldBePinged(const peer *, HttpRequest *); static void neighborRemove(peer *); static void neighborAlive(peer *, const MemObject *, const icp_common_t *); @@ -138,18 +138,14 @@ neighborType(const peer * p, const HttpRequest * request) return p->type; } -/* - * peerAllowedToUse - * - * this function figures out if it is appropriate to fetch REQUEST - * from PEER. +/** + * \return Whether it is appropriate to fetch REQUEST from PEER. */ -int +bool peerAllowedToUse(const peer * p, HttpRequest * request) { const struct _domain_ping *d = NULL; - int do_ping = 1; assert(request != NULL); if (neighborType(p, request) == PEER_SIBLING) { @@ -159,28 +155,27 @@ peerAllowedToUse(const peer * p, HttpRequest * request) debugs(15, 2, "peerAllowedToUse(" << p->name << ", " << request->GetHost() << ") : multicast-siblings optimization match"); #endif if (request->flags.nocache) - return 0; + return false; if (request->flags.refresh) - return 0; + return false; if (request->flags.loopdetect) - return 0; + return false; if (request->flags.need_validation) - return 0; + return false; } // CONNECT requests are proxy requests. Not to be forwarded to origin servers. // Unless the destination port matches, in which case we MAY perform a 'DIRECT' to this peer. if (p->options.originserver && request->method == METHOD_CONNECT && request->port != p->in_addr.GetPort()) - return 0; + return false; if (p->peer_domain == NULL && p->access == NULL) - return do_ping; - - do_ping = 0; + return true; + bool do_ping = false; for (d = p->peer_domain; d; d = d->next) { if (0 == matchDomainName(request->GetHost(), d->domain)) { do_ping = d->do_ping; @@ -190,8 +185,8 @@ peerAllowedToUse(const peer * p, HttpRequest * request) do_ping = !d->do_ping; } - if (p->peer_domain && 0 == do_ping) - return do_ping; + if (p->peer_domain && !do_ping) + return false; if (p->access == NULL) return do_ping; @@ -211,7 +206,7 @@ peerAllowedToUse(const peer * p, HttpRequest * request) #endif - return checklist.fastCheck(); + return (checklist.fastCheck() == ACCESS_ALLOWED); } /* Return TRUE if it is okay to send an ICP request to this peer. */ diff --git a/src/peer_select.cc b/src/peer_select.cc index 9e0145d852..ea8255bb4b 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -171,22 +171,22 @@ peerSelect(Comm::ConnectionList * paths, } static void -peerCheckNeverDirectDone(int answer, void *data) +peerCheckNeverDirectDone(allow_t answer, void *data) { ps_state *psstate = (ps_state *) data; psstate->acl_checklist = NULL; debugs(44, 3, "peerCheckNeverDirectDone: " << answer); - psstate->never_direct = answer ? 1 : -1; + psstate->never_direct = answer; peerSelectFoo(psstate); } static void -peerCheckAlwaysDirectDone(int answer, void *data) +peerCheckAlwaysDirectDone(allow_t answer, void *data) { ps_state *psstate = (ps_state *)data; psstate->acl_checklist = NULL; debugs(44, 3, "peerCheckAlwaysDirectDone: " << answer); - psstate->always_direct = answer ? 1 : -1; + psstate->always_direct = answer; peerSelectFoo(psstate); } @@ -346,7 +346,7 @@ peerSelectFoo(ps_state * ps) /** If we don't known whether DIRECT is permitted ... */ if (ps->direct == DIRECT_UNKNOWN) { - if (ps->always_direct == 0 && Config.accessList.AlwaysDirect) { + if (ps->always_direct == ACCESS_DUNNO && Config.accessList.AlwaysDirect) { /** check always_direct; */ ps->acl_checklist = new ACLFilledChecklist( Config.accessList.AlwaysDirect, @@ -354,10 +354,10 @@ peerSelectFoo(ps_state * ps) NULL); /* ident */ ps->acl_checklist->nonBlockingCheck(peerCheckAlwaysDirectDone, ps); return; - } else if (ps->always_direct > 0) { + } else if (ps->always_direct == ACCESS_ALLOWED) { /** if always_direct says YES, do that. */ ps->direct = DIRECT_YES; - } else if (ps->never_direct == 0 && Config.accessList.NeverDirect) { + } else if (ps->never_direct == ACCESS_DUNNO && Config.accessList.NeverDirect) { /** check never_direct; */ ps->acl_checklist = new ACLFilledChecklist( Config.accessList.NeverDirect, @@ -366,7 +366,7 @@ peerSelectFoo(ps_state * ps) ps->acl_checklist->nonBlockingCheck(peerCheckNeverDirectDone, ps); return; - } else if (ps->never_direct > 0) { + } else if (ps->never_direct == ACCESS_ALLOWED) { /** if always_direct says NO, do that. */ ps->direct = DIRECT_NO; } else if (request->flags.no_direct) { @@ -427,7 +427,7 @@ peerSelectFoo(ps_state * ps) peerSelectDnsPaths(ps); } -int peerAllowedToUse(const peer * p, HttpRequest * request); +bool peerAllowedToUse(const peer * p, HttpRequest * request); /** * peerSelectPinned @@ -867,8 +867,8 @@ ps_state::operator new(size_t) ps_state::ps_state() : request (NULL), entry (NULL), - always_direct (0), - never_direct (0), + always_direct(ACCESS_DUNNO), + never_direct(ACCESS_DUNNO), direct (0), callback (NULL), callback_data (NULL), diff --git a/src/snmp_core.cc b/src/snmp_core.cc index 6c92a749d6..7f2b8d4204 100644 --- a/src/snmp_core.cc +++ b/src/snmp_core.cc @@ -444,7 +444,12 @@ snmpDecodePacket(snmp_request_t * rq) u_char *Community; u_char *buf = rq->buf; int len = rq->len; - int allow = 0; + allow_t allow = ACCESS_DENIED; + + if (!Config.accessList.snmp) { + debugs(49, DBG_IMPORTANT, "WARNING: snmp_access not configured. agent query DENIED from : " << rq->from); + return; + } debugs(49, 5, HERE << "Called."); PDU = snmp_pdu_create(0); @@ -454,25 +459,26 @@ snmpDecodePacket(snmp_request_t * rq) /* Check if we have explicit permission to access SNMP data. * default (set above) is to deny all */ - if (Community && Config.accessList.snmp) { + if (Community) { ACLFilledChecklist checklist(Config.accessList.snmp, NULL, NULL); checklist.src_addr = rq->from; checklist.snmp_community = (char *) Community; allow = checklist.fastCheck(); - } - if ((snmp_coexist_V2toV1(PDU)) && (Community) && (allow)) { - rq->community = Community; - rq->PDU = PDU; - debugs(49, 5, "snmpAgentParse: reqid=[" << PDU->reqid << "]"); - snmpConstructReponse(rq); + if (allow == ACCESS_ALLOWED && (snmp_coexist_V2toV1(PDU))) { + rq->community = Community; + rq->PDU = PDU; + debugs(49, 5, "snmpAgentParse: reqid=[" << PDU->reqid << "]"); + snmpConstructReponse(rq); + } else { + debugs(49, DBG_IMPORTANT, "WARNING: SNMP agent query DENIED from : " << rq->from); + } + xfree(Community); + } else { - debugs(49, 1, HERE << "Failed SNMP agent query from : " << rq->from); + debugs(49, DBG_IMPORTANT, "WARNING: Failed SNMP agent query from : " << rq->from); snmp_free_pdu(PDU); } - - if (Community) - xfree(Community); } /* diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 07a135d9b4..17191716c0 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -240,7 +240,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (check) { Filled(check)->ssl_error = error_no; - if (check->fastCheck()) { + if (check->fastCheck() == ACCESS_ALLOWED) { debugs(83, 3, "bypassing SSL error " << error_no << " in " << buffer); ok = 1; } else { diff --git a/src/tests/stub_icp.cc b/src/tests/stub_icp.cc index 3fa18773c9..5f4709fb0f 100644 --- a/src/tests/stub_icp.cc +++ b/src/tests/stub_icp.cc @@ -22,7 +22,7 @@ Comm::ConnectionPointer icpOutgoingConn; Ip::Address theIcpPublicHostID; HttpRequest* icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from) STUB_RETVAL(NULL) -int icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) STUB_RETVAL(0) +bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) STUB_RETVAL(false) void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from) STUB icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID) int icpUdpSend(int, const Ip::Address &, icp_common_t *, log_type, int) STUB_RETVAL(0) diff --git a/src/tunnel.cc b/src/tunnel.cc index 0eac0eda0d..2f1f6697e1 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -617,7 +617,6 @@ tunnelStart(ClientHttpRequest * http, int64_t * size_ptr, int *status_ptr) /* Create state structure. */ TunnelStateData *tunnelState = NULL; ErrorState *err = NULL; - int answer; HttpRequest *request = http->request; char *url = http->uri; @@ -635,9 +634,7 @@ tunnelStart(ClientHttpRequest * http, int64_t * size_ptr, int *status_ptr) ACLFilledChecklist ch(Config.accessList.miss, request, NULL); ch.src_addr = request->client_addr; ch.my_addr = request->my_addr; - answer = ch.fastCheck(); - - if (answer == 0) { + if (ch.fastCheck() == ACCESS_DENIED) { debugs(26, 4, HERE << "MISS access forbidden."); err = errorCon(ERR_FORWARDING_DENIED, HTTP_FORBIDDEN, request); *status_ptr = HTTP_FORBIDDEN;