If you request an upgrade to TLS on your initial request, upgrading with a body
might still make sense. Especially if the server would respond with a 401. But
also if the request can be public, but the response needs to be secured.
If we blindly ignore the upgrade as ‘doesn’t make sense’, the next request
wouldn’t use encryption… but if we upgraded to TLS after the request, the next
request… with the authentication headers could be sent encrypted.
The server can’t choose if that first request makes sense or not… It has to
decide if upgrading the connection during the request makes sense… and if that
is possible.
If upgrading the first request doesn’t make sense the client should use a
different request (Like options *, or HEAD /).
If the server denies the request at least some parts have already travelled the
network unencrypted. Returning an error or not upgrading will only make sure
more requests will travel unencrypted.
The response is not the only thing encrypted when upgrading… all future
requests are. Upgrade is a connection level request, sent via a request.
Bert
Sent from Mail for Windows 10
From: Yann Ylavic
Sent: vrijdag 11 december 2015 02:22
To: httpd-dev
Subject: Re: Upgrade Summary
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 :)
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?
