diff --git a/src/SquidConfig.h b/src/SquidConfig.h index 0598a2369b6..c7de9be58eb 100644 --- a/src/SquidConfig.h +++ b/src/SquidConfig.h @@ -397,6 +397,7 @@ class SquidConfig /// spoof_client_ip squid.conf acl. /// nil unless configured acl_access* spoof_client_ip; + acl_access *errorSignallingAction; ///< error_signalling_action acl_access *on_unsupported_protocol; acl_access *ftp_epsv; diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 34bf9c43bbf..761e7cf9d6b 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -254,6 +254,9 @@ static void free_configuration_includes_quoted_values(bool *recognizeQuotedValue static void parse_on_unsupported_protocol(acl_access **access); static void dump_on_unsupported_protocol(StoreEntry *entry, const char *name, acl_access *access); static void free_on_unsupported_protocol(acl_access **access); +static void parse_error_signalling_action(acl_access **); +static void dump_error_signalling_action(StoreEntry *, const char *directiveName, const acl_access *); +static void free_error_signalling_action(acl_access **); static void ParseAclWithAction(acl_access **access, const Acl::Answer &action, const char *desc, Acl::Node *acl = nullptr); static void parse_http_upgrade_request_protocols(HttpUpgradeProtocolAccess **protoGuards); static void dump_http_upgrade_request_protocols(StoreEntry *entry, const char *name, HttpUpgradeProtocolAccess *protoGuards); @@ -4908,6 +4911,59 @@ free_on_unsupported_protocol(acl_access **access) free_acl_access(access); } +// TODO: Reduce code duplication with parse_on_unsupported_protocol() +static void +parse_error_signalling_action(acl_access ** const access) +{ + const auto actionName = LegacyParser.token("action name"); + + // XXX: Add an actions enum (while moving to Configuration::?) + // XXX: Reduce code duplication with dump_error_signalling_action() + auto action = Acl::Answer(ACCESS_ALLOWED); + if (actionName.cmp("gracefully_close") == 0) + action.kind = 1; + else if (actionName.cmp("reset") == 0) + action.kind = 2; + else if (actionName.cmp("respond") == 0) + action.kind = 3; + else + throw TextException(ToSBuf("unknown action name: ", actionName), Here()); + + if (LegacyParser.skipOptional("if")) + return ParseAclWithAction(access, action, cfg_directive); + + // OK: unconditional action; TODO: Warn if more actions follow. + + // XXX: Reject empty rules: `error_signalling_action reset if` + + // call to populate Config.accessList.errorSignallingAction even if there are no ACLs + ParseAclWithAction(access, action, cfg_directive); +} + +static void +dump_error_signalling_action(StoreEntry * const entry, const char * const directiveName, const acl_access * const access) +{ + static const std::vector actionNames = { + "none", + "gracefully_close", + "reset", + "respond" + }; + if (access) { + const auto actionsWithAclNames = ToTree(access).treeDump(directiveName, [](const Acl::Answer &action) { + Assure(action.kind > 0); + return actionNames.at(action.kind); + }); + dump_SBufList(entry, actionsWithAclNames); + } +} + +static void +free_error_signalling_action(acl_access ** const access) +{ + free_acl_access(access); +} + static void parse_http_upgrade_request_protocols(HttpUpgradeProtocolAccess **protoGuardsPtr) { diff --git a/src/cf.data.depend b/src/cf.data.depend index fa896248cf7..414c2fa6a79 100644 --- a/src/cf.data.depend +++ b/src/cf.data.depend @@ -64,6 +64,7 @@ memcachemode note acl obsolete onoff +error_signalling_action acl on_unsupported_protocol acl peer peer_access cache_peer acl diff --git a/src/cf.data.pre b/src/cf.data.pre index d6e03f5fd3b..95d72087e11 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -2156,6 +2156,78 @@ DOC_START DOC_END +NAME: error_signalling_action +TYPE: error_signalling_action +LOC: Config.accessList.errorSignallingAction +DEFAULT: none +DEFAULT_DOC: Proceed as usual (e.g., send an error message to the client) +DOC_START + Controls whether Squid sends an error response to the client or closes the + underlying client-to-Squid TCP connection instead. + + XXX: The current directive name is too broad and, hence, misleading: Squid + does not consult this directive on many transaction errors! Applicable + error cases are described further below. TODO: Find a better name. + + error_signalling_action [if [!]...] + + Supported actions are: + + * gracefully_close: Close client-to-Squid connection nicely. The client + should get a TCP FIN packet. For established TLS connections, Squid will + attempt to send a close_notify alert to the client before closing the + underlying TCP connection. The error response is not sent. + + * reset: Close client-to-Squid connection immediately. The client should + get a TCP RST packet. For established TLS connections, Squid will not + attempt to send a close_notify alert to the client. The error response + is not sent. + + * respond: Send an error response to the client. This is the default. In + some SslBump cases, sending a response may require establishing a TLS + connection with the client. Depending on the error context, Squid may + close the client-to-Squid TCP connection after sending the response. + + This directive applies to cases where Squid normally attempts to send an + error response to the client. In many other problematic cases, especially + cases where a transaction already wrote some TCP payload bytes to the + client, Squid aborts the transaction and closes the client-to-Squid TCP + connection without consulting this directive. + + Squid may attempt to bypass or retry certain errors. This directive is + consulted only for final transaction errors, after all such recovery + attempts have been exhausted, and it becomes clear that the transaction + will not be successful. + + Certain transaction problems trigger on_unsupported_protocol processing. + This directive is not checked while making those on_unsupported_protocol + decisions. However, this directive is usually checked after a "respond" + decision was made by on_unsupported_protocol because that decision usually + triggers Squid attempt to send an error response to the client -- the + subject of this directive. See on_unsupported_protocol documentation for + "respond" caveats. Also, on_unsupported_protocol directive itself can be + used to close the client-to-Squid connection if needed. + + The rules are evaluated in configuration order. The first matching action + wins. If no rule matched or if no rules are configured, Squid acts as if a + "respond" action has matched. + + This clause only supports fast acl types. + See https://wiki.squid-cache.org/SquidFaq/SquidAcl for details. + + For example: + + # Send TCP RST (instead of bumping the client connection and sending + # an encrypted HTTP ERR_SECURE_CONNECT_FAIL response) to an + # intercepted TLS client that fails to establish a secure connection + # to a server. + acl secureConnectFailed squid_error ERR_SECURE_CONNECT_FAIL + acl subjectToSslBump myportname 443 + error_signalling_action reset if secureConnectFailed subjectToSslBump + + See also: squid_error ACL and %err_code/%err_detail logformat codes. +DOC_END + NAME: on_unsupported_protocol TYPE: on_unsupported_protocol LOC: Config.accessList.on_unsupported_protocol @@ -2180,8 +2252,11 @@ DOC_START respond: Respond with an error message, using the transfer protocol for the Squid port that received the request (e.g., HTTP - for connections intercepted at the http_port). This is the - default. + for connections intercepted at the http_port). In some cases (e.g., + ERR_REQUEST_PARSE_TIMEOUT, ERR_REQUEST_START_TIMEOUT, and client TLS + handshake parsing failure on SslBump handling path), Squid closes + the underlying TCP client-to-Squid connection instead of sending an + error response to the client. This action is the default. Squid expects the following traffic patterns: diff --git a/src/client_side.cc b/src/client_side.cc index deb64d19ea7..764c4edee9a 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1516,6 +1516,41 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) } #endif // USE_OPENSSL +bool +ConnStateData::closedOnError() +{ + if (!Config.accessList.errorSignallingAction) + return false; + + ACLFilledChecklist checklist(Config.accessList.errorSignallingAction, nullptr); + fillChecklist(checklist); + const auto answer = checklist.fastCheck(); + debugs(33, 5, answer.allowed() << '+' << answer.kind); + if (answer.allowed()) { + switch (answer.kind) { + case 1: // gracefully_close + // See TODO for case 2: below + clientConnection->close(); + return true; + case 2: + // TODO: We want to call terminateAll(), but we do not want it to + // set WITH_CLIENT detail, so we need to supply our own. We also + // want it to call comm_reset_close(). How to signal that? + // See also: clientProcessRequestFinished() + // sslServerBump->request->flags.resetTcp = true; // might already be true + // terminateAll(...); + comm_reset_close(clientConnection); + return true; + case 3: + return false; + default: + Assure(!"impossible answer.kind value"); + } + } + + return false; +} + /// initiate tunneling if possible or return false otherwise bool ConnStateData::tunnelOnError(const err_type requestError) @@ -3077,8 +3112,14 @@ ConnStateData::httpsPeeked(PinnedIdleContext pic) if (Comm::IsConnOpen(pic.connection)) { notePinnedConnectionBecameIdle(pic); debugs(33, 5, "bumped HTTPS server: " << tlsConnectHostOrIp); - } else + } else { debugs(33, 5, "Error while bumping: " << tlsConnectHostOrIp); + // Outside of SslBump, we notice the error when we get a Store response. + // On SslBump path, we have to close here, _before_ getSslContextStart() + // below sends something (e.g., TLS Server Hello) to the client. + if (closedOnError()) + return; + } getSslContextStart(); } diff --git a/src/client_side.h b/src/client_side.h index 4a67203f75a..b8c3282cdc2 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -240,6 +240,11 @@ class ConnStateData: void swanSong() override; void callException(const std::exception &) override; + /// closes client-to-Squid connection if needed, honoring error_signalling_action + /// \returns false if the caller should proceed as usual (i.e. when this + /// method or earlier code has not initiated connection closure) + bool closedOnError(); + /// Changes state so that we close the connection and quit after serving /// the client-side-detected error response instead of getting stuck. void quitAfterError(HttpRequest *request); // meant to be private diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 66c53916111..cd247b55796 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1963,6 +1963,8 @@ clientReplyContext::sendMoreData (StoreIOBuffer result) } if (conn->pinning.zeroReply) { debugs(33,3, "not sending more data after a pinned zero reply " << conn->clientConnection); + // Send nothing and wait for clientPinnedConnectionClosed() to close conn->clientConnection. + // TODO: Replace this hard-coded (in both locations) decision with error_signalling_action handling. return; } diff --git a/src/client_side_request.cc b/src/client_side_request.cc index b9a6d9d2bc5..6d6f0605f3c 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1475,6 +1475,12 @@ ClientHttpRequest::updateError(const Error &error) al->updateError(error); } +bool +ClientHttpRequest::seenError() const +{ + return (request && request->error) || al->error(); +} + bool ClientHttpRequest::gotEnough() const { diff --git a/src/client_side_request.h b/src/client_side_request.h index 6a93b87ddd6..04261f3720c 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -122,6 +122,9 @@ class ClientHttpRequest /// if necessary, stores new error information (if any) void updateError(const Error &); + /// whether this transaction has failed + bool seenError() const; + public: /// Request currently being handled by ClientHttpRequest. /// Usually remains nil until the virgin request header is parsed or faked. diff --git a/src/http/Stream.cc b/src/http/Stream.cc index 83f8329d5af..eb3258ca69e 100644 --- a/src/http/Stream.cc +++ b/src/http/Stream.cc @@ -274,6 +274,17 @@ Http::Stream::sendStartOfMessage(HttpReply *rep, StoreIOBuffer bodyData) debugs(11, 2, "HTTP Client " << clientConnection); debugs(11, 2, "HTTP Client REPLY:\n---------\n" << mb->buf << "\n----------"); + // Check error_signalling_action here rather than earlier, including FwdState::completed(): + // * sendClientOldEntry() and similar post-Store code may bypass errors that + // looked "final" from FwdState point of view + // * an error response to pipelined request B should not close client + // connection while we are still sending a response to request A + // * simplifies avoiding repeated error_signalling_action checks when catching both + // pre/non-Store and post-Store cases + // * dumping ready-to-be-sent reply headers (above) assists triage + if (http->seenError() && http->getConn()->closedOnError()) + return; + /* Save length of headers for persistent conn checks */ http->out.headers_sz = mb->contentSize(); diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 5b6cd63b34a..ab4489316d1 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -493,13 +493,26 @@ Ftp::Server::writeEarlyReply(const int code, const char *msg) void Ftp::Server::writeReply(MemBuf &mb) +{ + using Dialer = CommCbMemFunT; + AsyncCall::Pointer call = JobCallback(33, 5, Dialer, this, Ftp::Server::wroteReply); + writeReplyAndCall(mb, call); +} + +void +Ftp::Server::writeReplyAndCall(MemBuf &mb, AsyncCall::Pointer &call) { debugs(9, 2, "FTP Client " << clientConnection); debugs(9, 2, "FTP Client REPLY:\n---------\n" << mb.buf << "\n----------"); - typedef CommCbMemFunT Dialer; - AsyncCall::Pointer call = JobCallback(33, 5, Dialer, this, Ftp::Server::wroteReply); + const auto context = pipeline.front(); + Assure(context); + const auto http = context->http; + Assure(http); + if (http->seenError() && http->getConn()->closedOnError()) + return; + Comm::Write(clientConnection, &mb, call); } @@ -1201,12 +1214,7 @@ Ftp::Server::writeForwardedReplyAndCall(const HttpReply *reply, AsyncCall::Point MemBuf mb; mb.init(); Ftp::PrintReply(mb, reply); - - debugs(9, 2, "FTP Client " << clientConnection); - debugs(9, 2, "FTP Client REPLY:\n---------\n" << mb.buf << - "\n----------"); - - Comm::Write(clientConnection, &mb, call); + writeReplyAndCall(mb, call); } static void diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h index 500364c4385..5c372dc2389 100644 --- a/src/servers/FtpServer.h +++ b/src/servers/FtpServer.h @@ -146,6 +146,7 @@ class Server: public ConnStateData void writeForwardedReply(const HttpReply *reply); void writeForwardedReplyAndCall(const HttpReply *reply, AsyncCall::Pointer &call); void writeReply(MemBuf &mb); + void writeReplyAndCall(MemBuf &, AsyncCall::Pointer &); Http::Stream *earlyError(const EarlyErrorKind eek); bool handleRequest(HttpRequest *); diff --git a/src/tunnel.cc b/src/tunnel.cc index ff85638f059..67f00e26ad7 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1211,6 +1211,12 @@ tunnelStart(ClientHttpRequest * http) http->updateLoggingTags(LOG_TCP_TUNNEL); err = new ErrorState(ERR_FORWARDING_DENIED, Http::scForbidden, request, http->al); http->al->http.code = Http::scForbidden; + + // XXX: Move miss_access check inside TunnelStateData to reduce code + // duplication among errorSend() callers. + if (http->getConn()->closedOnError()) + return; + errorSend(http->getConn()->clientConnection, err); return; } @@ -1433,6 +1439,10 @@ TunnelStateData::sendError(ErrorState *finalError, const char *reason) *status_ptr = finalError->httpStatus; finalError->callback = tunnelErrorComplete; finalError->callback_data = this; + + if (http->getConn()->closedOnError()) + return; + errorSend(client.conn, finalError); }