Compliance: Forward 1xx control messages to clients that support them.

Forward 1xx control messages to all HTTP/1.1 clients and to HTTP/1.0
clients that sent an Expect: 100-continue header unless the 1xx message
fails the optional http_reply_access check described below.

RFC 2616 requires clients to accept 1xx control messages, even if they
did not send Expect headers. However, 1xx control messages prohibited by
http_reply_access are ignored and not forwarded. This can be used to
protect broken HTTP/1.1 clients or naive HTTP/1.0 clients that
unknowingly forward 100-continue headers, for example. Only fast checks
are supported at this time.

The patch removes ignore_expect_100 squid.conf option and the
corresponding code because

  - the reasons to treat 100-continue specially have changed since we
    can now forward 1xx responses;

  - rejection of 100-continue request can still be done using a
    combination of the existing http_access and deny_info options;

  - hiding of 100-continue header from next hops can still be done using
    an existing request_header_access option;

  - 100 (Continue) responses can be hidden from clients using
    http_reply_access check described above.

We still respond with 417 Expectation Failed to requests with
expectations other than 100-continue.

Implementation notes:

We forward control messages one-at-a-time and stop processing the server
response while the 1xx message is being written to client, to avoid
server-driven DoS attacks with large number of 1xx messages.

1xx forwarding is done via async calls from HttpStateData to
ConnStateData/ClientSocketContext. The latter then calls back to notify
HttpStateData that the message was written out to client. If any one of
the two async messages is not fired, HttpStateData will get stuck unless
it is destroyed due to an external event/error. The code assumes such
event/error will always happen because when
ConnStateData/ClientSocketContext is gone, HttpStateData job should be
terminated. This requires more testing/thought, but should still be
better than not forwarding 1xx messages at all.

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index de1e5229dc689cc74c05f59b0409f3d19605b732..762900b1ca9cb36c01a2a0962c00b7a84af380c2 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -644,3 +644,15 @@ HttpRequest::getRangeOffsetLimit()
 
     return rangeOffsetLimit;
 }
+
+bool
+HttpRequest::canHandle1xx() const
+{
+    // old clients do not support 1xx unless they sent Expect: 100-continue
+    // (we reject all other HDR_EXPECT values so just check for HDR_EXPECT)
+    if (http_ver <= HttpVersion(1,0) && !header.has(HDR_EXPECT))
+        return false;
+
+    // others must support 1xx control messages
+    return true;
+}

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 4e78882aea3a418702b8eb2e3dd98b9ed13666a8..fa38bb0a721f26215de594842b8a3f4b168f32e6 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -41,6 +41,7 @@
 #if ICAP_CLIENT
 #include "adaptation/icap/History.h"
 #endif
+#include "base/CbcPointer.h"
 #include "client_side.h"
 #if USE_SQUID_EUI
 #include "eui/Eui48.h"
@@ -88,6 +89,9 @@ public:
     /* are responses to this request potentially cachable */
     bool cacheable() const;
 
+    /// whether the client is likely to be able to handle a 1xx reply
+    bool canHandle1xx() const;
+
     /* Now that we care what host contains it is better off being protected. */
     /* HACK: These two methods are only inline to get around Makefile dependancies */
     /*      caused by HttpRequest being used in places it really shouldn't.        */
@@ -248,6 +252,9 @@ public:
         cbdataReferenceDone(pinned_connection);
     }
 
+    /// client-side conn manager, if known; used for 1xx response forwarding
+    CbcPointer<ConnStateData> clientConnection;
+
     int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */
 
 private:

diff --git a/src/cf.data.pre b/src/cf.data.pre
index b2b5a9770a7cdfafb759dbd34f0d01d3c67ba0d9..8a10aacaa96d682cd1ae1220da2b4f8a794ba002 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -4165,21 +4165,6 @@ DOC_START
        or response to be rejected.
 DOC_END
 
-NAME: ignore_expect_100
-COMMENT: on|off
-IFDEF: USE_HTTP_VIOLATIONS
-TYPE: onoff
-LOC: Config.onoff.ignore_expect_100
-DEFAULT: off
-DOC_START
-       This option makes Squid ignore any Expect: 100-continue header present
-       in the request. RFC 2616 requires that Squid being unable to satisfy
-       the response expectation MUST return a 417 error.
-
-       Note: Enabling this is a HTTP protocol violation, but some clients may
-       not handle it well..
-DOC_END
-
 COMMENT_START
  TIMEOUTS
  -----------------------------------------------------------------------------

