You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@apache.org> on 2015/12/08 14:37:55 UTC
Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn
commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
On Tue, Dec 8, 2015 at 2:30 PM, <yl...@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");
}
--
Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
Posted by Jim Jagielski <ji...@jaguNET.com>.
So what is current (r1718631) is likely what will be tagged and rolled
and, assuming enuff +1s, released.
Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 08.12.2015 um 16:18 schrieb William A Rowe Jr <wr...@rowe-clan.net>:
>
> On Tue, Dec 8, 2015 at 8:34 AM, Stefan Eissing <st...@greenbytes.de> wrote:
> +1 for deferring any upgrade changes that do not fix real issues - like the one proposed for backport by Bill - to 2.4.19
>
> Agreed, as spelled out in my top-post, simplest path to 2.4.18, and these
> interesting discussions over the past day point to no single simple path.
>
> Note that the pre-patch behavior caused tls to overwrite h2c, now the protocol
> API will overwrite tls upgrade advertisements on the first trip through.
>
> Does it make sense to @bug the new Protocol API's stating that these
> remain experimental and still subject to change, and refer prospective
> developer/consumers to dev@httpd? It seems something will change
> in a later 2.4 release, and its simply a matter of what is the appropriate
> straight path that can satisfy all of the prospective upgrade consumers.
+1
Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn
commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Dec 8, 2015 at 8:34 AM, Stefan Eissing <stefan.eissing@greenbytes.de
> wrote:
> +1 for deferring any upgrade changes that do not fix real issues - like
> the one proposed for backport by Bill - to 2.4.19
>
Agreed, as spelled out in my top-post, simplest path to 2.4.18, and these
interesting discussions over the past day point to no single simple path.
Note that the pre-patch behavior caused tls to overwrite h2c, now the
protocol
API will overwrite tls upgrade advertisements on the first trip through.
Does it make sense to @bug the new Protocol API's stating that these
remain experimental and still subject to change, and refer prospective
developer/consumers to dev@httpd? It seems something will change
in a later 2.4 release, and its simply a matter of what is the appropriate
straight path that can satisfy all of the prospective upgrade consumers.
Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
Posted by Stefan Eissing <st...@greenbytes.de>.
+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 <yl...@apache.org> wrote:
> On Tue, Dec 8, 2015 at 2:30 PM, <yl...@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
Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn
commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Dec 8, 2015 at 7:37 AM, Yann Ylavic <yl...@apache.org> wrote:
> On Tue, Dec 8, 2015 at 2:30 PM, <yl...@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
Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)
Posted by Stefan Eissing <st...@greenbytes.de>.
With the current implementation, that seems wise. This restriction can be removed once the change we discussed with output filter/hook is working.
> Am 08.12.2015 um 14:37 schrieb Yann Ylavic <yl...@apache.org>:
>
> On Tue, Dec 8, 2015 at 2:30 PM, <yl...@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");
> }
> --