On 11/15/2013 09:21 AM, Amos Jeffries wrote:
> On 24/08/2013 3:08 a.m., Alex Rousskov wrote:
>> Perhaps you misunderstood my suggestion because it does not lead to any
>> huge changes. In fact, it would probably simplify your patch. I suggest
>> that the existing request parser is adjusted to work like this:
>>
>> if (the buffered request content is too short is the magic prefix)
>> keep accumulating as if we did not receive the end of headers
>> else if (the buffered request content starts with the magic)
>> create the corresponding header structures (PRI v2.0)
>> else
>> old code
>>
>> The above is done in the parser so no caller will have to be changed
>> unless it has to be changed to handle HTTP2 PRI correctly anyway. And
>> all changes in the callers will deal with regular header structures. No
>> magic, no offset adjustments, no custom HTTP2 parsing code.
> Maybe we are miss-aligning on our definition of "parser". I mean
> HttpParser the class.
I mean the request parsing code, starting with parseHttpRequest() that
you modify.
> The first problem here is that the HttpParser class is currently not
> responsible for interpreting the first line. Only for splitting it up
> into SP-delimited pieces and validating that the bytes forming each are
> valid for the particular positions.
The design I propose does not depend on parsing or interpreting the
"first line" of HTTP/2 messages. Please see the sketch below for details.
> The second problem is that the rest of the magic after the first "line"
> is not valid Mime syntax. If it is treated/parsed as headers by
> client-side code we will end up dumping out "SM: " or ": SM" on the
> server-side which is intentionally invalid HTTP/2 magic to detect that
> kind of brokenness.
The design I propose does not depend on parsing or interpreting HTTP/2
magic as if it was an HTTP/1 or MIME message (because it is not).
Please see the sketch below for details.
> It is the parseHttpRequest() function which does the parsing
> interpretation. And that is the piece of code I am changing with this patch.
That function() does more than interpretation. It does some parsing as
well. Your changes may be in the right function, but they are applied
too late. They should be applied before HTTP/1-specific code, not in the
middle of it.
> NOTE: The shortest possible HTTP/1.1 request is *shorter* by 8 bytes
> than the HTTP/2.0 magic prefix. And the shortest possible request-line
> (HTTP/0.9) is shorter by 17 bytes.
Not a problem. The second part of my first "if" condition takes care of
that: No HTTP/1 or HTTP/0 request-line can be an HTTP/2 prefix.
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;
}
bool HttpParser::startsWithHttp2magic() const {
return the buffer starts with the ENTIRE Http2magic;
}
...
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.
> # >
> # > if (the buffered request content is too short is the magic prefix)
> # > keep accumulating as if we did not receive the end of headers
>
> The loop calling HttpParser handles both HTTP/1 requests which can be
> many bytes shorter than the HTTP/2 magic, and the first line of that
> magic prefix. It keeps accumulating bytes until a first-line syntax has
> been received.
Since HTTP/1 first-line parser is expensive _and_ loses information
required to correctly distinguish HTTP/2 Magic prefixes from something
that uses Magic elements but is not a Magic prefix, it is wrong to use
HTTP/1 first-line parsing code to handle HTTP/2 Magic IMO. My sketch
above avoids that flaw.
Yes, it is possible to first apply the HTTP/1 parser and only then
perform HTTP/2 accumulation logic and HTTP/2 magic detection (as your
patch appears to do), but, IMO, that approach is more complex and,
hence, more error-prone compared to the one sketched above. Moreover,
the whole existence of the hard-coded HTTP/2 magic in the HTTP/2
protocol tells me that [at least some of] its authors intended the magic
string to be used this way -- compare raw bytes and switch to HTTP/2
mode ASAP if needed.
>>>> 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?
> That method was specifically
> chosen from unused names such that implementations treating unknown
> HTTP/1.x request-line as HTTP 0.9 format and requests will "upgrade" it
> according to HTTP/1 rules.
This does not compute for me. What will "Implementations treating
unknown HTTP/1.x request-line as HTTP 0.9 format" do with an HTTP/1 PRI
request that will break something? If we are talking about HTTP/1
implementations, they would treat HTTP/1 PRI request as regular request
with an extension method and will not "upgrade" it.
> The result when they do that is the line "PRI * HTTP/1.1" ... this is
> sign of broken middleware implementation *not* a sign of PRI being used
> legitimately as method by HTTP/1.1 software.
>
> Mark and the others went out of their way to identify tokens for the
> request which were not used in existing traffic. So if anyone is using
> the PRI request in HTTP/1 for any legitimate reason we need to be made
> aware of that and bring that to the HTTPbis WG attention as fast as
> possible so the magic can be changed.
If HTTPbis WG wants to outlaw previously valid HTTP/1 requests, they
should say so explicitly. If they did, please point me to the WG
document saying as much. If they did not, that HTTP/1 request remains
valid and should be handled as such.
Needless to say that, if HTTPbis WG wanted to avoid this confusion, they
would have chosen HTTP/2 magic that cannot be the result of an
HTTP/0.9(?) "upgrade". That tells me that either no such upgrade is
possible (and HTTP/1 PRI remains valid) or HTTP/1 PRI should be
explicitly banned in some HTTP protocol documents, with some specific
justification for such a drastic retroactive action.
> The HTTP/2 specification does not cover handling of the PRI magic by
> HTTP/1 software. That is expected to be handled according to the RFC
> 2616 or at least HTTPbis updated specifications.
I am not asking about handling of PRI magic by HTTP/1 software. I am
asking about handling of HTTP/1 PRI request (which is not HTTP/2 PRI
magic -- it has a different protocol version and would have to have more
headers to include "SM" as a body).
> * In HTTP/2.0 it is magic octets to be checked and discarded.
>
> * In HTTP/1.1 it is a message to be processed and handled.
> - in 1.1-only forward/reverse proxy it should be rejected with HTTP/1.1
> error.
>
> - interception proxy can either reject (we do right now), or pass-thru
> and preserve end-to-end HTTP/2.0 (what this patch is trying to implement).
Agreed if you are talking about HTTP/2 magic.
Disagreed if you are talking about an HTTP/1 PRI request.
I was only asking about the latter.
> + // send full HTTP/2.0 magic connection header to server
> + mb.append("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n", 25);
HTTP/2 magic is 24 octets, right? Why are we appending 25 bytes here?
Please add a named (SBuf?) constant for this magic string as it is
reused in various places in your patch.
> + if (tunnelState->request->method == Http::METHOD_PRI) {
> + if (http->request->method == Http::METHOD_CONNECT || http->request->method == Http::METHOD_PRI) {
> + if (!redirect.status && (request->method == Http::METHOD_CONNECT || request->method == Http::METHOD_PRI)) {
> + assert(tunnelState->waitingForConnectRequest() || tunnelState->request->method == Http::METHOD_PRI);
> + if (tunnelState->clientExpectsConnectResponse() || tunnelState->request->method == Http::METHOD_PRI) {
> + assert(tunnelState->waitingForConnectExchange() || tunnelState->request->method == Http::METHOD_PRI);
> + } else if ((method == Http::METHOD_OPTIONS || method == Http::METHOD_TRACE || method == Http::METHOD_PRI) &&
Where possible, please use the major protocol version (2) instead of the
method (PRI) for detecting special HTTP/2 handling cases. This may
significantly reduce the amount of disputed code (and feels overall
better regardless of that dispute resolution).
> - if (tunnelState->clientExpectsConnectResponse()) {
> + if (tunnelState->clientExpectsConnectResponse() || tunnelState->request->method == Http::METHOD_PRI) {
> - assert(tunnelState->waitingForConnectExchange());
> + assert(tunnelState->waitingForConnectExchange() || tunnelState->request->method == Http::METHOD_PRI);
Do you think it would be better to rename and adjust various
expectsConnect/waitingForConnect/etc. methods so that they cover both
HTTP/1 CONNECT and HTTP/2 PRI tunnels? Are there any important cases
where that old code still only handles CONNECT and not PRI?
> I am expecting that a lot of old configurations will be used as-is
> without admin manually adding ACL to forbid PRI method to their peers.
> As such Squid will at least initially face a lot of situations where the
> peers do not support HTTP/2 and reject the PRI request. We need to
> handle those cleanly and run through the set of other destinations in
> case we can find a route that will work.
The opposite approach is a lot simpler and may be more practical/safer:
If a cache_peer is not configured to indicate HTTP/2 support, assume it
does not support HTTP/2 and send no HTTP/2 traffic to that peer. In
those [initially very rare] cases where the peer can actually handle
HTTP/2, the admins will be encouraged to explicitly configure the peer
accordingly by a "No HTTP/2 peers found for forwarding HTTP/2 requests."
Squid warning emitted once, when Squid discovers the problem for the
first time.
Broadcasting HTTP/2 requests to all peers in hope to find the one that
works also smells like a DoS amplification attack, especially since some
peers will probably break if they receive an HTTP/2 request.
The other old issues are either addressed (thank you!) or revolve around
HTTP/1 PRI handling. We will need to return to the latter group once
that argument is resolved.
Cheers,
Alex.
Received on Mon Nov 18 2013 - 19:42:56 MST
This archive was generated by hypermail 2.2.0 : Tue Nov 19 2013 - 12:00:10 MST