httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: Upgrade Summary
Date Fri, 11 Dec 2015 01:22:34 GMT
On Thu, Dec 10, 2015 at 11:46 AM, Stefan Eissing
<stefan.eissing@greenbytes.de> wrote:
> Given all the input on this thread, I arrive at the following pseudo code:

Thanks for _compiling_ this thread, quite exhaustive :)

I wonder if we could let each Protocols module hook wherever
appropriate in the current hooking set.

TLS handshake is already at the right place IMO, it possibly needs a
simple fix like,

Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c    (revision 1718341)
+++ modules/ssl/ssl_engine_kernel.c    (working copy)
@@ -233,7 +233,8 @@ int ssl_hook_ReadReq(request_rec *r)
      * has sent a suitable Upgrade header. */
     if (sc->enabled == SSL_ENABLED_OPTIONAL && !myConnConfig(r->connection)
         && (upgrade = apr_table_get(r->headers_in, "Upgrade")) != NULL
-        && ap_find_token(r->pool, upgrade, "TLS/1.0")) {
+        && ap_find_token(r->pool, upgrade, "TLS")
+        && !ap_has_request_body(r)) {
         if (upgrade_connection(r)) {
             return AP_FILTER_ERROR;
         }
--
so that we don't Upgrade when a body is given (looks like it's RFC
compliant since Upgrade is not mandatory).

Or maybe,
-        && ap_find_token(r->pool, upgrade, "TLS/1.0")) {
+        && ap_find_token(r->pool, upgrade, "TLS")) {
+        if (ap_has_request_body(r)) {
+            return HTTP_REQUEST_ENTITY_TOO_LARGE;
+        }
         if (upgrade_connection(r)) {
             return AP_FILTER_ERROR;
         }
--
because a body in clear text while requiring TLS makes few sense, and
clients that send TLS body directly (no header) after the Upgrade
would notice (now), that looks safer.
But stricly speaking this looks not very RFC7230 compliant too, I
could not find an "Upgrade: TLS" exception there (the whole HTTP/1
request must be read otherwise before upgrade)...

Possibly we could also,
+            ap_discard_request_body(r);
but I'd feel safer with the previous patch, WDYT?


Regarding WebSocket, we already have proxy_wstunnel that handshakes
successfully as a (proxy_)handler.
I don't know if the upcoming body is to be HTTP/1 or not, but should
this be enforced we could use ap_get_brigade() to forward it until
complete (while still detecting HTTP/1 "errors"), and then use the
current TCP forwarding until EOF on either side.


>
> 1. Post Read Request Hook:
>
>         if (Upgrade: request header present) {
>                 collect protocol proposals;
>                 ps = protocol with highest preference from proposals;
>                 if (ps && ps != current) {

Here I would use ap_add_output_filter(switch_protocol_filter, r); with
switch_protocol_filter() which would flush out the 101 response (or
not) based on r->need_upgrade and r->current_protocol, before any
Protocols data.
Maybe this filter could also call ap_discard_request_body(r) when
needed/asked to (eg. r->discard_body), that'd need CONN_SENSE handling
with MPM event possibly.
For the headers in the 101 response, another hook called from
switch_protocol_filter() could be used.

If I'm talking about new r-> field, that could also be in
core_request_config, reachable with
ap_get_core_module_config(r->request_config), for possibly a better
backportabily (but that's an implementation detail).

So now I think we can let the request fall through.
If a Protocols' module needs to bypass/handle auth[nz] itself, it
would hook after this one (post_read_request still, and return DONE).
Otherwise, as a handler, each Protocols' module would check
r->current_protocol against its own one, and either handle it or
DECLINE.
If the selected module doesn't care about the request body (ie. never
reads it), switch_protocol_filter() would do the right thing (discard
the body) before switching.
Otherwise, the module can do whatever it wants with the (full) HTTP/1
request, and even DECLINE if it finally wants to fall through HTTP/1
(after resetting r->need_upgrade probably).

>
> 3. Common protocol switch hook behavior:
>
>         apr_status_t process_body(apr_bucket_brigade *bb, void *ctx)
>         {
>                 // brigade contains unchunked body data of request
>                 // may be called several times. EOS bucket in brigade
>                 // indicates end of body.
>                 // If request has no body, will be called once with EOS bucket.

A handler could use ap_get_brigade(r->input_filters) and friends more
easily, or ap_discard_request_body() explicitely, and/or
ap_pass_brigade(FLUSH) to force the 101 response, ..., and/or go to :

>
>                 // c->output_filters can be written
>                 // c->input_filters can be read [...]
>                 process protocol;

Followed by:
                  r->keepalive = AP_CONN_CLOSE;
                  return OK;
>         }

Wouldn't that work for every case we discussed?

Mime
View raw message