fix: validate-before-mutate in recipients add/rm + verify messaging + doc/UX polish (EGB-283)
- Fix 1 (IMPORTANT): _recipients_rm and _recipients_add now call _load_recipients BEFORE any mutation. A hand-corrupted recipients.txt dies at validation, leaving the file untouched — prevents inconsistent state where the file is changed but blobs are not re-encrypted. On a legacy store (no recipients.txt), _load_recipients succeeds via the derived-pubkey path so the bootstrap path still works. - Fix 2 (MINOR): Guard _check_blob_recipient_count behind a successful decrypt in both _verify_all and _verify_project — an undecryptable blob no longer produces a spurious "encrypted to 0 recipients" finding. Reword _verify_all summary to "failed (decrypt or recipient-count)" since both failure modes now increment the counter. - Fix 3 (MINOR): Correct README offboarding comment from "New blobs are no longer readable" (contradicts the re-encrypt of EVERY blob) to "Existing blobs are re-encrypted; the removed key can no longer decrypt them." - Fix 4 (MINOR): cmd_reencrypt prints an advisory when no recipients.txt exists (single-key store), so the operator knows they can add teammates. - Fix 5 (MINOR): Test coverage for ambiguous-name rm refusing to remove when multiple recipients share a --name label. Tests: 5 new tests in test/recipients.bats (34 total, all pass). Full suite 276 tests: 5 known pre-existing failures (3 mode-600/stat, 2 jq-PATH), none new. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
f7576a3eae
commit
17772dfec6
3 changed files with 84 additions and 11 deletions
|
|
@ -494,7 +494,7 @@ you age1yourpublickey…
|
||||||
# Remove the recipient by name (or public key) and re-encrypt the store
|
# Remove the recipient by name (or public key) and re-encrypt the store
|
||||||
secrets recipients rm alice
|
secrets recipients rm alice
|
||||||
# => Removes alice from recipients.txt, re-encrypts every blob, pushes.
|
# => Removes alice from recipients.txt, re-encrypts every blob, pushes.
|
||||||
# New blobs are no longer readable by alice's key.
|
# Existing blobs are re-encrypted; the removed key can no longer decrypt them.
|
||||||
```
|
```
|
||||||
|
|
||||||
> **Important:** git history can't be un-shared. If alice had access during a period when genuinely sensitive values were stored, rotate those values now (update them in the external system and run `secrets push`). The re-encrypt prevents future access; history is permanent.
|
> **Important:** git history can't be un-shared. If alice had access during a period when genuinely sensitive values were stored, rotate those values now (update them in the external system and run `secrets push`). The re-encrypt prevents future access; history is permanent.
|
||||||
|
|
|
||||||
28
secrets
28
secrets
|
|
@ -1854,6 +1854,9 @@ cmd_reencrypt() {
|
||||||
resolve_store
|
resolve_store
|
||||||
check_initialized
|
check_initialized
|
||||||
check_key
|
check_key
|
||||||
|
if [ ! -e "$RECIPIENTS_FILE" ]; then
|
||||||
|
info "No $RECIPIENTS_FILE_NAME — single-key store; re-encrypting to your own key only. Add teammates with 'secrets recipients add'."
|
||||||
|
fi
|
||||||
_load_recipients
|
_load_recipients
|
||||||
_reencrypt_all "reencrypt: re-encrypt all to current recipients"
|
_reencrypt_all "reencrypt: re-encrypt all to current recipients"
|
||||||
}
|
}
|
||||||
|
|
@ -2128,6 +2131,11 @@ _recipients_add() {
|
||||||
if [ -L "$RECIPIENTS_FILE" ]; then
|
if [ -L "$RECIPIENTS_FILE" ]; then
|
||||||
die "Refusing to write symlinked $RECIPIENTS_FILE_NAME."
|
die "Refusing to write symlinked $RECIPIENTS_FILE_NAME."
|
||||||
fi
|
fi
|
||||||
|
# Validate BEFORE any mutation: a hand-corrupted file dies here, leaving
|
||||||
|
# recipients.txt untouched (no half-mutate / blobs-not-reencrypted skew).
|
||||||
|
# On a legacy store (no recipients.txt) _load_recipients succeeds via the
|
||||||
|
# derived-pubkey path and does NOT die — so the bootstrap path still works.
|
||||||
|
_load_recipients
|
||||||
# Bootstrap a legacy store: seed this machine's key first so the operator
|
# Bootstrap a legacy store: seed this machine's key first so the operator
|
||||||
# stays a recipient (and can decrypt to re-encrypt).
|
# stays a recipient (and can decrypt to re-encrypt).
|
||||||
if [ ! -e "$RECIPIENTS_FILE" ]; then
|
if [ ! -e "$RECIPIENTS_FILE" ]; then
|
||||||
|
|
@ -2172,6 +2180,9 @@ _recipients_rm() {
|
||||||
[ -n "$target" ] || die "Usage: secrets recipients rm <age1...|name> [--yes]"
|
[ -n "$target" ] || die "Usage: secrets recipients rm <age1...|name> [--yes]"
|
||||||
[ -e "$RECIPIENTS_FILE" ] || die "No $RECIPIENTS_FILE_NAME — store is single-key; nothing to remove."
|
[ -e "$RECIPIENTS_FILE" ] || die "No $RECIPIENTS_FILE_NAME — store is single-key; nothing to remove."
|
||||||
[ -L "$RECIPIENTS_FILE" ] && die "Refusing to write symlinked $RECIPIENTS_FILE_NAME."
|
[ -L "$RECIPIENTS_FILE" ] && die "Refusing to write symlinked $RECIPIENTS_FILE_NAME."
|
||||||
|
# Validate BEFORE any mutation: a hand-corrupted file dies here, leaving
|
||||||
|
# recipients.txt untouched (no half-mutate / blobs-not-reencrypted skew).
|
||||||
|
_load_recipients
|
||||||
local k n match="" count=0 total=0
|
local k n match="" count=0 total=0
|
||||||
while IFS=$'\t' read -r k n; do
|
while IFS=$'\t' read -r k n; do
|
||||||
total=$((total + 1))
|
total=$((total + 1))
|
||||||
|
|
@ -2319,10 +2330,13 @@ _verify_all() {
|
||||||
rel="${f#"$SECRETS_DIR"/}"
|
rel="${f#"$SECRETS_DIR"/}"
|
||||||
echo "FAIL: $rel does not decrypt with the current key." >&2
|
echo "FAIL: $rel does not decrypt with the current key." >&2
|
||||||
failed=$((failed + 1))
|
failed=$((failed + 1))
|
||||||
fi
|
else
|
||||||
|
# Only check recipient count when the blob actually decrypted — an
|
||||||
|
# undecryptable blob would report "0 recipients" which is spurious.
|
||||||
if ! _check_blob_recipient_count "$f" "${f#"$SECRETS_DIR"/}" "$rexpected"; then
|
if ! _check_blob_recipient_count "$f" "${f#"$SECRETS_DIR"/}" "$rexpected"; then
|
||||||
failed=$((failed + 1))
|
failed=$((failed + 1))
|
||||||
fi
|
fi
|
||||||
|
fi
|
||||||
done < <(find "$dir" -type f -name '*.age')
|
done < <(find "$dir" -type f -name '*.age')
|
||||||
done
|
done
|
||||||
if [ "$checked" -eq 0 ]; then
|
if [ "$checked" -eq 0 ]; then
|
||||||
|
|
@ -2330,7 +2344,7 @@ _verify_all() {
|
||||||
return 0
|
return 0
|
||||||
fi
|
fi
|
||||||
if [ "$failed" -gt 0 ]; then
|
if [ "$failed" -gt 0 ]; then
|
||||||
echo "verify --all: $failed of $checked blob(s) failed to decrypt." >&2
|
echo "verify --all: $failed of $checked blob(s) failed (decrypt or recipient-count)." >&2
|
||||||
return 1
|
return 1
|
||||||
fi
|
fi
|
||||||
info "verify --all: OK — all $checked blob(s) decrypt with the current key (integrity only; run 'secrets verify' in a project for manifest consistency)."
|
info "verify --all: OK — all $checked blob(s) decrypt with the current key (integrity only; run 'secrets verify' in a project for manifest consistency)."
|
||||||
|
|
@ -2389,10 +2403,13 @@ _verify_project() {
|
||||||
if ! _verify_blob_decrypts "$blob"; then
|
if ! _verify_blob_decrypts "$blob"; then
|
||||||
echo "FINDING: blob for '$rel' ($project/$rel.age) does not decrypt with the current key." >&2
|
echo "FINDING: blob for '$rel' ($project/$rel.age) does not decrypt with the current key." >&2
|
||||||
findings=$((findings + 1))
|
findings=$((findings + 1))
|
||||||
fi
|
else
|
||||||
|
# Only check recipient count when the blob actually decrypted — an
|
||||||
|
# undecryptable blob would report "0 recipients" which is spurious.
|
||||||
if ! _check_blob_recipient_count "$blob" "$project/$rel.age" "$rexpected"; then
|
if ! _check_blob_recipient_count "$blob" "$project/$rel.age" "$rexpected"; then
|
||||||
findings=$((findings + 1))
|
findings=$((findings + 1))
|
||||||
fi
|
fi
|
||||||
|
fi
|
||||||
done < <(jq -r '.dotenv // [] | .[]' "$manifest")
|
done < <(jq -r '.dotenv // [] | .[]' "$manifest")
|
||||||
|
|
||||||
# ── external entries: missing-blob + decrypt ──
|
# ── external entries: missing-blob + decrypt ──
|
||||||
|
|
@ -2418,10 +2435,13 @@ _verify_project() {
|
||||||
if ! _verify_blob_decrypts "$eblob"; then
|
if ! _verify_blob_decrypts "$eblob"; then
|
||||||
echo "FINDING: external blob for '$epath' ($project/$erel) does not decrypt with the current key." >&2
|
echo "FINDING: external blob for '$epath' ($project/$erel) does not decrypt with the current key." >&2
|
||||||
findings=$((findings + 1))
|
findings=$((findings + 1))
|
||||||
fi
|
else
|
||||||
|
# Only check recipient count when the blob actually decrypted — an
|
||||||
|
# undecryptable blob would report "0 recipients" which is spurious.
|
||||||
if ! _check_blob_recipient_count "$eblob" "$project/$erel" "$rexpected"; then
|
if ! _check_blob_recipient_count "$eblob" "$project/$erel" "$rexpected"; then
|
||||||
findings=$((findings + 1))
|
findings=$((findings + 1))
|
||||||
fi
|
fi
|
||||||
|
fi
|
||||||
done < <(_json_external_entries "$manifest")
|
done < <(_json_external_entries "$manifest")
|
||||||
|
|
||||||
# ── orphan detection: any stored blob the manifest doesn't account for ──
|
# ── orphan detection: any stored blob the manifest doesn't account for ──
|
||||||
|
|
|
||||||
|
|
@ -341,6 +341,59 @@ make_second_identity() {
|
||||||
[[ "$output" == *"valid age recipient"* ]] || false
|
[[ "$output" == *"valid age recipient"* ]] || false
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# ── Fix 1: validate-before-mutate ────────────────────────────────────────────
|
||||||
|
|
||||||
|
@test "recipients rm dies without mutating a hand-corrupted recipients.txt" {
|
||||||
|
init_with_remote
|
||||||
|
make_second_identity
|
||||||
|
run "$SECRETS_BIN" recipients add "$BOB_PUB" --name bob # valid: self + bob
|
||||||
|
# Corrupt the file by hand.
|
||||||
|
printf 'age1-not-a-valid-key\n' >> "$SECRETS_DIR/recipients.txt"
|
||||||
|
before=$(cat "$SECRETS_DIR/recipients.txt")
|
||||||
|
run "$SECRETS_BIN" recipients rm bob
|
||||||
|
[ "$status" -ne 0 ]
|
||||||
|
[[ "$output" == *"Invalid recipient"* ]] || false
|
||||||
|
# File unchanged (no half-mutation).
|
||||||
|
[ "$(cat "$SECRETS_DIR/recipients.txt")" = "$before" ]
|
||||||
|
}
|
||||||
|
|
||||||
|
@test "recipients add dies without mutating a hand-corrupted recipients.txt" {
|
||||||
|
init_with_remote
|
||||||
|
make_second_identity
|
||||||
|
printf 'age1-not-a-valid-key\n' >> "$SECRETS_DIR/recipients.txt" # init seeded self; now corrupt
|
||||||
|
before=$(cat "$SECRETS_DIR/recipients.txt")
|
||||||
|
run "$SECRETS_BIN" recipients add "$BOB_PUB" --name bob
|
||||||
|
[ "$status" -ne 0 ]
|
||||||
|
[[ "$output" == *"Invalid recipient"* ]] || false
|
||||||
|
[ "$(cat "$SECRETS_DIR/recipients.txt")" = "$before" ]
|
||||||
|
}
|
||||||
|
|
||||||
|
# ── Fix 4: reencrypt advisory on a legacy store ───────────────────────────────
|
||||||
|
|
||||||
|
@test "reencrypt on a legacy store prints a single-key advisory" {
|
||||||
|
init_with_remote
|
||||||
|
rm -f "$SECRETS_DIR/recipients.txt"
|
||||||
|
create_project_dir myproj
|
||||||
|
run "$SECRETS_BIN" push
|
||||||
|
run "$SECRETS_BIN" reencrypt
|
||||||
|
[ "$status" -eq 0 ]
|
||||||
|
[[ "$output" == *"single-key"* ]] || false
|
||||||
|
}
|
||||||
|
|
||||||
|
# ── Fix 5: ambiguous-name rm coverage ────────────────────────────────────────
|
||||||
|
|
||||||
|
@test "recipients rm by an ambiguous name is refused" {
|
||||||
|
init_with_remote
|
||||||
|
make_second_identity
|
||||||
|
age-keygen -o "$TEST_TMPDIR/carol.txt" 2>/dev/null
|
||||||
|
CAROL_PUB=$(age-keygen -y "$TEST_TMPDIR/carol.txt")
|
||||||
|
run "$SECRETS_BIN" recipients add "$BOB_PUB" --name dup
|
||||||
|
run "$SECRETS_BIN" recipients add "$CAROL_PUB" --name dup
|
||||||
|
run "$SECRETS_BIN" recipients rm dup
|
||||||
|
[ "$status" -ne 0 ]
|
||||||
|
[[ "$output" == *"matches"* ]] || false
|
||||||
|
}
|
||||||
|
|
||||||
@test "SECURITY: a symlinked recipients.txt is refused on add and rm too" {
|
@test "SECURITY: a symlinked recipients.txt is refused on add and rm too" {
|
||||||
init_with_remote
|
init_with_remote
|
||||||
make_second_identity
|
make_second_identity
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue