SIEVE_PASSPHRASE_FILE/FD3 take precedence over TTY prompt#25
Merged
Conversation
…prompt Previous priority was TTY → file → FD 3, which meant operators who'd wired up SIEVE_PASSPHRASE_FILE (Docker secret, systemd LoadCredential=, Kubernetes mounted secret) still got prompted at the TTY first and had to either CTRL-C or close stdin before the file path was consulted. New priority: file → FD 3 → TTY → error. Security stance is unchanged — still no env var holding the passphrase itself; the env var holds a path. When the source is a file/FD 3, the TTY-flow "confirm" step is silently skipped (there's nothing to confirm against a static source). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Reorders Sieve’s passphrase acquisition priority so configured non-interactive sources (SIEVE_PASSPHRASE_FILE, then FD 3) are consulted before falling back to an interactive TTY prompt, and updates operator-facing documentation to match.
Changes:
- Updated
secrets.Acquire()dispatch order to prefer file → FD 3 → TTY, and adjusted the “no source” error message. - Updated binary/package and operator docs to reflect the new intake priority.
- Documented that the TTY “confirm” step is skipped when the passphrase is sourced from file/FD 3.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/secrets/prompt.go | Reordered passphrase source selection in Acquire() and updated its doc comment/error message. |
| cmd/sieve/main.go | Updated top-level documentation comment to reflect new intake priority. |
| docs/credential-encryption.md | Updated the “Passphrase intake” priority list and added note about skipped confirm for non-TTY sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot round 1 surfaced three variations on the same concern: with the new precedence (file/FD3 before TTY), --rotate-passphrase calls Acquire twice — once for the current passphrase, once for the new — and both reads would silently come from SIEVE_PASSPHRASE_FILE if set. The "new == current" guard then fails and rotation never happens. - secrets/PromptOptions: added RequireTTY. Set explicitly by callers capturing a *new* passphrase, or implied by Confirm=true (you can't confirm a value against a static source). With RequireTTY set, Acquire skips file/FD3 and errors out with a clear "this prompt requires a TTY" message when stdin isn't a TTY. - cmd/sieve: --rotate-passphrase's second Acquire (Confirm=true) now refuses to silently re-use the file source. The first Acquire still reads the file/FD3 source for the current passphrase so operators with unattended deployments can rotate without typing the existing passphrase. --setup also benefits from the implication (it already passes Confirm=true). - secrets/prompt_test.go: six focused unit tests pin the source- selection contract (file > FD3 > TTY > error; RequireTTY skips file/FD3; empty file fails loudly; Confirm implies RequireTTY). - docs/credential-encryption.md: new "Setup and rotation flows require a TTY" subsection documents the gate and shows the `env -u …` workaround for operators who hit the new error message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four wording corrections — no behavior change.
1. Acquire docstring: the "Confirm is ignored for file/FD 3" claim is
misleading because Confirm=true implies RequireTTY=true, so Acquire
never actually reaches the file/FD 3 branches. Reworded to say
Confirm only matters on the TTY path.
2. RequireTTY error message: the previous wording implied
SIEVE_PASSPHRASE_FILE being set was "not supported", which is
inaccurate — the env var works fine when stdin IS a TTY. The new
message names the actual cause ("stdin is not interactive") and
then explains why file/FD3 are skipped on this branch (so an
unattended file source can't satisfy a confirmation / new-passphrase
prompt).
3. docs/credential-encryption.md: removed the inaccurate "refuses to
run if SIEVE_PASSPHRASE_FILE is set" claim for --setup and
--rotate-passphrase. Replaced with "ignores file/FD 3 and reads
only from the TTY" — env var can stay set, it just isn't consulted
here.
4. Troubleshooting note: dropped the misleading "unset the env var"
instruction since unsetting doesn't help when the real cause is
non-interactive stdin. Replaced with "re-run from an interactive
shell" and a pointer to tmux/screen for the script-over-ssh case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three corrections from Copilot's review of 9eadde3: 1. docs: the standalone "When the passphrase comes from a file or FD 3, the confirm step is skipped" sentence implied first-run setup degrades gracefully under a file source. Reality: --setup forces the TTY path and refuses file/FD 3 entirely. Reworded so routine startup vs setup/rotate behavior is clearly separated, and the reader is pointed to the "Setup and rotation flows require a TTY" subsection for the operational consequences. 2. prompt_test.go: added a `//go:build !windows` tag. The FD 3 manipulation uses syscall.Dup / syscall.Dup2, which aren't in Windows' syscall package. Same pattern as cmd/sieve/rotate_test.go and internal/policy/script_path_test.go. 3. prompt.go error message: reworded `SIEVE_PASSPHRASE_FILE and FD 3 do not influence this branch` to `Neither SIEVE_PASSPHRASE_FILE nor FD 3 influences this branch`. The compound-subject "do" is defensible grammar but Copilot flagged it as ambiguous; the neither/nor form is unambiguous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two doc/test-shape corrections — no behavior change. 1. cmd/sieve/main.go: rotate's current-passphrase comment claimed the *new* passphrase "has RequireTTY=true" — the code below only sets Confirm=true and relies on the Confirm-implies-RequireTTY rule in secrets.Acquire. Reworded to surface that implication so the comment can't drift if Acquire's behavior ever changes. 2. internal/secrets/prompt_test.go: openFD3Pipe's docstring claimed the caller doesn't need to close the returned file, but the callers still `defer fd3.Close()`. Removed the defers (cleanup is already registered via t.Cleanup inside openFD3Pipe), dropped the now-unused return value, and tightened the docstring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reworded the TTY-required error message. The previous draft had "--setup and --rotate-passphrase's new-passphrase prompt only accept" — two subjects + singular "prompt" + plural "accept" don't agree. New form: "both --setup and --rotate-passphrase's new-passphrase prompt only accept" reads as "both X and Y ... accept", which is grammatical and clearer. Behavior unchanged; tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reorders Sieve's passphrase intake priority so that
SIEVE_PASSPHRASE_FILE(and FD 3) take precedence over the TTY prompt.Before: TTY → file → FD 3. An operator with the env var pointing at a Docker secret / systemd
LoadCredential=file / Kubernetes mounted secret still got prompted at the TTY first on every interactive start.After: file → FD 3 → TTY → error. Operators with wired-up credential plumbing aren't re-prompted.
What does NOT change
/proc/<pid>/environ,ps, crash dumps).Acquirechanged.--setup,--rotate-passphrase, and--reset-keyringstill use the same Acquire entry point — they just won't double-prompt when a file source is configured. The TTY-flow "confirm" step is silently skipped when reading from a file/FD 3 (nothing to confirm against a static source).Files
internal/secrets/prompt.go— reordered the dispatch inAcquire(); updated the priority-order doc comment.cmd/sieve/main.go— updated the package-doc summary of intake priority.docs/credential-encryption.md— updated the priority list + noted the skipped-confirm behavior.Test plan
unset SIEVE_PASSPHRASE_FILE; ./sieve→ prompts at TTY as before.SIEVE_PASSPHRASE_FILE=/run/secrets/pp ./sievefrom an interactive shell → reads the file, no TTY prompt.SIEVE_PASSPHRASE_FILEpointing at an empty file → fails loudly with "passphrase file %q is empty" (unchanged).SIEVE_PASSPHRASE_FILEpointing at a non-existent path → fails loudly with the wrapped open error (unchanged).go test ./internal/secrets/... ./cmd/sieve/...(already green).🤖 Generated with Claude Code