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

[nullPointerEnjoyer]

Problem

At startup the node logged its entire parsed command line at INFO level, including rpc_username, rpc_password and --chain-genesis-staking-settings (which carries staking and VRF private keys). The log file was created with default umask permissions (typically 0644) and its level was not configurable, so the secrets landed world-readable on disk on every start. Separately, clap's [env: NAME=value] help rendering displayed environment-sourced values for these options, and the API server daemons logged the postgres connection string including the password at debug level.

Change

• Stop logging the full options dump; log a curated, secret-free summary instead.
• Type-level redaction: new Redacted<T> wrapper (zeroizing Debug) for RPC credential fields; redacted Debug for GenesisStakingSettings (pool id stays visible, private keys do not).
• Log files (current and rotated, including files written by earlier versions) are restricted to 0600, with failures reported after logging is initialized; the scan anchors file identity in the directory entry so a path swapped mid-scan is detected.
• Secret-bearing CLI options hide their environment values in help output (hide_env_values) across node-lib, wallet-cli, wallet-rpc and the API server daemons; guard tests pin every option as classified secret/non-secret, and the classification helper recurses into subcommands.
• The postgres connection log redacts the password; clap parse errors never echo argument values (value-free diagnostics only).

Testing

• New unit tests: redacted debug rendering for credentials and staking settings, log-name matching, permission-fix error collection, env-value hiding guard tests (including subcommand coverage), value-free clap error rendering canaries, postgres log redaction.
• cargo test green for affected crates; workspace fmt/clippy checks pass.

[github-actions]

🔍 OpenCodeReview found 6 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)
• 📋 Routed to summary by policy: 5 comment(s)

---

📄 node-lib/src/options.rs (L299-L303)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category security)

AsRef hands the secret out as a plain reference, and runner.rs immediately converts the credentials back to &str for RpcCreds::new, which stores them in ordinary Strings (see rpc/src/rpc_creds.rs) that are never zeroized. This keeps the zeroization benefit limited to the wrapper's own copy, and the wide AsRef escape hatch makes it trivial for future code to extract the secret. Consider accepting/storing Redacted (or exposing access via a callback/with_secret closure instead of AsRef) so the credential's protected lifecycle extends into RpcCreds and the escape hatch is narrower.

💡 Suggested Change

Before:

impl<T: zeroize::Zeroize> AsRef<T> for Redacted<T> {
    fn as_ref(&self) -> &T {
        &self.0
    }
}

After:

// Consider a scoped-access API instead of AsRef to keep the secret's lifetime
// inside the wrapper:
impl<T: zeroize::Zeroize> Redacted<T> {
    pub fn with<R>(&self, f: impl FnOnce(&T) -> R) -> R {
        f(&self.0)
    }
}

---

📄 node-lib/src/runner.rs (L328-L331)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category security)

set_permissions follows symlinks, so if a log-named entry is swapped for a symlink between read_dir and this call, the 0o600 mode is applied to whatever the symlink now points at before the inode comparison merely reports the swap. The comment acknowledges the window; if you want to close it without a new platform dependency, std::os::unix::fs::OpenOptionsExt supports O_NOFOLLOW, and chmod can then be done via fchown/fchmod-style calls on the owned fd (or at minimum verify the identity via symlink_metadata before chmod and treat any mismatch as skip-not-chmod).

💡 Suggested Change

Before:

                    if let Err(err) = std::fs::set_permissions(
                        entry.path(),
                        std::fs::Permissions::from_mode(0o600),
                    )

After:

                    // Check identity before the chmod, not only after, so a swapped
                    // path is skipped rather than chmodded through the link:
                    let pre = std::fs::symlink_metadata(entry.path())?;
                    if pre.ino() != listed_ino {
                        return Err(std::io::Error::other("log file changed while scanning"));
                    }
                    std::fs::set_permissions(entry.path(), std::fs::Permissions::from_mode(0o600))?;

---

📄 utils/src/clap_utils.rs (L129-L129)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category style)

The "key" substring marker will flag benign non-secret options that merely contain "key" as a substring (e.g., --chain-key-format, --keypath-*, --monitored-key-prefix), producing noisy false positives and incentivizing list gymnastics that defeat the exhaustive classification. Consider matching on the key-prefixed/token forms that indicate credentials (e.g., split on '-' and compare whole words, or restrict the marker set to unambiguous ones like password, secret, token, passphrase, mnemonic).

💡 Suggested Change

Before:

    const SECRET_LIKE_MARKERS: &[&str] = &["password", "secret", "token", "key"];

