feat(plugins): migrate keystore CLI from FullNode to Toolkit#6637
feat(plugins): migrate keystore CLI from FullNode to Toolkit#6637barbatos2011 wants to merge 33 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
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
|
|
The new Before investing further effort here, I think we should first assess how many users are actually using the keystore feature. Given that |
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
readPassword() only stripped trailing line terminators, so a password file containing interior newlines — e.g. the two-line format used by `keystore update` (old\nnew) — was silently accepted as a literal password like "old\nnew". The keystore was created successfully and passed length validation, but neither visible line alone could unlock it later. Easy to trigger by accidentally reusing an update password file with `new` or `import`. After stripPasswordLine, explicitly reject files whose contents still contain \n or \r, with an error that points at the update two-line format as the likely cause. `keystore update` has its own two-line parser elsewhere and is unaffected. Tests: - KeystoreNewTest.testNewKeystoreRejectsMultiLinePasswordFile - KeystoreImportTest.testImportRejectsMultiLinePasswordFile Both assert exit code 1, presence of the "multiple lines" error, and that no keystore file was created.
…very
isValidKeystoreFile previously accepted any JSON with address, non-null
crypto, and version == 3 — so a stub like
{"address":"T...","version":3,"crypto":{}}
passed discovery even though Wallet.validate would reject it later for
missing cipher/KDF. That let keystore list show junk entries, keystore
import refuse a real import as a duplicate, and keystore update report
spurious multi-match / stub failures.
- Extract a shared validationError(WalletFile) helper in Wallet that
checks version, crypto != null, cipher in the supported set, and KDF
in the supported set. Both Wallet.validate (throws CipherException
with the specific error) and the new public
Wallet.isValidKeystoreFile (returns boolean) use it, so the
discovery predicate cannot drift from decrypt-time validation
- KeystoreCliUtils.isValidKeystoreFile now delegates to
Wallet.isValidKeystoreFile
- Side-effect fix: Wallet.validate no longer NPEs when crypto == null
Tests: existing KeystoreCliUtilsTest fixtures now build a supported
Crypto (cipher + kdf). Added four new cases covering the stub scenario:
empty crypto, unsupported cipher, unsupported KDF, and pbkdf2 acceptance.
The constant Wallet.AES_128_CTR held the string "pbkdf2" — its name implied a cipher but its value is actually the KDF identifier used in the Web3 Secret Storage keystore "kdf" field. A future contributor renaming or re-valuing it could silently break pbkdf2 keystore support, which is hit by the new Wallet.validationError check and by the Jackson subtype registration in WalletFile. Rename to PBKDF2 to match the string value. The only three references (declaration, KDF check in validationError, JsonSubTypes name in WalletFile) all follow. Wire format is unchanged — the constant string is still "pbkdf2". The inner class `WalletFile.Aes128CtrKdfParams` keeps its name because changing a public class would be a binary-incompatible break for any third-party consumer of the crypto artifact.
| WalletFile walletFile = objectMapper.readValue(source, WalletFile.class); | ||
| return Credentials.create(Wallet.decrypt(password, walletFile, ecKey)); |
There was a problem hiding this comment.
This is part of the SR startup path. We should consider adding a safeguard similar to KeystoreCliUtils.isSafeRegularFile to ensure the file is a regular file and explicitly reject symlinks or other non-regular file types.
There was a problem hiding this comment.
Thanks — agree loadCredentials should be hardened. Before I implement, want to flag a tradeoff I ran into so we can align on direction.
Option A (hard-reject, what you suggested) — aligned with KeystoreCliUtils.isSafeRegularFile semantics.
Concern: WalletUtils.loadCredentials is a public API consumed transitively by SR startup (WitnessInitializer), the deprecated --keystore-factory, and third-party projects that depend on the crypto artifact (e.g. wallet-cli). Some production deployments legitimately use symlinks to organize keystores — e.g. /opt/tron/keystore/witness.json → /mnt/encrypted-volume/witness.json, or container volume-mount paths. A hard reject would silently break these on upgrade, surfacing as Refusing to follow symbolic link and preventing SR start.
Alternative B: log WARN but proceed — preserves compatibility, gives operators a signal to verify the target.
Alternative C: default-reject + opt-out via -Dtron.keystore.allow-symlink=true — secure-by-default with an escape hatch.
The attack model that makes hardening meaningful here is an operator being social-engineered (or config-injected) into pointing at a path an attacker controls — in which case hard-reject is the strongest signal. Against that, hard-reject incurs real cost for the class of legit users above, and the plugins-CLI helper has this same compatibility concern one layer up. I don't have a strong prior on which side of the tradeoff wins — curious how you see it, and happy to implement once we've picked a direction together.
There was a problem hiding this comment.
@barbatos2011 Great point — I had previously overlooked this real-world symlink scenario for SR users. From this perspective, option A (hard-reject) does not seem appropriate, because for legitimate deployments it could introduce silent breakage after upgrade, manifesting as “SR fails to start.”
Option B (WARN + proceed) has the smallest change surface and the best compatibility. The downside is that warnings can be easily ignored, especially on witness nodes where symlink resolution happens only once at startup and the warning may quickly scroll past.
Option C (default-reject + opt-out) provides a reasonable secure default, but introduces some additional code complexity, and requires SR users with legitimate symlink usage to adjust their startup configuration after upgrading.
I also compared how Ethereum consensus clients handle keystores. In https://github.com/sigp/lighthouse, when voting_keystore_path: /path/to/voting-keystore.json is explicitly configured, symlinks are supported — the keystore is opened directly via File::open(path), and the OS follows the symlink.
Considering that the keystore-factory feature is being deprecated and loadCredentials will mainly be used by SR going forward: for SR, loadCredentials is only needed when using localwitnesskeystore, which requires specifying a concrete keystore file (not just a directory), similar to Lighthouse’s voting_keystore_path.
Therefore, based on the above considerations, I would lean toward option B (WARN + proceed).
There was a problem hiding this comment.
Implemented Option B (warn + proceed) in commit df91f4534. Lighthouse parity: loadCredentials does an lstat with NOFOLLOW_LINKS, emits logger.warn if the path is a symbolic link, then proceeds with the existing readValue (which follows the symlink) — exactly mirroring the File::open(path) semantics in your Lighthouse pointer. Tests verify a symlinked keystore still loads end-to-end and that a regular-file roundtrip is unaffected.
| if (!KeystoreCliUtils.isSafeRegularFile(file, err)) { | ||
| continue; | ||
| } | ||
| try { | ||
| WalletFile walletFile = MAPPER.readValue(file, WalletFile.class); |
There was a problem hiding this comment.
It looks like the same pattern appears in three places:
KeystoreList.java:62–66KeystoreImport.java:172–176KeystoreUpdate.java:202–206
The root cause is that isSafeRegularFile uses Files.readAttributes(..., NOFOLLOW_LINKS) for the initial check, but the subsequent MAPPER.readValue(file, WalletFile.class) re-opens the file via FileInputStream, which follows symlinks. This creates a TOCTOU window where an attacker in a writable directory could swap the file with a symlink (e.g., to /etc/shadow or an SSH key) between the check and the read.
The proposed fix makes sense: using Files.newByteChannel(path, READ, NOFOLLOW_LINKS) and then MAPPER.readValue(bytes, ...) closes this gap by enforcing the check at open time.
There are two additional spots that seem to follow the same pattern and might be worth addressing as well:
KeystoreUpdate.java:141– reads the file again after lookup, and is followed by decrypt/write logicWalletUtils.java:150(loadCredentials) – a core entry point where callers may pass in files not validated byisSafeRegularFile
One small implementation note: MAX_FILE_SIZE = 1024 in KeystoreCliUtils appears to be tuned for password files. Since typical V3 keystore JSONs are ~500–700 bytes but could grow, it might be safer to introduce a separate MAX_KEYSTORE_SIZE (e.g., 4KB or 8KB) to avoid silently dropping valid entries in the future.
There was a problem hiding this comment.
Thanks — the TOCTOU analysis is correct. Files.readAttributes(..., NOFOLLOW_LINKS) is lstat and happens in a separate syscall from the subsequent MAPPER.readValue(file, ...), which internally uses FileInputStream → open(2) without O_NOFOLLOW. An attacker who can write in the keystore directory between those two calls can swap the file for a symlink and redirect the read. Your proposed fix — Files.newByteChannel(path, READ, NOFOLLOW_LINKS) so the flag lands on the open syscall itself — is the right mechanism; the kernel enforces atomically rather than a check-then-use pattern.
You've also mapped the scope correctly: the same pattern shows up at
KeystoreList/KeystoreImport.findExistingKeystore/KeystoreUpdate.findKeystoreByAddress(the three directory-scan sites already gated byisSafeRegularFile)KeystoreUpdate.callat line 141 — the secondreadValueafter the scan identified the targetWalletUtils.loadCredentials(#discussion_r3135278548)
One practical wrinkle on threat modeling: exploiting this TOCTOU requires the attacker to already have write access to the keystore directory (or to whatever user-supplied path the operator passes). At that point they can usually achieve the same effect by writing a malicious keystore directly, so the layer this fix adds is more defense-in-depth against a narrow "sticky-bit-dir" or "create-only" class of writable-directory configurations than a new-attack mitigation. Worth the change, but useful context on priority.
Re MAX_FILE_SIZE: agreed that today's 1024 is password-file-sized and not applied to keystore reads at all — once we move to the byte-channel form we'll need a separate MAX_KEYSTORE_SIZE. Planning on 8 KiB (covers typical scrypt-parameterized V3 keystores with headroom, still small enough to bound DoS memory cost on a hostile directory of planted files).
Linking to #discussion_r3135278548: this comment and that one describe the same underlying issue from two angles — the other one asks whether loadCredentials should refuse symlinks (policy), and this one specifies how to implement that refusal correctly at the syscall layer (mechanism). The implementation here depends on the outcome there: if we go hard-reject, your newByteChannel(..., NOFOLLOW_LINKS) is exactly what we'd use; if we go warn-only (for compat with SR deployments that use symlinked keystore paths), we'd deliberately allow the symlink through and the TOCTOU concern is scoped differently. Parking this for now until the direction on the sibling thread converges, then handling both together in one commit that touches all four call sites consistently.
There was a problem hiding this comment.
@barbatos2011 The code in KeystoreList.java, KeystoreImport.java, and KeystoreUpdate.java is currently only used by the plugin CLI, which is a different scenario from loadCredentials discussed above.
Given that, I would still recommend fixing the TOCTOU issue here.
There was a problem hiding this comment.
Fixed in commit 29d7d20e8. Replaced the lstat-then-readValue(File) pattern with KeystoreCliUtils.readKeystoreFile — one read, opening via Files.newByteChannel(path, READ, NOFOLLOW_LINKS) so O_NOFOLLOW is enforced atomically by the kernel at open(2). Caller then feeds the byte[] to MAPPER.readValue(byte[], Class). isSafeRegularFile is gone (subsumed). All four call sites switched: KeystoreList, KeystoreImport.findExistingKeystore, KeystoreUpdate.findKeystoreByAddress, plus the re-read at KeystoreUpdate.call line 141. Added MAX_KEYSTORE_SIZE = 8 KiB with an explicit oversized-file warning so a hostile directory of large planted files surfaces clearly rather than consuming memory silently.
| private static Scanner sharedStdinScanner; | ||
|
|
||
| /** | ||
| * Visible for testing: reset the cached Scanner so subsequent calls | ||
| * see a freshly rebound {@link System#in}. | ||
| */ | ||
| static synchronized void resetSharedStdinScanner() { | ||
| sharedStdinScanner = null; | ||
| } | ||
|
|
||
| private static synchronized Scanner getSharedStdinScanner() { |
There was a problem hiding this comment.
In WalletUtils.java:239–293, several methods leak CLI concerns into the crypto layer.
inputPassword() / inputPassword2Twice() directly print English prompts via System.out.println(...), and also rely on a mutable static sharedStdinScanner with a resetSharedStdinScanner() test hook. The crypto module should not handle user interaction.
Suggestion: move this logic down to a framework-level helper, or at least inject a PrintStream instead of using System.out directly.
There was a problem hiding this comment.
Thanks — you're right that the layering is off here. Context: the code wasn't introduced in this PR — the refactor commit that moved the keystore package from framework into crypto brought inputPassword / inputPassword2Twice along as-is, and this PR's inputPassword2Twice sharedStdinScanner fix left it there. That doesn't make it right to leave, but it frames this as cleanup I should have done at move time.
Want to align on direction before I implement — two options, with the tradeoffs below.
Option A (minimum — matches your "at least inject a PrintStream" fallback): add an inputPassword(PrintStream prompt) overload and route all prompts through it; keep inputPassword() delegating to inputPassword(System.out). Methods stay in WalletUtils. Zero API-compat risk. Doesn't remove the mutable-static sharedStdinScanner, but Scanner lifecycle is arguably a concurrency concern more than a presentation concern, so less clearly misplaced.
Option B (move to framework, your primary suggestion): relocate inputPassword, inputPassword2Twice, sharedStdinScanner, and resetSharedStdinScanner into a framework-level helper. stripPasswordLine stays at the crypto layer because plugins' KeystoreCliUtils.readPassword also uses it.
The reason I'm slightly leaning A is that B doesn't actually fix what you're flagging — it relocates the System.out hard-coding and the mutable static rather than eliminating them. The concerns ("library shouldn't handle user interaction", static mutable state in a shared utility) are design-level; a real fix would be a proper PasswordReader abstraction with injected Console/PrintStream, which is a bigger follow-up. Against that, B does remove the two methods from crypto's public surface, which is a cleanness win — but it's also a binary-incompatible break for any third-party tool that imports WalletUtils.inputPassword directly from the crypto artifact (wallet-cli and potentially others). I can't audit external consumers, and while it's unlikely anyone depends on these specific helpers (CLI interaction, not crypto primitives), it's a silent compat break I'd rather be deliberate about.
So A = small, zero-risk, minimum interpretation of your comment. B = more structural, removes the methods from crypto's ABI, but doesn't really solve the layering — just relocates it. Neither truly re-layers the design.
Does A meet what you had in mind, or do you want B's stricter module-boundary even at the ABI-compat cost? If you want the deeper re-layering (proper PasswordReader), I'd split that into a follow-up PR to keep this one from growing further.
There was a problem hiding this comment.
@barbatos2011 It seems the benefits of option A are relatively limited:
- Only addresses
System.out.printlntestability / output injection - Does not address
WalletUtilsreading from stdin - Does not address CLI state being held in the crypto layer
- Does not address the
sharedStdinScannertest hook residing in the crypto layer
Given that, I would suggest leaving this legacy issue as-is for now and addressing it in a future cleanup.
There was a problem hiding this comment.
Agreed — your point that A only addresses one of the four concerns is fair. Leaving inputPassword / inputPassword2Twice / sharedStdinScanner / resetSharedStdinScanner in WalletUtils as-is for this PR; will track a follow-up to do the proper re-layering (likely a PasswordReader abstraction in framework with injectable Console/PrintStream, so all four concerns get addressed together rather than papered over). Thanks for the patient triage on this thread.
| public static String generateFullNewWalletFile(String password, File destinationDirectory, | ||
| boolean ecKey) | ||
| throws NoSuchAlgorithmException, NoSuchProviderException, |
There was a problem hiding this comment.
There appears to be some dead code in WalletUtils.java:47–70, 162–187 that could be cleaned up:
generateFullNewWalletFilegenerateLightNewWalletFilegenerateNewWalletFile(String, File, boolean, boolean)getDefaultKeyDirectorygetMainnetKeyDirectorygetTestnetKeyDirectory
These methods do not seem to have any production callers, and the default directory logic follows an Ethereum-style path, which is not meaningful in the Tron context. It would be better to remove them all.
There was a problem hiding this comment.
Good catch — fixed in commit b89790c64.
Verified all six methods had zero production callers. The only reference was SupplementTest.testGet calling each one with no assertions — a coverage-padding pattern inherited from the pre-refactor state. Deleted:
generateFullNewWalletFile/generateLightNewWalletFile/generateNewWalletFile(4-arg)— thin wrappers that forwarded to the realgenerateWalletFile(String, SignInterface, File, boolean)(kept, actively used byKeystoreNew,KeystoreImport,KeystoreFactoryand several tests)getDefaultKeyDirectory/getMainnetKeyDirectory/getTestnetKeyDirectory— returned web3j-style Ethereum paths (~/.ethereum,~/Library/Ethereum,%APPDATA%/Ethereum), never correct in a TRON context and actually misleading
Also removed the five noop invocations from SupplementTest.testGet and dropped the now-unused import org.tron.keystore.WalletUtils; (the passwordValid static import is kept — still covered by testPasswordValid).
Smaller net change than it looks: -64 lines, 0 production behavior change, crypto module's public API surface shrinks to only what TRON actually uses.
The following methods had no production callers after the keystore refactor — the only reference was a coverage-padding test at SupplementTest.testGet (no assertions, just "call these to bump the coverage number") plus internal self-references: - generateFullNewWalletFile(String, File, boolean) - generateLightNewWalletFile(String, File, boolean) - generateNewWalletFile(String, File, boolean, boolean) - getDefaultKeyDirectory() / getDefaultKeyDirectory(String) - getTestnetKeyDirectory() - getMainnetKeyDirectory() The three generate* wrappers were identical calls into the real generateWalletFile(String, SignInterface, File, boolean) (which is kept and actively used). The three getDirectory helpers returned Ethereum- style paths (~/Library/Ethereum, %APPDATA%/Ethereum, ~/.ethereum) — hard-coded from the web3j origin of this code, never applicable in a TRON context, and capable of misleading anyone who tried to use them. Also drops the corresponding five noop invocations from SupplementTest.testGet and its now-unused WalletUtils import (the static import of passwordValid elsewhere in the file is preserved — it's still exercised by testPasswordValid). No production behavior change. Reduces the crypto module's public API surface and keeps the keystore library focused on what TRON actually uses.
Aligns WalletUtils.loadCredentials with the Lighthouse-style policy: follow the symlink (preserving compatibility with legitimate SR deployments that organize keystores via symlinks — e.g. encrypted- volume mounts, container volume bindings, /opt/tron → /mnt/secrets layouts), but emit a logger.warn so operators have a chance to notice if a path they did not expect to be a symlink has become one. Hard-rejecting would silently break "SR fails to start" on upgrade for operators using these legitimate patterns; warn-and-proceed surfaces the situation without forcing a downtime cliff. The lstat probe uses LinkOption.NOFOLLOW_LINKS so an attacker cannot hide the symlink by racing the check; if the lstat itself fails for unrelated reasons (permission, fs error) we silently fall through and let the subsequent readValue surface the real error. Tests verify a symlinked keystore still loads end-to-end (the Lighthouse parity path) and that a regular-file roundtrip is unaffected.
The plugins CLI scans (KeystoreList, KeystoreImport.findExistingKeystore,
KeystoreUpdate.findKeystoreByAddress) and the second keystore read in
KeystoreUpdate.call previously combined a NOFOLLOW lstat check with a
subsequent MAPPER.readValue(file, ...). Jackson's File overload uses
FileInputStream → open(2) without O_NOFOLLOW, so an attacker who can
write in the keystore directory could swap a regular file for a symlink
between the stat and the read.
Replace the check-then-use pattern with a single read that pushes the
NOFOLLOW flag down to the open syscall:
- New KeystoreCliUtils.readKeystoreFile(File, PrintWriter) opens via
Files.newByteChannel(path, READ, NOFOLLOW_LINKS), reads at most
MAX_KEYSTORE_SIZE (8 KiB) bytes, and returns the byte[] for the
caller to feed into MAPPER.readValue(byte[], Class) — no second
file handle, no follow-symlink window.
- Drop isSafeRegularFile (now subsumed by readKeystoreFile).
- All four call sites switched: list, import duplicate scan, update
lookup scan, and update's re-read after lookup at KeystoreUpdate
.call line 141.
Also adds an explicit oversized-file warning ("skipping oversized
file") so a hostile directory of large planted files surfaces clearly
rather than silently consuming memory.
Note: this hardens the plugins CLI scenario specifically. The SR
startup path through WalletUtils.loadCredentials uses a different
policy (warn + follow, see commit df91f45) — that path explicitly
configures a single keystore file, parallel to Lighthouse's
voting_keystore_path, where symlink support is part of the contract.
| java -jar Toolkit.jar keystore import [-h] [--keystore-dir=<dir>] [--password-file=<file>] [--private-key-file=<file>] [--sm2] [--json] | ||
| # examples | ||
| java -jar Toolkit.jar keystore import # interactive prompt | ||
| java -jar Toolkit.jar keystore import --private-key-file key.txt --json # from file with JSON output |
There was a problem hiding this comment.
The option names in the README appear to be incorrect and may mislead users.
In plugins/README.md:178, 181, 204:
- The docs mention
--private-key-file, but the actual option inKeystoreImport.java:41is--key-file. - The docs mention
--new-password-file(forkeystore update), butKeystoreUpdate.java:45–47only defines a single--password-file(two-line format:oldPassword\nnewPassword); there is no--new-password-file.
There was a problem hiding this comment.
Good catch — fixed in commit d3530b1dc.
keystore import:--private-key-file→--key-file(matchesKeystoreImport.java:41); also surfaced the previously-missing--forceflag in the synopsis and Common Options.keystore update: removed the nonexistent--new-password-fileand added an inline note that--password-fileforupdateexpects a two-line file (current password, then new password).
Verified all option names in the README against @Option(names = ...) declarations across the four command classes — they now line up. Pure documentation change, no code touched.
| String[] lines = content.split("\\r?\\n|\\r"); | ||
| if (lines.length < 2) { | ||
| err.println( | ||
| "Password file must contain old and new passwords" | ||
| + " on separate lines."); | ||
| return 1; | ||
| } | ||
| oldPassword = WalletUtils.stripPasswordLine(lines[0]); | ||
| newPassword = WalletUtils.stripPasswordLine(lines[1]); |
There was a problem hiding this comment.
There appears to be an issue where a third line in the password file is silently ignored.
After content.split("\\r?\\n|\\r"), the code only checks lines.length < 2 and then reads lines[0] and lines[1]. As a result, input like old\nnew\nconfirm is accepted, but the third line (confirm) is silently ignored, and the password is changed to the second line (new).
One subtle point: String.split(regex) in Java drops trailing empty fields by default, so a trailing newline (old\nnew\n) still results in 2 lines, which aligns with the intent of trimming empty trailing lines. However, any non-empty third line will be ignored.
Suggestion:
- Require exactly 2 effective lines after parsing; otherwise return an error.
- Update the error message to clearly state that
keystore update --password-fileexpects exactly two lines: current password and new password (no confirm line). - Add a regression test for the three-line case (
old\nnew\nconfirm) to ensure it fails and the original keystore remains decryptable with the old password.
There was a problem hiding this comment.
Fixed in 8aa8df1aa.
- Tightened the check to
lines.length != 2and made the error message explicit:"Password file must contain exactly two lines: current password on the first line and new password on the second line (no confirmation line)." - Added a comment explaining that Java's zero-limit
String.splitalready drops trailing empty strings, soold\nnewandold\nnew\nboth still parse as length 2 (accepted), whileold\nnew\nconfirmis now rejected instead of silently truncated. - Added regression test
testUpdatePasswordFileThreeLinesthat feedsold\nnew\nconfirm, asserts the command exits non-zero with the new message, and verifies the original keystore is byte-for-byte unchanged and still decryptable with the old password. - Updated the existing
testUpdatePasswordFileOnlyOneLineassertion to match the new message.
| // Legacy-truncation hint: if this keystore was created with | ||
| // `FullNode.jar --keystore-factory` in non-TTY mode (e.g. | ||
| // `echo PASS | java ...`), the legacy code encrypted with only | ||
| // the first whitespace-separated word of the password. Emit the | ||
| // tip only when the entered password has internal whitespace — | ||
| // otherwise truncation cannot be the cause. | ||
| if (e instanceof CipherException && pwd != null && pwd.matches(".*\\s.*")) { | ||
| logger.error( | ||
| "Tip: keystores created via `FullNode.jar --keystore-factory` in " | ||
| + "non-TTY mode were encrypted with only the first " | ||
| + "whitespace-separated word of the password. Try restarting " | ||
| + "with only that first word as `-p`, then reset the password " | ||
| + "via `java -jar Toolkit.jar keystore update`."); | ||
| } |
There was a problem hiding this comment.
There seems to be missing test coverage for key security paths in WitnessInitializer.
-
In
f6811241a, a “legacy-truncation” warning was added inWitnessInitializer.java:89–103, butWitnessInitializerKeystoreTest.javadoes not cover this behavior (neither the whitespace nor non-whitespace cases). The Toolkit side (KeystoreUpdateTest.java:677–738) covers both branches, but the witness path currently has no coverage. -
In
368d10419, an address-mismatch check was added inWallet.decrypt, butWitnessInitializerKeystoreTest.java:73–85only covers the happy path. There is no test exercising a tampered keystore case (e.g., declared address X but actually encrypted for Y) through the witness loading path.
Since the SR startup path is the primary consumer of these fixes, it would benefit from dedicated regression tests here.
There was a problem hiding this comment.
Fixed in 4dbc8ce93.
Added three regression tests to WitnessInitializerKeystoreTest:
testLegacyTruncationTipFiresOnWhitespacePassword— supplies a wrong password containing whitespace, assertsTronError(WITNESS_KEYSTORE_LOAD)is thrown, and uses a LogbackListAppenderattached to theWitnessInitializerlogger to verify the tip ("first whitespace-separated word") was emitted.testLegacyTruncationTipSuppressedOnNoWhitespacePassword— supplies a wrong password without whitespace, asserts the load failure is still logged ("Witness node start failed"), and verifies the tip is NOT emitted (so it doesn't add noise on the common wrong-password case).testTamperedKeystoreRejectedAtSrLoading— generates a real keystore, then rewrites the JSONaddressfield to a value that does not match the address derived from the encrypted private key. Asserts the SR startup path throwsTronError(WITNESS_KEYSTORE_LOAD)whose cause is aCipherExceptioncarrying "address mismatch".
These mirror the Toolkit-side coverage at KeystoreUpdateTest.java:677-738 (legacy tip) and KeystoreUpdateTest.java:553-591 (tampered keystore), now exercised through the witness loading path.
A small attachAppender / detachAppender / renderLogs helper is added to capture log events without bringing in any new dependency (Logback is already on the test classpath).
Two CLI flags in plugins/README.md did not match the actual @option declarations in the picocli command classes — users following the docs verbatim would hit "Unknown option" errors: - `keystore import` was documented with `--private-key-file`, but the actual option in KeystoreImport.java:41 is `--key-file`. Updated the full-command synopsis and the example accordingly, and added the missing `--force` flag (used by the duplicate-address override path). - `keystore update` was documented with a separate `--new-password-file`, but KeystoreUpdate.java only defines a single `--password-file` whose contents are a two-line file (current password, then new password). Removed the bogus flag and added an explicit note next to the synopsis describing the expected file format, plus an updated entry in the Common Options list. No code changes — purely documentation alignment so the README matches the implemented CLI surface.
Previously `keystore update --password-file` checked `lines.length < 2`,
which silently discarded any third (or further) line. A user who
mistakenly added a confirmation line, or pointed the flag at the wrong
file altogether, would have their input misinterpreted instead of
diagnosed. Tighten the check to require exactly two lines and make the
error message explicit ("no confirmation line"). Java's `String.split`
zero-limit form already drops trailing empty strings, so `old\nnew` and
`old\nnew\n` both still parse as length 2 and remain accepted.
Add a regression test that feeds `old\nnew\nconfirm`, asserts the
command exits non-zero with the new message, and verifies the original
keystore file is byte-for-byte unchanged and still decryptable with the
old password.
The Toolkit side (KeystoreUpdateTest) already covered both the legacy truncation tip behavior (whitespace and non-whitespace branches) and the address-mismatch defense, but the SR startup path through WitnessInitializer.initFromKeystore had no equivalent coverage. Since witnesses are the primary consumer of these fixes, exercise them here too. Three new tests: * testLegacyTruncationTipFiresOnWhitespacePassword — wrong password containing whitespace must trigger the FullNode keystore-factory hint, with TronError code WITNESS_KEYSTORE_LOAD. * testLegacyTruncationTipSuppressedOnNoWhitespacePassword — wrong password without whitespace must still log the load failure but must NOT surface the legacy tip (otherwise it would be noise on the common wrong-password case). * testTamperedKeystoreRejectedAtSrLoading — a keystore whose declared address does not match the address derived from its encrypted private key must be rejected with a CipherException carrying "address mismatch", wrapped in TronError(WITNESS_KEYSTORE_LOAD). A small Logback ListAppender helper attaches to the WitnessInitializer logger to capture log output for the tip assertions.
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