diff --git a/src/client_side.cc b/src/client_side.cc
index 33f70e3a14d36eb92ccc56ddd85b999080b17893..49cbf7c5267aa1579f0b642f4357c9987b272afd 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -339,6 +339,59 @@ ClientSocketContextNew(ClientHttpRequest * http)
     return newContext;
 }
 
+void
+ClientSocketContext::writeControlMsg(HttpControlMsg &msg)
+{
+    HttpReply *rep = msg.reply;
+    Must(rep);
+
+    // apply selected clientReplyContext::buildReplyHeader() mods
+    // it is not clear what headers are required for control messages
+    rep->header.removeHopByHopEntries();
+    rep->header.putStr(HDR_CONNECTION, "keep-alive");
+    httpHdrMangleList(&rep->header, http->request, ROR_REPLY);
+
+    // remember the callback
+    cbControlMsgSent = msg.cbSuccess;
+
+    MemBuf *mb = rep->pack();
+
+    AsyncCall::Pointer call = commCbCall(33, 5, "ClientSocketContext::wroteControlMsg",
+                                         CommIoCbPtrFun(&WroteControlMsg, this));
+    comm_write_mbuf(fd(), mb, call);
+
+    delete mb;
+}
+
+/// called when we wrote the 1xx response
+void
+ClientSocketContext::wroteControlMsg(int fd, char *, size_t, comm_err_t errflag, int xerrno)
+{
+    if (errflag == COMM_ERR_CLOSING)
+        return;
+
+    if (errflag == COMM_OK) {
+        ScheduleCallHere(cbControlMsgSent);
+        return;
+    }
+
+    debugs(33, 3, HERE << "1xx writing failed: " << xstrerr(xerrno));
+    // no error notification: see HttpControlMsg.h for rationale and
+    // note that some errors are detected elsewhere (e.g., close handler)
+
+    // close on 1xx errors to be conservative and to simplify the code
+    // (if we do not close, we must notify the source of a failure!)
+    comm_close(fd);
+}
+
+/// wroteControlMsg() wrapper: ClientSocketContext is not an AsyncJob
+void
+ClientSocketContext::WroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno, void *data)
+{
+    ClientSocketContext *context = static_cast<ClientSocketContext*>(data);
+    context->wroteControlMsg(fd, bufnotused, size, errflag, xerrno);
+}
+
 #if USE_IDENT
 static void
 clientIdentDone(const char *ident, void *data)
@@ -2467,16 +2520,9 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
     }
 
     if (request->header.has(HDR_EXPECT)) {
-        int ignore = 0;
-#if USE_HTTP_VIOLATIONS
-        if (Config.onoff.ignore_expect_100) {
-            String expect = request->header.getList(HDR_EXPECT);
-            if (expect.caseCmp("100-continue") == 0)
-                ignore = 1;
-            expect.clean();
-        }
-#endif
-        if (!ignore) {
+        const String expect = request->header.getList(HDR_EXPECT);
+        const bool supportedExpect = (expect.caseCmp("100-continue") == 0);
+        if (!supportedExpect) {
             clientStreamNode *node = context->getClientReplyContext();
             clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
             assert (repContext);
@@ -3761,6 +3807,24 @@ ConnStateData::In::~In()
     delete bodyParser; // TODO: pool
 }
 
+void
+ConnStateData::sendControlMsg(HttpControlMsg msg)
+{
+    ClientSocketContext::Pointer context = getCurrentContext();
+    if (context != NULL) {
+        context->writeControlMsg(msg); // will call msg.cbSuccess
+        return;
+    }
+
+    if (!isOpen()) {
+        debugs(33, 3, HERE << "ignoring 1xx due to earlier closure");
+        return;
+    }
+
+    debugs(33, 3, HERE << " closing due to missing context for 1xx");
+    comm_close(fd);
+}
+
 /* This is a comm call normally scheduled by comm_close() */
 void
 ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)

diff --git a/src/client_side.h b/src/client_side.h
index 4220f1cfca82d72ec9ddf1b06f8a1540f90231a5..6f932f35dec99e87ae54700fd3fc7256ac280f78 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -40,6 +40,7 @@
 #include "CommCalls.h"
 #include "eui/Eui48.h"
 #include "eui/Eui64.h"
+#include "HttpControlMsg.h"
 #include "RefCount.h"
 #include "StoreIOBuffer.h"
 
@@ -112,6 +113,13 @@ public:
     void registerWithConn();
     void noteIoError(const int xerrno); ///< update state to reflect I/O error
 
+    /// starts writing 1xx control message to the client
+    void writeControlMsg(HttpControlMsg &msg);
+
+protected:
+    static void WroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno, void *data);
+    void wroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno);
+
 private:
     CBDATA_CLASS(ClientSocketContext);
     void prepareReply(HttpReply * rep);
@@ -120,6 +128,9 @@ private:
     void deRegisterWithConn();
     void doClose();
     void initiateClose(const char *reason);
+
+    AsyncCall::Pointer cbControlMsgSent; ///< notifies HttpControlMsg Source
+
     bool mayUseConnection_; /* This request may use the connection. Don't read anymore requests for now */
     bool connRegistered_;
 };
@@ -128,7 +139,7 @@ private:
 class ConnectionDetail;
 
 /** A connection to a socket */
-class ConnStateData : public BodyProducer/*, public RefCountable*/
+class ConnStateData : public BodyProducer, public HttpControlMsgSink
 {
 
 public:
@@ -149,6 +160,9 @@ public:
     bool isOpen() const;
     void checkHeaderLimits();
 
+    // HttpControlMsgSink API
+    virtual void sendControlMsg(HttpControlMsg msg);
+
     int fd;
 
     struct In {

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 5c79db6d425b2881dac0b2b50d28877d559bafed..19763b7076bb3fbd6264bac231f98455a98ed269 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -261,6 +261,8 @@ clientReplyContext::processExpired()
     http->storeEntry(entry);
     assert(http->out.offset == 0);
 
+    http->request->clientConnection = http->getConn();
+
     /*
      * A refcounted pointer so that FwdState stays around as long as
      * this clientReplyContext does
@@ -680,6 +682,8 @@ clientReplyContext::processMiss()
         if (http->flags.internal)
             r->protocol = PROTO_INTERNAL;
 
+        r->clientConnection = http->getConn();
+
         /** Start forwarding to get the new object from network */
         FwdState::fwdStart(http->getConn() != NULL ? http->getConn()->fd : -1,
                            http->storeEntry(),

diff --git a/src/http.cc b/src/http.cc
index 8a29fc8e779fac05510ad22916985a4310aee0a7..c07aba8c076a305d7089190c08e7208760fe5ac6 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -42,6 +42,7 @@
 
 #include "acl/FilledChecklist.h"
 #include "auth/UserRequest.h"
+#include "base/AsyncJobCalls.h"
 #include "base/TextException.h"
 #if DELAY_POOLS
 #include "DelayPools.h"
@@ -49,6 +50,7 @@
 #include "errorpage.h"
 #include "fde.h"
 #include "http.h"
+#include "HttpControlMsg.h"
 #include "HttpHdrContRange.h"
 #include "HttpHdrSc.h"
 #include "HttpHdrScTarget.h"
@@ -705,23 +707,9 @@ HttpStateData::processReplyHeader()
         readBuf->consume(header_bytes_read);
     }
 
-    /* Skip 1xx messages for now. Advertised in Via as an internal 1.0 hop */
     if (newrep->sline.protocol == PROTO_HTTP && newrep->sline.status >= 100 && newrep->sline.status < 200) {
-
-#if WHEN_HTTP11_EXPECT_HANDLED
-        /* When HTTP/1.1 check if the client is expecting a 1xx reply and maybe pass it on */
-        if (orig_request->header.has(HDR_EXPECT)) {
-            // TODO: pass to the client anyway?
-        }
-#endif
-        delete newrep;
-        debugs(11, 2, HERE << "1xx headers consume " << header_bytes_read << " bytes header.");
-        header_bytes_read = 0;
-        if (reply_bytes_read > 0)
-            debugs(11, 2, HERE << "1xx headers consume " << reply_bytes_read << " bytes reply.");
-        reply_bytes_read = 0;
+        handle1xx(newrep);
         ctx_exit(ctx);
-        processReplyHeader();
         return;
     }
 
@@ -752,6 +740,64 @@ HttpStateData::processReplyHeader()
     ctx_exit(ctx);
 }
 
+/// ignore or start forwarding the 1xx response (a.k.a., control message)
+void
+HttpStateData::handle1xx(HttpReply *reply)
+{
+    HttpMsgPointerT<HttpReply> msg(reply); // will destroy reply if unused
+
+    // one 1xx at a time: we must not be called while waiting for previous 1xx
+    Must(!flags.handling1xx);
+    flags.handling1xx = true;
+
+    if (!orig_request->canHandle1xx()) {
+        debugs(11, 2, HERE << "ignoring client-unsupported 1xx");
+        proceedAfter1xx();
+        return;
+    }
+
+#if USE_HTTP_VIOLATIONS
+    // check whether the 1xx response forwarding is allowed by squid.conf
+    if (Config.accessList.reply) {
+        ACLFilledChecklist ch(Config.accessList.reply, request, NULL);
+        ch.reply = HTTPMSGLOCK(reply);
+        if (!ch.fastCheck()) { // TODO: support slow lookups?
+            debugs(11, 3, HERE << "ignoring denied 1xx");
+            proceedAfter1xx();
+            return;
+               }
+    }
+#endif // USE_HTTP_VIOLATIONS
+
+    debugs(11, 2, HERE << "forwarding 1xx to client");
+
+    // the Sink will use this to call us back after writing 1xx to the client
+    typedef NullaryMemFunT<HttpStateData> CbDialer;
+    const AsyncCall::Pointer cb = JobCallback(11, 3, CbDialer, this,
+                                              HttpStateData::proceedAfter1xx);
+    CallJobHere1(11, 4, orig_request->clientConnection, ConnStateData,
+                 ConnStateData::sendControlMsg, HttpControlMsg(msg, cb));
+    // If the call is not fired, then the Sink is gone, and HttpStateData
+    // will terminate due to an aborted store entry or another similar error.
+    // If we get stuck, it is not handle1xx fault if we could get stuck
+    // for similar reasons without a 1xx response.
+}
+
+/// restores state and resumes processing after 1xx is ignored or forwarded
+void
+HttpStateData::proceedAfter1xx()
+{
+    Must(flags.handling1xx);
+
+    debugs(11, 2, HERE << "consuming " << header_bytes_read <<
+        " header and " << reply_bytes_read << " body bytes read after 1xx");
+    header_bytes_read = 0;
+    reply_bytes_read = 0;
+
+    CallJobHere(11, 3, this, HttpStateData, HttpStateData::processReply);
+}
+
+
 /**
  * returns true if the peer can support connection pinning
 */
@@ -1132,6 +1178,20 @@ HttpStateData::readReply(const CommIoCbParams &io)
         }
     }
 
