You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2020/08/19 10:08:48 UTC

mod_http2 behavior in case of too many or too large request headers

If mod_http2 detects too many or too large request headers in h2_stream_add_header or h2_stream_end_headers it does not create a
pseudo HTTP/1.1 request but directly responds back on the HTTP/2 stream. While this is very efficient from the HTTP/2 perspective
it has some bad drawbacks from my perspective:

1. The request is not logged in the access log.
2. Possible custom ErrorDocuments for this status code (HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE, 431) are ignored.

I would like to see this changed to have the drawbacks above addressed. Thoughts?

One approach I thought about is that set_error_response would set the http_status in a not yet existing field (e.g. early_status)
of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
I could hack something up for more detailed discussion if this approach seems acceptable.

BTW: Is the check for the field length in h2_stream_end_headers still needed as I think that h2_req_add_header called via
h2_request_add_header by h2_stream_add_header already takes care of it?

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 9/10/20 11:28 AM, Stefan Eissing wrote:
> 
> 
>> Am 10.09.2020 um 11:24 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 9/10/20 9:31 AM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>
>>>>
>>>>
>>>> On 9/9/20 10:21 AM, Stefan Eissing wrote:
>>>>>
>>>>>
>>>>>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/8/20 9:22 AM, Stefan Eissing wrote:

>>
>> That was a quick reply. So the now updated PR is fine for you?
> 
> ;) You got me in the right moment. The PR looks fine.
> 
> Can we merge PRs against httpd on github now? If not, maybe a PR against <https://github.com/icing/mod_h2> would make things easier?

No, but I have tooling in place that makes it easy for me to merge a PR into trunk.
Done as r1881620 and mirrored to Github as https://github.com/apache/httpd/commit/a4fba223668c554e06bc78d6e3a88f33d4238ae4 .
Travis pending.

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 10.09.2020 um 11:24 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> 
> 
> On 9/10/20 9:31 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem <rp...@apache.org>:
>>> 
>>> 
>>> 
>>> On 9/9/20 10:21 AM, Stefan Eissing wrote:
>>>> 
>>>> 
>>>>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>>>>>> 
>>>>>> 
>>>>>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>> 
>>>>>>> Any feedback or comments?
>>>>>> 
>>>>>> Sorry about the delay, my inbox in unhealthy these days.
>>>>> 
>>>>> No problem. Even more thanks then for taking time for a review.
>>>> 
>>>> Thanks for improving this.
>>>>> 
>>>>>> 
>>>>>> Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.
>>>>> 
>>>>> Updated the PR and renamed early_http_status to http_status.
>>>>> What do you mean with / what is the purpose of HTTP_NEEDS_FURTHER_PROCESSING? Should http_status be initialized with this define
>>>>> or do you want to replace conditions of the type (http_status) with (http_status != HTTP_NEEDS_FURTHER_PROCESSING)?
>>>>> At what place should we define it? In h2.h?
>>>> 
>>>> Just a thought that 0 could indicate that the http status has not been determined yet (default) or in case of an early error the code to return. Which then prevents further processing. The name for such a value was not entirely serious. We could just check on != 0.
>>> It is already the case that a value of 0 in http_status indicates that the http status has not been determined yet and 0 is
>>> already the default value via the apr_pcalloc of the structure.
>>> 
>>> Would you like to see the following?
>>> 
>>> 1. Make a define like HTTP_NEEDS_FURTHER_PROCESSING (I would propose H2_HTTP_STATUS_UNSET, sweet naming discussion :-))
>>> 2. In addition to the apr_pcalloc which already makes http_status zero set http_status explicitly to H2_HTTP_STATUS_UNSET.
>>> 3. Replace the (http_status) conditions in the ifs with (http_status != H2_HTTP_STATUS_UNSET)
>> 
>> I like that very much. I think it makes good reading.
> 
> That was a quick reply. So the now updated PR is fine for you?

;) You got me in the right moment. The PR looks fine.

Can we merge PRs against httpd on github now? If not, maybe a PR against <https://github.com/icing/mod_h2> would make things easier?

Cheers, Stefan

> 
> Regards
> 
> Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 9/10/20 9:31 AM, Stefan Eissing wrote:
> 
> 
>> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 9/9/20 10:21 AM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>
>>>>
>>>>
>>>> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>>>>>
>>>>>
>>>>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>>>>> Any feedback or comments?
>>>>>
>>>>> Sorry about the delay, my inbox in unhealthy these days.
>>>>
>>>> No problem. Even more thanks then for taking time for a review.
>>>
>>> Thanks for improving this.
>>>>
>>>>>
>>>>> Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.
>>>>
>>>> Updated the PR and renamed early_http_status to http_status.
>>>> What do you mean with / what is the purpose of HTTP_NEEDS_FURTHER_PROCESSING? Should http_status be initialized with this define
>>>> or do you want to replace conditions of the type (http_status) with (http_status != HTTP_NEEDS_FURTHER_PROCESSING)?
>>>> At what place should we define it? In h2.h?
>>>
>>> Just a thought that 0 could indicate that the http status has not been determined yet (default) or in case of an early error the code to return. Which then prevents further processing. The name for such a value was not entirely serious. We could just check on != 0.
>> It is already the case that a value of 0 in http_status indicates that the http status has not been determined yet and 0 is
>> already the default value via the apr_pcalloc of the structure.
>>
>> Would you like to see the following?
>>
>> 1. Make a define like HTTP_NEEDS_FURTHER_PROCESSING (I would propose H2_HTTP_STATUS_UNSET, sweet naming discussion :-))
>> 2. In addition to the apr_pcalloc which already makes http_status zero set http_status explicitly to H2_HTTP_STATUS_UNSET.
>> 3. Replace the (http_status) conditions in the ifs with (http_status != H2_HTTP_STATUS_UNSET)
> 
> I like that very much. I think it makes good reading.

That was a quick reply. So the now updated PR is fine for you?

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> 
> 
> On 9/9/20 10:21 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem <rp...@apache.org>:
>>> 
>>> 
>>> 
>>> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>>>> 
>>>> 
>>>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>>>>>> 
>>>>>> 
>>>>>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
> 
>>>>> Any feedback or comments?
>>>> 
>>>> Sorry about the delay, my inbox in unhealthy these days.
>>> 
>>> No problem. Even more thanks then for taking time for a review.
>> 
>> Thanks for improving this.
>>> 
>>>> 
>>>> Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.
>>> 
>>> Updated the PR and renamed early_http_status to http_status.
>>> What do you mean with / what is the purpose of HTTP_NEEDS_FURTHER_PROCESSING? Should http_status be initialized with this define
>>> or do you want to replace conditions of the type (http_status) with (http_status != HTTP_NEEDS_FURTHER_PROCESSING)?
>>> At what place should we define it? In h2.h?
>> 
>> Just a thought that 0 could indicate that the http status has not been determined yet (default) or in case of an early error the code to return. Which then prevents further processing. The name for such a value was not entirely serious. We could just check on != 0.
> It is already the case that a value of 0 in http_status indicates that the http status has not been determined yet and 0 is
> already the default value via the apr_pcalloc of the structure.
> 
> Would you like to see the following?
> 
> 1. Make a define like HTTP_NEEDS_FURTHER_PROCESSING (I would propose H2_HTTP_STATUS_UNSET, sweet naming discussion :-))
> 2. In addition to the apr_pcalloc which already makes http_status zero set http_status explicitly to H2_HTTP_STATUS_UNSET.
> 3. Replace the (http_status) conditions in the ifs with (http_status != H2_HTTP_STATUS_UNSET)

I like that very much. I think it makes good reading.

> 
> 
> Regards
> 
> Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 9/9/20 10:21 AM, Stefan Eissing wrote:
> 
> 
>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>
>>>>
>>>>
>>>> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>>>>>
>>>>>
>>>>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>>>>>
>>>>>>
>>>>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:

>>>> Any feedback or comments?
>>>
>>> Sorry about the delay, my inbox in unhealthy these days.
>>
>> No problem. Even more thanks then for taking time for a review.
> 
> Thanks for improving this.
>>
>>>
>>> Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.
>>
>> Updated the PR and renamed early_http_status to http_status.
>> What do you mean with / what is the purpose of HTTP_NEEDS_FURTHER_PROCESSING? Should http_status be initialized with this define
>> or do you want to replace conditions of the type (http_status) with (http_status != HTTP_NEEDS_FURTHER_PROCESSING)?
>> At what place should we define it? In h2.h?
> 
> Just a thought that 0 could indicate that the http status has not been determined yet (default) or in case of an early error the code to return. Which then prevents further processing. The name for such a value was not entirely serious. We could just check on != 0.
It is already the case that a value of 0 in http_status indicates that the http status has not been determined yet and 0 is
already the default value via the apr_pcalloc of the structure.

Would you like to see the following?

1. Make a define like HTTP_NEEDS_FURTHER_PROCESSING (I would propose H2_HTTP_STATUS_UNSET, sweet naming discussion :-))
2. In addition to the apr_pcalloc which already makes http_status zero set http_status explicitly to H2_HTTP_STATUS_UNSET.
3. Replace the (http_status) conditions in the ifs with (http_status != H2_HTTP_STATUS_UNSET)


Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> 
> 
> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
>>> 
>>> 
>>> 
>>> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>>>> 
>>>> 
>>>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>>>> 
>>>>> 
>>>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>> 
>>>> 
>>>>>> 
>>>>>> What about:
>>>>>> 
>>>>>> set_error_response would set the http_status in a not yet existing field (e.g. early_status)
>>>>>> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
>>>>>> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
>>>>>> I could transform this into a patch for more gory discussion if you like.
>>>>> 
>>>>> ;-)
>>>>> 
>>>>> a) without making the complete requests, but a special one that knows how to fail? Go for it!
>>>> 
>>>> How about https://github.com/apache/httpd/pull/137 ?
>>> 
>>> 
>>> Any feedback or comments?
>> 
>> Sorry about the delay, my inbox in unhealthy these days.
> 
> No problem. Even more thanks then for taking time for a review.

Thanks for improving this.
> 
>> 
>> Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.
> 
> Updated the PR and renamed early_http_status to http_status.
> What do you mean with / what is the purpose of HTTP_NEEDS_FURTHER_PROCESSING? Should http_status be initialized with this define
> or do you want to replace conditions of the type (http_status) with (http_status != HTTP_NEEDS_FURTHER_PROCESSING)?
> At what place should we define it? In h2.h?

Just a thought that 0 could indicate that the http status has not been determined yet (default) or in case of an early error the code to return. Which then prevents further processing. The name for such a value was not entirely serious. We could just check on != 0.

Cheers, Stefan
> 
> Regards
> 
> Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 9/8/20 9:22 AM, Stefan Eissing wrote:
> 
> 
>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>>>
>>>
>>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>>>
>>>>
>>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>
>>>>>
>>>>>
>>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>>>
>>>>>>
>>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>
>>>
>>>>>
>>>>> What about:
>>>>>
>>>>> set_error_response would set the http_status in a not yet existing field (e.g. early_status)
>>>>> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
>>>>> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
>>>>> I could transform this into a patch for more gory discussion if you like.
>>>>
>>>> ;-)
>>>>
>>>> a) without making the complete requests, but a special one that knows how to fail? Go for it!
>>>
>>> How about https://github.com/apache/httpd/pull/137 ?
>>
>>
>> Any feedback or comments?
> 
> Sorry about the delay, my inbox in unhealthy these days.

No problem. Even more thanks then for taking time for a review.

> 
> Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.

Updated the PR and renamed early_http_status to http_status.
What do you mean with / what is the purpose of HTTP_NEEDS_FURTHER_PROCESSING? Should http_status be initialized with this define
or do you want to replace conditions of the type (http_status) with (http_status != HTTP_NEEDS_FURTHER_PROCESSING)?
At what place should we define it? In h2.h?

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> 
> 
> On 8/21/20 9:20 AM, Ruediger Pluem wrote:
>> 
>> 
>> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>> 
>>> 
>>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>> 
>>>> 
>>>> 
>>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>> 
>>>>> 
>>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>>> 
>> 
>>>> 
>>>> What about:
>>>> 
>>>> set_error_response would set the http_status in a not yet existing field (e.g. early_status)
>>>> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
>>>> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
>>>> I could transform this into a patch for more gory discussion if you like.
>>> 
>>> ;-)
>>> 
>>> a) without making the complete requests, but a special one that knows how to fail? Go for it!
>> 
>> How about https://github.com/apache/httpd/pull/137 ?
> 
> 
> Any feedback or comments?

Sorry about the delay, my inbox in unhealthy these days.

Had a quick look. My read: it looks like a good approach. The request still needs to be processed in a worker, but that should be very light and fast. I was first confused by the "early_http_status" term as there is the "103 early hints" intermediate response code stuck in my brain. Maybe we should just call it http_status and have a HTTP_NEEDS_FURTHER_PROCESSING (0) in our server.

Stefan

> 
> Regards
> 
> Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 8/21/20 9:20 AM, Ruediger Pluem wrote:
> 
> 
> On 8/20/20 11:38 AM, Stefan Eissing wrote:
>>
>>
>>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>>
>>>
>>>
>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>>
>>>>
>>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>>
>>>>>
>>>>>
>>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>>
>>>>>>
>>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>>
> 
>>>
>>> What about:
>>>
>>> set_error_response would set the http_status in a not yet existing field (e.g. early_status)
>>> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
>>> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
>>> I could transform this into a patch for more gory discussion if you like.
>>
>> ;-)
>>
>> a) without making the complete requests, but a special one that knows how to fail? Go for it!
> 
> How about https://github.com/apache/httpd/pull/137 ?


Any feedback or comments?

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 8/20/20 11:38 AM, Stefan Eissing wrote:
> 
> 
>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>
>>>>
>>>>
>>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>>>
>>>>>
>>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>

>>
>> What about:
>>
>> set_error_response would set the http_status in a not yet existing field (e.g. early_status)
>> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
>> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
>> I could transform this into a patch for more gory discussion if you like.
> 
> ;-)
> 
> a) without making the complete requests, but a special one that knows how to fail? Go for it!

How about https://github.com/apache/httpd/pull/137 ?

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> 
> 
> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>> 
>>> 
>>> 
>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>> 
>>>> 
>>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
> 
>> 
>> Understood. I do not see 2. descending from the heavens either and I myself am unable/unwilling to work on this.
>> 
>> About hacking this somehow, the options I saw:
>> 
>> a) do the header checking once the request_rec is created and scheduled on a secondary connection in a separate thread
>>   - this means we need to take the billions of header lines into out memory first and then schedule a worker to reject it. Seems like begging for a DoS.
>> b) create a request_rec on the main connection, trigger the fail on the main thread and hope that the response can be buffered and will not block the whole h2 handling. Also begging for a DoS.
>> c) rewrite the error response handling in the server, which most likely we will not be able to port back to 2.4
>> 
>> Neither of those is attractive in my eyes. Maybe you see another way?
> 
> What about:
> 
> set_error_response would set the http_status in a not yet existing field (e.g. early_status)
> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
> I could transform this into a patch for more gory discussion if you like.

;-)

a) without making the complete requests, but a special one that knows how to fail? Go for it!

cheers, Stefan

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.


On 8/20/20 10:47 AM, Stefan Eissing wrote:
> 
> 
>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:

> 
> Understood. I do not see 2. descending from the heavens either and I myself am unable/unwilling to work on this.
> 
> About hacking this somehow, the options I saw:
> 
> a) do the header checking once the request_rec is created and scheduled on a secondary connection in a separate thread
>    - this means we need to take the billions of header lines into out memory first and then schedule a worker to reject it. Seems like begging for a DoS.
> b) create a request_rec on the main connection, trigger the fail on the main thread and hope that the response can be buffered and will not block the whole h2 handling. Also begging for a DoS.
> c) rewrite the error response handling in the server, which most likely we will not be able to port back to 2.4
> 
> Neither of those is attractive in my eyes. Maybe you see another way?

What about:

set_error_response would set the http_status in a not yet existing field (e.g. early_status)
of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
I could transform this into a patch for more gory discussion if you like.

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> 
> 
> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>> 
>> 
>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>> 
>>> If mod_http2 detects too many or too large request headers in h2_stream_add_header or h2_stream_end_headers it does not create a
>>> pseudo HTTP/1.1 request but directly responds back on the HTTP/2 stream. While this is very efficient from the HTTP/2 perspective
>>> it has some bad drawbacks from my perspective:
>>> 
>>> 1. The request is not logged in the access log.
>>> 2. Possible custom ErrorDocuments for this status code (HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE, 431) are ignored.
>> 
>> The main reasons (as far as my memory serves) are:
>> - that at that time, the whole h1 infrastructure has not been set up yet.
>> - reporting on requests and error doc handling is only possible from h1
> 
> This is my understanding as well.
> 
>> - it seems counter-intuitive to construct and schedule a h1 request for something that is not valid input
> 
> I understand that from a developer perspective and as said I see the current flow as more efficient. On the other hand from a user
> perspective it is counter-intuitive if the "same" request behaves differently whether it is requested via HTTP/1.1 or via HTTP/2.
> In the first case it is logged and a custom error page if defined is returned, in the second case this does not happen.

I totally agree.

>> 
>>> I would like to see this changed to have the drawbacks above addressed. Thoughts?
>> 
>> Ceterum censeo: to make all features currently tied to h1 work for other protocol, one needs to separate them into a "http" layer that is agnostic to the serialization of requests/responses.
> 
> Agreed, but what does this mean? I think we have two ways forward here:
> 
> 1. We continue to hack around this limitation with better or worse hacks that repeat themselves.
> 2. We stop all changes that would require this layer until this layer is in place.
> 
> While 2. is the cleaner way I doubt that the layer will be availabe any time soon. Hence 2. would mean to stop some further and
> maybe important work on the http2 module. Hence I tend to go for 1., but I am keen for other opinions or further options that I
> missed.

Understood. I do not see 2. descending from the heavens either and I myself am unable/unwilling to work on this.

About hacking this somehow, the options I saw:

a) do the header checking once the request_rec is created and scheduled on a secondary connection in a separate thread
   - this means we need to take the billions of header lines into out memory first and then schedule a worker to reject it. Seems like begging for a DoS.
b) create a request_rec on the main connection, trigger the fail on the main thread and hope that the response can be buffered and will not block the whole h2 handling. Also begging for a DoS.
c) rewrite the error response handling in the server, which most likely we will not be able to port back to 2.4

Neither of those is attractive in my eyes. Maybe you see another way?

- Stefan

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Ruediger Pluem <rp...@apache.org>.

On 8/19/20 12:18 PM, Stefan Eissing wrote:
> 
> 
>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>> If mod_http2 detects too many or too large request headers in h2_stream_add_header or h2_stream_end_headers it does not create a
>> pseudo HTTP/1.1 request but directly responds back on the HTTP/2 stream. While this is very efficient from the HTTP/2 perspective
>> it has some bad drawbacks from my perspective:
>>
>> 1. The request is not logged in the access log.
>> 2. Possible custom ErrorDocuments for this status code (HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE, 431) are ignored.
> 
> The main reasons (as far as my memory serves) are:
> - that at that time, the whole h1 infrastructure has not been set up yet.
> - reporting on requests and error doc handling is only possible from h1

This is my understanding as well.

> - it seems counter-intuitive to construct and schedule a h1 request for something that is not valid input

I understand that from a developer perspective and as said I see the current flow as more efficient. On the other hand from a user
perspective it is counter-intuitive if the "same" request behaves differently whether it is requested via HTTP/1.1 or via HTTP/2.
In the first case it is logged and a custom error page if defined is returned, in the second case this does not happen.

> 
>> I would like to see this changed to have the drawbacks above addressed. Thoughts?
> 
> Ceterum censeo: to make all features currently tied to h1 work for other protocol, one needs to separate them into a "http" layer that is agnostic to the serialization of requests/responses.

Agreed, but what does this mean? I think we have two ways forward here:

1. We continue to hack around this limitation with better or worse hacks that repeat themselves.
2. We stop all changes that would require this layer until this layer is in place.

While 2. is the cleaner way I doubt that the layer will be availabe any time soon. Hence 2. would mean to stop some further and
maybe important work on the http2 module. Hence I tend to go for 1., but I am keen for other opinions or further options that I
missed.

Regards

Rüdiger

Re: mod_http2 behavior in case of too many or too large request headers

Posted by Stefan Eissing <st...@greenbytes.de>.


> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem <rp...@apache.org>:
> 
> If mod_http2 detects too many or too large request headers in h2_stream_add_header or h2_stream_end_headers it does not create a
> pseudo HTTP/1.1 request but directly responds back on the HTTP/2 stream. While this is very efficient from the HTTP/2 perspective
> it has some bad drawbacks from my perspective:
> 
> 1. The request is not logged in the access log.
> 2. Possible custom ErrorDocuments for this status code (HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE, 431) are ignored.

The main reasons (as far as my memory serves) are:
- that at that time, the whole h1 infrastructure has not been set up yet.
- reporting on requests and error doc handling is only possible from h1
- it seems counter-intuitive to construct and schedule a h1 request for something that is not valid input

> I would like to see this changed to have the drawbacks above addressed. Thoughts?

Ceterum censeo: to make all features currently tied to h1 work for other protocol, one needs to separate them into a "http" layer that is agnostic to the serialization of requests/responses.

As another example, see: https://github.com/icing/mod_h2/issues/203

> One approach I thought about is that set_error_response would set the http_status in a not yet existing field (e.g. early_status)
> of the h2_request struct via stream->rtmp instead of creating a brigade in stream->out_buffer and adding to it.
> h2_request_create_rec could then check for this field and if non zero call ap_die after doing some minimal request_rec setup.
> I could hack something up for more detailed discussion if this approach seems acceptable.
> 
> BTW: Is the check for the field length in h2_stream_end_headers still needed as I think that h2_req_add_header called via
> h2_request_add_header by h2_stream_add_header already takes care of it?

Can't say without a closer look which I atm lack the time.

- Stefan
> 
> Regards
> 
> Rüdiger