From 17772dfec651170c9f3794628a9180a5480295d9 Mon Sep 17 00:00:00 2001 From: Brian Majewski Date: Wed, 24 Jun 2026 13:37:56 -0700 Subject: [PATCH] fix: validate-before-mutate in recipients add/rm + verify messaging + doc/UX polish (EGB-283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- README.md | 2 +- secrets | 40 ++++++++++++++++++++++++--------- test/recipients.bats | 53 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 2a1ddfa..8f83d91 100644 --- a/README.md +++ b/README.md @@ -494,7 +494,7 @@ you age1yourpublickey… # Remove the recipient by name (or public key) and re-encrypt the store secrets recipients rm alice # => 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. diff --git a/secrets b/secrets index e250d1b..a66d90a 100755 --- a/secrets +++ b/secrets @@ -1854,6 +1854,9 @@ cmd_reencrypt() { resolve_store check_initialized 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 _reencrypt_all "reencrypt: re-encrypt all to current recipients" } @@ -2128,6 +2131,11 @@ _recipients_add() { if [ -L "$RECIPIENTS_FILE" ]; then die "Refusing to write symlinked $RECIPIENTS_FILE_NAME." 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 # stays a recipient (and can decrypt to re-encrypt). if [ ! -e "$RECIPIENTS_FILE" ]; then @@ -2172,6 +2180,9 @@ _recipients_rm() { [ -n "$target" ] || die "Usage: secrets recipients rm [--yes]" [ -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." + # 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 while IFS=$'\t' read -r k n; do total=$((total + 1)) @@ -2319,9 +2330,12 @@ _verify_all() { rel="${f#"$SECRETS_DIR"/}" echo "FAIL: $rel does not decrypt with the current key." >&2 failed=$((failed + 1)) - fi - if ! _check_blob_recipient_count "$f" "${f#"$SECRETS_DIR"/}" "$rexpected"; then - failed=$((failed + 1)) + 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 + failed=$((failed + 1)) + fi fi done < <(find "$dir" -type f -name '*.age') done @@ -2330,7 +2344,7 @@ _verify_all() { return 0 fi 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 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)." @@ -2389,9 +2403,12 @@ _verify_project() { if ! _verify_blob_decrypts "$blob"; then echo "FINDING: blob for '$rel' ($project/$rel.age) does not decrypt with the current key." >&2 findings=$((findings + 1)) - fi - if ! _check_blob_recipient_count "$blob" "$project/$rel.age" "$rexpected"; then - findings=$((findings + 1)) + 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 + findings=$((findings + 1)) + fi fi done < <(jq -r '.dotenv // [] | .[]' "$manifest") @@ -2418,9 +2435,12 @@ _verify_project() { if ! _verify_blob_decrypts "$eblob"; then echo "FINDING: external blob for '$epath' ($project/$erel) does not decrypt with the current key." >&2 findings=$((findings + 1)) - fi - if ! _check_blob_recipient_count "$eblob" "$project/$erel" "$rexpected"; then - findings=$((findings + 1)) + 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 + findings=$((findings + 1)) + fi fi done < <(_json_external_entries "$manifest") diff --git a/test/recipients.bats b/test/recipients.bats index 4c11199..1ed71af 100644 --- a/test/recipients.bats +++ b/test/recipients.bats @@ -341,6 +341,59 @@ make_second_identity() { [[ "$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" { init_with_remote make_second_identity