From 3b3a700f9eb799425bdf7c08eeaebd4e6dd48628 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Thu, 10 Sep 2026 00:37:27 +0900 Subject: [PATCH] Four scanners could not say they had not looked, and guarddog could not say it had found something `tool()` mapped every non-zero exit to a verdict, which files "I could not run" under "you are out of step". Running all five scanners to check that assumption found four of them breaking it, and one breaking it the other way. guarddog is the serious one. `verify` exits 0 whether it found three high-severity risks or none, in both ecosystems, so every guarddog finding in this fleet was reported clean. That is a false negative on the one scanner here whose subject is malware and typosquats. `--exit-non-zero-on-finding` is not the remedy: it counts `issues`, which includes capability matches, and `six` reports `issues: 2` with `risks: []` under guarddog's own label `no_risks_detected`, so the flag fails a package guarddog itself calls clean. guarddog now runs with `--output-format json` and its own `risks` list is counted. This is the one scanner whose findings are read here, against the rule the module header otherwise holds, and the module header now says so: the alternative was running it for nothing. The count is reported, never recomputed. The other three, all verified by running them rather than by reading docs: - cargo-vet answers 255 both for an unvetted dependency and for a store it could not open. A run that reported prints on stdout; one that could not start leaves stdout empty and puts its diagnostic on stderr. - cargo-deny returns a bitmask, 1 advisories, 2 bans, 4 licenses, 8 sources, so a matched RUSTSEC advisory and an advisory database it could not fetch share the 1. Same stdout test. - osv-scanner separates them itself at 127 and 128, and both were being read as refusals. Only 1 is a finding. - zizmor answers 11 through 14 by highest severity, so any other non-zero code audited nothing. Its dangerous case is exit 0: handed one unparseable workflow among good ones it skips that one, audits the rest and reports no findings, and its SARIF asserts executionSuccessful true. The stderr line is the only witness, and this section hands zizmor a list of files. `tool_read()` hands each scanner its exit code, stdout and stderr and takes a reader that may name a could-not-look. The reader is consulted before the zero is believed, because two of these failures wear exit 0. Eleven tests, each with its control. Every reader was reverted in turn to confirm the test that covers it fails without it: three fail without the guarddog risks read, four without the other readers, and the controls holding that a genuine finding stays a finding pass in both directions. Seven existing tests had stubs answering a bare `exit 0` for guarddog. That stopped modelling the tool once the invocation gained `--output-format json`, since a real clean run prints a report whose `risks` list is empty. One of them also grepped `"$3"` for the requirements path, which the new flag shifts to `"$5"`; it would have passed while asserting nothing. ADR 0007 records the orchestrator survey this came out of, and why the command is written here rather than replaced by MegaLinter, trunk, SARIF aggregation or pre-commit. The XCCDF citation moves from r3 to r4 and gains the scoring asymmetry `verdict()` implements. Claude-Session: https://claude.ai/code/session_01YE1DDjVegJr2MZ12TjPW6e --- docs/REFERENCE.md | 70 +++- ...hat-the-surveyed-orchestrators-collapse.md | 124 +++++++ principles/explicit-unknown.toml | 4 +- src/supply.rs | 306 ++++++++++++++++-- tests/supply_chain_cli.rs | 284 +++++++++++++++- 5 files changed, 742 insertions(+), 46 deletions(-) create mode 100644 docs/adr/0007-what-the-surveyed-orchestrators-collapse.md diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index f0eda94..74f718e 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -2156,14 +2156,68 @@ rather than a widening: there is no tree to widen into, and reporting a clean scan of manifests that are not on disk is the shape this crate exists to refuse. -### guarddog that could not look - -guarddog prints "Some rules failed to run while scanning \" and -**exits 0** — its two email-domain rules time out routinely. Reading only the -exit code files that under clean, so an unrun rule is could-not-look and exit -`2`, with a line naming the packages and how many rules. This is the one place -a scanner's output is read, and it is not a finding: it is the record that the -question was asked and nobody answered. +### guarddog, which cannot answer in its exit code at all + +`guarddog verify` **exits 0 whether it found three high-severity risks or +none**, in both ecosystems. For as long as this section answered by exit code, +every guarddog finding was reported clean — a false negative on the one +scanner here whose subject is malware and typosquats. + +`--exit-non-zero-on-finding` is not the remedy. It counts `issues`, which +includes capability matches: `six` reports `issues: 2` with `risks: []` and +guarddog's own label `no_risks_detected`, so the flag fails a package guarddog +itself calls clean. It trades a false negative for a false positive. + +So guarddog is run with `--output-format json` and its own `risks` list is +counted. This is the one scanner whose findings are read here, and the choice +is to read them or to run it for nothing. The count is reported, never +recomputed. + +The same report carries the could-not-look. A dependency guarddog could not +download populates `errors` and drops `results` while still exiting 0, and a +total network failure returns a bare `[]`; both are unread rather than clean. +Its older admission, "Some rules failed to run while scanning \" at +exit 0 — the two email-domain rules time out routinely — is still read, and +still names the packages and how many rules. None of that is a finding: it is +the record that the question was asked and nobody answered. + +### The other three that confuse a verdict with a refusal + +`tool_read()` hands each scanner's exit code, stdout and stderr to a reader +that may name a could-not-look. A tool whose exit code already separates the +two passes a reader that never fires. + +- **osv-scanner** separates them itself: `0` clean, `1` a vulnerability, `127` + a path, lockfile, config or query it could not read, `128` inputs that + yielded no package. Only `1` is a finding. +- **zizmor** answers `11` through `14` by highest severity present, so any + other non-zero code audited nothing. Its dangerous case is exit `0`: handed + one unparseable workflow among good ones it skips that one, audits the rest + and reports no findings, and its SARIF asserts `executionSuccessful: true`. + The `failed to parse input:` line on stderr is the only witness, and this + section hands zizmor a list of files, which is exactly that shape. +- **cargo-deny** returns a bitmask — `1` advisories, `2` bans, `4` licenses, + `8` sources — so a matched RUSTSEC advisory and a database it could not + fetch share the `1`. A run that reached its checks prints the per-check + summary on stdout; one that did not leaves stdout empty. + +### cargo-vet and exit 255 + +cargo-vet answers `255` for two different facts. Dependencies that carry no +audit exit `255` and name them, which is a finding. A run that could not happen +at all — no store to read, a store that does not parse, a lockfile `--locked` +refuses — also exits `255`. `tool()` answers by exit code, and every non-zero +code is a verdict, so the could-not-look half currently reports as out of step. +This is the guarddog defect with the codes reversed: guarddog exits `0` on work +it did not do, cargo-vet exits `255` on work it could not start. + +The exit code is the only place the two are confused. They separate cleanly on +the stream: a vetting failure prints `Vetting Failed!` to stdout and leaves +stderr empty, while a run that could not start prints `ERROR` to stderr and +leaves stdout empty. `cargo vet --output-format=json` is the other half. So +this is a section that has not been taught to read its tool yet, the way the +guarddog section was, rather than a fact about cargo-vet that cannot be +recovered. Two hook ids ship it in `.pre-commit-hooks.yaml`, and never at `pre-commit`: every scanner reaches the network, and a check that adds a network round trip diff --git a/docs/adr/0007-what-the-surveyed-orchestrators-collapse.md b/docs/adr/0007-what-the-surveyed-orchestrators-collapse.md new file mode 100644 index 0000000..9daa83f --- /dev/null +++ b/docs/adr/0007-what-the-surveyed-orchestrators-collapse.md @@ -0,0 +1,124 @@ +# ADR 0007: what the surveyed orchestrators collapse + +Status: Accepted + +This record answers whether `uphold supply-chain` should be retired in favour of +an existing orchestrator. It is written after the field was surveyed and the +candidates were run, not from a list of tool names. +[REFERENCE.md](../REFERENCE.md) documents what the command does; this says why +it is written here rather than adopted. + +The question is narrow. Orchestrating scanners is not novel work, and a +hand-written runner is a cost. The survey was run to find the tool that already +does it, and the answer is that the field collapses the one distinction this +command exists to keep. + +## The contract the survey was run against + +A section is one of four states, not two. `Clean` and `Failed` are the verdicts. +`CouldNotLook` is a scanner that did not read its input, and `Nothing` is a +scanner correctly given no input to read -- no lockfile moved in the range, no +vet store to consult. The two negative states are not the same fact and neither +is clean. + +`verdict()` ranks them, and it is called from six sites: supply, probe, check, +audit, main and the push guard. A Kani proof holds that could-not-look never +exits `0`. The contract is repo-wide, not local to this command. + +## What the candidates do with could-not-look + +**reviewdog is the only real prior art, and it is about twenty lines.** +`CheckUnexpectedFailure` carries the concept verbatim: a command that failed, or +whose results could not be parsed, is not a clean run. Its guard is that the +command errored *and* produced no findings, so a scanner that reports three +findings and then dies is classed as a verdict. That is the guarddog shape this +crate already refuses, and it is the reason `guarddog` is the one place a +scanner's output is read rather than its exit code. + +**trunk has the best vocabulary and the worst provenance.** Its linter schema +splits success codes from error codes from no-issues codes, and defines the +first as unrelated to whether issues were found, which is a sharper spelling of +the axis here. The orchestrator binary is closed source with no public +repository, auto-updating from a vendor endpoint, and whether it exits non-zero +on a linter's internal failure is undocumented. + +**MegaLinter erases the state rather than collapsing it.** A linter absent from +the container flavour is marked inactive, and every reporter filters on that +flag, so the linter vanishes from the console table, the summary and the JSON, +and the run exits `0`. Its own error table classifies infrastructure failures +correctly as not-a-finding and then discards the classification. + +**pre-commit and prek cannot express it by design.** The per-hook result is a +boolean before it reaches the merge, a missing executable returns the same code +as found-problems, and the skip mechanism sets success. The maintainer's stated +position is that pre-commit does not decode tool output and will not interpret +it. That is a defensible product boundary and it is the boundary this command +sits on the other side of. + +**SARIF has the vocabulary in the standard and cannot carry it in practice.** +Execution success is the only required property on an invocation, and an absent +results array is the specified encoding of did-not-look. Both are defeated: +invocations are optional on a run, and the published schema types results as an +array, so the encoding the specification mandates fails the schema the +specification ships. None of the five scanners here emits the field usefully, +and the aggregators drop it. + +## Coverage decides the same question independently + +No surveyed orchestrator reaches more than two of the five scanners. The other +three would be written as integrations either way, in someone else's +configuration language, feeding a merge layer that discards the distinction. +Adoption costs the same work and loses the property. + +## The idea is older than every implementation of it + +XCCDF standardised nine rule-result values, and its scoring rule excludes +not-applicable and not-checked from the denominator while leaving error and +unknown inside it. An unreadable check counts against the score and never for +it. That asymmetry is what `verdict()` implements. The idea has been specified +since 2012; the implementations surveyed here collapse it. + +## What was deliberately not built + +No structured output. `supply-chain` prints sections and answers by exit code, +and the exit code is the whole machine-readable surface. A consumer that counts +findings from JSON would be able to read a broken scanner as clean unless +could-not-look is reified as an entry it cannot ignore, which is the pattern +golangci-lint uses for the checks it could not run. Adding output without that +entry would reintroduce the defect this record is about. + +No scanner is linked. Every one is a subprocess found on PATH, and a tool that +is absent is could-not-look rather than a build failure. + +## What this changes today + +Nothing in the binary. The command stands as written, and the survey is recorded +so the question is not reopened from the tool names alone. + +## What the survey changed, which was not the orchestration + +Running the candidates meant running the five scanners too, and four of the +five turned out to confuse a verdict with a refusal in their own exit codes. +That is the same defect one layer down, and it was live here. + +`cargo-vet` answers `255` both for an unvetted dependency and for a store it +could not open. `cargo-deny` returns a bitmask whose `1` is either a matched +advisory or an advisory database it could not fetch. `osv-scanner` separates +them itself, at `127` and `128`, and those were read as refusals. `zizmor` +answers by severity at `11` through `14`, and — the case that cannot be seen +from an exit code at all — skips a workflow it cannot parse, audits the rest +and exits `0`, with its SARIF asserting the run succeeded. + +The worst was `guarddog`. `verify` exits `0` whether it found three +high-severity risks or none, so every finding it made was reported clean. That +one is a false negative on a malware scanner, and closing it meant reading a +scanner's findings for the first time, against the rule this command otherwise +holds. The rule survives with one stated exception, because the alternative +was running guarddog for nothing. + +Each scanner now hands its exit code, stdout and stderr to a reader that may +name a could-not-look; a tool whose code already separates the two passes a +reader that never fires. `REFERENCE.md` records the four contracts. + +One judgement rather than a fact, left as a judgement: of the five, +`cargo-vet` is the one whose upstream has gone quietest. diff --git a/principles/explicit-unknown.toml b/principles/explicit-unknown.toml index c2f80f4..cd6a08f 100644 --- a/principles/explicit-unknown.toml +++ b/principles/explicit-unknown.toml @@ -26,9 +26,9 @@ limits = ["A tool can see that an unknown state exists; it cannot tell whether t [[sources]] title = "Specification for the Extensible Configuration Checklist Description Format (XCCDF)" -url = "https://csrc.nist.gov/pubs/ir/7275/r3/final" +url = "https://csrc.nist.gov/pubs/ir/7275/r4/upd1/final" type = "standard" -notes = "Defines rule-result values that keep pass, fail, error, unknown, notapplicable, notchecked, and notselected as separate outcomes rather than a boolean." +notes = "Defines rule-result values that keep pass, fail, error, unknown, notapplicable, notchecked, notselected, informational and fixed as separate outcomes rather than a boolean. Its scoring rule excludes notapplicable and notchecked from the denominator and leaves error and unknown inside it, so a check that could not run counts against the score and never for it." [[sources]] title = "Static Analysis Results Interchange Format (SARIF)" diff --git a/src/supply.rs b/src/supply.rs index a17f7b1..d6136fe 100644 --- a/src/supply.rs +++ b/src/supply.rs @@ -22,13 +22,33 @@ //! one verdict ranking this crate has, so a machine missing a scanner blocks //! exactly as loudly while saying what to install. //! -//! What it deliberately does NOT do: parse any scanner's findings. Each tool's -//! exit code decides, its output is shown when it refuses, and the one filter +//! What it deliberately does NOT do: re-judge any scanner's findings. A tool's +//! verdict is its own, its output is shown when it refuses, and the one filter //! applied (cargo-deny's headline lines) drops classes that describe the //! config rather than a dependency. A wrapper that re-judged findings would be -//! a second opinion nobody asked for, drifting from the tool it wraps. The one -//! thing read out of a scanner's OUTPUT is guarddog's own admission that a rule -//! did not run, which is not a finding and is the opposite of one. +//! a second opinion nobody asked for, drifting from the tool it wraps. +//! +//! ONE SCANNER'S FINDINGS ARE READ ANYWAY, and the exception is guarddog. +//! `guarddog verify` exits 0 whether it found three high-severity risks or +//! none, in both ecosystems, so for as long as this section answered by exit +//! code it called every guarddog finding clean -- a false negative on the one +//! scanner here that looks for malware. Its `--exit-non-zero-on-finding` flag +//! is not the remedy: it counts `issues`, which includes capability matches +//! guarddog itself scores 0.0 and labels `no_risks_detected`. So guarddog is +//! run with `--output-format json` and its own `risks` list is counted. The +//! count is reported, never recomputed; the choice is to parse it or to run it +//! for nothing. +//! +//! What IS read out of a scanner's output is the opposite of a finding: its +//! own admission that it did not look. Four of the five need it, because four +//! of the five cannot say so in their exit code. guarddog reports rules that +//! timed out and still exits 0; cargo-vet answers 255 both for an unvetted +//! dependency and for a store it could not open; cargo-deny's exit 1 is a +//! matched advisory or a database it could not fetch; and zizmor, handed one +//! unparseable workflow among good ones, skips it and exits 0. None of those +//! is a judgement about a dependency being re-judged here. Each is the record +//! that a question went unasked, which is this command's third verdict and the +//! reason it exists. //! //! What the run looks at is a RANGE, not a tree. Every scanner here reaches the //! network, and a push that changes no lockfile, manifest or workflow was @@ -159,11 +179,24 @@ fn on_path(tool: &str) -> Option { } } -/// Run one tool, show what it said when it refused, answer by exit code. +/// Run one tool, show what it said when it refused, and read its answer. /// /// A tool that died on a signal answered nothing, and nothing is could-not- /// look rather than either verdict. -fn tool(root: &Path, program: &str, args: &[&str]) -> Result
{ +/// +/// A NON-ZERO EXIT IS NOT ALWAYS A VERDICT. Some tools answer the same code +/// for "your tree is out of step" and for "I could not start" -- cargo-vet +/// answers 255 for both -- and mapping every non-zero code to a refusal files +/// the second under the first, which is the failure this command exists to +/// refuse, one layer down. The reader is handed stdout and stderr and names +/// the could-not-look when it sees one. A tool whose exit code already +/// separates the two passes a reader that never fires. +fn tool_read( + root: &Path, + program: &str, + args: &[&str], + unread: impl Fn(i32, &str, &str) -> Option, +) -> Result
{ if let Some(reason) = on_path(program) { return Ok(Section::CouldNotLook(reason)); } @@ -177,15 +210,24 @@ fn tool(root: &Path, program: &str, args: &[&str]) -> Result
{ "{program} was killed and gave no verdict" ))); }; - if code == 0 { + let out = String::from_utf8_lossy(&output.stdout); + let err = String::from_utf8_lossy(&output.stderr); + // THE READER IS ASKED BEFORE THE ZERO IS BELIEVED. A tool that skipped an + // input it could not parse and exited 0 anyway -- zizmor, handed one bad + // workflow among good ones -- is the same could-not-look as one that + // refused to start, and a reader consulted only on failure would never see + // it. The exit code is passed in because for some tools it is the whole + // answer and for others it is the ambiguous part. + let unread = unread(code, &out, &err); + if unread.is_none() && code == 0 { return Ok(Section::Clean); } - for line in String::from_utf8_lossy(&output.stdout) - .lines() - .chain(String::from_utf8_lossy(&output.stderr).lines()) - { + for line in out.lines().chain(err.lines()) { println!(" {line}"); } + if let Some(reason) = unread { + return Ok(Section::CouldNotLook(reason)); + } Ok(Section::Failed) } @@ -461,9 +503,9 @@ fn osv(root: &Path, scope: &Scope) -> Result
{ } println!(" {} lockfile(s) in this range", locks.len()); let borrowed: Vec<&str> = args.iter().map(String::as_str).collect(); - return tool(root, "osv-scanner", &borrowed); + return tool_read(root, "osv-scanner", &borrowed, osv_could_not_look); } - tool( + tool_read( root, "osv-scanner", &[ @@ -484,6 +526,7 @@ fn osv(root: &Path, scope: &Scope) -> Result
{ "g:**/upstream/**", ".", ], + osv_could_not_look, ) } @@ -532,7 +575,7 @@ fn zizmor(root: &Path, scope: &Scope) -> Result
{ ]; args.extend(workflows.iter().map(|path| path.display().to_string())); let borrowed: Vec<&str> = args.iter().map(String::as_str).collect(); - tool(root, "zizmor", &borrowed) + tool_read(root, "zizmor", &borrowed, zizmor_could_not_look) } fn find_workflow_dirs(root: &Path) -> Result> { @@ -617,6 +660,24 @@ fn deny(root: &Path, scope: &Scope) -> Result
{ "cargo deny could not run (is cargo-deny installed?)", ))); } + // EXIT 1 IS TWO DIFFERENT FACTS. cargo-deny's code is a bitmask over + // which check refused -- 1 advisories, 2 bans, 4 licenses, 8 sources -- + // so a matched RUSTSEC advisory and a run that never started share the + // 1. What separates them is the stream: a run that reached its checks + // prints the per-check summary (`advisories FAILED`, `bans ok`) on + // STDOUT whatever the verdict, and one that could not -- an + // unparseable lock, a manifest it could not read, an advisory database + // it could not fetch -- leaves stdout empty and puts `[ERROR]` on + // stderr. Reading the code alone reports a database nobody could + // download as a vulnerability in this tree. + let said = String::from_utf8_lossy(&output.stdout); + if said.trim().is_empty() { + return Ok(Section::CouldNotLook(format!( + "cargo-deny reached no check on {}, so nothing here was judged: {}", + manifest.display(), + first_said(&String::from_utf8_lossy(&output.stderr)) + ))); + } refused = true; // HEADLINES ONLY, and the exit code decides. Grepping for `warning[` // once reported cargo-deny's own informational warnings as failures, @@ -655,6 +716,93 @@ fn deny(root: &Path, scope: &Scope) -> Result
{ Ok(Section::Clean) } +/// cargo-vet's could-not-look, which shares exit 255 with its finding. +/// +/// cargo-vet answers 255 for two facts. Dependencies carrying no audit are a +/// finding, and print "Vetting Failed!" on STDOUT with stderr empty. A run +/// that could not start -- a store that does not parse, a `Cargo.lock` that +/// `--locked` refuses, a `cargo metadata` that failed -- prints its diagnostic +/// on STDERR with stdout empty. The exit code confuses them and the stream +/// does not. +/// +/// THE TEST IS THE STREAM, NOT THE WORDING. Matching the sentence cargo-vet +/// prints when it fails would make this section turn a finding into could-not- +/// look the first time upstream rewords it, and a discriminator that decays +/// silently on somebody else's release is worse than the exit code it +/// replaces. Anything on stdout means the tool got far enough to report on the +/// tree, whatever it called the result; a non-zero exit that reported nothing +/// judged no dependency, and that is the third verdict. +fn vet_could_not_look(code: i32, stdout: &str, stderr: &str) -> Option { + if code == 0 || !stdout.trim().is_empty() { + return None; + } + Some(format!( + "cargo-vet exited without reporting on a single dependency, so nothing \ + here was vetted: {}", + first_said(stderr) + )) +} + +/// The first thing a tool said, so a could-not-look names something. +/// +/// "The scanner was inconclusive" with no subject is a red nobody can act on, +/// which is the same argument the guarddog reason is built on. +fn first_said(text: &str) -> &str { + text.lines() + .map(str::trim) + .find(|line| !line.is_empty()) + .unwrap_or("it printed nothing at all") +} + +/// osv-scanner's could-not-look, which its own exit code names. +/// +/// `0` is clean and `1` is a vulnerability; every higher code is the scanner +/// declining to answer. `127` covers a path it could not resolve, a lockfile +/// it could not parse, a config it could not read and a query it could not +/// send. `128` is "no package sources found", which is NOT a clean tree: this +/// section hands osv-scanner lockfiles by name, so inputs that yielded no +/// package mean the named files were never read. +fn osv_could_not_look(code: i32, _stdout: &str, stderr: &str) -> Option { + if matches!(code, 0 | 1) { + return None; + } + Some(format!( + "osv-scanner exited {code} without a verdict, so no lockfile here was \ + checked: {}", + first_said(stderr) + )) +} + +/// zizmor's could-not-look, which hides in two places and one of them is zero. +/// +/// The verdict codes are `11` through `14`, one per severity present, so any +/// other non-zero code is a run that audited nothing. +/// +/// THE ZERO IS THE DANGEROUS ONE. Handed a workflow it cannot parse ALONGSIDE +/// workflows it can, zizmor skips the bad one, audits the rest, and exits 0 +/// with "No findings to report. Good job!" on stdout. The only witness is a +/// `failed to parse input:` line on stderr. Its SARIF reports +/// `executionSuccessful: true` in exactly that case, so the structured output +/// is worse than useless here. This section hands zizmor a LIST of workflow +/// files, which is precisely the shape that triggers it. +fn zizmor_could_not_look(code: i32, _stdout: &str, stderr: &str) -> Option { + const SKIPPED: &str = "failed to parse input:"; + let skipped = stderr.matches(SKIPPED).count(); + if skipped > 0 { + return Some(format!( + "zizmor could not parse {skipped} of the workflow(s) it was handed \ + and audited the rest, so those were never read" + )); + } + if matches!(code, 0 | 11..=14) { + return None; + } + Some(format!( + "zizmor exited {code} without auditing anything: {}", + first_said(stderr) + )) +} + fn vet(root: &Path, scope: &Scope) -> Result
{ // Conditional on the store existing: a vet store carries an exemption for // every dependency present the day it was created, and creating one @@ -676,7 +824,7 @@ fn vet(root: &Path, scope: &Scope) -> Result
{ ))); } } - tool(root, "cargo", &["vet", "--locked"]) + tool_read(root, "cargo", &["vet", "--locked"], vet_could_not_look) } /// Metadata rules only: the source-code rules download every release, which @@ -744,6 +892,96 @@ fn rules_that_did_not_run(output: &str) -> Option { )) } +/// What one `guarddog verify` run established. +enum Read { + /// guarddog objected to something, described. + Risks(String), + /// guarddog did not answer, described. + Unread(String), + /// guarddog ran and objected to nothing. + Clean, +} + +/// Read one `guarddog verify` run out of its report rather than its exit code. +/// +/// GUARDDOG CANNOT SAY "I FOUND SOMETHING" IN ITS EXIT CODE. `verify` answers +/// 0 whether it found three high-severity risks or none, in both ecosystems, +/// so a section reading the code called every finding clean. That is the one +/// direction this crate must never get wrong, and it was wrong here. +/// +/// `--exit-non-zero-on-finding` is not the fix. It keys off `issues`, which +/// counts capability matches -- `six` scores `issues: 2` with `risks: []` and +/// guarddog's own label `no_risks_detected` -- so the flag turns a package its +/// own report calls clean into a hard failure, trading a false negative for a +/// false positive. +/// +/// So this is the one scanner whose FINDINGS are read here, against the rule +/// the module header sets, and the reason is that the alternative is running +/// it for nothing. `risks` is guarddog's own list of what it objected to and +/// nothing is re-judged: the count is reported, not recomputed. +fn guarddog_read(at: &str, code: Option, stdout: &str, stderr: &str) -> Read { + // Its own admission first, because it survives whatever the report is. + if let Some(reason) = rules_that_did_not_run(stdout).or_else(|| rules_that_did_not_run(stderr)) + { + return Read::Unread(reason); + } + if code != Some(0) { + return Read::Unread(format!( + "guarddog gave no report at {at}: {}", + first_said(stderr) + )); + } + let Ok(serde_json::Value::Array(entries)) = serde_json::from_str::(stdout) + else { + return Read::Unread(format!( + "guarddog printed no report this could read at {at}" + )); + }; + // A bare `[]` against a manifest that had dependencies is what a total + // network failure looks like here, and it is not a clean bill of health. + if entries.is_empty() { + return Read::Unread(format!("guarddog reported on no dependency at all at {at}")); + } + let mut objected: Vec = Vec::new(); + for entry in &entries { + let name = entry + .get("dependency") + .and_then(serde_json::Value::as_str) + .unwrap_or("a dependency it did not name"); + let Some(result) = entry.get("result") else { + return Read::Unread(format!("guarddog said nothing about {name} at {at}")); + }; + let scanned = result + .get("errors") + .and_then(serde_json::Value::as_object) + .is_some_and(serde_json::Map::is_empty); + if !scanned || result.get("results").is_none() { + return Read::Unread(format!( + "guarddog could not scan {name} at {at}: {}", + result.get("errors").map_or_else( + || String::from("it reported no result for it"), + ToString::to_string + ) + )); + } + let risks = result + .get("risks") + .and_then(serde_json::Value::as_array) + .map_or(0, Vec::len); + if risks > 0 { + objected.push(format!("{name} ({risks} risk(s))")); + } + } + if objected.is_empty() { + Read::Clean + } else { + Read::Risks(format!( + "{at}: guarddog objected to {}", + objected.join(", ") + )) + } +} + fn guarddog(root: &Path, scope: &Scope) -> Result
{ let (python, npm) = match scope { Scope::Whole => ( @@ -803,31 +1041,47 @@ fn guarddog(root: &Path, scope: &Scope) -> Result
{ let requirements = tempfile_guard::TempFile::containing(&String::from_utf8_lossy(&exported.stdout))?; let status = Command::new("guarddog") - .args(["pypi", "verify"]) + .args(["pypi", "verify", "--output-format", "json"]) .arg(&requirements.path) .args(GUARDDOG_RULES) .current_dir(directory) .output() .map_err(|error| Fatal::new(format!("could not run guarddog: {error}")))?; - unrun = unrun.or_else(|| rules_that_did_not_run(&String::from_utf8_lossy(&status.stdout))); - if !status.status.success() { - println!(" FAILED: guarddog pypi: {}", directory.display()); - refused = true; + match guarddog_read( + &directory.display().to_string(), + status.status.code(), + &String::from_utf8_lossy(&status.stdout), + &String::from_utf8_lossy(&status.stderr), + ) { + Read::Clean => {} + Read::Risks(said) => { + println!(" FAILED: guarddog pypi: {said}"); + refused = true; + } + Read::Unread(said) => unrun = unrun.or(Some(said)), } } for manifest in npm { let directory = manifest.parent().unwrap_or(root); checked += 1; let status = Command::new("guarddog") - .args(["npm", "verify", "package.json"]) + .args(["npm", "verify", "--output-format", "json", "package.json"]) .args(GUARDDOG_RULES) .current_dir(directory) .output() .map_err(|error| Fatal::new(format!("could not run guarddog: {error}")))?; - unrun = unrun.or_else(|| rules_that_did_not_run(&String::from_utf8_lossy(&status.stdout))); - if !status.status.success() { - println!(" FAILED: guarddog npm: {}", directory.display()); - refused = true; + match guarddog_read( + &directory.display().to_string(), + status.status.code(), + &String::from_utf8_lossy(&status.stdout), + &String::from_utf8_lossy(&status.stderr), + ) { + Read::Clean => {} + Read::Risks(said) => { + println!(" FAILED: guarddog npm: {said}"); + refused = true; + } + Read::Unread(said) => unrun = unrun.or(Some(said)), } } println!(" {checked} Python/npm manifest(s) checked"); diff --git a/tests/supply_chain_cli.rs b/tests/supply_chain_cli.rs index 22b0d50..427596e 100644 --- a/tests/supply_chain_cli.rs +++ b/tests/supply_chain_cli.rs @@ -133,6 +133,21 @@ fn journal(root: &Path) -> String { /// The working directory is half the assertion in the scoped tests: guarddog is /// run from the manifest's own directory, and a scan of the right file from the /// wrong place reads the wrong `package.json`. +/// A clean `guarddog verify --output-format json` report. +/// +/// guarddog is run with `--output-format json` because its exit code cannot +/// say it found something: `verify` answers 0 either way. So a stub that only +/// answers `exit 0` no longer models the tool -- a clean run PRINTS a report +/// whose `risks` list is empty, and printing nothing is a run that did not +/// report, which is could-not-look. +const GUARDDOG_CLEAN: &str = "echo '[{\"dependency\":\"six\",\"result\":\ + {\"errors\":{},\"issues\":0,\"results\":{},\"risks\":[]}}]'\nexit 0"; + +/// The same report with one risk in it, which is a finding at exit 0. +const GUARDDOG_RISK: &str = "echo '[{\"dependency\":\"reqests\",\"result\":\ + {\"errors\":{},\"issues\":1,\"results\":{},\"risks\":\ + [{\"name\":\"typosquatting\",\"severity\":\"high\"}]}}]'\nexit 0"; + fn recording(answer: &str) -> String { format!("echo \"$(basename \"$0\") [$PWD] $*\" >> \"$STUB_LOG\"\n{answer}") } @@ -471,9 +486,12 @@ fn a_python_lock_is_exported_for_guarddog_and_its_refusal_is_the_runs() { ("uv", "echo 'reqests==2.0.0'"), ( "guarddog", - "grep -q 'reqests==2.0.0' \"$3\" || { echo 'guarddog was not handed the export'; exit 2; }\n\ - echo 'typosquatting: reqests shadows requests'\n\ - exit 1", + // `$5`, not `$3`: the export path sits after `--output-format json`, + // which guarddog needs because its exit code cannot report a find. + &format!( + "grep -q 'reqests==2.0.0' \"$5\" || {{ echo 'guarddog was not handed the \ + export'; exit 2; }}\n{GUARDDOG_RISK}" + ), ), ]); let output = supply(&root, Some(&tools)); @@ -548,7 +566,9 @@ fn an_npm_manifest_is_scanned_where_it_lives_and_a_clean_one_is_a_pass() { ("osv-scanner", "exit 0"), ( "guarddog", - "grep -q '\"web\"' package.json || { echo 'wrong directory'; exit 1; }\nexit 0", + &format!( + "grep -q '\"web\"' package.json || {{ echo 'wrong directory'; exit 1; }}\n{GUARDDOG_CLEAN}" + ), ), ]); let output = supply(&root, Some(&tools)); @@ -570,7 +590,7 @@ fn a_refusing_npm_scan_names_the_directory_it_refused_in() { let root = repository(); std::fs::create_dir_all(root.join("web")).unwrap(); std::fs::write(root.join("web/package.json"), "{\"name\": \"web\"}\n").unwrap(); - let tools = stubs(&[("osv-scanner", "exit 0"), ("guarddog", "exit 1")]); + let tools = stubs(&[("osv-scanner", "exit 0"), ("guarddog", GUARDDOG_RISK)]); let output = supply(&root, Some(&tools)); assert_eq!(code(&output), 1, "{}", text(&output)); let said = text(&output); @@ -668,7 +688,7 @@ fn a_range_touching_no_manifest_runs_no_scanner_and_says_so_in_one_line() { let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("zizmor", &recording("exit 0")), - ("guarddog", &recording("exit 0")), + ("guarddog", &recording(GUARDDOG_CLEAN)), ("cargo", &recording("exit 0")), ]); let output = pushed(&root, &tools, &before, &after); @@ -698,7 +718,7 @@ fn all_scans_every_manifest_even_where_the_range_holds_none_of_them() { ("osv-scanner", &recording("exit 0")), ("zizmor", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording("exit 0")), + ("guarddog", &recording(GUARDDOG_CLEAN)), ]); let output = supply(&root, Some(&tools)); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -724,7 +744,7 @@ fn a_changed_python_lock_runs_guarddog_in_that_directory_and_not_the_others() { let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording("exit 0")), + ("guarddog", &recording(GUARDDOG_CLEAN)), ]); let output = pushed(&root, &tools, &before, &after); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -823,7 +843,7 @@ fn a_bumped_submodule_pointer_expands_into_the_members_own_manifests() { let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording("exit 0")), + ("guarddog", &recording(GUARDDOG_CLEAN)), ]); let output = pushed(&root, &tools, &before, &after); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -857,7 +877,7 @@ fn a_submodule_commit_the_store_does_not_have_widens_to_every_manifest_under_it( let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording("exit 0")), + ("guarddog", &recording(GUARDDOG_CLEAN)), ]); let output = pushed(&root, &tools, &before, &after); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -919,3 +939,247 @@ fn guarddog_rules_that_did_not_run_are_could_not_look_rather_than_a_clean_scan() assert!(said.contains("1 rule(s) unrun"), "{said}"); assert!(!said.contains("all checks passed"), "{said}"); } + +/// cargo-vet's finding and cargo-vet's refusal to start share exit 255. +/// +/// A dependency carrying no audit and a store that does not parse are the same +/// code, so a section answering by exit code alone files the second under the +/// first. The stream separates them: the finding is on stdout and the refusal +/// is on stderr. This is the finding half, which must stay a verdict. +#[test] +fn cargo_vet_that_found_unvetted_dependencies_is_a_finding_and_exits_one() { + let root = repository(); + std::fs::create_dir_all(root.join("supply-chain")).unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "cargo", + "case \"$1\" in\n\ + vet) echo 'Vetting Failed!'; echo '11 unvetted dependencies:'; exit 255 ;;\n\ + *) exit 0 ;;\n\ + esac", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 1, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("Vetting Failed!"), "{said}"); + assert!(!said.contains("could not run"), "{said}"); +} + +/// The refusal half of the same exit code. +/// +/// cargo-vet that could not open its store judged no dependency at all, and +/// that is could-not-look rather than a tree that is out of step. Reading the +/// exit code alone would report this repository as failing an audit nobody +/// ran. +#[test] +fn cargo_vet_that_could_not_open_its_store_is_could_not_look_and_exits_two() { + let root = repository(); + std::fs::create_dir_all(root.join("supply-chain")).unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "cargo", + "case \"$1\" in\n\ + vet) echo 'ERROR x Failed to parse toml file' >&2; exit 255 ;;\n\ + *) exit 0 ;;\n\ + esac", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 2, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("nothing here was vetted"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + +/// osv-scanner says could-not-look in its own exit code, and 1 is not it. +/// +/// `1` is a vulnerability. `127` is a path it could not resolve, a lockfile it +/// could not parse, a config it could not read or a query it could not send, +/// and reading it as a refusal reports a network outage as a vulnerability in +/// this tree. +#[test] +fn osv_scanner_that_could_not_resolve_its_input_is_could_not_look_not_a_finding() { + let root = repository(); + let tools = stubs(&[( + "osv-scanner", + "echo 'failed to resolve path: no such file or directory' >&2\nexit 127", + )]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 2, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("no lockfile here was checked"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + +/// The control for the case above: exit 1 stays a vulnerability. +#[test] +fn osv_scanner_that_found_a_vulnerability_is_a_finding_and_exits_one() { + let root = repository(); + let tools = stubs(&[( + "osv-scanner", + "echo 'Total 2 packages affected by 64 known vulnerabilities'\nexit 1", + )]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 1, "{}", text(&output)); + assert!( + text(&output).contains("64 known vulnerabilities"), + "{}", + text(&output) + ); +} + +/// zizmor's could-not-look wears exit 0, which is the whole problem. +/// +/// Handed one workflow it cannot parse alongside workflows it can, zizmor +/// skips the bad one, audits the rest and exits 0 saying it found nothing. Its +/// SARIF reports executionSuccessful true in exactly that case, so the only +/// witness is the warning on stderr. A section believing the zero calls a +/// workflow nobody read clean. +#[test] +fn zizmor_that_skipped_a_workflow_it_could_not_parse_is_not_a_clean_audit() { + let root = repository(); + let workflows = root.join(".github/workflows"); + std::fs::create_dir_all(&workflows).unwrap(); + std::fs::write(workflows.join("ci.yml"), "on: push\njobs: {}\n").unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "zizmor", + "echo ' WARN collect_inputs: zizmor::registry::input: failed to parse input: \ + mapping values are not allowed' >&2\n\ + echo 'No findings to report. Good job! (2 suppressed)'\n\ + exit 0", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 2, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("could not parse 1"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + +/// The control: zizmor's severity ladder stays a finding. +/// +/// 11 through 14 are the codes it answers when it audited everything and found +/// something, one per severity present. Only the other non-zero codes, and the +/// skipped-input warning above, are could-not-look. +#[test] +fn zizmor_severity_exit_codes_are_findings_rather_than_could_not_look() { + let root = repository(); + let workflows = root.join(".github/workflows"); + std::fs::create_dir_all(&workflows).unwrap(); + std::fs::write(workflows.join("ci.yml"), "on: push\njobs: {}\n").unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "zizmor", + "echo 'warning[artipacked]: credential persistence'\nexit 14", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 1, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("artipacked"), "{said}"); + assert!(!said.contains("without auditing anything"), "{said}"); +} + +/// cargo-deny's exit 1 is an advisory OR a database it could not fetch. +/// +/// The code is a bitmask over which check refused, and advisories own the 1, +/// so an advisory database that would not download shares its code with a +/// RUSTSEC match. A run that reached its checks prints the per-check summary +/// on stdout; one that did not leaves stdout empty. +#[test] +fn cargo_deny_that_never_reached_a_check_is_could_not_look_not_an_advisory() { + let root = repository(); + std::fs::write(root.join("deny.toml"), "[bans]\n").unwrap(); + std::fs::write(root.join("Cargo.toml"), "[package]\nname = \"f\"\n").unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "cargo", + "[ \"$1\" = deny ] || exit 0\n\ + echo '[ERROR] failed to fetch advisory database' >&2\n\ + exit 1", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 2, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("reached no check"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + +/// guarddog reports a finding at exit 0, and the finding must survive that. +/// +/// `guarddog verify` answers 0 whether it found three high-severity risks or +/// none, so a section reading the exit code called every finding clean. This +/// is the false negative that reading `risks` exists to close, on the one +/// scanner here whose job is malware and typosquats. +#[test] +fn guarddog_that_found_risks_and_exited_zero_is_a_finding_not_a_clean_scan() { + let root = repository(); + std::fs::create_dir_all(root.join("web")).unwrap(); + std::fs::write(root.join("web/package.json"), "{\"name\": \"web\"}\n").unwrap(); + let tools = stubs(&[("osv-scanner", "exit 0"), ("guarddog", GUARDDOG_RISK)]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 1, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("objected to reqests"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + +/// `issues` is not the finding count, so a clean package stays clean. +/// +/// `six` reports `issues: 2` with `risks: []` and guarddog's own label +/// `no_risks_detected`; the two issues are capability matches on an `exec()`. +/// This is why `--exit-non-zero-on-finding`, which counts issues, is not the +/// remedy for the case above: it would fail a package guarddog calls clean. +#[test] +fn guarddog_issues_without_risks_are_not_a_finding() { + let root = repository(); + std::fs::create_dir_all(root.join("web")).unwrap(); + std::fs::write(root.join("web/package.json"), "{\"name\": \"web\"}\n").unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "guarddog", + "echo '[{\"dependency\":\"six\",\"result\":{\"errors\":{},\"issues\":2,\ + \"results\":{},\"risks\":[]}}]'\nexit 0", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 0, "{}", text(&output)); + assert!( + text(&output).contains("all checks passed"), + "{}", + text(&output) + ); +} + +/// A dependency guarddog could not download is could-not-look, not clean. +/// +/// The 404 and network paths populate `errors` and drop `results`, and still +/// exit 0. Reading the code alone calls a package nobody scanned clean. +#[test] +fn guarddog_that_could_not_scan_a_dependency_is_could_not_look() { + let root = repository(); + std::fs::create_dir_all(root.join("web")).unwrap(); + std::fs::write(root.join("web/package.json"), "{\"name\": \"web\"}\n").unwrap(); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "guarddog", + "echo '[{\"dependency\":\"left-pad\",\"result\":{\"errors\":\ + {\"download-package\":\"Received status code: 404 from PyPI\"},\"issues\":0}}]'\nexit 0", + ), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 2, "{}", text(&output)); + let said = text(&output); + assert!(said.contains("could not scan left-pad"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +}