diff --git a/Cargo.lock b/Cargo.lock index 7a946c293..e8a458b52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8116,6 +8116,7 @@ dependencies = [ "serde_json", "subxt 0.50.0", "subxt-signer 0.50.0", + "tempfile", "thiserror 2.0.18", "tokio", "toml 0.9.12+spec-1.1.0", diff --git a/changes/node/changed/seed-files-hardened-reader.md b/changes/node/changed/seed-files-hardened-reader.md new file mode 100644 index 000000000..6b95fef41 --- /dev/null +++ b/changes/node/changed/seed-files-hardened-reader.md @@ -0,0 +1,14 @@ +#node #security +# Consensus seed files read via the hardened file reader + +`aura_seed_file`, `grandpa_seed_file`, and `cross_chain_seed_file` are now +read through `validated_file::safe_read_to_string` (regular-file check, +4 KiB size cap) instead of a plain `fs::read_to_string`. Symlinks remain +allowed because Kubernetes secret mounts expose files as symlinks into +`..data/`. + +The `validated_file` module moved from the node crate to +`midnight-node-ledger-helpers` so the toolkit can reuse it for its new +`--*-file` secret arguments; the node re-exports it at the old path. + +PR: diff --git a/changes/toolkit/added/seed-file-args-and-value-based-redaction.md b/changes/toolkit/added/seed-file-args-and-value-based-redaction.md new file mode 100644 index 000000000..10400085d --- /dev/null +++ b/changes/toolkit/added/seed-file-args-and-value-based-redaction.md @@ -0,0 +1,29 @@ +#toolkit #security +# File-based secret args for generate-intent and value-based log redaction + +`generate-intent` deploy/maintain now accept secrets from files, mirroring the +node's `*_seed_file` options, so key material stays off the command line +(argv is world-readable via `ps` and captured by shells and CI logs): + +- `deploy --authority-seed-file ` (alternative to `--authority-seed`) +- `maintain --signing-file ` (alternative to `--signing`) +- `maintain contract --new-authority-file ` (alternative to `--new-authority`) + +Files are read via the hardened reader (regular-file check, 4 KiB cap; +symlinks allowed for Kubernetes secret mounts), trimmed, and intermediate +buffers zeroized. + +Also fixes a debug-log key disclosure: redaction of child-process arguments +was keyed on flag names, which missed the maintain-contract new-authority +seed passed positionally. Redaction is now value-based - any argument equal +to a known secret is replaced with `[REDACTED]` - with a regression test. + +Secrets are kept out of the toolkit-js child's argv too: the parent writes +them to owner-only (0600) temp files, deleted when the child exits, and +`bin.ts` expands `--signing-file`/`--new-authority-file` into its in-memory +argv only - `/proc//cmdline` is a snapshot taken at exec, so the child's +process table entry never contains key material either. The signing-key CLI +fields are also `Debug`-redacted (`SecretString`) so `--dry-run` info logs +can't leak them. + +PR: diff --git a/ledger/helpers/Cargo.toml b/ledger/helpers/Cargo.toml index 0c675d892..e05835f80 100644 --- a/ledger/helpers/Cargo.toml +++ b/ledger/helpers/Cargo.toml @@ -51,6 +51,9 @@ serde_json.workspace = true serde_bytes = "0.11" zeroize = { workspace = true } +[dev-dependencies] +tempfile.workspace = true + [features] default = [] can-panic = [] diff --git a/ledger/helpers/src/lib.rs b/ledger/helpers/src/lib.rs index 8adf27fc2..1ee15e0d1 100644 --- a/ledger/helpers/src/lib.rs +++ b/ledger/helpers/src/lib.rs @@ -15,6 +15,7 @@ mod utils; pub use utils::find_dependency_version; pub mod extract_tx_with_context; +pub mod validated_file; /// Strategy for ordering candidate coins/UTXOs during input selection. /// diff --git a/node/src/cfg/validated_file.rs b/ledger/helpers/src/validated_file.rs similarity index 100% rename from node/src/cfg/validated_file.rs rename to ledger/helpers/src/validated_file.rs diff --git a/node/src/cfg/mod.rs b/node/src/cfg/mod.rs index c32bb2e3e..f76de1890 100644 --- a/node/src/cfg/mod.rs +++ b/node/src/cfg/mod.rs @@ -49,7 +49,7 @@ pub mod meta_cfg; pub mod midnight_cfg; pub mod storage_monitor_params_cfg; pub mod substrate_cfg; -pub mod validated_file; +pub use midnight_node_ledger_helpers::validated_file; mod validation_utils; pub mod error; diff --git a/node/src/command.rs b/node/src/command.rs index 4dd2378dc..455170b41 100644 --- a/node/src/command.rs +++ b/node/src/command.rs @@ -169,6 +169,25 @@ fn decode_genesis_state( Ok(genesis_state) } +/// Seed files hold at most a mnemonic or hex seed; 4 KiB is generous. +const SEED_FILE_MAX_SIZE: u64 = 4 * 1024; + +/// Read a consensus seed file via the hardened reader (regular-file check, size cap). +/// Symlinks are allowed because Kubernetes secret mounts expose files as symlinks +/// into `..data/`. +fn read_seed_file(seed_file: &str, label: &str) -> Result { + crate::cfg::validated_file::safe_read_to_string( + seed_file, + &crate::cfg::validated_file::SafeReadOpts { + max_size: SEED_FILE_MAX_SIZE, + unsafe_allow_symlinks: true, + }, + ) + .map_err(|e| { + sc_cli::Error::Input(format!("error when reading {label} seed file at {seed_file}: {e}")) + }) +} + fn run_node(cfg: Cfg) -> sc_cli::Result<()> { bail_if_runtime_benchmarks("running a node"); let run_midnight = RunMidnight::try_parse_from(cfg.substrate_cfg.clone().argv()) @@ -219,11 +238,7 @@ fn run_node(cfg: Cfg) -> sc_cli::Result<()> { }; if let Some(seed_file) = &cfg.midnight_cfg.aura_seed_file { - let seed = std::fs::read_to_string(seed_file).map_err(|e| { - sc_cli::Error::Input(format!( - "error when reading AURA seed file at {seed_file}. Error: {e}" - )) - })?; + let seed = read_seed_file(seed_file, "AURA")?; let seed = seed.trim(); let (keypair, _) = sp_core::sr25519::Pair::from_string_with_seed(seed, None) .map_err(|e| sc_cli::Error::Input(format!("Invalid AURA seed: {e}")))?; @@ -232,11 +247,7 @@ fn run_node(cfg: Cfg) -> sc_cli::Result<()> { } if let Some(seed_file) = &cfg.midnight_cfg.grandpa_seed_file { - let seed = std::fs::read_to_string(seed_file).map_err(|e| { - sc_cli::Error::Input(format!( - "error when reading GRANDPA seed file at {seed_file}. Error: {e}" - )) - })?; + let seed = read_seed_file(seed_file, "GRANDPA")?; let seed = seed.trim(); let (keypair, _) = sp_core::ed25519::Pair::from_string_with_seed(seed, None) .map_err(|e| sc_cli::Error::Input(format!("Invalid GRANDPA seed: {e}")))?; @@ -245,11 +256,7 @@ fn run_node(cfg: Cfg) -> sc_cli::Result<()> { } if let Some(seed_file) = &cfg.midnight_cfg.cross_chain_seed_file { - let seed = std::fs::read_to_string(seed_file).map_err(|e| { - sc_cli::Error::Input(format!( - "error when reading CROSS_CHAIN seed file at {seed_file}. Error: {e}" - )) - })?; + let seed = read_seed_file(seed_file, "CROSS_CHAIN")?; let seed = seed.trim(); let (keypair, _) = sp_core::ecdsa::Pair::from_string_with_seed(seed, None) .map_err(|e| sc_cli::Error::Input(format!("Invalid CROSS_CHAIN seed: {e}")))?; diff --git a/util/toolkit-js/src/bin.ts b/util/toolkit-js/src/bin.ts index d7f1aa143..2f794154d 100644 --- a/util/toolkit-js/src/bin.ts +++ b/util/toolkit-js/src/bin.ts @@ -1,6 +1,14 @@ #!/usr/bin/env node +import { readFileSync } from 'node:fs'; + import { installCompactcResolver, resolveCompactcVersion } from './compactc-resolver.js'; +import { expandSecretFileArgs } from './secret-args.js'; + +// Secrets arrive via temp files, never argv (see secret-args.ts). Expand them +// into process.argv before the CLI parses it; the kernel-visible cmdline is +// unaffected. +expandSecretFileArgs(process.argv, (path) => readFileSync(path, 'utf8')); // Determine the supported variant for the current COMPACTC_VERSION and install the resolution hook that // redirects `compact-js*` / `compact-runtime` imports to that variant's pinned copies. diff --git a/util/toolkit-js/src/secret-args.ts b/util/toolkit-js/src/secret-args.ts new file mode 100644 index 000000000..dec53f515 --- /dev/null +++ b/util/toolkit-js/src/secret-args.ts @@ -0,0 +1,37 @@ +// This file is part of midnight-node. +// Copyright (C) Midnight Foundation +// SPDX-License-Identifier: Apache-2.0 +// Licensed under the Apache License, Version 2.0 (the "License"); +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/** + * Expand secret-file flags into in-memory argv values. + * + * The Rust toolkit hands secrets over via owner-only temp files instead of + * argv, because a process's argv is world-readable on Linux via + * `/proc//cmdline` for its whole lifetime. Mutating the argv array here + * is invisible to the kernel: `/proc//cmdline` is a snapshot taken at + * exec time, so the secret never appears in it. + * + * - `--signing-file ` -> `--signing ` + * - `--new-authority-file ` -> `` (positional, in place, as + * maintain-contract takes the new authority positionally) + */ +export function expandSecretFileArgs(argv: string[], readSecret: (path: string) => string): void { + // Backwards, so splices don't shift unvisited indices; each flag consumes + // the element after it, hence `length - 2`. + for (let i = argv.length - 2; i >= 0; i--) { + if (argv[i] === '--signing-file') { + argv.splice(i, 2, '--signing', readSecret(argv[i + 1]).trim()); + } else if (argv[i] === '--new-authority-file') { + argv.splice(i, 2, readSecret(argv[i + 1]).trim()); + } + } +} diff --git a/util/toolkit-js/test/SecretArgs.test.ts b/util/toolkit-js/test/SecretArgs.test.ts new file mode 100644 index 000000000..73f91a45f --- /dev/null +++ b/util/toolkit-js/test/SecretArgs.test.ts @@ -0,0 +1,52 @@ +// This file is part of midnight-node. +// Copyright (C) Midnight Foundation +// SPDX-License-Identifier: Apache-2.0 +// Licensed under the Apache License, Version 2.0 (the "License"); +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, expect, it } from 'vitest'; + +import { expandSecretFileArgs } from '../src/secret-args.js'; + +const SECRETS: Record = { + '/tmp/signing': 'deadbeef01\n', + '/tmp/authority': 'cafebabe02\n', +}; +const readSecret = (path: string): string => { + const secret = SECRETS[path]; + if (secret === undefined) throw new Error(`unexpected path ${path}`); + return secret; +}; + +describe('expandSecretFileArgs', () => { + it('expands --signing-file into --signing with trimmed file contents', () => { + const argv = ['node', 'bin.js', 'maintain', 'circuit', '--signing-file', '/tmp/signing', 'addr', 'my_circuit']; + expandSecretFileArgs(argv, readSecret); + expect(argv).toEqual(['node', 'bin.js', 'maintain', 'circuit', '--signing', 'deadbeef01', 'addr', 'my_circuit']); + }); + + it('expands --new-authority-file positionally, in place', () => { + const argv = ['node', 'bin.js', 'maintain', 'contract', '--signing-file', '/tmp/signing', 'addr', '--new-authority-file', '/tmp/authority']; + expandSecretFileArgs(argv, readSecret); + expect(argv).toEqual(['node', 'bin.js', 'maintain', 'contract', '--signing', 'deadbeef01', 'addr', 'cafebabe02']); + }); + + it('leaves argv without secret-file flags untouched', () => { + const argv = ['node', 'bin.js', 'deploy', '-c', 'config.ts', '--coin-public', 'aabb']; + expandSecretFileArgs(argv, readSecret); + expect(argv).toEqual(['node', 'bin.js', 'deploy', '-c', 'config.ts', '--coin-public', 'aabb']); + }); + + it('ignores a trailing flag with no value (CLI will reject it)', () => { + const argv = ['node', 'bin.js', 'maintain', 'circuit', '--signing-file']; + expandSecretFileArgs(argv, readSecret); + expect(argv).toEqual(['node', 'bin.js', 'maintain', 'circuit', '--signing-file']); + }); +}); diff --git a/util/toolkit/README.md b/util/toolkit/README.md index 15b958932..1ff2e484b 100644 --- a/util/toolkit/README.md +++ b/util/toolkit/README.md @@ -445,14 +445,20 @@ $ midnight-node-toolkit generate-intent maintain-contract > --contract-address 3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806 > --output-intent out/intent.bin > --signing 0000000000000000000000000000000000000000000000000000000000000001 -> 0000000000000000000000000000000000000000000000000000000000000002 +> --new-authority 0000000000000000000000000000000000000000000000000000000000000002 [..]Executing generate-intent[..] [..]Executing maintain command -[..]Executing ../toolkit-js/dist/bin.js with arguments: ["maintain", "contract", "-c", "[CWD]/../toolkit-js/test/contract/contract.config.ts", "--network", "undeployed", "--coin-public", "aa0d72bb77ea46f986a800c66d75c4e428a95bd7e1244f1ed059374e6266eb98", "--input", "[CWD]/test-data/contract/counter/contract_state.mn", "--output", "[CWD]/out/intent.bin", "--signing", "0000000000000000000000000000000000000000000000000000000000000001", "3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806", "0000000000000000000000000000000000000000000000000000000000000002"]... +[..]Executing ../toolkit-js/dist/bin.js with arguments: ["maintain", "contract", "-c", "[CWD]/../toolkit-js/test/contract/contract.config.ts", "--network", "undeployed", "--coin-public", "aa0d72bb77ea46f986a800c66d75c4e428a95bd7e1244f1ed059374e6266eb98", "--input", "[CWD]/test-data/contract/counter/contract_state.mn", "--output", "[CWD]/out/intent.bin", "--signing-file", "[..]", "3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806", "--new-authority-file", "[..]"]... [..]written: out/intent.bin ``` +Secrets can also be read from files instead of inline values, keeping key +material out of argv, shell history, and CI logs: `--signing-file ` +(instead of `--signing`), `--new-authority-file ` (instead of +`--new-authority`), and `deploy --authority-seed-file ` (instead of +`--authority-seed`). + To send it, see "Generate and send a tx from an intent" above - Generate a circuit maintenance intent @@ -469,7 +475,7 @@ $ midnight-node-toolkit generate-intent maintain-circuit > ./test-data/contract/counter/keys/increment.verifier [..]Executing generate-intent[..] [..]Executing maintain command -[..]Executing ../toolkit-js/dist/bin.js with arguments: ["maintain", "circuit", "-c", "[CWD]/../toolkit-js/test/contract/contract.config.ts", "--network", "undeployed", "--coin-public", "aa0d72bb77ea46f986a800c66d75c4e428a95bd7e1244f1ed059374e6266eb98", "--input", "[CWD]/test-data/contract/counter/contract_state.mn", "--output", "[CWD]/out/intent.bin", "--signing", "0000000000000000000000000000000000000000000000000000000000000001", "3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806", "increment", "[CWD]/test-data/contract/counter/keys/increment.verifier"]... +[..]Executing ../toolkit-js/dist/bin.js with arguments: ["maintain", "circuit", "-c", "[CWD]/../toolkit-js/test/contract/contract.config.ts", "--network", "undeployed", "--coin-public", "aa0d72bb77ea46f986a800c66d75c4e428a95bd7e1244f1ed059374e6266eb98", "--input", "[CWD]/test-data/contract/counter/contract_state.mn", "--output", "[CWD]/out/intent.bin", "--signing-file", "[..]", "3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806", "increment", "[CWD]/test-data/contract/counter/keys/increment.verifier"]... [..]written: out/intent.bin ``` diff --git a/util/toolkit/src/cli_parsers.rs b/util/toolkit/src/cli_parsers.rs index bddd8c217..b2ceb920b 100644 --- a/util/toolkit/src/cli_parsers.rs +++ b/util/toolkit/src/cli_parsers.rs @@ -15,6 +15,7 @@ use std::str::FromStr; use midnight_node_ledger_helpers::*; use serde::Deserialize; +use zeroize::Zeroize; use crate::tx_generator::source::FetchCacheConfig; @@ -70,6 +71,74 @@ pub fn wallet_seed_decode(input: &str) -> Result }) } +/// Cap for secret files (hex seed, mnemonic, or signing key); 4 KiB is generous. +const SECRET_FILE_MAX_SIZE: u64 = 4 * 1024; + +/// A secret string (e.g. a hex signing key). `Debug` and `Display` are +/// redacted so formatting (including `derive(Debug)` on arg structs) can't +/// leak it into logs, and the value is zeroized on drop. Access the value +/// explicitly via `Deref` (`&*secret`). +#[derive(Clone, Zeroize, zeroize::ZeroizeOnDrop)] +pub struct SecretString(String); + +impl core::fmt::Debug for SecretString { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str("REDACTED") + } +} + +impl core::fmt::Display for SecretString { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str("REDACTED") + } +} + +impl core::ops::Deref for SecretString { + type Target = str; + fn deref(&self) -> &str { + &self.0 + } +} + +/// Parse an inline secret argument value into a [`SecretString`]. +pub fn secret_string_decode(input: &str) -> Result { + Ok(SecretString(input.to_string())) +} + +fn clap_value_error(msg: String) -> clap::error::Error { + let mut err = clap::Error::new(clap::error::ErrorKind::ValueValidation); + err.insert(clap::error::ContextKind::Custom, clap::error::ContextValue::String(msg)); + err +} + +/// Read a secret from a file so it never appears on a command line (argv is +/// world-readable via `ps`, and shells/CI log it). Mirrors the node's +/// `*_seed_file` options. Symlinks are allowed because Kubernetes secret +/// mounts expose files as symlinks into `..data/`. +pub fn secret_string_from_file(path: &str) -> Result { + let mut raw = validated_file::safe_read_to_string( + path, + &validated_file::SafeReadOpts { + max_size: SECRET_FILE_MAX_SIZE, + unsafe_allow_symlinks: true, + }, + ) + .map_err(clap_value_error)?; + let secret = SecretString(raw.trim().to_string()); + raw.zeroize(); + Ok(secret) +} + +/// Parse a [`WalletSeed`] from a file path argument (see [`secret_string_from_file`]). +pub fn wallet_seed_from_file(path: &str) -> Result { + let mut secret = secret_string_from_file(path)?; + let seed = secret + .parse() + .map_err(|e| clap_value_error(format!("failed to parse seed from file '{path}': {e}"))); + secret.zeroize(); + seed +} + pub fn keypair_from_str(input: &str) -> Result { input.parse().map_err(|e| { let mut err = clap::Error::new(clap::error::ErrorKind::ValueValidation); @@ -278,6 +347,41 @@ pub fn semver_decode(input: &str) -> Result { mod tests { use super::*; + // `wallet_seed_from_file` / `secret_string_from_file` — secrets stay off argv. + + /// `derive(Debug)` on arg structs holding a [`SecretString`] must not leak + /// the value (the dry-run info logs Debug-format whole arg structs). + #[test] + fn secret_string_debug_is_redacted() { + let secret = secret_string_decode("deadbeef").unwrap(); + assert_eq!(format!("{secret:?}"), "REDACTED"); + assert_eq!(format!("{secret}"), "REDACTED"); + assert_eq!(format!("{:?}", Some(&secret)), "Some(REDACTED)"); + assert_eq!(&*secret, "deadbeef"); + } + + #[test] + fn wallet_seed_from_file_reads_and_trims() { + use std::io::Write; + let mut f = tempfile::NamedTempFile::new().unwrap(); + writeln!(f, "{}", "11".repeat(32)).unwrap(); + let seed = wallet_seed_from_file(f.path().to_str().unwrap()).unwrap(); + assert_eq!(seed, wallet_seed_decode(&"11".repeat(32)).unwrap()); + } + + #[test] + fn wallet_seed_from_file_rejects_garbage() { + use std::io::Write; + let mut f = tempfile::NamedTempFile::new().unwrap(); + writeln!(f, "not a seed").unwrap(); + assert!(wallet_seed_from_file(f.path().to_str().unwrap()).is_err()); + } + + #[test] + fn secret_string_from_file_rejects_missing_file() { + assert!(secret_string_from_file("/nonexistent/secret").is_err()); + } + // `coin_public_decode` — untagged per ADR-0022. #[test] diff --git a/util/toolkit/src/toolkit_js/mod.rs b/util/toolkit/src/toolkit_js/mod.rs index e261bdd65..6992c227e 100644 --- a/util/toolkit/src/toolkit_js/mod.rs +++ b/util/toolkit/src/toolkit_js/mod.rs @@ -1,4 +1,7 @@ -use std::{io::BufRead, path::PathBuf}; +use std::{ + io::{BufRead, Write}, + path::PathBuf, +}; use clap::{ Args, Subcommand, @@ -8,7 +11,7 @@ use hex::ToHex; use midnight_node_ledger_helpers::{ CoinPublicKey, ContractAddress, UnshieldedWallet, WalletSeed, serialize_untagged, }; -use zeroize::Zeroize; +use zeroize::{Zeroize, Zeroizing}; pub(crate) mod encoded_zswap_local_state; pub use encoded_zswap_local_state::{EncodedOutput, EncodedZswapLocalState}; @@ -124,8 +127,11 @@ pub struct DeployArgs { #[arg(long, value_parser = cli::coin_public_decode)] pub coin_public: CoinPublicKey, /// Contract maintenance authority seed. - #[arg(long, value_parser = cli::wallet_seed_decode)] + #[arg(long, value_parser = cli::wallet_seed_decode, conflicts_with = "authority_seed_file")] pub authority_seed: Option, + /// Read the contract maintenance authority seed from a file (keeps the secret off argv). + #[arg(long, value_parser = cli::wallet_seed_from_file)] + pub authority_seed_file: Option, /// The output file of the intent #[arg(long, value_parser = PathBufValueParser::new().map(|p| RelativePath::from(p)))] pub output_intent: RelativePath, @@ -153,9 +159,12 @@ pub struct SharedMaintainArgs { /// A user public key capable of receiving Zswap coins, hex or Bech32m encoded. #[arg(long, value_parser = cli::coin_public_decode)] coin_public: CoinPublicKey, - /// A public BIP-340 signing key, hex encoded. - #[arg(long)] - signing: Option, + /// A BIP-340 signing key, hex encoded. Treated as secret: redacted from logs. + #[arg(long, conflicts_with = "signing_file", value_parser = cli::secret_string_decode)] + signing: Option, + /// Read the BIP-340 signing key from a file (keeps the secret off argv). + #[arg(long, value_parser = cli::secret_string_from_file)] + signing_file: Option, /// Input file containing the current on-chain circuit state #[arg(long, value_parser = PathBufValueParser::new().map(|p| RelativePath::from(p)))] input_onchain_state: RelativePath, @@ -168,9 +177,18 @@ pub struct SharedMaintainArgs { pub struct MaintainContractArgs { #[command(flatten)] shared: SharedMaintainArgs, - #[arg(long, value_parser = cli::wallet_seed_decode)] - /// A public BIP-340 signing key, hex encoded. Replaces the signing key for the contract. - new_authority: WalletSeed, + /// Seed of the new contract maintenance authority (secret: redacted from logs). + /// Replaces the signing key for the contract. + #[arg( + long, + value_parser = cli::wallet_seed_decode, + required_unless_present = "new_authority_file", + conflicts_with = "new_authority_file" + )] + new_authority: Option, + /// Read the new contract maintenance authority seed from a file (keeps the secret off argv). + #[arg(long, value_parser = cli::wallet_seed_from_file)] + new_authority_file: Option, } #[derive(Args, Debug)] @@ -261,24 +279,26 @@ impl ToolkitJs { cmd_args.extend_from_slice(&["--network", &args.network]); } - let mut signing_key = args + // `Zeroizing` clears the hex key even when an early `?` returns. + let signing_key = args .authority_seed + .or(args.authority_seed_file) .map(|s| { let mut bytes = serialize_untagged(UnshieldedWallet::default(s).signing_key()) .map_err(ToolkitJsError::ExecutionError)?; - let hex = bytes.encode_hex::(); + let hex = Zeroizing::new(bytes.encode_hex::()); bytes.zeroize(); - Ok::(hex) + Ok::, ToolkitJsError>(hex) }) .transpose()?; - if let Some(ref key) = signing_key { - cmd_args.extend_from_slice(&["--signing", key]); + let signing_tmp = signing_key.as_deref().map(|s| secret_temp_file(s)).transpose()?; + if let Some((_, ref path)) = signing_tmp { + cmd_args.extend_from_slice(&["--signing-file", path]); } // Add positional args cmd_args.extend(args.constructor_args.iter().map(|s| s.as_str())); - let result = self.execute_js(&cmd_args); - signing_key.as_mut().map(|s| s.zeroize()); - result?; + let secrets: Vec<&str> = signing_key.as_deref().map(|s| s.as_str()).into_iter().collect(); + self.execute_js(&cmd_args, &secrets)?; log::info!( "written: {}, {}, {}", args.output_intent, @@ -341,7 +361,7 @@ impl ToolkitJs { // Add positional args cmd_args.extend_from_slice(&[&contract_address_str, &args.circuit_id]); cmd_args.extend(args.call_args.iter().map(|s| s.as_str())); - self.execute_js(&cmd_args)?; + self.execute_js(&cmd_args, &[])?; log::info!( "written: {}, {}, {}", args.output_intent, @@ -375,19 +395,33 @@ impl ToolkitJs { cmd_args.extend_from_slice(&["--network", &args.network]); } - if let Some(ref signing) = args.signing { - cmd_args.extend_from_slice(&["--signing", signing]); + let signing = args.signing.as_deref().or(args.signing_file.as_deref()); + let signing_tmp = signing.map(secret_temp_file).transpose()?; + if let Some((_, ref path)) = signing_tmp { + cmd_args.extend_from_slice(&["--signing-file", path]); } // Add positional args cmd_args.push(&contract_address_str); - let mut new_authority = match &command { - MaintainCommand::Contract(MaintainContractArgs { new_authority, .. }) => { - Some(new_authority.as_bytes().encode_hex::()) + // `Zeroizing` clears the hex seed even when an early `?` returns. + let new_authority = match &command { + MaintainCommand::Contract(MaintainContractArgs { + new_authority, + new_authority_file, + .. + }) => { + let seed = new_authority.as_ref().or(new_authority_file.as_ref()).expect( + "clap enforces one of --new-authority/--new-authority-file being present", + ); + Some(Zeroizing::new(seed.as_bytes().encode_hex::())) }, _ => None, }; - if let Some(ref new_authority) = new_authority { - cmd_args.push(new_authority) + // bin.ts expands this back into the positional new-authority value, so + // it must sit exactly where the positional would (after the address). + let new_authority_tmp = + new_authority.as_deref().map(|s| secret_temp_file(s)).transpose()?; + if let Some((_, ref path)) = new_authority_tmp { + cmd_args.extend_from_slice(&["--new-authority-file", path]); } if let MaintainCommand::Circuit(args) = &command { cmd_args.push(&args.circuit_id); @@ -395,33 +429,24 @@ impl ToolkitJs { cmd_args.push(&vk_path); } } - let result = self.execute_js(&cmd_args); - new_authority.as_mut().map(|s| s.zeroize()); - result?; + let secrets: Vec<&str> = signing + .into_iter() + .chain(new_authority.as_deref().map(|s| s.as_str())) + .collect(); + self.execute_js(&cmd_args, &secrets)?; log::info!("written: {}", args.output_intent); Ok(()) } - fn execute_js(&self, args: &[&str]) -> Result<(), ToolkitJsError> { + /// `secrets` lists argument values that must never reach the logs. In normal + /// operation secrets travel via temp files (see [`secret_temp_file`]) and + /// never appear in `args` at all; the value-based redaction here is a + /// backstop against future code reintroducing inline secrets. + fn execute_js(&self, args: &[&str], secrets: &[&str]) -> Result<(), ToolkitJsError> { let cmd = PathBuf::from(&self.path).join(BUILD_DIST).to_string_lossy().to_string(); log::info!("Executing {cmd}..."); if log::log_enabled!(log::Level::Debug) { - let redacted_args: Vec<&str> = { - let mut result = Vec::with_capacity(args.len()); - let mut redact_next = false; - for &arg in args { - if redact_next { - result.push("[REDACTED]"); - redact_next = false; - } else if arg == "--signing" || arg == "--new-authority" { - result.push(arg); - redact_next = true; - } else { - result.push(arg); - } - } - result - }; + let redacted_args = redact_args(args, secrets); log::debug!("Executing {cmd} with arguments: {redacted_args:?}..."); } @@ -462,3 +487,69 @@ impl ToolkitJs { Ok(()) } } + +/// Hand a secret to toolkit-js via an owner-only temp file instead of argv +/// (0600 on Unix — the `tempfile` crate's default; on other platforms it is +/// whatever `NamedTempFile` provides): a child's argv is world-readable on +/// Linux via `/proc//cmdline` +/// for its whole (ZK-slow) lifetime. `bin.ts` expands the `*-file` flag back +/// into the in-memory argv, which the kernel's cmdline snapshot never sees. +/// The file is deleted when the returned handle drops, so keep it alive until +/// the child has exited. +fn secret_temp_file(secret: &str) -> Result<(tempfile::NamedTempFile, String), ToolkitJsError> { + let mut file = tempfile::NamedTempFile::new().map_err(ToolkitJsError::ExecutionError)?; + file.write_all(secret.as_bytes()).map_err(ToolkitJsError::ExecutionError)?; + file.flush().map_err(ToolkitJsError::ExecutionError)?; + let path = file.path().to_string_lossy().into_owned(); + Ok((file, path)) +} + +/// Replace every argument that exactly matches a secret with `[REDACTED]`. +fn redact_args<'a>(args: &[&'a str], secrets: &[&str]) -> Vec<&'a str> { + args.iter() + .map(|&arg| if secrets.contains(&arg) { "[REDACTED]" } else { arg }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + const SECRET: &str = "9f2d3a4b5c6d7e8f9f2d3a4b5c6d7e8f9f2d3a4b5c6d7e8f9f2d3a4b5c6d7e8f"; + + /// Regression test for the April key-disclosure report. Production code no + /// longer puts secrets in the child argv at all (temp-file handoff), so + /// this guards the backstop: if inline secrets are ever reintroduced, + /// redaction must catch them both as flag values (`--signing `) and + /// positionally (maintain-contract's new authority). + #[test] + fn redacts_flagged_and_positional_secrets() { + let args = ["maintain", "contract", "--signing", SECRET, "0011aabb", SECRET, "my_circuit"]; + let redacted = redact_args(&args, &[SECRET]); + let rendered = format!("{redacted:?}"); + assert!(!rendered.contains(SECRET), "secret leaked into log output: {rendered}"); + assert_eq!(redacted.iter().filter(|a| **a == "[REDACTED]").count(), 2); + } + + /// Secrets travel to toolkit-js via owner-only temp files, never argv. + #[test] + fn secret_temp_file_is_owner_only_and_deleted_on_drop() { + let (file, path) = secret_temp_file(SECRET).unwrap(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), SECRET); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "secret temp file must be owner-only"); + } + drop(file); + assert!(!std::path::Path::new(&path).exists(), "temp file must vanish on drop"); + } + + #[test] + fn non_secrets_pass_through_unchanged() { + let args = ["deploy", "-c", "config.ts", "--coin-public", "aabbccdd"]; + assert_eq!(redact_args(&args, &[]), args); + assert_eq!(redact_args(&args, &[SECRET]), args); + } +} diff --git a/util/toolkit/tests/common/toolkit_helper.rs b/util/toolkit/tests/common/toolkit_helper.rs index c71e551f5..55686352b 100644 --- a/util/toolkit/tests/common/toolkit_helper.rs +++ b/util/toolkit/tests/common/toolkit_helper.rs @@ -340,6 +340,7 @@ impl ToolkitTestHelper { coin_public: cli_parsers::coin_public_decode(coin_public) .expect("invalid coin public key"), authority_seed: None, + authority_seed_file: None, output_intent: RelativePath(intent.clone()), output_private_state: RelativePath(private_state.clone()), output_zswap_state: RelativePath(zswap_state.clone()),