feat(cli): predefined policy packs — run many policies in one invocation - #364
Open
refeed wants to merge 2 commits into
Open
feat(cli): predefined policy packs — run many policies in one invocation#364refeed wants to merge 2 commits into
refeed wants to merge 2 commits into
Conversation
`generate_evaluator_result` walks the list a provider returns and folds each
result into one verdict. A provider error within `error_tolerance` marked the
check skipped by assigning `has_evaluation_passed = None` unconditionally — so a
tolerated miss arriving *after* a resource that genuinely violated the policy
overwrote the failure with the skip marker.
The verdict therefore depended on the order the provider happened to emit
resources, which is not part of the policy. Both orderings are reachable on an
ordinary plan: one resource of the target type violating the rule, another the
provider cannot read — a destroy, whose `after` is null.
Under `--fail-on-error` this is the difference between exit 3 and exit 1: a real
violation was reported as "nothing was checked". Failing open is the wrong
direction for a gate.
The fix is to not overwrite a verdict that already exists. A `False` stands; a
`True` still yields to the skip, because nothing was actually verified about the
tolerated resource:
[FAIL, skip] -> False (was None)
[skip, FAIL] -> False
[PASS, skip] -> None
[skip, PASS] -> None
That answers the open `[PASS, skip, PASS]` question by keeping current behaviour
rather than by making a second change in the same commit. Regression test covers
all four orderings and fails without this fix.
Verdict-changing, so it gets its own CHANGELOG section rather than being filed
under "backward compatible".
Closes #293
…policy pack `tirith` evaluated exactly one policy file per run: `start_policy_evaluation` did an `open()` and a `json.load()`, and a directory raised `IsADirectoryError` into the broad handler and exited 1. Running a set of rules meant one process per rule and no aggregate verdict, which is why there was nowhere for a predefined pack to live. Three parts. **The engine.** `start_policy_set_evaluation` reads the input document and the variables once and shares them across every policy, so a pack parses the plan once rather than N times. Each policy keeps its own result document unchanged — a set run is the single-policy result repeated, plus a summary — so anything that already reads a Tirith result can read one element of `policies` without knowing where it came from. A policy that fails to load is recorded as `errored` and does not take the run down with it: a pack is shipped content, and the useful answer is "these 103 ran, this one is broken". **The CLI.** `-policy-path` now accepts a directory, walked recursively for `*.json`. `--pack NAME` runs a bundled pack, is repeatable, and combines with `-policy-path` so local and bundled rules produce one verdict. `--list-packs` lists what is installed. A single policy *file* is untouched — same result document, same exit codes, same printer. The rule is that the shape follows how the run was asked for, not how many policies matched, so a directory holding one policy still reports as a set. That keeps it stateable in one sentence and keeps the golden-file contract in `tests/core/test_output_compatibility.py` intact. **`skipped` is not a failure**, and this is the part that decides whether a pack is usable at all. A check applies only to plans that touch the resource it names, so on any real plan most of a large pack has nothing to look at: 97 of 104 on the `aws_instance` fixture. Those are counted separately and do not affect the exit code. Counting them as errors would make every pack run red regardless of the infrastructure, and the exit code would carry no information. With `--fail-on-error`: 3 if any policy failed, 0 if none failed and at least one reached a verdict, 1 if nothing did. **The pack.** `terraform-baseline`, 104 baseline security and configuration checks for Terraform plans across AWS, Azure, GCP, Kubernetes and seven smaller providers. Generated by `tools/sync_pack.py` from tirith-policy-corpus, whose `confirmed` tier is its strongest evidence class: the verdict flips against a synthesized compliant and violating document, *and* the tool the rule was translated from independently agreed on both. Checks carry StackGuardian identifiers and nothing else. `meta.id` is `SG_TF_<NNNN>`, allocated once and frozen in the corpus so a report can name a check and mean the same check next release; filenames are `SG_TF_0042_<resource>_<attribute>.json`; tags are lowercased with a `cloud:<name>` added. The generator refuses to write a file in which an upstream name, check id or API vocabulary survived, and `tests/packs` refuses to ship one — two independent gates, so a single edit cannot disable both. Attribution for the derived rules is in NOTICE, as Apache-2.0 requires. Every packed policy is tested in CI: structurally valid via Tirith's own `check_policy`, unique id, no leaked name, and *still flipping* against its two fixtures. That last one is what catches an engine change silently turning a check into a no-op — a pack is generated once and then sits still while the engine moves underneath it. Wheel verified: built, installed into a clean venv, and run from an unrelated directory. `package_data` is declared as well as `MANIFEST.in` for the reason already recorded there — a wheel built from the tree ignores MANIFEST, and the TUI shipped empty that way once. Widening the pack is `sync_pack.py --tier exact` (962 policies) or `--tier verified` (2,696): content, not code. Closes #319
|
❌ The last analysis has failed. |
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
tirithevaluated exactly one policy file per run —start_policy_evaluationdid anopen()and a
json.load(), and a directory raisedIsADirectoryErrorinto the broad handler andexited 1. Running a set of rules meant one process per rule and no aggregate verdict, which is
why there was nowhere for a predefined pack to live.
This adds the runner, the pack format, and the first pack.
Two commits, on purpose
4ee02cc—fix(core): a tolerated skip no longer erases an earlier real failure.Verdict-changing, and reviewable on its own. A provider error within
error_tolerancemarkedthe check skipped unconditionally, so a tolerated miss arriving after a resource that
genuinely violated the policy overwrote the failure. The verdict depended on the order the
provider emitted resources, which is not part of the policy; under
--fail-on-errorthat isexit 1 where exit 3 was correct — a real violation reported as "nothing was checked".
[FAIL, skip]NoneFalse[skip, FAIL]FalseFalse[PASS, skip]NoneNone[skip, PASS]NoneNone[PASS, skip, PASS]is answered by keeping current behaviour rather than by a second change inthe same commit. Regression test covers all four orderings and fails without the fix. It has
its own CHANGELOG section rather than being filed under "backward compatible".
Closes #293
660d25d—feat(cli): the runner and the pack. Closes #319The runner
start_policy_set_evaluationreads the input document and the variables once and shares them,so a pack parses the plan once rather than N times. Each policy keeps its own result document
unchanged — a set run is the single-policy result repeated, plus a summary — so anything that
already reads a Tirith result can read one element of
policies. A policy that fails to load isrecorded as
erroredand does not take the run down with it.-policy-pathnow accepts a directory (recursive*.json);--pack NAMEis repeatable andcombines with it, so local and bundled rules produce one verdict;
--list-packslists what isinstalled.
A single policy file is untouched — same result document, same exit codes, same printer. The
rule is that the shape follows how the run was asked for, not how many policies matched, so a
directory holding one policy still reports as a set. That keeps it stateable in one sentence and
keeps
tests/core/test_output_compatibility.py's golden bytes intact.skippedis not a failureThis is the part that decides whether a pack is usable at all. A check applies only to plans
that touch the resource it names, so on any real plan most of a large pack has nothing to look
at — 97 of 104 above. Those are counted separately and do not affect the exit code. Counting
them as errors would make every pack run red regardless of the infrastructure, and the exit code
would carry no information. With
--fail-on-error:3if any policy failed,0if none failedand at least one reached a verdict,
1if nothing did.The pack
terraform-baseline— 104 baseline security and configuration checks for Terraform plans acrossAWS, Azure, GCP, Kubernetes and seven smaller providers. Generated by
tools/sync_pack.pyfromtirith-policy-corpus, whoseconfirmedtier is its strongest evidence class: the verdict flipsagainst a synthesized compliant and violating document, and the tool the rule was translated
from independently agreed on both.
Checks carry StackGuardian identifiers and nothing else.
meta.idisSG_TF_<NNNN>, allocatedonce and frozen in the corpus so a report can name a check and mean the same check next release;
filenames are
SG_TF_0042_<resource>_<attribute>.json; tags are lowercased with acloud:<name>added and the redundant
terraformdropped.Two independent gates on the renaming, so one edit cannot disable both: the generator refuses to
write a file in which an upstream name, check id or API vocabulary survived, and
tests/packsrefuses to ship one. The sweep found branded text in two obvious places (104
meta.id, 13descriptions) and then 53 more descriptions quoting upstream's API —
missing_block_result,ANY_VALUE,BaseResourceValueCheck— which is not a brand name but points at one particularscanner as plainly as one. All 51 affected policies were rewritten; see
StackGuardian/tirith-policy-corpus#2, which holds the id registry and the rewrites and is the
only place the upstream identifier survives.
Attribution for the derived rules is in the new
NOTICE, as Apache-2.0 requires.Verification
pytest tests— 902 pass. The 11 failures are pre-existing and environmental(
FileNotFoundError: .test_tmp/plan.json; they needterraformlocally), identical onunmodified
main.check_policy, uniqueSG_TF_ids, theno-leak sweep, and all 104 still flipping against their fixtures. That last one is what
catches an engine change silently turning a check into a no-op — a pack is generated once and
then sits still while the engine moves underneath it. Fixtures live in
tests/packs/fixturesand are deliberately not in the wheel.
package_datais declared as well asMANIFEST.infor the reason already recorded there — awheel built from the tree ignores MANIFEST, and the TUI shipped empty that way once.
black --checkandpydocstyleclean.sync_pack.pyre-run is idempotent: ids byte-identical.Two things worth knowing before this becomes a CIS story
confirmedis 104 policies because the differential was sampled at 120, not because the restare worse — absence is unproven, not disproven. Widening is
sync_pack.py --tier exact(962) or--tier verified(2,696): content, not code.And CIS coverage is thinner than the roadmap's R2-16 assumes. Joining every translated corpus
record to its inventory row: 795 / 2,697 carry any compliance-framework tag, 108 carry an
unversioned
cisboolean, and zero carry a versioned CIS control id — all 2,130cis_v###_#_#ids live on the out-of-scope cloud-API mods. Thousands of CIS checks runs through #349, not
through regrouping the IaC translations. Relevant to #331 and #327.
Not in scope
resource_filter(#316),no_match_resultat the provider level (#297),tirith test(#318/#301), policies-from-git (#325),
--fail-on-severity(#298). On that last one:meta.severityis present on 1 of the 1,224 policies from this source, so severity gating is real work but it is
not gating for this pack.