feat(plugins): migrate keystore CLI from FullNode to Toolkit#6637
feat(plugins): migrate keystore CLI from FullNode to Toolkit#6637barbatos2011 wants to merge 24 commits intotronprotocol:developfrom
Conversation
barbatos2011
requested review from
317787106,
3for,
Federico2014,
halibobo1205,
kuny0707 and
xxo1shine
as code owners
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
3 times, most recently
from
f30d460 to
cc02b8c
Compare
| if (!keystoreDir.exists() || !keystoreDir.isDirectory()) { | ||
| return null; | ||
| } | ||
| File[] files = keystoreDir.listFiles((dir, name) -> name.endsWith(".json")); |
There was a problem hiding this comment.
When duplicate-address keystores exist, keystore update <address> does not have a uniquely defined target. findKeystoreByAddress() returns the first matching file from keystoreDir.listFiles(), but that enumeration order is unspecified. In practice this makes the command nondeterministic: it may update the wrong file or fail depending on which matching file is encountered first.
There was a problem hiding this comment.
Fixed. findKeystoreByAddress() now collects all matches instead of returning the first one. When multiple keystores share the same address, it prints all matching filenames and returns an error:
Multiple keystores found for address TXxx...:
UTC--2026-04-02T10-00-00--TXxx.json
UTC--2026-04-02T11-00-00--TXxx.json
Please remove duplicates and retry.
This makes the behavior deterministic — the command refuses to proceed rather than silently picking one at random.
| + " for the selected algorithm."); | ||
| return 1; | ||
| } | ||
| String fileName = WalletUtils.generateWalletFile(password, keyPair, keystoreDir, true); |
There was a problem hiding this comment.
[Medium] No duplicate-address warning on import
When importing a private key whose address already exists in keystoreDir, the command silently creates a second keystore file. Combined with the nondeterministic findKeystoreByAddress in KeystoreUpdate, this creates a foot-gun: import the same key twice, then keystore update picks one at random.
Suggestion: scan the directory for existing keystores with the same address before writing, and print a warning:
WARNING: keystore for address already exists:
Still allow the import (for legitimate use cases), but make the user aware.
There was a problem hiding this comment.
Fixed. KeystoreImport now scans the target directory before writing and prints a warning if a keystore for the same address already exists:
WARNING: keystore for address TXxx... already exists: UTC--2026-04-02T10-00-00--TXxx.json
The import still proceeds (legitimate use case: re-importing with a different password), but the user is made aware. Combined with the findKeystoreByAddress fix in KeystoreUpdate, the foot-gun is mitigated: import warns upfront, and update refuses to operate on ambiguous duplicates.
| } | ||
| try { | ||
| oldPassword = new String(oldPwd); | ||
| newPassword = new String(newPwd); |
There was a problem hiding this comment.
[Low] Password String objects remain on the heap after use
The char[] arrays from Console.readPassword() are correctly zeroed in finally blocks (lines 115-117), but the passwords are then copied into immutable String objects (oldPassword = new String(oldPwd) at line 107, newPassword = new
String(newPwd) at line 108). These String copies cannot be zeroed and remain in memory until GC collects them.
The same pattern exists in KeystoreCliUtils.readPassword() (line 74: String password = new String(pwd1)).
This is a known Java limitation. Worth documenting with a comment at minimum, or refactoring the downstream APIs (Wallet.decrypt, Wallet.createStandard) to accept char[] in a follow-up.
There was a problem hiding this comment.
Acknowledged — this is a known Java limitation. The downstream APIs (Wallet.decrypt, Wallet.createStandard, WalletUtils.loadCredentials) all accept String parameters, so converting char[] to String is unavoidable without refactoring the entire keystore API chain.
Current mitigations:
char[]fromConsole.readPassword()is zeroed infinallyblocksbyte[]fromFiles.readAllBytes()is zeroed after conversion- Password comparison uses
Arrays.equals(char[], char[])to avoid creating a second String
Full char[]-throughout refactoring is tracked as a follow-up.
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
from
cc02b8c to
1492a49
Compare
|
|
Thanks for the detailed review with geth source references @3for! Here's my response to each point: 1. Duplicate import — now blocked by defaultFixed in d7f7c6b. A 2. Error messages improvedFixed in d7f7c6b. When Note: our tool already supports interactive password input when 3. Plaintext key file — same as geth
|
| if (address.equals(wf.getAddress())) { | ||
| return file.getName(); |
There was a problem hiding this comment.
The definition of “what counts as a keystore” is inconsistent with KeystoreList.java here. This check should also require wf.getCrypto() != null && wf.getVersion() == 3.
Right now, KeystoreList, KeystoreImport, and KeystoreUpdate each deserialize WalletFile and apply their own filtering logic separately. Suggest extracting a shared helper like static boolean isKeystoreFile(WalletFile wf) and reusing the same predicate across list / import / update so the behavior stays consistent.
There was a problem hiding this comment.
Good point. Extracted KeystoreCliUtils.isValidKeystoreFile(WalletFile) that checks address != null && crypto != null && version == 3, now shared across list, import, and update.
| System.err.println("Multiple keystores found for address " | ||
| + targetAddress + ":"); | ||
| for (File m : matches) { | ||
| System.err.println(" " + m.getName()); | ||
| } | ||
| System.err.println("Please remove duplicates and retry."); | ||
| return null; |
There was a problem hiding this comment.
When this branch is taken, it prints Multiple keystores found for address... and returns null. But the caller above interprets that null as “not found” and prints No keystore found for address.... Those two messages seem inconsistent.
There was a problem hiding this comment.
Fixed. Moved the error messages into findKeystoreByAddress itself — each case (not found, multiple matches, no directory) prints exactly one specific message. The caller no longer adds a redundant "No keystore found".
| List<String> privateKeys = new ArrayList<>(); | ||
| try { | ||
| Credentials credentials = WalletUtils.loadCredentials(pwd, new File(fileName)); | ||
| Credentials credentials = WalletUtils.loadCredentials(pwd, new File(fileName), |
There was a problem hiding this comment.
#6588 has already decided to remove the unused SM2 algorithm and related configuration. Why is this PR still adding compatibility for it (e.g., the boolean ecKey parameter and --sm2 option)?
There was a problem hiding this comment.
@halibobo1205 It has currently been decided to retain SM2. See: #6627 (comment)
|
The new Before investing further effort here, I think we should first assess how many users are actually using the keystore feature. Given that |
| KeystoreUpdate.class | ||
| }, | ||
| commandListHeading = "%nCommands:%n%nThe most commonly used keystore commands are:%n" | ||
| ) |
There was a problem hiding this comment.
[SHOULD]: Suggest moving keystore-related classes into the org.tron.plugins.keystore package. Similarly, existing db-related commands could be moved into org.tron.plugins.db. Previously, with only db commands in the plugins module, having everything in the top-level package was fine. But now that keystore is being added, organizing by feature package would be cleaner.
[MUST]: update README.md for new commands.
There was a problem hiding this comment.
README: Already added a Keystore section to plugins/README.md with migration mapping and usage examples for all subcommands.
Package restructuring: Agree this would be cleaner. The current flat layout in org.tron.plugins is the existing convention — all db command classes (Db, DbConvert, DbCopy, etc.) are already at the top-level package. The keystore classes follow the same pattern. Since a proper restructuring would involve moving both db and keystore classes together for consistency, I'd suggest handling it as a separate refactor PR to keep this one focused.
| @Command(name = "keystore", | ||
| mixinStandardHelpOptions = true, | ||
| version = "keystore command 1.0", | ||
| description = "Manage keystore files for witness account keys.", |
There was a problem hiding this comment.
[SHOULD]: The description says "witness account keys", but the implementation is generic and works for any account, not just witnesses. Suggest changing to "Manage keystore files for account keys."
There was a problem hiding this comment.
Good catch. Updated to "Manage keystore files for account keys." — the implementation is indeed generic and not witness-specific.
| } catch (IOException e) { | ||
| System.err.println("Warning: could not set file permissions on " + file.getName()); | ||
| } | ||
| } |
There was a problem hiding this comment.
[NIT]: Suggest printing a warning to the user when file permissions cannot be set — both for the Windows UnsupportedOperationException case (currently silently skipped) and the IOException case (currently only prints the file name without guidance). For example:
catch (UnsupportedOperationException | IOException e) {
System.err.println("Warning: could not set file permissions on " + file.getName()
+ ". Please manually restrict access to this file.");
}This way, users are aware that the keystore file is not protected and can take action themselves.
There was a problem hiding this comment.
Updated. Both UnsupportedOperationException and IOException now print a warning with guidance to manually restrict access.
Clarify that 'keystore list' displays the declared address from each keystore's JSON without decrypting it. A tampered keystore may claim an address that does not correspond to its encrypted key; verification only happens at decryption time (e.g. 'keystore update').
Move the temp-file + atomic-rename + POSIX 0600 permissions pattern from plugins into WalletUtils.generateWalletFile and a new public WalletUtils.writeWalletFile method. Use Files.createTempFile with PosixFilePermissions.asFileAttribute to atomically create the temp file with owner-only permissions, eliminating the brief world-readable window present in the previous File.createTempFile + setOwnerOnly sequence. All callers benefit automatically, including framework/KeystoreFactory (which previously wrote 0644 files) and any third-party project that consumes the crypto artifact. Removes ~68 lines of duplicated plumbing from KeystoreCliUtils. - Windows/non-POSIX fallback is best-effort (DOS attributes) and documented as such in JavaDoc - Error path suppresses secondary IOException via addSuppressed so the original cause is preserved - OWNER_ONLY wrapped with Collections.unmodifiableSet for defense in depth - WalletUtilsWriteTest covers permissions, replacement, temp cleanup, parent-directory creation, and a failure-path test that forces move to fail and asserts no temp file remains - KeystoreUpdateTest adversarially pre-loosens perms to 0644 and verifies update narrows back to 0600
…closure By default java.nio.file.Files.readAllBytes follows symbolic links, allowing an attacker who can plant files in a user-supplied path to redirect reads to arbitrary files (e.g. /etc/shadow, SSH private keys). The first ~1KB of the linked file would be misused as a password or private key. Introduces KeystoreCliUtils.readRegularFile that: - stats the path with LinkOption.NOFOLLOW_LINKS (lstat semantics) - rejects symlinks, directories, FIFOs and other non-regular files - opens the byte channel with LinkOption.NOFOLLOW_LINKS too, closing the TOCTOU window between stat and open - enforces a single size check via the lstat-returned attributes instead of a separate File.length() call All three call sites are migrated: - KeystoreCliUtils.readPassword (used by new/import) - KeystoreImport.readPrivateKey (key file) - KeystoreUpdate.call (password file for old+new passwords) Tests: - unit tests for readRegularFile covering success, missing file, too-large, symlink refused, directory refused, and empty file - end-to-end tests in KeystoreImportTest that provide a symlinked --key-file and --password-file and assert refusal
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
from
ba01b3b to
40ec5ce
Compare
WalletUtils.inputPassword() previously applied trim() + split("\\s+")[0]
on the non-TTY (stdin) branch, silently truncating passphrases like
"correct horse battery staple" to "correct" when piped via stdin. This
produced keystores encrypted with only the first word, which would then
fail to decrypt under new CLIs that correctly preserve the full password.
- Replace trim() + split("\\s+")[0] with stripPasswordLine() that only
strips the UTF-8 BOM and trailing line terminators, preserving internal
whitespace
- Expose stripPasswordLine as the canonical public helper and consolidate
KeystoreCliUtils.stripLineEndings into it (plugins now delegates)
- Fix a pre-existing Scanner lifecycle bug: inputPassword() used to
allocate a fresh Scanner(System.in) on every call, so the second call
in inputPassword2Twice() lost bytes the first Scanner had buffered.
The Scanner is now lazily-cached and shared across calls, with a
resetSharedStdinScanner() hook for tests
- KeystoreFactory.importPrivateKey keeps trim()+split for private-key
input (hex strings have no legitimate whitespace)
New tests cover: internal whitespace preservation, tabs, CRLF, BOM,
leading/trailing spaces, the inputPassword2Twice piped double-read path,
and direct unit tests of stripPasswordLine (null, empty, only-BOM,
only-terminators, BOM-then-terminator).
…failure
When `keystore update` decryption fails and the provided old password
contains whitespace, print a hint pointing users at the
`--keystore-factory` non-TTY truncation bug: keystores created with
`echo PASS | java -jar FullNode.jar --keystore-factory` were encrypted
using only the first whitespace-separated word of the password.
The hint is gated on whitespace in the supplied password so that the
overwhelmingly common case (user simply mistyped) does not get noise
about an unrelated legacy bug. Wording explicitly tells the user to
re-run with just the first word as the current password and offers the
full phrase as the new password.
Two tests cover both branches of the gate (hint fires when password
contains whitespace, suppressed otherwise).
Deliberately not adding automatic fallback to split("\\s+")[0] during
decryption — that would lower effective password entropy by giving an
attacker a free first-token brute-force channel. Recovery stays explicit
and user-driven.
…lure When WitnessInitializer fails to load the SR keystore due to a CipherException and the provided password contains whitespace, log a tip pointing the operator at the same legacy `--keystore-factory` non-TTY truncation bug handled in the Toolkit plugins CLI. Gives SR operators a breadcrumb when their node refuses to start after upgrading past the inputPassword() fix: their keystore may have been encrypted using only the first word of their password, and they can recover by using that first word temporarily, then resetting with Toolkit.jar keystore update. The tip only fires when the password has whitespace (truncation cannot explain a failure otherwise), keeping the common wrong-password case quiet.
halibobo1205
assigned
xxo1shine,
317787106,
Federico2014 and
lxcmyf and unassigned
xxo1shine,
317787106,
Federico2014 and
lxcmyf
| List<Map<String, String>> entries = new ArrayList<>(); | ||
| for (File file : files) { | ||
| try { | ||
| WalletFile walletFile = MAPPER.readValue(file, WalletFile.class); |
There was a problem hiding this comment.
[SHOULD]Keystore directory scans still follow symlinks
The PR hardened --password-file and --key-file reads with NOFOLLOW_LINKS, but the keystore-directory scans in list, import, and update still call MAPPER.readValue(file, ...) directly on every *.json entry. In a hostile or group-writable keystore directory, a symlink or special file can still make the process read arbitrary files or block on non-regular files. The same regular-file / nofollow gate used by readRegularFile() needs to be applied before deserializing candidate keystores.
There was a problem hiding this comment.
Good catch — fixed in commit 340669cb6.
Added KeystoreCliUtils.isSafeRegularFile(file, err) which stats with LinkOption.NOFOLLOW_LINKS and rejects symlinks, directories, FIFOs, and devices (the same predicate used by readRegularFile). Applied it before every MAPPER.readValue(file, ...) in the keystore-directory scans:
KeystoreList.call— listing pathKeystoreImport.findExistingKeystore— duplicate-address checkKeystoreUpdate.findKeystoreByAddress— target lookup
Tests added for all three commands: plant a *.json symlink in the keystore dir pointing at a file outside; each command warns with Warning: skipping symbolic link: evil.json and proceeds as if the entry weren't there. Non-regular files (directories / FIFOs / devices) go through the same guard.
| return null; | ||
| } | ||
| try { | ||
| String password = WalletUtils.stripPasswordLine( |
There was a problem hiding this comment.
[SHOULD]Multi-line password files become literal passwords in new/import
readPassword() reads the entire file and only strips trailing line endings. For keystore new and keystore import, a two-line file therefore becomes a password like old\nnew, which still passes validation and creates a keystore successfully, but neither visible line alone will unlock it later. This is easy to trigger by accidentally reusing an update password file. The non-update path should reject multi-line files or explicitly consume only the first line.
| /** | ||
| * Check if a WalletFile represents a valid V3 keystore. | ||
| */ | ||
| static boolean isValidKeystoreFile(WalletFile wf) { |
There was a problem hiding this comment.
[SHOULD]Keystore detection accepts unusable JSON stubs
isValidKeystoreFile() currently treats any JSON with address, non-null crypto, and version == 3 as a keystore. A stub like {"address":"T...","version":3,"crypto":{}} now passes discovery even though Wallet.validate() would later reject it for missing or unsupported cipher/KDF data. That means keystore list can show junk entries, keystore import can refuse a valid import as a duplicate, and keystore update can report duplicate matches or fail against the bogus file. Discovery should reuse Wallet.validate() or at least require supported cipher/KDF fields so only decryptable keystores are matched.
crypto/build.gradle's jacocoTestReport reads exec data from ../framework/build/jacoco only — so tests in crypto/src/test itself produce no signal in the crypto coverage report that the Coverage Gate job checks. WalletFilePojoTest was already placed in framework for this reason; this commit makes the other seven keystore tests consistent. Moves (pure git mv, no code changes needed — same package `org.tron.keystore`, all test deps available in framework): - CredentialsTest - CrossImplTest - WalletAddressValidationTest - WalletFileTest - WalletPropertyTest - WalletUtilsInputPasswordTest - WalletUtilsWriteTest After the move, crypto/src/test/java/org/tron/keystore/ is empty and all keystore test classes live in framework/src/test/java/org/tron/ keystore/ alongside WalletFilePojoTest.
Earlier commits hardened --password-file / --key-file reads against symlink-following, but the keystore-directory scans in list, import (duplicate check), and update (findKeystoreByAddress) still called MAPPER.readValue(file, ...) on every *.json entry without a symlink guard. In a hostile or group-writable keystore directory, a planted symlink could redirect deserialization to arbitrary files, and FIFOs or devices could block the scan indefinitely. - Add KeystoreCliUtils.isSafeRegularFile(file, err) helper that stats with LinkOption.NOFOLLOW_LINKS and rejects symbolic links, directories, FIFOs, and devices with a warning to err - Apply it before MAPPER.readValue in KeystoreList.call, KeystoreImport.findExistingKeystore, and KeystoreUpdate.findKeystoreByAddress - Add three end-to-end tests (one per command) that plant a .json symlink in the keystore dir and verify each command warns and continues without reading the symlink target
8 participants
What
Migrate the
--keystore-factoryinteractive REPL from FullNode.jar to Toolkit.jar as picocli subcommands, extract the keystore core library fromframeworktocryptomodule, and add new capabilities (list, update, SM2 support, JSON output, password-file support).Why
The current
--keystore-factoryhas several limitations:--password-fileor structured outputScanner-based password input echoes to terminal; no--key-fileoption forces private keys to be typed interactively or passed as command-line arguments (visible inps, shell history)Wallet.java,WalletUtils.java) lives inframeworkand callsArgs.getInstance(), preventing reuse byplugins(Toolkit.jar) without pulling in the entire framework dependency chainChanges
Commit 1:
refactor(crypto): extract keystore library from framework moduleExtract the keystore package from
frameworktocryptomodule to break the framework dependency:Wallet.javacrypto/, removedArgsimport, addedboolean ecKeyparameter todecrypt(), changed MAC comparison to constant-timeMessageDigest.isEqual()WalletUtils.javacrypto/, removedArgsimport, addedboolean ecKeyparameter togenerateNewWalletFile(),generateFullNewWalletFile(),generateLightNewWalletFile(),loadCredentials()Credentials.java,WalletFile.javacrypto/unchangedWitnessInitializer.javaArgs.getInstance().isECKeyCryptoEngine()toloadCredentials()KeystoreFactory.javaCommonParameter.getInstance().isECKeyCryptoEngine()to keystore callsplugins/build.gradleimplementation project(":crypto")with exclusions (libp2p, prometheus, aspectj, httpcomponents)CredentialsTest.javakeystroetypo directory) intocryptomoduleWalletFileTest.javacryptomoduleCommit 2:
feat(plugins): add keystore new and import commands to ToolkitAdd picocli subcommands for the two core keystore operations:
Security design:
Console.readPassword()(no echo), with null check for EOF (Ctrl+D)char[]compared directly viaArrays.equals()(avoids creating unnecessary String), zeroed infinallyblocksbyte[]zeroed after reading viaArrays.fill(bytes, (byte) 0)--key-fileor interactive prompt, never from command-line arguments0x-prefixed keys (common Ethereum format)Files.setPosixFilePermissionsObjectMapper.writeValueAsString(), not string concatenation--sm2flag for SM2 algorithm supportShared utilities:
KeystoreCliUtils—readPassword(),ensureDirectory()(usesFiles.createDirectories),setOwnerOnly(),printJson()ensureDirectory()usesFiles.createDirectories()to avoid TOCTOU race conditionsToolkit.javaupdated to registerKeystore.classin subcommandsTest infrastructure:
Commit 3:
feat(plugins): add keystore list and update commands, deprecate --keystore-factorylist: scans directory for keystore JSON files, displays address + filename, skips non-keystore files. JSON serialization failure returns exit code 1.update: decrypts with old password, re-encrypts with new password. Atomic file write (temp file +Files.move(ATOMIC_MOVE)) prevents corruption on interrupt. Wrong old password fails without modifying the file. Supports Windows line endings in password file.@Deprecatedannotation onKeystoreFactoryclass + deprecation warning instart()+ migration notice inhelp()methodWitnessInitializerKeystoreTestverifies new tool's keystore can be loaded byWitnessInitializer.initFromKeystore()Scope
localwitnesskeystoreconfig —WitnessInitializercontinues to load keystore files at node startup--keystore-factoryflag remains functional with a deprecation warningTest
WitnessInitializerTestandSupplementTestupdated for new signatures.WitnessInitializerTest,ArgsTest,SupplementTestall passcheckstyleMain+checkstyleTest)Cross-implementation compatibility
Migration from --keystore-factory
--keystore-factoryToolkit.jar keystoreGenKeystorekeystore newImportPrivateKeykeystore importkeystore listkeystore update--password-file,--key-file--json--sm2--keystore-dirConsole.readPassword)The old
--keystore-factoryremains functional with a deprecation warning during the transition period.Usage
Commands
newimportlistupdateCommon Options
--keystore-dir <path>./Wallet)--json--password-file <path>--sm2--key-file <path>importonly)Examples