From 8358e1f98955aae10e93f2046906e525a6e5589b Mon Sep 17 00:00:00 2001 From: gruebel Date: Fri, 14 Mar 2025 09:24:48 +0100 Subject: [PATCH 1/2] validate Baggage key by W3C standards --- opentelemetry/CHANGELOG.md | 1 + opentelemetry/benches/baggage.rs | 8 +++--- opentelemetry/src/baggage.rs | 48 +++++++++++++++++++++++++++----- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index dbe051e88a..18955a1969 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -13,6 +13,7 @@ - Align `Baggage.remove()` signature with `.get()` to take the key as a reference - `Baggage` can't be retrieved from the `Context` directly anymore and needs to be accessed via `context.baggage()` - `with_baggage()` and `current_with_baggage()` override any existing `Baggage` in the `Context` + - `Baggage` keys can't be empty and don't allow `"(),/:;<=>?@[\]{}` chars (see [RFC7230, Section 3.2.6](https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6)) - Changed `Context` to use a stack to properly handle out of order dropping of `ContextGuard`. This imposes a limit of `65535` nested contexts on a single thread. See #[2378](https://github.com/open-telemetry/opentelemetry-rust/pull/2284) and #[1887](https://github.com/open-telemetry/opentelemetry-rust/issues/1887). - Added additional `name: Option<&str>` parameter to the `event_enabled` method on the `Logger` trait. This allows implementations (SDK, processor, exporters) diff --git a/opentelemetry/benches/baggage.rs b/opentelemetry/benches/baggage.rs index 1854fcd2e3..706353ba5f 100644 --- a/opentelemetry/benches/baggage.rs +++ b/opentelemetry/benches/baggage.rs @@ -11,10 +11,10 @@ const MAX_KEY_VALUE_PAIRS: usize = 64; // Apple M4 Pro // Total Number of Cores: 14 (10 performance and 4 efficiency) // Results: -// set_baggage_static_key_value 12 ns -// set_baggage_static_key 28 ns -// set_baggage_dynamic 60 ns -// set_baggage_dynamic_with_metadata 112 ns +// set_baggage_static_key_value 14 ns +// set_baggage_static_key 26 ns +// set_baggage_dynamic 54 ns +// set_baggage_dynamic_with_metadata 75 ns fn criterion_benchmark(c: &mut Criterion) { set_baggage_static_key_value(c); diff --git a/opentelemetry/src/baggage.rs b/opentelemetry/src/baggage.rs index 69447f5c36..36fbcdac46 100644 --- a/opentelemetry/src/baggage.rs +++ b/opentelemetry/src/baggage.rs @@ -29,6 +29,12 @@ static DEFAULT_BAGGAGE: OnceLock = OnceLock::new(); const MAX_KEY_VALUE_PAIRS: usize = 64; const MAX_LEN_OF_ALL_PAIRS: usize = 8192; +// https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6 +const INVALID_ASCII_KEY_CHARS: [u8; 17] = [ + b'(', b')', b',', b'/', b':', b';', b'<', b'=', b'>', b'?', b'@', b'[', b'\\', b']', b'{', + b'}', b'"', +]; + /// Returns the default baggage, ensuring it is initialized only once. #[inline] fn get_default_baggage() -> &'static Baggage { @@ -160,16 +166,14 @@ impl Baggage { S: Into, { let (key, value, metadata) = (key.into(), value.into(), metadata.into()); - if !key.as_str().is_ascii() { - return None; - } - let entry_content_len = - key_value_metadata_bytes_size(key.as_str(), value.as_str(), metadata.as_str()); let entries_count = self.inner.len(); match self.inner.entry(key) { Entry::Occupied(mut occupied_entry) => { + let key_str = occupied_entry.key().as_str(); + let entry_content_len = + key_value_metadata_bytes_size(key_str, value.as_str(), metadata.as_str()); let prev_content_len = key_value_metadata_bytes_size( - occupied_entry.key().as_str(), + key_str, occupied_entry.get().0.as_str(), occupied_entry.get().1.as_str(), ); @@ -181,9 +185,15 @@ impl Baggage { Some(occupied_entry.insert((value, metadata))) } Entry::Vacant(vacant_entry) => { + let key_str = vacant_entry.key().as_str(); + if !Self::is_key_valid(key_str.as_bytes()) { + return None; + } if entries_count == MAX_KEY_VALUE_PAIRS { return None; } + let entry_content_len = + key_value_metadata_bytes_size(key_str, value.as_str(), metadata.as_str()); let new_content_len = self.kv_content_len + entry_content_len; if new_content_len > MAX_LEN_OF_ALL_PAIRS { return None; @@ -215,11 +225,18 @@ impl Baggage { pub fn iter(&self) -> Iter<'_> { self.into_iter() } + + fn is_key_valid(key: &[u8]) -> bool { + !key.is_empty() + && key + .iter() + .all(|b| b.is_ascii() && !INVALID_ASCII_KEY_CHARS.contains(b)) + } } /// Get the number of bytes for one key-value pair fn key_value_metadata_bytes_size(key: &str, value: &str, metadata: &str) -> usize { - key.bytes().len() + value.bytes().len() + metadata.bytes().len() + key.len() + value.len() + metadata.len() } /// An iterator over the entries of a [`Baggage`]. @@ -619,4 +636,21 @@ mod tests { baggage.remove("foo"); assert!(baggage.is_empty()); } + + #[test] + fn test_insert_invalid_key() { + let mut baggage = Baggage::default(); + + // empty + baggage.insert("", "1"); + assert!(baggage.is_empty()); + + // non-ascii + baggage.insert("Grüße", "1"); + assert!(baggage.is_empty()); + + // invalid ascii chars + baggage.insert("(example)", "1"); + assert!(baggage.is_empty()); + } } From 2ffc5bbd6ce94d4cf703b7649f9a143fd95a1e30 Mon Sep 17 00:00:00 2001 From: gruebel Date: Tue, 18 Mar 2025 23:10:14 +0100 Subject: [PATCH 2/2] restrict baggage key further --- opentelemetry/CHANGELOG.md | 2 +- opentelemetry/src/baggage.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index 18955a1969..00ab0be983 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -13,7 +13,7 @@ - Align `Baggage.remove()` signature with `.get()` to take the key as a reference - `Baggage` can't be retrieved from the `Context` directly anymore and needs to be accessed via `context.baggage()` - `with_baggage()` and `current_with_baggage()` override any existing `Baggage` in the `Context` - - `Baggage` keys can't be empty and don't allow `"(),/:;<=>?@[\]{}` chars (see [RFC7230, Section 3.2.6](https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6)) + - `Baggage` keys can't be empty and only allow ASCII visual chars, except `"(),/:;<=>?@[\]{}` (see [RFC7230, Section 3.2.6](https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6)) - Changed `Context` to use a stack to properly handle out of order dropping of `ContextGuard`. This imposes a limit of `65535` nested contexts on a single thread. See #[2378](https://github.com/open-telemetry/opentelemetry-rust/pull/2284) and #[1887](https://github.com/open-telemetry/opentelemetry-rust/issues/1887). - Added additional `name: Option<&str>` parameter to the `event_enabled` method on the `Logger` trait. This allows implementations (SDK, processor, exporters) diff --git a/opentelemetry/src/baggage.rs b/opentelemetry/src/baggage.rs index 36fbcdac46..43d796bfd6 100644 --- a/opentelemetry/src/baggage.rs +++ b/opentelemetry/src/baggage.rs @@ -230,7 +230,7 @@ impl Baggage { !key.is_empty() && key .iter() - .all(|b| b.is_ascii() && !INVALID_ASCII_KEY_CHARS.contains(b)) + .all(|b| b.is_ascii_graphic() && !INVALID_ASCII_KEY_CHARS.contains(b)) } }