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

[Ruediger Pluem]

On 9/10/20 11:28 AM, Stefan Eissing wrote:
> 
> 
>> Am 10.09.2020 um 11:24 schrieb Ruediger Pluem :
>>
>>
>>
>> On 9/10/20 9:31 AM, Stefan Eissing wrote:
>>>
>>>
 Am 10.09.2020 um 09:29 schrieb Ruediger Pluem :



 On 9/9/20 10:21 AM, Stefan Eissing wrote:
>
>
>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem :
>>
>>
>>
>> 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 
>  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

[Stefan Eissing]

> Am 10.09.2020 um 11:24 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/10/20 9:31 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> On 9/9/20 10:21 AM, Stefan Eissing wrote:
 
 
> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> 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 :
>> 
>> 
>> 
>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>> 
>>> 
 Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
 
 
 
 On 8/19/20 12:18 PM, Stefan Eissing wrote:
> 
> 
>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem 
>> :
>>> 
>>> 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 
 would make things easier?

Cheers, Stefan

> 
> Regards
> 
> Rüdiger

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

[Ruediger Pluem]

On 9/10/20 9:31 AM, Stefan Eissing wrote:
> 
> 
>> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem :
>>
>>
>>
>> On 9/9/20 10:21 AM, Stefan Eissing wrote:
>>>
>>>
 Am 08.09.2020 um 21:11 schrieb Ruediger Pluem :



 On 9/8/20 9:22 AM, Stefan Eissing wrote:
>
>
>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :
>>
>>
>>
>> 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 :
>
>
>
> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>
>>
>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>>>
>>>
>>>
>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:


> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>>
>> 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

[Stefan Eissing]

> Am 10.09.2020 um 09:29 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/9/20 10:21 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> On 9/8/20 9:22 AM, Stefan Eissing wrote:
 
 
> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :
> 
> 
> 
> 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 :
 
 
 
 On 8/20/20 10:47 AM, Stefan Eissing wrote:
> 
> 
>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>> 
>> 
>> 
>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>> 
>>> 
 Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
> 
> 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

[Ruediger Pluem]

On 9/9/20 10:21 AM, Stefan Eissing wrote:
> 
> 
>> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem :
>>
>>
>>
>> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>>>
>>>
 Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :



 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 :
>>>
>>>
>>>
>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:


> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>
>
>
> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>
>>
>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :

 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

[Stefan Eissing]

> Am 08.09.2020 um 21:11 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/8/20 9:22 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> 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 :
>> 
>> 
>> 
>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>> 
>>> 
 Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
 
 
 
 On 8/19/20 12:18 PM, Stefan Eissing wrote:
> 
> 
>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>> 
 
>> 
>> 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

[Ruediger Pluem]

On 9/8/20 9:22 AM, Stefan Eissing wrote:
> 
> 
>> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :
>>
>>
>>
>> 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 :
>
>
>
> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>
>>
>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>>>
>>>
>>>
>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:


> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>
>>>
>
> 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

[Stefan Eissing]

> Am 08.09.2020 um 08:27 schrieb Ruediger Pluem :
> 
> 
> 
> 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 :
 
 
 
 On 8/20/20 10:47 AM, Stefan Eissing wrote:
> 
> 
>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>> 
>> 
>> 
>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>> 
>>> 
 Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
 
>> 
 
 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

[Ruediger Pluem]

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 :
>>>
>>>
>>>
>>> On 8/20/20 10:47 AM, Stefan Eissing wrote:


> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>
>
>
> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>>
>>
>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>>>
> 
>>>
>>> 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

[Ruediger Pluem]

On 8/20/20 11:38 AM, Stefan Eissing wrote:
> 
> 
>> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem :
>>
>>
>>
>> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>>>
>>>
 Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :



 On 8/19/20 12:18 PM, Stefan Eissing wrote:
>
>
>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>>

>>
>> 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

[Stefan Eissing]

> Am 20.08.2020 um 11:35 schrieb Ruediger Pluem :
> 
> 
> 
> On 8/20/20 10:47 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> On 8/19/20 12:18 PM, Stefan Eissing wrote:
 
 
> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
> 
>> 
>> 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

[Ruediger Pluem]

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

> 
> 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

[Stefan Eissing]

> Am 20.08.2020 um 10:01 schrieb Ruediger Pluem :
> 
> 
> 
> On 8/19/20 12:18 PM, Stefan Eissing wrote:
>> 
>> 
>>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>>> 
>>> 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

[Ruediger Pluem]

On 8/19/20 12:18 PM, Stefan Eissing wrote:
> 
> 
>> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
>>
>> 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

[Stefan Eissing]

> Am 19.08.2020 um 12:08 schrieb Ruediger Pluem :
> 
> 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