http: fixing CONNECT to not advertise chunk encoding. #11245
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
source/common/http/header_utility.cc
Outdated
| bool HeaderUtility::isConnectResponse(const RequestHeaderMapPtr& request_headers, | ||
| const ResponseHeaderMap& response_headers) { | ||
| return request_headers.get() && isConnect(*request_headers) && | ||
| Http::Utility::getResponseStatus(response_headers) == 200; |
| @@ -165,8 +165,12 @@ void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& head | |||
| } else if (connection_.protocol() == Protocol::Http10) { | |||
| chunk_encoding_ = false; | |||
| } else { | |||
| encodeFormattedHeader(Headers::get().TransferEncoding.get(), | |||
| Headers::get().TransferEncodingValues.Chunked); | |||
| // For responses to connct requests, do not send the chunked encoding header: | |||
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| tcp_client->waitForData("\r\n\r\nreturn-payload", false); | ||
|
|
||
| tcp_client->close(); | ||
| // The response will be rejected because chunked headers are not allowed with CONNECT upgrades. |
There was a problem hiding this comment.
Let me see if I understand this correctly: does this means that Envoy will reject a proxied HTTP CONNECT if the upstream proxy server happens to send a TE header in the response?
This seems like it might break some upstream proxies that aren't carefully implemented. Is there a reason to reject these responses rather than just ignoring the header?
There was a problem hiding this comment.
Unfortunately Envoy will strip the TE header (that's pretty strongly asserted across the code base) and not add it back. AFIK if there is a TE header there ought to be TE-chunked data, and so by altering that header we'd be sending potentially compromised payload.
There was a problem hiding this comment.
Just so I'm clear: do you mean Envoy strips TE whenever it appears or just for the CONNECT case?
There was a problem hiding this comment.
Envoy always strips TE for headers passing through Envoy.
It re-emits TE for HTTP/1.1 requests/responses which may have bodies if there's no content length provided. Given for connect it is not allowed to have T-E it wouldn't know to reapply it, so it'd end up in an inconsistent state I'd like to avoid.
Worst case if there's a widely deployed proxy which does this illegal thing we could transform T-E into some Had-TE-Header and reinstate it, but I'd rather err on the side of simplicity until-if someone complains.
There was a problem hiding this comment.
That seems fair. Thanks for educating me :)
Signed-off-by: Spencer Lewis <slewis@squareup.com> * master: (33 commits) docs: break release notes into categories (envoyproxy#11217) admin: extract more handlers to separate classes (envoyproxy#11258) Load reporting service documentation (envoyproxy#10962) http: testing 304-with-body behavior (envoyproxy#11261) fixing typos and breaking link issues (envoyproxy#11270) devex: initial commit of devcontainer setup (envoyproxy#11207) security: update policy for fix/disclosure SLOs. (envoyproxy#11243) http: fixing CONNECT to not advertise chunk encoding. (envoyproxy#11245) docs: update upstream network filters description (envoyproxy#11231) deps: update datadog tracer to v1.1.5 (envoyproxy#11253) test: Fix missing instantiation of parameterized tests. (envoyproxy#11247) fix go mirror when no changes (envoyproxy#11249) docs: host_rewrite -> host_rewrite_literal (envoyproxy#11229) wasm: update V8 to v8.3.110.9. (envoyproxy#11233) tls: update BoringSSL to 107c03cf (4103). (envoyproxy#11232) bazelci: always exclude nocoverage tag in coverage config (envoyproxy#11226) ci: save api revision in go-control-plane (envoyproxy#11220) build: fix cares build (envoyproxy#11225) stats: Pre-allocate codec stats for http1 and http2 (envoyproxy#11135) api: manifest based edge default documentation. (envoyproxy#11151) ...
Risk Level: low (connect only)
Testing: UT, IT
Docs Changes: n/a
Release Notes: n/a
Fixes #11227
Part of #1451