diff --git a/specs/016-native-tools/contracts/scanner-adapter.md b/specs/016-native-tools/contracts/scanner-adapter.md index e9e4529..3800a37 100644 --- a/specs/016-native-tools/contracts/scanner-adapter.md +++ b/specs/016-native-tools/contracts/scanner-adapter.md @@ -50,7 +50,10 @@ pub struct ScanRequest { pub target: PathBuf, /// Language hint, where the scanner needs one. pub lang: Option, - /// Wall-clock budget. Exceeded ⇒ `Failed`, never a partial `Completed`. + /// Wall-clock budget for the **whole scan**, not an allowance each child draws afresh: the + /// `scan` tool turns it into a deadline and gives every child what is left of it, so a + /// multi-step scanner cannot run to a multiple of the ceiling its operator set. Exceeded ⇒ + /// `Failed`, never a partial `Completed`. pub timeout: Duration, } ``` @@ -178,11 +181,12 @@ Consumes an operator-provisioned, version-pinned **bundle** — `codeql-bundle-< `codeql-action` v4.37.3 pins `codeql-bundle-v2.26.1` / CLI `2.26.1` (`src/defaults.json`). ```text -probe : pin + bundle presence + a version pinned at all + is this language analysable +probe : pin + a version pinned at all + bundle presence + is this language analysable no process is spawned to answer any of it + unpinned ⇒ Unavailable { BundleMismatch { expected: UNPINNED, found: None } } preflight : codeql version --format=json → compare against grant.bundle_version - mismatch, unreadable, or unpinned ⇒ Unavailable { BundleMismatch { expected, found } } + mismatch or unreadable ⇒ Unavailable { BundleMismatch { expected, found } } steps : database create --language= --build-mode=none --source-root= database analyze --format=sarif-latest --output= @@ -193,7 +197,9 @@ The granted binary **is** the bundle's CLI, so the inode pin already covers what granted binary — otherwise the version verified belongs to a different CodeQL than the one that scans. `bundle_version` accepts either spelling (`codeql-bundle-v2.26.1` or `2.26.1`), because the operator has the bundle version to hand while the CLI only ever reports the CLI version. An -**unpinned** bundle yields no preflight and no command: nothing unverified is reached by any route. +**unpinned** bundle is refused by `probe` as `Unavailable` — nothing has run at that point, so it is +never a `Failed` — and yields no preflight and no command besides: nothing unverified is reached by +any route. `grant.rules`, when set, names a query suite and replaces the default `codeql/-queries` pack. The database lives in the per-call scratch directory and is removed with it — it is large, and it is diff --git a/specs/016-native-tools/contracts/tool-outcome.md b/specs/016-native-tools/contracts/tool-outcome.md index 5ca5e65..6eafb21 100644 --- a/specs/016-native-tools/contracts/tool-outcome.md +++ b/specs/016-native-tools/contracts/tool-outcome.md @@ -32,12 +32,18 @@ pub enum UnavailableReason { NotGranted { name: String }, /// The binary at the granted path no longer matches its pinned identity (FR-009). PinMismatch { path: PathBuf }, - /// A required provisioned artefact is absent or the wrong version (FR-011). + /// A required provisioned artefact is absent, the wrong version, or unpinned (FR-011). The + /// unpinned case carries `expected: UNPINNED` — there is no version it was allowed to be — and + /// renders as the setting that would pin it rather than as a version comparison. BundleMismatch { expected: String, found: Option }, /// The requested language has no grammar in this build. LanguageUnsupported { lang: String, compiled_in: Vec }, /// The path is not inside a repository (US5 scenario 3). NotARepository { path: PathBuf }, + /// The requested language can only be analysed by observing a build, which bee will not do + /// (FR-011, US6 scenario 3). Distinct from `LanguageUnsupported`: that one is a build that + /// lacks a grammar, this one is a deliberate refusal no rebuild changes. + LanguageRequiresBuild { lang: String, supported: Vec }, } ``` @@ -53,9 +59,12 @@ This is enforced three ways: 1. **Type-level.** Construction sites for `Completed` are confined to the success arm of each tool's worker. `?` on any fallible step yields `Failed`; a probe failure yields `Unavailable`. -2. **Test.** `tests/scanner_adapter.rs` asserts each of the seven `UnavailableReason` variants is - produced by its triggering condition and that none of them serialises to something a caller could - mistake for a clean scan. +2. **Test.** `tests/scanner_adapter.rs` asserts each of the eight `UnavailableReason` variants is + produced by its triggering condition, and `tests/fail_closed_tools.rs` asserts via `every_reason()` + that none of them serialises to something a caller could mistake for a clean scan. `every_reason()` + holds exactly one entry per variant — the audit-slug check depends on it — so a second *spelling* + of a variant (the unpinned `BundleMismatch`, which renders through its own arm) earns its own test + beside that list rather than an extra entry in it. 3. **Rendering.** The model-facing and operator-facing renderings of `Unavailable` and `Failed` never contain the phrase used for a clean result. A clean scan reads *"scanned N files, no findings"*; an unavailable one reads *"did not run: "*. diff --git a/src/scanners/codeql.rs b/src/scanners/codeql.rs index 2ca470b..5f903ad 100644 --- a/src/scanners/codeql.rs +++ b/src/scanners/codeql.rs @@ -31,7 +31,7 @@ use std::path::{Path, PathBuf}; use super::{probe_binary, validate_target, ScanRequest, ScannerAdapter, ScannerGrant}; -use crate::tools::outcome::UnavailableReason; +use crate::tools::outcome::{UnavailableReason, UNPINNED}; pub struct CodeQl; @@ -113,20 +113,33 @@ impl ScannerAdapter for CodeQl { } /// Four questions, none of which costs a process: is the pinned binary still the pinned binary, - /// is the bundle where the operator said, was a version pinned at all, and is this a language - /// bee will analyse. + /// was a version pinned at all, is the bundle where the operator said, and is this a language + /// bee will analyse. Each is a reason the scan *could not run* — so each is `Unavailable`, and + /// none of them is reported as a scan that ran and broke. fn probe(&self, grant: &ScannerGrant, req: &ScanRequest) -> Result<(), UnavailableReason> { probe_binary(grant)?; + // Was a version pinned at all. This is a refusal to *run*, not a run that broke — nothing + // has executed at this point — so it is `Unavailable`, and it belongs here rather than in + // `steps`, which is reached only after this method has already passed the request as + // answerable (contract `scanner-adapter.md`). + if grant.bundle_version.is_none() { + return Err(UnavailableReason::BundleMismatch { + expected: UNPINNED.to_string(), + found: None, + }); + } + // A bundle path is optional — the granted binary is the bundle's CLI, so the pin already // covers what actually runs. When the operator *does* name one, it has to be the bundle the // granted binary came out of, or the version verified below belongs to a different CodeQL // than the one that will do the scanning. if let Some(bundle) = &grant.bundle { + // Unwrap-free: the no-pin case returned above, so a pin is present by construction. let expected = grant .bundle_version .clone() - .unwrap_or_else(|| "(unpinned)".to_string()); + .unwrap_or_else(|| UNPINNED.to_string()); if !bundle.exists() { return Err(UnavailableReason::BundleMismatch { expected, @@ -233,12 +246,14 @@ impl ScannerAdapter for CodeQl { ) -> Result>, String> { validate_target(&req.target)?; + // `probe` refuses an unpinned bundle as `Unavailable` before anything reaches here, which is + // where that refusal is *stated* — this is the same belt-and-braces the language check below + // gets, for the same reason: `steps` authors a command line, so it re-checks what it is + // about to write rather than trusting a caller to have asked first. if grant.bundle_version.is_none() { return Err( - "no bundle version pinned for codeql: set `[security.scanners.codeql] \ - bundle_version` to the provisioned bundle's version (for example \ - `codeql-bundle-v2.26.1`). Running an unverified analysis bundle is refused, \ - because its queries decide what counts as a finding." + "no bundle version pinned for codeql: refusing to build a command for an \ + unverified analysis bundle" .to_string(), ); } @@ -399,6 +414,23 @@ mod tests { assert!(err.to_string().contains("2.26.1")); } + /// The refusal an unpinned bundle earns is "could not run", not "ran and broke". Nothing has + /// executed when it fires, and the two render with different prefixes and different audit slugs + /// — so the distinction is the operator's, not a detail of where the check happened to live. + #[test] + fn an_unpinned_bundle_could_not_run_rather_than_ran_and_broke() { + let mut f = fixture(); + f.grant.bundle_version = None; + let err = CodeQl.probe(&f.grant, &req(&f, "python")).unwrap_err(); + let UnavailableReason::BundleMismatch { expected, found } = &err else { + panic!("expected a bundle mismatch, got {err:?}"); + }; + assert_eq!(expected, UNPINNED); + assert!(found.is_none()); + assert_eq!(err.slug(), "bundle_mismatch"); + assert!(err.to_string().contains("bundle_version"), "{err}"); + } + #[test] fn a_binary_outside_the_configured_bundle_is_refused() { let mut f = fixture(); @@ -511,11 +543,13 @@ mod tests { f.grant.bundle_version = None; // Nothing to verify against... assert!(CodeQl.preflight(&f.grant).is_none()); - // ...and therefore nothing to run. + // ...and therefore nothing to run. The sentence naming `bundle_version` belongs to the + // `Unavailable` that `probe` returns (asserted above); what `steps` owes is a refusal to + // author a command line, whichever route reached it. let err = CodeQl .steps(&f.grant, &req(&f, "python"), &f.scratch, &f.out) .unwrap_err(); - assert!(err.contains("bundle_version"), "{err}"); + assert!(err.contains("unverified analysis bundle"), "{err}"); } #[test] diff --git a/src/tools/outcome.rs b/src/tools/outcome.rs index 36a118d..f008ba7 100644 --- a/src/tools/outcome.rs +++ b/src/tools/outcome.rs @@ -28,6 +28,15 @@ pub const UNAVAILABLE_PREFIX: &str = "did not run:"; /// The phrase every "ran and broke" renders with. pub const FAILED_PREFIX: &str = "failed:"; +/// The `expected` an adapter reports when the operator pinned no version at all. +/// +/// A missing pin is a *kind* of bundle mismatch — there is no version the bundle is allowed to be — +/// so it travels as [`UnavailableReason::BundleMismatch`] rather than a variant of its own +/// (contract `scanner-adapter.md`: "mismatch, unreadable, or unpinned ⇒ `BundleMismatch`"). It reads +/// nothing like a version, so it cannot be confused with one, and [`UnavailableReason`]'s `Display` +/// gives it the sentence an operator can act on. +pub const UNPINNED: &str = "(unpinned)"; + /// The outcome of one security-tool invocation. `T` is the tool's success payload — `Vec`, /// `Vec`, `Severity`, and so on. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -106,6 +115,18 @@ impl std::fmt::Display for UnavailableReason { path.display() ) } + // No pin at all comes first: with nothing to compare against, neither of the two + // sentences below is true — a bundle may well be present, and there is no expected + // version to name. + UnavailableReason::BundleMismatch { expected, .. } if expected == UNPINNED => { + write!( + f, + "no bundle version is pinned: set the scanner's `bundle_version` to the \ + provisioned bundle's version (for example `codeql-bundle-v2.26.1`). Running \ + an unverified analysis bundle is refused, because its queries decide what \ + counts as a finding" + ) + } UnavailableReason::BundleMismatch { expected, found } => match found { Some(found) => write!( f, diff --git a/src/tools/scanner.rs b/src/tools/scanner.rs index e33d4ba..32da881 100644 --- a/src/tools/scanner.rs +++ b/src/tools/scanner.rs @@ -202,6 +202,12 @@ impl Tool for ScanTool { lang: args.lang, timeout: budget, }; + // The budget is a ceiling on the *scan*, not an allowance each child draws afresh. A CodeQL + // scan is a version check plus two children; handing each one the full budget would let a + // scan run to three times the limit its operator set, which is not a limit. So the budget + // becomes a deadline here, and every child gets what is left of it. + let deadline = std::time::Instant::now() + budget; + let remaining = || deadline.saturating_duration_since(std::time::Instant::now()); // ── 3. Can this scanner answer this request at all? ───────────────────────────────────── // Before argv construction and before any spawn, so unavailability costs no process — and, @@ -250,14 +256,16 @@ impl Tool for ScanTool { // ── 4. Preflight: ask the binary what it is, in scope, before trusting its answers ────── if let Some(argv) = adapter.preflight(grant) { - let probe = match run_child_timed(sandbox, &program, &argv, budget).await { + let probe = match run_child_timed(sandbox, &program, &argv, remaining()).await { Ok(r) => r, - Err(ChildError::TimedOut(d)) => { + Err(ChildError::TimedOut(_)) => { cleanup(); + // The budget, not the slice of it this child got: the operator set the former + // and would have to work backwards from the latter. return failed(format!( - "{} did not answer a version check within {}s", + "{} timed out on a version check within its {}s scan budget", args.scanner, - d.as_secs() + budget.as_secs() )); } Err(ChildError::Spawn(e)) => { @@ -287,18 +295,18 @@ impl Tool for ScanTool { // analysed, and analysing it anyway would produce an empty report — a clean scan by // accident, which is the one outcome this tool may never manufacture (FR-012). for (i, argv) in steps.iter().enumerate() { - let run = match run_child_timed(sandbox, &program, argv, budget).await { + let run = match run_child_timed(sandbox, &program, argv, remaining()).await { Ok(r) => r, - Err(ChildError::TimedOut(d)) => { + Err(ChildError::TimedOut(_)) => { // No partial answer: whatever it wrote is by definition incomplete, and // presenting an incomplete scan as a scan is the failure this feature is built // against. let _ = std::fs::remove_file(&report_path); cleanup(); return failed(format!( - "{} timed out after {}s; no partial findings are reported", + "{} timed out within its {}s scan budget; no partial findings are reported", args.scanner, - d.as_secs() + budget.as_secs() )); } Err(ChildError::Spawn(e)) => { diff --git a/tests/codeql_adapter.rs b/tests/codeql_adapter.rs index b427e6a..19a0c3b 100644 --- a/tests/codeql_adapter.rs +++ b/tests/codeql_adapter.rs @@ -21,7 +21,7 @@ use std::path::{Path, PathBuf}; use bee::scanners::{self, ScannerGrant}; use bee::security::{ScannerConfig, SecurityConfig}; -use bee::tools::outcome::CLEAN_PREFIX; +use bee::tools::outcome::{CLEAN_PREFIX, UNAVAILABLE_PREFIX}; use bee::tools::scanner::ScanTool; use bee::tools::Tool; @@ -43,15 +43,28 @@ fn sandbox() -> bee::Sandbox { /// from `database analyze`. Every invocation is appended to `argv.log`, so a test can assert both /// what bee built and — more often — that bee built nothing at all. fn stub_codeql(dir: &Path, reported_version: &str) -> PathBuf { + stub_codeql_taking(dir, reported_version, 0) +} + +/// The same stub, but every invocation takes `secs` first. A CodeQL scan is three children, so this +/// is what makes the difference between a budget that bounds the *scan* and one each child draws +/// afresh observable at all. +fn stub_codeql_taking(dir: &Path, reported_version: &str, secs: u32) -> PathBuf { let path = dir.join("codeql"); let log = dir.join("argv.log"); let version_json = format!(r#"{{"version":"{reported_version}"}}"#); + let delay = if secs > 0 { + format!("sleep {secs}\n") + } else { + String::new() + }; std::fs::write( &path, format!( r#"#!/bin/sh printf '%s\n' "$@" >> {log} printf -- '--- end of invocation\n' >> {log} +{delay} if [ "$1" = "version" ]; then printf '%s\n' '{version}' @@ -80,6 +93,7 @@ exit 1 "#, log = log.display(), version = version_json, + delay = delay, body = FIXTURE, ), ) @@ -155,6 +169,35 @@ fn pinned_fixture() -> Fixture { fixture(PINNED_CLI, Some(PINNED), None) } +/// A correctly pinned bundle whose every child takes `secs`, under a `budget`-second ceiling. +fn slow_fixture(secs: u32, budget: u64) -> Fixture { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().to_path_buf(); + let bin = stub_codeql_taking(&dir, PINNED_CLI, secs); + let target = dir.join("src"); + std::fs::create_dir_all(&target).unwrap(); + std::fs::write(target.join("vuln.py"), "import subprocess\n").unwrap(); + + let mut security = SecurityConfig::default(); + security.scanners.insert( + "codeql".to_string(), + ScannerConfig { + bundle: None, + bundle_version: Some(PINNED.to_string()), + timeout_secs: Some(budget), + ..Default::default() + }, + ); + let grants = scanners::grants_from_policy(Some(&policy_granting(&bin)), &security); + Fixture { + _tmp: tmp, + dir: dir.clone(), + grants, + ledger: bee::findings::Ledger::at(dir.join("findings")), + target, + } +} + async fn scan_lang(f: &Fixture, lang: &str) -> bee::ToolResult { ScanTool::new(f.grants.clone(), f.ledger.clone(), "run-1") .with_bee_exe(bee_exe()) @@ -250,12 +293,46 @@ async fn an_unpinned_bundle_yields_no_scan_and_says_which_setting_is_missing() { assert!(r.is_error); assert!(r.content.contains("bundle_version"), "{}", r.content); assert!(!r.content.contains(CLEAN_PREFIX), "{}", r.content); + // "could not run", not "ran and broke". Nothing executed — the invocation log below says so — + // and the two states carry different audit slugs, so an operator grepping for setup problems + // must not have to look among the crashes to find this one. + assert!( + r.content.starts_with(UNAVAILABLE_PREFIX), + "an unpinned bundle is a refusal to run, not a failed run: {}", + r.content + ); assert!( invocations(&f).is_empty(), "with nothing to verify against, there is nothing to run" ); } +/// The operator's `timeout_secs` bounds the **scan**, not each child of it. +/// +/// A CodeQL scan is three children — version, create, analyze. Given the budget afresh, each of +/// them fits inside it comfortably and the scan runs to three times the ceiling while every +/// individual child looks well-behaved. That is not a ceiling; it is a ceiling per child, which is +/// a different and much weaker promise than the one `timeout_secs` makes. +#[tokio::test] +async fn the_budget_bounds_the_whole_scan_not_each_child_of_it() { + // Two seconds a child, three children, a three-second ceiling: comfortably within budget + // per-child, comfortably over it in total. + let f = slow_fixture(2, 3); + let started = std::time::Instant::now(); + let r = scan_lang(&f, "python").await; + let elapsed = started.elapsed(); + + assert!(r.is_error, "the scan outran its budget: {}", r.content); + assert!(!r.content.contains(CLEAN_PREFIX), "{}", r.content); + assert!( + elapsed < std::time::Duration::from_secs(5), + "the scan took {elapsed:?} under a 3s budget — the budget is being handed to each child \ + afresh rather than bounding the scan" + ); + // And nothing partial was kept: the report a half-finished scan leaves behind is not a result. + assert!(!f.ledger.log_path().exists()); +} + // ── US6 scenario 3 · a language that can only be analysed by watching a build ──────────────────── #[tokio::test] diff --git a/tests/fail_closed_tools.rs b/tests/fail_closed_tools.rs index 65ff509..cbedf2b 100644 --- a/tests/fail_closed_tools.rs +++ b/tests/fail_closed_tools.rs @@ -38,9 +38,43 @@ fn every_reason() -> Vec { UnavailableReason::NotARepository { path: "/tmp/notarepo".into(), }, + UnavailableReason::LanguageRequiresBuild { + lang: "cpp".into(), + supported: vec!["python".into(), "ruby".into()], + }, ] } +/// The no-pin spelling of a mismatch renders through its own arm, so it gets its own test rather +/// than a second entry in `every_reason` — it is the same variant, and that list is one-per-variant +/// so the slug check above means something. +/// +/// Two obligations: it must not read like a clean scan (the invariant this file exists for), and it +/// must name the setting that fixes it. A refusal an operator cannot act on is only marginally +/// better than a silent one. +#[test] +fn an_unpinned_bundle_names_the_setting_that_would_pin_it() { + let rendered = render_empty(ToolOutcome::unavailable( + UnavailableReason::BundleMismatch { + expected: bee::tools::outcome::UNPINNED.into(), + found: None, + }, + )); + assert!(rendered.is_error); + assert!(rendered.content.starts_with(UNAVAILABLE_PREFIX)); + assert!(!rendered.content.contains(CLEAN_PREFIX)); + assert!( + rendered.content.contains("bundle_version"), + "an unpinned bundle must name the setting: {}", + rendered.content + ); + assert!( + !rendered.content.contains("expected version"), + "there is no expected version when nothing was pinned: {}", + rendered.content + ); +} + fn render_empty(outcome: ToolOutcome>) -> bee::tools::ToolResult { outcome.into_tool_result(|v| clean_scan(v.len(), "files")) }