You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Eissing <st...@greenbytes.de> on 2015/12/08 11:07:24 UTC

Upgrade Summary

Trying to summarize the status of the discussion and where the issues are with the current Upgrade implementation.

Clarified:
A. any 100 must be sent out *before* a 101 response
B. request bodies are to be read in the original protocol, input filters like chunk can be used, indeed are necessary, as if the request is being processed normally
C. if a protocol supports upgrade on request bodies is up to the protocol implementation and needs to be checked in the "propose" phase

Open:
1. Protocols like Websocket need to take over the 101 sending themselves in the "switch protocol" phase. (correct, Jacob?). Should we delegate the sending of the 101 to the protocol switch handler?
2. General handling of request bodies. Options:
  a setaside in core of up to nnn bytes before switch invocation
  b do nothing, let protocol switch handler care about it
3. When to do the upgrade dance:
  a post_read_request: upgrade precedes authentication
  b handler: upgrade only honored on authenticated and otherwise ok requests
  c both: introduce separate hooks? have an additional parameter? more complexity
4. status code of protocol switch handler: if we move the task of 101 sending, the switch handler might not do it and keep the connection on the "old" protocol. Then a connection close is not necessary. So, we would do the close only when the switch handler returns APR_EOF.
5. Will it be possible to migrate the current TLS upgrade handling to this revised scheme?

Anything I missed?

PS. Re 5: with change 1+4, a TLS upgrade switcher could install an output filter, return and have it being processed normally. The output filter would then send the 101 and do the TLS handshake. Would that work?

> Am 08.12.2015 um 10:42 schrieb Stefan Eissing <st...@greenbytes.de>:
> 
> 
>> Am 07.12.2015 um 23:42 schrieb Jacob Champion <ch...@gmail.com>:
>> 
>> On 12/07/2015 02:30 PM, Bert Huijben wrote:
>>> Is this a h2 limitation or a mod_h2 limitation?
>>> 
>>> If I would like h2c upgrade from Subversion without an additional
>>> request I would have to send the upgrade request with a very short
>>> OPTIONS request that has a body.
>> 
>> The HTTP/2 spec (RFC 7540, sec 3.2) appears to allow the sending of a request body in an h2c upgrade (h2 upgrades are prohibited). So it would appear to be a mod_http2 limitation for now.
>> 
>> Hopefully Stefan can confirm tomorrow (it's pretty late in Germany).
> 
> It is possible, but difficult to do unless you can read the body and set it aside somewhere. Which again imposes an arbitrary (for the client) limit. Experience shows that this leads to design of clients that work will in developments, but then encounter requests usage/sites/servers that have other limits and break.
> 
> If Apache accepts body lengths of up to 64KB (or something configurable) and some other server implements 8K and some third 1MB, how will that help design of a client that, for some reason, *wants* an upgrade to succeed? 
> 
> //Stefan
> 


Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
Bert,

why can't you use h2c in direct mode? I assume you could store server support for this in the checkout meta data somewhere.

//Stefan

> Am 08.12.2015 um 11:30 schrieb Bert Huijben <be...@qqmail.nl>:
> 
>> -----Original Message-----
>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>> Sent: dinsdag 8 december 2015 11:07
>> To: dev@httpd.apache.org
>> Subject: Upgrade Summary
>> 
>> Trying to summarize the status of the discussion and where the issues are
>> with the current Upgrade implementation.
>> 
>> Clarified:
>> A. any 100 must be sent out *before* a 101 response
>> B. request bodies are to be read in the original protocol, input filters
> like
>> chunk can be used, indeed are necessary, as if the request is being
>> processed normally
>> C. if a protocol supports upgrade on request bodies is up to the protocol
>> implementation and needs to be checked in the "propose" phase
>> 
>> Open:
>> 1. Protocols like Websocket need to take over the 101 sending themselves
> in
>> the "switch protocol" phase. (correct, Jacob?). Should we delegate the
>> sending of the 101 to the protocol switch handler?
> 
> If possible I would recommend avoiding this. I think the original protocol
> should setup the response and then the final protocol should somehow be able
> to annotate this.
> 
> The problems we try to solve now originate from the problem of doing things
> different in different protocol handlers, while in theory many upgrades are
> very similar.
> 
> The TLS and H2C upgrades both begin in one form and end in a different form.
> Websockets are kind of different in that they require a bad request response
> in a specific case. I'm not sure in which protocol this error needs to be
> send though.
> 
> In TLS and H2C further errors can always be produced in the new protocol, so
> when the handshake succeeds things can just go on.
> 
>> 2. General handling of request bodies. Options:
>>  a setaside in core of up to nnn bytes before switch invocation
>>  b do nothing, let protocol switch handler care about it
> 
> For Subversion to be able to use upgrade we would need to support a small
> body on a request (few hundred bytes. Content-Length header provided).
> 
> During our current (already optimized) handshake all requests have bodies,
> and introducing an additional request just to upgrade will slow things down
> quite measurable on operation like 'svn log', that are mostly bound by the
> handshake time.
> 
> We can't just switch the handshake to be something else... our handshake was
> built upon WEBDAV and DELTA/V. We added several headers to avoid many
> requests we previously performed, but we can't move away from that initial
> OPTIONS request without slowing down against all older servers.
> 
> 	Bert


Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/08/2015 02:30 AM, Bert Huijben wrote:
> The TLS and H2C upgrades both begin in one form and end in a different form.
> Websockets are kind of different in that they require a bad request response
> in a specific case. I'm not sure in which protocol this error needs to be
> send though.

For WebSocket, any upgrade failures have to be sent in the original 
HTTP/1.1 protocol using HTTP error codes.

WebSocket is different from TLS/h2c because, conceptually, you're 
upgrading the connection to a specific request target (a ws:// URL). The 
"response" sent back to the client after the upgrade (if you want to 
call it a response) is simply the establishment of a WebSocket 
connection for that URL.

--Jacob

RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: dinsdag 8 december 2015 11:07
> To: dev@httpd.apache.org
> Subject: Upgrade Summary
> 
> Trying to summarize the status of the discussion and where the issues are
> with the current Upgrade implementation.
> 
> Clarified:
> A. any 100 must be sent out *before* a 101 response
> B. request bodies are to be read in the original protocol, input filters
like
> chunk can be used, indeed are necessary, as if the request is being
> processed normally
> C. if a protocol supports upgrade on request bodies is up to the protocol
> implementation and needs to be checked in the "propose" phase
> 
> Open:
> 1. Protocols like Websocket need to take over the 101 sending themselves
in
> the "switch protocol" phase. (correct, Jacob?). Should we delegate the
> sending of the 101 to the protocol switch handler?

If possible I would recommend avoiding this. I think the original protocol
should setup the response and then the final protocol should somehow be able
to annotate this.

The problems we try to solve now originate from the problem of doing things
different in different protocol handlers, while in theory many upgrades are
very similar.

The TLS and H2C upgrades both begin in one form and end in a different form.
Websockets are kind of different in that they require a bad request response
in a specific case. I'm not sure in which protocol this error needs to be
send though.

In TLS and H2C further errors can always be produced in the new protocol, so
when the handshake succeeds things can just go on.

> 2. General handling of request bodies. Options:
>   a setaside in core of up to nnn bytes before switch invocation
>   b do nothing, let protocol switch handler care about it

For Subversion to be able to use upgrade we would need to support a small
body on a request (few hundred bytes. Content-Length header provided).

During our current (already optimized) handshake all requests have bodies,
and introducing an additional request just to upgrade will slow things down
quite measurable on operation like 'svn log', that are mostly bound by the
handshake time.

We can't just switch the handshake to be something else... our handshake was
built upon WEBDAV and DELTA/V. We added several headers to avoid many
requests we previously performed, but we can't move away from that initial
OPTIONS request without slowing down against all older servers.

	Bert



Re: Upgrades

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 10.12.2015 um 02:07 schrieb William A Rowe Jr <wr...@rowe-clan.net>:
> 
> There is a case to be made for the h2c to also be an 'early' upgrade decision,
> AIUI (correct me if I'm wrong Stefan) the mod_http2 module sees a request,
> grabs ahold of the network in the main request thread, and then 're-processes'
> the request, repeating all of the request processing hooks for the initial request
> on the secondary h2c 'worker' thread.  By interrupting very early, during the
> post_read_request phase, these steps will no longer be duplicated.

Yes, correct. Good point for making the upgrade earlier for protocols like h2c.

Re: Upgrades

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Dec 9, 2015 at 6:16 PM, Tim Bannister <is...@c8h10n4o2.org.uk>
wrote:

> On 9 Dec 2015, at 23:19, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> > Because the request body is inbound already at some state of completion
> > or incomplete transmission, it is competing with the TLS handshake, which
> > is a bidirectional engagement with the same socket between the user agent
> > and server.  Unlike h2c and websocket, where Roy suggests we leave the
> > http/1 input filter in place and inject the http/2 output filter for the
> duration
> > of the initial Upgrade: request, we must pass TLS traffic in both
> directions
> > at once during Upgrade: TLS.
> …
> > Please remember that a request handler is based on a method and location,
> > while Upgrade is not the request itself, but a proposal to switch
> protocols.
> > In the case of TLS and h2c, that request is satisfied over the
> corresponding
> > HTTP/1 or HTTP/2 output filter, but I'm not clear whether websocket has
> > any equivalence in terms of a content handler phase.
>
> In a sense, all upgrades happen after of an existing request (& response).
> So how about this high level behaviour model:
> httpd keeps note, for each connection, of what upgrades are (1) feasible
> and (2) agreed upon.
> At end of any given request, either zero or one of the feasible upgrades
> will have been agreed between web server and client. That's when the
> upgrade should happen if it has been negotiated.
>

I think that is the design we are arriving at, that each module is queried
early
if an upgrade should be advertised, and given the chance to begin the
upgrade,
and then immediately before the content handler (after auth) these steps
are
repeated, giving the protocol modules a chance to offer an upgrade or back
out of an intended offer.  The offers must be assembled before the content
handler causes the http/1 protocol filter to push out the response headers.

In the TLS case, it must happen very early for TLS
handshake/certificates/SNI
inspection for auth purposes, and prior to beginning the response.  No
upgrade
can happen after a response (except within the next request) because the
upgrade is a product of the 101-switching protocols.

There is a case to be made for the h2c to also be an 'early' upgrade
decision,
AIUI (correct me if I'm wrong Stefan) the mod_http2 module sees a request,
grabs ahold of the network in the main request thread, and then
're-processes'
the request, repeating all of the request processing hooks for the initial
request
on the secondary h2c 'worker' thread.  By interrupting very early, during
the
post_read_request phase, these steps will no longer be duplicated.

The main thread can still leave the http/1 input filter in place to provide
the
initial request body to that initial worker thread, but won't see
additional http/2
requests until that request body is consumed, completing the 101-switching
protocols contract.

The reason not to leave the initial http/1 request in the primary thread
until
it has been satisfied is that the connection would block on both the initial
request body and the initial response, which would be a huge penalty for
an initial request that performed some back-end processing and might
have database or other back-end contention.

Eg, for a new, inbound port 80 connection: an upgrade to TLS or h2c would
> be feasible. During the first request it may transpire that an upgrade to
> WebSocket is feasible too (authz having been satisfied). Once the request
> in which the upgrade has been negotiated is complete, said upgrade takes
> place.
>

For every request within a connection, such an upgrade remains feasible.


> Any subsequent requests on the same TCP connection won't be eligible for
> upgrade to WebSocket. This kind of rule ought to live outside the HTTP/1.x
> implementation as it has more to do with WebSocket than HTTP.
>

I'm wondering the same thing, why not on the third or twentieth request?
It simply can't be downgraded back to HTTP/1, if I understand the spec
correctly.

Re: Upgrades

Posted by Jacob Champion <ch...@gmail.com>.
On 12/09/2015 04:16 PM, Tim Bannister wrote:
> Any subsequent requests on the same TCP connection won't be eligible for upgrade to WebSocket.

Why not?

--Jacob


Re: Upgrades

Posted by Tim Bannister <is...@c8h10n4o2.org.uk>.
On 9 Dec 2015, at 23:19, William A Rowe Jr <wr...@rowe-clan.net> wrote:

> Because the request body is inbound already at some state of completion
> or incomplete transmission, it is competing with the TLS handshake, which
> is a bidirectional engagement with the same socket between the user agent
> and server.  Unlike h2c and websocket, where Roy suggests we leave the
> http/1 input filter in place and inject the http/2 output filter for the duration
> of the initial Upgrade: request, we must pass TLS traffic in both directions 
> at once during Upgrade: TLS.
…
> Please remember that a request handler is based on a method and location,
> while Upgrade is not the request itself, but a proposal to switch protocols.
> In the case of TLS and h2c, that request is satisfied over the corresponding 
> HTTP/1 or HTTP/2 output filter, but I'm not clear whether websocket has
> any equivalence in terms of a content handler phase.

In a sense, all upgrades happen after of an existing request (& response). So how about this high level behaviour model:
httpd keeps note, for each connection, of what upgrades are (1) feasible and (2) agreed upon.
At end of any given request, either zero or one of the feasible upgrades will have been agreed between web server and client. That's when the upgrade should happen if it has been negotiated.

Eg, for a new, inbound port 80 connection: an upgrade to TLS or h2c would be feasible. During the first request it may transpire that an upgrade to WebSocket is feasible too (authz having been satisfied). Once the request in which the upgrade has been negotiated is complete, said upgrade takes place.

Any subsequent requests on the same TCP connection won't be eligible for upgrade to WebSocket. This kind of rule ought to live outside the HTTP/1.x implementation as it has more to do with WebSocket than HTTP.


-- 
Tim Bannister – isoma@c8h10n4o2.org.uk


Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 11, 2015 at 2:22 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> 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.

This filter would be AP_FILTER_PROTOCOL+x or (low)
AP_FILTER_CONNECTION (before mod_ssl).

>
> Wouldn't that work for every case we discussed?

I did not mentioned h2c, but it could run in post_read_request right?

Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Dec 12, 2015 at 12:41 AM, Yann Ylavic <yl...@gmail.com> wrote:
> Hi Jacob,
>
> On Fri, Dec 11, 2015 at 11:47 PM, Jacob Champion <ch...@gmail.com> wrote:
>>
>> This is where we disagree, for the case of WebSocket and WebSocket alone.
>> RFC 6455 4.2.1:
>>
>>    The client's opening handshake consists of the following parts.  If
>>    the server, while reading the handshake, finds that the client did
>>    not send a handshake that matches the description below (note that as
>>    per [RFC2616], the order of the header fields is not important),
>>    including but not limited to any violations of the ABNF grammar
>>    specified for the components of the handshake, the server MUST stop
>>    processing the client's handshake and return an HTTP response with an
>>    appropriate error code (such as 400 Bad Request).
>
> I guess the RFC is talking about a Websocket server that also
> implements HTTP/1 for Websocket purpose only.
> In httpd, the protocol parsing is done by the core http module before
> any websocket module is called, so there can't (at least shouldn't) be
> an invalid HTTP/1 grammar at that level (post_read_request[_header]),
> HTTP/1 failures ought to be handled by the core (http module),
> including ill/invalid body transfer encoding/length.
> So the error could be an unacceptable handshake for the websocket
> module, which should then let httpd handle the HTTP/1 request
> (ignoring the handshake and probably websocket headers too).

Please note that httpd's default (HTTP/1) handler can always catch
"http/1" not in Protocols and return "505 Not Supported" if the
request reaches it.
The administrator configures acceptable protocols, and httpd gives
each one's module a chance, defaulting to HTTP/1 (or 505) if none
accepts the handshake.

Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
Hi Jacob,

On Fri, Dec 11, 2015 at 11:47 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> This is where we disagree, for the case of WebSocket and WebSocket alone.
> RFC 6455 4.2.1:
>
>    The client's opening handshake consists of the following parts.  If
>    the server, while reading the handshake, finds that the client did
>    not send a handshake that matches the description below (note that as
>    per [RFC2616], the order of the header fields is not important),
>    including but not limited to any violations of the ABNF grammar
>    specified for the components of the handshake, the server MUST stop
>    processing the client's handshake and return an HTTP response with an
>    appropriate error code (such as 400 Bad Request).

I guess the RFC is talking about a Websocket server that also
implements HTTP/1 for Websocket purpose only.
In httpd, the protocol parsing is done by the core http module before
any websocket module is called, so there can't (at least shouldn't) be
an invalid HTTP/1 grammar at that level (post_read_request[_header]),
HTTP/1 failures ought to be handled by the core (http module),
including ill/invalid body transfer encoding/length.
So the error could be an unacceptable handshake for the websocket
module, which should then let httpd handle the HTTP/1 request
(ignoring the handshake and probably websocket headers too).

That does not mean that Protocols handlers can't return "applicative"
errors in handshake (eg. Protocols' auth[nz] failures), but if it
accepts/handles the handshake it MUST respond in the Upgraded
protocol, not in HTTP/1.

Regards,
Yann.

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
n 12/11/2015 02:24 PM, William A Rowe Jr wrote:
> On Fri, Dec 11, 2015 at 2:55 PM, Jacob Champion <champion.p@gmail.com
> <ma...@gmail.com>> wrote:
>     That additional constraint doesn't conflict with any part of
>     HTTP/1.1, as far as I can tell. Returning 4xx directly to an Upgrade
>     request is allowed within HTTP/1.1, right? The spec says nothing
>     about the internal server architecture.
>
> The spec describes the exception handling;
>    "A client MAY send a list of protocols in the Upgrade
>     header field of a request to invite the server to switch to one or
>     more of those protocols, in order of descending preference, before
>     sending the final response.  A server MAY ignore a received Upgrade
>     header field if it wishes to continue using the current protocol on
>     that connection.  Upgrade cannot be used to insist on a protocol
>     change."
> Unambiguous, there is no "error" in not upgrading, period.

Is your argument that, because the spec does not explicitly say that the 
server may take some action, such an action is prohibited?

> To the extent that any RFC6455
> contradiction to RFC7230 is invalid, RFC7230 is binding,

Agreed.

> but it doesn't look that way to me.  From RFC6455 4.1,
>
>     Once the client's opening handshake has been sent, the client MUST
>     wait for a response from the server before sending any further data.
>     The client MUST validate the server's response as follows:
>
>     1.  If the status code received from the server is not 101, the
>         client handles the response per HTTP [RFC2616 <https://tools.ietf.org/html/rfc2616>] procedures.  In
>         particular, the client might perform authentication if it
>         receives a 401 status code; the server might redirect the client
>         using a 3xx status code (but clients are not required to follow
>         them), etc.  Otherwise, proceed as follows.
>
> In other words, in the absence of the websocket module declining to honor
> the upgrade request, it still remains an http/1 request and response.

Agreed. A properly implemented WebSocket client is also, by necessity, 
an HTTP/1.1 client, because the server may not have any idea what 
WebSocket is.

> Whatever auth or other technical details cause that request to fail will be
> represented as an http/1 response.

Yes.

> If the implementer wants to present a successful http/1
> response to the request in lieu of establishing a websocket connection with
> the corresponding 101-switching protocols, by offering a resource at the
> given URL, that option remains.

This is where we disagree, for the case of WebSocket and WebSocket 
alone. RFC 6455 4.2.1:

    The client's opening handshake consists of the following parts.  If
    the server, while reading the handshake, finds that the client did
    not send a handshake that matches the description below (note that as
    per [RFC2616], the order of the header fields is not important),
    including but not limited to any violations of the ABNF grammar
    specified for the components of the handshake, the server MUST stop
    processing the client's handshake and return an HTTP response with an
    appropriate error code (such as 400 Bad Request).

I maintain that this requirement (stop, do not pass Go, return an error 
immediately) is not in conflict with 723x, but I am open to persuasion 
otherwise. At the moment, I just don't agree that the paragraph you 
cited above (RFC 7230 6.7)  prohibits this behavior. Are there other 
cases you can show me where omission is tantamount to prohibition?

--Jacob

Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Dec 11, 2015 at 2:55 PM, Jacob Champion <ch...@gmail.com>
wrote:

> On 12/11/2015 12:12 PM, William A Rowe Jr wrote:
>
>> On Fri, Dec 11, 2015 at 1:13 PM, Jacob Champion <champion.p@gmail.com
>> <ma...@gmail.com>> wrote:
>>
>>     On 12/11/2015 02:36 AM, Bert Huijben wrote:
>>         Not upgrading is 100% better than failing.
>>
>>     ...except for those protocols for which failure instead of upgrade
>>     is *required* by their spec (e.g. WebSocket).
>>
>> No, no protocol overrides the HTTP/1 RFCs *until* 101-switching
>> protocols has occurred at which point the new protocol definition is in
>> force, this behavior is all defined by RFC7230 section 6.7.
>>
>
> That additional constraint doesn't conflict with any part of HTTP/1.1, as
> far as I can tell. Returning 4xx directly to an Upgrade request is allowed
> within HTTP/1.1, right? The spec says nothing about the internal server
> architecture.


The spec describes the exception handling;
  "A client MAY send a list of protocols in the Upgrade
   header field of a request to invite the server to switch to one or
   more of those protocols, in order of descending preference, before
   sending the final response.  A server MAY ignore a received Upgrade
   header field if it wishes to continue using the current protocol on
   that connection.  Upgrade cannot be used to insist on a protocol
   change."
Unambiguous, there is no "error" in not upgrading, period.

If there is no underlying HTTP/1.1 resource resulting from -not- upgrading
the response, or additional constraints are in place (e.g. upgrade required,
http/1 not honored etc) then those are outside of the scope of the Protocol
switch API and fall in the domain of the normal httpd request hook scope,
IMHO.


> If there is an http:// representation of the websocket resource, that is
>> served instead of the 101-switching protocols.  If there isn't such a
>> representation, then the request of course does fail :)
>>
>
> I have to serve two masters here. All of the following are allowed by RFCs
> 723x, upon receiving a WebSocket upgrade offer:
>
> 1) Upgrade the connection to WebSocket.
> 2) Ignore the upgrade entirely, and continue processing as a normal HTTP
> request, which may or may not fail for any number of reasons.
> 3) Return a 4xx *immediately*. (This is allowed, in that it is not
> disallowed: from the point of view of 723x, it's as if the server ignored
> the upgrade but found something otherwise objectionable in the request.)
>
> RFC 6455 removes 2) from my list of choices. There is no conflict between
> the specifications here, only a reduction in available options for the
> server.
>
>     Protocol switch implementations need to have the opportunity to fail
>>     after they have been selected for that reason, but of course the TLS
>>     and HTTP/2 implementations should/would not make use of that.
>>
>> Any module can cause a request to fail for any of hundreds of reasons,
>> we must avoid that logic in the Protocol API schema because the protocol
>> handler module can deal with this in any of the request hooks it likes
>> and is most appropriate for the failure case.
>>
>
> As long as I can still satisfy 6455, I have no objection.
>
> In other words, there is one and only one recognized mechanism to establish
>> a TLS h2 connection.  I don't think we want to honor other mechanics.
>>
>
> I'm inclined to agree.
>
>     IMHO, dedicated clients who are serious about WebSocket security
>>     will have connected via a wss:// connection instead of upgrading
>>     from an initially unsecure connection. (I'm not saying it's
>>     impossible to design a secure system that uses an upgrade, but the
>>     percentage of users who can get it 100% right is likely to be low.)
>>
>> Your call as a websocket implementer, but I'd be inclined to ignore this
>> combination just as with TLS+h2c.
>>
>
> +1.


