diff --git a/CHANGELOG.md b/CHANGELOG.md index e4f176f2..915a1fd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The `awssm` and `scaleway` providers now treat a JSON `null` in a `ref` field + as no value, the same as an absent key, so the provider chain continues. + Previously it was rendered as the four-character string `null`, which + satisfied a required secret and reached the program as a password or token + spelled `n-u-l-l`. The `bw` and `dashlane` providers already behaved this way. + An `extract` pointer is unchanged: it names one location and still reports a + `null` there, and the two policies now sit next to each other in one place. - The `awssm` provider now accepts a trailing slash in `?prefix=` without inserting a second slash into the AWS secret name. For example, `?prefix=myteam/` resolves to `myteam/secretspec/...`, matching diff --git a/secretspec/src/json_field.rs b/secretspec/src/json_field.rs new file mode 100644 index 00000000..829d430f --- /dev/null +++ b/secretspec/src/json_field.rs @@ -0,0 +1,80 @@ +//! Rendering one selected JSON value as a secret. +//! +//! Three call sites select a single value out of a JSON document: the `awssm` +//! and `scaleway` providers, which take a flat `field` key, and +//! `Secrets::extract_stored_value`, which takes a JSON Pointer. Selection +//! differs on purpose and stays with each caller. Rendering the selected value +//! is shared, and each caller states its own policy for a JSON null. + +use secrecy::SecretString; + +/// Renders a selected JSON value as a secret. +/// +/// A string is taken as-is; anything else is serialized, so a port stays `5432` +/// and a flag stays `true`. A null renders as `"null"`, which is what an +/// `extract` pointer wants: it was asked for one specific location, and the +/// document genuinely holds a JSON null there. +pub(crate) fn render(value: &serde_json::Value) -> SecretString { + match value { + serde_json::Value::String(text) => SecretString::new(text.clone().into()), + other => SecretString::new(other.to_string().into()), + } +} + +/// Renders a value selected by a provider's `field`, treating a null as absent. +/// +/// A provider answers "is this secret set here?", so a null is no value and the +/// resolver moves on to the next provider in the chain. Rendering it would +/// produce the four-character secret `null`, which satisfies a required secret +/// and reaches the program as a password or token spelled n-u-l-l. The `bw` and +/// `dashlane` providers already treat a null this way. +/// +/// This is deliberately not the same policy as [`render`]: an `extract` pointer +/// names one location and reports what is there, while a provider `field` is a +/// lookup that can come up empty. +pub(crate) fn render_field(value: &serde_json::Value) -> Option { + match value { + serde_json::Value::Null => None, + other => Some(render(other)), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use secrecy::ExposeSecret; + + fn parse(raw: &str) -> serde_json::Value { + serde_json::from_str(raw).unwrap() + } + + #[test] + fn render_takes_a_string_as_is_and_serializes_the_rest() { + assert_eq!(render(&parse(r#""s3cret""#)).expose_secret(), "s3cret"); + assert_eq!(render(&parse("5432")).expose_secret(), "5432"); + assert_eq!(render(&parse("true")).expose_secret(), "true"); + assert_eq!(render(&parse(r#"{"a":1}"#)).expose_secret(), r#"{"a":1}"#); + } + + #[test] + fn render_keeps_a_null_because_an_extract_pointer_reports_what_is_there() { + assert_eq!(render(&parse("null")).expose_secret(), "null"); + } + + #[test] + fn render_field_treats_a_null_as_absent() { + assert!(render_field(&parse("null")).is_none()); + } + + #[test] + fn render_field_agrees_with_render_on_everything_else() { + for raw in [r#""s3cret""#, "5432", "true", r#"{"a":1}"#, r#"["x"]"#] { + let value = parse(raw); + assert_eq!( + render_field(&value).unwrap().expose_secret(), + render(&value).expose_secret(), + "{raw}" + ); + } + } +} diff --git a/secretspec/src/lib.rs b/secretspec/src/lib.rs index 5648048a..59ad7662 100644 --- a/secretspec/src/lib.rs +++ b/secretspec/src/lib.rs @@ -49,6 +49,7 @@ mod composition; mod config; mod error; pub(crate) mod generator; +pub(crate) mod json_field; mod manifest; mod plan; mod report; diff --git a/secretspec/src/provider/awssm.rs b/secretspec/src/provider/awssm.rs index 60ced960..ecddfd83 100644 --- a/secretspec/src/provider/awssm.rs +++ b/secretspec/src/provider/awssm.rs @@ -262,12 +262,9 @@ impl AwssmProvider { name, json_key, e )) })?; - match json.get(json_key) { - Some(serde_json::Value::String(s)) => Ok(Some(SecretString::new(s.clone().into()))), - // Non-string JSON values (numbers, bools) are rendered as-is. - Some(other) => Ok(Some(SecretString::new(other.to_string().into()))), - None => Ok(None), - } + // Selection is a flat key here; rendering the selected value is shared + // with the scaleway provider and Secrets::extract_stored_value. + Ok(json.get(json_key).and_then(crate::json_field::render_field)) } /// Retrieves a secret by its full name/ARN, optionally extracting one key @@ -797,6 +794,32 @@ mod tests { ); } + #[test] + fn extract_json_key_null_is_none_not_the_string_null() { + // A null value is no value. Rendering it as "null" would satisfy a + // required secret and hand the program a password spelled n-u-l-l. + let value = r#"{"password": null}"#; + assert!( + AwssmProvider::extract_json_key("db", value, "password") + .unwrap() + .is_none() + ); + } + + #[test] + fn extract_json_key_null_matches_a_missing_key() { + let with_null = r#"{"password": null}"#; + let without = r#"{"username": "admin"}"#; + assert_eq!( + AwssmProvider::extract_json_key("db", with_null, "password") + .unwrap() + .is_none(), + AwssmProvider::extract_json_key("db", without, "password") + .unwrap() + .is_none() + ); + } + #[test] fn extract_json_key_missing_key_is_none() { let value = r#"{"username": "admin"}"#; diff --git a/secretspec/src/provider/scaleway.rs b/secretspec/src/provider/scaleway.rs index 6d372ed6..70a85e8c 100644 --- a/secretspec/src/provider/scaleway.rs +++ b/secretspec/src/provider/scaleway.rs @@ -265,11 +265,8 @@ impl ScalewayProvider { "secret '{name}' is not JSON, cannot extract key '{json_key}': {e}" )) })?; - match json.get(json_key) { - Some(serde_json::Value::String(s)) => Ok(Some(SecretString::new(s.clone().into()))), - Some(other) => Ok(Some(SecretString::new(other.to_string().into()))), - None => Ok(None), - } + // See the AWS provider: flat-key selection, shared rendering. + Ok(json.get(json_key).and_then(crate::json_field::render_field)) } async fn get_async( @@ -672,6 +669,17 @@ mod tests { assert!(!p.uri().contains("SCW_SECRET_KEY")); } + #[test] + fn extract_json_key_null_is_none_not_the_string_null() { + // Mirrors the AWS provider: a null value is no value. + let v = r#"{"user": "admin", "password": null}"#; + assert!( + ScalewayProvider::extract_json_key("s", v, "password") + .unwrap() + .is_none() + ); + } + #[test] fn extract_json_key_reads_string_and_renders_scalars() { let v = r#"{"user":"admin","port":5432,"tls":true}"#; diff --git a/secretspec/src/secrets.rs b/secretspec/src/secrets.rs index d7de1116..95390caa 100644 --- a/secretspec/src/secrets.rs +++ b/secretspec/src/secrets.rs @@ -1777,13 +1777,12 @@ impl Secrets { ), } })?; - let selected = match selected { - serde_json::Value::String(value) => value.clone(), - value => { - serde_json::to_string(value).expect("serializing JSON value cannot fail") - } - }; - Ok(SecretString::new(selected.into())) + // Rendering is shared with the awssm and scaleway providers. + // A null renders as "null" here: this caller was asked for one + // pointer and reports what the document holds, unlike a + // provider `field`, where a null means "not set" and the chain + // continues. See crate::json_field. + Ok(crate::json_field::render(selected)) } } } diff --git a/secretspec/src/tests.rs b/secretspec/src/tests.rs index d00f2e4c..39ec6f98 100644 --- a/secretspec/src/tests.rs +++ b/secretspec/src/tests.rs @@ -5338,6 +5338,34 @@ FALLBACK = {{ description = "logical default", providers = ["documents"], ref = assert_eq!(fs::read_to_string(document_path).unwrap(), original); } +#[test] +fn test_json_extract_renders_a_null_while_a_provider_field_treats_it_as_absent() { + use crate::config::{ExtractFormat, SecretExtract}; + + // An extract pointer names one location and reports what the document + // holds there, so a null renders. test_json_extract_resolves_structured_ + // values_after_decoding pins this end to end. + let extract = SecretExtract { + format: ExtractFormat::Json, + pointer: "/database/password".to_string(), + }; + let rendered = + Secrets::extract_stored_value(&extract, "PASSWORD", r#"{"database":{"password":null}}"#) + .unwrap(); + assert_eq!(rendered.expose_secret(), "null"); + + // A provider `field` is a lookup that can come up empty, so the same null + // is absent and the provider chain continues. + let value: serde_json::Value = serde_json::from_str(r#"{"password":null}"#).unwrap(); + assert!(crate::json_field::render_field(&value["password"]).is_none()); + + // Everything that is not null renders identically on both paths. + let port = + Secrets::extract_stored_value(&extract, "PASSWORD", r#"{"database":{"password":5432}}"#) + .unwrap(); + assert_eq!(port.expose_secret(), "5432"); +} + #[test] fn test_json_extract_errors_do_not_expose_stored_documents() { use crate::config::{ExtractFormat, SecretExtract};