From 7c08974704934e23ac32e3dd60ca7efbb69150fb Mon Sep 17 00:00:00 2001 From: Smana Date: Tue, 29 Sep 2026 18:38:45 +0200 Subject: [PATCH] fix(secrets): seed never creates a secret before its value exists cmd_seed ran seed_body | store_create for each key. GCP's store_create creates the Secret Manager secret, then adds a version from stdin -- two calls -- and AWS's buffers stdin into a temp file before its one create-secret call. Either way, a failing or empty seed_body still let store_create run: a version-less secret on GCP, or one holding an empty string on AWS. set -o pipefail doesn't catch this -- pipefail reports the pipeline's rightmost failing command, so a failing seed_body piped into a succeeding store_create reports success. Worse, store_has then sees the secret on every later run and skips it, so the entry stays broken forever. Capture seed_body's output into a variable first, check it is non-empty and that seed_body succeeded, and only then call store_create with the value on stdin. On failure, report which key failed (never the value) and continue to the next entry, tallying a failed count -- matching cmd_grant's existing per-key convention -- and make cmd_seed's own exit status reflect it. --- scripts/ci/tests/test-secret-store-seed.sh | 159 +++++++++++++++++++++ scripts/provision/secret-store.sh | 32 ++++- 2 files changed, 188 insertions(+), 3 deletions(-) create mode 100755 scripts/ci/tests/test-secret-store-seed.sh diff --git a/scripts/ci/tests/test-secret-store-seed.sh b/scripts/ci/tests/test-secret-store-seed.sh new file mode 100755 index 000000000..6ae124446 --- /dev/null +++ b/scripts/ci/tests/test-secret-store-seed.sh @@ -0,0 +1,159 @@ +#!/usr/bin/env bash +# shellcheck disable=SC2034 +# (CLOUD, STORE, APPLY, GENERATABLE, REGION and PROJECT are read by the +# bodies of cmd_seed()/store_create()/aws_sm()/gcp_sm(), which are eval'd in +# from the script under test, so static analysis cannot see that use. Same +# reason as test-secret-store-migrate-keys.sh.) +# +# Regression test for `secret-store.sh seed`'s create-before-value bug. +# +# THE BUG: cmd_seed ran `seed_body "$name" | store_create "$name"`. GCP's +# store_create creates the Secret Manager secret, THEN adds a version from +# stdin -- two calls. AWS's buffers stdin into a temp file unconditionally +# before its one create-secret call. Either way, if seed_body failed or +# produced nothing, store_create still ran: a secret with no version on GCP, +# or one holding an empty string on AWS. `set -o pipefail` does not catch +# this -- under pipefail the pipeline's exit status is the RIGHTMOST failing +# command, so a failing seed_body piped into a succeeding store_create +# reports success. Worse, once the secret exists, every later run's +# store_has sees it and skips it -- broken forever. +# +# Exercises the real cmd_seed and store_create as shipped, against stub aws/ +# gcloud binaries. seed_body is stubbed per case so failure/empty/good +# bodies are each exercised without needing a real generator to fail. +set -uo pipefail +HERE="$(cd "$(dirname "$0")" && pwd)" +cd "$HERE/../../.." || exit 1 + +STUB="$(mktemp -d)"; trap 'rm -rf "$STUB"' EXIT +fail=0 +check() { # label expected actual + if [ "$2" = "$3" ]; then printf ' ok %s\n' "$1" + else printf ' FAIL %s: expected %q got %q\n' "$1" "$2" "$3"; fail=1; fi +} + +# aws stub: logs every call (argv, plus the content of any --secret-string +# file:// payload) to $STUB_LOG. describe-secret always reports "not found" +# so store_has treats every test key as absent and cmd_seed proceeds to +# create it. +cat > "$STUB/aws" <<'EOF' +#!/usr/bin/env bash +{ + printf 'CALL:'; printf ' %q' "$@"; printf '\n' + args=("$@") + for ((i = 0; i < $#; i++)); do + if [ "${args[$i]}" = "--secret-string" ]; then + f="${args[$((i + 1))]#file://}" + printf 'BODY:%s\n' "$(cat "$f" 2>/dev/null)" + fi + done +} >> "$STUB_LOG" +for a in "$@"; do + [ "$a" = "describe-secret" ] && { echo "ResourceNotFoundException" >&2; exit 254; } +done +exit 0 +EOF + +# gcloud stub: same call-logging. "describe" always reports "not found". +# "versions add --data-file=-" is the only call that reads stdin in the real +# script, so only that branch consumes it. +cat > "$STUB/gcloud" <<'EOF' +#!/usr/bin/env bash +printf 'CALL:' >> "$STUB_LOG" +printf ' %q' "$@" >> "$STUB_LOG" +printf '\n' >> "$STUB_LOG" +for a in "$@"; do [ "$a" = "auth" ] && exit 0; done +for a in "$@"; do + if [ "$a" = "describe" ]; then + echo "ERROR: NOT_FOUND" >&2 + exit 1 + fi +done +for a in "$@"; do + [ "$a" = "add" ] && { printf 'BODY:%s\n' "$(cat)" >> "$STUB_LOG"; exit 0; } +done +exit 0 +EOF +chmod +x "$STUB/aws" "$STUB/gcloud" +PATH="$STUB:$PATH" +export STUB_LOG="$STUB/calls.log" + +# Lift the real functions out of the script under test, so a change there is +# a change under test. Order matters: cmd_seed calls store_has and +# store_create, which call aws_sm/gcp_sm. +for fn in aws_sm gcp_sm store_has store_create cmd_seed; do + body="$(sed -n "/^${fn}() {/,/^}/p" scripts/provision/secret-store.sh)" + [ -n "$body" ] || { echo "could not extract ${fn}() from scripts/provision/secret-store.sh" >&2; exit 1; } + eval "$body" +done + +# shellcheck source=scripts/lib/gcloud-adc.sh +. "$HERE/../../lib/gcloud-adc.sh" + +REGION="" PROJECT="" + +# run_seed -> logs to $STUB_LOG, returns cmd_seed's rc +run_seed() { + local cloud="$1" + : > "$STUB_LOG" + CLOUD="$cloud" STORE="$cloud" APPLY="true" + GENERATABLE=("test-key") + local rc=0 + cmd_seed >"$STUB/seed-out" 2>&1 || rc=$? + cat "$STUB/seed-out" + return "$rc" +} + +create_call_count() { # + if [ "$1" = "aws" ]; then grep -c 'create-secret' "$STUB_LOG" + else grep -c 'CALL:.*secrets create ' "$STUB_LOG"; fi +} +version_call_count() { # + if [ "$1" = "aws" ]; then grep -c 'create-secret' "$STUB_LOG" # aws: one call does both + else grep -c 'CALL:.*secrets versions add ' "$STUB_LOG"; fi +} + +for cloud in aws gcp; do + # --- a failing seed_body leads to no create ----------------------------- + seed_body() { echo "derive step failed" >&2; return 1; } + out="$(run_seed "$cloud")"; rc=$? + check "$cloud: failing seed_body -> no create call" "0" "$(create_call_count "$cloud")" + check "$cloud: failing seed_body -> cmd_seed reports non-zero" "1" "$rc" + if printf '%s' "$out" | grep -q '\[FAILED \] test-key'; then + printf ' ok %s\n' "$cloud: failing seed_body -> [FAILED] names the key" + else + printf ' FAIL %s: failing seed_body -> [FAILED] line missing:\n%s\n' "$cloud" "$out"; fail=1 + fi + + # --- an empty (but successful) seed_body leads to no create ------------ + seed_body() { printf ''; return 0; } + run_seed "$cloud" >/dev/null; rc=$? + check "$cloud: empty seed_body -> no create call" "0" "$(create_call_count "$cloud")" + check "$cloud: empty seed_body -> cmd_seed reports non-zero" "1" "$rc" + + # --- a good body leads to exactly one create + one version add --------- + seed_body() { printf '{"password":"s3cr3t-do-not-print"}'; return 0; } # pragma: allowlist secret + out="$(run_seed "$cloud")"; rc=$? + check "$cloud: good body -> exactly one create call" "1" "$(create_call_count "$cloud")" + check "$cloud: good body -> exactly one version-add call" "1" "$(version_call_count "$cloud")" + check "$cloud: good body -> cmd_seed reports success" "0" "$rc" + if grep -q 'BODY:{"password":"s3cr3t-do-not-print"}' "$STUB_LOG"; then # pragma: allowlist secret + printf ' ok %s\n' "$cloud: good body -> value reaches the store on stdin" + else + printf ' FAIL %s: good body -> value never reached the store via stdin\n' "$cloud"; fail=1 + fi + if grep -q 's3cr3t-do-not-print' <(grep '^CALL:' "$STUB_LOG"); then + printf ' FAIL %s: value appeared on a CLI argv\n' "$cloud"; fail=1 + else + printf ' ok %s: value never appears on argv\n' "$cloud" + fi + if printf '%s' "$out" | grep -q '\[created\] test-key'; then + printf ' ok %s\n' "$cloud: good body -> [created] reported" + else + printf ' FAIL %s: good body -> [created] line missing\n' "$cloud"; fail=1 + fi +done + +echo +if [ "$fail" -eq 0 ]; then echo "all checks passed"; else echo "FAILURES"; fi +exit "$fail" diff --git a/scripts/provision/secret-store.sh b/scripts/provision/secret-store.sh index 869249274..a2c7b583b 100755 --- a/scripts/provision/secret-store.sh +++ b/scripts/provision/secret-store.sh @@ -787,7 +787,7 @@ store_create() { cmd_seed() { [ -n "$CLOUD" ] || { echo "--cloud is required" >&2; exit 2; } - local created=0 skipped=0 + local created=0 skipped=0 failed=0 for name in "${GENERATABLE[@]}"; do if store_has "$name"; then echo "[skip ] $name -- already exists, left untouched" @@ -799,13 +799,35 @@ cmd_seed() { created=$((created + 1)) continue fi - seed_body "$name" | store_create "$name" + + # Capture the body BEFORE calling store_create, and check it. The old + # `seed_body "$name" | store_create "$name"` let store_create run + # unconditionally: GCP's arm creates the secret, then adds a version + # from stdin -- two calls -- and AWS's arm buffers stdin into a temp + # file before its one create-secret call. Either way, a failing or + # empty seed_body still left a secret behind: version-less on GCP, or + # holding an empty string on AWS. `set -o pipefail` does not catch + # this either -- pipefail reports the pipeline's RIGHTMOST failing + # command, so a failing seed_body piped into a succeeding + # store_create reports success. And once the secret exists, every + # later run's store_has sees it and skips it forever, so the run + # aborting here never even self-heals on retry. + # + # Never on argv: the body stays in a shell variable and reaches + # store_create only on stdin, same as before. + local body + if ! body=$(seed_body "$name") || [ -z "$body" ]; then + echo "[FAILED ] $name -- seed_body produced no value, not created" >&2 + failed=$((failed + 1)) + continue + fi + printf '%s' "$body" | store_create "$name" echo "[created] $name" created=$((created + 1)) done echo - echo "created: ${created}, skipped (already present): ${skipped}" + echo "created: ${created}, skipped (already present): ${skipped}, failed: ${failed}" echo echo "Generated secrets only. Everything else the cluster needs is issued by" echo "another system -- run 'check' to see what is still missing." @@ -813,6 +835,10 @@ cmd_seed() { echo echo "This was a DRY RUN. Re-run with --apply to create them." fi + # Matches cmd_grant's convention: one bad key is reported and skipped, not + # fatal to the rest of the run, but the command's own exit status still + # says so. + [ "$failed" -eq 0 ] } # Grant External Secrets read access to every key the cluster actually asks for.