Merge pull request #269 from benthecarman/codex/loupe-secret-files

Harden local secret file handling

Author: tankyleo

ldk-server-cli/src/main.rs

@@ -15,8 +15,8 @@ use clap_complete::{generate, Shell};
 use hex_conservative::{DisplayHex, FromHex};
 use ldk_server_client::client::LdkServerClient;
 use ldk_server_client::config::{
-	get_default_config_path, load_config, resolve_api_key, resolve_base_url, resolve_cert_path,
-	DEFAULT_GRPC_SERVICE_ADDRESS,
+	get_default_config_path, load_config, read_tls_certificate, resolve_api_key, resolve_base_url,
+	resolve_cert_path, DEFAULT_GRPC_SERVICE_ADDRESS,
 };
 use ldk_server_client::error::LdkServerError;
 use ldk_server_client::error::LdkServerErrorCode::{
@@ -655,10 +655,15 @@ async fn main() {
 		},
 	};
 
-	let api_key = resolve_api_key(cli.api_key, config.as_ref()).unwrap_or_else(|| {
-		eprintln!("API key not provided. Use --api-key or ensure the api_key file exists at {DEFAULT_DIR}/[network]/api_key");
-		std::process::exit(1);
-	});
+	let api_key = resolve_api_key(cli.api_key, config.as_ref())
+		.unwrap_or_else(|e| {
+			eprintln!("Failed to resolve API key: {e}");
+			std::process::exit(1);
+		})
+		.unwrap_or_else(|| {
+			eprintln!("API key not provided. Use --api-key or ensure the api_key file exists at {DEFAULT_DIR}/[network]/api_key");
+			std::process::exit(1);
+		});
 
 	let base_url = resolve_base_url(cli.base_url, config.as_ref());
 
@@ -668,8 +673,8 @@ async fn main() {
 			std::process::exit(1);
 		});
 
