feat: implement os keychain support for secure private key storage #243
Conversation
WalkthroughThis change replaces the CLI's decrypted private key caching mechanism, moving from temporary file storage to secure OS keychain storage using the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant KeychainManager
participant FileSystem
User->>CLI: Run 'keygen unlock'
CLI->>KeychainManager: isKeychainAvailable()
KeychainManager-->>CLI: true/false
alt Keychain available
CLI->>FileSystem: Check keystore file exists
FileSystem-->>CLI: exists/doesn't exist
alt Keystore exists
CLI->>User: Prompt for password
User-->>CLI: Enter password
CLI->>FileSystem: Read keystore
CLI->>CLI: Decrypt keystore
CLI->>KeychainManager: storePrivateKey(privateKey)
KeychainManager-->>CLI: Success/Failure
else Keystore missing
CLI-->>User: Show error
end
else Keychain unavailable
CLI-->>User: Show error
end
sequenceDiagram
participant User
participant CLI
participant KeychainManager
User->>CLI: Run 'keygen lock'
CLI->>KeychainManager: isKeychainAvailable()
KeychainManager-->>CLI: true/false
alt Keychain available
CLI->>KeychainManager: getPrivateKey()
KeychainManager-->>CLI: privateKey/null
alt Private key exists
CLI->>KeychainManager: removePrivateKey()
KeychainManager-->>CLI: Success/Failure
else No cached key
CLI-->>User: Wallet already locked
end
else Keychain unavailable
CLI-->>User: Show error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code Graph Analysis (1)tests/actions/unlock.test.ts (1)
🪛 Gitleaks (8.27.2)tests/actions/unlock.test.ts19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
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: 5
🧹 Nitpick comments (8)
tests/libs/baseAction.test.ts (1)
86-86: Clean up empty line from removed temp file mocking.This empty line appears to be leftover from removing temp file method mocking. Consider removing it for cleaner code.
-src/commands/keygen/index.ts (2)
22-28: Consider adding error handling for the unlock commandThe command directly awaits the action execution without handling potential errors. While the action classes handle their own errors, the CLI should gracefully handle any unhandled exceptions.
Consider wrapping the action execution in a try-catch block:
keygenCommand .command("unlock") .description("Unlock your wallet by storing the decrypted private key in OS keychain") .action(async () => { - const unlockAction = new UnlockAction(); - await unlockAction.execute(); + try { + const unlockAction = new UnlockAction(); + await unlockAction.execute(); + } catch (error) { + console.error("Failed to execute unlock command:", error); + process.exit(1); + } });
30-36: Consider adding error handling for the lock commandSimilar to the unlock command, consider adding error handling to ensure graceful failure handling.
Apply the same error handling pattern as suggested for the unlock command.
tests/actions/lock.test.ts (1)
7-21: Consider extracting mock setup to reduce duplicationThe mock setup is comprehensive but quite verbose. Consider extracting common mock patterns to improve maintainability.
Create a helper function for common mock setup:
+ function setupLockActionMocks(lockAction: LockAction) { + vi.spyOn(lockAction as any, "startSpinner").mockImplementation(() => {}); + vi.spyOn(lockAction as any, "setSpinnerText").mockImplementation(() => {}); + vi.spyOn(lockAction as any, "succeedSpinner").mockImplementation(() => {}); + vi.spyOn(lockAction as any, "failSpinner").mockImplementation(() => {}); + + vi.spyOn(lockAction["keychainManager"], "isKeychainAvailable").mockResolvedValue(true); + vi.spyOn(lockAction["keychainManager"], "getPrivateKey").mockResolvedValue("test-private-key"); + vi.spyOn(lockAction["keychainManager"], "removePrivateKey").mockResolvedValue(true); + } beforeEach(() => { vi.clearAllMocks(); lockAction = new LockAction(); - - // Mock the BaseAction methods - vi.spyOn(lockAction as any, "startSpinner").mockImplementation(() => {}); - // ... other mocks + setupLockActionMocks(lockAction); });tests/actions/unlock.test.ts (1)
13-18: Consider using more realistic mock dataThe mock keystore data could be more realistic to better simulate actual encrypted keystores.
Consider using more realistic mock data:
const mockKeystoreData = { - encrypted: '{"address":"test","crypto":{"cipher":"aes-128-ctr"}}' + encrypted: '{"address":"0x742f35Cc6B5C23d6dE4dcB913c95eD6b96aE78B8","crypto":{"cipher":"aes-128-ctr","ciphertext":"...","cipherparams":{"iv":"..."},"kdf":"scrypt","kdfparams":{"dklen":32,"n":8192,"p":1,"r":8,"salt":"..."},"mac":"..."},"id":"...","version":3}' };src/lib/config/KeychainManager.ts (1)
7-7: Remove unnecessary empty constructorThe empty constructor adds no value and can be removed to simplify the class.
- constructor() {}src/commands/keygen/unlock.ts (2)
29-29: Consider validating private key before storage.While ethers.js provides the private key after successful decryption, adding an explicit validation step could improve robustness.
const wallet = await ethers.Wallet.fromEncryptedJson(keystoreData.encrypted, password); + + if (!wallet.privateKey || wallet.privateKey.length !== 66 || !wallet.privateKey.startsWith('0x')) { + this.failSpinner("Invalid private key format after decryption."); + return; + } await this.keychainManager.storePrivateKey(wallet.privateKey);
32-32: Improve error message specificity.The generic "Failed to unlock wallet" message could be more helpful by providing context about the specific failure.
- this.failSpinner("Failed to unlock wallet.", error); + if (error instanceof Error && error.message.includes('invalid password')) { + this.failSpinner("Invalid password provided. Please try again with the correct password."); + } else if (error instanceof Error && error.message.includes('keychain')) { + this.failSpinner("Failed to store private key in OS keychain.", error); + } else { + this.failSpinner("Failed to unlock wallet. Please check your keystore file and password.", error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
README.md(1 hunks)esbuild.config.dev.js(1 hunks)esbuild.config.prod.js(1 hunks)package.json(1 hunks)src/commands/keygen/index.ts(2 hunks)src/commands/keygen/lock.ts(1 hunks)src/commands/keygen/unlock.ts(1 hunks)src/lib/actions/BaseAction.ts(4 hunks)src/lib/config/ConfigFileManager.ts(0 hunks)src/lib/config/KeychainManager.ts(1 hunks)tests/actions/lock.test.ts(1 hunks)tests/actions/unlock.test.ts(1 hunks)tests/commands/keygen.test.ts(2 hunks)tests/libs/baseAction.test.ts(2 hunks)tests/libs/configFileManager.test.ts(0 hunks)tests/libs/keychainManager.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- tests/libs/configFileManager.test.ts
- src/lib/config/ConfigFileManager.ts
🧰 Additional context used
🧠 Learnings (2)
package.json (1)
Learnt from: epsjunior
PR: #239
File: package.json:60-66
Timestamp: 2025-07-10T23:50:34.628Z
Learning: In the genlayer-cli project, dotenv is used with manual parsing via dotenv.parse() rather than automatic loading via dotenv.config(), so warnings about implicit .env.local auto-loading changes in dotenv v17 are not applicable to this project.
README.md (1)
Learnt from: epsjunior
PR: #239
File: package.json:60-66
Timestamp: 2025-07-10T23:50:34.628Z
Learning: In the genlayer-cli project, dotenv is used with manual parsing via dotenv.parse() rather than automatic loading via dotenv.config(), so warnings about implicit .env.local auto-loading changes in dotenv v17 are not applicable to this project.
🧬 Code Graph Analysis (7)
tests/commands/keygen.test.ts (3)
src/commands/keygen/index.ts (1)
initializeKeygenCommands(6-39)src/commands/keygen/unlock.ts (1)
UnlockAction(5-35)src/commands/keygen/lock.ts (1)
LockAction(3-31)
tests/libs/keychainManager.test.ts (1)
src/lib/config/KeychainManager.ts (1)
KeychainManager(3-29)
src/commands/keygen/unlock.ts (1)
src/lib/actions/BaseAction.ts (1)
BaseAction(14-236)
src/commands/keygen/lock.ts (1)
src/lib/actions/BaseAction.ts (1)
BaseAction(14-236)
tests/actions/lock.test.ts (1)
src/commands/keygen/lock.ts (1)
LockAction(3-31)
src/commands/keygen/index.ts (2)
src/commands/keygen/unlock.ts (1)
UnlockAction(5-35)src/commands/keygen/lock.ts (1)
LockAction(3-31)
src/lib/actions/BaseAction.ts (1)
src/lib/config/KeychainManager.ts (1)
KeychainManager(3-29)
🪛 GitHub Actions: CI Workflow
tests/commands/keygen.test.ts
[error] Test suite failed during 'npm run test:coverage'.
tests/libs/baseAction.test.ts
[error] Test suite failed during 'npm run test:coverage'.
tests/libs/keychainManager.test.ts
[error] Test suite failed during 'npm run test:coverage'.
tests/actions/lock.test.ts
[error] Test suite failed during 'npm run test:coverage'.
tests/actions/unlock.test.ts
[error] Test suite failed during 'npm run test:coverage'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sherlock AI Review
🔇 Additional comments (30)
package.json (1)
69-69: Approve keytar@^7.9.0 – no known vulnerabilities
- keytar@7.9.0 is the latest published version and no security advisories were found in the npm registry.
- keytar does not declare an
enginesfield, nor does your project’spackage.json. If your project targets specific Node.js versions, consider adding an"engines"entry to ensure compatibility.esbuild.config.prod.js (1)
14-14: LGTM!Correctly marking
keytaras external is necessary since it's a native module that cannot be bundled. This follows the established pattern for other native dependencies.esbuild.config.dev.js (1)
14-14: LGTM!Properly marks
keytaras external in the development build configuration, maintaining consistency with the production config.README.md (1)
15-32: Excellent documentation addition!The Linux Dependencies section is comprehensive and well-structured. It covers the major Linux distributions and clearly explains why
libsecretis required for the keychain functionality. This will help users avoid common installation issues.tests/libs/baseAction.test.ts (2)
290-295: LGTM! Test correctly updated for keychain functionality.The test properly reflects the change from temp file caching to keychain-based private key storage. The spy on
keychainManager.getPrivateKeyand the assertion are appropriate.
1-479: Verify and fix the CI coverage configurationThe new keychain integration already has corresponding tests (so missing test coverage isn’t the root cause). The pipeline error (
sh: 1: vitest: not found) suggests Vitest isn’t being invoked correctly in your CI environment.• Existing test files for keychain functionality:
- tests/libs/keychainManager.test.ts
- tests/actions/lock.test.ts
- tests/actions/unlock.test.ts
Recommendations:
- Ensure
vitestis listed in your project’s devDependencies and installed in CI (npm ci/yarn install).- Update your coverage script to explicitly invoke Vitest, e.g.
"scripts": { "test:coverage": "npx vitest run --coverage" }- Rerun the pipeline to confirm coverage now reports correctly.
tests/commands/keygen.test.ts (2)
5-6: LGTM: Proper imports for new action classesThe imports correctly reference the new UnlockAction and LockAction classes that implement the keychain-based wallet management functionality.
9-10: LGTM: Consistent mocking patternThe mocking approach follows the existing pattern established for the KeypairCreator mock.
src/commands/keygen/index.ts (1)
3-4: LGTM: Clean imports for action classesThe imports correctly reference the new UnlockAction and LockAction classes that handle the keychain operations.
tests/actions/lock.test.ts (5)
1-3: LGTM: Proper test importsThe imports correctly include all necessary testing utilities and the LockAction class being tested.
27-37: LGTM: Comprehensive success scenario testThis test properly verifies the complete flow for successful wallet locking, including all spinner updates and keychain interactions.
39-47: LGTM: Proper keychain unavailability handlingThe test correctly verifies that when keychain is unavailable, the appropriate error message is shown and no keychain operations are attempted.
49-56: LGTM: Handles already locked wallet scenarioThis test properly covers the case where no cached key exists, ensuring the appropriate success message is displayed.
58-65: LGTM: Proper error handling testThe test correctly verifies that errors during key removal are properly caught and displayed to the user.
tests/actions/unlock.test.ts (7)
1-9: LGTM: Proper mocking setup for dependenciesThe imports and mocks correctly cover all external dependencies including fs operations, ethers wallet functionality, and user input prompts.
47-61: LGTM: Comprehensive success scenario testThis test thoroughly verifies the complete unlock flow, including all necessary checks, file operations, wallet decryption, and keychain storage.
63-71: LGTM: Proper keychain unavailability handlingThe test correctly verifies that when keychain is unavailable, the process stops early without attempting sensitive operations.
73-80: LGTM: Handles missing keystore configurationThis test properly covers the scenario where no keystore path is configured.
82-89: LGTM: Handles non-existent keystore fileThe test correctly verifies behavior when the keystore file doesn't exist on disk.
91-99: LGTM: Proper error handling for decryption failuresThis test ensures that wallet decryption errors are properly caught and handled without attempting keychain storage.
101-108: LGTM: Handles keychain storage errorsThe test properly verifies error handling when keychain storage fails after successful wallet decryption.
src/lib/config/KeychainManager.ts (4)
1-1: LGTM: Proper keytar importThe default import correctly imports the keytar library for cross-platform keychain access.
4-5: LGTM: Well-defined service constantsThe static constants provide clear identifiers for keychain operations and follow a consistent naming pattern.
22-24: LGTM: Clean private key retrievalThe method properly retrieves the private key from the keychain and returns null if not found.
26-28: LGTM: Proper private key removalThe method correctly removes the private key from the keychain and returns the success status.
src/commands/keygen/lock.ts (1)
3-31: LGTM! Well-implemented lock functionality.The
LockActionclass follows good practices:
- Early validation of keychain availability
- Clear user feedback at each step
- Proper error handling for keychain operations
- Graceful handling of already-locked state
The implementation is consistent with the
UnlockActionpattern and provides a clean user experience.tests/libs/keychainManager.test.ts (1)
1-112: KeychainManager tests are solid; CI failure likely stems from pipeline configurationThe test suite in
tests/libs/keychainManager.test.ts
- Covers every
KeychainManagermethod (availability check, store, retrieve, remove)- Mocks
keytarcorrectly for success, failure, and edge cases- No import or duplicate-test conflicts found
The pipeline error appears unrelated to the test code itself. Please review your CI setup (e.g., ensure
vitestandkeytarare installed in the CI environment, check your test runner configuration) to resolve the failure.src/lib/actions/BaseAction.ts (3)
2-2: LGTM! Clean integration of KeychainManager.The addition of
KeychainManagerimport, property declaration, and initialization follows good practices and maintains consistency with the existing codebase structure.Also applies to: 21-21, 26-26
103-105: Excellent refactoring of private key caching mechanism.The transition from temporary file storage to secure keychain storage is well-implemented:
- Maintains the same fallback logic (cached key → decrypt keystore)
- Proper async/await handling
- Preserves existing error handling flow
This significantly improves security by eliminating plaintext private keys stored in temporary files.
148-148: Appropriate keychain cleanup in keypair creation.Clearing the cached private key from the keychain when creating a new keypair ensures no stale keys remain and maintains security hygiene.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/validate-code.yml (1)
22-24: Trim the package install to speed up CI.Using
--no-install-recommendsprevents Apt from pulling in unrelated ancillary packages, saving several seconds and a few MB on every matrix run.- run: sudo apt-get update && sudo apt-get install -y libsecret-1-0 + run: | + sudo apt-get update -y + sudo apt-get install -y --no-install-recommends libsecret-1-0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/validate-code.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sherlock AI Review
Add OS Keychain Integration for Secure Private Key Management
Summary
Implements secure private key storage using the operating system's native keychain (macOS Keychain, Windows Credential Manager, GNOME Keyring) to eliminate password prompts for write operations while maintaining security. This enhancement provides a user-friendly unlock/lock workflow and automatic key retrieval, significantly improving developer experience and workflow efficiency.
Changes
🚀 New Features
genlayer keygen unlockandgenlayer keygen lockcommands for explicit key management🎯 Problem Solved
Issue: The GenLayer CLI was asking for keystore password on every write operation, which is inconvenient and disrupts workflow efficiency.
Solution:
genlayer keygen unlockto store the private key securely in OS keychaingenlayer keygen lockto remove the key from keychain🔧 Infrastructure Updates
keytarlibrary for reliable cross-platform keychain access🏗️ Implementation Details
genlayer-cli) and account (default-user) for key storage📁 File Structure
🔐 Security Features
🧪 Testing
KeychainManager Tests (
tests/libs/keychainManager.test.ts):BaseAction Tests (
tests/libs/baseAction.test.ts):Action Tests (Various action test files):
🔧 Usage
Unlock Wallet (Store Key in Keychain)
Write Operations (No Password Required)
Lock Wallet (Remove Key from Keychain)
✨ Code Quality
🎯 Authentication Flow
📊 Performance Benefits
🛡️ Security Considerations
🔗 Dependencies
BaseActionpatterns🛡️ Backward Compatibility
Testing: ✅ All tests pass with 100% coverage on new code
Security: ✅ OS-level security, user isolation, explicit control
Performance: ✅ Zero authentication for writes after unlock
Cross-Platform: ✅ Works on macOS, Windows, and Linux
User Experience: ✅ Major workflow improvement with explicit unlock/lock control
Future-Proof: ✅ Extensible keychain integration for additional features
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores