Skip to content

feat(vault): add encrypted credential vault with audit log#446

Open
0r0b0r011 wants to merge 2 commits into
fathah:mainfrom
0r0b0r011:split/vault-credentials
Open

feat(vault): add encrypted credential vault with audit log#446
0r0b0r011 wants to merge 2 commits into
fathah:mainfrom
0r0b0r011:split/vault-credentials

Conversation

@0r0b0r011

Copy link
Copy Markdown

Summary

Adds an encrypted credential vault for profile-scoped API keys and secrets:

  • Storage: SQLite vault at $HERMES_HOME/desktop/vault.db with AES-256-GCM encryption
  • Key management: OS keychain via Electron safeStorage when available; password fallback
  • Main process: src/main/vault/* + vault-handlers.ts IPC surface
  • Preload: vault namespace (add/get/update/delete credentials, audit log, key rotation, export)
  • Renderer: Vault screen with masked values and add/delete flows
  • Tests: tests/vault.test.ts covers encryption-at-rest, profile activation, audit log, rotation

Part of the split from #438. Follows #444 and #445.

Security notes for reviewers

  • Encryption model: Master key derived/stored via keychain.ts; credentials encrypted before SQLite write
  • Key storage: Prefers OS safe storage; falls back to password-init path when unavailable
  • IPC exposure: Full CRUD + export exposed to renderer via preload — review redaction in getCredentials (masked values only)
  • Audit log: Actions logged on add/update/delete; verify retention and whether logs leak secret metadata
  • SQLite path: Under $HERMES_HOME/desktop/ — confirm backup/migration behavior
  • Profile activation: Vault can write .env from stored credentials — review filesystem write safety

Test plan

  • npm run typecheck
  • npm test -- tests/vault.test.ts tests/ipc-handlers.test.ts tests/preload-api-surface.test.ts
  • npm run build
  • Manual: add credential, confirm masked display, delete, check audit log
  • Manual: confirm vault unavailable in remote-only mode

Made with Cursor

Store profile-scoped secrets in an encrypted SQLite vault with IPC
handlers, preload namespace, and a Vault screen for credential CRUD.

Co-authored-by: Cursor <cursoragent@cursor.com>

@pmos69 pmos69 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. The vault direction is useful, but I think this should not merge in its current shape. Credential storage has a high bar, and this first slice currently combines storage, renderer CRUD, export, key rotation, profile activation, .env writing, and migration helpers before the boundaries are safe enough.

Requested changes / findings:

  1. Main-process vault IPC is not guarded for remote-only mode.
    The renderer hides Vault behind RemoteNotice, but src/main/ipc/vault-handlers.ts registers live handlers for add/get/update/delete/audit/rotate/export/init with no isRemoteOnlyMode() check. Any renderer code can still call window.hermesAPI.vault.* and mutate or export the local vault. Please enforce this in the main-process handlers, using the same pattern as other local-only features.

  2. The renderer API has broader authority than the visible UI implies.
    The preload API exposes full CRUD plus export, and the handlers accept arbitrary profile and raw id values. Since profile listing is already exposed elsewhere, renderer code can enumerate profiles, read masked credentials/audit metadata for each, and update/delete by id. Please validate profile names and enforce the intended authorization/scope at the IPC boundary, not just in React state.

  3. vault-export should not return the entire vault database to renderer code.
    vault-export reads vault.db and returns it as base64. The secret values are encrypted, but the DB also contains plaintext metadata such as profile/provider/label/audit timestamps, and exporting it to any renderer caller creates an unnecessary offline-attack/distribution path. I would omit export from the first slice, or implement it only as an explicit user-confirmed save flow entirely in main process.

  4. The password fallback is advertised but not usable from the UI.
    Vault.tsx calls getCredentials(profile) during load with no try/catch, no unlock prompt, and no initWithPassword() path. If safeStorage is unavailable or the password vault is locked, the call throws and the screen can sit on Loading vault... forever. Please add an actual locked/unlock/init UI state before claiming password fallback support.

  5. activateProfile() can destroy existing profile configuration.
    The activation path writes a new .env containing only vault-managed entries and comments. That can silently delete unrelated keys such as API_SERVER_KEY, provider/model config, platform tokens, user comments, and local overrides. This should merge/update only managed keys and preserve everything else.

  6. .env values are written without escaping.
    buildEnvContent() writes raw ${envKey}=${value} lines. Secrets containing newlines, quotes, #, or control characters can corrupt the file or inject extra entries. Please add safe .env serialization and tests for multiline/control-character values.

  7. The advertised profile activation/migration/copy flows are not wired into the app.
    activateProfile, deactivateProfile, migratePlaintextEnv, and copyProfileSecrets appear to be used only by tests. They are not connected to profile switching, profile creation/deletion, or a migration UI. Either wire the flows fully and safely, or defer them from this PR.

  8. auth.json handling is unsafe/asymmetric.
    The activation comment says it writes .env + auth.json, but activation only writes .env. deactivateProfile(wipe=true) deletes auth.json, which can remove OAuth credentials the vault never created or managed. Please do not delete unrelated auth.json data, and do not advertise auth.json activation unless implemented safely.

  9. The vault DB file is not explicitly created with private permissions.
    The master key files use mode: 0o600, but vault.db is opened with new Database(VAULT_DB_PATH) and no equivalent permission hardening. Even with encrypted secret values, the DB stores plaintext metadata and audit history. Please ensure vault.db and its WAL/SHM companions are private, especially on Linux/macOS.

  10. The key file format is ambiguous across safeStorage and password modes.
    master.key.enc stores a safeStorage-encrypted blob in one mode and a raw password-mode verifier in another, with no format/version/mode discriminator. initMasterKey() decides how to read the file based on the current safeStorage.isEncryptionAvailable() result. A vault created in password/headless mode and later opened where safeStorage is available can be misread as a safeStorage blob. Please use a versioned key envelope that records mode/format explicitly and handles migration.

  11. Master-key rotation is not atomic and can make the vault unrecoverable.
    rotateMasterKey() persists the new key first, then loops through rows to re-encrypt them. A crash or kill between those operations leaves the new key on disk while some/all secrets remain encrypted with the old key. Rotation needs a transaction/recovery strategy, or should be deferred until that is designed.

  12. The password verifier design should be revisited.
    Password mode stores sha256(masterKey) as the unlock verifier. That gives an attacker who obtains the file a direct verification value for guesses of the derived encryption key. Please use a versioned envelope with a domain-separated verifier, HMAC/encrypted known plaintext, or another standard construction that does not store a plain hash of the AES key.

  13. Listing credentials decrypts every secret just to show a masked suffix.
    getCredentials() decrypts each credential value on routine screen load in order to compute the last characters. That repeatedly materializes every secret in memory unnecessarily. Store non-sensitive display metadata, such as the masked suffix, when writing/updating the secret instead.

  14. The audit schema is brittle.
    Delete audit rows reference the deleted secret with ON DELETE CASCADE. Foreign keys may not be enforced today, but if they are enabled later, delete audit history can disappear. Audit history should be durable and not cascade away with the secret row.

  15. The Vault CSS introduces global UI regressions.
    The PR adds global .card, .screen-header, .btn-primary, and .btn-secondary rules near the end of main.css, including redefinitions of existing button classes. Please scope these styles to the Vault screen or reuse existing app classes to avoid unrelated UI changes.

  16. Test coverage does not cover the sensitive boundaries.
    The tests are mostly service-layer happy paths. Please add tests for remote-only IPC rejection, profile/id authorization, locked vault UI behavior, export gating, .env preservation and escaping, DB/key file permissions, key-format mode transitions, rotation failure/recovery, audit retention, and avoiding routine decrypts during list rendering where practical.

Given the number of integration risks, my recommendation is to shrink this first PR to a safer minimal slice: local-only guarded vault storage, add/delete, masked list, working lock/unlock UI, private file permissions, and targeted security tests. Defer export, activation, migration, profile copy, key rotation, and .env/auth.json writing until each has preservation logic, recovery behavior, and test coverage.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants