Mailing List Archive

Broken: apache/httpd#804 (trunk - 97bc128)
Build Update for apache/httpd
-------------------------------------

Build: #804
Status: Broken

Duration: 12 mins and 35 secs
Commit: 97bc128 (trunk)
Author: Ruediger Pluem
Message: * Have the HTTP 0.9 / 1.1 processing code reject requests for
HTTP >= 2.0 with a HTTP Version Not Support status code.


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1878708 13f79535-47bb-0310-9956-ffa450edef68

View the changeset: https://github.com/apache/httpd/compare/75350541f0af...97bc128df241

View the full build log and details: https://travis-ci.org/github/apache/httpd/builds/696809088?utm_medium=notification&utm_source=email

--

You can unsubscribe from build emails from the apache/httpd repository going to https://travis-ci.org/account/preferences/unsubscribe?repository=69847&utm_medium=notification&utm_source=email.
Or unsubscribe from *all* email updating your settings at https://travis-ci.org/account/preferences/unsubscribe?utm_medium=notification&utm_source=email.
Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
Sorry for not testing before and breaking stuff with r1878708. There are basically two failures here:

1. The test failure in t/apache/http_strict.t is because I missed to adjust the test and I think the following would do
(at least it now passes again):

Index: t/apache/http_strict.t
===================================================================
--- t/apache/http_strict.t (revision 1878712)
+++ t/apache/http_strict.t (working copy)
@@ -12,6 +12,10 @@
need_min_apache_fix("2.4.34", "2.5.1") :
need_min_apache_version('2.4.34');

+my $test_unsupported_version = defined(&need_min_apache_fix) ?
+ need_min_apache_fix("2.5.1") :
+ need_min_apache_version('2.5.1');
+
# possible expected results:
# 0: any HTTP error
# 1: any HTTP success
@@ -32,7 +36,7 @@
[ "GET\t/ HTTP/1.0\r\n\r\n" => 400],
[ "GET / HTT/1.0\r\n\r\n" => 0],
[ "GET / HTTP/1.0\r\nHost: localhost\r\n\r\n" => 1],
- [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1],
+ [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1, 505, $test_unsupported_version],
[ "GET / HTTP/1.2\r\nHost: localhost\r\n\r\n" => 1],
[ "GET / HTTP/1.11\r\nHost: localhost\r\n\r\n" => 400],
[ "GET / HTTP/10.0\r\nHost: localhost\r\n\r\n" => 400],


2. The test failures in t/modules/http2.t are caused because in modules/http2/h2_request.c(h2_request_create_rec)
we call ap_parse_request_line with a static 'HTTP/2.0' version string in order to
'Validate HTTP/1 request' as the comment states. In practice this request that we give to ap_parse_request_line
should be a valid HTTP/1.x request as otherwise ap_parse_request_line could fail. So I though of the following patch
below which makes the tests pass again. I will reset r->proto_num and r->protocol afterwards to ensure that for
further processing (e.g. env vars, logging) it is clear that this was actually a HTTP/2.0 request and keep the previous
behavior with regards to this.
Comments or different approaches?

Index: modules/http2/h2_request.c
===================================================================
--- modules/http2/h2_request.c (revision 1878470)
+++ modules/http2/h2_request.c (working copy)
@@ -278,7 +278,11 @@ request_rec *h2_request_create_rec(const h2_reques

/* Time to populate r with the data we have. */
r->request_time = req->request_time;
- r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
+ /*
+ * Use HTTP/1.1 as ap_parse_request_line only deals with
+ * HTTP/1.x requests.
+ */
+ r->the_request = apr_psprintf(r->pool, "%s %s HTTP/1.1",
req->method, req->path ? req->path : "");
r->headers_in = apr_table_clone(r->pool, req->headers);

@@ -293,8 +297,14 @@ request_rec *h2_request_create_rec(const h2_reques
r->per_dir_config = r->server->lookup_defaults;
access_status = r->status;
r->status = HTTP_OK;
+ /* Note that this is actually a HTTP/2.0 request */
+ r->proto_num = HTTP_VERSION(2,0);
+ r->protocol = "HTTP/2.0";
goto die;
}
+ /* Note that this is actually a HTTP/2.0 request */
+ r->proto_num = HTTP_VERSION(2,0);
+ r->protocol = "HTTP/2.0";

/* we may have switched to another server */
r->per_dir_config = r->server->lookup_defaults;


Regards

Rüdiger

On 6/10/20 2:46 PM, Travis CI wrote:
> apache
>
> /
>
> httpd
>
> <https://travis-ci.org/github/apache/httpd?utm_medium=notification&utm_source=email>
>
> branch icontrunk <https://github.com/apache/httpd/tree/trunk>
>
> build has failed
> Build #804 was broken <https://travis-ci.org/github/apache/httpd/builds/696809088?utm_medium=notification&utm_source=email>
> arrow to build time
> clock icon12 mins and 35 secs
>
> Ruediger Pluem avatarRuediger Pluem
>
> 97bc128 CHANGESET ? <https://github.com/apache/httpd/compare/75350541f0af...97bc128df241>
>
> * Have the HTTP 0.9 / 1.1 processing code reject requests for
> HTTP >= 2.0 with a HTTP Version Not Support status code.
>
>
> git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1878708 13f79535-47bb-0310-9956-ffa450edef68
>
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
I would like to unblock the test failure on trunk soon. Any comments on the below?

Regards

Rüdiger

On 6/10/20 4:31 PM, Ruediger Pluem wrote:
> Sorry for not testing before and breaking stuff with r1878708. There are basically two failures here:
>
> 1. The test failure in t/apache/http_strict.t is because I missed to adjust the test and I think the following would do
> (at least it now passes again):
>
> Index: t/apache/http_strict.t
> ===================================================================
> --- t/apache/http_strict.t (revision 1878712)
> +++ t/apache/http_strict.t (working copy)
> @@ -12,6 +12,10 @@
> need_min_apache_fix("2.4.34", "2.5.1") :
> need_min_apache_version('2.4.34');
>
> +my $test_unsupported_version = defined(&need_min_apache_fix) ?
> + need_min_apache_fix("2.5.1") :
> + need_min_apache_version('2.5.1');
> +
> # possible expected results:
> # 0: any HTTP error
> # 1: any HTTP success
> @@ -32,7 +36,7 @@
> [ "GET\t/ HTTP/1.0\r\n\r\n" => 400],
> [ "GET / HTT/1.0\r\n\r\n" => 0],
> [ "GET / HTTP/1.0\r\nHost: localhost\r\n\r\n" => 1],
> - [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1],
> + [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1, 505, $test_unsupported_version],
> [ "GET / HTTP/1.2\r\nHost: localhost\r\n\r\n" => 1],
> [ "GET / HTTP/1.11\r\nHost: localhost\r\n\r\n" => 400],
> [ "GET / HTTP/10.0\r\nHost: localhost\r\n\r\n" => 400],
>
>
> 2. The test failures in t/modules/http2.t are caused because in modules/http2/h2_request.c(h2_request_create_rec)
> we call ap_parse_request_line with a static 'HTTP/2.0' version string in order to
> 'Validate HTTP/1 request' as the comment states. In practice this request that we give to ap_parse_request_line
> should be a valid HTTP/1.x request as otherwise ap_parse_request_line could fail. So I though of the following patch
> below which makes the tests pass again. I will reset r->proto_num and r->protocol afterwards to ensure that for
> further processing (e.g. env vars, logging) it is clear that this was actually a HTTP/2.0 request and keep the previous
> behavior with regards to this.
> Comments or different approaches?
>
> Index: modules/http2/h2_request.c
> ===================================================================
> --- modules/http2/h2_request.c (revision 1878470)
> +++ modules/http2/h2_request.c (working copy)
> @@ -278,7 +278,11 @@ request_rec *h2_request_create_rec(const h2_reques
>
> /* Time to populate r with the data we have. */
> r->request_time = req->request_time;
> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
> + /*
> + * Use HTTP/1.1 as ap_parse_request_line only deals with
> + * HTTP/1.x requests.
> + */
> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/1.1",
> req->method, req->path ? req->path : "");
> r->headers_in = apr_table_clone(r->pool, req->headers);
>
> @@ -293,8 +297,14 @@ request_rec *h2_request_create_rec(const h2_reques
> r->per_dir_config = r->server->lookup_defaults;
> access_status = r->status;
> r->status = HTTP_OK;
> + /* Note that this is actually a HTTP/2.0 request */
> + r->proto_num = HTTP_VERSION(2,0);
> + r->protocol = "HTTP/2.0";
> goto die;
> }
> + /* Note that this is actually a HTTP/2.0 request */
> + r->proto_num = HTTP_VERSION(2,0);
> + r->protocol = "HTTP/2.0";
>
> /* we may have switched to another server */
> r->per_dir_config = r->server->lookup_defaults;
>
>
> Regards
>
> Rüdiger
>
> On 6/10/20 2:46 PM, Travis CI wrote:
>> apache
>>
>> /
>>
>> httpd
>>
>> <https://travis-ci.org/github/apache/httpd?utm_medium=notification&utm_source=email>
>>
>> branch icontrunk <https://github.com/apache/httpd/tree/trunk>
>>
>> build has failed
>> Build #804 was broken <https://travis-ci.org/github/apache/httpd/builds/696809088?utm_medium=notification&utm_source=email>
>> arrow to build time
>> clock icon12 mins and 35 secs
>>
>> Ruediger Pluem avatarRuediger Pluem
>>
>> 97bc128 CHANGESET ? <https://github.com/apache/httpd/compare/75350541f0af...97bc128df241>
>>
>> * Have the HTTP 0.9 / 1.1 processing code reject requests for
>> HTTP >= 2.0 with a HTTP Version Not Support status code.
>>
>>
>> git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1878708 13f79535-47bb-0310-9956-ffa450edef68
>>
>
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <rpluem@apache.org>:
>
> I would like to unblock the test failure on trunk soon. Any comments on the below?

I am not really familiar with ap_parse_request_line() and why it was added there. Yann?

As to "faking" the http version of a request, that does not look good. Do we need to preserve the fairy tale for our code that everything is HTTP/1.x?

- Stefan

> Regards
>
> Rüdiger
>
> On 6/10/20 4:31 PM, Ruediger Pluem wrote:
>> Sorry for not testing before and breaking stuff with r1878708. There are basically two failures here:
>>
>> 1. The test failure in t/apache/http_strict.t is because I missed to adjust the test and I think the following would do
>> (at least it now passes again):
>>
>> Index: t/apache/http_strict.t
>> ===================================================================
>> --- t/apache/http_strict.t (revision 1878712)
>> +++ t/apache/http_strict.t (working copy)
>> @@ -12,6 +12,10 @@
>> need_min_apache_fix("2.4.34", "2.5.1") :
>> need_min_apache_version('2.4.34');
>>
>> +my $test_unsupported_version = defined(&need_min_apache_fix) ?
>> + need_min_apache_fix("2.5.1") :
>> + need_min_apache_version('2.5.1');
>> +
>> # possible expected results:
>> # 0: any HTTP error
>> # 1: any HTTP success
>> @@ -32,7 +36,7 @@
>> [ "GET\t/ HTTP/1.0\r\n\r\n" => 400],
>> [ "GET / HTT/1.0\r\n\r\n" => 0],
>> [ "GET / HTTP/1.0\r\nHost: localhost\r\n\r\n" => 1],
>> - [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1],
>> + [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1, 505, $test_unsupported_version],
>> [ "GET / HTTP/1.2\r\nHost: localhost\r\n\r\n" => 1],
>> [ "GET / HTTP/1.11\r\nHost: localhost\r\n\r\n" => 400],
>> [ "GET / HTTP/10.0\r\nHost: localhost\r\n\r\n" => 400],
>>
>>
>> 2. The test failures in t/modules/http2.t are caused because in modules/http2/h2_request.c(h2_request_create_rec)
>> we call ap_parse_request_line with a static 'HTTP/2.0' version string in order to
>> 'Validate HTTP/1 request' as the comment states. In practice this request that we give to ap_parse_request_line
>> should be a valid HTTP/1.x request as otherwise ap_parse_request_line could fail. So I though of the following patch
>> below which makes the tests pass again. I will reset r->proto_num and r->protocol afterwards to ensure that for
>> further processing (e.g. env vars, logging) it is clear that this was actually a HTTP/2.0 request and keep the previous
>> behavior with regards to this.
>> Comments or different approaches?
>>
>> Index: modules/http2/h2_request.c
>> ===================================================================
>> --- modules/http2/h2_request.c (revision 1878470)
>> +++ modules/http2/h2_request.c (working copy)
>> @@ -278,7 +278,11 @@ request_rec *h2_request_create_rec(const h2_reques
>>
>> /* Time to populate r with the data we have. */
>> r->request_time = req->request_time;
>> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>> + /*
>> + * Use HTTP/1.1 as ap_parse_request_line only deals with
>> + * HTTP/1.x requests.
>> + */
>> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/1.1",
>> req->method, req->path ? req->path : "");
>> r->headers_in = apr_table_clone(r->pool, req->headers);
>>
>> @@ -293,8 +297,14 @@ request_rec *h2_request_create_rec(const h2_reques
>> r->per_dir_config = r->server->lookup_defaults;
>> access_status = r->status;
>> r->status = HTTP_OK;
>> + /* Note that this is actually a HTTP/2.0 request */
>> + r->proto_num = HTTP_VERSION(2,0);
>> + r->protocol = "HTTP/2.0";
>> goto die;
>> }
>> + /* Note that this is actually a HTTP/2.0 request */
>> + r->proto_num = HTTP_VERSION(2,0);
>> + r->protocol = "HTTP/2.0";
>>
>> /* we may have switched to another server */
>> r->per_dir_config = r->server->lookup_defaults;
>>
>>
>> Regards
>>
>> Rüdiger
>>
>> On 6/10/20 2:46 PM, Travis CI wrote:
>>> apache
>>>
>>> /
>>>
>>> httpd
>>>
>>> <https://travis-ci.org/github/apache/httpd?utm_medium=notification&utm_source=email>
>>>
>>> branch icontrunk <https://github.com/apache/httpd/tree/trunk>
>>>
>>> build has failed
>>> Build #804 was broken <https://travis-ci.org/github/apache/httpd/builds/696809088?utm_medium=notification&utm_source=email>
>>> arrow to build time
>>> clock icon12 mins and 35 secs
>>>
>>> Ruediger Pluem avatarRuediger Pluem
>>>
>>> 97bc128 CHANGESET ? <https://github.com/apache/httpd/compare/75350541f0af...97bc128df241>
>>>
>>> * Have the HTTP 0.9 / 1.1 processing code reject requests for
>>> HTTP >= 2.0 with a HTTP Version Not Support status code.
>>>
>>>
>>> git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1878708 13f79535-47bb-0310-9956-ffa450edef68
>>>
>>
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
On 6/16/20 9:29 AM, Stefan Eissing wrote:
>> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <rpluem@apache.org>:
>>
>> I would like to unblock the test failure on trunk soon. Any comments on the below?
>
> I am not really familiar with ap_parse_request_line() and why it was added there. Yann?
>
> As to "faking" the http version of a request, that does not look good. Do we need to preserve the fairy tale for our code that everything is HTTP/1.x?

I think the point is that ap_parse_request_line is only designed to parse <= HTTP/1.x requests, if it should parse >= HTTP/2.0
I guess we need to adjust the API such that we can tell it which major and probably minor version to consider.
But looking at the code again that brought me to the point that I would need to reset r->the_request to

r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, req->path ? req->path : "");

after the ap_parse_request_line() call.
And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
ap_parse_request_line doesn't sound good. OTOH calling ap_parse_request_line makes it possible to apply the same policies to
HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
Keen to see Yann's feedback on this :-)

Regards

Rüdiger

>
> - Stefan
>
>> Regards
>>
>> Rüdiger
>>
>> On 6/10/20 4:31 PM, Ruediger Pluem wrote:
>>> Sorry for not testing before and breaking stuff with r1878708. There are basically two failures here:
>>>
>>> 1. The test failure in t/apache/http_strict.t is because I missed to adjust the test and I think the following would do
>>> (at least it now passes again):
>>>
>>> Index: t/apache/http_strict.t
>>> ===================================================================
>>> --- t/apache/http_strict.t (revision 1878712)
>>> +++ t/apache/http_strict.t (working copy)
>>> @@ -12,6 +12,10 @@
>>> need_min_apache_fix("2.4.34", "2.5.1") :
>>> need_min_apache_version('2.4.34');
>>>
>>> +my $test_unsupported_version = defined(&need_min_apache_fix) ?
>>> + need_min_apache_fix("2.5.1") :
>>> + need_min_apache_version('2.5.1');
>>> +
>>> # possible expected results:
>>> # 0: any HTTP error
>>> # 1: any HTTP success
>>> @@ -32,7 +36,7 @@
>>> [ "GET\t/ HTTP/1.0\r\n\r\n" => 400],
>>> [ "GET / HTT/1.0\r\n\r\n" => 0],
>>> [ "GET / HTTP/1.0\r\nHost: localhost\r\n\r\n" => 1],
>>> - [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1],
>>> + [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1, 505, $test_unsupported_version],
>>> [ "GET / HTTP/1.2\r\nHost: localhost\r\n\r\n" => 1],
>>> [ "GET / HTTP/1.11\r\nHost: localhost\r\n\r\n" => 400],
>>> [ "GET / HTTP/10.0\r\nHost: localhost\r\n\r\n" => 400],
>>>
>>>
>>> 2. The test failures in t/modules/http2.t are caused because in modules/http2/h2_request.c(h2_request_create_rec)
>>> we call ap_parse_request_line with a static 'HTTP/2.0' version string in order to
>>> 'Validate HTTP/1 request' as the comment states. In practice this request that we give to ap_parse_request_line
>>> should be a valid HTTP/1.x request as otherwise ap_parse_request_line could fail. So I though of the following patch
>>> below which makes the tests pass again. I will reset r->proto_num and r->protocol afterwards to ensure that for
>>> further processing (e.g. env vars, logging) it is clear that this was actually a HTTP/2.0 request and keep the previous
>>> behavior with regards to this.
>>> Comments or different approaches?
>>>
>>> Index: modules/http2/h2_request.c
>>> ===================================================================
>>> --- modules/http2/h2_request.c (revision 1878470)
>>> +++ modules/http2/h2_request.c (working copy)
>>> @@ -278,7 +278,11 @@ request_rec *h2_request_create_rec(const h2_reques
>>>
>>> /* Time to populate r with the data we have. */
>>> r->request_time = req->request_time;
>>> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>>> + /*
>>> + * Use HTTP/1.1 as ap_parse_request_line only deals with
>>> + * HTTP/1.x requests.
>>> + */
>>> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/1.1",
>>> req->method, req->path ? req->path : "");
>>> r->headers_in = apr_table_clone(r->pool, req->headers);
>>>
>>> @@ -293,8 +297,14 @@ request_rec *h2_request_create_rec(const h2_reques
>>> r->per_dir_config = r->server->lookup_defaults;
>>> access_status = r->status;
>>> r->status = HTTP_OK;
>>> + /* Note that this is actually a HTTP/2.0 request */
>>> + r->proto_num = HTTP_VERSION(2,0);
>>> + r->protocol = "HTTP/2.0";
>>> goto die;
>>> }
>>> + /* Note that this is actually a HTTP/2.0 request */
>>> + r->proto_num = HTTP_VERSION(2,0);
>>> + r->protocol = "HTTP/2.0";
>>>
>>> /* we may have switched to another server */
>>> r->per_dir_config = r->server->lookup_defaults;
>>>
>>>
>>> Regards
>>>
>>> Rüdiger
>>>
>>> On 6/10/20 2:46 PM, Travis CI wrote:
>>>> apache
>>>>
>>>> /
>>>>
>>>> httpd
>>>>
>>>> <https://travis-ci.org/github/apache/httpd?utm_medium=notification&utm_source=email>
>>>>
>>>> branch icontrunk <https://github.com/apache/httpd/tree/trunk>
>>>>
>>>> build has failed
>>>> Build #804 was broken <https://travis-ci.org/github/apache/httpd/builds/696809088?utm_medium=notification&utm_source=email>
>>>> arrow to build time
>>>> clock icon12 mins and 35 secs
>>>>
>>>> Ruediger Pluem avatarRuediger Pluem
>>>>
>>>> 97bc128 CHANGESET ? <https://github.com/apache/httpd/compare/75350541f0af...97bc128df241>
>>>>
>>>> * Have the HTTP 0.9 / 1.1 processing code reject requests for
>>>> HTTP >= 2.0 with a HTTP Version Not Support status code.
>>>>
>>>>
>>>> git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1878708 13f79535-47bb-0310-9956-ffa450edef68
>>>>
>>>
>
>
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
On Tue, Jun 16, 2020 at 12:08:41PM +0200, Ruediger Pluem wrote:
>
>
> On 6/16/20 9:29 AM, Stefan Eissing wrote:
> >> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <rpluem@apache.org>:
> >>
> >> I would like to unblock the test failure on trunk soon. Any comments on the below?
> >
> > I am not really familiar with ap_parse_request_line() and why it was added there. Yann?
> >
> > As to "faking" the http version of a request, that does not look good. Do we need to preserve the fairy tale for our code that everything is HTTP/1.x?
>
> I think the point is that ap_parse_request_line is only designed to parse <= HTTP/1.x requests, if it should parse >= HTTP/2.0
> I guess we need to adjust the API such that we can tell it which major and probably minor version to consider.
> But looking at the code again that brought me to the point that I would need to reset r->the_request to
>
> r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, req->path ? req->path : "");
>
> after the ap_parse_request_line() call.
> And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
> ap_parse_request_line doesn't sound good. OTOH calling ap_parse_request_line makes it possible to apply the same policies to
> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
> Keen to see Yann's feedback on this :-)

I'm not Yann but my 2c is that ap_parse_request_line() is designed to
safely parse an HTTP/1.x request-line off the wire and probably
shouldn't be used in mod_h2. The complexity of that function is dealing
with all the error cases, which is not useful since none of them will be
hit with HTTP/2.

It looks cheaper - and not too complex - to set the fields of the
request_rec directly as appropriate for HTTP/2 as
ap_parse_request_line() does for HTTP/1.1?

Regards, Joe
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <jorton@redhat.com> wrote:
>
> > And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
> > ap_parse_request_line doesn't sound good.

At this point it isn't an HTTP/2 request IMHO, it's an HTTP/1 request
extracted from an h2 stream.
I suppose that using r->protocol = HTTP/2.0 is for (custom-)logging
purpose, or is it specified? Stefan?

> > OTOH calling ap_parse_request_line makes it possible to apply the same policies to
> > HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
> > Keen to see Yann's feedback on this :-)

I think we should not mark h2 to h1 requests using r->protocol, it's
confusing whereas it somehow should be transparent for h1
core/modules.
If something really needs to know, at worst r->proto_num could do, but
I'd prefer a separate field or notes. If it's only for logging,
possibly we could override that at a later stage/hook?
As seen here, they really are not h2 requests as for h1 core/modules
which actually handle them.

>
> I'm not Yann but my 2c is that ap_parse_request_line() is designed to
> safely parse an HTTP/1.x request-line off the wire and probably
> shouldn't be used in mod_h2. The complexity of that function is dealing
> with all the error cases, which is not useful since none of them will be
> hit with HTTP/2.

I'm not sure, what guarantees otherwise that the method and URI-path
extracted from HTTP/2 (i.e. in h2_request_add_header) are valid?
Do we check that elsewhere?

>
> It looks cheaper - and not too complex - to set the fields of the
> request_rec directly as appropriate for HTTP/2 as
> ap_parse_request_line() does for HTTP/1.1?

Besides r->protocol which we set to a known/valid value, we probably
would have to duplicate all the other checks.
I find it quite sane to reuse the same validation, unless I'm missing
something like h2 to h1 requests are always valid (at this point)...


Regards;
Yann.
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
On 6/17/20 10:43 AM, Joe Orton wrote:
> On Tue, Jun 16, 2020 at 12:08:41PM +0200, Ruediger Pluem wrote:
>>
>>
>> On 6/16/20 9:29 AM, Stefan Eissing wrote:
>>>> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>>
>>>> I would like to unblock the test failure on trunk soon. Any comments on the below?
>>>
>>> I am not really familiar with ap_parse_request_line() and why it was added there. Yann?
>>>
>>> As to "faking" the http version of a request, that does not look good. Do we need to preserve the fairy tale for our code that everything is HTTP/1.x?
>>
>> I think the point is that ap_parse_request_line is only designed to parse <= HTTP/1.x requests, if it should parse >= HTTP/2.0
>> I guess we need to adjust the API such that we can tell it which major and probably minor version to consider.
>> But looking at the code again that brought me to the point that I would need to reset r->the_request to
>>
>> r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, req->path ? req->path : "");
>>
>> after the ap_parse_request_line() call.
>> And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
>> ap_parse_request_line doesn't sound good. OTOH calling ap_parse_request_line makes it possible to apply the same policies to
>> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
>> Keen to see Yann's feedback on this :-)
>
> I'm not Yann but my 2c is that ap_parse_request_line() is designed to

