Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decoding response from gRPC REST proxy results in "Invalid chunk end CR" error #2171

Closed
thomaseizinger opened this issue Apr 1, 2020 · 10 comments · Fixed by grpc-ecosystem/grpc-gateway#1499 or #2357

Comments

@thomaseizinger
Copy link

I am integrating with a go application that uses the gRPC REST proxy to provide an HTTP API.
My HTTP client is reqwest which uses hyper:0.13.3 at this stage.

The go application sends the following response:

HTTP/1.1 500 Internal Server Error
Content-Type: application/json
Trailer: Grpc-Trailer-Content-Type
Date: Wed, 01 Apr 2020 00:16:52 GMT
Transfer-Encoding: chunked

5e
{"error":"there are no existing invoices","message":"there are no existing invoices","code":2}
0
Grpc-Trailer-Content-Type: application/grpc

{"error":"there are no existing invoices","message":"there are no existing invoices","code":2}%

I've added a test to the hyper test suite that reproduces the issue: thomaseizinger@a608b28

I don't know whether this is a valid HTTP response. Reading through the HTTP spec, it seems like this grpc trailer stuff is violating the Transfer-Encoding: chunked spec because the chunked is advertised as 0 bytes which from what I understand means the response is over, yet there are more bytes following.

Interestingly, curl is able to handle the response just fine but that could just be an artifact of curl being very robust to these kind of mistakes.

Even if this is an invalid response, it would be nice if hyper could be robust here and just ignore everything that is coming after the 0 chunk size indicator.

@seanmonstar
Copy link
Member

That is actually valid chunked encoding, using chunked trailers. hyper just hasn't supported them, because practically nothing has ever used them and they are annoying to deal with.

gRPC of HTTP/1.1 decided "oh hey, we can do trailers with this obscure detail of chunked encoding". Grumble.

@seanmonstar
Copy link
Member

Checking RFC7230, a server shouldn't send chunked trailers if the client didn't include a TE: trailers header. Does your request include one?

Perhaps hyper should strip that header as long as it doesn't have h1 trailers support.

@thomaseizinger
Copy link
Author

That is actually valid chunked encoding, using chunked trailers. hyper just hasn't supported them, because practically nothing has ever used them and they are annoying to deal with.
gRPC of HTTP/1.1 decided "oh hey, we can do trailers with this obscure detail of chunked encoding". Grumble.

That is really interesting, I didn't know that :)

Checking RFC7230, a server shouldn't send chunked trailers if the client didn't include a TE: trailers header. Does your request include one?

Perhaps hyper should strip that header as long as it doesn't have h1 trailers support.

No my request did not include such header. I guess that results in two follow-up actions.
I will open an issue at the grpc-gateway repository to inform them about the spec mismatch.

@strobil
Copy link

strobil commented Jul 3, 2020

I have the same problem. Original bug report is on linkerd2 repo
@thomaseizinger any news about that?

@strobil
Copy link

strobil commented Jul 3, 2020

Even if this is an invalid response, it would be nice if hyper could be robust here and just ignore everything that is coming after the 0 chunk size indicator.

Agree with you.

@strobil
Copy link

strobil commented Jul 3, 2020

Checking RFC7230, a server shouldn't send chunked trailers if the client didn't include a TE: trailers header. Does your request include one?

Perhaps hyper should strip that header as long as it doesn't have h1 trailers support.

Actually hyper strips that trailer, but after this i am getting net::ERR_INCOMPLETE_CHUNKED_ENCODING error in Google Chrome.
The only difference is after hyper strips that header, it strips last CR LF too. But probably should not. Please check attachments (wireshark screenshots with linkerd enabled and direct request).

after_hyper
valid

@seanmonstar
Copy link
Member

hyper doesn't have any handling of the te: trailers header in its HTTP/1 code.

@strobil
Copy link

strobil commented Jul 4, 2020

hyper doesn't have any handling of the te: trailers header in its HTTP/1 code.

Helllo @seanmonstar! I think we are misunderstood each other.
It's not about handing that header, it's about the response with trailers passed through the hyper becomes invalid. Please take a look on screenshots above. On the first screenshot we can see a response passed through the hyper, pay attention on the end of response body. The are missing CR LF on the end and the trailer which exists in original response. On the second screenshot we can see an original response.

@jeromegn
Copy link

Just stumbled on this ourselves. We're proxying a response from a nginx instance that uses add_trailer and this appears to close the connection prematurely. HTTP clients tend to complain, like curl:

* transfer closed with outstanding read data remaining
100 76655    0 76655    0     0   467k      0 --:--:-- --:--:-- --:--:--  467k
* Closing connection 0
curl: (18) transfer closed with outstanding read data remaining

76655 is the full size of the transfer, but since it produces an unexpected EOF, that curl errors.

I know this way of doing things is not very common, but some of our customers might eventually do it and stumble on this bug. It wasn't very easy to troubleshoot. My only clue was "error reading a body from connection: Invalid chunk end CR" which led me here.

seanmonstar pushed a commit that referenced this issue Dec 15, 2020
Previously, hyper returned an "Invalid chunk end CR" error on chunked
responses with trailers, as described in RFC 7230 Section 4.1.2. This
commit adds code to ignore the trailers.

Closes #2171
@jeromegn
Copy link

I suppose this specific issue is fixed, but now I wonder if it would be possible to accept trailers?

Our use case is: upstream clients can send back trailer headers containing metrics, including timing which may only be available after initial headers have been sent.

MOZGIII pushed a commit to vectordotdev/hyper that referenced this issue Dec 16, 2020
Previously, hyper returned an "Invalid chunk end CR" error on chunked
responses with trailers, as described in RFC 7230 Section 4.1.2. This
commit adds code to ignore the trailers.

Closes hyperium#2171
seanmonstar pushed a commit that referenced this issue Dec 16, 2020
Previously, hyper returned an "Invalid chunk end CR" error on chunked
responses with trailers, as described in RFC 7230 Section 4.1.2. This
commit adds code to ignore the trailers.

Closes #2171

Co-authored-by: Alex Rebert <[email protected]>
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this issue Jul 26, 2021
Previously, hyper returned an "Invalid chunk end CR" error on chunked
responses with trailers, as described in RFC 7230 Section 4.1.2. This
commit adds code to ignore the trailers.

Closes hyperium#2171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants