+1 for deferring any upgrade changes that do not fix real issues - like the one proposed for backport by Bill - to 2.4.19
> Am 08.12.2015 um 15:29 schrieb William A Rowe Jr <wr...@rowe-clan.net>: > > On Tue, Dec 8, 2015 at 7:37 AM, Yann Ylavic <yla...@apache.org> wrote: > On Tue, Dec 8, 2015 at 2:30 PM, <yla...@apache.org> 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
