feat(ui-react): add Secure Vault for encrypted SSH private key storage#5903
feat(ui-react): add Secure Vault for encrypted SSH private key storage#5903
Conversation
06e4ad4 to
ab6b7e7
Compare
ab6b7e7 to
78bbe9f
Compare
|
/review |
|
Claude finished @gustavosbarreto's task in 8m 48s —— View job Code Review CompleteReviewed 39 files across code quality, security, testing, TypeScript/React patterns, and architecture. 14 inline issues posted (1 high, 9 medium, 4 low). HighlightsSecurity — The crypto implementation is sound: AES-256-GCM with PBKDF2-SHA256 at 600k iterations, fresh random IVs, session key in module scope (not React state or localStorage), vault locked on logout. Challenge-response SSH signing correctly keeps the private key client-side. One gap: Architecture — The Performance — Six components use Additional notesTesting gaps — 311 tests cover the store, crypto, SSH utils, backend, hook, and some UI components well. However, four vault UI components have zero test coverage: Token expiry — The If you've addressed the feedback and want a new review, tag |
ui-react/apps/admin/src/components/vault/VaultSettingsSection.tsx
Outdated
Show resolved
Hide resolved
Add sshpk and node-rsa for SSH private key parsing, fingerprint extraction, and challenge-response signing. Add vite-plugin-node-polyfills to provide Node.js globals (process, Buffer) required by these libraries in the browser environment.
ff5cfac to
9a318ab
Compare
|
/review |
|
Claude finished @gustavosbarreto's task in 3m 11s —— View job Code Review CompleteReviewed 39 files across code quality, security, testing, TypeScript/React patterns, and architecture. 6 inline issues posted (0 high, 4 medium, 2 low). All fixes from the previous two review rounds were verified as correctly applied. HighlightsSecurity — PBKDF2 Type Safety — Vault data shape validation checks UX — Three vault dialogs ( Additional notesTesting gaps — Same 5 components from prior rounds still have zero test coverage: No cross-repo impact — The If you've addressed the feedback and want a new review, tag |
| useEffect(() => { | ||
| if (open) setPassword(""); | ||
| }, [open]); |
There was a problem hiding this comment.
medium · UX: Stale store error shown on reopen. The vault store's error field is shared across VaultUnlockDialog, VaultSetupDialog, and ChangePasswordDrawer, but none of them clear it when the dialog opens. If the user triggers an error (e.g., wrong password), closes the dialog, and reopens it, the old error message is displayed alongside empty inputs.
| useEffect(() => { | |
| if (open) setPassword(""); | |
| }, [open]); | |
| useEffect(() => { | |
| if (open) { | |
| setPassword(""); | |
| useVaultStore.setState({ error: null }); | |
| } | |
| }, [open]); |
Same issue also at:
ui-react/apps/admin/src/components/vault/VaultSetupDialog.tsx:24-32ui-react/apps/admin/src/components/vault/VaultSettingsSection.tsx:27-33(ChangePasswordDrawer)
| const isValid = parsed.every( | ||
| (item: unknown) => | ||
| typeof item === "object" && | ||
| item !== null && | ||
| typeof (item as Record<string, unknown>).id === "string" && | ||
| typeof (item as Record<string, unknown>).name === "string" && | ||
| typeof (item as Record<string, unknown>).data === "string" && | ||
| typeof (item as Record<string, unknown>).fingerprint === "string", | ||
| ); |
There was a problem hiding this comment.
medium · type safety: Vault data validation is missing the hasPassphrase field. VaultKeyEntry requires hasPassphrase: boolean, but the shape check only validates id, name, data, and fingerprint as strings. If stored data has a missing or non-boolean hasPassphrase, the cast on line 167 produces objects that violate the type contract — the passphrase prompt in ConnectDrawer could incorrectly show or hide.
| const isValid = parsed.every( | |
| (item: unknown) => | |
| typeof item === "object" && | |
| item !== null && | |
| typeof (item as Record<string, unknown>).id === "string" && | |
| typeof (item as Record<string, unknown>).name === "string" && | |
| typeof (item as Record<string, unknown>).data === "string" && | |
| typeof (item as Record<string, unknown>).fingerprint === "string", | |
| ); | |
| const isValid = parsed.every( | |
| (item: unknown) => | |
| typeof item === "object" && | |
| item !== null && | |
| typeof (item as Record<string, unknown>).id === "string" && | |
| typeof (item as Record<string, unknown>).name === "string" && | |
| typeof (item as Record<string, unknown>).data === "string" && | |
| typeof (item as Record<string, unknown>).fingerprint === "string" && | |
| typeof (item as Record<string, unknown>).hasPassphrase === "boolean", | |
| ); |
| export async function deriveKey( | ||
| password: string, | ||
| salt: Uint8Array, | ||
| iterations: number = PBKDF2_ITERATIONS, | ||
| ): Promise<CryptoKey> { |
There was a problem hiding this comment.
medium · security: deriveKey accepts any iterations value without validation. Since verifyPassword passes meta.iterations from localStorage (untrusted), an attacker with localStorage write access (e.g., XSS, browser extension) could set iterations to 1, making the derived key trivially brute-forceable against the known verifier ciphertext. Adding a minimum bound prevents silent key-strength degradation.
| export async function deriveKey( | |
| password: string, | |
| salt: Uint8Array, | |
| iterations: number = PBKDF2_ITERATIONS, | |
| ): Promise<CryptoKey> { | |
| export async function deriveKey( | |
| password: string, | |
| salt: Uint8Array, | |
| iterations: number = PBKDF2_ITERATIONS, | |
| ): Promise<CryptoKey> { | |
| if (!Number.isInteger(iterations) || iterations < 100_000) { | |
| throw new Error("PBKDF2 iteration count is too low or invalid"); | |
| } |
| case WS_KIND.SIGNATURE: { | ||
| if (!session.privateKey) return; | ||
| const challengeBuffer = Buffer.from(msg.data, "base64"); | ||
| try { | ||
| const signature = generateSignature( | ||
| session.privateKey, | ||
| challengeBuffer, | ||
| session.passphrase, | ||
| ); | ||
| ws.send(JSON.stringify({ kind: WS_KIND.SIGNATURE, data: signature })); | ||
| } catch { | ||
| term.write("\r\n\x1b[1;31mFailed to sign authentication challenge.\x1b[0m\r\n"); | ||
| useTerminalStore.getState().clearSensitiveData(session.id); | ||
| ws.close(); | ||
| return; | ||
| } | ||
| // Clear sensitive key material after successful handshake | ||
| useTerminalStore.getState().clearSensitiveData(session.id); | ||
| registerResizeHandler(); |
There was a problem hiding this comment.
medium · security: The ws.onmessage closure captures the session prop object from effect creation time (effect depends only on [session.id]). When clearSensitiveData updates the store, the original session object in this closure still holds privateKey and passphrase for the entire WebSocket lifetime. The store state is cleared, but the closure retains the original reference — so the private key stays reachable in memory until the component unmounts.
Consider reading the key material from the store inside the handler rather than from the closure:
case WS_KIND.SIGNATURE: {
const { privateKey, passphrase } = useTerminalStore.getState()
.sessions.find(s => s.id === session.id) ?? {};
if (!privateKey) return;
// ... sign and clear
}This way, clearSensitiveData at line 169 actually removes the last reference to the key material.
| <ConfirmDialog | ||
| open={resetOpen} | ||
| onClose={() => setResetOpen(false)} | ||
| onConfirm={async () => { | ||
| await resetVault(); | ||
| setResetOpen(false); | ||
| }} |
There was a problem hiding this comment.
low · UX: resetConfirmText is only cleared when the "Reset Vault" button is clicked (line 208). If the user opens the dialog, partially types (e.g., "RES"), closes via Cancel or backdrop, and reopens, the stale text persists.
| <ConfirmDialog | |
| open={resetOpen} | |
| onClose={() => setResetOpen(false)} | |
| onConfirm={async () => { | |
| await resetVault(); | |
| setResetOpen(false); | |
| }} | |
| <ConfirmDialog | |
| open={resetOpen} | |
| onClose={() => { | |
| setResetConfirmText(""); | |
| setResetOpen(false); | |
| }} | |
| onConfirm={() => { | |
| resetVault(); | |
| setResetOpen(false); | |
| }} |
| if (file.size > 512 * 1024) { | ||
| setFileSizeError(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
low · robustness: When a file exceeds the size limit, fileSizeError is set to true but fileReadError is not cleared. If a user previously encountered a file read error and then drops an oversized file, both error messages are displayed simultaneously.
| if (file.size > 512 * 1024) { | |
| setFileSizeError(true); | |
| return; | |
| } | |
| if (file.size > 512 * 1024) { | |
| setFileSizeError(true); | |
| setFileReadError(false); | |
| return; | |
| } |
…tation Define IVaultBackend interface for storage abstraction, implement LocalVaultBackend using localStorage for community/self-hosted edition, and add singleton factory for backend instantiation. The adapter pattern allows future server-side storage without changing vault logic.
Add private key validation with encrypted key detection (KeyEncryptedError), MD5 fingerprint extraction, and challenge-response signature generation using sshpk and node-rsa. Supports RSA (pkcs1-sha1) and ED25519/ECDSA (sha512) key types.
Implement vault state management with full lifecycle: initialize, unlock, lock, and key CRUD operations (add, update, remove). Includes duplicate key prevention (name and data uniqueness), master password change, vault reset, and legacy key migration from the old Vue UI format.
Add VaultSetupDialog for first-time master password creation, VaultUnlockDialog for vault unlock with autocomplete suppression, VaultLockedBanner for locked state indication, and VaultSettingsSection with master password change, lock, and reset vault functionality.
Add the main Secure Vault page with three states: uninitialized (onboarding), locked (banner), and unlocked (key table with search). Includes KeyDrawer for add/edit with automatic encrypted key detection and passphrase validation, KeyDeleteDialog for key removal, and duplicate key prevention with per-field error messages.
Clear the vault session key from memory when the user logs out, covering explicit logout, token expiration, and 401 responses.
Extract the duplicated file-input logic (FileReader, 512KB limit, drag-and-drop, paste interception, file/text mode toggle) into useKeyFileInput hook and KeyFileInput component. Refactor public-keys KeyDataInput to use them.
Replace the inline file/text input block with the shared KeyFileInput component, removing duplicated file-input logic.
Cover vault crypto utilities, localStorage backend, SSH key utils, vault Zustand store, KeyFileInput hook and component, and secure-vault page components.
|
All 4 items from round 3 addressed:
|
9a318ab to
a0c6f5e
Compare
What
Encrypted SSH private key storage in the browser, protected by a master password. Keys are encrypted with AES-256-GCM (PBKDF2-SHA256 derived key) and persisted in localStorage. Private key authentication uses SSH challenge-response signing — the raw key never leaves the browser.
Why
ShellHub previously stored private keys in plain text in localStorage. The Secure Vault encrypts keys at rest behind a master password, matching the security model of tools like Termius Vault. This is the community/self-hosted implementation; the backend adapter pattern (
IVaultBackend) supports future server-side storage for cloud/enterprise.Changes
vault-crypto.ts): AES-256-GCM encryption/decryption, PBKDF2-SHA256 key derivation (600k iterations), verifier-based password checking without storing the password hash. Session key held in module scope (not in React state or localStorage).vault-backend.ts,vault-backend-local.ts):IVaultBackendinterface with localStorage implementation and singleton factory.ssh-keys.ts): sshpk-based key validation withKeyEncryptedErrordetection for auto-detecting passphrase-protected keys, MD5 fingerprint extraction, and challenge-response signature generation (RSA via node-rsapkcs1-sha1, ED25519/ECDSA via sshpksha512).vaultStore.ts): Zustand store managing vault lifecycle (uninitialized/locked/unlocked), key CRUD with duplicate name/data prevention, master password change, vault reset, and legacy key migration from the Vue UI format.TerminalInstancenow implements the WebSocket challenge-response protocol (kind 3 Signature) for private key auth — POST sends fingerprint only, browser signs challenges locally. Sensitive key material is cleared immediately after signing and on component unmount.ConnectDraweradds vault key selector with vault/manual toggle and inline unlock prompt.useKeyFileInputhook andKeyFileInputcomponent, used by both public keys and secure vault drawers.Testing
shellhub-vault-meta,shellhub-vault-data— no plaintext).privateKeysexists in localStorage (legacy Vue UI), verify migration on first vault setup.