fix: implement chunked Windows secret storage functionality by saschajohn · Pull Request #108 · agentdesktop-dev/agentdesktop

saschajohn · 2026-09-18T15:44:37Z

Add support for Windows secret storage with chunking and manifest validation.

Fixes #107

Add support for Windows secret storage with chunking and manifest validation.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot review overview

🟡 Changes recommended

Non-atomic updates can corrupt credentials or permanently orphan old secret chunks.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

Handle chunk cleanup failures after committing the new secret · New
Bound manifest chunk counts before allocation or deletion · New
Add Windows chunk lifecycle and fault-injection tests · New

+                if let Some(existing_chunks) = &existing_chunks {
+                    delete_windows_chunks(service, account, 0..existing_chunks.len())?;


+    let chunk_count = chunk_count
+        .parse::<usize>()
+        .context("parse Windows secret chunk manifest")?;
+    if chunk_count < 2 {
+        bail!("Windows secret chunk manifest must contain at least two chunks");
+    }
+    Ok(Some(chunk_count))


+    #[test]
+    fn windows_secret_helpers_preserve_utf8_boundaries() {
+        let secret = format!("{}🙂tail", "a".repeat(WINDOWS_SECRET_CHUNK_SIZE - 1));
+        let chunks = utf8_chunks(&secret, WINDOWS_SECRET_CHUNK_SIZE).collect::<Vec<_>>();


peterj

thanks for the PR! can you address the comments please?

LutzLange · 2026-10-14T19:05:26Z

Tested this on a Windows 11 Enterprise VM (build 26300, not domain-joined) with the daemon installed as the AgentDesktop service (LocalSystem, --config C:\ProgramData\AgentDesktop\config.yaml, same layout as the MSI), enrolling against a Keycloak realm whose tokens include offline_access.

On main (05d3030) enrollment can't complete. The controller enrolls the device, then the daemon fails storing the credentials and starts the sign-in again:

ERROR agentdesktop_agent::remote: controller integration failed; restarting controller=https://127.0.0.1:9443 retry_in_seconds=1 error=store OAuth credentials: store secret in operating system credential store: Value of 'password encoded as UTF-16' is longer than the platform limit of 2560 chars

With this PR (58bc554) the same setup enrolls, connects and applies the pushed configuration.

Building on the chunking here, I tried an approach for the three review points, as one commit on top of 58bc554: LutzLange@3804cd8

Commit and cleanup: two fixed chunk slots (a, b). A write fills the slot the current value doesn't use and then switches the manifest (agentdesktop-secret-chunks-v2:<slot>:<count>:<tag>) in one entry write, so the current value stays complete until the new one is committed and the restore path isn't needed. Cleanup after the commit is best effort and doesn't fail the write. Chunks are written ascending and deleted descending, so leftovers stay a contiguous run and the next set or delete finds them by walking up to the first missing entry (one lookup per slot when there is nothing to clean).
Concurrent reads: each chunk starts with the write's random tag, and a read checks the tags against the manifest and reads again if a write replaced the chunks meanwhile, so a read that overlaps a token refresh can't return a value spliced from two writes.
Bound: 2..=64 chunks (about 75 KB), checked before writing and when reading the manifest.
Compatibility: v1 manifests from this PR's first commit are still read and their chunks cleaned up on the next write; values starting with the reserved prefix are always chunked, so a plain secret can't be read as a manifest.
Tests: the chunk logic sits behind a small credential-store trait, tested on every platform with an in-memory store that enforces the real limit (2560 bytes of UTF-16) and injects failures: failed chunk write, failed manifest write, a manifest write that reports an error after being applied, failed cleanup after commit, failed delete then retry, a read racing two writes. A Windows test round-trips long, short and shrinking values through the real Credential Manager and checks both slots after every write.

On the VM the tests pass as SYSTEM, including the Credential Manager round trip, and the service enrolls on the first attempt and reconnects after a restart without a new sign-in.

Happy to adjust, or to open it against your branch if that's easier.

howardjohn · 2026-10-14T19:08:30Z

is chunking really the best approach? or store a key (small) in the storage and encrypt the data (arbitrary size) on the disk?

LutzLange · 2026-10-15T07:26:55Z

@howardjohn Agreed, chunking is much more complicated. I implemented your suggestion in #171: each secret is an AES-256-GCM file in the private state directory, and Credential Manager keeps only its 32-byte key. The Windows service enrolls as LocalSystem against Keycloak (offline_access) and keeps its identity across restarts. Secrets already stored in Credential Manager are still read, and get replaced on the next write.

@saschajohn this builds on your analysis in #107.

…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>

…ntial Manager (#171) * fix(agent): store Windows secrets as encrypted files keyed from Credential Manager Windows Credential Manager limits a credential to 2560 bytes, and OAuth credentials with refresh tokens exceed it, so enrolling the Windows service failed (#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 #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> * fix: address review on Windows secret envelope - 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> * fix: address Copilot review on Windows secret envelope - 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> * fix: serialize Windows secret writes across processes 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> * fix: scope Windows file keys to their store and never pair a new key 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> * fix: treat a non-UTF-8 store identity as malformed `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> * fix: take the store lock shared for reads 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> * test: fail the file write of the secret being updated 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> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

fix: implement chunked Windows secret storage functionality

58bc554

Add support for Windows secret storage with chunking and manifest validation.

peterj requested a balanced review from Copilot September 28, 2026 19:32

Copilot started reviewing on behalf of peterj September 28, 2026 19:33

View session

Copilot AI reviewed Sep 28, 2026

peterj requested a balanced review from Copilot September 29, 2026 18:09

Copilot started reviewing on behalf of peterj September 29, 2026 18:11

View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

peterj requested changes Sep 29, 2026

View reviewed changes

LutzLange mentioned this pull request Oct 14, 2026

A failed local save after enrollment leaves an orphaned device at the controller #161

Open

This was referenced Oct 14, 2026

feat: report the browser PAC per device, with an optional compliance check #162

Open

fix(agent): store Windows secrets as encrypted files keyed from Credential Manager #171

Merged

This was referenced Oct 20, 2026

feat(agent): launch Claude Code in a Windows AppContainer (MXC) from the user-mode daemon LutzLange/agentdesktop#37

Draft

feat(agent): per-session pairing and the /sandbox route on the local LLM proxy LutzLange/agentdesktop#41

Draft

Navigation Menu

Repository navigation

fix: implement chunked Windows secret storage functionality - #108

fix: implement chunked Windows secret storage functionality#108
saschajohn wants to merge 1 commit into
agentdesktop-dev:mainfrom
saschajohn:fix/107-windows-chunks

saschajohn commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

peterj left a comment

Uh oh!

LutzLange commented Oct 14, 2026

Uh oh!

howardjohn commented Oct 14, 2026

Uh oh!

LutzLange commented Oct 15, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		if let Some(existing_chunks) = &existing_chunks {
		delete_windows_chunks(service, account, 0..existing_chunks.len())?;

Conversation

saschajohn commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

peterj left a comment

Choose a reason for hiding this comment

Uh oh!

LutzLange commented Oct 14, 2026

Uh oh!

howardjohn commented Oct 14, 2026

Uh oh!

LutzLange commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LutzLange commented Oct 15, 2026 •

edited

Loading