Reject HTTP protocols >= 2.0 in ap_parse_request_line?

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem
I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
in ap_parse_request_line.
This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
and sending a

GET /something HTTP/2.0

as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
A possible patch could look like the following (which rejects such requests with a HTTP_VERSION_NOT_SUPPORTED status code):

Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1878470)
+++ server/protocol.c (working copy)
@@ -748,7 +748,7 @@ AP_DECLARE(int) ap_parse_request_line(request_rec
     enum {
         rrl_none, rrl_badmethod, rrl_badwhitespace, rrl_excesswhitespace,
         rrl_missinguri, rrl_baduri, rrl_badprotocol, rrl_trailingtext,
-        rrl_badmethod09, rrl_reject09
+        rrl_badmethod09, rrl_reject09, rrl_versionnotsupported
     } deferred_error = rrl_none;
     apr_size_t len = 0;
     char *uri, *ll;
@@ -897,6 +897,11 @@ rrl_done:
         r->proto_num = HTTP_VERSION(0, 9);
     }

+    if (strict && deferred_error == rrl_none
+        && r->proto_num >= HTTP_VERSION(2, 0)) {
+        deferred_error = rrl_versionnotsupported;
+    }
+
     /* Determine the method_number and parse the uri prior to invoking error
      * handling, such that these fields are available for substitution
      */
@@ -918,6 +923,7 @@ rrl_done:
      * we can safely resume any deferred error reporting
      */
     if (deferred_error != rrl_none) {
+        r->status = HTTP_BAD_REQUEST;
         if (deferred_error == rrl_badmethod)
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03445)
                           "HTTP Request Line; Invalid method token: '%.*s'",
@@ -954,7 +960,13 @@ rrl_done:
                           "HTTP Request Line; Unrecognized protocol '%.*s' "
                           "(perhaps whitespace was injected?)",
                           field_name_len(r->protocol), r->protocol);
-        r->status = HTTP_BAD_REQUEST;
+        else if (deferred_error == rrl_versionnotsupported) {
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
+                          "HTTP Request Line; Protocol '%.*s' >= HTTP/2.0 not"
+                          " supported", field_name_len(r->protocol),
+                          r->protocol);
+            r->status = HTTP_VERSION_NOT_SUPPORTED;
+        }
         goto rrl_failed;
     }



Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Yann Ylavic
On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
>
> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> in ap_parse_request_line.

Why not >= 1.2 ?

> A possible patch could look like the following (which rejects such requests with a HTTP_VERSION_NOT_SUPPORTED status code):

Looks good.


In the same category, could we reject invalid URI paths earlier
(request-target per RFC-7230 5.3)?
Today it fails in ap_core_translate(), but possibly the below would be better:

Index: server/protocol.c
===================================================================
--- server/protocol.c    (revision 1878328)
+++ server/protocol.c    (working copy)
@@ -627,6 +627,14 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
     }
     else {
         status = apr_uri_parse(r->pool, uri, &r->parsed_uri);
+        if (status == APR_SUCCESS
+            && (r->parsed_uri.path != NULL)
+            && (r->parsed_uri.path[0] != '/')
+            && (r->method_number != M_OPTIONS
+                || strcmp(r->parsed_uri.path, "*") != 0)) {
+            /* Invalid request-target per rfc7230#section-5.3 */
+            status = APR_EINVAL;
+        }
     }

     if (status == APR_SUCCESS) {
--

Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Julian Reschke
On 08.06.2020 16:59, Yann Ylavic wrote:
> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
>>
>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>> in ap_parse_request_line.
>
> Why not >= 1.2 ?

In *theory*, there could a future HTTP/1.2 version that shares the wire
format with 1.0 and 1.1.

> ...

Best regards, Julian
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Yann Ylavic
On Mon, Jun 8, 2020 at 5:43 PM Julian Reschke <[hidden email]> wrote:

>
> On 08.06.2020 16:59, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
> >>
> >> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> >> in ap_parse_request_line.
> >
> > Why not >= 1.2 ?
>
> In *theory*, there could a future HTTP/1.2 version that shares the wire
> format with 1.0 and 1.1.

Sure, but we have quite some time to adapt the check then :)


Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem


On 6/8/20 6:06 PM, Yann Ylavic wrote:

> On Mon, Jun 8, 2020 at 5:43 PM Julian Reschke <[hidden email]> wrote:
>>
>> On 08.06.2020 16:59, Yann Ylavic wrote:
>>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
>>>>
>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>>> in ap_parse_request_line.
>>>
>>> Why not >= 1.2 ?
>>
>> In *theory*, there could a future HTTP/1.2 version that shares the wire
>> format with 1.0 and 1.1.

Exactly this was my reason for not >= 1.2 as this case is IMHO already handled in a compliant way by the current code.
https://tools.ietf.org/html/rfc7230#section-2.6 states:

The interpretation of a header field does not change between minor
   versions of the same major HTTP version, though the default behavior
   of a recipient in the absence of such a field can change.  Unless
   specified otherwise, header fields defined in HTTP/1.1 are defined
   for all versions of HTTP/1.x.  In particular, the Host and Connection
   header fields ought to be implemented by all HTTP/1.x implementations
   whether or not they advertise conformance with HTTP/1.1.

   New header fields can be introduced without changing the protocol
   version if their defined semantics allow them to be safely ignored by
   recipients that do not recognize them.  Header field extensibility is
   discussed in Section 3.2.1.


I interpret this that if we handle a potential HTTP 1.2 request as if

- headers already known in HTTP 1.1 are handled in the same way as if the request was HTTP 1.1
- headers unknown in HTTP 1.2 and added and in HTTP 1.2 are ignored

we are on the safe side if we sent back a HTTP 1.1 response.

Furthermore I get from https://tools.ietf.org/html/rfc7231#section-6.6.6 that
505 indicates that we reject the processing of the major version which would be wrong in the HTTP 1.2 case
as we will process HTTP 1.0 and HTTP 1.1.

Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem
In reply to this post by Yann Ylavic


On 6/8/20 4:59 PM, Yann Ylavic wrote:

> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
>>
>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>> in ap_parse_request_line.
>
> Why not >= 1.2 ?
>
>> A possible patch could look like the following (which rejects such requests with a HTTP_VERSION_NOT_SUPPORTED status code):
>
> Looks good.
>
>
> In the same category, could we reject invalid URI paths earlier
> (request-target per RFC-7230 5.3)?
> Today it fails in ap_core_translate(), but possibly the below would be better:

I think we could, but I am not sure if we have ap_parse_uri callers in other parts of the code that do not pass absolute URI's

>
> Index: server/protocol.c
> ===================================================================
> --- server/protocol.c    (revision 1878328)
> +++ server/protocol.c    (working copy)
> @@ -627,6 +627,14 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
>      }
>      else {
>          status = apr_uri_parse(r->pool, uri, &r->parsed_uri);
> +        if (status == APR_SUCCESS
> +            && (r->parsed_uri.path != NULL)
> +            && (r->parsed_uri.path[0] != '/')
> +            && (r->method_number != M_OPTIONS
> +                || strcmp(r->parsed_uri.path, "*") != 0)) {
> +            /* Invalid request-target per rfc7230#section-5.3 */
> +            status = APR_EINVAL;
> +        }
>      }
>
>      if (status == APR_SUCCESS) {

Don't we miss in  server/protocol.c:

@@ -906,6 +911,12 @@

     ap_parse_uri(r, uri);

+    if (strict && deferred_error == rrl_none
+        && r->status == HTTP_BAD_REQUEST) {
+        deferred_error = rrl_invaliduri
+    }
+
+

Plus adding rrl_invaliduri to the enum and handling later on?

Regards

Rüdiger


Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Yann Ylavic
On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem <[hidden email]> wrote:

>
> On 6/8/20 4:59 PM, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
> >>
> >> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> >> in ap_parse_request_line.
> >
> > Why not >= 1.2 ?
> >
> >> A possible patch could look like the following (which rejects such requests with a HTTP_VERSION_NOT_SUPPORTED status code):
> >
> > Looks good.
> >
> >
> > In the same category, could we reject invalid URI paths earlier
> > (request-target per RFC-7230 5.3)?
> > Today it fails in ap_core_translate(), but possibly the below would be better:
>
> I think we could, but I am not sure if we have ap_parse_uri callers in other parts of the code that do not pass absolute URI's

This patch works with absolute URIs too since apr_parse_uri will split
it in r->parsed_uri and we consider r->parsed_uri.path only.

>
> >
> > Index: server/protocol.c
> > ===================================================================
> > --- server/protocol.c    (revision 1878328)
> > +++ server/protocol.c    (working copy)
> > @@ -627,6 +627,14 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
> >      }
> >      else {
> >          status = apr_uri_parse(r->pool, uri, &r->parsed_uri);
> > +        if (status == APR_SUCCESS
> > +            && (r->parsed_uri.path != NULL)
> > +            && (r->parsed_uri.path[0] != '/')
> > +            && (r->method_number != M_OPTIONS
> > +                || strcmp(r->parsed_uri.path, "*") != 0)) {
> > +            /* Invalid request-target per rfc7230#section-5.3 */
> > +            status = APR_EINVAL;
> > +        }
> >      }
> >
> >      if (status == APR_SUCCESS) {
>
> Don't we miss in  server/protocol.c:
>
> @@ -906,6 +911,12 @@
>
>      ap_parse_uri(r, uri);
>
> +    if (strict && deferred_error == rrl_none
> +        && r->status == HTTP_BAD_REQUEST) {
> +        deferred_error = rrl_invaliduri
> +    }

Somehow r->status != HTTP_OK is caught below all the deferred_error
cases (which we want to report in priority?), and then jumps to the
rrl_failed label.


Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Yann Ylavic
On Mon, Jun 8, 2020 at 10:05 PM Yann Ylavic <[hidden email]> wrote:
>
> On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem <[hidden email]> wrote:
> >
> > I think we could, but I am not sure if we have ap_parse_uri callers in other parts of the code that do not pass absolute URI's
>
> This patch works with absolute URIs too since apr_parse_uri will split
> it in r->parsed_uri and we consider r->parsed_uri.path only.

But to be complete on that side, and handle the proxy case of the
asterisk-form (section 5.3.4 [1]), I think we also need this hunk:

Index: server/protocol.c
===================================================================
--- server/protocol.c    (revision 1878328)
+++ server/protocol.c    (working copy)
@@ -640,8 +648,15 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
         }

         r->args = r->parsed_uri.query;
-        r->uri = r->parsed_uri.path ? r->parsed_uri.path
-                 : apr_pstrdup(r->pool, "/");
+        if (r->parsed_uri.path) {
+            r->uri = r->parsed_uri.path;
+        }
+        else if (r->method_number == M_OPTIONS) {
+            r->uri = apr_pstrdup(r->pool, "*");
+        }
+        else {
+            r->uri = apr_pstrdup(r->pool, "/");
+        }

 #if defined(OS2) || defined(WIN32)
         /* Handle path translations for OS/2 and plug security hole.
--

[1] https://tools.ietf.org/html/rfc7230#section-5.3.4
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem
In reply to this post by Yann Ylavic


On 6/8/20 10:05 PM, Yann Ylavic wrote:

> On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem <[hidden email]> wrote:
>>
>> On 6/8/20 4:59 PM, Yann Ylavic wrote:
>>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
>>>>
>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>>> in ap_parse_request_line.
>>>
>>> Why not >= 1.2 ?
>>>
>>>> A possible patch could look like the following (which rejects such requests with a HTTP_VERSION_NOT_SUPPORTED status code):
>>>
>>> Looks good.
>>>
>>>
>>> In the same category, could we reject invalid URI paths earlier
>>> (request-target per RFC-7230 5.3)?
>>> Today it fails in ap_core_translate(), but possibly the below would be better:
>>
>> I think we could, but I am not sure if we have ap_parse_uri callers in other parts of the code that do not pass absolute URI's
>
> This patch works with absolute URIs too since apr_parse_uri will split
> it in r->parsed_uri and we consider r->parsed_uri.path only.

I guess I was not precise enough with my concern. I meant that I haven't checked if there are callers which pass in relative URI's.

>
>>
>>>
>>> Index: server/protocol.c
>>> ===================================================================
>>> --- server/protocol.c    (revision 1878328)
>>> +++ server/protocol.c    (working copy)
>>> @@ -627,6 +627,14 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
>>>      }
>>>      else {
>>>          status = apr_uri_parse(r->pool, uri, &r->parsed_uri);
>>> +        if (status == APR_SUCCESS
>>> +            && (r->parsed_uri.path != NULL)
>>> +            && (r->parsed_uri.path[0] != '/')
>>> +            && (r->method_number != M_OPTIONS
>>> +                || strcmp(r->parsed_uri.path, "*") != 0)) {
>>> +            /* Invalid request-target per rfc7230#section-5.3 */
>>> +            status = APR_EINVAL;
>>> +        }
>>>      }
>>>
>>>      if (status == APR_SUCCESS) {
>>
>> Don't we miss in  server/protocol.c:
>>
>> @@ -906,6 +911,12 @@
>>
>>      ap_parse_uri(r, uri);
>>
>> +    if (strict && deferred_error == rrl_none
>> +        && r->status == HTTP_BAD_REQUEST) {
>> +        deferred_error = rrl_invaliduri
>> +    }
>
> Somehow r->status != HTTP_OK is caught below all the deferred_error
> cases (which we want to report in priority?), and then jumps to the
> rrl_failed label.

Fair enough :-) I missed this one.

Regards

Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Yann Ylavic
On Mon, Jun 8, 2020 at 10:12 PM Ruediger Pluem <[hidden email]> wrote:

>
> On 6/8/20 10:05 PM, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem <[hidden email]> wrote:
> >>
> >> On 6/8/20 4:59 PM, Yann Ylavic wrote:
> >>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
> >>>>
> >>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> >>>> in ap_parse_request_line.
> >>>
> >>> Why not >= 1.2 ?
> >>>
> >>>> A possible patch could look like the following (which rejects such requests with a HTTP_VERSION_NOT_SUPPORTED status code):
> >>>
> >>> Looks good.
> >>>
> >>>
> >>> In the same category, could we reject invalid URI paths earlier
> >>> (request-target per RFC-7230 5.3)?
> >>> Today it fails in ap_core_translate(), but possibly the below would be better:
> >>
> >> I think we could, but I am not sure if we have ap_parse_uri callers in other parts of the code that do not pass absolute URI's
> >
> > This patch works with absolute URIs too since apr_parse_uri will split
> > it in r->parsed_uri and we consider r->parsed_uri.path only.
>
> I guess I was not precise enough with my concern. I meant that I haven't checked if there are callers which pass in relative URI's.

Oh no, actually _I_ missed the "do _not_ pass" :)

I don't think ap_parse_uri() should accept an URI-path which is not
HTTP compliant (i.e. does not start with '/'), that's where we
initialize the request_rec after all.. So yes apr_uri_parse() accepts
that and sets a relative path in r->parsed_uri.path, but
ap_parse_uri() if for HTTP parsing IMHO.

Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Yann Ylavic
In reply to this post by Ruediger Pluem
On Mon, Jun 8, 2020 at 8:38 PM Ruediger Pluem <[hidden email]> wrote:

>
> On 6/8/20 6:06 PM, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 5:43 PM Julian Reschke <[hidden email]> wrote:
> >>
> >> On 08.06.2020 16:59, Yann Ylavic wrote:
> >>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <[hidden email]> wrote:
> >>>>
> >>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> >>>> in ap_parse_request_line.
> >>>
> >>> Why not >= 1.2 ?
> >>
> >> In *theory*, there could a future HTTP/1.2 version that shares the wire
> >> format with 1.0 and 1.1.
>
> Exactly this was my reason for not >= 1.2 as this case is IMHO already handled in a compliant way by the current code.
> https://tools.ietf.org/html/rfc7230#section-2.6 states:
>
> The interpretation of a header field does not change between minor
>    versions of the same major HTTP version, though the default behavior
>    of a recipient in the absence of such a field can change.  Unless
>    specified otherwise, header fields defined in HTTP/1.1 are defined
>    for all versions of HTTP/1.x.  In particular, the Host and Connection
>    header fields ought to be implemented by all HTTP/1.x implementations
>    whether or not they advertise conformance with HTTP/1.1.
>
>    New header fields can be introduced without changing the protocol
>    version if their defined semantics allow them to be safely ignored by
>    recipients that do not recognize them.  Header field extensibility is
>    discussed in Section 3.2.1.
>
>
> I interpret this that if we handle a potential HTTP 1.2 request as if
>
> - headers already known in HTTP 1.1 are handled in the same way as if the request was HTTP 1.1
> - headers unknown in HTTP 1.2 and added and in HTTP 1.2 are ignored
>
> we are on the safe side if we sent back a HTTP 1.1 response.
>
> Furthermore I get from https://tools.ietf.org/html/rfc7231#section-6.6.6 that
> 505 indicates that we reject the processing of the major version which would be wrong in the HTTP 1.2 case
> as we will process HTTP 1.0 and HTTP 1.1.

Fair enough ;)

Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Roy T. Fielding
In reply to this post by Ruediger Pluem
> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>
> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> in ap_parse_request_line.
> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
> and sending a
>
> GET /something HTTP/2.0
>
> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.

That isn't how these things typically work. New protocols are
advanced with either deliberate backwards-compat or deliberate
backwards-break, with an expectation that it will either do
something useful on an older-protocol server or cause a safe
error in an expected way.

Hence, we might still see an HTTP/4.0 that is designed to be
parsed like HTTP/1.1 (by an old server) while at the same time
work perfectly for a new server. That would be some hefty magic,
but it remains possible. Likewise, we might want to deploy a
version of h2 or HTTP/3 that works on unix domain sockets or
localhost over non-Internet TCP.

This is why the existing code did not error on protocols >= 2.0.
Doing so is both unnecessary and counterproductive. If parsing
fails for some other reason, we want that other reason to be
in the response (because that's what the new protocol will be
expecting from an old protocol server). If it doesn't fail, we
want to provide the successful response because the request
was deliberately crafted that way to save a round trip.

Note that the incoming request protocol version should always
be distinct from any forwarded request protocol or response
protocol versions.

....Roy

Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem


On 6/18/20 12:09 AM, Roy T. Fielding wrote:

>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>>
>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>> in ap_parse_request_line.
>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
>> and sending a
>>
>> GET /something HTTP/2.0
>>
>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
>
> That isn't how these things typically work. New protocols are
> advanced with either deliberate backwards-compat or deliberate
> backwards-break, with an expectation that it will either do
> something useful on an older-protocol server or cause a safe
> error in an expected way.
>
> Hence, we might still see an HTTP/4.0 that is designed to be
> parsed like HTTP/1.1 (by an old server) while at the same time
> work perfectly for a new server. That would be some hefty magic,
> but it remains possible. Likewise, we might want to deploy a
> version of h2 or HTTP/3 that works on unix domain sockets or
> localhost over non-Internet TCP.
>
> This is why the existing code did not error on protocols >= 2.0.
> Doing so is both unnecessary and counterproductive. If parsing
> fails for some other reason, we want that other reason to be
> in the response (because that's what the new protocol will be
> expecting from an old protocol server). If it doesn't fail, we
> want to provide the successful response because the request
> was deliberately crafted that way to save a round trip.
>
> Note that the incoming request protocol version should always
> be distinct from any forwarded request protocol or response
> protocol versions.

As always many thanks for the insights. I see two ways forward now:

1. Roll back r1878708 and all the follow ups and be done.
2. Keep r1878708 and all the follow ups but adjust the code in ap_parse_request_line
   to be more specific when to sent a 505. Given today's state I think a 505 would still be
   correct in case of

   1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an implementation of
      HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
   2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has that
      deliberate backwards-break as I understand it. OTOH you could argue that with
      mod_http2 present 505 is the wrong reply since the server supports HTTP/2 but not the
      way it was tried and hence another code would be the correct response (400 ??) if we see a
      HTTP/1.x request with Protocol HTTP/2.0.

Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Stefan Eissing

Stefan Eissing

<green/>bytes GmbH
Hafenweg 16
48155 Münster
www.greenbytes.de

> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem <[hidden email]>:
>
>
>
> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>>>
>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>> in ap_parse_request_line.
>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
>>> and sending a
>>>
>>> GET /something HTTP/2.0
>>>
>>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
>>
>> That isn't how these things typically work. New protocols are
>> advanced with either deliberate backwards-compat or deliberate
>> backwards-break, with an expectation that it will either do
>> something useful on an older-protocol server or cause a safe
>> error in an expected way.
>>
>> Hence, we might still see an HTTP/4.0 that is designed to be
>> parsed like HTTP/1.1 (by an old server) while at the same time
>> work perfectly for a new server. That would be some hefty magic,
>> but it remains possible. Likewise, we might want to deploy a
>> version of h2 or HTTP/3 that works on unix domain sockets or
>> localhost over non-Internet TCP.
>>
>> This is why the existing code did not error on protocols >= 2.0.
>> Doing so is both unnecessary and counterproductive. If parsing
>> fails for some other reason, we want that other reason to be
>> in the response (because that's what the new protocol will be
>> expecting from an old protocol server). If it doesn't fail, we
>> want to provide the successful response because the request
>> was deliberately crafted that way to save a round trip.
>>
>> Note that the incoming request protocol version should always
>> be distinct from any forwarded request protocol or response
>> protocol versions.
>
> As always many thanks for the insights. I see two ways forward now:
>
> 1. Roll back r1878708 and all the follow ups and be done.

+1

If we want the HTTP/1.x protocol handler to balk at higher protocol versions, this
check should be done in ap_read_request() (which should be placed in modules/http
(which should then be named modules/http1)).


> 2. Keep r1878708 and all the follow ups but adjust the code in ap_parse_request_line
>   to be more specific when to sent a 505. Given today's state I think a 505 would still be
>   correct in case of

>
>   1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an implementation of
>      HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
>   2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has that
>      deliberate backwards-break as I understand it. OTOH you could argue that with
>      mod_http2 present 505 is the wrong reply since the server supports HTTP/2 but not the
>      way it was tried and hence another code would be the correct response (400 ??) if we see a
>      HTTP/1.x request with Protocol HTTP/2.0.
>
> Regards
>
> Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem


On 6/18/20 10:37 AM, Stefan Eissing wrote:

>
> Stefan Eissing
>
> <green/>bytes GmbH
> Hafenweg 16
> 48155 Münster
> www.greenbytes.de
>
>> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem <[hidden email]>:
>>
>>
>>
>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>>>>
>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>>> in ap_parse_request_line.
>>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
>>>> and sending a
>>>>
>>>> GET /something HTTP/2.0
>>>>
>>>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
>>>
>>> That isn't how these things typically work. New protocols are
>>> advanced with either deliberate backwards-compat or deliberate
>>> backwards-break, with an expectation that it will either do
>>> something useful on an older-protocol server or cause a safe
>>> error in an expected way.
>>>
>>> Hence, we might still see an HTTP/4.0 that is designed to be
>>> parsed like HTTP/1.1 (by an old server) while at the same time
>>> work perfectly for a new server. That would be some hefty magic,
>>> but it remains possible. Likewise, we might want to deploy a
>>> version of h2 or HTTP/3 that works on unix domain sockets or
>>> localhost over non-Internet TCP.
>>>
>>> This is why the existing code did not error on protocols >= 2.0.
>>> Doing so is both unnecessary and counterproductive. If parsing
>>> fails for some other reason, we want that other reason to be
>>> in the response (because that's what the new protocol will be
>>> expecting from an old protocol server). If it doesn't fail, we
>>> want to provide the successful response because the request
>>> was deliberately crafted that way to save a round trip.
>>>
>>> Note that the incoming request protocol version should always
>>> be distinct from any forwarded request protocol or response
>>> protocol versions.
>>
>> As always many thanks for the insights. I see two ways forward now:
>>
>> 1. Roll back r1878708 and all the follow ups and be done.
>
> +1
>
> If we want the HTTP/1.x protocol handler to balk at higher protocol versions, this
> check should be done in ap_read_request() (which should be placed in modules/http

This is an interesting proposal, but I think that the two points I mentioned below in 2.
would apply as well if the check is done there.

> (which should then be named modules/http1)).

Given Roy's comments above I think that at least in theory the stuff in modules/http could
still be used for protocols > HTTP/2.x. So I am not sure if this rename is justified.

>
>
>> 2. Keep r1878708 and all the follow ups but adjust the code in ap_parse_request_line
>>   to be more specific when to sent a 505. Given today's state I think a 505 would still be
>>   correct in case of
>
>>
>>   1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an implementation of
>>      HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
>>   2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has that
>>      deliberate backwards-break as I understand it. OTOH you could argue that with
>>      mod_http2 present 505 is the wrong reply since the server supports HTTP/2 but not the
>>      way it was tried and hence another code would be the correct response (400 ??) if we see a
>>      HTTP/1.x request with Protocol HTTP/2.0.
>>

Regards

Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Stefan Eissing
> Am 18.06.2020 um 11:49 schrieb Ruediger Pluem <[hidden email]>:
>
>
>
> On 6/18/20 10:37 AM, Stefan Eissing wrote:
>>
>> Stefan Eissing
>>
>> <green/>bytes GmbH
>> Hafenweg 16
>> 48155 Münster
>> www.greenbytes.de
>>
>>> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem <[hidden email]>:
>>>
>>>
>>>
>>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>>>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>>>>>
>>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>>>> in ap_parse_request_line.
>>>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
>>>>> and sending a
>>>>>
>>>>> GET /something HTTP/2.0
>>>>>
>>>>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
>>>>
>>>> That isn't how these things typically work. New protocols are
>>>> advanced with either deliberate backwards-compat or deliberate
>>>> backwards-break, with an expectation that it will either do
>>>> something useful on an older-protocol server or cause a safe
>>>> error in an expected way.
>>>>
>>>> Hence, we might still see an HTTP/4.0 that is designed to be
>>>> parsed like HTTP/1.1 (by an old server) while at the same time
>>>> work perfectly for a new server. That would be some hefty magic,
>>>> but it remains possible. Likewise, we might want to deploy a
>>>> version of h2 or HTTP/3 that works on unix domain sockets or
>>>> localhost over non-Internet TCP.
>>>>
>>>> This is why the existing code did not error on protocols >= 2.0.
>>>> Doing so is both unnecessary and counterproductive. If parsing
>>>> fails for some other reason, we want that other reason to be
>>>> in the response (because that's what the new protocol will be
>>>> expecting from an old protocol server). If it doesn't fail, we
>>>> want to provide the successful response because the request
>>>> was deliberately crafted that way to save a round trip.
>>>>
>>>> Note that the incoming request protocol version should always
>>>> be distinct from any forwarded request protocol or response
>>>> protocol versions.
>>>
>>> As always many thanks for the insights. I see two ways forward now:
>>>
>>> 1. Roll back r1878708 and all the follow ups and be done.
>>
>> +1
>>
>> If we want the HTTP/1.x protocol handler to balk at higher protocol versions, this
>> check should be done in ap_read_request() (which should be placed in modules/http
>
> This is an interesting proposal, but I think that the two points I mentioned below in 2.
> would apply as well if the check is done there.
>
>> (which should then be named modules/http1)).
>
> Given Roy's comments above I think that at least in theory the stuff in modules/http could
> still be used for protocols > HTTP/2.x. So I am not sure if this rename is justified.

The pre-condition was that *if* modules/http code refuses processing >= HTTP/2.0, it should be named modules/http1.

But I agree that there is mostly stuff in there that we want to use for *any* http request.

>>
>>
>>> 2. Keep r1878708 and all the follow ups but adjust the code in ap_parse_request_line
>>>  to be more specific when to sent a 505. Given today's state I think a 505 would still be
>>>  correct in case of
>>
>>>
>>>  1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an implementation of
>>>     HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
>>>  2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has that
>>>     deliberate backwards-break as I understand it. OTOH you could argue that with
>>>     mod_http2 present 505 is the wrong reply since the server supports HTTP/2 but not the
>>>     way it was tried and hence another code would be the correct response (400 ??) if we see a
>>>     HTTP/1.x request with Protocol HTTP/2.0.
>>>
>
> Regards
>
> Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Ruediger Pluem


On 6/18/20 12:21 PM, Stefan Eissing wrote:

>> Am 18.06.2020 um 11:49 schrieb Ruediger Pluem <[hidden email]>:
>>
>>
>>
>> On 6/18/20 10:37 AM, Stefan Eissing wrote:
>>>
>>> Stefan Eissing
>>>
>>> <green/>bytes GmbH
>>> Hafenweg 16
>>> 48155 Münster
>>> www.greenbytes.de
>>>
>>>> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem <[hidden email]>:
>>>>
>>>>
>>>>
>>>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>>>>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>>>>>>
>>>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>>>>> in ap_parse_request_line.
>>>>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
>>>>>> and sending a
>>>>>>
>>>>>> GET /something HTTP/2.0
>>>>>>
>>>>>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
>>>>>
>>>>> That isn't how these things typically work. New protocols are
>>>>> advanced with either deliberate backwards-compat or deliberate
>>>>> backwards-break, with an expectation that it will either do
>>>>> something useful on an older-protocol server or cause a safe
>>>>> error in an expected way.
>>>>>
>>>>> Hence, we might still see an HTTP/4.0 that is designed to be
>>>>> parsed like HTTP/1.1 (by an old server) while at the same time
>>>>> work perfectly for a new server. That would be some hefty magic,
>>>>> but it remains possible. Likewise, we might want to deploy a
>>>>> version of h2 or HTTP/3 that works on unix domain sockets or
>>>>> localhost over non-Internet TCP.
>>>>>
>>>>> This is why the existing code did not error on protocols >= 2.0.
>>>>> Doing so is both unnecessary and counterproductive. If parsing
>>>>> fails for some other reason, we want that other reason to be
>>>>> in the response (because that's what the new protocol will be
>>>>> expecting from an old protocol server). If it doesn't fail, we
>>>>> want to provide the successful response because the request
>>>>> was deliberately crafted that way to save a round trip.
>>>>>
>>>>> Note that the incoming request protocol version should always
>>>>> be distinct from any forwarded request protocol or response
>>>>> protocol versions.
>>>>
>>>> As always many thanks for the insights. I see two ways forward now:
>>>>
>>>> 1. Roll back r1878708 and all the follow ups and be done.
>>>
>>> +1
>>>
>>> If we want the HTTP/1.x protocol handler to balk at higher protocol versions, this
>>> check should be done in ap_read_request() (which should be placed in modules/http
>>
>> This is an interesting proposal, but I think that the two points I mentioned below in 2.
>> would apply as well if the check is done there.
>>
>>> (which should then be named modules/http1)).
>>
>> Given Roy's comments above I think that at least in theory the stuff in modules/http could
>> still be used for protocols > HTTP/2.x. So I am not sure if this rename is justified.
>
> The pre-condition was that *if* modules/http code refuses processing >= HTTP/2.0, it should be named modules/http1.

Fair enough. I somehow missed this if. I will wait how this discussion evolves to see what next steps are the best ones.

Regards

Rüdiger


Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

William A Rowe Jr
 
>>>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>>>>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
>>>>>>
>>>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
>>>>>> in ap_parse_request_line.
>>>>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
>>>>>> and sending a
>>>>>>
>>>>>> GET /something HTTP/2.0
>>>>>>
>>>>>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.

Correct, it starts an HTTP/1.1 connection, and the response should reflect HTTP/1.1.

>>>>> That isn't how these things typically work. New protocols are
>>>>> advanced with either deliberate backwards-compat or deliberate
>>>>> backwards-break, with an expectation that it will either do
>>>>> something useful on an older-protocol server or cause a safe
>>>>> error in an expected way.
>>>>>
>>>>> Hence, we might still see an HTTP/4.0 that is designed to be
>>>>> parsed like HTTP/1.1 (by an old server) while at the same time
>>>>> work perfectly for a new server. That would be some hefty magic,
>>>>> but it remains possible. Likewise, we might want to deploy a
>>>>> version of h2 or HTTP/3 that works on unix domain sockets or
>>>>> localhost over non-Internet TCP.
>>>>>
>>>>> This is why the existing code did not error on protocols >= 2.0.
>>>>> Doing so is both unnecessary and counterproductive. If parsing
>>>>> fails for some other reason, we want that other reason to be
>>>>> in the response (because that's what the new protocol will be
>>>>> expecting from an old protocol server). If it doesn't fail, we
>>>>> want to provide the successful response because the request
>>>>> was deliberately crafted that way to save a round trip.
>>>>>
>>>>> Note that the incoming request protocol version should always
>>>>> be distinct from any forwarded request protocol or response
>>>>> protocol versions.
 
Precisely. If mod_http2 or quic/mod_http3 can do something with the connection
based on the request line, it's up to them through the hook facility to take ownership
of the connection.

If they cannot/do not, then the core http1 connection/request processors remain
in place and in response to "please speak in HTTP/4.0" this server will respond,
"sure, here is your HTTP/1.1 response" as expected and defined by the RFC.


Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Stefan Eissing

Stefan Eissing

<green/>bytes GmbH
Hafenweg 16
48155 Münster
www.greenbytes.de

> Am 18.06.2020 um 16:51 schrieb William A Rowe Jr <[hidden email]>:
>
>  
> >>>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
> >>>>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:
> >>>>>>
> >>>>>> I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
> >>>>>> in ap_parse_request_line.
> >>>>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
> >>>>>> and sending a
> >>>>>>
> >>>>>> GET /something HTTP/2.0
> >>>>>>
> >>>>>> as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.
>
> Correct, it starts an HTTP/1.1 connection, and the response should reflect HTTP/1.1.
>
> >>>>> That isn't how these things typically work. New protocols are
> >>>>> advanced with either deliberate backwards-compat or deliberate
> >>>>> backwards-break, with an expectation that it will either do
> >>>>> something useful on an older-protocol server or cause a safe
> >>>>> error in an expected way.
> >>>>>
> >>>>> Hence, we might still see an HTTP/4.0 that is designed to be
> >>>>> parsed like HTTP/1.1 (by an old server) while at the same time
> >>>>> work perfectly for a new server. That would be some hefty magic,
> >>>>> but it remains possible. Likewise, we might want to deploy a
> >>>>> version of h2 or HTTP/3 that works on unix domain sockets or
> >>>>> localhost over non-Internet TCP.
> >>>>>
> >>>>> This is why the existing code did not error on protocols >= 2.0.
> >>>>> Doing so is both unnecessary and counterproductive. If parsing
> >>>>> fails for some other reason, we want that other reason to be
> >>>>> in the response (because that's what the new protocol will be
> >>>>> expecting from an old protocol server). If it doesn't fail, we
> >>>>> want to provide the successful response because the request
> >>>>> was deliberately crafted that way to save a round trip.
> >>>>>
> >>>>> Note that the incoming request protocol version should always
> >>>>> be distinct from any forwarded request protocol or response
> >>>>> protocol versions.
>  
> Precisely. If mod_http2 or quic/mod_http3 can do something with the connection
> based on the request line, it's up to them through the hook facility to take ownership
> of the connection.

That is not issue. That works well.

> If they cannot/do not, then the core http1 connection/request processors remain
> in place and in response to "please speak in HTTP/4.0" this server will respond,
> "sure, here is your HTTP/1.1 response" as expected and defined by the RFC.

There are now several RFCs and they differentiate between HTTP/1.1 transport and
the pure HTTP semantics. This split is not reflected in our code, yet. We have
functions that mix both. Not as a mistake, it's historical.

ap_parse_request_line() for example, checks the initial HTTP/1.1 request line *and*
the method names, uri, header_only and other request_rec fields.

We can either copy the latter into mod_http2 and maintain it in two places or
have a core function for it to be invoked by mod_http and mod_http2. That seems
to be the design decision to make.

I used ap_parse_request_line() from mod_http2 to *not* duplicate the other code,
and someone added checks in trunk that imply it only ever gets called for HTTP/1.x.
So, now it pukes. Which is good, as it brought up this discussion.

- Stefan

Reply | Threaded
Open this post in threaded view
|

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

Roy T. Fielding
On Jun 18, 2020, at 9:03 AM, Stefan Eissing <[hidden email]> wrote:
Am 18.06.2020 um 16:51 schrieb William A Rowe Jr <[hidden email]>:


On 6/18/20 12:09 AM, Roy T. Fielding wrote:
On Jun 8, 2020, at 12:56 AM, Ruediger Pluem <[hidden email]> wrote:

I came across the question if we should not reject HTTP protocols >= 2.0 in the request line when we parse it
in ap_parse_request_line.
This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are not parsed via ap_parse_request_line
and sending a

GET /something HTTP/2.0

as request line is not a valid way to start a HTTP 2.0 connection and I doubt that it will be for future major versions.

Correct, it starts an HTTP/1.1 connection, and the response should reflect HTTP/1.1.

That isn't how these things typically work. New protocols are
advanced with either deliberate backwards-compat or deliberate
backwards-break, with an expectation that it will either do
something useful on an older-protocol server or cause a safe
error in an expected way.

Hence, we might still see an HTTP/4.0 that is designed to be
parsed like HTTP/1.1 (by an old server) while at the same time
work perfectly for a new server. That would be some hefty magic,
but it remains possible. Likewise, we might want to deploy a
version of h2 or HTTP/3 that works on unix domain sockets or
localhost over non-Internet TCP.

This is why the existing code did not error on protocols >= 2.0.
Doing so is both unnecessary and counterproductive. If parsing
fails for some other reason, we want that other reason to be
in the response (because that's what the new protocol will be
expecting from an old protocol server). If it doesn't fail, we
want to provide the successful response because the request
was deliberately crafted that way to save a round trip.

Note that the incoming request protocol version should always
be distinct from any forwarded request protocol or response
protocol versions.

Precisely. If mod_http2 or quic/mod_http3 can do something with the connection
based on the request line, it's up to them through the hook facility to take ownership
of the connection.

That is not issue. That works well. 

If they cannot/do not, then the core http1 connection/request processors remain
in place and in response to "please speak in HTTP/4.0" this server will respond,
"sure, here is your HTTP/1.1 response" as expected and defined by the RFC.

There are now several RFCs and they differentiate between HTTP/1.1 transport and
the pure HTTP semantics. This split is not reflected in our code, yet. We have 
functions that mix both. Not as a mistake, it's historical.

ap_parse_request_line() for example, checks the initial HTTP/1.1 request line *and*
the method names, uri, header_only and other request_rec fields.

We can either copy the latter into mod_http2 and maintain it in two places or
have a core function for it to be invoked by mod_http and mod_http2. That seems 
to be the design decision to make.

I think that is the right way forward, though we have always tried
to minimize overhead for the common case. I guess the extra cycles
are irrelevant now.

I used ap_parse_request_line() from mod_http2 to *not* duplicate the other code,
and someone added checks in trunk that imply it only ever gets called for HTTP/1.x.
So, now it pukes. Which is good, as it brought up this discussion.

Yep. The new RFCs should be "finished" by next month due to HTTP/3
being in WGLC this month.  If anyone's interested:


...Roy

12