From e18ca83a3b1646532ae332081f69122fd4e18ad8 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Wed, 7 Jun 2023 15:47:47 +0100 Subject: [PATCH] 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. --- CHANGES | 3 ++ bin/tests/system/statschannel/tests.sh | 61 ++++++++++++++++++++++++++ lib/isc/httpd.c | 54 +++++++++++++++-------- 3 files changed, 99 insertions(+), 19 deletions(-) diff --git a/CHANGES b/CHANGES index 03de55c173..a10dea3466 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +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 c7b643ad1e..219f72995f 100644 --- a/bin/tests/system/statschannel/tests.sh +++ b/bin/tests/system/statschannel/tests.sh @@ -392,6 +392,11 @@ Connection: close EOF lines=$(grep -c "^ 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 "^ 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 "^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); @@ -432,16 +438,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]; @@ -476,14 +484,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; @@ -773,7 +785,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; @@ -782,7 +794,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); @@ -905,6 +920,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; } @@ -1012,13 +1028,13 @@ 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) + { eresult = ISC_R_EOF; } /* - * Calling httpd_request() with region NULL restarts - * reading. + * Calling httpd_request() with region NULL restarts reading. */ httpd_request(handle, eresult, NULL, httpd);