[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 <squid3 at treenet.co.nz>:

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