Improve statschannel HTTP Connection: header protocol conformance

In HTTP/1.0 and HTTP/1.1, RFC 9112 section 9.6 says the last response
in a connection should include a `Connection: close` header, but the
statschannel server omitted it.

In an HTTP/1.0 response, the statschannel server can sometimes send a
`Connection: keep-alive` header when it is about to close the
connection. There are two ways:

If the first request on a connection is keep-alive and the second
request is not, then _both_ responses have `Connection: keep-alive`
but the connection is (correctly) closed after the second response.

If a single request contains

	Connection: close
	Connection: keep-alive

then RFC 9112 section 9.3 says the keep-alive header is ignored, but
the statschannel sends a spurious keep-alive in its response, though
it correctly closes the connection.

To fix these bugs, make it more clear that the `httpd->flags` are part
of the per-request-response state. The Connection: flags are now
described in terms of the effect they have instead of what causes them
to be set.

(manually picked from commit e18ca83a3b1646532ae332081f69122fd4e18ad8)

diff --git a/CHANGES b/CHANGES
index ce55e61dc2fa9c789e786c047bce238df0969ed7..98d3b8036210cfbb1cebfae2f2a0a8886d4047b4 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -7,10 +7,12 @@
                        at non-referral nodes to be cached in addition to the
                        referrals that are normally cached. [GL #3325]
 
-
 6200.  [bug]           Fix nslookup erroneously reporting a timeout when the
                        input is delayed. [GL #4044]
 
+6199.  [bug]           Improve HTTP Connection: header protocol conformance
+                       in the statistics channel. [GL #4126]
+
 6198.  [func]          Remove the holes in the isc_result_t enum to compact
                        the isc_result tables. [GL #4149]
 

diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh
index 7c871db27f965e946d867ef852f05be5f9f5b12d..6ce63923c10afc5956f56c013e5fe8f553808815 100644 (file)
--- a/bin/tests/system/statschannel/tests.sh
+++ b/bin/tests/system/statschannel/tests.sh
@@ -394,6 +394,11 @@ Connection: close
 EOF
     lines=$(grep -c "^<statistics version" nc.out$n)
     test "$lines" = 2 || ret=1
+    # keep-alive not needed in HTTP/1.1, second response has close
+    lines=$(grep -c "^Connection: Keep-Alive" nc.out$n)
+    test "$lines" = 0 || ret=1
+    lines=$(grep -c "^Connection: close" nc.out$n)
+    test "$lines" = 1 || ret=1
 else
     echo_i "skipping test as nc not found"
 fi
@@ -421,6 +426,62 @@ Connection: close
 EOF
     lines=$(grep -c "^<statistics version" nc.out$n)
     test "$lines" = 2 || ret=1
+    # keep-alive not needed in HTTP/1.1, second response has close
+    lines=$(grep -c "^Connection: Keep-Alive" nc.out$n)
+    test "$lines" = 0 || ret=1
+    lines=$(grep -c "^Connection: close" nc.out$n)
+    test "$lines" = 1 || ret=1
+else
+    echo_i "skipping test as nc not found"
+fi
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+n=$((n + 1))
+
+echo_i "Check HTTP/1.0 keep-alive ($n)"
+ret=0
+if [ -x "${NC}" ]; then
+    "${NC}" 10.53.0.3 "${EXTRAPORT1}" << EOF > nc.out$n || ret=1
+GET /xml/v3/status HTTP/1.0
+Connection: keep-alive
+
+GET /xml/v3/status HTTP/1.0
+
+EOF
+    # should be two responses
+    lines=$(grep -c "^<statistics version" nc.out$n)
+    test "$lines" = 2 || ret=1
+    # first response has keep-alive, second has close
+    lines=$(grep -c "^Connection: Keep-Alive" nc.out$n)
+    test "$lines" = 1 || ret=1
+    lines=$(grep -c "^Connection: close" nc.out$n)
+    test "$lines" = 1 || ret=1
+else
+    echo_i "skipping test as nc not found"
+fi
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+n=$((n + 1))
+
+echo_i "Check inconsistent Connection: headers ($n)"
+ret=0
+if [ -x "${NC}" ]; then
+    "${NC}" 10.53.0.3 "${EXTRAPORT1}" << EOF > nc.out$n || ret=1
+GET /xml/v3/status HTTP/1.0
+Connection: keep-alive
+Connection: close
+
+GET /xml/v3/status HTTP/1.0
+
+EOF
+    # should be one response (second is ignored)
+    lines=$(grep -c "^<statistics version" nc.out$n)
+    test "$lines" = 1 || ret=1
+    # no keep-alive, one close
+    lines=$(grep -c "^Connection: Keep-Alive" nc.out$n)
+    test "$lines" = 0 || ret=1
+    lines=$(grep -c "^Connection: close" nc.out$n)
+    test "$lines" = 1 || ret=1
 else
     echo_i "skipping test as nc not found"
 fi

diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c
index 448e6cad554c950ee137eb3e135f17987e550fdb..4e3ab595238c066c3e4f3bc3778751af1a1a696b 100644 (file)
--- a/lib/isc/httpd.c
+++ b/lib/isc/httpd.c
@@ -54,10 +54,11 @@
 #define HTTP_HEADERS_NUM     100
 #define HTTP_MAX_REQUEST_LEN 4096
 
-#define HTTPD_CLOSE         0x0001 /* Got a Connection: close header */
-#define HTTPD_FOUNDHOST             0x0002 /* Got a Host: header */
-#define HTTPD_KEEPALIVE             0x0004 /* Got a Connection: Keep-Alive */
-#define HTTPD_ACCEPT_DEFLATE 0x0008
+typedef enum httpd_flags {
+       CONNECTION_CLOSE = 1 << 0,      /* connection must close */
+       CONNECTION_KEEP_ALIVE = 1 << 1, /* response needs a keep-alive header */
+       ACCEPT_DEFLATE = 1 << 2,        /* response can be compressed */
+} httpd_flags_t;
 
 #define HTTPD_MAGIC    ISC_MAGIC('H', 't', 'p', 'd')
 #define VALID_HTTPD(m) ISC_MAGIC_VALID(m, HTTPD_MAGIC)
@@ -96,8 +97,6 @@ struct isc_httpd {
        isc_nmhandle_t *handle;     /* Permanent pointer to handle */
        isc_nmhandle_t *readhandle; /* Waiting for a read callback */
 
-       int flags;
-
        /*%
         * Received data state.
         */
@@ -107,6 +106,7 @@ struct isc_httpd {
 
        method_t method;
        int minor_version;
+       httpd_flags_t flags;
        const char *path;
        isc_url_parser_t up;
        isc_time_t if_modified_since;
@@ -422,8 +422,14 @@ process_request(isc_httpd_t *httpd, size_t last_len) {
        }
        httpd->path = path;
 
+       /*
+        * Examine headers that can affect this request's response
+        */
+       httpd->flags = 0;
+
        ssize_t content_len = 0;
        bool keep_alive = false;
+       bool host_header = false;
 
        isc_time_set(&httpd->if_modified_since, 0, 0);
 
@@ -439,16 +445,18 @@ process_request(isc_httpd_t *httpd, size_t last_len) {
                                return (ISC_R_RANGE);
                        }
                } else if (name_match(header, "Connection")) {
+                       /* multiple fields in a connection header are allowed */
                        if (value_match(header, "close")) {
-                               httpd->flags |= HTTPD_CLOSE;
-                       } else if (value_match(header, "keep-alive")) {
+                               httpd->flags |= CONNECTION_CLOSE;
+                       }
+                       if (value_match(header, "keep-alive")) {
                                keep_alive = true;
                        }
                } else if (name_match(header, "Host")) {
-                       httpd->flags |= HTTPD_FOUNDHOST;
+                       host_header = true;
                } else if (name_match(header, "Accept-Encoding")) {
                        if (value_match(header, "deflate")) {
-                               httpd->flags |= HTTPD_ACCEPT_DEFLATE;
+                               httpd->flags |= ACCEPT_DEFLATE;
                        }
                } else if (name_match(header, "If-Modified-Since")) {
                        char timestamp[ISC_FORMATHTTPTIMESTAMP_SIZE + 1];
@@ -483,14 +491,18 @@ process_request(isc_httpd_t *httpd, size_t last_len) {
 
        switch (httpd->minor_version) {
        case 0:
-               if (keep_alive == true) {
-                       httpd->flags |= HTTPD_KEEPALIVE;
+               /*
+                * RFC 9112 section 9.3 says close takes priority if
+                * keep-alive is also present
+                */
+               if ((httpd->flags & CONNECTION_CLOSE) == 0 && keep_alive) {
+                       httpd->flags |= CONNECTION_KEEP_ALIVE;
                } else {
-                       httpd->flags |= HTTPD_CLOSE;
+                       httpd->flags |= CONNECTION_CLOSE;
                }
                break;
        case 1:
-               if ((httpd->flags & HTTPD_FOUNDHOST) == 0) {
+               if (!host_header) {
                        return (ISC_R_RANGE);
                }
                break;
@@ -804,7 +816,7 @@ prepare_response(isc_httpdmgr_t *mgr, isc_httpd_t *httpd,
        }
 
 #ifdef HAVE_ZLIB
-       if ((httpd->flags & HTTPD_ACCEPT_DEFLATE) != 0) {
+       if ((httpd->flags & ACCEPT_DEFLATE) != 0) {
                result = httpd_compress(req);
                if (result == ISC_R_SUCCESS) {
                        is_compressed = true;
@@ -813,7 +825,10 @@ prepare_response(isc_httpdmgr_t *mgr, isc_httpd_t *httpd,
 #endif /* ifdef HAVE_ZLIB */
 
        httpd_response(httpd, req);
-       if ((httpd->flags & HTTPD_KEEPALIVE) != 0) {
+       /* RFC 9112 § 9.6: SHOULD send Connection: close in last response */
+       if ((httpd->flags & CONNECTION_CLOSE) != 0) {
+               httpd_addheader(req, "Connection", "close");
+       } else if ((httpd->flags & CONNECTION_KEEP_ALIVE) != 0) {
                httpd_addheader(req, "Connection", "Keep-Alive");
        }
        httpd_addheader(req, "Content-Type", req->mimetype);
@@ -943,6 +958,7 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
                return;
        }
 
+       /* XXXFANF it would be more polite to reply 400 bad request */
        if (result != ISC_R_SUCCESS) {
                goto close_readhandle;
        }
@@ -1045,7 +1061,8 @@ httpd_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) {
                goto detach;
        }
 
-       if (eresult == ISC_R_SUCCESS && (httpd->flags & HTTPD_CLOSE) == 0) {
+       if (eresult == ISC_R_SUCCESS && (httpd->flags & CONNECTION_CLOSE) == 0)
+       {
                /*
                 * Calling httpd_request() with region NULL restarts
                 * reading.