-	let server_cert_pem = std::fs::read(&tls_cert_path).unwrap_or_else(|e| {
-		eprintln!("Failed to read server certificate file '{}': {}", tls_cert_path.display(), e);
+	let server_cert_pem = read_tls_certificate(&tls_cert_path).unwrap_or_else(|e| {
+		eprintln!("{e}");
 		std::process::exit(1);
 	});
 

ldk-server-client/src/config.rs

@@ -13,14 +13,18 @@
 //! locating the server's TLS certificate and API key on disk, so multiple clients (CLI, MCP
 //! bridge, etc.) can resolve connection credentials in a consistent way.
 
-use std::path::PathBuf;
+use std::io::{self, ErrorKind, Read};
+use std::path::{Path, PathBuf};
 
 use hex_conservative::DisplayHex;
 use serde::{Deserialize, Serialize};
 
 const DEFAULT_CONFIG_FILE: &str = "config.toml";
 const DEFAULT_CERT_FILE: &str = "tls.crt";
 const API_KEY_FILE: &str = "api_key";
+const API_KEY_LEN: usize = 32;
+const CONFIG_FILE_SIZE_LIMIT: usize = 1024 * 1024;
+const TLS_CERT_FILE_SIZE_LIMIT: usize = 1024 * 1024;
 
 /// Default address of the `ldk-server` gRPC endpoint when no explicit value is configured.
 pub const DEFAULT_GRPC_SERVICE_ADDRESS: &str = "127.0.0.1:3536";
@@ -124,13 +128,21 @@ impl Config {
 }
 
 /// Reads and parses the `ldk-server` configuration file at `path`.
-pub fn load_config(path: &PathBuf) -> Result<Config, String> {
-	let contents = std::fs::read_to_string(path)
+pub fn load_config(path: &Path) -> Result<Config, String> {
+	let contents = read_to_string_with_limit(path, CONFIG_FILE_SIZE_LIMIT)
 		.map_err(|e| format!("Failed to read config file '{}': {}", path.display(), e))?;
 	toml::from_str(&contents)
 		.map_err(|e| format!("Failed to parse config file '{}': {}", path.display(), e))
 }
 
+/// Reads the server TLS certificate at `path`.
+///
+/// Returns an error if the file exceeds 1 MiB.
+pub fn read_tls_certificate(path: &Path) -> Result<Vec<u8>, String> {
+	read_with_limit(path, TLS_CERT_FILE_SIZE_LIMIT)
+		.map_err(|e| format!("Failed to read server certificate file '{}': {e}", path.display()))
+}
+
 /// Resolves the base URL of the `ldk-server` gRPC endpoint.
 ///
 /// Prefers `override_url`, falls back to the configuration file, and finally to
@@ -146,18 +158,65 @@ pub fn resolve_base_url(override_url: Option<String>, config: Option<&Config>) -
 /// Prefers `override_key`, falls back to reading the API key file from the configured storage
 /// directory, and finally from the OS-specific default data directory. The raw bytes read from
 /// disk are lower-hex encoded before being returned.
-pub fn resolve_api_key(override_key: Option<String>, config: Option<&Config>) -> Option<String> {
-	override_key.or_else(|| {
-		let network =
-			config.and_then(|c| c.network().ok()).unwrap_or_else(|| "bitcoin".to_string());
-		storage_dir(config)
-			.map(|dir| api_key_path_for_storage_dir(dir, &network))
-			.and_then(|path| std::fs::read(&path).ok())
-			.or_else(|| {
-				get_default_api_key_path(&network).and_then(|path| std::fs::read(&path).ok())
-			})
-			.map(|bytes| bytes.to_lower_hex_string())
-	})
+///
+/// Returns an error if a candidate API key file exists but cannot be read or does not contain
+/// exactly 32 bytes.
+pub fn resolve_api_key(
+	override_key: Option<String>, config: Option<&Config>,
+) -> Result<Option<String>, String> {
+	if override_key.is_some() {
+		return Ok(override_key);
+	}
+
+	let network = config.and_then(|c| c.network().ok()).unwrap_or_else(|| "bitcoin".to_string());
+	if let Some(dir) = storage_dir(config) {
+		let path = api_key_path_for_storage_dir(dir, &network);
+		if let Some(api_key) = read_api_key(&path)? {
+			return Ok(Some(api_key));
+		}
+	}
+
+	match get_default_api_key_path(&network) {
+		Some(path) => read_api_key(&path),
+		None => Ok(None),
+	}
+}
+
+fn read_api_key(path: &Path) -> Result<Option<String>, String> {
+	let file = match std::fs::File::open(path) {
+		Ok(file) => file,
+		Err(e) if e.kind() == ErrorKind::NotFound => return Ok(None),
+		Err(e) => return Err(format!("Failed to read API key file '{}': {e}", path.display())),
+	};
+	let mut bytes = Vec::with_capacity(API_KEY_LEN + 1);
+	file.take((API_KEY_LEN + 1) as u64)
+		.read_to_end(&mut bytes)
+		.map_err(|e| format!("Failed to read API key file '{}': {e}", path.display()))?;
+	if bytes.len() != API_KEY_LEN {
+		return Err(format!(
+			"API key file '{}' must contain exactly {API_KEY_LEN} bytes",
+			path.display()
+		));
+	}
+	Ok(Some(bytes.to_lower_hex_string()))
+}
+
+fn read_with_limit(path: &Path, limit: usize) -> io::Result<Vec<u8>> {
+	let file = std::fs::File::open(path)?;
+	let mut contents = Vec::new();
+	file.take(limit.saturating_add(1) as u64).read_to_end(&mut contents)?;
+	if contents.len() > limit {
+		return Err(io::Error::new(
+			io::ErrorKind::InvalidData,
+			format!("File '{}' exceeds the {limit} byte limit", path.display()),
+		));
+	}
+	Ok(contents)
+}
+
+fn read_to_string_with_limit(path: &Path, limit: usize) -> io::Result<String> {
+	String::from_utf8(read_with_limit(path, limit)?)
+		.map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))
 }
 
 /// Resolves the path to the server's TLS certificate (PEM).
@@ -187,7 +246,10 @@ fn default_grpc_service_address() -> String {
 
 #[cfg(test)]
 mod tests {
-	use super::{resolve_base_url, Config, DEFAULT_GRPC_SERVICE_ADDRESS};
+	use super::{
+		load_config, read_tls_certificate, resolve_base_url, Config, CONFIG_FILE_SIZE_LIMIT,
+		DEFAULT_GRPC_SERVICE_ADDRESS, TLS_CERT_FILE_SIZE_LIMIT,
+	};
 
 	#[test]
 	fn config_defaults_grpc_service_address() {
@@ -282,4 +344,28 @@ mod tests {
 	fn resolve_base_url_falls_back_to_default() {
 		assert_eq!(resolve_base_url(None, None), DEFAULT_GRPC_SERVICE_ADDRESS);
 	}
+
+	#[test]
+	fn read_tls_certificate_rejects_oversized_file() {
+		let path = std::env::temp_dir()
+			.join(format!("ldk-server-client-oversized-cert-{}", std::process::id()));
+		std::fs::write(&path, vec![0; TLS_CERT_FILE_SIZE_LIMIT + 1]).unwrap();
+
+		let error = read_tls_certificate(&path).unwrap_err();
+		assert!(error.contains("exceeds"));
+
+		std::fs::remove_file(path).unwrap();
+	}
+
+	#[test]
+	fn load_config_rejects_oversized_file() {
+		let path = std::env::temp_dir()
+			.join(format!("ldk-server-client-oversized-config-{}", std::process::id()));
+		std::fs::write(&path, vec![b'a'; CONFIG_FILE_SIZE_LIMIT + 1]).unwrap();
+
+		let error = load_config(&path).unwrap_err();
+		assert!(error.contains("exceeds"));
+
+		std::fs::remove_file(path).unwrap();
+	}
 }

ldk-server-mcp/src/config.rs

@@ -10,7 +10,8 @@
 use std::path::PathBuf;
 
 use ldk_server_client::config::{
-	get_default_config_path, load_config, resolve_api_key, resolve_base_url, resolve_cert_path,
+	get_default_config_path, load_config, read_tls_certificate, resolve_api_key, resolve_base_url,
+	resolve_cert_path,
 };
 
 pub struct ResolvedConfig {
@@ -39,7 +40,7 @@ pub fn resolve_config(config_path: Option<String>) -> Result<ResolvedConfig, Str
 
 	let base_url = resolve_base_url(env_base_url, config.as_ref());
 
-	let api_key = resolve_api_key(env_api_key, config.as_ref()).ok_or_else(
+	let api_key = resolve_api_key(env_api_key, config.as_ref())?.ok_or_else(
 		|| "API key not provided. Set LDK_API_KEY or ensure the api_key file exists at ~/.ldk-server/[network]/api_key".to_string()
 	)?;
 
@@ -48,9 +49,7 @@ pub fn resolve_config(config_path: Option<String>) -> Result<ResolvedConfig, Str
 			.to_string()
 	})?;
 
-	let tls_cert_pem = std::fs::read(&tls_cert_path).map_err(|e| {
-		format!("Failed to read server certificate file '{}': {}", tls_cert_path.display(), e)
-	})?;
+	let tls_cert_pem = read_tls_certificate(&tls_cert_path)?;
 
 	Ok(ResolvedConfig { base_url, api_key, tls_cert_pem })
 }
@@ -203,7 +202,7 @@ mod tests {
 
 		let cert_path = custom_storage.join("tls.crt");
 		std::fs::write(&cert_path, b"storage-cert").unwrap();
-		std::fs::write(custom_storage.join("regtest").join("api_key"), [0xAB, 0xCD]).unwrap();
+		std::fs::write(custom_storage.join("regtest").join("api_key"), [0xAB; 32]).unwrap();
 
 		std::fs::write(
 			&config_path,
@@ -226,7 +225,7 @@ mod tests {
 		let resolved = resolve_config(Some(config_path.display().to_string())).unwrap();
 
 		assert_eq!(resolved.base_url, DEFAULT_GRPC_SERVICE_ADDRESS);
-		assert_eq!(resolved.api_key, "abcd");
+		assert_eq!(resolved.api_key, "ab".repeat(32));
 		assert_eq!(resolved.tls_cert_pem, b"storage-cert");
 
 		std::fs::remove_dir_all(temp_dir).unwrap();

ldk-server/src/io/persist/sqlite_store/mod.rs

@@ -7,15 +7,18 @@
 // You may not use this file except in accordance with one or both of these
 // licenses.
 
+use std::fs::OpenOptions;
+use std::io;
+use std::os::unix::fs::OpenOptionsExt;
 use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
-use std::{fs, io};
 
 use ldk_node::lightning::types::string::PrintableString;
 use rusqlite::{named_params, Connection};
 
 use crate::io::persist::paginated_kv_store::{ListResponse, PaginatedKVStore};
 use crate::io::utils::check_namespace_key_validity;
+use crate::util::create_dir_all_private;
 
 /// The default database file name.
 pub const DEFAULT_SQLITE_DB_FILE_NAME: &str = "ldk_server_data.sqlite";
@@ -48,7 +51,7 @@ impl SqliteStore {
 		let paginated_kv_table_name =
 			paginated_kv_table_name.unwrap_or(DEFAULT_PAGINATED_KV_TABLE_NAME.to_string());
 
-		fs::create_dir_all(data_dir.clone()).map_err(|e| {
+		create_dir_all_private(&data_dir).map_err(|e| {
 			let msg = format!(
 				"Failed to create database destination directory {}: {}",
 				data_dir.display(),
@@ -58,6 +61,15 @@ impl SqliteStore {
 		})?;
 		let mut db_file_path = data_dir;
 		db_file_path.push(db_file_name);
+		match OpenOptions::new().create_new(true).write(true).mode(0o600).open(&db_file_path) {
+			Ok(_) => {},
+			Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {},
+			Err(e) => {
+				let msg =
+					format!("Failed to create database file {}: {}", db_file_path.display(), e);
+				return Err(io::Error::other(msg));
+			},
+		}
 
 		let connection = Connection::open(db_file_path.clone()).map_err(|e| {
 			let msg =

ldk-server/src/main.rs

@@ -14,6 +14,7 @@ mod util;
 
 use std::collections::HashSet;
 use std::fs;
+use std::io::Read;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 use std::time::{Duration, SystemTime, UNIX_EPOCH};
@@ -54,9 +55,10 @@ use crate::util::logger::{LogConfig, ServerLogger};
 use crate::util::metrics::Metrics;
 use crate::util::proto_adapter::{forwarded_payment_to_proto, payment_to_proto};
 use crate::util::tls::get_or_generate_tls_config;
-use crate::util::{systemd, write_new};
+use crate::util::{create_dir_all_private, systemd, write_new};
 
 const API_KEY_FILE: &str = "api_key";
+const API_KEY_LEN: usize = 32;
 const FULL_VERSION: &str = concat!(env!("CARGO_PKG_VERSION"), " (", env!("GIT_HASH"), ")");
 
 pub fn get_default_data_dir() -> Option<PathBuf> {
@@ -969,15 +971,31 @@ fn upsert_payment_details(
 fn load_or_generate_api_key(storage_dir: &Path) -> std::io::Result<String> {
 	let api_key_path = storage_dir.join(API_KEY_FILE);
 
-	if api_key_path.exists() {
-		let key_bytes = fs::read(&api_key_path)?;
+	let file = match fs::File::open(&api_key_path) {
+		Ok(file) => Some(file),
+		Err(e) if e.kind() == std::io::ErrorKind::NotFound => None,
+		Err(e) => return Err(e),
+	};
+
+	if let Some(file) = file {
+		let mut key_bytes = Vec::with_capacity(API_KEY_LEN + 1);
+		file.take((API_KEY_LEN + 1) as u64).read_to_end(&mut key_bytes)?;
+		if key_bytes.len() != API_KEY_LEN {
+			return Err(std::io::Error::new(
+				std::io::ErrorKind::InvalidData,
+				format!(
+					"API key file '{}' must contain exactly {API_KEY_LEN} bytes",
+					api_key_path.display()
+				),
+			));
+		}
 		Ok(key_bytes.to_lower_hex_string())
 	} else {
 		// Ensure the storage directory exists
-		fs::create_dir_all(storage_dir)?;
+		create_dir_all_private(storage_dir)?;
 
 		// Generate a 32-byte random API key
-		let mut key_bytes = [0u8; 32];
+		let mut key_bytes = [0u8; API_KEY_LEN];
 		getrandom::getrandom(&mut key_bytes).map_err(std::io::Error::other)?;
 
 		write_new(&api_key_path, &key_bytes, 0o400)?;
@@ -1005,6 +1023,23 @@ mod tests {
 
 	use super::*;
 
+	#[test]
+	fn load_api_key_rejects_invalid_lengths() {
+		let nonce = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos();
+		let dir = std::env::temp_dir()
+			.join(format!("ldk-server-api-key-length-{}-{nonce}", std::process::id()));
+		fs::create_dir_all(&dir).unwrap();
+		let path = dir.join(API_KEY_FILE);
+
+		for len in [0, 1, API_KEY_LEN - 1, API_KEY_LEN + 1] {
+			fs::write(&path, vec![0x42; len]).unwrap();
+			let error = load_or_generate_api_key(&dir).unwrap_err();
+			assert_eq!(error.kind(), std::io::ErrorKind::InvalidData);
+		}
+
+		fs::remove_dir_all(dir).unwrap();
+	}
+
 	#[test]
 	fn test_is_channel_open_failure_classification() {
 		assert!(is_channel_open_failure(Some(&ClosureReason::FundingTimedOut)));