diff --git a/bin/core/src/api/execute/stack.rs b/bin/core/src/api/execute/stack.rs index ec710f6a0d..64d08ddc9e 100644 --- a/bin/core/src/api/execute/stack.rs +++ b/bin/core/src/api/execute/stack.rs @@ -5,7 +5,7 @@ use database::mungos::mongodb::bson::{ doc, oid::ObjectId, to_bson, to_document, }; use formatting::format_serror; -use interpolate::Interpolator; +use interpolate::{Interpolator, stack_environment_secret_replacers}; use komodo_client::{ api::{execute::*, write::RefreshStackCache}, entities::{ @@ -182,7 +182,7 @@ impl Resolve for DeployStack { interpolator.secret_replacers } else { - Default::default() + stack_environment_secret_replacers(&stack.config.environment)? }; let DeployStackResponse { @@ -886,7 +886,7 @@ pub async fn pull_stack_inner( } interpolator.secret_replacers } else { - Default::default() + stack_environment_secret_replacers(&stack.config.environment)? }; let res = periphery_client(server) @@ -1376,7 +1376,7 @@ impl Resolve for RunStackService { interpolator.secret_replacers } else { - Default::default() + stack_environment_secret_replacers(&stack.config.environment)? }; let log = periphery_client(&server) diff --git a/bin/core/src/api/write/stack.rs b/bin/core/src/api/write/stack.rs index 275efc72ce..9be2f77b7e 100644 --- a/bin/core/src/api/write/stack.rs +++ b/bin/core/src/api/write/stack.rs @@ -65,7 +65,6 @@ impl Resolve for CreateStack { fields( operator = user.id, stack = self.name, - config = serde_json::to_string(&self.config).unwrap(), ) )] async fn resolve( @@ -127,7 +126,6 @@ impl Resolve for UpdateStack { fields( operator = user.id, stack = self.id, - update = serde_json::to_string(&self.config).unwrap(), ) )] async fn resolve( @@ -1139,3 +1137,34 @@ impl Resolve for BatchCheckStackForUpdate { Ok(res) } } + +#[cfg(test)] +mod tests { + fn resolve_attribute<'a>( + source: &'a str, + implementation: &str, + ) -> &'a str { + let implementation = source + .split_once(implementation) + .expect("missing Stack write implementation") + .1; + implementation + .split_once("async fn resolve") + .expect("missing Stack write resolver") + .0 + } + + #[test] + fn stack_write_spans_do_not_serialize_config() { + let source = include_str!("stack.rs"); + for implementation in [ + "impl Resolve for CreateStack", + "impl Resolve for UpdateStack", + ] { + assert!( + !resolve_attribute(source, implementation) + .contains("self.config") + ); + } + } +} diff --git a/bin/core/src/api/write/variable.rs b/bin/core/src/api/write/variable.rs index 339464413c..5873abfd9e 100644 --- a/bin/core/src/api/write/variable.rs +++ b/bin/core/src/api/write/variable.rs @@ -1,5 +1,6 @@ use anyhow::{Context, anyhow}; use database::mungos::mongodb::bson::doc; +use interpolate::REDACTED; use komodo_client::{ api::write::*, entities::{Operation, ResourceTarget, variable::Variable}, @@ -19,6 +20,17 @@ use crate::{ use super::WriteArgs; +fn variable_record(variable: &Variable) -> String { + if variable.is_secret { + format!( + "Variable {{\n name: {:?},\n value: {REDACTED:?},\n description: {:?},\n is_secret: true,\n}}", + variable.name, variable.description + ) + } else { + format!("{variable:#?}") + } +} + impl Resolve for CreateVariable { #[instrument( "CreateVariable", @@ -73,7 +85,7 @@ impl Resolve for CreateVariable { ); update - .push_simple_log("Create Variable", format!("{variable:#?}")); + .push_simple_log("Create Variable", variable_record(&variable)); update.finalize(); @@ -133,8 +145,7 @@ impl Resolve for UpdateVariableValue { let log = if variable.is_secret { format!( - "variable: '{name}'\nfrom: {}\nto: {value}", - variable.value.replace(|_| true, "#") + "variable: '{name}'\nfrom: {REDACTED}\nto: {REDACTED}" ) } else { format!( @@ -255,7 +266,7 @@ impl Resolve for DeleteVariable { ); update - .push_simple_log("Delete Variable", format!("{variable:#?}")); + .push_simple_log("Delete Variable", variable_record(&variable)); update.finalize(); add_update(update).await?; @@ -263,3 +274,25 @@ impl Resolve for DeleteVariable { Ok(variable) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn secret_variable_record_omits_synthetic_marker() { + const MARKER: &str = "komodo-redaction-marker-4d9c1d48f7b84a9e"; + let variable = Variable { + name: "SYNTHETIC_SECRET".to_string(), + value: MARKER.to_string(), + description: "redaction acceptance".to_string(), + is_secret: true, + }; + + let record = variable_record(&variable); + + assert!(!record.contains(MARKER)); + assert!(record.contains(REDACTED)); + assert!(record.contains("SYNTHETIC_SECRET")); + } +} diff --git a/bin/core/src/resource/mod.rs b/bin/core/src/resource/mod.rs index b17bcecc9c..7758ceada7 100644 --- a/bin/core/src/resource/mod.rs +++ b/bin/core/src/resource/mod.rs @@ -205,6 +205,15 @@ pub trait KomodoResource { update: &mut Update, ) -> anyhow::Result<()>; + /// Values which must be removed from persisted create/update records for + /// this resource. Most resources rely only on Komodo Secret interpolation; + /// resource types with direct secret-bearing config can override this. + fn update_secret_replacers( + _resource: &Resource, + ) -> anyhow::Result> { + Ok(Vec::new()) + } + // ======= // RENAME // ======= @@ -717,6 +726,9 @@ pub async fn create( T::post_create(&resource, &mut update).await?; + let replacers = T::update_secret_replacers(&resource)?; + update.sanitize(&replacers); + refresh_all_resources_cache().await; update.finalize(); @@ -740,6 +752,7 @@ pub async fn update( PermissionLevel::Write.into(), ) .await?; + let mut replacers = T::update_secret_replacers(&resource)?; if T::busy(&resource.id).await? { return Err(anyhow!("{} busy", T::resource_type())); @@ -812,7 +825,9 @@ pub async fn update( let updated = get::(id_or_name).await?; + replacers.extend(T::update_secret_replacers(&updated)?); T::post_update(&updated, &mut update).await?; + update.sanitize(&replacers); refresh_all_resources_cache().await; diff --git a/bin/core/src/resource/stack.rs b/bin/core/src/resource/stack.rs index 6fe8f4bde7..165894450f 100644 --- a/bin/core/src/resource/stack.rs +++ b/bin/core/src/resource/stack.rs @@ -2,6 +2,7 @@ use anyhow::Context; use database::mungos::mongodb::Collection; use formatting::format_serror; use indexmap::IndexSet; +use interpolate::stack_environment_secret_replacers; use komodo_client::{ api::write::RefreshStackCache, entities::{ @@ -345,6 +346,16 @@ impl super::KomodoResource for Stack { Self::post_create(updated, update).await } + fn update_secret_replacers( + stack: &Resource, + ) -> anyhow::Result> { + Ok( + stack_environment_secret_replacers(&stack.config.environment)? + .into_iter() + .collect(), + ) + } + // RENAME fn rename_operation() -> Operation { diff --git a/client/core/rs/src/entities/update.rs b/client/core/rs/src/entities/update.rs index 071f05fdf3..c0003712a8 100644 --- a/client/core/rs/src/entities/update.rs +++ b/client/core/rs/src/entities/update.rs @@ -107,6 +107,22 @@ impl Update { self.end_ts = Some(komodo_timestamp()); self.status = UpdateStatus::Complete; } + + /// Removes secret values from every persisted free-form field on an + /// update. This is intentionally broader than [Log::sanitize] so resource + /// audit snapshots cannot bypass the same redaction seam. + #[cfg(feature = "svi")] + pub fn sanitize(&mut self, replacers: &Vec<(String, String)>) { + for log in &mut self.logs { + log.sanitize(replacers); + } + self.other_data = + svi::replace_in_string(&self.other_data, replacers); + self.prev_toml = + svi::replace_in_string(&self.prev_toml, replacers); + self.current_toml = + svi::replace_in_string(&self.current_toml, replacers); + } } /// Minimal representation of an action performed by Komodo. @@ -212,6 +228,30 @@ impl Log { } } +#[cfg(all(test, feature = "svi"))] +mod tests { + use super::*; + + #[test] + fn update_sanitizes_logs_and_audit_snapshots() { + const MARKER: &str = "komodo-redaction-marker-4d9c1d48f7b84a9e"; + let mut update = Update { + logs: vec![Log::simple("Synthetic", MARKER.to_string())], + other_data: MARKER.to_string(), + prev_toml: format!("environment = 'SECRET={MARKER}'"), + current_toml: format!("environment = 'SECRET={MARKER}'"), + ..Default::default() + }; + + update + .sanitize(&vec![(MARKER.to_string(), "SECRET".to_string())]); + let serialized = serde_json::to_string(&update).unwrap(); + + assert!(!serialized.contains(MARKER)); + assert!(serialized.contains("")); + } +} + /// An update's status #[typeshare] #[derive( diff --git a/lib/interpolate/src/lib.rs b/lib/interpolate/src/lib.rs index 1842cb0a22..c734be3628 100644 --- a/lib/interpolate/src/lib.rs +++ b/lib/interpolate/src/lib.rs @@ -2,10 +2,63 @@ use std::collections::{HashMap, HashSet}; use anyhow::Context; use komodo_client::entities::{ - EnvironmentVar, build::Build, deployment::Deployment, repo::Repo, - stack::Stack, update::Log, + EnvironmentVar, build::Build, deployment::Deployment, + environment_vars_from_str, repo::Repo, stack::Stack, update::Log, }; +/// The marker used when a secret value is removed from persisted output. +pub const REDACTED: &str = ""; + +/// Returns replacement pairs for values in a Stack environment whose key is +/// conventionally sensitive. Stack environment entries do not carry the +/// `is_secret` metadata available to global Komodo Variables, so this is the +/// fail-safe seam for direct environment values. +pub fn stack_environment_secret_replacers( + environment: &str, +) -> anyhow::Result> { + let variables = environment_vars_from_str(environment) + .context("failed to parse Stack environment for redaction")?; + Ok( + variables + .into_iter() + .filter(|variable| { + !variable.value.is_empty() + && sensitive_environment_key(&variable.variable) + }) + .map(|variable| (variable.value, variable.variable)) + .collect(), + ) +} + +fn sensitive_environment_key(key: &str) -> bool { + let words = key + .split(|character: char| !character.is_ascii_alphanumeric()) + .filter(|word| !word.is_empty()) + .map(str::to_ascii_uppercase) + .collect::>(); + + words.iter().any(|word| { + matches!( + word.as_str(), + "PASSWORD" + | "PASSWD" + | "SECRET" + | "TOKEN" + | "CREDENTIAL" + | "CREDENTIALS" + | "DSN" + ) + }) || words.windows(2).any(|pair| { + matches!( + (pair[0].as_str(), pair[1].as_str()), + ("API", "KEY") + | ("PRIVATE", "KEY") + | ("ACCESS", "KEY") + | ("CONNECTION", "STRING") + ) + }) +} + pub struct Interpolator<'a> { variables: Option<&'a HashMap>, secrets: &'a HashMap, @@ -31,6 +84,11 @@ impl<'a> Interpolator<'a> { stack: &mut Stack, ) -> anyhow::Result<&mut Self> { if stack.config.skip_secret_interp { + self.secret_replacers.extend( + stack_environment_secret_replacers( + &stack.config.environment, + )?, + ); return Ok(self); } self @@ -40,7 +98,13 @@ impl<'a> Interpolator<'a> { .interpolate_string(&mut stack.config.post_deploy.command)? .interpolate_string(&mut stack.config.compose_cmd_wrapper)? .interpolate_extra_args(&mut stack.config.extra_args)? - .interpolate_extra_args(&mut stack.config.build_extra_args) + .interpolate_extra_args(&mut stack.config.build_extra_args)?; + self + .secret_replacers + .extend(stack_environment_secret_replacers( + &stack.config.environment, + )?); + Ok(self) } pub fn interpolate_repo( @@ -180,3 +244,88 @@ impl<'a> Interpolator<'a> { } } } + +#[cfg(test)] +mod tests { + use super::*; + + const SYNTHETIC_SECRET: &str = + "komodo-redaction-marker-4d9c1d48f7b84a9e"; + + #[test] + fn stack_environment_collects_only_sensitive_values() { + let replacers = stack_environment_secret_replacers(&format!( + "PUBLIC_URL=https://example.com\nAPI_TOKEN={SYNTHETIC_SECRET}\nDATABASE_CONNECTION_STRING=postgres://user:{SYNTHETIC_SECRET}@db/app\nEMPTY_SECRET=\n" + )) + .unwrap(); + + assert!(replacers.contains(&( + SYNTHETIC_SECRET.to_string(), + "API_TOKEN".to_string() + ))); + assert!( + replacers + .iter() + .any(|(_, key)| { key == "DATABASE_CONNECTION_STRING" }) + ); + assert!(!replacers.iter().any(|(_, key)| key == "PUBLIC_URL")); + assert!(!replacers.iter().any(|(value, _)| value.is_empty())); + assert!(!sensitive_environment_key("WIKI_ACCESS_AUDIENCE")); + assert!(sensitive_environment_key("SQL_DSN")); + for key in [ + "STRIPE_API_SECRET", + "STRIPE_WEBHOOK_SECRET", + "SESSION_SECRET", + "CRYPTO_SECRET", + "POSTGRES_PASSWORD", + "REDIS_PASSWORD", + "LIVE_BRIDGE_INTERNAL_SECRET", + "ROUTER_LIVE_SHARED_SECRET", + "DARIO_API_KEY", + ] { + assert!(sensitive_environment_key(key), "{key}"); + } + } + + #[test] + fn stack_interpolation_redacts_synthetic_marker() { + let mut stack = Stack::default(); + stack.config.environment = + format!("ROUTER_SHARED_SECRET={SYNTHETIC_SECRET}"); + let variables = HashMap::new(); + let secrets = HashMap::new(); + let mut interpolator = + Interpolator::new(Some(&variables), &secrets); + + interpolator.interpolate_stack(&mut stack).unwrap(); + let rendered = format!( + "environment:\n ROUTER_SHARED_SECRET: {SYNTHETIC_SECRET}" + ); + let sanitized = svi::replace_in_string( + &rendered, + &interpolator.secret_replacers, + ); + + assert!(!sanitized.contains(SYNTHETIC_SECRET)); + assert!(sanitized.contains("")); + } + + #[test] + fn stack_redaction_does_not_depend_on_interpolation() { + let mut stack = Stack::default(); + stack.config.skip_secret_interp = true; + stack.config.environment = + format!("ROUTER_SHARED_SECRET={SYNTHETIC_SECRET}"); + let variables = HashMap::new(); + let secrets = HashMap::new(); + let mut interpolator = + Interpolator::new(Some(&variables), &secrets); + + interpolator.interpolate_stack(&mut stack).unwrap(); + + assert!(interpolator.secret_replacers.contains(&( + SYNTHETIC_SECRET.to_string(), + "ROUTER_SHARED_SECRET".to_string() + ))); + } +}