feat(keychain): actually wire the macOS Keychain provider by czxtm · Pull Request #18 · darkmatter/himitsu

cooper (czxtm) · 2026-05-10T02:57:00Z

Summary

key_provider: macos-keychain was cosmetic — the wizard offered it, the config persisted it, but every read and write went through `<data_dir>/key` regardless. The keychain stayed empty; the secret sat on disk in the same place as the disk provider.

This PR wires it up: the secret actually lands in the macOS Keychain, every decrypt path consults the keychain when the provider is set to keychain, and switching providers on an already-initialized machine migrates the on-disk secret over and deletes the disk copy.

Wiring

New crypto::keystore module owns provider-aware persistence: `store_new_key`, `load_identity`, `migrate_disk_to_keychain`, plus an `is_initialized` probe that uses the pubkey file (always written, both providers) as the "has init run" signal.
`Context` gains `key_provider` populated once at dispatcher boot. New `Context::load_identity()` is the single chokepoint — 13 call sites (`get`, `tag`, `exec`, `rekey`, `search`, `ls`, `generate`, `export`, viewer, etc.) migrated from `age::read_identity(&ctx.key_path())`.
`init.rs` reordered so the provider is settled before keypair generation. Old order was: write key to disk → maybe set provider in config → emit success. New order: ensure config exists → apply `--key-provider` → resolve active provider → handle migration → generate-and-store via the right backend.
Migration — switching to keychain on an existing install moves the secret into the keychain and removes the disk file. Keychain write happens before the disk delete, so partial failures don't lose the key.
"Is initialized" check — both `cli::Cli::run` (auto-init bootstrap) and `tui::should_launch_init_flow` switched from probing `key` to `key.pub`. Without this, post-migration himitsu would loop into auto-init on every command.
Docs — README `Configuration` section explains the two providers and the migration behavior.

Also folds in a small bystander polish on the envs view (`Ctrl+W` works as a save alias alongside `Ctrl+S`).

Test plan

`cargo test` — 704 passed (+4 new in `crypto::keystore`)
`cargo clippy --bin himitsu -- -D warnings` clean
`himitsu init --no-tui --key-provider macos-keychain` on a real machine: `<data_dir>/key.pub` exists, `<data_dir>/key` does NOT exist, `security find-generic-password -s io.darkmatter.himitsu.agekey.byfp.v1` finds an entry keyed by the pubkey fingerprint
`himitsu --remote czxtm/secrets get aws-sandbox-access-key-id` against a populated store decrypts via the keychain (returns plaintext)
Pre-existing disk-based install + `himitsu init --key-provider macos-keychain`: prints "✓ Migrated age key from disk to macOS Keychain", removes the disk file, decrypts succeed afterward

Out of scope

Linux Secret Service / Windows Credential Manager backends. The trait is in place; only the macOS impl is shipped.
Scope-pointer (`io.darkmatter.himitsu.agekey.scope.v1`) usage. The mapping API exists in `keyring` but isn't yet consumed — single-key-per-machine is fine for now.
Migrating BACK from keychain to disk. Picking `disk` after `macos-keychain` would not auto-extract today.

Selecting `key_provider: macos-keychain` (via `himitsu init` or the TUI wizard) used to be cosmetic — the wizard offered the choice, the config flag persisted, but every read/write code path went through `<data_dir>/key` regardless. The keychain stayed empty and the secret sat on disk in the same place as the disk provider. Wiring: - New `crypto::keystore` module owns provider-aware persistence: `store_new_key`, `load_identity`, `migrate_disk_to_keychain`, plus a pubkey-file probe (`is_initialized`) that's the canonical "has the user run init" check regardless of provider. - `Context` gains a `key_provider` field, populated once at dispatcher boot (and at TUI launch) by reading the user config. New `Context::load_identity()` chokepoints every command that decrypts — 13 call sites migrated from `age::read_identity(&ctx.key_path())`. - `init.rs` reorders so the active provider is settled BEFORE keypair generation. With keychain selected: 1. The secret goes to a `security add-generic-password` entry under `io.darkmatter.himitsu.agekey.byfp.v1`, keyed by the pubkey's fingerprint. 2. The pubkey file is still written to disk (provider-agnostic). 3. No `<data_dir>/key` is created. - Migration: switching to keychain on an already-initialized machine moves the existing on-disk secret into the keychain and deletes the disk file (only after the keychain write succeeds — safe to retry). - "Is initialized" probes (`cli::mod::Cli::run`, `tui::mod::should_launch_init_flow`) now check `key.pub` instead of `key`, so post-migration himitsu doesn't loop into auto-init on every command. - README explains the per-provider semantics and migration behavior. Bonus: the envs view picks up Ctrl+W as a save alias alongside Ctrl+S (stashed local polish that was sitting in the working tree). Verified manually: - Fresh init with `--key-provider macos-keychain`: pubkey on disk, no secret on disk, keychain entry under .byfp.v1. - Decrypt against a real store via the keychain succeeds (`himitsu get`, `ls`, `search` all work without the on-disk secret). - 704 tests pass, clippy clean.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a80a31e424

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

chatgpt-codex-connector · 2026-05-10T02:59:34Z

+    std::fs::create_dir_all(data_dir)?;
+    std::fs::write(pubkey_path(data_dir), format!("{pubkey}\n"))?;


Avoid marking keychain init complete before the key is stored

When key_provider is macos-keychain, this writes key.pub before checking Keychain availability or successfully storing the private key. If security add-generic-password fails, or the provider is selected on a non-macOS host, himitsu init exits with an error but leaves key.pub behind; the new is_initialized() probe then treats the install as initialized, skips key generation on later runs, and all decrypting commands fail because neither data_dir/key nor a Keychain item exists. Store the private key first or remove key.pub on failure so failed initialization remains retryable.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-05-10T02:59:34Z

+    /// Path to the age private key file. Only valid for the
+    /// [`Disk`](crate::config::KeyProvider::Disk) provider — with the
+    /// keychain provider this path doesn't exist, so callers should
+    /// reach the secret through [`Self::load_identity`] instead of
+    /// reading the path directly.


Use key.pub for self-recipient flows

The new keychain provider explicitly makes ctx.key_path() absent, but not all non-decryption callers were moved off it: join::read_own_pubkey() and recipient add --self still parse the private-key file to discover the public key. After himitsu init --key-provider macos-keychain, only key.pub is written, so himitsu join and himitsu recipient add --self ... fail even though the user is initialized. These callers should read ctx.pubkey_path() instead of the disk secret path.

Useful? React with 👍 / 👎.