After:

    const SECRET_LIKE_MARKERS: &[&str] = &["password", "secret", "token", "passphrase", "mnemonic"];

---

📄 utils/Cargo.toml (L51-L51)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category other)

test-util is only guarded by convention: the guard helpers are pub (merely #[doc(hidden)]), and in this update several consumer crates (scanner-daemon, web-server, node-lib, wallet-cli-lib, wallet-rpc-lib) enable the feature on the regular utils dependency rather than through a [dev-dependencies] entry, so the assert helpers are compiled into production release binaries. If test-only compilation is intended, either document/enforce that the feature must only be enabled via dev-dependencies (e.g., #[cfg(any(test, feature = "test-util"))] plus a compile_error guard, or required-features usage in consumers), or accept the dead code in binaries and note the trade-off in the feature doc.

---

📄 utils/src/clap_utils.rs (L93-L94)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category test)

The classification guards (assert_secret_options_hidden, assert_options_classified) only inspect arguments via get_long(). Arguments that have no long name (positionals, or short-only options) are invisible to both the secret check and the exhaustive classification, so a future short-only secret option could carry a visible env value in --help without any guard firing. Consider also handling arg.get_short()/positional args, or at least asserting that every argument in the tree has a long name so this gap cannot silently reopen.

💡 Suggested Change

Before:

    for arg in all_args {
        if let Some(long) = arg.get_long().filter(|long| secrets.contains(long)) {

After:

    for arg in all_args {
        assert!(arg.get_long().is_some() || arg.get_short().is_some(), "argument has neither a long nor short name");
        if let Some(long) = arg.get_long().filter(|long| secrets.contains(long)) {

[github-actions]

Repo-wide, nothing adopts env_adder_hiding_env_values or the guard assertions: none of the config modules changed in this update (scanner-daemon, web-server, node-lib, wallet-cli-lib, wallet-rpc-lib) wire them into their test suites. As shipped the helpers are dead code, and the fail-open exact-name matching means a consumer that forgets (or misspells) an entry silently leaks env secret values in --help. Either adopt the helpers in the consuming crates in this PR (pairing each env_adder_hiding_env_values call site with assert_secret_options_hidden), or defer landing the helpers until the consumers exist.

[nullPointerEnjoyer]

Pushed 1835cbd addressing the review items: the help guard builds its lookup set once, the unset marker is now a named constant next to the redaction marker, the postgres config log uses a named capture for the password part, the PostgresConfig maintenance contract is documented, skipped non-regular log entries are reported on their own info-level line, and the log permission scan is skipped under --clean-data-dir. The RedactedSummary item is a false positive: the trait has two implementors (scanner-daemon/src/config.rs:90 and web-server/src/config.rs:179) with callers in both main.rs files — those files fell outside the reviewed range.

[github-actions]

Redacting Debug here is incomplete: the Display impl for this same struct (below, unchanged) still hex-encodes and prints both stake_private_key and vrf_private_key. Any code path that formats this struct with {} or to_string() will still disclose staking key material. If the goal is to prevent key leakage in diagnostics, Display should also redact (or the redaction rationale comment should note that Display intentionally remains sensitive because it is used to serialize the setting for input parsing).

Suggestion:

Suggested change

	// 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 {
	// Consider redacting Display as well, or documenting why Display must keep the keys
	// (e.g. it is only used for round-tripping settings into CLI input, never for logging).
	impl Display for GenesisStakingSettings {
	fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
	// ...
	if !stake_private_key.is_empty() {
	values.push("stake_private_key:<redacted>".to_string())
	}
	// ...

[nullPointerEnjoyer]

Documented in 17566ee: the Display impl now carries a comment stating it is the value format of the --chain-genesis-staking-settings option (clap default rendering + FromStr round-trip), that it is an input format and never a log format, and that Debug prints the redacted form. Redacting Display itself would break the option's default-value rendering and parsing round-trip.

[github-actions]

SECRET_OPTIONS only hides env-sourced values in --help; the staking/VRF private keys inside ChainConfigOptions/chain-genesis-staking-settings still derive Debug and are not wrapped in Redacted, as the comment itself admits. Any future {:?} of the full options (e.g. re-adding the removed options dump, or debug logs elsewhere) leaks the keys. Consider wrapping the key fields in Redacted so the protection is type-level rather than convention-based.

[nullPointerEnjoyer]

This is already covered by this PR one level down: GenesisStakingSettings no longer derives Debug — common/src/chain/config/regtest.rs in this branch gives it a hand-written Debug that prints the pool id and <redacted> for both keys (with a pinning test). So any future :? of the full options renders the redacted form without wrapping the field at this layer; a Redacted wrapper here would additionally break the clap value parsing and default rendering that Display provides.

[github-actions]

The zeroize guarantee of Redacted<String> ends here: RpcCreds::basic copies the borrowed username/password into plain Strings (username.as_ref().to_owned() in rpc/src/rpc_creds.rs) that are never zeroized, and those copies live for the lifetime of the RPC server. The plaintext credential thus persists in memory regardless of this wrapper. Consider either adding zeroize to RpcCreds' storage (e.g. Zeroizing<String> or a custom Drop) or documenting that Redacted only covers debug/log redaction, not credential lifetime hygiene, so the guarantee is not overstated.

[nullPointerEnjoyer]

Correct on the mechanics; the scope is now documented on the Redacted type in 17566ee (the wrapper's zeroization covers its own copy; copies consumers make out of it are ordinary values). Widening this to RpcCreds' storage means reworking the rpc crate's credential handling, which goes beyond this changeset's redaction scope.

[github-actions]

The chmod itself follows symlinks: a path swapped in between the directory listing and set_permissions redirects the 0o600 mode change to whatever the path now references (including a symlink to another user's file). The inode comparison only detects the swap after the chmod has already been applied. If the workspace can tolerate a small platform dependency (or #[cfg(unix)] gate already present), performing fchmod on an O_NOFOLLOW-opened fd (via libc/nix, or open + File + set_permissions through the fd path) would close the race rather than merely report it.

[nullPointerEnjoyer]

This is the documented residual window: the code comment above the call explains that fully closing it needs platform-specific open flags (O_NOFOLLOW or non-blocking open), which would add a libc/nix dependency no crate in this workspace carries. The identity comparison detects and reports the swap rather than silently applying the mode to the wrong target.

[github-actions]

These info! skip messages are emitted before init_logging_generic runs (the call happens inside collect_log_permission_fix_errors, invoked above logging init), so they may be silently dropped by the logging backend. Move the skipped-entry reporting out of this function and return the list alongside permission_fix_errors, so all diagnostics are emitted after logging is initialized as the doc comment promises.

[nullPointerEnjoyer]

Fixed in 17566ee: the scan now returns the skipped entries alongside the failures, and both are reported after logging is initialized.

[github-actions]

No consumer in this changeset pairs env_adder_hiding_env_values with assert_secret_options_hidden, so the safety contract documented here ("Always pair this helper with assert_secret_options_hidden") is currently unenforced: a misspelled or renamed entry in hidden silently keeps leaking the environment-provided secret value in --help with no test failure to catch it. Consider checking the migration that introduces consumers, or strengthen the helper (e.g. make the guard the only entry point, or fail loudly in debug builds when hidden matches nothing).

[nullPointerEnjoyer]

Every consumer is paired with a guard test in this changeset: api-server/scanner-daemon/src/config.rs and api-server/web-server/src/config.rs call assert_options_classified (which subsumes assert_secret_options_hidden), node-lib/src/options.rs has secret_options_env_values_are_hidden, and both wallet crates pin their secret lists through assert_secret_options_hidden. A renamed or misspelled entry fails those tests, since the matched set must equal the list exactly.

[github-actions]

The help-output hiding for secret env values relies on SECRET_OPTIONS string matching, and RunOptions/Command still derive Debug with chain-genesis-staking-settings (carrying staking/VRF private keys) unredacted. The comment admits the options dump was removed rather than redacted, so protection against debug-formatting the staking keys rests on a single deleted log line plus the curated log message. Consider wrapping the staking-settings field (or its key accessors) in a redacted/debug-safe newtype so any future {options:?} or tracing site elsewhere in the workspace cannot print the keys by accident.

Suggestion:

Suggested change

	const SECRET_OPTIONS: &[&str] = &["rpc-username", "rpc-password", "chain-genesis-staking-settings"];
	#[derive(Subcommand, Clone, Debug)]
	pub enum Command {
	// Also give ChainConfigOptions/genesis staking settings a debug-redacting wrapper so the
	// Debug derives above cannot emit staking/VRF private keys from any future logging site.
	const SECRET_OPTIONS: &[&str] = &["rpc-username", "rpc-password", "chain-genesis-staking-settings"];
	#[derive(Subcommand, Clone, Debug)]
	pub enum Command {

[nullPointerEnjoyer]

This redaction already exists on this branch one level below the option field: GenesisStakingSettings in common/src/chain/config/regtest.rs no longer derives Debug — it has a hand-written Debug that renders the pool id and <redacted> for both stake_private_key and vrf_private_key (pinned by debug_output_omits_private_keys). Since ChainConfigOptions and Command derive Debug over that type, any future :? of the full options already prints the redacted form; no further wrapper is needed at this layer.

[nullPointerEnjoyer]

Addressed the remaining review items (fe3fa4c):

• The FromStr impl of Redacted<T> now documents that the intermediate string handed over by the parser (and the parser's own argument copies) are not zeroized, so the wipe guarantee is scoped to the wrapper's value.
• The curated options log line is now built from an exhaustive destructure of the top-level options, so adding a field forces a decision about whether it belongs in the line instead of silently disappearing from operator diagnostics.

The remaining items were already covered: the redundant #[cfg(unix)] attribute was removed earlier, and the options-Debug item was addressed by the redacted Debug impl pinned by the existing test.

[github-actions]

The comment itself acknowledges that a newly added secret-bearing option cannot be auto-classified: assert_options_classified only fails if the option is left out of BOTH lists, so a developer who blindly adds a new credential option (e.g. postgres-password-file) to NON_SECRET_OPTIONS (or to a flattened struct that only they control) leaks it via Debug logging and help output. Since the failure mode is a secret leak, consider making the helper's default conservative: any option whose long name matches a secret-like convention (e.g. contains "password", "secret", "token", "key") should be treated as secret unless explicitly pinned in NON_SECRET_OPTIONS, so forgetting to classify fails safe instead of failing open.

Suggestion:

Suggested change

	const SECRET_OPTIONS: &[&str] = &["node-rpc-username", "node-rpc-password", "postgres-password"];
	// Suggested inside clap_utils::assert_options_classified (helper consumed here):
	// let secret_like = |long: &str| {
	// ["password", "secret", "token"].iter().any(|kw| long.contains(kw))
	// };
	// // unclassified entries matching secret_like must panic unless present in secret_options
	const SECRET_OPTIONS: &[&str] = &["node-rpc-username", "node-rpc-password", "postgres-password"];

[github-actions]

chain_genesis_staking_settings uses default_value_t, so clap renders the default via Display — which deliberately includes the private keys — directly in --help for the regtest command. This is currently safe only because the defaults are compile-time constants; if the defaults ever become user-derived or the option is reused with real key material, the keys leak into help output. The SECRET_OPTIONS entry only hides environment values, not the default rendering. Consider hide_default_value(true) (or the equivalent in the hiding helper) for this option.

Suggestion:

Suggested change

	// 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.
	/// PoS Genesis staking settings
	#[clap(long, default_value_t, hide_default_value = true)]
	pub chain_genesis_staking_settings: GenesisStakingSettings,

[github-actions]

The exhaustive classification in assert_options_classified treats every long option found by collect_args as needing classification, but clap auto-generates --help (and --version) args once a Command is built (e.g. by any prior get_matches/build() call, or derive commands that build eagerly). If cmd was built before this assertion runs, help/version will show up as 'unclassified' and force callers to pad non_secret_options with framework-generated names, weakening the exhaustiveness guarantee. Filtering these well-known auto args here keeps the contract tied to the crate's own options.

Suggestion:

Suggested change

	pub fn assert_options_classified(
	cmd: &clap::Command,
	secret_options: &[&str],
	non_secret_options: &[&str],
	) {
	pub fn assert_options_classified(
	cmd: &clap::Command,
	secret_options: &[&str],
	non_secret_options: &[&str],
	) {
	// Ignore clap's auto-generated args so callers only classify real options.
	const AUTO_ARGS: [&str; 2] = ["help", "version"];

[github-actions]

The env-binding check only asserts that some variable is bound. Since add_env deterministically derives the variable name as ML_{INFIX}_{LONG}, the guard could (and arguably should) verify the exact expected id. As written, a refactor that rewires an option's env binding (e.g., a different infix or a hand-set .env(...)) would produce a bound-but-wrong variable while this assertion still passes, weakening the secret-hiding contract it exists to protect. Consider taking the infix parameter here and asserting arg.get_env() == Some(format!("ML_{infix}_{long}").to_os_string()), or at minimum checking the id contains the infix prefix.

Suggestion:

Suggested change

	assert!(
	arg.get_env().is_some(),
	"--{long} has no environment variable bound; env wiring was dropped"
	);
	let expected = format!("ML_{infix}_{long}");
	assert_eq!(
	arg.get_env().map(|v| v.to_string_lossy()),
	Some(expected.clone()),
	"--{long} is not bound to the expected environment variable {expected}"
	);