Sorry I didn't want to exclude anyone from this discussion. All comments are very welcome and helpful.

Regards

Rüdiger
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
I just read ap_parse_request_line() again. It seems to consist of 2 parts:
1. the parsing of the text line into protocol/method/uri bits with error checks
2. the check on method/uri correctness in combination with some protocol version specific checks

I think the 2nd part is vital to run for every HTTP version as it concerns itself with "pure" HTTP semantics (which method is known and allowed, which uri values do we accept for processing).

"Faking" a request line just for running the 2nd part, well...it's a bit ugly. Can we not split the method after line 904? If not, I'd rather copy the assignments and applicable checks into mod_h2.

Cheers, Stefan

PS. There is a basic theme here where the server, mainly for historical and understandable reasons, assumes HTTP == HTTP/1.x with some special glue for the honorable HTTP/0.9 ancestor. There is architecture for other protocols, but not for different http protocols. The separation between HTTP semantics and HTTP transports.

Another example is ap_remove_output_filter_byhandle(r->output_filters, "HTTP_HEADER"); in h2_h2.c:#728 where mod_h2 needs to prevent the server of serializing the response headers in HTTP/1.x format. This is ap_http_header_filter() from modules/http/http_filters.c which also does HTTP generic checks (for example permissible headers in a 304 response) *and* puts the response headers onto the output brigade in HTTP/1.1 format.

The first half of that h1 filter is duplicated into h2_filter_headers_out() from h2_from_h1.c#525, submethod create_response(), line 171. I hope they did not drift too much apart over the years.


