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


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.