You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(agent): store Windows secrets as encrypted files keyed from Credential Manager - #171
Fixes#107. This is an alternative to #108, following @howardjohn's suggestion there: keep a small key in Credential Manager and the secret, at any size, encrypted on disk. It needs no chunking and keeps one credential per secret.
What it does
Windows only: each secret is an AES-256-GCM file in the daemon's private state directory (<state>/secrets/<sha256(service, account)>, the same layout as on Linux, with a protected DACL for SYSTEM, Administrators and the owner). Credential Manager holds only that secret's 32-byte random key (base64, 44 characters) at <service>.file-key / <store id>/<account>. The store id (secrets/.store-id, random) scopes keys to their state directory, since Credential Manager is shared by everything running as one Windows account.
Writes: the key is created once and reused, so the atomic file replace is the only step that changes a stored secret. A failed key or file write leaves the current value readable. secrets/.lock serializes the store across threads and processes: writes and deletes hold it exclusively, so two first writes can't pair one write's key with the other write's file; reads hold it shared, so a read never spans a delete and re-create.
Binding: the file is authenticated with its service and account as AAD, so a file copied to another entry's path does not open.
Upgrade: values that earlier versions stored directly in Credential Manager (those that fit) are still read, and are replaced on the next write. Delete removes such a value first, so a delete that stops halfway can't bring an old value back.
Errors: a missing key (for example, Credential Manager reset or the state directory restored without it) reads as absent, with a warning, so the OAuth flows sign in again instead of failing on every request. A malformed key or a tampered file is a read error, which identity::save already handles (delete, then set). The next write repairs either case: when it has to create a new key, it removes the old file first, so a failure partway leaves a secret that reads as absent, never a file next to the wrong key.
Cost per write: a key read and the file write. Only the first write also removes an earlier version's Credential Manager value.
Linux and macOS are unchanged. No new dependencies: aws-lc-rs is already in the tree through rustls (the controller uses it directly).
Tested
Windows 11 Enterprise VM, not domain-joined, with the daemon installed as the AgentDesktop service (LocalSystem, --config C:\ProgramData\AgentDesktop\config.yaml) and Keycloak tokens with offline_access. Enrollment completes (it fails on main). The OAuth credential file is 2154 bytes, which would be about 4.3 KB as UTF-16 in Credential Manager. After a service restart the device reconnects to the controller with the same identity.
Unit tests for the envelope logic behind a small key-store trait, run on every platform with an in-memory store and injected failures: round trip beyond the credential limit, key reuse, upgrade from a stored value, failed key, file and cleanup writes, file bound to its entry, tampered file / missing key / malformed key, partial delete. They also ran as SYSTEM on the VM.
One Windows test round-trips a 10,000-character secret through the real Credential Manager.
On upgrade, the first write removes the old Credential Manager value. Rolling back to an earlier release after that means re-enrolling the device; keeping the value would leave a stale copy in Credential Manager.
Race between ciphertext reads and key replacement causes authentication failures
crates/agent/src/secret_store.rs:259
A reader can capture old ciphertext before a recovery write replaces a missing or malformed key, then read the replacement key and fail authentication. The same mismatch can occur across a concurrent delete followed by set. Removing the old file before rekeying does not protect ciphertext already held by a reader. This reports a read error even when the replacement secret is valid. Hold the same lock across the entire file/key read, and add a regression test for these interleavings.
Test failure preserves existing secret when updating path
crates/agent/src/secret_store.rs:859
The failed write targets blocked, but the assertion reads path, which that write never touches. This test would still pass if a failed update destroyed its destination, so it does not verify preservation of the existing secret. Inject a deterministic file-write or replacement failure while updating path itself, then verify that the original secret remains readable and its stored key is unchanged.
…ntial Manager
Windows Credential Manager limits a credential to 2560 bytes, and OAuth
credentials with refresh tokens exceed it, so enrolling the Windows service
failed (agentdesktop-dev#107). Store each secret as an AES-256-GCM file in the daemon's
private state directory and keep only its 32-byte random key in Credential
Manager, as suggested in agentdesktop-dev#108.
- The key is created once per secret and reused, so replacing the file
atomically is the only step that changes a stored secret.
- The file is bound to its service and account (AAD), so a file moved to
another entry's path does not open.
- Values stored directly in Credential Manager by earlier versions are still
read, and are replaced on the next write. Delete removes such a value first,
so a delete that stops halfway never brings an old value back.
- The envelope logic sits behind a small key-store trait and is tested on
every platform with an in-memory store and injected failures; one Windows
test round-trips a 10,000-character secret through Credential Manager.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A secret file whose key is missing reads as absent (with a warning), so
the GitHub, Anthropic and gateway OIDC flows sign in again instead of
failing on every request. A malformed key or tampered file stays an error.
- Only the first write removes an earlier version's Credential Manager
value, so later writes skip that call.
- The test credential store checks the limit in UTF-16 bytes, as keyring
stores it.
- aws-lc-rs is a Windows and dev dependency only.
- One SecretStore::new branch for Linux and Windows; the key lookup returns
an enum instead of a nested Option.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A read that misses the file and then finds no earlier-version value looks
at the file again: a concurrent first write stores the file before it
removes that value, so the read no longer reports a secret that existed
throughout as absent. Regression test for that interleaving.
- The entry-binding test gives both entries the same key, so only the AAD
binding can reject the copied file.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The in-process mutex did not cover a second daemon instance, which runs
secret-using tasks before it finds the first one on the named pipe. Two
competing first writes could then leave one write's file next to the
other's key. Writes and deletes now hold an exclusive lock on
`secrets/.lock` (std File::lock, LockFileEx on Windows), which serializes
threads and processes alike. Regression test with competing first writes,
each on its own lock handle; the failed-file-write test now fails at the
file replace rather than at a missing directory.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…with an old file
- Credential Manager is shared by everything running as one Windows
account, while the files and the lock are per state directory. Each
secrets directory now has a random identity (`secrets/.store-id`), and
file keys live at `<service>.file-key` / `<store id>/<account>`, so two
state directories no longer overwrite or delete each other's keys.
- When a write has to create a new key (missing or malformed), it removes
the old file first: nothing can open it, and a later failure then leaves
a secret that reads as absent instead of a file next to the wrong key.
- Regression tests for both, with two directories on one key store and a
check that no file exists when a new key is stored.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`read_to_string` failed with InvalidData on such a file, which blocked
every write until the file was removed by hand. Read the bytes and treat
anything that is not a valid identity as malformed, so the next write
replaces it. Other filesystem errors still propagate.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A read could load a file, then a delete and a new write could replace its
key before the read looked the key up; the read then reported the old
file as corrupt, which identity::load turns into IdentityUnreadable.
Reads now hold the store lock shared (writes and deletes hold it
exclusively), so reads still run in parallel but never span a replace.
This also covers the first-write race, so the re-read of the file in
get() is gone. Test: a writer on its own handle cannot take the lock while
a read looks up the key.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The failed-write test failed a write to another path, so it would pass
even if a failed update destroyed its own file. A test-only switch now
fails the next file write; the test updates the stored secret itself and
checks that the file and value are unchanged (it fails if set() removes
the file before writing).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.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
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.
Fixes #107. This is an alternative to #108, following @howardjohn's suggestion there: keep a small key in Credential Manager and the secret, at any size, encrypted on disk. It needs no chunking and keeps one credential per secret.
What it does
<state>/secrets/<sha256(service, account)>, the same layout as on Linux, with a protected DACL for SYSTEM, Administrators and the owner). Credential Manager holds only that secret's 32-byte random key (base64, 44 characters) at<service>.file-key/<store id>/<account>. The store id (secrets/.store-id, random) scopes keys to their state directory, since Credential Manager is shared by everything running as one Windows account.secrets/.lockserializes the store across threads and processes: writes and deletes hold it exclusively, so two first writes can't pair one write's key with the other write's file; reads hold it shared, so a read never spans a delete and re-create.identity::savealready handles (delete, then set). The next write repairs either case: when it has to create a new key, it removes the old file first, so a failure partway leaves a secret that reads as absent, never a file next to the wrong key.Linux and macOS are unchanged. No new dependencies:
aws-lc-rsis already in the tree through rustls (the controller uses it directly).Tested
AgentDesktopservice (LocalSystem,--config C:\ProgramData\AgentDesktop\config.yaml) and Keycloak tokens withoffline_access. Enrollment completes (it fails onmain). The OAuth credential file is 2154 bytes, which would be about 4.3 KB as UTF-16 in Credential Manager. After a service restart the device reconnects to the controller with the same identity.Notes for review
CryptProtectData), which needs no stored key. I kept the design suggested in fix: implement chunked Windows secret storage functionality #108 so it stays close to the other platforms' secret-store model.