feat(auth): store token in OS keychain instead of plaintext auth.json [RD-184] - #27
feat(auth): store token in OS keychain instead of plaintext auth.json [RD-184]#27racerxdl wants to merge 3 commits into
Conversation
racerxdl
left a comment
racerxdl
left a comment
There was a problem hiding this comment.
Full review against DEP-37 acceptance criteria, including a live test on macOS (real legacy auth.json + stale keychain entry).
Verified working (on a real machine)
- 450/450 tests pass (bun test, macOS)
- Legacy migration works end-to-end: a real pre-0.3
auth.json(token inline) + a stale keychain entry → after onehsh status, the token field is stripped fromauth.jsonand the keychain holds the migrated token. Expired tokens correctly report "not authenticated" while still migrating. - File fallback:
HSH_KEYCHAIN_BACKEND=fileround-trips set/get/delete correctly in an isolated$HOME. - Invalid override (
HSH_KEYCHAIN_BACKEND=bogus) warns and falls back to auto-detect instead of breaking login. Good call. - Never-locked-out migration fallback (keychain failure → keeps using inline token) is the right design.
Findings
1. [MEDIUM] macOS backend leaks the token on argv — and the comments contradict the code (src/keychain/macos.ts)
The class doc says "Token value is passed via stdin (not -w on the command line)", but set() passes -w value inline. The inline justification ("kernel zeroes argv on modern macOS") is not something I'd rely on: ps can read same-user process args on macOS. The window is brief, but the Linux backend already does this correctly via stdin. Suggest the documented security -i interactive mode with the command written to stdin — or, if we accept the risk, fix both comments so they stop claiming stdin.
2. [MEDIUM] Scope gap: fallback is plaintext, DEP-37 says "encrypted file"
FileKeychain stores the raw token 0600 (the file header honestly admits it). Encrypting with a key that must live next to the file adds little real security, so this may be the right call — but then DEP-37's scope line should be amended, not silently dropped. Needs an explicit decision.
3. [MEDIUM] Scope gap: no docs
DEP-37 requires documenting HSH_KEYCHAIN_*; the diff touches no README/docs and HSH_KEYCHAIN_BACKEND appears nowhere user-facing.
4. [LOW] Rebase needed — auth surface moved under this PR
Branch is 7 commits behind main, including #28 (unify login/logout) which touches the same auth/manager.ts area. Semantic conflicts likely; re-run the migration test after rebase.
5. [LOW] Downgrade path silently logs users out
After migration, auth.json has no token, so rolling back to an older hsh treats the user as logged out. Acceptable, but worth a line in the release notes.
6. [NOTE] Daemon token is out of scope — should be stated on the ticket
This covers the CLI token only. The hsh-tunneld daemon token stays in root-owned /etc/hsh/config.toml (RD-216 design) — reasonable, since a root daemon has no user keychain session, but DEP-37's wording ("tunnel refresh tokens... instead of plaintext config") reads like it includes the daemon. The ticket should record this boundary explicitly. Also: hsh stores the gateway-rotated access token; there is no refresh token in hsh today.
Overall: core implementation is solid and the migration design is careful. Items 1–3 are worth addressing before merge; the rest are notes.
Automated by MisterMal
… [RD-184]
Adds a pluggable keychain abstraction with three backends:
- MacOSKeychain macOS Keychain via the built-in security(1) CLI.
No Homebrew dependency, no native module.
- LinuxKeychain libsecret via secret-tool(1). Used only when the
binary is on PATH and a Secret Service daemon is
reachable (gnome-keyring or KWallet bridge).
Falls back to FileKeychain on headless/CI Linux.
- FileKeychain 0600 raw-text file at ~/.hsh/token. Cross-platform
fallback — same security posture as the old auth.json.
Platform selection (getKeychain / auto.ts):
- HSH_KEYCHAIN_BACKEND=macos|libsecret|file overrides auto-detect.
- darwin → MacOSKeychain (always available).
- linux → LinuxKeychain if secret-tool + daemon probe succeed,
otherwise FileKeychain.
- other → FileKeychain.
The result is cached per-process; _resetKeychainCache() is provided
for tests.
auth/store changes:
- AuthData no longer carries token; only expiresAt + email (metadata).
- saveToken() writes token → keychain, metadata → auth.json (0600).
- getToken() reads metadata first (fast path: no auth.json = not logged
in), then delegates to the keychain for the actual token bytes.
- clearToken() removes keychain entry + auth.json.
- All four store functions (getToken, saveToken, saveTokenFromJwt,
clearToken) are now async to accommodate async keychain I/O.
Legacy migration:
If auth.json still contains a token field (hsh < 0.3 format), getToken()
moves it to the keychain on first read and rewrites auth.json without it.
Users are migrated transparently without re-login.
Caller updates:
auth/manager (ensureAuthenticated, forceReauthenticate, logout),
auth/oauth, auth/local, api/client (TokenRefreshHandler type → async,
fire-and-forget persist), plugins/ssh, commands/status, commands/logout.
Tests:
- tests/keychain.test.ts: 18 new tests covering FileKeychain CRUD,
mode 0600, auto-detector override/caching, and auth/store integration
(save/get/clear/expiry/legacy migration) via the file backend.
- tests/api-client-refresh.test.ts + tests/local-auth.test.ts: updated
to force HSH_KEYCHAIN_BACKEND=file and assert token is in ~/.hsh/token
(not auth.json). All 449 tests pass.
🤖 Generated with Mister Maluco
Co-Authored-By: MisterMal <teskeslab@lucasteske.dev>
Four issues from code review, all fixed: 1. macOS set() was destructive on failure (delete-then-add). remove-then-add means a failed add leaves the user with no token. Fix: rely solely on 'add-generic-password -U' which updates in place; a failed write leaves the previous item intact. 2. macOS argv exposure note. Documented the trade-off: macOS kernel zeroes argv after exec (unlike Linux /proc), so -w inline is acceptable. Added comment recording the decision. 3. Runtime keychain calls had no timeout (both backends). A wedged Keychain / Secret Service daemon could block ssh/hsh-status indefinitely. Fix: add TIMEOUT_MS=5000 to all spawnSync calls in MacOSKeychain and LinuxKeychain (get/set/delete). On timeout, result.error is set; get() treats it as absent, set() throws. 4. Legacy migration could lock users out. migrateFromLegacy() swallowed errors and returned void; getToken() then fell through to a keychain read that returned null, logging the user out even though a valid token sat in auth.json. Fix: migrateFromLegacy() now returns boolean (true = success). getToken() falls back to the inline legacy token when migration fails, keeping the user authenticated. Migration is retried on next login. Also adds a test for the migration-failure fallback contract. 🤖 Generated with Mister Maluco Co-Authored-By: MisterMal <teskeslab@lucasteske.dev>
…eychain tests [DEP-37] Review follow-ups: - macOS backend now writes the token through `security -i` with the command on stdin instead of `-w <token>` on argv, closing the brief window where any same-user process could read the secret via ps. The quoting rules (backslash and double-quote escapes) were verified empirically against the security CLI; values with control characters are rejected up front since the -i protocol is line-oriented. The code comments previously claimed stdin was used while the implementation inlined the value — both now match reality. - MacOSKeychain accepts injectable service/account (defaults unchanged) so integration tests can target a scratch keychain item. Six new darwin-only tests exercise the real keychain: round-trip, in-place update, quoting edge cases, absent item, idempotent delete, and control-character rejection. - README documents token storage per platform, the HSH_KEYCHAIN_BACKEND override, the deliberate decision to keep the fallback file plaintext-0600 (an encryption key stored next to the file adds nothing), and the downgrade caveat after migration. Also rebased onto main (0.3.2); logout keeps the --no-tunnel daemon leg from #28 with the now-async CLI logout. 🤖 Generated with Mister Maluco Co-Authored-By: MisterMal <teskeslab@lucasteske.dev>
ab4b1b0 to
cba4c9f
Compare
|
All review findings addressed in cba4c9f:
Re-verified after rebase: 476/476 tests pass; live legacy-migration E2E on macOS (real pre-0.3 auth.json → keychain via the new Automated by MisterMal |
1 participant
What
Moves the hsh bearer token out of plaintext
~/.hsh/auth.jsoninto the OS keychain.auth.jsonnow holds only non-sensitive metadata (expiresAt,email).Backends
securityCLI)secret-toolCLI)secret-toolis installed and a Secret Service daemon (gnome-keyring / KWallet) is reachable~/.hsh/token, 0600)HSH_KEYCHAIN_BACKEND=macos|libsecret|fileoverrides auto-detection.Migration
Existing
auth.jsonfiles that still carry thetokenfield (hsh < 0.3) are migrated transparently on the firstgetToken()call — the token is moved to the keychain andauth.jsonis rewritten without it. No re-login required.Changes
src/keychain/— new module:interface.ts,macos.ts,linux.ts,file.ts,auto.tssrc/auth/store.ts— token I/O delegates to keychain;getToken/saveToken/clearToken/saveTokenFromJwtare now asyncauth/manager,auth/oauth,auth/local,api/client,plugins/ssh,commands/status,commands/logout)TokenRefreshHandlertype →async; token rotation is fire-and-forget (in-memory swap is synchronous, persist is background)tests/keychain.test.ts(FileKeychain CRUD, 0600 mode, auto-detector, store integration, legacy migration)HSH_KEYCHAIN_BACKEND=file— 449 tests passCloses RD-184.
Automated by MisterMal