feat(plugins): migrate keystore CLI from FullNode to Toolkit#12
Open
barbatos2011 wants to merge 41 commits intodevelopfrom
Open
feat(plugins): migrate keystore CLI from FullNode to Toolkit#12barbatos2011 wants to merge 41 commits intodevelopfrom
barbatos2011 wants to merge 41 commits intodevelopfrom
Conversation
Signed-off-by: imalasong <imalasong@qq.com>
Use BigInteger for memory size computation to improve precision and add unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
2 times, most recently
from
c46573a to
730aa0a
Compare
Federico2014
reviewed
Apr 2, 2026
| implementation fileTree(dir: 'libs', include: '*.jar') | ||
| testImplementation project(":framework") | ||
| testImplementation project(":framework").sourceSets.test.output | ||
| implementation project(":crypto") |
There was a problem hiding this comment.
Recommended change:
implementation(project(":crypto")) {
exclude(group: 'io.github.tronprotocol', module: 'libp2p')
exclude(group: 'io.prometheus')
exclude(group: 'org.aspectj')
exclude(group: 'org.apache.httpcomponents')
}
Without exclusions, this pulls in all transitive dependencies from the crypto module, including:
- libp2p (P2P networking)
- prometheus (metrics)
- aspectj (AOP)
- httpcomponents (HTTP client)
None of these are used by the keystore commands (new, import, list, update), which only need:
- Bouncy Castle crypto (SCrypt, AES)
- Signature utilities (SignUtils, SignInterface)
- Basic utilities (ByteArray, Utils)
Impact: This should reduce Toolkit.jar from 86 MB to 56 MB.
Federico2014
reviewed
Apr 2, 2026
| return 1; | ||
| } | ||
|
|
||
| boolean ecKey = true; |
There was a problem hiding this comment.
ecKey should be a configurable parameter, defaulting to true, and it can be set to false to support the SM2 algorithm.
There was a problem hiding this comment.
yes，The “migration” is not behaviorally equivalent for non-EC deployments: SM2 users will generate/import keystores with the wrong engine semantics.
Federico2014
reviewed
Apr 2, 2026
| return 1; | ||
| } | ||
|
|
||
| boolean ecKey = true; |
There was a problem hiding this comment.
ecKey should be a configurable parameter, defaulting to true, and it can be set to false to support the SM2 algorithm.
Federico2014
reviewed
Apr 2, 2026
| return 1; | ||
| } | ||
|
|
||
| boolean ecKey = true; |
Federico2014
reviewed
Apr 2, 2026
| } | ||
|
|
||
| // Decrypt with old password | ||
| boolean ecKey = true; |
0xbigapple
reviewed
Apr 2, 2026
| char[] pwd1 = console.readPassword("Enter password: "); | ||
| char[] pwd2 = console.readPassword("Confirm password: "); | ||
| String password1 = new String(pwd1); | ||
| String password2 = new String(pwd2); |
There was a problem hiding this comment.
Sensitive Data in Memory. Console.readPassword() correctly returns a char[] to prevent memory leaks, but it is immediately converted into an immutable String. In Java, a String cannot be manually wiped and remains in memory until garbage collection. Recommendation: Handle passwords directly as char[] or byte[] where possible, and immediately wipe the array using Arrays.fill(pwd1, '\0') after use.
|
The |
0xbigapple
reviewed
Apr 2, 2026
|
|
||
| boolean ecKey = true; | ||
| SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), ecKey); | ||
| String fileName = WalletUtils.generateWalletFile(password, keyPair, keystoreDir, true); |
There was a problem hiding this comment.
Loose File Permissions. The new Keystore file is created without explicitly setting restrictive file permissions. It will be created with the system's default umask (often 644, world-readable). Since this file contains an encrypted private key, Recommendation: Explicitly set the file permissions to 600 (owner read/write only) immediately after creation using Files.setPosixFilePermissions.
0xbigapple
reviewed
Apr 2, 2026
| } | ||
| } | ||
|
|
||
| private String readPassword() throws IOException { |
There was a problem hiding this comment.
Logic Duplication. The readPassword() method, including the logic for handling the --password-file argument and console input, is copy-pasted across multiple classes. Recommendation: Extract this interactive input and file reading logic into a common utility class (e.g., KeystoreCliUtils) within the plugins module to reduce duplication and improve maintainability.
0xbigapple
reviewed
Apr 2, 2026
| private static final String FilePath = "Wallet"; | ||
|
|
||
| public static void start() { | ||
| System.err.println("WARNING: --keystore-factory is deprecated and will be removed " |
There was a problem hiding this comment.
Since the --keystore-factory command is officially being deprecated and now prints a warning to the console, please also add the @Deprecated annotation to the class declaration KeystoreFactory.
Federico2014
reviewed
Apr 2, 2026
| System.err.println(" keystore import - Import a private key"); | ||
| System.err.println(" keystore list - List keystores"); | ||
| System.err.println(" keystore update - Change password"); | ||
| System.err.println(); |
There was a problem hiding this comment.
The deprecation warning (lines 23-30) points users to the new Toolkit.jar commands:
Please use: java -jar Toolkit.jar keystore
keystore new - Generate a new keystore
keystore import - Import a private key
keystore list - List keystores
keystore update - Change password
However, the help() method inside the old REPL (lines 109-115) only shows the legacy commands without mentioning the new Toolkit.jar alternative:
private void help() {
System.out.println("You can enter the following command: ");
System.out.println("GenKeystore");
System.out.println("ImportPrivateKey");
System.out.println("Exit or Quit");
System.out.println("Input any one of them, you will get more tips.");
}
Impact: Users who continue using the old --keystore-factory REPL (during the transition period) won't see the deprecation notice when they type help, missing the opportunity to learn about the new commands.
Suggested fix: Update the help() method to include the deprecation notice:
private void help() {
System.out.println("NOTE: --keystore-factory is deprecated. Use Toolkit.jar instead:");
System.out.println(" java -jar Toolkit.jar keystore new|import|list|update");
System.out.println();
System.out.println("Legacy commands (will be removed):");
System.out.println("GenKeystore");
System.out.println("ImportPrivateKey");
System.out.println("Exit or Quit");
}
This ensures consistent messaging throughout the user experience.
0xbigapple
reviewed
Apr 2, 2026
|
|
||
| // Decrypt with old password | ||
| boolean ecKey = true; | ||
| WalletFile walletFile = MAPPER.readValue(keystoreFile, WalletFile.class); |
There was a problem hiding this comment.
keystore update rewrites the only keystore file in place via MAPPER.writeValue(keystoreFile, newWalletFile) after decrypting it. If serialization fails or the process dies mid-write, the user can lose the only copy of the keystore. This should write to a temp file and atomically rename on
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
3 times, most recently
from
41ceeb3 to
fd9e353
Compare
Federico2014
reviewed
Apr 2, 2026
| private static final String FilePath = "Wallet"; | ||
|
|
||
| public static void start() { | ||
| System.err.println("WARNING: --keystore-factory is deprecated and will be removed " |
There was a problem hiding this comment.
What is the intended timeline for removing KeystoreFactory from FullNode entirely?
Options:
-
Keep indefinitely - FullNode users who have scripts/automation depending on
--keystore-factorycan continue using it without changes. The deprecation is just guidance. -
Remove in next major release - Add a timeline (e.g., "will be removed in v4.0") to give users time to migrate.
-
Remove soon (e.g., next release) - Since Toolkit.jar provides identical functionality, there's no technical reason to maintain the code in FullNode.
Suggested clarification:
If the intent is option 1 or 2, consider updating the warning message to include a timeline or clarify that the feature will continue to work during a transition period:
WARNING: --keystore-factory is deprecated and will be removed in a future release.
Please use: java -jar Toolkit.jar keystore
(This feature will remain functional until version X.X)
This helps operators plan their migration accordingly.
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
from
fd9e353 to
6643897
Compare
Federico2014
reviewed
Apr 2, 2026
| @@ -0,0 +1,19 @@ | |||
| package org.tron.plugins; | |||
There was a problem hiding this comment.
It should be better to place the keystore-related classes in a separate package.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 30 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java:32">
P2: `readSinglePassword` validates password strength when reading from file but skips validation when reading from console. For a method intended to accept an existing password (no confirmation prompt), the strength check on the file path would incorrectly reject valid short passwords. Remove the validation to match the console path.</violation>
</file>
<file name="framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java">
<violation number="1" location="framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java:15">
P2: Add a timeout to prevent this test from hanging CI if the REPL loop doesn't terminate. Since `KeystoreFactory.start()` enters a `Scanner`-based `while(in.hasNextLine())` loop, any unexpected input-stream behavior will block indefinitely.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreList.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreList.java:75">
P2: JSON serialization errors return exit code 0 (success). Since this tool is explicitly designed for CI/CD scripting, returning success when `--json` output could not be written will silently break downstream consumers. Return a non-zero exit code on write failure.</violation>
</file>
<file name="plugins/src/test/java/org/tron/plugins/KeystoreListTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreListTest.java:132">
P2: This assertion is a false-positive risk: `"".split("\\n")` returns `[""]` (length 1), so the test passes even when the command outputs nothing. Add a non-empty check on the output before asserting line count.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreNew.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreNew.java:50">
P2: Unnecessary expensive scrypt decrypt. The `keyPair` is already in memory — use `Credentials.create(keyPair).getAddress()` to derive the address directly instead of reading and decrypting the file just written.</violation>
</file>
<file name="crypto/src/test/java/org/tron/keystore/CredentialsTest.java">
<violation number="1" location="crypto/src/test/java/org/tron/keystore/CredentialsTest.java:33">
P2: Missing `Assert.fail()` after the statement expected to throw. If `Credentials.create(...)` stops throwing, this test silently passes, hiding the regression. Add a `fail()` call, or use `@Test(expected = IllegalArgumentException.class)` instead.</violation>
</file>
<file name="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java:53">
P2: `testNewKeystoreJsonOutput` doesn't verify JSON output. The `StringWriter out` is allocated but never asserted against, and the command writes JSON through `System.out` rather than picocli's writer. This test only validates that `--json` doesn't crash, not that the output is correct.
Consider having `KeystoreNew` use the picocli `spec.commandLine().getOut()` writer for output so it can be captured in tests, or redirect `System.out` in the test to verify the JSON content.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java:105">
P2: `console.readPassword()` returns `null` on EOF (e.g., Ctrl+D). This causes an NPE on `new String(key)`, and then `Arrays.fill(key, '\0')` in the `finally` block throws a second NPE that suppresses the first, making it hard to diagnose.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if (passwordFile != null) { | ||
| byte[] bytes = Files.readAllBytes(passwordFile.toPath()); | ||
| String password = new String(bytes, StandardCharsets.UTF_8).trim(); | ||
| if (!WalletUtils.passwordValid(password)) { |
There was a problem hiding this comment.
P2: readSinglePassword validates password strength when reading from file but skips validation when reading from console. For a method intended to accept an existing password (no confirmation prompt), the strength check on the file path would incorrectly reject valid short passwords. Remove the validation to match the console path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java, line 32:
<comment>`readSinglePassword` validates password strength when reading from file but skips validation when reading from console. For a method intended to accept an existing password (no confirmation prompt), the strength check on the file path would incorrectly reject valid short passwords. Remove the validation to match the console path.</comment>
<file context>
@@ -0,0 +1,129 @@
+ if (passwordFile != null) {
+ byte[] bytes = Files.readAllBytes(passwordFile.toPath());
+ String password = new String(bytes, StandardCharsets.UTF_8).trim();
+ if (!WalletUtils.passwordValid(password)) {
+ System.err.println("Invalid password: must be at least 6 characters.");
+ return null;
</file context>
| String output = baos.toString(StandardCharsets.UTF_8.name()); | ||
| String[] lines = output.trim().split("\\n"); | ||
| // Should list only the valid keystore, not the readme.json or notes.txt | ||
| assertEquals("Should list only 1 valid keystore", 1, lines.length); |
There was a problem hiding this comment.
P2: This assertion is a false-positive risk: "".split("\\n") returns [""] (length 1), so the test passes even when the command outputs nothing. Add a non-empty check on the output before asserting line count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/test/java/org/tron/plugins/KeystoreListTest.java, line 132:
<comment>This assertion is a false-positive risk: `"".split("\\n")` returns `[""]` (length 1), so the test passes even when the command outputs nothing. Add a non-empty check on the output before asserting line count.</comment>
<file context>
@@ -0,0 +1,137 @@
+ String output = baos.toString(StandardCharsets.UTF_8.name());
+ String[] lines = output.trim().split("\\n");
+ // Should list only the valid keystore, not the readme.json or notes.txt
+ assertEquals("Should list only 1 valid keystore", 1, lines.length);
+ } finally {
+ System.setOut(originalOut);
</file context>
287ba36 to
8bdd691
Compare
…etOwnerAddress-unpack-type feat(actuator): correct protobuf unpack types in getOwnerAddress()
…#6631) * feat(protocol): add protoLint script for enum validation * feat(protocol): optimize protoLint performance and caching * fix(proto): resolve gradle implicit task dependency warning * docs(protocol): clarify enum discriminator in protoLint * build(protocol): harden proto lint buf config * docs: update comment to reflect dynamic include path derivation
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java">
<violation number="1" location="framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java:123">
P2: Provide password lines before `exit`; otherwise the `exit` token is consumed by `inputPassword2Twice()` and the REPL never prints `Exit !!!`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // caught and logged, and the REPL continues. | ||
| ByteArrayOutputStream outContent = new ByteArrayOutputStream(); | ||
| System.setOut(new PrintStream(outContent)); | ||
| System.setIn(new ByteArrayInputStream("genkeystore\nexit\n".getBytes())); |
There was a problem hiding this comment.
P2: Provide password lines before exit; otherwise the exit token is consumed by inputPassword2Twice() and the REPL never prints Exit !!!.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java, line 123:
<comment>Provide password lines before `exit`; otherwise the `exit` token is consumed by `inputPassword2Twice()` and the REPL never prints `Exit !!!`.
</comment>
<file context>
@@ -2,34 +2,146 @@
+ // caught and logged, and the REPL continues.
+ ByteArrayOutputStream outContent = new ByteArrayOutputStream();
+ System.setOut(new PrintStream(outContent));
+ System.setIn(new ByteArrayInputStream("genkeystore\nexit\n".getBytes()));
+
+ KeystoreFactory.start();
</file context>
Suggested change
| System.setIn(new ByteArrayInputStream("genkeystore\nexit\n".getBytes())); | |
| System.setIn(new ByteArrayInputStream("genkeystore\npassword123\npassword123\nexit\n".getBytes())); |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
…col#6614) * test(framework): merge duplicate CredentialsTest coverage Consolidate the misplaced keystroe CredentialsTest into org.tron.keystore.CredentialsTest. - remove the duplicate test under the misspelled keystroe package - add explicit equals behavior coverage for address and cryptoEngine - normalize assertions to JUnit Assert and remove legacy TestCase usage * test(framework): stabilize CredentialsTest fixtures Replace random Credentials test setup with deterministic SignInterface mocks so the suite no longer depends on platform-specific SecureRandom providers or probabilistic retries. - remove NativePRNG usage from CredentialsTest - replace random key generation with fixed address fixtures via mocked SignInterface - assert create(SignInterface) returns the expected base58check address - keep equals/hashCode contract coverage with deterministic inputs
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.
…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.
- 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
- 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
…ssages - Extract isValidKeystoreFile(WalletFile) to KeystoreCliUtils, requiring address != null, crypto != null, and version == 3 consistently across list, import, and update commands - Fix update command printing both "Multiple keystores found" and "No keystore found" for the same address — move error messages into findKeystoreByAddress so each case prints exactly one message
- Add tests for update with nonexistent/file keystore-dir, old Mac CR line endings, and invalid-version keystore files - Add test for import duplicate check skipping invalid-version files - Add test for list skipping invalid-version and no-crypto keystores - Fix password file split regex to handle old Mac CR-only line endings
Cover utility methods (stripLineEndings, jsonMap, isValidKeystoreFile, checkFileExists, readPassword, ensureDirectory, printJson, printSecurityTips, atomicMove, generateKeystoreFile, setOwnerOnly) with direct unit tests, including edge cases like BOM, various line endings, empty strings, and both ECDSA/SM2 key paths.
- Add WalletFilePojoTest covering all getter/setter, equals, hashCode paths for WalletFile and its inner classes (Crypto, CipherParams, Aes128CtrKdfParams, ScryptKdfParams), plus JSON deserialization with both scrypt and pbkdf2 KDF types - Expand KeystoreFactoryDeprecationTest to cover help, invalid command, quit, empty line, and genKeystore/importprivatekey dispatch paths - Tests live in framework module so their coverage data is included in crypto module's jacoco report (crypto:jacocoTestReport reads framework exec files)
Prevents address-spoofing attacks where a crafted keystore declares one address in JSON but encrypts a different private key. After decryption, verify that the declared address matches the address derived from the decrypted key. Null/empty addresses are still accepted for compatibility with Ethereum-style keystores that omit the field.
- Remove newWalletFile.setAddress(walletFile.getAddress()) — createStandard already sets the correctly-derived address, and propagating the JSON address could carry a spoofed value forward - Use the derived address from newWalletFile for output messages instead of walletFile.getAddress(), keeping the output correct as a defense-in-depth measure even if upstream validation is weakened - Add tests for tampered-address rejection and derived-address output
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
added 5 commits
April 21, 2026 17:31
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.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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