client_side_request.cc:2028 "request->method.id()" assertion (#971)

ConnStateData::tunnelOnError() ignored its method parameter and always
called initiateTunneledRequest() with METHOD_NONE. buildFakeRequest()
then set HttpRequest::method to METHOD_NONE. Squid does not support such
HttpRequest objects well because quite a bit of code assumes that
HttpRequest::method must be known. For example, depending on
configuration and other factors, Squid may assert.

Moreover, many Squids using the on_unsupported_protocol directive also
have special rules for handling tunnels and those rules may not work as
intended for these METHOD_NONE transactions.

Squid now uses CONNECT method when it creates a CONNECT-like request
that facilitates on_unsupported_protocol tunneling. This helps meet code
expectations about HttpRequest::method being defined and natural admin
expectations about tunneled requests having a CONNECT method.

Admins that want to distinguish on_unsupported_protocol tunnels from
other tunnels can use ACL annotations (for now). If needed, one can add
a better/dedicated way of identifying on_unsupported_protocol tunnels.

Also removed the method parameter from clientTunnelOnError() and related
methods. That method was extracted from a low-level parser field and

- for cases where the higher-level code deemed input to be non-HTTP, it
   was wrong to use essentially garbage/non-HTTP chars as a method name;
- for other cases, the method is available via HttpRequest::method.

TODO: We may be able to remove more duplicated parameters or unnecessary
checks on this code path: Perhaps clientReplyContext::setReplyToError()
method parameter can be retrieved from errstate->request? Perhaps
errstate->request itself is always the same as http->request?

This is a Measurement Factory project.

diff --git a/src/client_side.cc b/src/client_side.cc
index f52b9a5aa0bbdb1d21fc836ead31722405394844..1d770b5e7f4fe66b2e94a5557183b4600e864ee6 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1553,7 +1553,7 @@ bool ConnStateData::serveDelayedError(Http::Stream *context)
 
 /// initiate tunneling if possible or return false otherwise
 bool
-ConnStateData::tunnelOnError(const HttpRequestMethod &, const err_type requestError)
+ConnStateData::tunnelOnError(const err_type requestError)
 {
     if (!Config.accessList.on_unsupported_protocol) {
         debugs(33, 5, "disabled; send error: " << requestError);
@@ -1577,7 +1577,7 @@ ConnStateData::tunnelOnError(const HttpRequestMethod &, const err_type requestEr
         if (context)
             context->finished(); // Will remove from pipeline queue
         Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0);
-        return initiateTunneledRequest(request, Http::METHOD_NONE, "unknown-protocol", preservedClientData);
+        return initiateTunneledRequest(request, "unknown-protocol", preservedClientData);
     }
     debugs(33, 3, "denied; send error: " << requestError);
     return false;
@@ -1667,7 +1667,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         conn->quitAfterError(request.getRaw());
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
         assert (repContext);
-        repContext->setReplyToError(ERR_UNSUP_REQ, Http::scNotImplemented, request->method, NULL,
+        repContext->setReplyToError(ERR_UNSUP_REQ, Http::scNotImplemented, nullptr,
                                     conn, request.getRaw(), nullptr, nullptr);
         assert(context->http->out.offset == 0);
         context->pullData();
@@ -1681,7 +1681,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
         assert (repContext);
         conn->quitAfterError(request.getRaw());
-        repContext->setReplyToError(ERR_INVALID_REQ, frameStatus, request->method, nullptr, conn, request.getRaw(), nullptr, nullptr);
+        repContext->setReplyToError(ERR_INVALID_REQ, frameStatus, nullptr, conn, request.getRaw(), nullptr, nullptr);
         assert(context->http->out.offset == 0);
         context->pullData();
         clientProcessRequestFinished(conn, request);
@@ -1717,7 +1717,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
             assert (repContext);
             conn->quitAfterError(request.getRaw());
             repContext->setReplyToError(ERR_TOO_BIG,
-                                        Http::scPayloadTooLarge, Http::METHOD_NONE, NULL,
+                                        Http::scPayloadTooLarge, nullptr,
                                         conn, http->request, nullptr, nullptr);
             assert(context->http->out.offset == 0);
             context->pullData();
@@ -2104,7 +2104,6 @@ ConnStateData::abortChunkedRequestBody(const err_type error)
         const Http::StatusCode scode = (error == ERR_TOO_BIG) ?
                                        Http::scPayloadTooLarge : HTTP_BAD_REQUEST;
         repContext->setReplyToError(error, scode,
-                                    repContext->http->request->method,
                                     repContext->http->uri,
                                     CachePeer,
                                     repContext->http->request,
@@ -2140,7 +2139,7 @@ ConnStateData::requestTimeout(const CommTimeoutCbParams &io)
 
     const err_type error = receivedFirstByte_ ? ERR_REQUEST_PARSE_TIMEOUT : ERR_REQUEST_START_TIMEOUT;
     updateError(error);
-    if (tunnelOnError(HttpRequestMethod(), error))
+    if (tunnelOnError(error))
         return;
 
     /*
@@ -2936,7 +2935,7 @@ ConnStateData::parseTlsHandshake()
         HttpRequest::Pointer request = context->http->request;
         debugs(83, 5, "Got something other than TLS Client Hello. Cannot SslBump.");
         updateError(ERR_PROTOCOL_UNKNOWN, parseErrorDetails);
-        if (!tunnelOnError(HttpRequestMethod(), ERR_PROTOCOL_UNKNOWN))
+        if (!tunnelOnError(ERR_PROTOCOL_UNKNOWN))
             clientConnection->close();
         return;
     }
@@ -3018,7 +3017,7 @@ ConnStateData::splice()
         // respond with "Connection Established" to the client.
         // This fake CONNECT request required to allow use of SNI in
         // doCallout() checks and adaptations.
-        return initiateTunneledRequest(request, Http::METHOD_CONNECT, "splice", preservedClientData);
+        return initiateTunneledRequest(request, "splice", preservedClientData);
     }
 }
 
@@ -3110,7 +3109,7 @@ ConnStateData::handleSslBumpHandshakeError(const Security::IoResult &handshakeRe
 
     }
 
-    if (!tunnelOnError(HttpRequestMethod(), errCategory))
+    if (!tunnelOnError(errCategory))
         clientConnection->close();
 }
 
@@ -3148,7 +3147,7 @@ ConnStateData::httpsPeeked(PinnedIdleContext pic)
 #endif /* USE_OPENSSL */
 
 bool
-ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, Http::MethodType const method, const char *reason, const SBuf &payload)
+ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, const char *reason, const SBuf &payload)
 {
     // fake a CONNECT request to force connState to tunnel
     SBuf connectHost;
@@ -3182,7 +3181,7 @@ ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, Http::
     }
 
     debugs(33, 2, "Request tunneling for " << reason);
-    ClientHttpRequest *http = buildFakeRequest(method, connectHost, connectPort, payload);
+    ClientHttpRequest *http = buildFakeRequest(connectHost, connectPort, payload);
     HttpRequest::Pointer request = http->request;
     request->flags.forceTunnel = true;
     http->calloutContext = new ClientRequestContext(http);
@@ -3211,7 +3210,7 @@ ConnStateData::fakeAConnectRequest(const char *reason, const SBuf &payload)
         connectHost.assign(ip);
     }
 
-    ClientHttpRequest *http = buildFakeRequest(Http::METHOD_CONNECT, connectHost, connectPort, payload);
+    ClientHttpRequest *http = buildFakeRequest(connectHost, connectPort, payload);
 
     http->calloutContext = new ClientRequestContext(http);
     HttpRequest::Pointer request = http->request;
@@ -3221,7 +3220,7 @@ ConnStateData::fakeAConnectRequest(const char *reason, const SBuf &payload)
 }
 
 ClientHttpRequest *
-ConnStateData::buildFakeRequest(Http::MethodType const method, SBuf &useHost, unsigned short usePort, const SBuf &payload)
+ConnStateData::buildFakeRequest(SBuf &useHost, unsigned short usePort, const SBuf &payload)
 {
     ClientHttpRequest *http = new ClientHttpRequest(this);
     Http::Stream *stream = new Http::Stream(clientConnection, http);
@@ -3246,9 +3245,8 @@ ConnStateData::buildFakeRequest(Http::MethodType const method, SBuf &useHost, un
     // Setup Http::Request object. Maybe should be replaced by a call to (modified)
     // clientProcessRequest
     HttpRequest::Pointer request = new HttpRequest(mx);
-    AnyP::ProtocolType proto = (method == Http::METHOD_NONE) ? AnyP::PROTO_AUTHORITY_FORM : AnyP::PROTO_HTTP;
-    request->url.setScheme(proto, nullptr);
-    request->method = method;
+    request->url.setScheme(AnyP::PROTO_AUTHORITY_FORM, nullptr);
+    request->method = Http::METHOD_CONNECT;
     request->url.host(useHost.c_str());
     request->url.port(usePort);
 
@@ -3257,8 +3255,7 @@ ConnStateData::buildFakeRequest(Http::MethodType const method, SBuf &useHost, un
 
     request->manager(this, http->al);
 
-    if (proto == AnyP::PROTO_HTTP)
-        request->header.putStr(Http::HOST, useHost.c_str());
+    request->header.putStr(Http::HOST, useHost.c_str());
 
     request->sources |= ((switchedToHttps() || port->transport.protocol == AnyP::PROTO_HTTPS) ? Http::Message::srcHttps : Http::Message::srcHttp);
 #if USE_AUTH

diff --git a/src/client_side.h b/src/client_side.h
index 891fbd20694fbe5c5e625fa4f49108fcb1cfd8c7..1c4d040c09b5caa71f241868e7acc3d4b773976d 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -335,14 +335,14 @@ public:
     bool fakeAConnectRequest(const char *reason, const SBuf &payload);
 
     /// generates and sends to tunnel.cc a fake request with a given payload
-    bool initiateTunneledRequest(HttpRequest::Pointer const &cause, Http::MethodType const method, const char *reason, const SBuf &payload);
+    bool initiateTunneledRequest(HttpRequest::Pointer const &cause, const char *reason, const SBuf &payload);
 
     /// whether we should start saving inBuf client bytes in anticipation of
     /// tunneling them to the server later (on_unsupported_protocol)
     bool shouldPreserveClientData() const;
 
     /// build a fake http request
-    ClientHttpRequest *buildFakeRequest(Http::MethodType const method, SBuf &useHost, unsigned short usePort, const SBuf &payload);
+    ClientHttpRequest *buildFakeRequest(SBuf &useHost, unsigned short usePort, const SBuf &payload);
 
     /// From-client handshake bytes (including bytes at the beginning of a
     /// CONNECT tunnel) which we may need to forward as-is if their syntax does
@@ -434,7 +434,7 @@ protected:
     /// whether preservedClientData is valid and should be kept up to date
     bool preservingClientData_ = false;
 
-    bool tunnelOnError(const HttpRequestMethod &, const err_type);
+    bool tunnelOnError(const err_type);
 
 private:
     /* ::Server API */

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 94edd1604f93d011ebd043be656a6de8a35c0cd4..f35c1cc117da41b35decf22b03960307127ed3fd 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -98,7 +98,7 @@ clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) :
  */
 void
 clientReplyContext::setReplyToError(
-    err_type err, Http::StatusCode status, const HttpRequestMethod& method, char const *uri,
+    err_type err, Http::StatusCode status, char const *uri,
     const ConnStateData *conn, HttpRequest *failedrequest, const char *unparsedrequest,
 #if USE_AUTH
     Auth::UserRequest::Pointer auth_user_request
@@ -115,7 +115,7 @@ clientReplyContext::setReplyToError(
 #if USE_AUTH
     errstate->auth_user_request = auth_user_request;
 #endif
-    setReplyToError(method, errstate);
+    setReplyToError(failedrequest ? failedrequest->method : HttpRequestMethod(Http::METHOD_NONE), errstate);
 }
 
 void clientReplyContext::setReplyToError(const HttpRequestMethod& method, ErrorState *errstate)

diff --git a/src/client_side_reply.h b/src/client_side_reply.h
index 8548b4b70266cad0988ccd0d5e42474ad1f662a9..9963d5c573beb0ada0d56fe653a423e1041285f6 100644 (file)
--- a/src/client_side_reply.h
+++ b/src/client_side_reply.h
@@ -43,7 +43,7 @@ public:
     /// replaces current response store entry with the given one
     void setReplyToStoreEntry(StoreEntry *e, const char *reason);
     /// builds error using clientBuildError() and calls setReplyToError() below
-    void setReplyToError(err_type, Http::StatusCode, const HttpRequestMethod&, char const *, const ConnStateData *, HttpRequest *, const char *,
+    void setReplyToError(err_type, Http::StatusCode, char const *, const ConnStateData *, HttpRequest *, const char *,
 #if USE_AUTH
                          Auth::UserRequest::Pointer);
 #else

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 09ae8ba42f26df2382929e3169f48fcc452a1c58..bbdd45ac64ca541cf7e53cf9b4cfbc0d4b754db6 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -563,7 +563,7 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B)
     clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
     assert (repContext);
     repContext->setReplyToError(ERR_CONFLICT_HOST, Http::scConflict,
-                                http->request->method, NULL,
+                                nullptr,
                                 http->getConn(),
                                 http->request,
                                 NULL,

diff --git a/src/redirect.cc b/src/redirect.cc
index fc6bfd2404b119dd1bbbfc525570fb0389cdaac8..d72384d9a370ba6466449eac78be2029875f9ecd 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -259,7 +259,7 @@ constructHelperQuery(const char *name, helper *hlp, HLPCB *replyHandler, ClientH
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
         assert (repContext);
         repContext->setReplyToError(ERR_GATEWAY_FAILURE, status,
-                                    http->request->method, NULL,
+                                    nullptr,
                                     http->getConn(),
                                     http->request,
                                     NULL,

diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc
index c672d226e6a91e8404500e189c087532675b318b..9b6e56f83cbbe64f50b43b9371a836199863e239 100644 (file)
--- a/src/servers/Http1Server.cc
+++ b/src/servers/Http1Server.cc
@@ -123,8 +123,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context)
         assert(http->log_uri);
 
         const char * requestErrorBytes = inBuf.c_str();
-        if (!tunnelOnError(parser_->method(), errPage)) {
-            setReplyError(context, request, parser_->method(), errPage, parser_->parseStatusCode, requestErrorBytes);
+        if (!tunnelOnError(errPage)) {
+            setReplyError(context, request, errPage, parser_->parseStatusCode, requestErrorBytes);
             // HttpRequest object not build yet, there is no reason to call
             // clientProcessRequestFinished method
         }
@@ -142,8 +142,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context)
         http->setLogUriToRawUri(http->uri, parser_->method());
 
         const char * requestErrorBytes = inBuf.c_str();
-        if (!tunnelOnError(parser_->method(), ERR_INVALID_URL)) {
-            setReplyError(context, request, parser_->method(), ERR_INVALID_URL, Http::scBadRequest, requestErrorBytes);
+        if (!tunnelOnError(ERR_INVALID_URL)) {
+            setReplyError(context, request, ERR_INVALID_URL, Http::scBadRequest, requestErrorBytes);
             // HttpRequest object not build yet, there is no reason to call
             // clientProcessRequestFinished method
         }
@@ -161,8 +161,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context)
         http->setLogUriToRawUri(http->uri, parser_->method());
 
         const char * requestErrorBytes = NULL; //HttpParserHdrBuf(parser_);
-        if (!tunnelOnError(parser_->method(), ERR_UNSUP_HTTPVERSION)) {
-            setReplyError(context, request, parser_->method(), ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, requestErrorBytes);
+        if (!tunnelOnError(ERR_UNSUP_HTTPVERSION)) {
+            setReplyError(context, request, ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, requestErrorBytes);
             clientProcessRequestFinished(this, request);
         }
         return false;
@@ -174,8 +174,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context)
         // setReplyToError() requires log_uri
         http->setLogUriToRawUri(http->uri, parser_->method());
         const char * requestErrorBytes = NULL; //HttpParserHdrBuf(parser_);
-        if (!tunnelOnError(parser_->method(), ERR_INVALID_REQ)) {
-            setReplyError(context, request, parser_->method(), ERR_INVALID_REQ, Http::scBadRequest, requestErrorBytes);
+        if (!tunnelOnError(ERR_INVALID_REQ)) {
+            setReplyError(context, request, ERR_INVALID_REQ, Http::scBadRequest, requestErrorBytes);
             clientProcessRequestFinished(this, request);
         }
         return false;
@@ -204,7 +204,7 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context)
 }
 
 void
-Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, const HttpRequestMethod& method, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes)
+Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes)
 {
     quitAfterError(request.getRaw());
     if (!context->connRegistered()) {
@@ -216,7 +216,7 @@ Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Poin
     clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
     assert (repContext);
 
-    repContext->setReplyToError(requestError, errStatusCode, method, context->http->uri, this, nullptr, requestErrorBytes, nullptr);
+    repContext->setReplyToError(requestError, errStatusCode, context->http->uri, this, nullptr, requestErrorBytes, nullptr);
 
     assert(context->http->out.offset == 0);
     context->pullData();
@@ -259,7 +259,7 @@ Http::One::Server::processParsedRequest(Http::StreamPointer &context)
             assert(http->log_uri);
             clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
             assert (repContext);
-            repContext->setReplyToError(ERR_INVALID_REQ, Http::scExpectationFailed, request->method, http->uri,
+            repContext->setReplyToError(ERR_INVALID_REQ, Http::scExpectationFailed, http->uri,
                                         this, request.getRaw(), nullptr, nullptr);
             assert(context->http->out.offset == 0);
             context->pullData();

diff --git a/src/servers/Http1Server.h b/src/servers/Http1Server.h
index a69751c2d8eab9778b0580dce6b5b89cf9cb84f7..303070794da5198e0ca687b28eca220e1ffe09e2 100644 (file)
--- a/src/servers/Http1Server.h
+++ b/src/servers/Http1Server.h
@@ -56,7 +56,7 @@ private:
     /// Return false if parsing is failed, true otherwise.
     bool buildHttpRequest(Http::StreamPointer &context);
 
-    void setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, const HttpRequestMethod& method, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes);
+    void setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes);
 
     Http1::RequestParserPointer parser_;
     HttpRequestMethod method_; ///< parsed HTTP method