grpc-web: suppress content-length header from grpc-web requests

[negator]

grpc-web: suppress content-length header from web requests

Description:
When using envoy in a tiered deployment, with a grpc-web filter on the downstream envoy, requests to upstream envoys fail over http2 when a content-length header is present, since the underlying nghttp2 library strictly adheres to the http2 spec. This header is usually passed up from browsers, but reflects the total http1.1 payload size, not the total DATA frame sizes.

Risk Level: Low

Testing:
A simple unit test was added to verify that content-length headers are suppressed.

Release Notes:
N/A

Fixes #2942

[lizan]

LGTM, modulo style fix.

Please run tools/check_format.py fix.

[alyssawilk]

Curious, can you explain why this is a grpc specific issue and not an H2 issue in general? I just want to make sure we're not doing too narrow of a fix.
@mattklein123 who may know offhand :-P

[negator]

You’re right, it’s not a grpc specific issue. It could occur anytime an incorrect content-length header is passed to a secondary envoy backend over h2. However this fix is necessary regardless of any other general fix, since browsers (currently) know nothing about h2 data framing and the content-length header is almost always going to be incorrect.

[alyssawilk]

Removing the content length header may be necessary but it'd be great if we can find one place to do it for all of HTTP/2 rather than have each filter do their own clearing.

Offhand I think the best place for this is in the HTTP/2 codec, but I'd like to wait for Matt to weigh in.

[negator]

I don’t think removing it in all cases is the right thing to do, since it could lead to surprising behavior for clients that expect it to be there. But I do think we’d want to perform validation in the h2 codec, or elsewhere, prior to invoking an h2 backend. But I’d defer to you.

[mattklein123]

On the general topic of this change, this problem is actually not specific to h2. It's really specific to all HTTP versions and any filter which fundamentally alters body during processing. I.e., the same issue would occur over HTTP/1.1.

I also don't think removing in all cases is the right thing to do. It really comes down to filters that do this type of modification have to do one of two things:

1. Strip content length and switch to chunk encoding. Envoy essentially does this transparently. In the HTTP/1.1 codec if no content-length header is seen, it will start chunk encoding. HTTP/2 is inherently chunk encoding at the binary level and content-length is really a hint.
2. Write a new content-length that will match the final body size.

We could consider somehow checking/warning/enforcing this at the Envoy level so at least you would know at the local level if you were doing something wrong...

[mattklein123]

Please add a comment as to why this is being removed.

[sayrer]

Switching to chunked transfer encoding is what nginx does in this situation, fwiw.

#2 always runs into the the problem of buffering the entire body to calculate the length, unless you can emit it as a trailer. Maybe feasible for h2, but not 1.1.

[lizan]

@sayrer in grpc-web context the upstream must be http2, there is no chunked encoding, hence removing content length is enough.

[alyssawilk]

Ah see I misunderstood the problem - I thought upstream was choking on content length being present not because the gRPC stack was changing the payload and not updating the header. In that case yeah, it should be a fix to the filter and we can add codec-level validation later on.

[lizan]

We could consider somehow checking/warning/enforcing this at the Envoy level so at least you would know at the local level if you were doing something wrong...

or at nghttp2 level, since it does enforce (RST_STREAM with PROTOCOL_ERROR) this for incoming request, so it can raise at least a warning for outgoing request too.

[mattklein123]

Thank you!