feat: use keystore to store private keys#235
Conversation
WalkthroughThe changes transition the application's key management from handling raw private keys and keypairs to using encrypted keystore files with password protection. This involves removing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant BaseAction
participant FileSystem
participant ethers
User->>CLI: Run keygen create command
CLI->>BaseAction: createKeypairAction(options)
BaseAction->>BaseAction: Prompt user for password (twice)
BaseAction->>ethers: Create random wallet
BaseAction->>ethers: Encrypt wallet with password
ethers-->>BaseAction: Encrypted keystore JSON
BaseAction->>FileSystem: Write keystore JSON to file
BaseAction->>CLI: Show success message
sequenceDiagram
participant User
participant CLI
participant BaseAction
participant FileSystem
participant ethers
User->>CLI: Run command needing private key
CLI->>BaseAction: getPrivateKey()
BaseAction->>FileSystem: Check for keystore file
alt Keystore exists and valid
BaseAction->>User: Prompt for password (up to 3 tries)
User-->>BaseAction: Enter password
BaseAction->>ethers: Decrypt keystore with password
ethers-->>BaseAction: Private key
else Keystore missing/invalid
BaseAction->>User: Prompt to create new keypair
User-->>BaseAction: Confirm
BaseAction->>BaseAction: createKeypair(...)
BaseAction->>ethers: Create and encrypt new wallet
ethers-->>BaseAction: Encrypted keystore JSON
BaseAction->>FileSystem: Write keystore JSON to file
BaseAction->>ethers: Return new private key
end
BaseAction-->>CLI: Return private key
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/lib/actions/BaseAction.ts (2)
39-46: Consider enhancing keystore format validation.While the current validation is good, consider adding a check to ensure the encrypted field contains valid JSON that ethers.js expects.
private isValidKeystoreFormat(data: any): data is KeystoreData { return ( data && data.version === 1 && typeof data.encrypted === "string" && - typeof data.address === "string" + typeof data.address === "string" && + data.encrypted.length > 0 && + (() => { + try { + JSON.parse(data.encrypted); + return true; + } catch { + return false; + } + })() ); }
93-129: Good implementation with room for UX improvements.The keystore creation follows security best practices. Consider these enhancements:
- Show progress during encryption as it can be slow
- Consider adding password strength requirements beyond length
- Display success message with the address after creation
protected async createKeypair(outputPath: string, overwrite: boolean): Promise<string> { const finalOutputPath = this.getFilePath(outputPath); this.stopSpinner(); if (existsSync(finalOutputPath) && !overwrite) { this.failSpinner(`The file at ${finalOutputPath} already exists. Use the '--overwrite' option to replace it.`); process.exit(1); } const wallet = ethers.Wallet.createRandom(); const password = await this.promptPassword("Enter password to encrypt your keystore:"); const confirmPassword = await this.promptPassword("Confirm password:"); if (password !== confirmPassword) { this.failSpinner("Passwords do not match"); process.exit(1); } if (password.length < 8) { this.failSpinner("Password must be at least 8 characters long"); process.exit(1); } + this.startSpinner("Encrypting keystore..."); const encryptedJson = await wallet.encrypt(password); + this.stopSpinner(); const keystoreData: KeystoreData = { version: 1, encrypted: encryptedJson, address: wallet.address, }; writeFileSync(finalOutputPath, JSON.stringify(keystoreData, null, 2)); this.writeConfig('keyPairPath', finalOutputPath); + this.logSuccess(`Keystore created successfully at ${finalOutputPath}`); + this.logInfo(`Address: ${wallet.address}`); + return wallet.privateKey; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/commands/keygen/create.ts(1 hunks)src/commands/keygen/index.ts(1 hunks)src/lib/accounts/KeypairManager.ts(0 hunks)src/lib/actions/BaseAction.ts(2 hunks)src/lib/interfaces/KeystoreData.ts(1 hunks)tests/actions/create.test.ts(1 hunks)tests/libs/accounts/KeypairManager.test.ts(0 hunks)tests/libs/baseAction.test.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/lib/accounts/KeypairManager.ts
- tests/libs/accounts/KeypairManager.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/commands/keygen/index.ts (1)
src/commands/keygen/create.ts (2)
CreateKeypairOptions(3-6)KeypairCreator(8-23)
tests/libs/baseAction.test.ts (1)
src/lib/actions/BaseAction.ts (1)
BaseAction(13-214)
src/lib/actions/BaseAction.ts (2)
src/lib/config/ConfigFileManager.ts (1)
ConfigFileManager(5-51)src/lib/interfaces/KeystoreData.ts (1)
KeystoreData(1-5)
🪛 Gitleaks (8.26.0)
tests/libs/baseAction.test.ts
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
src/lib/interfaces/KeystoreData.ts (1)
1-5: LGTM!The
KeystoreDatainterface is well-structured and aligns with the encrypted keystore format requirements outlined in the PR objectives.src/commands/keygen/index.ts (1)
12-17: Command updates are consistent with keystore implementation.The terminology changes and async handler conversion properly support the new encrypted keystore functionality.
tests/actions/create.test.ts (1)
10-44: Test refactoring aligns well with the new async implementation.The tests properly mock internal methods and validate the expected behavior for both success and error scenarios with appropriate spinner messages.
src/commands/keygen/create.ts (1)
13-22: Async implementation correctly handles keystore creation.The method properly awaits the keystore creation process and provides clear user feedback through spinner messages.
tests/libs/baseAction.test.ts (2)
29-29: Test mock data is appropriate.The static analysis warning is a false positive - this is clearly a mock private key value used for testing purposes.
198-371: Comprehensive test coverage for keystore operations.The test suite thoroughly covers all aspects of the new keystore functionality including:
- Password prompting and validation
- Keystore decryption with retry logic
- Error handling and edge cases
- File operations and overwrite scenarios
Excellent coverage of the security-critical components!
src/lib/actions/BaseAction.ts (3)
9-11: LGTM! Appropriate imports for keystore functionality.The addition of ethers for wallet encryption and specific fs imports for file operations aligns well with the new keystore implementation.
55-66: LGTM! Proper integration with async private key retrieval.The method correctly awaits the new async
getPrivateKey()method, maintaining compatibility with the encrypted keystore approach.
131-147: LGTM! Secure password prompting implementation.Good use of inquirer with password masking and validation. The empty password check prevents accidental security issues.
|
|
||
| const wallet = ethers.Wallet.createRandom(); | ||
|
|
||
| const password = await this.promptPassword("Enter password to encrypt your keystore:"); |
There was a problem hiding this comment.
@epsjunior here we must explain the password requirements
| return wallet.privateKey; | ||
| } catch (error) { | ||
| if (attempt >= 3) { | ||
| this.failSpinner("Maximum password attempts exceeded (3/3)."); |
There was a problem hiding this comment.
@epsjunior how is this preventing a bad actor to try again? Shouldn't the file be removed or something after x unsuccessful attempts?
There was a problem hiding this comment.
Hi @cristiam86,
Since we're using ethers.js, the wallet is encrypted using the Ethereum keystore V3 format with scrypt as the KDF (N = 131072 by default). That makes each decryption attempt slow (~0.5–1s), which is the real protection against brute-force attacks.
Deleting the file after failed attempts wouldn’t prevent a bad actor — they could just copy the keystore file and run the command again. So limiting to 3 retries and exiting is mostly a UX choice, not a security layer.
2 participants
🔐 Implement Encrypted Keystore Security
Overview
Replaces the insecure
keypairManagersystem with enterprise-grade encrypted keystore functionality using ethers.js HDNodeWallet encryption. Centralizes all keystore logic in BaseAction to eliminate code duplication and prevent cyclic import issues across commands.🚀 Key Features
Security Enhancements
Architecture Improvements
error.message.includes())UX Improvements
🛠️ Technical Implementation
Centralized BaseAction Design
Benefits of Centralization
Security Features
Keystore Format
{ "version": 1, "encrypted": "ethers-encrypted-json-wallet", "address": "0x..." }📋 Breaking Changes
keypairManagerclass and all referencesgetPrivateKey()now handles encrypted keystore directlycreateKeypair()returns private key instead of void🧪 Testing Coverage
promptPassword, validation, retry mechanisms🔄 Migration Path
📈 Benefits
Files Changed:
src/lib/actions/BaseAction.ts- Centralized keystore implementationsrc/commands/keygen/create.ts- Updated integrationsrc/lib/interfaces/KeystoreData.ts- New type definitionstests/libs/baseAction.test.ts- Comprehensive test coverageFiles Removed:
src/lib/accounts/KeypairManager.ts- Logic moved to BaseActiontests/libs/accounts/KeypairManager.test.ts- Tests consolidatedSecurity Review: ✅ All private keys encrypted, proper attempt limiting, no plain text storage
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests