fix(desktop): one OS credential per secret, lifting the 2560-byte Windows cap - #5275
Open
j-goodz wants to merge 1 commit into
Open
fix(desktop): one OS credential per secret, lifting the 2560-byte Windows cap#5275j-goodz wants to merge 1 commit into
j-goodz wants to merge 1 commit into
Conversation
…nt cap Every managed-agent private key shared a single Windows Generic credential. The keyring crate validates the blob against CRED_MAX_CREDENTIAL_BLOB_SIZE (2560 bytes) before CredWriteW, so an oversized write fails atomically rather than truncating. A real store holding an identity plus 8 agents already occupies ~2380 bytes, leaving ~90 bytes of headroom against the ~139 that each further `agent:<npub>` -> `nsec...` entry needs. Agent block#9 onward therefore could never reach the OS keyring at all. The failure was silent, which is what made it hard to spot: the store fell back to writing the key inline in managed-agents.json, so the app kept working and surfaced no error, while those private keys quietly stopped being protected by the OS credential store. SecretStore now writes one credential per secret, keyed by the existing per-secret name under the same service, so no single blob can reach the cap. load_all_readonly() and the sign-out wipe need to ENUMERATE secrets, which per-credential storage cannot do portably, so this adds an index holding key NAMES only. The index is chunked at 1000 chars per credential; a single index credential would have re-created the identical cliff at ~17 agents, since an `agent:<npub>` name is ~70 chars. load() addresses credentials by name and never consults the index, so a damaged index degrades enumeration but cannot lose a key. Migration runs once on first access under the existing advisory lock and is all-or-nothing: read the blob, write each secret to its own credential, read each one back from the OS and verify it, add the names to the index, and only then delete the blob. Any failure restores every credential the attempt touched to its prior value and leaves the blob byte-for-byte intact with migrated=false, so the next access retries and reads fall back to the surviving blob. The deleted lines are the now-dead blob machinery (mutate_blob, write_blob_raw, load_blob, read_blob_raw_keyring) plus probe_legacy_key_keyring and migrate_legacy_key_keyring. Those last two became actively harmful: per-key entries written by block#1264 live at exactly the address the new format uses, so the legacy migrator would have written a credential and then deleted it. Tests use an in-process fake backend that rejects writes over CRED_MAX_CREDENTIAL_BLOB_SIZE exactly as the Windows backend does, so they need no real keychain. sixteen_agents_plus_identity_all_reach_the_credential_store asserts the fixture exceeds 2560 bytes when packed as one blob, so it cannot pass vacuously; the rollback tests assert that a failed write leaves no partial credentials behind and an intact blob. A public-API guard test pins every entry point callers bind to. Verified on Windows 11: a build without this change shows a single `secrets.buzz-desktop` credential, while a build with it shows one credential per agent (`agent:<pubkey>.buzz-desktop-dev`). Known trade-offs: on macOS the per-item "Always Allow" grant means the first load_all_readonly() after upgrade can prompt once per credential (Windows has no prompts). Once migrated, an older Buzz build will not find the secrets. Signed-off-by: j-goodz <22462858+j-goodz@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Duplicate search: no open PR or issue found covering the Windows credential blob size cap.
Problem
On Windows every secret shared a single Generic credential. The
keyringcrate validates the blob againstCRED_MAX_CREDENTIAL_BLOB_SIZE(2560 bytes) beforeCredWriteW, so an oversized write fails atomically rather than truncating.A real store holding an identity plus 8 agents already occupies ~2380 bytes, leaving ~90 bytes of headroom against the ~139 that each further
agent:<npub>→nsec...entry needs. Agent #9 onward could never reach the OS keyring at all.Why it wasn't obvious
The failure was silent. The store fell back to writing the key inline in
managed-agents.json, so the app kept working and surfaced no error — while those private keys quietly stopped being protected by the OS credential store. Nothing in the UI or logs indicated the downgrade.The change
SecretStorenow writes one credential per secret, keyed by the existing per-secret name under the same service, so no single blob can reach the cap.load_all_readonly()and the sign-out wipe need to enumerate secrets, which per-credential storage cannot do portably, so this adds an index holding key names only. The index is chunked at 1000 chars per credential — a single index credential would have re-created the identical cliff at ~17 agents, since anagent:<npub>name is ~70 chars.load()addresses credentials by name and never consults the index, so a damaged index degrades enumeration but cannot lose a key.The deleted lines are the now-dead blob machinery (
mutate_blob,write_blob_raw,load_blob,read_blob_raw_keyring) plusprobe_legacy_key_keyring/migrate_legacy_key_keyring. Those last two became actively harmful: per-key entries written by #1264 live at exactly the address the new format uses, so the legacy migrator would have written a credential and then deleted it.Migration safety
Migration runs once on first access under the existing advisory lock and is all-or-nothing:
Any failure restores every credential the attempt touched to its prior value and leaves the blob byte-for-byte intact with
migrated = false, so the next access retries and reads fall back to the surviving blob.Known limitation, stated rather than left to review
If step 5 — deleting the now-redundant blob — fails, the code logs and still marks the store migrated. The blob then survives alongside the per-secret credentials. A
delete(key)afterwards removes the per-secret credential but not the blob's copy, so the next process start re-runs migration, reads the stale blob, and can resurrect that secret.It needs a failing
CredDeleteWto reach, which is why it has not been observed in practice, and it is strictly narrower than the bug being fixed. I have deliberately left it rather than fold an unverified behaviour change into a diff that has been running against a real Windows credential store. Happy to fix it here or as a follow-up — maintainer's preference.Tests
Tests use an in-process fake backend that rejects writes over
CRED_MAX_CREDENTIAL_BLOB_SIZEexactly as the Windows backend does, so they need no real keychain:sixteen_agents_plus_identity_all_reach_the_credential_storeasserts the fixture exceeds 2560 bytes when packed as one blob, so it cannot pass vacuouslymigration_rolls_back_and_leaves_blob_intact_when_one_write_failsandmigration_rollback_restores_prior_credential_valuesassert a failed write leaves no partial credentials and an intact blobpublic_api_surface_is_unchangedpins every entry point callers bind toVerification
Verified on Windows 11: a build without this change shows a single
secrets.buzz-desktopcredential, while a build with it shows one credential per agent (agent:<pubkey>.buzz-desktop-dev).cargo fmt --all -- --checkis clean, and the change introduces zero new clippy warnings measured as a delta against the pristine base (47 pre-existing before and after).cargo test -p buzz-desktop --libgives 2151 passed / 1 failed against a baseline of 2145 / 1 — the one failure,managed_agents::runtime::tests::claude_spawn_uses_the_probed_cli_executable, fails identically on an unmodified checkout and passes in isolation. It is a pre-existing parallel-ordering flake, unrelated to this change.Trade-offs
load_all_readonly()after upgrade can prompt once per credential. Windows has no prompts.