Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial on_error implementation #276

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/SquidConfig.h
Original file line number Diff line number Diff line change
@@ -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;
56 changes: 56 additions & 0 deletions src/cache_cf.cc
Original file line number Diff line number Diff line change
@@ -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<const char *> 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)
{
1 change: 1 addition & 0 deletions src/cf.data.depend
Original file line number Diff line number Diff line change
@@ -64,6 +64,7 @@ memcachemode
note acl
obsolete
onoff
error_signalling_action acl
on_unsupported_protocol acl
peer
peer_access cache_peer acl
79 changes: 77 additions & 2 deletions src/cf.data.pre
Original file line number Diff line number Diff line change
@@ -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 <action> [if [!]<aclname>...]

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:

43 changes: 42 additions & 1 deletion src/client_side.cc
Original file line number Diff line number Diff line change
@@ -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();
}
5 changes: 5 additions & 0 deletions src/client_side.h
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions src/client_side_reply.cc
Original file line number Diff line number Diff line change
@@ -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;
}

6 changes: 6 additions & 0 deletions src/client_side_request.cc
Original file line number Diff line number Diff line change
@@ -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
{
3 changes: 3 additions & 0 deletions src/client_side_request.h
Original file line number Diff line number Diff line change
@@ -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.
11 changes: 11 additions & 0 deletions src/http/Stream.cc
Original file line number Diff line number Diff line change
@@ -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();

24 changes: 16 additions & 8 deletions src/servers/FtpServer.cc
Original file line number Diff line number Diff line change
@@ -493,13 +493,26 @@ Ftp::Server::writeEarlyReply(const int code, const char *msg)

void
Ftp::Server::writeReply(MemBuf &mb)
{
using Dialer = CommCbMemFunT<Server, CommIoCbParams>;
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<Server, CommIoCbParams> 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
1 change: 1 addition & 0 deletions src/servers/FtpServer.h
Original file line number Diff line number Diff line change
@@ -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 *);
10 changes: 10 additions & 0 deletions src/tunnel.cc
Original file line number Diff line number Diff line change
@@ -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);
}