Re: [PATCH] HTTP/2.0 initial support

On 11/18/2013 04:22 PM, Amos Jeffries wrote:
> On 2013-11-19 08:42, Alex Rousskov wrote:
>> Here are the same proposed conditions again, detailed further using a
>> code sketch:
>>
>> const SBuf HttpParser::Http2magic = // 24 magic octets:
>> "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n";
>>
>> bool HttpParser::tooShortForHttp2magic() const {
>> return bufsiz < Http2magic.size();
>> }
>>
>> bool HttpParser::isHttp2magicPrefix() const {
>> return the ENTIRE buffer is an Http2magic prefix;
>
> Does this mean?
> return HttpParser::Http2magic.cmp(buf, bufsiz) == 0;

Yeah, that looks correct to me. You probably do not need the
HttpParser:: prefix inside HttpParser class, but that is very minor, of
course (and more polishing would be needed to finalize this sketch anyway).

>> }
>>
>> bool HttpParser::startsWithHttp2magic() const {
>> return the buffer starts with the ENTIRE Http2magic;
>
> Does this mean?
> return HttpParser::Http2magic.cmp(buf,
> HttpParser::Http2magic.size()) == 0;

Perhaps not exactly because the above would be unsafe if bufsiz is less
than Http2magic.size(). I think you want something like

    return bufsiz >= Http2magic.size() &&
        Http2magic.cmp(buf, Http2magic.size()) == 0;

but it is all the same as long as we are just sketching.

>> parseHttpRequest()
>> {
>> if (hp->tooShortForHttp2magic() && hp->isHttp2magicPrefix())
>> return NULL; // keep accumulating
>>
>> if (hp->startsWithHttp2magic()) {
>> // create the corresponding header structures (HTTP/2 PRI)
>> // this is the only place where HTTP/2 structures are created
>> // the old code should be adjusted to fail on HTTP/2 requests
>> // eventually, full HTTP/2 parser will be engaged here
>> createHttp2request();
>> } else {
>> // old HTTP/[01] parsing/interpretation code stays here
>> parseHttp1request();
>> }
>>
>> common HTTP/* interpretation code, if any, stays here
>> }
>>
>>
>> The above is just a sketch, of course, but I hope it clarifies where I
>> think the HTTP/2 detection should happen and how to leave the old code
>> mostly unchanged. It can be polished to further and better separate
>> HTTP/2 and HTTP/1 parsing, namespaces, interpretation, etc.

> Thank you. That clarifies a lot what you are after and I agree it is a
> nice way to go.

Glad we are making progress.

> I still maintain that createHttp2request() and parseHttp1request() in
> the current code will be duplicate code.

No. createHttp2request() will immediately create an HTTP v2.0 Magic
request object. No parsing is needed or should be done for that (beyond
the preconditions that have been already sketched above) because there
is (and will be) only one such Magic request.

Old parseHttp1request() code will continue to parse requests [using
HTTP/1 rules]. Eventually, we will have parseHttp2request() equivalent
as well, of course (but not for the HTTP/2 Magic).

> That said, if you are happy to let the short-term duplication go through
> audit I am okay with it.

The above sketch does not require duplication IMO. Needless to say, if
duplications happens to be the best way to implement that sketch,
proving me wrong, I will support it.

>>>>>> Why prohibit PRI HTTP/1.1 requests? That method was not reserved and,
>>>>>> hence, ought to be a valid extension method for HTTP1.
>>>>>
>>>>> If PRI is being used as a method in HTTP/1 that is a problem for the
>>>>> HTTP/2 magic and we need to start rejecting it
>>>>
>>>> I do not see why handling HTTP/1.x PRI as before is a problem. What
>>>> exactly will it suddenly break?
>>
>>> What breaks is HTTP/2 failure detection.
>>
>> Sorry, but I still do not understand. What exactly is this "HTTP/2
>> failure" that you want to detect?
>>
>
> Client --2.0--> broken firewall --1.1--> Squid.
>
> Where client sends valid magic and Squid receives:
>
> PRI * HTTP/1.1\r\n
> \r\n
> \r\n
> SM\r\n
> ...
>
>
> or replace "broken firewall" with squid-2.7 server:
>
> PRI * HTTP/1.1\r\n
> X-Original-Version: HTTP/0.9\r\n
> <other HTTP/1.1 headers inserted by Squid>
> \r\n
> \r\n
> SM\r\n
> ...

Please note that both of the above blobs show two HTTP requests (from
HTTP/1 point of view), not one. The second requests starts after the
CRLFCRLF sequence, and only has SM method on the request-line. Thus, not
only do we need to block valid HTTP/1 PRI requests, but terminate their
connections, so that there is no chance that HTTP/2 payload that follows
the first PRI request will be misinterpreted as an HTTP/1 request.

Furthermore, since there is not guarantee something in the network will
not rewrite HTTP/2.0 as HTTP/1.0 or even HTTP/0.9, or will not replace
the rare "*" URL with something else, we should essentially terminate
any incoming connection that starts with "PRI " (4 octets) unless it
starts with HTTP/2 Magic.

I am OK with that kind of implementation if you think it is warranted.

Please note that the above approach does _not_ create HTTP/1 PRI
requests so there is no need to treat them specially anywhere in Squid
code! The overall sketch becomes:

  if we got HTTP/2 Magic
      handle HTTP/2 connection
      (as discussed in more detail earlier)
  else if we got "PRI " prefix
      terminate the connection
      (perhaps after responding with a new error message)
  else
      handle [HTTP/1] request as before

The above sketch does not show the details necessary to handle content
accumulation while testing the above two conditions, but the earlier
sketch illustrates how that can be done correctly.

> Passing these requests upstream is wrong:

... and usually impossible because they lack the host header or the
usable URL. The problem is not the initial request(s) themselves but the
following payload that may be misinterpreted as an HTTP request, opening
various request smuggling opportunities. Thus, if we step on this route,
we have to terminate the connection as discussed above, without creating
or parsing HTTP/1 PRI requests.

To summarize this part of the discussion, I am OK with "conservatively"
rejecting 4-octet "PRI " sequences by closing their connection. This
approach does not lead to special treatment of HTTP/1 PRI requests as
far as Squid code is concerned.

HTH,

Alex.
Received on Tue Nov 19 2013 - 05:09:51 MST