From f40cc16bdbb5f0a719f45d004c16d62326fcb58d Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 2 Aug 2026 14:47:54 +0000 Subject: [PATCH] Close the three holes in the test-image scoping guard The guard that stops a consumer naming the shared mutable test image had three ways through it, so a regression could walk back in past something that looked like it was watching. It matched only the literal fips-test:latest, but docker resolves a bare untagged fips-test to exactly that, so image: fips-test reintroduced the defect verbatim and passed. The match now catches the untagged form while still allowing the run-scoped tag and the documented FIPS_TEST_IMAGE fallback. It did not look at the build context at all, so reverting a build.context to the shared testing/docker directory passed. That half of the original defect is now guarded the same way as the image name, matching a path whose last component is docker rather than exempting bind mounts and per-run directories by name. Its allowlist was whole-file, and the comment claiming none of those files was reachable from a CI run was false for two of them. Entries are now scoped to the specific reference that is justified, so a new shared-tag reference anywhere in a formerly exempt file is still caught, and the comment says plainly which files CI does reach and why their particular references are safe. The guard's own exemption is bounded by asserting it starts no docker command. git ls-files failure is now checked rather than discarded, so a sweep that could not run exits 2 instead of reporting a clean tree. Each hole was reproduced against both the old and new guard: all three are red now and were green before. The healthy tree still passes, and the legitimate forms do not false-positive. --- testing/check-image-scoping.sh | 189 +++++++++++++++++++++++++-------- 1 file changed, 145 insertions(+), 44 deletions(-) diff --git a/testing/check-image-scoping.sh b/testing/check-image-scoping.sh index 1190184..0939749 100755 --- a/testing/check-image-scoping.sh +++ b/testing/check-image-scoping.sh @@ -1,22 +1,34 @@ #!/bin/bash -# ── Test-image scoping guard ──────────────────────────────────────────────── -# A local CI run builds fips-test: and hands it to every suite through -# FIPS_TEST_IMAGE / FIPS_TEST_APP_IMAGE. It does NOT write fips-test:latest, -# deliberately: while a bridge back to that shared mutable name existed, a -# consumer that named it directly kept working while resolving whichever -# concurrent run wrote the tag last, and the verdict was then recorded against -# a commit whose binaries had not run. Nothing in the harness compares a -# running container's binary against the commit under test, so that failure is -# silent and leaves no artifact. +# ── Test-image and build-context scoping guard ────────────────────────────── +# A local CI run builds fips-test: from a build context of its own and +# hands both to every suite, through FIPS_TEST_IMAGE / FIPS_TEST_APP_IMAGE and +# FIPS_BUILD_CONTEXT. It does NOT write fips-test:latest and does not build from +# the shared testing/docker directory, deliberately. +# +# Both halves protect the same thing. While a bridge back to the shared mutable +# tag existed, a consumer that named it directly kept working while resolving +# whichever concurrent run wrote the tag last. The shared build context is that +# same failure one level down: testing/docker/ is a single directory in the +# working tree, so two runs racing on its CONTENTS produce a correctly +# per-run-tagged image built from the other run's binaries — scoping the tag +# alone does not close it. Either way the verdict is recorded against a commit +# whose binaries had not run, and nothing in the harness compares a running +# container's binary against the commit under test, so that failure is silent +# and leaves no artifact. # # The bridge is gone, so a consumer that names the shared tag now fails loudly # at run time. This guard is the static half: it stops one being reintroduced, # because the reintroduction is invisible on any host where a hand build has -# left an fips-test:latest lying around. +# left an fips-test:latest and a populated testing/docker/ lying around. # -# What counts as a violation: a reference to the shared tag that is neither a -# comment, nor a documented default of the ${FIPS_TEST_IMAGE:-...} form, nor in -# a file on the allowlist below. +# What counts as a violation: +# * a reference to the shared image name, whether tagged :latest or left +# untagged — docker resolves a bare `fips-test` to fips-test:latest, so +# `image: fips-test` reintroduces the identical defect; +# * a build context that names the shared testing/docker directory; +# unless it is a comment, the documented ${FIPS_TEST_IMAGE:-...} or +# ${FIPS_BUILD_CONTEXT:-...} fallback, or one of the individually justified +# references listed below. # # Exit 0 = clean. Exit 1 = an unexpected reference. Exit 2 = the guard could # not run; never treated as a pass. @@ -25,26 +37,71 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -# Files permitted to name the shared tag outright, each for a stated reason. -# Every one of these is a hand-run path: none is reachable from ci-local.sh -# with FIPS_TEST_IMAGE set. +# The shared image name: tagged :latest, or left untagged, which docker +# resolves to :latest. The run-scoped forms fips-test:${CI_RUN_ID} and the +# comparison harness's fips-test:compare- must NOT match, so the untagged +# half requires the name to end at a character that can continue neither a +# repository name nor a tag. +SHARED_IMAGE_RE="fips-test(-app)?:latest|fips-test(-app)?([^-:._[:alnum:]]|\$)" + +# The shared build context: a path whose LAST component is the tracked +# testing/docker directory. A path that continues past it — ../docker/resolv.conf, +# "\$SCRIPT_DIR"/docker/* — names a file inside the directory rather than using +# the directory as a context, and is not what races. The per-run context is +# docker-, so the terminator class also keeps that from matching. +SHARED_CONTEXT_RE="/docker([[:space:]\"'}]|\$)|^[[:space:]]*(build|context):[[:space:]]*[\"']?docker/?[\"']?[[:space:]]*\$" + +# Individually justified references, as "::". The exemption is scoped to the reference, not to the file: a +# NEW shared-tag or shared-context reference added anywhere in one of these +# files is still a violation. # -# scripts/build.sh the developer build; :latest IS its product -# sidecar/scripts/test-sidecar.sh hand build, behind its --skip-build guard, -# which ci-local always passes -# static/scripts/iperf-compare-refs.sh hand-run A/B against two refs -# ci-cleanup.sh last-resort name for the image ip(8) runs in, -# after the caller's and the run's have failed -# check-image-scoping.sh this guard, which has to name what it looks -# for; it resolves no image -ALLOWED=( - "scripts/build.sh" - "sidecar/scripts/test-sidecar.sh" - "static/scripts/iperf-compare-refs.sh" - "ci-cleanup.sh" - "check-image-scoping.sh" +# Two of these files ARE reached from ci-local.sh, which is why a +# whole-file exemption would not be honest here: +# * sidecar/scripts/test-sidecar.sh is invoked with --skip-build, so the +# build block holding its two references never executes under CI; +# * ci-cleanup.sh is invoked to reap, and its references only pick the +# throwaway image ip(8) runs in — it addresses no test container. +ALLOWED_REFS=( + # The developer build. The shared directory is its context and the shared + # tags are its product; that is the whole point of the script, and CI does + # not run it. + 'scripts/build.sh::DOCKER_DIR="$TESTING_DIR/docker"' + 'scripts/build.sh::docker build -t fips-test:latest "$DOCKER_DIR"' + 'scripts/build.sh::docker build -t fips-test-app:latest -f "$DOCKER_DIR/Dockerfile.app"' + 'scripts/build.sh::echo "Done. Images: fips-test:latest, fips-test-app:latest"' + + # The hand-run sidecar build, behind its --skip-build guard. ci-local.sh + # always passes that flag, so under CI these two lines do not run and the + # image comes from FIPS_TEST_IMAGE like everywhere else. + 'sidecar/scripts/test-sidecar.sh::docker build -t fips-test:latest "$DOCKER_DIR"' + 'sidecar/scripts/test-sidecar.sh::docker build -t fips-test-app:latest -f "$DOCKER_DIR/Dockerfile.app"' + + # Hand-run A/B between two refs: it retags whatever the developer build + # last produced, under a name of its own. + 'static/scripts/iperf-compare-refs.sh::docker tag fips-test:latest "$tag"' + + # The image ip(8) runs in during a reap, wanted only for its iproute2. The + # repository-wide listing and the literal tag are the last two fallbacks, + # after --veth-image and the run's own FIPS_TEST_IMAGE have both come back + # empty; neither addresses a container under test. + 'ci-cleanup.sh::docker image ls --format' + 'ci-cleanup.sh::VETH_IMAGE="fips-test:latest"' + + # Per-ref interop images. This copies three files OUT of the shared + # directory into a context of its own and builds from that, so it reads the + # shared directory but never uses it as a context. + 'interop/build-images.sh::DOCKER_CTX_SRC="$REPO_ROOT/testing/docker"' ) +# This guard is the one file that must contain the patterns it looks for, so it +# is swept out of its own file list. The exemption is bounded rather than +# trusted: the guard must not be able to hide a real invocation behind it, so +# assert it runs no container command. That catches one written as its own +# statement, which is the shape a reintroduction here would take; it would not +# catch one buried inside a command substitution. +GUARD_REL="check-image-scoping.sh" + if ! command -v git >/dev/null 2>&1; then echo "check-image-scoping: git not available, cannot sweep" >&2 exit 2 @@ -53,23 +110,49 @@ if [[ ! -d "$SCRIPT_DIR" ]]; then echo "check-image-scoping: $SCRIPT_DIR missing" >&2 exit 2 fi +if [[ ! -f "$SCRIPT_DIR/$GUARD_REL" ]]; then + echo "check-image-scoping: cannot find itself at $SCRIPT_DIR/$GUARD_REL" >&2 + exit 2 +fi -# Tracked files only, and no documentation: prose naming the tag is describing -# it, not resolving it. -mapfile -t files < <(git -C "$SCRIPT_DIR/.." ls-files -- testing/ | grep -vE '\.md$') -if [[ ${#files[@]} -eq 0 ]]; then +# Tracked files only, and no documentation: prose naming the tag or the +# directory is describing it, not resolving it. +if ! tracked="$(git -C "$SCRIPT_DIR/.." ls-files -- testing/)"; then + echo "check-image-scoping: git ls-files failed, refusing to pass" >&2 + exit 2 +fi +if [[ -z "$tracked" ]]; then echo "check-image-scoping: no tracked files under testing/, refusing to pass" >&2 exit 2 fi +mapfile -t files < <(printf '%s\n' "$tracked" | grep -vE '\.md$') +if [[ ${#files[@]} -eq 0 ]]; then + echo "check-image-scoping: no tracked non-doc files under testing/, refusing to pass" >&2 + exit 2 +fi violations=0 + +if grep -qE '^[[:space:]]*docker[[:space:]]' "$SCRIPT_DIR/$GUARD_REL"; then + echo "FAIL testing/$GUARD_REL runs a docker command." + echo " It is exempt from its own sweep only because it resolves no image." + violations=$((violations + 1)) +fi + +# True when this file:line is one of the justified references above. +allowed_ref() { + local rel="$1" text="$2" entry path frag + for entry in "${ALLOWED_REFS[@]}"; do + path="${entry%%::*}" + frag="${entry#*::}" + [[ "$rel" == "$path" && "$text" == *"$frag"* ]] && return 0 + done + return 1 +} + for f in "${files[@]}"; do rel="${f#testing/}" - skip=0 - for a in "${ALLOWED[@]}"; do - [[ "$rel" == "$a" ]] && skip=1 && break - done - [[ $skip -eq 1 ]] && continue + [[ "$rel" == "$GUARD_REL" ]] && continue [[ -f "$SCRIPT_DIR/../$f" ]] || continue while IFS= read -r hit; do @@ -83,20 +166,38 @@ for f in "${files[@]}"; do [[ "$text" == *'${FIPS_TEST_IMAGE:-fips-test:latest}'* ]] && continue [[ "$text" == *'${FIPS_TEST_APP_IMAGE:-fips-test-app:latest}'* ]] && continue [[ "$text" == *'os.environ.get("FIPS_TEST_IMAGE", "fips-test:latest")'* ]] && continue + allowed_ref "$rel" "$text" && continue echo "FAIL $f:$n names the shared test image directly:" echo " $text" violations=$((violations + 1)) - done < <(grep -n 'fips-test:latest\|fips-test-app:latest' "$SCRIPT_DIR/../$f" 2>/dev/null) + done < <(grep -nE "$SHARED_IMAGE_RE" "$SCRIPT_DIR/../$f" 2>/dev/null) + + while IFS= read -r hit; do + n="${hit%%:*}" + text="${hit#*:}" + [[ "$text" =~ ^[[:space:]]*# ]] && continue + # The same indirection for the context: the shared directory is allowed + # only as the fallback a bare hand run gets. Compose resolves a relative + # build.context against the compose FILE's directory, which is why the + # harness exports an absolute path here. + [[ "$text" == *'${FIPS_BUILD_CONTEXT:-'* ]] && continue + allowed_ref "$rel" "$text" && continue + echo "FAIL $f:$n builds from the shared testing/docker context:" + echo " $text" + violations=$((violations + 1)) + done < <(grep -nE "$SHARED_CONTEXT_RE" "$SCRIPT_DIR/../$f" 2>/dev/null) done if [[ $violations -gt 0 ]]; then echo "" - echo "check-image-scoping: $violations reference(s) to the shared mutable test image." - echo "Read FIPS_TEST_IMAGE (default \${FIPS_TEST_IMAGE:-fips-test:latest}) instead." - echo "A run that resolves the shared tag can execute a concurrent run's binaries" + echo "check-image-scoping: $violations unscoped reference(s) to the shared test image" + echo "or the shared build context." + echo "Read FIPS_TEST_IMAGE (default \${FIPS_TEST_IMAGE:-fips-test:latest}) for the image" + echo "and FIPS_BUILD_CONTEXT (default \${FIPS_BUILD_CONTEXT:-../docker}) for the context." + echo "A run that resolves either shared name can execute a concurrent run's binaries" echo "and record the verdict against this commit." exit 1 fi -echo "check-image-scoping: no unscoped references to the shared test image" +echo "check-image-scoping: no unscoped references to the shared test image or build context" exit 0