I think we are essentially on the same page.  To the extent that any RFC6455
contradiction to RFC7230 is invalid, RFC7230 is binding, but it doesn't
look that
way to me.  From RFC6455 4.1,

   Once the client's opening handshake has been sent, the client MUST
   wait for a response from the server before sending any further data.
   The client MUST validate the server's response as follows:

   1.  If the status code received from the server is not 101, the
       client handles the response per HTTP [RFC2616
<https://tools.ietf.org/html/rfc2616>] procedures.  In
       particular, the client might perform authentication if it
       receives a 401 status code; the server might redirect the client
       using a 3xx status code (but clients are not required to follow
       them), etc.  Otherwise, proceed as follows.


In other words, in the absence of the websocket module declining to honor
the upgrade request, it still remains an http/1 request and response.
Whatever
auth or other technical details cause that request to fail will be
represented as
an http/1 response.  If the implementer wants to present a successful http/1
response to the request in lieu of establishing a websocket connection with
the corresponding 101-switching protocols, by offering a resource at the
given
URL, that option remains.  If there is no resource except a websocket at
that
URL, an error will occur.

I hope that makes things clearer?

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/11/2015 12:12 PM, William A Rowe Jr wrote:
> On Fri, Dec 11, 2015 at 1:13 PM, Jacob Champion <champion.p@gmail.com
> <ma...@gmail.com>> wrote:
>
>     On 12/11/2015 02:36 AM, Bert Huijben wrote:
>         Not upgrading is 100% better than failing.
>
>     ...except for those protocols for which failure instead of upgrade
>     is *required* by their spec (e.g. WebSocket).
>
> No, no protocol overrides the HTTP/1 RFCs *until* 101-switching
> protocols has occurred at which point the new protocol definition is in
> force, this behavior is all defined by RFC7230 section 6.7.

That additional constraint doesn't conflict with any part of HTTP/1.1, 
as far as I can tell. Returning 4xx directly to an Upgrade request is 
allowed within HTTP/1.1, right? The spec says nothing about the internal 
server architecture.

> If there is an http:// representation of the websocket resource, that is
> served instead of the 101-switching protocols.  If there isn't such a
> representation, then the request of course does fail :)

I have to serve two masters here. All of the following are allowed by 
RFCs 723x, upon receiving a WebSocket upgrade offer:

