From c6dda716f4286e18911a76e2bf679672456108d0 Mon Sep 17 00:00:00 2001 From: "glm-5.1" Date: Mon, 15 Jun 2026 06:13:48 +0000 Subject: [PATCH] Add mtime TOCTOU check and wildcard flag to ConfigReloadHandle (ADR-029/030) Extract shared read_and_validate_config() with before/after mtime check to detect mid-write config file changes. Add ReloadError enum with FileChangedDuringRead variant. Return HTTP 409 Conflict on mtime change from admin reload endpoint. Store cli_allow_wildcard_bind in ConfigReloadHandle and use it in reload() validation instead of hardcoded false. Update all ConfigReloadHandle::new() call sites. --- src/admin/handler.rs | 38 ++--- src/config/dynamic_config.rs | 24 ++- src/config/mod.rs | 282 +++++++++++++++++++++++++++++++++++ src/health.rs | 4 +- src/main.rs | 1 + src/shutdown.rs | 23 ++- tests/integration_test.rs | 4 +- 7 files changed, 335 insertions(+), 41 deletions(-) diff --git a/src/admin/handler.rs b/src/admin/handler.rs index 42dec53..0b6b52d 100644 --- a/src/admin/handler.rs +++ b/src/admin/handler.rs @@ -40,10 +40,26 @@ pub struct RotateKeyResponse { } pub async fn reload_handler(State(state): State>) -> impl IntoResponse { - let config_content = match tokio::fs::read_to_string(&state.config_path).await { - Ok(content) => content, + let result = crate::config::read_and_validate_config( + &state.config_path, + state.reload_handle.cli_allow_wildcard_bind(), + ) + .await; + + let (new_static, new_dynamic) = match result { + Ok(configs) => configs, + Err(crate::config::ReloadError::FileChangedDuringRead) => { + tracing::warn!("admin reload: config file changed during read"); + return ( + StatusCode::CONFLICT, + Json(ReloadResponse { + status: "error", + message: Some("config file changed during read, please retry".to_string()), + }), + ); + } Err(e) => { - tracing::error!("admin reload: failed to read config file: {}", e); + tracing::error!("admin reload: config read/validate failed: {}", e); return ( StatusCode::INTERNAL_SERVER_ERROR, Json(ReloadResponse { @@ -54,22 +70,6 @@ pub async fn reload_handler(State(state): State>) -> impl IntoRe } }; - let full_config = match crate::config::FullConfig::parse(&config_content) { - Ok(c) => c, - Err(e) => { - tracing::error!("admin reload: failed to parse config file: {}", e); - return ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(ReloadResponse { - status: "error", - message: Some("reload failed".to_string()), - }), - ); - } - }; - - let (new_static, new_dynamic) = full_config.into_static_and_dynamic(); - match state.reload_handle.reload(new_static, new_dynamic).await { Ok(changed_fields) => { if !changed_fields.is_empty() { diff --git a/src/config/dynamic_config.rs b/src/config/dynamic_config.rs index 29444eb..b4dcb6c 100644 --- a/src/config/dynamic_config.rs +++ b/src/config/dynamic_config.rs @@ -112,14 +112,20 @@ pub struct ConfigReloadHandle { config: Arc>, static_config: ArcSwap, reload_mutex: Mutex<()>, + cli_allow_wildcard_bind: bool, } impl ConfigReloadHandle { - pub fn new(config: Arc>, static_config: StaticConfig) -> Self { + pub fn new( + config: Arc>, + static_config: StaticConfig, + cli_allow_wildcard_bind: bool, + ) -> Self { Self { config, static_config: ArcSwap::from_pointee(static_config), reload_mutex: Mutex::new(()), + cli_allow_wildcard_bind, } } @@ -131,6 +137,10 @@ impl ConfigReloadHandle { self.static_config.load_full() } + pub fn cli_allow_wildcard_bind(&self) -> bool { + self.cli_allow_wildcard_bind + } + pub async fn reload( &self, new_static: StaticConfig, @@ -138,7 +148,7 @@ impl ConfigReloadHandle { ) -> anyhow::Result> { let _guard = self.reload_mutex.lock().await; - validate(&new_static, &new_dynamic, false).map_err(|errors| { + validate(&new_static, &new_dynamic, self.cli_allow_wildcard_bind).map_err(|errors| { anyhow::anyhow!( "{}", errors @@ -193,7 +203,7 @@ mod tests { let initial = test_fixtures::test_dynamic_config(); let config_arc = Arc::new(ArcSwap::from_pointee(initial.clone())); let static_config = test_fixtures::test_static_config(); - let handle = ConfigReloadHandle::new(config_arc.clone(), static_config); + let handle = ConfigReloadHandle::new(config_arc.clone(), static_config, false); let loaded = handle.load(); assert_eq!(loaded.sites.len(), 1); @@ -231,7 +241,7 @@ mod tests { let initial = test_fixtures::test_dynamic_config(); let config_arc = Arc::new(ArcSwap::from_pointee(initial.clone())); let static_config = test_fixtures::test_static_config(); - let handle = ConfigReloadHandle::new(config_arc.clone(), static_config); + let handle = ConfigReloadHandle::new(config_arc.clone(), static_config, false); let mut invalid_dynamic = initial.clone(); invalid_dynamic.rate_limit.requests_per_second = 0; @@ -250,7 +260,7 @@ mod tests { let initial = test_fixtures::test_dynamic_config(); let config_arc = Arc::new(ArcSwap::from_pointee(initial.clone())); let static_config = test_fixtures::test_static_config(); - let handle = Arc::new(ConfigReloadHandle::new(config_arc.clone(), static_config)); + let handle = Arc::new(ConfigReloadHandle::new(config_arc.clone(), static_config, false)); let mut handles = Vec::new(); for i in 1..=5u32 { @@ -299,7 +309,7 @@ mod tests { let initial = test_fixtures::test_dynamic_config(); let config_arc = Arc::new(ArcSwap::from_pointee(initial.clone())); let original_static = test_fixtures::test_static_config(); - let handle = ConfigReloadHandle::new(config_arc.clone(), original_static.clone()); + let handle = ConfigReloadHandle::new(config_arc.clone(), original_static.clone(), false); let mut changed_static = original_static.clone(); changed_static.health_check_port = 8080; @@ -322,7 +332,7 @@ mod tests { let initial = test_fixtures::test_dynamic_config(); let config_arc = Arc::new(ArcSwap::from_pointee(initial.clone())); let original_static = test_fixtures::test_static_config(); - let handle = ConfigReloadHandle::new(config_arc.clone(), original_static.clone()); + let handle = ConfigReloadHandle::new(config_arc.clone(), original_static.clone(), false); let mut changed_static = original_static.clone(); changed_static.health_check_port = 8080; diff --git a/src/config/mod.rs b/src/config/mod.rs index 05b22d0..763afbd 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -11,6 +11,31 @@ pub use static_config::{ListenerConfig, LoggingConfig, StaticConfig, TlsConfig}; pub use validation::{validate, ValidationError}; use serde::Deserialize; +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum ReloadError { + #[error("IO error reading config file: {0}")] + Io(#[from] std::io::Error), + #[error("Failed to parse config file: {0}")] + Parse(#[from] toml::de::Error), + #[error("Config validation failed: {0}")] + Validation(String), + #[error("config file changed during read, please retry")] + FileChangedDuringRead, +} + +impl From> for ReloadError { + fn from(errors: Vec) -> Self { + ReloadError::Validation( + errors + .iter() + .map(|e| e.to_string()) + .collect::>() + .join("; "), + ) + } +} #[derive(Debug, Deserialize)] pub struct FullConfig { @@ -56,3 +81,260 @@ impl FullConfig { (static_config, dynamic_config) } } + +pub async fn read_and_validate_config( + config_path: &str, + cli_allow_wildcard_bind: bool, +) -> Result<(StaticConfig, DynamicConfig), ReloadError> { + let metadata_before = tokio::fs::metadata(config_path).await?; + let config_content = tokio::fs::read_to_string(config_path).await?; + let metadata_after = tokio::fs::metadata(config_path).await?; + + if metadata_before.modified().ok() != metadata_after.modified().ok() { + tracing::warn!( + event = "CONFIG_RELOAD", + status = "rejected", + reason = "file_mtime_changed", + path = config_path + ); + return Err(ReloadError::FileChangedDuringRead); + } + + let full_config: FullConfig = toml::from_str(&config_content)?; + let (new_static, new_dynamic) = full_config.into_static_and_dynamic(); + + validate(&new_static, &new_dynamic, cli_allow_wildcard_bind)?; + + Ok((new_static, new_dynamic)) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Write; + + fn valid_config_toml() -> &'static str { + r#" +health_check_port = 9900 +admin_key_path = "/tmp/test-admin-key" + +[logging] +level = "info" +format = "text" + +[rate_limit] +requests_per_second = 20 +burst = 40 + +[body] +limit_bytes = 104857600 + +[[listeners]] +bind_addr = "127.0.0.1" +http_port = 80 +https_port = 443 + +[listeners.tls] +mode = "acme" +acme_domains = ["test.local"] +acme_cache_dir = "/tmp/acme-cache" +acme_directory = "staging" +acme_contact = "mailto:admin@test.local" + +[[listeners.sites]] +host = "test.local" +upstream = "127.0.0.1:8080" +"# + } + + #[tokio::test] + async fn read_and_validate_config_valid_file() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + std::fs::write(&config_path, valid_config_toml()).unwrap(); + + let result = read_and_validate_config(config_path.to_str().unwrap(), false).await; + assert!(result.is_ok()); + let (static_config, dynamic_config) = result.unwrap(); + assert_eq!(dynamic_config.rate_limit.requests_per_second, 20); + assert_eq!(static_config.health_check_port, 9900); + } + + #[tokio::test] + async fn read_and_validate_config_missing_file() { + let result = read_and_validate_config("/nonexistent/config.toml", false).await; + assert!(result.is_err()); + match result.unwrap_err() { + ReloadError::Io(_) => {} + e => panic!("expected Io error, got: {:?}", e), + } + } + + #[tokio::test] + async fn read_and_validate_config_invalid_toml() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + std::fs::write(&config_path, "invalid toml {{{").unwrap(); + + let result = read_and_validate_config(config_path.to_str().unwrap(), false).await; + assert!(result.is_err()); + match result.unwrap_err() { + ReloadError::Parse(_) => {} + e => panic!("expected Parse error, got: {:?}", e), + } + } + + #[tokio::test] + async fn read_and_validate_config_validation_fails() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + let config_content = r#" +health_check_port = 9900 + +[logging] +level = "info" +format = "text" + +[rate_limit] +requests_per_second = 0 +burst = 20 + +[body] +limit_bytes = 104857600 + +[[listeners]] +bind_addr = "127.0.0.1" +http_port = 80 +https_port = 443 + +[listeners.tls] +mode = "acme" +acme_domains = ["test.local"] +acme_cache_dir = "/tmp/acme-cache" +acme_contact = "mailto:admin@test.local" +"#; + std::fs::write(&config_path, config_content).unwrap(); + + let result = read_and_validate_config(config_path.to_str().unwrap(), false).await; + assert!(result.is_err()); + match result.unwrap_err() { + ReloadError::Validation(msg) => { + assert!(msg.contains("requests_per_second")); + } + e => panic!("expected Validation error, got: {:?}", e), + } + } + + #[tokio::test] + async fn read_and_validate_config_wildcard_bind_allowed_with_flag() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + let config_content = r#" +health_check_port = 9900 + +[logging] +level = "info" +format = "text" + +[rate_limit] +requests_per_second = 10 +burst = 20 + +[body] +limit_bytes = 104857600 + +[[listeners]] +bind_addr = "0.0.0.0" +http_port = 80 +https_port = 443 + +[listeners.tls] +mode = "acme" +acme_domains = ["test.local"] +acme_cache_dir = "/tmp/acme-cache" +acme_contact = "mailto:admin@test.local" + +[[listeners.sites]] +host = "test.local" +upstream = "127.0.0.1:8080" +"#; + std::fs::write(&config_path, config_content).unwrap(); + + let result = read_and_validate_config(config_path.to_str().unwrap(), true).await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn read_and_validate_config_wildcard_bind_rejected_without_flag() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + let config_content = r#" +health_check_port = 9900 + +[logging] +level = "info" +format = "text" + +[rate_limit] +requests_per_second = 10 +burst = 20 + +[body] +limit_bytes = 104857600 + +[[listeners]] +bind_addr = "0.0.0.0" +http_port = 80 +https_port = 443 + +[listeners.tls] +mode = "acme" +acme_domains = ["test.local"] +acme_cache_dir = "/tmp/acme-cache" +acme_contact = "mailto:admin@test.local" + +[[listeners.sites]] +host = "test.local" +upstream = "127.0.0.1:8080" +"#; + std::fs::write(&config_path, config_content).unwrap(); + + let result = read_and_validate_config(config_path.to_str().unwrap(), false).await; + assert!(result.is_err()); + match result.unwrap_err() { + ReloadError::Validation(msg) => { + assert!(msg.contains("0.0.0.0")); + } + e => panic!("expected Validation error, got: {:?}", e), + } + } + + #[tokio::test] + async fn read_and_validate_config_mtime_change_detected() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + std::fs::write(&config_path, valid_config_toml()).unwrap(); + + let path = config_path.clone(); + let handle = tokio::spawn(async move { + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + let mut file = std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(&path) + .unwrap(); + file.write_all(valid_config_toml().as_bytes()).unwrap(); + file.sync_all().unwrap(); + }); + + let result = read_and_validate_config(config_path.to_str().unwrap(), false).await; + + handle.await.unwrap(); + + match result { + Err(ReloadError::FileChangedDuringRead) => {} + Ok(_) => {} + Err(e) => panic!("expected FileChangedDuringRead or Ok, got: {:?}", e), + } + } +} diff --git a/src/health.rs b/src/health.rs index 208537a..f968ecb 100644 --- a/src/health.rs +++ b/src/health.rs @@ -84,7 +84,7 @@ mod tests { test_fixtures::test_dynamic_config(), )); let static_config = test_fixtures::test_static_config(); - let reload_handle = Arc::new(ConfigReloadHandle::new(config_arc, static_config)); + let reload_handle = Arc::new(ConfigReloadHandle::new(config_arc, static_config, false)); let admin_state = Arc::new(AdminState { reload_handle, @@ -169,7 +169,7 @@ upstream = "127.0.0.1:8080" test_fixtures::test_dynamic_config(), )); let static_config = test_fixtures::test_static_config(); - let reload_handle = Arc::new(ConfigReloadHandle::new(config_arc, static_config)); + let reload_handle = Arc::new(ConfigReloadHandle::new(config_arc, static_config, false)); let admin_state = Arc::new(AdminState { reload_handle, diff --git a/src/main.rs b/src/main.rs index 9e467f3..e019b50 100644 --- a/src/main.rs +++ b/src/main.rs @@ -85,6 +85,7 @@ async fn run_server(loaded_config: cli::LoadedConfig, config_path: &str) -> Resu let reload_handle = Arc::new(ConfigReloadHandle::new( config_arc.clone(), loaded_config.static_config.clone(), + loaded_config.allow_wildcard_bind, )); reverse_proxy::shutdown::register_signal_handlers( diff --git a/src/shutdown.rs b/src/shutdown.rs index 54da39b..a9b0df4 100644 --- a/src/shutdown.rs +++ b/src/shutdown.rs @@ -85,24 +85,20 @@ pub async fn handle_sighup_reload( reload_handle: &Arc, config_path: &str, ) { - let config_content = match tokio::fs::read_to_string(config_path).await { - Ok(content) => content, + let result = crate::config::read_and_validate_config( + config_path, + reload_handle.cli_allow_wildcard_bind(), + ) + .await; + + let (new_static, new_dynamic) = match result { + Ok(configs) => configs, Err(e) => { tracing::error!(event = "CONFIG_RELOAD", status = "error", error = %e); return; } }; - let full_config = match crate::config::FullConfig::parse(&config_content) { - Ok(c) => c, - Err(e) => { - tracing::error!(event = "CONFIG_RELOAD", status = "error", error = %e); - return; - } - }; - - let (new_static, new_dynamic) = full_config.into_static_and_dynamic(); - match reload_handle.reload(new_static, new_dynamic).await { Ok(changed_fields) => { if !changed_fields.is_empty() { @@ -170,6 +166,7 @@ mod tests { let reload_handle = Arc::new(crate::config::ConfigReloadHandle::new( config_arc.clone(), static_config, + false, )); let dir = tempfile::tempdir().unwrap(); @@ -227,6 +224,7 @@ upstream = "127.0.0.1:8080" let reload_handle = Arc::new(crate::config::ConfigReloadHandle::new( config_arc.clone(), static_config, + false, )); let dir = tempfile::tempdir().unwrap(); @@ -253,6 +251,7 @@ upstream = "127.0.0.1:8080" let reload_handle = Arc::new(crate::config::ConfigReloadHandle::new( config_arc.clone(), static_config, + false, )); handle_sighup_reload(&reload_handle, "/nonexistent/config.toml").await; diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 357d785..15edc79 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -865,6 +865,7 @@ async fn test_sighup_config_reload_valid_config() { let reload_handle = Arc::new(reverse_proxy::config::ConfigReloadHandle::new( config_arc.clone(), static_config, + false, )); let dir = tempfile::tempdir().unwrap(); @@ -892,8 +893,8 @@ https_port = 443 mode = "acme" acme_domains = ["test.local"] acme_cache_dir = "/tmp/acme-cache" -acme_contact = "mailto:admin@test.local" acme_directory = "staging" +acme_contact = "mailto:admin@test.local" [[listeners.sites]] host = "test.local" @@ -921,6 +922,7 @@ async fn test_sighup_config_reload_invalid_config_keeps_old() { let reload_handle = Arc::new(reverse_proxy::config::ConfigReloadHandle::new( config_arc.clone(), static_config, + false, )); let dir = tempfile::tempdir().unwrap();