fix(byok): harden the Windows DPAPI worker lifecycle - #6282
fix(byok): harden the Windows DPAPI worker lifecycle#6282open-design-crew[bot] wants to merge 4 commits into
Conversation
lefarcen
added
size/M
|
Fixes #6285 WhyUse case: While validating the Windows prerelease package, the existing BYOK credential smoke timed out even though the package itself built successfully. Pain: The Windows backend launched a new encoded What users will seeOn Windows, saving, listing, resolving, and deleting BYOK profiles no longer pays a new PowerShell startup cost for every credential operation. The daemon initializes one DPAPI worker and reuses it. Existing DPAPI-encrypted The first packaged DPAPI initialization can still take tens of seconds on a cold machine, but subsequent operations complete in milliseconds instead of repeatedly incurring that delay. Surface area
ScreenshotsNot applicable. This changes the daemon's Windows credential backend only. Bug fix verification
Validation
Security and compatibility
|
Keep one PowerShell DPAPI worker alive for the daemon lifetime so packaged Windows pays the expensive script and assembly startup cost once, then reuses a local named pipe for credential operations. Preserve the existing DPAPI CurrentUser storage format and cover worker reuse across availability, set, get, and delete operations.
Match the packaged Windows smoke budget so the first DPAPI worker initialization can complete while retaining the existing timeout boundary for stalled desktop IPC calls.
a9d7e6a to
193e63d
Compare
open-design-crew
Bot
changed the title
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The long-lived worker removes repeated PowerShell startup cost, but its current lifecycle makes a transient worker failure permanent for the daemon and permits worker stalls to leave credential requests pending indefinitely. Both paths need bounded failure and recovery before this high-risk backend change is safe to merge.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| } | ||
|
|
||
| private getWorker(): Promise<WindowsDpapiWorkerClient> { | ||
| this.worker ??= this.options.createWorker?.() ?? WindowsDpapiWorker.create(); |
There was a problem hiding this comment.
Reset and recreate the cached worker after a worker-level failure. getWorker() stores one promise forever, while WindowsDpapiWorker.fail() permanently closes that instance; therefore a PowerShell crash, pipe disconnect, malformed response, or failed startup makes every later set/get/delete fail until the daemon is restarted. The previous per-operation implementation recovered naturally on the next call, so this is a production availability regression in the changed lifecycle. Wrap worker use so a failure clears this exact cached promise/instance (without racing a newer replacement), terminates the failed worker, and lets the next operation create a fresh worker; add a test that fails the first worker and proves a subsequent operation succeeds through a second worker.
| ? {} | ||
| : { secret: Buffer.from(secret, 'utf8').toString('base64') }), | ||
| }; | ||
| const response = await new Promise<WindowsDpapiWorkerResponse>((resolve, reject) => { |
There was a problem hiding this comment.
Bound worker startup and each request with explicit deadlines. The connection race only rejects if PowerShell exits, readyPromise only rejects if the pipe/process emits a failure, and this response promise only settles when data or an error arrives. A live but wedged PowerShell process can therefore leave availability or BYOK CRUD pending forever; the new 60-second desktop eval timeout merely abandons the caller while the daemon operation and serialized queue remain stuck. Add startup/readiness and per-operation timers that call fail(), reject the pending work, kill the worker, and clean up the pipe server, with fake-clock tests for a worker that never connects, never sends ready, and never answers an operation.
Bound startup, handshake, operation, and shutdown phases, recycle failed worker generations without replaying uncertain mutations, and close owned PowerShell processes during daemon shutdown. Add secret-safe diagnostics, protocol and input limits, plus regression coverage for crash recovery, parent-death cleanup, and the 60-second packaged eval contract. Validated with focused BYOK lifecycle tests, daemon and tools-pack typechecks, pnpm guard, and the workspace typecheck.
|
Implemented the lifecycle/recovery follow-up in Both blocking bot comments were valid and were fixed:
The same commit also adds daemon-owned shutdown, parent-PID orphan protection for force-terminated daemons, secret-safe structured diagnostics, API/protocol size limits, Windows native worker cleanup, and permanent regression tests. No bot suggestion was dismissed. Local validation: 39 focused daemon tests passed with 2 Windows-only tests skipped on macOS; the exact 60-second tools-pack contract test passed; daemon/tools-pack package typechecks, Remaining Windows evidence/security boundaries are explicit: the new native CRUD and parent-death tests still need exact-head Windows execution, and named-pipe ACL/peer hardening remains a separate security decision. Earlier non-publishing packaged evidence proves worker reuse and warm-path performance, but it does not substitute for validating this new lifecycle commit on Windows. |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The worker recovery, bounded deadlines, and daemon shutdown ownership address the earlier lifecycle blockers. One security boundary remains: the long-lived plaintext credential channel does not authenticate the process that connects, so named-pipe hardening must land with this change rather than remain a follow-up.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| // the random per-process local pipe below, keeping plaintext credentials | ||
| // out of argv, environment variables, and temporary files. | ||
| child.stdin.end(); | ||
| const connection = waitForConnection(server); |
There was a problem hiding this comment.
Authenticate the connecting pipe client before treating it as the DPAPI worker. This code races waitForConnection(server) against child failure and accepts whichever process connects first; the random pipe name is the only proof of identity. After line 360, every set request sends the API key in plaintext over that accepted socket, while get returns the decrypted key on it. A local process that discovers or guesses the pipe name can therefore win the race, send a fake ready frame, and read or forge credential operations. Restrict the pipe to the daemon user and verify the spawned worker (for example, validate the named-pipe client PID against child.pid, or require a per-worker challenge-response secret before ready), rejecting and closing any unverified connection. Add a Windows test where an impostor connects before the spawned child and prove it cannot receive a request or satisfy readiness.
|
Addressed the remaining PerishCode named-pipe peer-authentication blocker in Decision: the review suggestion was valid; no suggestion was dismissed.
The HMAC gate closes the first-connector identity/confidentiality issue without adding a native dependency or changing the worker/daemon lifetime. Node's named-pipe server still uses the OS default descriptor; an unauthenticated connector can receive only a one-time random challenge and cannot reach readiness or credential traffic. A custom logon-SID DACL or explicit remote-client rejection would be an additional OS-level defense-in-depth layer, not the identity proof for this protocol. Validation:
The exact-head native Windows/non-publishing validation is still required before merge because local macOS cannot execute the real DPAPI and Windows orphan cases. |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
@app/open-design-crew The current head addresses the worker-lifecycle and named-pipe security concerns: failed generations are retired without replaying side effects, startup/readiness/operations/shutdown are bounded, daemon shutdown owns worker cleanup, and the HMAC challenge gates all credential traffic. The focused recovery, timeout, shutdown, orphan, native-Windows, and impostor-connection tests cover the changed behavior well. Thanks for the thorough follow-up and for turning each earlier blocker into explicit regression coverage.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Heads-up: PR #6308 is also open against the same Windows BYOK/DPAPI area. Both PRs touch |
open-design-crew
Bot
changed the title
|
Closing as superseded by #6308. That PR already merged the chosen fix direction for the same Windows BYOK/DPAPI surface, and this PR now conflicts with main while overlapping the same files and review area. If maintainers want to revisit the worker-reuse approach later, it can come back as a fresh PR against the current base. |
2 participants
Fixes #6285
Why
Use case: The
0.17.0prerelease Windows packaged smoke repeatedly timed out while creating, listing, and deleting secure BYOK profiles.Pain: The original Windows backend launched a new encoded
powershell.exeprocess for every DPAPI probe, read, write, and delete. In the packaged environment, cold PowerShell andSystem.Securitystartup took about 36–42 seconds while DPAPI itself took milliseconds, so repeated cold starts exceeded the 60-second desktop eval budget. A long-lived worker also needed bounded lifecycle, recovery, shutdown, and authenticated IPC behavior before it was safe to merge.What users will see
Windows packaged users keep the same
CurrentUserDPAPI-encrypted profiles and the same API and UI behavior. The first access may still pay one cold PowerShell startup, but later BYOK operations reuse the warm worker. A fatal worker failure fails the current operation, retires that worker generation, and lets the next independent request recover with a fresh worker; uncertain mutations are never replayed automatically. Daemon shutdown now closes the worker it owns, and tools-pack keeps the exact 60-second Windows desktop eval budget.There are no visible changes on macOS or Linux, and this PR does not change routes, API contracts, the desktop entry flow, or BYOK storage paths and ciphertext compatibility.
Surface area
Screenshots
Not applicable — this is a backend-only Windows runtime change.
Implementation notes
setordeleteafter an uncertain failure, avoiding duplicate or misreported side effects.Bug fix verification
apps/daemon/tests/byok/credential-service.test.ts,apps/daemon/tests/byok/credential-service.windows.test.ts,apps/daemon/tests/byok/windows-dpapi-backend-lifecycle.test.ts,apps/daemon/tests/byok/windows-dpapi-worker.test.ts,apps/daemon/tests/byok/windows-dpapi-worker-orphan.windows.test.ts,apps/daemon/tests/server-shutdown.test.ts, andtools/pack/tests/win-lifecycle-timeouts.test.ts.main: the release failure is the production red-spec evidence. The exact non-publishing Windows diagnostic was intentionally temporary and removed after proving the repeated cold-start cause, so the permanent tests focus on the worker lifecycle, security, and timeout contracts.Windows tools-pack payload testsjob validates payload construction but does not execute packaged DPAPI or the Windows-only orphan and peer-authentication paths.Validation
apps/daemontypecheck: passed.tools/packtimeout suite and typecheck: passed.pnpm guard: passed.pnpm typecheck: passed.git diff --check: passed.Remaining boundaries
powerformer/vela#961are related context but independent changes.