+        store_new_key(
+            &ProviderChoice::Disk,
+            dir.path(),
+            "AGE-SECRET-KEY-1ABCDEF",


Copilot

Pull request overview

This PR makes key_provider: macos-keychain functional by introducing a provider-aware keystore layer, routing decryption through a single Context::load_identity() chokepoint, and adding a disk→Keychain migration path (with the init/bootstrap probe switched to key.pub so migrated installs don’t auto-init loop).

Changes:

Add crypto::keystore to store/load age identities via disk or macOS Keychain and migrate disk keys into the Keychain.
Thread key_provider through Context and update decrypt call sites (CLI + TUI) to use Context::load_identity().
Update init/TUI bootstrap logic and docs; small TUI polish (Ctrl+W save alias in env editor).

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
rust/src/tui/views/secret_viewer.rs	Switch decrypt path to `ctx.load_identity()`; update test contexts with `key_provider`.
rust/src/tui/views/search.rs	Propagate `key_provider` into cloned contexts; use `load_identity()` for decrypt.
rust/src/tui/views/remote_add.rs	Update test context construction to include `key_provider`.
rust/src/tui/views/new_secret.rs	Update test context construction to include `key_provider`.
rust/src/tui/views/envs.rs	Add `Ctrl+W` save alias; ensure cloned contexts carry `key_provider`; update help + tests.
rust/src/tui/mod.rs	Use keystore “initialized” probe; load `key_provider` into TUI context post-init.
rust/src/tui/harness.rs	Update test harness context with `key_provider`.
rust/src/tui/app.rs	Ensure cloned contexts include `key_provider`.
rust/src/crypto/mod.rs	Export new `crypto::keystore` module.
rust/src/crypto/keystore.rs	New provider-aware key persistence + migration helpers and tests.
rust/src/cli/tag.rs	Decrypt via `ctx.load_identity()` instead of reading disk key path.
rust/src/cli/sync.rs	Ensure per-store contexts carry `key_provider`.
rust/src/cli/search.rs	Use `ctx.load_identity().ok()` for optional description decrypt path.
rust/src/cli/schema.rs	Update tests to include `key_provider` on `Context`.
rust/src/cli/rekey.rs	Decrypt via `ctx.load_identity()` instead of reading disk key path.
rust/src/cli/recipient.rs	Update tests to include `key_provider` on `Context`.
rust/src/cli/mod.rs	Add `Context.key_provider` + `Context::load_identity()`; update bootstrap init probe and context construction.
rust/src/cli/ls.rs	Use `ctx.load_identity().ok()` for tag-dependent decrypt path.
rust/src/cli/join.rs	Update tests to include `key_provider` on `Context`.
rust/src/cli/init.rs	Reorder init to settle provider first, migrate if needed, then generate/store via chosen backend.
rust/src/cli/import.rs	Update tests to include `key_provider` on `Context`.
rust/src/cli/get.rs	Decrypt via `ctx.load_identity()` instead of reading disk key path.
rust/src/cli/generate.rs	Decrypt via `ctx.load_identity()` instead of reading disk key path.
rust/src/cli/export.rs	Decrypt via `ctx.load_identity()` instead of reading disk key path.
rust/src/cli/exec.rs	Decrypt via `ctx.load_identity()`; adjust imports accordingly.
rust/src/cli/codegen.rs	Update tests to include `key_provider` on `Context`.
README.md	Document disk vs macOS Keychain providers and disk→Keychain migration behavior.
.beads/issues.jsonl	Update issue tracker entries (adds/reopens items).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        .to_string();
+    let fp = fingerprint(&pubkey);
+    let identity = age::read_identity(&secret_path)?;
+    let secret_str = secrecy::ExposeSecret::expose_secret(&identity.to_string()).to_string();


+        ProviderChoice::MacosKeychain => {
+            ensure_keychain_available()?;
+            let fp = fingerprint(pubkey);
+            MacOSKeychain.store_key(&fp, secret)?;
+        }


+    // If the user just switched to keychain on an already-initialized
+    // machine, move the existing on-disk secret into the keychain. The
+    // pubkey file stays in place — it's the provider-agnostic "is
+    // initialized" probe.
+    if crate::crypto::keystore::needs_disk_to_keychain_migration(&active_provider, data_dir)? {
+        crate::crypto::keystore::migrate_disk_to_keychain(data_dir)?;
+        eprintln!("✓ Migrated age key from disk to macOS Keychain");
+    }
+
+    // ── 2. Generate a fresh keypair if none exists yet ───────────────────
+    let key_existed = crate::crypto::keystore::is_initialized(data_dir);


+        let key_provider = crate::config::Config::load(&crate::config::config_path())
+            .map(|c| c.key_provider)
+            .unwrap_or_default();


+        let key_provider = crate::config::Config::load(&crate::config::config_path())
+            .map(|c| c.key_provider)
+            .unwrap_or_default();


    let tui = Config::load(&config_path())?.tui;
    theme::set_theme(&tui.theme)?;
    icons::set_use_nerd_fonts(tui.nerd_fonts);

+    let cfg = Config::load(&config_path()).unwrap_or_default();
    let ctx = Context {
        data_dir: crate::config::data_dir(),
        state_dir: crate::config::state_dir(),
        store: crate::config::resolve_store(None).unwrap_or_default(),
        recipients_path: None,
+        key_provider: cfg.key_provider,
    };


-{"_type": "issue", "id": "hm-79o", "title": "Docs: update README for search-as-root TUI flow", "description": "README still references the dashboard-root flow. Update screenshots/gifs to match search-as-root, refresh the key-binding table to list current shortcuts (Ctrl+N new, Ctrl+S store, Ctrl+Y copy, ? help, Esc quit, e edit, d delete with y/n confirm).", "status": "closed", "priority": 3, "issue_type": "task", "assignee": "Cooper Maruyama", "owner": "demo@himitsu.dev", "created_at": "2026-04-15T06:53:34Z", "created_by": "Cooper Maruyama", "updated_at": "2026-04-15T07:01:04Z", "closed_at": "2026-04-15T07:01:04Z", "close_reason": "Merged via parallel worktree agents; cargo test --bin himitsu tui::harness:: \u2192 3 passed", "dependency_count": 0, "dependent_count": 0, "comment_count": 0}
-{"_type": "issue", "id": "hm-6ia", "title": "CI: run vhs against demo tapes to catch TUI flow regressions", "description": "Add a GitHub Actions job that installs vhs and runs the us-008..us-013 tapes (non-interactive, no gif diffing needed \u2014 just exit code) so broken tapes fail CI before they land. Cache cargo build, reuse release binary.", "status": "closed", "priority": 3, "issue_type": "task", "assignee": "Cooper Maruyama", "owner": "demo@himitsu.dev", "created_at": "2026-04-15T06:53:33Z", "created_by": "Cooper Maruyama", "updated_at": "2026-04-15T07:01:04Z", "closed_at": "2026-04-15T07:01:04Z", "close_reason": "Merged via parallel worktree agents; cargo test --bin himitsu tui::harness:: \u2192 3 passed", "dependency_count": 0, "dependent_count": 0, "comment_count": 0}
+{"_type":"issue","id":"hm-csd","title":"floating description tooltip / row expansion for selected search result","description":"On the search view, the currently selected item should have a floating tooltip containing the whole description. Or, alternatively, the currently selected item should expand to take up multiple rows so that the entire description is always visible.","status":"open","priority":2,"issue_type":"task","owner":"demo@himitsu.dev","created_at":"2026-05-10T02:31:59Z","created_by":"Cooper Maruyama","updated_at":"2026-05-10T02:31:59Z","dependency_count":0,"dependent_count":0,"comment_count":0}
+{"_type":"issue","id":"hm-634","title":"on search view, the currently selected item should have a floating tooltip containing the whole description. or, the currently selected item should expand to take up multiple rows so that the entire description is always visible","status":"open","priority":2,"issue_type":"task","owner":"demo@himitsu.dev","created_at":"2026-05-09T01:40:16Z","created_by":"Cooper Maruyama","updated_at":"2026-05-09T01:40:16Z","dependency_count":0,"dependent_count":0,"comment_count":0}


+        ProviderChoice::MacosKeychain => {
+            ensure_keychain_available()?;
+            let fp = fingerprint(pubkey);
+            MacOSKeychain.store_key(&fp, secret)?;
+        }


Copilot AI review requested due to automatic review settings May 10, 2026 02:57

Copilot started reviewing on behalf of cooper (czxtm) May 10, 2026 02:57 View session

chatgpt-codex-connector Bot reviewed May 10, 2026

View reviewed changes

github-advanced-security AI found potential problems May 10, 2026

View reviewed changes

Comment thread rust/src/crypto/keystore.rs

store_new_key(

&ProviderChoice::Disk,

dir.path(),

"AGE-SECRET-KEY-1ABCDEF",

Copilot AI reviewed May 10, 2026

View reviewed changes

cooper (czxtm) mentioned this pull request May 10, 2026

feat(tui): stop auto-init in projects + dual global/project health pills #19

Merged

5 tasks

cooper (czxtm) closed this pull request by merging all changes into main in 580629e May 10, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(keychain): actually wire the macOS Keychain provider#18

feat(keychain): actually wire the macOS Keychain provider#18
1 commit merged intomainfrom
feat-keychain-wiring

cooper (czxtm) commented May 10, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 10, 2026

Uh oh!

chatgpt-codex-connector Bot May 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		std::fs::create_dir_all(data_dir)?;
		std::fs::write(pubkey_path(data_dir), format!("{pubkey}\n"))?;

Conversation

cooper (czxtm) commented May 10, 2026

Summary

Wiring

Test plan

Out of scope

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 10, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot May 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants