From f926256b8f790875274448e8c37aa770fc753636 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 22 Mar 2018 23:17:40 -0600 Subject: [PATCH 01/59] Extracted Squid-peer CONNECT exchange ... from tunnel.cc into Http::Tunneler (inspired by PeerConnector). The next step is to use the extracted Tunneler in FwdState. Passes basic tests but needs a lot of polishing. --- src/clients/HttpTunneler.cc | 384 ++++++++++++++++++++++++++++++ src/clients/HttpTunneler.h | 104 ++++++++ src/clients/HttpTunnelerAnswer.cc | 39 +++ src/clients/HttpTunnelerAnswer.h | 54 +++++ src/clients/Makefile.am | 4 + src/clients/forward.h | 6 + src/tunnel.cc | 341 ++++++++------------------ 7 files changed, 693 insertions(+), 239 deletions(-) create mode 100644 src/clients/HttpTunneler.cc create mode 100644 src/clients/HttpTunneler.h create mode 100644 src/clients/HttpTunnelerAnswer.cc create mode 100644 src/clients/HttpTunnelerAnswer.h diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc new file mode 100644 index 00000000000..41febe10303 --- /dev/null +++ b/src/clients/HttpTunneler.cc @@ -0,0 +1,384 @@ +/* + * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "clients/HttpTunneler.h" +#include "clients/HttpTunnelerAnswer.h" +#include "comm/Read.h" +#include "comm/Write.h" +#include "errorpage.h" +#include "fde.h" +#include "http.h" +#include "http/StateFlags.h" +#include "HttpRequest.h" +#include "StatCounters.h" +#include "SquidConfig.h" + +CBDATA_NAMESPACED_CLASS_INIT(Http, Tunneler); + +Http::Tunneler::Tunneler(AsyncCall::Pointer &aCallback): + AsyncJob("Http::Tunneler"), + callback(aCallback), + lifetimeLimit(0), + startTime(squid_curtime), + requestWritten(false), + tunnelEstablished(false) +{ + debugs(83, 5, "Http::Tunneler constructed, this=" << (void*)this); + // detect callers supplying cb dialers that are not our CbDialer + assert(callback); + assert(dynamic_cast(callback->getDialer())); +} + +Http::Tunneler::~Tunneler() +{ + debugs(83, 5, "Http::Tunneler destructed, this=" << (void*)this); +} + +bool +Http::Tunneler::doneAll() const +{ + return !callback || (requestWritten && tunnelEstablished); +} + +/// convenience method to get to the answer fields +Http::TunnelerAnswer & +Http::Tunneler::answer() +{ + Must(callback); + const auto dialer = dynamic_cast(callback->getDialer()); + Must(dialer); + return dialer->answer(); +} + +void +Http::Tunneler::start() +{ + AsyncJob::start(); + + Must(request); + Must(connection); + Must(al); + Must(callback); + Must(url.length()); + Must(lifetimeLimit >= 0); + + watchForClosures(); + writeRequest(); + startReadingResponse(); +} + +void +Http::Tunneler::handleConnectionClosure(const CommCloseCbParams ¶ms) +{ + mustStop("server connection gone"); + callback = nullptr; // the caller must monitor closures +} + +/// make sure we quit if/when the connection is gone +void +Http::Tunneler::watchForClosures() +{ + Must(Comm::IsConnOpen(connection)); + Must(!fd_table[connection->fd].closing()); + + debugs(83, 5, connection); + + Must(!closer); + typedef CommCbMemFunT Dialer; + closer = JobCallback(9, 5, Dialer, this, Http::Tunneler::handleConnectionClosure); + comm_add_close_handler(connection->fd, closer); +} + +void +Http::Tunneler::setReadTimeout() +{ + // TODO: polish to use time_t and extract to reduce duplication with similar code in PeerConnector + int timeToRead; + if (lifetimeLimit > 0) { + const int timeUsed = squid_curtime - startTime; + const int timeLeft = max(0, static_cast(lifetimeLimit - timeUsed)); + timeToRead = min(static_cast(::Config.Timeout.read), timeLeft); + } else + timeToRead = ::Config.Timeout.read; + // TODO: Same as PeerConnector, but who removes this timeout after we send a + // positive answer? Should not we clean after ourselves in swanSong()? + AsyncCall::Pointer nil; + commSetConnTimeout(connection, timeToRead, nil); +} + +void +Http::Tunneler::handleException(const std::exception& e) +{ + debugs(83, 2, e.what() << status()); + connection->close(); + bailWith(new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw(), al)); +} + +void +Http::Tunneler::startReadingResponse() +{ + debugs(83, 5, connection << status()); + + readBuf.reserveCapacity(SQUID_TCP_SO_RCVBUF); + readMore(); +} + +void +Http::Tunneler::writeRequest() +{ + debugs(83, 5, connection); + + HttpHeader hdr_out(hoRequest); + Http::StateFlags flags; + memset(&flags, '\0', sizeof(flags)); + flags.proxying = request->flags.proxying; + MemBuf mb; + mb.init(); + mb.appendf("CONNECT %s HTTP/1.1\r\n", url.c_str()); + HttpStateData::httpBuildRequestHeader(request.getRaw(), + nullptr, // StoreEntry + al, + &hdr_out, + flags); + hdr_out.packInto(&mb); + hdr_out.clean(); + mb.append("\r\n", 2); + + debugs(11, 2, "Tunnel Server REQUEST: " << connection << + ":\n----------\n" << mb.buf << "\n----------"); + + typedef CommCbMemFunT Dialer; + writer = JobCallback(5, 5, Dialer, this, Http::Tunneler::handleWrittenRequest); + Comm::Write(connection, &mb, writer); +} + +/// Called when we are done writing a CONNECT request header to a peer. +void +Http::Tunneler::handleWrittenRequest(const CommIoCbParams &io) +{ + Must(writer); + writer = nullptr; + + if (io.flag == Comm::ERR_CLOSING) + return; + + request->hier.notePeerWrite(); + + if (io.flag != Comm::OK) { + const auto error = new ErrorState(ERR_WRITE_ERROR, Http::scBadGateway, request.getRaw(), al); + error->xerrno = io.xerrno; + bailWith(error); + return; + } + + statCounter.server.all.kbytes_out += io.size; + statCounter.server.other.kbytes_out += io.size; + requestWritten = true; + debugs(83, 5, status()); +} + +/// Called when we read [a part of] CONNECT response from the peer +void +Http::Tunneler::handleReadyRead(const CommIoCbParams &io) +{ + Must(reader); + reader = nullptr; + + if (io.flag == Comm::ERR_CLOSING) + return; + + CommIoCbParams rd(this); + rd.conn = io.conn; + rd.size = readBuf.spaceSize(); // XXX: from.bytesWanted(...) + + switch (Comm::ReadNow(rd, readBuf)) { + case Comm::INPROGRESS: + readMore(); + return; + + case Comm::OK: { +#if USE_DELAY_POOLS && XXX_IMPLEMENT + delayId.bytesIn(rd.size); +#endif + statCounter.server.all.kbytes_in += rd.size; + statCounter.server.other.kbytes_in += rd.size; // TODO: other or http? + request->hier.notePeerRead(); + len += rd.size; + handleResponse(false); + return; + } + + case Comm::ENDFILE: { + // TODO: Should we (and everybody else) call request->hier.notePeerRead() on zero reads? + handleResponse(true); + return; + } + + // case Comm::COMM_ERROR: + default: // no other flags should ever occur + { + const auto error = new ErrorState(ERR_READ_ERROR, Http::scBadGateway, request.getRaw(), al); + error->xerrno = rd.xerrno; + bailWith(error); + return; + } + } + + assert(false); // not reached +} + +void +Http::Tunneler::readMore() +{ + Must(Comm::IsConnOpen(connection)); + Must(!fd_table[connection->fd].closing()); + Must(!reader); + + typedef CommCbMemFunT Dialer; + reader = JobCallback(93, 3, Dialer, this, Http::Tunneler::handleReadyRead); + Comm::Read(connection, reader); + setReadTimeout(); +} + +/// Parses [possibly incomplete] CONNECT response and reacts to it. +void +Http::Tunneler::handleResponse(const bool eof) +{ + // mimic the basic parts of HttpStateData::processReplyHeader() + // TODO: HttpStateData::processReplyHeader() tries harder on eof! + // TODO: Do not parse headers. Use Http1::ResponseParser. See HttpStateData::processReplyHeader(). + HttpReply rep; + Http::StatusCode parseErr = Http::scNone; + const bool parsed = rep.parse(readBuf.c_str(), readBuf.length(), eof, &parseErr); + if (!parsed) { + if (parseErr > 0) { // unrecoverable parsing error + bailOnResponseError("malformed CONNECT response from peer", 0); + return; + } + + // need more data + assert(!eof); + assert(!parseErr); + + if (readBuf.length() >= SQUID_TCP_SO_RCVBUF) { + bailOnResponseError("huge CONNECT response from peer", 0); + return; + } + + readMore(); + return; + } + + // CONNECT response was successfully parsed + auto &futureAnswer = answer(); + futureAnswer.peerResponseStatus = rep.sline.status(); + request->hier.peer_reply_status = rep.sline.status(); + + // XXX: Raw() prints an extra leading space. TODO: Add/use Raw::gap(false). + debugs(11, 2, "HTTP Server " << connection); + debugs(11, 2, "HTTP Server RESPONSE:\n---------\n" << + Raw(nullptr, readBuf.rawContent(), rep.hdr_sz).minLevel(2) << + "----------"); + + // bail if we did not get an HTTP 200 (Connection Established) response + if (rep.sline.status() != Http::scOkay) { + // TODO: To reuse the connection, extract the whole error response. + bailOnResponseError("unsupported CONNECT response status code", rep.hdr_sz); + return; + } + + // preserve any bytes sent by the server after the CONNECT response + futureAnswer.leftovers = readBuf.substr(rep.hdr_sz); + // delay pools were using this field to throttle CONNECT response + len = futureAnswer.leftovers.length(); + + tunnelEstablished = true; + debugs(83, 5, status()); +} + +void +Http::Tunneler::bailOnResponseError(const char *error, const size_t peerResponseSize) +{ + debugs(83, 3, error << status()); + + if (!peerResponseSize) { + // with no reply suitable for relaying, answer with 502 (Bad Gateway) + bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); + } else { + answer().peerError = readBuf.substr(0, peerResponseSize); + callBack(); + } +} + +void +Http::Tunneler::bailWith(ErrorState *error) +{ + Must(error); + answer().squidError = error; + callBack(); +} + +void +Http::Tunneler::callBack() +{ + debugs(83, 5, connection << status()); + auto cb = callback; + callback = nullptr; + ScheduleCallHere(cb); +} + +void +Http::Tunneler::swanSong() +{ + AsyncJob::swanSong(); + + if (callback) { + if (requestWritten && tunnelEstablished) { + assert(answer().positive()); + callBack(); // success + } else { + // we should have bailed when we discovered the job-killing problem + debugs(83, DBG_IMPORTANT, "BUG: Unexpected state while establishing a CONNECT tunnel " << connection << status()); + bailWith(new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw(), al)); + } + assert(!callback); + } + + if (closer) { + comm_remove_close_handler(connection->fd, closer); + closer = nullptr; + } + + if (reader) { + Comm::ReadCancel(connection->fd, reader); + reader = nullptr; + } +} + +const char * +Http::Tunneler::status() const +{ + static MemBuf buf; + buf.reset(); + + // TODO: redesign AsyncJob::status() API to avoid + // id and stop reason reporting duplication. + buf.append(" [", 2); + // TODO: report true requestWritten, true tunnelEstablished, and nil callback + if (stopReason != NULL) { + buf.append("Stopped, reason:", 16); + buf.appendf("%s",stopReason); + } + if (connection != NULL) + buf.appendf(" FD %d", connection->fd); + buf.appendf(" %s%u]", id.prefix(), id.value); + buf.terminate(); + + return buf.content(); +} diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h new file mode 100644 index 00000000000..975fc339276 --- /dev/null +++ b/src/clients/HttpTunneler.h @@ -0,0 +1,104 @@ +/* + * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_SRC_CLIENTS_HTTP_TUNNELER_H +#define SQUID_SRC_CLIENTS_HTTP_TUNNELER_H + +#include "base/AsyncCbdataCalls.h" +#include "base/AsyncJob.h" +#include "CommCalls.h" +#include "http/forward.h" +#include "clients/forward.h" + +class ErrorState; +class AccessLogEntry; +typedef RefCount AccessLogEntryPointer; + +namespace Http +{ + +// TODO: Reformat description using Doxygen. + +// Establishes an HTTP CONNECT tunnel through another proxy. +// +// The caller receives a call back with Http::TunnelerAnswer. +// +// The caller must monitor the connection for closure because this job will not +// inform the caller about such events. +// +// This job never closes the connection, even on errors. If a 3rd-party closes +// the connection, this job simply quits without informing the caller. +class Tunneler: virtual public AsyncJob +{ + CBDATA_CLASS(Tunneler); + +public: + /// Callback dialer API to allow Tunneler to set the answer. + class CbDialer + { + public: + virtual ~CbDialer() {} + /// gives Tunneler access to the in-dialer answer + virtual Http::TunnelerAnswer &answer() = 0; + }; + +public: + explicit Tunneler(AsyncCall::Pointer &aCallback); + Tunneler(const Tunneler &) = delete; + Tunneler &operator =(const Tunneler &) = delete; + + /* configuration; too many fields to use constructor parameters */ + HttpRequestPointer request; ///< peer connection trigger or cause + Comm::ConnectionPointer connection; ///< TCP connection to peer or origin + AccessLogEntryPointer al; ///< info for the future access.log entry + AsyncCall::Pointer callback; ///< we call this with the results + SBuf url; ///< request-target for the CONNECT request + time_t lifetimeLimit; ///< do not run longer than this + +protected: + /* AsyncJob API */ + virtual ~Tunneler(); + virtual void start(); + virtual bool doneAll() const; + virtual void swanSong(); + virtual const char *status() const; + + void handleConnectionClosure(const CommCloseCbParams&); + void watchForClosures(); + void setReadTimeout(); + void handleException(const std::exception&); + void startReadingResponse(); + void writeRequest(); + void handleWrittenRequest(const CommIoCbParams&); + void handleReadyRead(const CommIoCbParams&); + void readMore(); + void handleResponse(const bool eof); + void bailOnResponseError(const char *error, const size_t peerResponseSize); + void bailWith(ErrorState*); + void callBack(); + + TunnelerAnswer &answer(); + +private: + AsyncCall::Pointer writer; ///< called when the request has been written + AsyncCall::Pointer reader; ///< called when the response should be read + AsyncCall::Pointer closer; ///< called when the connection is being closed + + SBuf readBuf; ///< either unparsed response or post-response bytes + + const time_t startTime; ///< when the tunnel establishment started + + size_t len; // XXX: Delay ID needs something like this? + + bool requestWritten; ///< whether we successfully wrote the request + bool tunnelEstablished; ///< whether we got a 200 OK response +}; + +} // namespace Http + +#endif /* SQUID_SRC_CLIENTS_HTTP_TUNNELER_H */ diff --git a/src/clients/HttpTunnelerAnswer.cc b/src/clients/HttpTunnelerAnswer.cc new file mode 100644 index 00000000000..fec1c6fe91d --- /dev/null +++ b/src/clients/HttpTunnelerAnswer.cc @@ -0,0 +1,39 @@ +/* + * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "comm/Connection.h" +#include "errorpage.h" +#include "clients/HttpTunnelerAnswer.h" + +Http::TunnelerAnswer::~TunnelerAnswer() +{ + delete squidError.get(); +} + +std::ostream & +Http::operator <<(std::ostream &os, const TunnelerAnswer &answer) +{ + os << '['; + + if (const auto squidError = answer.squidError.get()) { + os << "SquidErr:" << squidError->page_id; + } else if (const auto errBytes = answer.peerError.length()) { + os << "PeerErr:" << answer.peerError.length(); + } else { + os << "OK"; + if (const auto extraBytes = answer.leftovers.length()) + os << '+' << extraBytes; + } + + if (answer.peerResponseStatus != Http::scNone) + os << ' ' << answer.peerResponseStatus; + + os << ']'; + return os; +} diff --git a/src/clients/HttpTunnelerAnswer.h b/src/clients/HttpTunnelerAnswer.h new file mode 100644 index 00000000000..0ed48eee1b0 --- /dev/null +++ b/src/clients/HttpTunnelerAnswer.h @@ -0,0 +1,54 @@ +/* + * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_SRC_CLIENTS_HTTP_TUNNELERANSWER_H +#define SQUID_SRC_CLIENTS_HTTP_TUNNELERANSWER_H + +#include "base/CbcPointer.h" +#include "comm/forward.h" +#include "http/StatusCode.h" +#include "sbuf/SBuf.h" + +class ErrorState; + +namespace Http { + +// Three mutually exclusive answers are possible: +// +// * Squid-generated error object (TunnelerAnswer::squidError); +// * peer-generated error message (TunnelerAnswer::peerError); +// * successful tunnel establishment (none of the above are present). + +// HTTP CONNECT tunnel setup results (supplied via a callback). The tunnel +// through the peer was established if and only if the error member is nil. +class TunnelerAnswer +{ +public: + TunnelerAnswer() {} + ~TunnelerAnswer(); ///< deletes squidError if it is still set + + bool positive() const { return !squidError && peerError.isEmpty(); } + + Comm::ConnectionPointer conn; ///< to-peer connection (XXX: remove?) + + // answer recipients must clear the error member in order to keep its info + // XXX: We should refcount ErrorState instead of cbdata-protecting it. + CbcPointer squidError; ///< problem details (or nil) + + SBuf peerError; ///< peer-generated error message header (or empty) + SBuf leftovers; ///< peer-generated bytes after a positive answer (or empty) + + /// the status code of the successfully parsed CONNECT response (or scNone) + StatusCode peerResponseStatus = scNone; +}; + +std::ostream &operator <<(std::ostream &, const Http::TunnelerAnswer &); + +} // namepace Http + +#endif /* SQUID_SRC_CLIENTS_HTTP_TUNNELERANSWER_H */ diff --git a/src/clients/Makefile.am b/src/clients/Makefile.am index 2b911de4343..7e4203dac38 100644 --- a/src/clients/Makefile.am +++ b/src/clients/Makefile.am @@ -17,4 +17,8 @@ libclients_la_SOURCES = \ FtpClient.h \ FtpGateway.cc \ FtpRelay.cc \ + HttpTunneler.cc \ + HttpTunneler.h \ + HttpTunnelerAnswer.cc \ + HttpTunnelerAnswer.h \ forward.h diff --git a/src/clients/forward.h b/src/clients/forward.h index 0c0d916cab8..81338c65916 100644 --- a/src/clients/forward.h +++ b/src/clients/forward.h @@ -18,6 +18,12 @@ class AsyncJob; template class CbcPointer; typedef CbcPointer AsyncJobPointer; +namespace Http +{ +class Tunneler; +class TunnelerAnswer; +} + namespace Ftp { diff --git a/src/tunnel.cc b/src/tunnel.cc index 826bc32b961..923fa45f01a 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -15,6 +15,8 @@ #include "cbdata.h" #include "client_side.h" #include "client_side_request.h" +#include "clients/HttpTunneler.h" +#include "clients/HttpTunnelerAnswer.h" #include "comm.h" #include "comm/Connection.h" #include "comm/ConnOpener.h" @@ -80,12 +82,6 @@ class TunnelStateData: public PeerSelectionInitiator static void WriteClientDone(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data); static void WriteServerDone(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data); - /// Starts reading peer response to our CONNECT request. - void readConnectResponse(); - - /// Called when we may be done handling a CONNECT exchange with the peer. - void connectExchangeCheckpoint(); - bool noConnections() const; char *url; CbcPointer http; @@ -119,16 +115,15 @@ class TunnelStateData: public PeerSelectionInitiator (request->flags.interceptTproxy || request->flags.intercepted)); } - /// Sends "502 Bad Gateway" error response to the client, - /// if it is waiting for Squid CONNECT response, closing connections. - void informUserOfPeerError(const char *errMsg, size_t); - /// starts connecting to the next hop, either for the first time or while /// recovering from the previous connect failure void startConnecting(); void noteConnectFailure(const Comm::ConnectionPointer &conn); + /// called when negotiations with the peer have been successfully completed + void notePeerReadyToShovel(); + class Connection { @@ -225,17 +220,44 @@ class TunnelStateData: public PeerSelectionInitiator /// details of the "last tunneling attempt" failure (if it failed) ErrorState *savedError = nullptr; + // XXX: Find a better way to pass answers than to create a dedicated class + // for each peer connector/tunneler! + /// Gives Http::Tunneler access to Answer in the TunnelStateData callback dialer. + class MyAnswerDialer2: public CallDialer, public Http::Tunneler::CbDialer + { + public: + typedef void (TunnelStateData::*Method)(Http::TunnelerAnswer &); + + MyAnswerDialer2(Method method, TunnelStateData *tunnel): + method_(method), tunnel_(tunnel), answer_() {} + + /* CallDialer API */ + virtual bool canDial(AsyncCall &call) { return tunnel_.valid(); } + void dial(AsyncCall &call) { ((&(*tunnel_))->*method_)(answer_); } + virtual void print(std::ostream &os) const { + os << '(' << tunnel_.get() << ", " << answer_ << ')'; + } + + /* Http::Tunneler::CbDialer API */ + virtual Http::TunnelerAnswer &answer() { return answer_; } + + private: + Method method_; + CbcPointer tunnel_; + Http::TunnelerAnswer answer_; + }; + + /// resumes operations after the (possibly failed) HTTP CONNECT exchange + void tunnelEstablishmentDone(Http::TunnelerAnswer &answer); + public: bool keepGoingAfterRead(size_t len, Comm::Flag errcode, int xerrno, Connection &from, Connection &to); void copy(size_t len, Connection &from, Connection &to, IOCB *); - void handleConnectResponse(const size_t chunkSize); void readServer(char *buf, size_t len, Comm::Flag errcode, int xerrno); void readClient(char *buf, size_t len, Comm::Flag errcode, int xerrno); void writeClientDone(char *buf, size_t len, Comm::Flag flag, int xerrno); void writeServerDone(char *buf, size_t len, Comm::Flag flag, int xerrno); - static void ReadConnectResponseDone(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag errcode, int xerrno, void *data); - void readConnectResponseDone(char *buf, size_t len, Comm::Flag errcode, int xerrno); void copyClientBytes(); void copyServerBytes(); }; @@ -249,8 +271,6 @@ static CLCB tunnelClientClosed; static CTCB tunnelTimeout; static EVH tunnelDelayedClientRead; static EVH tunnelDelayedServerRead; -static void tunnelConnected(const Comm::ConnectionPointer &server, void *); -static void tunnelRelayConnectRequest(const Comm::ConnectionPointer &server, void *); static void tunnelServerClosed(const CommCloseCbParams ¶ms) @@ -426,131 +446,6 @@ TunnelStateData::readServer(char *, size_t len, Comm::Flag errcode, int xerrno) copy(len, server, client, WriteClientDone); } -/// Called when we read [a part of] CONNECT response from the peer -void -TunnelStateData::readConnectResponseDone(char *, size_t len, Comm::Flag errcode, int xerrno) -{ - debugs(26, 3, server.conn << ", read " << len << " bytes, err=" << errcode); - assert(waitingForConnectResponse()); - - if (errcode == Comm::ERR_CLOSING) - return; - - if (len > 0) { - connectRespBuf->appended(len); - server.bytesIn(len); - statCounter.server.all.kbytes_in += len; - statCounter.server.other.kbytes_in += len; - request->hier.notePeerRead(); - } - - if (keepGoingAfterRead(len, errcode, xerrno, server, client)) - handleConnectResponse(len); -} - -void -TunnelStateData::informUserOfPeerError(const char *errMsg, const size_t sz) -{ - server.len = 0; - - if (logTag_ptr) - logTag_ptr->update(LOG_TCP_TUNNEL); - - if (!clientExpectsConnectResponse()) { - // closing the connection is the best we can do here - debugs(50, 3, server.conn << " closing on error: " << errMsg); - server.conn->close(); - return; - } - - // if we have no reply suitable to relay, use 502 Bad Gateway - if (!sz || sz > static_cast(connectRespBuf->contentSize())) - return sendError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al), - "peer error without reply"); - - // if we need to send back the server response. write its headers to the client - server.len = sz; - memcpy(server.buf, connectRespBuf->content(), server.len); - copy(server.len, server, client, TunnelStateData::WriteClientDone); - // then close the server FD to prevent any relayed keep-alive causing CVE-2015-5400 - server.closeIfOpen(); -} - -/* Read from client side and queue it for writing to the server */ -void -TunnelStateData::ReadConnectResponseDone(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag errcode, int xerrno, void *data) -{ - TunnelStateData *tunnelState = (TunnelStateData *)data; - assert (cbdataReferenceValid (tunnelState)); - - tunnelState->readConnectResponseDone(buf, len, errcode, xerrno); -} - -/// Parses [possibly incomplete] CONNECT response and reacts to it. -/// If the tunnel is being closed or more response data is needed, returns false. -/// Otherwise, the caller should handle the remaining read data, if any. -void -TunnelStateData::handleConnectResponse(const size_t chunkSize) -{ - assert(waitingForConnectResponse()); - - // Ideally, client and server should use MemBuf or better, but current code - // never accumulates more than one read when shoveling data (XXX) so it does - // not need to deal with MemBuf complexity. To keep it simple, we use a - // dedicated MemBuf for accumulating CONNECT responses. TODO: When shoveling - // is optimized, reuse server.buf for CONNEC response accumulation instead. - - /* mimic the basic parts of HttpStateData::processReplyHeader() */ - HttpReply rep; - Http::StatusCode parseErr = Http::scNone; - const bool eof = !chunkSize; - connectRespBuf->terminate(); // Http::Message::parse requires terminated string - const bool parsed = rep.parse(connectRespBuf->content(), connectRespBuf->contentSize(), eof, &parseErr); - if (!parsed) { - if (parseErr > 0) { // unrecoverable parsing error - informUserOfPeerError("malformed CONNECT response from peer", 0); - return; - } - - // need more data - assert(!eof); - assert(!parseErr); - - if (!connectRespBuf->hasSpace()) { - informUserOfPeerError("huge CONNECT response from peer", 0); - return; - } - - // keep reading - readConnectResponse(); - return; - } - - // CONNECT response was successfully parsed - request->hier.peer_reply_status = rep.sline.status(); - - // bail if we did not get an HTTP 200 (Connection Established) response - if (rep.sline.status() != Http::scOkay) { - // if we ever decide to reuse the peer connection, we must extract the error response first - *status_ptr = rep.sline.status(); // we are relaying peer response - informUserOfPeerError("unsupported CONNECT response status code", rep.hdr_sz); - return; - } - - if (rep.hdr_sz < connectRespBuf->contentSize()) { - // preserve bytes that the server already sent after the CONNECT response - server.len = connectRespBuf->contentSize() - rep.hdr_sz; - memcpy(server.buf, connectRespBuf->content()+rep.hdr_sz, server.len); - } else { - // reset; delay pools were using this field to throttle CONNECT response - server.len = 0; - } - - delete connectRespBuf; - connectRespBuf = NULL; - connectExchangeCheckpoint(); -} - void TunnelStateData::Connection::error(int const xerrno) { @@ -834,17 +729,6 @@ TunnelStateData::copyRead(Connection &from, IOCB *completion) comm_read(from.conn, from.buf, bw, call); } -void -TunnelStateData::readConnectResponse() -{ - assert(waitingForConnectResponse()); - - AsyncCall::Pointer call = commCbCall(5,4, "readConnectResponseDone", - CommIoCbPtrFun(ReadConnectResponseDone, this)); - comm_read(server.conn, connectRespBuf->space(), - server.bytesWanted(1, connectRespBuf->spaceSize()), call); -} - void TunnelStateData::copyClientBytes() { @@ -930,60 +814,70 @@ tunnelConnectedWriteDone(const Comm::ConnectionPointer &conn, char *, size_t len tunnelStartShoveling(tunnelState); } -/// Called when we are done writing CONNECT request to a peer. -static void -tunnelConnectReqWriteDone(const Comm::ConnectionPointer &conn, char *, size_t ioSize, Comm::Flag flag, int, void *data) +void +TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) { - TunnelStateData *tunnelState = (TunnelStateData *)data; - debugs(26, 3, conn << ", flag=" << flag); - tunnelState->server.writer = NULL; - assert(tunnelState->waitingForConnectRequest()); + server.len = 0; - tunnelState->request->hier.notePeerWrite(); + if (logTag_ptr) + logTag_ptr->update(LOG_TCP_TUNNEL); - if (flag != Comm::OK) { - *tunnelState->status_ptr = Http::scInternalServerError; - tunnelErrorComplete(conn->fd, data, 0); + if (answer.peerResponseStatus != Http::scNone) + *status_ptr = answer.peerResponseStatus; + + if (answer.positive()) { + // XXX: Copy any post-200 OK bytes from answer to our buffer! + notePeerReadyToShovel(); return; } - statCounter.server.all.kbytes_out += ioSize; - statCounter.server.other.kbytes_out += ioSize; + // TODO: Reuse to-peer connections after a CONNECT error response. - tunnelState->connectReqWriting = false; - tunnelState->connectExchangeCheckpoint(); -} + // TODO: We can and, hence, should close now, but tunnelServerClosed() + // cannot yet tell whether ErrorState is still writing an error response. + // server.closeIfOpen(); -void -TunnelStateData::connectExchangeCheckpoint() -{ - if (waitingForConnectResponse()) { - debugs(26, 5, "still reading CONNECT response on " << server.conn); - } else if (waitingForConnectRequest()) { - debugs(26, 5, "still writing CONNECT request on " << server.conn); - } else { - assert(!waitingForConnectExchange()); - debugs(26, 3, "done with CONNECT exchange on " << server.conn); - tunnelConnected(server.conn, this); + if (!clientExpectsConnectResponse()) { + // closing the non-HTTP client connection is the best we can do + debugs(50, 3, server.conn << " closing on CONNECT-to-peer error"); + server.closeIfOpen(); // TODO: Add client.closeIfOpen(). + return; } + + if (ErrorState *error = answer.squidError.get()) { + answer.squidError.clear(); // preserve error for errorSendComplete() + sendError(error, "Error while connecting to peer"); + return; + } + + assert(!answer.peerError.isEmpty()); + + // if we cannot relay the peer response, generate 502 (Bad Gateway) + if (answer.peerError.length() > SQUID_TCP_SO_RCVBUF) { + *status_ptr = Http::scBadGateway; + sendError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al), "long peer response"); + return; + } + + // send back the actual peer response to the client + server.len = answer.peerError.length(); + assert(server.len <= SQUID_TCP_SO_RCVBUF); + memcpy(server.buf, answer.peerError.rawContent(), server.len); + copy(server.len, server, client, TunnelStateData::WriteClientDone); + // then close the server FD to prevent any relayed keep-alive causing CVE-2015-5400 + server.closeIfOpen(); } -/* - * handle the write completion from a proxy request to an upstream origin - */ -static void -tunnelConnected(const Comm::ConnectionPointer &server, void *data) +void +TunnelStateData::notePeerReadyToShovel() { - TunnelStateData *tunnelState = (TunnelStateData *)data; - debugs(26, 3, HERE << server << ", tunnelState=" << tunnelState); - - if (!tunnelState->clientExpectsConnectResponse()) - tunnelStartShoveling(tunnelState); // ssl-bumped connection, be quiet + if (!clientExpectsConnectResponse()) + tunnelStartShoveling(this); // ssl-bumped connection, be quiet else { - *tunnelState->status_ptr = Http::scOkay; + *status_ptr = Http::scOkay; AsyncCall::Pointer call = commCbCall(5,5, "tunnelConnectedWriteDone", - CommIoCbPtrFun(tunnelConnectedWriteDone, tunnelState)); - tunnelState->client.write(conn_established, strlen(conn_established), call, NULL); + CommIoCbPtrFun(tunnelConnectedWriteDone, this)); + client.write(conn_established, strlen(conn_established), call, NULL); } } @@ -1061,23 +955,24 @@ tunnelConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xe tunnelState->request->peer_host = conn->getPeer() ? conn->getPeer()->host : NULL; comm_add_close_handler(conn->fd, tunnelServerClosed, tunnelState); - debugs(26, 4, HERE << "determine post-connect handling pathway."); if (conn->getPeer()) { tunnelState->request->peer_login = conn->getPeer()->login; tunnelState->request->peer_domain = conn->getPeer()->domain; tunnelState->request->flags.auth_no_keytab = conn->getPeer()->options.auth_no_keytab; tunnelState->request->flags.proxying = !(conn->getPeer()->options.originserver); + debugs(26, 4, "post-connect(2) proxying: " << tunnelState->request->flags.proxying); } else { tunnelState->request->peer_login = NULL; tunnelState->request->peer_domain = NULL; tunnelState->request->flags.auth_no_keytab = false; tunnelState->request->flags.proxying = false; + debugs(26, 4, "post-connect(2) proxying: direct"); } if (tunnelState->request->flags.proxying) tunnelState->connectToPeer(); else { - tunnelConnected(conn, tunnelState); + tunnelState->notePeerReadyToShovel(); } AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", @@ -1158,52 +1053,20 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) return; } - tunnelRelayConnectRequest(server.conn, this); -} - -static void -tunnelRelayConnectRequest(const Comm::ConnectionPointer &srv, void *data) -{ - TunnelStateData *tunnelState = (TunnelStateData *)data; - assert(!tunnelState->waitingForConnectExchange()); - HttpHeader hdr_out(hoRequest); - Http::StateFlags flags; - debugs(26, 3, HERE << srv << ", tunnelState=" << tunnelState); - flags.proxying = tunnelState->request->flags.proxying; - MemBuf mb; - mb.init(); - mb.appendf("CONNECT %s HTTP/1.1\r\n", tunnelState->url); - HttpStateData::httpBuildRequestHeader(tunnelState->request.getRaw(), - NULL, /* StoreEntry */ - tunnelState->al, /* AccessLogEntry */ - &hdr_out, - flags); /* flags */ - hdr_out.packInto(&mb); - hdr_out.clean(); - mb.append("\r\n", 2); - - debugs(11, 2, "Tunnel Server REQUEST: " << tunnelState->server.conn << - ":\n----------\n" << mb.buf << "\n----------"); - - AsyncCall::Pointer writeCall = commCbCall(5,5, "tunnelConnectReqWriteDone", - CommIoCbPtrFun(tunnelConnectReqWriteDone, - tunnelState)); - - tunnelState->server.write(mb.buf, mb.size, writeCall, mb.freeFunc()); - tunnelState->connectReqWriting = true; - - tunnelState->connectRespBuf = new MemBuf; - // SQUID_TCP_SO_RCVBUF: we should not accumulate more than regular I/O buffer - // can hold since any CONNECT response leftovers have to fit into server.buf. - // 2*SQUID_TCP_SO_RCVBUF: Http::Message::parse() zero-terminates, which uses space. - tunnelState->connectRespBuf->init(SQUID_TCP_SO_RCVBUF, 2*SQUID_TCP_SO_RCVBUF); - tunnelState->readConnectResponse(); - - assert(tunnelState->waitingForConnectExchange()); - - AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", - CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); - commSetConnTimeout(srv, Config.Timeout.read, timeoutCall); + assert(!waitingForConnectExchange()); + // XXX: make waitingForConnectExchange a boolean; set it after Start() below + + AsyncCall::Pointer callback = asyncCall(5,4, + "TunnelStateData::tunnelEstablishmentDone", + MyAnswerDialer2(&TunnelStateData::tunnelEstablishmentDone, this)); + const auto tunneler = new Http::Tunneler(callback); + tunneler->connection = server.conn; + tunneler->al = al; + tunneler->request = request; + tunneler->url = url; + tunneler->lifetimeLimit = Config.Timeout.lifetime; + AsyncJob::Start(tunneler); + // and wait for the tunnelEstablishmentDone() call } static Comm::ConnectionPointer From 6bf68d7486965735dc91c1c0bdaa16a26d364748 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 23 Mar 2018 10:48:07 -0600 Subject: [PATCH 02/59] Documented flag.proxying inconsistency and suggested a fix Also, the flag should be computed in FwdState (when a peer is selected) rather than in (various places inside) http.cc. --- src/http.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/http.cc b/src/http.cc index 0a96bd950e7..03b399e8ef0 100644 --- a/src/http.cc +++ b/src/http.cc @@ -100,6 +100,8 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : _peer = cbdataReference(fwd->serverConnection()->getPeer()); /* might be NULL */ if (_peer) { + // XXX: tunnel.cc and sendRequest() exclude originserver from proxying! + // TODO: split {proxying,originserver} into {peering,toProxy,toOrigin} request->flags.proxying = true; /* * This NEIGHBOR_PROXY_ONLY check probably shouldn't be here. From 37b7d27cf5145d64cb55f0022d89d90df92bae73 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 23 Mar 2018 20:03:57 -0600 Subject: [PATCH 03/59] Use the new Http::Tunneler in FwdState Just like tunnel.cc, FwdState can now establish HTTP CONNECT tunnels through HTTP proxies. Unlike tunnel.cc, FwdState cannot yet forward virgin peer errors to clients (TODO). Both clients need polishing. HTTPS proxies are not yet supported primarily because Squid cannot do TLS-in-TLS with a single fde::ssl field (both SslBump and an HTTPS proxy connection would need one TLS connection field for themselves). We could support HTTPS proxies when not using SslBump, but such exceptional support would probably make the already mind boggling "do we need TLS here?" conditions even worse. --- src/FwdState.cc | 133 ++++++++++++++++++++++++++++++++++++++++++++---- src/FwdState.h | 6 +++ 2 files changed, 130 insertions(+), 9 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 6c7a0147c47..68eb1e3169a 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -18,6 +18,8 @@ #include "CachePeer.h" #include "client_side.h" #include "clients/forward.h" +#include "clients/HttpTunneler.h" +#include "clients/HttpTunnelerAnswer.h" #include "comm/Connection.h" #include "comm/ConnOpener.h" #include "comm/Loops.h" @@ -101,6 +103,33 @@ class FwdStatePeerAnswerDialer: public CallDialer, public Security::PeerConnecto Security::EncryptorAnswer answer_; }; +// XXX: Find a better way to pass answers than to create a dedicated class +// for each peer connector/tunneler! +/// Gives Http::Tunneler access to Answer in the FwdState callback dialer. +class FwdStatePeerAnswerDialer2: public CallDialer, public Http::Tunneler::CbDialer +{ +public: + typedef void (FwdState::*Method)(Http::TunnelerAnswer &); + + FwdStatePeerAnswerDialer2(Method method, FwdState *fwd): + method_(method), fwd_(fwd), answer_() {} + + /* CallDialer API */ + virtual bool canDial(AsyncCall &call) { return fwd_.valid(); } + void dial(AsyncCall &call) { ((&(*fwd_))->*method_)(answer_); } + virtual void print(std::ostream &os) const { + os << '(' << fwd_.get() << ", " << answer_ << ')'; + } + + /* Http::Tunneler::CbDialer API */ + virtual Http::TunnelerAnswer &answer() { return answer_; } + +private: + Method method_; + CbcPointer fwd_; + Http::TunnelerAnswer answer_; +}; + void FwdState::abort(void* d) { @@ -709,9 +738,12 @@ FwdState::handleUnregisteredServerEnd() retryOrBail(); } +/// handles an established TCP connection to peer (including origin servers) void FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xerrno) { + calls.connector = nullptr; // TODO: Remove calls.connector? + if (status != Comm::OK) { ErrorState *const anErr = makeConnectingError(ERR_CONNECT_FAIL); anErr->xerrno = xerrno; @@ -737,6 +769,80 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in // only set when we dispatch the request to an existing (pinned) connection. assert(!request->flags.pinned); + if (const CachePeer *peer = serverConnection()->getPeer()) { + // We need a CONNECT tunnel to send encrypted traffic through a proxy, + // but we do not support TLS inside TLS, so we exclude HTTPS proxies. + const bool originWantsEncryptedTraffic = + request->method == Http::METHOD_CONNECT || + request->flags.sslPeek; + if (originWantsEncryptedTraffic && // the "encrypted traffic" part + !peer->options.originserver && // the "through a proxy" part + !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part + return establishTunnelThruProxy(); + } + + secureConnectionToPeerIfNeeded(); +} + +void +FwdState::establishTunnelThruProxy() +{ + AsyncCall::Pointer callback = asyncCall(17,4, + "FwdState::tunnelEstablishmentDone", + FwdStatePeerAnswerDialer2(&FwdState::tunnelEstablishmentDone, this)); + const auto tunneler = new Http::Tunneler(callback); + tunneler->connection = serverConnection(); + tunneler->al = al; + tunneler->request = request; + tunneler->url = request->url.authority(); + // TODO: Refactor to avoid code duplication. + const auto connTimeout = serverDestinations[0]->connectTimeout(start_t); + tunneler->lifetimeLimit = positiveTimeout(connTimeout); + AsyncJob::Start(tunneler); + // and wait for the tunnelEstablishmentDone() call +} + +/// resumes operations after the (possibly failed) HTTP CONNECT exchange +void +FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) +{ + if (answer.positive()) { + // XXX: Copy any post-200 OK bytes from answer to our buffer! + secureConnectionToPeerIfNeeded(); + return; + } + + // TODO: Reuse to-peer connections after a CONNECT error response. + + if (const auto peer = serverConnection()->getPeer()) + peerConnectFailed(peer); + + if (const auto error = answer.squidError.get()) { + answer.squidError.clear(); // preserve error for fail() + fail(error); + closeServerConnection("Squid-generated CONNECT error"); + retryOrBail(); + return; + } + + assert(!answer.peerError.isEmpty()); + + // XXX: Stuffing raw answer.peerError SBuf into Store feels wrong so we + // replace that raw peer error with our generic error page, recreating bugs + // fixed in master commits 971003b and 4d27d0a! + // TODO: Revise how Tunneler reports peer error responses, + // possibly using an HttpReply *ErrorState::response_ field or similar. + fail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request, al)); + closeServerConnection("peer-generated CONNECT error"); + retryOrBail(); +} + +/// handles an established TCP connection to peer (including origin servers) +void +FwdState::secureConnectionToPeerIfNeeded() +{ + assert(!request->flags.pinned); + const CachePeer *p = serverConnection()->getPeer(); const bool peerWantsTls = p && p->secure.encryptTransport; // userWillTlsToPeerForUs assumes CONNECT == HTTPS @@ -764,10 +870,10 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in } // if not encrypting just run the post-connect actions - Security::EncryptorAnswer nil; - connectedToPeer(nil); + successfullyConnectedToPeer(); } +/// called when all negotiations with the TLS-speaking peer have been completed void FwdState::connectedToPeer(Security::EncryptorAnswer &answer) { @@ -790,6 +896,13 @@ FwdState::connectedToPeer(Security::EncryptorAnswer &answer) return; } + successfullyConnectedToPeer(); +} + +/// called when all negotiations with the peer have been completed +void +FwdState::successfullyConnectedToPeer() +{ // should reach ConnStateData before the dispatched Client job starts CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, ConnStateData::notePeerConnection, serverConnection()); @@ -877,13 +990,15 @@ FwdState::connectStart() request->hier.startPeerClock(); - // Do not fowrward bumped connections to parent proxy unless it is an - // origin server - if (serverDestinations[0]->getPeer() && !serverDestinations[0]->getPeer()->options.originserver && request->flags.sslBumped) { - debugs(50, 4, "fwdConnectStart: Ssl bumped connections through parent proxy are not allowed"); - const auto anErr = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al); - fail(anErr); - stopAndDestroy("SslBump misconfiguration"); + // Bumped requests require their pinned connection. Since we failed to reuse + // the pinned connection, we now must terminate the bumped request. + if (request->flags.sslBumped) { + // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]). + static int occurrences = 0; + const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; + debugs(17, level, "BUG: Lost previously bumped from-Squid connection. Rejecting bumped request."); + fail(new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al)); + self = NULL; // refcounted return; } diff --git a/src/FwdState.h b/src/FwdState.h index 57707026b72..a46ff2648c5 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -10,6 +10,7 @@ #define SQUID_FORWARD_H #include "base/RefCount.h" +#include "clients/forward.h" #include "comm.h" #include "comm/Connection.h" #include "err_type.h" @@ -134,6 +135,11 @@ class FwdState: public RefCountable, public PeerSelectionInitiator void connectedToPeer(Security::EncryptorAnswer &answer); static void RegisterWithCacheManager(void); + void establishTunnelThruProxy(); + void tunnelEstablishmentDone(Http::TunnelerAnswer &answer); + void secureConnectionToPeerIfNeeded(); + void successfullyConnectedToPeer(); + /// stops monitoring server connection for closure and updates pconn stats void closeServerConnection(const char *reason); From 5464afd59b6ef5eba8351ee9a3bd5ac686706e01 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 23 Mar 2018 20:27:56 -0600 Subject: [PATCH 04/59] Support post-200 OK "early" bytes on the tunnel.cc side --- src/tunnel.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tunnel.cc b/src/tunnel.cc index 923fa45f01a..fc03591dfb0 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -826,7 +826,8 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) *status_ptr = answer.peerResponseStatus; if (answer.positive()) { - // XXX: Copy any post-200 OK bytes from answer to our buffer! + // copy any post-200 OK bytes to our buffer + preReadServerData = answer.leftovers; notePeerReadyToShovel(); return; } From 4fe4bbedde9dee2c8618d0341123009a00ad62de Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 23 Mar 2018 22:23:40 -0600 Subject: [PATCH 05/59] Reject post-200 OK "early" bytes on the FwdState side ... because we do not have a ready-to-use mechanism to supply them to the PeerConnector and because TLS servers should not send them. --- src/FwdState.cc | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 68eb1e3169a..2f3e055fe0f 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -807,8 +807,21 @@ void FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) { if (answer.positive()) { - // XXX: Copy any post-200 OK bytes from answer to our buffer! - secureConnectionToPeerIfNeeded(); + if (answer.leftovers.isEmpty()) { + secureConnectionToPeerIfNeeded(); + return; + } + // This should not happen because TLS servers do not speak first. If we + // have to handle this, then pass answer.leftovers via a PeerConnector + // to ServerBio. See ClientBio::setReadBufData(). + static int occurrences = 0; + const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; + debugs(17, level, "ERROR: Early data after CONNECT response. " << + "Found " << answer.leftovers.length() << " bytes. " << + "Closing " << serverConnection()); + fail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request)); + closeServerConnection("found early data after CONNECT response"); + retryOrBail(); return; } From f42fe88bdcccf46d7a72ae9d46b1e964f54e6d08 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 10:56:49 +0300 Subject: [PATCH 06/59] FwdState::ConnectingTimeout Investigate the FwdState::ConnectingTimeout method to avoid code duplication --- src/FwdState.cc | 17 ++++++++++------- src/FwdState.h | 3 +++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 2f3e055fe0f..0b90d1b1a40 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -795,9 +795,7 @@ FwdState::establishTunnelThruProxy() tunneler->al = al; tunneler->request = request; tunneler->url = request->url.authority(); - // TODO: Refactor to avoid code duplication. - const auto connTimeout = serverDestinations[0]->connectTimeout(start_t); - tunneler->lifetimeLimit = positiveTimeout(connTimeout); + tunneler->lifetimeLimit = connectingTimeout(serverDestinations[0]); AsyncJob::Start(tunneler); // and wait for the tunnelEstablishmentDone() call } @@ -868,9 +866,7 @@ FwdState::secureConnectionToPeerIfNeeded() AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::ConnectedToPeer", FwdStatePeerAnswerDialer(&FwdState::connectedToPeer, this)); - // Use positive timeout when less than one second is left. - const time_t connTimeout = serverDestinations[0]->connectTimeout(start_t); - const time_t sslNegotiationTimeout = positiveTimeout(connTimeout); + const time_t sslNegotiationTimeout = connectingTimeout(serverDestinations[0]); Security::PeerConnector *connector = nullptr; #if USE_OPENSSL if (request->flags.sslPeek) @@ -1058,7 +1054,7 @@ FwdState::connectStart() GetMarkingsToServer(request, *serverDestinations[0]); calls.connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); - const time_t connTimeout = serverDestinations[0]->connectTimeout(start_t); + const time_t connTimeout = connectingTimeout(serverDestinations[0]); Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], calls.connector, connTimeout); if (host) cs->setHost(host); @@ -1443,6 +1439,13 @@ FwdState::pinnedCanRetry() const return true; } +time_t +FwdState::connectingTimeout(const Comm::ConnectionPointer &conn) const +{ + const auto connTimeout = conn->connectTimeout(start_t); + return positiveTimeout(connTimeout); +} + /**** PRIVATE NON-MEMBER FUNCTIONS ********************************************/ /* diff --git a/src/FwdState.h b/src/FwdState.h index a46ff2648c5..de623d207bd 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -149,6 +149,9 @@ class FwdState: public RefCountable, public PeerSelectionInitiator /// whether we have used up all permitted forwarding attempts bool exhaustedTries() const; + /// \returns the time left for this connection to become connected or 1 second if it is less than one second left + time_t connectingTimeout(const Comm::ConnectionPointer &conn) const; + public: StoreEntry *entry; HttpRequest *request; From d4e31024998efd92c52248ddb26c6fd4da6e05fc Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 12:10:29 +0300 Subject: [PATCH 07/59] Comm::SetClientObjectReadTimeout new function It can be used to set read timeout based on objects lifetime. This function replaces the HttpTunneler::setReadTimeout and PeerConnector::setReadTimeout methods. --- src/clients/HttpTunneler.cc | 23 +++++------------------ src/clients/HttpTunneler.h | 1 - src/comm/Read.cc | 14 ++++++++++++++ src/comm/Read.h | 5 +++++ src/security/PeerConnector.cc | 21 ++++++--------------- src/security/PeerConnector.h | 4 ---- 6 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 41febe10303..7f016330da3 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -95,23 +95,6 @@ Http::Tunneler::watchForClosures() comm_add_close_handler(connection->fd, closer); } -void -Http::Tunneler::setReadTimeout() -{ - // TODO: polish to use time_t and extract to reduce duplication with similar code in PeerConnector - int timeToRead; - if (lifetimeLimit > 0) { - const int timeUsed = squid_curtime - startTime; - const int timeLeft = max(0, static_cast(lifetimeLimit - timeUsed)); - timeToRead = min(static_cast(::Config.Timeout.read), timeLeft); - } else - timeToRead = ::Config.Timeout.read; - // TODO: Same as PeerConnector, but who removes this timeout after we send a - // positive answer? Should not we clean after ourselves in swanSong()? - AsyncCall::Pointer nil; - commSetConnTimeout(connection, timeToRead, nil); -} - void Http::Tunneler::handleException(const std::exception& e) { @@ -243,7 +226,11 @@ Http::Tunneler::readMore() typedef CommCbMemFunT Dialer; reader = JobCallback(93, 3, Dialer, this, Http::Tunneler::handleReadyRead); Comm::Read(connection, reader); - setReadTimeout(); + + // TODO: Same as PeerConnector, but who removes this timeout after we send a + // positive answer? Should not we clean after ourselves in swanSong()? + AsyncCall::Pointer nil; + Comm::SetClientObjectReadTimeout(connection, startTime, lifetimeLimit, nil); } /// Parses [possibly incomplete] CONNECT response and reacts to it. diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index 975fc339276..2c5876ec346 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -70,7 +70,6 @@ class Tunneler: virtual public AsyncJob void handleConnectionClosure(const CommCloseCbParams&); void watchForClosures(); - void setReadTimeout(); void handleException(const std::exception&); void startReadingResponse(); void writeRequest(); diff --git a/src/comm/Read.cc b/src/comm/Read.cc index c8f290885e1..affd06b766f 100644 --- a/src/comm/Read.cc +++ b/src/comm/Read.cc @@ -19,6 +19,7 @@ #include "fd.h" #include "fde.h" #include "sbuf/SBuf.h" +#include "SquidConfig.h" #include "StatCounters.h" // Does comm check this fd for read readiness? @@ -241,3 +242,16 @@ Comm::ReadCancel(int fd, AsyncCall::Pointer &callback) Comm::SetSelect(fd, COMM_SELECT_READ, NULL, NULL, 0); } +void +Comm::SetClientObjectReadTimeout(const Comm::ConnectionPointer &conn, time_t startTime, time_t lifetimeLimit, AsyncCall::Pointer &callback) +{ + time_t timeToRead; + if (lifetimeLimit > 0) { + Must(squid_curtime >= startTime); + const time_t timeUsed = squid_curtime - startTime; + const time_t timeLeft = (lifetimeLimit > timeUsed) ? (lifetimeLimit - timeUsed) : 0; + timeToRead = min(::Config.Timeout.read, timeLeft); + } else + timeToRead = ::Config.Timeout.read; + commSetConnTimeout(conn, timeToRead, callback); +} diff --git a/src/comm/Read.h b/src/comm/Read.h index ab65864752e..4222140e7af 100644 --- a/src/comm/Read.h +++ b/src/comm/Read.h @@ -50,6 +50,11 @@ void ReadCancel(int fd, AsyncCall::Pointer &callback); /// callback handler to process an FD which is available for reading extern PF HandleRead; +/** + * Sets a read timeout based on configured read timeout and caller lifetime + * limit and start time + */ +void SetClientObjectReadTimeout(const Comm::ConnectionPointer &conn, time_t startTime, time_t lifeTimeLimit, AsyncCall::Pointer &callback); } // namespace Comm // Legacy API to be removed diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 050c8a747d7..5fe276b8bf7 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "acl/FilledChecklist.h" #include "comm/Loops.h" +#include "comm/Read.h" #include "Downloader.h" #include "errorpage.h" #include "fde.h" @@ -141,20 +142,6 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) return true; } -void -Security::PeerConnector::setReadTimeout() -{ - int timeToRead; - if (negotiationTimeout) { - const int timeUsed = squid_curtime - startTime; - const int timeLeft = max(0, static_cast(negotiationTimeout - timeUsed)); - timeToRead = min(static_cast(::Config.Timeout.read), timeLeft); - } else - timeToRead = ::Config.Timeout.read; - AsyncCall::Pointer nil; - commSetConnTimeout(serverConnection(), timeToRead, nil); -} - void Security::PeerConnector::recordNegotiationDetails() { @@ -482,7 +469,11 @@ Security::PeerConnector::noteWantRead() } } #endif - setReadTimeout(); + + // read timeout to avoid getting stuck while reading from a silent server + AsyncCall::Pointer nil; + Comm::SetClientObjectReadTimeout(serverConnection(), startTime, negotiationTimeout, nil); + Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, new Pointer(this), 0); } diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h index 581503b6cf7..63b48cbece7 100644 --- a/src/security/PeerConnector.h +++ b/src/security/PeerConnector.h @@ -101,10 +101,6 @@ class PeerConnector: virtual public AsyncJob /// handler to monitor the socket. bool prepareSocket(); - /// Sets the read timeout to avoid getting stuck while reading from a - /// silent server - void setReadTimeout(); - /// \returns true on successful TLS session initialization virtual bool initialize(Security::SessionPointer &); From 83089a7b57a0712c72d0e8e109812abff05de343 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 13:05:44 +0300 Subject: [PATCH 08/59] Remove the unneeded Http::TunnelerAnswer::conn The connection object to be used with HttpTunneler is passed by the caller, it does not make sense to report it back to the caller. --- src/clients/HttpTunnelerAnswer.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/clients/HttpTunnelerAnswer.h b/src/clients/HttpTunnelerAnswer.h index 0ed48eee1b0..7dc9f45eb6a 100644 --- a/src/clients/HttpTunnelerAnswer.h +++ b/src/clients/HttpTunnelerAnswer.h @@ -34,8 +34,6 @@ class TunnelerAnswer bool positive() const { return !squidError && peerError.isEmpty(); } - Comm::ConnectionPointer conn; ///< to-peer connection (XXX: remove?) - // answer recipients must clear the error member in order to keep its info // XXX: We should refcount ErrorState instead of cbdata-protecting it. CbcPointer squidError; ///< problem details (or nil) From 071bad1052065a3b2ac534d7c3b0178fc92d580e Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 16:17:57 +0300 Subject: [PATCH 09/59] report tunneler state in Http::Tunneler::status --- src/clients/HttpTunneler.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 7f016330da3..ab1c828cfb4 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -356,10 +356,13 @@ Http::Tunneler::status() const // TODO: redesign AsyncJob::status() API to avoid // id and stop reason reporting duplication. - buf.append(" [", 2); - // TODO: report true requestWritten, true tunnelEstablished, and nil callback + buf.append(" [state:", 8); + if (requestWritten) buf.append('w'); // request sent + if (tunnelEstablished) buf.append('t'); // tunnel established + if (!callback) buf.append('x'); // caller informed + if (stopReason != NULL) { - buf.append("Stopped, reason:", 16); + buf.append(" stopped, reason:", 16); buf.appendf("%s",stopReason); } if (connection != NULL) From 8a7fe7830fe81f7a513664efaef92b94d2b48a67 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 16:44:42 +0300 Subject: [PATCH 10/59] Convert comment for Http::Tunneler to a doxygen document --- src/clients/HttpTunneler.h | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index 2c5876ec346..4ba911cdcc2 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -22,17 +22,15 @@ typedef RefCount AccessLogEntryPointer; namespace Http { -// TODO: Reformat description using Doxygen. - -// Establishes an HTTP CONNECT tunnel through another proxy. -// -// The caller receives a call back with Http::TunnelerAnswer. -// -// The caller must monitor the connection for closure because this job will not -// inform the caller about such events. -// -// This job never closes the connection, even on errors. If a 3rd-party closes -// the connection, this job simply quits without informing the caller. +/// Establishes an HTTP CONNECT tunnel through another proxy. +/// +/// The caller receives a call back with Http::TunnelerAnswer. +/// +/// The caller must monitor the connection for closure because this job will not +/// inform the caller about such events. +/// +/// This job never closes the connection, even on errors. If a 3rd-party closes +/// the connection, this job simply quits without informing the caller. class Tunneler: virtual public AsyncJob { CBDATA_CLASS(Tunneler); From c982089d822bd57dc5ce223b7eae4fb9846d0e06 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 17:56:11 +0300 Subject: [PATCH 11/59] fix commit dff428997b22ec512fa111e2eb900aeeb94d5e59 --- src/clients/HttpTunneler.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index ab1c828cfb4..3506b197c0d 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -357,9 +357,9 @@ Http::Tunneler::status() const // TODO: redesign AsyncJob::status() API to avoid // id and stop reason reporting duplication. buf.append(" [state:", 8); - if (requestWritten) buf.append('w'); // request sent - if (tunnelEstablished) buf.append('t'); // tunnel established - if (!callback) buf.append('x'); // caller informed + if (requestWritten) buf.append("w", 1); // request sent + if (tunnelEstablished) buf.append("t", 1); // tunnel established + if (!callback) buf.append("x", 1); // caller informed if (stopReason != NULL) { buf.append(" stopped, reason:", 16); From 2da06ccbdf7304060209cd9b857f82aaecc63940 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 17:58:54 +0300 Subject: [PATCH 12/59] Remove TODO comment about client connection closure from TunnelStateData::tunnelEstablishmentDone No need to close the client connection. The tunnelServerClosed will close the client connection if required. --- src/tunnel.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tunnel.cc b/src/tunnel.cc index fc03591dfb0..76c39f5d528 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -841,7 +841,7 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) if (!clientExpectsConnectResponse()) { // closing the non-HTTP client connection is the best we can do debugs(50, 3, server.conn << " closing on CONNECT-to-peer error"); - server.closeIfOpen(); // TODO: Add client.closeIfOpen(). + server.closeIfOpen(); return; } From 57b4deb533bfd1d21be050e5ff19fa561cd2314f Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 18:16:33 +0300 Subject: [PATCH 13/59] Make the TunnelStateData::waitingForConnectExchange a simple bool member Remove the TunnelStateData::waitingForConnectRequest() method Remove the TunnelStateData::waitingForConnectResponse() method Remove the TunnelStateData::connectReqWriting member Remove the TunnelStateData::connectRespBuf member Make the waitingForConnectExchange() method a simple bool member and update it when the Tunneler starts and when the tunneler respond. --- src/tunnel.cc | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/tunnel.cc b/src/tunnel.cc index 76c39f5d528..681b7a6c979 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -93,13 +93,6 @@ class TunnelStateData: public PeerSelectionInitiator return (server.conn != NULL && server.conn->getPeer() ? server.conn->getPeer()->host : request->url.host()); }; - /// Whether we are writing a CONNECT request to a peer. - bool waitingForConnectRequest() const { return connectReqWriting; } - /// Whether we are reading a CONNECT response from a peer. - bool waitingForConnectResponse() const { return connectRespBuf; } - /// Whether we are waiting for the CONNECT request/response exchange with the peer. - bool waitingForConnectExchange() const { return waitingForConnectRequest() || waitingForConnectResponse(); } - /// Whether the client sent a CONNECT request to us. bool clientExpectsConnectResponse() const { // If we are forcing a tunnel after receiving a client CONNECT, then we @@ -168,11 +161,13 @@ class TunnelStateData: public PeerSelectionInitiator Connection client, server; int *status_ptr; ///< pointer for logging HTTP status LogTags *logTag_ptr; ///< pointer for logging Squid processing code - MemBuf *connectRespBuf; ///< accumulates peer CONNECT response when we need it - bool connectReqWriting; ///< whether we are writing a CONNECT request to a peer + SBuf preReadClientData; SBuf preReadServerData; time_t startTime; ///< object creation time, before any peer selection/connection attempts + /// Whether we are waiting for the CONNECT request/response exchange with the peer. + bool waitingForConnectExchange; + void copyRead(Connection &from, IOCB *completion); @@ -328,8 +323,6 @@ tunnelClientClosed(const CommCloseCbParams ¶ms) } TunnelStateData::TunnelStateData(ClientHttpRequest *clientRequest) : - connectRespBuf(NULL), - connectReqWriting(false), startTime(squid_curtime) { debugs(26, 3, "TunnelStateData constructed this=" << this); @@ -360,7 +353,6 @@ TunnelStateData::~TunnelStateData() assert(noConnections()); xfree(url); serverDestinations.clear(); - delete connectRespBuf; delete savedError; } @@ -764,7 +756,7 @@ TunnelStateData::copyServerBytes() static void tunnelStartShoveling(TunnelStateData *tunnelState) { - assert(!tunnelState->waitingForConnectExchange()); + assert(!tunnelState->waitingForConnectExchange); *tunnelState->status_ptr = Http::scOkay; if (tunnelState->logTag_ptr) tunnelState->logTag_ptr->update(LOG_TCP_TUNNEL); @@ -825,6 +817,8 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) if (answer.peerResponseStatus != Http::scNone) *status_ptr = answer.peerResponseStatus; + waitingForConnectExchange = false; + if (answer.positive()) { // copy any post-200 OK bytes to our buffer preReadServerData = answer.leftovers; @@ -1054,7 +1048,7 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) return; } - assert(!waitingForConnectExchange()); + assert(!waitingForConnectExchange); // XXX: make waitingForConnectExchange a boolean; set it after Start() below AsyncCall::Pointer callback = asyncCall(5,4, @@ -1067,6 +1061,7 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) tunneler->url = url; tunneler->lifetimeLimit = Config.Timeout.lifetime; AsyncJob::Start(tunneler); + waitingForConnectExchange = true; // and wait for the tunnelEstablishmentDone() call } From 4bba56e7e0cb1a5cf7406a3a1c9f100ca63c3e30 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Jun 2018 18:22:18 +0300 Subject: [PATCH 14/59] Remove a comment about removing the FwdState::calls.connector The calls.connector holds the AsyncCall callback passed to CommOpener object It is used to cancel the CommOpener when the FwdState is gone for a reason (eg because client closed the connection) --- src/FwdState.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 0b90d1b1a40..f6f13eb5552 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -742,7 +742,7 @@ FwdState::handleUnregisteredServerEnd() void FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xerrno) { - calls.connector = nullptr; // TODO: Remove calls.connector? + calls.connector = nullptr; if (status != Comm::OK) { ErrorState *const anErr = makeConnectingError(ERR_CONNECT_FAIL); From 59244d72f49902b1084a075cc9a7844d45b7392c Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 22 Jun 2018 16:58:37 +0300 Subject: [PATCH 15/59] Forward the server error response to the client if possible If the Http::Tunneler received a valid error response from peer forward it to the client instead of sending a squid generated error page. This patch add the ERR_RELAY_REMOTE err_type to describe ErrorState objects which are created over a peer HttpReply. Maybe the Http::Tunneler should wait to retrieve all of body of peer response. Currently waits and forwards only the HTTP headers. --- src/FwdState.cc | 24 ++++++---------------- src/clients/HttpTunneler.cc | 33 +++++++++++++++--------------- src/clients/HttpTunneler.h | 3 ++- src/clients/HttpTunnelerAnswer.cc | 2 -- src/clients/HttpTunnelerAnswer.h | 3 +-- src/err_type.h | 1 + src/errorpage.cc | 34 +++++++++++++++++++++++++++++++ src/errorpage.h | 7 +++++++ src/tunnel.cc | 26 ++++------------------- 9 files changed, 72 insertions(+), 61 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index f6f13eb5552..3818b27b539 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -817,7 +817,7 @@ FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) debugs(17, level, "ERROR: Early data after CONNECT response. " << "Found " << answer.leftovers.length() << " bytes. " << "Closing " << serverConnection()); - fail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request)); + fail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request, al)); closeServerConnection("found early data after CONNECT response"); retryOrBail(); return; @@ -828,23 +828,11 @@ FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) if (const auto peer = serverConnection()->getPeer()) peerConnectFailed(peer); - if (const auto error = answer.squidError.get()) { - answer.squidError.clear(); // preserve error for fail() - fail(error); - closeServerConnection("Squid-generated CONNECT error"); - retryOrBail(); - return; - } - - assert(!answer.peerError.isEmpty()); - - // XXX: Stuffing raw answer.peerError SBuf into Store feels wrong so we - // replace that raw peer error with our generic error page, recreating bugs - // fixed in master commits 971003b and 4d27d0a! - // TODO: Revise how Tunneler reports peer error responses, - // possibly using an HttpReply *ErrorState::response_ field or similar. - fail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request, al)); - closeServerConnection("peer-generated CONNECT error"); + const auto error = answer.squidError.get(); + Must(error); + answer.squidError.clear(); // preserve error for fail() + fail(error); + closeServerConnection("Squid-generated CONNECT error"); retryOrBail(); } diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 3506b197c0d..ec10432ee29 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -240,12 +240,12 @@ Http::Tunneler::handleResponse(const bool eof) // mimic the basic parts of HttpStateData::processReplyHeader() // TODO: HttpStateData::processReplyHeader() tries harder on eof! // TODO: Do not parse headers. Use Http1::ResponseParser. See HttpStateData::processReplyHeader(). - HttpReply rep; + HttpReply::Pointer rep = new HttpReply; Http::StatusCode parseErr = Http::scNone; - const bool parsed = rep.parse(readBuf.c_str(), readBuf.length(), eof, &parseErr); + const bool parsed = rep->parse(readBuf.c_str(), readBuf.length(), eof, &parseErr); if (!parsed) { if (parseErr > 0) { // unrecoverable parsing error - bailOnResponseError("malformed CONNECT response from peer", 0); + bailOnResponseError("malformed CONNECT response from peer", nullptr); return; } @@ -254,7 +254,7 @@ Http::Tunneler::handleResponse(const bool eof) assert(!parseErr); if (readBuf.length() >= SQUID_TCP_SO_RCVBUF) { - bailOnResponseError("huge CONNECT response from peer", 0); + bailOnResponseError("huge CONNECT response from peer", nullptr); return; } @@ -264,24 +264,24 @@ Http::Tunneler::handleResponse(const bool eof) // CONNECT response was successfully parsed auto &futureAnswer = answer(); - futureAnswer.peerResponseStatus = rep.sline.status(); - request->hier.peer_reply_status = rep.sline.status(); + futureAnswer.peerResponseStatus = rep->sline.status(); + request->hier.peer_reply_status = rep->sline.status(); // XXX: Raw() prints an extra leading space. TODO: Add/use Raw::gap(false). debugs(11, 2, "HTTP Server " << connection); debugs(11, 2, "HTTP Server RESPONSE:\n---------\n" << - Raw(nullptr, readBuf.rawContent(), rep.hdr_sz).minLevel(2) << + Raw(nullptr, readBuf.rawContent(), rep->hdr_sz).minLevel(2) << "----------"); // bail if we did not get an HTTP 200 (Connection Established) response - if (rep.sline.status() != Http::scOkay) { + if (rep->sline.status() != Http::scOkay) { // TODO: To reuse the connection, extract the whole error response. - bailOnResponseError("unsupported CONNECT response status code", rep.hdr_sz); + bailOnResponseError("unsupported CONNECT response status code", rep.getRaw()); return; } // preserve any bytes sent by the server after the CONNECT response - futureAnswer.leftovers = readBuf.substr(rep.hdr_sz); + futureAnswer.leftovers = readBuf.substr(rep->hdr_sz); // delay pools were using this field to throttle CONNECT response len = futureAnswer.leftovers.length(); @@ -290,17 +290,18 @@ Http::Tunneler::handleResponse(const bool eof) } void -Http::Tunneler::bailOnResponseError(const char *error, const size_t peerResponseSize) +Http::Tunneler::bailOnResponseError(const char *error, HttpReply *errorReply) { debugs(83, 3, error << status()); - if (!peerResponseSize) { - // with no reply suitable for relaying, answer with 502 (Bad Gateway) - bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); + ErrorState *err; + if (errorReply) { + err = new ErrorState(errorReply); } else { - answer().peerError = readBuf.substr(0, peerResponseSize); - callBack(); + // with no reply suitable for relaying, answer with 502 (Bad Gateway) + err = new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al); } + bailWith(err); } void diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index 4ba911cdcc2..c3df3036b61 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -15,6 +15,7 @@ #include "http/forward.h" #include "clients/forward.h" +class HttpReply; class ErrorState; class AccessLogEntry; typedef RefCount AccessLogEntryPointer; @@ -75,7 +76,7 @@ class Tunneler: virtual public AsyncJob void handleReadyRead(const CommIoCbParams&); void readMore(); void handleResponse(const bool eof); - void bailOnResponseError(const char *error, const size_t peerResponseSize); + void bailOnResponseError(const char *error, HttpReply *); void bailWith(ErrorState*); void callBack(); diff --git a/src/clients/HttpTunnelerAnswer.cc b/src/clients/HttpTunnelerAnswer.cc index fec1c6fe91d..9f3a037003b 100644 --- a/src/clients/HttpTunnelerAnswer.cc +++ b/src/clients/HttpTunnelerAnswer.cc @@ -23,8 +23,6 @@ Http::operator <<(std::ostream &os, const TunnelerAnswer &answer) if (const auto squidError = answer.squidError.get()) { os << "SquidErr:" << squidError->page_id; - } else if (const auto errBytes = answer.peerError.length()) { - os << "PeerErr:" << answer.peerError.length(); } else { os << "OK"; if (const auto extraBytes = answer.leftovers.length()) diff --git a/src/clients/HttpTunnelerAnswer.h b/src/clients/HttpTunnelerAnswer.h index 7dc9f45eb6a..331edc8a0b7 100644 --- a/src/clients/HttpTunnelerAnswer.h +++ b/src/clients/HttpTunnelerAnswer.h @@ -32,13 +32,12 @@ class TunnelerAnswer TunnelerAnswer() {} ~TunnelerAnswer(); ///< deletes squidError if it is still set - bool positive() const { return !squidError && peerError.isEmpty(); } + bool positive() const { return !squidError; } // answer recipients must clear the error member in order to keep its info // XXX: We should refcount ErrorState instead of cbdata-protecting it. CbcPointer squidError; ///< problem details (or nil) - SBuf peerError; ///< peer-generated error message header (or empty) SBuf leftovers; ///< peer-generated bytes after a positive answer (or empty) /// the status code of the successfully parsed CONNECT response (or scNone) diff --git a/src/err_type.h b/src/err_type.h index 97e774008d1..a925d6b5051 100644 --- a/src/err_type.h +++ b/src/err_type.h @@ -76,6 +76,7 @@ typedef enum { ERR_SECURE_ACCEPT_FAIL, // Rejects the SSL connection intead of error page ERR_REQUEST_START_TIMEOUT, // Aborts the connection instead of error page + ERR_RELAY_REMOTE, // Relay remotes server reply /* Cache Manager GUI can install a manager index/home page */ MGR_INDEX, diff --git a/src/errorpage.cc b/src/errorpage.cc index 7e867e877fe..ff1daf2ed32 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -687,6 +687,37 @@ ErrorState::ErrorState(err_type t, Http::StatusCode status, HttpRequest * req, c } } +ErrorState::ErrorState(HttpReply *errorReply) : + type(ERR_RELAY_REMOTE), + page_id(ERR_RELAY_REMOTE), + err_language(NULL), + httpStatus(Http::scNone), +#if USE_AUTH + auth_user_request (NULL), +#endif + request(NULL), + url(NULL), + xerrno(0), + port(0), + dnsError(), + ttl(0), + src_addr(), + redirect_url(NULL), + callback(NULL), + callback_data(NULL), + request_hdrs(NULL), + err_msg(NULL), +#if USE_OPENSSL + detail(NULL), +#endif + detailCode(ERR_DETAIL_NONE), + response_(errorReply) +{ + memset(&ftp, 0, sizeof(ftp)); + Must(errorReply); + httpStatus = errorReply->sline.status(); +} + void errorAppendEntry(StoreEntry * entry, ErrorState * err) { @@ -1260,6 +1291,9 @@ ErrorState::validate() HttpReply * ErrorState::BuildHttpReply() { + if (response_) + return response_.getRaw(); + HttpReply *rep = new HttpReply; const char *name = errorPageName(page_id); /* no LMT for error pages; error pages expire immediately */ diff --git a/src/errorpage.h b/src/errorpage.h index 002db460d65..f9df2d2bd06 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -78,6 +78,7 @@ typedef void ERCB(int fd, void *, size_t); class MemBuf; class StoreEntry; class wordlist; +typedef RefCount HttpReplyPointer; namespace ErrorPage { @@ -93,6 +94,10 @@ class ErrorState public: ErrorState(err_type type, Http::StatusCode, HttpRequest * request, const AccessLogEntryPointer &al); ErrorState() = delete; // not implemented. + + /// Build an ERR_RELAY_REMOTE ErrorState object + ErrorState(HttpReply *); + ~ErrorState(); /// Creates a general request forwarding error with the right http_status. @@ -199,6 +204,8 @@ class ErrorState /// overwrites xerrno; overwritten by detail, if any. int detailCode = ERR_DETAIL_NONE; + HttpReplyPointer response_; + private: void noteBuildError_(const char *msg, const char *near, const bool forceBypass); diff --git a/src/tunnel.cc b/src/tunnel.cc index 681b7a6c979..48819708f4b 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -839,28 +839,10 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) return; } - if (ErrorState *error = answer.squidError.get()) { - answer.squidError.clear(); // preserve error for errorSendComplete() - sendError(error, "Error while connecting to peer"); - return; - } - - assert(!answer.peerError.isEmpty()); - - // if we cannot relay the peer response, generate 502 (Bad Gateway) - if (answer.peerError.length() > SQUID_TCP_SO_RCVBUF) { - *status_ptr = Http::scBadGateway; - sendError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al), "long peer response"); - return; - } - - // send back the actual peer response to the client - server.len = answer.peerError.length(); - assert(server.len <= SQUID_TCP_SO_RCVBUF); - memcpy(server.buf, answer.peerError.rawContent(), server.len); - copy(server.len, server, client, TunnelStateData::WriteClientDone); - // then close the server FD to prevent any relayed keep-alive causing CVE-2015-5400 - server.closeIfOpen(); + ErrorState *error = answer.squidError.get(); + Must(error); + answer.squidError.clear(); // preserve error for errorSendComplete() + sendError(error, "peer error"); } void From f1ddf945fa8f7aa5e3c1cbce2c3601038f843585 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 22 Jun 2018 17:21:20 +0300 Subject: [PATCH 16/59] Add the Raw::gap method and use it to print received bytes in HttpTunneler This is can be used to omit leading space if the label is not defined --- src/Debug.h | 5 ++++- src/clients/HttpTunneler.cc | 3 +-- src/debug.cc | 5 ++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Debug.h b/src/Debug.h index 7fb1ed54944..756f498860e 100644 --- a/src/Debug.h +++ b/src/Debug.h @@ -185,7 +185,7 @@ class Raw { public: Raw(const char *label, const char *data, const size_t size): - level(-1), label_(label), data_(data), size_(size), useHex_(false) {} + level(-1), label_(label), data_(data), size_(size), useHex_(false), useGap_(true) {} /// limit data printing to at least the given debugging level Raw &minLevel(const int aLevel) { level = aLevel; return *this; } @@ -193,6 +193,8 @@ class Raw /// print data using two hex digits per byte (decoder: xxd -r -p) Raw &hex() { useHex_ = true; return *this; } + Raw &gap(bool useGap = true) { useGap_ = useGap; return *this; } + /// If debugging is prohibited by the current debugs() or section level, /// prints nothing. Otherwise, dumps data using one of these formats: /// " label[size]=data" if label was set and data size is positive @@ -213,6 +215,7 @@ class Raw const char *data_; ///< raw data to be printed size_t size_; ///< data length bool useHex_; ///< whether hex() has been called + bool useGap_; ///< whether to print leading space if label is missing }; inline diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index ec10432ee29..f5d53d80d40 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -267,10 +267,9 @@ Http::Tunneler::handleResponse(const bool eof) futureAnswer.peerResponseStatus = rep->sline.status(); request->hier.peer_reply_status = rep->sline.status(); - // XXX: Raw() prints an extra leading space. TODO: Add/use Raw::gap(false). debugs(11, 2, "HTTP Server " << connection); debugs(11, 2, "HTTP Server RESPONSE:\n---------\n" << - Raw(nullptr, readBuf.rawContent(), rep->hdr_sz).minLevel(2) << + Raw(nullptr, readBuf.rawContent(), rep->hdr_sz).minLevel(2).gap(false) << "----------"); // bail if we did not get an HTTP 200 (Connection Established) response diff --git a/src/debug.cc b/src/debug.cc index 58205af1e16..a93cfd49899 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -895,7 +895,10 @@ Raw::print(std::ostream &os) const const int finalLevel = (level >= 0) ? level : (size_ > 40 ? DBG_DATA : Debug::SectionLevel()); if (finalLevel <= Debug::SectionLevel()) { - os << (label_ ? '=' : ' '); + if (label_) + os << '='; + else if (useGap_) + os << ' '; if (data_) { if (useHex_) printHex(os); From 7ad0ad30a3b7188da936a8fe56d73bfd08b393b9 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 22 Jun 2018 17:24:14 +0300 Subject: [PATCH 17/59] Fix XXX comment about passing answers to caller classes --- src/tunnel.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tunnel.cc b/src/tunnel.cc index 48819708f4b..912ad393981 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -216,7 +216,7 @@ class TunnelStateData: public PeerSelectionInitiator ErrorState *savedError = nullptr; // XXX: Find a better way to pass answers than to create a dedicated class - // for each peer connector/tunneler! + // for each service with a callback! /// Gives Http::Tunneler access to Answer in the TunnelStateData callback dialer. class MyAnswerDialer2: public CallDialer, public Http::Tunneler::CbDialer { From 679645fc5d7dcb5af7a22012a17f519aa0bb7c08 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Mon, 25 Jun 2018 18:59:50 +0300 Subject: [PATCH 18/59] Try implement delay pools for Http::Tunneler The caller is responsible to set correct DelayID for the Http::Tunneler. Please see the comments in the new code. This patch also fixes the delay pools related code inside tunnel.cc, it was completely broken (it is broken for squid-5.x and squid-4.x releases). XXX: Probably we should take care of Delay pools inside Ssl::ServerBump where we are building a fake Store entry to store Bumping errors. --- src/FwdState.cc | 20 ++++++++++++++++++++ src/clients/HttpTunneler.cc | 8 ++++++-- src/clients/HttpTunneler.h | 6 ++++++ src/tunnel.cc | 9 ++++++--- 4 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 3818b27b539..c955439c77a 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -796,6 +796,26 @@ FwdState::establishTunnelThruProxy() tunneler->request = request; tunneler->url = request->url.authority(); tunneler->lifetimeLimit = connectingTimeout(serverDestinations[0]); +#if USE_DELAY_POOLS + Must(serverConnection()->getPeer()); + if (!serverConnection()->getPeer()->options.no_delay) { +#if 1 + if (auto conn = request->clientConnectionManager.get()) { + Http::StreamPointer context = conn->pipeline.front(); + if (context && context->http) + tunneler->delayId = DelayId(DelayId::DelayClient(context->http)); + } +#else + // The following looks a better option however will not work because + // the entry is the one created internally to Ssl::ServerBump to keep + // bumping error replies. + // We need to call sc->setDelayId(DelayId::DelayClient(http)) somewhere + // inside Ssl::ServerBump constructor where the storeEntry created + // and attached a StoreClient. + tunneler->delayId = entry->mem_obj->mostBytesAllowed(); +#endif + } +#endif AsyncJob::Start(tunneler); // and wait for the tunnelEstablishmentDone() call } diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index f5d53d80d40..ee4c3270956 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -178,7 +178,11 @@ Http::Tunneler::handleReadyRead(const CommIoCbParams &io) CommIoCbParams rd(this); rd.conn = io.conn; - rd.size = readBuf.spaceSize(); // XXX: from.bytesWanted(...) +#if USE_DELAY_POOLS + rd.size = delayId.bytesWanted(1, readBuf.spaceSize()); +#else + rd.size = readBuf.spaceSize(); +#endif switch (Comm::ReadNow(rd, readBuf)) { case Comm::INPROGRESS: @@ -186,7 +190,7 @@ Http::Tunneler::handleReadyRead(const CommIoCbParams &io) return; case Comm::OK: { -#if USE_DELAY_POOLS && XXX_IMPLEMENT +#if USE_DELAY_POOLS delayId.bytesIn(rd.size); #endif statCounter.server.all.kbytes_in += rd.size; diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index c3df3036b61..f9cef288348 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -12,6 +12,9 @@ #include "base/AsyncCbdataCalls.h" #include "base/AsyncJob.h" #include "CommCalls.h" +#if USE_DELAY_POOLS +#include "DelayId.h" +#endif #include "http/forward.h" #include "clients/forward.h" @@ -58,6 +61,9 @@ class Tunneler: virtual public AsyncJob AsyncCall::Pointer callback; ///< we call this with the results SBuf url; ///< request-target for the CONNECT request time_t lifetimeLimit; ///< do not run longer than this +#if USE_DELAY_POOLS + DelayId delayId; +#endif protected: /* AsyncJob API */ diff --git a/src/tunnel.cc b/src/tunnel.cc index 912ad393981..999774989bf 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -150,7 +150,7 @@ class TunnelStateData: public PeerSelectionInitiator // XXX: make these an AsyncCall when event API can handle them TunnelStateData *readPending; EVH *readPendingFunc; - private: + #if USE_DELAY_POOLS DelayId delayId; @@ -998,7 +998,7 @@ tunnelStart(ClientHttpRequest * http) tunnelState = new TunnelStateData(http); #if USE_DELAY_POOLS - //server.setDelayId called from tunnelConnectDone after server side connection established + tunnelState->server.setDelayId(DelayId::DelayClient(http)); #endif tunnelState->startSelectingDestinations(request, http->al, nullptr); } @@ -1042,6 +1042,9 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) tunneler->request = request; tunneler->url = url; tunneler->lifetimeLimit = Config.Timeout.lifetime; +#if USE_DELAY_POOLS + tunneler->delayId = server.delayId; +#endif AsyncJob::Start(tunneler); waitingForConnectExchange = true; // and wait for the tunnelEstablishmentDone() call @@ -1219,7 +1222,7 @@ switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm:: #if USE_DELAY_POOLS /* no point using the delayIsNoDelay stuff since tunnel is nice and simple */ - if (srvConn->getPeer() && srvConn->getPeer()->options.no_delay) + if (!srvConn->getPeer() || !srvConn->getPeer()->options.no_delay) tunnelState->server.setDelayId(DelayId::DelayClient(context->http)); #endif From 9f64a224f8af0def97b3bb8f70cc1777b07a3d30 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 26 Jun 2018 12:15:26 +0300 Subject: [PATCH 19/59] Code formatting fixes --- src/clients/HttpTunneler.cc | 1 - src/comm/Read.cc | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index ee4c3270956..bf55ec238e2 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -364,7 +364,6 @@ Http::Tunneler::status() const if (requestWritten) buf.append("w", 1); // request sent if (tunnelEstablished) buf.append("t", 1); // tunnel established if (!callback) buf.append("x", 1); // caller informed - if (stopReason != NULL) { buf.append(" stopped, reason:", 16); buf.appendf("%s",stopReason); diff --git a/src/comm/Read.cc b/src/comm/Read.cc index affd06b766f..b696200fe85 100644 --- a/src/comm/Read.cc +++ b/src/comm/Read.cc @@ -248,7 +248,7 @@ Comm::SetClientObjectReadTimeout(const Comm::ConnectionPointer &conn, time_t sta time_t timeToRead; if (lifetimeLimit > 0) { Must(squid_curtime >= startTime); - const time_t timeUsed = squid_curtime - startTime; + const time_t timeUsed = squid_curtime - startTime; const time_t timeLeft = (lifetimeLimit > timeUsed) ? (lifetimeLimit - timeUsed) : 0; timeToRead = min(::Config.Timeout.read, timeLeft); } else From 1368b23ddef2abd6f17c6ddecc72ae4c86401cf1 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 26 Jun 2018 18:41:28 +0300 Subject: [PATCH 20/59] Use Http1::ResponseParser to parse server reply. Similar to HttpStateData::processReplyHeader() but a liitle simpler --- src/clients/HttpTunneler.cc | 51 ++++++++++++++++++++++--------------- src/clients/HttpTunneler.h | 4 +-- 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index bf55ec238e2..fd28882a92e 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -14,6 +14,7 @@ #include "errorpage.h" #include "fde.h" #include "http.h" +#include "http/one/ResponseParser.h" #include "http/StateFlags.h" #include "HttpRequest.h" #include "StatCounters.h" @@ -196,7 +197,6 @@ Http::Tunneler::handleReadyRead(const CommIoCbParams &io) statCounter.server.all.kbytes_in += rd.size; statCounter.server.other.kbytes_in += rd.size; // TODO: other or http? request->hier.notePeerRead(); - len += rd.size; handleResponse(false); return; } @@ -242,30 +242,43 @@ void Http::Tunneler::handleResponse(const bool eof) { // mimic the basic parts of HttpStateData::processReplyHeader() - // TODO: HttpStateData::processReplyHeader() tries harder on eof! - // TODO: Do not parse headers. Use Http1::ResponseParser. See HttpStateData::processReplyHeader(). - HttpReply::Pointer rep = new HttpReply; - Http::StatusCode parseErr = Http::scNone; - const bool parsed = rep->parse(readBuf.c_str(), readBuf.length(), eof, &parseErr); - if (!parsed) { - if (parseErr > 0) { // unrecoverable parsing error - bailOnResponseError("malformed CONNECT response from peer", nullptr); + if (hp == NULL) + hp = new Http1::ResponseParser; + + bool parsedOk = hp->parse(readBuf); + readBuf = hp->remaining(); + if (hp->needsMoreData()) { + if (!eof) { + if (readBuf.length() >= SQUID_TCP_SO_RCVBUF) { + bailOnResponseError("huge CONNECT response from peer", nullptr); + return; + } + readMore(); return; } - // need more data - assert(!eof); - assert(!parseErr); + //eof, handle truncated response + readBuf.append("\r\n\r\n", 4); + parsedOk = hp->parse(readBuf); + readBuf = hp->remaining(); + } - if (readBuf.length() >= SQUID_TCP_SO_RCVBUF) { - bailOnResponseError("huge CONNECT response from peer", nullptr); - return; - } + if (!parsedOk) { + bailOnResponseError("malformed CONNECT response from peer", nullptr); + return; + } - readMore(); + // Assume the Http reply parsed correctly + HttpReply::Pointer rep = new HttpReply; + rep->sline.set(hp->messageProtocol(), hp->messageStatus()); + // parse headers + if (!rep->parseHeader(*hp)) { + bailOnResponseError("malformed CONNECT response from peer", nullptr); return; } + rep->sources |= HttpMsg::srcHttp; + // CONNECT response was successfully parsed auto &futureAnswer = answer(); futureAnswer.peerResponseStatus = rep->sline.status(); @@ -284,9 +297,7 @@ Http::Tunneler::handleResponse(const bool eof) } // preserve any bytes sent by the server after the CONNECT response - futureAnswer.leftovers = readBuf.substr(rep->hdr_sz); - // delay pools were using this field to throttle CONNECT response - len = futureAnswer.leftovers.length(); + futureAnswer.leftovers = readBuf; tunnelEstablished = true; debugs(83, 5, status()); diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index f9cef288348..4a590599f03 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -94,11 +94,11 @@ class Tunneler: virtual public AsyncJob AsyncCall::Pointer closer; ///< called when the connection is being closed SBuf readBuf; ///< either unparsed response or post-response bytes + /// Parser being used at present to parse the HTTP peer response. + Http1::ResponseParserPointer hp; const time_t startTime; ///< when the tunnel establishment started - size_t len; // XXX: Delay ID needs something like this? - bool requestWritten; ///< whether we successfully wrote the request bool tunnelEstablished; ///< whether we got a 200 OK response }; From 74cd510d2c4834b688ce349f1019f83b007eb7db Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 29 Jun 2018 19:13:49 +0300 Subject: [PATCH 21/59] Some changes requested by Alex - Remove FwdState::calls::connector member - Remove the Comm::SetClientObjectReadTimeout and replace with Comm::MortalReadTimeout function with a commSetConnTimeout call - Restruct ErrorPage constructors - Other polishing fixes --- src/FwdState.cc | 11 ++------ src/FwdState.h | 5 ---- src/clients/HttpTunneler.cc | 10 +++----- src/comm/Read.cc | 13 ++++------ src/comm/Read.h | 7 ++--- src/err_type.h | 2 +- src/errorpage.cc | 48 +++++++++++++---------------------- src/errorpage.h | 5 +++- src/security/PeerConnector.cc | 3 ++- src/tunnel.cc | 4 +-- 10 files changed, 40 insertions(+), 68 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index c955439c77a..df27a780e3c 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -317,11 +317,6 @@ FwdState::~FwdState() entry = NULL; - if (calls.connector != NULL) { - calls.connector->cancel("FwdState destructed"); - calls.connector = NULL; - } - if (Comm::IsConnOpen(serverConn)) closeServerConnection("~FwdState"); @@ -742,8 +737,6 @@ FwdState::handleUnregisteredServerEnd() void FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xerrno) { - calls.connector = nullptr; - if (status != Comm::OK) { ErrorState *const anErr = makeConnectingError(ERR_CONNECT_FAIL); anErr->xerrno = xerrno; @@ -1061,9 +1054,9 @@ FwdState::connectStart() GetMarkingsToServer(request, *serverDestinations[0]); - calls.connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); + AsyncCall::Pointer connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); const time_t connTimeout = connectingTimeout(serverDestinations[0]); - Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], calls.connector, connTimeout); + Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], connector, connTimeout); if (host) cs->setHost(host); ++n_tries; diff --git a/src/FwdState.h b/src/FwdState.h index de623d207bd..8722fd41e0f 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -166,11 +166,6 @@ class FwdState: public RefCountable, public PeerSelectionInitiator time_t start_t; int n_tries; ///< the number of forwarding attempts so far - // AsyncCalls which we set and may need cancelling. - struct { - AsyncCall::Pointer connector; ///< a call linking us to the ConnOpener producing serverConn. - } calls; - struct { bool connected_okay; ///< TCP link ever opened properly. This affects retry of POST,PUT,CONNECT,etc bool dont_retry; diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index fd28882a92e..af4cf3ad547 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -234,7 +234,8 @@ Http::Tunneler::readMore() // TODO: Same as PeerConnector, but who removes this timeout after we send a // positive answer? Should not we clean after ourselves in swanSong()? AsyncCall::Pointer nil; - Comm::SetClientObjectReadTimeout(connection, startTime, lifetimeLimit, nil); + time_t timeout = Comm::MortalReadTimeout(startTime, lifetimeLimit); + commSetConnTimeout(connection, timeout, nil); } /// Parses [possibly incomplete] CONNECT response and reacts to it. @@ -268,17 +269,14 @@ Http::Tunneler::handleResponse(const bool eof) return; } - // Assume the Http reply parsed correctly HttpReply::Pointer rep = new HttpReply; + rep->sources |= HttpMsg::srcHttp; rep->sline.set(hp->messageProtocol(), hp->messageStatus()); - // parse headers if (!rep->parseHeader(*hp)) { bailOnResponseError("malformed CONNECT response from peer", nullptr); return; } - rep->sources |= HttpMsg::srcHttp; - // CONNECT response was successfully parsed auto &futureAnswer = answer(); futureAnswer.peerResponseStatus = rep->sline.status(); @@ -310,7 +308,7 @@ Http::Tunneler::bailOnResponseError(const char *error, HttpReply *errorReply) ErrorState *err; if (errorReply) { - err = new ErrorState(errorReply); + err = new ErrorState(request.getRaw(), errorReply); } else { // with no reply suitable for relaying, answer with 502 (Bad Gateway) err = new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al); diff --git a/src/comm/Read.cc b/src/comm/Read.cc index b696200fe85..d9a25585a59 100644 --- a/src/comm/Read.cc +++ b/src/comm/Read.cc @@ -242,16 +242,13 @@ Comm::ReadCancel(int fd, AsyncCall::Pointer &callback) Comm::SetSelect(fd, COMM_SELECT_READ, NULL, NULL, 0); } -void -Comm::SetClientObjectReadTimeout(const Comm::ConnectionPointer &conn, time_t startTime, time_t lifetimeLimit, AsyncCall::Pointer &callback) +time_t +Comm::MortalReadTimeout(const time_t startTime, const time_t lifetimeLimit) { - time_t timeToRead; if (lifetimeLimit > 0) { - Must(squid_curtime >= startTime); - const time_t timeUsed = squid_curtime - startTime; + const time_t timeUsed = (squid_curtime > startTime) ? (squid_curtime - startTime) : 0; const time_t timeLeft = (lifetimeLimit > timeUsed) ? (lifetimeLimit - timeUsed) : 0; - timeToRead = min(::Config.Timeout.read, timeLeft); + return min(::Config.Timeout.read, timeLeft); } else - timeToRead = ::Config.Timeout.read; - commSetConnTimeout(conn, timeToRead, callback); + return ::Config.Timeout.read; } diff --git a/src/comm/Read.h b/src/comm/Read.h index 4222140e7af..7699f0db65d 100644 --- a/src/comm/Read.h +++ b/src/comm/Read.h @@ -50,11 +50,8 @@ void ReadCancel(int fd, AsyncCall::Pointer &callback); /// callback handler to process an FD which is available for reading extern PF HandleRead; -/** - * Sets a read timeout based on configured read timeout and caller lifetime - * limit and start time - */ -void SetClientObjectReadTimeout(const Comm::ConnectionPointer &conn, time_t startTime, time_t lifeTimeLimit, AsyncCall::Pointer &callback); +/// maximum read delay for readers with limited lifetime +time_t MortalReadTimeout(const time_t startTime, const time_t lifetimeLimit); } // namespace Comm // Legacy API to be removed diff --git a/src/err_type.h b/src/err_type.h index a925d6b5051..c6aa94426b8 100644 --- a/src/err_type.h +++ b/src/err_type.h @@ -76,7 +76,7 @@ typedef enum { ERR_SECURE_ACCEPT_FAIL, // Rejects the SSL connection intead of error page ERR_REQUEST_START_TIMEOUT, // Aborts the connection instead of error page - ERR_RELAY_REMOTE, // Relay remotes server reply + ERR_RELAY_REMOTE, // Sends server reply instead of error page /* Cache Manager GUI can install a manager index/home page */ MGR_INDEX, diff --git a/src/errorpage.cc b/src/errorpage.cc index ff1daf2ed32..d7abfdce525 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -671,51 +671,39 @@ ErrorState::NewForwarding(err_type type, HttpRequestPointer &request, const Acce return new ErrorState(type, status, request.getRaw(), ale); } -ErrorState::ErrorState(err_type t, Http::StatusCode status, HttpRequest * req, const AccessLogEntry::Pointer &anAle) : +ErrorState::ErrorState(err_type t) : type(t), page_id(t), - httpStatus(status), - callback(nullptr), - ale(anAle) + callback(nullptr) +{ +} + +ErrorState::ErrorState(err_type t, Http::StatusCode status, HttpRequest * req, const AccessLogEntry::Pointer &anAle) : + ErrorState(t) { if (page_id >= ERR_MAX && ErrorDynamicPages[page_id - ERR_MAX]->page_redirect != Http::scNone) httpStatus = ErrorDynamicPages[page_id - ERR_MAX]->page_redirect; + else + httpStatus = status; if (req) { request = req; src_addr = req->client_addr; } + + ale = anAle; } -ErrorState::ErrorState(HttpReply *errorReply) : - type(ERR_RELAY_REMOTE), - page_id(ERR_RELAY_REMOTE), - err_language(NULL), - httpStatus(Http::scNone), -#if USE_AUTH - auth_user_request (NULL), -#endif - request(NULL), - url(NULL), - xerrno(0), - port(0), - dnsError(), - ttl(0), - src_addr(), - redirect_url(NULL), - callback(NULL), - callback_data(NULL), - request_hdrs(NULL), - err_msg(NULL), -#if USE_OPENSSL - detail(NULL), -#endif - detailCode(ERR_DETAIL_NONE), - response_(errorReply) +ErrorState::ErrorState(HttpRequest * req, HttpReply *errorReply) : + ErrorState(ERR_RELAY_REMOTE) { - memset(&ftp, 0, sizeof(ftp)); Must(errorReply); httpStatus = errorReply->sline.status(); + if (req != NULL) { + request = req; + HTTPMSGLOCK(request); + src_addr = req->client_addr; + } } void diff --git a/src/errorpage.h b/src/errorpage.h index f9df2d2bd06..ae75fcbeb59 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -96,7 +96,7 @@ class ErrorState ErrorState() = delete; // not implemented. /// Build an ERR_RELAY_REMOTE ErrorState object - ErrorState(HttpReply *); + explicit ErrorState(HttpRequest * request, HttpReply *); ~ErrorState(); @@ -120,6 +120,9 @@ class ErrorState private: typedef ErrorPage::Build Build; + /// used by public constructors + ErrorState(err_type type); + /// locates the right error page template for this error and compiles it SBuf buildBody(); diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 5fe276b8bf7..eccd8fdb656 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -472,7 +472,8 @@ Security::PeerConnector::noteWantRead() // read timeout to avoid getting stuck while reading from a silent server AsyncCall::Pointer nil; - Comm::SetClientObjectReadTimeout(serverConnection(), startTime, negotiationTimeout, nil); + time_t timeout = Comm::MortalReadTimeout(startTime, negotiationTimeout); + commSetConnTimeout(serverConnection(), timeout, nil); Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, new Pointer(this), 0); } diff --git a/src/tunnel.cc b/src/tunnel.cc index 999774989bf..8818ff04e67 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -323,7 +323,8 @@ tunnelClientClosed(const CommCloseCbParams ¶ms) } TunnelStateData::TunnelStateData(ClientHttpRequest *clientRequest) : - startTime(squid_curtime) + startTime(squid_curtime), + waitingForConnectExchange(false) { debugs(26, 3, "TunnelStateData constructed this=" << this); client.readPendingFunc = &tunnelDelayedClientRead; @@ -1031,7 +1032,6 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) } assert(!waitingForConnectExchange); - // XXX: make waitingForConnectExchange a boolean; set it after Start() below AsyncCall::Pointer callback = asyncCall(5,4, "TunnelStateData::tunnelEstablishmentDone", From f93db6b28c5800b4e1fe47f7274d1df2cf4c67c8 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 29 Jun 2018 19:37:49 +0300 Subject: [PATCH 22/59] Polishing fixes Run formatter Other fixes --- src/clients/HttpTunneler.cc | 7 ++++--- src/clients/HttpTunneler.h | 1 + src/clients/HttpTunnelerAnswer.cc | 1 + src/clients/HttpTunnelerAnswer.h | 1 + src/comm/Read.cc | 1 + src/errorpage.cc | 3 +-- src/tunnel.cc | 3 +-- 7 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index af4cf3ad547..a8ab63058c5 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -243,7 +243,7 @@ void Http::Tunneler::handleResponse(const bool eof) { // mimic the basic parts of HttpStateData::processReplyHeader() - if (hp == NULL) + if (hp == nullptr) hp = new Http1::ResponseParser; bool parsedOk = hp->parse(readBuf); @@ -373,14 +373,15 @@ Http::Tunneler::status() const if (requestWritten) buf.append("w", 1); // request sent if (tunnelEstablished) buf.append("t", 1); // tunnel established if (!callback) buf.append("x", 1); // caller informed - if (stopReason != NULL) { + if (stopReason != nullptr) { buf.append(" stopped, reason:", 16); buf.appendf("%s",stopReason); } - if (connection != NULL) + if (connection != nullptr) buf.appendf(" FD %d", connection->fd); buf.appendf(" %s%u]", id.prefix(), id.value); buf.terminate(); return buf.content(); } + diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index 4a590599f03..1efbdc4a6fb 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -106,3 +106,4 @@ class Tunneler: virtual public AsyncJob } // namespace Http #endif /* SQUID_SRC_CLIENTS_HTTP_TUNNELER_H */ + diff --git a/src/clients/HttpTunnelerAnswer.cc b/src/clients/HttpTunnelerAnswer.cc index 9f3a037003b..a10e699e7c4 100644 --- a/src/clients/HttpTunnelerAnswer.cc +++ b/src/clients/HttpTunnelerAnswer.cc @@ -35,3 +35,4 @@ Http::operator <<(std::ostream &os, const TunnelerAnswer &answer) os << ']'; return os; } + diff --git a/src/clients/HttpTunnelerAnswer.h b/src/clients/HttpTunnelerAnswer.h index 331edc8a0b7..5e0cb5c14ea 100644 --- a/src/clients/HttpTunnelerAnswer.h +++ b/src/clients/HttpTunnelerAnswer.h @@ -49,3 +49,4 @@ std::ostream &operator <<(std::ostream &, const Http::TunnelerAnswer &); } // namepace Http #endif /* SQUID_SRC_CLIENTS_HTTP_TUNNELERANSWER_H */ + diff --git a/src/comm/Read.cc b/src/comm/Read.cc index d9a25585a59..58f43158c19 100644 --- a/src/comm/Read.cc +++ b/src/comm/Read.cc @@ -252,3 +252,4 @@ Comm::MortalReadTimeout(const time_t startTime, const time_t lifetimeLimit) } else return ::Config.Timeout.read; } + diff --git a/src/errorpage.cc b/src/errorpage.cc index d7abfdce525..349f2ee674d 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -699,9 +699,8 @@ ErrorState::ErrorState(HttpRequest * req, HttpReply *errorReply) : { Must(errorReply); httpStatus = errorReply->sline.status(); - if (req != NULL) { + if (req != nullptr) { request = req; - HTTPMSGLOCK(request); src_addr = req->client_addr; } } diff --git a/src/tunnel.cc b/src/tunnel.cc index 8818ff04e67..fe0586c997c 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -168,7 +168,6 @@ class TunnelStateData: public PeerSelectionInitiator /// Whether we are waiting for the CONNECT request/response exchange with the peer. bool waitingForConnectExchange; - void copyRead(Connection &from, IOCB *completion); /// continue to set up connection to a peer, going async for SSL peers @@ -855,7 +854,7 @@ TunnelStateData::notePeerReadyToShovel() *status_ptr = Http::scOkay; AsyncCall::Pointer call = commCbCall(5,5, "tunnelConnectedWriteDone", CommIoCbPtrFun(tunnelConnectedWriteDone, this)); - client.write(conn_established, strlen(conn_established), call, NULL); + client.write(conn_established, strlen(conn_established), call, nullptr); } } From a8345e15524c20c58d2c126782c00a44ab9bed73 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 29 Jun 2018 20:28:22 +0300 Subject: [PATCH 23/59] clear Http::Tunneler::readbuf after eof before read all of the headers --- src/clients/HttpTunneler.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index a8ab63058c5..f5fc680920b 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -261,7 +261,7 @@ Http::Tunneler::handleResponse(const bool eof) //eof, handle truncated response readBuf.append("\r\n\r\n", 4); parsedOk = hp->parse(readBuf); - readBuf = hp->remaining(); + readBuf.clear(); } if (!parsedOk) { From bb89498515064edd314acda4988658ecb46a2048 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 5 Jul 2018 15:53:33 -0600 Subject: [PATCH 24/59] Removed TODO based on Christos's investigation of current practices ... and complications that make correct timeout canceling difficult. Passing invalid timeouts to others is essentially a bug, but lots of code does that, most bugs are usually harmless, and fixing all those bugs is quite difficult (because there is no API for the cleanup code to know whether the currently set timeout belongs to this job). Tunneler is not the best place to bring attention to this complex problem. --- src/clients/HttpTunneler.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index f5fc680920b..f9072c70c61 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -231,8 +231,6 @@ Http::Tunneler::readMore() reader = JobCallback(93, 3, Dialer, this, Http::Tunneler::handleReadyRead); Comm::Read(connection, reader); - // TODO: Same as PeerConnector, but who removes this timeout after we send a - // positive answer? Should not we clean after ourselves in swanSong()? AsyncCall::Pointer nil; time_t timeout = Comm::MortalReadTimeout(startTime, lifetimeLimit); commSetConnTimeout(connection, timeout, nil); From b90352a2205557eef47d37742582b89dba7fbf79 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 5 Jul 2018 19:56:34 -0600 Subject: [PATCH 25/59] More const, auto Also fixed ConnOpener() constructor API that was pretending to modify its second argument. --- src/FwdState.cc | 10 +++++----- src/clients/HttpTunneler.cc | 4 ++-- src/comm/ConnOpener.cc | 2 +- src/comm/ConnOpener.h | 2 +- src/security/PeerConnector.cc | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index df27a780e3c..f144ed745d9 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -793,7 +793,7 @@ FwdState::establishTunnelThruProxy() Must(serverConnection()->getPeer()); if (!serverConnection()->getPeer()->options.no_delay) { #if 1 - if (auto conn = request->clientConnectionManager.get()) { + if (const auto conn = request->clientConnectionManager.get()) { Http::StreamPointer context = conn->pipeline.front(); if (context && context->http) tunneler->delayId = DelayId(DelayId::DelayClient(context->http)); @@ -867,7 +867,7 @@ FwdState::secureConnectionToPeerIfNeeded() AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::ConnectedToPeer", FwdStatePeerAnswerDialer(&FwdState::connectedToPeer, this)); - const time_t sslNegotiationTimeout = connectingTimeout(serverDestinations[0]); + const auto sslNegotiationTimeout = connectingTimeout(serverDestinations[0]); Security::PeerConnector *connector = nullptr; #if USE_OPENSSL if (request->flags.sslPeek) @@ -1054,9 +1054,9 @@ FwdState::connectStart() GetMarkingsToServer(request, *serverDestinations[0]); - AsyncCall::Pointer connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); - const time_t connTimeout = connectingTimeout(serverDestinations[0]); - Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], connector, connTimeout); + const AsyncCall::Pointer connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); + const auto connTimeout = connectingTimeout(serverDestinations[0]); + const auto cs = new Comm::ConnOpener(serverDestinations[0], connector, connTimeout); if (host) cs->setHost(host); ++n_tries; diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index f9072c70c61..fec66cff2a5 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -232,7 +232,7 @@ Http::Tunneler::readMore() Comm::Read(connection, reader); AsyncCall::Pointer nil; - time_t timeout = Comm::MortalReadTimeout(startTime, lifetimeLimit); + const auto timeout = Comm::MortalReadTimeout(startTime, lifetimeLimit); commSetConnTimeout(connection, timeout, nil); } @@ -244,7 +244,7 @@ Http::Tunneler::handleResponse(const bool eof) if (hp == nullptr) hp = new Http1::ResponseParser; - bool parsedOk = hp->parse(readBuf); + auto parsedOk = hp->parse(readBuf); // may be refined below readBuf = hp->remaining(); if (hp->needsMoreData()) { if (!eof) { diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index 9d41cbe3eae..d494a5a49d7 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -30,7 +30,7 @@ class CachePeer; CBDATA_NAMESPACED_CLASS_INIT(Comm, ConnOpener); -Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &c, AsyncCall::Pointer &handler, time_t ctimeout) : +Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &c, const AsyncCall::Pointer &handler, time_t ctimeout) : AsyncJob("Comm::ConnOpener"), host_(NULL), temporaryFd_(-1), diff --git a/src/comm/ConnOpener.h b/src/comm/ConnOpener.h index 350582f60ca..ee459f63352 100644 --- a/src/comm/ConnOpener.h +++ b/src/comm/ConnOpener.h @@ -33,7 +33,7 @@ class ConnOpener : public AsyncJob virtual bool doneAll() const; - ConnOpener(Comm::ConnectionPointer &, AsyncCall::Pointer &handler, time_t connect_timeout); + ConnOpener(Comm::ConnectionPointer &, const AsyncCall::Pointer &handler, time_t connect_timeout); ~ConnOpener(); void setHost(const char *); ///< set the hostname note for this connection diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index eccd8fdb656..f088371b2b0 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -472,7 +472,7 @@ Security::PeerConnector::noteWantRead() // read timeout to avoid getting stuck while reading from a silent server AsyncCall::Pointer nil; - time_t timeout = Comm::MortalReadTimeout(startTime, negotiationTimeout); + const auto timeout = Comm::MortalReadTimeout(startTime, negotiationTimeout); commSetConnTimeout(serverConnection(), timeout, nil); Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, new Pointer(this), 0); From 31901c4d58c3811115fd9760bcd63ded4533e1c2 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 5 Jul 2018 22:01:33 -0600 Subject: [PATCH 26/59] Moved "explicit" to the right ErrorState ctor Also documented how to pick the right ErrorState ctor. --- src/errorpage.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/errorpage.h b/src/errorpage.h index ae75fcbeb59..9394f3d2d5d 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -92,11 +92,12 @@ class ErrorState CBDATA_CLASS(ErrorState); public: + /// creates an error of type other than ERR_RELAY_REMOTE ErrorState(err_type type, Http::StatusCode, HttpRequest * request, const AccessLogEntryPointer &al); ErrorState() = delete; // not implemented. - /// Build an ERR_RELAY_REMOTE ErrorState object - explicit ErrorState(HttpRequest * request, HttpReply *); + /// creates an ERR_RELAY_REMOTE error + ErrorState(HttpRequest * request, HttpReply *); ~ErrorState(); @@ -120,8 +121,8 @@ class ErrorState private: typedef ErrorPage::Build Build; - /// used by public constructors - ErrorState(err_type type); + /// initializations shared by public constructors + explicit ErrorState(err_type type); /// locates the right error page template for this error and compiles it SBuf buildBody(); From 0c514151cc85f03ad4db181f08054d05eb0e16d3 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 6 Jul 2018 13:48:58 +0300 Subject: [PATCH 27/59] Fix HttpTunneler response when receives a malformed response When the HttpTunneler receives a valid HTTP status line but the headers are malformed: - If the HTTP status code is "200 OK" return an squid error page - If the Http status code is not "200 OK" return truncated the original peer response. --- src/clients/HttpTunneler.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index fec66cff2a5..b34daa08a9f 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -270,7 +270,7 @@ Http::Tunneler::handleResponse(const bool eof) HttpReply::Pointer rep = new HttpReply; rep->sources |= HttpMsg::srcHttp; rep->sline.set(hp->messageProtocol(), hp->messageStatus()); - if (!rep->parseHeader(*hp)) { + if (!rep->parseHeader(*hp) && rep->sline.status() == Http::scOkay) { bailOnResponseError("malformed CONNECT response from peer", nullptr); return; } From 13c8c719a71b3bea9e88128ea2a02af880137012 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 6 Jul 2018 19:26:12 +0300 Subject: [PATCH 28/59] Fix Ssl::ServerBump code to set DelayPools Id while creates its StoreEntry ... and use storeEntry to get DelayId in FwdState, instead of recomputing one --- src/FwdState.cc | 17 +---------------- src/client_side.cc | 8 +++++--- src/client_side.h | 2 +- src/client_side_request.cc | 4 ++-- src/ssl/ServerBump.cc | 9 +++++++-- src/ssl/ServerBump.h | 3 ++- 6 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index f144ed745d9..35257861684 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -791,23 +791,8 @@ FwdState::establishTunnelThruProxy() tunneler->lifetimeLimit = connectingTimeout(serverDestinations[0]); #if USE_DELAY_POOLS Must(serverConnection()->getPeer()); - if (!serverConnection()->getPeer()->options.no_delay) { -#if 1 - if (const auto conn = request->clientConnectionManager.get()) { - Http::StreamPointer context = conn->pipeline.front(); - if (context && context->http) - tunneler->delayId = DelayId(DelayId::DelayClient(context->http)); - } -#else - // The following looks a better option however will not work because - // the entry is the one created internally to Ssl::ServerBump to keep - // bumping error replies. - // We need to call sc->setDelayId(DelayId::DelayClient(http)) somewhere - // inside Ssl::ServerBump constructor where the storeEntry created - // and attached a StoreClient. + if (!serverConnection()->getPeer()->options.no_delay) tunneler->delayId = entry->mem_obj->mostBytesAllowed(); -#endif - } #endif AsyncJob::Start(tunneler); // and wait for the tunnelEstablishmentDone() call diff --git a/src/client_side.cc b/src/client_side.cc index 5610bc4c086..040aef0e839 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2913,9 +2913,11 @@ ConnStateData::getSslContextDone(Security::ContextPointer &ctx) } void -ConnStateData::switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode) +ConnStateData::switchToHttps(ClientHttpRequest *http, Ssl::BumpMode bumpServerMode) { assert(!switchedToHttps_); + Must(http->request); + auto &request = http->request; sslConnectHostOrIp = request->url.host(); tlsConnectPort = request->url.port(); @@ -2934,10 +2936,10 @@ ConnStateData::switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode) // without even peeking at the origin server certificate. if (bumpServerMode == Ssl::bumpServerFirst && !sslServerBump) { request->flags.sslPeek = true; - sslServerBump = new Ssl::ServerBump(request); + sslServerBump = new Ssl::ServerBump(http); } else if (bumpServerMode == Ssl::bumpPeek || bumpServerMode == Ssl::bumpStare) { request->flags.sslPeek = true; - sslServerBump = new Ssl::ServerBump(request, NULL, bumpServerMode); + sslServerBump = new Ssl::ServerBump(http, NULL, bumpServerMode); } // commSetConnTimeout() was called for this request before we switched. diff --git a/src/client_side.h b/src/client_side.h index ba955dc8ae6..318730dd36b 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -247,7 +247,7 @@ class ConnStateData : public Server, public HttpControlMsgSink, private Independ /// Proccess response from ssl_crtd. void sslCrtdHandleReply(const Helper::Reply &reply); - void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode); + void switchToHttps(ClientHttpRequest *, Ssl::BumpMode bumpServerMode); void parseTlsHandshake(); bool switchedToHttps() const { return switchedToHttps_; } Ssl::ServerBump *serverBump() {return sslServerBump;} diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 0cc0a041258..e6bf50203a4 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1584,7 +1584,7 @@ ClientHttpRequest::sslBumpEstablish(Comm::Flag errflag) #endif assert(sslBumpNeeded()); - getConn()->switchToHttps(request, sslBumpNeed_); + getConn()->switchToHttps(this, sslBumpNeed_); } void @@ -1849,7 +1849,7 @@ ClientHttpRequest::doCallouts() // We have to serve an error, so bump the client first. sslBumpNeed(Ssl::bumpClientFirst); // set final error but delay sending until we bump - Ssl::ServerBump *srvBump = new Ssl::ServerBump(request, e, Ssl::bumpClientFirst); + Ssl::ServerBump *srvBump = new Ssl::ServerBump(this, e, Ssl::bumpClientFirst); errorAppendEntry(e, calloutContext->error); calloutContext->error = NULL; getConn()->setServerBump(srvBump); diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 90a6062a350..4f520d14866 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "anyp/Uri.h" #include "client_side.h" +#include "client_side_request.h" #include "FwdState.h" #include "http/Stream.h" #include "ssl/ServerBump.h" @@ -19,10 +20,11 @@ CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump); -Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e, Ssl::BumpMode md): - request(fakeRequest), +Ssl::ServerBump::ServerBump(ClientHttpRequest *http, StoreEntry *e, Ssl::BumpMode md): step(bumpStep1) { + assert(http->request); + request = http->request; debugs(33, 4, "will peek at " << request->url.authority(true)); act.step1 = md; act.step2 = act.step3 = Ssl::bumpNone; @@ -39,6 +41,9 @@ Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e, Ssl::BumpMo // We do not need to be a client because the error contents will be used // later, but an entry without any client will trim all its contents away. sc = storeClientListAdd(entry, this); +#if USE_DELAY_POOLS + sc->setDelayId(DelayId::DelayClient(http)); +#endif } Ssl::ServerBump::~ServerBump() diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index bb6b186f8bb..dd5035cbf40 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -19,6 +19,7 @@ class ConnStateData; class store_client; +class ClientHttpRequest; namespace Ssl { @@ -31,7 +32,7 @@ class ServerBump CBDATA_CLASS(ServerBump); public: - explicit ServerBump(HttpRequest *fakeRequest, StoreEntry *e = NULL, Ssl::BumpMode mode = Ssl::bumpServerFirst); + explicit ServerBump(ClientHttpRequest *http, StoreEntry *e = NULL, Ssl::BumpMode mode = Ssl::bumpServerFirst); ~ServerBump(); void attachServerSession(const Security::SessionPointer &); ///< Sets the server TLS session object const Security::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors From 57c00c6d70d58642902f2523512f2292a1021677 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 6 Jul 2018 20:07:59 +0300 Subject: [PATCH 29/59] Fixes to allow bumped client-first connections use a CachePeer - The request->flags.sslPeek flag cover all bumping modes except client-fiest - The boolean needTlsToOrigin: const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS; is true on client-first IF not a peer is used. - The request->flags.sslBumped is true on all bumping modes - Inside Security::BlindPeerConnector requires some cases to separate cases where the BlindPeerConnector is used to encrypt CachePeer connections OR the connections to the origin server. --- src/FwdState.cc | 6 +++--- src/security/BlindPeerConnector.cc | 17 ++++++++++------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 35257861684..66bc9a8b6b6 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -767,7 +767,7 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in // but we do not support TLS inside TLS, so we exclude HTTPS proxies. const bool originWantsEncryptedTraffic = request->method == Http::METHOD_CONNECT || - request->flags.sslPeek; + request->flags.sslBumped; if (originWantsEncryptedTraffic && // the "encrypted traffic" part !peer->options.originserver && // the "through a proxy" part !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part @@ -847,7 +847,7 @@ FwdState::secureConnectionToPeerIfNeeded() request->method == Http::METHOD_CONNECT; const bool needTlsToPeer = peerWantsTls && !userWillTlsToPeerForUs; const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS; - if (needTlsToPeer || needTlsToOrigin || request->flags.sslPeek) { + if (needTlsToPeer || needTlsToOrigin || request->flags.sslBumped) { HttpRequest::Pointer requestPointer = request; AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::ConnectedToPeer", @@ -987,7 +987,7 @@ FwdState::connectStart() // Bumped requests require their pinned connection. Since we failed to reuse // the pinned connection, we now must terminate the bumped request. - if (request->flags.sslBumped) { + if (request->clientConnectionManager.valid() && request->clientConnectionManager->serverBump()) { // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]). static int occurrences = 0; const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 2baf08b761b..18aa92dbd52 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -22,10 +22,10 @@ CBDATA_NAMESPACED_CLASS_INIT(Security, BlindPeerConnector); Security::ContextPointer Security::BlindPeerConnector::getTlsContext() { - if (const CachePeer *peer = serverConnection()->getPeer()) { - assert(peer->secure.encryptTransport); + const CachePeer *peer = serverConnection()->getPeer(); + if (peer && peer->secure.encryptTransport) return peer->sslContext; - } + return ::Config.ssl_client.sslContext; } @@ -37,7 +37,8 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession return false; } - if (const CachePeer *peer = serverConnection()->getPeer()) { + const CachePeer *peer = serverConnection()->getPeer(); + if (peer && peer->secure.encryptTransport) { assert(peer); // NP: domain may be a raw-IP but it is now always set @@ -64,6 +65,8 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession void Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) { + auto *peer = serverConnection()->getPeer(); + if (error) { debugs(83, 5, "error=" << (void*)error); // XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but @@ -71,12 +74,12 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) // It is not clear whether we should call peerConnectSucceeded/Failed() // based on TCP results, SSL results, or both. And the code is probably not // consistent in this aspect across tunnelling and forwarding modules. - if (CachePeer *p = serverConnection()->getPeer()) - peerConnectFailed(p); + if (peer && peer->secure.encryptTransport) + peerConnectFailed(peer); return; } - if (auto *peer = serverConnection()->getPeer()) { + if (peer && peer->secure.encryptTransport) { const int fd = serverConnection()->fd; Security::MaybeGetSessionResumeData(fd_table[fd].ssl, peer->sslSession); } From 8fc030e8b2365c039e95f495f2431bb4924126b3 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 11 Aug 2018 15:52:54 -0600 Subject: [PATCH 30/59] Fixed "make check" broken by earlier branch commits --- src/tests/stub_client_side.cc | 2 +- src/tests/stub_libcomm.cc | 2 +- src/tests/stub_libsecurity.cc | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc index 75f2d8deec1..3986130b7cb 100644 --- a/src/tests/stub_client_side.cc +++ b/src/tests/stub_client_side.cc @@ -47,7 +47,7 @@ void ConnStateData::getSslContextStart() STUB void ConnStateData::getSslContextDone(Security::ContextPointer &) STUB void ConnStateData::sslCrtdHandleReplyWrapper(void *, const Helper::Reply &) STUB void ConnStateData::sslCrtdHandleReply(const Helper::Reply &) STUB -void ConnStateData::switchToHttps(HttpRequest *, Ssl::BumpMode) STUB +void ConnStateData::switchToHttps(ClientHttpRequest *, Ssl::BumpMode) STUB void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &) STUB bool ConnStateData::serveDelayedError(Http::Stream *) STUB_RETVAL(false) #endif diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc index 5cdf18c6e2e..e0813e4ca8a 100644 --- a/src/tests/stub_libcomm.cc +++ b/src/tests/stub_libcomm.cc @@ -32,7 +32,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Comm, ConnOpener); bool Comm::ConnOpener::doneAll() const STUB_RETVAL(false) void Comm::ConnOpener::start() STUB void Comm::ConnOpener::swanSong() STUB -Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &, AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") STUB +Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") STUB Comm::ConnOpener::~ConnOpener() STUB void Comm::ConnOpener::setHost(const char *) STUB const char * Comm::ConnOpener::getHost() const STUB_RETVAL(NULL) diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index cc48ce5e539..f97eff64d67 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -58,7 +58,6 @@ const char *PeerConnector::status() const STUB_RETVAL("") void PeerConnector::commCloseHandler(const CommCloseCbParams &) STUB void PeerConnector::connectionClosed(const char *) STUB bool PeerConnector::prepareSocket() STUB_RETVAL(false) -void PeerConnector::setReadTimeout() STUB bool PeerConnector::initialize(Security::SessionPointer &) STUB_RETVAL(false) void PeerConnector::negotiate() STUB bool PeerConnector::sslFinalized() STUB_RETVAL(false) From ab2d4ecfa67230c328a719b5de051ab4863e29b1 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 11 Aug 2018 16:14:13 -0600 Subject: [PATCH 31/59] Fixed "make distcheck" and builds without OpenSSL ... broken by earlier branch commits. ConnStateData::serverBump() is only defined for USE_OPENSSL builds. --- src/FwdState.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/FwdState.cc b/src/FwdState.cc index 66bc9a8b6b6..bf69b3e0e9b 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -985,6 +985,7 @@ FwdState::connectStart() request->hier.startPeerClock(); +#if USE_OPENSSL // Bumped requests require their pinned connection. Since we failed to reuse // the pinned connection, we now must terminate the bumped request. if (request->clientConnectionManager.valid() && request->clientConnectionManager->serverBump()) { @@ -996,6 +997,7 @@ FwdState::connectStart() self = NULL; // refcounted return; } +#endif // USE_OPENSSL // Use pconn to avoid opening a new connection. const char *host = NULL; From d1e4f76ba21143b8e8597c0aa62297f1e4155ee0 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 11 Aug 2018 17:17:08 -0600 Subject: [PATCH 32/59] Undo branch commit 91ab5f8 ... because it broke the basic "stare all/bump all" use case. I do not know which 91ab5f8 changes are responsible for that breakage. I know that FwdState::connectStart() change immediately triggers the BUG message inside the modified if statement, and that undoing that change alone is not sufficient to restore stare all/bump all" support. It is possible that most of the other 91ab5f8 changes are correct, but they need to be isolated and tested to weed out the bad apple. Also, sslPeek/sslBumped changes/comments seem to mismatch sslPeek/sslBumped flag descriptions in RequestFlags.h. If the changes/comments are correct, then those descriptions need to be fixed. XXX: Without 91ab5f8, the following SslBump configurations fail basic tests: bump at step1, client-first, and server-first. --- src/FwdState.cc | 8 +++----- src/security/BlindPeerConnector.cc | 17 +++++++---------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index bf69b3e0e9b..35257861684 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -767,7 +767,7 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in // but we do not support TLS inside TLS, so we exclude HTTPS proxies. const bool originWantsEncryptedTraffic = request->method == Http::METHOD_CONNECT || - request->flags.sslBumped; + request->flags.sslPeek; if (originWantsEncryptedTraffic && // the "encrypted traffic" part !peer->options.originserver && // the "through a proxy" part !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part @@ -847,7 +847,7 @@ FwdState::secureConnectionToPeerIfNeeded() request->method == Http::METHOD_CONNECT; const bool needTlsToPeer = peerWantsTls && !userWillTlsToPeerForUs; const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS; - if (needTlsToPeer || needTlsToOrigin || request->flags.sslBumped) { + if (needTlsToPeer || needTlsToOrigin || request->flags.sslPeek) { HttpRequest::Pointer requestPointer = request; AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::ConnectedToPeer", @@ -985,10 +985,9 @@ FwdState::connectStart() request->hier.startPeerClock(); -#if USE_OPENSSL // Bumped requests require their pinned connection. Since we failed to reuse // the pinned connection, we now must terminate the bumped request. - if (request->clientConnectionManager.valid() && request->clientConnectionManager->serverBump()) { + if (request->flags.sslBumped) { // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]). static int occurrences = 0; const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; @@ -997,7 +996,6 @@ FwdState::connectStart() self = NULL; // refcounted return; } -#endif // USE_OPENSSL // Use pconn to avoid opening a new connection. const char *host = NULL; diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 18aa92dbd52..2baf08b761b 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -22,10 +22,10 @@ CBDATA_NAMESPACED_CLASS_INIT(Security, BlindPeerConnector); Security::ContextPointer Security::BlindPeerConnector::getTlsContext() { - const CachePeer *peer = serverConnection()->getPeer(); - if (peer && peer->secure.encryptTransport) + if (const CachePeer *peer = serverConnection()->getPeer()) { + assert(peer->secure.encryptTransport); return peer->sslContext; - + } return ::Config.ssl_client.sslContext; } @@ -37,8 +37,7 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession return false; } - const CachePeer *peer = serverConnection()->getPeer(); - if (peer && peer->secure.encryptTransport) { + if (const CachePeer *peer = serverConnection()->getPeer()) { assert(peer); // NP: domain may be a raw-IP but it is now always set @@ -65,8 +64,6 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession void Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) { - auto *peer = serverConnection()->getPeer(); - if (error) { debugs(83, 5, "error=" << (void*)error); // XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but @@ -74,12 +71,12 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) // It is not clear whether we should call peerConnectSucceeded/Failed() // based on TCP results, SSL results, or both. And the code is probably not // consistent in this aspect across tunnelling and forwarding modules. - if (peer && peer->secure.encryptTransport) - peerConnectFailed(peer); + if (CachePeer *p = serverConnection()->getPeer()) + peerConnectFailed(p); return; } - if (peer && peer->secure.encryptTransport) { + if (auto *peer = serverConnection()->getPeer()) { const int fd = serverConnection()->fd; Security::MaybeGetSessionResumeData(fd_table[fd].ssl, peer->sslSession); } From b80d4e258663208a3fac36deb413aebf01a28da4 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 13 Aug 2018 22:16:44 -0600 Subject: [PATCH 33/59] Support proxy authentication for SslBump-controlled CONNECTs The core of this change is calling prepForPeering() in Http::Tunneler::start() to set the right HttpRequest metadata for the later httpFixupAuthentication() call to properly adjust the to-peer CONNECT request. The rest is reducing code duplication around HttpRequest metadata setting. One (positive) side effect of that reduction is that FwdState should now set request->flags.proxying correctly when re-forwarding a non-HTTP request directly to an origin server after a failed peer transaction -- the old code did not reset flags.proxying in that case. I do not know whether non-HTTP code actually cares about flags.proxying. --- src/FwdState.cc | 12 ++++-------- src/HttpRequest.cc | 22 ++++++++++++++++++++++ src/HttpRequest.h | 12 ++++++++++-- src/clients/HttpTunneler.cc | 6 ++++++ src/http.cc | 2 +- src/tunnel.cc | 32 ++++++++------------------------ 6 files changed, 51 insertions(+), 35 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 35257861684..2468a41f0b7 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1151,17 +1151,13 @@ FwdState::dispatch() } #endif - if (serverConnection()->getPeer() != NULL) { - ++ serverConnection()->getPeer()->stats.fetches; - request->peer_login = serverConnection()->getPeer()->login; - request->peer_domain = serverConnection()->getPeer()->domain; - request->flags.auth_no_keytab = serverConnection()->getPeer()->options.auth_no_keytab; + if (const auto peer = serverConnection()->getPeer()) { + ++peer->stats.fetches; + request->prepForPeering(*peer); httpStart(this); } else { assert(!request->flags.sslPeek); - request->peer_login = NULL; - request->peer_domain = NULL; - request->flags.auth_no_keytab = 0; + request->prepForDirect(); switch (request->url.getScheme()) { diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 80f77768738..56efe68dea8 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -12,6 +12,7 @@ #include "AccessLogEntry.h" #include "acl/AclSizeLimit.h" #include "acl/FilledChecklist.h" +#include "CachePeer.h" #include "client_side.h" #include "client_side_request.h" #include "dns/LookupDetails.h" @@ -452,6 +453,27 @@ HttpRequest::bodyNibbled() const return body_pipe != NULL && body_pipe->consumedSize() > 0; } +void +HttpRequest::prepForPeering(const CachePeer &peer) +{ + // XXX: Saving two pointers to memory controlled by an independent object. + peer_login = peer.login; + peer_domain = peer.domain; + flags.auth_no_keytab = peer.options.auth_no_keytab; + flags.proxying = !peer.options.originserver; + debugs(11, 4, this << " to " << peer.host << (flags.proxying ? " proxy" : " origin")); +} + +void +HttpRequest::prepForDirect() +{ + peer_login = nullptr; + peer_domain = nullptr; + flags.auth_no_keytab = false; + flags.proxying = false; + debugs(11, 4, this); +} + void HttpRequest::detailError(err_type aType, int aDetail) { diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 909a17ef642..13e9d4eee1f 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -34,10 +34,11 @@ #include "eui/Eui64.h" #endif -class ConnStateData; -class Downloader; class AccessLogEntry; typedef RefCount AccessLogEntryPointer; +class CachePeer; +class ConnStateData; +class Downloader; /* Http Request */ void httpRequestPack(void *obj, Packable *p); @@ -87,6 +88,13 @@ class HttpRequest: public Http::Message Adaptation::Icap::History::Pointer icapHistory() const; #endif + /* If a request goes through several destinations, then the following two + * methods will be called several times, in destinations-dependent order. */ + /// get ready to be sent to the given cache_peer, including originserver + void prepForPeering(const CachePeer &peer); + /// get ready to be sent directly to an origin server, excluding originserver + void prepForDirect(); + void recordLookup(const Dns::LookupDetails &detail); /// sets error detail if no earlier detail was available diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index b34daa08a9f..2a42524c1e4 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -7,6 +7,7 @@ */ #include "squid.h" +#include "CachePeer.h" #include "clients/HttpTunneler.h" #include "clients/HttpTunnelerAnswer.h" #include "comm/Read.h" @@ -69,6 +70,11 @@ Http::Tunneler::start() Must(url.length()); Must(lifetimeLimit >= 0); + if (const auto peer = connection->getPeer()) + request->prepForPeering(*peer); + else + request->prepForDirect(); + watchForClosures(); writeRequest(); startReadingResponse(); diff --git a/src/http.cc b/src/http.cc index 03b399e8ef0..b9cbf9bf3ad 100644 --- a/src/http.cc +++ b/src/http.cc @@ -100,7 +100,7 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : _peer = cbdataReference(fwd->serverConnection()->getPeer()); /* might be NULL */ if (_peer) { - // XXX: tunnel.cc and sendRequest() exclude originserver from proxying! + // XXX: HttpRequest::prepForPeering excludes originserver from proxying! // TODO: split {proxying,originserver} into {peering,toProxy,toOrigin} request->flags.proxying = true; /* diff --git a/src/tunnel.cc b/src/tunnel.cc index fe0586c997c..c5c0219cb5f 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -932,19 +932,10 @@ tunnelConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xe tunnelState->request->peer_host = conn->getPeer() ? conn->getPeer()->host : NULL; comm_add_close_handler(conn->fd, tunnelServerClosed, tunnelState); - if (conn->getPeer()) { - tunnelState->request->peer_login = conn->getPeer()->login; - tunnelState->request->peer_domain = conn->getPeer()->domain; - tunnelState->request->flags.auth_no_keytab = conn->getPeer()->options.auth_no_keytab; - tunnelState->request->flags.proxying = !(conn->getPeer()->options.originserver); - debugs(26, 4, "post-connect(2) proxying: " << tunnelState->request->flags.proxying); - } else { - tunnelState->request->peer_login = NULL; - tunnelState->request->peer_domain = NULL; - tunnelState->request->flags.auth_no_keytab = false; - tunnelState->request->flags.proxying = false; - debugs(26, 4, "post-connect(2) proxying: direct"); - } + if (const auto * const peer = conn->getPeer()) + tunnelState->request->prepForPeering(*peer); + else + tunnelState->request->prepForDirect(); if (tunnelState->request->flags.proxying) tunnelState->connectToPeer(); @@ -1229,17 +1220,10 @@ switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm:: comm_add_close_handler(srvConn->fd, tunnelServerClosed, tunnelState); debugs(26, 4, "determine post-connect handling pathway."); - if (srvConn->getPeer()) { - request->peer_login = srvConn->getPeer()->login; - request->peer_domain = srvConn->getPeer()->domain; - request->flags.auth_no_keytab = srvConn->getPeer()->options.auth_no_keytab; - request->flags.proxying = !(srvConn->getPeer()->options.originserver); - } else { - request->peer_login = nullptr; - request->peer_domain = nullptr; - request->flags.auth_no_keytab = false; - request->flags.proxying = false; - } + if (const auto peer = srvConn->getPeer()) + request->prepForPeering(*peer); + else + request->prepForDirect(); AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); From eb76ee7e63847f32b523659ab8c203ce8bb43f4b Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 21 Aug 2018 18:31:00 +0300 Subject: [PATCH 34/59] Fixes to allow bumped client-first connections use a CachePeer This is the refactored commit '91ab5f8' which reverted with the commit '6686c62' For the client-first bumped requests: - client send CONNECT request to squid which assumed as https request - Squid establish TLS connection with the client - client send the first http request to squid - the FwdState.cc code receives a "get https" request to process and forward Also Squid can handle and serve with a similar way the "get https//xxx" requests. This patch fixes Squid to: - Inside FwdState::connectDone add a check about https url scheme to "origin-wants-encrypted-traffic" check - Inside FwdState::secureConnectionToPeerIfNeeded replaces the old check "need-Tls-to-origin" which considers that the cache-peers can not be used to serve client-first bumped requests and 'get https://xxx' proxy requests, with a new one which allow to use a cache peer with these requests if the cache-peer does not require encryption (because we are not supporting TLS inside TLS yet). - Inside FwdState::connectStart where checks if bumped HTTP requests are tunneled through a valid pinned connection, exclude client-first bumping cases: At the time the first HTTP request on a client-first bumped connection enters squid and processed by FwdState.cc code the connection to the server is not established yet. (Q. Are/can we allowing reconnect to server on client-first bumping mode?) - The BlindPeerConnector used to serve client-first bumped requests and 'get https://xxx' proxy requests. The current code assumes if a cache-peer is used with BlindPeerConnector it must require encryption. This patch add the required fixes to allow use any cache-peer with BlindPeerConnector --- src/FwdState.cc | 14 +++++++++++--- src/security/BlindPeerConnector.cc | 17 ++++++++++------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 2468a41f0b7..5f23f1f3250 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -767,7 +767,8 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in // but we do not support TLS inside TLS, so we exclude HTTPS proxies. const bool originWantsEncryptedTraffic = request->method == Http::METHOD_CONNECT || - request->flags.sslPeek; + request->flags.sslPeek || + request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' or bump client-first if (originWantsEncryptedTraffic && // the "encrypted traffic" part !peer->options.originserver && // the "through a proxy" part !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part @@ -846,7 +847,11 @@ FwdState::secureConnectionToPeerIfNeeded() const bool userWillTlsToPeerForUs = p && p->options.originserver && request->method == Http::METHOD_CONNECT; const bool needTlsToPeer = peerWantsTls && !userWillTlsToPeerForUs; - const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS; + + // 'get https://xxx' requests or bump client-first, excluding + // https proxies (TLS inside TLS is not supported yet) + const bool needTlsToOrigin = !peerWantsTls && request->url.getScheme() == AnyP::PROTO_HTTPS; + if (needTlsToPeer || needTlsToOrigin || request->flags.sslPeek) { HttpRequest::Pointer requestPointer = request; AsyncCall::Pointer callback = asyncCall(17,4, @@ -987,7 +992,10 @@ FwdState::connectStart() // Bumped requests require their pinned connection. Since we failed to reuse // the pinned connection, we now must terminate the bumped request. - if (request->flags.sslBumped) { + // For client-first bumping mode the request is already bumped but the + // connection to the server is not established yet. + bool clientFirstBump = request->clientConnectionManager.valid() && request->clientConnectionManager->sslBumpMode == Ssl::bumpClientFirst; + if (request->flags.sslBumped && !clientFirstBump) { // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]). static int occurrences = 0; const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 2baf08b761b..18aa92dbd52 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -22,10 +22,10 @@ CBDATA_NAMESPACED_CLASS_INIT(Security, BlindPeerConnector); Security::ContextPointer Security::BlindPeerConnector::getTlsContext() { - if (const CachePeer *peer = serverConnection()->getPeer()) { - assert(peer->secure.encryptTransport); + const CachePeer *peer = serverConnection()->getPeer(); + if (peer && peer->secure.encryptTransport) return peer->sslContext; - } + return ::Config.ssl_client.sslContext; } @@ -37,7 +37,8 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession return false; } - if (const CachePeer *peer = serverConnection()->getPeer()) { + const CachePeer *peer = serverConnection()->getPeer(); + if (peer && peer->secure.encryptTransport) { assert(peer); // NP: domain may be a raw-IP but it is now always set @@ -64,6 +65,8 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession void Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) { + auto *peer = serverConnection()->getPeer(); + if (error) { debugs(83, 5, "error=" << (void*)error); // XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but @@ -71,12 +74,12 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) // It is not clear whether we should call peerConnectSucceeded/Failed() // based on TCP results, SSL results, or both. And the code is probably not // consistent in this aspect across tunnelling and forwarding modules. - if (CachePeer *p = serverConnection()->getPeer()) - peerConnectFailed(p); + if (peer && peer->secure.encryptTransport) + peerConnectFailed(peer); return; } - if (auto *peer = serverConnection()->getPeer()) { + if (peer && peer->secure.encryptTransport) { const int fd = serverConnection()->fd; Security::MaybeGetSessionResumeData(fd_table[fd].ssl, peer->sslSession); } From a240487289257bf48625029ebeb2162e81b477dd Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 22 Aug 2018 16:48:42 +0300 Subject: [PATCH 35/59] server-first bumped connections aborts when a cache peer is used On server-first bumping mode the related FwdState object lacks the AccessLogEntry. The Http::Tunneler async job started from FwdState aborts because of a Must(al) inside Http::Tunneler::start method. --- src/client_side.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/client_side.cc b/src/client_side.cc index 040aef0e839..0da976ebc5c 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3012,8 +3012,10 @@ ConnStateData::parseTlsHandshake() getSslContextStart(); return; } else if (sslServerBump->act.step1 == Ssl::bumpServerFirst) { + Http::StreamPointer context = pipeline.front(); + ClientHttpRequest *http = context ? context->http : nullptr; // will call httpsPeeked() with certificate and connection, eventually - FwdState::fwdStart(clientConnection, sslServerBump->entry, sslServerBump->request.getRaw()); + FwdState::Start(clientConnection, sslServerBump->entry, sslServerBump->request.getRaw(), http ? http->al : nullptr); } else { Must(sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare); startPeekAndSplice(); From b91a1f56f15c8ea04e3e0f32b2b49f1c09c04f97 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 22 Aug 2018 18:19:32 +0300 Subject: [PATCH 36/59] Bumped connections on step1 aborts with "BUG: Lost previously ..." message The bumped connections on step1 are equivalent to client-first bumping mode however are not handled with the same way inside FwdState::connectStart where checks if a request marked as bumped received inside a bumped connection. In these cases the ConnStateData::bumpMode is bumpBump but the is not any ConnStateData::sslServerBump object. Alternate solutions: - convert bumpBump action on step1 to bumpClientFirst action - or try build ConnStateData::sslServerBump object in all cases of bumping and check the ConnStateData::sslServerBump.act.step1 for bumpClientFirst or bumpBump modes. --- src/FwdState.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 5f23f1f3250..6646df031e9 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -994,7 +994,13 @@ FwdState::connectStart() // the pinned connection, we now must terminate the bumped request. // For client-first bumping mode the request is already bumped but the // connection to the server is not established yet. - bool clientFirstBump = request->clientConnectionManager.valid() && request->clientConnectionManager->sslBumpMode == Ssl::bumpClientFirst; + // When a connection bumped on step1 the result is equivalent to + // client-first bumping mode. We can recognise such cases because there is + // not ConnStateData::sslServerBump object. + bool clientFirstBump = request->clientConnectionManager.valid() && + (request->clientConnectionManager->sslBumpMode == Ssl::bumpClientFirst || + (request->clientConnectionManager->sslBumpMode == Ssl::bumpBump && !request->clientConnectionManager->serverBump()) + ); if (request->flags.sslBumped && !clientFirstBump) { // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]). static int occurrences = 0; From cd793101b839380d28a39ccaa1f185ef58e828b4 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 22 Aug 2018 18:37:51 +0300 Subject: [PATCH 37/59] Squid send full URL as path on bumped HTTP requests when a cache peer is used Squid must send bumped HTTP requests as they are destined to an origin TLS server, even if a cache peer is used. --- src/http.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/http.cc b/src/http.cc index b9cbf9bf3ad..c9b9a69744a 100644 --- a/src/http.cc +++ b/src/http.cc @@ -96,7 +96,9 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : surrogateNoStore = false; serverConnection = fwd->serverConnection(); - if (fwd->serverConnection() != NULL) + bool alwaysToOrigin = request->flags.sslBumped || // http request through bumped connection + request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' request (this check superset request->flags.sslBump check) + if (!alwaysToOrigin && fwd->serverConnection() != NULL) _peer = cbdataReference(fwd->serverConnection()->getPeer()); /* might be NULL */ if (_peer) { From be67b12ae0b0f443f90db527c090983ccf14d4c9 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 7 Nov 2018 14:33:32 +0200 Subject: [PATCH 38/59] Fixes to allow compile after cherry-pick completed --- src/clients/HttpTunneler.cc | 2 +- src/errorpage.cc | 3 ++- src/tunnel.cc | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 2a42524c1e4..ca87112c94e 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -274,7 +274,7 @@ Http::Tunneler::handleResponse(const bool eof) } HttpReply::Pointer rep = new HttpReply; - rep->sources |= HttpMsg::srcHttp; + rep->sources |= Http::Message::srcHttp; rep->sline.set(hp->messageProtocol(), hp->messageStatus()); if (!rep->parseHeader(*hp) && rep->sline.status() == Http::scOkay) { bailOnResponseError("malformed CONNECT response from peer", nullptr); diff --git a/src/errorpage.cc b/src/errorpage.cc index 349f2ee674d..f4a257c6886 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -699,7 +699,8 @@ ErrorState::ErrorState(HttpRequest * req, HttpReply *errorReply) : { Must(errorReply); httpStatus = errorReply->sline.status(); - if (req != nullptr) { + + if (req) { request = req; src_addr = req->client_addr; } diff --git a/src/tunnel.cc b/src/tunnel.cc index c5c0219cb5f..e92f457a4fa 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -842,7 +842,7 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) ErrorState *error = answer.squidError.get(); Must(error); answer.squidError.clear(); // preserve error for errorSendComplete() - sendError(error, "peer error"); + sendError(error, "tunneler returns error"); } void From 2ec06bc9a9da424916dec5847fb18d495de0da7f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Nov 2018 22:12:18 -0700 Subject: [PATCH 39/59] Fixed build without USE_OPENSSL --- src/FwdState.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 6646df031e9..0ef7f964cc8 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -997,10 +997,14 @@ FwdState::connectStart() // When a connection bumped on step1 the result is equivalent to // client-first bumping mode. We can recognise such cases because there is // not ConnStateData::sslServerBump object. - bool clientFirstBump = request->clientConnectionManager.valid() && +#if USE_OPENSSL + const auto clientFirstBump = request->clientConnectionManager.valid() && (request->clientConnectionManager->sslBumpMode == Ssl::bumpClientFirst || (request->clientConnectionManager->sslBumpMode == Ssl::bumpBump && !request->clientConnectionManager->serverBump()) ); +#else + const auto clientFirstBump = false; +#endif /* USE_OPENSSL */ if (request->flags.sslBumped && !clientFirstBump) { // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]). static int occurrences = 0; From 09736e534b5ac3cb10633a5933e775bcfb6ad708 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 14 Nov 2018 13:38:37 +0200 Subject: [PATCH 40/59] Fix commit 70b53312 The commit 70b53312 did not set the HttpStateData::_peer in the case of not "proxying", consider this variable useless in the case we are speaking to an origin server even if we are using a tunnel through a cache peer. In most cases this is looks that it is more-or-less correct but looks confusing. This patch: - Remove the RequestFlags::proxying flag. The httpstateflags has a similar flag which should used instead. - Inside HttpStateData class set the proxying flag when (a) a cache peer is used, (b) it is not an HttpRequest tunneled through the peer, and the peer is not an origin server. The proxying flag is set when the request destined to the peer, and not to the origin server (even if a peer used to tunnel it). - Try to correctly set other flags and counters (eg keepalive related) based on flags.proxying. --- src/HttpRequest.cc | 4 +--- src/RequestFlags.h | 2 -- src/clients/HttpTunneler.cc | 2 +- src/http.cc | 36 ++++++++++++++++++++---------------- src/tunnel.cc | 13 ++++++++----- 5 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 56efe68dea8..5eaabe824af 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -460,8 +460,7 @@ HttpRequest::prepForPeering(const CachePeer &peer) peer_login = peer.login; peer_domain = peer.domain; flags.auth_no_keytab = peer.options.auth_no_keytab; - flags.proxying = !peer.options.originserver; - debugs(11, 4, this << " to " << peer.host << (flags.proxying ? " proxy" : " origin")); + debugs(11, 4, this << " to " << peer.host << (!peer.options.originserver ? " proxy" : " origin")); } void @@ -470,7 +469,6 @@ HttpRequest::prepForDirect() peer_login = nullptr; peer_domain = nullptr; flags.auth_no_keytab = false; - flags.proxying = false; debugs(11, 4, this); } diff --git a/src/RequestFlags.h b/src/RequestFlags.h index 6b984e36357..b4a880c82cb 100644 --- a/src/RequestFlags.h +++ b/src/RequestFlags.h @@ -36,8 +36,6 @@ class RequestFlags bool loopDetected = false; /** the connection can be kept alive */ bool proxyKeepalive = false; - /* this should be killed, also in httpstateflags */ - bool proxying = false; /** content has expired, need to refresh it */ bool refresh = false; /** request was redirected by redirectors */ diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index ca87112c94e..7303c352265 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -127,7 +127,7 @@ Http::Tunneler::writeRequest() HttpHeader hdr_out(hoRequest); Http::StateFlags flags; memset(&flags, '\0', sizeof(flags)); - flags.proxying = request->flags.proxying; + flags.proxying = true; MemBuf mb; mb.init(); mb.appendf("CONNECT %s HTTP/1.1\r\n", url.c_str()); diff --git a/src/http.cc b/src/http.cc index c9b9a69744a..10aedf8b25f 100644 --- a/src/http.cc +++ b/src/http.cc @@ -96,15 +96,17 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : surrogateNoStore = false; serverConnection = fwd->serverConnection(); - bool alwaysToOrigin = request->flags.sslBumped || // http request through bumped connection - request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' request (this check superset request->flags.sslBump check) - if (!alwaysToOrigin && fwd->serverConnection() != NULL) + if (fwd->serverConnection() != NULL) _peer = cbdataReference(fwd->serverConnection()->getPeer()); /* might be NULL */ + bool tlsTunnel = request->flags.sslBumped || // http request through bumped connection + request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' request (this check superset request->flags.sslBump check) + + // TODO: split {proxying,originserver} into {peering,toProxy,toOrigin} + flags.originpeer = (_peer != NULL && _peer->options.originserver); + flags.proxying = (_peer != NULL && !tlsTunnel && !flags.originpeer); + if (_peer) { - // XXX: HttpRequest::prepForPeering excludes originserver from proxying! - // TODO: split {proxying,originserver} into {peering,toProxy,toOrigin} - request->flags.proxying = true; /* * This NEIGHBOR_PROXY_ONLY check probably shouldn't be here. * We might end up getting the object from somewhere else if, @@ -628,11 +630,11 @@ void HttpStateData::keepaliveAccounting(HttpReply *reply) { if (flags.keepalive) - if (_peer) + if (_peer && flags.proxying) ++ _peer->stats.n_keepalives_sent; if (reply->keep_alive) { - if (_peer) + if (_peer && flags.proxying) ++ _peer->stats.n_keepalives_recv; if (Config.onoff.detect_broken_server_pconns @@ -647,7 +649,7 @@ HttpStateData::keepaliveAccounting(HttpReply *reply) void HttpStateData::checkDateSkew(HttpReply *reply) { - if (reply->date > -1 && !_peer) { + if (reply->date > -1 && !flags.proxying) { int skew = abs((int)(reply->date - squid_curtime)); if (skew > 86400) @@ -848,6 +850,9 @@ HttpStateData::peerSupportsConnectionPinning() const if (!_peer) return true; + if (!flags.proxying) + return true; + /*If this peer does not support connection pinning (authenticated connections) return false */ @@ -1661,7 +1666,7 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe Http::HdrType header = flags.originpeer ? Http::HdrType::AUTHORIZATION : Http::HdrType::PROXY_AUTHORIZATION; /* Nothing to do unless we are forwarding to a peer */ - if (!request->flags.proxying) + if (!flags.proxying) return; /* Needs to be explicitly enabled */ @@ -1870,7 +1875,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, /* append Authorization if known in URL, not in header and going direct */ if (!hdr_out->has(Http::HdrType::AUTHORIZATION)) { - if (!request->flags.proxying && !request->url.userInfo().isEmpty()) { + if (!flags.proxying && !request->url.userInfo().isEmpty()) { static char result[base64_encode_len(MAX_URL*2)]; // should be big enough for a single URI segment struct base64_encode_ctx ctx; base64_encode_init(&ctx); @@ -2156,7 +2161,7 @@ HttpStateData::buildRequestPrefix(MemBuf * mb) * not the one we are sending. Needs checking. */ const AnyP::ProtocolVersion httpver = Http::ProtocolVersion(); - const SBuf url(_peer && !_peer->options.originserver ? request->effectiveRequestUri() : request->url.path()); + const SBuf url(flags.proxying ? request->effectiveRequestUri() : request->url.path()); mb->appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\r\n", SQUIDSBUFPRINT(request->method.image()), SQUIDSBUFPRINT(url), @@ -2219,9 +2224,6 @@ HttpStateData::sendRequest() Dialer, this, HttpStateData::wroteLast); } - flags.originpeer = (_peer != NULL && _peer->options.originserver); - flags.proxying = (_peer != NULL && !flags.originpeer); - /* * Is keep-alive okay for all request methods? */ @@ -2233,6 +2235,8 @@ HttpStateData::sendRequest() flags.keepalive = false; else if (_peer == NULL) flags.keepalive = true; + else if (!flags.proxying) + flags.keepalive = true; else if (_peer->stats.n_keepalives_sent < 10) flags.keepalive = true; else if ((double) _peer->stats.n_keepalives_recv / @@ -2251,7 +2255,7 @@ HttpStateData::sendRequest() But I suppose it was a bug */ - if (neighborType(_peer, request->url) == PEER_SIBLING && !_peer->options.allow_miss) + if (flags.proxying && neighborType(_peer, request->url) == PEER_SIBLING && !_peer->options.allow_miss) flags.only_if_cached = true; flags.front_end_https = _peer->front_end_https; diff --git a/src/tunnel.cc b/src/tunnel.cc index e92f457a4fa..3763f090bb5 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -932,16 +932,19 @@ tunnelConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xe tunnelState->request->peer_host = conn->getPeer() ? conn->getPeer()->host : NULL; comm_add_close_handler(conn->fd, tunnelServerClosed, tunnelState); - if (const auto * const peer = conn->getPeer()) + bool peering; + if (const auto * const peer = conn->getPeer()) { tunnelState->request->prepForPeering(*peer); - else + peering = !peer->options.originserver; + } else { tunnelState->request->prepForDirect(); + peering = false; + } - if (tunnelState->request->flags.proxying) + if (peering) tunnelState->connectToPeer(); - else { + else tunnelState->notePeerReadyToShovel(); - } AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); From 5eb5300db701a78951a712f8912b46c6623e42cf Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 30 Jan 2019 18:28:55 +0200 Subject: [PATCH 41/59] Replace the StateFlags proxying/originpeer with the toOrigin/toProxy/peering The three new flags make easier to handle the different cases: toOrigin: Request destined to an origin (peer or direct) toProxy: Request destined to a proxy peering: Request destined to a proxy (origin peer or proxy) Maybe the toOrigin must be replaced with a flag toOriginPeer, it is used only to express origin peers in conjunction with the peering flag. Also this patch documents in the code the undocumented login=PROXYPASS cache_peer option. --- src/clients/HttpTunneler.cc | 3 +- src/http.cc | 55 ++++++++++++++++++++----------------- src/http/StateFlags.h | 5 ++-- 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 7303c352265..3c80ba9dae5 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -127,7 +127,8 @@ Http::Tunneler::writeRequest() HttpHeader hdr_out(hoRequest); Http::StateFlags flags; memset(&flags, '\0', sizeof(flags)); - flags.proxying = true; + flags.toProxy = true; + flags.peering = true; MemBuf mb; mb.init(); mb.appendf("CONNECT %s HTTP/1.1\r\n", url.c_str()); diff --git a/src/http.cc b/src/http.cc index 10aedf8b25f..4a7f38c433b 100644 --- a/src/http.cc +++ b/src/http.cc @@ -102,9 +102,9 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : bool tlsTunnel = request->flags.sslBumped || // http request through bumped connection request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' request (this check superset request->flags.sslBump check) - // TODO: split {proxying,originserver} into {peering,toProxy,toOrigin} - flags.originpeer = (_peer != NULL && _peer->options.originserver); - flags.proxying = (_peer != NULL && !tlsTunnel && !flags.originpeer); + flags.toOrigin = (_peer || tlsTunnel || _peer->options.originserver); + flags.toProxy = (_peer && !flags.toOrigin); + flags.peering = (_peer && !tlsTunnel); if (_peer) { /* @@ -112,7 +112,7 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : * We might end up getting the object from somewhere else if, * for example, the request to this neighbor fails. */ - if (_peer->options.proxy_only) + if (flags.peering && _peer->options.proxy_only) entry->releaseRequest(true); #if USE_DELAY_POOLS @@ -630,11 +630,11 @@ void HttpStateData::keepaliveAccounting(HttpReply *reply) { if (flags.keepalive) - if (_peer && flags.proxying) + if (flags.peering) ++ _peer->stats.n_keepalives_sent; if (reply->keep_alive) { - if (_peer && flags.proxying) + if (flags.peering) ++ _peer->stats.n_keepalives_recv; if (Config.onoff.detect_broken_server_pconns @@ -649,7 +649,7 @@ HttpStateData::keepaliveAccounting(HttpReply *reply) void HttpStateData::checkDateSkew(HttpReply *reply) { - if (reply->date > -1 && !flags.proxying) { + if (reply->date > -1 && !flags.peering) { int skew = abs((int)(reply->date - squid_curtime)); if (skew > 86400) @@ -847,10 +847,7 @@ HttpStateData::proceedAfter1xx() bool HttpStateData::peerSupportsConnectionPinning() const { - if (!_peer) - return true; - - if (!flags.proxying) + if (!flags.peering) // implies _peer!=nil return true; /*If this peer does not support connection pinning (authenticated @@ -1663,16 +1660,15 @@ HttpStateData::doneWithServer() const static void httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHeader * hdr_out, const Http::StateFlags &flags) { - Http::HdrType header = flags.originpeer ? Http::HdrType::AUTHORIZATION : Http::HdrType::PROXY_AUTHORIZATION; - /* Nothing to do unless we are forwarding to a peer */ - if (!flags.proxying) + if (!flags.peering) return; /* Needs to be explicitly enabled */ if (!request->peer_login) return; + Http::HdrType header = flags.toOrigin ? Http::HdrType::AUTHORIZATION : Http::HdrType::PROXY_AUTHORIZATION; /* Maybe already dealt with? */ if (hdr_out->has(header)) return; @@ -1681,8 +1677,17 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe if (strcmp(request->peer_login, "PASSTHRU") == 0) return; - /* PROXYPASS is a special case, single-signon to servers with the proxy password (basic only) */ - if (flags.originpeer && strcmp(request->peer_login, "PROXYPASS") == 0 && hdr_in->has(Http::HdrType::PROXY_AUTHORIZATION)) { + /* PROXYPASS is a special case, single-signon to servers with + the proxy password (basic only) + This is made undocumented with commit ee0b94f4 + + Documentation from commit 11e4c5e5: + Send login details received from client to this peer. + Only WWW-Authorization headers are passed to the peer. If the + 'originserver' option is also used this will convert Proxy-Authorization: + to WWW-Authorization: before relaying. The header content is not altered. + */ + if (flags.toOrigin && strcmp(request->peer_login, "PROXYPASS") == 0 && hdr_in->has(Http::HdrType::PROXY_AUTHORIZATION)) { const char *auth = hdr_in->getStr(Http::HdrType::PROXY_AUTHORIZATION); if (auth && strncasecmp(auth, "basic ", 6) == 0) { @@ -1875,7 +1880,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, /* append Authorization if known in URL, not in header and going direct */ if (!hdr_out->has(Http::HdrType::AUTHORIZATION)) { - if (!flags.proxying && !request->url.userInfo().isEmpty()) { + if (!flags.peering && !request->url.userInfo().isEmpty()) { static char result[base64_encode_len(MAX_URL*2)]; // should be big enough for a single URI segment struct base64_encode_ctx ctx; base64_encode_init(&ctx); @@ -1960,7 +1965,7 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co * Only pass on proxy authentication to peers for which * authentication forwarding is explicitly enabled */ - if (!flags.originpeer && flags.proxying && request->peer_login && + if (flags.toProxy && request->peer_login && (strcmp(request->peer_login, "PASS") == 0 || strcmp(request->peer_login, "PROXYPASS") == 0 || strcmp(request->peer_login, "PASSTHRU") == 0)) { @@ -1985,9 +1990,7 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co /** \par WWW-Authorization: * Pass on WWW authentication */ - if (!flags.originpeer) { - hdr_out->addEntry(e->clone()); - } else { + if (flags.toOrigin && flags.peering) { /** \note In accelerators, only forward authentication if enabled * (see also httpFixupAuthentication for special cases) */ @@ -1997,6 +2000,8 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co strcmp(request->peer_login, "PROXYPASS") == 0)) { hdr_out->addEntry(e->clone()); } + } else { + hdr_out->addEntry(e->clone()); } break; @@ -2161,7 +2166,7 @@ HttpStateData::buildRequestPrefix(MemBuf * mb) * not the one we are sending. Needs checking. */ const AnyP::ProtocolVersion httpver = Http::ProtocolVersion(); - const SBuf url(flags.proxying ? request->effectiveRequestUri() : request->url.path()); + const SBuf url(flags.toProxy ? request->effectiveRequestUri() : request->url.path()); mb->appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\r\n", SQUIDSBUFPRINT(request->method.image()), SQUIDSBUFPRINT(url), @@ -2235,7 +2240,7 @@ HttpStateData::sendRequest() flags.keepalive = false; else if (_peer == NULL) flags.keepalive = true; - else if (!flags.proxying) + else if (!flags.peering) flags.keepalive = true; else if (_peer->stats.n_keepalives_sent < 10) flags.keepalive = true; @@ -2243,7 +2248,7 @@ HttpStateData::sendRequest() (double) _peer->stats.n_keepalives_sent > 0.50) flags.keepalive = true; - if (_peer) { + if (flags.peering) { /*The old code here was if (neighborType(_peer, request->url) == PEER_SIBLING && ... which is equivalent to: @@ -2255,7 +2260,7 @@ HttpStateData::sendRequest() But I suppose it was a bug */ - if (flags.proxying && neighborType(_peer, request->url) == PEER_SIBLING && !_peer->options.allow_miss) + if (neighborType(_peer, request->url) == PEER_SIBLING && !_peer->options.allow_miss) flags.only_if_cached = true; flags.front_end_https = _peer->front_end_https; diff --git a/src/http/StateFlags.h b/src/http/StateFlags.h index cb7cc90fca9..dd769be614b 100644 --- a/src/http/StateFlags.h +++ b/src/http/StateFlags.h @@ -16,12 +16,13 @@ class StateFlags { public: unsigned int front_end_https = 0; ///< send "Front-End-Https: On" header (off/on/auto=2) - bool proxying = false; bool keepalive = false; bool only_if_cached = false; bool handling1xx = false; ///< we are ignoring or forwarding 1xx response bool headers_parsed = false; - bool originpeer = false; + bool toOrigin = false; ///< Request destined to an origin (peer or direct) + bool toProxy = false; ///< Request destined to a proxy + bool peering = false; ///< Request destined to a proxy (origin peer or proxy) bool keepalive_broken = false; bool abuse_detected = false; bool request_sent = false; From a27bae5988103cb979fa3cb492b46f2ecfdcab33 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 15 Feb 2019 16:49:32 -0700 Subject: [PATCH 42/59] Polished new/renamed StatFlags documentation I am not sure whether my definitions match the code because the code has bugs (to be fixed later in this branch) and because two of the definitions I am polishing (toProxy and peering) were the same. In the subsequent commits, I will make the code match my definitions and/or adjust the definitions. I also did not like how easy it was to misinterpret "Request destined" as the ultimate/final request destination rather than the next hop. These polished definitions expose one or two problems: 1. We have two mutually exclusive boolean flags. That dangerous flag redundancy should be replaced by methods. (Marked as an XXX for now.) 2. The .peering flag seems to duplicate HttpStateData::_peer member. That duplication is necessary if non-HttpStateData users of StateFlags need this flag, but I have not checked whether they do. (Marked with a TODO for now.) --- src/http/StateFlags.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/http/StateFlags.h b/src/http/StateFlags.h index dd769be614b..f4d2566854e 100644 --- a/src/http/StateFlags.h +++ b/src/http/StateFlags.h @@ -20,9 +20,14 @@ class StateFlags bool only_if_cached = false; bool handling1xx = false; ///< we are ignoring or forwarding 1xx response bool headers_parsed = false; - bool toOrigin = false; ///< Request destined to an origin (peer or direct) - bool toProxy = false; ///< Request destined to a proxy - bool peering = false; ///< Request destined to a proxy (origin peer or proxy) + + /* these three flags describe the next TCP hop */ + // XXX: .toOrigin is !.toProxy + // TODO: confirm that .peering is needed or use _peer instead + bool toOrigin = false; ///< an origin server or originserver cache_peer + bool toProxy = false; ///< a non-originserver cache_peer + bool peering = false; ///< any cache_peer, including originserver + bool keepalive_broken = false; bool abuse_detected = false; bool request_sent = false; From 8eb385a8d6f8e932f8f03ae80a8a23aee070dd72 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 15 Feb 2019 18:18:40 -0700 Subject: [PATCH 43/59] Polished resurrected PROXYPASS documentation Resurrecting PROXYPASS documentation from squid.conf may have been a good idea, but that squid.conf documentation covered _all_ PROXYPASS cases while the if-statement where it was resurrected covered a very specific/special sub-case. Placing the original documentation here "as is" resulted in comments that did not match the code and confused me quite a bit! I adjusted the docs to match their location and to emphasize that this is not the only place where PROXYPASS is handled. Also split the long/complex if-statement condition into multiple lines. --- src/http.cc | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/http.cc b/src/http.cc index 4a7f38c433b..28737cf91b6 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1677,17 +1677,14 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe if (strcmp(request->peer_login, "PASSTHRU") == 0) return; - /* PROXYPASS is a special case, single-signon to servers with - the proxy password (basic only) - This is made undocumented with commit ee0b94f4 - - Documentation from commit 11e4c5e5: - Send login details received from client to this peer. - Only WWW-Authorization headers are passed to the peer. If the - 'originserver' option is also used this will convert Proxy-Authorization: - to WWW-Authorization: before relaying. The header content is not altered. - */ - if (flags.toOrigin && strcmp(request->peer_login, "PROXYPASS") == 0 && hdr_in->has(Http::HdrType::PROXY_AUTHORIZATION)) { + // Dangerous and undocumented PROXYPASS is a single-signon to servers with + // the proxy password. Only Basic Authentication can work this way. This + // statement forwards a "basic" Proxy-Authorization value from our client + // to an originserver peer. Other PROXYPASS cases are handled lower. + if (flags.toOrigin && + strcmp(request->peer_login, "PROXYPASS") == 0 && + hdr_in->has(Http::HdrType::PROXY_AUTHORIZATION)) { + const char *auth = hdr_in->getStr(Http::HdrType::PROXY_AUTHORIZATION); if (auth && strncasecmp(auth, "basic ", 6) == 0) { From d69070b86a65bc5430e14602e1a169186c190a76 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 16 Feb 2019 12:37:04 -0700 Subject: [PATCH 44/59] Fixed misnamed "peering" in tunnelConnectDone() The variable value and usage were correct, but they match (!StateFlags::toOrigin) rather than StateFlags::peering. Also removed unnecessary difference with master. --- src/tunnel.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/tunnel.cc b/src/tunnel.cc index 3763f090bb5..010c387d370 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -932,19 +932,20 @@ tunnelConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xe tunnelState->request->peer_host = conn->getPeer() ? conn->getPeer()->host : NULL; comm_add_close_handler(conn->fd, tunnelServerClosed, tunnelState); - bool peering; + bool toOrigin = false; // same semantics as StateFlags::toOrigin if (const auto * const peer = conn->getPeer()) { tunnelState->request->prepForPeering(*peer); - peering = !peer->options.originserver; + toOrigin = peer->options.originserver; } else { tunnelState->request->prepForDirect(); - peering = false; + toOrigin = true; } - if (peering) + if (!toOrigin) tunnelState->connectToPeer(); - else + else { tunnelState->notePeerReadyToShovel(); + } AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); From 359ce5d7c54f8bc41a5ae9f838c1ae9e8d1dd088 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 16 Feb 2019 15:13:33 -0700 Subject: [PATCH 45/59] Clarified why (toOrigin && peering) means "In accelerators" In many setups, talking to a cache_peer originserver parent does _not_ imply that the child proxy is an accelerator, but that is what this code currently assumes. I am not changing the code here -- just clarifying the (sometimes incorrect) assumption that the code is based on. --- src/http.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/http.cc b/src/http.cc index 28737cf91b6..c882063531b 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1988,7 +1988,8 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co * Pass on WWW authentication */ if (flags.toOrigin && flags.peering) { - /** \note In accelerators, only forward authentication if enabled + /** \note Assume that talking to a cache_peer originserver makes + * us a reverse proxy and only forward authentication if enabled * (see also httpFixupAuthentication for special cases) */ if (request->peer_login && From 7102e2f03378f5b1bf037f290e57c709ced8a946 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 16 Feb 2019 16:04:24 -0700 Subject: [PATCH 46/59] Removed false implication that HttpTunneler may talk to origin servers We are sending unconditional CONNECTs. We better be talking to proxies! --- src/clients/HttpTunneler.cc | 7 +++---- src/clients/HttpTunneler.h | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 3c80ba9dae5..f99bbb7acea 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -70,10 +70,9 @@ Http::Tunneler::start() Must(url.length()); Must(lifetimeLimit >= 0); - if (const auto peer = connection->getPeer()) - request->prepForPeering(*peer); - else - request->prepForDirect(); + const auto peer = connection->getPeer(); + Must(peer); // bail if our peer was reconfigured away + request->prepForPeering(*peer); watchForClosures(); writeRequest(); diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index 1efbdc4a6fb..da279a18446 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -26,7 +26,7 @@ typedef RefCount AccessLogEntryPointer; namespace Http { -/// Establishes an HTTP CONNECT tunnel through another proxy. +/// Establishes an HTTP CONNECT tunnel through a forward proxy. /// /// The caller receives a call back with Http::TunnelerAnswer. /// @@ -56,7 +56,7 @@ class Tunneler: virtual public AsyncJob /* configuration; too many fields to use constructor parameters */ HttpRequestPointer request; ///< peer connection trigger or cause - Comm::ConnectionPointer connection; ///< TCP connection to peer or origin + Comm::ConnectionPointer connection; ///< TCP connection to the cache_peer AccessLogEntryPointer al; ///< info for the future access.log entry AsyncCall::Pointer callback; ///< we call this with the results SBuf url; ///< request-target for the CONNECT request From 453d3a4bec2156e304831e32a566d70a9b7fd079 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 16 Feb 2019 16:22:20 -0700 Subject: [PATCH 47/59] Fixed crashes due to a typo in toOrigin initialization --- src/http.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http.cc b/src/http.cc index c882063531b..94be51f54d7 100644 --- a/src/http.cc +++ b/src/http.cc @@ -102,7 +102,7 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : bool tlsTunnel = request->flags.sslBumped || // http request through bumped connection request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' request (this check superset request->flags.sslBump check) - flags.toOrigin = (_peer || tlsTunnel || _peer->options.originserver); + flags.toOrigin = (!_peer || tlsTunnel || _peer->options.originserver); flags.toProxy = (_peer && !flags.toOrigin); flags.peering = (_peer && !tlsTunnel); From b8d512cb1233ef99d29046b513df9bcd6d3a1a0e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 17 Feb 2019 14:55:50 -0700 Subject: [PATCH 48/59] Redefined toOrigin as describing the next HTTP, not TCP hop ... and refactored related flags to better accommodate its users. .toOrigin initialization in the HttpStateData constructor has not changed, only the flag definition has. Both are in sync now. .peering initialization has changed to avoid the difference between .peering and _peer. Complete elimination of .peering is difficult because some StateFlags users outside HttpStateData do not have easy access to _peer, but if the two fields are the same, then there is no conflict between them, despite the duplication -- where both are available, they can be used interchangeably. .tunneling was added to handle cases where HTTP headers are not going to be interpreted by the peer we are talking to and, hence, should not be based on that peer HTTP properties or affect that peer HTTP statistics. Several HttpStateData flags uses were changed, including flags.keepalive calculation. I believe the current uses are correct (or at least "better"), but they can benefit from another review/testing. Also removed explicit memset(flags) because StateFlags implicit default constructor does that. --- src/clients/HttpTunneler.cc | 4 +-- src/http.cc | 50 ++++++++++++++++++++++--------------- src/http/StateFlags.h | 24 +++++++++++++----- 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index f99bbb7acea..48850e5d67b 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -125,9 +125,9 @@ Http::Tunneler::writeRequest() HttpHeader hdr_out(hoRequest); Http::StateFlags flags; - memset(&flags, '\0', sizeof(flags)); - flags.toProxy = true; flags.peering = true; + // flags.tunneling = false; // the CONNECT request itself is not tunneled + // flags.toOrigin = false; // the next HTTP hop is a non-originserver peer MemBuf mb; mb.init(); mb.appendf("CONNECT %s HTTP/1.1\r\n", url.c_str()); diff --git a/src/http.cc b/src/http.cc index 94be51f54d7..ee9a17d1157 100644 --- a/src/http.cc +++ b/src/http.cc @@ -99,12 +99,14 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : if (fwd->serverConnection() != NULL) _peer = cbdataReference(fwd->serverConnection()->getPeer()); /* might be NULL */ - bool tlsTunnel = request->flags.sslBumped || // http request through bumped connection - request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' request (this check superset request->flags.sslBump check) + // XXX: FwdState::connectDone() uses request->flags.sslPeek instead + const auto https = + request->flags.sslBumped || // HTTP request inside a bumped connection + request->url.getScheme() == AnyP::PROTO_HTTPS; // GET https://... - flags.toOrigin = (!_peer || tlsTunnel || _peer->options.originserver); - flags.toProxy = (_peer && !flags.toOrigin); - flags.peering = (_peer && !tlsTunnel); + flags.peering = _peer; + flags.tunneling = (_peer && https); + flags.toOrigin = (!_peer || _peer->options.originserver || https); if (_peer) { /* @@ -112,7 +114,7 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : * We might end up getting the object from somewhere else if, * for example, the request to this neighbor fails. */ - if (flags.peering && _peer->options.proxy_only) + if (!flags.tunneling && _peer->options.proxy_only) entry->releaseRequest(true); #if USE_DELAY_POOLS @@ -630,11 +632,11 @@ void HttpStateData::keepaliveAccounting(HttpReply *reply) { if (flags.keepalive) - if (flags.peering) + if (flags.peering && !flags.tunneling) ++ _peer->stats.n_keepalives_sent; if (reply->keep_alive) { - if (flags.peering) + if (flags.peering && !flags.tunneling) ++ _peer->stats.n_keepalives_recv; if (Config.onoff.detect_broken_server_pconns @@ -649,7 +651,7 @@ HttpStateData::keepaliveAccounting(HttpReply *reply) void HttpStateData::checkDateSkew(HttpReply *reply) { - if (reply->date > -1 && !flags.peering) { + if (reply->date > -1 && flags.toOrigin) { int skew = abs((int)(reply->date - squid_curtime)); if (skew > 86400) @@ -847,7 +849,11 @@ HttpStateData::proceedAfter1xx() bool HttpStateData::peerSupportsConnectionPinning() const { - if (!flags.peering) // implies _peer!=nil + if (!_peer) + return true; + + // we are talking "through" rather than "to" our _peer + if (flags.tunneling) return true; /*If this peer does not support connection pinning (authenticated @@ -1664,11 +1670,15 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe if (!flags.peering) return; + // our HTTP headers are not for the peer we tunnel this request through + if (flags.tunneling) + return; + /* Needs to be explicitly enabled */ if (!request->peer_login) return; - Http::HdrType header = flags.toOrigin ? Http::HdrType::AUTHORIZATION : Http::HdrType::PROXY_AUTHORIZATION; + const auto header = flags.toOrigin ? Http::HdrType::AUTHORIZATION : Http::HdrType::PROXY_AUTHORIZATION; /* Maybe already dealt with? */ if (hdr_out->has(header)) return; @@ -1877,7 +1887,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, /* append Authorization if known in URL, not in header and going direct */ if (!hdr_out->has(Http::HdrType::AUTHORIZATION)) { - if (!flags.peering && !request->url.userInfo().isEmpty()) { + if (flags.toOrigin && !request->url.userInfo().isEmpty()) { static char result[base64_encode_len(MAX_URL*2)]; // should be big enough for a single URI segment struct base64_encode_ctx ctx; base64_encode_init(&ctx); @@ -1962,7 +1972,7 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co * Only pass on proxy authentication to peers for which * authentication forwarding is explicitly enabled */ - if (flags.toProxy && request->peer_login && + if (!flags.toOrigin && request->peer_login && (strcmp(request->peer_login, "PASS") == 0 || strcmp(request->peer_login, "PROXYPASS") == 0 || strcmp(request->peer_login, "PASSTHRU") == 0)) { @@ -1987,7 +1997,9 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co /** \par WWW-Authorization: * Pass on WWW authentication */ - if (flags.toOrigin && flags.peering) { + if (!flags.toOriginPeer()) { + hdr_out->addEntry(e->clone()); + } else { /** \note Assume that talking to a cache_peer originserver makes * us a reverse proxy and only forward authentication if enabled * (see also httpFixupAuthentication for special cases) @@ -1998,8 +2010,6 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co strcmp(request->peer_login, "PROXYPASS") == 0)) { hdr_out->addEntry(e->clone()); } - } else { - hdr_out->addEntry(e->clone()); } break; @@ -2164,7 +2174,7 @@ HttpStateData::buildRequestPrefix(MemBuf * mb) * not the one we are sending. Needs checking. */ const AnyP::ProtocolVersion httpver = Http::ProtocolVersion(); - const SBuf url(flags.toProxy ? request->effectiveRequestUri() : request->url.path()); + const SBuf url(flags.toOrigin ? request->url.path() : request->effectiveRequestUri()); mb->appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\r\n", SQUIDSBUFPRINT(request->method.image()), SQUIDSBUFPRINT(url), @@ -2236,9 +2246,9 @@ HttpStateData::sendRequest() flags.keepalive = request->persistent(); else if (!Config.onoff.server_pconns) flags.keepalive = false; - else if (_peer == NULL) + else if (flags.tunneling) flags.keepalive = true; - else if (!flags.peering) + else if (_peer == NULL) flags.keepalive = true; else if (_peer->stats.n_keepalives_sent < 10) flags.keepalive = true; @@ -2246,7 +2256,7 @@ HttpStateData::sendRequest() (double) _peer->stats.n_keepalives_sent > 0.50) flags.keepalive = true; - if (flags.peering) { + if (_peer && !flags.tunneling) { /*The old code here was if (neighborType(_peer, request->url) == PEER_SIBLING && ... which is equivalent to: diff --git a/src/http/StateFlags.h b/src/http/StateFlags.h index f4d2566854e..de3e6878a16 100644 --- a/src/http/StateFlags.h +++ b/src/http/StateFlags.h @@ -21,12 +21,24 @@ class StateFlags bool handling1xx = false; ///< we are ignoring or forwarding 1xx response bool headers_parsed = false; - /* these three flags describe the next TCP hop */ - // XXX: .toOrigin is !.toProxy - // TODO: confirm that .peering is needed or use _peer instead - bool toOrigin = false; ///< an origin server or originserver cache_peer - bool toProxy = false; ///< a non-originserver cache_peer - bool peering = false; ///< any cache_peer, including originserver + /// Whether the next TCP hop is a cache_peer, including originserver + bool peering = false; + + /// Whether this request is being forwarded inside a CONNECT tunnel + /// through a [non-originserver] cache_peer; implies peering and toOrigin + bool tunneling = false; + + /// Whether the next HTTP hop is an origin server, including an + /// originserver cache_peer. The three possible cases are: + /// 1. a direct TCP/HTTP connection to an origin server, + /// 2. a direct TCP/HTTP connection to an originserver cache_peer, and + /// 3. a CONNECT tunnel through a [non-originserver] cache_peer [to an origin server] + /// Thus, toOrigin is false only when the HTTP request is sent over + /// a direct TCP/HTTP connection to a non-originserver cache_peer. + bool toOrigin = false; + + /// Whether the next TCP/HTTP hop is an originserver cache_peer. + bool toOriginPeer() const { return toOrigin && peering && !tunneling; } bool keepalive_broken = false; bool abuse_detected = false; From 2efe6fb261844fa3fd44546d6d6691c74772d64f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 17 Feb 2019 15:12:40 -0700 Subject: [PATCH 49/59] No NULLs in new code. Also spellchecked and polished a few comments. --- src/FwdState.cc | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 0ef7f964cc8..b9fee6454c9 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -768,7 +768,7 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in const bool originWantsEncryptedTraffic = request->method == Http::METHOD_CONNECT || request->flags.sslPeek || - request->url.getScheme() == AnyP::PROTO_HTTPS; // 'get https://xxx' or bump client-first + request->url.getScheme() == AnyP::PROTO_HTTPS; // 'GET https://...' or bump client-first if (originWantsEncryptedTraffic && // the "encrypted traffic" part !peer->options.originserver && // the "through a proxy" part !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part @@ -848,8 +848,8 @@ FwdState::secureConnectionToPeerIfNeeded() request->method == Http::METHOD_CONNECT; const bool needTlsToPeer = peerWantsTls && !userWillTlsToPeerForUs; - // 'get https://xxx' requests or bump client-first, excluding - // https proxies (TLS inside TLS is not supported yet) + // 'GET https://...' requests or bump client-first, excluding HTTPS + // proxies (TLS inside TLS is not supported yet) const bool needTlsToOrigin = !peerWantsTls && request->url.getScheme() == AnyP::PROTO_HTTPS; if (needTlsToPeer || needTlsToOrigin || request->flags.sslPeek) { @@ -990,13 +990,12 @@ FwdState::connectStart() request->hier.startPeerClock(); - // Bumped requests require their pinned connection. Since we failed to reuse - // the pinned connection, we now must terminate the bumped request. - // For client-first bumping mode the request is already bumped but the - // connection to the server is not established yet. - // When a connection bumped on step1 the result is equivalent to - // client-first bumping mode. We can recognise such cases because there is - // not ConnStateData::sslServerBump object. + // Requests bumped at step2+ require their pinned connection. Since we + // failed to reuse the pinned connection, we now must terminate the + // bumped request. For client-first and step1 bumped requests, the + // from-client connection is already bumped, but the connection to the + // server is not established/pinned so they must be excluded. We can + // recognize step1 bumping by nil ConnStateData::serverBump(). #if USE_OPENSSL const auto clientFirstBump = request->clientConnectionManager.valid() && (request->clientConnectionManager->sslBumpMode == Ssl::bumpClientFirst || @@ -1011,7 +1010,7 @@ FwdState::connectStart() const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; debugs(17, level, "BUG: Lost previously bumped from-Squid connection. Rejecting bumped request."); fail(new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al)); - self = NULL; // refcounted + self = nullptr; // refcounted return; } From 12fa4efdb82c14f4cfad49567374d81d3c2f3554 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 5 Mar 2019 13:11:40 +0200 Subject: [PATCH 50/59] Set ErrorState::response_ to passed HttpReply object This is lost with the commit 3265fd78 where the ErrorPage constructors fixes. As a result squid sent back the status code read from server but reported "Internal Error: Missing Template ERR_RELAY_REMOTE" (produced while tried to build a new HttpReply object). --- src/errorpage.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/errorpage.cc b/src/errorpage.cc index f4a257c6886..1d696609d5c 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -698,6 +698,7 @@ ErrorState::ErrorState(HttpRequest * req, HttpReply *errorReply) : ErrorState(ERR_RELAY_REMOTE) { Must(errorReply); + response_ = errorReply; httpStatus = errorReply->sline.status(); if (req) { From 2b181ed7d5070a630baa60993eb352153c5a57bc Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 7 Mar 2019 13:35:11 +0200 Subject: [PATCH 51/59] Convert Http::Tunneler::CbDialer to be a template class ... which derived from CallDialer and Http::TunnelerAnswer, to avoid creating a dedicated class for each peer connector/tunneler. The classes FwdStatePeerAnswerDialer2 and TunnelStateData::MyAnswerDialer2 are not needed anymore and removed. --- src/FwdState.cc | 30 +----------------------------- src/clients/HttpTunneler.cc | 9 ++++----- src/clients/HttpTunneler.h | 25 ++++++++++++++++++++----- src/tunnel.cc | 30 +----------------------------- 4 files changed, 26 insertions(+), 68 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index b9fee6454c9..5b6e3b080e9 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -19,7 +19,6 @@ #include "client_side.h" #include "clients/forward.h" #include "clients/HttpTunneler.h" -#include "clients/HttpTunnelerAnswer.h" #include "comm/Connection.h" #include "comm/ConnOpener.h" #include "comm/Loops.h" @@ -103,33 +102,6 @@ class FwdStatePeerAnswerDialer: public CallDialer, public Security::PeerConnecto Security::EncryptorAnswer answer_; }; -// XXX: Find a better way to pass answers than to create a dedicated class -// for each peer connector/tunneler! -/// Gives Http::Tunneler access to Answer in the FwdState callback dialer. -class FwdStatePeerAnswerDialer2: public CallDialer, public Http::Tunneler::CbDialer -{ -public: - typedef void (FwdState::*Method)(Http::TunnelerAnswer &); - - FwdStatePeerAnswerDialer2(Method method, FwdState *fwd): - method_(method), fwd_(fwd), answer_() {} - - /* CallDialer API */ - virtual bool canDial(AsyncCall &call) { return fwd_.valid(); } - void dial(AsyncCall &call) { ((&(*fwd_))->*method_)(answer_); } - virtual void print(std::ostream &os) const { - os << '(' << fwd_.get() << ", " << answer_ << ')'; - } - - /* Http::Tunneler::CbDialer API */ - virtual Http::TunnelerAnswer &answer() { return answer_; } - -private: - Method method_; - CbcPointer fwd_; - Http::TunnelerAnswer answer_; -}; - void FwdState::abort(void* d) { @@ -783,7 +755,7 @@ FwdState::establishTunnelThruProxy() { AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::tunnelEstablishmentDone", - FwdStatePeerAnswerDialer2(&FwdState::tunnelEstablishmentDone, this)); + Http::Tunneler::CbDialer(&FwdState::tunnelEstablishmentDone, this)); const auto tunneler = new Http::Tunneler(callback); tunneler->connection = serverConnection(); tunneler->al = al; diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 48850e5d67b..d5fa7b79515 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -9,7 +9,6 @@ #include "squid.h" #include "CachePeer.h" #include "clients/HttpTunneler.h" -#include "clients/HttpTunnelerAnswer.h" #include "comm/Read.h" #include "comm/Write.h" #include "errorpage.h" @@ -34,7 +33,7 @@ Http::Tunneler::Tunneler(AsyncCall::Pointer &aCallback): debugs(83, 5, "Http::Tunneler constructed, this=" << (void*)this); // detect callers supplying cb dialers that are not our CbDialer assert(callback); - assert(dynamic_cast(callback->getDialer())); + assert(dynamic_cast(callback->getDialer())); } Http::Tunneler::~Tunneler() @@ -53,9 +52,9 @@ Http::TunnelerAnswer & Http::Tunneler::answer() { Must(callback); - const auto dialer = dynamic_cast(callback->getDialer()); - Must(dialer); - return dialer->answer(); + const auto answer = dynamic_cast(callback->getDialer()); + Must(answer); + return *answer; } void diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index da279a18446..1485d33f209 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -11,12 +11,14 @@ #include "base/AsyncCbdataCalls.h" #include "base/AsyncJob.h" +#include "clients/forward.h" +#include "clients/HttpTunnelerAnswer.h" #include "CommCalls.h" #if USE_DELAY_POOLS #include "DelayId.h" #endif #include "http/forward.h" -#include "clients/forward.h" + class HttpReply; class ErrorState; @@ -41,12 +43,25 @@ class Tunneler: virtual public AsyncJob public: /// Callback dialer API to allow Tunneler to set the answer. - class CbDialer + template + class CbDialer: public CallDialer, public Http::TunnelerAnswer { public: - virtual ~CbDialer() {} - /// gives Tunneler access to the in-dialer answer - virtual Http::TunnelerAnswer &answer() = 0; + // initiator method to receive our answer + typedef void (Initiator::*Method)(Http::TunnelerAnswer &); + + CbDialer(Method method, Initiator *initiator): initiator_(initiator), method_(method) {} + virtual ~CbDialer() = default; + + /* CallDialer API */ + bool canDial(AsyncCall &) { return initiator_.valid(); } + void dial(AsyncCall &) {((*initiator_).*method_)(*this); } + virtual void print(std::ostream &os) const override { + os << '(' << static_cast(*this) << ')'; + } + private: + CbcPointer initiator_; ///< object to deliver the answer to + Method method_; ///< initiator_ method to call with the answer }; public: diff --git a/src/tunnel.cc b/src/tunnel.cc index 010c387d370..a04bb448da8 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -16,7 +16,6 @@ #include "client_side.h" #include "client_side_request.h" #include "clients/HttpTunneler.h" -#include "clients/HttpTunnelerAnswer.h" #include "comm.h" #include "comm/Connection.h" #include "comm/ConnOpener.h" @@ -214,33 +213,6 @@ class TunnelStateData: public PeerSelectionInitiator /// details of the "last tunneling attempt" failure (if it failed) ErrorState *savedError = nullptr; - // XXX: Find a better way to pass answers than to create a dedicated class - // for each service with a callback! - /// Gives Http::Tunneler access to Answer in the TunnelStateData callback dialer. - class MyAnswerDialer2: public CallDialer, public Http::Tunneler::CbDialer - { - public: - typedef void (TunnelStateData::*Method)(Http::TunnelerAnswer &); - - MyAnswerDialer2(Method method, TunnelStateData *tunnel): - method_(method), tunnel_(tunnel), answer_() {} - - /* CallDialer API */ - virtual bool canDial(AsyncCall &call) { return tunnel_.valid(); } - void dial(AsyncCall &call) { ((&(*tunnel_))->*method_)(answer_); } - virtual void print(std::ostream &os) const { - os << '(' << tunnel_.get() << ", " << answer_ << ')'; - } - - /* Http::Tunneler::CbDialer API */ - virtual Http::TunnelerAnswer &answer() { return answer_; } - - private: - Method method_; - CbcPointer tunnel_; - Http::TunnelerAnswer answer_; - }; - /// resumes operations after the (possibly failed) HTTP CONNECT exchange void tunnelEstablishmentDone(Http::TunnelerAnswer &answer); @@ -1029,7 +1001,7 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) AsyncCall::Pointer callback = asyncCall(5,4, "TunnelStateData::tunnelEstablishmentDone", - MyAnswerDialer2(&TunnelStateData::tunnelEstablishmentDone, this)); + Http::Tunneler::CbDialer(&TunnelStateData::tunnelEstablishmentDone, this)); const auto tunneler = new Http::Tunneler(callback); tunneler->connection = server.conn; tunneler->al = al; From 1b550768697a65b50fa90d946597f2515c8f2b7b Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 8 Mar 2019 09:48:43 +0200 Subject: [PATCH 52/59] Fix build on centos-7 Jenkins build on centos-7 reported: "error: declaration of 'answer' shadows a member of 'this'" --- src/clients/HttpTunneler.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index d5fa7b79515..ebf46d358f5 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -52,9 +52,9 @@ Http::TunnelerAnswer & Http::Tunneler::answer() { Must(callback); - const auto answer = dynamic_cast(callback->getDialer()); - Must(answer); - return *answer; + const auto tunnelerAnswer = dynamic_cast(callback->getDialer()); + Must(tunnelerAnswer); + return *tunnelerAnswer; } void From 23337dedaffabf12e5ab9669c005e2f23b67d71d Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 19 Mar 2019 17:11:24 +0200 Subject: [PATCH 53/59] Polishing fixes after Amos review Fix comments and debugs, remove empty lines, replace NULLs with nullptr, sort includes and other fixes --- src/client_side.cc | 2 +- src/clients/HttpTunneler.cc | 6 +++--- src/clients/HttpTunneler.h | 4 +--- src/clients/HttpTunnelerAnswer.cc | 4 ++-- src/clients/HttpTunnelerAnswer.h | 21 +++++++++++---------- src/errorpage.h | 1 - src/http.cc | 2 +- src/http/StateFlags.h | 6 +++--- src/ssl/ServerBump.h | 2 +- 9 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/client_side.cc b/src/client_side.cc index 0da976ebc5c..4aaf8f6d668 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2939,7 +2939,7 @@ ConnStateData::switchToHttps(ClientHttpRequest *http, Ssl::BumpMode bumpServerMo sslServerBump = new Ssl::ServerBump(http); } else if (bumpServerMode == Ssl::bumpPeek || bumpServerMode == Ssl::bumpStare) { request->flags.sslPeek = true; - sslServerBump = new Ssl::ServerBump(http, NULL, bumpServerMode); + sslServerBump = new Ssl::ServerBump(http, nullptr, bumpServerMode); } // commSetConnTimeout() was called for this request before we switched. diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index ebf46d358f5..b69da8303f6 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -1,5 +1,5 @@ /* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * Copyright (C) 1996-2019 The Squid Software Foundation and contributors * * Squid software is distributed under GPLv2+ license and includes * contributions from numerous individuals and organizations. @@ -285,8 +285,8 @@ Http::Tunneler::handleResponse(const bool eof) futureAnswer.peerResponseStatus = rep->sline.status(); request->hier.peer_reply_status = rep->sline.status(); - debugs(11, 2, "HTTP Server " << connection); - debugs(11, 2, "HTTP Server RESPONSE:\n---------\n" << + debugs(11, 2, "Tunnel Server " << connection); + debugs(11, 2, "Tunnel Server RESPONSE:\n---------\n" << Raw(nullptr, readBuf.rawContent(), rep->hdr_sz).minLevel(2).gap(false) << "----------"); diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index 1485d33f209..c11ba681710 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * Copyright (C) 1996-2019 The Squid Software Foundation and contributors * * Squid software is distributed under GPLv2+ license and includes * contributions from numerous individuals and organizations. @@ -19,8 +19,6 @@ #endif #include "http/forward.h" - -class HttpReply; class ErrorState; class AccessLogEntry; typedef RefCount AccessLogEntryPointer; diff --git a/src/clients/HttpTunnelerAnswer.cc b/src/clients/HttpTunnelerAnswer.cc index a10e699e7c4..66e605c6ebc 100644 --- a/src/clients/HttpTunnelerAnswer.cc +++ b/src/clients/HttpTunnelerAnswer.cc @@ -1,5 +1,5 @@ /* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * Copyright (C) 1996-2019 The Squid Software Foundation and contributors * * Squid software is distributed under GPLv2+ license and includes * contributions from numerous individuals and organizations. @@ -7,9 +7,9 @@ */ #include "squid.h" +#include "clients/HttpTunnelerAnswer.h" #include "comm/Connection.h" #include "errorpage.h" -#include "clients/HttpTunnelerAnswer.h" Http::TunnelerAnswer::~TunnelerAnswer() { diff --git a/src/clients/HttpTunnelerAnswer.h b/src/clients/HttpTunnelerAnswer.h index 5e0cb5c14ea..b549dbbfa0c 100644 --- a/src/clients/HttpTunnelerAnswer.h +++ b/src/clients/HttpTunnelerAnswer.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * Copyright (C) 1996-2019 The Squid Software Foundation and contributors * * Squid software is distributed under GPLv2+ license and includes * contributions from numerous individuals and organizations. @@ -18,14 +18,14 @@ class ErrorState; namespace Http { -// Three mutually exclusive answers are possible: -// -// * Squid-generated error object (TunnelerAnswer::squidError); -// * peer-generated error message (TunnelerAnswer::peerError); -// * successful tunnel establishment (none of the above are present). - -// HTTP CONNECT tunnel setup results (supplied via a callback). The tunnel -// through the peer was established if and only if the error member is nil. +/// Three mutually exclusive answers are possible: +/// +/// * Squid-generated error object (TunnelerAnswer::squidError); +/// * peer-generated error message (TunnelerAnswer::peerError); +/// * successful tunnel establishment (none of the above are present). +/// +/// HTTP CONNECT tunnel setup results (supplied via a callback). The tunnel +/// through the peer was established if and only if the error member is nil. class TunnelerAnswer { public: @@ -34,7 +34,8 @@ class TunnelerAnswer bool positive() const { return !squidError; } - // answer recipients must clear the error member in order to keep its info + // Destructor will erase squidError if it is still set. Answer recipients + // must clear this member to keep its info. // XXX: We should refcount ErrorState instead of cbdata-protecting it. CbcPointer squidError; ///< problem details (or nil) diff --git a/src/errorpage.h b/src/errorpage.h index 9394f3d2d5d..9b7e6f2a1b4 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -78,7 +78,6 @@ typedef void ERCB(int fd, void *, size_t); class MemBuf; class StoreEntry; class wordlist; -typedef RefCount HttpReplyPointer; namespace ErrorPage { diff --git a/src/http.cc b/src/http.cc index ee9a17d1157..ab1fa876269 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1670,7 +1670,7 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe if (!flags.peering) return; - // our HTTP headers are not for the peer we tunnel this request through + // This request is going "through" rather than "to" our _peer. if (flags.tunneling) return; diff --git a/src/http/StateFlags.h b/src/http/StateFlags.h index de3e6878a16..b6d87a2e981 100644 --- a/src/http/StateFlags.h +++ b/src/http/StateFlags.h @@ -30,9 +30,9 @@ class StateFlags /// Whether the next HTTP hop is an origin server, including an /// originserver cache_peer. The three possible cases are: - /// 1. a direct TCP/HTTP connection to an origin server, - /// 2. a direct TCP/HTTP connection to an originserver cache_peer, and - /// 3. a CONNECT tunnel through a [non-originserver] cache_peer [to an origin server] + /// -# a direct TCP/HTTP connection to an origin server, + /// -# a direct TCP/HTTP connection to an originserver cache_peer, and + /// -# a CONNECT tunnel through a [non-originserver] cache_peer [to an origin server] /// Thus, toOrigin is false only when the HTTP request is sent over /// a direct TCP/HTTP connection to a non-originserver cache_peer. bool toOrigin = false; diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index dd5035cbf40..5bb81b288d6 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -32,7 +32,7 @@ class ServerBump CBDATA_CLASS(ServerBump); public: - explicit ServerBump(ClientHttpRequest *http, StoreEntry *e = NULL, Ssl::BumpMode mode = Ssl::bumpServerFirst); + explicit ServerBump(ClientHttpRequest *http, StoreEntry *e = nullptr, Ssl::BumpMode mode = Ssl::bumpServerFirst); ~ServerBump(); void attachServerSession(const Security::SessionPointer &); ///< Sets the server TLS session object const Security::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors From 16e3fc3f0d95761da4ec7b0872190e42242fe0b8 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 20 Mar 2019 15:36:38 +0200 Subject: [PATCH 54/59] Use constructor parameters to initialise HttpTunneler members Make all HttpTunneler members private. Implement the HttpTunneler::setDelayId to set delayid parameters --- src/FwdState.cc | 10 +++------- src/clients/HttpTunneler.cc | 19 ++++++++++++++----- src/clients/HttpTunneler.h | 21 ++++++++++++--------- src/tunnel.cc | 9 ++------- 4 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 5b6e3b080e9..13a186bd956 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -756,16 +756,12 @@ FwdState::establishTunnelThruProxy() AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::tunnelEstablishmentDone", Http::Tunneler::CbDialer(&FwdState::tunnelEstablishmentDone, this)); - const auto tunneler = new Http::Tunneler(callback); - tunneler->connection = serverConnection(); - tunneler->al = al; - tunneler->request = request; - tunneler->url = request->url.authority(); - tunneler->lifetimeLimit = connectingTimeout(serverDestinations[0]); + HttpRequest::Pointer requestPointer = request; + const auto tunneler = new Http::Tunneler(serverConnection(), requestPointer, callback, connectingTimeout(serverConnection()), al); #if USE_DELAY_POOLS Must(serverConnection()->getPeer()); if (!serverConnection()->getPeer()->options.no_delay) - tunneler->delayId = entry->mem_obj->mostBytesAllowed(); + tunneler->setDelayId(entry->mem_obj->mostBytesAllowed()); #endif AsyncJob::Start(tunneler); // and wait for the tunnelEstablishmentDone() call diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index b69da8303f6..58a24acd662 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -22,18 +22,24 @@ CBDATA_NAMESPACED_CLASS_INIT(Http, Tunneler); -Http::Tunneler::Tunneler(AsyncCall::Pointer &aCallback): +Http::Tunneler::Tunneler(const Comm::ConnectionPointer &conn, const HttpRequest::Pointer &req, AsyncCall::Pointer &aCallback, time_t timeout, const AccessLogEntryPointer &alp): AsyncJob("Http::Tunneler"), + connection(conn), + request(req), callback(aCallback), - lifetimeLimit(0), + lifetimeLimit(timeout), + al(alp), startTime(squid_curtime), requestWritten(false), tunnelEstablished(false) { debugs(83, 5, "Http::Tunneler constructed, this=" << (void*)this); // detect callers supplying cb dialers that are not our CbDialer + assert(request); + assert(connection); assert(callback); assert(dynamic_cast(callback->getDialer())); + url = request->url.authority(); } Http::Tunneler::~Tunneler() @@ -41,6 +47,12 @@ Http::Tunneler::~Tunneler() debugs(83, 5, "Http::Tunneler destructed, this=" << (void*)this); } +void +Http::Tunneler::setDelayId(DelayId delay_id) +{ + delayId = delay_id; +} + bool Http::Tunneler::doneAll() const { @@ -62,10 +74,7 @@ Http::Tunneler::start() { AsyncJob::start(); - Must(request); - Must(connection); Must(al); - Must(callback); Must(url.length()); Must(lifetimeLimit >= 0); diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index c11ba681710..c7563075022 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -63,19 +63,12 @@ class Tunneler: virtual public AsyncJob }; public: - explicit Tunneler(AsyncCall::Pointer &aCallback); +explicit Tunneler(const Comm::ConnectionPointer &conn, const HttpRequestPointer &req, AsyncCall::Pointer &aCallback, time_t timeout, const AccessLogEntryPointer &alp); Tunneler(const Tunneler &) = delete; Tunneler &operator =(const Tunneler &) = delete; - /* configuration; too many fields to use constructor parameters */ - HttpRequestPointer request; ///< peer connection trigger or cause - Comm::ConnectionPointer connection; ///< TCP connection to the cache_peer - AccessLogEntryPointer al; ///< info for the future access.log entry - AsyncCall::Pointer callback; ///< we call this with the results - SBuf url; ///< request-target for the CONNECT request - time_t lifetimeLimit; ///< do not run longer than this #if USE_DELAY_POOLS - DelayId delayId; + void setDelayId(DelayId delay_id); #endif protected: @@ -106,6 +99,16 @@ class Tunneler: virtual public AsyncJob AsyncCall::Pointer reader; ///< called when the response should be read AsyncCall::Pointer closer; ///< called when the connection is being closed + Comm::ConnectionPointer connection; ///< TCP connection to the cache_peer + HttpRequestPointer request; ///< peer connection trigger or cause + AsyncCall::Pointer callback; ///< we call this with the results + SBuf url; ///< request-target for the CONNECT request + time_t lifetimeLimit; ///< do not run longer than this + AccessLogEntryPointer al; ///< info for the future access.log entry +#if USE_DELAY_POOLS + DelayId delayId; +#endif + SBuf readBuf; ///< either unparsed response or post-response bytes /// Parser being used at present to parse the HTTP peer response. Http1::ResponseParserPointer hp; diff --git a/src/tunnel.cc b/src/tunnel.cc index a04bb448da8..c3bd96baa9c 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1002,14 +1002,9 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) AsyncCall::Pointer callback = asyncCall(5,4, "TunnelStateData::tunnelEstablishmentDone", Http::Tunneler::CbDialer(&TunnelStateData::tunnelEstablishmentDone, this)); - const auto tunneler = new Http::Tunneler(callback); - tunneler->connection = server.conn; - tunneler->al = al; - tunneler->request = request; - tunneler->url = url; - tunneler->lifetimeLimit = Config.Timeout.lifetime; + const auto tunneler = new Http::Tunneler(server.conn, request, callback, Config.Timeout.lifetime, al); #if USE_DELAY_POOLS - tunneler->delayId = server.delayId; + tunneler->setDelayId(server.delayId); #endif AsyncJob::Start(tunneler); waitingForConnectExchange = true; From 646e43d940c65aa447163e39e97bf871ca0b5e24 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Mar 2019 14:44:06 +0200 Subject: [PATCH 55/59] The "GET https://" requests must forwarded to the cache peer as is This is the traditional squid behaviour. --- src/FwdState.cc | 16 +++++++++++----- src/http.cc | 9 ++------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 13a186bd956..5f71a3b972d 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -735,12 +735,16 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in assert(!request->flags.pinned); if (const CachePeer *peer = serverConnection()->getPeer()) { + // Assume that it is only possible for the client-first from the + // bumping modes to try connect to a remote server. The bumped + // requests with other modes are using pinned connections or fails. + const bool clientFirstBump = request->flags.sslBumped; // We need a CONNECT tunnel to send encrypted traffic through a proxy, // but we do not support TLS inside TLS, so we exclude HTTPS proxies. const bool originWantsEncryptedTraffic = request->method == Http::METHOD_CONNECT || request->flags.sslPeek || - request->url.getScheme() == AnyP::PROTO_HTTPS; // 'GET https://...' or bump client-first + clientFirstBump; if (originWantsEncryptedTraffic && // the "encrypted traffic" part !peer->options.originserver && // the "through a proxy" part !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part @@ -815,12 +819,14 @@ FwdState::secureConnectionToPeerIfNeeded() const bool userWillTlsToPeerForUs = p && p->options.originserver && request->method == Http::METHOD_CONNECT; const bool needTlsToPeer = peerWantsTls && !userWillTlsToPeerForUs; + const bool clientFirstBump = request->flags.sslBumped; // client-first (already) bumped connection + const bool needsBump = request->flags.sslPeek || clientFirstBump; - // 'GET https://...' requests or bump client-first, excluding HTTPS - // proxies (TLS inside TLS is not supported yet) - const bool needTlsToOrigin = !peerWantsTls && request->url.getScheme() == AnyP::PROTO_HTTPS; + // 'GET https://...' requests. If a peer is used the request is forwarded + // as is + const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS && !clientFirstBump; - if (needTlsToPeer || needTlsToOrigin || request->flags.sslPeek) { + if (needTlsToPeer || needTlsToOrigin || needsBump) { HttpRequest::Pointer requestPointer = request; AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::ConnectedToPeer", diff --git a/src/http.cc b/src/http.cc index ab1fa876269..d3997bc7516 100644 --- a/src/http.cc +++ b/src/http.cc @@ -99,14 +99,9 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : if (fwd->serverConnection() != NULL) _peer = cbdataReference(fwd->serverConnection()->getPeer()); /* might be NULL */ - // XXX: FwdState::connectDone() uses request->flags.sslPeek instead - const auto https = - request->flags.sslBumped || // HTTP request inside a bumped connection - request->url.getScheme() == AnyP::PROTO_HTTPS; // GET https://... - flags.peering = _peer; - flags.tunneling = (_peer && https); - flags.toOrigin = (!_peer || _peer->options.originserver || https); + flags.tunneling = (_peer && request->flags.sslBumped); + flags.toOrigin = (!_peer || _peer->options.originserver || request->flags.sslBumped); if (_peer) { /* From facef8baf68e33ff52d6117290716e2bdade8d61 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 21 Mar 2019 15:58:05 +0200 Subject: [PATCH 56/59] Fix step1 bumping and pconnPool Do not use pconnPool for Http requests bumped at step1 when a peer is used. Current implementation may cause squid to send HTTP requests to wrong HTTP servers. To support pconn pool for these cases we need to use as pconn keys a combination of HTTP destination (e.g., origin1:443 in our case), peer (e.g., peer1:3128 in our case), and probably some kind of "secured" or "needs TLS" setting. --- src/FwdState.cc | 3 ++- src/http.cc | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 5f71a3b972d..9c4a5cb214b 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -993,11 +993,12 @@ FwdState::connectStart() if (!serverDestinations[0]->getPeer()) host = request->url.host(); + bool bumpThroughPeer = request->flags.sslBumped && serverDestinations[0]->getPeer(); Comm::ConnectionPointer temp; // Avoid pconns after races so that the same client does not suffer twice. // This does not increase the total number of connections because we just // closed the connection that failed the race. And re-pinning assumes this. - if (pconnRace != raceHappened) + if (pconnRace != raceHappened && !bumpThroughPeer) temp = pconnPop(serverDestinations[0], host); const bool openedPconn = Comm::IsConnOpen(temp); diff --git a/src/http.cc b/src/http.cc index d3997bc7516..5f462ad80f6 100644 --- a/src/http.cc +++ b/src/http.cc @@ -2242,7 +2242,8 @@ HttpStateData::sendRequest() else if (!Config.onoff.server_pconns) flags.keepalive = false; else if (flags.tunneling) - flags.keepalive = true; + // tunneled non pinned bumped requests must not keepalive + flags.keepalive = !request->flags.sslBumped; else if (_peer == NULL) flags.keepalive = true; else if (_peer->stats.n_keepalives_sent < 10) From d62eabfef0316d3937ec0c8b0394368ed3924550 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 22 Mar 2019 12:20:56 +0200 Subject: [PATCH 57/59] Make setDelayId inlined Also fix build error caused because the Http::Tunneler::setDelayId implementation is not wrapped inside "#if USE_DELAY_POOLS/#endif" --- src/clients/HttpTunneler.cc | 6 ------ src/clients/HttpTunneler.h | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 58a24acd662..6da0c605d1a 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -47,12 +47,6 @@ Http::Tunneler::~Tunneler() debugs(83, 5, "Http::Tunneler destructed, this=" << (void*)this); } -void -Http::Tunneler::setDelayId(DelayId delay_id) -{ - delayId = delay_id; -} - bool Http::Tunneler::doneAll() const { diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index c7563075022..a7aa327c876 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -68,7 +68,7 @@ explicit Tunneler(const Comm::ConnectionPointer &conn, const HttpRequestPointer Tunneler &operator =(const Tunneler &) = delete; #if USE_DELAY_POOLS - void setDelayId(DelayId delay_id); + void setDelayId(DelayId delay_id) {delayId = delay_id;} #endif protected: From b9d5083b5e1036261b8092c218683a4fb7d16068 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 22 Mar 2019 12:24:15 +0200 Subject: [PATCH 58/59] remove 'exlpicit' from Http::Tunneler constructor It is not needed any more. --- src/clients/HttpTunneler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index a7aa327c876..4af96c5b701 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -63,7 +63,7 @@ class Tunneler: virtual public AsyncJob }; public: -explicit Tunneler(const Comm::ConnectionPointer &conn, const HttpRequestPointer &req, AsyncCall::Pointer &aCallback, time_t timeout, const AccessLogEntryPointer &alp); + Tunneler(const Comm::ConnectionPointer &conn, const HttpRequestPointer &req, AsyncCall::Pointer &aCallback, time_t timeout, const AccessLogEntryPointer &alp); Tunneler(const Tunneler &) = delete; Tunneler &operator =(const Tunneler &) = delete; From 7a0d8637149396f47fe968b63b0eb0b84384c15e Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Mon, 25 Mar 2019 11:34:54 +0200 Subject: [PATCH 59/59] Update fd status in Http::Tunneler Update fd_note () inside Http::Tunneler::writeRequest with details of what the FD is now being used for. --- src/clients/HttpTunneler.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 6da0c605d1a..fb0c8d336f5 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -12,6 +12,7 @@ #include "comm/Read.h" #include "comm/Write.h" #include "errorpage.h" +#include "fd.h" #include "fde.h" #include "http.h" #include "http/one/ResponseParser.h" @@ -144,6 +145,7 @@ Http::Tunneler::writeRequest() debugs(11, 2, "Tunnel Server REQUEST: " << connection << ":\n----------\n" << mb.buf << "\n----------"); + fd_note(connection->fd, "Tunnel Server CONNECT"); typedef CommCbMemFunT Dialer; writer = JobCallback(5, 5, Dialer, this, Http::Tunneler::handleWrittenRequest);