feat(plugins): migrate keystore CLI from FullNode to Toolkit#6637
feat(plugins): migrate keystore CLI from FullNode to Toolkit#6637barbatos2011 wants to merge 8 commits intotronprotocol:developfrom
Conversation
barbatos2011
requested review from
317787106,
3for,
Federico2014,
halibobo1205,
kuny0707 and
xxo1shine
as code owners
Move keystore package (Wallet, WalletUtils, Credentials, WalletFile)
from framework to crypto module to enable reuse by Toolkit.jar without
pulling in the entire framework dependency.
- Replace Args.getInstance().isECKeyCryptoEngine() calls with injected
boolean ecKey parameter in Wallet.decrypt(), WalletUtils.loadCredentials(),
generateNewWalletFile(), generateFullNewWalletFile(), generateLightNewWalletFile()
- Update callers (KeystoreFactory, WitnessInitializer) to pass ecKey
- Add implementation project(":crypto") to plugins build
- Merge two CredentialsTest files (fix keystroe typo dir) into crypto module
- Move WalletFileTest to crypto module
- CipherException already in common module, no move needed
Add picocli subcommands for keystore management in Toolkit.jar: - `keystore new`: generate new keypair and encrypt to keystore file - `keystore import`: import existing private key into keystore file Both commands support: - --password-file for non-interactive password input - --keystore-dir for custom output directory - --json for structured output - Console.readPassword() for no-echo interactive input - Clear error when no TTY available (directs to --password-file) Import reads private key from --key-file or interactive prompt, never from command-line arguments (security: avoids ps/history exposure). Also adds roundtrip property tests (100 random encrypt/decrypt cycles) and cross-implementation compatibility tests to crypto module. Note: jqwik was planned for property testing but replaced with plain JUnit loops due to Gradle dependency verification overhead.
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.
…store-factory Add remaining keystore subcommands to Toolkit.jar: - `keystore list`: display all keystore files and addresses in a directory - `keystore update <address>`: re-encrypt a keystore with a new password Both support --keystore-dir, --json, and --password-file options. Add deprecation warning to --keystore-factory in FullNode.jar, directing users to the new Toolkit.jar keystore commands. The old REPL continues to function normally during the transition period.
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
from
cc02b8c to
1492a49
Compare
- KeystoreUpdate: findKeystoreByAddress now detects multiple keystores with the same address and returns an error with file list, instead of silently picking one nondeterministically. - KeystoreImport: scan directory before writing and print WARNING if a keystore for the same address already exists. Import still proceeds (legitimate use case) but user is made aware. - KeystoreUpdate: strip UTF-8 BOM from password file before parsing. - KeystoreUpdate: add comment clarifying old password skips validation. - KeystoreList: add version != 3 check to filter non-keystore JSON.
|
…rity tips - Reject duplicate-address import by default, add --force flag to override - Add friendly error messages when --password-file or --key-file not found - Print security tips after keystore new/import (aligned with geth output) - Add file existence checks in KeystoreCliUtils and KeystoreUpdate
|
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.
| 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.
| 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.
| privateKey = privateKey.substring(2); | ||
| } | ||
| if (!isValidPrivateKey(privateKey)) { | ||
| System.err.println("Invalid private key: must be 64 hex characters."); |
There was a problem hiding this comment.
The keystore commands use System.out.println / System.err.println directly, while existing db commands in the plugins module consistently use spec.commandLine().getOut() / spec.commandLine().getErr() with getColorScheme().errorText(). Suggest aligning with the existing pattern:
- Use
spec.commandLine().getOut()/getErr()for console output — this enables testable output streams, colored error text, and stays consistent with the rest of the module - For non-sensitive information, also log via
logger.info/logger.errorfor traceability
There was a problem hiding this comment.
Updated all keystore commands to use spec.commandLine().getOut()/getErr() instead of System.out/err, consistent with the existing db commands pattern. Tests updated accordingly to use cmd.setOut()/setErr().
Skipping logger for keystore commands — they handle passwords and private keys, so logging non-sensitive output would require careful per-line auditing with limited traceability value.
|
@halibobo1205 Thanks for the feedback. A few clarifications:
|
- Replace System.out/err with spec.commandLine().getOut()/getErr() in all keystore commands, consistent with existing db commands pattern - Fix description from "witness account keys" to "account keys" - Unify permission warning for UnsupportedOperationException and IOException - Add Keystore section to plugins/README.md with migration guide - Update tests to use cmd.setOut()/setErr() instead of System.setOut()
- Fix TOCTOU race: keystore files were world-readable between creation and permission setting. Now use temp-file + setOwnerOnly + atomic-rename pattern (already used by update command) for new and import commands - Fix KeystoreUpdate password file parsing: apply stripLineEndings to both old and new passwords to handle old-Mac line endings - Warn on unreadable JSON files during keystore lookup instead of silently skipping, so users are aware of corrupted files - Make WalletUtils.getWalletFileName public for reuse - Extract atomicMove utility to KeystoreCliUtils for shared use
- Add tests for update: JSON output, single-line password file, no TTY, password file not found, SM2 keystore, multiple same-address keystores, large password file, BOM password file - Add tests for import: 0x/0X prefix key, corrupted file warning, POSIX file permissions - Add tests for new: large password file, BOM password file, POSIX file permissions - Add tests for list: empty/nonexistent directory JSON output, corrupted file warning, output content verification - Strengthen existing tests with error message assertions to prevent false passes from wrong failure reasons
5 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