From 146db80d2c010dfef7eec74d76b3ee0de387a990 Mon Sep 17 00:00:00 2001 From: Neil Galvin Date: Tue, 1 Sep 2026 11:56:17 +0100 Subject: [PATCH] fix(sops-audit): allowlist non-secret values, and skip TOML-encrypted files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #74. The plaintext-secret scan matched any 8+ character value after a secret-ish key, with no notion of what a value MEANS. Ordinary configuration tripped it: `update_password: on_create` matches on the `password` substring, and `on_create` is nine characters. That is the documented API of ansible.builtin.user, so a consumer cannot reword their way out of it, and their only lever was plaintext-scan-exclude on a whole file — disarming real detection across a file that manages credentials to silence one line. Two classes are now suppressed, both cases where the value CANNOT be a secret: 1. Indirection — the value names something rather than being it. Jinja/Go templates, shell and env expansion, Ansible vault refs, lookups, and SOPS ENC[...] ciphertext. 2. Config enums — a closed list of setting values: on_create, always, never, true/false, enabled/disabled, and similar. Deliberately NOT allowlisted: changeme, placeholder, password, secret. Those are plausible real bad values and keep failing. The allowlist only ever suppresses; it never widens what counts as a secret. Also fixes the SOPS skip missing TOML. The regex covered the YAML, JSON and dotenv metadata markers but not `[sops]`, so an encrypted .toml was scanned rather than skipped even though .toml is in the scanned extension list. No false positive today only because ENC[...] breaks the value character class. tests/plaintext-scan.test.sh, wired into self-test.yml. 21 fixtures, and the important half are real secrets that must STILL be caught — an AWS key with slashes, a base64 private key, a github token, and changeme. This matters more than usual. The workflow reaches every consumer repo through the floating @v2 tag, and an allowlist can only make the scan LESS sensitive, which is the direction that fails silently: a scanner that stops detecting reports success. So the test extracts KEY/SEP/Q/ENUMS from the workflow itself rather than restating them, because a drifted copy would prove nothing about what actually runs. Verified the guard bites: adding `changeme` to the enum list turns the suite red rather than quietly green. Note for whoever merges: @v2 is a floating tag, so re-pointing it publishes this to every consumer at once. Worth landing deliberately. --- .github/workflows/self-test.yml | 13 ++++++ .github/workflows/sops-audit.yml | 50 ++++++++++++++++++-- tests/plaintext-scan.test.sh | 79 ++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 5 deletions(-) create mode 100755 tests/plaintext-scan.test.sh diff --git a/.github/workflows/self-test.yml b/.github/workflows/self-test.yml index 961d903..4349624 100644 --- a/.github/workflows/self-test.yml +++ b/.github/workflows/self-test.yml @@ -19,6 +19,19 @@ jobs: runs-on: '["ubuntu-latest"]' yamllint-paths: ".github/ .yamllint.yml" + # The plaintext-secret scan in sops-audit.yml reaches every consumer repo + # through the floating @v2 tag, and its allowlist can only make the scan + # LESS sensitive — the direction that fails silently. Half these fixtures + # are real secrets that must still be caught, so widening the allowlist + # carelessly turns this red instead of quietly turning detection off. + plaintext-scan: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Plaintext-secret scan fixtures + run: ./tests/plaintext-scan.test.sh + composite-actions-syntax: runs-on: ubuntu-latest timeout-minutes: 10 diff --git a/.github/workflows/sops-audit.yml b/.github/workflows/sops-audit.yml index c05e028..fdc0a9d 100644 --- a/.github/workflows/sops-audit.yml +++ b/.github/workflows/sops-audit.yml @@ -85,7 +85,38 @@ jobs: # ERE that matches common secret-assignment forms in config files. # Anchors on the key name so prose hits like "this is a token for X" # are excluded by the [:=] requirement. - pattern='(password|passwd|secret|api[_-]?key|token|private[_-]?key|access[_-]?key)[[:space:]]*[:=][[:space:]]*["'"'"']?[A-Za-z0-9/_+.=-]{8,}' + KEY='(password|passwd|secret|api[_-]?key|token|private[_-]?key|access[_-]?key)' + SEP='[[:space:]]*[:=][[:space:]]*' + Q='["'"'"']?' + pattern="${KEY}${SEP}${Q}[A-Za-z0-9/_+.=-]{8,}" + + # --- Allowlist ------------------------------------------------- + # The pattern above has no notion of what a value MEANS, so + # ordinary configuration trips it: `update_password: on_create` is + # the documented API of ansible.builtin.user, and `on_create` + # clears the 8-character floor. Consumers cannot reword task code + # around it, and their only lever was excluding a whole file from + # scanning — disarming real detection to silence one line. + # + # Two classes are suppressed, both cases where the value CANNOT be + # a secret: + # + # 1. Indirection — the value names something else rather than + # being the thing. Jinja/Go templates, shell and env + # expansion, Ansible vault refs, lookups, and SOPS ciphertext. + IND="${KEY}${SEP}${Q}([{][{]|[$][{]|[$][A-Za-z_]|ENC[[]|!vault|lookup[(])" + # + # 2. Config enums — a closed set of literals that are values of + # a setting, not credentials. + ENUMS='on_create|always|never|absent|present|true|false|yes|no|none|null|enabled|disabled|required|optional|latest|stable|default|auto|ignore|prompt' + ENUM="${KEY}${SEP}${Q}(${ENUMS})${Q}[[:space:]]*(#.*)?$" + # + # Deliberately NOT allowlisted: changeme, placeholder, secret, + # password and friends. Those are plausible real bad values and + # must keep failing. The allowlist only ever suppresses; it never + # widens what counts as a secret. + # + # See https://github.com/nkg/github-actions/issues/74 any=0 while IFS= read -r file; do # Skip via case-globs from input. @@ -102,13 +133,22 @@ jobs: *.env|*.yml|*.yaml|*.json|*.toml) ;; *) continue ;; esac - # Don't flag SOPS-encrypted files (the ciphertext can look secret-ish). - if grep -q -E '^sops:|^sops_|"sops"' "$file" 2>/dev/null; then + # Don't flag SOPS-encrypted files (the ciphertext can look + # secret-ish). `[sops]` is the TOML form of the metadata block; + # without it an encrypted .toml was scanned rather than skipped, + # even though .toml is in the extension list above. + if grep -q -E '^sops:|^sops_|"sops"|^\[sops\]' "$file" 2>/dev/null; then continue fi - if grep -E -i -q "$pattern" "$file" 2>/dev/null; then + # Match, then drop the allowlisted forms. Reported from the same + # filtered list that decides pass/fail, so what you are shown is + # exactly what failed the build. + hits=$(grep -E -i -n "$pattern" "$file" 2>/dev/null \ + | grep -v -E -i "$IND" \ + | grep -v -E -i "$ENUM" || true) + if [ -n "$hits" ]; then echo "::error file=$file::potential plaintext secret" - grep -E -i -n "$pattern" "$file" | head -5 + printf '%s\n' "$hits" | head -5 any=1 fi done < <(git ls-files) diff --git a/tests/plaintext-scan.test.sh b/tests/plaintext-scan.test.sh new file mode 100755 index 0000000..5e9d0aa --- /dev/null +++ b/tests/plaintext-scan.test.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# +# Test the plaintext-secret scan in .github/workflows/sops-audit.yml. +# +# This workflow is consumed by every repo in the org through a floating @v2 +# tag, so a change here reaches all of them at once. The allowlist added for +# issue #74 can only ever make the scan LESS sensitive, which is exactly the +# direction that fails silently — a scan that stops detecting reports success. +# +# So: the fixtures below are half true-positives that must still fail. If the +# allowlist is ever widened carelessly, those go green and this test goes red. +# +# The patterns are extracted FROM the workflow rather than restated here. +# A copy would drift, and a drifted test proves nothing about what runs. +# SC2153/SC2154: KEY/SEP/Q/ENUMS come from the eval below, which shellcheck +# cannot follow — that indirection is the point, so the test cannot drift +# from the workflow it is testing. +# SC2016: the fixtures deliberately contain literal $VAR and ${VAR}. Single +# quotes are correct; expanding them would destroy the case under test. +# shellcheck disable=SC2153,SC2154,SC2016 + +set -uo pipefail + +WF=".github/workflows/sops-audit.yml" +[ -f "$WF" ] || { echo "run from the repo root" >&2; exit 1; } + +# Pull the four assignments out of the run: block verbatim. +eval "$(grep -E "^ +(KEY|SEP|Q|ENUMS)=" "$WF" | sed 's/^ *//')" + +pattern="${KEY}${SEP}${Q}[A-Za-z0-9/_+.=-]{8,}" +IND="${KEY}${SEP}${Q}([{][{]|[$][{]|[$][A-Za-z_]|ENC[[]|!vault|lookup[(])" +ENUM="${KEY}${SEP}${Q}(${ENUMS})${Q}[[:space:]]*(#.*)?$" + +scan() { # 0 = flagged, 1 = clean + printf '%s\n' "$1" \ + | grep -E -i "$pattern" \ + | grep -v -E -i "$IND" \ + | grep -v -E -i "$ENUM" \ + | grep -q . +} + +pass=0; fail=0 +check() { #