fix(otlp-http): limit response body size to prevent memory exhaustion#3501
fix(otlp-http): limit response body size to prevent memory exhaustion#3501SATVIKsynopsis wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3501 +/- ##
=======================================
- Coverage 84.1% 82.8% -1.3%
=======================================
Files 126 130 +4
Lines 26720 27289 +569
=======================================
+ Hits 22490 22622 +132
- Misses 4230 4667 +437 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| #[cfg(feature = "experimental-http-retry")] | ||
| use crate::retry_classification::http::classify_http_error; | ||
|
|
||
| /// This represents the maximum bytes read from an oltp hhtp response body. |
There was a problem hiding this comment.
| /// This represents the maximum bytes read from an oltp hhtp response body. | |
| /// This represents the maximum bytes read from an OTLP Http response body. |
|
|
||
| /// This represents the maximum bytes read from an oltp hhtp response body. | ||
| /// It guards against memory exhaustion from a malicious endpoint. | ||
| const MAX_RESPONSE_BODY_BYTES: usize = 10 * 1024 * 1024; |
There was a problem hiding this comment.
wondering what is a good limit to put here! 10MB feel high (just out of my intuition!)
Any prior arts to follow?
There was a problem hiding this comment.
I initially though to use a small figure like 5 mb but then thought for easier flow 10 mb could be used. any suggestions you have on this matter?
There was a problem hiding this comment.
i am thinking 4-5 mb could be good to use.
There was a problem hiding this comment.
.NET picked 4 MB. Let's pick that!
@lalitb do you know if there is a good max limit to pick?
There was a problem hiding this comment.
4 MB would be nice to go with
| // Return the response, consuming the body to save a copy | ||
| Ok(response.into_body()) | ||
| let body = response.into_body(); | ||
| if body.len() > MAX_RESPONSE_BODY_BYTES { |
There was a problem hiding this comment.
This fix does not prevent memory exhaustion yet. By the time this code checks body.len(), the HTTP client has already read the full response into Response<Bytes>. So a malicious endpoint can still force the large allocation; this only skips parsing after the memory is already used.
A better fix is to apply the limit while reading the response body in the HTTP client implementations, like the reqwest and hyper clients in opentelemetry-http. They should read the body in chunks and stop once it crosses the max size. Then OtlpHttpClient can safely consume the already-limited Bytes.
There was a problem hiding this comment.
Thanks. That makes sense. I will fix it according to the suggestion
|
The current fix checks the size after the full body is already buffered. To truly prevent the allocation, the body needs to be read in chunks and aborted early as suggested earlier as well. I have an approach using |
Yes, that approach sounds right. The key requirement is that the limit is enforced while the body is being read, before it becomes Can we try to accommodate this in next release - #3508 - @cijothomas if it can be fixed soon |
|
Updated the fix to use chunked reading in all three HTTP client implementations as in reqwest async, reqwest blocking and hyper so the limit is enforced while the body is being read before it becomes |
| let mut body_bytes = bytes::BytesMut::new(); | ||
| let mut body = response.into_body(); | ||
| while let Some(frame) = body.frame().await { | ||
| if let Ok(frame) = frame { |
There was a problem hiding this comment.
I think we should not ignore the Err case here. Before this change, collect().await? would return the body read error to the caller. With if let Ok(frame) = frame, a mid-body read failure can be silently skipped and we may return a truncated body as if it succeeded.
| let headers = std::mem::take(response.headers_mut()); | ||
| let mut body_bytes = bytes::BytesMut::new(); | ||
| while let Some(chunk) = response.chunk().await? { | ||
| body_bytes.extend_from_slice(&chunk); |
There was a problem hiding this comment.
nit - can we do check before extending -
body_bytes.len() + chunk.len() > MAX_RESPONSE_BODY_BYTES
|
I have added the |
|
Hi, just checking in. let me know if any further changes are needed or if there's anything blocking the review. Happy to address any feedback. |
Fixes #3439
Changes
I traced all HTTP response handling to export_http_once in
opentelemetry-oltp/src/exporter/http/mod.rsand since all three signal exporters funnel through this one function, fixing it here covered everything. I also added aMAX_RESPONSE_BODY_BYTES (10 mb)cap on 2xx response bodies and stopped reading non-2xx bodies entirely. I also added a test with a mock client that returns an oversized body to verify the cap works.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes