feat(desktop/vault): TOTP UI, SSH-key fingerprints + ed25519 keygen, coded service errors (#320)#347
Conversation
physercoe
left a comment
There was a problem hiding this comment.
Crypto is sound — TOTP is RFC-6238 correct, ed25519 keygen uses the OS CSPRNG, private keys are never logged, and all secrets go through the keychain. Three things to address before merge:
Major — UI-thread freeze (recorded lesson: sync #[tauri::command] runs on the main thread)
ssh_generate_key (desktop/src-tauri/src/ssh.rs:91) is a synchronous command, but key.encrypt(&mut SshOsRng, p) runs bcrypt-pbkdf — a deliberately slow KDF (tens–hundreds of ms). On the UI thread that janks/freezes the app on keygen-with-passphrase. Make it pub async fn (no body change needed). ssh_parse_key (ssh.rs:429) does KDF work on decrypt and shares the class — fix both to close the class, not just the instance.
Minor — double macOS keychain prompt (recorded lesson: consolidated-store batched writes)
generateKey (desktop/src/state/keys.ts:257-258) does two sequential secretSet calls (pk, then passphrase), each rewriting the whole consolidated keychain document → two prompts on an unsigned build. Batch via secretSetMany({ [pkKey(id)]: gen.pem, ...(passphrase !== "" && { [passKey(id)]: passphrase }) }). Same pattern in importKey (keys.ts:85-86).
Minor — otpauth params silently ignored → wrong codes
parseSeed (desktop/src/state/totp.ts:306-317) reads only secret; an otpauth:// URI with non-default algorithm/digits/period (e.g. SHA256 / 8-digit / 60s, used by some banks/AWS) mints wrong codes with no error. Honor those params, or reject non-default ones rather than emit silently-incorrect codes.
Nits: generateKey calls invoke without the isTauri() guard its sibling importKey has; VaultManager.tsx:513 / totp.ts:322 use the literal 30 instead of STEP_S.
…coded service errors (physercoe#320)
…otpauth params (physercoe#320) - ssh_parse_key + ssh_generate_key are now pub async fn — bcrypt-pbkdf KDF work must not run on the main thread (review: sync tauri commands freeze UI) - importKey + generateKey batch their keychain writes via secretSetMany — one consolidated-store flush, one macOS prompt (review) - parseSeed honors otpauth algorithm/digits/period (SHA-1/256/512, 6-8 digits, custom period) and rejects params it can't honor instead of silently minting wrong codes (review) - nits: isTauri() guard on generateKey; no literal 30 outside STEP_S
a98f270 to
96b1949
Compare
|
Review addressed in 96b1949 (branch also rebased onto current origin/main): Major — UI-thread freeze: both commands are now Minor — double macOS keychain prompt: confirmed Minor — otpauth params silently ignored: honored, since WebCrypto HMAC covers SHA-1/SHA-256/SHA-512 cleanly. Nits: Re-verified after rebase: |
physercoe
left a comment
There was a problem hiding this comment.
Re-review of fix 96b1949 — all findings addressed. Both ssh_generate_key and ssh_parse_key are now async (off the UI thread); the SshOsRng bridge is correct against the locked crate versions (impls TryRng/TryCryptoRng, auto-satisfies CryptoRng via the blanket impl). secretSetMany batches pk+passphrase (importKey + generateKey); parseParams honors algorithm/digits/period and rejects non-defaults instead of minting wrong codes; isTauri guard + STEP_S nits fixed. VoiceStrings required-field change has its sole call site updated. No regressions. (Awaiting CI green before merge.)
# Conflicts: # desktop/src/surfaces/VaultManager.tsx
…ic-tail merges Since v0.3.79: - fix(terminal): tell xterm the pty is ConPTY on Windows so a repainting TUI's intermediate frames no longer pile up in the scrollback (2b26a16). - 8 epic-tail PRs merged: #341 terminal connect-phase + SSH split-duplicate, #342 PDF struct-tree a11y + annotation shortcuts, #343 token burn-down / plural / responsive, #344 PDF fit-page / rotation / hand-pan / ink, #345 perf (page memo, debounce, chart cap, sync dirty-tracking), #346 modal migration + dirty-close guards, #347 vault TOTP / ed25519 / coded errors, #348 read reconcile / AuthorNav / table nav / frame check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes #320
The remaining tail of the vault security polish (the rest shipped in d328a7c / 650f1eb / bcb29d1 / bbe1ff0).
What
totpsecret slot was in the login model (and syncs) with no UI. Newstate/totp.tscomputes 6-digit RFC 6238 codes via WebCrypto HMAC-SHA-1 (no new Rust command), accepting bare base32 seeds and otpauth:// URIs. ItemEditor gains a masked TOTP seed field (reusesPasswordInput); ItemDetail gains a live code with a 30s countdown ring + seconds aria-label, and Copy routes throughcopySecret(auto-clears the clipboard).ssh_parse_keynow also returns the SHA-256 fingerprint (stored on import, shown truncated in the key list); newssh_generate_keycommand generates an ed25519 keypair in Rust via russh's pinned ssh-key crate (no new dependency) and the web layer stores the PEM + passphrase in the OS keychain exactly like the import path. A Generate ed25519 button sits beside Import (reuses the name + passphrase fields).vault/service.tsthrows codedVaultErrors (noKey/conflict/empty/noRecovery) which VaultPanel maps to localized messages at the catch site;voice/session.tsdrops its hardcoded English fallbacks by makingVoiceStringsrequired (Composer already passes the i18n entries).Why
Service layers have no
t(), so user-visible errors must not be string literals there; TOTP seeds synced from mobile were invisible/uneditable on desktop; the key list couldn't identify keys and import was the only way to get one.How verified
npm run build(sync:tokens + tsc --noEmit + vite build) — green.scripts/lint-desktop-tokens.sh— clean (no new off-token counts, no phantom tokens; ring uses--accent/--border-strong).ssh_key::rand_corere-export because the crate's direct rand_core dep is 0.6 while ssh-key 0.7 wants rand_core 0.10 traits.