Re: svn commit: r1714751 - /httpd/httpd/trunk/modules/http2/h2_request.c
> Am 17.11.2015 um 13:13 schrieb Plüm, Rüdiger, Vodafone Group > : >> -Ursprüngliche Nachricht- >> Von: Stefan Eissing >> Gesendet: Dienstag, 17. November 2015 12:58 >> An: [email protected] >> Betreff: Re: svn commit: r1714751 - >> /httpd/httpd/trunk/modules/http2/h2_request.c >> >> There is no "Transfer-Encoding" header in HTTP/2. When forwarding the >> request for processing into httpd core, there are currently two paths: >> - H2SerializeHeaders off (default), where request_rec is created >> directly >> and >> - H2SerializeHeaders on, where the slave connection carries a serialized >> HTTP/1.1 request that is parsed and processed as if it where a real >> client connection. >> >> if content-length has not been specified in the HTTP/2 request, the code >> - adds "Transfer-Encoding" if DATA is to be expected (no EOS signaled) >> - adds "Content-Length: 0" if EOS has already been signaled. This is to >> allow the eventual HTTP/1.1 request parsing to know that no data will >> come. Adding this only for requests with "Content-Type" present is >> up for discussion. I want to avoid adding headers to common requests >> that do not normally carry a body, like GET, DELETE, HEAD and others. > > From performance point of view this is quite understandable, but we need to > ensure that we do not open > up the door for request smuggling. By which you mean sneaking in a request inside the body of another one, confusing boundaries? Fair enough. The job of preventing that lies in the HTTP/2 framing layer which also keeps the stream states. Sending any more HEADER/DATA frames for a closed stream is a protocol error and must lead to termination of the connection. If the framing layer does its job, signaling an end-of-stream - means there will not be any more DATA to come - therefore the request input is complete and has 0 length But mistakes are easily made and I appreciated that you're looking critically at this. //Stefan > > Regards > > Rüdiger
Re: svn commit: r1714751 - /httpd/httpd/trunk/modules/http2/h2_request.c
There is no "Transfer-Encoding" header in HTTP/2. When forwarding the
request for processing into httpd core, there are currently two paths:
- H2SerializeHeaders off (default), where request_rec is created directly
and
- H2SerializeHeaders on, where the slave connection carries a serialized
HTTP/1.1 request that is parsed and processed as if it where a real
client connection.
if content-length has not been specified in the HTTP/2 request, the code
- adds "Transfer-Encoding" if DATA is to be expected (no EOS signaled)
- adds "Content-Length: 0" if EOS has already been signaled. This is to
allow the eventual HTTP/1.1 request parsing to know that no data will
come. Adding this only for requests with "Content-Type" present is
up for discussion. I want to avoid adding headers to common requests
that do not normally carry a body, like GET, DELETE, HEAD and others.
//Stefan
> Am 17.11.2015 um 12:21 schrieb Yann Ylavic :
>
> On Tue, Nov 17, 2015 at 11:26 AM, wrote:
>> Author: icing
>> Date: Tue Nov 17 10:26:38 2015
>> New Revision: 1714751
>>
>> URL: http://svn.apache.org/viewvc?rev=1714751&view=rev
>> Log:
>> handling body of chunked requests without content-length and content-type
>> correctly
>>
>> Modified:
>>httpd/httpd/trunk/modules/http2/h2_request.c
>>
>> Modified: httpd/httpd/trunk/modules/http2/h2_request.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1714751&r1=1714750&r2=1714751&view=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_request.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_request.c Tue Nov 17 10:26:38 2015
>> @@ -254,18 +254,21 @@ apr_status_t h2_request_end_headers(h2_r
>> else {
>> /* no content-length given */
>> req->content_length = -1;
>> -s = apr_table_get(req->headers, "Content-Type");
>> -if (eos && s) {
>> -req->chunked = 0;
>> -apr_table_setn(req->headers, "Content-Length", "0");
>> -}
>> -else if (s) {
>> -/* We have not seen a content-length, but a content-type.
>> - * must pass any request content in chunked form.
>> +if (!eos) {
>> +/* We have not seen a content-length and have no eos,
>> + * simulate a chunked encoding for our HTTP/1.1 infrastructure,
>> + * in case we have "H2SerializeHeaders on" here
>> */
>> req->chunked = 1;
>> apr_table_mergen(req->headers, "Transfer-Encoding", "chunked");
>> }
>> +else if (apr_table_get(req->headers, "Content-Type")) {
>> +/* If we have a content-type, but already see eos, no more
>> + * data will come. Signal a zero content length explicitly.
>> + */
>> +req->chunked = 0;
>> +apr_table_setn(req->headers, "Content-Length", "0");
>> +}
>> }
>
> Not sure to understand here, are HTTP2 requirements about "Message
> Body" different from those in HTTP1 (rfc7230, section 3.3)?
> For the latter, Content-Type has no role to play, message bodies are
> solely given by either "Content-Length" or "Transfer-Encoding: [...,]
> chunked" headers.
> Any request (whose method "defines a meaning for an enclosed payload
> body") SHOULD contain one of those, otherwise it is considered to have
> no body.
> IMHO we should be strict about messages boundaries to avoid HTTP
> requests/responses smuggling.
> Did I miss something?
Re: svn commit: r1714751 - /httpd/httpd/trunk/modules/http2/h2_request.c
On Tue, Nov 17, 2015 at 11:26 AM, wrote:
> Author: icing
> Date: Tue Nov 17 10:26:38 2015
> New Revision: 1714751
>
> URL: http://svn.apache.org/viewvc?rev=1714751&view=rev
> Log:
> handling body of chunked requests without content-length and content-type
> correctly
>
> Modified:
> httpd/httpd/trunk/modules/http2/h2_request.c
>
> Modified: httpd/httpd/trunk/modules/http2/h2_request.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1714751&r1=1714750&r2=1714751&view=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_request.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_request.c Tue Nov 17 10:26:38 2015
> @@ -254,18 +254,21 @@ apr_status_t h2_request_end_headers(h2_r
> else {
> /* no content-length given */
> req->content_length = -1;
> -s = apr_table_get(req->headers, "Content-Type");
> -if (eos && s) {
> -req->chunked = 0;
> -apr_table_setn(req->headers, "Content-Length", "0");
> -}
> -else if (s) {
> -/* We have not seen a content-length, but a content-type.
> - * must pass any request content in chunked form.
> +if (!eos) {
> +/* We have not seen a content-length and have no eos,
> + * simulate a chunked encoding for our HTTP/1.1 infrastructure,
> + * in case we have "H2SerializeHeaders on" here
> */
> req->chunked = 1;
> apr_table_mergen(req->headers, "Transfer-Encoding", "chunked");
> }
> +else if (apr_table_get(req->headers, "Content-Type")) {
> +/* If we have a content-type, but already see eos, no more
> + * data will come. Signal a zero content length explicitly.
> + */
> +req->chunked = 0;
> +apr_table_setn(req->headers, "Content-Length", "0");
> +}
> }
Not sure to understand here, are HTTP2 requirements about "Message
Body" different from those in HTTP1 (rfc7230, section 3.3)?
For the latter, Content-Type has no role to play, message bodies are
solely given by either "Content-Length" or "Transfer-Encoding: [...,]
chunked" headers.
Any request (whose method "defines a meaning for an enclosed payload
body") SHOULD contain one of those, otherwise it is considered to have
no body.
IMHO we should be strict about messages boundaries to avoid HTTP
requests/responses smuggling.
Did I miss something?