1) Upgrade the connection to WebSocket.
2) Ignore the upgrade entirely, and continue processing as a normal HTTP 
request, which may or may not fail for any number of reasons.
3) Return a 4xx *immediately*. (This is allowed, in that it is not 
disallowed: from the point of view of 723x, it's as if the server 
ignored the upgrade but found something otherwise objectionable in the 
request.)

RFC 6455 removes 2) from my list of choices. There is no conflict 
between the specifications here, only a reduction in available options 
for the server.

>     Protocol switch implementations need to have the opportunity to fail
>     after they have been selected for that reason, but of course the TLS
>     and HTTP/2 implementations should/would not make use of that.
>
> Any module can cause a request to fail for any of hundreds of reasons,
> we must avoid that logic in the Protocol API schema because the protocol
> handler module can deal with this in any of the request hooks it likes
> and is most appropriate for the failure case.

As long as I can still satisfy 6455, I have no objection.

> In other words, there is one and only one recognized mechanism to establish
> a TLS h2 connection.  I don't think we want to honor other mechanics.

I'm inclined to agree.

>     IMHO, dedicated clients who are serious about WebSocket security
>     will have connected via a wss:// connection instead of upgrading
>     from an initially unsecure connection. (I'm not saying it's
>     impossible to design a secure system that uses an upgrade, but the
>     percentage of users who can get it 100% right is likely to be low.)
>
> Your call as a websocket implementer, but I'd be inclined to ignore this
> combination just as with TLS+h2c.

+1.

--Jacob


Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Dec 11, 2015 at 1:13 PM, Jacob Champion <ch...@gmail.com>
wrote:

> On 12/11/2015 02:36 AM, Bert Huijben wrote:
>
>> -----Original Message-----
>>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>>> 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.
>>
>> +1
>> Not upgrading is 100% better than failing.
>>
>
> ...except for those protocols for which failure instead of upgrade is
> *required* by their spec (e.g. WebSocket).


No, no protocol overrides the HTTP/1 RFCs *until* 101-switching protocols
has occurred at which point the new protocol definition is in force, this
behavior is all defined by RFC7230 section 6.7.

If there is an http:// representation of the websocket resource, that is
served instead of the 101-switching protocols.  If there isn't such a
representation, then the request of course does fail :)


> Protocol switch implementations need to have the opportunity to fail after
> they have been selected for that reason, but of course the TLS and HTTP/2
> implementations should/would not make use of that.
>

Any module can cause a request to fail for any of hundreds of reasons, we
must avoid that logic in the Protocol API schema because the protocol
handler module can deal with this in any of the request hooks it likes and
is most appropriate for the failure case.

One other thing that sticks in my mind:
>> Is it possible to upgrade to TLS in one request and to h2c later?
>>
>
> I believe so; is there anything in the specs that would make you think
> otherwise? An Upgrade: TLS by itself still results in an HTTP/1.1
> connection, which can respond to further upgrades, right?
>

I was under the impression that particular case was disallowed by the
HTTP/2 RFC, but I could be mistaken.


> I'll go further though: is it possible to upgrade to TLS and then to h2c
> in the *same* request?:
>
>    Upgrade: TLS/1.2, h2c