> Am 17.06.2020 um 10:43 schrieb Joe Orton <jorton@redhat.com>:
>
> On Tue, Jun 16, 2020 at 12:08:41PM +0200, Ruediger Pluem wrote:
>>
>>
>> On 6/16/20 9:29 AM, Stefan Eissing wrote:
>>>> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>>
>>>> I would like to unblock the test failure on trunk soon. Any comments on the below?
>>>
>>> I am not really familiar with ap_parse_request_line() and why it was added there. Yann?
>>>
>>> As to "faking" the http version of a request, that does not look good. Do we need to preserve the fairy tale for our code that everything is HTTP/1.x?
>>
>> I think the point is that ap_parse_request_line is only designed to parse <= HTTP/1.x requests, if it should parse >= HTTP/2.0
>> I guess we need to adjust the API such that we can tell it which major and probably minor version to consider.
>> But looking at the code again that brought me to the point that I would need to reset r->the_request to
>>
>> r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, req->path ? req->path : "");
>>
>> after the ap_parse_request_line() call.
>> And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
>> ap_parse_request_line doesn't sound good. OTOH calling ap_parse_request_line makes it possible to apply the same policies to
>> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
>> Keen to see Yann's feedback on this :-)
>
> I'm not Yann but my 2c is that ap_parse_request_line() is designed to
> safely parse an HTTP/1.x request-line off the wire and probably
> shouldn't be used in mod_h2. The complexity of that function is dealing
> with all the error cases, which is not useful since none of them will be
> hit with HTTP/2.
>
> It looks cheaper - and not too complex - to set the fields of the
> request_rec directly as appropriate for HTTP/2 as
> ap_parse_request_line() does for HTTP/1.1?
>
> Regards, Joe
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
On 6/17/20 11:52 AM, Yann Ylavic wrote:
> On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <jorton@redhat.com> wrote:
>>
>>> And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
>>> ap_parse_request_line doesn't sound good.
>
> At this point it isn't an HTTP/2 request IMHO, it's an HTTP/1 request
> extracted from an h2 stream.
> I suppose that using r->protocol = HTTP/2.0 is for (custom-)logging
> purpose, or is it specified? Stefan?
>
>>> OTOH calling ap_parse_request_line makes it possible to apply the same policies to
>>> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
>>> Keen to see Yann's feedback on this :-)
>
> I think we should not mark h2 to h1 requests using r->protocol, it's
> confusing whereas it somehow should be transparent for h1
> core/modules.
> If something really needs to know, at worst r->proto_num could do, but
> I'd prefer a separate field or notes. If it's only for logging,
> possibly we could override that at a later stage/hook?
> As seen here, they really are not h2 requests as for h1 core/modules
> which actually handle them.
>
>>
>> I'm not Yann but my 2c is that ap_parse_request_line() is designed to
>> safely parse an HTTP/1.x request-line off the wire and probably
>> shouldn't be used in mod_h2. The complexity of that function is dealing
>> with all the error cases, which is not useful since none of them will be
>> hit with HTTP/2.
>
> I'm not sure, what guarantees otherwise that the method and URI-path
> extracted from HTTP/2 (i.e. in h2_request_add_header) are valid?

I think both views are somehow correct. I think some of the checks in ap_parse_request_line() are
not needed for these HTTP/1 requests extracted from a HTTP/2 stream. Others could be quite useful like
correct URI encoding or checking that only registered methods are used and we avoid code duplication
here. So what I can think of here is that we either split ap_parse_request_line even further
in tests useful only for the line from the wire in the HTTP/1 case and the checks useful for the HTTP/2
use case as well and have HTTP/1 use both and HTTP/2 only one or we supply the expected HTTP version
to ap_parse_request_line and behave accordingly.

Regards

