> Am 11.12.2015 um 02:22 schrieb Yann Ylavic <ylavic....@gmail.com>:
>
> On Thu, Dec 10, 2015 at 11:46 AM, Stefan Eissing
> <stefan.eiss...@greenbytes.de> wrote:
>> Given all the input on this thread, I arrive at the following pseudo code:
>
> Thanks for _compiling_ this thread, quite exhaustive :)
Code often says more than a thousand...tokens.
> 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,
I am not an expert on where TLS handshake would fit best. It would
be good, and we are on the way of working on it, to have a good
Upgrade handling in core, where TLS, h2c and websocket all are
comfortable with.
Currently, TLS and current core step on each others toes when it
comes to the Upgrade response header. This we should fix.
If it turns out that TLS, h2c and websocket all need the actual
switching at different phases of request processing, then we need
to accomodate for that.
What I can see:
- TLS and h2c should switch in post_read_req
- websocket should switch just before the handler phase
Regarding request bodies:
- websocket will never switch on request bodies
- h2c currently does not, but clients like serf (and curl) really would prefer
it to
- TLS could switch with arbitrary request bodies, but maybe need not to
Protocol implementations should make up their minds in the "propose" phase, I
think,
because once a protocol gets selected, the upgrade *should* succeed unless the
connection catches fire or something. Not upgrading is better than failing.
> 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?
I think the first way, don't Upgrade if you know it will not work, is better.
>> [...]
>> 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.
I am not convinced that a flushing is necessary. If a protocol needs
this, it can just pass out a flush bucket, or?
> 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.
Some safeguard+comfort filter magic to make protocol's life with
Upgrade request bodies easier would be nice.
h2c needs to receive request body separate, get an indication
that EOS for that body has been reached, and then continue reading
HTTP/2 protocol data on the connection. I am not sure what is the
best way to do that and, when upgrading on post_read_req hook, if
all necessary request filters are in place by then.
> For the headers in the 101 response, another hook called from
> switch_protocol_filter() could be used.
That'd be the pre_switch hook Jacob and I talked about and where he
posted his implementation yesterday.
>> [...]
>
> 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 :
Yes, maybe that is easier.
>
>> // 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?
I though the TLS wanted to switch protocol and then let normal HTTP/1
processing continue on the connection?
//Stefan
