Hold every secret-store reference to the one manifest that declares it - #245
Conversation
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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Brings in the burn-rate budget gate. Three collisions. ─── scripts/tests/controls.py, scripts/tests/run.py EXPECTED ─── Additive on both sides — each added a mutator, a registry entry and a test module at the same insertion point, and neither edited the other's. All kept. ─── scripts/tests/run.py MAX_UNCOVERED_GATES ─── Both sides moved it 11 -> 10 off the same base, each because a gate arrived with its own tests. The value agrees; only the prose differed. Main's wording is kept: it says the same thing and carries the extra clause about an import removed later giving that coverage back. The merged tree holds 27 gate scripts and the count stays at 10, so the ratchet is right rather than coincidentally satisfied. ─── scripts/tests/reverify-gates.sh ─── The planted defects are additive and all kept. The floor is not: both sides raised MIN_CHECKS off the same base of 41, the burn-rate work to 44 for its three `run` lines and this branch to 48 for its seven. The floor tracks the harness's own size at three under the `run` count, which is the margin that lets a check be retired without turning the harness red while still catching one that stopped executing most of itself. The merged file carries 51 `run` lines, so the floor is 48. Taking either side verbatim resolves it downward: at 44, seven of the checks this branch adds could stop running and the floor would still pass.
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.
CI Results
All checks passed. |
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.
CI Results
All checks passed. |
|
Merged The mergeThree collisions.
Two of the four CodeQL alerts were realThe reflex reading is "false positive, that file only holds a store name". Two were not.
Closed in two moves, neither a tuning of the flagged lines:
The contract is compared as bytes. Proven with an AWS-key-shaped string planted in the contract, in a consumer's The other two are this repository's own namingThey sit on the failure loop and the success line. Everything either prints is a repo-relative path, a count, or a value The taint source is the file names: I have not suppressed them. Anything further would be writing for the scanner rather than the reader — reconstructing verified strings character by character from a literal alphabet breaks the dataflow and explains nothing to anyone reading the code. Three options, and the call is yours: an in-source Verification at
|
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.
CI Results
One or more checks failed — see the job logs. |
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.
CI Results
All checks passed. |
|
In-source suppression taken, with the reason. 30 of 31 checks green; CodeQL still red, and the reason is not the suppression's content — it is the mechanism. What is at the siteBoth markers carry the reasoning: the query matches on the names of the files this gate reads, and a secret store is the thing that holds secrets rather than a secret. The comment records that this repository has now made that same distinction in two systems — Two tests hold the shape rather than the text: I had the placement wrong first
And it still does not turn the check greenCorrected placement, re-run, both alerts still CodeQL here is default setup — So in-source suppression is available here as a record and not as a mechanism. The marker is correct, legible, travels with the code, dies with the line, and will suppress for anyone running the CLI or a pipeline that consumes the SARIF. The GitHub check stays red regardless. What is left is a decision, and it is not mine
Say which and I'll do it. Not mine, and already clearedOne run also failed the image-vulnerability gate: Verification at
|
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.
CI Results
All checks passed. |
2 participants
Closes #203.
Gated, not derived — and which one matters
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 the answer is: one manifest owns the name, 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 repos. This repository restates the name ten more times:
addons/bootstrap/secret-stores/cluster-secret-store.yaml— the declarationsecretStoreRefin plain manifestssecretStoreRefin Go-template chart source (catalog/druid)applicationsets/secret-stores.yamlkustomize patch target"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 missing store at least records
SecretSyncedErroron its own status. The ApplicationSet patch target records nothing:That patch rewrites the store's AWS region per cluster from the cluster Secret's
regionlabel. Unmatched, every cluster silently keepsus-west-2and looks its secrets up in the wrong region — green render, green kubeconform, nothing anywhere reporting a failure.scripts/check-secret-store-refs.pyNothing in it is a list. It reads the declaration and requires exactly one cluster-wide store, that every
secretStoreRefnames 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 locates thesecretStoreRefblock by its key and reads fields from inside it, so aname:belonging to the target or the metadata cannot be mistaken for the store.contracts/secret-store.jsonThe published half — option 1 in the issue — for the four repos this seat cannot edit. Name, kind and apiVersion, generated by
--writeand 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 the issue's own comments established they go stale together. Five charts declared
external-secrets.io/v1beta1against a CRD that lists but does not serve it — that passes helm, kubeconform and chart lint, because the version really is on the CRD..gitignorecaught the contract, and the rule was right*secret*.jsonunder "# Secrets and credentials" swallowed it. Not force-added: the rule already carries!*secret*-store*.yamland!*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.gitleaks dir .finds nothing in it.Proof
Eight defects planted, each caught and each naming the file:
grafana-token.yamlexternalsecret.yamlv1beta1Then the tests by mutation,
__pycache__cleared between each:test_every_reference_the_tree_holds_is_in_the_corpus,test_the_corpus_spans_both_readerstest_a_patch_target_that_stops_matching_is_reportedtest_a_contract_that_drifted_from_the_manifest_is_reportedtest_two_cluster_stores_leave_the_contract_undecidedtest_two_external_secret_versions_leave_nothing_to_publishname:in the fileThe first is the load-bearing one, and
test_the_corpus_spans_both_readersasserts the tree still exercises both readers so the text one cannot become dead code unnoticed.Not closed, and adjacent
The patch-target no-op is general. Eleven named kustomize patch targets exist across the ApplicationSets; only the one naming a store is checked here. The other ten —
Grafana/external, fourExternalSecrets, threeGrafanaDatasources,EC2NodeClass/default,CronJob/grafana-token-rotator— each silently do nothing if the object they name is renamed. Same failure, same silence, wider than this issue.The four repositories still hardcoding the name are not this seat's; the contract is what their CI can assert against, and the gate says so in its own scope statement.
Verification
ruff clean, mypy clean over 50 files, yamllint clean, gitleaks clean, 522 tests across 20 modules, coverage 45.4% against the 42% floor with 14 per-gate floors held,
controls.py19 controls,empty-corpus.py32 probed gates,reverify-gates.sh48/48 against the floor of 45,check-named-things.py197 references resolve,task validateclean.