Rüdiger
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
On 6/17/20 3:11 PM, Ruediger Pluem wrote:
>
>
> On 6/17/20 11:52 AM, Yann Ylavic wrote:
>> On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <jorton@redhat.com> wrote:
>>>
>>>> And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
>>>> ap_parse_request_line doesn't sound good.
>>
>> At this point it isn't an HTTP/2 request IMHO, it's an HTTP/1 request
>> extracted from an h2 stream.
>> I suppose that using r->protocol = HTTP/2.0 is for (custom-)logging
>> purpose, or is it specified? Stefan?
>>
>>>> OTOH calling ap_parse_request_line makes it possible to apply the same policies to
>>>> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
>>>> Keen to see Yann's feedback on this :-)
>>
>> I think we should not mark h2 to h1 requests using r->protocol, it's
>> confusing whereas it somehow should be transparent for h1
>> core/modules.
>> If something really needs to know, at worst r->proto_num could do, but
>> I'd prefer a separate field or notes. If it's only for logging,
>> possibly we could override that at a later stage/hook?
>> As seen here, they really are not h2 requests as for h1 core/modules
>> which actually handle them.
>>
>>>
>>> I'm not Yann but my 2c is that ap_parse_request_line() is designed to
>>> safely parse an HTTP/1.x request-line off the wire and probably
>>> shouldn't be used in mod_h2. The complexity of that function is dealing
>>> with all the error cases, which is not useful since none of them will be
>>> hit with HTTP/2.
>>
>> I'm not sure, what guarantees otherwise that the method and URI-path
>> extracted from HTTP/2 (i.e. in h2_request_add_header) are valid?
>
> I think both views are somehow correct. I think some of the checks in ap_parse_request_line() are
> not needed for these HTTP/1 requests extracted from a HTTP/2 stream. Others could be quite useful like
> correct URI encoding or checking that only registered methods are used and we avoid code duplication
> here. So what I can think of here is that we either split ap_parse_request_line even further
> in tests useful only for the line from the wire in the HTTP/1 case and the checks useful for the HTTP/2
> use case as well and have HTTP/1 use both and HTTP/2 only one or we supply the expected HTTP version
> to ap_parse_request_line and behave accordingly.

Even with r1878938 and r1878939 now in place and trunk back to green I think no final decision on the
approach is done. But after r1878926 I wanted to fix the shortcomings of my own initial proposal
in order to have a complete approach in trunk. I am happy to go a different way if this is thought to be
better.

Regards

Rüdiger
Re: Broken: apache/httpd#804 (trunk - 97bc128) [ In reply to ]
> Am 17.06.2020 um 21:19 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 6/17/20 3:11 PM, Ruediger Pluem wrote:
>>
>>
>> On 6/17/20 11:52 AM, Yann Ylavic wrote:
>>> On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <jorton@redhat.com> wrote:
>>>>
>>>>> And yes this makes me think if this kind of fake really makes sense. The need to reset 3 request_rec fields after
>>>>> ap_parse_request_line doesn't sound good.
>>>
>>> At this point it isn't an HTTP/2 request IMHO, it's an HTTP/1 request
>>> extracted from an h2 stream.
>>> I suppose that using r->protocol = HTTP/2.0 is for (custom-)logging
>>> purpose, or is it specified? Stefan?
>>>
>>>>> OTOH calling ap_parse_request_line makes it possible to apply the same policies to
>>>>> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change to the API is the way out.
>>>>> Keen to see Yann's feedback on this :-)
>>>
>>> I think we should not mark h2 to h1 requests using r->protocol, it's
>>> confusing whereas it somehow should be transparent for h1
>>> core/modules.
>>> If something really needs to know, at worst r->proto_num could do, but
>>> I'd prefer a separate field or notes. If it's only for logging,
>>> possibly we could override that at a later stage/hook?
>>> As seen here, they really are not h2 requests as for h1 core/modules
>>> which actually handle them.
>>>
>>>>
>>>> I'm not Yann but my 2c is that ap_parse_request_line() is designed to
>>>> safely parse an HTTP/1.x request-line off the wire and probably
>>>> shouldn't be used in mod_h2. The complexity of that function is dealing
>>>> with all the error cases, which is not useful since none of them will be
>>>> hit with HTTP/2.
>>>
>>> I'm not sure, what guarantees otherwise that the method and URI-path
>>> extracted from HTTP/2 (i.e. in h2_request_add_header) are valid?
>>
>> I think both views are somehow correct. I think some of the checks in ap_parse_request_line() are
>> not needed for these HTTP/1 requests extracted from a HTTP/2 stream. Others could be quite useful like
>> correct URI encoding or checking that only registered methods are used and we avoid code duplication
>> here. So what I can think of here is that we either split ap_parse_request_line even further
>> in tests useful only for the line from the wire in the HTTP/1 case and the checks useful for the HTTP/2
>> use case as well and have HTTP/1 use both and HTTP/2 only one or we supply the expected HTTP version
>> to ap_parse_request_line and behave accordingly.
>
> Even with r1878938 and r1878939 now in place and trunk back to green I think no final decision on the
> approach is done. But after r1878926 I wanted to fix the shortcomings of my own initial proposal
> in order to have a complete approach in trunk. I am happy to go a different way if this is thought to be
> better.

Thanks, Rüdiger.

After all this, I think ap_parse_request_line() should not balk at higher HTTP versions. Or being split into a HTTP/1.x part and HTTP.

>
> Regards
>
> Rüdiger