From edfcda1ea52b98eaaa6393db020a4f7f810df575 Mon Sep 17 00:00:00 2001 From: Sebastian Jeltsch Date: Wed, 5 Mar 2025 11:31:06 +0100 Subject: [PATCH] Restore secret redaction and config merging. --- client/testfixture/config.textproto | 1 + trailbase-core/src/admin/config/get_config.rs | 28 +- .../src/admin/config/update_config.rs | 21 +- trailbase-core/src/admin/table/drop_table.rs | 3 +- trailbase-core/src/app_state.rs | 30 +- trailbase-core/src/config.rs | 366 +++++++++++------- 6 files changed, 258 insertions(+), 191 deletions(-) diff --git a/client/testfixture/config.textproto b/client/testfixture/config.textproto index c9d4a656..16bd31ab 100644 --- a/client/testfixture/config.textproto +++ b/client/testfixture/config.textproto @@ -10,6 +10,7 @@ auth { key: "discord" value { client_id: "invalid_discord_id" + client_secret: "" provider_id: DISCORD display_name: "Discord" } diff --git a/trailbase-core/src/admin/config/get_config.rs b/trailbase-core/src/admin/config/get_config.rs index fd95bfa8..3fae77e4 100644 --- a/trailbase-core/src/admin/config/get_config.rs +++ b/trailbase-core/src/admin/config/get_config.rs @@ -1,35 +1,23 @@ use axum::extract::State; use axum_extra::protobuf::Protobuf; -use base64::prelude::*; use crate::admin::AdminError as Error; use crate::app_state::AppState; -use crate::config::proto::GetConfigResponse; +use crate::config::{ + proto::{hash_config, GetConfigResponse}, + redact_secrets, +}; pub async fn get_config_handler( State(state): State, ) -> Result, Error> { let config = state.get_config(); - let hash = config.hash(); + let hash = hash_config(&config); - // NOTE: We used to strip the secrets to avoid exposing them in the admin dashboard. This would - // be mostly relevant if no TLS (plain text transmission) or if we wanted to have less privileged - // dashboard users than admins. - // We went back on this for now, since this requires very complicated merging. For example, an - // oauth provider is already configured and an admin adds another one. You get back: - // - // [ - // { provider_id: X, client_id: "old" }, - // { provider_id: Y, client_id: "new", client_secret: "new_secret" }, - // ] - // - // which fails validation because "old" is missing the "secret". We'd have to merge secrets back - // before validation on entries, which haven't been removed, ... and this true for all secrets. - // - // let (stripped, _secrets) = strip_secrets(&config)?; + let (stripped, _secrets) = redact_secrets(&config)?; return Ok(Protobuf(GetConfigResponse { - config: Some(config), - hash: Some(BASE64_URL_SAFE.encode(hash.to_le_bytes())), + config: Some(stripped), + hash: Some(hash), })); } diff --git a/trailbase-core/src/admin/config/update_config.rs b/trailbase-core/src/admin/config/update_config.rs index 7d3cced9..42555a0d 100644 --- a/trailbase-core/src/admin/config/update_config.rs +++ b/trailbase-core/src/admin/config/update_config.rs @@ -2,32 +2,29 @@ use axum::extract::State; use axum::http::StatusCode; use axum::response::IntoResponse; use axum_extra::protobuf::Protobuf; -use base64::prelude::*; use crate::admin::AdminError as Error; use crate::app_state::AppState; -use crate::config::proto::UpdateConfigRequest; -use crate::config::ConfigError; +use crate::config::proto::{UpdateConfigRequest, Vault}; +use crate::config::{merge_vault_and_env, redact_secrets}; pub async fn update_config_handler( State(state): State, Protobuf(request): Protobuf, ) -> Result { - let Some(Ok(hash)) = request.hash.map(|h| BASE64_URL_SAFE.decode(h)) else { + let Some(hash) = request.hash else { return Err(Error::Precondition("Missing hash".to_string())); }; let Some(config) = request.config else { return Err(Error::Precondition("Missing config".to_string())); }; - let current_hash = state.get_config().hash(); - if current_hash.to_le_bytes() == *hash { - state - .validate_and_update_config(config, Some(current_hash)) - .await?; + let current = state.get_config(); + let (_, secrets) = redact_secrets(¤t)?; - return Ok((StatusCode::OK, "Config updated")); - } + let merged = merge_vault_and_env(config, Vault { secrets })?; - return Err(ConfigError::Update("Concurrent edit. Stale admin-UI cache?".to_string()).into()); + state.validate_and_update_config(merged, Some(hash)).await?; + + return Ok((StatusCode::OK, "Config updated")); } diff --git a/trailbase-core/src/admin/table/drop_table.rs b/trailbase-core/src/admin/table/drop_table.rs index bb055fac..abf34142 100644 --- a/trailbase-core/src/admin/table/drop_table.rs +++ b/trailbase-core/src/admin/table/drop_table.rs @@ -10,6 +10,7 @@ use ts_rs::TS; use crate::admin::AdminError as Error; use crate::app_state::AppState; +use crate::config::proto::hash_config; use crate::transaction::TransactionRecorder; #[derive(Clone, Debug, Deserialize, TS)] @@ -60,7 +61,7 @@ pub async fn drop_table_handler( // Update configuration: remove all APIs reference the no longer existing table. let mut config = state.get_config(); - let old_config_hash = config.hash(); + let old_config_hash = hash_config(&config); config.record_apis.retain(move |c| { if let Some(ref table) = c.table_name { diff --git a/trailbase-core/src/app_state.rs b/trailbase-core/src/app_state.rs index e4949c8f..705e8c71 100644 --- a/trailbase-core/src/app_state.rs +++ b/trailbase-core/src/app_state.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use crate::auth::jwt::JwtHelper; use crate::auth::oauth::providers::{ConfiguredOAuthProviders, OAuthProviderType}; -use crate::config::proto::{Config, RecordApiConfig, S3StorageConfig}; +use crate::config::proto::{hash_config, Config, RecordApiConfig, S3StorageConfig}; use crate::config::{validate_config, write_config_and_vault_textproto}; use crate::constants::SITE_URL_DEFAULT; use crate::data_dir::DataDir; @@ -218,27 +218,27 @@ impl AppState { pub async fn validate_and_update_config( &self, config: Config, - hash: Option, + hash: Option, ) -> Result<(), crate::config::ConfigError> { validate_config(self.table_metadata(), &config)?; match hash { Some(hash) => { let old_config = self.state.config.load(); - if old_config.hash() == hash { - let success = self - .state - .config - .compare_and_swap(old_config, Arc::new(config)); - - if !success { - return Err(crate::config::ConfigError::Update( - "Config compare-exchange failed".to_string(), - )); - } - } else { + if hash_config(&old_config) != hash { return Err(crate::config::ConfigError::Update( - "Safe config update failed: mismatching hash".to_string(), + "Config update failed: mismatching or stale hash".to_string(), + )); + } + + let success = self + .state + .config + .compare_and_swap(old_config, Arc::new(config)); + + if !success { + return Err(crate::config::ConfigError::Update( + "Config compare-exchange failed".to_string(), )); } } diff --git a/trailbase-core/src/config.rs b/trailbase-core/src/config.rs index cba73f3f..a9a35f8c 100644 --- a/trailbase-core/src/config.rs +++ b/trailbase-core/src/config.rs @@ -45,47 +45,23 @@ fn parse_env_var( Ok(None) } -#[cfg(test)] -mod test_env { - use lazy_static::lazy_static; - use parking_lot::Mutex; - use std::collections::HashMap; - - lazy_static! { - pub static ref ENV: Mutex> = Mutex::new(HashMap::new()); - } - - pub(super) fn parse_env_var( - name: &str, - ) -> Result, ::Err> { - if let Some(value) = ENV.lock().get(name) { - return Ok(Some(value.parse::()?)); - } - Ok(None) - } - - pub(super) fn set(name: &str, value: Option<&str>) { - match value { - None => ENV.lock().remove(name), - Some(v) => ENV.lock().insert(name.to_string(), v.to_string()), - }; - } -} - #[cfg(test)] use test_env::parse_env_var; pub(super) fn apply_parsed_env_var( - name: &str, - mut f: impl FnMut(T), + msg: &mut DynamicMessage, + field_desc: &FieldDescriptor, + var_name: &str, + f: impl Fn(T) -> prost_reflect::Value, ) -> Result<(), ::Err> { - if let Some(v) = parse_env_var::(name)? { - f(v); + if let Some(v) = parse_env_var::(var_name)? { + msg.set_field(field_desc, f(v)); } Ok(()) } pub mod proto { + use base64::prelude::*; use chrono::Duration; use lazy_static::lazy_static; use prost::Message; @@ -193,13 +169,6 @@ pub mod proto { return Ok(format!("{PREFACE}\n{text}")); } - - pub fn hash(&self) -> u64 { - let encoded = self.encode_to_vec(); - let mut s = DefaultHasher::new(); - encoded.hash(&mut s); - return s.finish(); - } } impl AuthConfig { @@ -214,6 +183,15 @@ pub mod proto { ); } } + + pub fn hash_config(config: &Config) -> String { + let encoded = config.encode_to_vec(); + let mut s = DefaultHasher::new(); + encoded.hash(&mut s); + let hash = s.finish(); + + return BASE64_URL_SAFE.encode(hash.to_le_bytes()); + } } fn is_secret(field_descriptor: &FieldDescriptor) -> bool { @@ -230,6 +208,15 @@ fn is_secret(field_descriptor: &FieldDescriptor) -> bool { return false; } +/// Merges settings from environment variables and secrets into the base msg/config. +/// +/// NOTE: the merging semantics are different for env variables and secrets. The former are +/// overrides and will be set unconditionally, secrets will only be inserted for string fields +/// where the value is `PLACEHOLDER`. This allows changing secret values, w/o them getting +/// overridden when merging into a new config. +/// +/// We could consider breaking the two up. We could even use serialized field descriptors as keys +/// in secrets fiel rather than env variable names. fn recursively_merge_vault_and_env( msg: &mut DynamicMessage, vault: &proto::Vault, @@ -247,8 +234,6 @@ fn recursively_merge_vault_and_env( trace!("{var_name}: {secret}"); - let mut set_field = |v: Value| msg.set_field(&field_descr, v); - match field_descr.kind() { Kind::Message(_) => { // FIXME: We're skipping missing optional message fields, which means potentially present @@ -298,20 +283,31 @@ fn recursively_merge_vault_and_env( } } Kind::String => { - if let Ok(Some(value)) = parse_env_var(&var_name) { - set_field(Value::String(value)); + // Env overrides takes priority letting user override any value whether from base config or + // secrets. + if let Some(value) = parse_env_var::(&var_name).expect("infalliable") { + msg.set_field(&field_descr, Value::String(value)); } else if secret { if let Some(stored_secret) = vault.secrets.get(&var_name) { - set_field(Value::String(stored_secret.to_string())); + match *msg.get_field(&field_descr) { + Value::String(ref x) if x == PLACEHOLDER => { + msg.set_field(&field_descr, Value::String(stored_secret.clone())); + } + _ => { + // The secret was replaced. + } + }; } } } - Kind::Int32 => apply_parsed_env_var::(&var_name, |v| set_field(Value::I32(v)))?, - Kind::Uint32 => apply_parsed_env_var::(&var_name, |v| set_field(Value::U32(v)))?, - Kind::Int64 => apply_parsed_env_var::(&var_name, |v| set_field(Value::I64(v)))?, - Kind::Uint64 => apply_parsed_env_var::(&var_name, |v| set_field(Value::U64(v)))?, - Kind::Bool => apply_parsed_env_var::(&var_name, |v| set_field(Value::Bool(v)))?, - Kind::Enum(_) => apply_parsed_env_var::(&var_name, |v| set_field(Value::EnumNumber(v)))?, + Kind::Int32 => apply_parsed_env_var::(msg, &field_descr, &var_name, Value::I32)?, + Kind::Uint32 => apply_parsed_env_var::(msg, &field_descr, &var_name, Value::U32)?, + Kind::Int64 => apply_parsed_env_var::(msg, &field_descr, &var_name, Value::I64)?, + Kind::Uint64 => apply_parsed_env_var::(msg, &field_descr, &var_name, Value::U64)?, + Kind::Bool => apply_parsed_env_var::(msg, &field_descr, &var_name, Value::Bool)?, + Kind::Enum(_) => { + apply_parsed_env_var::(msg, &field_descr, &var_name, Value::EnumNumber)? + } _ => { error!("Config merging not implemented for: {field_descr:?}"); } @@ -321,7 +317,7 @@ fn recursively_merge_vault_and_env( return Ok(()); } -fn merge_vault_and_env( +pub(crate) fn merge_vault_and_env( config: proto::Config, vault: proto::Vault, ) -> Result { @@ -330,34 +326,39 @@ fn merge_vault_and_env( return Ok(dyn_config.transcode_to::()?); } -fn recursively_strip_secrets( +const PLACEHOLDER: &str = ""; + +fn recursively_redact_secrets( msg: &mut DynamicMessage, secrets: &mut HashMap, parent_path: Vec, ) -> Result<(), ConfigError> { for field_descr in msg.descriptor().fields() { + // If the field is empty, there's nothing to redact. + if !msg.has_field(&field_descr) { + continue; + } + let path = { let mut path = parent_path.clone(); path.push(field_descr.name().to_uppercase()); path }; - if !msg.has_field(&field_descr) { - continue; - } - - let var_name = format!("TRAIL_{path}", path = path.join("_")); let secret = is_secret(&field_descr); + match msg.get_field_mut(&field_descr) { - Value::Message(child) => recursively_strip_secrets(child, secrets, path)?, + Value::Message(child) => recursively_redact_secrets(child, secrets, path)?, Value::Map(child_map) => { for (key, value) in child_map { match (key, value) { (MapKey::String(k), Value::Message(m)) => { + // NOTE: We're pushing a second time here, making the path segment: + // "_". let mut keyed = path.clone(); keyed.push(k.to_uppercase()); - recursively_strip_secrets(m, secrets, keyed)? + recursively_redact_secrets(m, secrets, keyed)? } x => { warn!("Unexpected message type: {x:?}"); @@ -367,8 +368,14 @@ fn recursively_strip_secrets( } Value::String(field) => { if secret { - secrets.insert(var_name, field.clone()); - msg.clear_field(&field_descr); + // Insert into map. + secrets.insert( + format!("TRAIL_{path}", path = path.join("_")), + field.clone(), + ); + + // Then redact the field. + msg.set_field(&field_descr, Value::String(PLACEHOLDER.to_string())); } } x => { @@ -382,12 +389,12 @@ fn recursively_strip_secrets( return Ok(()); } -pub(crate) fn strip_secrets( +pub(crate) fn redact_secrets( config: &proto::Config, ) -> Result<(proto::Config, HashMap), ConfigError> { let mut secrets = HashMap::::new(); let mut dyn_config = config.transcode_to_dynamic(); - recursively_strip_secrets(&mut dyn_config, &mut secrets, vec![])?; + recursively_redact_secrets(&mut dyn_config, &mut secrets, vec![])?; let stripped = dyn_config.transcode_to::()?; return Ok((stripped, secrets)); @@ -441,7 +448,7 @@ pub async fn load_or_init_config_textproto( fn split_config(config: &proto::Config) -> Result<(proto::Config, proto::Vault), ConfigError> { let mut new_vault = proto::Vault::default(); - let (stripped_config, secrets) = strip_secrets(config)?; + let (stripped_config, secrets) = redact_secrets(config)?; for (key, value) in secrets { new_vault.secrets.insert(key, value); @@ -626,6 +633,37 @@ pub(crate) fn validate_config( return Ok(()); } +#[cfg(test)] +mod test_env { + use lazy_static::lazy_static; + use parking_lot::Mutex; + use std::collections::HashMap; + + lazy_static! { + pub static ref ENV: Mutex> = Mutex::new(HashMap::new()); + } + + pub fn parse_env_var( + name: &str, + ) -> Result, ::Err> { + if let Some(value) = ENV.lock().get(name) { + return Ok(Some(value.parse::()?)); + } + Ok(None) + } + + pub fn set(name: &str, value: Option<&str>) { + match value { + None => ENV.lock().remove(name), + Some(v) => ENV.lock().insert(name.to_string(), v.to_string()), + }; + } + + pub fn clear() { + ENV.lock().clear(); + } +} + #[cfg(test)] mod test { use std::collections::HashMap; @@ -635,14 +673,13 @@ mod test { use crate::config::proto::{AuthConfig, Config, EmailConfig, OAuthProviderConfig}; #[tokio::test] - async fn test_config_tests_sequentially() -> anyhow::Result<()> { + async fn test_config_tests_sequentially() { // Run sequentially to avoid concurrent tests clobbering their env variables. test_default_config_is_valid().await; - test_config_merging()?; - test_config_stripping()?; - test_config_merging_from_env_and_vault()?; - - Ok(()) + test_config_merging(); + test_config_stripping(); + test_config_merging_from_env_and_vault(); + test_strip_and_merge(); } async fn test_default_config_is_valid() { @@ -653,7 +690,7 @@ mod test { validate_config(&table_metadata, &config).unwrap(); } - fn test_config_merging() -> anyhow::Result<()> { + fn test_config_merging() { let config = proto::Config { email: proto::EmailConfig { smtp_username: Some("user".to_string()), @@ -662,73 +699,13 @@ mod test { ..Default::default() }; let vault = proto::Vault::default(); - let merged = merge_vault_and_env(config.clone(), vault)?; + let merged = merge_vault_and_env(config.clone(), vault).unwrap(); assert_eq!(config, merged); - - return Ok(()); } - fn test_config_merging_from_env_and_vault() -> anyhow::Result<()> { - // Set username via env var. - test_env::set("TRAIL_EMAIL_SMTP_USERNAME", Some("username")); - - let client_secret = "secret".to_string(); - let vault = proto::Vault { - secrets: HashMap::::from([ - ( - "TRAIL_EMAIL_SMTP_PASSWORD".to_string(), - "password".to_string(), - ), - ( - "TRAIL_AUTH_OAUTH_PROVIDERS_KEY_CLIENT_SECRET".to_string(), - client_secret.clone(), - ), - ]), - }; - + fn test_config_stripping() { let config = proto::Config { - auth: AuthConfig { - oauth_providers: HashMap::::from([( - "key".to_string(), - OAuthProviderConfig { - client_id: Some("my_client_id".to_string()), - ..Default::default() - }, - )]), - ..Default::default() - }, - ..Default::default() - }; - - let merged = merge_vault_and_env(config.clone(), vault)?; - test_env::set("TRAIL_EMAIL_SMTP_USERNAME", None); - - // Update config to match what we would expect after merging. - let expected = { - let mut expected = config.clone(); - expected.email = EmailConfig { - smtp_username: Some("username".to_string()), - smtp_password: Some("password".to_string()), - ..Default::default() - }; - expected - .auth - .oauth_providers - .get_mut("key") - .unwrap() - .client_secret = Some(client_secret); - - expected - }; - - assert_eq!(merged, expected); - - return Ok(()); - } - - fn test_config_stripping() -> anyhow::Result<()> { - let mut config = proto::Config { email: proto::EmailConfig { smtp_username: Some("user".to_string()), smtp_password: Some("pass".to_string()), @@ -748,17 +725,22 @@ mod test { ..Default::default() }; - let (stripped, secrets) = strip_secrets(&config)?; + let expected = { + let mut expected = config.clone(); + // Redact field + expected.email.smtp_password = Some(PLACEHOLDER.to_string()); + // Redact map entry. + expected + .auth + .oauth_providers + .get_mut("key") + .unwrap() + .client_secret = Some(PLACEHOLDER.to_string()); + expected + }; - config.email.smtp_password = None; - config - .auth - .oauth_providers - .get_mut("key") - .unwrap() - .client_secret = None; - - assert_eq!(config, stripped); + let (stripped, secrets) = redact_secrets(&config).unwrap(); + assert_eq!(stripped, expected); assert_eq!( secrets.get("TRAIL_EMAIL_SMTP_PASSWORD"), Some(&"pass".to_string()) @@ -767,8 +749,106 @@ mod test { secrets.get("TRAIL_AUTH_OAUTH_PROVIDERS_KEY_CLIENT_SECRET"), Some(&"secret".to_string()) ); + } - return Ok(()); + fn test_config_merging_from_env_and_vault() { + // Set username via env var. + let username = "secret_username"; + test_env::set("TRAIL_EMAIL_SMTP_USERNAME", Some(username)); + + let password = "secret_password"; + let client_secret = "secret".to_string(); + let outh_map_key = "fake_provider"; + let vault = proto::Vault { + secrets: HashMap::::from([ + ( + "TRAIL_EMAIL_SMTP_PASSWORD".to_string(), + password.to_string(), + ), + ( + format!( + "TRAIL_AUTH_OAUTH_PROVIDERS_{}_CLIENT_SECRET", + outh_map_key.to_uppercase() + ), + client_secret.clone(), + ), + ( + format!("TRAIL_AUTH_OAUTH_PROVIDERS_MISSING_CLIENT_SECRET"), + "SHOULD NOT BE SET".to_string(), + ), + ]), + }; + + let config = proto::Config { + email: EmailConfig { + smtp_username: Some(PLACEHOLDER.to_string()), + smtp_password: Some(PLACEHOLDER.to_string()), + ..Default::default() + }, + auth: AuthConfig { + oauth_providers: HashMap::::from([( + outh_map_key.to_string(), + OAuthProviderConfig { + client_id: Some("my_client_id".to_string()), + client_secret: Some(PLACEHOLDER.to_string()), + ..Default::default() + }, + )]), + ..Default::default() + }, + ..Default::default() + }; + + let merged = merge_vault_and_env(config.clone(), vault).unwrap(); + test_env::clear(); + + // Build expected config with secrets. + let expected = { + let mut expected = config.clone(); + expected.email = EmailConfig { + smtp_username: Some(username.to_string()), + smtp_password: Some(password.to_string()), + ..Default::default() + }; + expected + .auth + .oauth_providers + .get_mut(outh_map_key) + .unwrap() + .client_secret = Some(client_secret); + + expected + }; + + assert_eq!(merged, expected); + } + + fn test_strip_and_merge() { + let config = proto::Config { + email: EmailConfig { + smtp_username: Some("secret_username".to_string()), + smtp_password: Some("secret_password".to_string()), + ..Default::default() + }, + auth: AuthConfig { + oauth_providers: HashMap::::from([( + "fake_provider".to_string(), + OAuthProviderConfig { + client_id: Some("my_client_id".to_string()), + client_secret: Some("secret_client_secret".to_string()), + ..Default::default() + }, + )]), + ..Default::default() + }, + ..Default::default() + }; + + let (stripped, secrets) = redact_secrets(&config).unwrap(); + let vault = proto::Vault { secrets }; + let merged = merge_vault_and_env(stripped, vault).unwrap(); + + assert_eq!(config, merged); } }