Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)

[Yann Ylavic]

On Tue, Dec 8, 2015 at 2:37 PM, Yann Ylavic  wrote:
> On Tue, Dec 8, 2015 at 2:30 PM,   wrote:
>> Author: ylavic
>> Date: Tue Dec  8 13:30:30 2015
>> New Revision: 1718595
>>
>> URL: http://svn.apache.org/viewvc?rev=1718595=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):

Actually, since there is already an Upgrade handling above, wouldn't a more
correct patch be (trunk):

Index: modules/ssl/ssl_engine_kernel.c
===
--- modules/ssl/ssl_engine_kernel.c(revision 1718341)
+++ modules/ssl/ssl_engine_kernel.c(working copy)
@@ -230,10 +230,13 @@ int ssl_hook_ReadReq(request_rec *r)

 /* Perform TLS upgrade here if "SSLEngine optional" is configured,
  * SSL is not already set up for this connection, and the client
- * has sent a suitable Upgrade header. */
+ * has sent a suitable Upgrade header. Note this must happen before
+ * map_to_storage and OPTIONS * request processing is completed.
+ */
 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/1.0")
+&& !r->main && !ap_has_request_body(r)) {
 if (upgrade_connection(r)) {
 return AP_FILTER_ERROR;
 }
@@ -246,17 +249,6 @@ int ssl_hook_ReadReq(request_rec *r)
 sslconn = myConnConfig(r->connection->master);
 }

-/* If "SSLEngine optional" is configured, this is not an SSL
- * connection, and this isn't a subrequest, send an Upgrade
- * response header.  Note this must happen before map_to_storage
- * and OPTIONS * request processing is completed.
- */
-if (sc->enabled == SSL_ENABLED_OPTIONAL && !(sslconn && sslconn->ssl)
-&& !r->main) {
-apr_table_setn(r->headers_out, "Upgrade", "TLS/1.0, HTTP/1.1");
-apr_table_mergen(r->headers_out, "Connection", "upgrade");
-}
-
 if (!sslconn) {
 return DECLINED;
 }
?

Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)

[Jim Jagielski]

My only suggestion is that instead of willy-nilly suggesting
patches that will be included in a release, that we actually take
time to think of the correct patch, to implement it and TEST against
it and only THEN have it backported.

Please.

We don't want to put out a semi-immediate 2.4.19 because one
of these last-minute patches broke other stuff. It looks, and
is, really, really bad and amateur.

Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)

[William A Rowe Jr]

On Tue, Dec 8, 2015 at 7:37 AM, Yann Ylavic  wrote:

> On Tue, Dec 8, 2015 at 2:30 PM,   wrote:
> > Author: ylavic
> > Date: Tue Dec  8 13:30:30 2015
> > New Revision: 1718595
> >
> > URL: http://svn.apache.org/viewvc?rev=1718595=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)

[William A Rowe Jr]

On Tue, Dec 8, 2015 at 8:34 AM, Stefan Eissing  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)

[Stefan Eissing]

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 :
> 
> On Tue, Dec 8, 2015 at 2:30 PM,   wrote:
>> Author: ylavic
>> Date: Tue Dec  8 13:30:30 2015
>> New Revision: 1718595
>> 
>> URL: http://svn.apache.org/viewvc?rev=1718595=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)

[Stefan Eissing]

> Am 08.12.2015 um 16:18 schrieb William A Rowe Jr :
> 
> On Tue, Dec 8, 2015 at 8:34 AM, Stefan Eissing  
> 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)

[Stefan Eissing]

+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 :
> 
> On Tue, Dec 8, 2015 at 7:37 AM, Yann Ylavic  wrote:
> On Tue, Dec 8, 2015 at 2:30 PM,   wrote:
> > Author: ylavic
> > Date: Tue Dec  8 13:30:30 2015
> > New Revision: 1718595
> >
> > URL: http://svn.apache.org/viewvc?rev=1718595=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)

[Yann Ylavic]

On Tue, Dec 8, 2015 at 3:17 PM, Jim Jagielski  wrote:
> My only suggestion is that instead of willy-nilly suggesting
> patches that will be included in a release, that we actually take
> time to think of the correct patch, to implement it and TEST against
> it and only THEN have it backported.
>
> Please.

Suggestions have to start somewhere, I did not mean to rush on this,
just expecting feedbacks (including ones like yours, which is indeed
very sensible :)

My point was that if we were backport r1717816 in 2.4.18 (for OPTIONS
to work back), we needed more changes for RFC-compliance wrt TLS/1.x
Upgrades, the one w/o the other is not suitable.

So I think we all agree on the need to think/test more about this ;)

Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)

[Jim Jagielski]

> On Dec 8, 2015, at 11:11 AM, Yann Ylavic  wrote:
> 
> On Tue, Dec 8, 2015 at 3:17 PM, Jim Jagielski  wrote:
>> My only suggestion is that instead of willy-nilly suggesting
>> patches that will be included in a release, that we actually take
>> time to think of the correct patch, to implement it and TEST against
>> it and only THEN have it backported.
>> 
>> Please.
> 
> Suggestions have to start somewhere, I did not mean to rush on this,
> just expecting feedbacks (including ones like yours, which is indeed
> very sensible :)
> 
> My point was that if we were backport r1717816 in 2.4.18 (for OPTIONS
> to work back), we needed more changes for RFC-compliance wrt TLS/1.x
> Upgrades, the one w/o the other is not suitable.
> 
> So I think we all agree on the need to think/test more about this ;)

Oh, agreed 100%.

Re: Upgrade when !ap_request_has_body(r) only for 2.4.18? (was: svn commit: r1718595 - /httpd/httpd/branches/2.4.x/STATUS)

[Jim Jagielski]

So what is current (r1718631) is likely what will be tagged and rolled
and, assuming enuff +1s, released.