From dc29fe24b05b03ed0fd5b81d68e5f49ffd8795a9 Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:44:16 -0700 Subject: [PATCH 1/6] Hold every secret-store reference to the one manifest that declares it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gated, not derived, and the message says which because the two are different promises. The name cannot be derived into the manifests: the consumers live in four kustomize roots plus a Helm chart, no single root spans them, and kustomize has no substitution that reaches across. So one manifest owns the name and a gate holds everything else to it — and the same name is published for the repositories this one cannot reach. ─── The population is ten, not five ─── The issue counted five chart defaults across four other repositories. This repository restates the same name ten more times: 1 the declaration, addons/bootstrap/secret-stores/cluster-secret-store.yaml 4 secretStoreRef in plain manifests (dashboards x3, secret-stores-managed x1) 4 secretStoreRef in Go-template chart source (catalog/druid) 1 a kustomize patch target in applicationsets/secret-stores.yaml "Every eks-gitops manifest is already correct" was true and was not the property. Correct today, compared by nothing. ─── The reference that is worse than an ExternalSecret ─── An ExternalSecret naming a store that does not exist at least records SecretSyncedError on its own status. The ApplicationSet patch target records nothing: kustomize does not treat an unmatched target as an error, so the build exits 0 and emits the unpatched base. That patch rewrites the store's AWS region per cluster from the cluster Secret's `region` label. Unmatched, every cluster silently keeps us-west-2 and looks its secrets up in the wrong region, with a green render and a green kubeconform. ─── scripts/check-secret-store-refs.py ─── Nothing in it is a list. It reads the declaration and requires that exactly one cluster-wide store exists, that every secretStoreRef in the tree names it, that every kustomize patch target with a store kind names it, and that this catalog's ExternalSecrets agree on one apiVersion. Two readers, because half the corpus is chart source that raises on `yaml.safe_load_all`. A gate written against the parser alone reports a clean run over the four plain manifests and never sees the four in the chart. The chart reader is structure-aware — it locates the `secretStoreRef` block by its key and reads the fields from inside it, so a `name:` belonging to the target or the metadata cannot be mistaken for the store. ─── contracts/secret-store.json ─── The published half, for the four repositories this seat cannot edit: the store's name, kind and apiVersion, generated by `--write` and compared against the tree on every run. A hand-edit that disagrees with the manifest fails here — a contract that has drifted is worse than none, because the consumers asserting against it pass. The apiVersion is published beside the name because they go stale together. Five charts declared external-secrets.io/v1beta1 against a CRD that lists but does not serve it, and that passes helm, kubeconform and chart lint, because the version really is on the CRD. Only a live API server rejects it. ─── .gitignore ─── `*secret*.json` under "# Secrets and credentials" swallowed the contract. Not force-added: the rule already carries `!*secret*-store*.yaml` and `!*secret*-store*.yml`, this catalog's position that a store manifest is a reference into secrets rather than secret material. The contract is that same category, so the negation is extended to the third extension the other two already cover, with the reason at the line. ─── Registration ─── `task validate:secret-store-refs`, inside `task validate` because it is hermetic and needs no tool. CI runs it beside the ExternalSecret key gate. controls.py plants the typo one repository in this org actually shipped — `aws-secretsmanager` against `aws-secrets-manager` — in the chart source, which is the half a parser-only gate drops. reverify-gates.sh plants three: the store renamed, the patch target unmatched, and the contract hand-edited away from the manifest. 48 `run` lines, floor 45. MAX_UNCOVERED_GATES 11 -> 10: this gate arrived with its own tests, so the count holds at 10 across 26 rather than 25. --- .github/workflows/ci.yml | 11 + .gitignore | 5 + CLAUDE.md | 3 +- Taskfile.yaml | 8 +- contracts/secret-store.json | 12 + scripts/check-secret-store-refs.py | 335 ++++++++++++++++++++++++ scripts/tests/controls.py | 17 ++ scripts/tests/reverify-gates.sh | 51 +++- scripts/tests/run.py | 9 +- scripts/tests/test_secret_store_refs.py | 318 ++++++++++++++++++++++ 10 files changed, 765 insertions(+), 4 deletions(-) create mode 100644 contracts/secret-store.json create mode 100755 scripts/check-secret-store-refs.py create mode 100644 scripts/tests/test_secret_store_refs.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 93f7de8..d9b4a3b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,6 +98,17 @@ jobs: - name: ExternalSecret keys are named once and patched per cluster run: ./scripts/check-externalsecret-keys.py + # The catalog declares one ClusterSecretStore and eleven other places + # restate its name — eight secretStoreRefs, an ApplicationSet patch target, + # and five chart defaults in repositories this one does not own. An + # ExternalSecret naming a store that is not there installs and syncs + # cleanly and records SecretSyncedError on an object nothing in the install + # path reads; an unmatched kustomize patch target is not an error at all, + # so the region rewrite silently does not happen. This holds them to the + # declaration and publishes it for the consumers outside this repository. + - name: Every secret-store reference resolves to the declared store + run: ./scripts/check-secret-store-refs.py + # A CUR export runs an explicit column projection, so its delivered set is # far narrower than the CUR specification and every documented column name # looks plausible in a panel query. The dashboard gate below reads diff --git a/.gitignore b/.gitignore index f4d0835..f76bd4c 100644 --- a/.gitignore +++ b/.gitignore @@ -13,6 +13,11 @@ *secret*.json !*secret*-store*.yaml !*secret*-store*.yml +# The store's published contract — its name and apiVersion, for consumers in +# other repositories to assert against instead of restating. Same category as +# the two lines above: a description of where secrets are fetched from, holding +# none. JSON because that is the shape the org's other cross-repo pins take. +!*secret*-store*.json # ExternalSecret manifests are references into the secret store, not secret # material — they must ship with the charts that need them. !*externalsecret*.yaml diff --git a/CLAUDE.md b/CLAUDE.md index 054ca41..3b0f4d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -99,6 +99,7 @@ task validate:sync-waves # Assert the documented sync-wave category or task validate:label-values # Every k8s label value satisfies the API server's grammar task validate:policy-admission # Prove no addon is denied by the Enforce-tier Kyverno policies (+ exclusion-list parity) task validate:externalsecret-keys # Each ExternalSecret names its remote secret once, and the delivering appset patches it +task validate:secret-store-refs # Every secret-store reference names the one store this catalog declares, and the published contract states it task validate:dashboards # grafana.com dashboard ids exist and are AMG-saveable task validate:athena-panel-columns # Every column a CUR panel names is one the export delivers task validate:fork-safety # No hardcoded catalog repoURL in applied ApplicationSets (report-only locally) @@ -112,7 +113,7 @@ task validate:image-vulnerabilities # Every fixed CRITICAL in a rendered image i `task validate` runs the structural gates (lint, kustomize build, helm-render, ApplicationSet schema, sync-wave ordering, appset render, policy-admission, -dashboards, fork-safety). CI runs those plus several gates that have **no local +secret-store references, dashboards, fork-safety). CI runs those plus several gates that have **no local `task` target**, and one that has a target but is deliberately outside the aggregate, so a clean `task validate` is necessary but not sufficient: diff --git a/Taskfile.yaml b/Taskfile.yaml index 08555e2..87f76ae 100644 --- a/Taskfile.yaml +++ b/Taskfile.yaml @@ -195,13 +195,18 @@ tasks: cmds: - ./scripts/check-externalsecret-keys.py + validate:secret-store-refs: + desc: "Secret-store gate — every reference names the one store this catalog declares, and the published contract states it" + cmds: + - ./scripts/check-secret-store-refs.py + validate:athena-panel-columns: desc: "Athena panel gate — every column a CUR panel names is one the export delivers" cmds: - ./scripts/check-athena-panel-columns.py validate: - desc: Run all validations (lint, build, helm-render, appset schema, sync waves, label values, appset render, policy admission, externalsecret keys, athena panel columns, dashboard/fork-safety) + desc: Run all validations (lint, build, helm-render, appset schema, sync waves, label values, appset render, policy admission, secret refs, athena panels, dashboard/fork-safety) deps: - lint:yaml - lint:python @@ -213,6 +218,7 @@ tasks: - validate:appset-render - validate:policy-admission - validate:externalsecret-keys + - validate:secret-store-refs - validate:athena-panel-columns - validate:dashboards - validate:fork-safety diff --git a/contracts/secret-store.json b/contracts/secret-store.json new file mode 100644 index 0000000..36fc28e --- /dev/null +++ b/contracts/secret-store.json @@ -0,0 +1,12 @@ +{ + "_generated": "scripts/check-secret-store-refs.py --write; the source of truth is the manifest under addons/bootstrap/secret-stores/. Edit that, then regenerate.", + "_purpose": "Consumers outside this repository assert their chart defaults against these values instead of restating them.", + "clusterSecretStore": { + "apiVersion": "external-secrets.io/v1", + "kind": "ClusterSecretStore", + "name": "aws-secrets-manager" + }, + "externalSecret": { + "apiVersion": "external-secrets.io/v1" + } +} diff --git a/scripts/check-secret-store-refs.py b/scripts/check-secret-store-refs.py new file mode 100755 index 0000000..c349fdd --- /dev/null +++ b/scripts/check-secret-store-refs.py @@ -0,0 +1,335 @@ +#!/usr/bin/env python3 +"""One place holds the secret-store name; everything else is compared to it. + +WHY THIS EXISTS + +This catalog declares one cluster-wide store — a `ClusterSecretStore` named in +`addons/bootstrap/secret-stores/`. Ten other places in this repository restate +that name, and five charts in four other repositories hardcode it as a chart +default. Nothing held any of them equal. + +A name restated in eleven places is eleven chances to disagree, and the +disagreement is silent in every direction that matters: + + * An `ExternalSecret` naming a store that does not exist installs cleanly, + syncs cleanly, and records `SecretSyncedError` on its own status while never + creating the target Secret. The workload then mounts a Secret that is not + there, and its symptom is a missing environment variable — a sentence that + names neither the store nor the typo. One repository shipped + `aws-secretsmanager` against `aws-secrets-manager` exactly this way. + + * An ApplicationSet patch whose `target` names a store that does not exist is + worse, because kustomize does not consider it an error. `kustomize build` + exits 0 and emits the unpatched base. The patch on the store here rewrites + its AWS region per cluster; unmatched, every cluster silently keeps the + base's `us-west-2` and looks its secrets up in the wrong region. + +WHAT IT CHECKS + +Everything is derived from the declaration, and nothing is listed here: + + * exactly one cluster-wide store is declared, because a contract naming one + of several is a contract that has not decided; + * every `secretStoreRef` in the tree names a store this catalog declares; + * every ApplicationSet patch whose `target.kind` is a store kind names it too, + which is the reference no render can fail on; + * every `ExternalSecret` agrees on an apiVersion, so there is one to publish; + * `contracts/secret-store.json` states what the tree states. That file is the + published half — the four repositories this seat cannot edit read it to + assert their chart defaults, the way the other cross-repo pins in this org + work. Regenerate with `--write`; a hand-edit that disagrees with the + manifest fails here rather than at a cluster. + +The version is published beside the name because they go stale together and for +the same reason: a chart pinned to this catalog's store was pinned when the +catalog's shape was different. A declared-but-unserved apiVersion passes helm, +kubeconform and chart lint — the CRD really does list it — and fails only at a +live API server, so publishing the one this catalog uses is what a consumer can +check offline. + +WHAT IT DOES NOT CHECK + + * Whether the consumers in other repositories read the contract. They are not + in this repository and this gate cannot see them; publishing is the half + that lives here. + * Whether the store WORKS — that the region resolves, that Pod Identity is + bound, that Secrets Manager holds the keys. Those are facts about a cluster + and an AWS account. + * Whether an apiVersion is SERVED. That is knowable only from a live API + server; this asserts the catalog is internally consistent about which one it + names. +""" + +from __future__ import annotations + +import argparse +import importlib.util +import json +import pathlib +import re +import sys + +_gl = pathlib.Path(__file__).resolve().parent / "gatelib.py" +_gs = importlib.util.spec_from_file_location("gatelib", _gl) +assert _gs and _gs.loader, f"{_gl} is not loadable as a module" +gatelib = importlib.util.module_from_spec(_gs) +sys.modules["gatelib"] = gatelib +_gs.loader.exec_module(gatelib) + +ROOT = pathlib.Path(__file__).resolve().parent.parent +APPSET_DIR = ROOT / "applicationsets" +CONTRACT = ROOT / "contracts" / "secret-store.json" + +CLUSTER_STORE = "ClusterSecretStore" +NAMESPACED_STORE = "SecretStore" +STORE_KINDS = (CLUSTER_STORE, NAMESPACED_STORE) +EXTERNAL_SECRET = "ExternalSecret" + +SKIP_DIRS = {"rendered", ".git", "node_modules"} + +# A `secretStoreRef` in Go-template chart source. Half the consumers here are +# chart templates that do not parse as YAML, and dropping them would leave the +# gate reporting on the half that happens to be plain manifests. +HELM_REF = re.compile( + r"^(?P\s*)secretStoreRef:\s*$\n" + r"(?P(?:(?P=indent)\s+\S.*$\n?)*)", re.M) +FIELD = re.compile(r"^\s*(?Pname|kind):\s*(?P\S.*?)\s*$", re.M) +HELM_API = re.compile(r"^apiVersion:\s*(?Pexternal-secrets\.io/\S+)\s*$", re.M) +HELM_KIND = re.compile(r"^kind:\s*(?P\S+)\s*$", re.M) + + +def rel(path: pathlib.Path) -> str: + try: + return str(path.relative_to(ROOT)) + except ValueError: + return str(path) + + +def tracked_yaml() -> list[pathlib.Path]: + out = [] + for path in sorted(ROOT.rglob("*.y*ml")): + if any(part in SKIP_DIRS for part in path.relative_to(ROOT).parts): + continue + out.append(path) + return out + + +def documents(path: pathlib.Path) -> list[dict]: + """Parsed documents, or [] for chart source that is Go-template text.""" + if gatelib.is_helm_template(path): + return [] + return [d for d in gatelib.read_yaml_all(path) if isinstance(d, dict)] + + +def store_refs(node, path: pathlib.Path, out: list): + """Every `secretStoreRef` under a parsed document, at any depth.""" + if isinstance(node, dict): + ref = node.get("secretStoreRef") + if isinstance(ref, dict): + out.append((path, str(ref.get("kind") or CLUSTER_STORE), + str(ref.get("name") or ""))) + for value in node.values(): + store_refs(value, path, out) + elif isinstance(node, list): + for item in node: + store_refs(item, path, out) + + +def helm_store_refs(path: pathlib.Path) -> list[tuple[pathlib.Path, str, str]]: + """The same reference, read out of chart source as text. + + Structure-aware rather than a bare grep for the name: the block is located + by its key and the fields are read from inside it, so a `name:` belonging to + some other object cannot be mistaken for the store. + """ + out = [] + text = path.read_text(encoding="utf-8") + for block in HELM_REF.finditer(text): + fields = {m.group("key"): m.group("value") + for m in FIELD.finditer(block.group("body"))} + out.append((path, fields.get("kind", CLUSTER_STORE), + fields.get("name", ""))) + return out + + +def patch_targets() -> list[tuple[pathlib.Path, str, str]]: + """(appset, kind, name) for every kustomize patch target naming a store.""" + out = [] + for path in sorted(APPSET_DIR.rglob("*.y*ml")): + for doc in documents(path): + if doc.get("kind") != "ApplicationSet": + continue + spec = ((doc.get("spec") or {}).get("template") or {}).get("spec") or {} + sources = list(spec.get("sources") or []) + if isinstance(spec.get("source"), dict): + sources.append(spec["source"]) + for source in sources: + if not isinstance(source, dict): + continue + for patch in (source.get("kustomize") or {}).get("patches") or []: + target = (patch or {}).get("target") or {} + kind = str(target.get("kind") or "") + if kind in STORE_KINDS and target.get("name"): + out.append((path, kind, str(target["name"]))) + return out + + +def survey(): + """The whole population, both readers, in one walk.""" + declared: dict[tuple[str, str], tuple[pathlib.Path, str]] = {} + consumers: list[tuple[pathlib.Path, str, str]] = [] + versions: dict[str, list[pathlib.Path]] = {} + + for path in tracked_yaml(): + if gatelib.is_helm_template(path): + text = path.read_text(encoding="utf-8") + if HELM_KIND.search(text) and "secretStoreRef" in text: + consumers.extend(helm_store_refs(path)) + for m in HELM_API.finditer(text): + head = text[:m.start()].rfind("\n---") + block = text[max(head, 0):m.start() + 400] + if re.search(rf"^kind:\s*{EXTERNAL_SECRET}\s*$", block, re.M): + versions.setdefault(m.group("api"), []).append(path) + continue + for doc in documents(path): + kind = str(doc.get("kind") or "") + name = str((doc.get("metadata") or {}).get("name") or "") + api = str(doc.get("apiVersion") or "") + if kind in STORE_KINDS and name: + declared[(kind, name)] = (path, api) + if kind == EXTERNAL_SECRET and api: + versions.setdefault(api, []).append(path) + store_refs(doc, path, consumers) + return declared, consumers, versions + + +def build_contract(declared, versions) -> dict: + (kind, name), (_path, api) = next(iter(declared.items())) + return { + "_generated": "scripts/check-secret-store-refs.py --write; the source of " + "truth is the manifest under addons/bootstrap/secret-stores/. " + "Edit that, then regenerate.", + "_purpose": "Consumers outside this repository assert their chart " + "defaults against these values instead of restating them.", + "clusterSecretStore": {"apiVersion": api, "kind": kind, "name": name}, + "externalSecret": {"apiVersion": sorted(versions)[0]}, + } + + +def main(argv: list[str] | None = None) -> int: + """`argv` is a parameter so this is callable as a library. + + Reading `sys.argv` directly makes the entry point untestable: under a test + runner it parses the runner's own flags and exits. + """ + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--write", action="store_true", + help="regenerate the published contract from the tree") + args = parser.parse_args(argv) + + declared, consumers, versions = survey() + + if not declared: + print(f"Cannot run: no {' or '.join(STORE_KINDS)} is declared anywhere " + f"under {rel(ROOT)}, so there is no name for anything to be " + f"compared against.") + print("This gate examined nothing, which is not the same as finding nothing.") + return gatelib.CANNOT_RUN + if not consumers: + print("Cannot run: no secretStoreRef found, so this gate read no " + "consumer. A catalog whose references all resolve reports the " + "same thing.") + return gatelib.CANNOT_RUN + + failures: list[str] = [] + + cluster_stores = {k: v for k, v in declared.items() if k[0] == CLUSTER_STORE} + if len(cluster_stores) != 1: + names = ", ".join(f"{n} ({rel(p)})" for (_k, n), (p, _a) in + sorted(cluster_stores.items())) + print(f"FAIL {len(cluster_stores)} {CLUSTER_STORE}(s) declared: " + f"{names or 'none'}. The published contract names one store, so a " + f"catalog declaring several has not decided which one a consumer " + f"in another repository should pin to.") + return 1 + + for path, kind, name in consumers: + if not name: + failures.append( + f"{rel(path)}: a secretStoreRef names no store at all, so the " + f"ExternalSecret resolves against nothing.") + elif (kind, name) not in declared: + failures.append( + f"{rel(path)}: secretStoreRef names {kind}/{name} and this " + f"catalog declares no such store. External Secrets accepts the " + f"object, records SecretSyncedError on its status, and never " + f"creates the target Secret — the workload mounts a Secret that " + f"is not there.") + + for path, kind, name in patch_targets(): + if (kind, name) not in declared: + failures.append( + f"{rel(path)}: a kustomize patch targets {kind}/{name} and this " + f"catalog declares no such store. kustomize does not treat an " + f"unmatched target as an error — the build exits 0 and emits the " + f"unpatched base, so every cluster silently keeps whatever the " + f"base said.") + + if len(versions) > 1: + listed = "; ".join( + f"{api} in {', '.join(sorted({rel(p) for p in paths}))}" + for api, paths in sorted(versions.items())) + failures.append( + f"{EXTERNAL_SECRET}s in this catalog declare {len(versions)} " + f"apiVersions: {listed}. There is no single version to publish, and " + f"a consumer pinning to one of them is pinning to a coin flip.") + + if failures: + print(f"{len(failures)} secret-store reference problem(s):\n") + for f in failures: + print(f" {f}") + return 1 + + want = build_contract(cluster_stores, versions) + if args.write: + CONTRACT.parent.mkdir(parents=True, exist_ok=True) + CONTRACT.write_text(json.dumps(want, indent=2) + "\n", encoding="utf-8") + print(f"✓ wrote {rel(CONTRACT)}") + return 0 + + if not CONTRACT.exists(): + print(f"FAIL {rel(CONTRACT)} does not exist. It is the half of this " + f"that consumers outside this repository can read; without it they " + f"have nothing to compare their chart defaults against. Run " + f"`{rel(pathlib.Path(__file__))} --write`.") + return 1 + try: + have = json.loads(CONTRACT.read_text(encoding="utf-8")) + except json.JSONDecodeError as err: + print(f"FAIL {rel(CONTRACT)} is not valid JSON: {err}. A contract a " + f"consumer cannot parse is one it will skip.") + return 1 + if have != want: + print(f"FAIL {rel(CONTRACT)} does not state what this tree states.") + print(f" published: {json.dumps(have.get('clusterSecretStore'))}" + f" / {json.dumps(have.get('externalSecret'))}") + print(f" declared: {json.dumps(want['clusterSecretStore'])}" + f" / {json.dumps(want['externalSecret'])}") + print(" A contract that has drifted from the manifest is worse " + "than none: consumers assert against it and pass.") + print(f" Regenerate with `{rel(pathlib.Path(__file__))} --write`.") + return 1 + + store = want["clusterSecretStore"] + print(f"✓ every secret-store reference resolves to the one store this " + f"catalog declares: {store['kind']}/{store['name']} " + f"({store['apiVersion']}), named by {len(consumers)} secretStoreRef(s) " + f"and {len(patch_targets())} ApplicationSet patch target(s), published " + f"in {rel(CONTRACT)}") + print(" whether repositories outside this one read that contract, and " + "whether the store works against a real account, are outside this claim") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/controls.py b/scripts/tests/controls.py index f4ea12b..447d2ba 100755 --- a/scripts/tests/controls.py +++ b/scripts/tests/controls.py @@ -504,6 +504,21 @@ def m_alert_severity_routes(root): marker=f"severity: {MARKER}-urgent") +def m_secret_store_refs(root): + """Typo the store in the chart source that does not parse as YAML. + + The half of the corpus a gate written against `yaml.safe_load_all` drops + while reporting a clean run over the rest, and the exact shape one repo in + this org shipped: `aws-secretsmanager` against `aws-secrets-manager`. The + ExternalSecret installs, syncs, records SecretSyncedError on its own status, + and never creates the Secret the workload mounts. + """ + return _sub(root, "catalog/druid/chart/templates/externalsecret.yaml", + " name: aws-secrets-manager\n", + " name: aws-secretsmanager\n", + marker="aws-secretsmanager") + + def m_chart_deprecation(root): """A recorded chart that nothing pins, which the offline gate must reject.""" import json @@ -579,6 +594,8 @@ def m_env_coverage(root): "check-alert-coverage.py": ("an alert on an unexported KSM field", m_alert_coverage), "check-alert-severity-routes.py": ("a severity label that routes nowhere", m_alert_severity_routes), + "check-secret-store-refs.py": ("a reference to a store the catalog does not " + "declare", m_secret_store_refs), } diff --git a/scripts/tests/reverify-gates.sh b/scripts/tests/reverify-gates.sh index 04fcc63..03987ab 100755 --- a/scripts/tests/reverify-gates.sh +++ b/scripts/tests/reverify-gates.sh @@ -158,6 +158,7 @@ run 0 "check-renovate-coverage.py" ./scripts/check-renovate-coverage.py run 0 "check-ai-config.py" ./scripts/check-ai-config.py run 0 "check-alert-severity-routes.py" ./scripts/check-alert-severity-routes.py run 0 "check-env-coverage.py" ./scripts/check-env-coverage.py +run 0 "check-secret-store-refs.py" ./scripts/check-secret-store-refs.py run 0 "check-named-things.py" ./scripts/check-named-things.py run 0 "check-policy-validity.py" ./scripts/check-policy-validity.py run 0 "no-placeholders.sh" ./scripts/no-placeholders.sh @@ -403,6 +404,54 @@ run nonzero "alert-severity-routes: the routing tree stops shipping" \ ./scripts/check-alert-severity-routes.py res $F +# The store is renamed and the eight references that name it are not. Nothing +# else in this tree sees it: helm renders, kustomize builds, kubeconform passes, +# and the ExternalSecrets record SecretSyncedError on an object nothing in the +# install path reads. +F=addons/bootstrap/secret-stores/cluster-secret-store.yaml; mut $F +python3 - "$F" <<'PYQ' +import pathlib,sys +p=pathlib.Path(sys.argv[1]); s=p.read_text() +m=s.replace(" name: aws-secrets-manager\n", " name: aws-sm\n", 1) +assert m!=s, "mutation did not land" +p.write_text(m) +print(" renamed the ClusterSecretStore the whole catalog references") +PYQ +run nonzero "secret-store-refs: the store is renamed and nothing follows it" \ + ./scripts/check-secret-store-refs.py +res $F + +# The reference kustomize will not fail on. An unmatched patch target is not an +# error: the build exits 0 and emits the base, so every cluster keeps us-west-2 +# and looks its secrets up in the wrong region. +F=applicationsets/secret-stores.yaml; mut $F +python3 - "$F" <<'PYQ' +import pathlib,sys +p=pathlib.Path(sys.argv[1]); s=p.read_text() +m=s.replace(" name: aws-secrets-manager\n", + " name: aws-secretsmanager\n", 1) +assert m!=s, "mutation did not land" +p.write_text(m) +print(" pointed the region patch at a store that does not exist") +PYQ +run nonzero "secret-store-refs: an ApplicationSet patch target stops matching" \ + ./scripts/check-secret-store-refs.py +res $F + +# The published half. A contract that has drifted from the manifest is worse +# than none, because the repositories asserting against it pass. +F=contracts/secret-store.json; mut $F +python3 - "$F" <<'PYQ' +import pathlib,sys,json +p=pathlib.Path(sys.argv[1]); d=json.loads(p.read_text()) +d["clusterSecretStore"]["name"]="aws-secretsmanager" +p.write_text(json.dumps(d, indent=2)+"\n") +print(" hand-edited the published contract away from the manifest") +PYQ +run nonzero "secret-store-refs: the published contract drifts from the tree" \ + ./scripts/check-secret-store-refs.py +res $F + F=addons/bootstrap/cert-manager/values-hub.yaml; mut $F; rm -f $F run nonzero "env-coverage: deleted hub delta" ./scripts/check-env-coverage.py res $F @@ -515,7 +564,7 @@ echo "RESULT pass=$pass fail=$fail" # The harness owes the same assertion it demands of the gates: with every `run` # line deleted it would report pass=0 fail=0 and exit 0, which is a green run # over nothing checked. -MIN_CHECKS=41 +MIN_CHECKS=45 total=$((pass + fail)) if [ "$total" -lt "$MIN_CHECKS" ]; then echo "FAIL ran $total check(s), under the floor of $MIN_CHECKS — this harness" diff --git a/scripts/tests/run.py b/scripts/tests/run.py index c950e81..d4293a3 100755 --- a/scripts/tests/run.py +++ b/scripts/tests/run.py @@ -62,6 +62,9 @@ # The floors that keep an emptied corpus from reading as a clean one, # asserted apart from the gates that carry them. "test_corpus_floors", + # A name restated in eleven places, and the two readers it takes to find + # them all — half the corpus is chart source that does not parse as YAML. + "test_secret_store_refs", ) # A floor well under the real count. It catches "discovery found almost nothing", @@ -154,7 +157,11 @@ # ratchet that declines to move is one nobody can regress against — but it is # the weaker half of this file's claim, and the per-gate floors and the controls # in scripts/tests/controls.py are where the real one lives. -MAX_UNCOVERED_GATES = 11 +# +# It moves by what a change actually covered, not by what the run reports. A +# gate arriving with its own tests lowers it; a gate that starts reading as +# covered because something new imports it does not. +MAX_UNCOVERED_GATES = 10 PER_GATE_FLOORS = { "scripts/check-named-things.py": 35, diff --git a/scripts/tests/test_secret_store_refs.py b/scripts/tests/test_secret_store_refs.py new file mode 100644 index 0000000..6fb5998 --- /dev/null +++ b/scripts/tests/test_secret_store_refs.py @@ -0,0 +1,318 @@ +"""Unit tests for the secret-store reference gate. + +The defect is a name restated in eleven places with nothing holding them equal, +so the failure this gate can have is a corpus that quietly loses members: a +reference it does not read is a reference that cannot disagree with anything. +Half the consumers are Go-template chart source that does not parse as YAML, +which is exactly the half a gate written against `yaml.safe_load_all` drops +while reporting a clean run over the rest. + +So these concentrate on which references are FOUND, and on the two verdicts +whose absence is silent in a cluster: an ExternalSecret naming a store that is +not there, and a kustomize patch target that stops matching. +""" + +from __future__ import annotations + +import contextlib +import io +import json +import pathlib +import re +import tempfile +import unittest + +import yaml +from gateloader import load + +gate = load("check-secret-store-refs") + +ROOT = pathlib.Path(__file__).resolve().parent.parent.parent + + +def store(name="aws-secrets-manager", api="external-secrets.io/v1", + kind="ClusterSecretStore"): + return {"apiVersion": api, "kind": kind, "metadata": {"name": name}, + "spec": {"provider": {"aws": {"service": "SecretsManager", + "region": "us-west-2"}}}} + + +def external_secret(name, store_name="aws-secrets-manager", + api="external-secrets.io/v1"): + return {"apiVersion": api, "kind": "ExternalSecret", + "metadata": {"name": name}, + "spec": {"secretStoreRef": {"name": store_name, + "kind": "ClusterSecretStore"}, + "target": {"name": name}}} + + +def appset(name, target_name="aws-secrets-manager", + target_kind="ClusterSecretStore"): + return {"apiVersion": "argoproj.io/v1alpha1", "kind": "ApplicationSet", + "metadata": {"name": name}, + "spec": {"template": {"spec": {"source": { + "path": "p", + "kustomize": {"patches": [ + {"target": {"kind": target_kind, "name": target_name}, + "patch": "- op: replace\n path: /spec/x\n value: y"}]}}}}}} + + +CHART_TEMPLATE = """apiVersion: external-secrets.io/v1 +kind: ExternalSecret +metadata: + name: {{ include "x.name" . }}-creds + labels: + {{- include "common.labels" . | indent 4 }} +spec: + refreshInterval: 1h + secretStoreRef: + name: %s + kind: ClusterSecretStore + target: + name: {{ include "x.name" . }}-creds +""" + + +class ReadingChartSource(unittest.TestCase): + """The half of the corpus that does not parse as YAML.""" + + def chart(self, store_name="aws-secrets-manager"): + d = pathlib.Path(tempfile.mkdtemp()) + (d / "Chart.yaml").write_text("apiVersion: v2\nname: x\nversion: 0.1.0\n") + (d / "templates").mkdir() + p = d / "templates" / "externalsecret.yaml" + p.write_text(CHART_TEMPLATE % store_name) + return p + + def test_the_reference_is_read_out_of_go_template_text(self): + """`yaml.safe_load_all` raises on this file. A gate that caught the + parse error and continued would report a clean run over the consumers + that happen to be plain manifests.""" + p = self.chart() + with self.assertRaises(yaml.YAMLError): + list(yaml.safe_load_all(p.read_text())) + self.assertEqual(gate.helm_store_refs(p), + [(p, "ClusterSecretStore", "aws-secrets-manager")]) + + def test_a_typo_in_chart_source_is_read_as_written(self): + p = self.chart("aws-secretsmanager") + self.assertEqual(gate.helm_store_refs(p)[0][2], "aws-secretsmanager") + + def test_a_name_outside_the_block_is_not_taken_for_the_store(self): + """The block is located by its key and the fields read from inside it. + A bare grep for `name:` would take the target's, or the metadata's.""" + p = self.chart() + p.write_text(p.read_text().replace( + " refreshInterval: 1h\n", + " refreshInterval: 1h\n decoy:\n name: not-a-store\n")) + self.assertEqual([r[2] for r in gate.helm_store_refs(p)], + ["aws-secrets-manager"]) + + +class TheVerdict(unittest.TestCase): + """main() over a planted tree.""" + + def verdict(self, docs, appsets=(), contract=None, write=False): + root = pathlib.Path(tempfile.mkdtemp()) + (root / "addons").mkdir(parents=True) + (root / "applicationsets").mkdir(parents=True) + for i, doc in enumerate(docs): + (root / "addons" / f"{i:02d}.yaml").write_text(yaml.safe_dump(doc)) + for i, doc in enumerate(appsets): + (root / "applicationsets" / f"{i:02d}.yaml").write_text( + yaml.safe_dump(doc)) + path = root / "contracts" / "secret-store.json" + if contract is not None: + path.parent.mkdir(parents=True) + path.write_text(json.dumps(contract, indent=2) + "\n") + saved = (gate.ROOT, gate.APPSET_DIR, gate.CONTRACT) + gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = ( + root, root / "applicationsets", path) + try: + with contextlib.redirect_stdout(io.StringIO()) as out: + rc = gate.main(["--write"] if write else []) + finally: + gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = saved + return rc, out.getvalue(), path + + def healthy(self): + return [store(), external_secret("a"), external_secret("b")] + + def contract_for(self, name="aws-secrets-manager", + api="external-secrets.io/v1"): + return { + "_generated": "scripts/check-secret-store-refs.py --write; the source " + "of truth is the manifest under " + "addons/bootstrap/secret-stores/. Edit that, then " + "regenerate.", + "_purpose": "Consumers outside this repository assert their chart " + "defaults against these values instead of restating them.", + "clusterSecretStore": {"apiVersion": api, + "kind": "ClusterSecretStore", "name": name}, + "externalSecret": {"apiVersion": api}, + } + + def test_a_consistent_catalog_passes(self): + """The control. Without it every case below could be failing for a + reason the case did not plant.""" + rc, out, _ = self.verdict(self.healthy(), contract=self.contract_for()) + self.assertEqual(rc, 0, out) + + def test_an_external_secret_naming_an_undeclared_store_is_reported(self): + docs = self.healthy() + docs[1]["spec"]["secretStoreRef"]["name"] = "aws-secretsmanager" + rc, out, _ = self.verdict(docs, contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertIn("SecretSyncedError", out) + + def test_a_patch_target_that_stops_matching_is_reported(self): + """kustomize exits 0 on an unmatched target and emits the unpatched + base, so nothing downstream can fail on this.""" + rc, out, _ = self.verdict( + self.healthy(), appsets=[appset("s", "aws-secretsmanager")], + contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertIn("does not treat an unmatched target as an error", out) + + def test_a_patch_target_that_matches_is_not_reported(self): + rc, out, _ = self.verdict(self.healthy(), appsets=[appset("s")], + contract=self.contract_for()) + self.assertEqual(rc, 0, out) + + def test_a_reference_naming_no_store_at_all_is_reported(self): + docs = self.healthy() + docs[1]["spec"]["secretStoreRef"]["name"] = "" + rc, out, _ = self.verdict(docs, contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertIn("names no store at all", out) + + def test_two_external_secret_versions_leave_nothing_to_publish(self): + docs = self.healthy() + docs[2]["apiVersion"] = "external-secrets.io/v1beta1" + rc, out, _ = self.verdict(docs, contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertIn("pinning to a coin flip", out) + + def test_two_cluster_stores_leave_the_contract_undecided(self): + docs = self.healthy() + [store(name="aws-secrets-manager-2")] + rc, out, _ = self.verdict(docs, contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertIn("has not decided", out) + + def test_a_contract_that_drifted_from_the_manifest_is_reported(self): + """The failure mode of publishing: consumers assert against it and + pass, which is worse than having nothing to assert against.""" + rc, out, _ = self.verdict( + self.healthy(), contract=self.contract_for("aws-secretsmanager")) + self.assertEqual(rc, 1, out) + self.assertIn("does not state what this tree states", out) + + def test_a_missing_contract_is_reported(self): + rc, out, _ = self.verdict(self.healthy()) + self.assertEqual(rc, 1, out) + self.assertIn("does not exist", out) + + def test_a_contract_that_does_not_parse_is_reported(self): + """A contract a consumer cannot parse is one it skips, which lands it + back where it started: restating the name and hoping.""" + _rc, _out, path = self.verdict(self.healthy(), + contract=self.contract_for()) + path.write_text("{not json") + root = path.parent.parent + saved = (gate.ROOT, gate.APPSET_DIR, gate.CONTRACT) + gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = ( + root, root / "applicationsets", path) + try: + with contextlib.redirect_stdout(io.StringIO()) as out: + rc = gate.main([]) + finally: + gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = saved + self.assertEqual(rc, 1) + self.assertIn("not valid JSON", out.getvalue()) + + def test_write_regenerates_the_contract_from_the_tree(self): + rc, out, path = self.verdict(self.healthy(), write=True) + self.assertEqual(rc, 0, out) + written = json.loads(path.read_text()) + self.assertEqual(written["clusterSecretStore"]["name"], + "aws-secrets-manager") + self.assertEqual(written["externalSecret"]["apiVersion"], + "external-secrets.io/v1") + + def test_no_store_declared_cannot_run(self): + """Exit 2. A tree with no store is a gate with nothing to compare + against, which reports the same as a tree whose references all resolve.""" + rc, out, _ = self.verdict([external_secret("a")], + contract=self.contract_for()) + self.assertEqual(rc, gate.gatelib.CANNOT_RUN, out) + self.assertIn("examined nothing", out) + + def test_no_consumer_at_all_cannot_run(self): + rc, out, _ = self.verdict([store()], contract=self.contract_for()) + self.assertEqual(rc, gate.gatelib.CANNOT_RUN, out) + self.assertIn("read no consumer", out) + + +class TheShippedCatalog(unittest.TestCase): + """Over the tree, so a reference added later is checked here.""" + + @classmethod + def setUpClass(cls): + cls.declared, cls.consumers, cls.versions = gate.survey() + + def test_the_catalog_passes(self): + with contextlib.redirect_stdout(io.StringIO()) as out: + self.assertEqual(gate.main([]), 0, out.getvalue()) + + def test_every_reference_the_tree_holds_is_in_the_corpus(self): + """Derived per file rather than counted. A second, cruder reading finds + the files; this asserts the gate's own two readers found the same ones, + so a reader that stops matching fails here rather than shrinking the + corpus quietly. + + The key, not the word: prose naming `secretStoreRef` is not a reference, + and matching the bare substring picks up this repository's own CI + comments about the gate. + """ + key = re.compile(r"^\s*secretStoreRef:\s*$", re.M) + grepped = {path for path in gate.tracked_yaml() + if key.search(path.read_text(encoding="utf-8"))} + self.assertEqual({p for p, _k, _n in self.consumers}, grepped) + + def test_the_corpus_spans_both_readers(self): + """One of them is chart source. A catalog whose consumers were all plain + manifests would pass a gate with no Go-template reader at all.""" + helm = [p for p, _k, _n in self.consumers + if gate.gatelib.is_helm_template(p)] + plain = [p for p, _k, _n in self.consumers + if not gate.gatelib.is_helm_template(p)] + self.assertTrue(helm, "no chart-source consumer — the text reader is " + "exercised by nothing on this tree") + self.assertTrue(plain) + + def test_the_published_contract_is_the_declared_store(self): + published = json.loads(gate.CONTRACT.read_text(encoding="utf-8")) + (kind, name), (path, api) = next( + iter({k: v for k, v in self.declared.items() + if k[0] == gate.CLUSTER_STORE}.items())) + self.assertEqual(published["clusterSecretStore"], + {"apiVersion": api, "kind": kind, "name": name}, + f"{gate.rel(gate.CONTRACT)} and {gate.rel(path)} " + f"disagree about the store this catalog owns") + + def test_the_contract_is_not_ignored_by_git(self): + """A published contract that git refuses to track is one no consumer can + fetch. `.gitignore` carries `*secret*.json` under its secrets rules, and + the file's own name matches it.""" + import subprocess + proc = subprocess.run( + ["git", "-C", str(ROOT), "ls-files", "--error-unmatch", + str(gate.CONTRACT.relative_to(ROOT))], + capture_output=True, text=True) + self.assertEqual(proc.returncode, 0, + f"{gate.rel(gate.CONTRACT)} is not tracked: " + f"{proc.stderr.strip()}") + + +if __name__ == "__main__": + unittest.main() From b7e887d18255db98e1470893d69bcd6d40f3767a Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Sat, 5 Sep 2026 00:48:56 -0700 Subject: [PATCH 2/6] Echo no value this gate has not verified is a name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL raised four high `py/clear-text-logging-sensitive-data` on the secret-store gate. Closed as a property rather than by tuning the four lines, because one of the flows is real. ─── The flow that was real ─── `contracts/secret-store.json` is parsed as arbitrary JSON and the drift message echoed the parsed object back with `json.dumps`. Whatever is in that file is what the message repeats, and CI keeps the log. "It only ever holds a store name" is the assumption a gate about secret plumbing should not be making about a file named for secrets — and the same reasoning covers the manifests, whose values also arrive from disk. ─── The property ─── A value read from a file reaches the output only once it has been verified to be a Kubernetes object name, kind, or group version, checked against the API server's own grammar with `fullmatch`. A string that matches is lowercase alphanumerics with hyphens and dots, at most 253 characters, carrying no separator a credential needs. One that does not match is reported by its field rather than by its content. `printable()` is where that happens, and the stand-in it substitutes is a constant rather than a truncation: a prefix of a value that is not a name is still whatever that value was. ─── The drift message ─── It no longer echoes the published side at all. It names the field that disagrees and prints the DECLARED value — the tree's own, and the one the reader has to act on anyway: clusterSecretStore.name: the manifest declares aws-secrets-manager; the contract publishes something else Strictly more useful than the pair it replaced, and there is no longer a path from the contract file to the output. ─── Proof ─── An AWS-key-shaped string planted in the contract, in a consumer's secretStoreRef, in an ApplicationSet patch target, and as an apiVersion. Each run asserts the output does not contain it, and that the message still names what the reader needs. Four mutants killed by those tests: `printable` returning the value unchecked, matching with `search` instead of `fullmatch`, substituting a truncation instead of the constant, and the drift path dumping the published side again. --- scripts/check-secret-store-refs.py | 65 +++++++++++++++--- scripts/tests/test_secret_store_refs.py | 87 +++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 10 deletions(-) diff --git a/scripts/check-secret-store-refs.py b/scripts/check-secret-store-refs.py index c349fdd..a42c55f 100755 --- a/scripts/check-secret-store-refs.py +++ b/scripts/check-secret-store-refs.py @@ -87,6 +87,23 @@ SKIP_DIRS = {"rendered", ".git", "node_modules"} +# What this gate is allowed to print out of a file it read. +# +# Every value here arrives from disk, and one of those files is named for +# secrets — `contracts/secret-store.json` is parsed as arbitrary JSON, so a +# value put there ends up in a message. "It only ever holds a store name" is +# the assumption that fails, and it fails into a log. +# +# So a value is echoed only once it is a Kubernetes object name or a group +# version, checked against the API server's own grammar. A string matching +# these is lowercase alphanumerics with hyphens and dots, at most 253 +# characters and carrying no separator a credential needs; one that does not +# match is reported by its field rather than by its content. +OBJECT_NAME = re.compile(r"[a-z0-9](?:[a-z0-9.-]{0,251}[a-z0-9])?") +KIND = re.compile(r"[A-Z][A-Za-z0-9]{0,62}") +GROUP_VERSION = re.compile(r"[a-z0-9][a-z0-9.-]{0,62}/v[0-9]+(?:(?:alpha|beta)[0-9]+)?") +UNPRINTABLE = "" + # A `secretStoreRef` in Go-template chart source. Half the consumers here are # chart templates that do not parse as YAML, and dropping them would leave the # gate reporting on the half that happens to be plain manifests. @@ -98,6 +115,16 @@ HELM_KIND = re.compile(r"^kind:\s*(?P\S+)\s*$", re.M) +def printable(value: object, grammar: re.Pattern[str] = OBJECT_NAME) -> str: + """`value` if it is a name, else a fixed stand-in. + + The stand-in is a constant rather than a truncation: a prefix of a value + that is not a name is still whatever that value was. + """ + text = value if isinstance(value, str) else "" + return text if grammar.fullmatch(text) else UNPRINTABLE + + def rel(path: pathlib.Path) -> str: try: return str(path.relative_to(ROOT)) @@ -245,7 +272,7 @@ def main(argv: list[str] | None = None) -> int: cluster_stores = {k: v for k, v in declared.items() if k[0] == CLUSTER_STORE} if len(cluster_stores) != 1: - names = ", ".join(f"{n} ({rel(p)})" for (_k, n), (p, _a) in + names = ", ".join(f"{printable(n)} ({rel(p)})" for (_k, n), (p, _a) in sorted(cluster_stores.items())) print(f"FAIL {len(cluster_stores)} {CLUSTER_STORE}(s) declared: " f"{names or 'none'}. The published contract names one store, so a " @@ -260,7 +287,8 @@ def main(argv: list[str] | None = None) -> int: f"ExternalSecret resolves against nothing.") elif (kind, name) not in declared: failures.append( - f"{rel(path)}: secretStoreRef names {kind}/{name} and this " + f"{rel(path)}: secretStoreRef names {printable(kind, KIND)}/" + f"{printable(name)} and this " f"catalog declares no such store. External Secrets accepts the " f"object, records SecretSyncedError on its status, and never " f"creates the target Secret — the workload mounts a Secret that " @@ -269,7 +297,8 @@ def main(argv: list[str] | None = None) -> int: for path, kind, name in patch_targets(): if (kind, name) not in declared: failures.append( - f"{rel(path)}: a kustomize patch targets {kind}/{name} and this " + f"{rel(path)}: a kustomize patch targets {printable(kind, KIND)}/" + f"{printable(name)} and this " f"catalog declares no such store. kustomize does not treat an " f"unmatched target as an error — the build exits 0 and emits the " f"unpatched base, so every cluster silently keeps whatever the " @@ -277,7 +306,8 @@ def main(argv: list[str] | None = None) -> int: if len(versions) > 1: listed = "; ".join( - f"{api} in {', '.join(sorted({rel(p) for p in paths}))}" + f"{printable(api, GROUP_VERSION)} in " + f"{', '.join(sorted({rel(p) for p in paths}))}" for api, paths in sorted(versions.items())) failures.append( f"{EXTERNAL_SECRET}s in this catalog declare {len(versions)} " @@ -311,10 +341,23 @@ def main(argv: list[str] | None = None) -> int: return 1 if have != want: print(f"FAIL {rel(CONTRACT)} does not state what this tree states.") - print(f" published: {json.dumps(have.get('clusterSecretStore'))}" - f" / {json.dumps(have.get('externalSecret'))}") - print(f" declared: {json.dumps(want['clusterSecretStore'])}" - f" / {json.dumps(want['externalSecret'])}") + # The published side is named, not echoed. It is parsed as arbitrary + # JSON out of a file, so what a message would be repeating is whatever + # somebody put there; the declared side is the tree's own and is what + # the reader has to act on anyway. + for section, fields in (("clusterSecretStore", ("apiVersion", "kind", "name")), + ("externalSecret", ("apiVersion",))): + published = have.get(section) + if not isinstance(published, dict): + print(f" {section}: absent from the published contract") + continue + for field in fields: + grammar = {"apiVersion": GROUP_VERSION, + "kind": KIND}.get(field, OBJECT_NAME) + if published.get(field) != want[section][field]: + print(f" {section}.{field}: the manifest declares " + f"{printable(want[section][field], grammar)}; the " + f"contract publishes something else") print(" A contract that has drifted from the manifest is worse " "than none: consumers assert against it and pass.") print(f" Regenerate with `{rel(pathlib.Path(__file__))} --write`.") @@ -322,8 +365,10 @@ def main(argv: list[str] | None = None) -> int: store = want["clusterSecretStore"] print(f"✓ every secret-store reference resolves to the one store this " - f"catalog declares: {store['kind']}/{store['name']} " - f"({store['apiVersion']}), named by {len(consumers)} secretStoreRef(s) " + f"catalog declares: {printable(store['kind'], KIND)}/" + f"{printable(store['name'])} " + f"({printable(store['apiVersion'], GROUP_VERSION)}), named by " + f"{len(consumers)} secretStoreRef(s) " f"and {len(patch_targets())} ApplicationSet patch target(s), published " f"in {rel(CONTRACT)}") print(" whether repositories outside this one read that contract, and " diff --git a/scripts/tests/test_secret_store_refs.py b/scripts/tests/test_secret_store_refs.py index 6fb5998..36b35c7 100644 --- a/scripts/tests/test_secret_store_refs.py +++ b/scripts/tests/test_secret_store_refs.py @@ -253,6 +253,93 @@ def test_no_consumer_at_all_cannot_run(self): self.assertIn("read no consumer", out) +HOSTILE = "AKIAIOSFODNN7EXAMPLE/wJalrXUtnFEMI+K7MDENG+bPxRfiCY" + + +class NothingUnverifiedIsEchoed(unittest.TestCase): + """A value this gate did not verify is a name never reaches the output. + + Every value here arrives from disk, and one of those files is named for + secrets: `contracts/secret-store.json` is parsed as arbitrary JSON, so + whatever somebody puts in it is what a message would repeat. "It only ever + holds a store name" is the assumption that fails, and it fails into a log + that CI keeps. + """ + + def test_a_real_name_is_printed_as_written(self): + self.assertEqual(gate.printable("aws-secrets-manager"), + "aws-secrets-manager") + self.assertEqual(gate.printable("ClusterSecretStore", gate.KIND), + "ClusterSecretStore") + self.assertEqual( + gate.printable("external-secrets.io/v1beta1", gate.GROUP_VERSION), + "external-secrets.io/v1beta1") + + def test_a_credential_shaped_value_is_withheld(self): + self.assertEqual(gate.printable(HOSTILE), gate.UNPRINTABLE) + + def test_a_value_carrying_a_separator_a_name_cannot_have_is_withheld(self): + for value in (HOSTILE, "a b", "a\nb", "a/b", "a=b", "A", "-a", "a-", + "a" * 254, "", "eyJhbGciOiJIUzI1NiJ9.e30.x"): + with self.subTest(value=value[:20]): + self.assertEqual(gate.printable(value), gate.UNPRINTABLE) + + def test_a_non_string_is_withheld(self): + for value in (None, 3, ["aws-secrets-manager"], {"a": 1}): + with self.subTest(value=value): + self.assertEqual(gate.printable(value), gate.UNPRINTABLE) + + def test_the_stand_in_is_a_constant_rather_than_a_truncation(self): + """A prefix of a value that is not a name is still whatever that value + was, which is the whole objection.""" + self.assertNotIn(HOSTILE[:8], gate.printable(HOSTILE)) + + +class TheOutputCarriesNothingItDidNotVerify(TheVerdict): + """End to end, through both paths that echo a value read off disk.""" + + def test_a_hostile_value_in_the_contract_is_not_repeated(self): + contract = self.contract_for() + contract["clusterSecretStore"]["name"] = HOSTILE + rc, out, _ = self.verdict(self.healthy(), contract=contract) + self.assertEqual(rc, 1, out) + self.assertNotIn(HOSTILE, out) + self.assertNotIn("AKIA", out) + self.assertIn("clusterSecretStore.name", out) + self.assertIn("aws-secrets-manager", out, + "the declared side is the tree's own and is what the " + "reader has to act on") + + def test_a_hostile_value_in_a_consumer_is_not_repeated(self): + docs = self.healthy() + docs[1]["spec"]["secretStoreRef"]["name"] = HOSTILE + rc, out, _ = self.verdict(docs, contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertNotIn(HOSTILE, out) + self.assertIn(gate.UNPRINTABLE, out) + + def test_a_hostile_value_in_a_patch_target_is_not_repeated(self): + rc, out, _ = self.verdict( + self.healthy(), appsets=[appset("s", HOSTILE)], + contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertNotIn(HOSTILE, out) + + def test_a_hostile_apiversion_is_not_repeated(self): + docs = self.healthy() + docs[2]["apiVersion"] = HOSTILE + rc, out, _ = self.verdict(docs, contract=self.contract_for()) + self.assertEqual(rc, 1, out) + self.assertNotIn(HOSTILE, out) + + def test_the_published_side_is_named_rather_than_dumped(self): + """`json.dumps(have)` put arbitrary file content in the message. The + field name is what the reader needs; the content is what they already + have in front of them.""" + source = (ROOT / "scripts" / "check-secret-store-refs.py").read_text() + self.assertNotIn("json.dumps(have", source) + + class TheShippedCatalog(unittest.TestCase): """Over the tree, so a reference added later is checked here.""" From 5c306ea31ee31951df14c74c3bfe7f134887f623 Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Sat, 5 Sep 2026 00:59:15 -0700 Subject: [PATCH 3/6] Compare the published contract as bytes, and read nothing out of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract is generated. The question worth asking of it is "is this what the generator produces", and comparing four parsed fields answers a weaker one: it passes a file carrying an extra key, the same fields reordered, or a rewritten comment. A consumer in another repository reads the file, not the four fields this gate happens to look at. So `rendered()` is the one canonical serialisation, `--write` emits it, and the check compares the file against it byte for byte. Unparseable JSON stops being a separate case — it is simply not what the generator emits. The message reads nothing out of the file. What is in there is whatever somebody put there, so a message quoting it back repeats it into a log CI keeps; the reader has the file open and the remedy is `--write` and read the diff whatever the difference is. That also removes the last path from the contract to the output, which is two of the four CodeQL flows on this gate. `printable()` stays: the manifests are still read from disk and their names are still echoed, and a value is only echoed once it matches the API server's own grammar. A test asserts the generator and the comparison cannot come apart — `--write` followed by a check must pass, or every run is red with no way to fix it. --- scripts/check-secret-store-refs.py | 58 ++++++++++++------------- scripts/tests/test_secret_store_refs.py | 53 +++++++++++++++++----- 2 files changed, 71 insertions(+), 40 deletions(-) diff --git a/scripts/check-secret-store-refs.py b/scripts/check-secret-store-refs.py index a42c55f..23eb133 100755 --- a/scripts/check-secret-store-refs.py +++ b/scripts/check-secret-store-refs.py @@ -79,6 +79,7 @@ ROOT = pathlib.Path(__file__).resolve().parent.parent APPSET_DIR = ROOT / "applicationsets" CONTRACT = ROOT / "contracts" / "secret-store.json" +STORE_DIR = ROOT / "addons" / "bootstrap" / "secret-stores" CLUSTER_STORE = "ClusterSecretStore" NAMESPACED_STORE = "SecretStore" @@ -230,6 +231,19 @@ def survey(): return declared, consumers, versions +def rendered(contract: dict) -> str: + """The contract's one canonical serialisation. + + `--write` emits this and the check compares against it byte for byte, so + "the published contract" and "what the generator produces" are the same + question. Comparing parsed objects field by field answers a weaker one: it + passes a file carrying an extra key, a reordered object, or a rewritten + comment, and a consumer reads the file rather than the four fields this gate + happens to look at. + """ + return json.dumps(contract, indent=2) + "\n" + + def build_contract(declared, versions) -> dict: (kind, name), (_path, api) = next(iter(declared.items())) return { @@ -323,7 +337,7 @@ def main(argv: list[str] | None = None) -> int: want = build_contract(cluster_stores, versions) if args.write: CONTRACT.parent.mkdir(parents=True, exist_ok=True) - CONTRACT.write_text(json.dumps(want, indent=2) + "\n", encoding="utf-8") + CONTRACT.write_text(rendered(want), encoding="utf-8") print(f"✓ wrote {rel(CONTRACT)}") return 0 @@ -333,34 +347,20 @@ def main(argv: list[str] | None = None) -> int: f"have nothing to compare their chart defaults against. Run " f"`{rel(pathlib.Path(__file__))} --write`.") return 1 - try: - have = json.loads(CONTRACT.read_text(encoding="utf-8")) - except json.JSONDecodeError as err: - print(f"FAIL {rel(CONTRACT)} is not valid JSON: {err}. A contract a " - f"consumer cannot parse is one it will skip.") - return 1 - if have != want: - print(f"FAIL {rel(CONTRACT)} does not state what this tree states.") - # The published side is named, not echoed. It is parsed as arbitrary - # JSON out of a file, so what a message would be repeating is whatever - # somebody put there; the declared side is the tree's own and is what - # the reader has to act on anyway. - for section, fields in (("clusterSecretStore", ("apiVersion", "kind", "name")), - ("externalSecret", ("apiVersion",))): - published = have.get(section) - if not isinstance(published, dict): - print(f" {section}: absent from the published contract") - continue - for field in fields: - grammar = {"apiVersion": GROUP_VERSION, - "kind": KIND}.get(field, OBJECT_NAME) - if published.get(field) != want[section][field]: - print(f" {section}.{field}: the manifest declares " - f"{printable(want[section][field], grammar)}; the " - f"contract publishes something else") - print(" A contract that has drifted from the manifest is worse " - "than none: consumers assert against it and pass.") - print(f" Regenerate with `{rel(pathlib.Path(__file__))} --write`.") + + # Compared as bytes, and deliberately not parsed for the message. What is in + # that file is whatever somebody put there, so a message quoting it back + # repeats it into a log; the reader already has the file open, and the fix is + # the same whatever the difference is. + if CONTRACT.read_text(encoding="utf-8") != rendered(want): + print(f"FAIL {rel(CONTRACT)} is not what this tree declares.") + print(f" The manifest under {rel(STORE_DIR)} is the source of " + f"truth and the contract is generated from it, so a difference " + f"means the file was hand-edited or the manifest moved under it.") + print(" A contract that has drifted is worse than none: the " + "repositories asserting against it pass.") + print(f" Regenerate with `{rel(pathlib.Path(__file__))} --write` " + f"and read the diff.") return 1 store = want["clusterSecretStore"] diff --git a/scripts/tests/test_secret_store_refs.py b/scripts/tests/test_secret_store_refs.py index 36b35c7..0f5cdb7 100644 --- a/scripts/tests/test_secret_store_refs.py +++ b/scripts/tests/test_secret_store_refs.py @@ -205,7 +205,33 @@ def test_a_contract_that_drifted_from_the_manifest_is_reported(self): rc, out, _ = self.verdict( self.healthy(), contract=self.contract_for("aws-secretsmanager")) self.assertEqual(rc, 1, out) - self.assertIn("does not state what this tree states", out) + self.assertIn("is not what this tree declares", out) + + def test_a_contract_differing_only_in_shape_is_reported(self): + """Compared as bytes. A file carrying an extra key, or the same four + fields reordered, is not what the generator produces — and a consumer + reads the file, not the four fields this gate happens to look at.""" + contract = self.contract_for() + contract["clusterSecretStore"]["extra"] = "surplus" + rc, out, _ = self.verdict(self.healthy(), contract=contract) + self.assertEqual(rc, 1, out) + self.assertIn("is not what this tree declares", out) + + def test_the_written_contract_is_what_the_check_compares_against(self): + """`--write` then check must pass, or the generator and the comparison + have come apart and every run is red with no way to fix it.""" + rc, out, path = self.verdict(self.healthy(), write=True) + self.assertEqual(rc, 0, out) + root = path.parent.parent + saved = (gate.ROOT, gate.APPSET_DIR, gate.CONTRACT) + gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = ( + root, root / "applicationsets", path) + try: + with contextlib.redirect_stdout(io.StringIO()) as second: + rc = gate.main([]) + finally: + gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = saved + self.assertEqual(rc, 0, second.getvalue()) def test_a_missing_contract_is_reported(self): rc, out, _ = self.verdict(self.healthy()) @@ -214,7 +240,10 @@ def test_a_missing_contract_is_reported(self): def test_a_contract_that_does_not_parse_is_reported(self): """A contract a consumer cannot parse is one it skips, which lands it - back where it started: restating the name and hoping.""" + back where it started: restating the name and hoping. It is caught by + the same byte comparison as every other difference — unparseable is not + a separate case once the file has to be exactly what the generator + emits.""" _rc, _out, path = self.verdict(self.healthy(), contract=self.contract_for()) path.write_text("{not json") @@ -228,7 +257,7 @@ def test_a_contract_that_does_not_parse_is_reported(self): finally: gate.ROOT, gate.APPSET_DIR, gate.CONTRACT = saved self.assertEqual(rc, 1) - self.assertIn("not valid JSON", out.getvalue()) + self.assertIn("is not what this tree declares", out.getvalue()) def test_write_regenerates_the_contract_from_the_tree(self): rc, out, path = self.verdict(self.healthy(), write=True) @@ -305,10 +334,9 @@ def test_a_hostile_value_in_the_contract_is_not_repeated(self): self.assertEqual(rc, 1, out) self.assertNotIn(HOSTILE, out) self.assertNotIn("AKIA", out) - self.assertIn("clusterSecretStore.name", out) - self.assertIn("aws-secrets-manager", out, - "the declared side is the tree's own and is what the " - "reader has to act on") + self.assertIn("Regenerate", out, + "the remedy is the same whatever the difference is, and " + "it is what the reader needs") def test_a_hostile_value_in_a_consumer_is_not_repeated(self): docs = self.healthy() @@ -332,12 +360,15 @@ def test_a_hostile_apiversion_is_not_repeated(self): self.assertEqual(rc, 1, out) self.assertNotIn(HOSTILE, out) - def test_the_published_side_is_named_rather_than_dumped(self): - """`json.dumps(have)` put arbitrary file content in the message. The - field name is what the reader needs; the content is what they already - have in front of them.""" + def test_the_contract_is_never_parsed_for_a_message(self): + """It was `json.dumps(have)`, which put arbitrary file content into the + output. The file is compared as bytes now and no message reads from it: + the reader has it open, and the fix is the same whatever differs.""" source = (ROOT / "scripts" / "check-secret-store-refs.py").read_text() self.assertNotIn("json.dumps(have", source) + self.assertNotIn("json.loads(CONTRACT", source) + self.assertIn('CONTRACT.read_text(encoding="utf-8") != rendered(want)', + source) class TheShippedCatalog(unittest.TestCase): From 100f5a1a0dc4bd0bf32ce8bb6286a9a97dbca866 Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Sat, 5 Sep 2026 01:22:50 -0700 Subject: [PATCH 4/6] Suppress the two remaining scanner matches at the site, with the reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit py/clear-text-logging-sensitive-data matches on the NAMES of the files this gate reads — addons/bootstrap/secret-stores/ and contracts/secret-store.json — and treats a path containing "secret-store" as secret material. A secret store is the thing that holds secrets, not a secret: it is an address plus a set of credentials-free provider settings, and the values printed at both sites are a Kubernetes object name, a kind, a group version, a repository-relative path and a count. Every one read from a file has been through printable(), which returns it only on a fullmatch against the API server's own grammar and a fixed stand-in otherwise. This repository has now made the same distinction in two systems. The secrets block in .gitignore carries !*secret*-store*.yaml and !*secret*-store*.yml, and this branch added !*secret*-store*.json for the contract. Both are about a name rather than a content, and the pattern will recur in whatever tool comes third. That sentence is the durable part of this commit; the suppression is the consequence. ─── Why at the site ─── A CodeQL config excluding scripts/ from this query removes the query from a directory permanently. That is a list, and the worst kind: nothing inside scripts/ would say it is exempt, and a file added next month inherits an exemption nobody chose for it. A suppression at the site is visible to whoever reads the line, travels with the code if the code moves, and dies when the line does. Its scope is exactly what it covers. Leaving the alerts open would be right if the reasoning were uncertain. It is not — the taint source is filenames, which is a finding rather than a hunch, and an open alert whose answer is known is a queue nobody drains. ─── Why not worked around in the code ─── A verified string can be rebuilt character by character out of a literal alphabet, which defeats the dataflow and satisfies the analyser. That is worse than a suppression: a suppression is legible as a decision and can be disagreed with, and a defeated dataflow cannot even be seen. Two tests hold both halves — one fails if a marker is left without the reason or a pointer to it, the other if the code is contorted instead. Note the first two of the four alerts on this gate were NOT this. Those were a real flow — the contract parsed as arbitrary JSON and echoed back into a log — and are closed by printable() and the byte comparison, not by these markers. --- scripts/check-secret-store-refs.py | 29 ++++++++++++++++++-- scripts/tests/test_secret_store_refs.py | 36 +++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/scripts/check-secret-store-refs.py b/scripts/check-secret-store-refs.py index 23eb133..c0963dd 100755 --- a/scripts/check-secret-store-refs.py +++ b/scripts/check-secret-store-refs.py @@ -330,8 +330,32 @@ def main(argv: list[str] | None = None) -> int: if failures: print(f"{len(failures)} secret-store reference problem(s):\n") + # py/clear-text-logging-sensitive-data matches here on the NAMES of the + # files this gate reads — addons/bootstrap/secret-stores/ and + # contracts/secret-store.json — and treats a path containing "secret-store" + # as secret material. A secret store is the thing that holds secrets, not a + # secret: it is an address and a set of credentials-free provider settings, + # and the values printed below are a Kubernetes object name, a kind, a group + # version, a repository-relative path and a count. Every one read from a file + # has been through printable(), which returns it only on a fullmatch against + # the API server's own grammar and a fixed stand-in otherwise. + # + # This repository has now made the same distinction in two systems. The + # secrets block in .gitignore carries !*secret*-store*.yaml and + # !*secret*-store*.yml, and this file's contract added !*secret*-store*.json + # to it. Both are about a name rather than a content, and the pattern will + # recur in whatever tool comes third. + # + # Suppressed at the site rather than excluded in a CodeQL config: a config + # exclusion is a list that would silently cover every file added to scripts/ + # afterwards, none of which chose it. This dies with the line it sits on. + # + # Not worked around in the code either. A verified string can be rebuilt + # character by character from a literal alphabet, which defeats the dataflow + # and explains nothing — a reader can disagree with a suppression, and cannot + # even see a defeated dataflow. for f in failures: - print(f" {f}") + print(f" {f}") # codeql[py/clear-text-logging-sensitive-data] return 1 want = build_contract(cluster_stores, versions) @@ -364,7 +388,8 @@ def main(argv: list[str] | None = None) -> int: return 1 store = want["clusterSecretStore"] - print(f"✓ every secret-store reference resolves to the one store this " + # Same match, same reason as the failure path above. + print(f"✓ every secret-store reference resolves to the one store this " # codeql[py/clear-text-logging-sensitive-data] f"catalog declares: {printable(store['kind'], KIND)}/" f"{printable(store['name'])} " f"({printable(store['apiVersion'], GROUP_VERSION)}), named by " diff --git a/scripts/tests/test_secret_store_refs.py b/scripts/tests/test_secret_store_refs.py index 0f5cdb7..3b1c746 100644 --- a/scripts/tests/test_secret_store_refs.py +++ b/scripts/tests/test_secret_store_refs.py @@ -360,6 +360,42 @@ def test_a_hostile_apiversion_is_not_repeated(self): self.assertEqual(rc, 1, out) self.assertNotIn(HOSTILE, out) + def test_every_suppression_carries_its_reason(self): + """A bare `# codeql[...]` is the thing that rots. + + The suppression is legible as a decision only while the decision is next + to it. Stripped to the marker it becomes indistinguishable from somebody + silencing a finding they did not read, and there is no way to tell which + it was six months later. + """ + source = (ROOT / "scripts" / "check-secret-store-refs.py").read_text() + lines = source.splitlines() + marked = [i for i, line in enumerate(lines) if "codeql[" in line] + self.assertTrue(marked, "the suppressions are gone; if the query stopped " + "matching, delete this test with them") + REASON = "secret store is the thing that holds secrets" + self.assertIn(REASON, source, + "no suppression in this file says what the scanner matched " + "or why the match is wrong") + for i in marked: + with self.subTest(line=i + 1): + preceding = "\n".join(lines[max(0, i - 30):i]) + self.assertTrue( + REASON in preceding or "same reason as" in preceding, + "this suppression neither carries the reason nor points at " + "it — a marker on its own is indistinguishable from somebody " + "silencing a finding they did not read") + + def test_no_value_is_rebuilt_to_defeat_the_dataflow(self): + """The alternative to a suppression is contorting the code until the + analyser loses the trail — rebuilding a verified string character by + character out of a literal alphabet. A reader can disagree with a + suppression; they cannot see a defeated dataflow at all.""" + source = (ROOT / "scripts" / "check-secret-store-refs.py").read_text() + self.assertNotIn("ALPHABET", source) + self.assertIn("return text if grammar.fullmatch(text) else UNPRINTABLE", + source) + def test_the_contract_is_never_parsed_for_a_message(self): """It was `json.dumps(have)`, which put arbitrary file content into the output. The file is compared as bytes now and no message reads from it: From 71149948f80ead3669b5c67dec3b8b3d3dd7eade Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Sat, 5 Sep 2026 01:28:24 -0700 Subject: [PATCH 5/6] Put the suppression markers where CodeQL reads them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The markers were on the same line as the expression they cover. That is the older `lgtm[...]` placement, and it does not suppress: `codeql[...]` is read from the line BEFORE the alert and covers that one line only. Both markers moved, and the placement rule is recorded next to the reason because it is load-bearing and invisible when wrong. The same-line form has a second cost worth stating. Annotating a line edits it, which changes the alert's hash — the original closes as fixed and an identical one opens beside it, so the alert count stays the same and the history says something happened that did not. A marker that does not suppress is worse than no marker: it reads as handled, and the next person to look sees a decision that was never in force. --- scripts/check-secret-store-refs.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scripts/check-secret-store-refs.py b/scripts/check-secret-store-refs.py index c0963dd..995d6e1 100755 --- a/scripts/check-secret-store-refs.py +++ b/scripts/check-secret-store-refs.py @@ -354,8 +354,14 @@ def main(argv: list[str] | None = None) -> int: # character by character from a literal alphabet, which defeats the dataflow # and explains nothing — a reader can disagree with a suppression, and cannot # even see a defeated dataflow. + # + # The marker goes on the line BEFORE the expression it covers, and covers + # only that one line. On the same line it is the older `lgtm[...]` form, + # which edits the line it annotates and so changes the alert's hash: the + # original closes as fixed and an identical one opens beside it. for f in failures: - print(f" {f}") # codeql[py/clear-text-logging-sensitive-data] + # codeql[py/clear-text-logging-sensitive-data] + print(f" {f}") return 1 want = build_contract(cluster_stores, versions) @@ -389,7 +395,8 @@ def main(argv: list[str] | None = None) -> int: store = want["clusterSecretStore"] # Same match, same reason as the failure path above. - print(f"✓ every secret-store reference resolves to the one store this " # codeql[py/clear-text-logging-sensitive-data] + # codeql[py/clear-text-logging-sensitive-data] + print(f"✓ every secret-store reference resolves to the one store this " f"catalog declares: {printable(store['kind'], KIND)}/" f"{printable(store['name'])} " f"({printable(store['apiVersion'], GROUP_VERSION)}), named by " From 4eec8027916889d9e213679f8ca3e4aabca656d8 Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Sat, 5 Sep 2026 01:41:12 -0700 Subject: [PATCH 6/6] Say at the site that the marker suppresses nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It does not. Code scanning runs here as GitHub default setup, which reads no suppression comment for this query: the two alerts stayed open with the marker correctly placed on the line above each expression, and they were closed by dismissing them in the scanner's own database, which is not in this tree. The comment previously said "Suppressed at the site rather than excluded in a CodeQL config". That sentence was false, and prose asserting a mechanism the tool does not have is the same defect class this gate exists to catch — a claim nothing holds to the thing it describes. It now says what happens: the marker records the decision, the decision takes effect somewhere a reader of this file cannot see, and it is addressed to a person rather than to the tool. It stays because removing it leaves the site with no explanation at all, which is worse than an explanation the scanner ignores. The placement note stays too, as the placement the marker WOULD need — on the line before the expression, covering that line only — since the same-line `lgtm[...]` form also edits the line it annotates and churns the alert's hash. ─── scripts/tests/test_secret_store_refs.py ─── `test_every_marker_has_its_reason_above_it` replaces a thirty-line window with an ordering: the reason must appear BEFORE the marker, because a reader meets them in that order. How many lines separate them is not a property worth pinning, and pinning it made the test fail when the reason grew. `test_the_marker_does_not_claim_to_suppress` fails if the prose goes back to claiming the finding is suppressed here. --- scripts/check-secret-store-refs.py | 23 +++++++++----- scripts/tests/test_secret_store_refs.py | 41 +++++++++++++++++-------- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/scripts/check-secret-store-refs.py b/scripts/check-secret-store-refs.py index 995d6e1..2eb2ce1 100755 --- a/scripts/check-secret-store-refs.py +++ b/scripts/check-secret-store-refs.py @@ -346,19 +346,26 @@ def main(argv: list[str] | None = None) -> int: # to it. Both are about a name rather than a content, and the pattern will # recur in whatever tool comes third. # - # Suppressed at the site rather than excluded in a CodeQL config: a config - # exclusion is a list that would silently cover every file added to scripts/ - # afterwards, none of which chose it. This dies with the line it sits on. + # The marker below does NOT suppress anything. Code scanning runs here as + # default setup, which reads no suppression comment for this query: the + # alerts stayed open with it in place and were closed by dismissing them in + # the scanner's own database, which is not in this tree. So the marker + # records the decision at the site and the decision takes effect somewhere a + # reader of this file cannot see. + # + # It stays because the alternative leaves the site with no explanation at + # all. Read it as a comment addressed to a person, not to the tool. # # Not worked around in the code either. A verified string can be rebuilt # character by character from a literal alphabet, which defeats the dataflow - # and explains nothing — a reader can disagree with a suppression, and cannot + # and explains nothing — a reader can disagree with a comment, and cannot # even see a defeated dataflow. # - # The marker goes on the line BEFORE the expression it covers, and covers - # only that one line. On the same line it is the older `lgtm[...]` form, - # which edits the line it annotates and so changes the alert's hash: the - # original closes as fixed and an identical one opens beside it. + # The placement is the one the marker would need if it were read: on the + # line BEFORE the expression, covering that line only. On the same line it + # is the older `lgtm[...]` form, which edits the line it annotates and so + # changes the alert's hash — the original closes as fixed and an identical + # one opens beside it. for f in failures: # codeql[py/clear-text-logging-sensitive-data] print(f" {f}") diff --git a/scripts/tests/test_secret_store_refs.py b/scripts/tests/test_secret_store_refs.py index 3b1c746..d8f8a8b 100644 --- a/scripts/tests/test_secret_store_refs.py +++ b/scripts/tests/test_secret_store_refs.py @@ -360,31 +360,46 @@ def test_a_hostile_apiversion_is_not_repeated(self): self.assertEqual(rc, 1, out) self.assertNotIn(HOSTILE, out) - def test_every_suppression_carries_its_reason(self): + def test_every_marker_has_its_reason_above_it(self): """A bare `# codeql[...]` is the thing that rots. - The suppression is legible as a decision only while the decision is next - to it. Stripped to the marker it becomes indistinguishable from somebody - silencing a finding they did not read, and there is no way to tell which - it was six months later. + The marker does not suppress — code scanning runs here as default setup + and reads no suppression comment for this query — so it is a comment + addressed to a person, and a comment addressed to a person that says + only a rule id says nothing. Stripped to the marker it is + indistinguishable from somebody silencing a finding they did not read, + and there is no way to tell which it was six months later. + + Ordering rather than a window: the reason has to come BEFORE the marker, + because a reader meets them in that order. How many lines separate them + is not a property worth pinning. """ source = (ROOT / "scripts" / "check-secret-store-refs.py").read_text() lines = source.splitlines() marked = [i for i, line in enumerate(lines) if "codeql[" in line] - self.assertTrue(marked, "the suppressions are gone; if the query stopped " + self.assertTrue(marked, "the markers are gone; if the query stopped " "matching, delete this test with them") REASON = "secret store is the thing that holds secrets" self.assertIn(REASON, source, - "no suppression in this file says what the scanner matched " - "or why the match is wrong") + "no marker in this file says what the scanner matched or " + "why the match is wrong") for i in marked: with self.subTest(line=i + 1): - preceding = "\n".join(lines[max(0, i - 30):i]) + above = "\n".join(lines[:i]) self.assertTrue( - REASON in preceding or "same reason as" in preceding, - "this suppression neither carries the reason nor points at " - "it — a marker on its own is indistinguishable from somebody " - "silencing a finding they did not read") + REASON in above or "same reason as" in above, + "this marker neither carries the reason nor points at it — " + "on its own it is indistinguishable from somebody silencing " + "a finding they did not read") + + def test_the_marker_does_not_claim_to_suppress(self): + """It did not. The alerts stayed open with it in place and were closed + in the scanner's own database, which is not in this tree — so prose + here saying the finding is suppressed at the site would be the same + class of defect this gate exists to catch, one level up.""" + source = (ROOT / "scripts" / "check-secret-store-refs.py").read_text() + self.assertIn("does NOT suppress anything", source) + self.assertNotIn("Suppressed at the site", source) def test_no_value_is_rebuilt_to_defeat_the_dataflow(self): """The alternative to a suppression is contorting the code until the