diff --git a/Cargo.lock b/Cargo.lock index 2f2aa860e9..09fd31b5d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5679,6 +5679,7 @@ dependencies = [ "toml 0.8.23", "utils", "utils-networking", + "zeroize", ] [[package]] diff --git a/api-server/api-server-common/src/lib.rs b/api-server/api-server-common/src/lib.rs index b0ce699678..3fc09296b5 100644 --- a/api-server/api-server-common/src/lib.rs +++ b/api-server/api-server-common/src/lib.rs @@ -19,6 +19,219 @@ pub mod streaming; use clap::Parser; use common::chain::config::ChainType; +/// Marker printed in place of any secret-bearing value. +pub const REDACTED: &str = ""; + +/// Marker printed in place of a secret-bearing value that is not set. +pub const UNSET_SENTINEL: &str = ""; + +/// Debug-safe replacement for an `Option` that may hold a secret. +pub fn redact(value: &Option) -> Option<&'static str> { + value.as_ref().map(|_| REDACTED) +} + +/// A loggable summary of the options, omitting every value that can hold a secret. +/// Implementors must consciously provide the summary; for types whose hand-written Debug +/// impl redacts the secret-bearing fields (with an exhaustive destructure guarding new +/// fields), the body is the Debug output itself. +pub trait RedactedSummary: std::fmt::Debug { + fn redacted_summary(&self) -> String; +} + +/// Returns a value-free ` for ` suffix naming the rejected argument, or an +/// empty string when the error carries no invalid-argument context. The rejected +/// value lives in a separate clap context and is never included in the result. +fn offending_arg_suffix(err: &clap::Error) -> String { + match err.get(clap::error::ContextKind::InvalidArg) { + Some(clap::error::ContextValue::String(name)) if name.starts_with('-') => { + // Only flag-shaped ids are echoed: for `UnknownArgument` raised on an + // unexpected positional token, clap places the raw token itself (the + // potential secret) into this same invalid-argument context. + format!(" for {name}") + } + _ => String::new(), + } +} + +/// Parses command line arguments, and on failure exits without echoing them: clap errors +/// for value-bearing arguments can repeat raw `--flag=value` tokens, which may carry +/// secrets. Only errors whose clap rendering is guaranteed to name flags or static text +/// (ArgumentConflict, MissingRequiredArgument, MissingSubcommand, NoEquals, plus help +/// and version) are printed as-is; every other error, including UnknownArgument, is +/// replaced by a generic message. +/// +/// # Contract with the argument definitions +/// +/// Help and version output is echoed verbatim, and the error fallback tells users to +/// run `--help`. Any secret-bearing argument bound to an environment variable must +/// therefore set `hide_env_values(true)` (see +/// `clap_utils::env_adder_hiding_env_values`), otherwise clap renders +/// `[env: NAME=value]` into the help text and this function prints the secret. +pub fn parse_args_or_exit() -> T { + match T::try_parse() { + Ok(args) => args, + Err(err) => { + if err.use_stderr() { + if error_is_value_free(err.kind()) { + let _ = err.print(); + } else { + // The error kind's rendering is a static category name and the + // offending flag's id is value-free, so both are safe to include; + // the value context (e.g. the rejected input) never is. + let offending_arg = offending_arg_suffix(&err); + eprintln!( + "Invalid command line arguments ({:?}){offending_arg}; run with --help for usage", + err.kind() + ); + } + std::process::exit(2); + } + let _ = err.print(); + std::process::exit(0); + } + } +} + +/// Whether the clap rendering of this error kind is guaranteed to name flags or static +/// text only, never raw argument values. +fn error_is_value_free(kind: clap::error::ErrorKind) -> bool { + use clap::error::ErrorKind; + // `DisplayHelpOnMissingArgumentOrSubcommand` is deliberately excluded even though its + // rendering is stderr-routed and value-free under the `hide_env_values` contract: a + // binary configured with `arg_required_else_help` should print the generic message + // rather than its full help when invoked with no arguments. + matches!( + kind, + ErrorKind::ArgumentConflict + | ErrorKind::MissingRequiredArgument + | ErrorKind::MissingSubcommand + | ErrorKind::NoEquals + ) +} + +#[cfg(test)] +mod parse_args_tests { + use super::offending_arg_suffix; + use clap::error::ErrorKind; + + #[test] + fn value_bearing_error_kinds_are_suppressed() { + assert!(!super::error_is_value_free(ErrorKind::InvalidValue)); + assert!(!super::error_is_value_free(ErrorKind::ValueValidation)); + assert!(!super::error_is_value_free(ErrorKind::UnknownArgument)); + } + + #[test] + fn flag_naming_error_kinds_are_printed() { + assert!(super::error_is_value_free( + ErrorKind::MissingRequiredArgument + )); + assert!(super::error_is_value_free(ErrorKind::ArgumentConflict)); + assert!(super::error_is_value_free(ErrorKind::MissingSubcommand)); + assert!(super::error_is_value_free(ErrorKind::NoEquals)); + } + + /// Pins that the clap rendering of the allowlisted kinds never embeds raw argument + /// values, so a clap upgrade that starts echoing them fails here instead of leaking + /// secrets in production. + #[test] + fn allowlisted_kinds_never_render_raw_values() { + let secret = "s3cret-canary-value"; + let cmd = clap::Command::new("canary") + .arg(clap::Arg::new("a").long("a").num_args(1)) + .arg( + clap::Arg::new("b") + .long("b") + .conflicts_with("a") + .action(clap::ArgAction::SetTrue), + ) + .arg(clap::Arg::new("c").long("c").required(true)); + + let conflict = + cmd.clone().try_get_matches_from(["canary", "--a", secret, "--b"]).unwrap_err(); + assert_eq!(conflict.kind(), clap::error::ErrorKind::ArgumentConflict); + assert!(!conflict.render().to_string().contains(secret)); + + let missing_required = + cmd.clone().try_get_matches_from(["canary", "--a", secret]).unwrap_err(); + assert_eq!( + missing_required.kind(), + clap::error::ErrorKind::MissingRequiredArgument + ); + assert!(!missing_required.render().to_string().contains(secret)); + + let requires_equals = clap::Command::new("canary") + .arg(clap::Arg::new("a").long("a").require_equals(true).num_args(1)); + let no_equals = + requires_equals.try_get_matches_from(["canary", "--a", secret]).unwrap_err(); + assert_eq!(no_equals.kind(), clap::error::ErrorKind::NoEquals); + assert!(!no_equals.render().to_string().contains(secret)); + + let subcommanded = clap::Command::new("canary") + .subcommand_required(true) + .subcommand(clap::Command::new("run")); + let missing_subcommand = subcommanded.try_get_matches_from(["canary"]).unwrap_err(); + assert_eq!( + missing_subcommand.kind(), + clap::error::ErrorKind::MissingSubcommand + ); + assert!(!missing_subcommand.render().to_string().contains(secret)); + } + + #[test] + fn offending_arg_suffix_names_the_flag_but_never_the_value() { + let cmd = clap::Command::new("canary").arg( + clap::Arg::new("postgres-port") + .long("postgres-port") + .value_parser(clap::value_parser!(usize)), + ); + let err = cmd + .try_get_matches_from(["canary", "--postgres-port", "99999999999999999999"]) + .unwrap_err(); + assert_eq!(err.kind(), clap::error::ErrorKind::ValueValidation); + let suffix = offending_arg_suffix(&err); + assert!(suffix.contains("postgres-port"), "{suffix}"); + assert!(!suffix.contains("99999999999999999999"), "{suffix}"); + } + + #[test] + fn no_echo_kinds_never_carry_a_value_in_the_invalid_arg_context() { + let secret = "s3cret-canary-value"; + // `InvalidValue` and `UnknownArgument` reach the generic no-echo message; + // neither may smuggle the rejected value through the invalid-argument context. + let cmd = clap::Command::new("canary") + .arg( + clap::Arg::new("rpc-password") + .long("rpc-password") + .value_parser(clap::value_parser!(u16)), + ) + .arg(clap::Arg::new("mode").long("mode").value_parser(["fast", "slow"])); + + let invalid_value = + cmd.clone().try_get_matches_from(["canary", "--mode", secret]).unwrap_err(); + assert_eq!(invalid_value.kind(), clap::error::ErrorKind::InvalidValue); + let suffix = offending_arg_suffix(&invalid_value); + assert!(suffix.contains("mode"), "{suffix}"); + assert!(!suffix.contains(secret), "{suffix}"); + + // Glue the value to the flag in one token: this is the form whose parse path + // could plausibly copy the raw token into the invalid-arg context. + let unknown = cmd + .clone() + .try_get_matches_from(["canary".to_owned(), format!("--unknown={secret}")]) + .unwrap_err(); + assert_eq!(unknown.kind(), clap::error::ErrorKind::UnknownArgument); + let suffix = offending_arg_suffix(&unknown); + assert!(!suffix.contains(secret), "{suffix}"); + + // An unexpected bare positional token is also echoed into the invalid-arg + // context by clap; the suffix must stay empty there. + let bare = cmd.try_get_matches_from(["canary".to_owned(), secret.to_owned()]).unwrap_err(); + assert_eq!(bare.kind(), clap::error::ErrorKind::UnknownArgument); + assert_eq!(offending_arg_suffix(&bare), ""); + } +} + #[derive(clap::ValueEnum, Clone, Debug)] pub enum Network { Mainnet, @@ -38,18 +251,26 @@ impl From for ChainType { } } -#[derive(Parser, Debug)] +const DEFAULT_POSTGRES_HOST: &str = "localhost"; +const DEFAULT_POSTGRES_PORT: u16 = 5432; +const DEFAULT_POSTGRES_USER: &str = "postgres"; +const DEFAULT_POSTGRES_MAX_CONNECTIONS: u32 = 10; + +// When adding a field, keep the Debug destructure, the `Default` impl, and the +// `default_matches_clap_defaults` test in sync, and classify the new option in the +// help-output guard tests of both daemons. +#[derive(Parser, PartialEq)] pub struct PostgresConfig { /// Postgres host - #[clap(long, default_value = "localhost")] + #[clap(long, default_value = DEFAULT_POSTGRES_HOST)] pub postgres_host: String, /// Postgres port - #[clap(long, default_value = "5432")] + #[clap(long, default_value_t = DEFAULT_POSTGRES_PORT)] pub postgres_port: u16, /// Postgres user - #[clap(long, default_value = "postgres")] + #[clap(long, default_value = DEFAULT_POSTGRES_USER)] pub postgres_user: String, /// Postgres password @@ -61,12 +282,74 @@ pub struct PostgresConfig { pub postgres_database: Option, /// Postgres max connections - #[clap(long, default_value = "10")] + #[clap(long, default_value_t = DEFAULT_POSTGRES_MAX_CONNECTIONS)] pub postgres_max_connections: u32, } +// The password must never appear in debug output; anything that embeds this struct in a +// derived Debug (command line option dumps, error types) is safe by construction. The +// destructure has no `..` so a newly added field fails to compile until it is either added +// here or shown to be safe. The database role name in `postgres_user` is not a credential +// and is shown in clear text, unlike the RPC username, which is part of the basic-auth +// credential pair. +impl std::fmt::Debug for PostgresConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let Self { + postgres_host, + postgres_port, + postgres_user, + postgres_password, + postgres_database, + postgres_max_connections, + } = self; + f.debug_struct("PostgresConfig") + .field("postgres_host", postgres_host) + .field("postgres_port", postgres_port) + .field("postgres_user", postgres_user) + .field("postgres_password", &redact(postgres_password)) + .field("postgres_database", postgres_database) + .field("postgres_max_connections", postgres_max_connections) + .finish() + } +} + impl Default for PostgresConfig { fn default() -> Self { - Self::parse() + // Shared constants, not parsing: parsing here would read the process's real + // arguments and is not safe outside of the actual CLI startup. + Self { + postgres_host: DEFAULT_POSTGRES_HOST.to_string(), + postgres_port: DEFAULT_POSTGRES_PORT, + postgres_user: DEFAULT_POSTGRES_USER.to_string(), + postgres_password: None, + postgres_database: None, + postgres_max_connections: DEFAULT_POSTGRES_MAX_CONNECTIONS, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn default_matches_clap_defaults() { + let from_clap = PostgresConfig::parse_from(["postgres-config"]); + assert_eq!(from_clap, PostgresConfig::default()); + } + + #[test] + fn debug_output_redacts_postgres_password() { + let config = PostgresConfig::parse_from([ + "postgres-config", + "--postgres-host", + "dbhost", + "--postgres-password", + "dbsecret", + ]); + let dump = format!("{config:?}"); + assert!(dump.contains("postgres_host: \"dbhost\"")); + assert!(dump.contains("postgres_password: Some(\"\")")); + assert!(!dump.contains("dbsecret")); } } diff --git a/api-server/api-server-common/src/storage/impls/postgres/mod.rs b/api-server/api-server-common/src/storage/impls/postgres/mod.rs index 5b15fa77e7..961ce8d1c8 100644 --- a/api-server/api-server-common/src/storage/impls/postgres/mod.rs +++ b/api-server/api-server-common/src/storage/impls/postgres/mod.rs @@ -18,7 +18,6 @@ pub mod transactional; mod queries; -use std::str::FromStr; use std::sync::Arc; use bb8_postgres::PostgresConnectionManager; @@ -64,27 +63,25 @@ impl TransactionalApiServerPostgresStorage { max_connections: u32, chain_config: Arc, ) -> Result { - let password_part = match password { - Some(p) => format!("password={}", p), - None => "".to_string(), + // The configuration is built from the discrete fields instead of a connection + // string, so the password never exists as a string that a parse error could echo. + let database = match database { + Some(d) => d.to_owned(), + None => format!("mintlayer-{}", chain_config.chain_type().name()), }; - - let database_part = match database { - Some(d) => format!("dbname={}", d), - None => format!("dbname=mintlayer-{}", chain_config.chain_type().name()), + let password_part = match password { + Some(_) => crate::REDACTED, + None => crate::UNSET_SENTINEL, }; - - let config_str = - format!("host={host} port={port} user={user} {password_part} {database_part}"); - logging::log::debug!("Using postgres config connection string: {}", config_str); - - let config: tokio_postgres::Config = - config_str.parse().map_err(|e: ::Err| { - ApiServerStorageError::InitializationError(format!( - "Postgres configuration parsing error: {}", - e - )) - })?; + logging::log::debug!( + "Using postgres config: host={host} port={port} user={user} password={password_part} database={database}" + ); + + let mut config = tokio_postgres::Config::new(); + config.host(host).port(port).user(user).dbname(database); + if let Some(password) = password { + config.password(password); + } let manager = PostgresConnectionManager::new(config.clone(), NoTls); let pool = Pool::builder().max_size(max_connections).build(manager).await.map_err(|e| { diff --git a/api-server/scanner-daemon/Cargo.toml b/api-server/scanner-daemon/Cargo.toml index d3641056a7..0ab0f819ec 100644 --- a/api-server/scanner-daemon/Cargo.toml +++ b/api-server/scanner-daemon/Cargo.toml @@ -24,6 +24,7 @@ tokio = { workspace = true, features = ["full"] } [dev-dependencies] test-utils = { path = "../../test-utils" } +utils = { path = "../../utils", features = ["test-util"] } [build-dependencies] build_utils = { path = "../../utils/build_utils" } diff --git a/api-server/scanner-daemon/src/config.rs b/api-server/scanner-daemon/src/config.rs index f112f35b65..551f929c91 100644 --- a/api-server/scanner-daemon/src/config.rs +++ b/api-server/scanner-daemon/src/config.rs @@ -15,12 +15,24 @@ use clap::Parser; -use api_server_common::{Network, PostgresConfig}; +use api_server_common::{Network, PostgresConfig, RedactedSummary, redact}; use utils::{app_version_with_git_info, clap_utils}; use utils_networking::NetworkAddressWithPort; -#[derive(Parser, Debug)] -#[clap(mut_args(clap_utils::env_adder("API_SCANNER_DAEMON")))] +/// Options whose values can hold a secret; their environment-sourced values are hidden +/// from the help output. +// NOTE: when adding a secret-bearing field to this struct, update BOTH the Debug impl +// below (the exhaustive destructure enforces this) AND this list (nothing enforces it; +// the classification check in the guard test rejects omissions: any new option left +// out of both SECRET_OPTIONS and NON_SECRET_OPTIONS fails the test, though it cannot +// tell which list an option belongs in). A secret-bearing field +// added to the flattened `PostgresConfig` escapes the destructure here: it is guarded +// by `PostgresConfig`'s own Debug impl in api-server-common, but it must still be +// added to this list. +const SECRET_OPTIONS: &[&str] = &["node-rpc-username", "node-rpc-password", "postgres-password"]; + +#[derive(Parser)] +#[clap(mut_args(clap_utils::env_adder_hiding_env_values("API_SCANNER_DAEMON", SECRET_OPTIONS)))] #[clap(version(app_version_with_git_info!().to_pretty_string()))] pub struct ApiServerScannerArgs { /// Network @@ -47,3 +59,127 @@ pub struct ApiServerScannerArgs { #[clap(flatten)] pub postgres_config: PostgresConfig, } + +// RPC credentials and the database password must never appear in debug output; a debug +// dump of the whole struct is therefore safe by construction. +impl std::fmt::Debug for ApiServerScannerArgs { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Destructure without `..` so a newly added field fails to compile until it is either + // added here or shown to be safe. + let Self { + network, + node_rpc_address, + node_rpc_cookie_file, + node_rpc_username, + node_rpc_password, + postgres_config, + } = self; + f.debug_struct("ApiServerScannerArgs") + .field("network", network) + .field("node_rpc_address", node_rpc_address) + .field("node_rpc_cookie_file", node_rpc_cookie_file) + .field("node_rpc_username", &redact(node_rpc_username)) + .field("node_rpc_password", &redact(node_rpc_password)) + .field("postgres_config", postgres_config) + .finish() + } +} + +// The hand-written Debug impl redacts the secret-bearing fields and guards new fields +// with an exhaustive destructure, so the summary is the Debug output itself. +impl RedactedSummary for ApiServerScannerArgs { + fn redacted_summary(&self) -> String { + format!("{self:?}") + } +} + +#[cfg(test)] +mod tests { + use super::*; + use api_server_common::REDACTED; + use clap::CommandFactory; + + #[test] + fn secret_options_are_hidden_from_help() { + let cmd = ApiServerScannerArgs::command(); + // Options explicitly classified as non-secret; anything else must be listed in + // SECRET_OPTIONS or the classification check below fails. + const NON_SECRET_OPTIONS: &[&str] = &[ + "network", + "node-rpc-address", + "node-rpc-cookie-file", + "postgres-host", + "postgres-port", + "postgres-user", + "postgres-database", + "postgres-max-connections", + ]; + // Every option must be classified as secret or non-secret; the helper includes + // the secret pin above and rejects stale non-secret entries. + clap_utils::assert_options_classified(&cmd, SECRET_OPTIONS, NON_SECRET_OPTIONS); + } + + #[test] + fn redacted_summary_omits_secrets() { + // Built directly so no ML_API_SCANNER_DAEMON_* environment fallback applies. + let args = ApiServerScannerArgs { + network: Network::Mainnet, + node_rpc_address: Some("127.0.0.1:12345".parse().unwrap()), + node_rpc_cookie_file: Some("/tmp/rpc-cookie-file".to_string()), + node_rpc_username: Some("alice".to_string()), + node_rpc_password: Some("supersecret".to_string()), + postgres_config: PostgresConfig { + postgres_host: "dbhost".to_string(), + postgres_port: 5432, + postgres_user: "dbuser".to_string(), + postgres_password: Some("dbsecret".to_string()), + postgres_database: None, + postgres_max_connections: 10, + }, + }; + let summary = args.redacted_summary(); + // Assertions are value-based rather than pinned to clap's Debug rendering. + // NetworkAddressWithPort renders the address and port as separate Debug fields, + // so they are asserted separately. + assert!(summary.contains("node_rpc_address")); + assert!(summary.contains("127.0.0.1")); + assert!(summary.contains("port: 12345")); + assert!(summary.contains(REDACTED)); + assert!(summary.contains("node_rpc_cookie_file")); + assert!(summary.contains("/tmp/rpc-cookie-file")); + assert!(!summary.contains("alice")); + assert!(!summary.contains("supersecret")); + assert!(!summary.contains("dbsecret")); + let debug_dump = format!("{args:?}"); + assert!(debug_dump.contains("ApiServerScannerArgs")); + assert!(debug_dump.contains(REDACTED)); + assert!(!debug_dump.contains("alice")); + assert!(!debug_dump.contains("supersecret")); + assert!(!debug_dump.contains("dbsecret")); + } + + #[test] + fn redacted_summary_renders_absent_secrets_as_none() { + // Built directly so no ML_API_SCANNER_DAEMON_* environment fallback applies. + let args = ApiServerScannerArgs { + network: Network::Mainnet, + node_rpc_address: None, + node_rpc_cookie_file: None, + node_rpc_username: None, + node_rpc_password: None, + postgres_config: PostgresConfig { + postgres_host: "dbhost".to_string(), + postgres_port: 5432, + postgres_user: "dbuser".to_string(), + postgres_password: None, + postgres_database: None, + postgres_max_connections: 10, + }, + }; + let summary = args.redacted_summary(); + assert!(!summary.contains(REDACTED)); + assert!(summary.contains("node_rpc_username")); + assert!(summary.contains("node_rpc_password")); + assert!(summary.contains("postgres_password")); + } +} diff --git a/api-server/scanner-daemon/src/main.rs b/api-server/scanner-daemon/src/main.rs index 7480bb54f3..45e7e3d829 100644 --- a/api-server/scanner-daemon/src/main.rs +++ b/api-server/scanner-daemon/src/main.rs @@ -16,7 +16,7 @@ use std::sync::Arc; use api_blockchain_scanner_daemon::{ApiServerScannerError, make_postgres_storage, run}; -use clap::Parser; +use api_server_common::RedactedSummary; use common::chain::config::ChainType; use config::ApiServerScannerArgs; use rpc::RpcAuthData; @@ -28,10 +28,16 @@ utils::enable_rust_backtrace!(); #[tokio::main] async fn main() -> Result<(), ApiServerScannerError> { - let args = ApiServerScannerArgs::parse(); + // Parse errors are not echoed because they can repeat secret-bearing command line tokens. + let args = api_server_common::parse_args_or_exit::(); logging::init_logging(); - logging::log::info!("Command line options: {args:?}"); + // The command line contains secrets (RPC credentials, database password), so the summary + // logged here omits them; see ApiServerScannerArgs::redacted_summary. + logging::log::info!( + "Command line options (secrets omitted): {}", + args.redacted_summary(), + ); let ApiServerScannerArgs { network, diff --git a/api-server/web-server/Cargo.toml b/api-server/web-server/Cargo.toml index a85649a59c..d22556616f 100644 --- a/api-server/web-server/Cargo.toml +++ b/api-server/web-server/Cargo.toml @@ -37,6 +37,7 @@ randomness = { path = "../../randomness" } rstest.workspace = true strum.workspace = true test-utils = { path = "../../test-utils" } +utils = { path = "../../utils", features = ["test-util"] } [build-dependencies] build_utils = { path = "../../utils/build_utils" } diff --git a/api-server/web-server/src/config.rs b/api-server/web-server/src/config.rs index 0a94a8f63e..8073901c1e 100644 --- a/api-server/web-server/src/config.rs +++ b/api-server/web-server/src/config.rs @@ -17,7 +17,7 @@ use std::{net::SocketAddr, ops::Deref}; use clap::Parser; -use api_server_common::{Network, PostgresConfig}; +use api_server_common::{Network, PostgresConfig, RedactedSummary, redact}; use tokio::net::TcpListener; use utils::{app_version_with_git_info, clap_utils}; use utils_networking::NetworkAddressWithPort; @@ -29,8 +29,20 @@ use api_server_common::streaming; const LISTEN_ADDRESS: &str = "127.0.0.1:3000"; -#[derive(Debug, Parser)] -#[clap(mut_args(clap_utils::env_adder("API_WEB_SRV")))] +/// Options whose values can hold a secret; their environment-sourced values are hidden +/// from the help output (see the `mut_args` attribute and the guard test below). +// NOTE: when adding a secret-bearing field to this struct, update BOTH the Debug impl +// below (the exhaustive destructure enforces this) AND this list (nothing enforces it; +// the classification check in the guard test rejects omissions: any new option left +// out of both SECRET_OPTIONS and NON_SECRET_OPTIONS fails the test, though it cannot +// tell which list an option belongs in). A secret-bearing field +// added to the flattened `PostgresConfig` escapes the destructure here: it is guarded +// by `PostgresConfig`'s own Debug impl in api-server-common, but it must still be +// added to this list. +const SECRET_OPTIONS: &[&str] = &["node-rpc-username", "node-rpc-password", "postgres-password"]; + +#[derive(Parser)] +#[clap(mut_args(clap_utils::env_adder_hiding_env_values("API_WEB_SRV", SECRET_OPTIONS)))] #[clap(version(app_version_with_git_info!().to_pretty_string()))] pub struct ApiServerWebServerConfig { /// Network @@ -113,6 +125,178 @@ pub struct ApiServerWebServerConfig { pub stream_events_keepalive_interval_secs: u64, } +// RPC credentials and the database password must never appear in debug output; a debug +// dump of the whole struct is therefore safe by construction. +impl std::fmt::Debug for ApiServerWebServerConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Destructure without `..` so a newly added field fails to compile until it is either + // added here or shown to be safe. + let Self { + network, + bind_address, + postgres_config, + enable_post_routes, + node_rpc_address, + node_rpc_cookie_file, + node_rpc_username, + node_rpc_password, + stream_events_broadcast_capacity, + stream_events_max_subscribers, + stream_events_poll_interval_secs, + stream_events_keepalive_interval_secs, + } = self; + f.debug_struct("ApiServerWebServerConfig") + .field("network", network) + .field("bind_address", bind_address) + .field("postgres_config", postgres_config) + .field("enable_post_routes", enable_post_routes) + .field("node_rpc_address", node_rpc_address) + .field("node_rpc_cookie_file", node_rpc_cookie_file) + .field("node_rpc_username", &redact(node_rpc_username)) + .field("node_rpc_password", &redact(node_rpc_password)) + .field( + "stream_events_broadcast_capacity", + stream_events_broadcast_capacity, + ) + .field( + "stream_events_max_subscribers", + stream_events_max_subscribers, + ) + .field( + "stream_events_poll_interval_secs", + stream_events_poll_interval_secs, + ) + .field( + "stream_events_keepalive_interval_secs", + stream_events_keepalive_interval_secs, + ) + .finish() + } +} + +// The hand-written Debug impl redacts the secret-bearing fields and guards new fields +// with an exhaustive destructure, so the summary is the Debug output itself. +impl RedactedSummary for ApiServerWebServerConfig { + fn redacted_summary(&self) -> String { + format!("{self:?}") + } +} + +#[cfg(test)] +mod tests { + use super::*; + use api_server_common::REDACTED; + use clap::CommandFactory; + + #[test] + fn secret_options_are_hidden_from_help() { + let cmd = ApiServerWebServerConfig::command(); + // Options explicitly classified as non-secret; anything else must be listed in + // SECRET_OPTIONS or the classification check below fails. + const NON_SECRET_OPTIONS: &[&str] = &[ + "network", + "bind-address", + "enable-post-routes", + "node-rpc-address", + "node-rpc-cookie-file", + "postgres-host", + "postgres-port", + "postgres-user", + "postgres-database", + "postgres-max-connections", + "stream-events-broadcast-capacity", + "stream-events-max-subscribers", + "stream-events-poll-interval-secs", + "stream-events-keepalive-interval-secs", + ]; + // Every option must be classified as secret or non-secret; the helper includes + // the secret pin above and rejects stale non-secret entries. + clap_utils::assert_options_classified(&cmd, SECRET_OPTIONS, NON_SECRET_OPTIONS); + } + + #[test] + fn redacted_summary_omits_secrets() { + // Built directly so no ML_API_WEB_SRV_* environment fallback applies. + let args = ApiServerWebServerConfig { + network: Network::Mainnet, + bind_address: Some(ListenAddress { + socket: "127.0.0.1:3000".parse().unwrap(), + }), + postgres_config: PostgresConfig { + postgres_host: "dbhost".to_string(), + postgres_port: 5432, + postgres_user: "dbuser".to_string(), + postgres_password: Some("dbsecret".to_string()), + postgres_database: None, + postgres_max_connections: 10, + }, + enable_post_routes: true, + node_rpc_address: Some("127.0.0.1:12345".parse().unwrap()), + node_rpc_cookie_file: None, + node_rpc_username: Some("alice".to_string()), + node_rpc_password: Some("supersecret".to_string()), + stream_events_broadcast_capacity: streaming::DEFAULT_STREAM_EVENTS_BROADCAST_CAPACITY, + stream_events_max_subscribers: streaming::DEFAULT_STREAM_EVENTS_MAX_SUBSCRIBERS, + stream_events_poll_interval_secs: streaming::DEFAULT_STREAM_EVENTS_POLL_INTERVAL + .as_secs(), + stream_events_keepalive_interval_secs: + streaming::DEFAULT_STREAM_EVENTS_KEEPALIVE_INTERVAL.as_secs(), + }; + let summary = args.redacted_summary(); + // Assertions are value-based rather than pinned to clap's Debug rendering. + assert!(summary.contains("127.0.0.1:3000")); + assert!(summary.contains("enable_post_routes")); + // NetworkAddressWithPort renders the address and port as separate Debug fields, + // so they are asserted separately. + assert!(summary.contains("node_rpc_address")); + assert!(summary.contains("127.0.0.1")); + assert!(summary.contains("port: 12345")); + assert!(summary.contains(REDACTED)); + assert!(!summary.contains("alice")); + assert!(!summary.contains("supersecret")); + assert!(!summary.contains("dbsecret")); + let debug_dump = format!("{args:?}"); + assert!(debug_dump.contains("ApiServerWebServerConfig")); + assert!(debug_dump.contains(REDACTED)); + assert!(!debug_dump.contains("alice")); + assert!(!debug_dump.contains("supersecret")); + assert!(!debug_dump.contains("dbsecret")); + } + + #[test] + fn redacted_summary_renders_absent_secrets_as_none() { + // Built directly so no ML_API_WEB_SRV_* environment fallback applies. + let args = ApiServerWebServerConfig { + network: Network::Mainnet, + bind_address: None, + postgres_config: PostgresConfig { + postgres_host: "localhost".to_string(), + postgres_port: 5432, + postgres_user: "postgres".to_string(), + postgres_password: None, + postgres_database: None, + postgres_max_connections: 10, + }, + enable_post_routes: false, + node_rpc_address: None, + node_rpc_cookie_file: None, + node_rpc_username: None, + node_rpc_password: None, + stream_events_broadcast_capacity: streaming::DEFAULT_STREAM_EVENTS_BROADCAST_CAPACITY, + stream_events_max_subscribers: streaming::DEFAULT_STREAM_EVENTS_MAX_SUBSCRIBERS, + stream_events_poll_interval_secs: streaming::DEFAULT_STREAM_EVENTS_POLL_INTERVAL + .as_secs(), + stream_events_keepalive_interval_secs: + streaming::DEFAULT_STREAM_EVENTS_KEEPALIVE_INTERVAL.as_secs(), + }; + let summary = args.redacted_summary(); + assert!(!summary.contains(REDACTED)); + assert!(summary.contains("node_rpc_username")); + assert!(summary.contains("node_rpc_password")); + assert!(summary.contains("postgres_password")); + } +} + #[derive(Clone, Debug, Parser)] pub struct ListenAddress { socket: SocketAddr, diff --git a/api-server/web-server/src/main.rs b/api-server/web-server/src/main.rs index 19790092ef..c8dce4898b 100644 --- a/api-server/web-server/src/main.rs +++ b/api-server/web-server/src/main.rs @@ -14,9 +14,9 @@ // limitations under the License. mod api; -mod config; mod error; +use api_server_common::RedactedSummary; use api_server_common::storage::impls::postgres::{ PostgresStreamEventSource, TransactionalApiServerPostgresStorage, }; @@ -25,7 +25,6 @@ use api_web_server::{ ApiServerWebServerState, CachedValues, MempoolQueryClient, StreamEventsHandle, TxSubmitClient, api::web_server, config::ApiServerWebServerConfig, streaming, tx_dependency_ordering, }; -use clap::Parser; use common::{ chain::config::{Builder, ChainType}, primitives::time::Time, @@ -64,8 +63,14 @@ fn supervise(name: &'static str, handle: tokio::task::JoinHandle<()>) { async fn main() -> Result<(), ApiServerWebServerInitError> { logging::init_logging(); - let args = ApiServerWebServerConfig::parse(); - log::info!("Command line options: {args:?}"); + // Parse errors are not echoed because they can repeat secret-bearing command line tokens. + let args = api_server_common::parse_args_or_exit::(); + // The command line contains secrets (RPC credentials, database password), so the summary + // logged here omits them; see ApiServerWebServerConfig::redacted_summary. + log::info!( + "Command line options (secrets omitted): {}", + args.redacted_summary(), + ); let chain_type: ChainType = args.network.into(); let chain_config = Arc::new(Builder::new(chain_type).build()); diff --git a/common/src/chain/config/regtest.rs b/common/src/chain/config/regtest.rs index e042834d5c..1bbdd3483f 100644 --- a/common/src/chain/config/regtest.rs +++ b/common/src/chain/config/regtest.rs @@ -42,13 +42,25 @@ const GENESIS_STAKE_PRIVATE_KEY: &str = "008717e6946febd3a33ccdc3f3a27629ec80c33461c33a0fc56b4836fcedd26638"; const GENESIS_VRF_PRIVATE_KEY: &str = "003fcf7b813bec2a293f574b842988895278b396dd72471de2583b242097a59f06e9f3cd7b78d45750afd17292031373fddb5e7a8090db51221038f5e05f29998e"; -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, PartialEq)] pub struct GenesisStakingSettings { pool_id: PoolId, stake_private_key: PrivateKey, vrf_private_key: VRFPrivateKey, } +// The private keys are omitted so debug formatting of the chain options cannot disclose +// staking key material; the pool id is not sensitive and stays visible. +impl std::fmt::Debug for GenesisStakingSettings { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("GenesisStakingSettings") + .field("pool_id", &self.pool_id) + .field("stake_private_key", &"") + .field("vrf_private_key", &"") + .finish() + } +} + #[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)] pub enum GenesisStakingSettingsInputErrors { #[error("Duplicate staking setting {0}")] @@ -180,7 +192,38 @@ impl FromStr for GenesisStakingSettings { } } +#[cfg(test)] +mod genesis_staking_settings_tests { + use super::*; + + #[test] + fn debug_output_omits_private_keys() { + let settings = GenesisStakingSettings::from_str(&format!( + "pool_id:{GENESIS_POOL_ID},stake_private_key:{GENESIS_STAKE_PRIVATE_KEY},\ + vrf_private_key:{GENESIS_VRF_PRIVATE_KEY}" + )) + .unwrap(); + let rendering = format!("{settings:?}"); + assert!(rendering.contains(""), "{rendering}"); + assert!(rendering.contains(GENESIS_POOL_ID), "{rendering}"); + assert!( + !rendering.contains("8717e6946febd3a33ccdc3f3a27629ec80c33461c33a0fc56b4836fcedd26638"), + "{rendering}" + ); + assert!(!rendering.contains("3fcf7b813bec2a29"), "{rendering}"); + assert!(!rendering.contains("Scalar"), "{rendering}"); + assert!(!rendering.contains("Schnorrkel"), "{rendering}"); + } +} + impl Display for GenesisStakingSettings { + // This rendering deliberately includes the private keys: it is the value format of + // the --chain-genesis-staking-settings option (rendered as the clap default and + // parsed back by FromStr). It is an input format, never a log format, and must not + // be used in diagnostics; Debug above prints the redacted form. Rendering the + // default in --help is intentional as well: the shipped defaults are public + // constants in this repository, and the rendering doubles as the format example + // for operators supplying their own settings. fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { let mut values = vec![]; diff --git a/node-lib/Cargo.toml b/node-lib/Cargo.toml index 47fa715edb..09b97fa39b 100644 --- a/node-lib/Cargo.toml +++ b/node-lib/Cargo.toml @@ -18,6 +18,7 @@ storage-lmdb = { path = "../storage/lmdb" } subsystem = { path = "../subsystem" } test-rpc-functions = { path = "../test-rpc-functions" } utils = { path = "../utils" } +zeroize.workspace = true utils-networking = { path = "../utils/networking" } anyhow.workspace = true @@ -36,6 +37,7 @@ toml.workspace = true [dev-dependencies] crypto = { path = "../crypto" } +utils = { path = "../utils", features = ["test-util"] } randomness = { path = "../randomness" } ctor.workspace = true diff --git a/node-lib/src/config_files/mod.rs b/node-lib/src/config_files/mod.rs index aa0e75f51f..7d5d529906 100644 --- a/node-lib/src/config_files/mod.rs +++ b/node-lib/src/config_files/mod.rs @@ -33,7 +33,7 @@ use std::{fs, path::Path}; use ::chainstate_launcher::ChainConfig; use anyhow::{Context, Result}; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use crate::RunOptions; @@ -45,7 +45,7 @@ use self::{ /// The node configuration. #[must_use] -#[derive(Serialize, Deserialize, Debug, Clone)] +#[derive(Deserialize, Debug, Clone)] #[serde(deny_unknown_fields)] pub struct NodeConfigFile { // Subsystems configurations. diff --git a/node-lib/src/config_files/rpc.rs b/node-lib/src/config_files/rpc.rs index fab55c463d..8aeae3679c 100644 --- a/node-lib/src/config_files/rpc.rs +++ b/node-lib/src/config_files/rpc.rs @@ -15,15 +15,15 @@ use std::{net::SocketAddr, str::FromStr}; -use crate::RunOptions; +use crate::{Redacted, RunOptions}; use chainstate_launcher::ChainConfig; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use super::DEFAULT_RPC_ENABLED; /// The rpc subsystem configuration. #[must_use] -#[derive(Serialize, Deserialize, Debug, Default, Clone)] +#[derive(Deserialize, Debug, Default, Clone)] #[serde(deny_unknown_fields)] pub struct RpcConfigFile { /// Whether http RPC is enabled @@ -33,10 +33,10 @@ pub struct RpcConfigFile { pub bind_address: Option, /// Username for RPC server basic authorization - pub username: Option, + pub username: Option>, /// Password for RPC server basic authorization - pub password: Option, + pub password: Option>, /// Custom file path for the RPC cookie file pub cookie_file: Option, @@ -70,9 +70,9 @@ impl RpcConfigFile { .rpc_enabled .unwrap_or_else(|| rpc_enabled.unwrap_or(DEFAULT_RPC_ENABLED)); - let username = username.or(options.rpc_username.clone()); - let password = password.or(options.rpc_password.clone()); - let cookie_file = cookie_file.or(options.rpc_cookie_file.clone()); + let username = username.or_else(|| options.rpc_username.clone()); + let password = password.or_else(|| options.rpc_password.clone()); + let cookie_file = cookie_file.or_else(|| options.rpc_cookie_file.clone()); RpcConfigFile { bind_address: Some(bind_address), diff --git a/node-lib/src/lib.rs b/node-lib/src/lib.rs index 0bdb1a3df4..649914ba72 100644 --- a/node-lib/src/lib.rs +++ b/node-lib/src/lib.rs @@ -34,7 +34,7 @@ pub use config_files::{ pub use node_daemon_runner::{ExitCode, run_node_daemon}; pub use options::{ CLEAN_DATA_OPTION_LONG_NAME, Command, IMPORT_BOOTSTRAP_FILE_OPTION_LONG_NAME, Options, - OptionsWithResolvedCommand, RunOptions, TopLevelOptions, + OptionsWithResolvedCommand, Redacted, RunOptions, TopLevelOptions, }; pub use runner::{NodeSetupResult, NodeType, setup}; diff --git a/node-lib/src/options.rs b/node-lib/src/options.rs index da61692d88..1e227649bb 100644 --- a/node-lib/src/options.rs +++ b/node-lib/src/options.rs @@ -17,9 +17,11 @@ use std::{ ffi::OsString, + fmt, net::{IpAddr, SocketAddr}, num::NonZeroU64, path::{Path, PathBuf}, + str::FromStr, }; use clap::{Args, CommandFactory, FromArgMatches, Parser, Subcommand}; @@ -170,16 +172,24 @@ impl TopLevelOptions { } } +// Options whose values can hold a secret: RPC credentials and the genesis staking +// settings, which carry staking and VRF private keys. Their environment-sourced values +// are hidden from the help output (see the `mut_args` attributes and the guard test). +// For chain-genesis-staking-settings this hides the help rendering only; the options +// dump that debug-printed the staking keys was removed rather than redacted, so debug +// output stays secret-safe only as long as no full-options debug formatting returns. +const SECRET_OPTIONS: &[&str] = &["rpc-username", "rpc-password", "chain-genesis-staking-settings"]; + #[derive(Subcommand, Clone, Debug)] pub enum Command { /// Run the mainnet node. - #[clap(mut_args(clap_utils::env_adder("MAINNET_NODE")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values("MAINNET_NODE", SECRET_OPTIONS)))] Mainnet(RunOptions), /// Run the testnet node. - #[clap(mut_args(clap_utils::env_adder("TESTNET_NODE")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values("TESTNET_NODE", SECRET_OPTIONS)))] Testnet(RunOptions), /// Run the regtest node. - #[clap(mut_args(clap_utils::env_adder("REGTEST_NODE")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values("REGTEST_NODE", SECRET_OPTIONS)))] Regtest(Box), } @@ -237,6 +247,75 @@ pub struct RegtestOptions { pub chain_config: ChainConfigOptions, } +/// A value that must never appear in debug output, meant for command line options holding +/// credentials so that any debug formatting of the containing struct stays secret-safe. +/// +/// Note: redaction covers debug output only — values passed on the command line remain +/// visible in process listings and shell history. +/// +/// Note: serialization is intentionally not supported so that configuration dumps cannot +/// emit credentials; the type is only deserialized when reading a configuration file. +/// +/// Note: equality comparison is intentionally not derived; a derived comparison invites +/// non-constant-time checks on secret values in future authentication decisions. +/// +/// Note: the inner value is zeroized when the value is dropped, so the memory holding +/// the credential is wiped instead of lingering in freed memory (for zeroizable types). +/// This covers this wrapper's copy only: copies a consumer makes out of the value (e.g. +/// [`std::string::String`]s inside an RPC server) are ordinary values with ordinary +/// lifetimes and are not scrubbed. +#[derive(Clone, Default)] +pub struct Redacted(T); + +impl Drop for Redacted { + fn drop(&mut self) { + self.0.zeroize(); + } +} + +impl fmt::Debug for Redacted { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("") + } +} + +impl From for Redacted { + fn from(value: T) -> Self { + Self(value) + } +} + +impl FromStr for Redacted { + type Err = T::Err; + + // Note that the intermediate `String` handed over by the command line parser and + // the parser's own argument copies are not zeroized, so the wipe only covers the + // value inside this wrapper. + fn from_str(s: &str) -> Result { + T::from_str(s).map(Self) + } +} + +impl AsRef for Redacted { + fn as_ref(&self) -> &T { + &self.0 + } +} + +// Serialization is intentionally not supported so that configuration dumps cannot emit +// credentials; only deserialization (reading a configuration file) is provided. +// Consuming access is intentionally not provided either: `into_inner` would move the +// value out of a type with a `Drop` impl, and dropped values are zeroized, not handed +// out. Read access goes through `AsRef`. +impl<'de, T> serde::Deserialize<'de> for Redacted +where + T: serde::Deserialize<'de> + zeroize::Zeroize, +{ + fn deserialize>(deserializer: D) -> Result { + T::deserialize(deserializer).map(Self) + } +} + #[derive(Args, Clone, Debug, Default)] pub struct RunOptions { /// Minimum number of connected peers to enable block production. @@ -412,12 +491,12 @@ pub struct RunOptions { /// Username for RPC server basic authorization. /// If not set, the cookie file is created. #[clap(long, value_name = "USERNAME")] - pub rpc_username: Option, + pub rpc_username: Option>, /// Password for RPC server basic authorization. /// If not set, the RPC cookie file is created. #[clap(long, value_name = "PASSWORD")] - pub rpc_password: Option, + pub rpc_password: Option>, /// Custom file path for the RPC cookie file. /// If not set, the cookie file is created in the data dir. @@ -451,7 +530,7 @@ pub fn default_data_dir(chain_type: ChainType) -> PathBuf { #[cfg(test)] mod tests { - use std::{collections::BTreeMap, str::FromStr as _}; + use std::collections::BTreeMap; use rstest::rstest; @@ -461,6 +540,17 @@ mod tests { }; use utils::concatln; + /// Pins the secret option list against the derived command: a renamed field would + /// silently stop matching the hiding list in the `mut_args` attributes and leak its + /// environment value into `--help`. + #[test] + fn secret_options_env_values_are_hidden() { + utils::clap_utils::assert_secret_options_hidden( + &::command(), + SECRET_OPTIONS, + ); + } + use super::*; #[rstest] @@ -576,3 +666,42 @@ mod tests { } } } + +#[cfg(test)] +mod redacted_tests { + use super::*; + + #[test] + fn run_options_debug_omits_rpc_credentials() { + let options = RunOptions { + rpc_username: Some("rpc_user_name".to_owned().into()), + rpc_password: Some("rpc_pass_word".to_owned().into()), + ..Default::default() + }; + + let debug_output = format!("{options:?}"); + assert!(debug_output.contains(""), "{debug_output}"); + assert!(!debug_output.contains("rpc_user_name"), "{debug_output}"); + assert!(!debug_output.contains("rpc_pass_word"), "{debug_output}"); + } + + #[test] + fn rpc_config_file_debug_omits_rpc_credentials() { + let options = RunOptions { + rpc_username: Some("rpc_user_name".to_owned().into()), + rpc_password: Some("rpc_pass_word".to_owned().into()), + ..Default::default() + }; + + let config = crate::RpcConfigFile::with_run_options( + &common::chain::config::create_testnet(), + crate::RpcConfigFile::default(), + &options, + ); + + let debug_output = format!("{config:#?}"); + assert!(debug_output.contains(""), "{debug_output}"); + assert!(!debug_output.contains("rpc_user_name"), "{debug_output}"); + assert!(!debug_output.contains("rpc_pass_word"), "{debug_output}"); + } +} diff --git a/node-lib/src/runner.rs b/node-lib/src/runner.rs index 78fdc86765..5485ecbe67 100644 --- a/node-lib/src/runner.rs +++ b/node-lib/src/runner.rs @@ -41,7 +41,7 @@ use crate::{ config_files::{DEFAULT_P2P_NETWORKING_ENABLED, DEFAULT_RPC_ENABLED, NodeConfigFile}, mock_time::set_mock_time, node_controller::NodeController, - options::{OptionsWithResolvedCommand, RunOptions, default_data_dir}, + options::{OptionsWithResolvedCommand, RunOptions, TopLevelOptions, default_data_dir}, }; const LOCK_FILE_NAME: &str = ".lock"; @@ -202,8 +202,8 @@ async fn initialize( if rpc_config.rpc_enabled.unwrap_or(DEFAULT_RPC_ENABLED) { let rpc_creds = RpcCreds::new( data_dir, - rpc_config.username.as_deref(), - rpc_config.password.as_deref(), + rpc_config.username.as_ref().map(|value| value.as_ref().as_str()), + rpc_config.password.as_ref().map(|value| value.as_ref().as_str()), rpc_config.cookie_file.as_deref(), )?; @@ -240,6 +240,120 @@ async fn initialize( Ok((manager, controller)) } +/// Restricts the permissions of log files left by earlier versions (which may hold +/// material dumped from the command line) to owner-only. Returns per-file failures and +/// the skipped non-regular entries so the caller can report everything once logging is +/// initialized. Unix only: Windows keeps its own ACL model, so no equivalent +/// restriction is applied there. +#[cfg(unix)] +fn collect_log_permission_fix_errors( + data_dir: &std::path::Path, + log_file_name: &str, +) -> ( + Vec<(std::path::PathBuf, std::io::Error)>, + Vec, +) { + use std::os::unix::fs::PermissionsExt; + // A rotated log file carries the base name plus a dot and a numeric suffix. + fn is_rotated_log_name(log_file_name: &str, name: &str) -> bool { + name.strip_prefix(log_file_name).is_some_and(|suffix| { + suffix + .strip_prefix('.') + .is_some_and(|rest| !rest.is_empty() && rest.bytes().all(|b| b.is_ascii_digit())) + }) + } + let mut permission_fix_errors: Vec<(std::path::PathBuf, std::io::Error)> = Vec::new(); + #[cfg(unix)] + let mut skipped_entries: Vec = Vec::new(); + match std::fs::read_dir(data_dir.join("logs")) { + // The first run has no logs directory yet. + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => permission_fix_errors.push((data_dir.join("logs"), err)), + Ok(entries) => { + // A failing entry yields only an io::Error without its path, so the failure + // is reported once against the directory; later repeats add no information. + let mut directory_read_error_reported = false; + let mut suppressed_read_errors: usize = 0; + for entry in entries { + let entry = match entry { + Ok(entry) => entry, + Err(err) => { + if !directory_read_error_reported { + permission_fix_errors.push((data_dir.join("logs"), err)); + directory_read_error_reported = true; + } else { + suppressed_read_errors += 1; + } + continue; + } + }; + // Skip entries that are not regular files so the mode is normally never + // applied through a link or to anything but a regular file; a matching + // name left as a directory, symlink, or special file is reported below + // instead of being silently ignored. + let name = entry.file_name().to_string_lossy().into_owned(); + let is_matching_log_name = + name == log_file_name || is_rotated_log_name(log_file_name, &name); + let is_regular_file = match entry.file_type() { + Ok(file_type) => file_type.is_file(), + Err(err) => { + if is_matching_log_name { + permission_fix_errors.push((entry.path(), err)); + } + continue; + } + }; + if !is_regular_file { + if is_matching_log_name { + skipped_entries.push(entry.path()); + } + continue; + } + // Only touch the log file of this executable and its rotated variants. + if is_matching_log_name { + // Anchor the identity in the directory entry itself: the inode comes + // from the readdir buffer, so it reflects the listing rather than the + // current state of the path. + use std::os::unix::fs::DirEntryExt; + let listed_ino = entry.ino(); + // Change the mode through the path rather than an open file: chmod + // never blocks, while opening a path swapped in for a FIFO would wait + // for a writer. The pre-checks above do not follow links, but chmod + // itself does, so a path swapped in between the listing and this call + // still redirects the mode change to whatever it now references; the + // identity comparison below then reports the swap. Fully closing the + // window would need platform-specific open flags (no-follow, or + // non-blocking to keep the FIFO case non-blocking), which would add a + // platform dependency this workspace does not carry. + if let Err(err) = std::fs::set_permissions( + entry.path(), + std::fs::Permissions::from_mode(0o600), + ) + .and_then(|()| { + use std::os::unix::fs::MetadataExt; + let current = std::fs::symlink_metadata(entry.path())?; + if listed_ino != current.ino() { + return Err(std::io::Error::other("log file changed while scanning")); + } + Ok(()) + }) { + permission_fix_errors.push((entry.path(), err)); + } + } + } + if suppressed_read_errors > 0 { + permission_fix_errors.push(( + data_dir.join("logs"), + std::io::Error::other(format!( + "{suppressed_read_errors} further directory read failures suppressed" + )), + )); + } + } + } + (permission_fix_errors, skipped_entries) +} + /// Processes options and potentially runs the node. pub async fn setup( options: OptionsWithResolvedCommand, @@ -260,23 +374,38 @@ pub async fn setup( // Init logging let main_log_writer_settings = logging::default_writer_settings(default_log_filter.to_owned()); + let log_file_name = std::env::current_exe().map_or_else( + |_| DEFAULT_LOG_FILE_NAME.to_owned(), + |exe| { + exe.as_path().file_stem().and_then(|stem| stem.to_str()).map_or_else( + || DEFAULT_LOG_FILE_NAME.to_owned(), + |s| format!("{}.log", s.to_owned()), + ) + }, + ); + // Log files written by earlier versions may have wider permissions; restrict them too. + // This best-effort hardening runs even when this invocation does not write logs to a + // file. The scan and the mode change are separate steps, so a path swapped in between + // is skipped or reported. Failures are reported once logging is initialized below. + // A --clean-data-dir run deletes the whole directory, so the scan is skipped there. + // The scan is a synchronous one-shot during startup, before the runtime serves any + // request, so blocking the async context here is acceptable. + #[cfg(unix)] + let (permission_fix_errors, skipped_log_entries) = if options.clean_data_option_set() { + (Vec::new(), Vec::new()) + } else { + collect_log_permission_fix_errors(&data_dir, &log_file_name) + }; if options.log_to_file_option_set() { - let log_file_name = std::env::current_exe().map_or_else( - |_| DEFAULT_LOG_FILE_NAME.to_owned(), - |exe| { - exe.as_path().file_stem().and_then(|stem| stem.to_str()).map_or_else( - || DEFAULT_LOG_FILE_NAME.to_owned(), - |s| format!("{}.log", s.to_owned()), - ) - }, - ); let log_file = FileRotate::new( data_dir.join(format!("logs/{}", log_file_name)), AppendCount::new(13), // total 14 files ContentLimit::Bytes(10_000_000), // 10MB each Compression::None, #[cfg(unix)] - None, + // Logs can contain sensitive information, so they are only readable by the owner, + // like the RPC cookie file. + Some(0o600), ); logging::init_logging_generic( main_log_writer_settings, @@ -292,6 +421,21 @@ pub async fn setup( } else { logging::init_logging_generic(main_log_writer_settings, logging::no_writer_settings()); } + #[cfg(unix)] + for (path, err) in &permission_fix_errors { + logging::log::warn!( + "Failed to restrict permissions of log file {}: {}", + path.display(), + err + ); + } + #[cfg(unix)] + for path in &skipped_log_entries { + logging::log::info!( + "Skipping log entry that is not a regular file: {}", + path.display() + ); + } // Clean data dir if needed if options.clean_data_option_set() { @@ -303,7 +447,25 @@ pub async fn setup( return Ok(NodeSetupResult::DataDirCleanedUp); } - logging::log::info!("Command line options: {options:?}"); + // The full command line contains secrets (RPC credentials, staking keys), so only a + // curated subset of options that cannot hold secrets is logged. Destructuring the + // top-level options exhaustively forces a decision about what the line includes + // whenever a new option is added. + let TopLevelOptions { + data_dir: _, + create_data_dir_if_missing, + log_to_file, + clean_data: _, + import_bootstrap_file: _, + } = &options.top_level; + logging::log::info!( + "Command line options (secrets omitted): network: {}, data directory: {:?}, \ + create data dir if missing: {:?}, log to file: {:?}", + options.command.chain_type().name(), + data_dir, + create_data_dir_if_missing, + log_to_file, + ); let mut options = options; diff --git a/node-lib/tests/cli.rs b/node-lib/tests/cli.rs index 7fbf9820b3..e969ad12e4 100644 --- a/node-lib/tests/cli.rs +++ b/node-lib/tests/cli.rs @@ -173,8 +173,8 @@ fn read_config_override_values() { max_tip_age: Some(max_tip_age), rpc_bind_address: Some(rpc_bind_address), rpc_enabled: Some(true), - rpc_username: Some(rpc_username.to_owned()), - rpc_password: Some(rpc_password.to_owned()), + rpc_username: Some(rpc_username.to_owned().into()), + rpc_password: Some(rpc_password.to_owned().into()), rpc_cookie_file: Some(rpc_cookie_file.to_owned()), min_tx_relay_fee_rate: Some(min_tx_relay_fee_rate), force_allow_run_as_root_outer: Default::default(), @@ -325,11 +325,11 @@ fn read_config_override_values() { assert!(config.rpc.clone().unwrap().rpc_enabled.unwrap()); assert_eq!( - config.rpc.as_ref().unwrap().username.as_deref(), + config.rpc.as_ref().unwrap().username.as_ref().map(|v| v.as_ref().as_str()), Some(rpc_username) ); assert_eq!( - config.rpc.as_ref().unwrap().password.as_deref(), + config.rpc.as_ref().unwrap().password.as_ref().map(|v| v.as_ref().as_str()), Some(rpc_password) ); assert_eq!( diff --git a/utils/Cargo.toml b/utils/Cargo.toml index eac3709fe8..b49cb44974 100644 --- a/utils/Cargo.toml +++ b/utils/Cargo.toml @@ -46,6 +46,9 @@ loom.workspace = true [features] tokio-console = ["logging/tokio-console"] dev = [] # used by fixed-hash +# Gates the clap test helpers; enabled through a dev-dependency entry by the crates +# whose test suites use them. +test-util = [] [[bench]] name = "benches" diff --git a/utils/src/clap_utils.rs b/utils/src/clap_utils.rs index f31c6f5b6b..acd75fb7fb 100644 --- a/utils/src/clap_utils.rs +++ b/utils/src/clap_utils.rs @@ -47,3 +47,234 @@ pub fn add_env(infix: &str, mut arg: Arg) -> Arg { pub fn env_adder(infix: &str) -> impl FnMut(Arg) -> Arg + '_ { |arg| add_env(infix, arg) } + +/// Same as [`env_adder`], but the values of the options named in `hidden` are excluded +/// from the help output, so secrets provided through the environment are not rendered +/// as `[env: NAME=value]`. +/// +/// Matching is by exact long name: an option missing from (or misspelled in) `hidden` +/// silently keeps rendering its environment value in `--help`. Always pair this helper +/// with [`assert_secret_options_hidden`] in a test so the list is pinned against the +/// derived command. +pub fn env_adder_hiding_env_values<'a>( + infix: &'a str, + hidden: &'a [&'a str], +) -> impl FnMut(Arg) -> Arg + 'a { + move |mut arg| { + arg = add_env(infix, arg); + if arg.get_long().is_some_and(|long| hidden.contains(&long)) { + arg = arg.hide_env_values(true); + } + arg + } +} + +// Gated additionally on `test-util` so consumer test suites can reach it through a +// dev-dependency feature entry, while this crate's own test units get it from plain +// `cfg(test)`. +#[cfg(any(test, feature = "test-util"))] +fn collect_args<'a>(cmd: &'a clap::Command, args: &mut Vec<&'a clap::Arg>) { + args.extend(cmd.get_arguments()); + for sub in cmd.get_subcommands() { + collect_args(sub, args); + } +} + +/// Test helper: asserts that every option named in `secret_options` exists in `cmd` +/// (recursively, including all nested subcommands), is bound to an environment variable, +/// and has its environment-sourced value hidden from the help output. +#[cfg(any(test, feature = "test-util"))] +#[doc(hidden)] +pub fn assert_secret_options_hidden(cmd: &clap::Command, secret_options: &[&str]) { + let mut all_args = Vec::new(); + collect_args(cmd, &mut all_args); + let secrets = secret_options.iter().copied().collect::>(); + let mut matched = std::collections::HashSet::new(); + for arg in all_args { + if let Some(long) = arg.get_long().filter(|long| secrets.contains(long)) { + assert!( + arg.is_hide_env_values_set(), + "env value of --{long} is not hidden from help" + ); + assert!( + arg.get_env().is_some(), + "--{long} has no environment variable bound; env wiring was dropped" + ); + matched.insert(long); + } + } + let mut matched: Vec<_> = matched.into_iter().collect(); + let mut expected: Vec<_> = secret_options.to_vec(); + matched.sort_unstable(); + expected.sort_unstable(); + assert_eq!( + matched, expected, + "the secret options list does not match the actual secret options; \ + stale entries pass silently and missing entries leak env values in --help" + ); +} + +/// Test helper: same as [`assert_secret_options_hidden`], plus an exhaustive +/// classification check: every option of `cmd` must be listed in `secret_options` or +/// `non_secret_options`, and every non-secret entry must exist in the command. +#[cfg(any(test, feature = "test-util"))] +#[doc(hidden)] +pub fn assert_options_classified( + cmd: &clap::Command, + secret_options: &[&str], + non_secret_options: &[&str], +) { + // Fail safe when a credential-shaped name is classified as non-secret; the + // exhaustive classification below is still the authoritative contract. + const SECRET_LIKE_MARKERS: &[&str] = &["password", "secret", "token", "key"]; + assert_secret_options_hidden(cmd, secret_options); + let mut all_args = Vec::new(); + collect_args(cmd, &mut all_args); + // Arguments that clap generates itself (`help`, `version`) are not part of the + // classification contract, which only covers the options this crate declares. + let longs: std::collections::HashSet<_> = all_args + .iter() + .filter_map(|arg| arg.get_long()) + .filter(|long| !matches!(*long, "help" | "version")) + .collect(); + let unclassified: Vec<_> = longs + .iter() + .copied() + .filter(|long| !secret_options.contains(long) && !non_secret_options.contains(long)) + .collect(); + assert!( + unclassified.is_empty(), + "options not classified as secret or non-secret: {unclassified:?}" + ); + let stale: Vec<_> = non_secret_options + .iter() + .copied() + .filter(|long| !longs.contains(long)) + .collect(); + assert!( + stale.is_empty(), + "non-secret options not found in the command (stale entries): {stale:?}" + ); + let secret_shaped: Vec<_> = non_secret_options + .iter() + .copied() + .filter(|long| SECRET_LIKE_MARKERS.iter().any(|marker| long.contains(marker))) + .collect(); + assert!( + secret_shaped.is_empty(), + "non-secret options with credential-shaped names: {secret_shaped:?}" + ); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn env_values_are_hidden_only_for_the_named_options() { + let mut wrap = env_adder_hiding_env_values("NODE", &["rpc-username", "rpc-password"]); + + let arg = wrap(clap::Arg::new("rpc_password").long("rpc-password")); + assert!(arg.is_hide_env_values_set()); + assert!(arg.get_env().is_some()); + + let arg = wrap(clap::Arg::new("rpc_username").long("rpc-username")); + assert!(arg.is_hide_env_values_set()); + assert!(arg.get_env().is_some()); + + let arg = wrap(clap::Arg::new("datadir").long("datadir")); + assert!(!arg.is_hide_env_values_set()); + assert!(arg.get_env().is_some()); + } +} + +// The helper carries the `test-util` gate (see its definition); these tests exercise it +// in this crate's own test units, which get it from plain `cfg(test)`. +#[cfg(test)] +mod secret_options_guard_tests { + use super::*; + + #[test] + fn guard_accepts_a_wired_command_tree() { + let cmd = clap::Command::new("root").subcommand( + clap::Command::new("sub").arg( + add_env("NODE", clap::Arg::new("rpc_password").long("rpc-password")) + .hide_env_values(true), + ), + ); + assert_secret_options_hidden(&cmd, &["rpc-password"]); + } + + #[test] + #[should_panic(expected = "does not match the actual secret options")] + fn guard_rejects_a_stale_entry() { + let cmd = clap::Command::new("root").arg( + add_env("NODE", clap::Arg::new("rpc_password").long("rpc-password")) + .hide_env_values(true), + ); + assert_secret_options_hidden(&cmd, &["rpc-password", "old-option"]); + } + + #[test] + #[should_panic(expected = "does not match the actual secret options")] + fn guard_rejects_a_missing_entry() { + let cmd = clap::Command::new("root") + .arg(add_env("NODE", clap::Arg::new("datadir").long("datadir"))); + assert_secret_options_hidden(&cmd, &["rpc-password"]); + } + + #[test] + #[should_panic(expected = "not hidden from help")] + fn guard_rejects_an_unhidden_option() { + let cmd = clap::Command::new("root").arg( + add_env("NODE", clap::Arg::new("rpc_password").long("rpc-password")) + // The env wiring is present but the value is not hidden. + .hide_env_values(false), + ); + assert_secret_options_hidden(&cmd, &["rpc-password"]); + } + + #[test] + #[should_panic(expected = "no environment variable bound")] + fn guard_rejects_a_dropped_env_binding() { + let cmd = clap::Command::new("root") + .arg(clap::Arg::new("rpc_password").long("rpc-password").hide_env_values(true)); + assert_secret_options_hidden(&cmd, &["rpc-password"]); + } + + #[test] + #[should_panic(expected = "not classified as secret or non-secret")] + fn classification_rejects_an_unclassified_option() { + let cmd = clap::Command::new("root") + .arg(add_env("NODE", clap::Arg::new("datadir").long("datadir"))); + assert_options_classified(&cmd, &[], &[]); + } + + #[test] + #[should_panic(expected = "stale entries")] + fn classification_rejects_a_stale_non_secret_entry() { + let cmd = clap::Command::new("root") + .arg(add_env("NODE", clap::Arg::new("datadir").long("datadir"))); + assert_options_classified(&cmd, &[], &["datadir", "old-option"]); + } + + #[test] + #[should_panic(expected = "credential-shaped names")] + fn classification_rejects_a_credential_shaped_non_secret_entry() { + let cmd = clap::Command::new("root").arg(add_env( + "NODE", + clap::Arg::new("api_token").long("api-token"), + )); + assert_options_classified(&cmd, &[], &["api-token"]); + } + + #[test] + fn classification_ignores_the_generated_help_and_version_args() { + let mut cmd = clap::Command::new("root") + .version("1.0") + .arg(add_env("NODE", clap::Arg::new("datadir").long("datadir"))); + // Build the command so clap generates its own help and version arguments. + cmd.build(); + assert_options_classified(&cmd, &[], &["datadir"]); + } +} diff --git a/wallet/wallet-cli-lib/Cargo.toml b/wallet/wallet-cli-lib/Cargo.toml index 2b9534cc15..a0f456158a 100644 --- a/wallet/wallet-cli-lib/Cargo.toml +++ b/wallet/wallet-cli-lib/Cargo.toml @@ -51,6 +51,7 @@ futures.workspace = true prettytable-rs = "0.10" [dev-dependencies] +utils = { path = "../../utils", features = ["test-util"] } blockprod = { path = "../../blockprod" } chainstate-storage = { path = "../../chainstate/storage" } crypto = { path = "../../crypto" } diff --git a/wallet/wallet-cli-lib/src/config.rs b/wallet/wallet-cli-lib/src/config.rs index dc984bfab3..57074c11c6 100644 --- a/wallet/wallet-cli-lib/src/config.rs +++ b/wallet/wallet-cli-lib/src/config.rs @@ -25,16 +25,51 @@ use wallet_rpc_lib::cmdline::CliHardwareWalletType; #[derive(Subcommand, Clone, Debug)] pub enum Network { - #[clap(mut_args(clap_utils::env_adder("MAINNET_WALLET")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "MAINNET_WALLET", + WALLET_SECRET_OPTIONS + )))] Mainnet(CliArgs), - #[clap(mut_args(clap_utils::env_adder("TESTNET_WALLET")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "TESTNET_WALLET", + WALLET_SECRET_OPTIONS + )))] Testnet(CliArgs), - #[clap(mut_args(clap_utils::env_adder("REGTEST_WALLET")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "REGTEST_WALLET", + WALLET_SECRET_OPTIONS + )))] Regtest(Box), - #[clap(mut_args(clap_utils::env_adder("SIGNET_WALLET")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "SIGNET_WALLET", + WALLET_SECRET_OPTIONS + )))] Signet(CliArgs), } +/// Options whose values can hold a secret; their environment-sourced values are hidden +/// from the help output. +const WALLET_SECRET_OPTIONS: &[&str] = &[ + "node-rpc-username", + "node-rpc-password", + "wallet-rpc-username", + "wallet-rpc-password", + "wallet-password", + "remote-rpc-wallet-username", + "remote-rpc-wallet-password", +]; + +#[cfg(test)] +mod tests { + use super::*; + use clap::CommandFactory; + + #[test] + fn every_secret_option_has_its_env_value_hidden() { + clap_utils::assert_secret_options_hidden(&WalletCliArgs::command(), WALLET_SECRET_OPTIONS); + } +} + #[derive(Args, Clone, Debug)] pub struct RegtestOptions { #[clap(flatten)] @@ -44,7 +79,7 @@ pub struct RegtestOptions { } #[derive(Parser, Debug)] -#[clap(mut_args(clap_utils::env_adder("WALLET")))] +#[clap(mut_args(clap_utils::env_adder_hiding_env_values("WALLET", WALLET_SECRET_OPTIONS)))] #[clap(version(app_version_with_git_info!().to_pretty_string()))] #[command(args_conflicts_with_subcommands = true)] pub struct WalletCliArgs { diff --git a/wallet/wallet-rpc-lib/Cargo.toml b/wallet/wallet-rpc-lib/Cargo.toml index ebd4a3280f..b4217d8ba1 100644 --- a/wallet/wallet-rpc-lib/Cargo.toml +++ b/wallet/wallet-rpc-lib/Cargo.toml @@ -42,6 +42,7 @@ tokio.workspace = true [dev-dependencies] +utils = { path = "../../utils", features = ["test-util"] } consensus = { path = "../../consensus" } mempool = { path = "../../mempool" } rpc = { path = "../../rpc", features = ["test-support"] } diff --git a/wallet/wallet-rpc-lib/src/cmdline.rs b/wallet/wallet-rpc-lib/src/cmdline.rs index 3762d10d6e..e5fbdfcb66 100644 --- a/wallet/wallet-rpc-lib/src/cmdline.rs +++ b/wallet/wallet-rpc-lib/src/cmdline.rs @@ -48,18 +48,32 @@ impl WalletRpcDaemonArgs { } } +/// Options whose values can hold a secret; their environment-sourced values are hidden +/// from the help output. +const WALLET_RPC_SECRET_OPTIONS: &[&str] = + &["rpc-username", "rpc-password", "node-rpc-username", "node-rpc-password"]; + #[derive(clap::Subcommand)] pub enum WalletRpcDaemonCommand { /// Run the mainnet wallet. - #[clap(mut_args(clap_utils::env_adder("MAINNET_WALLET_RPC_DAEMON")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "MAINNET_WALLET_RPC_DAEMON", + WALLET_RPC_SECRET_OPTIONS + )))] Mainnet(WalletRpcDaemonChainArgs), /// Run the testnet wallet. - #[clap(mut_args(clap_utils::env_adder("TESTNET_WALLET_RPC_DAEMON")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "TESTNET_WALLET_RPC_DAEMON", + WALLET_RPC_SECRET_OPTIONS + )))] Testnet(WalletRpcDaemonChainArgs), /// Run the regtest wallet. - #[clap(mut_args(clap_utils::env_adder("REGTEST_WALLET_RPC_DAEMON")))] + #[clap(mut_args(clap_utils::env_adder_hiding_env_values( + "REGTEST_WALLET_RPC_DAEMON", + WALLET_RPC_SECRET_OPTIONS + )))] Regtest { #[command(flatten)] args: WalletRpcDaemonChainArgs, @@ -316,3 +330,17 @@ pub enum ConfigError { #[error(transparent)] RunningAsRoot(anyhow::Error), } + +#[cfg(test)] +mod tests { + use super::*; + use clap::CommandFactory; + + #[test] + fn every_secret_option_has_its_env_value_hidden() { + clap_utils::assert_secret_options_hidden( + &WalletRpcDaemonArgs::command(), + WALLET_RPC_SECRET_OPTIONS, + ); + } +}