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>

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

Open

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

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LutzLange commented Oct 15, 2026 •

edited

Loading