By RFC 7230, yes (and in the order you illustrated - "if multiple protocol
layers are being switched, the sender MUST list the protocols in
layer-ascending order.").

But by RFC 7540 I think the answer is a clear "no"...

   A server MUST ignore an "h2" token in an Upgrade header field.
   Presence of a token with "h2" implies HTTP/2 over TLS, which is
   instead negotiated as described in Section 3.3
<https://tools.ietf.org/html/rfc7540#section-3.3>.


3.3 <https://tools.ietf.org/html/rfc7540#section-3.3>.  Starting
HTTP/2 for "https" URIs

   A client that makes a request to an "https" URI uses TLS [TLS12
<https://tools.ietf.org/html/rfc7540#ref-TLS12>] with
   the application-layer protocol negotiation (ALPN) extension
   [TLS-ALPN <https://tools.ietf.org/html/rfc7540#ref-TLS-ALPN>].

   HTTP/2 over TLS uses the "h2" protocol identifier.  The "h2c"
   protocol identifier MUST NOT be sent by a client or selected by a
   server; the "h2c" protocol identifier describes a protocol that does
   not use TLS.


In other words, there is one and only one recognized mechanism to establish
a TLS h2 connection.  I don't think we want to honor other mechanics.

TLS and then websockets...
>> I don't think that scenario should be supported either. Webbrowsers aren't
>> going to use it... and dedicated clients probably use a direct connection
>> of
>> some sort instead of websockets.
>>
>
> IMHO, dedicated clients who are serious about WebSocket security will have
> connected via a wss:// connection instead of upgrading from an initially
> unsecure connection. (I'm not saying it's impossible to design a secure
> system that uses an upgrade, but the percentage of users who can get it
> 100% right is likely to be low.)


Your call as a websocket implementer, but I'd be inclined to ignore this
combination
just as with TLS+h2c.

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/11/2015 02:36 AM, Bert Huijben wrote:
>> -----Original Message-----
>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>> 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.
>
> +1
> Not upgrading is 100% better than failing.

...except for those protocols for which failure instead of upgrade is 
*required* by their spec (e.g. WebSocket). Protocol switch 
implementations need to have the opportunity to fail after they have 
been selected for that reason, but of course the TLS and HTTP/2 
implementations should/would not make use of that.

> One other thing that sticks in my mind:
> Is it possible to upgrade to TLS in one request and to h2c later?

I believe so; is there anything in the specs that would make you think 
otherwise? An Upgrade: TLS by itself still results in an HTTP/1.1 
connection, which can respond to further upgrades, right?

I'll go further though: is it possible to upgrade to TLS and then to h2c 
in the *same* request?:

    Upgrade: TLS/1.2, h2c

> TLS and then websockets...
> I don't think that scenario should be supported either. Webbrowsers aren't
> going to use it... and dedicated clients probably use a direct connection of
> some sort instead of websockets.

IMHO, dedicated clients who are serious about WebSocket security will 
have connected via a wss:// connection instead of upgrading from an 
initially unsecure connection. (I'm not saying it's impossible to design 
a secure system that uses an upgrade, but the percentage of users who 
can get it 100% right is likely to be low.)

--Jacob

RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: vrijdag 11 december 2015 10:20
> To: dev@httpd.apache.org
> Subject: Re: Upgrade Summary


> 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.

+1
Not upgrading is 100% better than failing.

But for Subversion not upgrading will most likely also imply not retrying
later on the same connection.

The way Subversion uses requests implies that it needs to pipeline requests.
With h2 it can do this much more efficient and stable than with http/1.1,
but not pipelining will make operations to remote repositories unusable
slow. (And I think most of us at least sometimes use the ASF master
repository from a distance:-)


So once we get past that initial request on an operation like update,
checkout or merge we will start opening multiple http/1.1 connections and
pipelining requests on those connections without further upgrade headers.
With requests leaving and responses arriving out of sync there is no way to
upgrade later on.




One other thing that sticks in my mind:
Is it possible to upgrade to TLS in one request and to h2c later?
(The other direction is explicitly forbidden by h2)

I'm not going to implement this for the pipelining reasons noted above, but
perhaps other scenarios might want this. But more likely  we might want to
explicitly block this scenario.

TLS and then websockets...
I don't think that scenario should be supported either. Webbrowsers aren't
going to use it... and dedicated clients probably use a direct connection of
some sort instead of websockets.

	Bert


Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 11, 2015 at 10:20 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
>> Am 11.12.2015 um 02:22 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> On Thu, Dec 10, 2015 at 11:46 AM, Stefan Eissing
>> <st...@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.

++1 (though I won't propose code this time, possibly soon :p )

>
> Currently, TLS and current core step on each others toes when it
> comes to the Upgrade response header. This we should fix.

Agreed.

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

Yes, they could use one of those phase, their choice to hook wherever
they need to.

In my proposal, they'd rely on the core's Upgrade output filter being
in the chain (which will be the case since the core's
post_read_request (really-)first hook would have run).
The Upgrade output filter would catch the first brigade sent to the
client and issue the 101-Upgrade response first before removing itself
from the chain and forwarding the brigade.

- TLS (post_read_request):
0. make a decision when a request body exists (either bail out,
discard or return 413),
1. enable SSL/TLS engine and filters,
2. call ap_pass_brigade(FLUSH) for the Upgrade to occur,
3. return DECLINED.

- h2c/ws (wherever, post_read_request/[pre_]handler):
0. make a decision when a request body exists (either bail out,
discard or use it!),
1. run their process_request(),
2. return DONE.

As far as they use ap_{get,pass}_brigade() for reading/writing data
this should work.
They wouldn't do anything (ie. return DECLINED early) if
r->selected_protocol != their protocol.
If some headers are needed in the 101 response, one can fill in
r->headers_out for the core Upgrade output filter to use that when
Upgrading.

>
> 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.

Yes, all could be based on (communication via) current/selected proto
(set by the core!) for decisions, each module can use that to
determine if it was elected or not.
Whether the core Upgrade output filter would do the Upgrade or not can
be driven by a r->flag too (or anything else, special bucket, ...),
the module really handling the Protocols would set it unless the offer
is finally DECLINED (for some reason).

>> +        && ap_find_token(r->pool, upgrade, "TLS")
>> +        && !ap_has_request_body(r)) {
>> []
>
> I think the first way, don't Upgrade if you know it will not work, is better.

Agreed, let's do that in ssl_hook_ReadReq(), along with:
+        && !ap_has_request_body(r)
+        && !strcasecmp(r->selected_protocol, "TLS")) {
...

>>> 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?

Yes, this is what I meant, as maybe better explained above.

>
> h2c needs to receive request body separate, get an indication
> that EOS for that body has been reached,

ap_discard_request_body()?

> 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.

I think so with my proposal, unless I'm missing something (which is
ofter the case ;)

>
>> 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.

We wouldn't need it actually, as said above, why not simply use
r->headers_out while we are still HTTP/1...


Thanks for the feedbacks, Stefan.

Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 11.12.2015 um 02:22 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> On Thu, Dec 10, 2015 at 11:46 AM, Stefan Eissing
> <st...@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


RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
I’m not talking about why the client does want the upgrade…

Perhaps it doesn’t know if port 443 uses TLS or hosts the same content. In general it is just a lucky guess that it does have the same content. The Alt-Svc spec might help here, but currently this still requires the admin to configure things… and I don’t see that change anytime soon. (And if it would be automatic, it would still assume no firewalls, certificate availability, etc., etc.)


What I’m trying to say is: we should not just deny connections an upgrade for no reason. If we can upgrade the connection with a body… why explicitly deny it *after* we already received the body? If we want to reuse the connection we have to read the body anyway.


We can’t make the connection more secure by asking the client to retry the request… We can only make things less secure that way.


Returning a 4XX error just makes the client retry the request (perhaps after asking the user). The 4XX will be more work for the server, more work for the client and perhaps even for the end user.

We don’t make the web more secure by denying upgrades after the request is already sent. And as http/1.1 doesn’t have a way to stop a request before it is sent without resetting the connection I would say that we upgrade to TLS and/or H2c whenever possible.

The server can’t decide if the client should send the request unencrypted or not *after* it is already on the wire unencrypted.

By not upgrading we *as server* decide that the next request on the same connection will be unencrypted as well… By returning a 4XX (or other error) we ask the client to retry again; most likely on the same connection and just as unencrypted as the previous request.

On disconnecting we do the same thing….

The only way to ensure encryption or more advanced protocol support is honoring the upgrade request as soon as possible.

The deadlock scenarios on bodies that can’t be sent while the request is read are no reason to just block upgrades… These same deadlocks can occur for dozens of different reasons and in case of h2 even after upgrade. (Don’t send window updates and everything stalls… by design). At that point we can handle the error, just like how we handle timeouts on http/1.1

With h2 we can even force stream 1 (the response for the upgraded request) to close with an appropriate stream error in that case… so we can still reuse the connection.


With h2 you can in theory start the response in h2 while you are still reading the http/1.1 body, while with TLS you can’t as you need a two way handshake.

There will be cases where the upgrade path definitely breaks (e.g. huge/infinite chunked request echoed as response), but I think in most cases it can succeed without a problem. Just returning an error in easy cases for consistency with the cases where it can’t work doesn’t really help.



The problem with errors like 413 is that in clients like Subversion we can only handle them by showing them as fatal error to the user. The printer scenario for upgrades to TLS probably does the same thing. Printing works or doesn’t work… It doesn’t allow retrying with a different request.

Not upgrading is an option… But upgrading under less favorable conditions is what makes things work.

      Bert

Sent from Mail for Windows 10



From: Yann Ylavic
Sent: zaterdag 12 december 2015 01:46
To: Bert Huijben
Subject: Re: Upgrade Summary


On Sat, Dec 12, 2015 at 12:48 AM, Bert Huijben <be...@qqmail.nl> wrote:
> 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.

Why using Upgrade at the first place if you are confident enough (to
play auth) with the server being TLS ready?
Wouldn't a direct https connection be better?

Is there such an authentication protocol?
If so, I guess we can do the TLS Upgrade+handshake in the output
filter (as proposed) like any other Protocols Upgrade, letting httpd
handle that first clear text request body...
That's possible without setting aside the body anyway, still I doubt
there is a real need for it (if a TLS Upgrade is asked, the client
shouldn't mind the first (half) round trip and play it's auth on the
second (TLSed) request.

>
> 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.

My preference would be to return an error (413), so that the client
notices what's "better" for a TLS Upgrade (no body), but if there
really exist such a cleartext body use case I'm fine with honoring the
body too.

>
> If upgrading the first request doesn’t make sense the client should use a
> different request (Like options *, or HEAD /).

That are, AFAICT, the only use cases so far.

>
> 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 client would not send encrypted vs unencrypted (Upgrade) request
depending on the response status, it must send plaintext request
anyway, it ought to know that.
IOW the error response is not a security issue at all, on the contrary
if a client relies on current httpd behaviour to do the handshake
before reading the body, and hence sends private things in the body of
the Upgrade request, it would notice with a response error.
Anyway if the RFC is respected, the TLS handshake doesn't happen
before any body is (fully) received on the server, hence such client
would wait for the handshake before sending anything, and that would
timeout.
Again, an error response is better here...

>
> The response is not the only thing encrypted when upgrading… all future
> requests are. Upgrade is a connection level request, sent via a request.

Agreed.

Regards,
Yann.



RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
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
<st...@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?



Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 10, 2015 at 11:46 AM, Stefan Eissing
<st...@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?

Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 10.12.2015 um 14:57 schrieb Mike Rumph <mi...@oracle.com>:
> 
> Question below:
> 
> On 12/10/2015 2:46 AM, Stefan Eissing wrote:
>> 	int xxx_switch(conn_rec *c, request_rec *r, server_rec *s,
>> 		const char *protocol, int phase)
>> 	{
>> 		apr_status_t stats;
>> 
>> 		if (strcmp(my_protocol, protocol)) {
> Do you mean "if my_protocol is protocol"?

If the protocol to switch to is not mine, DECLINE.

Should have written that in prose...

>> 			return DECLINED;
>> 		}
>> 		
>> 		if (need_handler && phase != handler) {
>> 			return APR_EAGAIN;
>> 		}
>> 
>> 		status = ap_upgrade_request(r, protocol, phase, my_additional_header, process_body);
>> 		if (status != APR_SUCCESS) {
>> 			// any type of error, could also fail if NULL is passed instead of
>> 			// &process_body and request has body
>> 			return status;
>> 		}
>> 
>> 		// c->output_filters can be written
>> 		// c->input_filters can be read, will pass data to process_body() until body is done,
>> 		// then return data up the filter chain.
>> 		process protocol;
>> 
>> 		return APR_EOF;
>> 	}
>> 
>> 
> 


Re: Upgrade Summary

Posted by Mike Rumph <mi...@oracle.com>.
Question below:

On 12/10/2015 2:46 AM, Stefan Eissing wrote:
> 	int xxx_switch(conn_rec *c, request_rec *r, server_rec *s,
> 		const char *protocol, int phase)
> 	{
> 		apr_status_t stats;
>
> 		if (strcmp(my_protocol, protocol)) {
Do you mean "if my_protocol is protocol"?
> 			return DECLINED;
> 		}
> 		
> 		if (need_handler && phase != handler) {
> 			return APR_EAGAIN;
> 		}
>
> 		status = ap_upgrade_request(r, protocol, phase, my_additional_header, process_body);
> 		if (status != APR_SUCCESS) {
> 			// any type of error, could also fail if NULL is passed instead of
> 			// &process_body and request has body
> 			return status;
> 		}
>
> 		// c->output_filters can be written
> 		// c->input_filters can be read, will pass data to process_body() until body is done,
> 		// then return data up the filter chain.
> 		process protocol;
>
> 		return APR_EOF;
> 	}
>
>


RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.

> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: donderdag 10 december 2015 11:47
> To: dev@httpd.apache.org
> Subject: Re: Upgrade Summary
> 
> Given all the input on this thread, I arrive at the following pseudo code:
> 
> 1. Post Read Request Hook:
> 
> 	if (Upgrade: request header present) {
> 		collect protocol proposals;
> 		ps = protocol with highest preference from proposals;
> 	        if (ps && ps != current) {
> 			status = switch protocol(phase => post_read);
> 			if (status == APR_EOF) {
> 				close connection;
> 			}
> 			else if (status == APR_EAGAIN) {
> 				// protocol switch wants to be called later
> before handler
> 				if (request is "OPTIONS *") {
> 					// TODO: invoke again with (phase =>
> handler)?

Why handle 'OPTIONS *' different here?

Isn't that 'just another simple HTTP/1.1 request'?


>From what I read about that request, it is a recommended easy request if you
don't have anything to send yourself in this stage. (With "HEAD /" as
alternative).

If not required I would recommend not hardcoding specific behavior on this
request. There is not much that makes it that different from "GET /" or
"HEAD /", or every other request that doesn't have a request body.

	Bert


Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
Given all the input on this thread, I arrive at the following pseudo code:

1. Post Read Request Hook:
   
	if (Upgrade: request header present) {
		collect protocol proposals;
		ps = protocol with highest preference from proposals;
	        if (ps && ps != current) {
			status = switch protocol(phase => post_read);
			if (status == APR_EOF) {
				close connection;
			}
			else if (status == APR_EAGAIN) {
				// protocol switch wants to be called later before handler
				if (request is "OPTIONS *") {
					// TODO: invoke again with (phase => handler)?
				}
				request->upgrade_protocol = ps;
			}
			else {
				// continue, HTTP/1 processing. May happen on TLS Upgrade
				// or if the protocol managed to switch back to HTTP/1
				// process other stati than APR_SUCCESS as usual
			}
		}
        	else if ("http/1.1" not in Protocols) {
			answer "505 Not Supported";
		}
	}
	else if ("http/1.1" not in Protocols) {
		answer "426 Upgrade Required";
	}

2. Handler Hook (First or just before):

	if (r->upgrade_protocol) {
		status = switch protocol(phase => handler);
		if (status == APR_EOF) {
			close connection;
		}
		else {
			// continue, HTTP/1 processing. May happen on TLS Upgrade
			// or if the protocol managed to switch back to HTTP/1
			// process other stati than APR_SUCCESS as usual
		}
	}

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.
	}

	int xxx_switch(conn_rec *c, request_rec *r, server_rec *s, 
		const char *protocol, int phase)
	{
		apr_status_t stats;

		if (strcmp(my_protocol, protocol)) {
			return DECLINED;
		}
		
		if (need_handler && phase != handler) {
			return APR_EAGAIN;
		}

		status = ap_upgrade_request(r, protocol, phase, my_additional_header, process_body);
		if (status != APR_SUCCESS) {
			// any type of error, could also fail if NULL is passed instead of
			// &process_body and request has body
			return status;
		}

		// c->output_filters can be written
		// c->input_filters can be read, will pass data to process_body() until body is done,
		// then return data up the filter chain.
		process protocol;

		return APR_EOF;
	}


Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
Jacob, thanks for the code! I will let this and my pseudo code stew over the weekend and then probably next week start the changes to upgrade handling.

//Stefan

> Am 10.12.2015 um 23:18 schrieb Jacob Champion <ch...@gmail.com>:
> 
> Okay, I finally have actual code to share. This is the original experimental pre_protocol_switch hook that Stefan and I were talking about a while ago [1], rebased onto 2.4.18. The two patches are available at
> 
>    https://github.com/jchampio/httpd/commits/dev/websocket-protocols
> 
> It is *not* an implementation of Stefan's most recent pseudocode (a primary difference is that his proposal does the filter + upgrade in one hook instead of two), and it doesn't solve many of the use cases that you've already agreed must be handled:
> 
> - 100-continue is not handled correctly
> - the incoming request body is not correctly dealt with
> - there is only one switch point, at the handler stage
> - etc.
> 
> *But* it is a working implementation from mod_websocket's point of view, so I offer it up primarily for informational purposes. The module making use of the new hook is available at
> 
> https://github.com/jchampio/apache-websocket/blob/dev/hook/mod_websocket.c
> 
> You might be most interested in the mod_websocket_pre_protocol_switch() implementation (line 1627), which contains the pre-upgrade checks I need to do to remain compliant with RFC 6455.
> 
> == Asides ==
> 
> 1) The current Protocols API does not correctly ignore upgrades from HTTP/1.0 requests; the latest commit to my experimental branch patched this.
> 
> 2) As Stefan and I discussed in [1], Upgrade header values are case-sensitive (correct?), but the RFC 6455 declares that servers should respond to any protocol that's a case-insensitive match for "websocket". To make matters worse, the official IANA upgrade token is "WebSocket", but the RFC uses "websocket" throughout. I have seen both used in the wild (Autobahn uses WebSocket; Firefox uses websocket), and I'm not sure what the right way to resolve this is.
> 
> Hope this helps. Again, I don't expect this to actually be part of the final solution, but sometimes it's nice to see code.
> 
> --Jacob
> 
> [1] https://mail-archives.apache.org/mod_mbox/httpd-dev/201509.mbox/%3C55FF85F0.8040106@gmail.com%3E


Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
Okay, I finally have actual code to share. This is the original 
experimental pre_protocol_switch hook that Stefan and I were talking 
about a while ago [1], rebased onto 2.4.18. The two patches are available at

     https://github.com/jchampio/httpd/commits/dev/websocket-protocols

It is *not* an implementation of Stefan's most recent pseudocode (a 
primary difference is that his proposal does the filter + upgrade in one 
hook instead of two), and it doesn't solve many of the use cases that 
you've already agreed must be handled:

- 100-continue is not handled correctly
- the incoming request body is not correctly dealt with
- there is only one switch point, at the handler stage
- etc.

*But* it is a working implementation from mod_websocket's point of view, 
so I offer it up primarily for informational purposes. The module making 
use of the new hook is available at

 
https://github.com/jchampio/apache-websocket/blob/dev/hook/mod_websocket.c

You might be most interested in the mod_websocket_pre_protocol_switch() 
implementation (line 1627), which contains the pre-upgrade checks I need 
to do to remain compliant with RFC 6455.

== Asides ==

1) The current Protocols API does not correctly ignore upgrades from 
HTTP/1.0 requests; the latest commit to my experimental branch patched this.

2) As Stefan and I discussed in [1], Upgrade header values are 
case-sensitive (correct?), but the RFC 6455 declares that servers should 
respond to any protocol that's a case-insensitive match for "websocket". 
To make matters worse, the official IANA upgrade token is "WebSocket", 
but the RFC uses "websocket" throughout. I have seen both used in the 
wild (Autobahn uses WebSocket; Firefox uses websocket), and I'm not sure 
what the right way to resolve this is.

Hope this helps. Again, I don't expect this to actually be part of the 
final solution, but sometimes it's nice to see code.

--Jacob

[1] 
https://mail-archives.apache.org/mod_mbox/httpd-dev/201509.mbox/%3C55FF85F0.8040106@gmail.com%3E

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/10/2015 02:45 AM, Bert Huijben wrote:
> Websockets are different… and in some ways not really an upgrade of the
> connection… more like a hostile takeover with a final operation to a target

Ha! Yes -- although from the point of view of HTTP/1.1, all successful 
upgrades are hostile takeovers, no? But a failure to upgrade is more of 
a problem for a WebSocket client than a pure-HTTP client.

> More like a CONNECT request to a proxy…

Heh, interesting that you mention that. Proxy and cache behavior is, 
IIRC, the motivation for many of the strange-looking design decisions in 
the WebSocket protocol...

--Jacob

RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
Great to see where this discussion is headed:

+1 on the last design ideas.

 

Going with one ‘as early as possible’ upgrade and one ‘upgrade last’ should handle all these cases just fine.

 

I don’t think the h2c and TLS cases really have to be that different as suggested in the earlier parts of the discussion. Both want to upgrade as soon as possible, which is +- after the initial request is read, and before the response is generated.

 

TLS then really needs to exchange information both ways, while for h2c allowing exchange both ways allows doing some things a bit earlier.

 

 

Websockets are different… and in some ways not really an upgrade of the connection… more like a hostile takeover with a final operation to a targetJ

 

More like a CONNECT request to a proxy… or perhaps a PRI request to a HTTP/1.1 server that handles h2direct that way… one final request until the connection closes.

 

                Bert

 

 

From: William A Rowe Jr [mailto:wrowe@rowe-clan.net] 
Sent: donderdag 10 december 2015 07:40
To: httpd <de...@httpd.apache.org>
Subject: Re: Upgrade Summary

 

On Wed, Dec 9, 2015 at 9:22 PM, Jacob Champion <champion.p@gmail.com <ma...@gmail.com> > wrote:

On 12/09/2015 05:19 PM, William A Rowe Jr wrote:

 

    _If_ all the other protocols worked like WebSocket and required
    authnz before an upgrade could succeed, it wouldn't make sense for
    each upgrade handler to have to do the authnz check themselves. But
    in this case, WebSocket is different enough that I think it will
    probably have to.

No, because we will simply give you two chances to accept and trigger
the upgrade, once in the post_read_request, another in ap_invoke_handler
phase before filters are inserted.


Here you make it sound as if I won't have to duplicate the authnz checks in the handler phase, but in a previous email you said

websocket can ignore the Upgrade: websocket offer after inspecting authnz


Did I misunderstand? I feel like I'm missing something crucial to your point.

 

I propose two chances to catch the upgrade, early and late.  You need auth,

therefore you must inspect the late catch, which follows authnz but precedes 

the handler invocation.

 

If a websocket protocol module determines that authnz denied websocket

but not the resource, then it can proceed to the normal http/1 handler without

responding with a 101-switching protocols.  But if the request meets the

requirements to be handled by the websocket protocol, you reply to the

Protocol API with an 'upgrade accepted' and take ownership of the request

and the connection.

 

Sounds reasonable.

I'd certainly like to see a websocket prototype
or outline before we declare the protocol baked for the second time :)


Working on it. :) My original experimental hook for 2.4.17 rebased nicely onto 2.4.18, but mod_websocket has been refactored significantly since I wrote that and I couldn't reuse my work. Sorry I've been slow in actual code-writing; I've been under the weather this week.

 

No worries, 2.4.18 is an incremental set of improvements, and 2.4.19

will be moreso, but not in the next week :) 

 


Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Dec 9, 2015 at 9:22 PM, Jacob Champion <ch...@gmail.com> wrote:

> On 12/09/2015 05:19 PM, William A Rowe Jr wrote:
>
>>
>>     _If_ all the other protocols worked like WebSocket and required
>>     authnz before an upgrade could succeed, it wouldn't make sense for
>>     each upgrade handler to have to do the authnz check themselves. But
>>     in this case, WebSocket is different enough that I think it will
>>     probably have to.
>>
>> No, because we will simply give you two chances to accept and trigger
>> the upgrade, once in the post_read_request, another in ap_invoke_handler
>> phase before filters are inserted.
>>
>
> Here you make it sound as if I won't have to duplicate the authnz checks
> in the handler phase, but in a previous email you said
>
> websocket can ignore the Upgrade: websocket offer after inspecting authnz
>>
>
> Did I misunderstand? I feel like I'm missing something crucial to your
> point.


I propose two chances to catch the upgrade, early and late.  You need auth,
therefore you must inspect the late catch, which follows authnz but
precedes
the handler invocation.

If a websocket protocol module determines that authnz denied websocket
but not the resource, then it can proceed to the normal http/1 handler
without
responding with a 101-switching protocols.  But if the request meets the
requirements to be handled by the websocket protocol, you reply to the
Protocol API with an 'upgrade accepted' and take ownership of the request
and the connection.


> Sounds reasonable.
>
> I'd certainly like to see a websocket prototype
>> or outline before we declare the protocol baked for the second time :)
>>
>
> Working on it. :) My original experimental hook for 2.4.17 rebased nicely
> onto 2.4.18, but mod_websocket has been refactored significantly since I
> wrote that and I couldn't reuse my work. Sorry I've been slow in actual
> code-writing; I've been under the weather this week.


No worries, 2.4.18 is an incremental set of improvements, and 2.4.19
will be moreso, but not in the next week :)

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/09/2015 05:19 PM, William A Rowe Jr wrote:
>     The "handler phase" is simply the establishment of a bidirectional
>     WebSocket connection. If the client did something pathological like
>     sending a request body with their GET request, I think that needs to
>     go into the bit bucket before the upgrade completes.
>
> If that is permissible under the websocket API, then fine.

There's no mention of it either way. I think most people forget that GET 
requests can have (useless?) bodies, anyway.

>     _If_ all the other protocols worked like WebSocket and required
>     authnz before an upgrade could succeed, it wouldn't make sense for
>     each upgrade handler to have to do the authnz check themselves. But
>     in this case, WebSocket is different enough that I think it will
>     probably have to.
>
> No, because we will simply give you two chances to accept and trigger
> the upgrade, once in the post_read_request, another in ap_invoke_handler
> phase before filters are inserted.

Here you make it sound as if I won't have to duplicate the authnz checks 
in the handler phase, but in a previous email you said

> websocket can ignore the Upgrade: websocket offer after inspecting authnz

Did I misunderstand? I feel like I'm missing something crucial to your 
point.

> But you are painting all the other protocols into a corner.  TLS will resume
> the http/1 request on the same thread.

Cool, that answers my question then ("Are there any cases where, after a 
successful upgrade, we would not want to close the connection upon 
returning from the protocol handler?"). If TLS is able to switch the 
filters around and have the stack continue as normal, then that sounds 
like a good reason not to do what I was suggesting. ;D

> the h2c or websocket protocol marks
> the connection: close (or already closed/eof, depending) which prevents
> the original http/1 handler from treating the connection as a keep-alive
> connection.  In the websocket case, I expect you can do that as step #1
> in the protocol handler to address any paranoia about falling back into the
> request processing cycle.

Sounds reasonable.

> I'd certainly like to see a websocket prototype
> or outline before we declare the protocol baked for the second time :)

Working on it. :) My original experimental hook for 2.4.17 rebased 
nicely onto 2.4.18, but mod_websocket has been refactored significantly 
since I wrote that and I couldn't reuse my work. Sorry I've been slow in 
actual code-writing; I've been under the weather this week.

--Jacob


Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Dec 9, 2015 at 6:50 PM, Jacob Champion <ch...@gmail.com> wrote:

> On 12/09/2015 03:19 PM, William A Rowe Jr wrote:
>
>> Because the request body is inbound already at some state of completion
>> or incomplete transmission, it is competing with the TLS handshake, which
>> is a bidirectional engagement with the same socket between the user agent
>> and server.  Unlike h2c and websocket, where Roy suggests we leave the
>> http/1 input filter in place and inject the http/2 output filter for the
>> duration
>> of the initial Upgrade: request, we must pass TLS traffic in both
>> directions
>> at once during Upgrade: TLS.
>>
>> Also, the results of the TLS handshake are interesting to authnz
>> configuration
>> and need to be available throughout those request phases.  This means that
>> for any Upgrade: TLS request must be satisfied (101 switching protocols)
>> prior to those auth phases.
>>
>> In the case of h2c and websocket, the http/1.1 request headers are
>> sufficient
>> to begin processing the request. At the end of the first request, the
>> input
>> filter is also toggled into the new protocol to resume processing of all
>> subsequent requests.  So this Upgrade: handling must occur before the
>> request handler is invoked, even a simple handler like 'OPTIONS *',
>>
>
> Great, thanks for the clarification.
>
> Please remember that a request handler is based on a method and location,
>> while Upgrade is not the request itself, but a proposal to switch
>> protocols.
>>
>
> I... think I agree with you, but I'm not clear on the point you're trying
> to make. It is true that an Upgrade is a proposal to switch protocols. It
> is also true that an Upgrade is associated with a specific HTTP/1.1
> request, with a specific request-target.
>
> The latter point is (more?) important for protocols like WebSocket. When
> you do a [GET /blah + Upgrade: h2c], the resulting protocol (HTTP/2) has
> access to the full global scope of the server after the upgrade. Even if
> /blah doesn't exist(!), the upgrade can proceed, and I can use the
> resulting connection to perform other HTTP/2 requests.
>
> In contrast, a WebSocket upgrade limits the scope of the connection to
> whatever resource is at the request-target. If I have two WebSocket
> endpoints at /ws1 and /ws2, and I do a [GET /ws1 + Upgrade: websocket], I
> can't use the resulting connection to later switch to the resource at /ws2.
> I'm stuck with whatever is at /ws1 until I start a completely new
> connection.


That was my understanding, thanks for clarifying.


> In the case of TLS and h2c, that request is satisfied over the
>> corresponding
>> HTTP/1 or HTTP/2 output filter, but I'm not clear whether websocket has
>> any equivalence in terms of a content handler phase.
>>
>
> The "handler phase" is simply the establishment of a bidirectional
> WebSocket connection. If the client did something pathological like sending
> a request body with their GET request, I think that needs to go into the
> bit bucket before the upgrade completes.


If that is permissible under the websocket API, then fine.  If the
websocket
protocol disallows request bodies altogether, that's also fine.  You simply
ignore the Upgrade: request and we continue to process the request as
http/1.  If there is no http/1 representation, the appropriate error should
be
emitted in or before the handler phase.


>     Sounds reasonable. Unfortunate in the sense that it's possible for a
>>     module implementing the Upgrade API to screw up and accidentally
>>     ignore authnz, but since most protocols we're talking about can
>>     safely upgrade first and respond with 401/3 on the new protocol, I
>>     don't see much of an alternative.
>>
>> Howso?  Any module can have any bug, I don't see your objection.
>>
>
> Some APIs are easier to write bugs with than others. All else being equal,
> I have a personal preference for strong/defensive API boundaries over
> weaker/looser ones, that's all. Especially for security-related code.
>
> _If_ all the other protocols worked like WebSocket and required authnz
> before an upgrade could succeed, it wouldn't make sense for each upgrade
> handler to have to do the authnz check themselves. But in this case,
> WebSocket is different enough that I think it will probably have to.
>

No, because we will simply give you two chances to accept and trigger
the upgrade, once in the post_read_request, another in ap_invoke_handler
phase before filters are inserted.  A websocket protocol handler will likely
choose the later, obviously.  TLS and quite possibly h2c should choose
the former.  Other Upgrade: handers to be added later would make their
respective choice based on what they need to accomplish.


>     Why leave it up to the protocol handler (to potentially get wrong)?
>>     Are there any cases where, after a successful upgrade, we would not
>>     want to close the connection upon returning from the protocol handler?
>>
>> Why are you focused on such bugs?  It isn't making any sense.
>>
>
> Same reason as before. Reducing the number of places an upgrade handler
> can get it wrong by strengthening the API's guarantees is a Good Thing,
> IMHO, as long as the cost of doing that is reasonable and we aren't
> painting ourselves into a corner.


But you are painting all the other protocols into a corner.  TLS will resume
the http/1 request on the same thread.  h2c will resume the http/1 request
either in the same thread (problematic) or in an h2 worker thread.  That
request_rec will indicate completion, the h2c or websocket protocol marks
the connection: close (or already closed/eof, depending) which prevents
the original http/1 handler from treating the connection as a keep-alive
connection.  In the websocket case, I expect you can do that as step #1
in the protocol handler to address any paranoia about falling back into the
request processing cycle.

Good coding style should prevent the scenarios you describe, that's why
we leverage peer review, much like the recent review of the protocol api by
so many of the httpd devs.  I'd certainly like to see a websocket prototype
or outline before we declare the protocol baked for the second time :)

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/09/2015 03:19 PM, William A Rowe Jr wrote:
> Because the request body is inbound already at some state of completion
> or incomplete transmission, it is competing with the TLS handshake, which
> is a bidirectional engagement with the same socket between the user agent
> and server.  Unlike h2c and websocket, where Roy suggests we leave the
> http/1 input filter in place and inject the http/2 output filter for the
> duration
> of the initial Upgrade: request, we must pass TLS traffic in both
> directions
> at once during Upgrade: TLS.
>
> Also, the results of the TLS handshake are interesting to authnz
> configuration
> and need to be available throughout those request phases.  This means that
> for any Upgrade: TLS request must be satisfied (101 switching protocols)
> prior to those auth phases.
>
> In the case of h2c and websocket, the http/1.1 request headers are
> sufficient
> to begin processing the request. At the end of the first request, the input
> filter is also toggled into the new protocol to resume processing of all
> subsequent requests.  So this Upgrade: handling must occur before the
> request handler is invoked, even a simple handler like 'OPTIONS *',

Great, thanks for the clarification.

> Please remember that a request handler is based on a method and location,
> while Upgrade is not the request itself, but a proposal to switch protocols.

I... think I agree with you, but I'm not clear on the point you're 
trying to make. It is true that an Upgrade is a proposal to switch 
protocols. It is also true that an Upgrade is associated with a specific 
HTTP/1.1 request, with a specific request-target.

The latter point is (more?) important for protocols like WebSocket. When 
you do a [GET /blah + Upgrade: h2c], the resulting protocol (HTTP/2) has 
access to the full global scope of the server after the upgrade. Even if 
/blah doesn't exist(!), the upgrade can proceed, and I can use the 
resulting connection to perform other HTTP/2 requests.

In contrast, a WebSocket upgrade limits the scope of the connection to 
whatever resource is at the request-target. If I have two WebSocket 
endpoints at /ws1 and /ws2, and I do a [GET /ws1 + Upgrade: websocket], 
I can't use the resulting connection to later switch to the resource at 
/ws2. I'm stuck with whatever is at /ws1 until I start a completely new 
connection.

> In the case of TLS and h2c, that request is satisfied over the
> corresponding
> HTTP/1 or HTTP/2 output filter, but I'm not clear whether websocket has
> any equivalence in terms of a content handler phase.

The "handler phase" is simply the establishment of a bidirectional 
WebSocket connection. If the client did something pathological like 
sending a request body with their GET request, I think that needs to go 
into the bit bucket before the upgrade completes.

>     Sounds reasonable. Unfortunate in the sense that it's possible for a
>     module implementing the Upgrade API to screw up and accidentally
>     ignore authnz, but since most protocols we're talking about can
>     safely upgrade first and respond with 401/3 on the new protocol, I
>     don't see much of an alternative.
>
> Howso?  Any module can have any bug, I don't see your objection.

Some APIs are easier to write bugs with than others. All else being 
equal, I have a personal preference for strong/defensive API boundaries 
over weaker/looser ones, that's all. Especially for security-related code.

_If_ all the other protocols worked like WebSocket and required authnz 
before an upgrade could succeed, it wouldn't make sense for each upgrade 
handler to have to do the authnz check themselves. But in this case, 
WebSocket is different enough that I think it will probably have to.

>     Why leave it up to the protocol handler (to potentially get wrong)?
>     Are there any cases where, after a successful upgrade, we would not
>     want to close the connection upon returning from the protocol handler?
>
> Why are you focused on such bugs?  It isn't making any sense.

Same reason as before. Reducing the number of places an upgrade handler 
can get it wrong by strengthening the API's guarantees is a Good Thing, 
IMHO, as long as the cost of doing that is reasonable and we aren't 
painting ourselves into a corner.

--Jacob

Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Dec 9, 2015 at 1:57 PM, Jacob Champion <ch...@gmail.com> wrote:

> On 12/09/2015 09:17 AM, William A Rowe Jr wrote:
>
>> On Tue, Dec 8, 2015 at 9:10 PM, Roy T. Fielding <fielding@gbiv.com
>> <ma...@gbiv.com>> wrote:
>>
>>     This should be easily handled by adding a filter that translates the
>>     HTTP/1
>>     incoming body (if any) to a single channel of the new protocol.
>>     Just fake it.
>>     There is no need to wait or set aside the bytes, unless that is
>>     desired for
>>     other reasons (e.g., denial of denial of service attacks).
>>
>> However, we must read the request body complete before the input handler
>> is toggled to the h2c state, or before we switch to any bidirectional
>> protocol, e.g. TLS or otherwise.
>>
>
> For my own clarification: this is a protocol-specific limitation, right?
> As in, you can't begin a TLS handshake without first draining the incoming
> request body off the connection.


Because the request body is inbound already at some state of completion
or incomplete transmission, it is competing with the TLS handshake, which
is a bidirectional engagement with the same socket between the user agent
and server.  Unlike h2c and websocket, where Roy suggests we leave the
http/1 input filter in place and inject the http/2 output filter for the
duration
of the initial Upgrade: request, we must pass TLS traffic in both
directions
at once during Upgrade: TLS.

Also, the results of the TLS handshake are interesting to authnz
configuration
and need to be available throughout those request phases.  This means that
for any Upgrade: TLS request must be satisfied (101 switching protocols)
prior to those auth phases.

In the case of h2c and websocket, the http/1.1 request headers are
sufficient
to begin processing the request. At the end of the first request, the input
filter is also toggled into the new protocol to resume processing of all
subsequent requests.  So this Upgrade: handling must occur before the
request handler is invoked, even a simple handler like 'OPTIONS *',

Please remember that a request handler is based on a method and location,
while Upgrade is not the request itself, but a proposal to switch protocols.
In the case of TLS and h2c, that request is satisfied over the
corresponding
HTTP/1 or HTTP/2 output filter, but I'm not clear whether websocket has
any equivalence in terms of a content handler phase.

    > 3. When to do the upgrade dance:
>>     >  a post_read_request: upgrade precedes authentication
>>     >  b handler: upgrade only honored on authenticated and otherwise ok
>> requests
>>     >  c both: introduce separate hooks? have an additional parameter?
>> more complexity
>>
>>     (a).  We do want to upgrade non-ok responses.  If the "new" protocol
>>     wants to
>>     send a canned HTTP/1.1 error, it can do so without our help.
>>
>> +1 in the case of h2c, TLS.  I understand that websocket upgrades are
>> going to be conditional on authentication. As long as we give websocket
>> the chance to upgrade in the fixups or similar phase, after auth, then
>> websocket can ignore the Upgrade: websocket offer after inspecting authnz.
>>
>
> Sounds reasonable. Unfortunate in the sense that it's possible for a
> module implementing the Upgrade API to screw up and accidentally ignore
> authnz, but since most protocols we're talking about can safely upgrade
> first and respond with 401/3 on the new protocol, I don't see much of an
> alternative.
>

Howso?  Any module can have any bug, I don't see your objection.

I think we agree that TLS upgrades happen early, h2c/websocket upgrades
happen late (before the content handler is invoked), and if either the
protocol
module or an auth module has a bug, we fix the bugs as with any other code
in the server.  Right?

    > 4. status code of protocol switch handler: if we move the task of 101
>> sending, the switch handler might not do it and keep the connection on the
>> "old" protocol. Then a connection close is not necessary. So, we would do
>> the close only when the switch handler returns APR_EOF.
>>
>>     Eh?
>>
>> I'm wondering the same, the new protocol loop can do all of the
>> connection handling, no need for the protocol switch to do anything but
>> continue, if the connection is ended in the protocol handler, it's gone.
>>
>
> Why leave it up to the protocol handler (to potentially get wrong)? Are
> there any cases where, after a successful upgrade, we would not want to
> close the connection upon returning from the protocol handler?


Why are you focused on such bugs?  It isn't making any sense.  If the
module sends a response or indicates the connection is closed, there is
nothing more that happens in the request processing loop anyways.  This
isn't for the core Protocol API to solve, IMHO.

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/09/2015 09:17 AM, William A Rowe Jr wrote:
> On Tue, Dec 8, 2015 at 9:10 PM, Roy T. Fielding <fielding@gbiv.com
> <ma...@gbiv.com>> wrote:
>
>     > On Dec 8, 2015, at 2:07 AM, Stefan Eissing <stefan.eissing@greenbytes.de <ma...@greenbytes.de>>
>     wrote:
>     >
>     > Open:
>     > 1. Protocols like Websocket need to take over the 101 sending themselves in the "switch protocol" phase. (correct, Jacob?). Should we delegate the sending of the 101 to the protocol switch handler?
>
>     That seems unlikely.
>
>
> Agreed, anywhere we like we can send the 101 with headers, all that the
> websocket module needed to do was to set up output_headers or perhaps we
> add intermediate_output_headers in the request structure.  Easily fixed
> so this code is still common...

+1.

>     This should be easily handled by adding a filter that translates the
>     HTTP/1
>     incoming body (if any) to a single channel of the new protocol.
>     Just fake it.
>     There is no need to wait or set aside the bytes, unless that is
>     desired for
>     other reasons (e.g., denial of denial of service attacks).
>
> However, we must read the request body complete before the input handler
> is toggled to the h2c state, or before we switch to any bidirectional
> protocol, e.g. TLS or otherwise.

For my own clarification: this is a protocol-specific limitation, right? 
As in, you can't begin a TLS handshake without first draining the 
incoming request body off the connection.

>
>     > 3. When to do the upgrade dance:
>     >  a post_read_request: upgrade precedes authentication
>     >  b handler: upgrade only honored on authenticated and otherwise ok requests
>     >  c both: introduce separate hooks? have an additional parameter? more complexity
>
>     (a).  We do want to upgrade non-ok responses.  If the "new" protocol
>     wants to
>     send a canned HTTP/1.1 error, it can do so without our help.
>
>
> +1 in the case of h2c, TLS.  I understand that websocket upgrades are
> going to be conditional on authentication. As long as we give websocket
> the chance to upgrade in the fixups or similar phase, after auth, then
> websocket can ignore the Upgrade: websocket offer after inspecting authnz.

Sounds reasonable. Unfortunate in the sense that it's possible for a 
module implementing the Upgrade API to screw up and accidentally ignore 
authnz, but since most protocols we're talking about can safely upgrade 
first and respond with 401/3 on the new protocol, I don't see much of an 
alternative.

>     > 4. status code of protocol switch handler: if we move the task of 101 sending, the switch handler might not do it and keep the connection on the "old" protocol. Then a connection close is not necessary. So, we would do the close only when the switch handler returns APR_EOF.
>
>     Eh?
>
> I'm wondering the same, the new protocol loop can do all of the
> connection handling, no need for the protocol switch to do anything but
> continue, if the connection is ended in the protocol handler, it's gone.

Why leave it up to the protocol handler (to potentially get wrong)? Are 
there any cases where, after a successful upgrade, we would not want to 
close the connection upon returning from the protocol handler?

--Jacob


Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Dec 8, 2015 at 9:10 PM, Roy T. Fielding <fi...@gbiv.com> wrote:

> > On Dec 8, 2015, at 2:07 AM, Stefan Eissing <st...@greenbytes.de>
> wrote:
> >
> > Open:
> > 1. Protocols like Websocket need to take over the 101 sending themselves
> in the "switch protocol" phase. (correct, Jacob?). Should we delegate the
> sending of the 101 to the protocol switch handler?
>
> That seems unlikely.


Agreed, anywhere we like we can send the 101 with headers, all that the
websocket module needed to do was to set up output_headers or perhaps we
add intermediate_output_headers in the request structure.  Easily fixed so
this code is still common...


> > 2. General handling of request bodies. Options:
> >  a setaside in core of up to nnn bytes before switch invocation
> >  b do nothing, let protocol switch handler care about it
>
> I think folks are confusing implementation with protocol.  There is no need
> for the protocol being read on a connection to be the same protocol that is
> being written in response.  In other words, the incoming connection remains
> reading HTTP/1 bits until the message is finished, regardless of the
> decision
> to upgrade the response stream -- the other protocol engine doesn't care.
>

+1, between HTTP/1 and HTTP/2, or between HTTP/1 and websocket.  (And we
already worked out that websocket upgrade passes no request body anyways,
right?)


> This should be easily handled by adding a filter that translates the HTTP/1
> incoming body (if any) to a single channel of the new protocol.  Just fake
> it.
> There is no need to wait or set aside the bytes, unless that is desired for
> other reasons (e.g., denial of denial of service attacks).
>

However, we must read the request body complete before the input handler is
toggled to the h2c state, or before we switch to any bidirectional
protocol, e.g. TLS or otherwise.

> 3. When to do the upgrade dance:
> >  a post_read_request: upgrade precedes authentication
> >  b handler: upgrade only honored on authenticated and otherwise ok
> requests
> >  c both: introduce separate hooks? have an additional parameter? more
> complexity
>
> (a).  We do want to upgrade non-ok responses.  If the "new" protocol wants
> to
> send a canned HTTP/1.1 error, it can do so without our help.
>

+1 in the case of h2c, TLS.  I understand that websocket upgrades are going
to be conditional on authentication. As long as we give websocket the
chance to upgrade in the fixups or similar phase, after auth, then
websocket can ignore the Upgrade: websocket offer after inspecting authnz.


> > 4. status code of protocol switch handler: if we move the task of 101
> sending, the switch handler might not do it and keep the connection on the
> "old" protocol. Then a connection close is not necessary. So, we would do
> the close only when the switch handler returns APR_EOF.
>
> Eh?


I'm wondering the same, the new protocol loop can do all of the connection
handling, no need for the protocol switch to do anything but continue, if
the connection is ended in the protocol handler, it's gone.


> > 5. Will it be possible to migrate the current TLS upgrade handling to
> this revised scheme?
>
> TLS upgrade is never done with a body (and is broken code anyway).  Just
> fix it.
>

Fixing (leveraging set-aside code that already exists in mod_ssl for TLS
protocol renegotiation)... I see nothing in either RFC that suggests that
upgrade to TLS with a request body is not permitted.  You may be right
about existing CUPS clients.

Note that the upgrade token is "TLS" -- it does not have to be "TLS/1.0".
>

Funny, just working on this w.r.t. TLS vs TLS/1 vs TLS/1.x  - we need to
accept any of the above.  We should reject TLS/1.3, and TLS/2 however,
whenever mod_ssl/openssl is not configured handshake those protocols.

I interpret the token version as the minimum acceptable handshake, e.g. any
client that will handshake only TLS/1.1 or TLS/1.2 would send Upgrade: TLS
or Upgrade: TLS/1 or Upgrade: TLS/1.1. The TLS handshake itself will
present the best possible protocol level, while successfully negotiating
this earlier protocol.  We also must advertise TLS/1.1 (or TLS/1.2) if
mod_ssl/openssl is not configured to accept TLS/1.0 (or TLS/1.1)
handshakes. am I misinterpreting this?

Re: Upgrade Summary

Posted by "Roy T. Fielding" <fi...@gbiv.com>.
> On Dec 8, 2015, at 2:07 AM, Stefan Eissing <st...@greenbytes.de> wrote:
> 
> Trying to summarize the status of the discussion and where the issues are with the current Upgrade implementation.
> 
> Clarified:
> A. any 100 must be sent out *before* a 101 response
> B. request bodies are to be read in the original protocol, input filters like chunk can be used, indeed are necessary, as if the request is being processed normally

Yes.

> C. if a protocol supports upgrade on request bodies is up to the protocol implementation and needs to be checked in the "propose" phase

In some respects, yes, but Upgrade is defined by HTTP/1 and no other
protocol applies until after it is done.  That means ignoring any
idiotic handshake requirements of the "new" protocol that aren't
consistent with HTTP/1.  It also means h2's requirements on Upgrade
are irrelevant except for when it requires not upgrading to h2.

The client's assumption must be that the Upgrade will fail and any
attempt to use Expect will timeout, so the entire message will be
sent eventually regardless of anyone's stupid hacks to try and game
the protocol.  Hence, the server has no choice but to receive the
entire message as HTTP/1.1 even if it thinks it could have responded
fast enough to interrupt the client in mid-send.

> 
> Open:
> 1. Protocols like Websocket need to take over the 101 sending themselves in the "switch protocol" phase. (correct, Jacob?). Should we delegate the sending of the 101 to the protocol switch handler?

That seems unlikely.

> 2. General handling of request bodies. Options:
>  a setaside in core of up to nnn bytes before switch invocation
>  b do nothing, let protocol switch handler care about it

I think folks are confusing implementation with protocol.  There is no need
for the protocol being read on a connection to be the same protocol that is
being written in response.  In other words, the incoming connection remains
reading HTTP/1 bits until the message is finished, regardless of the decision
to upgrade the response stream -- the other protocol engine doesn't care.

This should be easily handled by adding a filter that translates the HTTP/1
incoming body (if any) to a single channel of the new protocol.  Just fake it.
There is no need to wait or set aside the bytes, unless that is desired for
other reasons (e.g., denial of denial of service attacks).

> 3. When to do the upgrade dance:
>  a post_read_request: upgrade precedes authentication
>  b handler: upgrade only honored on authenticated and otherwise ok requests
>  c both: introduce separate hooks? have an additional parameter? more complexity

(a).  We do want to upgrade non-ok responses.  If the "new" protocol wants to
send a canned HTTP/1.1 error, it can do so without our help.

> 4. status code of protocol switch handler: if we move the task of 101 sending, the switch handler might not do it and keep the connection on the "old" protocol. Then a connection close is not necessary. So, we would do the close only when the switch handler returns APR_EOF.

Eh?

> 5. Will it be possible to migrate the current TLS upgrade handling to this revised scheme?

TLS upgrade is never done with a body (and is broken code anyway).  Just fix it.
Note that the upgrade token is "TLS" -- it does not have to be "TLS/1.0".

....Roy


Re: Upgrade Summary

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Dec 8, 2015 at 3:43 PM, Jacob Champion <ch...@gmail.com> wrote:

> On 12/08/2015 01:03 PM, Jacob Champion wrote:
>
>> - The module that won the proposal is given one last chance to check the
>> incoming request and fail the upgrade with an immediate HTTP response.
>>
>
> And to add to this, for completeness: mod_websocket also needs to add any
> required headers (e.g. Sec-WebSocket-Accept/Protocol/Extensions) to the 101
> response during this same conceptual "check" phase.


It seems prudent in the API design to allow the target of the switch to also
specify any exceptional 100-continue interim response headers as well as
these 101-switching protocols interim response headers.

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/08/2015 01:03 PM, Jacob Champion wrote:
> - The module that won the proposal is given one last chance to check the
> incoming request and fail the upgrade with an immediate HTTP response.

And to add to this, for completeness: mod_websocket also needs to add 
any required headers (e.g. Sec-WebSocket-Accept/Protocol/Extensions) to 
the 101 response during this same conceptual "check" phase.

--Jacob

Re: Upgrade Summary

Posted by Jacob Champion <ch...@gmail.com>.
On 12/08/2015 02:07 AM, Stefan Eissing wrote:
> C. if a protocol supports upgrade on request bodies is up to the protocol implementation and needs to be checked in the "propose" phase

Only if the module's intent is to simply ignore upgrade requests if they 
have bodies. Other modules might choose to propose the protocol anyway, 
and explicitly fail the request with a 4xx if the upgrade comes to them.

> 1. Protocols like Websocket need to take over the 101 sending themselves in the "switch protocol" phase. (correct, Jacob?). Should we delegate the sending of the 101 to the protocol switch handler?

It's more that I need to be able to interrupt the upgrade if necessary 
-- whether that's handled by a separate hook, or by delegating the 
upgrade entirely to me, is up for debate. I worry about the duplication 
of the Upgrade code if you delegate that to the modules though... seems 
like core should understand how to upgrade, and handle it for the modules.

In my head the ideal handshake API goes like this:

- Request comes in with a proposed set of protocols to Upgrade to.
- Core uses the propose hook to get a list of protocols that are valid 
upgrades for the current request target. A protocol is chosen.
- The module that won the proposal is given one last chance to check the 
incoming request and fail the upgrade with an immediate HTTP response.
- Otherwise, core sends the 101.
- The switch handler takes over the connection.
- The connection is closed after the switch handler returns.

I am still not sure where the authnz calls fit in, though. h2c and 
tls+http/1.1 can send a 401 after the protocol switch if they want 
(right?), but WebSocket needs to send it before the protocol switch, and 
having to replicate the authnz checks in my switch handler would be a 
pain...

> 4. status code of protocol switch handler: if we move the task of 101 sending, the switch handler might not do it and keep the connection on the "old" protocol. Then a connection close is not necessary. So, we would do the close only when the switch handler returns APR_EOF.

That's an interesting idea. Seems like it would work.

--Jacob

Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 8, 2015 at 12:18 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Dec 8, 2015 at 11:54 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> Am 08.12.2015 um 11:44 schrieb Yann Ylavic <yl...@gmail.com>:
>>>
>>> On Tue, Dec 8, 2015 at 11:07 AM, Stefan Eissing
>>>>
>>>> PS. Re 5: with change 1+4, a TLS upgrade switcher could install an output filter, return and have it being processed normally. The output filter would then send the 101 and do the TLS handshake. Would that work?
>>>
>>> The issue is that this output filter would have to both read and write
>>> during the TLS handshake, not sure this is suitable.
>>
>> Ok, maybe from pre_read_request hook then?
>
> Hmm, pre_read of the second request, interesting.

Actually no, the previous response has to be Upgraded, so for the TLS
case we'd have needed the handshake already.
Also, pre_read has no server_rec selected, so on which configuration
would we base the upgrade on...

> But maybe output filter could work too after all, we'd need to set the
> correct CONN_SENSE for event to do right thing?

Looks like the most suitable option if that can work.

Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 08.12.2015 um 12:18 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> On Tue, Dec 8, 2015 at 11:54 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> 
>> Am 08.12.2015 um 11:44 schrieb Yann Ylavic <yl...@gmail.com>:
>>> 
>>> On Tue, Dec 8, 2015 at 11:07 AM, Stefan Eissing
>>>> 
>>>> PS. Re 5: with change 1+4, a TLS upgrade switcher could install an output filter, return and have it being processed normally. The output filter would then send the 101 and do the TLS handshake. Would that work?
>>> 
>>> The issue is that this output filter would have to both read and write
>>> during the TLS handshake, not sure this is suitable.
>> 
>> Ok, maybe from pre_read_request hook then?
> 
> Hmm, pre_read of the second request, interesting.
> But maybe output filter could work too after all, we'd need to set the
> correct CONN_SENSE for event to do right thing?
> pre_read may be simpler though, based on a flag in the conn_rec?

mod_ssl could have that flag in its SSLConnRec, i would assume...

Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 8, 2015 at 11:54 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> Am 08.12.2015 um 11:44 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> On Tue, Dec 8, 2015 at 11:07 AM, Stefan Eissing
>>>
>>> PS. Re 5: with change 1+4, a TLS upgrade switcher could install an output filter, return and have it being processed normally. The output filter would then send the 101 and do the TLS handshake. Would that work?
>>
>> The issue is that this output filter would have to both read and write
>> during the TLS handshake, not sure this is suitable.
>
> Ok, maybe from pre_read_request hook then?

Hmm, pre_read of the second request, interesting.
But maybe output filter could work too after all, we'd need to set the
correct CONN_SENSE for event to do right thing?
pre_read may be simpler though, based on a flag in the conn_rec?

Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 08.12.2015 um 14:08 schrieb Bert Huijben <be...@qqmail.nl>:
> [...]
> (I'm pretty sure we find new interesting proxy server scenarios :-( )

Not only that. Someone tested h2c against his server from various Tor exit nodes and got a failure rate > 10% because of network middle boxes...

Fun times.




RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: dinsdag 8 december 2015 13:54
> To: dev@httpd.apache.org
> Subject: Re: Upgrade Summary
> 
> I see. Delta-V goodies.
> 
> My proposal therefore is:
> - keep the upgrade/protocol switch mechanism as is for the 2.4.18 release
> - make the agreed upon changes, including TLS upgrade and small content
> h2c upgrades for the next release
> 
> Since the mechanism is already out with 2.4.17, I see no sense in delaying
> 2.4.18 at this point and rather give us time to change it later, so it
works for
> everyone.

Sure... my upgrade requests for Subversion are for 2.4.x future... Certainly
no rush here.

I don't expect this upgrade support to reach Subversion far before 1.10. 

Perhaps we will backport some of this to 1.9, but I don't think we can just
enable H2 support in a minor release without testing on a bigger group
first.
(I'm pretty sure we find new interesting proxy server scenarios :-( )

	Bert


Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
If so, for 2.4.18 we should probably backport Bill's proposal in
STATUS (r1717816), so that OPTIONS works as in 2.4.16...

On Tue, Dec 8, 2015 at 1:54 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
> I see. Delta-V goodies.
>
> My proposal therefore is:
> - keep the upgrade/protocol switch mechanism as is for the 2.4.18 release
> - make the agreed upon changes, including TLS upgrade and small content h2c upgrades for the next release
>
> Since the mechanism is already out with 2.4.17, I see no sense in delaying 2.4.18 at this point and rather give us time to change it later, so it works for everyone.
>
> //Stefan
>
>> Am 08.12.2015 um 13:46 schrieb Bert Huijben <be...@qqmail.nl>:
>>
>>
>>
>>> -----Original Message-----
>>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>>> Sent: dinsdag 8 december 2015 13:22
>>> To: dev@httpd.apache.org
>>> Subject: Re: Upgrade Summary
>>>
>>>
>>>> Am 08.12.2015 um 12:40 schrieb Bert Huijben <be...@qqmail.nl>:
>>>>> -----Original Message-----
>>>>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>>>>> Sent: dinsdag 8 december 2015 11:55
>>>>> To: dev@httpd.apache.org
>>>>> Subject: Re: Upgrade Summary
>>>>>>> [...]3. When to do the upgrade dance:
>>>>>>> a post_read_request: upgrade precedes authentication
>>>>>>
>>>>>> Looks like it can't be RFC compliant, is it?
>>>>>
>>>>> I think it will not be, right.
>>>>
>>>> I read the spec as H2c and HTTP/1.1 are equivalent protocols. Handling
>>>> authentication *after* switching should work just like when not
>> switching.
>>>
>>> It does. We are only talking about upgrade.
>>>
>>>> As client I would like to switch as soon as possible, as at that point I
>> can
>>>> start multiple requests.. potentially with their own auth, using the
>> same or
>>>> different realms; or even different auth schemes.
>>>
>>> Again, your easiest choice is a direct h2c connection. The second easiest
>> is
>>> an initial OPTIONS * request without *request* body. The response may
>>> have
>>> any body length it wants.
>>>
>>> The options * will, as I understand it, not trigger any authentication. In
>>> another mail, you describe that you already send such a request as 1st
>> thing.
>>> But you said it has a body. What does that contain, I wonder?
>>>
>>> If you can live without that request body, we are all fine and have a
>> simple
>>> implementation. If we need to implement upgrades to h2c on some length
>>> request bodies, I personally do not have the time to do that right away
>>> among
>>> all other changes that are being discussed. At least not this week.
>>>
>>> So, what is so relevant about the OPTIONS request body, may I ask?
>>
>> I can't live without that request body. That would just add another request
>> to my handshake... the thing I'm trying to avoid.
>>
>> I'm guessing the body of that initial body is something like
>> [[
>> <D:options xmlns:D="DAV:">
>>   <D:activity-collection-set />
>> </D:options>
>> ]]
>> (content-type text/xml of course)
>>
>> And this request is mostly handled by mod_dav, and further annotated with
>> additional headers by mod_dav_svn.
>>
>> I can't just redesign DAV to optimize for http/2. If we had a timemachine,
>> we would have made other decisions.
>>
>> Would have been nice if we knew DELTA/V was never really implemented outside
>> Subversion. But now we have compatibility guarantees with older Subversion
>> clients/servers and other implementations that may use some of the features.
>>
>>       Bert
>

Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
I see. Delta-V goodies.

My proposal therefore is:
- keep the upgrade/protocol switch mechanism as is for the 2.4.18 release
- make the agreed upon changes, including TLS upgrade and small content h2c upgrades for the next release

Since the mechanism is already out with 2.4.17, I see no sense in delaying 2.4.18 at this point and rather give us time to change it later, so it works for everyone.

//Stefan

> Am 08.12.2015 um 13:46 schrieb Bert Huijben <be...@qqmail.nl>:
> 
> 
> 
>> -----Original Message-----
>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>> Sent: dinsdag 8 december 2015 13:22
>> To: dev@httpd.apache.org
>> Subject: Re: Upgrade Summary
>> 
>> 
>>> Am 08.12.2015 um 12:40 schrieb Bert Huijben <be...@qqmail.nl>:
>>>> -----Original Message-----
>>>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>>>> Sent: dinsdag 8 december 2015 11:55
>>>> To: dev@httpd.apache.org
>>>> Subject: Re: Upgrade Summary
>>>>>> [...]3. When to do the upgrade dance:
>>>>>> a post_read_request: upgrade precedes authentication
>>>>> 
>>>>> Looks like it can't be RFC compliant, is it?
>>>> 
>>>> I think it will not be, right.
>>> 
>>> I read the spec as H2c and HTTP/1.1 are equivalent protocols. Handling
>>> authentication *after* switching should work just like when not
> switching.
>> 
>> It does. We are only talking about upgrade.
>> 
>>> As client I would like to switch as soon as possible, as at that point I
> can
>>> start multiple requests.. potentially with their own auth, using the
> same or
>>> different realms; or even different auth schemes.
>> 
>> Again, your easiest choice is a direct h2c connection. The second easiest
> is
>> an initial OPTIONS * request without *request* body. The response may
>> have
>> any body length it wants.
>> 
>> The options * will, as I understand it, not trigger any authentication. In
>> another mail, you describe that you already send such a request as 1st
> thing.
>> But you said it has a body. What does that contain, I wonder?
>> 
>> If you can live without that request body, we are all fine and have a
> simple
>> implementation. If we need to implement upgrades to h2c on some length
>> request bodies, I personally do not have the time to do that right away
>> among
>> all other changes that are being discussed. At least not this week.
>> 
>> So, what is so relevant about the OPTIONS request body, may I ask?
> 
> I can't live without that request body. That would just add another request
> to my handshake... the thing I'm trying to avoid.
> 
> I'm guessing the body of that initial body is something like
> [[
> <D:options xmlns:D="DAV:">
>   <D:activity-collection-set />
> </D:options>
> ]]
> (content-type text/xml of course)
> 
> And this request is mostly handled by mod_dav, and further annotated with
> additional headers by mod_dav_svn.
> 
> I can't just redesign DAV to optimize for http/2. If we had a timemachine,
> we would have made other decisions.
> 
> Would have been nice if we knew DELTA/V was never really implemented outside
> Subversion. But now we have compatibility guarantees with older Subversion
> clients/servers and other implementations that may use some of the features.
> 
> 	Bert


RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.

> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: dinsdag 8 december 2015 13:22
> To: dev@httpd.apache.org
> Subject: Re: Upgrade Summary
> 
> 
> > Am 08.12.2015 um 12:40 schrieb Bert Huijben <be...@qqmail.nl>:
> >> -----Original Message-----
> >> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> >> Sent: dinsdag 8 december 2015 11:55
> >> To: dev@httpd.apache.org
> >> Subject: Re: Upgrade Summary
> >>>> [...]3. When to do the upgrade dance:
> >>>> a post_read_request: upgrade precedes authentication
> >>>
> >>> Looks like it can't be RFC compliant, is it?
> >>
> >> I think it will not be, right.
> >
> > I read the spec as H2c and HTTP/1.1 are equivalent protocols. Handling
> > authentication *after* switching should work just like when not
switching.
> 
> It does. We are only talking about upgrade.
> 
> > As client I would like to switch as soon as possible, as at that point I
can
> > start multiple requests.. potentially with their own auth, using the
same or
> > different realms; or even different auth schemes.
> 
> Again, your easiest choice is a direct h2c connection. The second easiest
is
> an initial OPTIONS * request without *request* body. The response may
> have
> any body length it wants.
> 
> The options * will, as I understand it, not trigger any authentication. In
> another mail, you describe that you already send such a request as 1st
thing.
> But you said it has a body. What does that contain, I wonder?
> 
> If you can live without that request body, we are all fine and have a
simple
> implementation. If we need to implement upgrades to h2c on some length
> request bodies, I personally do not have the time to do that right away
> among
> all other changes that are being discussed. At least not this week.
> 
> So, what is so relevant about the OPTIONS request body, may I ask?

I can't live without that request body. That would just add another request
to my handshake... the thing I'm trying to avoid.

I'm guessing the body of that initial body is something like
[[
<D:options xmlns:D="DAV:">
   <D:activity-collection-set />
</D:options>
]]
(content-type text/xml of course)

And this request is mostly handled by mod_dav, and further annotated with
additional headers by mod_dav_svn.

I can't just redesign DAV to optimize for http/2. If we had a timemachine,
we would have made other decisions.

Would have been nice if we knew DELTA/V was never really implemented outside
Subversion. But now we have compatibility guarantees with older Subversion
clients/servers and other implementations that may use some of the features.

	Bert


Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 08.12.2015 um 12:40 schrieb Bert Huijben <be...@qqmail.nl>:
>> -----Original Message-----
>> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
>> Sent: dinsdag 8 december 2015 11:55
>> To: dev@httpd.apache.org
>> Subject: Re: Upgrade Summary
>>>> [...]3. When to do the upgrade dance:
>>>> a post_read_request: upgrade precedes authentication
>>> 
>>> Looks like it can't be RFC compliant, is it?
>> 
>> I think it will not be, right.
> 
> I read the spec as H2c and HTTP/1.1 are equivalent protocols. Handling
> authentication *after* switching should work just like when not switching.

It does. We are only talking about upgrade.

> As client I would like to switch as soon as possible, as at that point I can
> start multiple requests.. potentially with their own auth, using the same or
> different realms; or even different auth schemes.

Again, your easiest choice is a direct h2c connection. The second easiest is 
an initial OPTIONS * request without *request* body. The response may have 
any body length it wants. 

The options * will, as I understand it, not trigger any authentication. In
another mail, you describe that you already send such a request as 1st thing.
But you said it has a body. What does that contain, I wonder?

If you can live without that request body, we are all fine and have a simple
implementation. If we need to implement upgrades to h2c on some length
request bodies, I personally do not have the time to do that right away among
all other changes that are being discussed. At least not this week.

So, what is so relevant about the OPTIONS request body, may I ask?

//Stefan

RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: dinsdag 8 december 2015 12:41
> To: dev@httpd.apache.org
> Subject: RE: Upgrade Summary
> 


> I don't think we should require all auth to happen on HTTP/1.1.

If you want to go for equivalent implementations for things like the TLS
upgrade... then you really don't want to have AUTH completed before upgrade.

My preferred implementation would issue the 101 on HTTP/1.1 and then produce
the 401 on H2.
(I would be happy with an EOS on the response headers... I'm not interested
in the body :-))

I can handle '100 continue' before the 101 on HTTP/1.1, but also after
switching to H2 and before the actual result on H2... But I'm happy that we
don't have to support all possible scenarios there.

	Bert


RE: Upgrade Summary

Posted by Bert Huijben <be...@qqmail.nl>.

> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: dinsdag 8 december 2015 11:55
> To: dev@httpd.apache.org
> Subject: Re: Upgrade Summary
> 
> 
> > Am 08.12.2015 um 11:44 schrieb Yann Ylavic <yl...@gmail.com>:
> >
> > On Tue, Dec 8, 2015 at 11:07 AM, Stefan Eissing
> > <st...@greenbytes.de> wrote:
> >> [...]
> >> Open:
> >> 1. Protocols like Websocket need to take over the 101 sending
> themselves in the "switch protocol" phase. (correct, Jacob?). Should we
> delegate the sending of the 101 to the protocol switch handler?
> >> 2. General handling of request bodies. Options:
> >>  a setaside in core of up to nnn bytes before switch invocation
> >>  b do nothing, let protocol switch handler care about it
> >
> > a. is tempting, where nnn would be the limit above which we don't honor
> Upgrade.
> > But that could be a default behaviour, i.e. when "Protocols ... http1"
> > is finally elected.
> > Possibly specific modules would bypass that?
> >
> >> 3. When to do the upgrade dance:
> >>  a post_read_request: upgrade precedes authentication
> >
> > Looks like it can't be RFC compliant, is it?
> 
> I think it will not be, right.

I read the spec as H2c and HTTP/1.1 are equivalent protocols. Handling
authentication *after* switching should work just like when not switching.

As client I would like to switch as soon as possible, as at that point I can
start multiple requests.. potentially with their own auth, using the same or
different realms; or even different auth schemes.


I don't think we should require all auth to happen on HTTP/1.1.

With equivalent protocols we should switch as soon as possible... I don't
think servers should be configured as H2c only for authenticated users.
Especially if they also allow H2direct.

	Bert



Re: Upgrade Summary

Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 08.12.2015 um 11:44 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> On Tue, Dec 8, 2015 at 11:07 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> [...]
>> Open:
>> 1. Protocols like Websocket need to take over the 101 sending themselves in the "switch protocol" phase. (correct, Jacob?). Should we delegate the sending of the 101 to the protocol switch handler?
>> 2. General handling of request bodies. Options:
>>  a setaside in core of up to nnn bytes before switch invocation
>>  b do nothing, let protocol switch handler care about it
> 
> a. is tempting, where nnn would be the limit above which we don't honor Upgrade.
> But that could be a default behaviour, i.e. when "Protocols ... http1"
> is finally elected.
> Possibly specific modules would bypass that?
> 
>> 3. When to do the upgrade dance:
>>  a post_read_request: upgrade precedes authentication
> 
> Looks like it can't be RFC compliant, is it?

I think it will not be, right.

>>  b handler: upgrade only honored on authenticated and otherwise ok requests
> 
> This is probably the right thing to do, would OPTIONS be special is this case?

It currently is insofar as core installs a storage handler to invoke the, otherwise
same, upgrade handling code. It's a special case, but nothing the protocol handlers
need to care about...

>>  c both: introduce separate hooks? have an additional parameter? more complexity
> 
> Depends on whether OPTIONS (others?) need special treatment.

The current design works with it while exposing in the API only one set of hooks. If
those get called from one or the other does not matter. For each request, the hooks
are invoked at most once.

>> 4. status code of protocol switch handler: if we move the task of 101 sending, the switch handler might not do it and keep the connection on the "old" protocol. Then a connection close is not necessary. So, we would do the close only when the switch handler returns APR_EOF.
>> 5. Will it be possible to migrate the current TLS upgrade handling to this revised scheme?
> 
> We must do that IMHO, the scheme should be suite for all the cases.

Agreed.

>> PS. Re 5: with change 1+4, a TLS upgrade switcher could install an output filter, return and have it being processed normally. The output filter would then send the 101 and do the TLS handshake. Would that work?
> 
> The issue is that this output filter would have to both read and write
> during the TLS handshake, not sure this is suitable.

Ok, maybe from pre_read_request hook then?


Re: Upgrade Summary

Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 8, 2015 at 11:07 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
> Trying to summarize the status of the discussion and where the issues are with the current Upgrade implementation.
>
> Clarified:
> A. any 100 must be sent out *before* a 101 response
> B. request bodies are to be read in the original protocol, input filters like chunk can be used, indeed are necessary, as if the request is being processed normally
> C. if a protocol supports upgrade on request bodies is up to the protocol implementation and needs to be checked in the "propose" phase

Agreed.

>
> Open:
> 1. Protocols like Websocket need to take over the 101 sending themselves in the "switch protocol" phase. (correct, Jacob?). Should we delegate the sending of the 101 to the protocol switch handler?
> 2. General handling of request bodies. Options:
>   a setaside in core of up to nnn bytes before switch invocation
>   b do nothing, let protocol switch handler care about it

a. is tempting, where nnn would be the limit above which we don't honor Upgrade.
But that could be a default behaviour, i.e. when "Protocols ... http1"
is finally elected.
Possibly specific modules would bypass that?

> 3. When to do the upgrade dance:
>   a post_read_request: upgrade precedes authentication

Looks like it can't be RFC compliant, is it?

>   b handler: upgrade only honored on authenticated and otherwise ok requests

This is probably the right thing to do, would OPTIONS be special is this case?

>   c both: introduce separate hooks? have an additional parameter? more complexity

Depends on whether OPTIONS (others?) need special treatment.

> 4. status code of protocol switch handler: if we move the task of 101 sending, the switch handler might not do it and keep the connection on the "old" protocol. Then a connection close is not necessary. So, we would do the close only when the switch handler returns APR_EOF.
> 5. Will it be possible to migrate the current TLS upgrade handling to this revised scheme?

We must do that IMHO, the scheme should be suite for all the cases.

>
> PS. Re 5: with change 1+4, a TLS upgrade switcher could install an output filter, return and have it being processed normally. The output filter would then send the 101 and do the TLS handshake. Would that work?

The issue is that this output filter would have to both read and write
during the TLS handshake, not sure this is suitable.