Skip to content

feat(desktop): add non-secret exact-agent credential persistence attestation - #5312

Closed
NedMalki-Chief wants to merge 1 commit into
block:mainfrom
NedMalki-Chief:feat/desktop-credential-persistence-attestation
Closed

feat(desktop): add non-secret exact-agent credential persistence attestation#5312
NedMalki-Chief wants to merge 1 commit into
block:mainfrom
NedMalki-Chief:feat/desktop-credential-persistence-attestation

Conversation

@NedMalki-Chief

Copy link
Copy Markdown

Summary

Adds a public, non-secret per-agent credential-persistence attestation (buzz.desktop.exact_agent_credential_persistence.v1), so external controllers and downstream integrations can verify that a named managed agent's credential is OS-keyring-backed — exact-agent-bound, no inline fallback — without any access to key material and without scraping managed-agents.json or the OS keychain.

Proposed in #5311 (motivation, and how this relates to #2754 / #3721 / #4925 / #3205 — no duplicate found among open PRs).

What's in the change

  • managed_agents/persistence_attestation.rs — the schema (AgentPersistenceAttestation, extensible PersistenceBackend) and a pure builder. By construction no code path can carry the nsec: inputs are a presence boolean, a keyring probe value, and public identity material. Fail-closed errors instead of guesses: attestation_keyring_unreachable, attestation_credential_missing. attestation_hash (SHA-256 of the payload with that field empty) makes the object tamper-evident; verify_attestation_hash is exported so callers and tests share one definition.
  • managed_agents/storage.rsreadonly_agent_key_probe (via load_all_readonly, so it can never trigger migrate_legacy_key side effects) and observe_agent_credential_persistence, which reads the raw persisted store: post-hydrate_keys records carry the nsec even in the normal keyring-backed case, so only pre-hydration JSON can distinguish inline fallback from keyring-backed.
  • commands/agent_attestation.rs — one read-only #[tauri::command] get_agent_persistence_attestation(pubkey), spawn_blocking + store lock, following the list_managed_agents / get_identity non-secret-projection precedent. Self-checks the hash invariant before returning.
  • desktop/src/shared/api/agentAttestation.ts — thin typed wrapper. The wire shape stays snake_case verbatim: the hash binds the exact serialized payload, so client-side remapping would break external verification.
  • docs/agent-credential-persistence-attestation.md — schema, semantics, and explicit non-goals (not NIP-TR, not a NIP-OA replacement, not a secret channel).

Design notes

  • Tests live in #[path]-included sibling _tests.rs files per the 1000-line file-size gate; the storage-side probe is tested against the existing FakeKeyStore harness (present / other-agent's-entry / no-blob / outage / never-writes).
  • Builds without the system-keyring feature attest inline_file honestly rather than erroring — inline is a true statement about where the key lives there.
  • KeyringProbe::Unreachable is an error, not an attestation: "may exist but unprovable this boot" must never read as os_keyring.
  • One test asserts the serialized object contains exactly the nine schema fields and no nsec substring, so the no-secret guarantee is enforced by CI rather than by review attention.

Verification

Run on Windows (x86_64-pc-windows-msvc, toolchain 1.95.0):

  • cargo test --lib in desktop/src-tauri2156 passed, 1 failed, 10 ignored. The single failure is managed_agents::runtime::tests::claude_spawn_uses_the_probed_cli_executable, which passes in isolation (cargo test --lib claude_spawn_uses_the_probed_cli_executable) and is unrelated to this change — it mutates the process-wide PATH and is order-dependent on this machine. The 11 new tests (7 persistence_attestation, 4 readonly_probe_*) all pass.
  • cargo clippy -p buzz-desktop --all-targets --no-deps -- -D warnings — no findings in any file this PR touches. (Pre-existing findings elsewhere on main at 02f640b, e.g. unused imports in crates/buzz-terminal and managed_agents/nest.rs, are untouched by this PR.)
  • cargo fmt --check clean on all changed Rust files; biome clean on the new TS file.
  • Commit is DCO signed-off.

Happy to adjust surface details (command vs. settings export, field naming, casing) to maintainer preference.

…station

External controllers that assign work to a named managed agent need to
verify that its credential is OS-keyring-backed, bound to exactly that
agent, and not sitting in the inline JSON fallback - without any access
to key material and without scraping managed-agents.json or the OS
keychain.

Adds buzz.desktop.exact_agent_credential_persistence.v1: a pure builder
whose inputs cannot carry the nsec by construction, a read-only storage
observation (raw pre-hydration store for inline detection, presence via
load_all_readonly so it can never trigger migrate_legacy_key), one
read-only Tauri command following the get_identity non-secret-projection
precedent, a typed TS wrapper, and docs. Fails closed with
attestation_keyring_unreachable / attestation_credential_missing rather
than guessing; attestation_hash makes the object tamper-evident; the
backend enum is extensible so a future secrets-provider backend becomes
a new variant without breaking consumers.

Signed-off-by: Ned Malki <ned@ottomato.ai>
@NedMalki-Chief
NedMalki-Chief requested a review from a team as a code owner August 8, 2026 10:02

@wolfyy970 wolfyy970 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed the implementation at 96b4b06c20. I think the local storage signal is useful, but I don’t think this should merge as an external attestation in its current form.

The self-hash is only a checksum. A caller can change persistence_backend, stock_release_id, or issued_at and recompute it; there is no authenticated issuer, challenge, audience, expiry, replay protection, or launch binding. It cannot safely open the external compliance gate described in the PR.

The exact-agent and secret boundaries also are not yet true. An entry named agent:<pubkey> does not prove its stored nsec derives that pubkey, and load_all_readonly brings every stored agent key into this code path just to answer one presence question. That contradicts the stated no-secret path and widens exposure.

I would move the useful result into #2957 as narrow owner-local evidence: keyring verified, inline fallback, missing, unavailable, or mismatched. The storage layer should inspect only the target entry, and the UI should distinguish stored from used by the current launch. If an external controller needs a verifiable claim, that should be a separate challenge-bound signed receipt with an authenticated issuer and launch identity. A Tauri renderer command and self-hash are not that boundary.

@NedMalki-Chief

Copy link
Copy Markdown
Author

@wolfyy970 you're right on all four points. Verifying them against the code rather than just agreeing:

Self-hash. Correct, and my PR text calling it "tamper-evident" overstated it. attestation_hash is a checksum over a payload anyone can regenerate — no authenticated issuer, challenge, audience, expiry, replay protection, or launch binding. It catches accidental truncation and nothing adversarial. It cannot open the external gate I described, and naming the object an "attestation" invited exactly that misreading.

Exact-agent. Correct. An entry named agent:<pubkey> proves an entry exists under that name, not that the stored nsec derives that pubkey. Proving it means loading the secret and deriving — which this design deliberately refuses — so exact_agent_credential_persistence overclaims what the check can know.

load_all_readonly. Correct, and my justification for it was factually wrong. I used it to avoid migrate_legacy_key side effects, but probe() doesn't migrate — it calls the read-only probe_legacy_key. Only load() migrates (secret_store.rs:549). So I pulled a full HashMap of every stored secret into the command frame to dodge a side effect that wasn't there. (probe() reads the blob internally too, but it returns an enum instead of handing every secret to the caller.)

One more, found while checking the above, which argues for #2957's approach specifically: probe() falls back to probe_legacy_key when the blob is missing or lacks the key, so it still finds credentials in the old per-key format. load_all_readonly is load_blob() alone, with no legacy fallback. On first launch after an upgrade from the per-key format my version would report an un-migrated agent as attestation_credential_missing — a false negative on a credential that exists. #2957's probe()-based check is strictly more correct here, not merely narrower.

Trust boundary. Agreed. A Tauri renderer command is owner-local IPC, not an external trust surface, and a verifiable external claim would need to be a challenge-bound signed receipt with an authenticated issuer and launch binding. That's a different design and shouldn't be bolted onto this one.

Closing in favour of #2957. Thanks for the careful read.

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