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}"); +}