Re: [PATCH] HTTP/2.0 initial support

On 08/22/2013 11:57 PM, Amos Jeffries wrote:
> On 23/08/2013 5:11 a.m., Alex Rousskov wrote:
>> Perhaps more importantly, the way you parse PRI requests may violate
>> draft-ietf-httpbis-http2-04 that seems to require that those requests
>> start with a specific 24-octet magic, not with a general HTTP1 header
>> followed by the 6-octet magic. Why not parse HTTP2 Connection Header
>> correctly, without dealing with the apparently inappropriate HTTP1
>> request parser at all??

> The magic may still occur at any time thanks to Upgrade:, so the HTTP/1
> parser avoidance can only be added after the
> ConnStateData/ClientSocketContext/ClientHttpRequest interactions have
> all been re-worked for layer separation and multiplexed framing HTTP/2
> support added. I am not keep to do that all as one huge step.

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.

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

> because technically the
> "PRI" is now a HTTP/1 extension method for signalling that HTTP/2 is
> already started on this connection (SETTINGS frame will be in its
> payload in binary form).

That does not compute for me. Perhaps I do not understand how HTTP2 is
supposed to work, but my current understanding is that no HTTP1 request
can affect HTTP2 operation, including HTTP1 requests with method PRI.

HTTP2 requires HTTP/2.0 text for the magic to match so it will never
match any HTTP1 request, including HTTP1 PRI request. Thus, I do not see
why we need to prohibit HTTP1 PRI or even treat it specially.

If HTTP2 specs assign some special meaning to HTTP1 PRI, please point me
to the right documentation.

>>> + } else if (*method_p == Http::METHOD_PRI || *http_ver ==
>>> Http::ProtocolVersion(2,0)) {
>>> + // other uses of PRI method or HTTP/2.0 version moniker are
>>> not supported
>>> + hp->request_parse_status = Http::scMethodNotAllowed;
>>> + return parseHttpRequestAbort(csd, "error:method-not-allowed");
>>> + }
>> The error message seems wrong in case of a PRI method -- we do support
>> it.
>
> We support it but _do not allow it_ on this port type.

Please fix or remove the comment then. It says "not supported".

>>
>>> + // Let tunneling code be fully responsible for CONNECT and PRI
>>> requests
>>> + if (http->request->method == Http::METHOD_CONNECT ||
>>> http->request->method == Http::METHOD_PRI) {
>> and
>>
>>> - if (request->method == Http::METHOD_CONNECT && !redirect.status) {
>>> + if (!redirect.status && (request->method == Http::METHOD_CONNECT
>>> || request->method == Http::METHOD_PRI)) {
>> Since HTTP1 PRI requests are valid but should not be tunneled, the new
>> logic here should be based on version being 2.0 rather than the method
>> being PRI, I guess.
>>
>> More changes may be needed to get this right, depending on how we parse
>> and validate HTTP2 requests.
>
> If we make this check by version it will start matching GET/PUT/POST
> requests as soon as we start actually parsing the HTTP/2 messages.

Yes, more changes will be needed as soon as we allow more HTTP2 message
methods. Still, HTTP1 PRI should not cause tunnels.

>> 2) At memcpy() time, tunnelState->client.buf may already contain data
>> from the server. Note the tunnelState->copy() call above.

That was wrong, sorry. What I should have said is that at memcpy() time,
we may be comm_writing server bytes to the client already. The following
call in tunnelStartShoveling() starts writing server data to the client:

> tunnelState->copy(tunnelState->server.len, tunnelState->server, tunnelState->client, TunnelStateData::WriteClientDone);

>> I am concerned what would happen when both the
>> server has data to sent to the client and the client has data to send to
>> the server. I am not 100% sure tunnel.cc code can handle that.

That concern still exist. This is just a concern. I just do not know
whether the current code can handle what you are asking it to do. If you
studied it and think it is going to work, that is fine with me.

> You created the "shovel" logics here!

It should not be important, but I do not think it was me. I did create
the tunnelStartShoveling() function, but only to reuse the existing
tunnel.cc code. The shoveling logic itself goes way beyond that startup
method, and it was already there, possibly from the very beginning.

>>> /// Called when we are done writing CONNECT request to a peer.
>>> static void
>>> tunnelConnectReqWriteDone(const Comm::ConnectionPointer &conn, char
>>> *buf, size_t size, comm_err_t flag, int xerrno
>>> {
>>> TunnelStateData *tunnelState = (TunnelStateData *)data;
>>> debugs(26, 3, conn << ", flag=" << flag);
>>> - assert(tunnelState->waitingForConnectRequest());
>> Why was this assertion removed while the description of the function
>> remained unchanged? They go together.
>
> I have not yet fully sorted out how to handle failover between peers if
> the first one does not support HTTP/2 but the PRI is still permitted to
> go there. For now the recourse is to always deliver the peer response to
> the client. Just like CONNECT used to be handled before you added the
> whole waiting mechanism. So the waiting flag will be false in this pathway.
>
> Re-adding those assertions and managing server HTTP/1 rejections/replies
> is a TODO.

I am having trouble correlating my question with your answer. According
to the above quoted code, tunnelConnectReqWriteDone() is called when we
are done writing CONNECT request to a peer. Is that still true? If yes,
the assertion seems appropriate. If it is no longer true, let's fix the
function description accordingly and may be adjust the assertion as well.

As for failover, I am not sure we need it: If Squid is misconfigured to
send HTTP2 requests where they should not go, do we really want to try
to recover from that configuration failure automatically?

>>> // we have to eat the connect response from the peer (so
>>> that the client
>>> // does not see it) and only then start shoveling data to
>>> the client
>> ...
>>> -
>>> - assert(tunnelState->waitingForConnectExchange());
>> Why was this assertion removed? It seems to go together with the comment
>> above it (which was not adjusted).
>
> see above about the other one.

Ditto. Perhaps you can describe a specific case where these seemingly
valid assertions fail after your patch? IIRC, those two assertions deal
with sending a CONNECT request to a peer when the client did not send a
CONNECT request. That situation needs special care because Squid becomes
responsible for (a) sending the CONNECT and (b) hiding CONNECT response
from the client. Does HTTP2 need different handling in that situation?

>>> + // we support HTTP/2.0 magic PRI requests
>>> + if (r->method == Http::METHOD_PRI)
>>> + return (r->urlpath == "*");
>>> +
>> This will block HTTP1 PRI requests, which we should continue to support
>> and will allow HTTP2 non-PRI requests which we do not want to support
>> for now.
>
> Intentionally so on both points.
> a) see above about the HTTP/1 requests.

Yes, we disagree on whether HTTP1 PRI should be allowed.

> b) when we get on to parsing HTTP/2 requests we will face HttpRequest
> generated with version 2.0 and other non-PRI methods where the URL needs
> parsing.

"When we get on to parsing HTTP/2 requests", this code should be
adjusted further, of course. However, it seems wrong to allow something
we do not support yet. Should not this be the place to reject HTTP2
requests that we do not [yet] support?

>> * If you have not checked that the new code works with intercepted SSL
>> traffic forwarded to an origin server when ssl_bump none matches, please
>> do. That condition exercises the tunnel path you are changing using a
>> fake CONNECT request.
>
> I have not and cannot test the SSL cases. Instead I have simulated
> various data arrival permutations on a CONNECT.
> If the bumping code breaks with any of those flow types something is
> very broken in the bumping, but needs a separate fix.

Please do not commit your changes until the above common case is tested.
If you cannot test it, we will once you post the final patch.

>> * If you have not checked that the new code works with CONNECT traffic
>> forwarded to an SSL peer, please do. That condition exercises the tunnel
>> path you are changing when the CONNECT response from the SSL peer is
>> forwarded to the client. Please test with a simple CONNECT request and a
>> CONNECT request that has some data attached after it (bug 3371).
>
> I have tested CONNECT relayed to direct and peer with early pushed data
> from client and from server. These appear to be working fine.
> Given those results, I have to assume that a "normal" HTTPS inside
> CONNECT will retain its existing behaviour.
>
> I assume you are referring to the ssl_bump none case above because the
> ConnStateData has a buffer filled with SSL handshake data?

No. The key in this use case is that the client is sending CONNECT and
the peer is an SSL peer, which results in peer CONNECT response to Squid
being blindly forwarded to the client, without parsing. This is not
related to SslBump.

Cheers,

Alex.
Received on Fri Aug 23 2013 - 15:08:19 MDT