facebook/proxygen

HTTP2 Protocol Handling

SteveSelva opened this issue · 5 comments

I think Proxygen has left a case where the Server wants to communicate using HTTP/1.1 but while TLS Handshake, it negotiates a HTTP/2 protocol. So the Server sends an RST_STREAM with Error: HTTP_1_1_REQUIRED. For this message, proxygen is throwing an Ingress Error and Shutting down the Transport.

This is the image of packets captured using Wireshark and it shows the RST_STREAM with that error.
HTTP_1_1_REQUIRED

This is the log from Proxygen.

E20240715 13:01:42.151611 19228 HTTP2Codec.cpp:1812] Connection error PROTOCOL_ERROR Connection error with ingress=
E20240715 13:01:42.151958 19228 InfoCallback.cpp:38] onIngressError,proto=http/2,
E20240715 13:01:42.153701 19228 InfoCallback.cpp:43] IngressData,contentLen:13
E20240715 13:01:42.154003 19228 InfoCallback.cpp:44] Hex Dump:
00 00 04 03 00 00 00 00 01 00 00 00 0d 
E20240715 13:01:42.154361 19228 ProxyHandler.cpp:143] onServerError:what="Shutdown transport: MalformedInput", direction=2, proxygenError=MalformedInput, codecStatusCode=-1, httpStatusCode=0

Here I can correlate and confirm that this RST_STREAM can't be parsed by Proxygen.

Can you please fix this ASAP.

Can you include the full set of HTTP/2 bytes sent by the server in this case? Scanning the wireshark it looks OK - seems to be SETTINGS, WINDOW_UPDATE, SETTINGS(ack maybe?), RST_STREAM?

It should parse a RST_STREAM just fine regardless of the error code. The log indicates the codec found a session error but I'm not sure what it is. Or try with verbosity 4 (-v 4) and send the logs?

I have found the error code,

In header file ErrorCode.h, the value has for HTTP_1_1_REQUIRED has been set to 13. But in source file ErrorCode.cpp, the kMaxErrorCode has been set to 12 causing to throw a INTERNAL_ERROR instead of HTTP_1_1_REQUIRED.

HTTP_1_1_REQUIRED = 13,

const uint8_t kMaxErrorCode = 12;

I modified the value of kMaxErrorCode to 13, now its working fine.

@SteveSelva good catch! Feel free to share a PR so this gets fixed in the repo?

@afrind this could use a unit-test as well?

I have an another suggestion. In proxygen_proxy's ProxyHandler, for errors occurred at the Upstream is received at onServerError() function. In this case the error HTTP_1_1_REQUIRED is received. But in downstream, just an abort is sent. It's better to send the abort with particular error code.

I made some code changes to make the function void sendAbort(ErrorCode statusCode); to be public in HTTPTransaction.h. So that the abort will be sent with that specific error code instead of generic cancel.

Sent a PR.
#506