From c1538c4b7e33a93ee9e7881afecaea9b5030edfa Mon Sep 17 00:00:00 2001 From: Garrett Allen <98648590+Gerrrt@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:12:09 +0000 Subject: [PATCH] fix(validate): run the five checks that were CI-only, from one copy each (#175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validate.sh` claims "Everything CI runs, runnable locally" and five checks lived as inline shell in ci.yml with no local equivalent. The SOPS assert is the one that mattered: a file that is not encrypted is a committed plaintext secret, and the only thing that looked was a job you cannot consult before pushing — after which it has to be purged from history rather than reverted. Three image greps fold into check_image_pins.py, which already runs in both places and whose docstring already explains why a grep cannot see a pin that is simply absent (#65). They are now beside the parser that argument is about. Scope is `git ls-files`, not a walk of `.`. The shell versions walked the tree, which is right in CI's clean checkout and wrong here: this repository keeps git worktrees under .claude/worktrees/, so a local run would have descended into full copies of itself and reported another branch's findings as this one's. Tracked is also the right question — an untracked scratch file pinning :latest harms nobody. The floating-tag pattern keeps the original's blind spot exactly: `FOO=alpine:latest` is missed there too, because both require start-of-line or whitespace before the name. Parity, not a regression, and still a real gap. The digest check reads the parsed compose file rather than an awk over `image:` lines, so a quoted or flow-style value is not a hole, and it fails rather than passes when it finds no compose files at all. The dashboard PromQL parse uses the promtool binary-or-docker fallback the file already has, with a counted skip when neither is reachable. Its temp file is written inside REPO_ROOT, because the docker fallback bind-mounts REPO_ROOT and nothing else, and chmod 0644, because mktemp makes it 0600 and the reader is then the image's own user — which fails as "permission denied" and looks exactly like a broken query. The tracked-artefact check was written out twice and the copies had drifted. validate.sh's missed a second stack's .env, a nested .rendered/ and a certificates/ anywhere but the repository root — all three now caught, and .env.example still allowed. One script, both callers, second copy deleted. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 72 ++++------------------- .gitignore | 4 ++ scripts/check-sops-encrypted.sh | 70 ++++++++++++++++++++++ scripts/check-tracked-artefacts.sh | 75 ++++++++++++++++++++++++ scripts/check_image_pins.py | 94 ++++++++++++++++++++++++++++++ scripts/validate.sh | 73 +++++++++++++++++------ 6 files changed, 311 insertions(+), 77 deletions(-) create mode 100755 scripts/check-sops-encrypted.sh create mode 100755 scripts/check-tracked-artefacts.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fde80aa..c35ad8e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,47 +103,11 @@ jobs: # zricethezav/gitleaks sat pinned in this file's own env: block. Any # repo/name:version fails now. The illustrative version in the header of # scripts/pin-digests.sh is a comment and is filtered out below. - - name: Verify image versions are not duplicated outside compose.yaml - run: | - if grep -rnE '(^|[^a-zA-Z0-9._/-])[a-z0-9][a-z0-9._-]*/[a-z0-9][a-z0-9._-]*:v?[0-9]+\.[0-9]+' \ - --include='*.sh' --include='*.yml' --include='Makefile' \ - scripts .github Makefile 2>/dev/null \ - | grep -vE '^[^:]+:[0-9]+:[[:space:]]*#' ; then - echo "::error::pinned image version outside compose.yaml — use scripts/image-for.sh" - exit 1 - fi - echo "no duplicated image pins" - - # Covers every place an image is referenced, not just compose.yaml — the - # first version of this check only looked at the stack and let :latest - # through in the workflow itself and in the Makefile. - - name: Verify no image uses a floating tag - run: | - if grep -rnE '(^|[[:space:]])[a-z0-9._/-]+:latest([[:space:]]|$)' \ - --include='*.yaml' --include='*.yml' --include='Makefile' \ - --include='*.sh' . \ - | grep -v '^\./\.git/' \ - | grep -vE '^[^:]+:[0-9]+:[[:space:]]*#' ; then - echo "::error::floating :latest tag found — pin an explicit version" - exit 1 - fi - echo "all image references pinned" - - # A tag is a mutable pointer; a digest is the content hash. Every service - # image must carry both, so a moved tag cannot change what gets deployed. - - name: Verify every image is pinned by digest - run: | - missing=0 - while read -r ref; do - case "$ref" in - *@sha256:*) ;; - *) echo "::error::$ref is not pinned by digest — run make pin-digests"; missing=1 ;; - esac - done < <(./scripts/stacks.sh --paths | while read -r sd; do - awk '$1 == "image:" { print $2 }' "$sd/compose.yaml" - done) - exit "$missing" - + # The three pattern checks that used to live here — version pins outside + # compose.yaml, floating :latest, and every image digest-pinned — are now + # in scripts/check_image_pins.py, which runs in both places (#175). They + # were unrunnable locally, and they belong beside the parser whose + # docstring explains why a grep cannot see a pin that is simply absent. # The three checks above are all pattern matches, and #65 walked past all # three: `make backup` ran a bare `alpine` — no `:latest` literal, no # prom/ or grafana/ prefix, and not an `image:` line in compose.yaml. A @@ -432,25 +396,13 @@ jobs: # the filesystem scan skips it, and its contents are bare literals with no # keyword context for a rule to match. The control is simply that it must # never be a tracked file. + # Both of these were inline here and unrunnable locally, which is the + # wrong place for a check whose whole value is catching a plaintext + # secret BEFORE it reaches the remote (#175). The tracked-artefact one + # was also written out a second time in validate.sh, and the two copies + # had drifted. One script each, run from both. - name: Assert no decrypted artefact is tracked - run: | - fail=0 - for pattern in '.env' '.rendered/' '.purge-secrets.txt' 'certificates/' 'backups/'; do - if git ls-files | grep -E "(^|/)${pattern//./\\.}" | grep -v '\.env\.example'; then - echo "::error::tracked file matching '${pattern}' — it must be gitignored" - fail=1 - fi - done - exit "$fail" + run: ./scripts/check-tracked-artefacts.sh - name: Assert SOPS files are actually encrypted - run: | - shopt -s nullglob - fail=0 - for f in secrets/*.sops.yaml; do - if ! grep -q '^sops:' "$f"; then - echo "::error file=$f::not SOPS-encrypted" - fail=1 - fi - done - exit "$fail" + run: ./scripts/check-sops-encrypted.sh diff --git a/.gitignore b/.gitignore index 18c2413..357c315 100644 --- a/.gitignore +++ b/.gitignore @@ -77,3 +77,7 @@ software/ *.iso *.exe docs/hardware/*.pdf + +# validate.sh emits dashboard panel queries here for promtool; the trap removes +# it, but an interrupted run should not leave a puzzling untracked file behind. +.dashboard-exprs-*.yaml diff --git a/scripts/check-sops-encrypted.sh b/scripts/check-sops-encrypted.sh new file mode 100755 index 0000000..79fef93 --- /dev/null +++ b/scripts/check-sops-encrypted.sh @@ -0,0 +1,70 @@ +#!/usr/bin/env bash +# +# Assert every committed SOPS file is actually encrypted. +# +# A `secrets/.sops.yaml` that is not encrypted is a committed plaintext +# secret, and nothing else in this repository would notice: it is tracked on +# purpose, so the tracked-artefact check passes it; it is valid YAML, so every +# parser reads it happily; and `make render` would decrypt-and-copy it without +# complaint because sops treats an unencrypted file as nothing to do. +# +# The failure needs no mistake in this repository to happen — `sops --decrypt` +# writing over the source, an editor saving the decrypted buffer to the original +# path, or a merge resolved in favour of a decrypted side all produce it, and +# all of them look like an ordinary file change in a diff nobody reads closely. +# +# Until #175 the only thing that checked was a CI job, which is the one place +# you cannot consult before pushing. A plaintext secret that reaches the remote +# has to be purged from history rather than reverted, so the check is worth +# far more before the push than after it — see docs/runbooks/purge-git-history.md. +# +# The test is the `sops:` metadata block, which sops appends to every file it +# encrypts and which cannot survive the file being written back in the clear. +# Not "does it contain something that looks like ciphertext": a partially +# decrypted file is still a leak, and the block is absent for that too. +# +# Usage: +# scripts/check-sops-encrypted.sh one line per file, exit 1 on any failure +# +# No arguments and no options. Both callers — scripts/validate.sh and the +# workflow — want exactly this, and a --quiet nobody asked for is a flag that +# can be passed by accident in the one place the output matters. + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "${REPO_ROOT}" + +shopt -s nullglob +FILES=(secrets/*.sops.yaml) + +# Zero files is a failure, not a pass. This check exists because the encrypted +# secrets are the one artefact whose absence is silent — a glob that stops +# matching (a rename, a move to secrets//) would otherwise report +# "all encrypted" over nothing at all, which is the shape of #68's empty-input +# bug and of the --emit-promql guard in ci.yml. +if ((${#FILES[@]} == 0)); then + printf 'no secrets/*.sops.yaml found — this check has stopped checking anything\n' >&2 + exit 1 +fi + +plaintext=() +for f in "${FILES[@]}"; do + if grep -q '^sops:' "$f"; then + printf ' encrypted %s\n' "$f" + else + printf ' PLAINTEXT %s\n' "$f" >&2 + plaintext+=("$f") + fi +done + +if ((${#plaintext[@]})); then + printf '\nnot SOPS-encrypted: %s\n' "${plaintext[*]}" >&2 + printf 'Do NOT commit these.\n' >&2 + printf 'Re-encrypt with: sops --encrypt --in-place \n' >&2 + printf 'If it has already been pushed, it must be purged from history rather\n' >&2 + printf 'than reverted — see docs/runbooks/purge-git-history.md\n' >&2 + exit 1 +fi + +printf '%d SOPS file(s), all encrypted\n' "${#FILES[@]}" diff --git a/scripts/check-tracked-artefacts.sh b/scripts/check-tracked-artefacts.sh new file mode 100755 index 0000000..5694ad5 --- /dev/null +++ b/scripts/check-tracked-artefacts.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# +# Assert no decrypted, rendered or otherwise secret-bearing artefact is tracked. +# +# This check existed twice — inline in ci.yml and again in scripts/validate.sh — +# as two independent implementations of one sentence, and they had already +# drifted (#175): +# +# tracked file CI validate.sh +# stacks/observability/.env caught caught +# stacks/other-stack/.env caught MISSED — it matched ${STACK}/.env only +# some/other/.rendered/x caught MISSED — one hardcoded subdirectory +# nested/certificates/key.pem caught MISSED — anchored ^certificates/ +# +# Every miss failed on push instead of locally, which is the better direction to +# fail in and still the wrong number of implementations. This is the one, and +# both callers run it. +# +# The patterns are unanchored on purpose, which is where validate.sh's copy went +# wrong: `certificates/` matched only at the repository root, so the same key +# one directory down was invisible. A secret does not become safe by being +# nested. +# +# certificates/ is on the list because its contents were committed once and had +# to be removed by rewriting every commit in the repository. .purge-secrets.txt +# is on it because gitleaks cannot cover that file — it is gitignored, so the +# filesystem scan skips it, and it holds bare literals with no keyword context +# to match. Being untracked is the only control either has. +# +# Usage: +# scripts/check-tracked-artefacts.sh names every offender, exit 1 if any +# +# `git ls-files` and not the filesystem: the question is what is TRACKED. A +# rendered .env sitting in the working tree is correct and expected — that is +# what `make render` produces — and only its being committed is the defect. + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "${REPO_ROOT}" + +# Each entry is a fixed path fragment matched anywhere in a tracked path. The +# leading (^|/) makes it a whole path segment, so `certificates/` matches +# `certificates/x` and `a/certificates/x` but not `my-certificates-notes/x`. +PATTERNS=( + '\.env' + '\.rendered/' + '\.purge-secrets\.txt' + 'certificates/' + 'backups/' +) + +# .env.example is the documented template and is meant to be tracked. It is the +# only exception, and it is spelled out rather than pattern-matched so that a +# second exception has to be argued for in a diff. +tracked="$(git ls-files)" +fail=0 +for pattern in "${PATTERNS[@]}"; do + hits="$(printf '%s\n' "${tracked}" \ + | grep -E "(^|/)${pattern}" \ + | grep -v '\.env\.example$' || true)" + if [[ -n "${hits}" ]]; then + printf 'tracked artefact(s) matching %s — these must be gitignored:\n' "${pattern}" >&2 + printf '%s\n' "${hits}" | sed 's/^/ /' >&2 + fail=1 + fi +done + +if ((fail)); then + printf '\nA tracked secret cannot be fixed by deleting it in a later commit —\n' >&2 + printf 'it stays in history. See docs/runbooks/purge-git-history.md\n' >&2 + exit 1 +fi + +printf 'no rendered, decrypted, purge-secrets, certificate or backup files tracked\n' diff --git a/scripts/check_image_pins.py b/scripts/check_image_pins.py index f959a9e..8c10c9b 100755 --- a/scripts/check_image_pins.py +++ b/scripts/check_image_pins.py @@ -471,8 +471,102 @@ def sources() -> list[tuple[str, list[Line]]]: return out +# --------------------------------------------------------------------------- +# The three pattern checks, folded in from ci.yml (#175) +# --------------------------------------------------------------------------- +# These were inline shell in the workflow and unrunnable locally. They belong +# here because this file already owns the argument: the docstring above explains +# why a grep cannot see a *missing* pin, and these three are the greps it is +# explaining. Keeping them beside the parser is what makes that paragraph +# checkable rather than a note about code somewhere else. +# +# Scope is `git ls-files`, not a recursive walk of the working tree. The shell +# versions walked `.`, which is fine in CI's clean checkout and wrong here: this +# repository keeps git worktrees under .claude/worktrees/, so a local run would +# have descended into full copies of itself and reported another branch's +# findings as this one's. Tracked files are also the right question — an +# untracked scratch file pinning :latest harms nobody. + +VERSION_PIN = re.compile( + r"(^|[^a-zA-Z0-9._/-])" + r"[a-z0-9][a-z0-9._-]*/[a-z0-9][a-z0-9._-]*:v?[0-9]+\.[0-9]+" +) +FLOATING = re.compile(r"(^|\s)[a-z0-9._/-]+:latest(\s|$)") +COMMENT = re.compile(r"^\s*#") + +PIN_SCAN = ("scripts/", ".github/", "Makefile") +FLOAT_SUFFIXES = (".yaml", ".yml", ".sh") + + +def tracked_files() -> list[str]: + out = subprocess.run( + ["git", "ls-files"], cwd=REPO, capture_output=True, text=True, check=True + ) + return out.stdout.splitlines() + + +def pattern_problems() -> list[str]: + problems: list[str] = [] + files = tracked_files() + + for rel in files: + path = REPO / rel + if not path.is_file(): + continue + in_pin_scan = rel.startswith(PIN_SCAN) or rel == "Makefile" + is_float_scan = rel.endswith(FLOAT_SUFFIXES) or rel.endswith("Makefile") + if not (in_pin_scan or is_float_scan): + continue + try: + text = path.read_text(encoding="utf-8") + except UnicodeDecodeError: + continue + for n, line in enumerate(text.splitlines(), 1): + if COMMENT.search(line): + continue + # compose.yaml is where a version is SUPPOSED to live. + if in_pin_scan and VERSION_PIN.search(line): + problems.append( + f"{rel}:{n} pins an image version outside compose.yaml — " + f"resolve it with scripts/image-for.sh" + ) + if is_float_scan and FLOATING.search(line): + problems.append( + f"{rel}:{n} uses a floating :latest tag — pin an explicit " + f"version" + ) + return problems + + +def digest_problems() -> list[str]: + """Every service image in every stack carries a digest. + + A tag is a mutable pointer and a digest is the content hash, so an image + with only a tag can change what gets deployed without anything in this + repository changing. Read off the parsed compose file rather than an awk + over `image:` lines, so a quoted or flow-style value is not a hole. + """ + problems = [] + composes = sorted(REPO.glob("stacks/*/compose.yaml")) + if not composes: + return ["no stacks/*/compose.yaml found — this check has stopped checking"] + for cf in composes: + doc = yaml.safe_load(cf.read_text(encoding="utf-8")) or {} + for name, svc in (doc.get("services") or {}).items(): + image = (svc or {}).get("image") + if image and "@sha256:" not in image: + rel = cf.relative_to(REPO) + problems.append( + f"{rel}: {name} image {image} is not pinned by digest — " + f"run make pin-digests" + ) + return problems + + def main() -> int: problems, sites, files = [], 0, 0 + problems.extend(pattern_problems()) + problems.extend(digest_problems()) for rel, lines in sources(): found, seen = check_file(rel, lines) problems.extend(found) diff --git a/scripts/validate.sh b/scripts/validate.sh index 531ab38..9ceeb44 100755 --- a/scripts/validate.sh +++ b/scripts/validate.sh @@ -75,12 +75,14 @@ LINT_SKIPS="" LOKI_SKIPS="" TIMER_SKIPS="" ROUNDTRIP_SKIPS="" +DASH_EXPRS="" cleanup() { [[ -n "${TMP_ENV}" ]] && rm -f "${TMP_ENV}" [[ -n "${LINT_SKIPS}" ]] && rm -f "${LINT_SKIPS}" [[ -n "${LOKI_SKIPS}" ]] && rm -f "${LOKI_SKIPS}" [[ -n "${TIMER_SKIPS}" ]] && rm -f "${TIMER_SKIPS}" [[ -n "${ROUNDTRIP_SKIPS}" ]] && rm -f "${ROUNDTRIP_SKIPS}" + [[ -n "${DASH_EXPRS}" ]] && rm -f "${DASH_EXPRS}" return 0 } trap cleanup EXIT @@ -391,6 +393,42 @@ if have python3; then fail "${stack}: dashboard JSON and datasource references" fi done + + # A panel query that does not parse shows as an empty panel, not an error, so + # nothing about a broken dashboard is loud. This was CI-only until #175. + # + # --emit-promql deliberately fails on a stack with no dashboards: its output + # is the proof the queries parse, so emitting an empty file would pass + # promtool over nothing (#68). "This stack ships none" is knowable here, so + # the guard is here, and it is reported rather than skipped — a skip means + # "could not check", and this checked and found nothing to check. + if ((${#PROMTOOL[@]})); then + for stack in "${STACKS[@]}"; do + if ! compgen -G "stacks/${stack}/grafana/dashboards/*.json" >/dev/null; then + pass "${stack}: no dashboards — no panel queries to parse" + continue + fi + # Inside the repository, not /tmp: PROMTOOL may be a docker run that + # bind-mounts REPO_ROOT and nothing else, so a path outside it does not + # exist on the other side of the container boundary. + DASH_EXPRS="$(mktemp "${REPO_ROOT}/.dashboard-exprs-XXXXXX.yaml")" + # mktemp makes it 0600. When PROMTOOL is the docker fallback the reader + # is the image's own unprivileged user, not this one, and an unreadable + # file fails as "permission denied" — which looks exactly like a broken + # query and is not one. + chmod 0644 "${DASH_EXPRS}" + if python3 scripts/check_dashboards.py --stack "${stack}" --emit-promql > "${DASH_EXPRS}" \ + && "${PROMTOOL[@]}" check rules "${DASH_EXPRS#"${REPO_ROOT}/"}" >/dev/null 2>&1; then + pass "${stack}: dashboard panel queries parse as PromQL" + else + "${PROMTOOL[@]}" check rules "${DASH_EXPRS#"${REPO_ROOT}/"}" || true + fail "${stack}: dashboard panel queries parse as PromQL" + fi + rm -f "${DASH_EXPRS}"; DASH_EXPRS="" + done + else + skip "no promtool binary and no docker daemon — panel queries not parsed" + fi else skip "python3 not installed" fi @@ -548,25 +586,26 @@ else skip "no gitleaks binary and no docker daemon" fi -# Cheap belt-and-braces check that no rendered or decrypted artefact is staged. -# gitleaks cannot cover .purge-secrets.txt — it is gitignored (so the filesystem -# scan skips it) and holds bare literals with no keyword context to match. Being -# untracked is the control. -# certificates/ is in that list because its contents were committed once and -# had to be removed by rewriting every commit in the repository. The cheapest -# possible check is that it never becomes tracked again. -# Every stack's, not one stack's: a second stack means a second .env holding a -# decrypted Grafana password, and the check that it never becomes tracked has -# to grow with it. `git ls-files 'stacks/*/.env'` rather than a loop, so a -# stack added without touching this file is still covered. -if git ls-files 'stacks/*/.env' | grep -q . \ - || git ls-files 'stacks/*/.rendered' 'stacks/*/*/.rendered' | grep -q . \ - || git ls-files | grep -q '\.purge-secrets\.txt' \ - || git ls-files | grep -q '^certificates/' \ - || git ls-files | grep -q '^backups/'; then - fail "a rendered, decrypted, purge-secrets, certificate or backup file is tracked by git" +# Both of these were CI-only until #175, and CI is the one place you cannot +# consult before pushing. The SOPS assert is the sharper of the two: a file that +# is not encrypted is a committed plaintext secret, and a plaintext secret that +# reaches the remote has to be purged from history rather than reverted. +# +# One implementation each, shared with the workflow. The tracked-artefact check +# used to be written out twice — here and in ci.yml — and the two had already +# drifted, this copy missing a second stack's .env, a nested .rendered/ and a +# certificates/ anywhere but the repository root. +if ./scripts/check-sops-encrypted.sh; then + pass "every SOPS file is encrypted" else + fail "every SOPS file is encrypted" +fi + +if ./scripts/check-tracked-artefacts.sh >/dev/null; then pass "no rendered, decrypted, purge-secrets, certificate or backup files tracked" +else + ./scripts/check-tracked-artefacts.sh || true + fail "no rendered, decrypted, purge-secrets, certificate or backup files tracked" fi printf '\n'