diff --git a/infra/deploy/render-secrets.sh b/infra/deploy/render-secrets.sh index d9bc7ee..d4a434e 100755 --- a/infra/deploy/render-secrets.sh +++ b/infra/deploy/render-secrets.sh @@ -48,21 +48,26 @@ render_thermograph_secrets() { key_env=("SOPS_AGE_KEY=$keymat") fi local tmp; tmp=$(mktemp) - # The tmp file holds DECRYPTED secrets: a RETURN trap makes its removal - # unconditional (fires on every explicit return, success or failure, and is - # scoped to this function — it doesn't leak into or clobber the caller's traps). - trap 'rm -f "$tmp"' RETURN - : > "$tmp" - # Decrypt failures return 1 explicitly (no partial env, no reliance on the - # caller's shell options — a bare set -e abort would skip the RETURN trap and - # strand plaintext in /tmp). common first, host second, so a host value - # overrides a shared one (last-wins). + # The tmp file holds DECRYPTED secrets, so every exit path below removes it. + # + # Deliberately NOT a `trap ... RETURN`, which looks like the tidier way to make + # that unconditional: a RETURN trap set inside a SOURCED function persists in + # the CALLER's shell after the function returns, and a RETURN trap also fires + # when a `.`/source completes. deploy.sh sources /etc/thermograph.env a few + # lines after calling us, which would re-fire the trap at top level where `tmp` + # (function-local) is unset -- fatal under deploy.sh's `set -u`, and silent, + # because that line already redirects stderr to /dev/null. Explicit removal on + # each path is duller and correct. + : > "$tmp" || { rm -f "$tmp"; return 1; } + # Decrypt failures return 1 explicitly, so a partial env is never written and + # correctness doesn't depend on the caller's shell options. common first, host + # second, so a host value overrides a shared one (last-wins). if [ -f "$repo/deploy/secrets/common.yaml" ]; then env "${key_env[@]}" sops -d --input-type yaml --output-type dotenv \ - "$repo/deploy/secrets/common.yaml" >> "$tmp" || return 1 + "$repo/deploy/secrets/common.yaml" >> "$tmp" || { rm -f "$tmp"; return 1; } fi env "${key_env[@]}" sops -d --input-type yaml --output-type dotenv \ - "$repo/deploy/secrets/${env_name}.yaml" >> "$tmp" || return 1 + "$repo/deploy/secrets/${env_name}.yaml" >> "$tmp" || { rm -f "$tmp"; return 1; } # Write /etc/thermograph.env. Prefer an in-place write when the existing file is # writable by us (e.g. a group-writable 0660 root: on a box whose CI @@ -73,12 +78,19 @@ render_thermograph_secrets() { # next line of deploy.sh (`. /etc/thermograph.env` as that non-root user) can't read # it, so POSTGRES_PASSWORD never enters the env and `docker compose` dies on # interpolation. Fail loudly rather than deploy against stale secrets. + # rc + a single cleanup point: the plaintext tmp must go whichever branch runs, + # but the old trailing `rm` was also the function's last command, so it masked a + # failed in-place `cat` write to status 0. Capture the status instead, so a + # half-written /etc/thermograph.env fails loudly rather than deploying stale. + local rc=0 if [ -f /etc/thermograph.env ] && [ -w /etc/thermograph.env ]; then - cat "$tmp" > /etc/thermograph.env + cat "$tmp" > /etc/thermograph.env || rc=1 elif install -m 0640 "$tmp" /etc/thermograph.env 2>/dev/null; then : elif sudo install -m 0640 -o "$(id -un)" -g "$(id -gn)" "$tmp" /etc/thermograph.env 2>/dev/null; then : else echo "!! cannot write /etc/thermograph.env (need file write access or passwordless sudo)" >&2 - return 1 + rc=1 fi + rm -f "$tmp" + return "$rc" }