Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Amos Jeffries]

On 5/10/2016 2:34 a.m., Eduard Bagdasaryan wrote:
> 2016-09-26 1:23 GMT+03:00 Amos Jeffries :
> 
>> Okay finally got to this. Sorry for the delay.
>>
>> I think what confused me was the handling of the !multiDigits cases.
>>
>> For the RequestParser::parseVersionField() the relevant bits are:
>>
>> If a version label matches the "HTTP/" 1*DIGIT "." 1*DIGIT pattern from
>> RFC 2616 it should not be handled as 0.9 syntax. All unacceptible
>> versions that begin with "HTTP/" should get a 505.
>>
>> To be compliant with RFC 7230:
>>
>> - versions 1.2 thru 1.9 accept and handle normally. That is a SHOULD
>> requirement in section 2.6 final paragraph (bottom of page 15).
>>
>> - other single-digit versions should get the 505 error.
>>
>> - versions with multiple digits should get the 505 error.
> 
> 
> Adjusted the code accordingly and reattached the patch and testing results.
> 
> 

Yay. All passing :-)

I think the comment should say " ('0.0') " rather than '(zero)'. On
first reading I thought it might lead to '1.0' being identified.

+1, and applied as trunk rev.14866 with that documentation tweak.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Amos Jeffries]

Okay finally got to this. Sorry for the delay.

I think what confused me was the handling of the !multiDigits cases.

For the RequestParser::parseVersionField() the relevant bits are:

If a version label matches the "HTTP/" 1*DIGIT "." 1*DIGIT pattern from
RFC 2616 it should not be handled as 0.9 syntax. All unacceptible
versions that begin with "HTTP/" should get a 505.

To be compliant with RFC 7230:

- versions 1.2 thru 1.9 accept and handle normally. That is a SHOULD
requirement in section 2.6 final paragraph (bottom of page 15).

- other single-digit versions should get the 505 error.

- versions with multiple digits should get the 505 error.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Amos Jeffries]

On 19/09/2016 7:48 a.m., Eduard Bagdasaryan wrote:
> 
> Just a reminder of this issue. Amos, is there anything to do
> to move it forward? In my point of view, only the related unit test
> is to be fixed, but probably I am still misunderstand something.
> 

You had a few good points in your previous response. This has been
waiting me having enough time to go over the RFC logic in detail and
figure out where we are diverging on interpretation. Hopefully this week
sometime if not later today.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Eduard Bagdasaryan]

> 2016-09-06 17:03 GMT+03:00 Amos Jeffries :
>
>  > > This patch fixes Squid to return 505 (Version Not Supported) error
>  > > code for HTTP/2+ versions.
>  > >
>  > > Before this change, when a syntactically valid HTTP/1 request 
indicated

>  > > HTTP major version 2, Squid mangled and forwarded the request as an
>  > > HTTP/1 message.
>  >
>  > That is not true. The trunk code returns false the same as this 
patched

>  > version. Because the version field for HTTP/2 does not start with the
>  > static magic string "HTTP/1.
>
> Disagree: the trunk Http::One::RequestParser::parseHttpVersionField()
> returns true, because it treats HTTP/2 message as HTTP/0.9
> message. You can see this in my Co-Advisor attachment in "HTTP/1.1
> device MUST NOT use 2.1 HTTP version in requests" test case.
>
>  > > Since Squid does not and cannot correctly interpret an
>  > > HTTP/1 request using HTTP/2 rules, returning an error and 
closing the

>  > > connection appears to be the only correct action possible.
>  >
>  > That is not correct either. The old logic requires method to be 
GET for

>  > full HTTP/0.9 handling. HTTP/2 pseudo-method is PRI. If anything other
>  > than PRI is seen it is not valid HTTP/2.
>  >  At most we should have logic passing HTTP/2 transparently through the
>  > proxy (configurable). Which is the relay behaviour of HTTP/0.9
>  > processing, but not its header mangling behaviour (because PRI, 
not GET

>  > method).
>
> Not sure that is what Http::One::RequestParser::parseHttpVersionField()
> should care of. In my understanding, this method should parse the input
> with HTTP/1 rules, returning false if the input does not conform to
> these rules.
>
>  > Also, HTTP/2 does not obsolete HTTP/1.x RFC's, so there is an explicit
>  > possibility that HTTP/1.2+ will exist at some point. The IETF WG
>  > expectation is/was that 1.2+ (if any) would contain compatibility for
>  > HTTP/2+ features over HTTP/1.x syntax.
>
> OK, and the patched Squid would return 505 (HTTP Version Not Supported)
> for HTTP/1.2+.(it is not yet implemented).
>
>  > Just noticed that the unit test for "GET / HTTP/2.0" request-line is
>  > also failing due to this parser change now accepting that invalid
>  > version as valid.
>
> I suspect that test should be changed to check for 505 error code instead
> of 400.
>
>  > You should use tok.skip(Http1::MagicPrefix) instead of trying to count
>  > digits in the major version.
>
> I count digits in order to differentiate syntactically valid(but
> unsupported by Squid) HTTP versions and invalid(multi-digit)
> ones, returning 505 or 400 codes respectively.


Just a reminder of this issue. Amos, is there anything to do
to move it forward? In my point of view, only the related unit test
is to be fixed, but probably I am still misunderstand something.

Thanks,
Eduard.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Eduard Bagdasaryan]

2016-09-06 17:03 GMT+03:00 Amos Jeffries :

> > This patch fixes Squid to return 505 (Version Not Supported) error
> > code for HTTP/2+ versions.
> >
> > Before this change, when a syntactically valid HTTP/1 request indicated
> > HTTP major version 2, Squid mangled and forwarded the request as an
> > HTTP/1 message.
>
> That is not true. The trunk code returns false the same as this patched
> version. Because the version field for HTTP/2 does not start with the
> static magic string "HTTP/1.

Disagree: the trunk Http::One::RequestParser::parseHttpVersionField()
returns true, because it treats HTTP/2 message as HTTP/0.9
message. You can see this in my Co-Advisor attachment in "HTTP/1.1
device MUST NOT use 2.1 HTTP version in requests" test case.

> > Since Squid does not and cannot correctly interpret an
> > HTTP/1 request using HTTP/2 rules, returning an error and closing the
> > connection appears to be the only correct action possible.
>
> That is not correct either. The old logic requires method to be GET for
> full HTTP/0.9 handling. HTTP/2 pseudo-method is PRI. If anything other
> than PRI is seen it is not valid HTTP/2.
>  At most we should have logic passing HTTP/2 transparently through the
> proxy (configurable). Which is the relay behaviour of HTTP/0.9
> processing, but not its header mangling behaviour (because PRI, not GET
> method).

Not sure that is what Http::One::RequestParser::parseHttpVersionField()
should care of. In my understanding, this method should parse the input
with HTTP/1 rules, returning false if the input does not conform to
these rules.

> Also, HTTP/2 does not obsolete HTTP/1.x RFC's, so there is an explicit
> possibility that HTTP/1.2+ will exist at some point. The IETF WG
> expectation is/was that 1.2+ (if any) would contain compatibility for
> HTTP/2+ features over HTTP/1.x syntax.

OK, and the patched Squid would return 505 (HTTP Version Not Supported)
for HTTP/1.2+.(it is not yet implemented).

> Just noticed that the unit test for "GET / HTTP/2.0" request-line is
> also failing due to this parser change now accepting that invalid
> version as valid.

I suspect that test should be changed to check for 505 error code instead
of 400.

> You should use tok.skip(Http1::MagicPrefix) instead of trying to count
> digits in the major version.

I count digits in order to differentiate syntactically valid(but
unsupported by Squid) HTTP versions and invalid(multi-digit)
ones, returning 505 or 400 codes respectively.


Eduard.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Amos Jeffries]

On 7/09/2016 1:49 a.m., Amos Jeffries wrote:
> On 6/09/2016 9:06 a.m., Eduard Bagdasaryan wrote:
>> Hello,
>>
>> This patch fixes Squid to return 505 (Version Not Supported) error
>> code for HTTP/2+ versions.
>>
>> Before this change, when a syntactically valid HTTP/1 request indicated
>> HTTP major version 2, Squid mangled and forwarded the request as an
>> HTTP/1 message.
> 
> That is not true. The trunk code returns false the same as this patched
> version. Because the version field for HTTP/2 does not start with the
> static magic string "HTTP/1."
> 
> 
>> Since Squid does not and cannot correctly interpret an
>> HTTP/1 request using HTTP/2 rules, returning an error and closing the
>> connection appears to be the only correct action possible.
> 
> That is not correct either. The old logic requires method to be GET for
> full HTTP/0.9 handling. HTTP/2 pseudo-method is PRI. If anything other
> than PRI is seen it is not valid HTTP/2.
>  At most we should have logic passing HTTP/2 transparently through the
> proxy (configurable). Which is the relay behaviour of HTTP/0.9
> processing, but not its header mangling behaviour (because PRI, not GET
> method).
> 
> In the case where it is not HTTP/1.1, HTTP/1.0, HTTP/2, or GET without a
> version field. Then the version must start with "HTTP/" to be considered
> invalid HTTP version and the MUST clause applicable.
> 
> Otherwise it could be any protocol at all. HTTP/2, SMTP, Finger,
> Secure-HTTP, WebSocket, SPDY and ICAP can all be sent to this parse
> logic at various times.
> 
> Also, HTTP/2 does not obsolete HTTP/1.x RFC's, so there is an explicit
> possibility that HTTP/1.2+ will exist at some point. The IETF WG
> expectation is/was that 1.2+ (if any) would contain compatibility for
> HTTP/2+ features over HTTP/1.x syntax.
> 
>>
>> Also: since HTTPbis prohibits HTTP versions with multi-digits, treat
>> such requests as invalid.
>>
>> Attached Co-Advisor result pages for patched and unpatched Squid.
>>
> 
> For some reason I'm not sure of yet these changes result in unit test
> failure parsing a garbage line with various bad whitespace characters in it.
> 
> make check with testHttp1Parser debugging enabled produces:
> "
> TEST @1071, in="\r\t   \n"
> 
> testHttp1Parser.cc:106:Assertion
> Test name: testHttp1Parser::testParseRequestLineProtocols
> equality assertion failed
> - Expected: 0
> - Actual  : 1
> "
> 

Just noticed that the unit test for "GET / HTTP/2.0" request-line is
also failing due to this parser change now accepting that invalid
version as valid.

I think this other fail is due to "2.0" meeting both the version layout
and the "multiDigits' conditions. You should use
tok.skip(Http1::MagicPrefix) instead of trying to count digits in the
major version. This is an HTTP/1 parser, non-1.x versions (except 0.9
special case) are all unsuccessful parse if they occur here.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Amos Jeffries]

On 6/09/2016 9:06 a.m., Eduard Bagdasaryan wrote:
> Hello,
> 
> This patch fixes Squid to return 505 (Version Not Supported) error
> code for HTTP/2+ versions.
> 
> Before this change, when a syntactically valid HTTP/1 request indicated
> HTTP major version 2, Squid mangled and forwarded the request as an
> HTTP/1 message.

That is not true. The trunk code returns false the same as this patched
version. Because the version field for HTTP/2 does not start with the
static magic string "HTTP/1."


> Since Squid does not and cannot correctly interpret an
> HTTP/1 request using HTTP/2 rules, returning an error and closing the
> connection appears to be the only correct action possible.

That is not correct either. The old logic requires method to be GET for
full HTTP/0.9 handling. HTTP/2 pseudo-method is PRI. If anything other
than PRI is seen it is not valid HTTP/2.
 At most we should have logic passing HTTP/2 transparently through the
proxy (configurable). Which is the relay behaviour of HTTP/0.9
processing, but not its header mangling behaviour (because PRI, not GET
method).

In the case where it is not HTTP/1.1, HTTP/1.0, HTTP/2, or GET without a
version field. Then the version must start with "HTTP/" to be considered
invalid HTTP version and the MUST clause applicable.

Otherwise it could be any protocol at all. HTTP/2, SMTP, Finger,
Secure-HTTP, WebSocket, SPDY and ICAP can all be sent to this parse
logic at various times.

Also, HTTP/2 does not obsolete HTTP/1.x RFC's, so there is an explicit
possibility that HTTP/1.2+ will exist at some point. The IETF WG
expectation is/was that 1.2+ (if any) would contain compatibility for
HTTP/2+ features over HTTP/1.x syntax.

> 
> Also: since HTTPbis prohibits HTTP versions with multi-digits, treat
> such requests as invalid.
> 
> Attached Co-Advisor result pages for patched and unpatched Squid.
> 

For some reason I'm not sure of yet these changes result in unit test
failure parsing a garbage line with various bad whitespace characters in it.

make check with testHttp1Parser debugging enabled produces:
"
TEST @1071, in="\r\t   \n"

testHttp1Parser.cc:106:Assertion
Test name: testHttp1Parser::testParseRequestLineProtocols
equality assertion failed
- Expected: 0
- Actual  : 1
"


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev