Security Review

---
name: security-review
description: Pre-merge security review of a diff against this repo's threat model, with a checklist and verdict format. Use when reviewing or finishing a change that touches secrets or SOPS tooling, Terraform or cloud-init, network exposure (listeners, firewall, rate limits), per-device client material, Ansible tasks that handle secrets, or vpnd. Not for style or performance-only changes.
---

# Security review (vpn-deploy)

The adversary controls the network path, inspects traffic metadata, and actively probes endpoints. Nodes are disposable; the repository plus SOPS secrets is the recovery key. Exposure through Git, state/plan dumps or logs can outlive the session and requires credential recovery. The hard rules in `AGENTS.md` define what is forbidden; this skill is how to check a diff against them.

Review everything being handed off: the committed range (`git diff <base>...HEAD`), staged and unstaged edits (`git diff HEAD`), and untracked files (`git status --short`). `...HEAD` alone misses uncommitted work, so do not issue a verdict until all three are covered. Run the gates below that apply, and report a verdict: **APPROVE**, **REQUEST CHANGES** (fixable finding), or **REJECT** (the change's direction violates a hard rule, e.g. a CDN baseline, a public admin panel, or a cross-layer shortcut). Give file:line evidence for each finding.

## Read the complete change in bounded pieces

First inventory each source of changes with `--stat` and `--name-status`:

```bash
set -euo pipefail
# Replace the ref; stop if it cannot resolve to a commit.
review_base=$(git rev-parse --verify 'reviewed-base-ref^{commit}') || exit 1
nul_inventory() {
  python3 -c 'import json, os, sys
items = sys.stdin.buffer.read().split(b"\0")
print(json.dumps([os.fsdecode(item) for item in items if item], ensure_ascii=True, indent=2))'
}
git -c core.quotePath=true diff --stat "$review_base"...HEAD --
git diff --name-status -z "$review_base"...HEAD -- | nul_inventory
git -c core.quotePath=true diff --stat --cached --
git diff --name-status -z --cached -- | nul_inventory
git -c core.quotePath=true diff --stat --
git diff --name-status -z -- | nul_inventory
git ls-files --others --exclude-standard -z | nul_inventory
```

The consumer splits only on NUL and displays JSON-escaped fields in their
original order; status, old path and new path remain present for renames.
Never display the raw NUL stream: filenames can contain terminal controls.
JSON escapes are display notation, not literal shell pathspecs; recover the
exact pathname with a NUL-aware consumer before selecting it. Review every
handed-off path in each inventory, including both sides of renames. For one
selected path, read ordinary-context diffs separately in the same Bash session:

```bash
review_path='path/from/the/inventory'
: "${review_base:?Resolve the reviewed base before reading selected diffs}"
git --literal-pathspecs diff "$review_base"...HEAD -- "$review_path"
git --literal-pathspecs diff --cached -- "$review_path"
git --literal-pathspecs diff -- "$review_path"
```

Read permitted untracked source files directly; keep the root secret-read
prohibitions in force. Follow changed calls to the precise shared functions,
schemas and consumers needed to assess their behavior. Use smaller path or hunk
reads when output is truncated, until every changed hunk is reviewed. A compact
read is a retrieval boundary, not permission to omit files, checks or findings.

## Checklist

- **Secrets in Git and sensitive local artifacts.** No real credentials or deployment-specific identifiers (keys, tokens, live device UUIDs or shortIds, node addresses or hostnames) in tracked files. Providers may persist generated credentials in state and saved plans: require private files, encrypted state backups and no plaintext state/plan dump into transcripts. Operator credentials and VPN secrets must not be injected into Terraform configuration. Clearly fake fixture values are expected: `tests/fixtures/secrets-sample.yml` uses placeholder UUIDs and shortIds, RFC 5737 addresses, and `example.com` hosts that the strict schema tests require. Only the example, schema, README, and `.sops.yaml.example` under `secrets/` are tracked. `make validate` runs gitleaks over history and the staged tree only, so review unstaged and untracked files yourself.
- **Secret schema.** New secret keys appear in `secrets/prod.secrets.example.yaml` and `secrets/schema.json`; `scripts/check-secrets-coverage.py` and `scripts/validate-secrets.py` pass (both run in `make ci-fast`).
- **Terraform and cloud-init.** Nothing secret in `user_data`, variables, or outputs; outputs stay minimal. `make tf-policy-verify` runs the Conftest policies in `terraform/policy/` (including `no_secrets_in_user_data.rego`, `admin_port.rego`, `ssh_cidrs.rego`).
- **Ansible.** Tasks that template, register, or print secret values set `no_log: true`. Decrypted secrets exist on disk only as the operator-local `SECRETS_FILE` (mode 0600, default under `${XDG_RUNTIME_DIR:-${TMPDIR:-/tmp}}/vpn-provision-<uid>/`, see `docs/reference/secrets.md`); nothing copies it elsewhere.
- **Layer boundaries.** Terraform does not run Ansible or SSH (`local-exec`/`remote-exec`), cloud-init does not carry service config or secrets, and `vpnd` shells out to Make targets instead of reimplementing them.
- **Per-device material.** UUIDs, REALITY shortIds, AmneziaWG peer keys, and subscription tokens are generated per device (`scripts/new-client.sh`, `make issue-sub-token`), never copied from another device; revocation stays per device.
- **Exposure.** A new public listener is added to the Terraform `public_listeners` contract and the runtime role together (see `terraform/CLAUDE.md` and `ansible/roles/firewall/CLAUDE.md`). Admin, metrics, and status endpoints bind to loopback or the tailnet only. Rate limiting is single-layer per surface: browser-facing HTTP routes (`subscription-host`, the nginx-xhttp evaluation path) use nginx `limit_req`; `policy-ratelimit` (nftables) only throttles blackhole-routed client abuse and is not probe defence (`ansible/roles/policy-ratelimit/CLAUDE.md`).
- **Versions.** New binaries and packages are pinned and checksum-verified. The Xray pin is the SOPS secret `xray.version`; `ansible/roles/xray/defaults/main.yml` holds only an empty sentinel. Bumps update `docs/reference/xray-release-line.md` and pass `scripts/check-xray-breaking-changes.py`. Pre-releases go to staging only.
- **Bypass markers.** No `--no-verify`, `gitleaks:allow`, unjustified `# shellcheck disable=`, or skipped pre-commit hooks in the diff or its instructions.
- **Transport baseline.** No CDN-fronted path promoted to P0/P1 (`docs/decisions/0001-cdn-not-filtered-path-baseline.md`).

## Common findings

| Finding | Severity | Fix |
|---|---|---|
| Secret value reaches Ansible output or a debug task | HIGH | `no_log: true`; re-run the play and check the operator-side output |
| Real device UUID, shortId, key, or token copied into a fixture or doc | HIGH | Replace it with a placeholder in the style of `tests/fixtures/secrets-sample.yml` and rotate the real value |
| Listener opened in a provider firewall or nftables template without the contract | HIGH | Add it to `public_listeners` and the role together |
| Pre-release version on a production toggle | HIGH | Move it to a staging profile |
| Second rate-limit layer stacked on an HTTP route | MEDIUM | Keep nginx `limit_req` only for that surface |
| New role without an `enable_*` toggle in `ansible/group_vars/all.yml` | LOW | Add the toggle; enabling a profile is a config change |
| CDN added as a P0/P1 backend | REJECT | See `docs/decisions/0001-cdn-not-filtered-path-baseline.md` |