Security Review Checklist — scripts/rotate/*.sh
Use this checklist when reviewing any new or modified rotation script.
Apply ALL checks before approving a PR that touches scripts/rotate/.
BLOCK — These must pass. Fail = reject PR.
S1 — No hardcoded secret values
# grep for common patterns — zero matches expected
grep -n 'PASSWORD=.\|TOKEN=.\|KEY=.\|SECRET=.\|URI=mongodb://' "$SCRIPT" | grep -v 'CREDENTIAL_NAME=\|SOPS_FILE=\|GH_SECRET_NAME=\|GH_REPO=\|VERIFY_URL=\|SOPS_TMP=\|PAYLOAD_FILE=\|VERIFY_FILE=\|SOPS_KEY=\|:-\}'Acceptable hardcoded values: paths, server IPs (from CLAUDE.md), credential NAMES (not values),
VERIFY_URL, container image URLs. Nothing that is a secret value.
S2 — SOPS_AGE_RECIPIENT loaded dynamically, never hardcoded
The receiver public key must come from secrets/rotate-secret.env.sops at runtime.
Hardcoding breaks silently when the receiver key is rotated.
# Must NOT appear:
grep "SOPS_AGE_RECIPIENT=\"age1" "$SCRIPT" # 0 matches expected
# Must appear (dynamic load pattern):
grep "rotate-secret.env.sops" "$SCRIPT" # at least 1 match expected
grep "AGE_KEY_SOPS_SYNC_RECEIVER" "$SCRIPT" # at least 1 match expectedReference implementation: scripts/rotate/gf_rendering_renderer_token.sh §load-receiver-key.
S3 — SOPS_TMP path includes CREDENTIAL_NAME (no parallel-run collision)
When two scripts share the same SOPS_TMP path and the cron matrix runs them simultaneously,
the file gets corrupted mid-read. Fix: suffix with ${CREDENTIAL_NAME}.
# Must NOT appear:
grep 'SOPS_TMP="/tmp/rotate-[a-z-]*\.env\.sops"' "$SCRIPT" # collision risk if no suffix
# Must appear:
grep 'SOPS_TMP=.*${CREDENTIAL_NAME}' "$SCRIPT"S4 — Credential values never pass through shell argument lists
Check every SSH call and every printf/echo that carries a secret value.
OK patterns (value never in ps aux):
printf '%s\n' "$NEW_PASS" | ssh ... "python3 - <<'PYEOF' ... PYEOF" # stdin pipe
scp "$JS_FILE" remote:/tmp/file.js; ssh ... "/tmp/file.js" # file, not argNOT OK patterns:
ssh ... "command --password '$NEW_PASS'" # shows in ps aux on remote
echo "$NEW_PASS" | ssh ... "cat > /tmp/f" # echo shows in ps aux locallyException: mongosh -p '$ADMIN_PASS' in the MongoDB changeUserPassword step is an accepted
risk (SSH-encrypted channel, <1s in ps aux). Document it with a comment. Do NOT extend this
exception to other credentials.
S5 — All secret variables cleared after last use
# Pattern check — every secret var must be explicitly cleared before exit
grep -n 'ADMIN_PASS\|ADMIN_USER\|NEW_PASS\|NEW_URI\|DECRYPTED\|PAYLOAD_JSON\|PAYLOAD_B64' "$SCRIPT" | tail -10
# Verify the last line referencing each is a clear: VAR=""S6 — Temp files cleaned on all exit paths
fail() must rm -f every temp file created in the script.
Check that all mktemp calls and hardcoded /tmp/ paths are listed in fail() cleanup.
grep -n 'mktemp\|/tmp/' "$SCRIPT" # list all temp files
grep -n 'rm -f' "$SCRIPT" # verify all are covered in fail() and normal exitS7 — New credential added to CREDENTIAL_ALLOWLIST in sops-sync-receiver.py
No SOPS update can happen for an unlisted credential — the receiver rejects it at validation.
grep "CREDENTIAL_NAME" "$SCRIPT" | head -1 # get the value
grep "${CREDENTIAL_NAME}" scripts/sops-sync-receiver.py # must matchS8 — [ROTATE] log lines contain no secret values
Scan every log call:
grep 'log "' "$SCRIPT"Acceptable: lengths (length=${#NEW_PASS}), booleans, step names, HTTP codes, timestamps.
Not acceptable: the value itself, partial values, base64-encoded values.
WARN — Address or document accepted risk.
W1 — StrictHostKeyChecking=no
StrictHostKeyChecking=no allows MITM on the first connection. Accepted for internal network
(bms-1/2/3/4 known IPs, no untrusted networks between them).
If the script connects to a server that wasn’t in ~/.ssh/known_hosts before, add an explicit
ssh-keyscan step or pre-populate known_hosts.
W2 — No rollback on exit 2 (apply-step failure)
If MongoDB password changes but env update fails, the service is in a degraded state (works until next restart, then breaks). This is the accepted design trade-off.
Verify the script exits with code 2 (not 3 or 0) on apply failure, and that the commit comment documents this state clearly enough for the human reviewer.
W3 — Admin credentials in memory for the full duration of Steps 2–4
DECRYPTED holds all keys from the SOPS file. Minimize the window:
- Extract only
ADMIN_USERandADMIN_PASS - Clear
DECRYPTED=""immediately after extraction (before Step 3) - Clear
ADMIN_USERandADMIN_PASSimmediately after Step 4 (password change)
W4 — gh api reads SOPS file from dev branch
If SOPS was recently updated on a feature branch but not yet merged to dev, the script
reads a stale admin credential. This is acceptable (SOPS-on-dev is the source of truth),
but note it when debugging auth failures on the first post-merge rotation.
NIT — Best practice, not a blocker.
N1 — SOPS_TMP created world-readable briefly before chmod
# Current pattern (file briefly world-readable):
base64 -d > "$SOPS_TMP"
chmod 600 "$SOPS_TMP"
# Better (no readable window):
touch "$SOPS_TMP"; chmod 600 "$SOPS_TMP"
base64 -d > "$SOPS_TMP"Low risk (SOPS file is encrypted). Note but do not block.
N2 — JS temp file may persist on remote PRIMARY if SSH drops after scp
The mongosh JS file is removed by ;rm -f in the same SSH command as execution.
If SSH drops after scp but before the command runs, the JS file with the new password
remains on the PRIMARY server under /tmp/.
Mitigation (future improvement): use ssh PRIMARY 'trap "rm -f /tmp/file.js" EXIT; ...'
to guarantee cleanup. Current risk is low (file in /tmp with 600 perms on trusted server).
N3 — age-keygen -y writes to stdout — confirm it never logs to [ROTATE]
SOPS_AGE_RECIPIENT=$(age-keygen -y ...) output is captured to variable, not logged.
Verify with: grep "SOPS_AGE_RECIPIENT" "$SCRIPT" — the var should not appear in any log call.
GH_PAT scope requirements
The GH_PAT used by sops-sync-receiver.yml and by rotation scripts on bms-4 needs:
| Scope | Why |
|---|---|
repo (full) | Push commits to dev branch; checkout repo in GHA |
workflow | Trigger gh workflow run sops-sync-receiver.yml from bms-4 |
write:packages | Not required |
admin:repo_hook | Not required |
For setting GH Secrets in sops-sync-receiver.py (gh secret set):
- Classic PAT with
reposcope has implicit secrets write access. - Fine-grained PAT needs explicit
Secrets: Read and writepermission.
To verify current PAT scopes:
gh api user --jq '.login' # confirms PAT is valid
gh auth status # shows token scopesReview comment template
## Security Review — scripts/rotate/<service>.sh
### BLOCK findings
| # | Check | File | Line | Finding |
|---|---|---|---|---|
| S1 | No hardcoded secrets | … | … | … |
### WARN findings
| # | Check | Finding | Accepted risk? |
|---|---|---|---|
| W1 | StrictHostKeyChecking | … | yes / no |
### NIT findings
- N1: …
### Verdict: APPROVE / REQUEST CHANGESReferences
scripts/rotate/README.md— script contract (exit codes, log format,--dry-run)docs/plans/plan-2427-rotate-secret-system.md— architecturedocs/playbooks/secret-manager.md— SOPS file map and distribution chain- Phase 2 reference implementation:
scripts/rotate/gf_rendering_renderer_token.sh