diff --git a/backend/tests/instance_config.rs b/backend/tests/instance_config.rs index e0ac2a4feb..cc0b8bae2c 100644 --- a/backend/tests/instance_config.rs +++ b/backend/tests/instance_config.rs @@ -280,6 +280,7 @@ async fn test_apply_settings_diff_upserts_only(db: Pool) { m }, deletes: vec![], + ..Default::default() }; apply_settings_diff(&db, &diff).await.unwrap(); @@ -303,6 +304,7 @@ async fn test_apply_settings_diff_deletes_only(db: Pool) { let diff = SettingsDiff { upserts: BTreeMap::new(), deletes: vec!["to_delete_1".to_string(), "to_delete_2".to_string()], + ..Default::default() }; apply_settings_diff(&db, &diff).await.unwrap(); @@ -323,6 +325,7 @@ async fn test_apply_settings_diff_upserts_and_deletes(db: Pool) { m }, deletes: vec!["old_key".to_string()], + ..Default::default() }; apply_settings_diff(&db, &diff).await.unwrap(); @@ -358,6 +361,7 @@ async fn test_apply_settings_diff_upsert_overwrites(db: Pool) { m }, deletes: vec![], + ..Default::default() }; apply_settings_diff(&db, &diff).await.unwrap(); @@ -385,6 +389,7 @@ async fn test_apply_settings_diff_complex_json(db: Pool) { m }, deletes: vec![], + ..Default::default() }; apply_settings_diff(&db, &diff).await.unwrap(); @@ -396,7 +401,11 @@ async fn test_apply_settings_diff_complex_json(db: Pool) { #[sqlx::test(fixtures("base"))] async fn test_apply_settings_diff_delete_nonexistent_is_noop(db: Pool) { let diff = - SettingsDiff { upserts: BTreeMap::new(), deletes: vec!["does_not_exist".to_string()] }; + SettingsDiff { + upserts: BTreeMap::new(), + deletes: vec!["does_not_exist".to_string()], + ..Default::default() + }; // Should not error apply_settings_diff(&db, &diff).await.unwrap(); @@ -648,7 +657,11 @@ async fn test_roundtrip_to_settings_map_from_db_consistency(db: Pool) }; let map = original.to_settings_map(); - let diff = SettingsDiff { upserts: map.into_iter().collect(), deletes: vec![] }; + let diff = SettingsDiff { + upserts: map.into_iter().collect(), + deletes: vec![], + ..Default::default() + }; apply_settings_diff(&db, &diff).await.unwrap(); @@ -694,6 +707,7 @@ async fn test_idempotent_apply(db: Pool) { m }, deletes: vec![], + ..Default::default() }; // Apply twice diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 86233a8d21..3f46ee94b2 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -278,8 +278,27 @@ pub async fn set_global_setting_internal( key: String, value: serde_json::Value, ) -> error::Result<()> { + let value = if key == "retention_period_secs" { + instance_config::clamp_retention_period(value) + } else { + value + }; + run_setting_pre_write_hook(db, &key, &value).await?; + if key == "jwt_secret" { + match &value { + serde_json::Value::Null | serde_json::Value::String(_) + if value.as_str().map_or(true, |s| s.is_empty()) => + { + return Err(error::Error::BadRequest( + "jwt_secret cannot be set to empty or null".to_string(), + )); + } + _ => {} + } + } + match value { serde_json::Value::Null => { delete_global_setting(db, &key).await?; @@ -420,8 +439,8 @@ async fn get_instance_config_yaml( let config = InstanceConfig::from_db(&db) .await .map_err(|e| error::Error::internal_err(e.to_string()))?; - let yaml = serde_yml::to_string(&config) - .map_err(|e| error::Error::internal_err(format!("YAML serialization failed: {e}")))?; + let yaml = config.to_sorted_yaml() + .map_err(|e| error::Error::internal_err(e))?; Response::builder() .header("content-type", "application/yaml") .body(Body::from(yaml)) @@ -1141,7 +1160,7 @@ mod tests { )]), }; - let yaml = serde_yml::to_string(&config).unwrap(); + let yaml = config.to_sorted_yaml().unwrap(); // Verify key fields appear in the YAML output assert!(yaml.contains("base_url: https://windmill.example.com")); @@ -1167,4 +1186,110 @@ mod tests { ); assert_eq!(wc.init_bash.as_deref(), Some("apt-get update")); } + + #[test] + fn sorted_yaml_global_settings_alphabetical() { + let config = InstanceConfig { + global_settings: GlobalSettings { + retention_period_secs: Some(3600), + base_url: Some("https://test.com".to_string()), + expose_metrics: Some(true), + email_domain: Some("example.com".to_string()), + ..Default::default() + }, + worker_configs: BTreeMap::new(), + }; + + let yaml = config.to_sorted_yaml().unwrap(); + + // Keys must appear in alphabetical order + let base_url_pos = yaml.find("base_url:").unwrap(); + let email_pos = yaml.find("email_domain:").unwrap(); + let expose_pos = yaml.find("expose_metrics:").unwrap(); + let retention_pos = yaml.find("retention_period_secs:").unwrap(); + + assert!( + base_url_pos < email_pos + && email_pos < expose_pos + && expose_pos < retention_pos, + "global_settings keys should be alphabetically sorted, got yaml:\n{yaml}" + ); + } + + #[test] + fn sorted_yaml_worker_configs_default_and_native_first() { + let config = InstanceConfig { + global_settings: GlobalSettings::default(), + worker_configs: BTreeMap::from([ + ("gpu".to_string(), WorkerGroupConfig { + init_bash: Some("echo gpu".to_string()), + ..Default::default() + }), + ("native".to_string(), WorkerGroupConfig { + init_bash: Some("echo native".to_string()), + ..Default::default() + }), + ("default".to_string(), WorkerGroupConfig { + init_bash: Some("echo default".to_string()), + ..Default::default() + }), + ("alpha".to_string(), WorkerGroupConfig { + init_bash: Some("echo alpha".to_string()), + ..Default::default() + }), + ]), + }; + + let yaml = config.to_sorted_yaml().unwrap(); + + let default_pos = yaml.find("default:").unwrap(); + let native_pos = yaml.find("native:").unwrap(); + let alpha_pos = yaml.find("alpha:").unwrap(); + let gpu_pos = yaml.find("gpu:").unwrap(); + + assert!( + default_pos < native_pos + && native_pos < alpha_pos + && alpha_pos < gpu_pos, + "worker_configs should have default, native first, then rest alphabetically, got yaml:\n{yaml}" + ); + } + + #[test] + fn sorted_yaml_roundtrips() { + let config = InstanceConfig { + global_settings: GlobalSettings { + base_url: Some("https://rt.test".to_string()), + retention_period_secs: Some(7200), + expose_metrics: Some(false), + ..Default::default() + }, + worker_configs: BTreeMap::from([ + ("default".to_string(), WorkerGroupConfig { + worker_tags: Some(vec!["deno".to_string()]), + ..Default::default() + }), + ("native".to_string(), WorkerGroupConfig { + init_bash: Some("echo hi".to_string()), + ..Default::default() + }), + ]), + }; + + let yaml = config.to_sorted_yaml().unwrap(); + let deserialized: InstanceConfig = serde_yml::from_str(&yaml).unwrap(); + + assert_eq!(deserialized.global_settings.base_url.as_deref(), Some("https://rt.test")); + assert_eq!(deserialized.global_settings.retention_period_secs, Some(7200)); + assert_eq!(deserialized.global_settings.expose_metrics, Some(false)); + assert_eq!(deserialized.worker_configs.len(), 2); + assert_eq!( + deserialized.worker_configs["default"].worker_tags.as_deref(), + Some(["deno".to_string()].as_slice()) + ); + assert_eq!( + deserialized.worker_configs["native"].init_bash.as_deref(), + Some("echo hi") + ); + } } diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 653734d5ca..0e2fe5ad72 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -738,6 +738,10 @@ pub enum ApplyMode { pub struct SettingsDiff { pub upserts: BTreeMap, pub deletes: Vec, + /// Previous values for keys being updated or deleted (for logging). + pub previous_values: BTreeMap, + /// Count of desired keys that matched the current value exactly. + pub unchanged_count: usize, } /// The diff result for worker configs. @@ -754,10 +758,110 @@ pub const PROTECTED_SETTINGS: &[&str] = &[ "custom_instance_pg_databases", "uid", "rsa_keys", + "jwt_secret", + "min_keep_alive_version", ]; /// Internal settings that are never exposed via the API or included in config exports. -pub const HIDDEN_SETTINGS: &[&str] = &["uid", "rsa_keys", "jwt_secret", "min_keep_alive_version"]; +/// Note: jwt_secret is intentionally NOT hidden — it is included in YAML exports so that +/// operators can set it via ConfigMap. It is protected from deletion (PROTECTED_SETTINGS) +/// and from being set to empty/null, and its value is partially redacted in log output. +pub const HIDDEN_SETTINGS: &[&str] = &["uid", "rsa_keys", "min_keep_alive_version"]; + +/// Top-level settings whose entire value is sensitive and must be fully redacted in logs. +const SENSITIVE_SETTINGS: &[&str] = &[ + "jwt_secret", + "scim_token", + "hub_api_secret", + "license_key", + "pip_index_url", + "pip_extra_index_url", + "npm_config_registry", + "bunfig_install_scopes", + "maven_repos", + "ruby_repos", + "powershell_repo_pat", +]; + +/// Object-valued settings that contain sensitive sub-fields. +/// Maps a top-level key to the sub-field names that must be redacted. +const NESTED_SENSITIVE_FIELDS: &[(&str, &[&str])] = &[ + ("smtp_settings", &["smtp_password"]), + ("secret_backend", &["token"]), + ("object_store_cache_config", &["secret_key", "serviceAccountKey"]), +]; + +fn redact_json_value(value: &serde_json::Value) -> serde_json::Value { + match value { + serde_json::Value::String(s) => serde_json::json!(redact_string(s)), + _ => serde_json::json!("[redacted]"), + } +} + +fn mask_nested_sensitive(key: &str, value: &serde_json::Value) -> serde_json::Value { + for &(parent_key, sub_fields) in NESTED_SENSITIVE_FIELDS { + if key == parent_key { + if let serde_json::Value::Object(map) = value { + let mut masked = map.clone(); + for &field in sub_fields { + if let Some(v) = masked.get(field) { + masked.insert(field.to_string(), redact_json_value(v)); + } + } + return serde_json::Value::Object(masked); + } + } + } + // Settings that are maps-of-objects where each child has a sensitive sub-field. + const NESTED_MAP_SENSITIVE: &[(&str, &str)] = &[ + ("oauths", "secret"), + ("custom_instance_pg_databases", "user_pwd"), + ]; + for &(parent_key, child_field) in NESTED_MAP_SENSITIVE { + if key == parent_key { + if let serde_json::Value::Object(entries) = value { + let mut masked = entries.clone(); + for (_entry_key, entry_val) in masked.iter_mut() { + if let serde_json::Value::Object(ref mut obj) = entry_val { + if let Some(v) = obj.get(child_field) { + obj.insert(child_field.to_string(), redact_json_value(v)); + } + } + } + return serde_json::Value::Object(masked); + } + } + } + value.clone() +} + +fn redact_string(s: &str) -> String { + let char_count = s.chars().count(); + if char_count <= 6 { + "****".to_string() + } else { + let show = (char_count / 4).min(4); + let prefix: String = s.chars().take(show).collect(); + let suffix: String = s.chars().skip(char_count - show).collect(); + format!("{prefix}****{suffix}") + } +} + +fn format_setting_value(key: &str, value: &serde_json::Value) -> String { + if SENSITIVE_SETTINGS.contains(&key) { + return match value { + serde_json::Value::String(s) => format!("\"{}\"", redact_string(s)), + _ => "[redacted]".to_string(), + }; + } + let value = mask_nested_sensitive(key, value); + let s = value.to_string(); + if s.len() > 200 { + format!("{}...", &s[..197]) + } else { + s + } +} /// Extract the expiry timestamp from a license key JSON value. /// @@ -786,6 +890,36 @@ fn license_keys_same_except_expiry(a: &serde_json::Value, b: &serde_json::Value) a_parts[0] == b_parts[0] && a_parts[2] == b_parts[2] } +fn is_empty_or_null(value: &serde_json::Value) -> bool { + value.is_null() || value.as_str().map_or(false, |s| s.is_empty()) +} + +/// Maximum retention period in seconds for CE builds (30 days). +pub const CE_MAX_RETENTION_PERIOD_SECS: i64 = 30 * 24 * 3600; + +/// Clamp `retention_period_secs` to `CE_MAX_RETENTION_PERIOD_SECS` on CE builds. +/// Returns the (possibly clamped) value. On EE builds this is a no-op. +pub fn clamp_retention_period(value: serde_json::Value) -> serde_json::Value { + #[cfg(feature = "enterprise")] + { + value + } + #[cfg(not(feature = "enterprise"))] + { + if let Some(secs) = value.as_i64() { + if secs > CE_MAX_RETENTION_PERIOD_SECS { + tracing::warn!( + "Clamping retention_period_secs from {} to {} (CE max: 30 days)", + secs, + CE_MAX_RETENTION_PERIOD_SECS + ); + return serde_json::json!(CE_MAX_RETENTION_PERIOD_SECS); + } + } + value + } +} + /// Compute the diff between current and desired global settings. pub fn diff_global_settings( current: &BTreeMap, @@ -793,28 +927,50 @@ pub fn diff_global_settings( mode: ApplyMode, ) -> SettingsDiff { let mut upserts = BTreeMap::new(); - for (key, value) in desired { + let mut previous_values = BTreeMap::new(); + let mut unchanged_count: usize = 0; + for (key, desired_value) in desired { + if key == "jwt_secret" && is_empty_or_null(desired_value) { + tracing::warn!( + "Skipping jwt_secret update: value must not be empty or null" + ); + continue; + } + let value = if key == "retention_period_secs" { + clamp_retention_period(desired_value.clone()) + } else { + desired_value.clone() + }; match current.get(key) { - Some(existing) if existing == value => {} // no change + Some(existing) if *existing == value => { + unchanged_count += 1; + } Some(existing) if key == LICENSE_KEY_SETTING => { - if license_keys_same_except_expiry(existing, value) { + if license_keys_same_except_expiry(existing, &value) { let current_expiry = license_key_expiry(existing).unwrap_or(0); - let desired_expiry = license_key_expiry(value).unwrap_or(0); + let desired_expiry = license_key_expiry(&value).unwrap_or(0); if desired_expiry > current_expiry { - upserts.insert(key.clone(), value.clone()); + previous_values.insert(key.clone(), existing.clone()); + upserts.insert(key.clone(), value); } else { tracing::info!( "Skipping license_key update: desired expiry ({}) is not posterior to current expiry ({})", desired_expiry, current_expiry ); + unchanged_count += 1; } } else { - upserts.insert(key.clone(), value.clone()); + previous_values.insert(key.clone(), existing.clone()); + upserts.insert(key.clone(), value); } } - _ => { - upserts.insert(key.clone(), value.clone()); + Some(existing) => { + previous_values.insert(key.clone(), existing.clone()); + upserts.insert(key.clone(), value); + } + None => { + upserts.insert(key.clone(), value); } } } @@ -822,11 +978,12 @@ pub fn diff_global_settings( if matches!(mode, ApplyMode::Replace) { for key in current.keys() { if !desired.contains_key(key) && !PROTECTED_SETTINGS.contains(&key.as_str()) { + previous_values.insert(key.clone(), current[key].clone()); deletes.push(key.clone()); } } } - SettingsDiff { upserts, deletes } + SettingsDiff { upserts, deletes, previous_values, unchanged_count } } /// Compute the diff between current and desired worker configs. @@ -869,14 +1026,35 @@ pub async fn apply_settings_diff( .bind(value) .execute(db) .await?; - tracing::info!("Synced global setting: {key}"); + if let Some(old_value) = diff.previous_values.get(key) { + tracing::info!( + "Updated global setting: {key} ({} -> {})", + format_setting_value(key, old_value), + format_setting_value(key, value) + ); + } else { + tracing::info!( + "Created global setting: {key} (value: {})", + format_setting_value(key, value) + ); + } } for key in &diff.deletes { sqlx::query("DELETE FROM global_settings WHERE name = $1") .bind(key) .execute(db) .await?; - tracing::info!("Deleted global setting: {key}"); + if let Some(old_value) = diff.previous_values.get(key) { + tracing::info!( + "Deleted global setting: {key} (was: {})", + format_setting_value(key, old_value) + ); + } else { + tracing::info!("Deleted global setting: {key}"); + } + } + if diff.unchanged_count > 0 { + tracing::info!("{} global setting(s) unchanged", diff.unchanged_count); } Ok(()) } @@ -1038,6 +1216,68 @@ impl InstanceConfig { Ok(()) } + + /// Serialize to YAML with sorted global_settings keys and worker_configs + /// ordered with "default" and "native" first. + pub fn to_sorted_yaml(&self) -> Result { + let settings_map = self.global_settings.to_settings_map(); + + let mut yaml = String::from("global_settings:\n"); + for (key, value) in &settings_map { + let value_yaml = serde_yml::to_string(value) + .map_err(|e| format!("YAML serialization failed for {key}: {e}"))?; + write_yaml_field(&mut yaml, key, value, &value_yaml, 1); + } + + if !self.worker_configs.is_empty() { + yaml.push_str("worker_configs:\n"); + let priority_keys = ["default", "native"]; + for &pk in &priority_keys { + if let Some(wc) = self.worker_configs.get(pk) { + let wc_value = serde_json::to_value(wc) + .map_err(|e| format!("JSON serialization failed for {pk}: {e}"))?; + let wc_yaml = serde_yml::to_string(&wc_value) + .map_err(|e| format!("YAML serialization failed for {pk}: {e}"))?; + write_yaml_field(&mut yaml, pk, &wc_value, &wc_yaml, 1); + } + } + for (key, wc) in &self.worker_configs { + if priority_keys.contains(&key.as_str()) { + continue; + } + let wc_value = serde_json::to_value(wc) + .map_err(|e| format!("JSON serialization failed for {key}: {e}"))?; + let wc_yaml = serde_yml::to_string(&wc_value) + .map_err(|e| format!("YAML serialization failed for {key}: {e}"))?; + write_yaml_field(&mut yaml, key, &wc_value, &wc_yaml, 1); + } + } + + Ok(yaml) + } +} + +fn write_yaml_field( + yaml: &mut String, + key: &str, + value: &serde_json::Value, + value_yaml: &str, + indent: usize, +) { + use std::fmt::Write; + + let prefix = " ".repeat(indent); + let trimmed = value_yaml.trim(); + let is_nested = matches!(value, serde_json::Value::Object(_) | serde_json::Value::Array(_)); + if is_nested { + let _ = writeln!(yaml, "{prefix}{key}:"); + let inner_prefix = " ".repeat(indent + 1); + for line in trimmed.lines() { + let _ = writeln!(yaml, "{inner_prefix}{line}"); + } + } else { + let _ = writeln!(yaml, "{prefix}{key}: {trimmed}"); + } } // --------------------------------------------------------------------------- @@ -1393,7 +1633,7 @@ mod tests { } #[test] - fn diff_global_settings_replace_protects_all_three() { + fn diff_global_settings_replace_protects_all() { let mut current = BTreeMap::new(); for key in PROTECTED_SETTINGS { current.insert(key.to_string(), serde_json::json!("val"));