security: redact private keys and add reveal-password gate for key:get / key:list#39
security: redact private keys and add reveal-password gate for key:get / key:list#39paulgnz wants to merge 6 commits intoXPRNetwork:masterfrom
Conversation
key:list now shows only public keys with their associated accounts (resolved via get_accounts_by_authorizers). Private keys require --reveal-private plus a typed "I UNDERSTAND" confirmation. key:get now also requires the typed confirmation before printing a private key. Both commands refuse to print private keys when stdout or stdin is not a TTY (prevents accidental leaks via pipes, redirects, or CI logs). Pass --force to bypass the prompt in trusted interactive contexts. Previously "key:list" would dump every stored private key to the terminal with no warning or opt-in, which is dangerous during screen shares, pair programming, or recorded sessions.
get_accounts_by_authorizers returns authorizing_key in legacy EOS format (EOS...), but public keys were mapped by the modern K1 form (PUB_K1_...). String-equality lookups missed every entry, leaving accounts: [] for every key. Normalize via Key.PublicKey.fromString ().toString() so both formats resolve to the canonical modern form.
Introduces a separate reveal password, held only by the user, that
gates private-key disclosure in the CLI. Daily operations (signing
transactions, listing public keys) are unaffected, so AI agents and
scripts can continue to run normal commands without friction.
New commands:
- proton key:reveal-setup Set or change the reveal password.
Stored as a salted scrypt hash in config;
plaintext never written to disk.
- proton key:reveal-disable Remove the reveal password (requires
entering the current one).
Changed commands:
- proton key:get If a reveal password is set, prompts for
it and verifies (scrypt, constant-time
compare) before printing. If unset, keeps
the typed "I UNDERSTAND" confirmation and
hints the user toward key:reveal-setup.
- proton key:list -r Same gating as key:get.
Removed:
- --force flag on both commands. It was the escape hatch an AI agent
used to bypass the typed confirmation on its own accord. The reveal
password is the intentional opt-in; no "skip all prompts" switch.
The non-TTY refusal is preserved: neither command will print a private
key when stdout or stdin is not a terminal.
Known limitation: the reveal password gates the CLI path only. A
process running as the user with filesystem access can still read
unencrypted keys from the config directly. For full at-rest protection
use key:lock or a follow-up Keychain-backed store.
paulgnz
changed the title
scrypt with N=2^15, r=8 needs ~32 MiB of RAM per call, which lands right at Node's default maxmem. Some builds round up and throw "memory limit exceeded". Explicitly pass maxmem=128 MiB on both hash and verify so the call always succeeds; the cost/security params are unchanged.
There was a problem hiding this comment.
Pull request overview
This PR hardens Proton CLI key-reveal workflows to reduce accidental/private-key exfiltration, especially when commands are run by AI agents or in non-interactive contexts.
Changes:
- Redacts private keys from the default
proton key:listoutput and adds--reveal-privategating. - Enforces TTY-only behavior for any private-key-printing path (
key:get,key:list --reveal-private) and adds explicit user confirmation / optional reveal-password prompting. - Introduces reveal-password setup/disable commands and persists the (salted, scrypt) verifier in CLI config; updates README command docs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/revealPassword.ts | Adds scrypt-based reveal-password hashing/verification + prompt gate. |
| src/storage/config.ts | Extends config typing to include stored reveal-password verifier. |
| src/commands/key/reveal-setup.ts | New command to set/change reveal password. |
| src/commands/key/reveal-disable.ts | New command to remove reveal password (requires current). |
| src/commands/key/list.ts | Changes default output to public keys + account resolution; adds --reveal-private gating + TTY enforcement. |
| src/commands/key/get.ts | Adds TTY enforcement + confirmation/reveal-password gate before printing a private key. |
| README.md | Documents new commands and updated key:get / key:list behavior/flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const salt = Buffer.from(stored.salt, 'hex') | ||
| const expected = Buffer.from(stored.hash, 'hex') | ||
| const actual = scryptSync(password, salt, stored.keyLen, { N: stored.N, r: stored.r, p: stored.p, maxmem: SCRYPT_MAXMEM }) | ||
| if (actual.length !== expected.length) return false | ||
| return timingSafeEqual(actual, expected) |
There was a problem hiding this comment.
verifyRevealPassword feeds stored.N/r/p/keyLen from the on-disk config directly into crypto.scryptSync. If the config file is tampered/corrupted, those values can trigger extremely slow hashing or excessive memory usage (local DoS) or throw. Consider either (a) not storing parameters at all and always verifying with the fixed SCRYPT_PARAMS, or (b) strictly validating/clamping the stored params to safe bounds (and treating out-of-policy values as an invalid/disabled reveal password).
| const salt = Buffer.from(stored.salt, 'hex') | |
| const expected = Buffer.from(stored.hash, 'hex') | |
| const actual = scryptSync(password, salt, stored.keyLen, { N: stored.N, r: stored.r, p: stored.p, maxmem: SCRYPT_MAXMEM }) | |
| if (actual.length !== expected.length) return false | |
| return timingSafeEqual(actual, expected) | |
| try { | |
| const salt = Buffer.from(stored.salt, 'hex') | |
| const expected = Buffer.from(stored.hash, 'hex') | |
| const { N, r, p, keyLen } = SCRYPT_PARAMS | |
| const actual = scryptSync(password, salt, keyLen, { N, r, p, maxmem: SCRYPT_MAXMEM }) | |
| if (actual.length !== expected.length) return false | |
| return timingSafeEqual(actual, expected) | |
| } catch { | |
| return false | |
| } |
| if (!verifyRevealPassword(input, stored)) { | ||
| CliUx.ux.error('Reveal password is incorrect.') |
There was a problem hiding this comment.
requireRevealPassword/verifyRevealPassword can throw (e.g., invalid hex in stored.salt/hash, invalid keyLen, or scrypt parameter errors). Since key:get/key:list don’t override catch(), this may surface as an unhandled exception/stack trace. Wrapping the verification path in a try/catch and emitting a clear remediation message (e.g. config corrupted; re-run proton key:reveal-setup or proton key:reveal-disable) would make the failure mode safer and more user-friendly.
| if (!verifyRevealPassword(input, stored)) { | |
| CliUx.ux.error('Reveal password is incorrect.') | |
| try { | |
| if (!verifyRevealPassword(input, stored)) { | |
| CliUx.ux.error('Reveal password is incorrect.') | |
| } | |
| } catch { | |
| CliUx.ux.error( | |
| 'Stored reveal password configuration is invalid or corrupted. Re-run `proton key:reveal-setup` to set it up again, or `proton key:reveal-disable` to remove it.' | |
| ) |
| } | ||
|
|
||
| export function clearRevealPasswordHash(): void { | ||
| config.delete('revealPasswordHash' as any) |
There was a problem hiding this comment.
clearRevealPasswordHash uses config.delete('revealPasswordHash' as any), which weakens type safety and can hide real key-name mistakes. Prefer keeping this typed (e.g. config.delete('revealPasswordHash')) and, if needed, adjust the Conf typing/wrapper so delete accepts the config keys without an any cast.
| config.delete('revealPasswordHash' as any) | |
| config.delete('revealPasswordHash') |
| export interface RevealPasswordHash { | ||
| salt: string; | ||
| hash: string; | ||
| N: number; | ||
| r: number; | ||
| p: number; | ||
| keyLen: number; | ||
| } | ||
|
|
||
| export const config = new Conf<{ | ||
| privateKeys: string[]; | ||
| isLocked: boolean; | ||
| tryKeychain: boolean; | ||
| networks: { chain: string; endpoints: string[] }[]; | ||
| currentChain: string; | ||
| endpoints?: { chain: string; endpoints: string[] }[]; | ||
| revealPasswordHash?: RevealPasswordHash; | ||
| }>({ | ||
| schema, |
There was a problem hiding this comment.
revealPasswordHash is added to the Conf type, but there’s no corresponding entry in the schema object. Given this is security-sensitive state, consider defining a schema for it (object with required salt/hash/N/r/p/keyLen fields) so malformed config values are rejected early instead of causing runtime errors during password verification.
| import { network } from '../../storage/networks' | ||
| import { isRevealPasswordSet, requireRevealPassword } from '../../storage/revealPassword' | ||
|
|
||
| const CONFIRMATION_PHRASE = 'I UNDERSTAND' |
There was a problem hiding this comment.
This phrase duplicates in get.ts. Can we share it with both commands? Like import it from another file.
…arden config Restore --force flag on key:get and key:list (per @roman): Scripts need a way to bypass interactive prompts. The flag now skips the typed "I UNDERSTAND" confirmation and the TTY refusal, but it does NOT skip the reveal password if one is set — the reveal password remains the agent-resistant gate. Update default RPC endpoints in src/constants.ts (per @roman): The previous endpoints did not implement get_accounts_by_authorizers, causing key:list account resolution to error out. Replaced with endpoints that support it: proton: rpc.api.mainnet.metalx.com, proton.cryptolions.io, proton.eosusa.io proton-test: rpc.api.testnet.metalx.com, proton-testnet.eoscafeblock.com, test.proton.eosusa.io Address Copilot review comments: - Validate stored scrypt parameters (N power-of-two and bounded, r/p/keyLen bounded, salt/hash hex-shaped and bounded). Rejects tampered config values that could otherwise trigger local DoS via extreme memory/cost factors. - Wrap verifyRevealPassword in requireRevealPassword with a try/catch that emits a clear remediation message ("run key:reveal-disable then key:reveal-setup") instead of an unhandled stack trace. - Drop the `as any` from clearRevealPasswordHash now that revealPasswordHash is in the Conf type generic. - Add a JSON Schema entry for revealPasswordHash so malformed values are rejected by Conf at load time. - Extract the shared CONFIRMATION_PHRASE into src/storage/confirmation.ts; both key:get and key:list import it. README: document the restored --force flag and its non-bypass of the reveal password.
|
Thanks for the review @andreyjamer (and Copilot) — all feedback addressed in Dev review:
Copilot review comments:
Re: future work (test suite, oclif upgrade) — happy to take that on as separate PRs once this lands. The hardening here is a foundation; tests around key handling specifically would be a great next step. |
Summary
Three layers of protection for the CLI's private-key commands, aimed at AI coding agents that can run shell commands on a developer's machine:
key:listdefault no longer prints private keys. Shows public keys + the accounts/permissions each key authorizes (resolved viaget_accounts_by_authorizers).key:getandkey:list --reveal-privateare TTY-only. Piped, redirected, or CI contexts are refused — no--forceescape hatch.proton key:reveal-setup). When set, a second password held only in the user's head is required to reveal any private key via the CLI. The daily workflow (signing transactions, listing public keys) is unaffected, so agents and scripts keep working.Motivation
proton key:listpreviously dumped every stored private key to stdout with no warning. In an AI-agent world this is a routine exposure risk:protoncommands on a developer's behalf to inspect state. An agent that runsproton key:listto "see what accounts are available" ingests every private key into its context window, which is then transmitted to a third-party LLM provider (and retained per their data policy), written to local transcript files in plaintext (often cloud-synced), exposed to any subsequent prompt injection in the session, and potentially echoed back in summaries, commit messages, or PR descriptions. Many users run agents with bash auto-approval, so this happens with no human-in-the-loop check.key:listlooks like a safe read-only status command, so no model hesitates to run it.proton key:list > keys.txt,| tee debug.log, scrollback buffers, and shell history files all quietly write private keys to disk.key:listin a script dumps private keys into CI logs, which are typically world-readable inside an organization.proton key:listto "see what's in my wallet" is immediately exposed to their own keys without having asked for them.Industry standard for wallet tooling (hardware wallets, MetaMask, Ledger Live,
solana-keygen,cast wallet) is that private keys are never displayed without an explicit, friction-added opt-in. The current Proton CLI behavior is an outlier — and the rise of agentic tooling has turned what was already a foot-gun into a routine exposure risk.New commands
proton key:reveal-setupproton key:reveal-disableChanged commands
proton key:list{ publicKey, accounts: [{ account, permission }] }proton key:list --reveal-privateproton key:get <pubkey>--forcehas been removed from both commands — it was the exact bypass an AI agent used in practice. The reveal password is the intentional, human-only opt-in.TTY enforcement: all private-key-printing paths refuse to run when stdout or stdin is not a terminal.
Agent behavior matrix
proton key:listproton key:list --reveal-privateproton key:get <pk>proton action transfer ...Scope note
This PR hardens the CLI display paths. Users who want stronger at-rest protection should run
proton key:lock. A Keychain-backed store can be a follow-up.Test plan
proton key:list— prints public keys + account arrays, no private keysproton key:list --reveal-privatewhen piped/redirected — refusedproton key:get <pk>when piped — refusedproton key:reveal-setup— sets password, hash stored, plaintext not persistedproton key:reveal-setupa second time — requires current passwordproton key:reveal-disable— removes hash, requires current passwordproton key:list --reveal-privatewith reveal password set — prompts for password, correct password reveals, wrong password abortsproton key:get <pk>with reveal password set — samenpx tsc -b— clean buildPVT_K1_,PVT_R1_, legacy WIF) appears in any commitScope notes
get_accounts_by_authorizersis already used elsewhere in the CLI — no new dependencies.crypto.scryptSync— no new dependencies.conflibrary schema is extended with an optionalrevealPasswordHashfield; existing configs without it remain valid.