ext_proc: adds optional config to force content-length instead of chunked transfer encoding during STREAMED and FULL_DUPLEX_STREAMED body processing #39067
Conversation
|
Hi @dergraf, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
dergraf
force-pushed
the
keep-content-length-header
branch
from
13dbcb2 to
1d1c92f
Compare
| // It only works in combination with SEND header send mode. For any other combination of modes, it is ignored. | ||
| // | ||
| // If enabled and a content length header is present in a header mutation Envoy will not switch to chunked transfer encoding | ||
| // for http1 codec. It is the responsibility of the side stream server to provide a correct content length. |
There was a problem hiding this comment.
This would be a very dangerous thing to do, since ext_proc servers are not necessarily trusted. By proving smaller content length ext_proc servers can cause request smuggling.
There should be verification by the ext_proc filter that content-length matches the body length as it streams the body to upstream. If mismatch is detected the request must be reset.
There was a problem hiding this comment.
@yanavlasov thanks for your comment. I am aware of the security implications.
Where in the code would ext_proc filter apply such a content-length check?
There was a problem hiding this comment.
+1 The reason we remove content-length is because of serious security concern e.g., request smuggling. And in those modes described in #29536, ext_proc filter cannot guarantee the content-length matches the body length. Your trusted ext_proc server can return the correct length though
If you have legit use cases, this field should be set to false by default and with warning that customer can only enable it with their own caution e.g., at the risk of request smuggling etc if server is untrusted
There was a problem hiding this comment.
@dergraf Maybe give a thorough description on the use case why you need to keep the content-length header?
There was a problem hiding this comment.
@yanjunxiang-google Not the PR owner but if I may answer - one of our proxied backends handles large file uploads and relies on the presence of the Content-Length header in every request to run some validations before processing the body. I believe this isn't uncommon.
There was a problem hiding this comment.
IIUC, If ext_proc server is fully trusted, in a couple of use cases like below, Envoy does not have to remove content-length in these STREAMED body modes:
- if the ext_proc server just examine, but won't mutate the body;
- If the ext_proc server can calculate and send back the right content-length header in the header response.
There was a problem hiding this comment.
The use case is similar to the one mentioned by @ori-nat .
We're planning to migrate 200+ APIs to an Envoy based solution, trying to achieve a similar behavior wrt. to content-length handling as our current solution (request and response payloads could get as large as 200MB, definitely not something we want to use the buffered approach for.
As much as we'd like to switch to chunked transfer encoding, we have to deal with many different clients and target systems, severals dating back to the 80ies ;)
@ori-nat any ideas how this PR could be improved to better serve your use case?
@tyxia what would be the best approach to show a "warning" message regarding this feature?
There was a problem hiding this comment.
I think there are legit use cases when the ext_proc are trusted and under your control, but again this field should be disabled (i.e., false) by default given the security risk and be enabled by customer.
Regarding the warning message, you can use something like below which will be highlighted in the doc file
// .. warning::
//
There was a problem hiding this comment.
Added the following warning message, I assume having it in the proto file will add it to the rendered HTML documentation.
dergraf
force-pushed
the
keep-content-length-header
branch
from
48b629c to
302d91c
Compare
|
@tyxia I am currently struggling with the test cases. I copied some of the tests that verify that the content length is removed and adapt it to use the new feature. Could you or anyone else spot what I am doing wrong with my tests? Manually downloading the built Envoy package, properly configuring it to point to a locally running ext proc server works as expected (content length header is forwarded) |
@dergraf You will need to provide more details about your test failure like test details; error failure details, otherwise it is very hard for us to provide any helps. cc @yanjunxiang-google who is actively working ext_proc |
| body_mode_ == envoy::extensions::filters::http::ext_proc::v3::ProcessingMode::STREAMED || | ||
| body_mode_ == | ||
| envoy::extensions::filters::http::ext_proc::v3::ProcessingMode::FULL_DUPLEX_STREAMED || | ||
| (body_mode_ == envoy::extensions::filters::http::ext_proc::v3::ProcessingMode::STREAMED && |
There was a problem hiding this comment.
This logic can be simplified, like:
if (retain_content_length_header) {
return false;
}
...
There was a problem hiding this comment.
Would this also work for the BUFFERED BodySendMode + SKIP Headers? So we could rephrase the whole comment section above?
|
Please fix the DCO CI error first. |
Signed-off-by: Andre Graf <andre@dergraf.org>
dergraf
force-pushed
the
keep-content-length-header
branch
from
302d91c to
264ad4a
Compare
WIP TODO:
send_body_without_waiting_for_header_responseCommit Message: Adds a new configuration knob
retain_content_length_headerto the ext_proc filter enabling that a side stream server can use STREAMED and FULL_DUPLEX_STREAMED body processing modes without envoy automatically switching to chunked transfer encoding for HTTP1 codec. Ifretain_content_length_headeris set totrueforces Envoy to keep the content length header (instead of switching to chunked encoding).Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #39035, #38291
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]