From 5f2446a2df0ce02f7cbb34d1353e1a7992d8e1a0 Mon Sep 17 00:00:00 2001 From: Harshvardhan Shrivastava Date: Mon, 10 Aug 2026 21:07:37 +0530 Subject: [PATCH 1/3] OBE-11558 - conditions: stop an array-rooted event panicking the daemon --- lib/vector-core/src/event/vrl_target.rs | 128 ++++++++++++++++++++++++ src/conditions/equality.rs | 59 ++++++++++- src/conditions/vrl.rs | 67 ++++++++++++- 3 files changed, 244 insertions(+), 10 deletions(-) diff --git a/lib/vector-core/src/event/vrl_target.rs b/lib/vector-core/src/event/vrl_target.rs index 7ef2aec3b8..c76207db40 100644 --- a/lib/vector-core/src/event/vrl_target.rs +++ b/lib/vector-core/src/event/vrl_target.rs @@ -130,6 +130,38 @@ impl VrlTarget { ) } + /// Turn the target back into exactly one event, never fanning out. + /// + /// [`Self::into_events`] chooses its variant from the *root value type*, so an array root + /// becomes a multi-event fan-out whether or not anything was assigned. That is right for + /// `remap`, but wrong for a read-only caller such as a condition, which must get back the + /// event it was given - and an event can legitimately *arrive* array-rooted (the native + /// protobuf codec accepts one). Callers that assumed `One` and treated anything else as + /// unreachable could be panicked by input shape alone, including by an empty array. + /// + /// Here an array root stays a single event with an array root, so the round-trip is + /// shape-preserving for every possible input. + pub fn into_event(self, log_namespace: LogNamespace) -> Event { + match self { + VrlTarget::LogEvent(value, metadata) => match value { + value @ (Value::Object(_) | Value::Array(_)) => { + LogEvent::from_parts(value, metadata).into() + } + v => match log_namespace { + LogNamespace::Vector => LogEvent::from_parts(v, metadata).into(), + LogNamespace::Legacy => create_log_event(v, metadata).into(), + }, + }, + VrlTarget::Trace(value, metadata) => match value { + value @ (Value::Object(_) | Value::Array(_)) => { + TraceEvent::from(LogEvent::from_parts(value, metadata)).into() + } + v => TraceEvent::from(create_log_event(v, metadata)).into(), + }, + VrlTarget::Metric { metric, .. } => Event::Metric(metric), + } + } + /// Turn the target back into events. /// /// This returns an iterator of events as one event can be turned into multiple by assigning an @@ -1347,4 +1379,100 @@ mod test { // get single value (should be the last one) assert_eq!(metric.tag_value("foo"), Some("b".into())); } + + // OBE-11558: `into_events` picks its variant from the root value type, so an array-rooted + // event became a multi-event fan-out even though nothing was assigned. Read-only callers + // (conditions) treated anything but `One` as unreachable and panicked on input shape alone. + // A log event can legitimately arrive array-rooted over the native protobuf codec. + + fn empty_program_info() -> ProgramInfo { + ProgramInfo { + fallible: false, + abortable: false, + target_queries: vec![], + target_assignments: vec![], + } + } + + fn array_rooted_log(values: Vec) -> LogEvent { + LogEvent::from_parts(Value::Array(values), EventMetadata::default()) + } + + #[test] + fn into_event_keeps_an_array_root_as_one_event() { + for log_namespace in [LogNamespace::Legacy, LogNamespace::Vector] { + let log = array_rooted_log(vec![1.into(), 2.into()]); + let target = VrlTarget::new(Event::from(log), &empty_program_info(), false); + + let event = target.into_event(log_namespace); + + assert_eq!( + event.as_log().value(), + &Value::Array(vec![1.into(), 2.into()]), + "the array root must survive the round-trip intact ({log_namespace:?})" + ); + } + } + + #[test] + fn into_event_keeps_an_empty_array_root_as_one_event() { + // The sharpest case: `into_events` turns this into a fan-out of *zero* events, which is + // why blaming the fan-out on the condition was wrong - it comes purely from the input. + for log_namespace in [LogNamespace::Legacy, LogNamespace::Vector] { + let target = VrlTarget::new( + Event::from(array_rooted_log(vec![])), + &empty_program_info(), + false, + ); + + let event = target.into_event(log_namespace); + + assert_eq!(event.as_log().value(), &Value::Array(vec![])); + } + } + + #[test] + fn into_event_agrees_with_into_events_for_ordinary_roots() { + // Anything that was already `One` must be unaffected. + let roots = [ + Value::from(btreemap! { "a" => 1 }), + Value::from("hi"), + Value::from(42), + ]; + + for root in roots { + for log_namespace in [LogNamespace::Legacy, LogNamespace::Vector] { + let build = || { + VrlTarget::new( + Event::from(LogEvent::from_parts(root.clone(), EventMetadata::default())), + &empty_program_info(), + false, + ) + }; + + let via_into_event = build().into_event(log_namespace); + let via_into_events = match build().into_events(log_namespace) { + TargetEvents::One(event) => event, + _ => panic!("expected One for root {root:?}"), + }; + + assert_eq!(via_into_event, via_into_events, "root {root:?}"); + } + } + } + + #[test] + fn into_events_still_fans_out_an_array_root() { + // `remap` relies on this; the fix must not change it. + let target = VrlTarget::new( + Event::from(array_rooted_log(vec![1.into(), 2.into()])), + &empty_program_info(), + false, + ); + + match target.into_events(LogNamespace::Vector) { + TargetEvents::Logs(iter) => assert_eq!(iter.count(), 2), + _ => panic!("an array root must still fan out through into_events"), + } + } } diff --git a/src/conditions/equality.rs b/src/conditions/equality.rs index c806ec38dc..7cf2dd4072 100644 --- a/src/conditions/equality.rs +++ b/src/conditions/equality.rs @@ -7,7 +7,7 @@ use chrono::{DateTime, Utc}; use ordered_float::NotNan; use vector_lib::config::LogNamespace; use vector_lib::configurable::{configurable_component, ConfigurableString}; -use vector_lib::event::{TargetEvents, VrlTarget}; +use vector_lib::event::VrlTarget; use vector_lib::{event::Event, lookup::lookup_v2::ConfigTargetPath}; use vrl::compiler::{ProgramInfo, Target}; use vrl::core::Value; @@ -219,10 +219,10 @@ impl Equality { } pub(crate) fn into_event(target: VrlTarget, log_ns: LogNamespace) -> Event { - match target.into_events(log_ns) { - TargetEvents::One(event) => event, - _ => panic!("Event was modified in a condition. This is an internal compiler error."), - } + // Deliberately not `into_events`: that infers fan-out from the root value type, so an + // array-rooted event - which can arrive over the native protobuf codec, and may even be + // empty - would yield `Logs` and panic here on input shape alone. + target.into_event(log_ns) } pub(crate) fn check(&self, e: Event) -> (bool, Event) { @@ -619,6 +619,55 @@ mod tests { assert!(!eq_wrong.check(metric).0); } + // OBE-11558: an array-rooted log event made `into_events` report a fan-out, which + // `into_event` treated as an internal compiler error and panicked on. A transform panic + // becomes a fatal ShutdownError that exits the whole daemon, so input shape alone could kill + // Vector. Such an event can arrive over the native protobuf codec, and an *empty* array is + // enough. This is the fork-only sibling of the same bug in `conditions::vrl`. + fn array_rooted_event(values: Vec) -> Event { + Event::Log(LogEvent::from_parts( + Value::Array(values), + crate::event::EventMetadata::default(), + )) + } + + #[test] + fn check_on_array_rooted_event_does_not_panic() { + let eq = build(vec![(".foo", Constant::Integer(42))]); + let original = array_rooted_event(vec![Value::from(1), Value::from(2)]); + + let (matched, returned) = eq.check(original.clone()); + + // `.foo` cannot resolve against an array root, so the clause simply does not match. + assert!(!matched); + assert_eq!( + original, returned, + "a read-only condition must return the event unchanged" + ); + } + + #[test] + fn check_on_empty_array_rooted_event_does_not_panic() { + let eq = build(vec![(".foo", Constant::Integer(42))]); + let original = array_rooted_event(vec![]); + + let (matched, returned) = eq.check(original.clone()); + + assert!(!matched); + assert_eq!(original, returned); + } + + #[test] + fn check_with_context_on_array_rooted_event_does_not_panic() { + let eq = build(vec![(".foo", Constant::Integer(42))]); + let original = array_rooted_event(vec![]); + + let (result, returned) = eq.check_with_context(original.clone()); + + assert!(result.is_err()); + assert_eq!(original, returned); + } + // ---------- Equality::check_with_context ---------- #[test] diff --git a/src/conditions/vrl.rs b/src/conditions/vrl.rs index e8170e3ec5..5bca3f73cf 100644 --- a/src/conditions/vrl.rs +++ b/src/conditions/vrl.rs @@ -6,7 +6,6 @@ use vrl::diagnostic::Formatter; use vrl::value::Value; use crate::config::LogNamespace; -use crate::event::TargetEvents; use crate::{ conditions::{Condition, Conditional, ConditionalConfig}, event::{Event, VrlTarget}, @@ -103,10 +102,10 @@ impl Vrl { let timezone = TimeZone::default(); let result = Runtime::default().resolve(&mut target, &self.program, &timezone); - let original_event = match target.into_events(log_namespace) { - TargetEvents::One(event) => event, - _ => panic!("Event was modified in a condition. This is an internal compiler error."), - }; + // Deliberately not `into_events`: that infers fan-out from the root value type, so an + // array-rooted event - which can arrive over the native protobuf codec, and may even be + // empty - would yield `Logs` and panic here on input shape alone. + let original_event = target.into_event(log_namespace); (original_event, result) } } @@ -273,4 +272,62 @@ mod test { } } } + + // OBE-11558: an array-rooted log event made `into_events` report a fan-out, which this + // condition treated as an internal compiler error and panicked on. A transform panic becomes + // a fatal ShutdownError that exits the whole daemon, so input shape alone could kill Vector. + // An event can arrive array-rooted over the native protobuf codec, and an *empty* array is + // enough to trigger it. + mod array_rooted_events { + use super::*; + use vector_lib::event::{EventMetadata, LogEvent}; + use vrl::value::Value; + + fn array_rooted_event(values: Vec) -> Event { + Event::from(LogEvent::from_parts( + Value::Array(values), + EventMetadata::default(), + )) + } + + fn condition() -> Condition { + VrlConfig { + source: "true".to_owned(), + runtime: Default::default(), + } + .build(&Default::default()) + .expect("condition should build") + } + + #[test] + fn array_rooted_event_does_not_panic() { + let (result, event) = condition().check(array_rooted_event(vec![1.into(), 2.into()])); + + assert!(result); + assert_eq!( + event.as_log().value(), + &Value::Array(vec![1.into(), 2.into()]), + "a read-only condition must return the event unchanged" + ); + } + + #[test] + fn empty_array_rooted_event_does_not_panic() { + let (result, event) = condition().check(array_rooted_event(vec![])); + + assert!(result); + assert_eq!(event.as_log().value(), &Value::Array(vec![])); + } + + #[test] + fn object_rooted_event_is_unaffected() { + let (result, event) = condition().check(log_event!["foo" => "bar"]); + + assert!(result); + assert_eq!( + event.as_log().get("foo").map(|v| v.to_string_lossy()), + Some("bar".into()) + ); + } + } } From 61160b2f18d8eca29f3e4e2b34df151438254d65 Mon Sep 17 00:00:00 2001 From: Harshvardhan Shrivastava Date: Mon, 10 Aug 2026 21:08:06 +0530 Subject: [PATCH 2/3] OBE-11560 - secrets: stop a secret value altering config structure --- src/config/loading/secret.rs | 398 ++++++++++++++++++++++++++++++++++- src/config/vars.rs | 12 ++ 2 files changed, 403 insertions(+), 7 deletions(-) diff --git a/src/config/loading/secret.rs b/src/config/loading/secret.rs index 2107ec6757..8116d41aec 100644 --- a/src/config/loading/secret.rs +++ b/src/config/loading/secret.rs @@ -125,21 +125,162 @@ fn collect_secret_keys(input: &str, keys: &mut HashMap>) }); } +/// Cheap pre-filter: the overwhelming majority of configs contain no placeholder at all, and +/// this lets them skip the regex scan entirely. +const SECRET_MARKER: &str = "SECRET["; + +/// The lexical context a placeholder sits in, as far as a line-local scan can establish. +/// +/// Substitution happens on the raw config text *before* it is parsed, which is what preserves +/// the current behaviour that an unquoted placeholder yields a typed scalar (`port: SECRET[..]` +/// with a value of `8000` deserializes as an integer). The cost of that is a secret value can +/// otherwise terminate its enclosing scalar and become configuration structure, so each +/// substitution has to be treated according to where it lands. +#[derive(Debug, PartialEq, Eq)] +enum Context { + /// Inside a double-quoted scalar opened on this line. Escaping is portable here: TOML basic + /// strings, YAML double-quoted scalars and JSON strings share `\\`, `\"`, `\n`, `\r`, `\t` + /// and `\uXXXX`. + DoubleQuoted, + /// Everything else, and deliberately the default whenever the scan cannot positively + /// establish `DoubleQuoted`. + /// + /// This includes single-quoted scalars, because there is no portable escape: a TOML literal + /// string admits no escape sequence at all, so a `'` simply cannot be represented inside + /// one. It also covers comments, multi-line strings and YAML block scalars, whose state a + /// line-local scan cannot know. + Unknown, +} + +/// The text between the start of the placeholder's line and the placeholder itself. +fn line_prefix(input: &str, at: usize) -> &str { + let line_start = input[..at].rfind('\n').map_or(0, |i| i + 1); + &input[line_start..at] +} + +/// Establish the context of a placeholder from the text preceding it on its own line. +/// +/// Biased to return [`Context::Unknown`] whenever the answer is not certain. The two possible +/// misreadings are not symmetric, and this is what makes the bias the safe one: +/// +/// - Guessing `Unknown` when actually double-quoted means the value is required to be inert +/// instead of escaped. An inert value is equally harmless inside a string literal, so the +/// result is at worst a rejected secret with an actionable error. +/// - Guessing `DoubleQuoted` when actually unquoted would emit `\"`-style escapes into a bare +/// position and break config loading outright. +fn scan_context(prefix: &str) -> Context { + let bytes = prefix.as_bytes(); + let mut in_double = false; + let mut in_single = false; + let mut i = 0; + + while i < bytes.len() { + // A multi-line delimiter means state may have been carried in from an earlier line, so + // line-local reasoning no longer holds. + if bytes[i..].starts_with(br#"""""#) || bytes[i..].starts_with(b"'''") { + return Context::Unknown; + } + + match bytes[i] { + // Inside a double-quoted scalar a backslash escapes the next byte, so it cannot + // close the string. + b'\\' if in_double => { + i += 2; + continue; + } + b'"' if !in_single => in_double = !in_double, + b'\'' if !in_double => in_single = !in_single, + // A comment: the placeholder is not in a value position at all, but a newline in + // the value would still escape the comment and become structure. + b'#' if !in_double && !in_single => return Context::Unknown, + _ => {} + } + i += 1; + } + + if in_double { + Context::DoubleQuoted + } else { + Context::Unknown + } +} + +/// Escape a value so it cannot terminate the double-quoted scalar it is being spliced into. +fn escape_double_quoted(value: &str) -> String { + let mut escaped = String::with_capacity(value.len()); + for c in value.chars() { + match c { + '\\' => escaped.push_str(r"\\"), + '"' => escaped.push_str(r#"\""#), + '\n' => escaped.push_str(r"\n"), + '\r' => escaped.push_str(r"\r"), + '\t' => escaped.push_str(r"\t"), + c if c.is_control() => escaped.push_str(&format!("\\u{:04x}", c as u32)), + c => escaped.push(c), + } + } + escaped +} + +/// Whether a value can be spliced into an unquoted position without being able to become +/// anything other than a single scalar, in any supported format. +/// +/// An allowlist rather than a denylist, because the set of characters that are structurally +/// significant somewhere across TOML, YAML and JSON is large and easy to under-enumerate - a +/// bare `,` is enough to add an element in a YAML flow sequence, with no quote or newline +/// involved. The permitted set still covers what actually appears unquoted in practice: ports +/// and other numbers, versions, hostnames, paths, and base64 (hence `+`, `/` and `=`). +fn is_inert(value: &str) -> bool { + let mut chars = value.chars(); + // A leading `-`, `?`, `&`, `*`, `!` or similar is a YAML indicator, so require the first + // character to be unambiguous. + match chars.next() { + None => true, + Some(first) if first.is_ascii_alphanumeric() || first == '_' => value + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-' | '+' | '/' | '=')), + Some(_) => false, + } +} + pub fn interpolate(input: &str, secrets: &HashMap) -> Result> { + if !input.contains(SECRET_MARKER) { + return Ok(input.to_owned()); + } + let mut errors = Vec::::new(); let output = COLLECTOR .replace_all(input, |caps: &Captures<'_>| { - caps.get(1) + let matched = caps.get(0).expect("capture group 0 always matches"); + let Some(value) = caps + .get(1) .and_then(|b| caps.get(2).map(|k| (b, k))) .and_then(|(b, k)| secrets.get(&format!("{}.{}", b.as_str(), k.as_str()))) - .cloned() - .unwrap_or_else(|| { + else { + errors.push(format!( + "Unable to find secret replacement for {}.", + matched.as_str() + )); + return String::new(); + }; + + // The value is substituted into text that has not been parsed yet, so it must not be + // able to close its scalar and introduce configuration of its own. + match scan_context(line_prefix(input, matched.start())) { + Context::DoubleQuoted => escape_double_quoted(value), + Context::Unknown if is_inert(value) => value.clone(), + Context::Unknown => { + // Deliberately does not include the value. errors.push(format!( - "Unable to find secret replacement for {}.", - caps.get(0).unwrap().as_str() + "Secret {} resolves to a value containing characters that could alter the \ + configuration structure when substituted outside a double-quoted string. \ + Enclose the placeholder in double quotes, for example \"{}\".", + matched.as_str(), + matched.as_str() )); - "".to_string() - }) + String::new() + } + } }) .into_owned(); if errors.is_empty() { @@ -157,6 +298,249 @@ mod tests { use super::{collect_secret_keys, interpolate}; + /// OBE-11560: a retrieved secret is spliced into the raw config *text* before it is parsed, so + /// a value carrying a quote and a newline can terminate its scalar and become configuration - + /// demonstrated end to end with a working `exec` source. + /// + /// Substitution has to stay pre-parse to preserve typing (an unquoted placeholder yields a + /// typed scalar, which is the only form that works for a numeric field), so instead each + /// substitution is treated according to the context it lands in: escaped inside a + /// double-quoted scalar, required to be inert anywhere else. + mod structural_injection { + use super::*; + + /// A value that escapes its string and adds a source, in TOML. + const TOML_BREAKOUT: &str = + "x\"\n[sources.injected]\ntype = \"exec\"\ncommand = [\"id\"]\ny = \""; + /// The YAML equivalent; note the shape differs from the TOML one. + const YAML_BREAKOUT: &str = "x\"\ninjected:\n type: exec\n"; + + fn secrets(value: &str) -> HashMap { + vec![("b.k".to_string(), value.to_string())] + .into_iter() + .collect() + } + + // --- the injection is closed ------------------------------------------------------ + + #[test] + fn toml_breakout_inside_quotes_is_escaped_not_structural() { + let config = interpolate(r#"password = "SECRET[b.k]""#, &secrets(TOML_BREAKOUT)) + .expect("a quoted placeholder should substitute"); + + let parsed: toml::Table = toml::from_str(&config).expect("must still be valid TOML"); + + assert_eq!( + parsed.keys().collect::>(), + vec!["password"], + "the secret must not have introduced a table" + ); + assert_eq!( + parsed["password"].as_str(), + Some(TOML_BREAKOUT), + "the value must round-trip verbatim" + ); + } + + #[test] + fn yaml_breakout_inside_quotes_is_escaped_not_structural() { + let config = interpolate(r#"password: "SECRET[b.k]""#, &secrets(YAML_BREAKOUT)) + .expect("a quoted placeholder should substitute"); + + let parsed: serde_yaml::Value = + serde_yaml::from_str(&config).expect("must still be valid YAML"); + let map = parsed.as_mapping().expect("a mapping"); + + assert_eq!(map.len(), 1, "the secret must not have introduced a key"); + assert_eq!(map["password"].as_str(), Some(YAML_BREAKOUT)); + } + + #[test] + fn breakout_outside_quotes_is_rejected() { + let error = interpolate("password: SECRET[b.k]", &secrets(YAML_BREAKOUT)) + .expect_err("an unquoted placeholder must not accept a structural value"); + + assert_eq!(error.len(), 1); + assert!(error[0].contains("could alter the configuration structure")); + assert!( + !error[0].contains("injected"), + "the error must not leak the secret value" + ); + } + + #[test] + fn yaml_flow_sequence_comma_injection_is_rejected() { + // A comma alone adds an element in a flow sequence - no quote, no newline. This is the + // case a `\r\n\"` denylist misses, which is why unquoted values use an allowlist. + let error = interpolate("hosts: [SECRET[b.k]]", &secrets("a, b")) + .expect_err("a comma must not be accepted in an unquoted position"); + + assert!(error[0].contains("could alter the configuration structure")); + } + + #[test] + fn newline_in_a_comment_cannot_escape_into_structure() { + // The placeholder is not in a value position, but a newline would end the comment. + let error = interpolate("# see SECRET[b.k]", &secrets("x\ninjected: true")) + .expect_err("a comment must not accept a value containing a newline"); + + assert!(error[0].contains("could alter the configuration structure")); + } + + // --- today's behaviour is preserved ---------------------------------------------- + + #[test] + fn unquoted_numeric_secret_still_parses_as_an_integer() { + // The case that matters: this is the only form in which a numeric secret works, and it + // works *because* substitution precedes parsing. + let config = interpolate("port: SECRET[b.k]", &secrets("8000")) + .expect("a numeric secret is inert and must be accepted"); + + assert_eq!(config, "port: 8000"); + + let parsed: serde_yaml::Value = serde_yaml::from_str(&config).unwrap(); + assert_eq!( + parsed["port"].as_u64(), + Some(8000), + "an unquoted numeric secret must stay an integer" + ); + } + + #[test] + fn quoted_numeric_secret_still_parses_as_a_string() { + let config = interpolate(r#"port: "SECRET[b.k]""#, &secrets("8000")).unwrap(); + + let parsed: serde_yaml::Value = serde_yaml::from_str(&config).unwrap(); + assert_eq!(parsed["port"].as_str(), Some("8000")); + assert!( + parsed["port"].as_u64().is_none(), + "quoting is what makes it a string, both before and after this change" + ); + } + + #[test] + fn rich_password_round_trips_inside_quotes() { + // Realistic secrets contain characters that are structural somewhere; inside a + // double-quoted scalar they are simply escaped rather than rejected. + let password = r#"p@ss:w#rd,{}[]\|<>'"~$%^&*()"#; + let config = interpolate(r#"password = "SECRET[b.k]""#, &secrets(password)).unwrap(); + + let parsed: toml::Table = toml::from_str(&config).expect("must be valid TOML"); + assert_eq!(parsed["password"].as_str(), Some(password)); + assert_eq!(parsed.len(), 1); + } + + #[test] + fn base64_token_is_accepted_unquoted() { + let token = "aGVsbG8rd29ybGQvPT0="; + let config = interpolate("token: SECRET[b.k]", &secrets(token)).unwrap(); + + assert_eq!(config, format!("token: {token}")); + } + + #[test] + fn input_without_a_placeholder_is_returned_unchanged() { + let config = "sources:\n in:\n type: stdin\n"; + assert_eq!(Ok(config.to_string()), interpolate(config, &secrets("v"))); + } + + // --- context-scanning caveats, all of which must fail closed --------------------- + + #[test] + fn single_quoted_context_requires_an_inert_value() { + // A TOML literal string admits no escape sequence at all, so a `'` cannot be + // represented inside one; there is no portable escaping and the value must be inert. + let error = interpolate("password = 'SECRET[b.k]'", &secrets("has'quote")) + .expect_err("a single-quoted context must not be escaped into"); + assert!(error[0].contains("could alter the configuration structure")); + + // An inert value is still fine there. + assert_eq!( + Ok("password = 'abc123'".to_string()), + interpolate("password = 'SECRET[b.k]'", &secrets("abc123")) + ); + } + + #[test] + fn toml_multiline_string_on_the_same_line_requires_an_inert_value() { + let error = interpolate(r#"x = """a SECRET[b.k]"#, &secrets("has\"quote")) + .expect_err("a multi-line delimiter defeats line-local scanning"); + assert!(error[0].contains("could alter the configuration structure")); + } + + #[test] + fn toml_multiline_string_opened_on_an_earlier_line_requires_an_inert_value() { + // The scan only sees the placeholder's own line, which has no quote on it at all, so + // it must not conclude the value is safely quoted. + let input = "x = \"\"\"\nsome text SECRET[b.k]\n\"\"\"\n"; + let error = interpolate(input, &secrets("has\"quote")) + .expect_err("carried-in quote state must fail closed"); + assert!(error[0].contains("could alter the configuration structure")); + } + + #[test] + fn yaml_block_scalar_requires_an_inert_value() { + let input = "script: |\n echo SECRET[b.k]\n"; + let error = interpolate(input, &secrets("x\ninjected: true")) + .expect_err("a block scalar must fail closed"); + assert!(error[0].contains("could alter the configuration structure")); + + // ...and an inert value is still substituted there. + let ok = interpolate(input, &secrets("8000")).unwrap(); + assert_eq!(ok, "script: |\n echo 8000\n"); + } + + #[test] + fn an_escaped_quote_does_not_close_the_enclosing_string() { + // `\"` must not be read as terminating the scalar, or the value would be treated as + // unquoted and a legitimate rich secret would be rejected. + let config = interpolate(r#"x = "a\" then SECRET[b.k]""#, &secrets("has\"quote")) + .expect("still inside the string, so the value is escaped"); + + let parsed: toml::Table = toml::from_str(&config).expect("must be valid TOML"); + assert_eq!(parsed["x"].as_str(), Some("a\" then has\"quote")); + } + + #[test] + fn a_closed_quote_before_the_placeholder_is_not_quoted_context() { + // `"a"` opens and closes, so the placeholder that follows is unquoted. + let error = interpolate(r#"x = "a" SECRET[b.k]"#, &secrets("has\"quote")) + .expect_err("a balanced pair of quotes leaves the placeholder unquoted"); + assert!(error[0].contains("could alter the configuration structure")); + } + + #[test] + fn each_placeholder_is_judged_on_its_own_line() { + let input = "a = \"SECRET[b.k]\"\nb = SECRET[b.k]\n"; + + // The quoted one would be fine, the bare one is not, so the whole load fails. + let error = interpolate(input, &secrets("has\"quote")).expect_err("bare use must fail"); + assert_eq!( + error.len(), + 1, + "only the bare placeholder should be faulted" + ); + } + + // --- the TOML-bare caveat, recorded rather than assumed -------------------------- + + #[test] + fn toml_cannot_use_an_unquoted_placeholder_at_all() { + // Backend discovery (`SecretBackendLoader::prepare`) returns the text *without* + // substituting, and the loader then parses it. TOML has no unquoted string form, so a + // bare placeholder fails that first parse - meaning the unquoted-numeric form works in + // YAML only. Recorded here so the asymmetry is not rediscovered later. + assert!( + toml::from_str::("port = SECRET[b.k]").is_err(), + "a bare placeholder is not valid TOML" + ); + assert!( + serde_yaml::from_str::("port: SECRET[b.k]").is_ok(), + "but it is a valid YAML plain scalar" + ); + } + } + #[test] fn replacement() { let secrets: HashMap = vec![ diff --git a/src/config/vars.rs b/src/config/vars.rs index 63d6e08b2d..78b4f79876 100644 --- a/src/config/vars.rs +++ b/src/config/vars.rs @@ -20,6 +20,18 @@ pub static ENVIRONMENT_VARIABLE_INTERPOLATION_REGEX: LazyLock = LazyLock: }); /// Result +/// +/// Note this splices variable values into the raw config text before it is parsed, so a value +/// containing a quote and a newline can terminate its scalar and become configuration structure. +/// That is the same shape as the secret-interpolation issue fixed in +/// `config::loading::secret::interpolate`, and it is deliberately *not* guarded here: environment +/// variables are set by whoever deploys Vector, who already authors the config, so no trust +/// boundary is crossed. Values are also legitimately used to inject config fragments rather than +/// single scalars, which the checks applied to secrets would break. +/// +/// If a variable source is ever introduced that is not controlled by the config author, this +/// function needs the same treatment `secret::interpolate` received - do not assume the pattern is +/// safe because it appears here. pub fn interpolate(input: &str, vars: &HashMap) -> Result> { let mut errors = Vec::new(); From 30b9dc3c1d0fa733743b30eb34a9540eb08220f8 Mon Sep 17 00:00:00 2001 From: Harshvardhan Shrivastava Date: Tue, 11 Aug 2026 09:37:57 +0530 Subject: [PATCH 3/3] OBE-11560 - secrets: require a balanced quoted scalar before escaping --- src/config/loading/secret.rs | 186 ++++++++++++++++++++++++++++++----- 1 file changed, 162 insertions(+), 24 deletions(-) diff --git a/src/config/loading/secret.rs b/src/config/loading/secret.rs index 8116d41aec..26210e1d34 100644 --- a/src/config/loading/secret.rs +++ b/src/config/loading/secret.rs @@ -152,44 +152,68 @@ enum Context { Unknown, } -/// The text between the start of the placeholder's line and the placeholder itself. -fn line_prefix(input: &str, at: usize) -> &str { - let line_start = input[..at].rfind('\n').map_or(0, |i| i + 1); - &input[line_start..at] +/// Bytes a string opener may follow. +/// +/// A quote is only the start of a scalar in a value position: after a `=` or `:` (TOML and YAML +/// assignment), after a `,`, `[` or `{` (flow collections), after a `-` (a YAML sequence entry), +/// or at the start of a line. Anywhere else the quote is an ordinary character *inside* an +/// unquoted scalar - a YAML plain scalar such as `prod"x` is a single value containing a quote, +/// not the beginning of a string. +const OPENER_PREDECESSORS: &[u8] = b"=:,[{-"; + +/// Whether the quote at `at` is opening a scalar rather than sitting inside an unquoted one. +fn opens_scalar(line: &[u8], at: usize) -> bool { + line[..at] + .iter() + .rev() + .find(|b| !b.is_ascii_whitespace()) + .is_none_or(|b| OPENER_PREDECESSORS.contains(b)) } -/// Establish the context of a placeholder from the text preceding it on its own line. +/// Classify the position a placeholder occupies on its own line. +/// +/// Returns [`Context::DoubleQuoted`] only when the placeholder is enclosed by a **balanced, +/// same-line** double-quoted scalar: a quote in an opening position before it, and a closing +/// quote after it. Everything else is [`Context::Unknown`]. /// -/// Biased to return [`Context::Unknown`] whenever the answer is not certain. The two possible -/// misreadings are not symmetric, and this is what makes the bias the safe one: +/// Both halves of that test are load-bearing, and the reason is worth recording because the +/// obvious weaker version is unsafe. It is tempting to argue that misclassifying an unquoted +/// position as quoted is harmless because escaping would break parsing and so fail loudly. That +/// argument is **false**: [`escape_double_quoted`] neutralises `"`, `\` and control characters, +/// which is everything that matters *inside* a string, but it deliberately leaves `,`, `:`, `#` +/// and brackets alone - and those are exactly what is structural *outside* one. So a wrong +/// `DoubleQuoted` does not fail loudly, it silently skips the [`is_inert`] check and lets a value +/// inject configuration. Requiring an opener position defeats a stray quote in a plain scalar +/// (`{env: prod"x, region: SECRET[..]}`), and requiring a closer defeats an unbalanced one. /// -/// - Guessing `Unknown` when actually double-quoted means the value is required to be inert -/// instead of escaped. An inert value is equally harmless inside a string literal, so the -/// result is at worst a rejected secret with an actionable error. -/// - Guessing `DoubleQuoted` when actually unquoted would emit `\"`-style escapes into a bare -/// position and break config loading outright. -fn scan_context(prefix: &str) -> Context { - let bytes = prefix.as_bytes(); +/// Misclassifying the other way is safe: an inert value is equally harmless inside a string, so +/// the worst outcome is a rejected secret with an actionable error. +fn classify(input: &str, start: usize, end: usize) -> Context { + let line_start = input[..start].rfind('\n').map_or(0, |i| i + 1); + let prefix = input[line_start..start].as_bytes(); + let mut in_double = false; let mut in_single = false; let mut i = 0; - while i < bytes.len() { + while i < prefix.len() { // A multi-line delimiter means state may have been carried in from an earlier line, so // line-local reasoning no longer holds. - if bytes[i..].starts_with(br#"""""#) || bytes[i..].starts_with(b"'''") { + if prefix[i..].starts_with(br#"""""#) || prefix[i..].starts_with(b"'''") { return Context::Unknown; } - match bytes[i] { + match prefix[i] { // Inside a double-quoted scalar a backslash escapes the next byte, so it cannot // close the string. b'\\' if in_double => { i += 2; continue; } - b'"' if !in_single => in_double = !in_double, - b'\'' if !in_double => in_single = !in_single, + b'"' if in_double => in_double = false, + b'\'' if in_single => in_single = false, + b'"' if !in_single && opens_scalar(prefix, i) => in_double = true, + b'\'' if !in_double && opens_scalar(prefix, i) => in_single = true, // A comment: the placeholder is not in a value position at all, but a newline in // the value would still escape the comment and become structure. b'#' if !in_double && !in_single => return Context::Unknown, @@ -198,11 +222,30 @@ fn scan_context(prefix: &str) -> Context { i += 1; } - if in_double { - Context::DoubleQuoted - } else { - Context::Unknown + if !in_double { + return Context::Unknown; } + + // The scalar must also close on this line. Without this a stray opener-positioned quote + // earlier in the line would make an unquoted placeholder look quoted. + let line_end = input[end..] + .find('\n') + .map_or(input.len(), |offset| end + offset); + let suffix = input[end..line_end].as_bytes(); + let mut j = 0; + while j < suffix.len() { + match suffix[j] { + b'\\' => { + j += 2; + continue; + } + b'"' => return Context::DoubleQuoted, + _ => {} + } + j += 1; + } + + Context::Unknown } /// Escape a value so it cannot terminate the double-quoted scalar it is being spliced into. @@ -266,7 +309,7 @@ pub fn interpolate(input: &str, secrets: &HashMap) -> Result escape_double_quoted(value), Context::Unknown if is_inert(value) => value.clone(), Context::Unknown => { @@ -378,6 +421,101 @@ mod tests { assert!(error[0].contains("could alter the configuration structure")); } + #[test] + fn stray_quote_in_a_plain_scalar_does_not_enable_injection() { + // A single stray `"` inside a YAML plain scalar makes the line-local quote count odd. + // If that is read as "inside a double-quoted string" the value gets escaped instead of + // being required to be inert - and escaping does nothing to `,` or `:`, so structure + // is injected. + let config = interpolate( + r#"tags: {env: prod"x, region: SECRET[b.k]}"#, + &secrets("us, admin: true"), + ); + + if let Ok(rendered) = &config { + let parsed: serde_yaml::Value = serde_yaml::from_str(rendered).expect("valid YAML"); + let tags = parsed["tags"].as_mapping().expect("a mapping"); + assert!( + !tags.contains_key(serde_yaml::Value::from("admin")), + "secret injected an `admin` key: {rendered}" + ); + } + } + + #[test] + fn stray_quote_with_a_later_quote_on_the_line_is_still_rejected() { + // Requiring a closing quote is not sufficient on its own: here one exists further + // along the line, so only the opener-position test rules this out. + let error = interpolate( + r#"tags: {env: prod"x, region: SECRET[b.k], zone: "z"}"#, + &secrets("us, admin: true"), + ) + .expect_err("a stray quote must not make this look like quoted context"); + + assert!(error[0].contains("could alter the configuration structure")); + } + + #[test] + fn stray_quote_does_not_corrupt_the_secret_value() { + // The same misclassification also silently mangled values: the secret would be + // escaped as `has\"quote` in a position where nothing is escaped, so the config would + // receive a value the secret never contained. It must be rejected instead. + let error = interpolate(r#"key: a"b SECRET[b.k]"#, &secrets(r#"has"quote"#)) + .expect_err("an unquoted position must not silently escape the value"); + + assert!(error[0].contains("could alter the configuration structure")); + } + + #[test] + fn an_opener_quote_never_closed_on_the_line_requires_an_inert_value() { + let error = interpolate(r#"key: "unclosed SECRET[b.k]"#, &secrets(r#"has"quote"#)) + .expect_err("an unbalanced scalar must fail closed"); + assert!(error[0].contains("could alter the configuration structure")); + + // An inert value is still fine there. + assert_eq!( + Ok(r#"key: "unclosed 8000"#.to_string()), + interpolate(r#"key: "unclosed SECRET[b.k]"#, &secrets("8000")) + ); + } + + #[test] + fn a_genuinely_quoted_scalar_in_a_flow_mapping_is_escaped() { + // The mirror of the rejection cases: when the placeholder really is inside a quoted + // scalar, a value full of structural characters must still be accepted. + let value = "us, admin: true"; + let config = interpolate( + r#"tags: {env: prod, region: "SECRET[b.k]"}"#, + &secrets(value), + ) + .expect("a properly quoted placeholder must accept any value"); + + let parsed: serde_yaml::Value = serde_yaml::from_str(&config).unwrap(); + let tags = parsed["tags"].as_mapping().unwrap(); + assert_eq!(tags.len(), 2, "no key may have been injected"); + assert_eq!(tags["region"].as_str(), Some(value)); + } + + #[test] + fn json_and_sequence_positions_are_recognised_as_quoted() { + let value = r#"a,b:c"d"#; + + // JSON: the opener follows a `:`. + let json = interpolate(r#"{"password": "SECRET[b.k]"}"#, &secrets(value)).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).expect("valid JSON"); + assert_eq!(parsed["password"].as_str(), Some(value)); + + // YAML sequence entry: the opener follows a `-`. + let yaml = interpolate( + r#"hosts: + - "SECRET[b.k]""#, + &secrets(value), + ) + .unwrap(); + let parsed: serde_yaml::Value = serde_yaml::from_str(&yaml).expect("valid YAML"); + assert_eq!(parsed["hosts"][0].as_str(), Some(value)); + } + #[test] fn newline_in_a_comment_cannot_escape_into_structure() { // The placeholder is not in a value position, but a newline would end the comment.