ComputerSurge/bind9
3
0
Fork 0
Code Pull Requests 1 Activity

Improve statschannel HTTP Connection: header protocol conformance

Browse Source 
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.
This commit is contained in:
Tony Finch
2023-06-07 15:47:47 +01:00
parent a0efcf0e58
commit e18ca83a3b
3 changed files with 99 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
CHANGES
View File

@@ -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]

61
bin/tests/system/statschannel/tests.sh
View File

@@ -392,6 +392,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
@@ -419,6 +424,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

54
lib/isc/httpd.c
View File

@@ -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)
@@ -95,8 +96,6 @@ struct isc_httpd {
isc_nmhandle_t *handle; /* Permanent pointer to handle */
int flags;
/*%
* Received data state.
*/
@@ -106,6 +105,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;
@@ -415,8 +415,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);
@@ -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);