Recover from a failed secrets-init instead of skipping silently - #7
Merged
Conversation
…silently
Reported from the monitoring host, one step after the previous fix:
$ make secrets-edit
sops secrets/observability.sops.yaml
sops metadata not found
Two failures compounding.
The *original* broken script redirected sops' stdout into the destination:
sops --encrypt secrets/observability.example.yaml > secrets/observability.sops.yaml
Shell redirection creates the file before the command runs, so when sops failed
with "no matching creation rules found" it had already left a 0-byte
secrets/observability.sops.yaml behind.
The *fixed* script then tested only `[[ -f "${SECRETS_FILE}" ]]`, found the empty
file, reported "already exists — leaving it alone", and did nothing. `make
secrets-init` appeared to succeed while creating nothing, and the failure
surfaced one step later as an unhelpful sops error.
"The file exists" is not the same as "the secrets are set up". Three states now
get three answers:
* exists and contains a sops: metadata block -> genuinely done, left alone
* exists, non-empty, not encrypted -> refuse and explain. Never delete:
it could be real credentials
someone wrote and has not yet
encrypted. The error offers both
`sops --encrypt --in-place` and
the removal command.
* absent, or an empty leftover -> remove the empty file (it carries
no data) and create properly
Verified against a clone rolled back to the broken script, so the reproduction
is the real sequence rather than a synthetic one:
* a 0-byte leftover is removed and replaced with a genuinely encrypted file
* an already-encrypted file is byte-identical afterwards
* a non-empty unencrypted file is refused and left fully intact, contents
unchanged
There was a problem hiding this comment.
🟡 Not ready to approve
The new state detection hides certain underlying file-read errors and includes unquoted remediation commands that could mislead or be unsafe to copy/paste in edge cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves scripts/bootstrap.sh so make secrets-init can reliably recover from prior failed runs (notably the 0-byte leftover secrets file scenario) instead of silently skipping and failing later during make secrets-edit.
Changes:
- Detect “already set up” secrets by checking for a
sops:metadata block, not just file existence. - Refuse to overwrite non-empty, non-encrypted secrets files (to avoid destroying potential real credentials) and provide remediation steps.
- Automatically remove empty leftover secrets files and recreate them properly from the template.
File summaries
| File | Description |
|---|---|
| scripts/bootstrap.sh | Adds robust state detection for the secrets file (encrypted vs plaintext vs empty leftover) and safer recovery behavior during bootstrap. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+84
to
+88
| If it holds real values you want to keep, encrypt it in place: | ||
| sops --encrypt --in-place ${SECRETS_FILE} | ||
|
|
||
| If it is junk from a failed run, remove it and re-run: | ||
| rm ${SECRETS_FILE} && make secrets-init" |
Comment on lines
+75
to
+80
| if [[ -f "${SECRETS_FILE}" ]] && grep -q '^sops:' "${SECRETS_FILE}" 2>/dev/null; then | ||
| info "$(basename "${SECRETS_FILE}") already exists and is encrypted — leaving it alone" | ||
|
|
||
| elif [[ -s "${SECRETS_FILE}" ]]; then | ||
| # Non-empty but not SOPS-encrypted. Never delete this: it could be real | ||
| # credentials someone wrote by hand and has not encrypted yet. |
…paths
Review feedback, both correct.
An unreadable file was misreported, and the advice that followed was dangerous.
`[[ -s FILE ]]` is a stat, not a read, so it succeeds on a file the process
cannot open. A chmod 000 secrets file therefore fell through to the
"not SOPS-encrypted" branch, whose remediation tells you to delete it — on a
file whose contents were never inspected and which may be perfectly good
ciphertext.
Readability is now checked before anything tries to read, and the error says the
state is unknown and to fix ownership rather than remove the file. The `grep`
also no longer swallows stderr, so a genuine read error is visible.
Demonstrated against a `sops:`-bearing file at mode 000, run as nobody:
old: -> "not SOPS-encrypted" + advises: rm the file
new: -> "not readable" (state unknown, do not delete)
As root the branch cannot be exercised at all, since root reads anything — which
is exactly why the earlier testing missed it.
The paths printed in those messages are meant to be copied and pasted, so they
are now %q-quoted and `rm` takes `--`. Unquoted, a stack name containing a space
or a glob produced a command that acts on something else:
raw: rm -- /home/u/my stack/secrets/obs*.sops.yaml
-> [rm] [--] [/home/u/my] [stack/secrets/obs*.sops.yaml]
quoted: rm -- /home/u/my\ stack/secrets/obs\*.sops.yaml
-> [rm] [--] [/home/u/my stack/secrets/obs*.sops.yaml]
The three previously covered states are unchanged and re-verified: an encrypted
file is left byte-identical, an empty leftover is recovered, and a non-empty
unencrypted file is refused with its contents intact.
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.
Reported from the monitoring host, one step after #6:
Two failures compounding
The original broken script redirected
sopsstdout into the destination:sops --encrypt secrets/observability.example.yaml > secrets/observability.sops.yamlShell redirection creates the file before the command runs — so when
sopsfailed with "no matching creation rules found", it had already left a 0-bytesecrets/observability.sops.yamlbehind.The fixed script from #6 then tested only
[[ -f "${SECRETS_FILE}" ]], found that empty file, reported "already exists — leaving it alone", and did nothing.make secrets-initappeared to succeed while creating nothing, and the failure surfaced one step later as an unhelpfulsopserror."Exists" is not "set up"
Four states, four answers:
sops:blockThe unreadable case (from review)
[[ -s FILE ]]is a stat, not a read, so it succeeds on a file the process cannot open. Achmod 000secrets file therefore fell through to the "not SOPS-encrypted" branch — whose remediation tells you to delete it, on a file whose contents were never inspected and which may be perfectly good ciphertext.Readability is now checked before anything tries to read. Demonstrated against a real
sops:-bearing file at mode 000, run asnobody:As root the branch cannot be exercised at all, since root reads anything — which is exactly why the earlier testing missed it. The
grepalso no longer swallows stderr, so a genuine read error stays visible instead of being silently reclassified.Pasteable paths (from review)
The paths printed in those errors are meant to be copied, so they are
%q-quoted andrmtakes--. Unquoted, a stack name containing a space or a glob produced a command acting on something else:The non-empty unencrypted case
Deliberately never deleted — it could be real credentials written by hand and not yet encrypted. The error offers both paths:
Verification
Tested against a clone rolled back to the broken script, so the reproduction is the real sequence rather than a synthetic one:
size=0→size=5817,sops:block present)nobody→ refused without deleting; old code would have advised removing itOn the host
After this merges:
git pull make secrets-init # clears the empty leftover and creates the real file make secrets-editOr unblock immediately without waiting — the leftover is a 0-byte file, so nothing is lost: