From: Eduard Bagdasaryan Date: Fri, 11 May 2018 08:32:06 +0000 (+0000) Subject: Bug 4811: supply AccessLogEntry (ALE) for more fast ACL checks. (#182) X-Git-Tag: SQUID_4_0_25~7 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=96198f3a8b3b257c313b5ce6a0cc9dea1926c070;p=thirdparty%2Fsquid.git Bug 4811: supply AccessLogEntry (ALE) for more fast ACL checks. (#182) Supplying ALE for fast ACL checks allows those checks to use ACLs that assemble values from logformat %codes. Today, such ACLs are limited to misplaced external ACLs (that should not be used with "fast" directives!), but it is likely that fast ACLs like annotate_client will eventually require ALE. The "has" ACL documentation promises ALE for every transaction, but our code does not deliver on that promise. This change fixes a dozen of easy cases where ALE was available nearby. Also a non-trivial cache_peer_access case was fixed, which proved to be more complex because of the significant call depth of the peerAllowedToUse() check, which is a known design problem of its own. More cases need fixing, and the whole concept of ALE probably needs to be revised because logformat %code expansion is needed in the increasing number of contexts that have nothing to do with access logging. Also fixed triggering of (probably pointless) level-1 warnings: * ALE missing adapted HttpRequest object * ALE missing URL With fix applied, any ACLChecklist with ALE synchronizes it at 'pre-check' stage without logging level-1 warnings. Warnings are triggered only if for some reason this 'pre-check' synchronization was bypassed. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index 70d1bc8ca3..ec1414b8c2 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -323,7 +323,9 @@ FwdState::Start(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, Ht * we do NOT want the indirect client address to be tested here. */ ACLFilledChecklist ch(Config.accessList.miss, request, NULL); + ch.al = al; ch.src_addr = request->client_addr; + ch.syncAle(request, nullptr); if (ch.fastCheck().denied()) { err_type page_id; page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); @@ -1180,6 +1182,8 @@ FwdState::pconnPop(const Comm::ConnectionPointer &dest, const char *domain) bool retriable = checkRetriable(); if (!retriable && Config.accessList.serverPconnForNonretriable) { ACLFilledChecklist ch(Config.accessList.serverPconnForNonretriable, request, NULL); + ch.al = al; + ch.syncAle(request, nullptr); retriable = ch.fastCheck().allowed(); } // always call shared pool first because we need to close an idle diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index e94271daf9..7f8b1d1091 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -704,6 +704,7 @@ HttpRequest::manager(const CbcPointer &aMgr, const AccessLogEntry if (Config.accessList.spoof_client_ip) { ACLFilledChecklist *checklist = new ACLFilledChecklist(Config.accessList.spoof_client_ip, this, clientConnection->rfc931); checklist->al = al; + checklist->syncAle(this, nullptr); flags.spoofClientIp = checklist->fastCheck().allowed(); delete checklist; } else diff --git a/src/Notes.cc b/src/Notes.cc index a368ca4c18..7a9be83b84 100644 --- a/src/Notes.cc +++ b/src/Notes.cc @@ -42,7 +42,9 @@ Note::match(HttpRequest *request, HttpReply *reply, const AccessLogEntry::Pointe typedef Values::iterator VLI; ACLFilledChecklist ch(NULL, request, NULL); + ch.al = al; ch.reply = reply; + ch.syncAle(request, nullptr); if (reply) HTTPMSGLOCK(ch.reply); diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index a5de02a2e2..ea891fd327 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -141,7 +141,7 @@ ACL::matches(ACLChecklist *checklist) const } else { // make sure the ALE has as much data as possible if (requiresAle()) - checklist->syncAle(); + checklist->verifyAle(); // have to cast because old match() API is missing const result = const_cast(this)->match(checklist); diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 0a7646678b..c82b7ce77b 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -13,6 +13,8 @@ #include #include +class HttpRequest; + /// ACL checklist callback typedef void ACLCB(allow_t, void *); @@ -164,7 +166,10 @@ public: virtual bool hasRequest() const = 0; virtual bool hasReply() const = 0; virtual bool hasAle() const = 0; - virtual void syncAle() const = 0; + /// assigns uninitialized adapted_request and url ALE components + virtual void syncAle(HttpRequest *adaptedRequest, const char *logUri) const = 0; + /// warns if there are uninitialized ALE components and fills them + virtual void verifyAle() const = 0; /// change the current ACL list /// \return a pointer to the old list value (may be nullptr) diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc index e4db02c522..b5ce7d21ac 100644 --- a/src/acl/FilledChecklist.cc +++ b/src/acl/FilledChecklist.cc @@ -79,7 +79,7 @@ showDebugWarning(const char *msg) } void -ACLFilledChecklist::syncAle() const +ACLFilledChecklist::verifyAle() const { // make sure the ALE fields used by Format::assemble to // fill the old external_acl_type codes are set if any @@ -93,6 +93,8 @@ ACLFilledChecklist::syncAle() const if (request) { if (!al->request) { showDebugWarning("HttpRequest object"); + // XXX: al->request should be original, + // but the request may be already adapted al->request = request; HTTPMSGLOCK(al->request); } @@ -105,6 +107,8 @@ ACLFilledChecklist::syncAle() const if (al->url.isEmpty()) { showDebugWarning("URL"); + // XXX: al->url should be the request URL from client, + // but request->url may be different (e.g.,redirected) al->url = request->url.absolute(); } } @@ -123,6 +127,19 @@ ACLFilledChecklist::syncAle() const #endif } +void +ACLFilledChecklist::syncAle(HttpRequest *adaptedRequest, const char *logUri) const +{ + if (!al) + return; + if (!al->adapted_request) { + al->adapted_request = adaptedRequest; + HTTPMSGLOCK(al->adapted_request); + } + if (al->url.isEmpty()) + al->url = logUri; +} + ConnStateData * ACLFilledChecklist::conn() const { diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index 9455a6ceea..ccc41aa0eb 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -61,7 +61,8 @@ public: virtual bool hasRequest() const { return request != NULL; } virtual bool hasReply() const { return reply != NULL; } virtual bool hasAle() const { return al != NULL; } - virtual void syncAle() const; + virtual void syncAle(HttpRequest *adaptedRequest, const char *logUri) const; + virtual void verifyAle() const; public: Ip::Address src_addr; diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index 4baa5709f6..f99b2fd93e 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -135,6 +135,7 @@ Adaptation::AccessCheck::checkCandidates() if ((acl_checklist->reply = filter.reply)) HTTPMSGLOCK(acl_checklist->reply); acl_checklist->al = filter.al; + acl_checklist->syncAle(filter.request, nullptr); acl_checklist->nonBlockingCheck(AccessCheckCallbackWrapper, this); return; } diff --git a/src/client_side.cc b/src/client_side.cc index f9b0853d0a..e43a9da067 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -446,11 +446,14 @@ ClientHttpRequest::logRequest() al->adapted_request = request; HTTPMSGLOCK(al->adapted_request); } + // no need checklist.syncAle(): already synced + checklist.al = al; accessLogLog(al, &checklist); bool updatePerformanceCounters = true; if (Config.accessList.stats_collection) { ACLFilledChecklist statsCheck(Config.accessList.stats_collection, request, NULL); + statsCheck.al = al; if (al->reply) { statsCheck.reply = al->reply; HTTPMSGLOCK(statsCheck.reply); @@ -1520,7 +1523,9 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) bool allowDomainMismatch = false; if (Config.ssl_client.cert_error) { ACLFilledChecklist check(Config.ssl_client.cert_error, request, dash_str); + check.al = http->al; check.sslErrors = new Security::CertErrors(Security::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert)); + check.syncAle(request, http->log_uri); allowDomainMismatch = check.fastCheck().allowed(); delete check.sslErrors; check.sslErrors = NULL; @@ -1568,10 +1573,14 @@ clientTunnelOnError(ConnStateData *conn, Http::StreamPointer &context, HttpReque { if (conn->mayTunnelUnsupportedProto()) { ACLFilledChecklist checklist(Config.accessList.on_unsupported_protocol, request.getRaw(), nullptr); + checklist.al = (context && context->http) ? context->http->al : nullptr; checklist.requestErrorType = requestError; checklist.src_addr = conn->clientConnection->remote; checklist.my_addr = conn->clientConnection->local; checklist.conn(conn); + ClientHttpRequest *http = context ? context->http : nullptr; + const char *log_uri = http ? http->log_uri : nullptr; + checklist.syncAle(request.getRaw(), log_uri); allow_t answer = checklist.fastCheck(); if (answer.allowed() && answer.kind == 1) { debugs(33, 3, "Request will be tunneled to server"); @@ -2821,6 +2830,10 @@ ConnStateData::postHttpsAccept() HTTPMSGUNLOCK(acl_checklist->al->request); acl_checklist->al->request = request; HTTPMSGLOCK(acl_checklist->al->request); + Http::StreamPointer context = pipeline.front(); + ClientHttpRequest *http = context ? context->http : nullptr; + const char *log_uri = http ? http->log_uri : nullptr; + acl_checklist->syncAle(request, log_uri); acl_checklist->nonBlockingCheck(httpsSslBumpAccessCheckDone, this); #else fatal("FATAL: SSL-Bump requires --with-openssl"); @@ -3286,6 +3299,8 @@ ConnStateData::startPeekAndSplice() acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpNone)); acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst)); acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst)); + const char *log_uri = http ? http->log_uri : nullptr; + acl_checklist->syncAle(sslServerBump->request.getRaw(), log_uri); acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this); return; } @@ -3725,6 +3740,7 @@ clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http) ACLFilledChecklist *ch = new ACLFilledChecklist(acl, http->request, cbdataReferenceValid(conn) && conn != NULL && conn->clientConnection != NULL ? conn->clientConnection->rfc931 : dash_str); ch->al = http->al; + ch->syncAle(http->request, http->log_uri); /* * hack for ident ACL. It needs to get full addresses, and a place to store * the ident result on persistent connections... diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 4b5ac98fd5..0174d07252 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1784,8 +1784,10 @@ ClientHttpRequest::doCallouts() calloutContext->tosToClientDone = true; if (getConn() != NULL && Comm::IsConnOpen(getConn()->clientConnection)) { ACLFilledChecklist ch(NULL, request, NULL); + ch.al = calloutContext->http->al; ch.src_addr = request->client_addr; ch.my_addr = request->my_addr; + ch.syncAle(request, log_uri); tos_t tos = aclMapTOS(Ip::Qos::TheConfig.tosToClient, &ch); if (tos) Ip::Qos::setSockTos(getConn()->clientConnection, tos); @@ -1796,8 +1798,10 @@ ClientHttpRequest::doCallouts() calloutContext->nfmarkToClientDone = true; if (getConn() != NULL && Comm::IsConnOpen(getConn()->clientConnection)) { ACLFilledChecklist ch(NULL, request, NULL); + ch.al = calloutContext->http->al; ch.src_addr = request->client_addr; ch.my_addr = request->my_addr; + ch.syncAle(request, log_uri); nfmark_t mark = aclMapNfmark(Ip::Qos::TheConfig.nfmarkToClient, &ch); if (mark) Ip::Qos::setSockNfmark(getConn()->clientConnection, mark); diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc index 603ebd379b..b240a3ad28 100644 --- a/src/comm/TcpAcceptor.cc +++ b/src/comm/TcpAcceptor.cc @@ -267,6 +267,7 @@ logAcceptError(const Comm::ConnectionPointer &conn) ACLFilledChecklist ch(nullptr, nullptr, nullptr); ch.src_addr = conn->remote; ch.my_addr = conn->local; + ch.al = al; accessLogLog(al, &ch); } diff --git a/src/http.cc b/src/http.cc index d440758595..452ff28cd7 100644 --- a/src/http.cc +++ b/src/http.cc @@ -801,7 +801,9 @@ HttpStateData::handle1xx(HttpReply *reply) // check whether the 1xx response forwarding is allowed by squid.conf if (Config.accessList.reply) { ACLFilledChecklist ch(Config.accessList.reply, originalRequest(), NULL); + ch.al = fwd->al; ch.reply = reply; + ch.syncAle(originalRequest(), nullptr); HTTPMSGLOCK(ch.reply); if (!ch.fastCheck().allowed()) { // TODO: support slow lookups? debugs(11, 3, HERE << "ignoring denied 1xx"); @@ -2334,6 +2336,8 @@ HttpStateData::finishingBrokenPost() } ACLFilledChecklist ch(Config.accessList.brokenPosts, originalRequest(), NULL); + ch.al = fwd->al; + ch.syncAle(originalRequest(), nullptr); if (!ch.fastCheck().allowed()) { debugs(11, 5, HERE << "didn't match brokenPosts"); return false; diff --git a/src/neighbors.cc b/src/neighbors.cc index c7df0b8032..aebd42824b 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -136,7 +136,6 @@ neighborType(const CachePeer * p, const URL &url) bool peerAllowedToUse(const CachePeer * p, HttpRequest * request) { - assert(request != NULL); if (neighborType(p, request->url) == PEER_SIBLING) { @@ -167,7 +166,8 @@ peerAllowedToUse(const CachePeer * p, HttpRequest * request) return true; ACLFilledChecklist checklist(p->access, request, NULL); - +// checklist.al = ps->al; + checklist.syncAle(request, nullptr); return checklist.fastCheck().allowed(); } diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 9f14bd375c..08161a2422 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -131,6 +131,7 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) if (acl_access *acl = ::Config.ssl_client.cert_error) { ACLFilledChecklist *check = new ACLFilledChecklist(acl, request.getRaw(), dash_str); check->al = al; + check->syncAle(request.getRaw(), nullptr); // check->fd(fd); XXX: need client FD here SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check); } @@ -324,6 +325,7 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons if (acl_access *acl = ::Config.ssl_client.cert_error) { check = new ACLFilledChecklist(acl, request.getRaw(), dash_str); check->al = al; + check->syncAle(request.getRaw(), nullptr); } Security::CertErrors *errs = nullptr; diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index eb95af8d1a..a79a2fff92 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -70,6 +70,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice() acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpSplice)); if (!srvBio->canBump()) acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpBump)); + acl_checklist->syncAle(request.getRaw(), nullptr); acl_checklist->nonBlockingCheck(Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone, this); } diff --git a/src/tunnel.cc b/src/tunnel.cc index d46297150a..3197547adc 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1089,8 +1089,10 @@ tunnelStart(ClientHttpRequest * http) * default is to allow. */ ACLFilledChecklist ch(Config.accessList.miss, request, NULL); + ch.al = http->al; ch.src_addr = request->client_addr; ch.my_addr = request->my_addr; + ch.syncAle(request, http->log_uri); if (ch.fastCheck().denied()) { debugs(26, 4, HERE << "MISS access forbidden."); err = new ErrorState(ERR_FORWARDING_DENIED, Http::scForbidden, request);