-
Notifications
You must be signed in to change notification settings - Fork 38
feat(security): use files rather than pass by arg #1818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <path>` (alternative to `--authority-seed`) | ||
| - `maintain --signing-file <path>` (alternative to `--signing`) | ||
| - `maintain contract --new-authority-file <path>` (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/<pid>/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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing PR link |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<String, sc_cli::Error> { | ||
| 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")?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Talking about security: we depend on polkadot-sdk and keystore files. Is this code really indispensable? Supporting reading from two sources will always be at most as secure as the less secure of two ways. |
||
| 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}")))?; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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/<pid>/cmdline` for its whole lifetime. Mutating the argv array here | ||
| * is invisible to the kernel: `/proc/<pid>/cmdline` is a snapshot taken at | ||
| * exec time, so the secret never appears in it. | ||
| * | ||
| * - `--signing-file <path>` -> `--signing <contents>` | ||
| * - `--new-authority-file <path>` -> `<contents>` (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()); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, string> = { | ||
| '/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']); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", "[..]"]... | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not printing file path in logs is security via obscurity. If adversary has access to the machine with seeds files, then he has access to the files regardless if these files path is printed here or not. |
||
| [..]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 <path>` | ||
| (instead of `--signing`), `--new-authority-file <path>` (instead of | ||
| `--new-authority`), and `deploy --authority-seed-file <path>` (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 | ||
|
|
||
| ``` | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing PR link