From aa05a9c8e1b0e0d39683522b2f6e1df8fed40b43 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Sat, 4 Jul 2026 00:08:07 -0400 Subject: [PATCH 1/2] fix(operator): resolve_string mishandled ECS-native :jsonKey:: suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found via live E2E testing right after #1285 merged: applying a real manifest using the ECS-native valueFrom ARN format (the one the README itself documents as Format 1) failed with 'failed to resolve TELEGRAM_BOT_TOKEN', because resolve_string passed the full value - including the trailing ::: suffix - straight to GetSecretValue's secret_id parameter. That suffix is purely an ECS valueFrom convention (resolved by ECS itself at container launch, via register_task_definition), not something the Secrets Manager API understands; AWS docs confirm SecretId only ever accepts a plain ARN or name. The previous doc comment's claim that 'GetSecretValue resolves natively' was simply wrong. Fix: add split_ecs_json_key_suffix to detect and strip the ECS-only suffix from an ARN-shaped value, then extract the JSON key manually - the same way the aws-sm://...#... path already did. resolve_value_from (used for the actual ECS task definition, where ECS does understand the suffix) is unaffected. Verified live: re-ran oabctl apply against the real manifest (TELEGRAM_BOT_TOKEN as an ECS-native ARN with the JSON-key suffix) - now prints '✓ Telegram webhook registered: Webhook was set' instead of the previous resolution failure. Testing: cargo build/clippy --all-targets -D warnings/test clean on macmini; 29/29 tests pass (3 new, including the exact real-world ARN value that surfaced the bug as a regression test). --- operator/src/secrets.rs | 115 ++++++++++++++++++++++++++++++---------- 1 file changed, 87 insertions(+), 28 deletions(-) diff --git a/operator/src/secrets.rs b/operator/src/secrets.rs index d284f60d9..815e6f38c 100644 --- a/operator/src/secrets.rs +++ b/operator/src/secrets.rs @@ -2,9 +2,10 @@ //! //! Values can be either a Secrets Manager reference in ECS-native //! `valueFrom` format directly (a full ARN, optionally suffixed with -//! `:::` to extract one field of a JSON secret), or the same -//! `aws-sm://#` shorthand openab itself uses for -//! in-app secret refs (see `crates/openab-core/src/secrets.rs`) — kept +//! `:::` to extract one field of a JSON secret — an ECS-only +//! convention; the Secrets Manager API itself has no knowledge of it), or +//! the same `aws-sm://#` shorthand openab itself uses +//! for in-app secret refs (see `crates/openab-core/src/secrets.rs`) — kept //! identical here so a manifest author can write one convention across both //! `spec.secrets` (consumed by ECS at container launch) and `config.toml` //! (consumed by openab itself at runtime). @@ -55,48 +56,106 @@ pub async fn resolve_value_from( Ok(format!("{arn}:{json_key}::")) } +/// Split an ECS-native `valueFrom` value into its base secret ARN/name and +/// an optional JSON key, if it carries the `:::` suffix ECS uses to +/// extract one field of a JSON secret. Only applies to values already +/// shaped like an ARN (`arn:...:secret:-:::`) — a +/// bare secret name is never split, since ECS only recognizes the suffix on +/// a full ARN. Returns `(value, None)` unchanged if there's no such suffix. +fn split_ecs_json_key_suffix(value: &str) -> (&str, Option<&str>) { + if !value.starts_with("arn:") { + return (value, None); + } + // `:::` — strip the trailing empty segment (from the + // final `::`), then split off exactly one more `:` segment. A + // bare secret ARN has none of this, so `rest` won't match the pattern + // and falls through to the unchanged case. + let Some(without_trailing) = value.strip_suffix("::") else { + return (value, None); + }; + let Some((base, json_key)) = without_trailing.rsplit_once(':') else { + return (value, None); + }; + if json_key.is_empty() || !base.starts_with("arn:") { + return (value, None); + } + (base, Some(json_key)) +} + /// Resolve a `spec.secrets` value to its plain string content, for callers /// that need the actual secret value in-process (e.g. calling a third-party /// API on the caller's behalf) rather than an ECS `valueFrom` reference. /// Supports the same two forms as [`resolve_value_from`]: `aws-sm://...#...` /// (fetched and JSON-key-extracted here), or a plain/ECS-native Secrets -/// Manager ARN — including one already carrying a `:::` suffix, -/// which `GetSecretValue` resolves natively. +/// Manager ARN — including one already carrying a `:::` suffix. +/// That suffix is an ECS-only convention (resolved by ECS itself at +/// container launch, via `register_task_definition`'s `valueFrom` field) — +/// the Secrets Manager `GetSecretValue` API has no knowledge of it and +/// rejects it as an invalid secret ID, so it's stripped and the JSON key +/// extracted manually here, the same way the `aws-sm://` form is. pub async fn resolve_string(sm: &aws_sdk_secretsmanager::Client, value: &str) -> Result { - if let Some(parsed) = parse_aws_sm_uri(value) { - let (secret_id, json_key) = parsed?; - let secret_string = sm - .get_secret_value() - .secret_id(secret_id) - .send() - .await - .with_context(|| format!("failed to fetch secret '{secret_id}' from Secrets Manager"))? - .secret_string() - .with_context(|| format!("secret '{secret_id}' has no string value"))? - .to_string(); - let json: serde_json::Value = serde_json::from_str(&secret_string) - .with_context(|| format!("secret '{secret_id}' is not valid JSON"))?; - return json - .get(json_key) - .and_then(|v| v.as_str()) - .map(|v| v.to_string()) - .with_context(|| format!("JSON key '{json_key}' not found in secret '{secret_id}'")); - } + let (secret_id, json_key) = match parse_aws_sm_uri(value) { + Some(parsed) => { + let (id, key) = parsed?; + (id, Some(key)) + } + None => split_ecs_json_key_suffix(value), + }; - sm.get_secret_value() - .secret_id(value) + let secret_string = sm + .get_secret_value() + .secret_id(secret_id) .send() .await - .with_context(|| format!("failed to fetch secret '{value}' from Secrets Manager"))? + .with_context(|| format!("failed to fetch secret '{secret_id}' from Secrets Manager"))? .secret_string() - .with_context(|| format!("secret '{value}' has no string value")) + .with_context(|| format!("secret '{secret_id}' has no string value"))? + .to_string(); + + let Some(json_key) = json_key else { + return Ok(secret_string); + }; + let json: serde_json::Value = serde_json::from_str(&secret_string) + .with_context(|| format!("secret '{secret_id}' is not valid JSON"))?; + json.get(json_key) + .and_then(|v| v.as_str()) .map(|v| v.to_string()) + .with_context(|| format!("JSON key '{json_key}' not found in secret '{secret_id}'")) } #[cfg(test)] mod tests { use super::*; + #[test] + fn split_ecs_json_key_suffix_extracts_key_from_real_world_arn() { + // The exact shape that surfaced this bug: an ECS-native valueFrom + // ARN with a JSON-key suffix, passed to resolve_string (which used + // to hand this straight to GetSecretValue and fail, since that API + // has no knowledge of the trailing `:::` ECS convention). + let (base, key) = split_ecs_json_key_suffix( + "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP:TELEGRAM_BOT_TOKEN::", + ); + assert_eq!(base, "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP"); + assert_eq!(key, Some("TELEGRAM_BOT_TOKEN")); + } + + #[test] + fn split_ecs_json_key_suffix_unchanged_for_plain_arn() { + let (base, key) = split_ecs_json_key_suffix( + "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP", + ); + assert_eq!(base, "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP"); + assert_eq!(key, None); + } + + #[test] + fn split_ecs_json_key_suffix_unchanged_for_bare_secret_name() { + let (base, key) = split_ecs_json_key_suffix("plain-secret-name"); + assert_eq!(base, "plain-secret-name"); + assert_eq!(key, None); + } + #[test] fn parse_aws_sm_uri_extracts_id_and_key() { let (id, key) = parse_aws_sm_uri("aws-sm://oab/telegram/pahudxbot#TELEGRAM_BOT_TOKEN") From c7b2691f6602e7f7f69c41b3c0a6acbce33b32ed Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Sat, 4 Jul 2026 07:28:04 -0400 Subject: [PATCH 2/2] fix(operator): address code review findings on split_ecs_json_key_suffix Two real gaps found in code review of the initial fix: 1. arn:...:secret:mysecret:: was misparsed as json-key='mysecret' when it's actually a full-secret-value reference where 'mysecret' is part of the secret name/base ARN, with all three optional ECS suffix fields empty. Fixed by locating the unambiguous ':secret:' marker and treating everything up to the next colon as the secret-name segment (which per AWS's own docs never contains a colon), rather than blindly rsplit'ing on the last colon. 2. The ECS valueFrom suffix actually has three positional fields, not one: :: (confirmed against AWS's ECS docs' worked examples for referencing a specific key/version). The previous implementation only handled the json-key-only case and silently fell through to 'no suffix' for any value with a non-empty version-stage or version-id, which would then fail at GetSecretValue with a confusing error. Now parses all three fields and fails closed with a clear, specific error when a version is pinned - oabctl's own use case (fetching a secret's plaintext value in-process) never needs to pin a rotation version, so this is an intentional, documented limitation rather than a silent misparse. Testing: cargo build/clippy --all-targets -D warnings/test clean on macmini; 33/33 tests pass (7 new, covering both findings using AWS's own documented example ARN shapes). Re-verified live against the real manifest from the original bug report - still resolves correctly ('Webhook is already set'), confirming no regression. --- operator/src/secrets.rs | 133 +++++++++++++++++++++++++++++++--------- 1 file changed, 104 insertions(+), 29 deletions(-) diff --git a/operator/src/secrets.rs b/operator/src/secrets.rs index 815e6f38c..5c17be05a 100644 --- a/operator/src/secrets.rs +++ b/operator/src/secrets.rs @@ -56,30 +56,61 @@ pub async fn resolve_value_from( Ok(format!("{arn}:{json_key}::")) } -/// Split an ECS-native `valueFrom` value into its base secret ARN/name and -/// an optional JSON key, if it carries the `:::` suffix ECS uses to -/// extract one field of a JSON secret. Only applies to values already -/// shaped like an ARN (`arn:...:secret:-:::`) — a -/// bare secret name is never split, since ECS only recognizes the suffix on -/// a full ARN. Returns `(value, None)` unchanged if there's no such suffix. -fn split_ecs_json_key_suffix(value: &str) -> (&str, Option<&str>) { - if !value.starts_with("arn:") { - return (value, None); - } - // `:::` — strip the trailing empty segment (from the - // final `::`), then split off exactly one more `:` segment. A - // bare secret ARN has none of this, so `rest` won't match the pattern - // and falls through to the unchanged case. - let Some(without_trailing) = value.strip_suffix("::") else { - return (value, None); +/// Split an ECS-native `valueFrom` value into its base secret ARN and an +/// optional JSON key, if it carries ECS's suffix for extracting one field of +/// a JSON secret. Only applies to values already shaped like a Secrets +/// Manager ARN — a bare secret name is never split. Returns `(value, None)` +/// unchanged if there's no `secret:` component at all (not a Secrets +/// Manager ARN) or no suffix is present. +/// +/// The full ECS syntax has three optional positional fields after the +/// secret name, always present as colons even when empty: +/// `arn:...:secret:-:::` +/// (see the ECS docs' "Example referencing a specific key/version" section: +/// ). +/// Only the json-key field is supported here — `oabctl`'s own use case +/// (fetching a plaintext secret value in-process) never needs to pin a +/// specific rotation version. A value with a non-empty version-stage or +/// version-id fails closed with a clear error instead of silently +/// mis-splitting or ignoring those fields. +fn split_ecs_json_key_suffix(value: &str) -> Result<(&str, Option<&str>)> { + // The base ARN's secret-name segment is `secret:-<6-char-suffix>` + // and never contains a colon itself, so the first `:` after `secret:` + // unambiguously starts the optional field suffix (if any) — this is + // what let a value like `arn:...:secret:mysecret::` be misparsed before + // (treating `mysecret` as a json-key, when it's actually the secret + // name with all three optional fields empty). + let Some(secret_marker) = value.find(":secret:") else { + return Ok((value, None)); + }; + let after_name_start = secret_marker + ":secret:".len(); + let Some(name_end) = value[after_name_start..].find(':') else { + // No suffix at all — a bare secret ARN. + return Ok((value, None)); }; - let Some((base, json_key)) = without_trailing.rsplit_once(':') else { - return (value, None); + let base = &value[..after_name_start + name_end]; + let fields: Vec<&str> = value[after_name_start + name_end + 1..].split(':').collect(); + let (json_key, version_stage, version_id) = match fields.as_slice() { + [k] => (*k, "", ""), + [k, s] => (*k, *s, ""), + [k, s, i] => (*k, *s, *i), + _ => anyhow::bail!( + "unrecognized Secrets Manager ARN suffix in '{value}' — expected at most \ + ::" + ), }; - if json_key.is_empty() || !base.starts_with("arn:") { - return (value, None); + if !version_stage.is_empty() || !version_id.is_empty() { + anyhow::bail!( + "'{value}' pins a specific secret version (version-stage/version-id), which \ + oabctl does not support when resolving a secret's plaintext value in-process \ + (only when passing it through as an ECS task-definition valueFrom, where ECS \ + itself resolves the version) — use the secret's AWSCURRENT version instead" + ); } - (base, Some(json_key)) + if json_key.is_empty() { + return Ok((base, None)); + } + Ok((base, Some(json_key))) } /// Resolve a `spec.secrets` value to its plain string content, for callers @@ -99,7 +130,7 @@ pub async fn resolve_string(sm: &aws_sdk_secretsmanager::Client, value: &str) -> let (id, key) = parsed?; (id, Some(key)) } - None => split_ecs_json_key_suffix(value), + None => split_ecs_json_key_suffix(value)?, }; let secret_string = sm @@ -129,13 +160,14 @@ mod tests { #[test] fn split_ecs_json_key_suffix_extracts_key_from_real_world_arn() { - // The exact shape that surfaced this bug: an ECS-native valueFrom - // ARN with a JSON-key suffix, passed to resolve_string (which used - // to hand this straight to GetSecretValue and fail, since that API - // has no knowledge of the trailing `:::` ECS convention). + // The exact shape that surfaced the original bug: an ECS-native + // valueFrom ARN with a JSON-key suffix, passed to resolve_string + // (which used to hand this straight to GetSecretValue and fail, + // since that API has no knowledge of the trailing ECS suffix). let (base, key) = split_ecs_json_key_suffix( "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP:TELEGRAM_BOT_TOKEN::", - ); + ) + .unwrap(); assert_eq!(base, "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP"); assert_eq!(key, Some("TELEGRAM_BOT_TOKEN")); } @@ -144,18 +176,61 @@ mod tests { fn split_ecs_json_key_suffix_unchanged_for_plain_arn() { let (base, key) = split_ecs_json_key_suffix( "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP", - ); + ) + .unwrap(); assert_eq!(base, "arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP"); assert_eq!(key, None); } #[test] fn split_ecs_json_key_suffix_unchanged_for_bare_secret_name() { - let (base, key) = split_ecs_json_key_suffix("plain-secret-name"); + let (base, key) = split_ecs_json_key_suffix("plain-secret-name").unwrap(); assert_eq!(base, "plain-secret-name"); assert_eq!(key, None); } + #[test] + fn split_ecs_json_key_suffix_does_not_mistake_secret_name_for_json_key() { + // Review finding #1: a full-secret-value reference with all three + // optional fields empty (`::`) must not be misparsed as + // json-key="mysecret" — "mysecret" here is part of the secret + // name/base ARN, not a suffix field. + let (base, key) = + split_ecs_json_key_suffix("arn:aws:secretsmanager:us-east-1:903779448426:secret:mysecret::").unwrap(); + assert_eq!(base, "arn:aws:secretsmanager:us-east-1:903779448426:secret:mysecret"); + assert_eq!(key, None); + } + + #[test] + fn split_ecs_json_key_suffix_rejects_version_stage() { + // Review finding #2: version-stage/version-id pinning is out of + // scope for in-process resolution — fail closed with a clear error + // instead of silently mishandling it. + let err = split_ecs_json_key_suffix( + "arn:aws:secretsmanager:us-east-1:903779448426:secret:appauthexample-AbCdEf::AWSPREVIOUS:", + ) + .unwrap_err(); + assert!(err.to_string().contains("version")); + } + + #[test] + fn split_ecs_json_key_suffix_rejects_version_id() { + let err = split_ecs_json_key_suffix( + "arn:aws:secretsmanager:us-east-1:903779448426:secret:appauthexample-AbCdEf:::9d4cb84b-ad69-40c0-a0ab-cead3EXAMPLE", + ) + .unwrap_err(); + assert!(err.to_string().contains("version")); + } + + #[test] + fn split_ecs_json_key_suffix_rejects_key_and_version_stage_together() { + let err = split_ecs_json_key_suffix( + "arn:aws:secretsmanager:us-east-1:903779448426:secret:appauthexample-AbCdEf:username1:AWSPREVIOUS:", + ) + .unwrap_err(); + assert!(err.to_string().contains("version")); + } + #[test] fn parse_aws_sm_uri_extracts_id_and_key() { let (id, key) = parse_aws_sm_uri("aws-sm://oab/telegram/pahudxbot#TELEGRAM_BOT_TOKEN")