On Tue, Dec 8, 2015 at 7:37 AM, Yann Ylavic <[email protected]> wrote:
> On Tue, Dec 8, 2015 at 2:30 PM, <[email protected]> wrote: > > Author: ylavic > > Date: Tue Dec 8 13:30:30 2015 > > New Revision: 1718595 > > > > URL: http://svn.apache.org/viewvc?rev=1718595&view=rev > > Log: > > Comment about ap_request_has_body() check for Upgrade. > > > > Modified: > > httpd/httpd/branches/2.4.x/STATUS > > > [] > > trunk patch: http://svn.apache.org/r1717816 > > +1: wrowe, icing > > + ylavic: how about adding !ap_request_has_body(r) to the test then? > > E.g. (on top of r1717816): > > Index: modules/ssl/ssl_engine_kernel.c > =================================================================== > --- modules/ssl/ssl_engine_kernel.c (revision 1718341) > +++ modules/ssl/ssl_engine_kernel.c (working copy) > @@ -252,7 +252,7 @@ int ssl_hook_ReadReq(request_rec *r) > * and OPTIONS * request processing is completed. > */ > if (sc->enabled == SSL_ENABLED_OPTIONAL && !(sslconn && sslconn->ssl) > - && !r->main) { > + && ap_is_initial_req(r) && !ap_has_request_body(r)) { > apr_table_setn(r->headers_out, "Upgrade", "TLS/1.0, HTTP/1.1"); > apr_table_mergen(r->headers_out, "Connection", "upgrade"); > } > > The problem is that you are disabling *advertising* of the protocol based on the content of a request body. *This* request isn't going to be the target of the user's advertisement, that applies to their *next* request. It doesn't change what I think you meant to change. On the next request, your patch disables the advertising in an OPTIONS * request because you didn't send the first advertisement; that won't work out. You are also disabling per-context advertisement which is allowed by RFC7230. E.g. content under /cups/devices/ may actually have an SSLRequireSSL. Right now the 426 code path relies on advertising headers already being present. We save lots of response bytes on kept-alive SSLEngine optional resources by advertising less frequently as you suggest above, but we can't have an _initial_req toggle without later re-injecting these into all 426 exception paths, per the upgrade required exception's definition. The core Protocols API already suffers this problem, e.g. WebSockets. They may apply to all resources within /webapp/ but not to other resources on the server, and the current API won't present the upgrade header in response to the specific resources desired. Since we have the read-ahead buffering already, I don't see a reason to simply disable it at this time, the patch to switch the sequence of 101 and 100 should be straightforward. Since we aren't converging quite yet on an API fix to solve this multifaceted case, I'll look at the simple fix within mod_ssl alone. Since the logic was valid under RFC2718/2616 I don't think we have to do a hard stop to fix such a bug for RFC7230 here today, but can incorporate such fixes in 2.4.19 soon. Cheers, Bill
