fix(node-lib): keep rpc credentials and staking keys out of logs and help output

api-server/api-server-common/src/lib.rs

@@ -19,6 +19,145 @@ pub mod streaming;
 use clap::Parser;
 use common::chain::config::ChainType;
 
+/// Marker printed in place of any secret-bearing value.
+pub const REDACTED: &str = "<redacted>";
+
+/// Debug-safe replacement for an `Option` that may hold a secret.
+pub fn redact<T>(value: &Option<T>) -> 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;
+}
+
+/// 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: clap::Parser>() -> 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 can never
+                    // contain argument values, so it is safe to include here.
+                    eprintln!(
+                        "Invalid command line arguments ({:?}); 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 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));
+    }
+}
+
 #[derive(clap::ValueEnum, Clone, Debug)]
 pub enum Network {
     Mainnet,
@@ -38,18 +177,23 @@ impl From<Network> 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;
+
+#[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 +205,74 @@ pub struct PostgresConfig {
     pub postgres_database: Option<String>,
 
     /// 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(\"<redacted>\")"));
+        assert!(!dump.contains("dbsecret"));
     }
 }

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,26 @@ impl TransactionalApiServerPostgresStorage {
         max_connections: u32,
         chain_config: Arc<ChainConfig>,
     ) -> Result<Self, ApiServerStorageError> {
-        let password_part = match password {
-            Some(p) => format!("password={}", p),
-            None => "".to_string(),
-        };
-
-        let database_part = match database {
-            Some(d) => format!("dbname={}", d),
-            None => format!("dbname=mintlayer-{}", chain_config.chain_type().name()),
+        // 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()),
         };
+        logging::log::debug!(
+            "Using postgres config: host={host} port={port} user={user} password={} database={database}",
+            if password.is_some() {
+                crate::REDACTED
+            } else {
+                "<unset>"
+            }
+        );
 
-        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: <tokio_postgres::Config as FromStr>::Err| {
-                ApiServerStorageError::InitializationError(format!(
-                    "Postgres configuration parsing error: {}",
-                    e
-                ))
-            })?;
+        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| {

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" }