diff --git a/CHANGELOG.md b/CHANGELOG.md index 19330f0a..936db2b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ## [Unreleased] +## [1.3.0] - 2026-08-18 + ### Added +- `tirith -policy-path …` can be used as a CI gate, not only at a terminal. Three additions, all + optional and all off unless asked for, so an existing invocation behaves exactly as before -- which + matters because this command's `--json` output is a frozen contract: + - **`-policy-path` accepts a directory or a glob**, evaluating every policy it finds. A directory is + searched recursively for `*.tirith.json`, or failing that for any `.json` file shaped like a policy. + Pointing it at a directory previously failed with a bare `ERROR`. `-var-path` and `-var` apply to + every policy, so a parameterised policy behaves the same whether you name the file or its directory. + - **`--input-kind`** masks the document before evaluation. It matters even though nothing is uploaded: + evaluator messages embed the values they compared, and those messages are copied into whatever + comment a CI job posts. Opt-in because masking changes those messages, and they are the frozen + `--json` output. + - **`--output-json`, `--output-markdown`, `--comment-marker`, `--markdown-limit`, `--sha`** write the + verdict out for a job to publish, in the same shape `tirith platform check` writes. So one CI + integration drives either, and adding a StackGuardian organization later changes which policies + apply rather than how anything is wired. + + Two paths fail closed regardless of `--fail-on-error`, because both are tool health rather than policy + decisions: no policy files found at `-policy-path`, and a policy that could not be evaluated. A run + where every check was skipped also exits 1 -- nothing was examined, and this command has always + treated that as a failure rather than a pass. That last one is a deliberate difference from + `platform check`, which counts skips separately and reports them as a pass. + + This capability came from the GitHub Action, which owned the only implementation of it. It moved here + so a second front end -- the GitLab CI component -- drives one implementation rather than forking it. - `tirith ui`: an interactive interface with three tabs. - **Explorer** — read an evaluation's results down to the resource behind each one. The result document has always carried the resource address, the planned action and the before/after @@ -31,9 +56,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 a provider argument the operation does not read, an id referenced in `eval_expression` but never defined, a single `&` where `&&` was meant, an evaluator that does not exist. +### Changed +- The `--output-json` document now carries a `mode` key (`platform` or `local`), and every key exists + in both modes -- one with no meaning on a path is `null` rather than omitted, so a consumer needs + one parser rather than two. Additive: no key was removed or retyped. Specified in + [docs/output-contract.md](docs/output-contract.md). +- `tirith platform check` writes `--output-json` and `--output-markdown` on its failure paths too, + rather than returning having written nothing. A caller editing a sticky comment in place needs a + marker-first body on that path, or it orphans the comment it was updating. + ### Notes -- The local evaluation surface is untouched. `ui` is dispatched before the flat parser, like - `platform`, so `--json` output remains byte-identical to the golden file. +- The local evaluation surface is untouched. `ui` and `local` are dispatched before the flat parser, + like `platform`, so `--json` output remains byte-identical to the golden file. - No new runtime dependencies for anyone who does not install the extra. ## [1.2.0] - 2026-08-03 diff --git a/README.md b/README.md index e9dceeee..51f48fe4 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,7 @@ verdict, same exit codes. That mode is optional and is the only part that talks - [Run it in CI](#run-it-in-ci) - [Exit codes](#exit-codes) - [Evaluating against your StackGuardian organization](#evaluating-against-your-stackguardian-organization) +- [Evaluating committed policy files, in CI](#evaluating-committed-policy-files-in-ci) - [Example Tirith policies](#example-tirith-policies) - [error_tolerance](#error_tolerance-and-the-third-outcome) - [Terraform Plan](#terraform-plan-provider) @@ -177,7 +178,7 @@ pip install -e . ``` tirith --version -tirith 1.2.0 +tirith 1.3.0 ``` Congratulations! Tirith has been setup in your system @@ -185,21 +186,34 @@ Congratulations! Tirith has been setup in your system ## Usage ``` -usage: tirith [-h] [-policy-path PATH] [-input-path PATH] [-var-path PATH] - [-var PATH] [--json] [--verbose] [--fail-on-error] [--version] +usage: tirith [-h] [-policy-path PATH] [-input-path PATH] [-var-path PATH] [-var PATH] [--json] + [--verbose] [--fail-on-error] [--version] [--input-kind KIND] [--state-path PATH] + [--sha SHA] [--output-json PATH] [--output-markdown PATH] [--comment-marker TEXT] + [--markdown-limit N] Tirith (StackGuardian Policy Framework) options: - -h, --help show this help message and exit - -policy-path PATH Path containing Tirith policy as code - -input-path PATH Input file path - -var-path PATH Variable file path(s) - -var PATH Inline variable(s) - --json Only print the result in JSON form (useful for passing output to other programs) - --verbose Show detailed logs of from the run - --fail-on-error Exit 3 when a policy fails, instead of 0. Off by default for compatibility. - --version show program's version number and exit + -h, --help show this help message and exit + -policy-path PATH Path containing Tirith policy as code + -input-path PATH Input file path + -var-path PATH Variable file path(s) + -var PATH Inline variable(s) + --json Only print the result in JSON form (useful for passing output to other programs) + --verbose Show detailed logs of from the run + --fail-on-error Exit 3 when a policy fails, instead of 0. Off by default for compatibility. + --version show program's version number and exit + +reporting: + Write the verdict out for a CI job to publish. All optional. + + --input-kind KIND terraform_plan, terraform_state, kubernetes or json. Masks the input before evaluating + --state-path PATH Terraform state, when --input-kind is terraform_state + --sha SHA Revision these findings describe, recorded in the report + --output-json PATH Write the result document here (same shape as `tirith platform check`) + --output-markdown PATH Write a markdown report here, for a comment or a note + --comment-marker TEXT Opaque first line of the markdown, for comment stickiness + --markdown-limit N Truncate the markdown to this length. Default: 60000 Subcommands: @@ -210,6 +224,7 @@ Subcommands: About Tirith: + * Abstract away the implementation complexity of policy engine underneath. * Simplify creation of declarative policies that are easy to read and interpret. * Provide a standard framework for scanning various configurations with granularity. @@ -385,7 +400,7 @@ GitLab-specific: any runner that can execute a container and produce a plan work | 0 | Policies passed, or nothing was in scope to gate on | | 1 | Tirith could not complete the evaluation — bad input, a policy it could not evaluate, unreachable API | | 2 | Timed out waiting for a StackGuardian run | -| 3 | A policy failed. Only with `--fail-on-error`, on either surface | +| 3 | A policy failed. Only with `--fail-on-error`, on any of the three surfaces | | 130 | Interrupted | **Gate a CI job with `--fail-on-error`:** @@ -443,6 +458,42 @@ Common flags: Running this from GitHub Actions? Use [the action](#github-actions) instead — it wires up the plan discovery, the sticky pull-request comment, the check run and the exit codes for you. +## Evaluating committed policy files, in CI + +`tirith -policy-path … -input-path …` has always evaluated the policies in your repository. Three +additions make it usable as a CI gate rather than only at a terminal: + +``` +tirith -policy-path .tirith/policies -input-path plan.json \ + --input-kind terraform_plan --output-json result.json --output-markdown report.md \ + --fail-on-error +``` + +**`-policy-path` accepts a directory or a glob**, not just one file, and evaluates every policy it +finds — a directory is searched for `*.tirith.json`, or failing that for any `.json` file shaped like a +policy. Pointing it at a directory previously failed with a bare `ERROR`. + +**`--input-kind` masks the document** before evaluation. That matters even though nothing is uploaded: +evaluator messages embed the values they compared, and those messages end up in whatever comment your +CI job posts. It is opt-in because masking changes those messages, and they are this command's `--json` +output, which is a frozen contract — with no `--input-kind` the file is read exactly as before. + +**`--output-json` / `--output-markdown` / `--comment-marker`** write the verdict out for a job to +publish, in the [same shape](docs/output-contract.md) `tirith platform check` writes. So a CI +integration works against either, and adding a StackGuardian organization later changes which policies +apply rather than how anything is wired. + +Two behaviours worth knowing, both of which fail closed: + +- **No policy files found is an error, not a skip** — exit `1`, whether or not `--fail-on-error` was + passed. A green result for a change nothing was evaluated against is the one outcome this must never + produce. +- **A policy that could not be evaluated exits `1` regardless of `--fail-on-error`**, as does a run + where every check was skipped. "Could not evaluate" and "nothing was examined" are tool health, not + policy decisions. + +Full reference: [docs/evaluating-policy-files.md](docs/evaluating-policy-files.md). + ## Example Tirith policies [Examples using various providers](tests/providers) diff --git a/docs/evaluating-policy-files.md b/docs/evaluating-policy-files.md new file mode 100644 index 00000000..bcdd90df --- /dev/null +++ b/docs/evaluating-policy-files.md @@ -0,0 +1,122 @@ +# Evaluating policy files in your repository + +`tirith -policy-path … -input-path …` evaluates the policies committed in your repository. It needs no +account and makes no network calls, and it is the surface most open-source users are on. + +``` +tirith -policy-path .tirith/policies -input-path plan.json --fail-on-error +``` + +This page is about the flags that make it usable as a **CI gate**: several policies at once, the input +masked before its values reach a pull-request comment, and the verdict written out for a job to +publish. The alternative is +[`tirith platform check`](platform-check.md), which evaluates the policies a StackGuardian organization +enforces instead — and writes the [same result document](output-contract.md), so a CI integration built +against one works unchanged against the other. + +## Several policies at once + +`-policy-path` accepts a file, a **directory** or a **glob**. + +- **A file** is evaluated as given. Naming one explicitly is an instruction, so "that is not a policy" + is reported rather than the file being skipped. +- **A directory** is searched recursively for `*.tirith.json`. If it holds none, any `.json` file + *shaped* like a policy is used — an object with both `meta` and `evaluators`. +- **A glob** is expanded and filtered by that same shape test. + +The shape test is load-bearing rather than defensive: a policy directory routinely also holds the +document under evaluation. Without it, `plan.json` is evaluated *as a policy*, which reports a spurious +failure and buries the real findings. + +`-var-path` and `-var` apply to every policy evaluated, so a parameterised policy behaves the same +whether you name the file or the directory holding it. + +**No policy files found is an error, not a skip.** Pointed at a path with nothing in it, the command +exits `1`. A green result for a change nothing was evaluated against is the outcome this must never +produce, and "no policies found" is a configuration mistake rather than a deliberate skip. + +## Masking, and why it is opt-in + +`--input-kind` says what the document is, and supplying it **masks** the document before evaluation: +values terraform marked sensitive are replaced, root `variables` are dropped, `prior_state` is removed. + +It matters even though nothing is uploaded. Evaluator messages embed the actual attribute values they +compared, and those messages are copied verbatim into whatever comment or note your CI job posts — so +an unmasked run publishes plan values to a code host. Masking also keeps this verdict identical to +`platform check`'s for the same plan, because that path evaluates the masked document too. + +It is opt-in because masking changes the evaluator messages, and those messages are this command's +`--json` output, which is a frozen contract. With no `--input-kind` the file is read exactly as it +always has been. + +`terraform_plan` and `terraform_state` are masked. `kubernetes` and `json` are passed through +untouched: they carry no sensitivity markers to mask by, and tirith reads YAML for them, which a JSON +round-trip would break. + +## Writing the verdict out + +| | | +|---|---| +| `--output-json PATH` | The result document, in the [same shape](output-contract.md) `platform check` writes | +| `--output-markdown PATH` | A rendered report, for a pull-request comment or a merge-request note | +| `--comment-marker TEXT` | An opaque first line for the markdown, so a job can find and edit its own comment | +| `--markdown-limit N` | Truncate the markdown. Default 60000 | +| `--sha SHA` | The revision the findings describe, shown in the report | + +Both files are written on **every** exit path, including failures, and the markdown always begins with +`--comment-marker` when one is given. That matters to any caller that edits a sticky comment in place: +writing nothing on the failure path means the caller falls through to a body of its own with no marker +in it, and PATCHing that over a good comment orphans it permanently. It has happened. + +Nothing in this group changes what the command prints. + +## Exit codes + +| Code | Condition | +|---|---| +| 0 | Policies passed or warned. Also a failing policy without `--fail-on-error` | +| 1 | No policy files found; the input was missing or unparseable; a policy could not be evaluated; nothing was evaluated | +| 3 | A policy failed, with `--fail-on-error` | +| 130 | Interrupted | + +**A policy that could not be evaluated exits `1` regardless of `--fail-on-error`.** "Could not +evaluate" is tool health, not a policy decision. Such a policy also appears in the report as a visible +failure carrying its reason, so it is never mistakable for a pass and never silently dropped. + +**A run of nothing but skips exits `1`.** If every check was swallowed by its `error_tolerance` — +usually because the resources the policies name are not in the document — then nothing was examined, +and this command has always called that a failure rather than a pass. Note this is a deliberate +difference from `platform check`, which counts skips separately and reports them as a pass. + +## Flags + +The full surface, as the command prints it: + + usage: tirith [-h] [-policy-path PATH] [-input-path PATH] [-var-path PATH] [-var PATH] [--json] + [--verbose] [--fail-on-error] [--version] [--input-kind KIND] [--state-path PATH] + [--sha SHA] [--output-json PATH] [--output-markdown PATH] [--comment-marker TEXT] + [--markdown-limit N] + + Tirith (StackGuardian Policy Framework) + + options: + -h, --help show this help message and exit + -policy-path PATH Path containing Tirith policy as code + -input-path PATH Input file path + -var-path PATH Variable file path(s) + -var PATH Inline variable(s) + --json Only print the result in JSON form (useful for passing output to other programs) + --verbose Show detailed logs of from the run + --fail-on-error Exit 3 when a policy fails, instead of 0. Off by default for compatibility. + --version show program's version number and exit + + reporting: + Write the verdict out for a CI job to publish. All optional. + + --input-kind KIND terraform_plan, terraform_state, kubernetes or json. Masks the input before evaluating + --state-path PATH Terraform state, when --input-kind is terraform_state + --sha SHA Revision these findings describe, recorded in the report + --output-json PATH Write the result document here (same shape as `tirith platform check`) + --output-markdown PATH Write a markdown report here, for a comment or a note + --comment-marker TEXT Opaque first line of the markdown, for comment stickiness + --markdown-limit N Truncate the markdown to this length. Default: 60000 diff --git a/docs/output-contract.md b/docs/output-contract.md new file mode 100644 index 00000000..22f8e909 --- /dev/null +++ b/docs/output-contract.md @@ -0,0 +1,72 @@ +# The result document + +`--output-json` writes one document, in one shape, from both +[`tirith platform check`](platform-check.md) and [`tirith -policy-path …`](evaluating-policy-files.md). This page is +the contract, because two integrations read it — the +[GitHub Action](https://github.com/StackGuardian/tirith-iac-governance-action) and the GitLab CI +component — and turn it into comments, statuses and job outputs. + +## The rule + +**Every key exists in every mode.** A key with no meaning on a path is `null` (or `[]` for the lists), +never omitted and never invented. + +Omitting keys per mode would force every consumer to write two parsers, and the one that forgets +reports the wrong thing on the mode it was not tested against. Inventing them is worse: a fabricated +`wfrun_url` in local mode renders as a link to a run that does not exist. + +Both documents are built by one function, `tirith.platform.report.result_document`, so the shapes +cannot drift apart by editing one caller. + +## Keys + +| Key | Platform mode | Local mode | +|---|---|---| +| `mode` | `"platform"` | `"local"` | +| `status` | The workflow run's terminal status: `COMPLETED`, `ERRORED`, `CANCELLED`, `APPROVAL_REQUIRED` | `COMPLETED`, or `ERRORED` when nothing could be evaluated | +| `verdict` | `passed` · `warned` · `failed` · `no-policies` · `errored` | the same five | +| `counts` | `passed`, `failed`, `warned`, `approval_required`, `skipped`, `unknown` | the same six | +| `headline` | The one-line summary, e.g. `Tirith — 2 failed, 1 warned, 5 passed` | the same | +| `policy_results` | The `PolicyEvalResults` document from the run | the same shape, built from the local evaluation | +| `wfrun_id` | The run id | `null` | +| `wfrun_url` | Link to the run in StackGuardian | `null` | +| `monthly_cost` | `totalMonthlyCost` from the cost breakdown, or `null` | `null` — cost needs a second document | +| `archive_key` | Where the uploaded archive lives | `null` — nothing is uploaded | +| `source_packed` | Whether the archive actually holds the terraform source | `false` | +| `source_skipped_reason` | Why it does not, or `null` | `null` | +| `policies_evaluated` | `null` | How many policy files were evaluated | +| `policies_errored` | `null` | How many could not be | +| `policy_path` | `null` | The `--policy-path` that was used | +| `policy_errors` | `[]` | `[{"policy": path, "reason": str}]` | +| `policy_warnings` | `[]` | `[str]` — currently unrecognised `meta.enforcement` values | + +## Reading it + +**Read `mode` rather than inferring it** from which keys are populated. Inference is what makes +adding a key a breaking change. + +**`verdict` is the answer; the exit code is the gate.** They are related but not the same: `failed` +without `--fail-on-error` exits `0` on purpose. A consumer that wants to report the verdict and a +consumer that wants to block are reading two different things. + +**`counts.unknown` distinguishes "nothing failed" from "we could not read part of it."** Without it, +an errored run reports `failed: 0`, which a consumer copying counts to its own outputs turns into a +clean bill of health. A non-zero `unknown` is why `verdict` can be `errored` while `failed` is `0`. + +**`source_skipped_reason` is truthy only when there is a real reason.** It stays `null` in local mode +rather than carrying a sentinel like `"not_applicable"`, because a consumer's "the terraform source +was not uploaded" warning keys on truthiness — a sentinel would fire it on every local run, about an +upload that was never attempted. + +**`policy_errors` is structured so a front end can annotate without scraping stderr.** Everything in +it is also logged, but a log line is not a data source. + +## Written on every exit path + +Both `--output-json` and `--output-markdown` are written even when the check fails outright, with +`status: "ERRORED"`, `verdict: "errored"`, zeroed counts, and the reason in `policy_errors`. The +markdown carries `--comment-marker` as its first line. + +A consumer that edits a sticky comment in place depends on this. When these files were absent on the +failure path, the caller fell through to a body of its own with no marker in it, and PATCHing that +over a good comment orphaned it permanently. diff --git a/docs/platform-check.md b/docs/platform-check.md index 54eae4ec..c59d5f4d 100644 --- a/docs/platform-check.md +++ b/docs/platform-check.md @@ -7,6 +7,11 @@ The [GitHub Action](https://github.com/StackGuardian/tirith-iac-governance-actio around this command. Use the action on GitHub; use this directly anywhere else — GitLab CI, a Makefile, a local shell. +No StackGuardian organization? `tirith -policy-path … -input-path …` evaluates policy files committed +in your repository instead, with no credentials, and writes the same report — see +[evaluating policy files](evaluating-policy-files.md). The document both write is specified in +[the result document](output-contract.md). + ## What it does 1. **Masks the document on your machine**, before anything is uploaded. Values terraform marked diff --git a/documentation/docs/tirith-usage/cli-reference.md b/documentation/docs/tirith-usage/cli-reference.md index b8a726c5..c6244743 100644 --- a/documentation/docs/tirith-usage/cli-reference.md +++ b/documentation/docs/tirith-usage/cli-reference.md @@ -19,9 +19,14 @@ tirith -policy-path policy.json -input-path plan.json Run with no arguments, `tirith` prints its help text and exits `0`. -There is one subcommand, `tirith platform check`, which evaluates against the policies a -StackGuardian organization enforces instead of local files. It has its own flags and its own page: -[Platform Check](platform-check.md). +There is one subcommand, [`tirith platform check`](platform-check.md), which evaluates against the +policies a StackGuardian organization enforces instead of local files. It has its own flags and its own +page. + +The flags below are this command's own. Beyond evaluating a single policy file they also cover using it +as a CI gate — a directory of policies, a masked input, and the verdict written out for a job to publish +— which has [its own page](evaluating-policy-files.md). Both surfaces write the same +[result document](output-contract.md). ## Flags diff --git a/documentation/docs/tirith-usage/evaluating-policy-files.md b/documentation/docs/tirith-usage/evaluating-policy-files.md new file mode 100644 index 00000000..e52cace9 --- /dev/null +++ b/documentation/docs/tirith-usage/evaluating-policy-files.md @@ -0,0 +1,136 @@ +--- +id: evaluating-policy-files +title: Policy Files in CI +sidebar_label: Policy Files in CI +description: Using tirith -policy-path as a CI gate — a directory of policies, a masked input, and the verdict written out for a job to publish. +keywords: + - tirith + - ci + - policy as code + - gate +site_name: Tirith +slug: evaluating-policy-files/ +--- + +`tirith -policy-path … -input-path …` evaluates the policies committed in your repository. It needs no +account and makes no network calls, and it is the surface most open-source users are on. + +``` +tirith -policy-path .tirith/policies -input-path plan.json --fail-on-error +``` + +This page is about the flags that make it usable as a **CI gate**: several policies at once, the input +masked before its values reach a pull-request comment, and the verdict written out for a job to +publish. The alternative is +[`tirith platform check`](platform-check.md), which evaluates the policies a StackGuardian organization +enforces instead — and writes the [same result document](output-contract.md), so a CI integration built +against one works unchanged against the other. + +## Several policies at once + +`-policy-path` accepts a file, a **directory** or a **glob**. + +- **A file** is evaluated as given. Naming one explicitly is an instruction, so "that is not a policy" + is reported rather than the file being skipped. +- **A directory** is searched recursively for `*.tirith.json`. If it holds none, any `.json` file + *shaped* like a policy is used — an object with both `meta` and `evaluators`. +- **A glob** is expanded and filtered by that same shape test. + +The shape test is load-bearing rather than defensive: a policy directory routinely also holds the +document under evaluation. Without it, `plan.json` is evaluated *as a policy*, which reports a spurious +failure and buries the real findings. + +`-var-path` and `-var` apply to every policy evaluated, so a parameterised policy behaves the same +whether you name the file or the directory holding it. + +**No policy files found is an error, not a skip.** Pointed at a path with nothing in it, the command +exits `1`. A green result for a change nothing was evaluated against is the outcome this must never +produce, and "no policies found" is a configuration mistake rather than a deliberate skip. + +## Masking, and why it is opt-in + +`--input-kind` says what the document is, and supplying it **masks** the document before evaluation: +values terraform marked sensitive are replaced, root `variables` are dropped, `prior_state` is removed. + +It matters even though nothing is uploaded. Evaluator messages embed the actual attribute values they +compared, and those messages are copied verbatim into whatever comment or note your CI job posts — so +an unmasked run publishes plan values to a code host. Masking also keeps this verdict identical to +`platform check`'s for the same plan, because that path evaluates the masked document too. + +It is opt-in because masking changes the evaluator messages, and those messages are this command's +`--json` output, which is a frozen contract. With no `--input-kind` the file is read exactly as it +always has been. + +`terraform_plan` and `terraform_state` are masked. `kubernetes` and `json` are passed through +untouched: they carry no sensitivity markers to mask by, and tirith reads YAML for them, which a JSON +round-trip would break. + +## Writing the verdict out + +| | | +|---|---| +| `--output-json PATH` | The result document, in the [same shape](output-contract.md) `platform check` writes | +| `--output-markdown PATH` | A rendered report, for a pull-request comment or a merge-request note | +| `--comment-marker TEXT` | An opaque first line for the markdown, so a job can find and edit its own comment | +| `--markdown-limit N` | Truncate the markdown. Default 60000 | +| `--sha SHA` | The revision the findings describe, shown in the report | + +Both files are written on **every** exit path, including failures, and the markdown always begins with +`--comment-marker` when one is given. That matters to any caller that edits a sticky comment in place: +writing nothing on the failure path means the caller falls through to a body of its own with no marker +in it, and PATCHing that over a good comment orphans it permanently. It has happened. + +Nothing in this group changes what the command prints. + +## Exit codes + +| Code | Condition | +|---|---| +| 0 | Policies passed or warned. Also a failing policy without `--fail-on-error` | +| 1 | No policy files found; the input was missing or unparseable; a policy could not be evaluated; nothing was evaluated | +| 3 | A policy failed, with `--fail-on-error` | +| 130 | Interrupted | + +**A policy that could not be evaluated exits `1` regardless of `--fail-on-error`.** "Could not +evaluate" is tool health, not a policy decision. Such a policy also appears in the report as a visible +failure carrying its reason, so it is never mistakable for a pass and never silently dropped. + +**A run of nothing but skips exits `1`.** If every check was swallowed by its `error_tolerance` — +usually because the resources the policies name are not in the document — then nothing was examined, +and this command has always called that a failure rather than a pass. Note this is a deliberate +difference from `platform check`, which counts skips separately and reports them as a pass. + +## Flags + +The full surface, as the command prints it: + +```text +usage: tirith [-h] [-policy-path PATH] [-input-path PATH] [-var-path PATH] [-var PATH] [--json] + [--verbose] [--fail-on-error] [--version] [--input-kind KIND] [--state-path PATH] + [--sha SHA] [--output-json PATH] [--output-markdown PATH] [--comment-marker TEXT] + [--markdown-limit N] + +Tirith (StackGuardian Policy Framework) + +options: + -h, --help show this help message and exit + -policy-path PATH Path containing Tirith policy as code + -input-path PATH Input file path + -var-path PATH Variable file path(s) + -var PATH Inline variable(s) + --json Only print the result in JSON form (useful for passing output to other programs) + --verbose Show detailed logs of from the run + --fail-on-error Exit 3 when a policy fails, instead of 0. Off by default for compatibility. + --version show program's version number and exit + +reporting: + Write the verdict out for a CI job to publish. All optional. + + --input-kind KIND terraform_plan, terraform_state, kubernetes or json. Masks the input before evaluating + --state-path PATH Terraform state, when --input-kind is terraform_state + --sha SHA Revision these findings describe, recorded in the report + --output-json PATH Write the result document here (same shape as `tirith platform check`) + --output-markdown PATH Write a markdown report here, for a comment or a note + --comment-marker TEXT Opaque first line of the markdown, for comment stickiness + --markdown-limit N Truncate the markdown to this length. Default: 60000 +``` \ No newline at end of file diff --git a/documentation/docs/tirith-usage/output-contract.md b/documentation/docs/tirith-usage/output-contract.md new file mode 100644 index 00000000..e54b466d --- /dev/null +++ b/documentation/docs/tirith-usage/output-contract.md @@ -0,0 +1,84 @@ +--- +id: output-contract +title: The Result Document +sidebar_label: Result Document +description: The JSON document tirith writes with --output-json, identical whether policies come from your repository or your organization, and the contract CI integrations read it by. +keywords: + - tirith + - output + - json + - ci +site_name: Tirith +slug: output-contract/ +--- + +`--output-json` writes one document, in one shape, from both +[`tirith platform check`](platform-check.md) and [`tirith -policy-path …`](evaluating-policy-files.md). This page is +the contract, because two integrations read it — the +[GitHub Action](https://github.com/StackGuardian/tirith-iac-governance-action) and the GitLab CI +component — and turn it into comments, statuses and job outputs. + +## The rule + +**Every key exists in every mode.** A key with no meaning on a path is `null` (or `[]` for the lists), +never omitted and never invented. + +Omitting keys per mode would force every consumer to write two parsers, and the one that forgets +reports the wrong thing on the mode it was not tested against. Inventing them is worse: a fabricated +`wfrun_url` in local mode renders as a link to a run that does not exist. + +Both documents are built by one function, `tirith.platform.report.result_document`, so the shapes +cannot drift apart by editing one caller. + +## Keys + +| Key | Platform mode | Local mode | +|---|---|---| +| `mode` | `"platform"` | `"local"` | +| `status` | The workflow run's terminal status: `COMPLETED`, `ERRORED`, `CANCELLED`, `APPROVAL_REQUIRED` | `COMPLETED`, or `ERRORED` when nothing could be evaluated | +| `verdict` | `passed` · `warned` · `failed` · `no-policies` · `errored` | the same five | +| `counts` | `passed`, `failed`, `warned`, `approval_required`, `skipped`, `unknown` | the same six | +| `headline` | The one-line summary, e.g. `Tirith — 2 failed, 1 warned, 5 passed` | the same | +| `policy_results` | The `PolicyEvalResults` document from the run | the same shape, built from the local evaluation | +| `wfrun_id` | The run id | `null` | +| `wfrun_url` | Link to the run in StackGuardian | `null` | +| `monthly_cost` | `totalMonthlyCost` from the cost breakdown, or `null` | `null` — cost needs a second document | +| `archive_key` | Where the uploaded archive lives | `null` — nothing is uploaded | +| `source_packed` | Whether the archive actually holds the terraform source | `false` | +| `source_skipped_reason` | Why it does not, or `null` | `null` | +| `policies_evaluated` | `null` | How many policy files were evaluated | +| `policies_errored` | `null` | How many could not be | +| `policy_path` | `null` | The `--policy-path` that was used | +| `policy_errors` | `[]` | `[{"policy": path, "reason": str}]` | +| `policy_warnings` | `[]` | `[str]` — currently unrecognised `meta.enforcement` values | + +## Reading it + +**Read `mode` rather than inferring it** from which keys are populated. Inference is what makes +adding a key a breaking change. + +**`verdict` is the answer; the exit code is the gate.** They are related but not the same: `failed` +without `--fail-on-error` exits `0` on purpose. A consumer that wants to report the verdict and a +consumer that wants to block are reading two different things. + +**`counts.unknown` distinguishes "nothing failed" from "we could not read part of it."** Without it, +an errored run reports `failed: 0`, which a consumer copying counts to its own outputs turns into a +clean bill of health. A non-zero `unknown` is why `verdict` can be `errored` while `failed` is `0`. + +**`source_skipped_reason` is truthy only when there is a real reason.** It stays `null` in local mode +rather than carrying a sentinel like `"not_applicable"`, because a consumer's "the terraform source +was not uploaded" warning keys on truthiness — a sentinel would fire it on every local run, about an +upload that was never attempted. + +**`policy_errors` is structured so a front end can annotate without scraping stderr.** Everything in +it is also logged, but a log line is not a data source. + +## Written on every exit path + +Both `--output-json` and `--output-markdown` are written even when the check fails outright, with +`status: "ERRORED"`, `verdict: "errored"`, zeroed counts, and the reason in `policy_errors`. The +markdown carries `--comment-marker` as its first line. + +A consumer that edits a sticky comment in place depends on this. When these files were absent on the +failure path, the caller fell through to a body of its own with no marker in it, and PATCHing that +over a good comment orphaned it permanently. diff --git a/documentation/sidebars.js b/documentation/sidebars.js index df3c036e..bf72e006 100644 --- a/documentation/sidebars.js +++ b/documentation/sidebars.js @@ -25,6 +25,8 @@ module.exports = { "tirith-usage/exit-codes", "tirith-usage/ci-integration", "tirith-usage/platform-check", + "tirith-usage/evaluating-policy-files", + "tirith-usage/output-contract", ] }, { diff --git a/setup.py b/setup.py index 6dc8be09..2f49e1f7 100644 --- a/setup.py +++ b/setup.py @@ -22,7 +22,7 @@ def read(*names, **kwargs): setup( name="py-tirith", - version="1.2.0", + version="1.3.0", license="Apache", description="Tirith simplifies defining Policy as Code.", long_description_content_type="text/markdown", diff --git a/src/tirith/__init__.py b/src/tirith/__init__.py index 4c2aac77..b3fd7cbc 100644 --- a/src/tirith/__init__.py +++ b/src/tirith/__init__.py @@ -2,6 +2,6 @@ tirith: Execute policies defined using Tirith (StackGuardian Policy Framework) """ -__version__ = "1.2.0" +__version__ = "1.3.0" __author__ = "StackGuardian" __license__ = "Apache" diff --git a/src/tirith/cli.py b/src/tirith/cli.py index f75c9417..5667388e 100755 --- a/src/tirith/cli.py +++ b/src/tirith/cli.py @@ -151,6 +151,74 @@ def __init__(self, prog="PROG") -> None: ) parser.add_argument("--version", action="version", version=__version__) + # Everything below is additive, and off unless asked for. With none of it, this command + # behaves exactly as it always has -- which matters because its `--json` stdout is a frozen + # contract, pinned byte-for-byte by tests/core/test_output_compatibility.py. + # + # What they add is the shape a CI integration needs: many policies rather than one, the input + # masked before its values reach a report, and the verdict written out as a document and a + # markdown body for a front end to publish. Those are the same four files + # `tirith platform check` writes, so a GitHub Action or a GitLab component can drive either + # with one argv and one parser. + reporting = parser.add_argument_group( + "reporting", "Write the verdict out for a CI job to publish. All optional." + ) + reporting.add_argument( + "--input-kind", + dest="inputKind", + # A metavar, because the formatter aligns every option to the widest one and spelling the + # choices out here pushed the whole help block far to the right. + metavar="KIND", + choices=("terraform_plan", "terraform_state", "kubernetes", "json"), + default=None, + # Masking is opt-in because it changes the evaluator messages, and those messages are + # the frozen --json output. Omitted, the file is read exactly as it always has been. + help="terraform_plan, terraform_state, kubernetes or json. Masks the input before evaluating", + ) + reporting.add_argument( + "--state-path", + metavar="PATH", + dest="statePath", + default=None, + help="Terraform state, when --input-kind is terraform_state", + ) + reporting.add_argument( + "--sha", + metavar="SHA", + dest="sha", + default=None, + help="Revision these findings describe, recorded in the report", + ) + reporting.add_argument( + "--output-json", + metavar="PATH", + dest="outputJson", + default=None, + help="Write the result document here (same shape as `tirith platform check`)", + ) + reporting.add_argument( + "--output-markdown", + metavar="PATH", + dest="outputMarkdown", + default=None, + help="Write a markdown report here, for a comment or a note", + ) + reporting.add_argument( + "--comment-marker", + metavar="TEXT", + dest="commentMarker", + default=None, + help="Opaque first line of the markdown, for comment stickiness", + ) + reporting.add_argument( + "--markdown-limit", + metavar="N", + type=int, + dest="markdownLimit", + default=60000, + help="Truncate the markdown to this length. Default: 60000", + ) + args = parser.parse_args(argv) if not argv: @@ -174,6 +242,14 @@ def __init__(self, prog="PROG") -> None: else: setup_logging(verbose=args.verbose) + # The aggregate path: many policies, or any of the reporting flags. A single policy file with + # none of them falls through to the original code below, byte-for-byte -- which is what keeps + # the frozen --json contract frozen, and every existing caller unaffected. + from tirith.local import check as local_check + + if local_check.wanted(args): + return local_check.run(args) + try: result = start_policy_evaluation(args.policyPath, args.inputPath, args.varPaths, args.inlineVars) diff --git a/src/tirith/local/__init__.py b/src/tirith/local/__init__.py new file mode 100644 index 00000000..499eed68 --- /dev/null +++ b/src/tirith/local/__init__.py @@ -0,0 +1,27 @@ +""" +The aggregate path behind `tirith -policy-path ...` -- many policies, masked input, one report. + +Everything here runs on the machine it is invoked from and talks to nothing. Policy files committed +in the repository are evaluated against a document, and the same report the platform path produces +is written out, so a caller's reporting is identical in both modes. + +Two deliberate choices about *how* this reuses the rest of tirith, both about not owning a second +copy of anything: + + * Rendering, masking and document discovery are imported from `tirith.platform` + (`report` / `redact` / `discover`). A second markdown renderer would drift from the platform + path's, and the whole point is that both modes report identically. Those three modules live + under `platform/` for historical reasons and are mode-agnostic; they are not moved here, because + code outside this repository imports them by that path. + * Evaluation shells out to `tirith -policy-path P -input-path I --json`, one policy at a time. + That stdout document is tirith's frozen contract -- pinned byte-for-byte by + tests/core/test_output_compatibility.py -- which makes it the most stable interface it has. + Calling the engine's Python API instead would couple this to internals that carry no such + promise. Now that this code lives inside the package, the temptation to do so is real; the + argv is pinned by a test so that becomes a deliberate change rather than a silent one. + +This began life in the GitHub Action, which needed to be usable before anyone had a StackGuardian +account. It moved here so a second front end (GitLab CI) drives one implementation rather than forking +it -- and onto the existing flat command rather than a subcommand of its own, so there is one way to +evaluate committed policy files rather than two that drift. +""" diff --git a/src/tirith/local/check.py b/src/tirith/local/check.py new file mode 100644 index 00000000..2a8663a2 --- /dev/null +++ b/src/tirith/local/check.py @@ -0,0 +1,213 @@ +""" +The aggregate path behind `tirith -policy-path ...`. + +The flat command has always evaluated exactly one policy file and printed the engine's own document. +That is the right shape for a person at a terminal and the wrong one for a CI job, which needs many +policies from a directory, the input masked before its values reach a pull-request comment, and one +verdict written out for something else to publish. + +This module is that second shape. It is reached only when the caller asks for it -- a policy path that +is not a single file, or any of the reporting flags -- so the original path, and the frozen `--json` +output it prints, are untouched. + +It writes the same two files `tirith platform check` writes, in the same shapes, so a front end can +drive either with one argv and one parser. +""" + +import os +import tempfile + +from ..platform import report +from ..platform.check import log, write_output_json, write_output_markdown +from .evaluate import LocalError, discover_policies, evaluate, prepare_input + +# The flags that mean "give me the CI shape". Grouped here rather than checked inline so that adding +# one cannot accidentally leave the routing behind. +REPORTING_ARGS = ("inputKind", "statePath", "sha", "outputJson", "outputMarkdown", "commentMarker") + + +def wanted(args): + """ + Whether this path should handle the invocation. + + Narrow on purpose. A single policy file with none of the reporting flags is the invocation every + existing caller makes, and it must keep reaching the original code -- so this returns False for it + even though this module could handle it perfectly well. + """ + if any(getattr(args, name, None) for name in REPORTING_ARGS): + return True + # A directory or a glob. Previously this reached `open()` and failed with a bare "ERROR", so + # handling it is new capability rather than changed behaviour. + return not os.path.isfile(args.policyPath) + + +def run(args): + """Evaluate every policy found, write the report, and return an exit status.""" + from ..status import ExitStatus + + policy_path = args.policyPath + + try: + result = _evaluate(args, policy_path) + except LocalError as e: + log(f"ERROR: {e}") + _write_failure(args, str(e)) + return ExitStatus.ERROR + except KeyboardInterrupt: + log("Interrupted") + return ExitStatus.ERROR_CTRL_C + + if args.json: + # The aggregate document, not the engine's -- there is no single engine document when several + # policies ran. Only reachable once a reporting flag or a multi-policy path was asked for, so + # no existing caller sees this instead of what it used to get. + import json as _json + + print(_json.dumps(result, indent=2)) + + log(result["headline"]) + + if result["policies_errored"]: + # "Could not evaluate" is tool health, not a policy decision, so it ignores --fail-on-error -- + # the same reason an unreachable platform does in `platform check`. + log("Some policies could not be evaluated") + return ExitStatus.ERROR + + verdict = result["verdict"] + if verdict == "errored": + return ExitStatus.ERROR + if verdict == "failed" and args.failOnError: + return ExitStatus.ERROR_POLICY_FAILED + if verdict == "failed": + log("Policies failed, but --fail-on-error was not set") + if result.get("counts", {}).get("approval_required"): + log("Some policies ask for approval; reported as a warning, which does not block") + + return ExitStatus.SUCCESS + + +def _evaluate(args, policy_path): + policies = discover_policies(policy_path) + if not policies: + # A green result for a change nothing was evaluated against is the one outcome this must never + # produce. "No policies found" is a configuration mistake, not a deliberate skip. + raise LocalError( + f"Nothing to evaluate: no policy files found at '{policy_path}'. Point -policy-path at a " + "file, a directory or a glob containing tirith policies." + ) + + warnings = [] + + with tempfile.TemporaryDirectory(prefix="tirith-local-") as scratch: + input_path, redactions = prepare_input( + args.inputPath, + None, + None, + args.inputKind or "raw", + os.path.dirname(args.inputPath) or ".", + scratch, + state_path=args.statePath, + ) + if redactions: + log(f"Masked {redactions} sensitive value(s) before evaluating") + + log(f"Evaluating {len(policies)} policy file(s) from '{policy_path}'") + + def on_unknown_enforcement(value): + message = f"unrecognised meta.enforcement '{value}'; treating a failing policy as blocking" + warnings.append(message) + log(f"WARNING: {message}") + + policy_results, errored = evaluate( + policies, + input_path, + on_unknown_enforcement=on_unknown_enforcement, + var_paths=args.varPaths, + inline_vars=args.inlineVars, + ) + + for path, reason in errored: + log(f"WARNING: could not evaluate {path}: {reason}") + + counts, _findings = report.summarize(policy_results) + evaluated_something = any( + counts.get(key) for key in (report.PASS, report.FAIL, report.WARN, report.APPROVAL_REQUIRED, report.UNKNOWN) + ) + + # Every policy skipped means every check was swallowed by error_tolerance and nothing was actually + # examined. This command has always called that a failure rather than a pass -- "None is not a + # pass" -- and extending it must not quietly reverse that for a caller who passed a directory. + # + # Note this is a deliberate difference from `platform check`, which counts skips separately and + # reports them as a pass. Both are defensible; what is not defensible is one surface silently + # disagreeing with itself depending on how many policies you pointed it at. + status = "COMPLETED" + notes = None + if not evaluated_something: + status = "ERRORED" + notes = [ + f"All {len(policies)} policy file(s) were skipped, so nothing was evaluated. Every check " + "was swallowed by its error_tolerance -- usually because the resources the policies name " + "are not in this document." + ] + + result = report.result_document( + "local", + status, + policy_results, + policies_evaluated=len(policies), + policies_errored=len(errored), + policy_path=policy_path, + policy_errors=[{"policy": path, "reason": reason} for path, reason in errored], + policy_warnings=warnings, + ) + + write_output_json(args.outputJson, result) + if args.outputMarkdown: + write_output_markdown( + args.outputMarkdown, + report.render_markdown( + policy_results, + status, + None, + marker=args.commentMarker, + limit=args.markdownLimit, + commit=args.sha, + notes=notes, + ), + ) + return result + + +def _write_failure(args, message): + """ + Write both output files for a run that produced no verdict. + + A front end editing a sticky comment in place needs a marker-first body even here: writing nothing + means it falls through to a body of its own with no marker, and PATCHing that over a good comment + orphans it permanently. + """ + result = report.result_document( + "local", + "ERRORED", + {}, + policies_evaluated=0, + policies_errored=0, + policy_path=args.policyPath, + policy_errors=[{"policy": None, "reason": message}], + ) + write_output_json(args.outputJson, result) + if args.outputMarkdown: + write_output_markdown( + args.outputMarkdown, + report.render_markdown( + {}, + "ERRORED", + None, + marker=args.commentMarker, + limit=args.markdownLimit, + commit=args.sha, + notes=[message], + ), + ) + return result diff --git a/src/tirith/local/evaluate.py b/src/tirith/local/evaluate.py new file mode 100644 index 00000000..f9578d1e --- /dev/null +++ b/src/tirith/local/evaluate.py @@ -0,0 +1,345 @@ +""" +Discover policy files, mask the input document, and evaluate one policy at a time. + +Moved from the GitHub Action, where this was `tirith_action/local.py`, so that both front ends drive +one implementation. See the package docstring for why evaluation shells out. +""" + +import glob as _glob +import json +import os +import subprocess +import sys + +from ..platform import discover, redact, report + + +class LocalError(Exception): + """Local evaluation could not be completed. Always fails closed.""" + + +# Preferred policy naming. A directory holding any of these is treated as an explicit policy +# directory and nothing else in it is considered. +POLICY_SUFFIX = ".tirith.json" + +# Per policy. A policy is pure computation over a parsed document, so this only trips on a +# pathological evaluator, but a job that hangs forever is worse than one that fails. +EVALUATION_TIMEOUT = 300 + +# `meta.enforcement` values that downgrade a failing policy to a warning. Anything unrecognised +# fails instead -- an unlabelled or mislabelled policy must gate, not slip through. +# +# The approval spellings warn rather than gate, matching platform mode, where a policy carrying +# `onFail: APPROVAL_REQUIRED` also warns: the run finishes before the intent is known, so there is +# nothing to approve. Local mode has no approval mechanism at all, so failing closed on it would +# block a change with no way to unblock it. +WARN_ENFORCEMENTS = ( + "soft_mandatory", + "advisory", + "warn", + "warning", + "low", + "approval_required", + "approval-required", + "approval", +) + +# Recognised, and gate. Listed rather than left to the `else` so a correctly-labelled blocking +# policy does not raise the "unrecognised enforcement" warning on every run it fails -- +# `hard_mandatory` is what tirith's own golden test pins, so that fired constantly. +FAIL_ENFORCEMENTS = ("hard_mandatory", "mandatory", "fail", "error", "high", "critical", "blocking") + + +def _looks_like_policy(path): + """ + Whether a .json file is a tirith policy. + + Load-bearing rather than defensive. A policy directory routinely also holds the document under + evaluation (`plan.json`), and `--policy-path` accepts a glob that a user may well point at a + directory full of mixed JSON. Without this check the input document is evaluated *as a policy*, + which reports a spurious failure and buries the real findings. + """ + try: + with open(path) as f: + data = json.load(f) + except (json.JSONDecodeError, OSError, UnicodeDecodeError): + return False + return isinstance(data, dict) and "meta" in data and "evaluators" in data + + +def discover_policies(policy_path): + """ + Resolve `--policy-path` to a list of policy files. + + A file is taken as given -- if a user names one explicitly, evaluating it is the instruction, + and reporting "that is not a policy" is more useful than silently skipping it. + """ + if not policy_path: + return [] + + if os.path.isfile(policy_path): + return [policy_path] + + if "*" in policy_path or "?" in policy_path: + matches = sorted(p for p in _glob.glob(policy_path, recursive=True) if os.path.isfile(p)) + return [p for p in matches if _looks_like_policy(p)] + + if os.path.isdir(policy_path): + explicit = sorted(_glob.glob(os.path.join(policy_path, "**", "*" + POLICY_SUFFIX), recursive=True)) + if explicit: + return explicit + candidates = sorted(_glob.glob(os.path.join(policy_path, "**", "*.json"), recursive=True)) + return [p for p in candidates if _looks_like_policy(p)] + + return [] + + +def read_json(path, label): + """ + Read a JSON document, or raise LocalError. + + Deliberately near-identical to platform.check.read_json rather than shared with it: the two + raise different exception types, and ten lines is not worth an abstraction that would have to + take the exception class as a parameter. + """ + if not os.path.exists(path): + raise LocalError(f"{label} not found: {path}") + try: + with open(path) as f: + return json.load(f) + except json.JSONDecodeError as e: + raise LocalError(f"{label} is not valid JSON ({path}): {e}") + except OSError as e: + raise LocalError(f"Could not read {label} ({path}): {e}") + + +def prepare_input(input_path, plan_file, terraform_bin, input_kind, source_dir, scratch, state_path=None): + """ + Resolve the document to evaluate and mask it, returning (path, redaction_count). + + Masking matters here even though nothing is uploaded. Evaluator messages embed the actual + attribute values they compared, and those messages are copied verbatim into whatever comment or + note the caller posts -- so an unmasked local run publishes plan values to a code host. Masking + also keeps a local verdict identical to the platform one for the same plan, because the platform + path evaluates the masked document too. + + `json` and `kubernetes` documents are passed through untouched: they carry no sensitivity + markers to mask by, and tirith reads YAML for them, which a JSON round-trip here would break. + """ + # `raw` is what the flat command does with no --input-kind: read the named file, mask nothing. + # `json` and `kubernetes` are passed through for a different reason -- they carry no sensitivity + # markers to mask by, and tirith reads YAML for them, which a JSON round-trip here would break. + if input_kind not in ("terraform_plan", "terraform_state"): + if not input_path: + raise LocalError("-input-path is required.") + if not os.path.exists(input_path): + raise LocalError(f"input document not found: {input_path}") + return input_path, 0 + + if input_path and plan_file: + raise LocalError("--input-path and --plan-file cannot be combined; pass one of them.") + + # `--state-path` is how the platform path is told which document to evaluate for a + # terraform_state check. Ignoring it here sent local mode to discovery, which finds plan.json -- + # so the two modes evaluated *different documents* from identical inputs, and a violation + # present only in the state was reported as a pass. + if input_kind == "terraform_state" and not input_path and not plan_file and state_path: + input_path = state_path + + if plan_file: + try: + document = discover.terraform_show_json(plan_file, binary=terraform_bin or None) + except discover.DiscoveryError as e: + raise LocalError(str(e)) + elif input_path: + document = read_json(input_path, "input document") + else: + try: + resolved = discover.discover_input(source_dir or ".") + except discover.DiscoveryError as e: + raise LocalError(str(e)) + document = read_json(resolved, "input document") + + if input_kind == "terraform_state": + masked = redact.redact_state(document) + else: + masked = redact.redact_plan(document) + redactions = redact.count_redactions(masked) + + masked_path = os.path.join(scratch, "tirith-input.json") + try: + with open(masked_path, "w") as f: + json.dump(masked, f) + except OSError as e: + raise LocalError(f"Could not write the masked document to {masked_path}: {e}") + + return masked_path, redactions + + +def engine_argv(policy_path, input_path, var_paths=(), inline_vars=()): + """ + The argv used to evaluate one policy. + + `sys.executable -m tirith` rather than a `tirith` on PATH, so the interpreter that evaluates is + the one whose renderer was imported above -- a `tirith` on PATH could be a different + installation entirely. Factored out so a test can pin it: see the package docstring for why this + must stay a subprocess against the frozen `--json` contract. + + `-var-path` and `-var` are passed through because they are flags of the same command. Dropping + them when several policies are evaluated instead of one would make a parameterised policy work + against a file and fail against the directory holding it, for no reason a user could see. + """ + argv = [sys.executable, "-m", "tirith", "-policy-path", policy_path, "-input-path", input_path] + for var_path in var_paths or (): + argv += ["-var-path", var_path] + for inline_var in inline_vars or (): + argv += ["-var", inline_var] + return argv + + +def _evaluate_one(policy_path, input_path, var_paths=(), inline_vars=()): + """ + Run one policy. Returns (document, error_message); exactly one is set. + + `--json` disables logging process-wide and prints a bare `{}` on failure, so the failure reason + is not on either stream. When that happens the policy is re-run without `--json` purely to + recover a message worth showing -- one extra subprocess, only ever on the error path. + """ + argv = engine_argv(policy_path, input_path, var_paths, inline_vars) + + try: + completed = subprocess.run(argv + ["--json"], capture_output=True, text=True, timeout=EVALUATION_TIMEOUT) + except subprocess.TimeoutExpired: + return None, f"evaluation timed out after {EVALUATION_TIMEOUT}s" + except OSError as e: + return None, f"could not run tirith: {e}" + + if completed.returncode != 0: + return None, _recover_error(argv) + + try: + document = json.loads(completed.stdout) + except json.JSONDecodeError: + return None, "tirith produced no parseable result document" + + if not isinstance(document, dict) or not document: + return None, _recover_error(argv) + + # A bare {"errors": [...]} with no final_result is what unresolved policy variables produce. + if "final_result" not in document: + return None, "; ".join(str(e) for e in document.get("errors") or []) or "no result was produced" + + errors = document.get("errors") or [] + if errors: + return None, "; ".join(str(e) for e in errors) + + return document, None + + +def _recover_error(argv): + """Re-run without --json to get a human-readable reason out of the logger.""" + try: + completed = subprocess.run(argv, capture_output=True, text=True, timeout=EVALUATION_TIMEOUT) + except (subprocess.TimeoutExpired, OSError): + return "evaluation failed" + + stderr = (completed.stderr or "").strip() + if stderr: + # The logger writes a traceback for provider errors; the last line carries the cause. + return stderr.splitlines()[-1][:500] + return "evaluation failed" + + +def _fails(document): + """ + The failed evaluators, reshaped for the renderer. + + `description` is deliberately dropped rather than passed through. The renderer treats any entry + carrying a `description` key as a Checkov finding and reads only that field, skipping the + per-resource messages and addresses entirely -- and tirith's evaluator entries always carry the + key, often with a null value, which renders as a detail block with nothing in it at all. Sending + just `result` routes them down the branch written for this shape, which is where the actual + findings and the resource addresses come from. + + The evaluator description is not lost information worth keeping here: the rule name already + names the policy in both the table and the summary line, and the messages are specific to the + resources that failed. + """ + fails = [] + for evaluator in document.get("evaluators") or []: + if evaluator.get("passed"): + continue + entry = {"result": evaluator.get("result") or []} + if evaluator.get("id"): + entry["id"] = evaluator["id"] + fails.append(entry) + return fails + + +def _identity(document, policy_path): + """(policy_id, rule_name) for a result, falling back to the filename.""" + meta = document.get("meta") or {} + stem = os.path.basename(policy_path) + for suffix in (POLICY_SUFFIX, ".json"): + if stem.endswith(suffix): + stem = stem[: -len(suffix)] + break + policy_id = meta.get("id") or stem + return str(policy_id), str(meta.get("name") or policy_id) + + +def _failure_result(document, on_unknown_enforcement): + """FAIL, unless the policy labelled itself as advisory.""" + enforcement = (document.get("meta") or {}).get("enforcement") + if enforcement is None: + return report.FAIL + normalised = str(enforcement).strip().lower() + if normalised in WARN_ENFORCEMENTS: + return report.WARN + if normalised in FAIL_ENFORCEMENTS: + return report.FAIL + on_unknown_enforcement(str(enforcement)) + return report.FAIL + + +def evaluate(policy_paths, input_path, on_unknown_enforcement=lambda _: None, var_paths=(), inline_vars=()): + """ + Evaluate every policy and build the PolicyEvalResults document the renderer consumes. + + Returns (policy_results, errored) where `errored` lists (policy, reason) pairs. A policy that + could not be evaluated becomes a FAIL rule carrying an `exec_err`, which the renderer surfaces + as `engine: `: visible in the report, distinguishable from a real violation, and never + mistakable for a pass. The caller still fails regardless of --fail-on-error -- "could not + evaluate" is a tool failure, not a policy decision. + """ + policy_results = {} + errored = [] + + for policy_path in policy_paths: + document, error = _evaluate_one(policy_path, input_path, var_paths, inline_vars) + + if error is not None: + policy_id, rule_name = _identity({}, policy_path) + rule = { + "rule_name": rule_name, + "result": report.FAIL, + "evaluations": {"fails": [{"exec_err": f"{policy_path}: {error}"}]}, + } + errored.append((policy_path, error)) + else: + policy_id, rule_name = _identity(document, policy_path) + final = document.get("final_result") + if final is None: + rule = {"rule_name": rule_name, "skip": True} + elif final: + rule = {"rule_name": rule_name, "result": report.PASS} + else: + rule = { + "rule_name": rule_name, + "result": _failure_result(document, on_unknown_enforcement), + "evaluations": {"fails": _fails(document)}, + } + + policy_results.setdefault(policy_id, []).append(rule) + + return policy_results, errored diff --git a/src/tirith/platform/check.py b/src/tirith/platform/check.py index 643bf627..22bdbb4a 100644 --- a/src/tirith/platform/check.py +++ b/src/tirith/platform/check.py @@ -503,6 +503,64 @@ def upload_state_document(client, opts, state): ) +def write_output_markdown(path, body): + """Best-effort, mirroring write_output_json: a report we could not write is not a failed check.""" + if not path: + return + try: + with open(path, "w") as f: + f.write(body) + except OSError as e: + log(f"WARNING: could not write {path}: {e}") + + +def write_failure_report(opts, message): + """ + Write both output files for a check that produced no verdict. + + These paths used to return having written nothing, so a caller fell through to a body of its own + with no marker in it -- and PATCHing that over a good sticky comment orphans it permanently. + Writing both keeps the comment findable and puts the reason on the pull request rather than only + in the job log. + + Any run identity already recorded is preserved: the pre-poll write puts it there precisely so a + timeout leaves the run discoverable, and replacing it with nulls would throw that away. + """ + run_id = run_url = None + if opts.output_json and os.path.exists(opts.output_json): + try: + with open(opts.output_json) as f: + previous = json.load(f) + run_id = previous.get("wfrun_id") + run_url = previous.get("wfrun_url") + except (json.JSONDecodeError, OSError, AttributeError): + pass + + result = report.result_document( + "platform", + "ERRORED", + {}, + wfrun_id=run_id, + wfrun_url=run_url, + policy_errors=[{"policy": None, "reason": message}], + ) + write_output_json(opts.output_json, result) + + if opts.output_markdown: + body = report.render_markdown( + {}, + "ERRORED", + run_url, + marker=opts.comment_marker, + limit=opts.markdown_limit, + commit=opts.sha, + notes=[message], + ) + write_output_markdown(opts.output_markdown, body) + + return result + + def run_check(opts): """ Execute the check. Returns the result document. @@ -599,8 +657,13 @@ def run_check(opts): ) log(f"Run created: {run_url}") - # Written before polling so a timeout still leaves the run discoverable. - write_output_json(opts.output_json, {"status": "RUNNING", "wfrun_id": run_id, "wfrun_url": run_url}) + # Written before polling so a timeout still leaves the run discoverable. A full document rather + # than the three keys it used to carry: a consumer must not have to special-case a partial shape, + # and `verdict` is honestly `errored` until a run has produced one. + write_output_json( + opts.output_json, + report.result_document("platform", "RUNNING", {}, wfrun_id=run_id, wfrun_url=run_url), + ) try: status, _run = client.wait_for_run( @@ -659,42 +722,17 @@ def run_check(opts): # a view that serves only GET and POST. log(f"Retained the project archive for autofix: {key}") - counts, _findings = report.summarize(policy_results) - verdict_value = report.verdict(counts, status) - - result = { - "status": status, - "verdict": verdict_value, - "counts": { - "passed": counts.get(report.PASS, 0), - "failed": counts.get(report.FAIL, 0), - "warned": counts.get(report.WARN, 0), - "approval_required": counts.get(report.APPROVAL_REQUIRED, 0), - "skipped": counts.get("SKIPPED", 0), - # Published so a consumer can tell "nothing failed" from "we could not read part of - # it". Without it an errored run reported failed: 0, which the action copies straight - # to its `failed` output. - "unknown": counts.get(report.UNKNOWN, 0), - }, - "headline": report.headline(counts, verdict_value), - "wfrun_id": run_id, - "wfrun_url": run_url, - "policy_results": policy_results or {}, - # Surfaced for a caller aggregating several units into one comment of their own. - "monthly_cost": (cost_breakdown or {}).get("totalMonthlyCost"), - # Where the evaluated source lives. The autofix system reads this to fetch what produced - # the findings; it is also recorded on the run itself as SGCustomWorkflowRunFacts, so a - # consumer holding only a run id can find it without seeing this document. - "archive_key": key, - # Whether that archive actually contains the source. Normally true, and false when the tree - # was too large and got dropped so the check could still run. A consumer must not assume: - # "no code in the bundle" and "no code was wanted" need to be distinguishable. - # Derived from what the archive actually holds, not from what was asked for: a tree whose - # every file was excluded packs nothing, and this must not then claim otherwise while - # metadata.json says `present: false`. - "source_packed": bool(manifest.get("files")), - "source_skipped_reason": source_skipped, - } + result = report.result_document( + "platform", + status, + policy_results, + wfrun_id=run_id, + wfrun_url=run_url, + monthly_cost=(cost_breakdown or {}).get("totalMonthlyCost"), + archive_key=key, + source_packed=bool(manifest.get("files")), + source_skipped_reason=source_skipped, + ) write_output_json(opts.output_json, result) @@ -708,11 +746,7 @@ def run_check(opts): cost_breakdown=cost_breakdown, commit=opts.sha, ) - try: - with open(opts.output_markdown, "w") as f: - f.write(body) - except OSError as e: - log(f"WARNING: could not write {opts.output_markdown}: {e}") + write_output_markdown(opts.output_markdown, body) log(result["headline"]) return result diff --git a/src/tirith/platform/cli.py b/src/tirith/platform/cli.py index c4070b3e..60f4496c 100644 --- a/src/tirith/platform/cli.py +++ b/src/tirith/platform/cli.py @@ -14,7 +14,7 @@ from ..status import ExitStatus from . import discover, regions -from .check import DEFAULT_WORKFLOW_GROUP, INPUT_KINDS, CheckError, log, run_check +from .check import DEFAULT_WORKFLOW_GROUP, INPUT_KINDS, CheckError, log, run_check, write_failure_report # `Id` is a DRF SlugField on the platform, and the value is interpolated into every API path. WORKFLOW_ID_PATTERN = re.compile(r"^[A-Za-z0-9_-]{1,100}$") @@ -259,8 +259,10 @@ def main(argv): result = run_check(opts) except CheckError as e: # Fails closed: a run that produced no verdict must never look like a pass, whatever - # --fail-on-error says. + # --fail-on-error says. Both output files are still written, so a caller editing a sticky + # comment in place has a marker-first body to write and does not orphan the comment. log(f"ERROR: {e}") + write_failure_report(opts, str(e)) return ExitStatus.ERROR except KeyboardInterrupt: log("Interrupted") diff --git a/src/tirith/platform/report.py b/src/tirith/platform/report.py index ffb15cd5..e937570a 100644 --- a/src/tirith/platform/report.py +++ b/src/tirith/platform/report.py @@ -255,8 +255,94 @@ def render_cost(breakdown): return ["", f"{line}"] +def result_document( + mode, + status, + policy_results, + wfrun_id=None, + wfrun_url=None, + monthly_cost=None, + archive_key=None, + source_packed=False, + source_skipped_reason=None, + policies_evaluated=None, + policies_errored=None, + policy_path=None, + policy_errors=None, + policy_warnings=None, +): + """ + The machine-readable result document, for --output-json. + + Built here, for both modes, so the two cannot drift. Every key is present in every mode: a key + with no meaning on one path is None (or [] for the lists), never omitted and never invented. + Omitting them would force every consumer -- the GitHub Action, the GitLab component -- to write + two parsers, and inventing a `wfrun_url` in local mode would point at a run that does not exist. + + `mode` is the discriminator: "platform" or "local". Consumers should read it from here rather + than infer it from which keys are populated. + """ + counts, _findings = summarize(policy_results) + verdict_value = verdict(counts, status) + + return { + "mode": mode, + "status": status, + "verdict": verdict_value, + "counts": { + "passed": counts.get(PASS, 0), + "failed": counts.get(FAIL, 0), + "warned": counts.get(WARN, 0), + "approval_required": counts.get(APPROVAL_REQUIRED, 0), + "skipped": counts.get("SKIPPED", 0), + # Published so a consumer can tell "nothing failed" from "we could not read part of + # it". Without it an errored run reported failed: 0, which the action copies straight + # to its `failed` output. + "unknown": counts.get(UNKNOWN, 0), + }, + "headline": headline(counts, verdict_value), + "policy_results": policy_results or {}, + # Platform mode only: nothing is recorded on the platform in local mode. + "wfrun_id": wfrun_id, + "wfrun_url": wfrun_url, + # Surfaced for a caller aggregating several units into one report of their own. Cost needs a + # second document, which local mode does not evaluate, so it is None there. + "monthly_cost": monthly_cost, + # Where the evaluated source lives. The autofix system reads this to fetch what produced + # the findings; it is also recorded on the run itself as SGCustomWorkflowRunFacts, so a + # consumer holding only a run id can find it without seeing this document. + "archive_key": archive_key, + # Whether that archive actually contains the source. Normally true in platform mode, and + # false when the tree was too large and got dropped so the check could still run. A consumer + # must not assume: "no code in the bundle" and "no code was wanted" need to be + # distinguishable. Derived from what the archive actually holds, not from what was asked + # for: a tree whose every file was excluded packs nothing, and this must not then claim + # otherwise while metadata.json says `present: false`. Always false in local mode, which + # packs nothing at all. + "source_packed": source_packed, + # Left None rather than a sentinel string in local mode. A consumer's "the source was not + # uploaded" warning keys on truthiness, so a value like "not_applicable" would fire it on + # every local run. + "source_skipped_reason": source_skipped_reason, + # Local mode only. + "policies_evaluated": policies_evaluated, + "policies_errored": policies_errored, + "policy_path": policy_path, + # Structured rather than log-only, so a front end can annotate without scraping stderr. + "policy_errors": policy_errors or [], + "policy_warnings": policy_warnings or [], + } + + def render_markdown( - policy_results, run_status, run_url, marker=None, limit=COMMENT_LIMIT, cost_breakdown=None, commit=None + policy_results, + run_status, + run_url, + marker=None, + limit=COMMENT_LIMIT, + cost_breakdown=None, + commit=None, + notes=None, ): """ Render the results as markdown, truncating detail before the summary table. @@ -269,6 +355,11 @@ def render_markdown( place* across runs: without it a reader has no way to tell whether the verdict they are looking at is about the head of the branch or about a push from an hour ago. Rendered here rather than appended by the caller so the check-run summary and the job summary carry it too. + + `notes` are lines rendered under the header, in place of the generic narrative, when a caller + knows *why* there is no verdict. Without it a local evaluation that could not read its input + reported "the workflow run finished as ERRORED", which is untrue -- local mode has no run -- and + told the reader nothing about the actual cause. """ counts, findings = summarize(policy_results) verdict_value = verdict(counts, run_status) @@ -285,7 +376,12 @@ def render_markdown( # a run that produced NOTHING, and a run whose results included one this tool cannot read. # The second renders a populated table, under which "without producing policy results" # reads as a plain contradiction. - if counts.get(UNKNOWN): + # + # A caller that knows the real reason says so instead. Neither generic narrative fits a + # local evaluation, which has no workflow run to report the status of. + if notes: + header += [_html(str(note)) for note in notes] + [""] + elif counts.get(UNKNOWN): header += [ f"{counts[UNKNOWN]} policy result(s) could not be read, so this run has no verdict.", "This is reported as a failure rather than a pass: partial results are not a clean bill of health.", diff --git a/tests/cli/test_dispatch.py b/tests/cli/test_dispatch.py index b0dc8ab0..71a2b2fd 100644 --- a/tests/cli/test_dispatch.py +++ b/tests/cli/test_dispatch.py @@ -103,6 +103,10 @@ def test_the_subcommand_names_are_exactly_these(capsys): `ui` was added alongside it later, on the same terms: dispatched before the flat parser so the local surface and its golden-file output are untouched. The set is pinned rather than merely checked for membership, so a new subcommand has to be a deliberate edit here. + + A `local` subcommand was written and then removed: evaluating committed policy files belongs on the + flat surface that already does it, as flags, rather than as a second way to do the same thing. That + is why `-policy-path` accepts a directory and takes `--output-json`. """ assert cli.SUBCOMMANDS == {"platform", "ui"} diff --git a/tests/local/conftest.py b/tests/local/conftest.py new file mode 100644 index 00000000..624dcc54 --- /dev/null +++ b/tests/local/conftest.py @@ -0,0 +1,136 @@ +"""Fixtures shared by the local-mode tests.""" + +import json +import os +import sys + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), "src")) + +# A policy that fails on the plan below, and passes when the instance type is t3.micro. +POLICY = { + "meta": { + "id": "instance-type", + "name": "instance types are approved", + "required_provider": "stackguardian/terraform_plan", + "version": "v1", + }, + "evaluators": [ + { + "id": "ev", + "description": "instance_type must be t3.micro", + "condition": {"type": "Equals", "value": "t3.micro", "error_tolerance": 0}, + "provider_args": { + "operation_type": "attribute", + "terraform_resource_attribute": "instance_type", + "terraform_resource_type": "aws_instance", + }, + } + ], + "eval_expression": "ev", +} + + +def plan(instance_type="m5.24xlarge", secret=None): + """ + A one-resource terraform plan. + + `secret` lands in `after` *and* is named in `after_sensitive`, which is the shape masking exists + for -- a value terraform itself marked sensitive. + """ + after = {"instance_type": instance_type} + after_sensitive = {} + if secret is not None: + after["password"] = secret + after_sensitive["password"] = True + return { + "format_version": "1.2", + "terraform_version": "1.5.7", + "resource_changes": [ + { + "address": "aws_instance.app", + "mode": "managed", + "type": "aws_instance", + "name": "app", + "change": { + "actions": ["create"], + "before": None, + "after": after, + "after_sensitive": after_sensitive, + }, + } + ], + } + + +@pytest.fixture +def workspace(tmp_path): + """A directory holding one policy and one plan, with helpers to vary either.""" + + class Workspace: + def __init__(self, root): + self.root = root + self.policies = root / "policies" + self.policies.mkdir() + + def policy(self, name="instance-type.tirith.json", document=None): + path = self.policies / name + path.write_text(json.dumps(POLICY if document is None else document)) + return path + + def raw(self, name, text): + path = self.policies / name + path.write_text(text) + return path + + def plan(self, name="plan.json", **kwargs): + path = self.root / name + path.write_text(json.dumps(plan(**kwargs))) + return path + + def argv(self, *extra): + """ + The flat command, with the reporting flags a CI front end passes. + + Note `-policy-path` with one dash and `--output-json` with two: that asymmetry is the flat + surface's, inherited rather than chosen, and a caller has to get it right. + """ + return [ + "-policy-path", + str(self.policies), + "-input-path", + str(self.root / "plan.json"), + "--input-kind", + "terraform_plan", + "--output-json", + str(self.root / "out.json"), + "--output-markdown", + str(self.root / "out.md"), + ] + list(extra) + + def bare_argv(self, *extra): + """The same evaluation with none of the reporting flags -- the original code path.""" + return [ + "-policy-path", + str(self.policies), + "-input-path", + str(self.root / "plan.json"), + ] + list(extra) + + def result(self): + with open(self.root / "out.json") as f: + return json.load(f) + + def markdown(self): + with open(self.root / "out.md") as f: + return f.read() + + return Workspace(tmp_path) + + +def policy_with_enforcement(value): + """The shared policy, relabelled with a `meta.enforcement` value.""" + document = json.loads(json.dumps(POLICY)) + document["meta"]["enforcement"] = value + return document diff --git a/tests/local/test_local_cli.py b/tests/local/test_local_cli.py new file mode 100644 index 00000000..81257d20 --- /dev/null +++ b/tests/local/test_local_cli.py @@ -0,0 +1,225 @@ +""" +The aggregate path behind `tirith -policy-path ...`, end to end through the real CLI. + +The exit-code matrix is the point of this file. This is a *gate*, and the only thing that gates is the +exit code, so every branch of it is pinned here rather than inferred from the result document. + +The routing is pinned too: a single policy file with none of the reporting flags must keep reaching the +original code path, because its `--json` stdout is a frozen contract. +""" + +import json + +from conftest import policy_with_enforcement + +from tirith.cli import main +from tirith.status import ExitStatus + + +def test_a_failing_policy_exits_zero_by_default(workspace): + """ + Matches `platform check`: the report appears, the job stays green. Useful while someone is + writing their first policies, and the default a caller can rely on not changing. + """ + workspace.policy() + workspace.plan() + + assert main(workspace.argv()) == ExitStatus.SUCCESS + assert workspace.result()["verdict"] == "failed" + + +def test_a_failing_policy_exits_three_with_fail_on_error(workspace): + workspace.policy() + workspace.plan() + + assert main(workspace.argv("--fail-on-error")) == ExitStatus.ERROR_POLICY_FAILED + + +def test_a_passing_policy_exits_zero(workspace): + workspace.policy() + workspace.plan(instance_type="t3.micro") + + assert main(workspace.argv("--fail-on-error")) == ExitStatus.SUCCESS + assert workspace.result()["verdict"] == "passed" + + +def test_no_policies_is_a_failure_not_a_skip(workspace): + """ + The one outcome this mode must never produce is a green result for a change nothing was + evaluated against. Pointed at an empty directory, it fails -- and it fails whether or not + --fail-on-error was passed, because this is a configuration error, not a policy decision. + """ + workspace.plan() + + assert main(workspace.argv()) == ExitStatus.ERROR + assert main(workspace.argv("--fail-on-error")) == ExitStatus.ERROR + assert workspace.result()["verdict"] == "errored" + + +def test_a_policy_that_cannot_be_evaluated_fails_regardless_of_fail_on_error(workspace): + """ + "Could not evaluate" is tool health, not a policy decision, so it ignores --fail-on-error + exactly as an unreachable platform does in the other mode. Without this a broken policy file + reported a green job. + """ + workspace.policy(name="broken.tirith.json", document={"meta": {}, "evaluators": [{"nonsense": True}]}) + workspace.plan() + + assert main(workspace.argv()) == ExitStatus.ERROR + + result = workspace.result() + assert result["policies_errored"] == 1 + assert result["policy_errors"][0]["policy"].endswith("broken.tirith.json") + assert result["policy_errors"][0]["reason"] + + +def test_a_missing_input_document_fails_and_still_writes_a_report(workspace): + """ + Both output files are written on every exit path. A front end that edits a sticky comment in + place needs a marker-first body even here -- writing nothing is what orphaned the comment it was + updating. + """ + workspace.policy() + marker = "[//]: <> (tirith-comment, tag=default)" + + status = main(workspace.argv("--comment-marker", marker)) + + assert status == ExitStatus.ERROR + assert workspace.result()["verdict"] == "errored" + assert workspace.markdown().startswith(marker) + + +def test_the_failure_report_says_why_rather_than_naming_a_workflow_run(workspace): + """ + The renderer's generic errored narrative talks about "the workflow run", which local mode does + not have. The real reason is passed through instead. + """ + workspace.plan() + + main(workspace.argv()) + body = workspace.markdown() + + assert "workflow run" not in body + assert "no policy files found" in body + + +def test_the_marker_is_the_first_line_of_a_normal_report(workspace): + """Stickiness depends on it: a caller finds its own note by matching the marker as a prefix.""" + workspace.policy() + workspace.plan() + marker = "[//]: <> (tirith-comment, tag=envs-prod)" + + main(workspace.argv("--comment-marker", marker)) + + assert workspace.markdown().startswith(marker) + + +def test_an_advisory_policy_warns_instead_of_failing(workspace): + """`meta.enforcement` downgrades a failing policy, matching the platform's onFail handling.""" + workspace.policy(document=policy_with_enforcement("soft_mandatory")) + workspace.plan() + + assert main(workspace.argv("--fail-on-error")) == ExitStatus.SUCCESS + assert workspace.result()["verdict"] == "warned" + + +def test_an_unrecognised_enforcement_gates_and_says_so(workspace): + """ + An unlabelled or mislabelled policy must gate, not slip through -- but the reader has to be told, + or a typo in `enforcement` silently becomes policy. The notice is in the document as well as the + log so a front end can annotate without scraping stderr. + """ + workspace.policy(document=policy_with_enforcement("sort-of-important")) + workspace.plan() + + assert main(workspace.argv("--fail-on-error")) == ExitStatus.ERROR_POLICY_FAILED + + warnings = workspace.result()["policy_warnings"] + assert len(warnings) == 1 + assert "sort-of-important" in warnings[0] + + +def test_a_correctly_labelled_blocking_policy_raises_no_warning(workspace): + """ + `hard_mandatory` is what tirith's own golden test pins, so treating it as unrecognised fired the + notice on essentially every failing run. + """ + workspace.policy(document=policy_with_enforcement("hard_mandatory")) + workspace.plan() + + main(workspace.argv()) + + assert workspace.result()["policy_warnings"] == [] + + +def test_the_masked_document_is_what_gets_evaluated(workspace): + """ + Masking matters even though nothing is uploaded: evaluator messages embed the values they + compared, and those messages are copied verbatim into whatever note the caller posts. An + unmasked local run publishes plan values to a code host. + """ + workspace.policy() + workspace.plan(secret="hunter2-should-never-appear") + + main(workspace.argv()) + + assert "hunter2-should-never-appear" not in json.dumps(workspace.result()) + assert "hunter2-should-never-appear" not in workspace.markdown() + + +def test_without_input_kind_the_document_is_not_masked(workspace): + """ + No --input-kind means "read the file as this command always has". Masking is opt-in because it + changes the evaluator messages, and those messages are the frozen --json output. + """ + workspace.policy() + workspace.plan(secret="hunter2-visible-without-input-kind") + + main(workspace.bare_argv("--output-json", str(workspace.root / "out.json"))) + + assert "hunter2-visible-without-input-kind" not in workspace.result()["headline"] + # And with it, the value is masked out of the document that was evaluated. + main(workspace.argv()) + assert "hunter2-visible-without-input-kind" not in json.dumps(workspace.result()) + + +def test_a_single_policy_file_with_no_reporting_flags_takes_the_original_path(workspace): + """ + The routing guard. That path prints the engine's own document to stdout, pinned byte-for-byte by + tests/core/test_output_compatibility.py, so it must not be reached by the aggregate code. + """ + from tirith.local import check as local_check + + policy = workspace.policy() + workspace.plan() + + class Args: + policyPath = str(policy) + inputKind = None + statePath = None + sha = None + outputJson = None + outputMarkdown = None + commentMarker = None + + assert local_check.wanted(Args) is False + + # One reporting flag is enough to switch, and so is a directory. + Args.outputJson = "out.json" + assert local_check.wanted(Args) is True + + Args.outputJson = None + Args.policyPath = str(workspace.policies) + assert local_check.wanted(Args) is True + + +def test_a_directory_of_policies_is_evaluated_rather_than_erroring(workspace): + """ + `-policy-path` used to reach `open()` on a directory and fail with a bare "ERROR". Evaluating every + policy in it is new capability, not changed behaviour. + """ + workspace.policy(name="one.tirith.json") + workspace.policy(name="two.tirith.json") + workspace.plan() + + assert main(workspace.bare_argv()) == ExitStatus.SUCCESS diff --git a/tests/local/test_local_evaluate.py b/tests/local/test_local_evaluate.py new file mode 100644 index 00000000..782fd879 --- /dev/null +++ b/tests/local/test_local_evaluate.py @@ -0,0 +1,207 @@ +""" +Unit coverage for policy discovery, input preparation and the engine subprocess. + +The pinned-argv test near the bottom is the load-bearing one: it is what keeps the frozen `--json` +contract as the interface between this package and the engine, now that both live in the same +distribution and calling the engine's Python API directly has become easy. +""" + +import json +import sys + +import pytest +from conftest import POLICY, plan + +from tirith.local import evaluate +from tirith.local.evaluate import LocalError +from tirith.platform import report + + +def test_a_named_file_is_taken_as_given(tmp_path): + """ + Naming a file explicitly is an instruction. Reporting "that is not a policy" is more useful than + silently evaluating nothing. + """ + path = tmp_path / "not-really.json" + path.write_text("{}") + + assert evaluate.discover_policies(str(path)) == [str(path)] + + +def test_a_directory_prefers_the_tirith_suffix(tmp_path): + (tmp_path / "a.tirith.json").write_text(json.dumps(POLICY)) + (tmp_path / "b.json").write_text(json.dumps(POLICY)) + + found = evaluate.discover_policies(str(tmp_path)) + + assert [p.rsplit("/", 1)[-1] for p in found] == ["a.tirith.json"] + + +def test_a_directory_without_the_suffix_falls_back_to_shape(tmp_path): + (tmp_path / "policy.json").write_text(json.dumps(POLICY)) + + found = evaluate.discover_policies(str(tmp_path)) + + assert [p.rsplit("/", 1)[-1] for p in found] == ["policy.json"] + + +def test_the_input_document_sitting_in_the_policy_directory_is_not_evaluated_as_a_policy(tmp_path): + """ + Load-bearing rather than defensive. A policy directory routinely also holds the document under + evaluation; without the shape filter the plan is evaluated *as a policy*, which reports a + spurious failure and buries the real findings. + """ + (tmp_path / "policy.json").write_text(json.dumps(POLICY)) + (tmp_path / "plan.json").write_text(json.dumps(plan())) + + found = evaluate.discover_policies(str(tmp_path)) + + assert [p.rsplit("/", 1)[-1] for p in found] == ["policy.json"] + + +def test_a_glob_is_filtered_by_shape_too(tmp_path): + (tmp_path / "policy.json").write_text(json.dumps(POLICY)) + (tmp_path / "plan.json").write_text(json.dumps(plan())) + + found = evaluate.discover_policies(str(tmp_path / "*.json")) + + assert [p.rsplit("/", 1)[-1] for p in found] == ["policy.json"] + + +def test_an_empty_policy_path_finds_nothing(tmp_path): + assert evaluate.discover_policies("") == [] + assert evaluate.discover_policies(str(tmp_path / "nowhere")) == [] + + +def test_a_terraform_state_check_evaluates_the_state_not_a_discovered_plan(tmp_path): + """ + `--state-path` is how the platform path names the document for a terraform_state check. Ignoring + it sent local mode to discovery, which finds plan.json -- so the two modes evaluated *different + documents* from identical inputs, and a violation present only in the state was reported as a + pass. + """ + (tmp_path / "plan.json").write_text(json.dumps(plan())) + state = tmp_path / "state.json" + state.write_text( + json.dumps({"values": {"root_module": {"resources": [{"address": "aws_instance.only-in-state"}]}}}) + ) + + resolved, _ = evaluate.prepare_input( + None, None, None, "terraform_state", str(tmp_path), str(tmp_path), state_path=str(state) + ) + + with open(resolved) as f: + assert "only-in-state" in json.dumps(json.load(f)) + + +def test_a_json_document_is_passed_through_unmasked(tmp_path): + """ + `json` and `kubernetes` carry no sensitivity markers to mask by, and tirith reads YAML for them, + which a JSON round-trip here would break. So the original path is returned, not a copy. + + `raw` -- no --input-kind at all, which is what the flat command has always done -- takes the same + branch, for the different reason that the caller has not said what the document is. + """ + document = tmp_path / "anything.json" + document.write_text(json.dumps({"hello": "world"})) + + resolved, redactions = evaluate.prepare_input(str(document), None, None, "json", str(tmp_path), str(tmp_path)) + + assert resolved == str(document) + assert redactions == 0 + + +def test_a_json_document_must_be_named(tmp_path): + with pytest.raises(LocalError, match="-input-path is required"): + evaluate.prepare_input(None, None, None, "json", str(tmp_path), str(tmp_path)) + + +def test_input_path_and_plan_file_cannot_be_combined(tmp_path): + with pytest.raises(LocalError, match="cannot be combined"): + evaluate.prepare_input("a.json", "tfplan", None, "terraform_plan", str(tmp_path), str(tmp_path)) + + +def test_a_missing_document_raises_rather_than_evaluating_nothing(tmp_path): + with pytest.raises(LocalError, match="not found"): + evaluate.prepare_input(str(tmp_path / "gone.json"), None, None, "terraform_plan", str(tmp_path), str(tmp_path)) + + +def test_a_sensitive_value_is_masked_and_counted(tmp_path): + document = tmp_path / "plan.json" + document.write_text(json.dumps(plan(secret="hunter2"))) + + resolved, redactions = evaluate.prepare_input( + str(document), None, None, "terraform_plan", str(tmp_path), str(tmp_path) + ) + + assert redactions >= 1 + with open(resolved) as f: + assert "hunter2" not in f.read() + + +def test_the_engine_is_invoked_as_a_subprocess_against_the_frozen_json_contract(): + """ + Pinned deliberately. That stdout document is the most stable interface tirith has -- it is + byte-for-byte pinned by tests/core/test_output_compatibility.py -- whereas the engine's Python + API carries no such promise. Now that this code lives inside the package, calling + `start_policy_evaluation` directly would save an interpreter start per policy and silently couple + local mode to internals. This makes that a deliberate change with a red test, not a tidy-up. + + `sys.executable -m tirith` rather than a `tirith` on PATH: the interpreter that evaluates must be + the one whose renderer was imported, and a `tirith` on PATH could be a different installation. + """ + assert evaluate.engine_argv("p.json", "i.json") == [ + sys.executable, + "-m", + "tirith", + "-policy-path", + "p.json", + "-input-path", + "i.json", + ] + + +def test_an_unevaluable_policy_becomes_a_visible_fail_carrying_its_reason(tmp_path): + """ + Not a skip and not a silent drop: the renderer surfaces `exec_err` as `engine: `, so it + appears in the report, is distinguishable from a real violation, and is never mistakable for a + pass. + """ + broken = tmp_path / "broken.tirith.json" + broken.write_text(json.dumps({"meta": {}, "evaluators": [{"nonsense": True}]})) + document = tmp_path / "plan.json" + document.write_text(json.dumps(plan())) + + policy_results, errored = evaluate.evaluate([str(broken)], str(document)) + + assert len(errored) == 1 + rule = policy_results["broken"][0] + assert rule["result"] == report.FAIL + assert "exec_err" in rule["evaluations"]["fails"][0] + + +def test_the_enforcement_matrix(tmp_path): + """Every recognised spelling, plus the unrecognised case, in one place.""" + document = tmp_path / "plan.json" + document.write_text(json.dumps(plan())) + + for value, expected, expect_notice in ( + ("soft_mandatory", report.WARN, False), + ("advisory", report.WARN, False), + ("approval_required", report.WARN, False), + ("hard_mandatory", report.FAIL, False), + ("blocking", report.FAIL, False), + (None, report.FAIL, False), + ("who-knows", report.FAIL, True), + ): + policy = json.loads(json.dumps(POLICY)) + if value is not None: + policy["meta"]["enforcement"] = value + path = tmp_path / "p.tirith.json" + path.write_text(json.dumps(policy)) + + notices = [] + policy_results, _ = evaluate.evaluate([str(path)], str(document), on_unknown_enforcement=notices.append) + + assert policy_results["instance-type"][0]["result"] == expected, value + assert bool(notices) is expect_notice, value diff --git a/tests/local/test_output_contract.py b/tests/local/test_output_contract.py new file mode 100644 index 00000000..189c80c6 --- /dev/null +++ b/tests/local/test_output_contract.py @@ -0,0 +1,213 @@ +""" +The two modes produce one document shape. + +Both front ends -- the GitHub Action and the GitLab CI component -- read `--output-json` and turn it +into comments, statuses and job outputs. If the key set differs by mode, every consumer has to write +two parsers, and the one that forgets reports the wrong thing on the mode it was not tested against. + +So the contract is: **every key exists in every mode.** A key with no meaning on a path is None (or +[] for the lists), never omitted and never invented. That is enforced by construction -- +`report.result_document` builds both -- and pinned here, because construction is only a guarantee +while both callers keep using it. +""" + +import json + +from conftest import POLICY, plan + +from tirith.cli import main +from tirith.platform import report +from tirith.status import ExitStatus + +# Keys read by the GitHub Action and the GitLab component. Adding one is fine; removing or renaming +# one breaks a released consumer, which is what this list is for. +CONSUMER_KEYS = { + "mode", + "status", + "verdict", + "counts", + "headline", + "policy_results", + "wfrun_id", + "wfrun_url", + "monthly_cost", + "archive_key", + "source_packed", + "source_skipped_reason", + "policies_evaluated", + "policies_errored", + "policy_path", + "policy_errors", + "policy_warnings", +} + +COUNT_KEYS = {"passed", "failed", "warned", "approval_required", "skipped", "unknown"} + + +def _platform_document(): + """What a platform run produces, without needing a platform.""" + return report.result_document( + "platform", + "COMPLETED", + {"some-policy": [{"rule_name": "a rule", "result": report.PASS}]}, + wfrun_id="abc123", + wfrun_url="https://app.stackguardian.io/...", + monthly_cost=12.5, + archive_key="orgs/acme/wfs/K/artifacts/bundle.tar.gz", + source_packed=True, + ) + + +def _local_document(workspace): + workspace.policy() + workspace.plan() + main(workspace.argv()) + return workspace.result() + + +def test_both_modes_carry_the_same_keys(workspace): + platform = _platform_document() + local = _local_document(workspace) + + assert set(platform) == set(local) + assert set(local) == CONSUMER_KEYS + assert set(platform["counts"]) == set(local["counts"]) == COUNT_KEYS + + +def test_the_mode_discriminator_is_the_mode(workspace): + """ + Consumers should read `mode` rather than infer it from which keys are populated -- inference is + what makes a new key a breaking change. + """ + assert _platform_document()["mode"] == "platform" + assert _local_document(workspace)["mode"] == "local" + + +def test_local_mode_invents_no_run(workspace): + """ + A fabricated wfrun_url would render as a link to a run that does not exist. None is the honest + value, and consumers already guard on it. + """ + local = _local_document(workspace) + + assert local["wfrun_id"] is None + assert local["wfrun_url"] is None + assert local["archive_key"] is None + assert local["monthly_cost"] is None + + +def test_local_mode_reports_no_source_without_claiming_a_reason(workspace): + """ + `source_skipped_reason` stays None rather than a sentinel like "not_applicable". A consumer's + "the terraform source was not uploaded" warning keys on truthiness, so a sentinel would fire it + on every local run -- about an upload local mode never attempts. + """ + local = _local_document(workspace) + + assert local["source_packed"] is False + assert local["source_skipped_reason"] is None + + +def test_platform_mode_leaves_the_local_only_keys_empty(workspace): + platform = _platform_document() + + assert platform["policies_evaluated"] is None + assert platform["policies_errored"] is None + assert platform["policy_path"] is None + assert platform["policy_errors"] == [] + assert platform["policy_warnings"] == [] + + +def test_the_document_is_json_serialisable_in_both_modes(workspace): + """ + It is written with json.dump. A tuple or a set slipping into `policy_errors` would fail at the + end of a long run, having already done the work. + """ + json.dumps(_platform_document()) + json.dumps(_local_document(workspace)) + + +def test_a_run_of_nothing_but_skips_fails_rather_than_passing(workspace): + """ + The flat surface has always treated `final_result is None` -- every check swallowed by its + error_tolerance -- as a failure rather than a pass: nothing was examined, so green would be a lie. + Extending it to many policies must not quietly reverse that, which is what would have happened had + the aggregate verdict been taken from `report.verdict` alone. + + This is a deliberate difference from `platform check`, which counts skips separately and reports + them as a pass. Both readings are defensible; a single surface disagreeing with itself depending on + how many policies you pointed it at is not. + """ + # A policy naming a resource type this document does not contain, with the error_tolerance that + # turns "not found" into a skip rather than a failure. + document = json.loads(json.dumps(POLICY)) + document["evaluators"][0]["provider_args"]["terraform_resource_type"] = "aws_nonexistent" + document["evaluators"][0]["condition"]["error_tolerance"] = 2 + workspace.policy(document=document) + workspace.plan() + + assert main(workspace.argv()) == ExitStatus.ERROR + + result = workspace.result() + assert result["verdict"] == "errored" + assert result["counts"]["skipped"] == 1 + assert "nothing was evaluated" in workspace.markdown() + + # The reducer itself still answers `passed` for an all-skipped set -- that is platform mode's + # answer and it is not changed here. The difference is made by this path, deliberately. + counts, _ = report.summarize({"p": [{"rule_name": "r", "skip": True}]}) + assert report.verdict(counts, "COMPLETED") == "passed" + + +def test_the_platform_failure_report_is_a_full_document_with_the_reason(tmp_path): + """ + `platform check` used to return from its error paths having written nothing, so a caller editing a + sticky comment in place fell through to a marker-less body of its own -- which, PATCHed over a + good comment, orphaned it permanently. Both files are now written on that path too, in the same + shape as every other document, carrying the reason rather than a generic narrative about a + workflow run. + """ + from tirith.platform import check + + class Opts: + output_json = str(tmp_path / "out.json") + output_markdown = str(tmp_path / "out.md") + comment_marker = "[//]: <> (tirith-comment, tag=default)" + markdown_limit = 60000 + sha = None + + result = check.write_failure_report(Opts, "the API could not be reached") + + assert set(result) == CONSUMER_KEYS + assert result["mode"] == "platform" + assert result["verdict"] == "errored" + assert result["policy_errors"] == [{"policy": None, "reason": "the API could not be reached"}] + + with open(Opts.output_markdown) as f: + body = f.read() + assert body.startswith(Opts.comment_marker) + assert "the API could not be reached" in body + assert "workflow run finished" not in body + + +def test_the_platform_failure_report_keeps_a_run_it_already_recorded(tmp_path): + """ + The pre-poll write records the run id precisely so a timeout leaves the run discoverable. + Overwriting it with nulls on the way out would throw away the only link to the run that timed out. + """ + from tirith.platform import check + + class Opts: + output_json = str(tmp_path / "out.json") + output_markdown = None + comment_marker = None + markdown_limit = 60000 + sha = None + + with open(Opts.output_json, "w") as f: + json.dump({"wfrun_id": "wfrun-1", "wfrun_url": "https://app.stackguardian.io/run/1"}, f) + + result = check.write_failure_report(Opts, "timed out waiting for the run") + + assert result["wfrun_id"] == "wfrun-1" + assert result["wfrun_url"] == "https://app.stackguardian.io/run/1" diff --git a/tests/test_readme_is_current.py b/tests/test_readme_is_current.py index 7a02da86..e366cacb 100644 --- a/tests/test_readme_is_current.py +++ b/tests/test_readme_is_current.py @@ -117,6 +117,50 @@ def test_the_flag_reference_page_lists_every_flag_the_command_accepts(): assert not missing, f"flags accepted by `platform check` but absent from docs/platform-check.md: {missing}" +def test_the_reporting_flags_are_documented(): + """ + The flags that let a CI job drive the flat command. They are the difference between "there is a + local surface" and "the local surface is usable as a gate", and they went undocumented once already + in the form of a subcommand nobody could find. + """ + text = _readme() + reporting = set(re.findall(r"(?