+    processReply();
+}
+
+/// processes the already read and buffered response data, possibly after
+/// waiting for asynchronous 1xx control message processing
+void
+HttpStateData::processReply() {
+
+    if (flags.handling1xx) { // we came back after handling a 1xx response
+        debugs(11, 5, HERE << "done with 1xx handling");
+        flags.handling1xx = false;
+        Must(!flags.headers_parsed);
+    }
+
     if (!flags.headers_parsed) { // have not parsed headers yet?
         PROF_start(HttpStateData_processReplyHeader);
         processReplyHeader();
@@ -1155,6 +1215,12 @@ HttpStateData::readReply(const CommIoCbParams &io)
 bool
 HttpStateData::continueAfterParsingHeader()
 {
+    if (flags.handling1xx) {
+        debugs(11, 5, HERE << "wait for 1xx handling");
+        Must(!flags.headers_parsed);
+        return false;
+    }
+
     if (!flags.headers_parsed && !eof) {
         debugs(11, 9, HERE << "needs more at " << readBuf->contentSize());
         flags.do_next_read = 1;

diff --git a/src/http.h b/src/http.h
index b9a0af5ca2d6fd128e09f6e4ed66e43da0ef6855..b53092a3ba306d997bba259ef8cd1569146730b0 100644 (file)
--- a/src/http.h
+++ b/src/http.h
@@ -81,6 +81,10 @@ public:
 protected:
     virtual HttpRequest *originalRequest();
 
+    void processReply();
+    void proceedAfter1xx();
+    void handle1xx(HttpReply *msg);
+
 private:
     AsyncCall::Pointer closeHandler;
     enum ConnectionStatus {

diff --git a/src/structs.h b/src/structs.h
index 4612f7fcb6ca09ee1d1e3ab1148bf0c08048448a..5298668c30f1a0a9e0bb30cbbf7ac9f4b0da5234 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -393,7 +393,6 @@ struct SquidConfig {
 #if USE_HTTP_VIOLATIONS
 
         int reload_into_ims;
-        int ignore_expect_100;
 #endif
 
         int offline;
@@ -760,6 +759,7 @@ struct _http_state_flags {
     unsigned int proxying:1;
     unsigned int keepalive:1;
     unsigned int only_if_cached:1;
+    unsigned int handling1xx:1; ///< we are ignoring or forwarding 1xx response
     unsigned int headers_parsed:1;
     unsigned int front_end_https:2;
     unsigned int originpeer:1;