Skip to content

feat: OS-keychain API key storage (ROADMAP Movement IV groundwork) - #33

Merged
evangress merged 5 commits into
mainfrom
feat/secret-store
Jul 27, 2026
Merged

feat: OS-keychain API key storage (ROADMAP Movement IV groundwork)#33
evangress merged 5 commits into
mainfrom
feat/secret-store

Conversation

@evangress

Copy link
Copy Markdown
Collaborator

Why

Toril has no .env and no API key handling — a tree-wide grep for api_key, ANTHROPIC, OPENAI, dotenv, process.env, std::env::var and sops turns up only TAURI_DEV_HOST in vite.config.ts and a planned feature in ROADMAP Movement IV. Nothing in Toril talks to a network today.

So this is a greenfield decision made ahead of the AI layer that will consume it, rather than a refactor of anything existing.

Design: docs/superpowers/specs/2026-07-27-windows-install-and-secret-store-design.md (merged in #32)

The load-bearing decision: no getter

keystore::SecretStore::get() exists so Movement IV's provider calls can use a key from Rust. It is never registered with invoke_handler. The webview can learn whether a key is set; it has no call available to learn what it is.

This is why the design beats the two alternatives that were on the table:

Option Why not
System environment variables A setx value is plaintext-readable by any process running as that user, appears in child-process environments and crash dumps, cannot be set from inside the app, and needs a relaunch.
SOPS Built for secrets committed to a repo for a deployment pipeline. Needs an age/GPG key already on the machine — chicken-and-egg — plus a CLI install.
OS keychain DPAPI/Keychain/Secret-Service encrypted at rest, settable in-app, zero user setup.

But the storage backend is not where the security lives. §3.3 already treats webview content as untrusted and sanitizes it. Keeping the key on the Rust side means a future sanitizer bypass degrades to a rendering bug rather than a stolen credential — the property comes from the API shape. The invariant is stated in three places that would each have to be edited to break it: the absent registration, the comment at the registration site in lib.rs, and the ipc.ts docstring telling the next reader there is nothing to call.

Structure

src/ui/secrets.ts        masked input, "Configured / Not set". Never holds a key.
      │ invoke()
commands/secrets.rs      4 commands. No getter.
      │
crates/keystore          SecretStore trait; OsKeychain + MemoryStore.
      │
keyring → Credential Manager / Keychain / Secret Service
  • Provider is a closed enum (Anthropic, OpenAi), not a free-text id, so a typo cannot silently create an orphaned keychain entry. Ollama is deliberately absent: it is local and unauthenticated, and needs a host URL rather than a secret.
  • Validation rejects control characters and line breaks — those would corrupt an HTTP header once Movement IV sends the key, so entry is the honest place to catch them. Length bounds (8..=1024) are deliberately loose and provider-agnostic; a hard-coded sk-ant- prefix check would age badly.
  • clear() is idempotent — pressing Clear twice is not an error the user has to interpret.
  • zeroize wraps the in-flight key so it is overwritten on drop rather than left in freed heap.

Dependencies (CLAUDE.md §2)

Both checked against crates.io before adoption, not assumed:

Crate Version Last publish Recent downloads License Publisher
keyring 4.1.5 2026-07-14 7.08M MIT OR Apache-2.0 open-source-cooperative
zeroize 1.9.0 2026-06-12 149M Apache-2.0 OR MIT RustCrypto

Gates

cargo test -p keystore (11) added to the CI matrix — ninth logic crate. tests/secrets.test.ts (9). Full suite: 210 frontend, 97 Rust, cargo fmt --all --check clean, clippy clean in our code (all remaining warnings are inside the vendored glib, as §8 predicts).

The frontend gate pins the security-relevant behavior rather than form mechanics: the field is type=password, it is blanked after a successful save, configured state comes from the backend rather than a cached key, and a backend rejection surfaces instead of the row claiming success.

What is deliberately NOT tested

OsKeychain itself has no automated test. It needs a logged-in desktop session, and CI's Linux runners have no Secret Service — a test there could only fail or silently skip, and a silently-skipping test reads as coverage without being coverage. The MemoryStore suite pins the contract instead. This is stated in the CI comment and CLAUDE.md §8 rather than left to be rediscovered.

Outstanding — on-device verification

  • Set a key, restart Toril, confirm has_api_key still reports it (proving it survived in Credential Manager, not process memory).
  • Clear a key, confirm it disappears from rundll32.exe keymgr.dll,KRShowKeyMgr, and that Clear twice does not error.

Scope

This branch stores keys. Nothing sends an HTTP request to any provider — that is Movement IV branch 20.

🤖 Generated with Claude Code

evangress and others added 5 commits July 27, 2026 22:08
New ninth logic crate. Provider is a closed enum rather than a free-text id, so
a typo cannot silently create an orphaned keychain entry, and its as_str() is
the stable wire id shared with the frontend.

Validation rejects what cannot later be sent as an HTTP header — control
characters and line breaks — at entry rather than at request time. Bounds are
deliberately loose and provider-agnostic; a hard-coded sk-ant- prefix check
would age badly.

The MemoryStore double exists so the SecretStore contract has a gate that runs
on any machine, since the real keychain needs a desktop session.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Windows Credential Manager, macOS Keychain, or freedesktop Secret Service,
selected by keyring's default `v1` feature. clear() treats NoEntry as success,
so pressing Clear twice is not an error the user has to interpret.

No tests accompany this: OsKeychain needs a logged-in desktop session and CI's
Linux runners have no Secret Service, so a test here would fail or silently
skip. A silently-skipping test reads as coverage without being coverage. The
MemoryStore suite pins the contract instead, and the real backend is on the
on-device verification list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four commands: set, clear, has, and list. There is deliberately no
get_api_key, and the registration site in lib.rs says so, because the natural
instinct of the next contributor is to add one for a "show key" toggle.

The webview learns whether a key is set, never what it is. §3.3 already assumes
webview content is hostile; keeping the key on the Rust side means a future
sanitizer bypass degrades to a rendering bug rather than a stolen credential.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate pins the security-relevant behavior rather than the form mechanics:
the field is type=password, it is blanked after a successful save, configured
state comes from the backend rather than a cached key, and a backend rejection
surfaces instead of the row claiming success.

The dialog has no way to display a key even if someone wanted it to — there is
no getter command to call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the File → API Keys… item, the dialog overlay styles (theme variables, so
it follows System/Light/Dark), -p keystore in the CI matrix, and the §5 command
table entry recording why no getter exists.

The CI comment says plainly that keystore's gate covers the in-memory double
only, and that OsKeychain is untested there on purpose rather than by oversight.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@evangress
evangress merged commit 1dc584e into main Jul 27, 2026
9 checks passed
@evangress
evangress deleted the feat/secret-store branch July 27, 2026 22:49
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.

1 participant