diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000..38a0960 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# .githooks/pre-commit -- tracked, reviewable commit gate. +# +# Lives in the repo (unlike .git/hooks/, which is per-clone and invisible to +# review), so a change to what the gate checks arrives as a diff. +# +# Enable once per clone: +# git config core.hooksPath .githooks +# Emergency bypass: +# git commit --no-verify +# +# It lints only the STAGED files, so a commit is never blocked by a finding in +# a file it does not touch. Run the whole tree yourself with: +# ci/linter/run-all.sh +# +# Exit 2 (a linter is not installed) blocks the commit on purpose: a gate that +# skips itself when its tool is missing reports green while checking nothing. +set -uo pipefail + +ROOT="$(git rev-parse --show-toplevel)" +"$ROOT/ci/linter/run-all.sh" --staged +rc=$? +if [ "$rc" -eq 2 ]; then + echo "pre-commit: missing linters -- run ci/linter/install-linters.sh" >&2 +elif [ "$rc" -ne 0 ]; then + echo "pre-commit: lint failed -- fix, or commit with --no-verify" >&2 +fi +exit "$rc" diff --git a/.github/scripts/compute-versions.sh b/.github/scripts/compute-versions.sh new file mode 100755 index 0000000..e8c208a --- /dev/null +++ b/.github/scripts/compute-versions.sh @@ -0,0 +1,129 @@ +#!/usr/bin/env bash +# compute-versions.sh -- resolve the latest upstream versions + their sha256 and +# rewrite .github/versions.env in place. Used by bump.yml (weekly). Prints a +# short summary of what it resolved to stdout, which bump.yml quotes into the +# PR body. +# +# Resolves: +# nginx mainline (odd minor) + stable (even minor) -- nginx.org download page +# Angie latest release -- GitHub API tag_name +# +# Angie is RESOLVED from the GitHub API (the only machine-readable index of +# Angie releases) but DOWNLOADED from download.angie.software, which is what +# ci/tools/ci-build.sh fetches. The two archives differ byte-for-byte, so the +# digest must come from the URL we actually build from -- hashing the GitHub +# tag archive here would pin a sha that never matches at build time. +# +# Run locally to preview a bump: bash .github/scripts/compute-versions.sh +# (it rewrites versions.env; `git diff` to review, `git checkout` to discard). +# +# Requires: curl, jq, sha256sum. GITHUB_TOKEN honoured for API rate limits -- +# unauthenticated api.github.com from a shared runner IP gets 403-throttled. +set -euo pipefail + +VERSIONS_FILE=".github/versions.env" +FV=".github/scripts/fetch-verify.sh" +tmp="$(mktemp -d)" +trap 'rm -rf "$tmp"' EXIT + +api() { + local url="$1" + # Same resilience budget as fetch-verify.sh: a transient blip or a stalled + # connection must retry, not fail the weekly bump job or hang the runner. + local -a opts=(-fsSL --retry 3 --retry-delay 2 --connect-timeout 30 --max-time 300) + if [ -n "${GITHUB_TOKEN:-}" ]; then + curl "${opts[@]}" -H "Authorization: Bearer $GITHUB_TOKEN" "$url" + else + curl "${opts[@]}" "$url" + fi +} + +# sha256 of a URL (download to scratch, hash). Fails the job on download error. +sha_of_url() { + local url="$1" out="$tmp/dl.$RANDOM" sha + # In "-" mode fetch-verify.sh sends progress text to stderr, so stdout is + # exactly one "SHA OUTFILE" line. Read the first field directly rather than + # picking the last line out of mixed output -- an extra stdout line there + # would otherwise be captured as a digest with no error. + read -r sha _ < <(bash "$FV" "$url" - "$out") + # Validate the shape rather than trusting position. Moving the progress text + # to stderr fixes today's contamination; this catches the next one, because a + # non-digest silently written into versions.env would pin every future build + # to a value that can never match. + if ! printf '%s' "$sha" | grep -qE '^[0-9a-f]{64}$'; then + echo "::error::expected a sha256 from $FV for $url, got: ${sha:-}" >&2 + return 1 + fi + printf '%s' "$sha" +} + +echo "resolving nginx versions from nginx.org..." +dl_html="$(curl -fsSL --retry 3 --retry-delay 2 --connect-timeout 30 --max-time 300 \ + https://nginx.org/en/download.html)" +# nginx numbers mainline with an odd minor and stable with an even one. Take +# the highest of each rather than trusting page order. +NGX_MAINLINE="$(printf '%s' "$dl_html" | grep -oE 'nginx-1\.[0-9]+\.[0-9]+' \ + | awk -F. '$2%2==1' | sort -uV | tail -1 | sed 's/nginx-//')" +NGX_STABLE="$(printf '%s' "$dl_html" | grep -oE 'nginx-1\.[0-9]+\.[0-9]+' \ + | awk -F. '$2%2==0' | sort -uV | tail -1 | sed 's/nginx-//')" +[ -n "$NGX_MAINLINE" ] && [ -n "$NGX_STABLE" ] || { echo "::error::failed to resolve nginx versions" >&2; exit 1; } + +echo "resolving Angie latest release..." +ANGIE_TAG="$(api 'https://api.github.com/repos/webserver-llc/angie/releases/latest' | jq -r '.tag_name')" +[ -n "$ANGIE_TAG" ] && [ "$ANGIE_TAG" != "null" ] || { echo "::error::failed to resolve Angie" >&2; exit 1; } +# Upstream tags releases "Angie-1.12.1"; ci-build.sh wants the bare version. +ANGIE="${ANGIE_TAG#Angie-}" +if ! printf '%s' "$ANGIE" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "::error::unexpected Angie tag format: $ANGIE_TAG" >&2 + exit 1 +fi + +echo "hashing archives..." +NGX_MAINLINE_SHA="$(sha_of_url "https://nginx.org/download/nginx-${NGX_MAINLINE}.tar.gz")" +NGX_STABLE_SHA="$(sha_of_url "https://nginx.org/download/nginx-${NGX_STABLE}.tar.gz")" +ANGIE_SHA="$(sha_of_url "https://download.angie.software/files/angie-${ANGIE}.tar.gz")" + +cat > "$VERSIONS_FILE" <". +ANGIE_VERSION=${ANGIE} +ANGIE_SHA256=${ANGIE_SHA} +EOF + +echo "----- resolved -----" +echo "nginx mainline: ${NGX_MAINLINE}" +echo "nginx stable: ${NGX_STABLE}" +echo "angie: ${ANGIE}" diff --git a/.github/scripts/fetch-verify.sh b/.github/scripts/fetch-verify.sh new file mode 100755 index 0000000..feeabb8 --- /dev/null +++ b/.github/scripts/fetch-verify.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash +# fetch-verify.sh URL EXPECTED_SHA256 OUTFILE +# +# Download URL to OUTFILE and verify its sha256 against EXPECTED_SHA256. +# On mismatch: print the actual sha and exit 1 (fails the CI job — a changed +# or tampered upstream archive never reaches the build). +# +# Pass EXPECTED_SHA256="-" to skip verification and just print the computed +# sha (used by bump.yml to harvest fresh hashes for a version bump). +# +# If OUTFILE already exists with the right sha (warm actions/cache hit) the +# download is skipped — the sha check still runs, so a poisoned cache is caught. +set -euo pipefail + +url="${1:?usage: fetch-verify.sh URL SHA256 OUTFILE}" +want="${2:?missing expected sha256}" +out="${3:?missing output path}" + +sha_of() { sha256sum "$1" | cut -d' ' -f1; } + +if [ -f "$out" ] && [ "$want" != "-" ] && [ "$(sha_of "$out")" = "$want" ]; then + echo "cache hit (sha ok): $out" + exit 0 +fi + +# stderr, not stdout: in "-" mode stdout must carry ONLY the final +# "$got $out" line, which compute-versions.sh reads as the digest. +echo "downloading: $url" >&2 +# -f: fail on HTTP errors; -S: show errors; -L: follow redirects; retries. +# --connect-timeout/--max-time: a stalled upstream must not hold a runner open. +curl -fSL --retry 3 --retry-delay 2 \ + --connect-timeout 30 --max-time 300 -o "$out" "$url" + +got="$(sha_of "$out")" +if [ "$want" = "-" ]; then + echo "$got $out" + exit 0 +fi + +if [ "$got" != "$want" ]; then + echo "::error::sha256 MISMATCH for $url" >&2 + echo " expected: $want" >&2 + echo " actual: $got" >&2 + exit 1 +fi +echo "sha256 verified: $out" diff --git a/.github/scripts/load-versions.sh b/.github/scripts/load-versions.sh new file mode 100755 index 0000000..28ae7c2 --- /dev/null +++ b/.github/scripts/load-versions.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# load-versions.sh — export every pin from .github/versions.env into the +# workflow environment. Run as the first step of every CI job: +# +# - run: bash .github/scripts/load-versions.sh +# +# Skips comments/blanks; validates each line is KEY=value so a malformed +# versions.env fails loudly instead of injecting garbage into $GITHUB_ENV. +set -euo pipefail + +f=".github/versions.env" +[ -f "$f" ] || { echo "::error::$f not found" >&2; exit 1; } +# Outside a GitHub Actions step $GITHUB_ENV is unset, and set -u turns that +# into a bare "unbound variable" -- give anyone running this by hand a +# message that says what actually went wrong. +[ -n "${GITHUB_ENV:-}" ] || { echo "::error::GITHUB_ENV unset -- run this inside a GitHub Actions step" >&2; exit 1; } + +while IFS= read -r line || [ -n "$line" ]; do + case "$line" in + ''|\#*) continue ;; + esac + if ! printf '%s' "$line" | grep -qE '^[A-Za-z_][A-Za-z0-9_]*='; then + echo "::error::malformed line in $f: $line" >&2 + exit 1 + fi + echo "$line" >> "$GITHUB_ENV" + echo "loaded: ${line%%=*}" +done < "$f" diff --git a/.github/versions.env b/.github/versions.env new file mode 100644 index 0000000..9af3574 --- /dev/null +++ b/.github/versions.env @@ -0,0 +1,37 @@ +# Central version + sha256 pins for all CI workflows. +# +# SINGLE SOURCE OF TRUTH. Every workflow loads this file into $GITHUB_ENV as its +# first step (via .github/scripts/load-versions.sh); the weekly bump.yml job +# rewrites it and opens a PR. Tarballs are pinned by version string (release +# archives are immutable) AND verified against the sha256 recorded here, so a +# compromised or changed upstream archive fails the build instead of being +# compiled. +# +# Version and digest live on adjacent lines on purpose: they are bumped by one +# writer (compute-versions.sh) in one file, so a version can no longer move +# while its digest stays behind. +# +# Regenerate with .github/scripts/compute-versions.sh (bump.yml runs it weekly). +# Keep KEY=value, no spaces, no quotes -- this file is both `source`d by +# ci/tools/ci-build.sh and `cat`d into $GITHUB_ENV. + +# nginx mainline (odd minor) -- the default build everywhere; also the +# ci-deep "mainline" matrix cell. +NGINX_MAINLINE=1.31.3 +NGINX_MAINLINE_SHA256=a7657c50811c2d92d9895395e8b873ef60398142c4db21eb647811c38f6dd525 + +# nginx stable (even minor) -- ci-deep "stable" matrix cell only. +NGINX_STABLE=1.30.4 +NGINX_STABLE_SHA256=4261dc90e9e47c1c4041276e9aaa3d48ebe2e664f728e14fa95ae6c67d57a08b + +# nginx version used by every single-version job (build-test, asan, valgrind, +# codeql, fuzzing, security-scanners, ci-deep memcheck). Tracks mainline. +NGINX_VERSION=1.31.3 +NGINX_VERSION_SHA256=a7657c50811c2d92d9895395e8b873ef60398142c4db21eb647811c38f6dd525 + +# Angie (webserver-llc) -- ci-deep "angie" matrix cell. Pinned to the tarball +# from download.angie.software, NOT the GitHub tag archive: different bytes, +# so this digest is not interchangeable with a github.com/webserver-llc one. +# ANGIE_VERSION is the bare version; the upstream release tag is "Angie-". +ANGIE_VERSION=1.12.1 +ANGIE_SHA256=5f4f203be2aca6fe20770b489c720e46e51d337e521065e7e472b61e24e3d2f5 diff --git a/.github/workflows/asan.yml b/.github/workflows/asan.yml index 3e4bf73..3fa3627 100644 --- a/.github/workflows/asan.yml +++ b/.github/workflows/asan.yml @@ -20,26 +20,12 @@ name: A/UBSan env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: - push: - branches: [main] - paths: - - "src/**" - - "ci/t/**" - - "ci/tools/ci-build.sh" - - "ci/tools/soak.sh" - - ".github/workflows/asan.yml" - pull_request: - branches: [main] - paths: - - "src/**" - - "ci/t/**" - - "ci/tools/ci-build.sh" - - "ci/tools/soak.sh" - - ".github/workflows/asan.yml" - workflow_dispatch: {} + # Triggered by ci.yml, which lanes the PR suite so peak runner use stays + # bounded. No push/pull_request trigger here on purpose: two entry points + # would run this twice per PR and defeat the laning. + workflow_call: {} concurrency: group: asan-${{ github.workflow }}-${{ github.ref }} @@ -59,6 +45,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 6eb5c18..7531b35 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -11,17 +11,18 @@ name: Build&Test env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: - push: - branches: [main] - pull_request: - branches: [main] - workflow_dispatch: {} + # Triggered by ci.yml, which lanes the PR suite so peak runner use stays + # bounded. No push/pull_request trigger here on purpose: two entry points + # would run this twice per PR and defeat the laning. + workflow_call: {} concurrency: - group: ci-${{ github.workflow }}-${{ github.ref }} + # Prefix must be unique per workflow. As a workflow_call member this inherits + # the caller's github.workflow/github.ref, so a group string matching ci.yml's + # would cancel the caller rather than a stale copy of this workflow. + group: build-test-${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true permissions: @@ -93,6 +94,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install build dependencies run: | sudo apt-get update @@ -229,6 +233,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update @@ -284,6 +291,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install build dependencies run: | sudo apt-get update @@ -345,6 +355,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update diff --git a/.github/workflows/ci-deep.yml b/.github/workflows/ci-deep.yml index a43fe7d..7eee199 100644 --- a/.github/workflows/ci-deep.yml +++ b/.github/workflows/ci-deep.yml @@ -30,7 +30,6 @@ name: CI Deep env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: schedule: @@ -52,21 +51,27 @@ permissions: jobs: build-flavors: - name: Build & Test::Nginx (${{ matrix.flavor }} ${{ matrix.version }}) + # matrix.label, not the version: the job name is evaluated before any step + # runs, so the resolved version is not available here. + name: Build & Test::Nginx (${{ matrix.flavor }} ${{ matrix.label }}) runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || fromJSON('["self-hosted","builder02","lxc"]') }} timeout-minutes: 30 strategy: fail-fast: false + # A matrix is expanded before any step runs, so load-versions.sh cannot + # feed it. Each cell therefore names the versions.env KEY it wants, and + # the build step dereferences it after the pins are loaded -- keeping the + # versions themselves in one file instead of re-hardcoding them here. matrix: include: - flavor: nginx - version: "1.31.2" + version_key: NGINX_MAINLINE label: mainline - flavor: nginx - version: "1.30.3" + version_key: NGINX_STABLE label: stable - flavor: angie - version: "1.12.0" + version_key: ANGIE_VERSION label: angie steps: - name: Checkout module @@ -74,6 +79,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update @@ -82,28 +90,67 @@ jobs: cpanminus libio-socket-ssl-perl sudo cpanm --notest --quiet Test::Nginx + - name: Resolve matrix version + # The cell names a versions.env key (NGINX_MAINLINE/NGINX_STABLE/ + # ANGIE_VERSION); dereference it now that load-versions.sh has put the + # pins in the environment, so later steps can use env.MATRIX_VERSION. + env: + # Through env, not inline ${{ }}: an expression expanded straight into + # a run: block is template injection (zizmor template-injection). The + # matrix values are repo-controlled today, but the safe shape costs + # nothing and stops the pattern from being copied somewhere it is not. + MATRIX_VERSION_KEY: ${{ matrix.version_key }} + run: | + key="$MATRIX_VERSION_KEY" + value="${!key:-}" + if [ -z "$value" ]; then + echo "::error::$key is not set in .github/versions.env" + exit 1 + fi + echo "MATRIX_VERSION=$value" >> "$GITHUB_ENV" + echo "resolved $key=$value" + - name: Restore build caches uses: ./.github/actions/build-cache with: mode: debug flavor: ${{ matrix.flavor }} - nginx-version: ${{ matrix.version }} + nginx-version: ${{ env.MATRIX_VERSION }} - name: Build server and dynamic module + # angie names its binary objs/angie, nginx names it objs/nginx. Assert + # BOTH artifacts and export the server path, so the flavor's real binary + # name lives in one place instead of being hardcoded per step -- checking + # only the .so passes for angie while leaving the server unverified. + env: + MATRIX_FLAVOR: ${{ matrix.flavor }} # via env, see the note above run: | - bash ci/tools/ci-build.sh "${{ matrix.flavor }}" "${{ matrix.version }}" - build=".build/${{ matrix.flavor }}-${{ matrix.version }}-debug/objs" + set -euo pipefail + bash ci/tools/ci-build.sh "$MATRIX_FLAVOR" "$MATRIX_VERSION" + build=".build/$MATRIX_FLAVOR-${MATRIX_VERSION}-debug/objs" + case "$MATRIX_FLAVOR" in + angie) server="$build/angie" ;; + *) server="$build/nginx" ;; + esac test -f "$build/ngx_http_skel_module.so" + test -x "$server" + echo "SERVER_BIN=$GITHUB_WORKSPACE/$server" >> "$GITHUB_ENV" + echo "MODULE_SO=$GITHUB_WORKSPACE/$build/ngx_http_skel_module.so" >> "$GITHUB_ENV" - name: Run ci/t/ env: - TEST_NGINX_BINARY: ${{ github.workspace }}/.build/${{ matrix.flavor }}-${{ matrix.version }}-debug/objs/nginx - TEST_NGINX_LOAD_MODULES: ${{ github.workspace }}/.build/${{ matrix.flavor }}-${{ matrix.version }}-debug/objs/ngx_http_skel_module.so + # Set by the build step above -- it resolves the flavor's binary name + # (angie vs nginx) once, after the version is known. + TEST_NGINX_BINARY: ${{ env.SERVER_BIN }} + TEST_NGINX_LOAD_MODULES: ${{ env.MODULE_SO }} # See build-test.yml's identical setting for why this must be 20, not # the ~2s Test::Nginx::Socket default. TEST_NGINX_TIMEOUT: "20" TEST_NGINX_SERVROOT: ${{ github.workspace }}/ci/t/servroot - run: prove -v ci/t/ + # Test::Nginx probes a server on PATH for its version and Bailouts if it + # finds none -- TEST_NGINX_BINARY alone does not satisfy that probe, so + # the objs/ dir goes on PATH too. + run: PATH="$(dirname "$TEST_NGINX_BINARY"):$PATH" prove -v ci/t/ fuzz: name: Fuzz (scan core, long) @@ -115,6 +162,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install build deps run: | sudo apt-get update @@ -199,6 +249,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update @@ -231,6 +284,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..6ea5292 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,189 @@ +name: CI + +# Single entry point for the PR suite. The seven checks below used to be +# independent workflows, each with its own push/pull_request trigger, so a PR +# asked for all of them at once. On the builder02 runners that is the wrong +# shape: CI wall-clock here is dominated by jobs QUEUEING for a label-matching +# slot, not by the jobs themselves, so ten simultaneous requests just means the +# tail waits. builder02 has SIX ephemeral slots (ci-ephemeral@runner-01..04, +# @docker-01..02); everything below is shaped around that number. +# +# The self-hosted work is arranged as three lanes. A lane starts with its +# longest job and releases the slot to a shorter, independent follow-up. +# +# A Fuzzing (277-282s) = 277-282s +# B A/UBSan (84-97s) -> Valgrind (79-85s) -> Scanners (47-94s) = 228-269s +# C Build&Test (~60s) = 60s +# +# The budget is the longest single job -- fuzzing, which is a fixed 120s of +# fuzz plus its build -- because no arrangement can finish sooner than that. +# Lane B stays under it, so the suite is fuzz-bound. +# +# The ranges are not padding. They are the SPREAD BETWEEN TWO CONSECUTIVE GREEN +# RUNS of the same tree, runs 30591711577 (head 1b6d094, 286s) and 30592009272 +# (head 0a3ddcf, 280s), which differ only by a comment. Lane B finished 54s +# ahead of Fuzzing in the first and 1s ahead in the second; Security scanners +# alone read 47s and then 94s. builder02 also runs the package builds, so a job +# here is timed against whatever else the box is doing -- one sample tells you +# nothing, which is why the earlier "~59s of headroom" claim in this header was +# wrong the day after it was measured. +# +# So: treat lane B as roughly at budget, not comfortably under it. Adding work +# to it needs a fourth lane, not the headroom this comment used to promise. +# An earlier revision quoted 320/149/120/110/100/98s and paired Fuzzing with +# Security scanners, which put lane A at 348s: over budget, and the suite's +# actual critical path, for a 352s total. Scanners came down from 77s because +# semgrep gained --jobs=1 --metrics=off in the same change. Re-measure with +# `gh run view -R myguard-labs/nginx-skeleton-module --json jobs` over +# SEVERAL runs before trusting any number here. +# +# NOTE on "peak runner use": a lane is not a slot. Build&Test is a reusable +# workflow that fans out into FIVE concurrent self-hosted jobs, so the observed +# peak is 5 + Fuzzing + A/UBSan = 7 against 6 slots, and one job queues briefly +# at t=0. That is accepted -- it costs seconds and only at the start -- but do +# not read "three lanes" as "three runners". +# +# Lint, CodeQL and Detect-relevant-changes run on ubuntu-latest and take no +# self-hosted slot, so they are not laned at all and start immediately. +# +# Follow-ups use !cancelled() so a FAILING first check does not suppress an +# unrelated second one -- a red A/UBSan should still tell you whether Valgrind +# is clean. It also keeps the chain alive when A/UBSan is SKIPPED by the +# changed-files gate below, which a bare `needs:` would not. A superseded +# workflow is still cancelled as one unit, because cancel-in-progress below +# applies to the whole run. +# +# No `push:` trigger, by repo policy: CI runs on the PR, and the merge commit is +# identical to the tested PR head, so re-running it on merge buys nothing. + +on: + pull_request: + branches: [main] + workflow_dispatch: {} + +concurrency: + # "orchestrator-", not "ci-": build-test.yml already uses + # ci-${{ github.workflow }}-${{ github.ref }}, and a called workflow inherits + # the caller's github.workflow/github.ref -- so an identical group string + # makes the member cancel its own caller, and lane C dies before it starts a + # single job. The prefix is what keeps the two apart. + group: orchestrator-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + # --- lane A: the budget-setting job, alone ------------------------------- + # Nothing follows Fuzzing. It is the longest job in the suite, so the suite + # cannot finish sooner; anything chained behind it is pure added critical + # path. Security scanners used to sit here and cost exactly that. + fuzzing: + name: Fuzzing + permissions: + contents: read + uses: ./.github/workflows/fuzzing.yml + + # --- lane B -------------------------------------------------------------- + # asan.yml previously carried its own paths: filter (src/**, ci/t/**, + # ci-build.sh, soak.sh, its own workflow). A reusable workflow cannot filter + # its own triggering, so that gate moves here as an explicit job-level `if` + # over the changed-files output -- dropping it silently would run the 149s + # sanitizer job on every docs-only PR. + changes: + name: Detect relevant changes + runs-on: ubuntu-latest + permissions: + contents: read + outputs: + code: ${{ steps.filter.outputs.code }} + steps: + - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + persist-credentials: false + # Full history: a shallow clone has no common ancestor with the base, + # so `base...HEAD` dies with "no merge base" and the gate below would + # fall through to code=false -- skipping the sanitizer job on exactly + # the PRs that need it. + fetch-depth: 0 + - id: filter + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + # workflow_dispatch has no PR base; treat that as "run everything". + if [ -z "${BASE_SHA:-}" ]; then + echo "code=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + # Fail loudly rather than fail open: this gate decides whether a + # sanitizer runs, so an unusable diff must stop the job, not quietly + # answer "no relevant changes". + if ! changed="$(git diff --name-only "$BASE_SHA"...HEAD)"; then + echo "::error::cannot diff $BASE_SHA...HEAD -- refusing to guess" >&2 + exit 1 + fi + # One pattern, decision and log read the same variable, so they + # cannot drift the way two copies of the same regex eventually do. + gate='^(src/|ci/t/|ci/tools/ci-build\.sh|ci/tools/soak\.sh|\.github/workflows/(asan|ci)\.yml|\.github/versions\.env|\.github/scripts/)' + if printf '%s\n' "$changed" | grep -qE "$gate"; then + echo "code=true" >> "$GITHUB_OUTPUT" + else + echo "code=false" >> "$GITHUB_OUTPUT" + fi + echo "changed files matching the asan gate:" + printf '%s\n' "$changed" | grep -E "$gate" || echo " (none)" + + asan: + name: A/UBSan + needs: changes + if: ${{ needs.changes.outputs.code == 'true' }} + permissions: + contents: read + uses: ./.github/workflows/asan.yml + + valgrind: + name: Valgrind + needs: asan + if: ${{ !cancelled() }} + permissions: + contents: read + uses: ./.github/workflows/valgrind.yml + + security-scanners: + name: Security scanners + # Tail of lane B, not of lane A. The lane still fits under the fuzzing + # budget, so this job is free here and was critical path there. Header has + # the measured spread; lane B has less room than it looks. + needs: valgrind + if: ${{ !cancelled() }} + permissions: + contents: read + uses: ./.github/workflows/security-scanners.yml + + # --- lane C -------------------------------------------------------------- + build-test: + name: Build&Test + permissions: + contents: read + uses: ./.github/workflows/build-test.yml + + # --- hosted, unlaned ----------------------------------------------------- + # These run on ubuntu-latest, take no self-hosted slot, and so have no reason + # to wait behind anything. Lint is the fastest check in the suite and should + # report first. CodeQL used to hang off build-test to conserve a slot it never + # occupied -- that only delayed its result by ~50s. + lint: + name: Lint + permissions: + contents: read + uses: ./.github/workflows/lint.yml + + codeql: + name: CodeQL + permissions: + contents: read + # Granted per-job, not at workflow level: no other job here needs either. + security-events: write # CodeQL uploads its SARIF to code scanning + actions: read # the CodeQL action reads run metadata to attribute results + uses: ./.github/workflows/codeql.yml diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index d0c649f..81f4f70 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -20,18 +20,16 @@ name: CodeQL env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: - push: - branches: [main] - pull_request: - branches: [main] + # Triggered by ci.yml, which lanes the PR suite so peak runner use stays + # bounded. No push/pull_request trigger here on purpose: two entry points + # would run this twice per PR and defeat the laning. + workflow_call: {} schedule: - # Monthly, day 4 @ 04:47 UTC. Staggered away from ci-deep.yml (day 6) so the - # two heavy scheduled runs never contend for the same runners. + # Monthly, day 4 @ 04:47 UTC. Staggered away from ci-deep.yml (day 6) so + # the two heavy scheduled runs never contend for the same runners. - cron: "47 4 4 * *" - workflow_dispatch: {} concurrency: group: codeql-${{ github.ref }} @@ -54,6 +52,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install build dependencies run: | sudo apt-get update diff --git a/.github/workflows/fuzzing.yml b/.github/workflows/fuzzing.yml index 98f73ae..ee8586e 100644 --- a/.github/workflows/fuzzing.yml +++ b/.github/workflows/fuzzing.yml @@ -14,14 +14,12 @@ name: Fuzzing env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: - push: - branches: [main] - pull_request: - branches: [main] - workflow_dispatch: {} + # Triggered by ci.yml, which lanes the PR suite so peak runner use stays + # bounded. No push/pull_request trigger here on purpose: two entry points + # would run this twice per PR and defeat the laning. + workflow_call: {} concurrency: group: fuzzing-${{ github.workflow }}-${{ github.ref }} @@ -41,6 +39,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install build deps run: | sudo apt-get update diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..b080f98 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,66 @@ +name: Lint + +# The same ci/linter/ scripts a developer runs locally, run again on the PR -- +# so a clone that never enabled the hook (`git config core.hooksPath .githooks`) +# cannot land nginx-convention, shell, Perl, Python or workflow-syntax +# regressions. Local and remote run the SAME entry point on purpose: a CI-only +# reimplementation drifts from the hook and the two stop agreeing. +# +# LINT_ONLY leaves out the "c" checker: flawfinder/cppcheck/semgrep over src/ +# are already the job of security-scanners.yml, at the same thresholds +# (ci/linter/lint-c.sh mirrors that workflow). Running them twice per PR buys +# nothing but queue time. +# +# runs-on: ubuntu-latest, NOT the self-hosted pool. This job needs no nginx +# build, and ci.yml deliberately caps peak self-hosted use at three concurrent +# jobs; putting lint on a hosted runner keeps that cap intact while giving the +# fastest feedback in the suite. + +env: + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + +on: + # Triggered by ci.yml, like every other member of the suite. No + # push/pull_request trigger here: two entry points would run it twice per PR. + workflow_call: {} + +concurrency: + group: lint-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + lint: + name: Linters + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Checkout module + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + persist-credentials: false + + - name: Install linters + # One installer for CI and for a fresh clone; a second apt list here + # would be the thing that goes stale. + run: ci/linter/install-linters.sh + + - name: Verify the toolchain is complete + # A linter missing from PATH makes its check exit 2, which run-all.sh + # reports as a failure rather than skipping -- this step just surfaces + # WHICH one before the findings scroll past. + run: ci/linter/install-linters.sh --check + + - name: Selftest the gate + # Before trusting the next step's verdict, prove the gate can still say + # no: a selector that matches nothing must exit 2, not report clean. + # The step below narrows the run with LINT_ONLY, so a silent selector + # is exactly the failure that would make this whole job vacuous. + run: ci/linter/selftest.sh + + - name: Run linters + env: + LINT_ONLY: nginx sh python perl yaml + run: ci/linter/run-all.sh diff --git a/.github/workflows/security-scanners.yml b/.github/workflows/security-scanners.yml index 7e89514..8b394a3 100644 --- a/.github/workflows/security-scanners.yml +++ b/.github/workflows/security-scanners.yml @@ -9,10 +9,19 @@ name: Security scanners # to ignore it. # clang-tidy -- blocks on cert-* and clang-analyzer-security.*. These are # path-sensitive and near-zero-FP on code this size. -# semgrep -- ADVISORY (|| true). It is the highest-FP of the three; its -# value is the uploaded report a human reads, not a red X. If a -# semgrep rule earns its keep, promote it to blocking here -# deliberately -- do not leave it advisory forever by default. +# semgrep -- blocks at WARNING and above; INFO stays advisory. Promoted +# from fully-advisory 2026-07-30. +# WARNING, not ERROR, is the gate level on purpose: the +# registry's ERROR tier has no C rules that fire on this kind +# of code (verified -- an ERROR-only gate matched 48 rules and +# 0 findings against a deliberate strcpy/format-string file, +# i.e. it could never go red). WARNING is the lowest tier with +# real signal: it catches that same file via +# insecure-use-string-copy-fn. src/ is clean at WARNING+ today, +# so this goes in green. +# Registry C coverage is thin either way (~9 rules reach these +# files) -- semgrep is the third line here, behind flawfinder +# and clang-tidy, which carry the real C weight. # # Action pins (keep in sync with build-test.yml): # actions/checkout@v5 -> 93cb6efe18208431cddfb8368fd83d5badbf9bfd @@ -20,14 +29,12 @@ name: Security scanners env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: - push: - branches: [main] - pull_request: - branches: [main] - workflow_dispatch: {} + # Triggered by ci.yml, which lanes the PR suite so peak runner use stays + # bounded. No push/pull_request trigger here on purpose: two entry points + # would run this twice per PR and defeat the laning. + workflow_call: {} concurrency: group: security-scanners-${{ github.workflow }}-${{ github.ref }} @@ -47,6 +54,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update @@ -112,11 +122,37 @@ jobs: done exit "$status" - - name: semgrep (advisory) + - name: semgrep (gate on >=WARNING) run: | + set -euo pipefail command -v semgrep >/dev/null || { echo "semgrep missing from PATH" >&2; exit 1; } + # --jobs=1 is a correctness flag on a self-hosted runner, not a speed + # flag. semgrep-core defaults to one OCaml domain per core (32 on this + # box), each opening an io_uring ring against an 8 MB RLIMIT_MEMLOCK + # shared with every other job on the host. Under runner load it + # exhausts and semgrep-core dies with `Unix_error: Cannot allocate + # memory io_uring_queue_init` -> exit 2, i.e. a load-dependent red + # that has nothing to do with the code under review. Reproduced 3/3 on + # a busy box and 0/3 on an idle one. src/ is three files, so the + # parallelism was buying nothing. ci/linter/lint-c.sh carries the same + # flag -- keep the two in sync or local stops predicting remote. + # --metrics=off drops the scan-summary POST (2.76s -> 1.27s measured). + # + # Pass 1: full scan, every severity, never blocking. This is what + # lands in the uploaded artifact for a human to read -- INFO findings + # stay visible even though they do not gate. semgrep scan --config p/c --config p/security-audit \ + --jobs=1 --metrics=off \ "$GITHUB_WORKSPACE/src/" 2>&1 | tee semgrep.log || true + # Pass 2: the gate. --error makes semgrep exit 1 on any match at + # WARNING or ERROR. Exit 2 (fatal: bad config, network, parse error) + # also fails the step rather than passing silently -- a scanner that + # could not run is not a clean scan. + semgrep scan --config p/c --config p/security-audit \ + --severity=WARNING --severity=ERROR --error \ + --jobs=1 --metrics=off \ + "$GITHUB_WORKSPACE/src/" 2>&1 | tee -a semgrep.log + echo "semgrep: clean at >=WARNING" - name: Upload reports if: always() diff --git a/.github/workflows/valgrind.yml b/.github/workflows/valgrind.yml index 24f5841..b3a04b4 100644 --- a/.github/workflows/valgrind.yml +++ b/.github/workflows/valgrind.yml @@ -19,14 +19,12 @@ name: Valgrind env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true - NGINX_VERSION: "1.31.2" on: - push: - branches: [main] - pull_request: - branches: [main] - workflow_dispatch: {} + # Triggered by ci.yml, which lanes the PR suite so peak runner use stays + # bounded. No push/pull_request trigger here on purpose: two entry points + # would run this twice per PR and defeat the laning. + workflow_call: {} concurrency: group: valgrind-${{ github.workflow }}-${{ github.ref }} @@ -46,6 +44,9 @@ jobs: with: persist-credentials: false + - name: Load version pins + run: bash .github/scripts/load-versions.sh + - name: Install dependencies run: | sudo apt-get update diff --git a/.perlcriticrc b/.perlcriticrc new file mode 100644 index 0000000..6cbebdc --- /dev/null +++ b/.perlcriticrc @@ -0,0 +1,23 @@ +# perlcritic config -- used by ci/linter/lint-perl.sh and by editor plugins. +# +# The Perl in this repo is a Test::Nginx::Socket suite (ci/t/*.t), not library +# code. Four default policies at severity >=4 describe a .pm and fire on every +# well-formed .t file; leaving them on lands the gate red on arrival, which +# only teaches everyone to --no-verify. Each exclusion below is about the file +# SHAPE, not about tolerating a real defect: +# +# RequireExplicitPackage a .t is a script, not a module; wrapping the suite +# in a package would hide Test::Nginx's exported DSL. +# RequireEndWithOne same reason -- a script has no module return value. +# RequireUseStrict Test::Nginx::Socket enables strict and warnings in +# RequireUseWarnings its import, so the policies see "code before +# strictures" for pragmas that ARE in effect. +# +# Everything else at severity >=4 stays live. Add a policy here only with the +# reason it cannot apply, not because it is inconvenient. +severity = 4 + +[-Modules::RequireExplicitPackage] +[-Modules::RequireEndWithOne] +[-TestingAndDebugging::RequireUseStrict] +[-TestingAndDebugging::RequireUseWarnings] diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..04f0cd9 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,168 @@ +--- +# yamllint disable rule:line-length +# The exclude regex and the actionlint entry are single unsplittable +# strings; wrapping either changes what they match. +# Local pre-commit gates for nginx-skeleton-module. +# +# GENERATED by tools/gen-precommit-config.py -- re-run it rather than +# hand-editing paths, so every module's config stays derived from its real +# layout instead of drifting toward a copied-around guess. +# +# Purpose: catch the cheap half of what CI catches, before a push burns a +# remote round-trip. These hooks deliberately MIRROR the CI thresholds in +# .github/workflows/security-scanners.yml (flawfinder >=4, semgrep >=WARNING). +# If a threshold moves there, move it here in the same commit, or local-green +# stops predicting remote-green. +# +# Detected in this repo: +# C source dirs : src +# fuzz corpora : ci/fuzz/corpus, ci/fuzz/regressions +# +# What is NOT here, and why: +# clang-tidy -- needs ngx_auto_config.h, which only exists after nginx is +# configured. A hook cannot assume a built tree, and one that +# silently skips when the tree is missing is a vacuous gate. +# Stays CI-only on purpose. +# clang-format -- no .clang-format in these repos, so there is no agreed style +# to enforce; adding one would reformat existing code in an +# unrelated diff. +# codeql/asan/valgrind/fuzzing -- far too slow for a commit hook; CI-only. +# +# Fuzz harnesses (fuzz/, t/) are outside the lint scope: they deliberately do +# unsafe things with untrusted input and would trip flawfinder forever. +# +# Install (once per clone): +# pipx install pre-commit && pre-commit install +# Run against everything without committing: +# pre-commit run --all-files +# +# VERIFY BEFORE TRUSTING (a green hook proves nothing until seen red): +# printf '#!/bin/bash\ncd /tmp/x\nrm -rf "${B}/"*\n' > _p.sh +# pre-commit run shellcheck --files _p.sh # expect Failed +# rm _p.sh +# Note SC2086 is INFO and correctly does NOT trip shellcheck here -- probe with +# a real warning-severity finding (SC2164/SC2115) or you will misread the gate. + +# Fuzz corpus files are byte-exact test inputs: a stripped space or an added +# newline changes what is actually fuzzed. Never let a fixer touch them. +# +# .pem and .der are NOT in this list, deliberately. pre-commit applies the +# top-level exclude before any per-hook rule, so a suffix named here is +# invisible to EVERY hook -- including detect-private-key, whose entire job is +# catching a .pem. Verified: an identical fake key reads "(no files to check) +# Skipped" as probe.pem and "Private key found" as probe.txt. They are excluded +# per-hook on the three fixers below instead, which is the only reason they were +# ever in this list. +exclude: '^ci/fuzz/corpus/|^ci/fuzz/regressions/|\.traefik-ref/|\.claude/worktrees/|\.build/|(^|/)objs/|(^|/)vendor/|(^|/)node_modules/|(^|/)\.venv/|\.(bin|gz|zip|png|ico|so|o|a)$' + +repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v6.0.0 + hooks: + # The three fixers rewrite files in place, which is why keys and DER blobs + # have to stay out of their reach. Per-hook, not top-level: a top-level + # exclude would also blind detect-private-key below. + - id: trailing-whitespace + exclude: '\.(pem|der)$' + - id: end-of-file-fixer + exclude: '\.(pem|der)$' + - id: mixed-line-ending + args: [--fix=lf] + exclude: '\.(pem|der)$' + - id: check-merge-conflict + - id: check-yaml + - id: check-added-large-files + args: [--maxkb=512] + # Test vectors are not secrets. autocert's tests/unit/test_crypto.c and + # test_alpn.c carry locked PEM fixtures (an elided MIGH... blob and a + # documented P-256 vector); gating on those is a pure false positive, + # while the hook still guards every real path. + - id: detect-private-key + exclude: '^tests?/' + + - repo: local + hooks: + # Mirrors the CI "flawfinder (gate on >=4)" step: fires on exactly what + # CI blocks on. Noise below level 4 would train everyone to --no-verify. + - id: flawfinder + name: flawfinder (gate on >=4) + entry: flawfinder --minlevel=4 --error-level=4 --quiet + language: system + files: '^src/.*\.[ch]$' + pass_filenames: true + + # Mirrors the CI "semgrep (gate on >=WARNING)" step, same configs and + # same severity floor. WARNING not ERROR: the registry's ERROR tier has + # no C rules that fire on this kind of code, so an ERROR gate could never + # go red (verified 2026-07-30). + # + # require_serial IS LOAD-BEARING, do not remove it to "speed the hook up": + # pre-commit otherwise shards the file list across one process per core + # (32 here), and every semgrep-core allocates its own io_uring. That blows + # past RLIMIT_MEMLOCK (ulimit -l, 8192 KB on this box) and semgrep-core + # dies with + # Unix_error: Cannot allocate memory io_uring_queue_init + # surfacing as a bare "exit code 2" with no message. That failure is + # indistinguishable from a real finding at a glance, so it would have + # this gate permanently red for a reason that has nothing to do with the + # code. One process, one ring, no contention. + # + # --quiet is also deliberately ABSENT: it suppressed exactly the error + # text above and turned a diagnosable crash into a silent exit 2. + - id: semgrep + name: semgrep (gate on >=WARNING) + entry: semgrep scan --config p/c --config p/security-audit + --severity=WARNING --severity=ERROR --error + language: system + files: '^src/.*\.[ch]$' + pass_filenames: true + require_serial: true + + # CI workflows are code that breaks silently; actionlint catches bad if: + # expressions, unknown contexts and shell errors inside run: blocks. + # + # -config-file: declares our self-hosted-runner labels (builder02, lxc, + # b02lxc, docker) explicitly rather than relying on actionlint finding + # .github/actionlint.yaml by cwd-guessing -- pre-commit hooks don't + # guarantee cwd the way a bare `actionlint` invocation from a shell does. + # + # -ignore runner-label: actionlint 1.7.7 ships a FIXED list of GitHub + # *hosted* runner images and flags anything newer as "unknown label". + # That's a different gap than the self-hosted labels above -- there's no + # config-file section for "GitHub-hosted images this actionlint release + # doesn't know yet" -- so this stays even with the config file wired in. + # Several repos legitimately target ubuntu-26.04 / ubuntu-26.04-arm, + # which that version has never heard of. Every such report is a false + # positive about the linter's age, not the workflow, and leaving it on + # lands this hook red on arrival. Drop this ignore once actionlint is + # new enough. + # + # SHELLCHECK_OPTS applies to the shellcheck actionlint runs over `run:` + # blocks. Without it that embedded pass reports at INFO while the + # standalone shellcheck hook below gates at warning -- so the same SC2086 + # would be ignored in a .sh file and blocking in a workflow. Same floor + # in both places. + - id: actionlint + name: actionlint (workflow syntax) + entry: env SHELLCHECK_OPTS=-Swarning actionlint -config-file .github/actionlint.yaml -ignore 'label ".+" is unknown' + language: system + files: ^\.github/workflows/.*\.ya?ml$ + pass_filenames: true + + # -S warning: block on warning/error, not info. The info tier (e.g. + # SC2015) fires on existing scripts and would land this red on arrival, + # which just teaches everyone to --no-verify. + # + # NOTE some repos have pre-existing warning-level findings in build/test + # scripts (cache-turbo ci/tests/unit/extract_shm.sh SC2043, + # http-sentinel fuzz/build.sh SC2010, coraza-nginx build.sh SC2089/2090). + # They do NOT block normal work, because a commit hook only ever sees the + # files being committed -- they surface on `pre-commit run --all-files`. + # They are real and worth fixing; they are deliberately NOT suppressed + # here, so touching one of those scripts makes you fix it. + - id: shellcheck + name: shellcheck (gate on >=warning) + entry: shellcheck -S warning + language: system + files: \.(sh|bash)$ + pass_filenames: true diff --git a/.yamllint b/.yamllint new file mode 100644 index 0000000..ee6e4c2 --- /dev/null +++ b/.yamllint @@ -0,0 +1,25 @@ +# yamllint config for nginx-skeleton-module -- used by ci/linter/lint-yaml.sh +# and by any editor integration, so both agree. +# +# GitHub Actions workflows are not hand-written prose YAML; three default rules +# fight their syntax rather than catching bugs: +# document-start a workflow never starts with ---, and adding one everywhere +# is churn with no reader benefit. +# truthy `on:` is the trigger key, not the boolean `on`. check-keys +# off keeps the rule live for VALUES (yes/no/on/off), which is +# where a real YAML 1.1 surprise bites. +# line-length inline `run:` shell and ${{ }} expressions legitimately run +# long. 120 as a WARNING: visible, not blocking. lint-yaml.sh +# deliberately does not pass --strict, so warnings do not fail +# the gate -- errors (real syntax/structure problems) do. +extends: default + +rules: + document-start: disable + truthy: + check-keys: false + comments: + min-spaces-from-content: 1 + line-length: + max: 120 + level: warning diff --git a/README.md b/README.md index 58e04c2..d8a5c47 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,12 @@ ci/ everything that only exists to test/build the module soak.sh sustained matching/benign storm under valgrind/ASan valgrind.supp nginx-core-only suppressions rename-module.sh skel -> your name (delete after use) -.github/workflows/ eight workflows, see below + linter/ local lint gate, mirrors the CI thresholds + PROMPT-standardize-module.md prompt: bring an existing module to this standard + run-all.sh every checker; what the pre-commit hook runs + install-linters.sh apt-get -> pipx -> cpan -> upstream binary +.githooks/pre-commit tracked commit gate (opt in: core.hooksPath) +.github/workflows/ nine workflows, see below ``` `ci/t/` uses the same `Test::Nginx::Socket` framework as upstream nginx's own @@ -75,13 +80,34 @@ That split is why `ci/fuzz/fuzz_scan.c` can link and drive the *real* code path rather than a reimplementation of it. A fuzzer that tests a copy of the parser tests nothing; it just drifts from production quietly and reports green. +## Linting + +`ci/linter/` is the local half of CI: nginx source conventions, C, shell, +Python, Perl and workflow YAML, at the same thresholds the remote scanners +gate on. It runs from a tracked pre-commit hook so a finding costs two seconds +locally instead of a red PR. + +```sh +ci/linter/install-linters.sh # apt-get, then pipx/cpan for what apt lacks +git config core.hooksPath .githooks +ci/linter/run-all.sh # or --staged, what the hook runs +``` + +Full setup (per-tool apt/pip/cpan commands, hook install, how the gate was +verified red, how to extend it): **[ci/linter/README.md](ci/linter/README.md)**. + +`.githooks/pre-commit` replaces the `pre-commit`-framework hook if you enable +it — `core.hooksPath` makes git ignore `.git/hooks/` entirely. Pick one; the +linter README explains what each covers. + ## CI -Eight workflows. A failure surfaces as a red run plus the uploaded artifact — no +Nine workflows. A failure surfaces as a red run plus the uploaded artifact — no chat notifications wired. | Workflow | Trigger | Gates | |---|---|---| +| `lint.yml` | PR (via `ci.yml`) | the `ci/linter/` gate: nginx conventions, shellcheck, ruff, perlcritic + `perl -c`, yamllint, actionlint, **zizmor** (workflow security) — hosted runner, no self-hosted slot | | `build-test.yml` | PR + push | shellcheck/cppcheck/actionlint, build, **.so dlopens**, **bad config is rejected**, **`-T` survives merged multi-context config**, `-Werror` strict compile, Test::Nginx, ASan+UBSan, **rename smoke** | | `asan.yml` | PR + push (path-gated: `src/` + build/soak harness) | 60s ASan/UBSan request-storm soak (static `--add-module`) | | `fuzzing.yml` | PR + push | replay every past crash, then 120s fresh fuzz | @@ -226,6 +252,8 @@ Build: `build-essential curl libpcre2-dev zlib1g-dev` Tests: `cpanminus` + `cpanm Test::Nginx` Fuzz: `clang` (needs `-fsanitize=fuzzer`) Soak: `valgrind` +Lint: `ci/linter/install-linters.sh` (shellcheck, cppcheck, flawfinder, +yamllint, clang-tidy, perlcritic, ruff, semgrep, actionlint, Test::Nginx) ## Contributing diff --git a/ci/PROMPT-standardize-module.md b/ci/PROMPT-standardize-module.md new file mode 100644 index 0000000..82da74f --- /dev/null +++ b/ci/PROMPT-standardize-module.md @@ -0,0 +1,478 @@ +# Prompt — bring an existing nginx module up to myguard skeleton standard + +Copy the whole file into a fresh session, replace `` with the module +path, and run it. It is written to be executed by an agent with repo write +access, but it reads as a checklist for a human too. + +The reference implementation is `/opt/myguard/labs/nginx-skeleton-module` +(this repo). **Read the reference before changing the target** — the point is +not to make the target look similar, it is to give it the same *gates*. + +--- + +## Context you are given + +- **Target module:** `` (e.g. `/opt/myguard/labs/nginx-http-shield-module`) +- **Reference:** `/opt/myguard/labs/nginx-skeleton-module` +- **Memory mirror:** `/opt/myguard/memory/labs//` — read + `index.md`, `issues.md`, `lessons.md` FIRST. A trap already recorded there + outranks anything you infer from the code. +- Owner is the `myguard-labs` org; verify with + `git -C remote get-url origin`, never from a list. + +## Ground rules + +1. **One PR per phase**, in the phase order below. Each PR is independently + revertible and independently green. Do not open the next phase's PR until + the previous is merged — later phases move files the earlier ones edit. +2. **Remote CI green before merge.** No `[skip ci]`, no disabled workflows. +3. **Never weaken a gate to make it pass.** If the target genuinely cannot meet + a threshold, say so in the PR body with the file:line that proves it, and + leave the gate at the honest value with a comment naming the reason. +4. **Every gate must be seen red once.** A check you never observed failing is + a check you have not verified. Record the probe you used, in a comment or + the PR body. This applies to coverage, to fuzz harnesses and to linters. +5. **Existing behaviour is not in scope.** You are moving CI, not rewriting the + module. If you find a real bug, file it in the memory mirror's `issues.md` + and keep going; fix it only if it blocks a gate. +6. Comments explain **why**, at the decision, in the target's existing voice. + A rule with no recorded reason gets deleted by the next person. + +--- + +## Phase 0 — inventory and baseline (no changes) + +Produce a short written baseline before touching anything: + +```bash +cd +ls -d t tests fuzz ci src scripts .githooks 2>/dev/null +ls .github/workflows +git log --oneline -10 +gh run list -R myguard-labs/ --limit 20 \ + --json name,conclusion,startedAt,updatedAt,workflowName +``` + +Record, in the memory mirror's `index.md`: + +- current layout (which of `t/`, `tests/`, `fuzz/`, `ci/` exist) +- current workflow list and which of the reference's nine are missing +- **measured wall-clock per workflow** from `gh run list` — you need real + numbers for the lane work in Phase 7. Estimates are not acceptable there. +- current coverage number, if any tooling exists (usually none) + +Layouts seen across the org, so you know what you are walking into: +`t/` + `fuzz/` at the root (most modules), `tests/` + `fuzz/` (autocert), +already-migrated `ci/` (cache-turbo, label-autoconf). + +--- + +## Phase 1 — move all CI material under `ci/` + +Target layout, matching the reference exactly: + +```text +ci/ + t/ Test::Nginx suite (was t/ or tests/) + fuzz/ libFuzzer targets, dict, corpus/, regressions/ + vendor/nginx-tests/ upstream suite submodule + tools/ ci-build.sh, soak.sh, valgrind.supp, helpers + linter/ local lint gate (Phase 6) +``` + +Rules: + +- `git mv`, never copy-then-delete — blame must survive. +- A directory move breaks **every relative path that climbs out of it**. After + the move, grep for and fix, in this order: `../` in C `#include`s, `$PWD`/ + `dirname` logic in shell, `paths:` filters in workflows, `hashFiles()` keys, + `prove` invocations, fuzz corpus paths, `.gitmodules` submodule paths, + `.gitignore`, coverage exclude patterns, README references. + A missed climb compiles fine and silently tests the wrong tree. +- `git submodule update --init` still working after moving + `ci/vendor/nginx-tests` is a required check — the `.gitmodules` `path:` must + be edited, not just the directory moved. +- Run the suite locally after the move and before the workflow edits, so a + failure is attributable to the move and not to both at once: + `TEST_NGINX_TIMEOUT=20 prove -v ci/t/` +- Leave a dated pointer line in the memory mirror if any note references an old + path. + +**Acceptance:** local `prove` green, fuzz targets still build +(`ci/fuzz/build.sh`), no path outside `ci/` refers to `t/`, `tests/` or `fuzz/`. + +--- + +## Phase 2 — workflows and badges + +Bring the target to the reference's **nine** workflows. Do not copy blindly: +each one carries reference-specific paths and pins that must be re-derived for +the target. + +| Workflow | What it must gate in the target | +|---|---| +| `ci.yml` | orchestrator; the ONLY `pull_request` entry point | +| `lint.yml` | the `ci/linter/` gate (Phase 6), hosted runner | +| `build-test.yml` | build, `.so` dlopens, bad config rejected, `-T` survives merged multi-context config, `-Werror`, Test::Nginx, ASan+UBSan | +| `asan.yml` | ASan/UBSan request-storm soak, static `--add-module` | +| `fuzzing.yml` | replay every past crash, then fresh fuzz | +| `valgrind.yml` | memcheck soak | +| `security-scanners.yml` | flawfinder ≥4 blocks, clang-tidy blocks, semgrep ≥WARNING | +| `codeql.yml` | CodeQL over the **module TU only** | +| `ci-deep.yml` | monthly: long fuzz, memcheck, helgrind, nginx mainline+stable+angie matrix | +| `bump.yml` | weekly pin bump + `ci/vendor/nginx-tests` submodule update | + +Also port, adapting paths: + +- `.github/versions.env` — single source of truth for version **and sha256** + pins. Tarballs verified by digest, not just version string. +- `.github/scripts/{load-versions,compute-versions,fetch-verify}.sh` +- `.github/actions/build-cache/` composite action +- `.github/actionlint.yaml` — declares the self-hosted runner labels, otherwise + actionlint flags every `runs-on` and the lint step becomes noise people skip. + +Members are `workflow_call:`-only; only `ci.yml` has `pull_request:`. Two entry +points run everything twice per PR and defeat the laning. + +**Badges:** README badge block must list the workflows in the SAME ORDER as the +`## CI` table in that README, and both must match reality — a badge for a +workflow that no longer exists renders a permanent grey "no status" and is +worse than no badge. Order used by the reference README: +Build&Test, Security Scanners, Fuzzing, Valgrind, CodeQL, A/UBSan, CI Deep — +insert Lint where the CI table puts it, and keep the two lists in lockstep. + +**Acceptance:** `actionlint` clean; every badge resolves to a real workflow +file; the CI table and the badge row are in identical order. + +--- + +## Phase 3 — coverage to the honest maximum + +No coverage tooling exists in the reference yet. Add it to the target as part +of this work, and (recommendation) upstream it back to the skeleton afterwards. + +Wiring: + +- Build the scan core with `--coverage` (`-fprofile-arcs -ftest-coverage`) in a + dedicated `coverage` mode in `ci/tools/ci-build.sh` — a new mode, not a flag + bolted onto `debug`, so the cache keys stay separable. +- Report with `gcovr` over `src/` only. Exclude `ci/`, the fuzz harnesses and + vendored code — a harness inflating the number is the classic self-deception. +- Count coverage from BOTH drivers: Test::Nginx (`ci/t/`) and the fuzz targets + replaying `ci/fuzz/corpus/` + `regressions/`. The scan core is reachable from + both; measuring one hides the other's gaps. +- Gate at a floor slightly under the achieved number (the reference's sibling + repos use "achieved minus a couple of points"), so noise does not flap the + gate but a real regression trips it. Raise the floor in the same PR that + raises coverage; never lower it to make a red run green. + +**"Without cheating" is the hard part. These are rejected outright:** + +- a test whose assertion holds in both the pass and the fail state + (tell: a captured variable that is never compared) +- a control that hardcodes the verdict instead of calling the real function +- asserting a *precondition* rather than the claim +- one shared counter asserted at N call sites — it pins none of them +- a test written from the same misunderstanding as the code +- excluding a hard file from the coverage config to lift the percentage +- tests that only execute lines without asserting on the result + +**Required per new test:** a negative control. Break the code the test claims +to guard (flip a comparison, delete the bound check, swap a constant), confirm +the test FAILS, restore. A test that passes against the mutated code guards +nothing. Note the mutation you used in the test's comment. + +Push toward the maximum by targeting, in order: error paths, allocation +failure, malformed/truncated input, boundary values at every `MAX_*` constant, +cross-buffer seams, and the branches your gcovr report shows as never taken. +100% is not a goal; every *reachable* branch having a meaningful assertion is. + +**Acceptance:** coverage job in CI, floor enforced, report uploaded as an +artifact, and each added test observed failing against a stated mutation. + +--- + +## Phase 4 — ASan and fuzzing, retargeted to this module + +Fuzzing is per-module work; a copied harness that drives the skeleton's rule +table proves nothing about the target. + +- The fuzz target must call the **real** decision function with + `(const uint8_t *, size_t)`, not a reimplementation. If the target module has + no such seam — decision logic entangled with `ngx_http_request_t` — extract + it first (the reference's "one structural rule"). That refactor is in scope + here; it is what makes everything else measurable. +- Seed corpus from the module's actual domain: real headers/bodies/config + values it parses, plus every past crash under `ci/fuzz/regressions/`. +- `fuzz.dict` with the module's real tokens — separators, markers, keywords. + A dictionary of the skeleton's tokens actively misdirects the fuzzer. +- Replay-then-fuzz order in `fuzzing.yml`: every recorded regression first + (fast, deterministic), then the time-boxed fresh run. A crash that returns + must fail in seconds, not after the fresh budget. +- ASan soak (`asan.yml`) must drive the module's real request shape — its + directives enabled, its body path exercised — not a default config where the + handler never runs. Verify by checking the soak actually reaches the module + (a counter, a log line, or coverage from the soak build). +- Keep the ASan build static (`--add-module`); a dynamic module under ASan + loses interception on the parts that matter. +- Adapt the neighbours as needed: `valgrind.supp` needs target-specific + nginx-core suppressions; `codeql.yml`'s TU filter needs the target's file + names; `ci-deep.yml`'s matrix needs the target's nginx/angie compatibility + range. + +**Acceptance:** fuzz target links against production code, replays all +regressions, and a deliberately reintroduced past bug is caught by the replay +step (verify once, then revert). + +--- + +## Phase 5 — caching, all layers + +Every build goes through `ci/tools/ci-build.sh` as the single chokepoint; no +workflow duplicates cache logic. `.github/actions/build-cache` restores caches +for one mode. Layers, cheapest first: + +| Layer | Saves | Keyed on | +|---|---|---| +| **apt / packages** | package install per job | package set hash; on self-hosted, prefer a pre-baked runner image over re-installing | +| **ccache** | recompilation | content (`CCACHE_COMPILERCHECK=content`) | +| **mold** | link time | used when present; **skipped under ASan** | +| **eatmydata** | `configure` + apt fsync stalls | wrap the configure/install steps; never wrap something whose durability matters | +| **build tree** (`.build/nginx--`) | `./configure` | mode + version + `hashFiles(ci-build.sh, config, src/**)` | +| **source tarball** | the download | version (+ sha256 verified after restore) | + +Rules that are load-bearing: + +- nginx's `configure` **ignores a bare `CC=`** — ccache must be wired through + the configure argument the reference uses, not via env. +- ccache may use a `restore-keys` fallback ladder (content-hashed, a partial + hit cannot serve a wrong object). The **build-tree cache must stay + exact-match only** — do not "fix" that for consistency. +- Hybrid restore (on-disk warm dirs + `actions/cache` fallback) stays. Deleting + the fallback because the runners are persistent is how this silently degrades + the day they become ephemeral. +- GitHub scopes caches **by ref**: a PR run writes `refs/pull/N/merge` and + cannot read a branch's entries. A cold PR run is not a bug. +- A cache must never be able to serve a stale artifact into a green result. If + a key cannot express what invalidates it, do not cache that layer. +- State the honest win in the README. If caching saves 5s on a 2.5-minute gate, + say so — the reason to keep it is the heavier module that comes later. + +--- + +## Phase 6 — pre-commit linters + +Port `ci/linter/` from the reference and follow its README verbatim: +**[ci/linter/README.md](linter/README.md)** — per-tool `apt-get` (preferred), +then `pipx` for what Debian lacks, then `cpan` for Perl modules, then upstream +binary for actionlint. `ci/linter/install-linters.sh` is the single installer; +CI and a fresh clone use the same one. + +- Tracked hook at `.githooks/pre-commit`, enabled with + `git config core.hooksPath .githooks`. It lints STAGED files only. +- Checkers: C (flawfinder ≥4, cppcheck, semgrep ≥WARNING), nginx conventions + (libc vs `ngx_*`, tabs, 80 cols, include order), shell (shellcheck + `-S warning`), Python (ruff), Perl (`perl -c` + perlcritic ≥4), YAML + (yamllint + actionlint). +- Thresholds **mirror `security-scanners.yml`**. Move one there, move it here + in the same commit, or local-green stops predicting remote-green. +- A missing tool exits 2 and BLOCKS. Never a silent skip. +- Relaxations live in `.yamllint` / `.perlcriticrc` at the repo root, each with + its reason. If the target has pre-existing warning-level findings, fix them + or record why — do not add a blanket suppression. +- `lint.yml` runs the same `run-all.sh` on a hosted runner, so a clone that + never enabled the hook still cannot land a regression. + +### Speed budget: the whole hook under ~2s on a one-file commit + +A commit gate people wait on is a commit gate people bypass with `--no-verify`. +Measure it (`time ci/linter/run-all.sh `), and if it is over budget +the fix is scoping the slow checker — **never** dropping one, and never a +default-on skip flag. + +Carry these three from the reference; each was measured, not assumed: + +- **`semgrep --metrics=off`.** The end-of-scan telemetry POST to semgrep.dev + was 2.76s of a 2.76s scan; without it, 1.27s. More than half the gate was + upload. +- **`semgrep --jobs=1`** — a *correctness* flag, not a speed one. semgrep-core + defaults to one OCaml domain per core and each domain opens its own io_uring + ring against the host's `RLIMIT_MEMLOCK` (8 MB on builder02, shared with + every other job). When the runners are busy it exhausts and semgrep-core + aborts with `Unix_error: Cannot allocate memory io_uring_queue_init`, exit 2 + — a red gate caused by a *neighbouring* job, on a scan of three files where + the parallelism bought nothing. Reproduced 3/3 busy, 0/3 idle, so an idle-box + green tells you nothing here. The same flags go in `security-scanners.yml`: + same host, same crash, and the two must stay in sync anyway. +- **`run-all.sh` fans the checkers out** (`LINT_JOBS`, default one slot per + checker, `LINT_JOBS=1` to bisect a hang). Two requirements that are easy to + get wrong: + - Buffer each checker's output to its own file and replay it whole, in fixed + glob order — **never stream them interleaved.** Findings carry a + `file:line` but not a checker name, so interleaved output cannot be + attributed, which is the entire reason to buffer. Fixed order also makes + two runs of the same dirty tree byte-comparable. + - Each child writes its exit status to a file. The reaping `wait` is + collective, so per-child statuses are not otherwise recoverable, and a + **missing** status file (child SIGKILLed) must count as a failure, never as + a pass. + +Reference measurements, whole checkout: 3.8s → 1.45s, one C file 2.9s → 1.31s. +Yours will differ; record yours — **and check `/proc/loadavg` before you take +them.** The build host also runs the self-hosted CI slots; at load ~50 the same +full-tree run varied 2.2s–12.4s over six back-to-back attempts, a spread wider +than the whole improvement. A busy-box A/B measures the neighbouring jobs. This +is the same shared-resource contention as the semgrep memlock crash above, and +it is why every number in this phase has to say what the box was doing. + +**Acceptance:** run every probe in the linter README's "Verify before trusting" +section against the target and observe each one red — *after* the speed work, +not before. `--jobs`/`--metrics` are exactly the kind of flag that can silently +turn a checker into a no-op, so the semgrep probe in particular must still +fire (`insecure-use-string-copy-fn` on the malloc/strcpy file). Then run +`run-all.sh` with two different checkers failing at once and confirm both +appear in the output and both are named in the `== FAIL:` line. + +--- + +## Phase 7 — runner topology: lanes, at most four + +The reference lanes the suite so peak self-hosted use stays bounded — CI +wall-clock on builder02 is dominated by jobs QUEUEING for a label-matching +slot, not by the jobs themselves. Ten simultaneous requests just means the tail +waits. + +**Measure first, and measure the target, not the reference.** Take real per-job +durations from a recent green run: + +```sh +gh run list -R myguard-labs/ --workflow=ci.yml --limit 5 \ + --json databaseId,headSha,conclusion +gh run view -R myguard-labs/ --json jobs \ + -q '.jobs[] | [.name, .conclusion, + (((.completedAt|fromdate)-(.startedAt|fromdate))|tostring)+"s", + .startedAt, .completedAt] | @tsv' +``` + +Keep `startedAt`/`completedAt`, not just the durations — the gaps are what show +you queueing, and which lane is actually the critical path. + +Then: + +1. Identify the longest single **job**. That is the budget: no arrangement can + finish sooner. Chain **nothing** behind it — every second added there is a + second on the suite's critical path, whereas the same job appended to a + shorter lane is free. Pairing the longest job with a follow-up "to keep the + lane busy" is the single most common way this gets worse; it is what put the + reference's lane A at 348s against a 268s budget. +2. Build the **fewest lanes that fit**, four maximum, each a chain of `needs:` + where a long job releases its slot to a shorter, independent follow-up. No + lane's total may exceed the budget. Three lanes that all fit beat four that + also fit — fewer chains, fewer `!cancelled()` edges to reason about. Note + the headroom of the fullest lane in the comment, so the next person knows + how much a job can grow before the shape stops holding. +3. If it does not fit in four, the honest fixes are: move a check out-of-band + (monthly), time-box it, or put it on a hosted runner — not "add a fifth". +4. **A lane is not a slot.** Count the target's real slots + (`systemctl list-units | grep ci-ephemeral` on the runner host — six on + builder02), and remember a reusable workflow can fan out into many + concurrent jobs: the reference's Build&Test is *five*, so the observed peak + is 7 against 6 slots. Brief oversubscription at t=0 is acceptable; writing + "caps peak runner use at three" when it is seven is not. +5. Hosted jobs (lint, CodeQL) take no self-hosted slot, so they are **not laned + at all** — no `needs:`, start immediately, fastest feedback. Chaining a + hosted job behind a self-hosted one to "conserve a slot" conserves nothing + and just delays its result. +6. Follow-ups use `if: ${{ !cancelled() }}` so a failing first check does not + suppress an unrelated second one. A red ASan should still tell you whether + Valgrind is clean. It is also what keeps a chain alive when an earlier job + is *skipped* by a changed-files gate, which a bare `needs:` would not. +7. Concurrency groups must not collide. A called workflow inherits the caller's + `github.workflow`/`github.ref`, so an identical group string makes a member + cancel its own caller and a whole lane dies before it starts. Prefix the + orchestrator's group distinctly. +8. Path-gating a reusable workflow does not work — a called workflow cannot + filter its own triggering. Gates move to a `changes` job in the orchestrator + with an explicit job-level `if`. That diff job must **fail loudly** on an + unusable diff, never fall through to "no relevant changes" — failing open + skips the sanitizer on exactly the PRs that need it. + +The orchestrator's header comment is the only place this design is written +down, so it is part of the deliverable, not documentation of it. It must carry +the lane map, the measured durations, the run ID and date they came from, and +the command above to re-derive them. **Any lane change rewrites that comment in +the same commit** — a stale lane map reads as measurement and gets trusted. +Also record the lane map and timings in the memory mirror. + +--- + +## Phase 8 — self-hosted runner exposure + +Applies whenever `runs-on` includes `self-hosted`. A self-hosted runner +executing untrusted code is arbitrary code execution on the build host. + +Required: + +- **Fork routing.** Every self-hosted job uses the reference's expression: + `runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || fromJSON('["self-hosted","builder02","lxc"]') }}` + A fork PR never reaches the build host. +- **No `pull_request_target`**, ever, in a repo with self-hosted runners. It + runs with a writable token in the base-repo context; combined with a fork's + code it is a full compromise. If something seems to need it, it does not. +- **Least-privilege tokens.** `permissions: contents: read` at workflow level; + widen per-job only where genuinely needed (`security-events: write` for + CodeQL). Never `write-all`. +- `persist-credentials: false` on every checkout, so a later step cannot reuse + the token. +- **Pin every third-party action to a full commit SHA**, with the version in a + trailing comment. A tag is mutable. +- Pin every downloaded tool version (semgrep, actionlint, nginx tarballs) and + verify tarballs by sha256. This is code executing on a persistent host. +- Never expose secrets to a job that can run untrusted code. Prefer no secrets + at all in the PR lane; `bump.yml`-style writers run only from the default + branch on a schedule. +- Repo settings (check with `gh api`, fix or report): require approval for + first-time-contributor workflow runs, restrict which actions may run, + branch protection with required checks, and no self-hosted runner registered + at org level where a public repo can grab it. +- Runner containers are LXC/incus and persistent: assume a job can see the + previous job's leftovers. Nothing sensitive may be left in `$HOME` or the + work dir, and cleanup must not depend on a job succeeding. + +- **`zizmor --persona=pedantic --offline`** over `.github/workflows/`, already + wired into the reference's `ci/linter/lint-yaml.sh` and therefore into both + the hook and `lint.yml`. It mechanises most of this section: template + injection, dangerous triggers, `artipacked`, `unpinned-uses`, + `excessive-permissions`, plus a `self-hosted-runner` audit. Port it with the + rest of the linter dir; expect the target to be red on first run and fix + each finding rather than ignoring it. `# zizmor: ignore[rule]` at the line, + with a reason, is the only acceptable suppression. +- `${{ }}` interpolation of any attacker-controlled field (PR title, branch + name, body) directly into a `run:` block is template injection. Pass through + `env:` and quote. The same shape is required for `matrix.*` even though it is + repo-controlled — the safe form costs nothing and stops the unsafe one being + copied somewhere it matters. + +--- + +## Finishing + +- README rewritten, not appended to: badge row, `## CI` table, layout tree, + Requirements, and a Linting section linking `ci/linter/README.md`. +- `CONTRIBUTING.md` tells a contributor how to enable the hook. +- `CHANGES` entry describing the standardisation. +- Memory mirror updated: `index.md` (layout, lane map, measured times), + `issues.md` (anything found and not fixed), `lessons.md` (every trap that + cost you a red CI round-trip — `[RECURRING]` if it has bitten before). +- Any trap that is a *class* rather than a typo goes into the matching + `.claude/skills/audit-*/` reference, not only into memory. The skill is what + runs unprompted next time. +- Improvements you made that the skeleton lacks (coverage tooling, eatmydata) + get a PR back to `nginx-skeleton-module`. The template is only + worth keeping if it stays ahead of its clones. + +## Report back + +State plainly, per phase: what landed, what is red, what you left undone and +why. Include the measured before/after wall-clock and the coverage +before/after. Do not report a phase complete on a gate you never saw fail. diff --git a/ci/linter/README.md b/ci/linter/README.md new file mode 100644 index 0000000..0b7fd5a --- /dev/null +++ b/ci/linter/README.md @@ -0,0 +1,289 @@ +# ci/linter — local lint gate + +Mirrors the cheap half of remote CI so a push does not burn a round-trip on a +finding shellcheck could have named in two seconds. Every script is standalone; +`run-all.sh` runs them all and `.githooks/pre-commit` runs `run-all.sh --staged`. + +## Layout + +| Script | Covers | Gate | +|---|---|---| +| `lint-c.sh` | `src/*.[ch]` | flawfinder ≥4, cppcheck (warning/performance/portability), semgrep ≥WARNING (`p/c`, `p/security-audit`) | +| `lint-nginx.sh` | `src/*.[ch]` | nginx conventions: libc alloc/str/num/io instead of `ngx_*`, hard tabs, >80 columns, trailing whitespace, `ngx_config.h` include order | +| `lint-sh.sh` | `*.sh`, `*.bash`, `.githooks/*` | shellcheck `-S warning` | +| `lint-python.sh` | `*.py` | `ruff check` + `ruff format --check` | +| `lint-perl.sh` | `ci/t/*.t`, `*.pl`, `*.pm` | `perl -c` + perlcritic severity ≥4 | +| `lint-yaml.sh` | `*.yml`, `*.yaml` | yamllint (errors block, warnings visible), actionlint + zizmor (`--persona=pedantic`) on `.github/workflows/` | +| `run-all.sh` | all of the above | runs every check, reports once | +| `install-linters.sh` | — | apt-get → pipx → cpan → upstream binary | +| `lib.sh` | — | sourced helpers (file selection, missing-tool failure) | + +Rule config lives at the repo root so editors and these scripts agree: +`.yamllint` (workflow-shaped YAML), `.perlcriticrc` (Test::Nginx-shaped Perl). +Both carry the reason for every relaxation; read them before adding another. + +Thresholds deliberately match `.github/workflows/security-scanners.yml`. Move +one there and move it here **in the same commit**, or local-green stops +predicting remote-green — the only reason this directory exists. + +`clang-tidy` is **CI-only**: it needs `ngx_auto_config.h`, which exists only in +a configured nginx tree. A hook cannot assume one, and a check that skips +itself when the tree is missing is a vacuous gate. + +## 1. Install the linters + +```sh +ci/linter/install-linters.sh # install what is missing +ci/linter/install-linters.sh --check # report only +``` + +Preference order, and why each tool lands where it does: + +**apt-get (preferred — distro-managed, no PEP 668 fight)** + +```sh +sudo apt-get update +sudo apt-get install -y --no-install-recommends \ + shellcheck cppcheck flawfinder yamllint clang-tidy \ + libperl-critic-perl perl pipx cpanminus +``` + +**pip / pipx (Python tools Debian does not carry at the needed version)** + +Use `pipx`, not `pip3`: Debian 12+ marks the system interpreter +externally-managed, so a bare `pip3 install` fails and +`--break-system-packages` is a worse answer than a venv per tool. + +```sh +pipx install 'ruff==0.16.1' # pinned to the CI version on purpose +pipx install 'semgrep==1.169.0' # pinned to the CI version on purpose +``` + +`ruff` and `semgrep` are pinned because an unpinned upgrade changes findings +under you and local stops matching CI. Bump each here and in its CI consumer +together -- `ruff` in `install-linters.sh`, `semgrep` in +`security-scanners.yml` too. + +**cpan (Perl modules apt does not carry on every target release)** + +```sh +sudo cpanm --notest Test::Nginx::Socket # also what makes `perl -c` work on ci/t/*.t +sudo cpanm --notest Perl::Critic # only if libperl-critic-perl was unavailable +``` + +`--notest`: Test::Nginx's own suite wants a live nginx and a free port, which +an install step has no business demanding. + +```sh +pipx install zizmor # GitHub Actions security audit +``` + +`zizmor` is deliberately **not** pinned, unlike ruff and semgrep: its rule set +is the whole point, and a frozen security scanner stops finding what it was +added for. A new rule going red is a finding to triage, not drift to suppress. + +**upstream binary (no apt/pip/cpan source)** + +```sh +ver=1.7.7 +sha=023070a287cd8cccd71515fedc843f1985bf96c436b7effaecce67290e7e0757 +curl -fsSL -o actionlint.tgz \ + "https://github.com/rhysd/actionlint/releases/download/v${ver}/actionlint_${ver}_linux_amd64.tar.gz" +echo "$sha actionlint.tgz" | sha256sum -c - # must pass before the next line +tar -xzf actionlint.tgz actionlint && sudo install -m0755 actionlint /usr/local/bin/ +``` + +Do not pipe the tarball straight into `tar`: that installs whatever the network +returned. The digest is from the release's `actionlint_${ver}_checksums.txt` and +is pinned beside the version in `install-linters.sh` too — bump both together. + +Make sure `~/.local/bin` is on `PATH` for the pipx-installed tools. + +## 2. Enable the pre-commit hook + +The hook is tracked at `.githooks/pre-commit` so a change to the gate arrives +as a reviewable diff. Git does not use it until you point `core.hooksPath` at +the directory — once per clone: + +```sh +git config core.hooksPath .githooks +``` + +Verify it is live: + +```sh +git config --get core.hooksPath # -> .githooks +``` + +Bypass in an emergency with `git commit --no-verify`. + +**This replaces the `pre-commit` framework hook.** `core.hooksPath` makes git +ignore `.git/hooks/` entirely, including the hook `pre-commit install` writes +there. The repo's `.pre-commit-config.yaml` still exists and covers overlapping +ground (whitespace fixers, private-key detection, flawfinder, semgrep, +shellcheck, actionlint). Pick one: + +- `git config core.hooksPath .githooks` — this directory: also covers Perl, + Python and the nginx conventions, no Python framework needed. +- `pipx install pre-commit && pre-commit install` — the framework: also runs + the whitespace/EOF fixers and `detect-private-key`, but not `lint-nginx.sh` + or `lint-perl.sh`. Then leave `core.hooksPath` unset. + +Running both means running flawfinder and semgrep twice per commit. + +## 3. Use it + +```sh +ci/linter/run-all.sh # every tracked file +ci/linter/run-all.sh --staged # what the hook runs +ci/linter/run-all.sh src/foo.c # named files +LINT_ONLY="c nginx" ci/linter/run-all.sh +LINT_SKIP_SEMGREP=1 ci/linter/run-all.sh # loud opt-out of the slowest pass +LINT_JOBS=1 ci/linter/run-all.sh # serial, for bisecting a hang +ci/linter/run-all.sh --list +``` + +Exit codes: `0` clean, `1` findings, `2` a linter is missing. + +### Speed, and why it is shaped this way + +Measured 2026-07-31 on builder02 (i9-14900HX, 32 threads) with **no CI job +running** — see the caveat below before comparing against your own numbers: + +| | before | after | +|---|---|---| +| full tree | 3.8s | **1.45s** | +| one C file | 2.9s | **1.31s** | +| full tree, `LINT_JOBS=1` | 3.2s | 2.57s | + +Re-measure on an idle box or not at all. This host also runs six self-hosted CI +runner slots, and at load average ~50 the same full-tree run took 2.2s to 12.4s +across six back-to-back attempts — the run-to-run spread is wider than the +entire improvement, so a busy-box A/B measures the neighbours, not the change. +Check `/proc/loadavg` first. + +Two changes, only one of which is really about speed: + +- **`semgrep --metrics=off`.** The end-of-scan POST to semgrep.dev was 2.76s of + a 2.76s scan; without it the same scan is 1.27s. More than half the hook's + wall clock was telemetry. +- **`semgrep --jobs=1`.** A *correctness* fix. semgrep-core defaults to one + OCaml domain per core and each domain opens its own io_uring ring against + this host's 8 MB `RLIMIT_MEMLOCK`, which is shared with the self-hosted CI + runners. When the runners are busy it exhausts and semgrep-core aborts with + `Unix_error: Cannot allocate memory io_uring_queue_init`, exit 2 — a red + commit gate caused by neighbouring load, not by the diff. Reproduced 3/3 on a + busy box, 0/3 on an idle one. `src/` is three files, so nothing was gained by + the parallelism in the first place. `security-scanners.yml` carries the same + two flags; they run on the same host and must stay in sync. +- **`run-all.sh` fans the checkers out** (`LINT_JOBS`, default one slot per + checker). Each checker's output is buffered and replayed whole in glob order, + never streamed: findings carry a `file:line` but not a checker name, so + interleaved output is unattributable. Fixed order also keeps two runs of the + same dirty tree byte-comparable. + +The floor is now semgrep's own startup. If this creeps back over ~2s, scope +semgrep — do not drop a checker. + +Suppress one justified `lint-nginx.sh` finding with a trailing +`/* NOLINT-nginx */` on that line. Whole-rule suppression is deliberately not +supported: the exception belongs next to the code that needs it, where review +can see the reason. + +## Verify before trusting + +A green gate proves nothing until it has been seen red. Every probe below was +run against this tree and observed failing; re-run them after changing a +threshold. + +```sh +# shell: SC2164 + SC2115 -> exit 1 +printf '#!/bin/bash\ncd /tmp/x\nrm -rf "${B}/"*\n' > _p.sh +LINT_ONLY=sh ci/linter/run-all.sh _p.sh ; rm _p.sh + +# C + nginx conventions: malloc/strcpy -> exit 1 from both lint-c and lint-nginx +printf '#include \nvoid f(void){char*p=malloc(4);strcpy(p,"ab");}\n' > src/_probe.c +LINT_ONLY="c nginx" ci/linter/run-all.sh src/_probe.c ; rm src/_probe.c + +# python: unused import -> exit 1 +printf 'import os\nx=1\n' > _p.py +LINT_ONLY=python ci/linter/run-all.sh _p.py ; rm _p.py + +# perl: string eval + interpolated system() -> exit 1 +printf 'my $x = 1;\nsystem("ls $x");\neval "1";\n' > _p.pl +LINT_ONLY=perl ci/linter/run-all.sh _p.pl ; rm _p.pl + +# yaml: unterminated flow sequence -> exit 1 +printf 'a: [1,\n' > _p.yml +LINT_ONLY=yaml ci/linter/run-all.sh _p.yml ; rm _p.yml + +# workflow security: zizmor -> exit 1 on template-injection, artipacked, +# unpinned-uses and excessive-permissions, all from these seven lines +cat > .github/workflows/_probe.yml <<'EOF' +name: probe +on: [pull_request] +jobs: + p: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - run: echo "${{ github.event.pull_request.title }}" +EOF +LINT_ONLY=yaml ci/linter/run-all.sh .github/workflows/_probe.yml +rm .github/workflows/_probe.yml + +# missing tool -> exit 2, never a silent skip +printf 'x = 1\n' > _p.py +PATH="$(echo "$PATH" | tr : '\n' | grep -v "$HOME/.local/bin" | paste -sd:)" \ + LINT_ONLY=python ci/linter/run-all.sh _p.py ; rm _p.py + +# the hook itself blocks the commit +printf '#!/bin/bash\ncd /tmp/x\n' > _bad.sh && git add _bad.sh +git commit -m probe # -> blocked +git reset -q HEAD _bad.sh && rm _bad.sh +``` + +Note SC2086 is INFO and correctly does **not** trip `lint-sh.sh`; probe with a +warning-severity finding or you will misread the gate. + +### Workflow security (zizmor) + +`actionlint` reads a workflow as syntax; `zizmor` reads it as an attack +surface — template injection into `run:`, `pull_request_target`, credentials +persisted by `actions/checkout`, actions pinned to a mutable tag, over-broad +`permissions:`. On a repo with **self-hosted runners** that class of mistake is +arbitrary code execution on the build host, which is why it is a gate and not +advice. + +Run at `--persona=pedantic`: the default persona already passes on this tree, +so gating on it could never go red. Pedantic is what caught the `matrix.*` +interpolations in `ci-deep.yml` (now passed through `env:`) and the +undocumented CodeQL permissions. + +`--offline`, so a commit hook never needs a token. The online audits only add +repo-settings context; that belongs in a periodic review. + +Inapplicable finding → `# zizmor: ignore[rule]` on the line, with the reason. +Never a blanket disable in a `zizmor.yml`. + +## In CI + +`.github/workflows/lint.yml` runs `install-linters.sh` then +`LINT_ONLY="nginx sh python perl yaml" run-all.sh` — the same entry point as +the hook, so a clone that never enabled `core.hooksPath` still cannot land a +regression. It is wired into the `ci.yml` orchestrator and runs on +`ubuntu-latest`, taking no self-hosted slot. + +The `c` checker is left out there because `security-scanners.yml` already runs +flawfinder/clang-tidy/semgrep over `src/` at the same thresholds. That is also +why `lint-c.sh` must be edited in the same commit as that workflow. + +## Extending + +- New file type: drop a `ci/linter/lint-.sh` in place — `run-all.sh` + picks it up by glob. Keep "no files of this kind" exiting 0, and fail with + exit 2 (via `need`) when the tool is absent. +- New nginx convention: one more `rule ` call in + `lint-nginx.sh`. +- New dependency: add it to `install-linters.sh` **and** to the apt/pip/cpan + lists above, so a fresh clone is one command from armed. diff --git a/ci/linter/install-linters.sh b/ci/linter/install-linters.sh new file mode 100755 index 0000000..645c42e --- /dev/null +++ b/ci/linter/install-linters.sh @@ -0,0 +1,166 @@ +#!/usr/bin/env bash +# ci/linter/install-linters.sh -- install every linter ci/linter/ needs. +# +# Order of preference, per tool: apt-get (distro-managed, no PEP 668 fight) +# -> pipx/pip (Python tools apt does not carry) -> cpan (Perl modules apt does +# not carry) -> upstream binary (actionlint). Nothing here is a silent skip: +# a tool that fails to install prints why and the script exits non-zero, so a +# half-installed toolchain cannot masquerade as a working gate. +# +# Usage: +# ci/linter/install-linters.sh install what is missing +# ci/linter/install-linters.sh --check report only, install nothing +# ci/linter/install-linters.sh --force reinstall even if present +# +# Needs sudo for the apt-get and cpan steps; the pipx step installs into +# ~/.local/bin (make sure it is on PATH). +# +# Side effects: apt-get install, pipx install, cpanm install, and one curl of +# the actionlint release tarball into /usr/local/bin. +# +# Extend: add a line to the APT/PIPX/CPAN lists. Anything needing a bespoke +# install gets its own function at the bottom, next to install_actionlint. + +set -uo pipefail + +CHECK=0 FORCE=0 +case "${1:-}" in + --check) CHECK=1 ;; + --force) FORCE=1 ;; + -h|--help) sed -n '2,25p' "$0"; exit 0 ;; + "") ;; + *) echo "unknown argument: $1" >&2; exit 2 ;; +esac + +SUDO="" +[ "$(id -u)" -eq 0 ] || SUDO="sudo" + +# toolapt package -- checked with command -v +APT_TOOLS=( + "shellcheck:shellcheck" # sh/bash + "cppcheck:cppcheck" # C + "flawfinder:flawfinder" # C, risky-API scan + "yamllint:yamllint" # YAML + "clang-tidy:clang-tidy" # C, CI-only (needs a configured nginx tree) + "perlcritic:libperl-critic-perl" # Perl test suite + "perl:perl" +) +# Not in Debian/Ubuntu at the versions this repo targets -> Python packaging. +# pipx, not pip: PEP 668 marks the system interpreter externally-managed, so a +# bare `pip3 install` fails on Debian 12+ and `--break-system-packages` is a +# worse answer than an isolated venv per tool. +PIPX_TOOLS=( + "ruff:ruff==0.16.1" # Python lint + format check, pinned: + # an unpinned ruff changes findings under + # you and local stops matching CI, same + # reasoning as the semgrep pin below. + "zizmor:zizmor" # GitHub Actions security audit. Not + # pinned: its rule set is the point, and a + # frozen security scanner stops finding + # what it was added for. A new rule going + # red is a finding to triage, not drift. + "semgrep:semgrep==1.169.0" # C, pinned to the CI version on purpose: + # an unpinned semgrep changes findings + # under you and local stops matching CI. +) +# apt has no libtest-nginx-perl on every target release; cpan always does. +CPAN_MODULES=( + "Test::Nginx::Socket" # the ci/t/*.t suite; also makes `perl -c` work + "Perl::Critic" # fallback if libperl-critic-perl was missing +) + +have() { command -v "$1" >/dev/null 2>&1; } +step() { printf '\n== %s\n' "$*"; } +rc=0 + +report() { + printf '%-14s %s\n' "$1" "$(command -v "$1" 2>/dev/null || echo MISSING)" +} + +if [ "$CHECK" -eq 1 ]; then + step "linter status" + for e in "${APT_TOOLS[@]}" "${PIPX_TOOLS[@]}"; do report "${e%%:*}"; done + report actionlint + report zizmor + perl -MTest::Nginx::Socket -e1 2>/dev/null \ + && printf '%-14s ok\n' 'Test::Nginx' \ + || { printf '%-14s MISSING\n' 'Test::Nginx'; rc=1; } + exit "$rc" +fi + +step "apt-get" +NEED=() +for e in "${APT_TOOLS[@]}"; do + tool="${e%%:*}"; pkg="${e##*:}" + if [ "$FORCE" -eq 1 ] || ! have "$tool"; then NEED+=("$pkg"); fi +done +if [ "${#NEED[@]}" -gt 0 ]; then + echo "installing: ${NEED[*]}" + $SUDO apt-get update -qq || rc=1 + $SUDO apt-get install -y --no-install-recommends "${NEED[@]}" || rc=1 +else + echo "nothing to do" +fi + +step "pipx" +have pipx || $SUDO apt-get install -y --no-install-recommends pipx || rc=1 +for e in "${PIPX_TOOLS[@]}"; do + tool="${e%%:*}"; spec="${e#*:}" + if [ "$FORCE" -eq 1 ] || ! have "$tool"; then + echo "installing: $spec" + pipx install "$spec" || pipx install --force "$spec" || rc=1 + fi +done + +step "cpan" +have cpanm || $SUDO apt-get install -y --no-install-recommends cpanminus || rc=1 +for m in "${CPAN_MODULES[@]}"; do + if [ "$FORCE" -eq 1 ] || ! perl -M"$m" -e1 2>/dev/null; then + echo "installing: $m" + # --notest: Test::Nginx's own suite wants a live nginx binary and a + # free port, which is not something an install step should demand. + $SUDO cpanm --notest "$m" || rc=1 + fi +done + +install_actionlint() { + # No apt package on the target releases and no pip/cpan equivalent: a Go + # binary from the upstream release, pinned by version AND sha256. + # + # The digest is COMPARED, not printed. Printing `sha256sum` and moving on + # reads as a verification step but installs whatever arrived -- worse than + # no check, because it stops anyone from adding a real one. Version and + # digest sit on adjacent lines for the same reason .github/versions.env + # does it: a version must not be able to move while its digest stays behind. + # From the upstream release's actionlint__checksums.txt. + local ver="1.7.7" + local sha="023070a287cd8cccd71515fedc843f1985bf96c436b7effaecce67290e7e0757" + local tmp + tmp="$(mktemp -d)" + curl -fsSL -o "$tmp/al.tgz" \ + "https://github.com/rhysd/actionlint/releases/download/v${ver}/actionlint_${ver}_linux_amd64.tar.gz" || return 1 + if ! printf '%s %s\n' "$sha" "$tmp/al.tgz" | sha256sum -c - >/dev/null 2>&1; then + echo "actionlint ${ver}: sha256 mismatch" >&2 + echo " expected: $sha" >&2 + echo " got: $(sha256sum < "$tmp/al.tgz" | cut -d' ' -f1)" >&2 + rm -rf "$tmp" + return 1 + fi + tar -xzf "$tmp/al.tgz" -C "$tmp" actionlint || return 1 + $SUDO install -m0755 "$tmp/actionlint" /usr/local/bin/actionlint || return 1 + rm -rf "$tmp" +} + +step "actionlint" +if [ "$FORCE" -eq 1 ] || ! have actionlint; then + install_actionlint || { echo "actionlint install failed" >&2; rc=1; } +else + echo "present: $(command -v actionlint)" +fi + +step "result" +if [ "$rc" -ne 0 ]; then + echo "one or more installs FAILED -- ci/linter/ is not fully armed" >&2 + exit 1 +fi +echo "all linters installed; verify with: ci/linter/install-linters.sh --check" diff --git a/ci/linter/lib.sh b/ci/linter/lib.sh new file mode 100755 index 0000000..5d64ad9 --- /dev/null +++ b/ci/linter/lib.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash +# ci/linter/lib.sh -- shared helpers for the ci/linter/lint-*.sh scripts. +# +# Sourced, never executed. Provides: +# repo_root absolute path of the checkout +# lint_files emit the file list to lint, one per line, NUL-safe +# callers use `mapfile -t`. Honours LINT_MODE: +# staged (default in the git hook) -- staged files only +# all -- every tracked file +# and an explicit file list passed in "$@" by run-all.sh. +# need hard-fail with an install hint when a linter is absent +# say / warn / die consistent output +# +# Missing tools are a HARD FAILURE, never a silent skip: a gate that quietly +# disappears when its tool is uninstalled reports green while checking nothing. +# Run ci/linter/install-linters.sh to get them all. + +set -euo pipefail + +repo_root() { git rev-parse --show-toplevel; } + +say() { printf ' %s\n' "$*"; } +warn() { printf ' WARN: %s\n' "$*" >&2; } +die() { printf 'ERROR: %s\n' "$*" >&2; exit 2; } + +need() { + command -v "$1" >/dev/null 2>&1 && return 0 + die "$1 not found. Install it: $2 (or run ci/linter/install-linters.sh)" +} + +# Paths never linted: byte-exact fuzz inputs, vendored trees, build output. +LINT_EXCLUDE_RE='^ci/fuzz/corpus/|^ci/fuzz/regressions/|^ci/vendor/|(^|/)objs/|(^|/)\.build/|(^|/)node_modules/|(^|/)\.venv/' + +# lint_files [explicit files...] +lint_files() { + local match_re="$1"; shift + local list + if [ "$#" -gt 0 ]; then + list=$(printf '%s\n' "$@") + elif [ "${LINT_MODE:-all}" = "staged" ]; then + list=$(git diff --cached --name-only --diff-filter=ACMR) || die "git diff --cached failed" + else + list=$(git ls-files) || die "git ls-files failed" + fi + printf '%s\n' "$list" \ + | grep -Ev "$LINT_EXCLUDE_RE" \ + | grep -E "$match_re" \ + | while read -r f; do [ -f "$f" ] && printf '%s\n' "$f"; done \ + || true +} diff --git a/ci/linter/lint-c.sh b/ci/linter/lint-c.sh new file mode 100755 index 0000000..103ee8c --- /dev/null +++ b/ci/linter/lint-c.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# ci/linter/lint-c.sh -- C static analysis for src/*.[ch]. +# +# Mirrors .github/workflows/security-scanners.yml exactly: +# flawfinder gate at >=4 (below 4 is risky-API grep noise; gating on it +# trains everyone to --no-verify) +# semgrep gate at >=WARNING with p/c + p/security-audit +# plus cppcheck, which CI does not run and which is cheap enough locally. +# +# If a threshold moves in that workflow, move it here in the SAME commit -- +# otherwise local-green stops predicting remote-green, which is the only +# reason this script exists. +# +# NOT here, deliberately: +# clang-tidy -- needs ngx_auto_config.h, i.e. a configured nginx tree. A +# local hook cannot assume one, and a check that skips itself +# when the tree is missing is a vacuous gate. CI-only. +# +# Usage: ci/linter/lint-c.sh [files...] (no args => LINT_MODE, default all) +# Env: LINT_MODE=staged|all +# LINT_SKIP_SEMGREP=1 explicit, loud opt-out for slow machines +# +# Extend: add a scanner as one more block below; keep the CI mirror comment +# accurate or the header above becomes a lie. + +# shellcheck source=ci/linter/lib.sh +. "$(git rev-parse --show-toplevel)/ci/linter/lib.sh" + +mapfile -t FILES < <(lint_files '^src/.*\.[ch]$' "$@") +[ "${#FILES[@]}" -gt 0 ] || { echo "lint-c: no C files to check"; exit 0; } + +echo "lint-c: ${#FILES[@]} file(s)" +rc=0 + +need flawfinder "apt-get install flawfinder" +say "flawfinder (gate >=4)" +flawfinder --minlevel=4 --error-level=4 --quiet "${FILES[@]}" || rc=1 + +need cppcheck "apt-get install cppcheck" +say "cppcheck (error,warning)" +# --error-exitcode=1 makes any reported id fail. missingInclude/unusedFunction +# are suppressed: this is a module compiled INTO nginx, so its headers and its +# ngx_http_* callbacks are never resolvable or called from this tree alone. +cppcheck --quiet --error-exitcode=1 \ + --enable=warning,performance,portability \ + --inline-suppr \ + --suppress=missingInclude --suppress=missingIncludeSystem \ + --suppress=unusedFunction --suppress=unknownMacro \ + --suppress=normalCheckLevelMaxBranches \ + "${FILES[@]}" || rc=1 + +if [ -n "${LINT_SKIP_SEMGREP:-}" ]; then + warn "semgrep SKIPPED via LINT_SKIP_SEMGREP -- CI still gates on it" +else + need semgrep "pipx install semgrep==1.169.0" + say "semgrep (gate >=WARNING)" + # --quiet is deliberately absent: it hides semgrep-core's own crash text + # (io_uring/RLIMIT_MEMLOCK) and turns a diagnosable failure into a bare + # exit 2 that reads like a real finding. + # + # --jobs=1 is a correctness flag, not a speed flag. semgrep-core defaults to + # one OCaml domain per core (32 here), each of which opens its own io_uring + # ring against the 8 MB RLIMIT_MEMLOCK this host shares with the self-hosted + # CI runners. Under runner load it exhausts and semgrep-core dies with + # `Unix_error: Cannot allocate memory io_uring_queue_init` -> exit 2, which + # this script reports as a finding. Observed 3/3 crashed on a 3-file scan + # while runners were busy, 0/3 on an idle box: a load-dependent false RED. + # src/ is three files, so the parallelism was buying nothing to begin with. + # + # --metrics=off: no scan-summary POST to semgrep.dev. Measured 2.76s -> 1.27s + # on this tree, i.e. more than half the wall clock was that upload. + semgrep scan --config p/c --config p/security-audit \ + --severity=WARNING --severity=ERROR --error \ + --jobs=1 --metrics=off "${FILES[@]}" || rc=1 +fi + +exit "$rc" diff --git a/ci/linter/lint-nginx.sh b/ci/linter/lint-nginx.sh new file mode 100755 index 0000000..4e74078 --- /dev/null +++ b/ci/linter/lint-nginx.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# ci/linter/lint-nginx.sh -- nginx-specific source conventions for src/*.[ch]. +# +# What no generic C linter knows: an nginx module must use the nginx allocator, +# the nginx string/number helpers and the nginx source style, because it is +# compiled into a worker with a pool-based lifetime and a shared coding +# standard. flawfinder/cppcheck/semgrep all pass code that leaks a malloc() +# into a request pool or calls atoi() on attacker input. +# +# Checks (each one line per finding, "file:line: rule: text"): +# libc-alloc malloc/calloc/realloc/free -> ngx_palloc/ngx_pcalloc/ngx_pfree +# libc-str strcpy/strcat/sprintf/strncpy -> ngx_cpymem/ngx_snprintf +# libc-num atoi/atol/strtol on request data -> ngx_atoi/ngx_atoof +# libc-io bare printf/fprintf(stderr) -> ngx_log_error +# tabs hard tab in source -> nginx style is 4 spaces +# width line >80 columns -> nginx style limit +# trailing trailing whitespace +# include .c must include ngx_config.h before ngx_core.h +# +# Suppress a single justified line with a trailing /* NOLINT-nginx */ . +# Suppressing a whole rule is not supported on purpose: the exception belongs +# next to the code that needs it, where review can see the reason. +# +# Usage: ci/linter/lint-nginx.sh [files...] Env: LINT_MODE=staged|all +# Extend: add a rule as one more `rule ` call. + +# shellcheck source=ci/linter/lib.sh +. "$(git rev-parse --show-toplevel)/ci/linter/lib.sh" + +mapfile -t FILES < <(lint_files '^src/.*\.[ch]$' "$@") +[ "${#FILES[@]}" -gt 0 ] || { echo "lint-nginx: no C files to check"; exit 0; } + +echo "lint-nginx: ${#FILES[@]} file(s)" +rc=0 + +# rule -- report every matching line not marked NOLINT. +rule() { + local name="$1" re="$2" msg="$3" hits + hits=$(grep -nE "$re" "${FILES[@]}" 2>/dev/null | grep -v 'NOLINT-nginx' || true) + [ -n "$hits" ] || return 0 + printf '%s\n' "$hits" | sed "s/^/ /; s/$/ [$name: $msg]/" + rc=1 +} + +rule libc-alloc '(^|[^_[:alnum:]])(malloc|calloc|realloc|free)[[:space:]]*\(' \ + 'use ngx_palloc/ngx_pcalloc/ngx_pfree' +rule libc-str '(^|[^_[:alnum:]])(strcpy|strcat|sprintf|strncpy|strncat)[[:space:]]*\(' \ + 'use ngx_cpymem/ngx_snprintf' +rule libc-num '(^|[^_[:alnum:]])(atoi|atol|strtol|strtoul)[[:space:]]*\(' \ + 'use ngx_atoi/ngx_atoof' +rule libc-io '(^|[^_[:alnum:]])(printf|fprintf|perror)[[:space:]]*\(' \ + 'use ngx_log_error/ngx_conf_log_error' +rule tabs $'\t' 'nginx style is 4 spaces, no hard tabs' +rule trailing '[[:space:]]+$' 'trailing whitespace' +rule width '^.{81,}$' 'nginx style limit is 80 columns' + +for f in "${FILES[@]}"; do + case "$f" in + *.c) + # ngx_config.h defines the feature macros every later nginx header + # reads; including ngx_core.h first silently changes the build. Look at + # the FIRST ngx_ include, not a fixed head -N window: these files open + # with a long licence/design comment that would push the includes out + # of any window and make the check vacuous. + # Angle brackets only: a local "ngx_http__*.h" is this module's + # own header and carries its own ngx_config.h include -- matching it + # here reported every well-formed file. + first_ngx=$(grep -nE '^[[:space:]]*#[[:space:]]*include[[:space:]]*/dev/null || rc=1 +done + +need perlcritic "apt-get install libperl-critic-perl (or: cpan Perl::Critic)" +say "perlcritic (severity >=4)" +perlcritic --severity 4 --quiet "${FILES[@]}" || rc=1 + +exit "$rc" diff --git a/ci/linter/lint-python.sh b/ci/linter/lint-python.sh new file mode 100755 index 0000000..7cd7d27 --- /dev/null +++ b/ci/linter/lint-python.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# ci/linter/lint-python.sh -- ruff lint + format check over tracked *.py. +# +# ruff replaces flake8/pyflakes/isort/black here: one binary, no venv, and it +# is what the superrepo's tools/ scripts are already checked with. The module +# tree carries no Python today; this gate exists so the first helper script +# committed under ci/tools/ is checked from its first commit instead of after +# it has grown. +# +# `ruff format --check` reports formatting WITHOUT rewriting: a linter that +# edits files behind a commit hook changes what you are about to commit. +# +# Usage: ci/linter/lint-python.sh [files...] Env: LINT_MODE=staged|all +# Extend: per-rule config goes in a [tool.ruff] block in pyproject.toml at the +# repo root, not in flags here, so editors and CI see the same rules. + +# shellcheck source=ci/linter/lib.sh +. "$(git rev-parse --show-toplevel)/ci/linter/lib.sh" + +mapfile -t FILES < <(lint_files '\.py$' "$@") +[ "${#FILES[@]}" -gt 0 ] || { echo "lint-python: no Python files to check"; exit 0; } + +echo "lint-python: ${#FILES[@]} file(s)" +need ruff "apt-get install ruff (or: pipx install ruff)" +rc=0 +say "ruff check" +ruff check "${FILES[@]}" || rc=1 +say "ruff format --check" +ruff format --check "${FILES[@]}" || rc=1 +exit "$rc" diff --git a/ci/linter/lint-sh.sh b/ci/linter/lint-sh.sh new file mode 100755 index 0000000..79c6ea4 --- /dev/null +++ b/ci/linter/lint-sh.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash +# ci/linter/lint-sh.sh -- shellcheck over every *.sh / *.bash in the tree. +# +# -S warning, not info: the info tier (SC2015, SC2086 in safe positions) fires +# on existing build scripts and would land this gate red on arrival, which only +# teaches everyone to --no-verify. Same floor as the shellcheck the actionlint +# hook runs inside `run:` blocks (SHELLCHECK_OPTS=-Swarning), so an SC2164 is +# not ignored in a .sh file while blocking in a workflow. +# +# Usage: ci/linter/lint-sh.sh [files...] Env: LINT_MODE=staged|all +# Extend: raise to -S info only together with a pass that fixes the backlog. + +# shellcheck source=ci/linter/lib.sh +. "$(git rev-parse --show-toplevel)/ci/linter/lib.sh" + +# .githooks/* has no extension but is bash; an unchecked commit hook is the +# one script whose bug silently disables every other check here. +mapfile -t FILES < <(lint_files '\.(sh|bash)$|^\.githooks/' "$@") +[ "${#FILES[@]}" -gt 0 ] || { echo "lint-sh: no shell files to check"; exit 0; } + +echo "lint-sh: ${#FILES[@]} file(s)" +need shellcheck "apt-get install shellcheck" +shellcheck -S warning -x "${FILES[@]}" +say "clean" diff --git a/ci/linter/lint-yaml.sh b/ci/linter/lint-yaml.sh new file mode 100755 index 0000000..6b624a7 --- /dev/null +++ b/ci/linter/lint-yaml.sh @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# ci/linter/lint-yaml.sh -- yamllint over every *.yml/*.yaml, plus actionlint +# over .github/workflows/*. +# +# Workflows are code that fails silently: a bad `if:` expression, an unknown +# context or a shell error inside a `run:` block does not stop the run, it +# makes the job pass while checking nothing. actionlint is the only checker +# here that reads them as GitHub Actions rather than as YAML. +# +# -ignore 'label ".+" is unknown' +# actionlint 1.7.7 carries a FIXED list of runner images and flags newer +# ones (ubuntu-26.04, ubuntu-26.04-arm) as unknown. Every such report is +# about the linter's age, not the workflow. Drop the ignore once +# actionlint is new enough to know the labels this repo targets. +# SHELLCHECK_OPTS=-Swarning +# actionlint's embedded shellcheck otherwise reports at info while +# lint-sh.sh gates at warning -- same finding, two verdicts. +# +# No --strict on yamllint, on purpose: warnings (long inline `run:` lines) stay +# visible without failing the gate, errors still block. The rule set lives in +# .yamllint at the repo root so editors and this script agree. +# +# actionlint and zizmor are not alternatives: actionlint reads a workflow as +# SYNTAX (bad if:, unknown context, shell error in run:), zizmor reads it as an +# ATTACK SURFACE (template injection, dangerous triggers, leaked credentials, +# unpinned actions, over-broad permissions). This repo targets self-hosted +# runners, where a workflow-level mistake is arbitrary code execution on the +# build host -- so the security pass is not optional here. +# +# --persona=pedantic, not the default: the default already passes on this tree, +# so gating on it would never go red. Pedantic is what caught the matrix +# interpolations in ci-deep.yml and the undocumented CodeQL permissions. Findings +# that are genuinely inapplicable get a `# zizmor: ignore[rule]` at the line, +# with the reason -- never a blanket rule disable in zizmor.yml. +# +# --offline: no API calls from a commit hook. The online audits need a token and +# only add repo-settings context, which belongs in a periodic review, not here. +# +# Usage: ci/linter/lint-yaml.sh [files...] Env: LINT_MODE=staged|all +# Extend: yamllint rules live in .yamllint at the repo root. + +# shellcheck source=ci/linter/lib.sh +. "$(git rev-parse --show-toplevel)/ci/linter/lib.sh" + +mapfile -t FILES < <(lint_files '\.ya?ml$' "$@") +[ "${#FILES[@]}" -gt 0 ] || { echo "lint-yaml: no YAML files to check"; exit 0; } + +echo "lint-yaml: ${#FILES[@]} file(s)" +rc=0 + +need yamllint "apt-get install yamllint" +say "yamllint" +yamllint "${FILES[@]}" || rc=1 + +mapfile -t WF < <(printf '%s\n' "${FILES[@]}" | grep -E '^\.github/workflows/' || true) +if [ "${#WF[@]}" -gt 0 ]; then + need actionlint "go install github.com/rhysd/actionlint/cmd/actionlint@latest (see install-linters.sh)" + say "actionlint (${#WF[@]} workflow(s))" + SHELLCHECK_OPTS=-Swarning \ + actionlint -ignore 'label ".+" is unknown' "${WF[@]}" || rc=1 + + need zizmor "pipx install zizmor" + say "zizmor (workflow security, pedantic)" + zizmor --offline --persona=pedantic --no-progress "${WF[@]}" || rc=1 +fi + +exit "$rc" diff --git a/ci/linter/run-all.sh b/ci/linter/run-all.sh new file mode 100755 index 0000000..3531dec --- /dev/null +++ b/ci/linter/run-all.sh @@ -0,0 +1,126 @@ +#!/usr/bin/env bash +# ci/linter/run-all.sh -- run every ci/linter/lint-*.sh and report once. +# +# This is what .githooks/pre-commit calls and what a human should call before +# pushing. It does NOT stop at the first failure: seeing all four failures in +# one run beats four commit attempts. +# +# Usage: +# ci/linter/run-all.sh lint every tracked file +# ci/linter/run-all.sh --staged lint only staged files (hook mode) +# ci/linter/run-all.sh --list list the checks and exit +# ci/linter/run-all.sh ... lint exactly these files +# +# Env: +# LINT_MODE=staged|all same as --staged / default +# LINT_ONLY="c sh" run only these checkers (names below) +# LINT_SKIP_SEMGREP=1 loud opt-out of the slowest C pass +# LINT_JOBS=N parallel checkers (default 0 = one per checker, +# 1 = serial; use 1 when bisecting a hang) +# +# Exit: 0 all clean, 1 findings, 2 a linter is missing (install-linters.sh). +# +# The checkers are independent, so they run concurrently and each one's output +# is buffered to its own file and replayed whole, in the fixed glob order. Never +# stream them interleaved: findings carry a file:line but not a checker name, so +# interleaving makes them unattributable, which is the whole reason to buffer. +# Order stays glob order and not completion order so two runs of a dirty tree +# produce the same transcript. +# +# Extend: drop a new ci/linter/lint-.sh in place -- it is picked up by +# glob, no edit here. Keep the "no files of this kind" case exiting 0. + +set -uo pipefail + +ROOT="$(git rev-parse --show-toplevel)" +# || exit 2, not a bare cd: every checker resolves its paths relative to the +# repo root, so failing to get there would lint the wrong tree -- or nothing at +# all -- and report clean. Same "could not run" status as a missing linter. +cd "$ROOT" || exit 2 + +MODE="${LINT_MODE:-all}" +FILES=() +for a in "$@"; do + case "$a" in + --staged) MODE=staged ;; + --all) MODE=all ;; + --list) ls ci/linter/lint-*.sh; exit 0 ;; + -h|--help) sed -n '2,30p' "$0"; exit 0 ;; + *) FILES+=("$a") ;; + esac +done +export LINT_MODE="$MODE" + +echo "== lint (mode: $MODE) ==" + +# Selected checkers, in glob order. Built first so the replay loop below can +# walk the same list it launched. +ALL=() +SEL=() +for s in ci/linter/lint-*.sh; do + name="$(basename "$s" .sh)"; name="${name#lint-}" + ALL+=("$name") + if [ -n "${LINT_ONLY:-}" ] && [[ " $LINT_ONLY " != *" $name "* ]]; then + continue + fi + SEL+=("$name") +done + +# An empty selection is "could not run", never "clean". A typo in LINT_ONLY +# used to leave both loops below iterating zero times, printing "all linters +# clean" and exiting 0 -- a gate that silently lints nothing is worse than no +# gate. Same exit 2 as a missing linter. +if [ "${#SEL[@]}" -eq 0 ]; then + if [ "${#ALL[@]}" -eq 0 ]; then + echo "no ci/linter/lint-*.sh found -- wrong tree?" >&2 + else + echo "LINT_ONLY=\"${LINT_ONLY:-}\" matched no checker; known: ${ALL[*]}" >&2 + fi + exit 2 +fi + +JOBS="${LINT_JOBS:-0}" +[ "$JOBS" -eq 0 ] 2>/dev/null && JOBS="${#SEL[@]}" +[ "$JOBS" -ge 1 ] 2>/dev/null || JOBS=1 + +# mktemp -d, not a fixed path: two checkouts (or a hook racing a manual run) +# would otherwise share buffers. Trap covers the Ctrl-C path too. +BUF="$(mktemp -d "${TMPDIR:-/tmp}/lint-buf.XXXXXX")" +trap 'rm -rf "$BUF"' EXIT INT TERM + +for name in "${SEL[@]}"; do + # Bounded fan-out: block until a slot frees. `wait -n` needs bash 4.3+, + # which install-linters.sh already assumes. + while [ "$(jobs -rp | wc -l)" -ge "$JOBS" ]; do wait -n; done + { + "ci/linter/lint-$name.sh" "${FILES[@]+"${FILES[@]}"}" \ + > "$BUF/$name.out" 2>&1 + # Exit status travels in a file, not in $?: the reaping `wait` below is + # collective, so per-child statuses are not otherwise recoverable. + echo "$?" > "$BUF/$name.rc" + } & +done +wait + +rc=0 missing=0 failed=() +for name in "${SEL[@]}"; do + cat "$BUF/$name.out" + # A missing .rc means the subshell died before writing one (OOM-killer, + # SIGKILL). Treat it as a failure, never as a pass. + crc="$(cat "$BUF/$name.rc" 2>/dev/null || echo 1)" + case "$crc" in + 0) ;; + 2) missing=1; failed+=("$name(tool missing)") ;; + *) rc=1; failed+=("$name") ;; + esac +done + +if [ "$missing" -eq 1 ]; then + echo "== FAIL: ${failed[*]} -- run ci/linter/install-linters.sh ==" >&2 + exit 2 +fi +if [ "$rc" -ne 0 ]; then + echo "== FAIL: ${failed[*]} ==" >&2 + exit 1 +fi +echo "== all linters clean ==" diff --git a/ci/linter/selftest.sh b/ci/linter/selftest.sh new file mode 100755 index 0000000..1338d31 --- /dev/null +++ b/ci/linter/selftest.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# ci/linter/selftest.sh -- negative controls for the lint gate itself. +# +# The gate is the thing that decides whether everything else is allowed to +# land, so "the gate ran and said clean" has to be distinguishable from "the +# gate ran nothing and said clean". That distinction is not observable from a +# green lint job: a selector typo used to make run-all.sh print +# "== all linters clean ==" and exit 0 having executed zero checkers. +# +# Every case here asserts the FAILING direction -- a check that only ever +# asserts the passing direction cannot detect its own disarming. +# +# Usage: ci/linter/selftest.sh +# Exit: 0 all controls held, 1 one or more did not. +# +# Runs in about a second: no case invokes a real checker (LINT_ONLY values are +# either bogus or rejected before dispatch), so no linter needs to be installed. +# +# Extend: add a case() line. Keep each case asserting a specific exit status, +# and prefer a case where the OLD, broken behaviour would have passed. + +set -uo pipefail + +ROOT="$(git rev-parse --show-toplevel)" +cd "$ROOT" || exit 2 + +rc=0 + +# case -- command... +case_() { + local want="$1" desc="$2"; shift 2 + local out got + out="$("$@" 2>&1)"; got=$? + if [ "$got" -eq "$want" ]; then + echo "ok $desc (exit $got)" + else + echo "FAIL $desc: expected exit $want, got $got" >&2 + echo "$out" | sed 's/^/ | /' >&2 + rc=1 + fi +} + +# The regression itself: an unmatched selector must be "could not run" (2), +# not "clean" (0). +case_ 2 "unknown LINT_ONLY exits 2" \ + env LINT_ONLY=nosuchchecker ci/linter/run-all.sh + +# ...including when only SOME of the listed names are bogus in a way that +# leaves nothing selected. +case_ 2 "LINT_ONLY of only-bogus names exits 2" \ + env LINT_ONLY="c-lang shellscript" ci/linter/run-all.sh + +# The error names the offending value and the known checkers, or nobody can +# act on it. +# Captured first, not piped into grep: `set -o pipefail` would otherwise hand +# the pipeline run-all.sh's exit 2 and the assertion would read as failed no +# matter what the message said. +msg="$(env LINT_ONLY=nosuchchecker ci/linter/run-all.sh 2>&1)" +if printf '%s\n' "$msg" | grep -q 'matched no checker; known: .*sh'; then + echo "ok unmatched-selector message names value and known checkers" +else + echo "FAIL unmatched-selector message is not actionable" >&2 + rc=1 +fi + +# Positive control: the selector still selects. --list is used rather than a +# real run so this stays independent of which linters are installed. +case_ 0 "--list works" ci/linter/run-all.sh --list + +if [ "$rc" -eq 0 ]; then + echo "== lint gate selftest: all controls held ==" +else + echo "== lint gate selftest: FAILED ==" >&2 +fi +exit "$rc" diff --git a/ci/tools/bump-versions.sh b/ci/tools/bump-versions.sh index 612835b..d89915b 100755 --- a/ci/tools/bump-versions.sh +++ b/ci/tools/bump-versions.sh @@ -1,26 +1,32 @@ #!/usr/bin/env bash # -# Check nginx.org/angie.software for newer releases than what's pinned in this -# repo, and rewrite every pin in place. Called by .github/workflows/bump.yml on -# a schedule; also runnable locally to preview a bump before it lands. +# Refresh every upstream pin in this repo. Called by .github/workflows/bump.yml +# on a schedule; also runnable locally to preview a bump before it lands. # # ci/tools/bump-versions.sh [--dry-run] # -# What gets bumped, and why each one has to move together: -# - NGINX_VERSION (mainline pin) -- .github/workflows/{build-test,ci-deep,codeql,valgrind,asan,fuzzing,security-scanners}.yml -# (EVERY workflow with an NGINX_VERSION env -# -- miss one and its gate silently tests a -# stale nginx after a bump) -# - nginx stable + angie pins -- ci-deep.yml's build-flavors matrix -# - NGINX_SHA256 / ANGIE_SHA256 -- ci/tools/ci-build.sh (this script computes -# the digest itself from the same tarball -# ci-build.sh will later verify against) -# - ci/vendor/nginx-tests submodule -- `git submodule update --remote` +# Two things move here: +# - .github/versions.env -- nginx mainline/stable + angie versions +# AND their sha256s, rewritten wholesale +# by .github/scripts/compute-versions.sh +# - ci/vendor/nginx-tests submodule -- `git submodule update --remote` # -# A version bump with a stale sha256 pin is worse than no pin (ci-build.sh -# treats a missing pin as "print a warning", but a WRONG pin is a hard FATAL -- -# so every version edit here is paired with a digest computed from the exact -# tarball that version resolves to, never carried over from a previous entry. +# This script used to sed a version literal into each of seven workflow files, +# patch ci-deep.yml's matrix, and insert a matching digest into a table in +# ci-build.sh. Those pins now live on adjacent lines in one file with one +# writer, so the failure modes that design had -- a bumped version carrying a +# stale digest, or one workflow missed by the sed and silently gating on an old +# nginx -- are gone by construction, and so is the code that managed them. +# +# --dry-run reports what WOULD change without writing anything: versions.env is +# regenerated into a scratch copy and diffed, and the submodule is left alone. +# +# Exit status is 0 whether or not anything changed; the caller decides what to +# do with a dirty tree. Prints CHANGED=0|1 as its last line. +# +# GH_TOKEN is honoured (passed through to compute-versions.sh as GITHUB_TOKEN): +# the runners share an egress IP, so unauthenticated api.github.com calls are +# routinely rate-limited to 403s. set -euo pipefail @@ -29,165 +35,36 @@ DRY_RUN=0 cd "$(dirname "$0")/../.." -# --- discover latest versions ------------------------------------------- - -# nginx.org/en/download.html lists Mainline then Stable then Legacy, each as -# its own section header followed by a table whose first tarball link is that -# section's current release -- no JSON feed exists, so parse the one page -# nginx itself treats as authoritative. -latest_nginx() { - local branch="$1" # mainline | stable - local page section - page="$(curl -fsSL https://nginx.org/en/download.html)" - case "$branch" in - mainline) section="Mainline version" ;; - stable) section="Stable version" ;; - esac - # The page is one long line; cut everything before the section header, then - # take the first tarball link after it -- that link is that section's - # current release, per nginx.org's own page layout. - echo "${page#*"$section"}" \ - | grep -oE 'nginx-[0-9]+\.[0-9]+\.[0-9]+\.tar\.gz' | head -1 \ - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' -} - -latest_angie() { - local json - # The runners share an egress IP, so the unauthenticated API allowance - # (60/hr) is routinely exhausted and this call 403s. Send GH_TOKEN when - # one is present -- authenticated requests get their own, far larger quota. - local -a auth=() - [ -n "${GH_TOKEN:-}" ] && auth=(-H "Authorization: Bearer $GH_TOKEN") - if ! json="$(curl -fsSL "${auth[@]}" https://api.github.com/repos/webserver-llc/angie/releases/latest)"; then - echo "error: could not query the angie release API (rate limit? set GH_TOKEN)" >&2 - return 1 - fi - echo "$json" | grep -m1 '"tag_name"' | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' -} - -NEW_MAINLINE="$(latest_nginx mainline)" -NEW_STABLE="$(latest_nginx stable)" -NEW_ANGIE="$(latest_angie)" - -for v in NEW_MAINLINE NEW_STABLE NEW_ANGIE; do - if [ -z "${!v}" ]; then - echo "FATAL: could not determine $v -- refusing to bump with a blank version" >&2 - exit 1 - fi -done - -echo "latest: nginx mainline=$NEW_MAINLINE stable=$NEW_STABLE angie=$NEW_ANGIE" - -# Each matrix entry is "version:" immediately followed by "label:" (see -# ci-deep.yml's build-flavors job) -- pair them up rather than assuming -# ordering, so a future reordering of the matrix can't silently swap pins. -matrix_version_for_label() { - awk -v want="$1" ' - /version:/ { match($0, /"[0-9.]+"/); v = substr($0, RSTART+1, RLENGTH-2); next } - /label:/ { split($0, a, ":"); l = a[2]; gsub(/[ \t]/, "", l); if (l == want) { print v; exit } } - ' .github/workflows/ci-deep.yml -} - -CUR_MAINLINE="$(grep -m1 'NGINX_VERSION:' .github/workflows/build-test.yml | grep -oE '[0-9]+\.[0-9]+\.[0-9]+')" -CUR_STABLE="$(matrix_version_for_label stable)" -CUR_ANGIE="$(matrix_version_for_label angie)" - -echo "pinned: nginx mainline=$CUR_MAINLINE stable=$CUR_STABLE angie=$CUR_ANGIE" +# compute-versions.sh reads GITHUB_TOKEN; bump.yml historically set GH_TOKEN. +export GITHUB_TOKEN="${GITHUB_TOKEN:-${GH_TOKEN:-}}" CHANGED=0 +VERSIONS_FILE=".github/versions.env" -# --- sha256 helper -------------------------------------------------------- -sha256_for() { - local flavor="$1" version="$2" url tmp digest - case "$flavor" in - nginx) url="https://nginx.org/download/nginx-${version}.tar.gz" ;; - angie) url="https://download.angie.software/files/angie-${version}.tar.gz" ;; - esac - tmp="$(mktemp)" - curl -fsSL "$url" -o "$tmp" - digest="$(sha256sum "$tmp" | awk '{print $1}')" - rm -f "$tmp" - echo "$digest" -} - -# --- bump a version everywhere it's pinned -------------------------------- -bump_nginx_workflow_pin() { - local old="$1" new="$2" - [ "$old" = "$new" ] && return 0 - # Every workflow that pins NGINX_VERSION -- keep this list == the set that - # `grep -l 'NGINX_VERSION:' .github/workflows/*.yml` returns, or a bumped - # mainline leaves the omitted gate testing a stale nginx (silent + green). - for f in .github/workflows/build-test.yml .github/workflows/ci-deep.yml \ - .github/workflows/codeql.yml .github/workflows/valgrind.yml \ - .github/workflows/asan.yml .github/workflows/fuzzing.yml \ - .github/workflows/security-scanners.yml; do - sed -i "s/NGINX_VERSION: \"${old}\"/NGINX_VERSION: \"${new}\"/" "$f" - done - CHANGED=1 -} - -bump_matrix_pin() { - local label="$1" old="$2" new="$3" - [ "$old" = "$new" ] && return 0 - # Matrix entries are unique per label (mainline/stable/angie) in ci-deep.yml. - python3 - "$label" "$old" "$new" <<'PYEOF' -import re, sys -label, old, new = sys.argv[1:4] -path = ".github/workflows/ci-deep.yml" -text = open(path).read() -pattern = re.compile( - r'(version:\s*"' + re.escape(old) + r'"\n\s*label:\s*' + re.escape(label) + r')' -) -replaced = pattern.sub(lambda m: m.group(1).replace(old, new), text) -if replaced == text: - print(f"WARNING: no matrix entry matched for label={label} old={old}", file=sys.stderr) -open(path, "w").write(replaced) -PYEOF - CHANGED=1 -} - -bump_sha256_pin() { - local table="$1" old="$2" new="$3" digest="$4" - grep -q "\[\"${new}\"\]" ci/tools/ci-build.sh && return 0 # already pinned - # Insert the new pin right after the table's opening line; leave old - # entries in place (ci-build.sh keys by version, older callers still work). - sed -i "/declare -A ${table}=(/a\\ [\"${new}\"]=\"${digest}\"" ci/tools/ci-build.sh - CHANGED=1 -} - -if [ "$NEW_MAINLINE" != "$CUR_MAINLINE" ]; then - echo "bump nginx mainline: $CUR_MAINLINE -> $NEW_MAINLINE" - if [ "$DRY_RUN" = 0 ]; then - DIGEST="$(sha256_for nginx "$NEW_MAINLINE")" - echo " sha256 $DIGEST" - bump_nginx_workflow_pin "$CUR_MAINLINE" "$NEW_MAINLINE" - bump_matrix_pin mainline "$CUR_MAINLINE" "$NEW_MAINLINE" - bump_sha256_pin NGINX_SHA256 "$CUR_MAINLINE" "$NEW_MAINLINE" "$DIGEST" - else +# --- version + sha256 pins ------------------------------------------------- +if [ "$DRY_RUN" = 0 ]; then + # Tolerate a missing file: compute-versions.sh creates it from scratch, so + # bootstrapping (or regenerating after a delete) should report "changed" + # rather than dying here under set -e. + before="$(cat "$VERSIONS_FILE" 2>/dev/null || true)" + bash .github/scripts/compute-versions.sh + if [ "$before" != "$(cat "$VERSIONS_FILE")" ]; then + echo "--- versions.env changed ---" + git --no-pager diff -- "$VERSIONS_FILE" || true CHANGED=1 - fi -fi - -if [ "$NEW_STABLE" != "$CUR_STABLE" ]; then - echo "bump nginx stable: $CUR_STABLE -> $NEW_STABLE" - if [ "$DRY_RUN" = 0 ]; then - DIGEST="$(sha256_for nginx "$NEW_STABLE")" - echo " sha256 $DIGEST" - bump_matrix_pin stable "$CUR_STABLE" "$NEW_STABLE" - bump_sha256_pin NGINX_SHA256 "$CUR_STABLE" "$NEW_STABLE" "$DIGEST" else - CHANGED=1 + echo "versions.env already up to date" fi -fi - -if [ "$NEW_ANGIE" != "$CUR_ANGIE" ]; then - echo "bump angie: $CUR_ANGIE -> $NEW_ANGIE" - if [ "$DRY_RUN" = 0 ]; then - DIGEST="$(sha256_for angie "$NEW_ANGIE")" - echo " sha256 $DIGEST" - bump_matrix_pin angie "$CUR_ANGIE" "$NEW_ANGIE" - bump_sha256_pin ANGIE_SHA256 "$CUR_ANGIE" "$NEW_ANGIE" "$DIGEST" +else + # Regenerate into a scratch copy so the working tree is untouched. + scratch="$(mktemp -d)" + trap 'rm -rf "$scratch"' EXIT + cp -a .github "$scratch/.github" + ( cd "$scratch" && bash .github/scripts/compute-versions.sh >/dev/null ) + if diff -u "$VERSIONS_FILE" "$scratch/$VERSIONS_FILE"; then + echo "(dry-run: versions.env already up to date)" else + echo "(dry-run: versions.env would change as shown above)" CHANGED=1 fi fi diff --git a/ci/tools/ci-build.sh b/ci/tools/ci-build.sh index c6aef30..03c5f30 100755 --- a/ci/tools/ci-build.sh +++ b/ci/tools/ci-build.sh @@ -56,9 +56,39 @@ set -euo pipefail FLAVOR="${1:-nginx}" -VERSION="${2:-1.31.2}" MODE="${3:-debug}" +# Version pins (and their sha256s) all come from .github/versions.env -- see +# the integrity block below. Sourced this early so the default version tracks +# the pinned one instead of being a literal that silently rots. +VERSIONS_FILE="${VERSIONS_FILE:-$PWD/.github/versions.env}" +if [ ! -f "$VERSIONS_FILE" ]; then + echo "FATAL: $VERSIONS_FILE not found (run from the module root)" >&2 + exit 1 +fi +# Validate before sourcing. load-versions.sh applies the same KEY=value check +# on the CI side, but this script `source`s the file directly -- so without a +# check here, a stray line that is not a pin would be executed as shell rather +# than rejected. Same rule enforced in both consumers. +while IFS= read -r line || [ -n "$line" ]; do + case "$line" in + ''|\#*) continue ;; + esac + if ! printf '%s' "$line" | grep -qE '^[A-Za-z_][A-Za-z0-9_]*=[A-Za-z0-9._-]*$'; then + echo "FATAL: malformed line in $VERSIONS_FILE: $line" >&2 + exit 1 + fi +done < "$VERSIONS_FILE" +# shellcheck source=/dev/null +. "$VERSIONS_FILE" + +case "$FLAVOR" in + nginx) DEFAULT_VERSION="${NGINX_VERSION:-}" ;; + angie) DEFAULT_VERSION="${ANGIE_VERSION:-}" ;; + *) DEFAULT_VERSION="" ;; +esac +VERSION="${2:-$DEFAULT_VERSION}" + case "$MODE" in debug|asan|module) ;; *) @@ -87,61 +117,70 @@ esac NO_CACHE="${NO_CACHE:-0}" -# --- integrity: pinned SHA-256 for nginx tarballs we've actually verified --- +# --- integrity: sha256 pins come from .github/versions.env ------------------- # nginx.org serves plain HTTP-adjacent PGP signatures, not a sha256sum file, so # "verify against the vendor" means pinning a known-good digest for each source -# tarball we build, computed once from a tarball fetched over HTTPS from -# nginx.org and recorded here -- not fabricated. A version not in this table -# builds anyway (this skeleton tracks a moving nginx release, and refusing to -# build an unpinned version would break every future version bump until someone -# updates this table first) but prints a loud warning instead of silently -# skipping verification, so the gap is visible rather than assumed-safe. -declare -A NGINX_SHA256=( - ["1.31.1"]="9fcaaeb8f22544b09a19a761f3412c4112215422401634bebdd1296a403cc4bc" - ["1.31.2"]="af2a957c41da636ddc4f883e4523c6d140b4784dbce42000c364ae5092aa473c" - ["1.30.3"]="e5823dc6f45610993def93ebf6cfce68264af4958c77e874b7d20f3709001b8f" -) - -# Same idea as NGINX_SHA256, for the angie flavor. -declare -A ANGIE_SHA256=( - ["1.12.0"]="cd7867d200b22a80165b93696c30a1ac3a28c1162544b7f43c71232b19814ef6" -) +# tarball we build. Those digests used to live in a table right here, which put +# the version (in seven workflow files) and its digest (here) in different +# places under different writers -- so a bumped version with a stale digest was +# a live failure mode. Both now live on adjacent lines in .github/versions.env, +# written by one tool (.github/scripts/compute-versions.sh). +# +# Verification is MANDATORY: a version with no digest here is a hard failure, +# not a warning. compute-versions.sh always emits version+digest together, so +# the only way to reach this error is a hand-edit that half-did the job. +# +# (versions.env was already sourced near the top, to default $VERSION.) +# +# Pick the digest matching the flavor+version being built. The single-version +# jobs all build NGINX_VERSION; ci-deep's matrix also builds NGINX_STABLE and +# ANGIE_VERSION. Anything else has no pin and must not be built silently. +EXPECTED="" +if [ -z "$VERSION" ]; then + # Guard before the case below: an empty $VERSION would match an empty + # "${NGINX_STABLE:-}" pattern and silently adopt the wrong digest. + echo "FATAL: no version given and no default pin for flavor '$FLAVOR'" >&2 + exit 1 +fi +case "$FLAVOR" in + nginx) + case "$VERSION" in + "${NGINX_VERSION:-}") EXPECTED="${NGINX_VERSION_SHA256:-}" ;; + "${NGINX_MAINLINE:-}") EXPECTED="${NGINX_MAINLINE_SHA256:-}" ;; + "${NGINX_STABLE:-}") EXPECTED="${NGINX_STABLE_SHA256:-}" ;; + esac + ;; + angie) + case "$VERSION" in + "${ANGIE_VERSION:-}") EXPECTED="${ANGIE_SHA256:-}" ;; + esac + ;; +esac + +if [ -z "$EXPECTED" ]; then + echo "FATAL: no pinned sha256 for $FLAVOR $VERSION" >&2 + echo " $VERSIONS_FILE pins:" >&2 + echo " nginx ${NGINX_VERSION:-?} (mainline ${NGINX_MAINLINE:-?}, stable ${NGINX_STABLE:-?})" >&2 + echo " angie ${ANGIE_VERSION:-?}" >&2 + echo " Regenerate it with .github/scripts/compute-versions.sh, or add the" >&2 + echo " version + its sha256 there -- never build an unverified tarball." >&2 + exit 1 +fi # The mode is in the tree path. See the CACHING block above -- sharing one tree # across modes is what lets a cached debug objs/ silently disarm the asan job. SRCDIR="$ROOT/${DIR}-${MODE}" mkdir -p "$ROOT" -if [ ! -f "$ROOT/${DIR}.tar.gz" ]; then - curl -fsSL "$URL" -o "$ROOT/${DIR}.tar.gz" - - # A fresh download is the only time a bad tarball can enter the cache, so - # this is where verification belongs -- a tarball already sitting in - # .build/ from a prior verified run doesn't need re-checking every - # invocation. Pinned digests are computed once from an HTTPS fetch and - # recorded in NGINX_SHA256/ANGIE_SHA256 above; this is a stopgap for - # nginx.org's plain HTTP-adjacent PGP signatures (not a sha256sum file) -- - # see https://nginx.org/en/pgp_keys.html for the upstream-recommended - # `gpg --verify` method against nginx's published release-signing keys. - case "$FLAVOR" in - nginx) EXPECTED="${NGINX_SHA256[$VERSION]:-}" ;; - angie) EXPECTED="${ANGIE_SHA256[$VERSION]:-}" ;; - esac - if [ -n "$EXPECTED" ]; then - ACTUAL="$(sha256sum "$ROOT/${DIR}.tar.gz" | awk '{print $1}')" - if [ "$ACTUAL" != "$EXPECTED" ]; then - echo "FATAL: sha256 mismatch for ${DIR}.tar.gz" >&2 - echo " expected: $EXPECTED" >&2 - echo " actual: $ACTUAL" >&2 - rm -f "$ROOT/${DIR}.tar.gz" - exit 1 - fi - echo "sha256: OK ($VERSION)" - else - echo "WARNING: no pinned sha256 for $FLAVOR $VERSION -- add one to" \ - "${FLAVOR^^}_SHA256 in tools/ci-build.sh (downloaded tarball is" \ - "UNVERIFIED)" >&2 - fi +# fetch-verify.sh downloads (with retries/timeouts) and checks the digest. It +# re-checks a tarball already present in .build/ rather than trusting it, so a +# poisoned build cache is caught too -- and deletes nothing it did not verify. +if ! bash "$MODULE_DIR/.github/scripts/fetch-verify.sh" \ + "$URL" "$EXPECTED" "$ROOT/${DIR}.tar.gz"; then + # A tarball that fails verification must not survive to be picked up as a + # "cache hit" by the next run. + rm -f "$ROOT/${DIR}.tar.gz" + exit 1 fi if [ "$NO_CACHE" = "1" ]; then rm -rf "$SRCDIR" @@ -151,8 +190,18 @@ if [ ! -d "$SRCDIR" ]; then mv "$ROOT/$DIR" "$SRCDIR" fi -# Strict flags: this is hostile-input parser code, so warnings are errors. -CC_OPT="-g -Wall -Wextra -Wshadow" +# Whole-tree flags: --with-cc-opt lands in CFLAGS for EVERY object configure +# compiles -- the upstream core included, not just our module. So only warnings +# that upstream is expected to be clean under belong here. +# +# -Wshadow deliberately does NOT: angie's configure puts -Werror in its own +# default CFLAGS, so -Wshadow here turns shadow warnings in ANGIE's sources +# (ngx_http_client_module.c, ngx_http_prometheus_module.c) into hard build +# failures in code we neither own nor patch. Our own sources ARE kept clean +# under -Wshadow -Werror by the "Strict module compile" step in build-test.yml, +# which applies the full strict set to src/*.c alone -- that is the right place +# for it, and the reason this list is the laxer one. +CC_OPT="-g -Wall -Wextra" LD_OPT="" ADD_MODULE="--add-dynamic-module=$MODULE_DIR" diff --git a/src/ngx_http_skel_module.c b/src/ngx_http_skel_module.c index 5eb8b28..8de06b5 100644 --- a/src/ngx_http_skel_module.c +++ b/src/ngx_http_skel_module.c @@ -39,7 +39,8 @@ typedef struct { * handler just returns the verdict the body handler recorded here. */ typedef struct { - ngx_int_t status; /* verdict from the body pass; NGX_DECLINED = pass */ + /* verdict from the body pass; NGX_DECLINED = pass */ + ngx_int_t status; } ngx_http_skel_ctx_t; @@ -194,8 +195,9 @@ ngx_http_skel_handler(ngx_http_request_t *r) * reads and buffers the WHOLE body before the handler resumes -- in memory * up to client_body_buffer_size, then spooled to a temp file. The bound is * client_max_body_size, NOT skel_max_body: we inspect only the first - * skel_max_body bytes, but nginx still buffers everything first. So enabling - * the body scan (a) defeats request streaming (proxy_request_buffering off) + * skel_max_body bytes, but nginx still buffers everything first. So + * enabling the body scan (a) defeats request streaming + * (proxy_request_buffering off) * and (b) makes every upload up to client_max_body_size hit disk. Keep * client_max_body_size sane on body-scanned routes; do not read this as a * reason to raise skel_max_body -- that only widens the inspected prefix. @@ -242,9 +244,10 @@ ngx_http_skel_body_handler(ngx_http_request_t *r) : NGX_DECLINED; } - /* preserve_body: the content handler (proxy_pass, a POST target) still needs - * the bytes we just buffered; without this they are discarded and it sees an - * empty body. write_event_handler: the resume point once the engine parks. */ + /* preserve_body: the content handler (proxy_pass, a POST target) still + * needs the bytes we just buffered; without this they are discarded and + * it sees an empty body. write_event_handler: the resume point once the + * engine parks. */ r->preserve_body = 1; r->write_event_handler = ngx_http_core_run_phases; @@ -442,7 +445,8 @@ ngx_http_skel_scan_body(ngx_http_request_t *r, size_t max) scanned += len; } - /* End of the (bounded) body: flush any escape held mid-token at the seam. */ + /* End of the (bounded) body: flush any escape held mid-token at the + * seam. */ return ngx_http_skel_stream_final(&st); } @@ -496,7 +500,8 @@ ngx_http_skel_init(ngx_conf_t *cf) if (ngx_http_skel_scan_rules_valid() != NGX_OK) { ngx_log_error(NGX_LOG_EMERG, cf->log, 0, "skel: a scan rule is too long for the %d-byte " - "cross-buffer seam carry; raise NGX_HTTP_SKEL_MAX_RULE_LEN " + "cross-buffer seam carry; raise " + "NGX_HTTP_SKEL_MAX_RULE_LEN " "so that 3 * longest_rule < it", (int) NGX_HTTP_SKEL_MAX_RULE_LEN); return NGX_ERROR; diff --git a/src/ngx_http_skel_scan.c b/src/ngx_http_skel_scan.c index 11bcfbd..ded52a0 100644 --- a/src/ngx_http_skel_scan.c +++ b/src/ngx_http_skel_scan.c @@ -204,7 +204,8 @@ ngx_http_skel_normalize(u_char *dst, u_char *src, size_t len) * token and 'A' is literal. Whether a trailing '%' opens a token depends on the * decoder's state arriving there, so this walks ngx_unescape_uri()'s exact * three-state machine (type 0 path) over the whole window and reports only the - * END state -- it decides where a token is still open, never what it decodes to. + * END state -- it decides where a token is still open, never what it decodes + * to. */ static size_t ngx_http_skel_partial_escape(const u_char *data, size_t len)