Skip to content

fix(secrets): DPAPI migration failure is unobservable, no warning and no debug trace #487

Closed

Description

@basiliskus

Found while reviewing #464 at commit c46016c. Not a merge blocker; filing so it is tracked rather than lost in the review thread. Related to #461 (Windows support).

What this is not: not a regression (before the keychain-windows backend existed, every Windows host stored secrets as plaintext unconditionally, so a DPAPI-broken host lands exactly where it already was: an unrealized improvement, not a new exposure), and not a design error in the never-fail-the-read posture, which is correct and documented in the code comment. The write path is already loud about the identical failure.

The gap is purely observability: every failure path in _acq_secret_migrate_windows returns 0, the caller ignores the return, and nothing at all is emitted. Observed with a stub whose encrypt step always fails:

resolve -> rc=0, value returned correctly   (no message)
after 4 consecutive reads: file still plaintext on disk
ACQ_DEBUG=1: no output whatsoever

Consequence: on a host where PowerShell is present but DPAPI is unusable (e.g. Constrained Language Mode on a locked-down managed device), a user has no way to learn that the encryption-at-rest guarantee is not being met. For usai the 7-day key expiry pulls them onto the loud write path within a week; a github token has no forced expiry, so it can sit plaintext indefinitely. It also spends a PowerShell subprocess on every read, forever.

This is the case AGENTS.md's "Capture errors and document failures clearly" has in mind: the continuation is deliberate and correct here, but a security-relevant degradation should not be invisible.

Suggested fix: an acq_debug line on every migration failure plus a once-per-key stderr warning. No change to the best-effort, never-fail-the-read behavior.

Activity

  1. mogul commented on Oct 3, 2026

    @mogul
    Contributor

    Verified fixed by the merged #464 changes. Current main includes the migration observability path in acq.backends/secret-store.sh: failed Windows DPAPI legacy migration now calls acq_debug, emits a once-per-key warning through the preserved warning fd, leaves the legacy plaintext readable, and records a per-key warning marker. There is also a regression test covering the failed-migration/read-still-succeeds/warn-once behavior in test/bats/45-secret-store-unit.bats.\n\nLocal reproduction with a failing PowerShell stub showed:\n\n\nacq: warning: could not encrypt the 'acq.github' secret at rest with Windows DPAPI (DPAPI encryption failed); it remains in plaintext on disk. Re-run 'acq secret set' once DPAPI is available.\nr1=[legacyPLAINTEXT]\nr2=[legacyPLAINTEXT]\nat-rest=plaintext\nmarker=present\n\n\nThe focused bats runner could not be executed in this worktree because vendored bats submodules are not initialized, but the direct reproduction confirms the issue's observed failure mode is no longer silent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions