Skip to content

internal/sops: a signal during the plaintext window leaves a secret in the work tree #520

Description

@cameronsjo

Context

Stated as a known residual when env set --sops landed in #517, and filed so it is tracked somewhere other than a merged PR body.

internal/sops' work directory is created as a sibling of the targetos.MkdirTemp(parent, ".forgectl-sops-") in newWorkDir (internal/sops/driver.go). That location is deliberate and should not be changed casually: os.Rename across filesystems returns EXDEV, so a backup in $TMPDIR would fail to restore in exactly the error paths a backup exists for, while the message claimed it had succeeded. The sibling is also what keeps sops' upward walk for .sops.yaml reaching the same rules it would from the target itself.

The consequence is that the directory sits inside the repository, and cleanup is a single defer work.cleanup() (driver.go:150). There is no signal handler on this path.

The window

Two files in that directory hold plaintext during a run:

  • the staged value, written for the child editor to read
  • the decrypted read-back used for the byte-exact comparison

Both are now discarded the moment they are consumed (discardStagedValue, discardLandedValue), which shrinks the window to the span where each file must exist rather than the whole run. But a signal inside that span skips the deferred cleanup and leaves a plaintext secret in the work tree. Reproduced on the first of forty kill attempts before the discards were added.

The directory name begins with a dot, so it is hidden from ls but not from git status — an uncleaned run leaves a committable secret visible as an untracked path.

Why this is not #513

#513's first item is internal/env's writeAtomic temp file, and its conclusion there is that a signal handler is the wrong tool because SIGKILL cannot be caught. That reasoning does not transfer wholesale: the case reproduced here was SIGINT, which is catchable, so a handler does address the one that was actually observed. SIGKILL still needs the sweep.

Suggested approach

Both parts, because neither covers the other:

  1. A handler for the catchable signals (SIGINT, SIGTERM) that runs the same cleanup before exiting. This closes the reproduced case.
  2. A sweep of stale .forgectl-sops-* siblings at the start of the next run against the same directory, under the lock — the same shape internal/env residual hardening: temp file on SIGKILL, lone CR in encodeValue, unpinned child binary #513 proposes for .env-*.tmp, and the only thing that covers SIGKILL and a power loss.

Worth deciding as part of this: whether the sweep should refuse to proceed when it finds one, rather than silently removing it. A leftover directory is evidence that a previous run died mid-write, and the operator may want to know that before another write lands on the same file.

Out of scope

Moving the work directory out of the repository. The EXDEV constraint above is the reason it is where it is, and relocating it means solving the cross-filesystem restore first — a larger change than this issue.

Activity

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

    area:forgectlComponent: forgectl binaryarea:securitySubject: security. Severity lives on impact:*impact:lowCosmetic or local inconvenience; obvious workaround; nobody blockedkind:choreUpkeep with no intended behavior change: deps, CI, cleanuplikelihood:lowNeeds unusual preconditions; not expected within the horizon (30 days)

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions