Hi folks, sorry for the late interruption after we have already shipped
2.4.16, but there seems to be an issue that merits revisiting before the
2.4.16 API schema is widely adopted.
We seem to have misplaced the upgrade handler in the wrong hook. This is
easily shown by the fact that we had two different upgrade paths between
mod_ssl TLS upgrade, and h2c upgrade paths, and a crufty patch to solve the
OPTIONS * request exception which isn't an exception at all per RFC7230.
In mod_ssl connection upgrade we once emitted the TLS upgrade header in
fixups. Fixups are bypassed by OPTIONS *. My trunk patch moved this
mod_ssl TLS upgrade handling to post read req, leaving OPTIONS * with its
simple behavior.
In h2c this exists in an upgrade handler, and OPTIONS * was hacked to
invoke this function. Problem, it is a transport (hop by hop) detail and
should not be presented in the request, but rather in the connection
hooks. Moreover, none of the other *request* processing phases appear
valid prior to conducting the h2c upgrade, and some appear that they can be
harmful to the admin attempting to configure Protocol h2c.
The relevant specs are clear, it is a hop-by-hop connection transport offer
carried over the request headers. It is not generally subject to auth
processing, certainly not resource processing. The underlying request must
still be satisfied. The current Protocols logic doesn't appear to do this
because it was based off of tangential specs such as h2c that don't
override 2616bis. If we consider that a "method" has a "handler", and
transfer "headers" are "filtered" it becomes more obvious. This is not the
UPGRADE method.
https://tools.ietf.org/html/rfc7230#section-6.7 makes things more
interesting, it calls out that 101-continue and the request body read
precedes the 101-switching protocols. Not sure who decided that would be a
good idea, sigh... but mod_ssl TLS upgrade has these reversed for several
good reasons including the intent to encrypt the request body if present
and simple economics of processing.
I think that handling upgrade advertisement and alerting must be in post
read req, bypassing all request hooks until the 101-continue is presented,
any small request body read and set aside for the http input brigade, and
101-switching protocols is presented. This allows the request to still be
processed for tls-style upgrades, or discarded for relevant protocols.
The beginning of a patch is attached, I note that the ap_switch_protocol
return value was never properly interpreted and my patch doesn't begin to
catch all of the exceptional cases. Because 101-switching protocols has
already been sent, if the call fails to perform an upgrade, it seems we
need to respond with a generic 400 switch failed unless the registered
protocol switch handler is going to ap_die with a different sort of
specific error (e.g. "A server MUST NOT switch protocols unless the
received message semantics can be honored by the new protocol; an OPTIONS
request can be honored by any protocol.")
Stefan and other protocol gurus, please have a look at the proposed patch,
thanks.
Bill
Index: server/core.c
===================================================================
--- server/core.c (revision 1718419)
+++ server/core.c (working copy)
@@ -5346,7 +5346,7 @@
}
}
-static int core_upgrade_handler(request_rec *r)
+static int core_upgrade_request(request_rec *r)
{
conn_rec *c = r->connection;
const char *upgrade;
@@ -5374,8 +5374,15 @@
if (offers && offers->nelts > 0) {
const char *protocol = ap_select_protocol(c, r, NULL, offers);
if (protocol && strcmp(protocol, ap_get_protocol(c))) {
+ apr_status_t rv;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02909)
"Upgrade selects '%s'", protocol);
+ /* RFC7230 6.7: Stupid constraint, detect request body,
+ * send 100-continue, read it immediately, and set it aside
+ * for the filters/handler to process after upgrade
+ * XXX: TODO
+ */
+
/* Let the client know what we are upgrading to. */
apr_table_clear(r->headers_out);
apr_table_setn(r->headers_out, "Upgrade", protocol);
@@ -5385,11 +5392,13 @@
r->status_line = ap_get_status_line(r->status);
ap_send_interim_response(r, 1);
- ap_switch_protocol(c, r, r->server, protocol);
-
- /* make sure httpd closes the connection after this */
- c->keepalive = AP_CONN_CLOSE;
- return DONE;
+ rv = ap_switch_protocol(c, r, r->server, protocol);
+ if (rv == APR_EOF)
+ {
+ /* make sure httpd closes the connection after this */
+ c->keepalive = AP_CONN_CLOSE;
+ return DONE;
+ }
}
}
}
@@ -5407,16 +5416,7 @@
apr_table_setn(r->headers_out, "Connection", "Upgrade");
}
}
-
- return DECLINED;
-}
-static int core_upgrade_storage(request_rec *r)
-{
- if ((r->method_number == M_OPTIONS) && r->uri && (r->uri[0] == '*') &&
- (r->uri[1] == '\0')) {
- return core_upgrade_handler(r);
- }
return DECLINED;
}
@@ -5442,12 +5442,11 @@
ap_hook_check_config(core_check_config,NULL,NULL,APR_HOOK_FIRST);
ap_hook_test_config(core_dump_config,NULL,NULL,APR_HOOK_FIRST);
ap_hook_translate_name(ap_core_translate,NULL,NULL,APR_HOOK_REALLY_LAST);
- ap_hook_map_to_storage(core_upgrade_storage,NULL,NULL,APR_HOOK_REALLY_FIRST);
ap_hook_map_to_storage(core_map_to_storage,NULL,NULL,APR_HOOK_REALLY_LAST);
ap_hook_open_logs(ap_open_logs,NULL,NULL,APR_HOOK_REALLY_FIRST);
ap_hook_child_init(core_child_init,NULL,NULL,APR_HOOK_REALLY_FIRST);
ap_hook_child_init(ap_logs_child_init,NULL,NULL,APR_HOOK_MIDDLE);
- ap_hook_handler(core_upgrade_handler,NULL,NULL,APR_HOOK_REALLY_FIRST);
+ ap_hook_post_read_request(core_upgrade_request,NULL,NULL,APR_HOOK_LAST);
ap_hook_handler(default_handler,NULL,NULL,APR_HOOK_REALLY_LAST);
/* FIXME: I suspect we can eliminate the need for these do_nothings - Ben */
ap_hook_type_checker(do_nothing,NULL,NULL,APR_HOOK_REALLY_LAST);
