diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c23fe8..1555099 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 3ffd0ca..c5ac656 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..2eb2ce1 --- /dev/null +++ b/scripts/check-secret-store-refs.py @@ -0,0 +1,419 @@ +#!/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" +STORE_DIR = ROOT / "addons" / "bootstrap" / "secret-stores" + +CLUSTER_STORE = "ClusterSecretStore" +NAMESPACED_STORE = "SecretStore" +STORE_KINDS = (CLUSTER_STORE, NAMESPACED_STORE) +EXTERNAL_SECRET = "ExternalSecret" + +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. +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 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)) + 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 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 { + "_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"{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 " + 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 {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 " + 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 {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 " + f"base said.") + + if len(versions) > 1: + listed = "; ".join( + 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)} " + 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") + # 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. + # + # 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 comment, and cannot + # even see a defeated dataflow. + # + # 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}") + return 1 + + want = build_contract(cluster_stores, versions) + if args.write: + CONTRACT.parent.mkdir(parents=True, exist_ok=True) + CONTRACT.write_text(rendered(want), 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 + + # 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"] + # Same match, same reason as the failure path above. + # 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 " + 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 " + "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 198396c..ff275da 100755 --- a/scripts/tests/controls.py +++ b/scripts/tests/controls.py @@ -519,6 +519,21 @@ def m_burn_rate_budgets(root): marker="100% over 3d") +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 @@ -596,6 +611,8 @@ def m_env_coverage(root): m_alert_severity_routes), "check-burn-rate-budgets.py": ("a summary claiming a budget its expression " "does not spend", m_burn_rate_budgets), + "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 42b708c..ea77d51 100755 --- a/scripts/tests/reverify-gates.sh +++ b/scripts/tests/reverify-gates.sh @@ -159,6 +159,7 @@ 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-burn-rate-budgets.py" ./scripts/check-burn-rate-budgets.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 @@ -437,6 +438,54 @@ run nonzero "burn-rate-budgets: the panel measuring the objective stops shipping ./scripts/check-burn-rate-budgets.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 @@ -549,7 +598,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=44 +MIN_CHECKS=48 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 959549a..a65162e 100755 --- a/scripts/tests/run.py +++ b/scripts/tests/run.py @@ -65,6 +65,9 @@ # The arithmetic behind a figure an on-call reads at the worst moment, and # the anchoring that keeps it from being a constant compared to nothing. "test_burn_rate_budgets", + # 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", diff --git a/scripts/tests/test_secret_store_refs.py b/scripts/tests/test_secret_store_refs.py new file mode 100644 index 0000000..d8f8a8b --- /dev/null +++ b/scripts/tests/test_secret_store_refs.py @@ -0,0 +1,487 @@ +"""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("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()) + 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. 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") + 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("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) + 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) + + +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("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() + 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_every_marker_has_its_reason_above_it(self): + """A bare `# codeql[...]` is the thing that rots. + + 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 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 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): + above = "\n".join(lines[:i]) + self.assertTrue( + 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 + 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: + 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): + """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()