fix(operator): resolve_string mishandled ECS-native :jsonKey:: suffix

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
-//! `:<jsonKey>::` to extract one field of a JSON secret), or the same
-//! `aws-sm://<secret-id>#<json-key>` shorthand openab itself uses for
-//! in-app secret refs (see `crates/openab-core/src/secrets.rs`) — kept
+//! `:<jsonKey>::` 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://<secret-id>#<json-key>` 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,181 @@ pub async fn resolve_value_from(
     Ok(format!("{arn}:{json_key}::"))
 }
 
+/// 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:<name>-<suffix>:<json-key>:<version-stage>:<version-id>`
+/// (see the ECS docs' "Example referencing a specific key/version" section:
+/// <https://docs.aws.amazon.com/AmazonECS/latest/developerguide/secrets-envvar-secrets-manager.html>).
+/// 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:<name>-<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 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 \
+             <json-key>:<version-stage>:<version-id>"
+        ),
+    };
+    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"
+        );
+    }
+    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
 /// 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 `:<jsonKey>::` suffix,
-/// which `GetSecretValue` resolves natively.
+/// Manager ARN — including one already carrying a `:<jsonKey>::` 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<String> {
-    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 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"));
+    }
+
+    #[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",
+        )
+        .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").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")