From ac35d9346fa9d221144330435aea70a314f59473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFs=20Postula?= Date: Wed, 19 Aug 2026 11:45:51 +0200 Subject: [PATCH] fix(release): repair four gates that reported success without checking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the release command family surfaced four defects that each let a failure through silently, plus the supporting should-fixes. All sit in the #336/#337 stack and none change the published object shapes. bundle-linux could never have succeeded. cargo-deb's default output is `target_dir_base.join("debian")` with no target triple (config.rs `default_deb_output_dir`; the triple is appended only by `target_dependent_path`, which the deb output does not go through), so `--target` does not move the artifact and reading a tripled path was an unconditional ENOENT. `-o` is now passed explicitly at the per-triple directory, which is cleared first and from which exactly one .deb must emerge: the previous take-the-first-by-sort-order would ship a stale build. The production check-run gate was satisfied by the job it runs inside. `conclusion` is null until `status` reaches `completed`, so a queued or running check read as green, and this command's own check run is attached to the tagged commit, making the count-based no-checks refusal unreachable. The gate now takes named `--required-checks` and demands completed-and-success for each. BREAKING: `--required-checks` is mandatory unless `--skip-check-runs` is passed. fsl_libs supplies it via `vars.RELEASE_REQUIRED_CHECKS`, defaulting to `test`. Failure is considered only for the required names. A commit on main routinely carries unrelated bad check runs: measured on fsl_libs main, 20 check runs including a failed docker_build and several cancelled by concurrency. Refusing on any bad run anywhere would block every release. The same commit carries two runs named `test`, one failed and one successful, because the postsubmit and the nightly both attach to main's SHA, so the best outcome per name wins. resolve built the manifest URL from `channels.manifest_base`. channels.json is written by the promotion credential, whose stated invariant is that it cannot write artifacts; sourcing the manifest location from inside it hands over exactly that power, and a `kind: authenticode` entry then skips client signature verification on every platform. Resolution now uses the client's own production base, promote re-asserts the field on every write rather than only at creation, and the docs state it is informational. probe-store could not fail on the property it gates. Its only If-Match coverage asserted that a successful CAS update succeeded, which a store ignoring the precondition also does. A stale-ETag write is now attempted and must be refused. Note this may legitimately FAIL against the deployed MinIO, which is the point: publication must not be enabled until it passes. Also: publish verifies detached signatures, which are immutable once written but carry no manifest digest and so were never read back; artifacts stay compared against the digest the MANIFEST records rather than one recomputed from the bytes just sent, which would be self-consistent by construction. key_from_url takes the last bucket segment, since a base URL containing the bucket name yielded a doubled prefix that surfaced only after the artifacts were immutable. assert_monotonic parses the candidate version before the comparison loop, which an empty published set skipped entirely, permanently bricking future publishes. healthcheck distinguishes a definite 404 from a transport failure via a typed status error rather than matching on message text, and treats only 404 as absent: 403 against an anonymously readable bucket is a broken read policy, and calling it healthy let the workflow close its own tracking issue during an outage. license_macos is omitted when absent rather than serialized as null, which jq -r renders as the string "null". sign-linux feeds the passphrase over --passphrase-fd 0 instead of the argv, never echoes gpg's arguments into an error, and writes stdin concurrently with draining stdout so a full pipe buffer cannot deadlock the signing job. 15 new tests, 339 passing. The check-run gate is extracted to a pure function so the still-running, satisfied-by-itself and unrelated-failure cases are covered directly. Signed-off-by: Loïs Postula --- src/commands/release/bundle_linux.rs | 104 ++++++++- src/commands/release/healthcheck.rs | 179 +++++++++++---- src/commands/release/http.rs | 19 +- src/commands/release/probe_store.rs | 65 +++++- src/commands/release/promote.rs | 5 + src/commands/release/publish.rs | 24 +- src/commands/release/resolve.rs | 24 +- src/commands/release/sign_linux.rs | 112 ++++++--- src/commands/release/store.rs | 66 +++++- src/commands/release/types.rs | 31 ++- src/commands/release/verify_production.rs | 262 +++++++++++++++++++--- 11 files changed, 762 insertions(+), 129 deletions(-) diff --git a/src/commands/release/bundle_linux.rs b/src/commands/release/bundle_linux.rs index 30b4d60de..866cea45c 100644 --- a/src/commands/release/bundle_linux.rs +++ b/src/commands/release/bundle_linux.rs @@ -181,6 +181,32 @@ async fn download(url: &str) -> anyhow::Result> { super::http::get_bytes(&super::http::client()?, url).await } +/// The one .deb in `dir`. More than one is a hard failure rather than a choice: +/// picking by sort order would silently ship a stale .deb from an earlier +/// version, or, in cargo-deb's untripled default directory, another package's. +fn select_single_deb(dir: &Path) -> anyhow::Result { + let mut debs: Vec = std::fs::read_dir(dir) + .with_context(|| format!("cannot list {}", dir.display()))? + .filter_map(Result::ok) + .map(|entry| entry.path()) + .filter(|path| path.extension().is_some_and(|ext| ext == "deb")) + .collect(); + debs.sort(); + match debs.len() { + 1 => Ok(debs.remove(0)), + 0 => bail!("cargo deb produced no .deb under {}", dir.display()), + n => bail!( + "expected exactly one .deb under {}, found {n}: {}. \ + Refusing to guess which one to ship.", + dir.display(), + debs.iter() + .map(|p| p.file_name().unwrap_or_default().to_string_lossy()) + .collect::>() + .join(", ") + ), + } +} + fn make_executable(path: &Path) -> anyhow::Result<()> { std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)) .with_context(|| format!("cannot chmod {}", path.display())) @@ -247,6 +273,35 @@ pub async fn run( } // Package the .deb from the already-built binary. + // + // `-o` is not optional. cargo-deb's default output directory is + // `target_dir_base.join("debian")` with NO target triple (config.rs + // `default_deb_output_dir`; the triple is appended only by + // `target_dependent_path`, which the deb output does not go through), so + // `--target` does NOT move the artifact into a per-triple directory. + // Reading a tripled path without passing `-o` is an unconditional ENOENT. + // The per-triple directory is chosen over cargo-deb's untripled default + // because that default is shared by every package in the workspace, and the + // selection below refuses to choose between multiple .debs. + // + // Cleared, not just created: the target dir survives between runs on the + // persistent build pool, and a .deb left by an earlier version would now be + // a hard failure rather than something sort order silently resolved. The + // AppImage path below clears its own directory for the same reason. + let debian_dir = target_dir.join(TARGET_LINUX).join("debian"); + if debian_dir.exists() { + for entry in std::fs::read_dir(&debian_dir) + .with_context(|| format!("cannot list {}", debian_dir.display()))? + .filter_map(Result::ok) + .map(|entry| entry.path()) + .filter(|path| path.extension().is_some_and(|ext| ext == "deb")) + { + std::fs::remove_file(&entry) + .with_context(|| format!("cannot remove the stale {}", entry.display()))?; + } + } + std::fs::create_dir_all(&debian_dir) + .with_context(|| format!("cannot create {}", debian_dir.display()))?; run_step( "cargo", &[ @@ -258,22 +313,16 @@ pub async fn run( TARGET_LINUX, "--deb-version", &options.version, + // An existing directory puts the .deb inside it under cargo-deb's + // own generated filename. + "-o", + &debian_dir.to_string_lossy(), ], &package_dir, &[], ) .await?; - let debian_dir = target_dir.join(TARGET_LINUX).join("debian"); - let mut debs: Vec = std::fs::read_dir(&debian_dir) - .with_context(|| format!("cannot list {}", debian_dir.display()))? - .filter_map(Result::ok) - .map(|entry| entry.path()) - .filter(|path| path.extension().is_some_and(|ext| ext == "deb")) - .collect(); - debs.sort(); - let Some(deb) = debs.into_iter().next() else { - bail!("cargo deb produced no .deb under {}", debian_dir.display()); - }; + let deb = select_single_deb(&debian_dir)?; // Assemble the AppDir: binary, desktop entry, icon, AppRun. Nothing else. let out = target_dir.join(TARGET_LINUX).join("appimage"); @@ -438,6 +487,39 @@ DYNAMIC SYMBOL TABLE: assert!(exceeds_floor("x.y", "2.31").is_err()); } + #[test] + fn exactly_one_deb_is_required() { + let dir = tempfile::tempdir().unwrap(); + // None: cargo-deb wrote nowhere we looked. This is the shape the + // missing -o produced, except there the directory did not exist at all. + let err = select_single_deb(dir.path()).unwrap_err().to_string(); + assert!(err.contains("produced no .deb"), "{err}"); + + std::fs::write(dir.path().join("app_1.2.3_amd64.deb"), "x").unwrap(); + assert_eq!( + select_single_deb(dir.path()).unwrap().file_name().unwrap(), + "app_1.2.3_amd64.deb" + ); + + // Two: a stale build, or another package sharing cargo-deb's untripled + // default directory. Sort order would have shipped 1.2.3 as 1.2.4. + std::fs::write(dir.path().join("app_1.2.4_amd64.deb"), "x").unwrap(); + let err = select_single_deb(dir.path()).unwrap_err().to_string(); + assert!(err.contains("found 2"), "{err}"); + assert!(err.contains("Refusing to guess"), "{err}"); + assert!(err.contains("app_1.2.3_amd64.deb"), "names both: {err}"); + assert!(err.contains("app_1.2.4_amd64.deb"), "names both: {err}"); + } + + #[test] + fn a_missing_directory_is_an_error_not_an_empty_list() { + let dir = tempfile::tempdir().unwrap(); + let err = select_single_deb(&dir.path().join("nope")) + .unwrap_err() + .to_string(); + assert!(err.contains("cannot list"), "{err}"); + } + #[test] fn desktop_icon_line_is_parsed() { assert_eq!( diff --git a/src/commands/release/healthcheck.rs b/src/commands/release/healthcheck.rs index 181330400..f7731f6ef 100644 --- a/src/commands/release/healthcheck.rs +++ b/src/commands/release/healthcheck.rs @@ -2,15 +2,17 @@ //! credential-less, exactly as a client would: fetch channels.json, //! deserialize it against the contract types (stronger than schema //! validation - these ARE the schema), follow every channel/target pointer -//! to its manifest, confirm the index lists the version, HEAD every artifact -//! and detached signature, and download + digest-verify the artifacts -//! pointers actually expose (pointed versions only; full-history sweeps -//! would move hundreds of megabytes per run). +//! to its manifest, confirm the index lists the version, and download + +//! digest-verify the artifacts pointers actually expose (pointed versions +//! only; full-history sweeps would move hundreds of megabytes per run). +//! Detached signatures are HEADed for presence; in `--skip-digest-verification` +//! mode artifacts are HEADed too instead of downloaded. //! -//! A missing channels.json is "nothing promoted yet", which is healthy. An -//! index gap is reported as repairable. Problems make the command exit -//! nonzero with the report; the workflow maintains the single tracking -//! issue. +//! A missing channels.json is "nothing promoted yet", which is healthy, but +//! ONLY on a definite 404: anything else means we could not find out, and +//! reporting that as healthy would let the workflow close its own tracking +//! issue during an outage. An index gap is reported as repairable. Problems +//! make the command exit nonzero with the report. use std::collections::BTreeSet; use std::fmt::{Display, Formatter}; @@ -79,15 +81,27 @@ impl PrettyPrintable for HealthcheckResult { use super::http::{Client, client as https_client, head_present}; -/// GET collapsing every failure to `None`: a non-2xx status, an unreachable -/// host, or an unreadable body are all the same answer a client would -/// experience. -async fn get_bytes(client: &Client, url: &str) -> Option> { - super::http::get_bytes(client, url).await.ok() -} - -async fn head_ok(client: &Client, url: &str) -> bool { - head_present(client, url).await +/// `Ok(None)` means the object genuinely is not there (404). An `Err` means we +/// could not find out: a transport failure, DNS, TLS, 5xx. +/// +/// The distinction is the difference between "nothing promoted yet, healthy" +/// and "the store is down". Collapsing both to `None` made a full store outage +/// report every app as checked-and-healthy, and the workflow then CLOSED the +/// tracking issue with "Healthy again". +async fn get_bytes(client: &Client, url: &str) -> anyhow::Result>> { + match super::http::get_bytes(client, url).await { + Ok(bytes) => Ok(Some(bytes)), + // ONLY 404. 403 is deliberately not treated as absent: this check is + // credential-less against an anonymously readable bucket, so a 403 means + // the read policy is broken, not that the object is missing. Calling + // that "nothing promoted yet" would report a whole store as healthy and + // let the workflow close its own tracking issue. The two mistakes are + // not equally cheap, so anything other than a definite 404 is a problem. + Err(e) => match e.downcast_ref::() { + Some(status) if status.status == hyper::StatusCode::NOT_FOUND => Ok(None), + _ => Err(e), + }, + } } pub async fn run(options: &Options) -> anyhow::Result { @@ -98,10 +112,18 @@ pub async fn run(options: &Options) -> anyhow::Result { for app in &options.apps { let channels_url = format!("{base}/{}/{app}/channels.json", options.channels_bucket); - let Some(channels_bytes) = get_bytes(&client, &channels_url).await else { - // Healthy: nothing has been promoted for this app yet. - checked.push(format!("{app}: no channels.json yet; nothing promoted")); - continue; + let channels_bytes = match get_bytes(&client, &channels_url).await { + Ok(Some(bytes)) => bytes, + Ok(None) => { + // Healthy: nothing has been promoted for this app yet. + checked.push(format!("{app}: no channels.json yet; nothing promoted")); + continue; + } + Err(e) => { + // Could not determine. Never report this as healthy. + problems.push(format!("{app}: cannot reach {channels_url}: {e:#}")); + continue; + } }; let channels: Channels = match serde_json::from_slice(&channels_bytes) { Ok(channels) => channels, @@ -121,13 +143,17 @@ pub async fn run(options: &Options) -> anyhow::Result { let index_url = format!("{base}/{}/{app}/index.json", options.prod_bucket); let index: Option = match get_bytes(&client, &index_url).await { - None => { + Ok(None) => { problems.push(format!( "{app}: channels exist but index.json is missing at {index_url}" )); None } - Some(bytes) => match serde_json::from_slice::(&bytes) { + Err(e) => { + problems.push(format!("{app}: cannot reach {index_url}: {e:#}")); + None + } + Ok(Some(bytes)) => match serde_json::from_slice::(&bytes) { Ok(index) => { if index.schema_version != SCHEMA_VERSION { problems.push(format!( @@ -162,11 +188,18 @@ pub async fn run(options: &Options) -> anyhow::Result { "{base}/{}/{app}/{version}/manifest.json", options.prod_bucket ); - let Some(manifest_bytes) = get_bytes(&client, &manifest_url).await else { - problems.push(format!( - "{app}: a channel points at {version} but {manifest_url} is missing" - )); - continue; + let manifest_bytes = match get_bytes(&client, &manifest_url).await { + Ok(Some(bytes)) => bytes, + Ok(None) => { + problems.push(format!( + "{app}: a channel points at {version} but {manifest_url} is missing" + )); + continue; + } + Err(e) => { + problems.push(format!("{app}: cannot reach {manifest_url}: {e:#}")); + continue; + } }; let manifest: Manifest = match serde_json::from_slice(&manifest_bytes) { Ok(manifest) => manifest, @@ -193,18 +226,38 @@ pub async fn run(options: &Options) -> anyhow::Result { } for artifact in &manifest.artifacts { - if !head_ok(&client, &artifact.url).await { - problems.push(format!( - "{app} {version}: artifact missing: {}", - artifact.url - )); - continue; - } - if !options.skip_digest_verification { - // Pointed versions are what clients download; verify the bytes. - let Some(bytes) = get_bytes(&client, &artifact.url).await else { - problems.push(format!("{app} {version}: cannot download {}", artifact.url)); + if options.skip_digest_verification { + // Fast mode: presence only. + if !head_present(&client, &artifact.url).await { + problems.push(format!( + "{app} {version}: artifact missing: {}", + artifact.url + )); continue; + } + } else { + // The GET proves presence by itself, so no HEAD first: it was + // a second round trip against the identical URL for every + // artifact of every pointed version. + let bytes = match get_bytes(&client, &artifact.url).await { + Ok(Some(bytes)) => bytes, + Ok(None) => { + problems.push(format!( + "{app} {version}: artifact missing: {}", + artifact.url + )); + continue; + } + Err(e) => { + // Distinct from missing, and the reason matters: this + // is the branch that tells an outage from a broken + // publication. + problems.push(format!( + "{app} {version}: cannot reach {}: {e:#}", + artifact.url + )); + continue; + } }; let got = sha256_hex(&bytes); if got != artifact.sha256 { @@ -215,7 +268,7 @@ pub async fn run(options: &Options) -> anyhow::Result { } } if let ArtifactSignature::OpenpgpDetached { url: sig_url, .. } = &artifact.signature - && !head_ok(&client, sig_url).await + && !head_present(&client, sig_url).await { problems.push(format!( "{app} {version}: detached signature missing: {sig_url}" @@ -238,3 +291,49 @@ pub async fn run(options: &Options) -> anyhow::Result { Err(anyhow!("{result}")) } } + +#[cfg(test)] +mod tests { + use super::super::http::HttpStatus; + + /// The classification `get_bytes` performs, exercised on the error type + /// rather than through the network. A 404 is "absent, healthy"; everything + /// else must stay an error, because reporting an outage as healthy lets the + /// workflow close its own tracking issue while no client can resolve. + fn classifies_as_absent(status: hyper::StatusCode) -> bool { + let err: anyhow::Error = HttpStatus { + url: "https://x/y.json".into(), + status, + } + .into(); + matches!( + err.downcast_ref::(), + Some(s) if s.status == hyper::StatusCode::NOT_FOUND + ) + } + + #[test] + fn only_404_means_absent() { + assert!(classifies_as_absent(hyper::StatusCode::NOT_FOUND)); + // 403 in particular: this check is credential-less against an + // anonymously readable bucket, so a 403 is a broken read policy, not a + // missing object. Treating it as absent reported a whole store outage + // as "nothing promoted yet; healthy". + for status in [ + hyper::StatusCode::FORBIDDEN, + hyper::StatusCode::UNAUTHORIZED, + hyper::StatusCode::INTERNAL_SERVER_ERROR, + hyper::StatusCode::SERVICE_UNAVAILABLE, + hyper::StatusCode::BAD_GATEWAY, + ] { + assert!(!classifies_as_absent(status), "{status} read as absent"); + } + } + + #[test] + fn a_transport_error_is_never_absent() { + // No HttpStatus in the chain at all: DNS, TLS, connection reset. + let err = anyhow::anyhow!("connection reset by peer"); + assert!(err.downcast_ref::().is_none()); + } +} diff --git a/src/commands/release/http.rs b/src/commands/release/http.rs index cce2bd085..758408adb 100644 --- a/src/commands/release/http.rs +++ b/src/commands/release/http.rs @@ -22,6 +22,19 @@ pub(crate) type Client = HyperClient, Full> pub(crate) const MAX_REDIRECTS: usize = 5; +/// A non-2xx response, carrying the status so callers can branch on it. +/// +/// Detect with `err.downcast_ref::()`. Callers that must tell "the +/// object is not there" from "we could not find out" need the code itself: a +/// substring match on the message silently changes meaning the next time the +/// wording moves, and the two answers are not equally safe to guess at. +#[derive(Debug, thiserror::Error)] +#[error("GET {url} returned {status}")] +pub(crate) struct HttpStatus { + pub(crate) url: String, + pub(crate) status: StatusCode, +} + pub(crate) fn client() -> anyhow::Result { let _ = rustls::crypto::ring::default_provider().install_default(); let tls_config = rustls::ClientConfig::builder() @@ -93,7 +106,11 @@ async fn get_response( continue; } if !res.status().is_success() { - bail!("GET {current} returned {}", res.status()); + return Err(HttpStatus { + url: current.to_string(), + status: res.status(), + } + .into()); } return Ok(res); } diff --git a/src/commands/release/probe_store.rs b/src/commands/release/probe_store.rs index 9c0e0a1f2..a03c193e1 100644 --- a/src/commands/release/probe_store.rs +++ b/src/commands/release/probe_store.rs @@ -28,6 +28,12 @@ pub struct Options { pub prefix: String, } +/// An ETag that cannot match any stored object. The quotes are part of the +/// header value, not Rust syntax noise: an unquoted ETag is a malformed +/// If-Match and the store would reject it for the wrong reason, which the probe +/// would then report as a missing conditional-write feature. +const STALE_ETAG: &str = "\"00000000000000000000000000000000\""; + #[derive(Debug, Serialize, Clone)] pub struct ProbeCase { pub description: String, @@ -147,12 +153,63 @@ pub async fn run(options: &Options) -> anyhow::Result { cas.err().map(|e| format!("{e:#}")), ); - let readback = store.read(&key).await?; + // The decisive case. Everything above passes on a store that accepts + // If-Match but silently IGNORES it, because ignoring a precondition still + // yields a successful write. Lost-update protection on index.json and + // channels.json, and therefore promote's backward-move gate, rest entirely + // on the store refusing a write whose ETag no longer matches. + let refused_stale = store + .refuses_stale_if_match(&cas_key, STALE_ETAG, b"probe-body-stale".to_vec()) + .await; + let stale_ok = matches!(refused_stale, Ok(true)); case( - "refused overwrite left the original bytes", - readback == body_one, - None, + "If-Match with a stale ETag is refused", + stale_ok, + match &refused_stale { + Ok(true) => None, + Ok(false) => Some( + "the write SUCCEEDED against a stale ETag: this store does not enforce If-Match" + .into(), + ), + Err(e) => Some(format!("{e:#}")), + }, ); + // Only meaningful once the CAS update actually landed "updated"; if case 3 + // failed the document still reads "seed" and reporting that as a clobber + // would blame the store for a write it never made. + if cas_ok { + // A store that ignored the precondition also clobbered the document, so + // prove the bytes are intact independently of the error it reported. + match store.read(&cas_key).await { + Ok(bytes) => case( + "refused stale-ETag write left the document unchanged", + serde_json::from_slice::(&bytes) + .map(|v| v["probe"] == "updated") + .unwrap_or(false), + None, + ), + // Never `?`: the whole point of this command is to print a verdict, + // and propagating here would discard every case above it. + Err(e) => case( + "refused stale-ETag write left the document unchanged", + false, + Some(format!("could not re-read the document: {e:#}")), + ), + } + } + + match store.read(&key).await { + Ok(readback) => case( + "refused overwrite left the original bytes", + readback == body_one, + None, + ), + Err(e) => case( + "refused overwrite left the original bytes", + false, + Some(format!("could not re-read the object: {e:#}")), + ), + } let passed = cases.iter().all(|c| c.passed); Ok(ProbeStoreResult { diff --git a/src/commands/release/promote.rs b/src/commands/release/promote.rs index 02b8da45d..37541638f 100644 --- a/src/commands/release/promote.rs +++ b/src/commands/release/promote.rs @@ -142,6 +142,11 @@ pub fn apply_move( pointers.insert(target.clone(), options.version.clone()); } doc.updated_at = now.to_string(); + // Re-assert manifest_base on every write. It was previously set only in the + // initial document, so a value poisoned once stayed poisoned for the life of + // the file. Resolution no longer trusts this field, but it is published and + // read by humans, so it must not be left pointing somewhere else. + doc.manifest_base = format!("{}/{}", options.base_url, options.releases_bucket); doc.provenance.insert( 0, ProvenanceEntry { diff --git a/src/commands/release/publish.rs b/src/commands/release/publish.rs index 11bc098cd..a1c496b93 100644 --- a/src/commands/release/publish.rs +++ b/src/commands/release/publish.rs @@ -436,18 +436,38 @@ pub async fn run(options: &Options) -> anyhow::Result { }; let mut written = Vec::new(); + // Detached signatures are uploaded but are not manifest artifacts, so the + // manifest carries no digest for them. Record what we sent, purely so they + // can be read back below. + let mut signature_digests = Vec::new(); for (path, key) in &uploads { let bytes = std::fs::read(path).with_context(|| format!("cannot read {}", path.display()))?; + if !digests.contains_key(path) { + signature_digests.push((key.clone(), sha256_hex(&bytes))); + } store.put_immutable(key, bytes).await?; written.push(key.clone()); } - // Read back and digest-compare every manifest artifact while no manifest - // exists yet, so nothing broken can become published-and-immutable. + // Read back and digest-compare while no manifest exists yet, so nothing + // broken can become published-and-immutable. + // + // Manifest artifacts are compared against the digest THE MANIFEST RECORDS, + // not against a digest recomputed from the bytes we happened to send. The + // latter is self-consistent by construction and would pass even if the file + // changed between the digesting read and the upload read, committing a + // manifest whose recorded digest no object matches. Going through + // `key_from_url` also keeps that mapping exercised here rather than only in + // promote. for artifact in &manifest.artifacts { let key = store.key_from_url(&artifact.url)?; store.verify_key(key, &artifact.sha256).await?; } + // The signatures, which the loop above cannot cover: a truncated .asc would + // otherwise ship unverified and fail every client verification permanently. + for (key, want) in &signature_digests { + store.verify_key(key, want).await?; + } // The manifest is written LAST: its existence is the atomic commit // point. An AlreadyExists from the store propagates as-is; that version diff --git a/src/commands/release/resolve.rs b/src/commands/release/resolve.rs index 94c4f6dee..5d125a0fa 100644 --- a/src/commands/release/resolve.rs +++ b/src/commands/release/resolve.rs @@ -218,11 +218,25 @@ pub async fn run(options: &Options) -> anyhow::Result { ); }; - // Step 2: the manifest, from the base the channels document names. - let manifest_url = format!( - "{}/{}/{}/manifest.json", - channels.manifest_base, options.app, version - ); + // Step 2: the manifest, from the CLIENT'S OWN production base, never from + // `channels.manifest_base`. + // + // channels.json is written by the promotion credential, whose whole point + // is that it cannot write artifacts. Building the manifest URL from a field + // inside that document hands it exactly that power: point manifest_base at + // any reachable host and every digest check downstream is satisfied against + // the attacker's own manifest, while a `"kind": "authenticode"` entry skips + // client signature verification altogether. manifest_base stays in the + // contract as informational only. + let manifest_base = format!("{}/{}", options.base_url, options.prod_bucket); + if channels.manifest_base != manifest_base { + tracing::warn!( + "channels.json manifest_base is {} but resolution uses {manifest_base}; \ + the field is informational and is not trusted for resolution", + channels.manifest_base + ); + } + let manifest_url = format!("{manifest_base}/{}/{}/manifest.json", options.app, version); let bytes = http_get(&manifest_url) .await .with_context(|| format!("cannot fetch {manifest_url}"))?; diff --git a/src/commands/release/sign_linux.rs b/src/commands/release/sign_linux.rs index 41cc44903..9049c4425 100644 --- a/src/commands/release/sign_linux.rs +++ b/src/commands/release/sign_linux.rs @@ -73,17 +73,59 @@ fn parse_secret_key_listing(listing: &str) -> anyhow::Result<(String, String)> { Ok((key_id, fingerprint)) } -async fn gpg(gnupg_home: &Path, args: &[&str]) -> anyhow::Result { - let output = tokio::process::Command::new("gpg") +/// Run gpg with `stdin` fed to it. `label` is what appears in an error message: +/// the argv is NEVER echoed, because the signing invocation carries the +/// passphrase and a failure would otherwise print it into the job log. It is +/// masked there only by GitHub's secret filter, which does not apply to a log +/// read anywhere else. +async fn gpg_with_stdin( + gnupg_home: &Path, + label: &str, + args: &[&str], + stdin_data: Option<&[u8]>, +) -> anyhow::Result { + let mut command = tokio::process::Command::new("gpg"); + command .args(args) .env("GNUPGHOME", gnupg_home) - .output() - .await - .context("failed to run gpg")?; + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(if stdin_data.is_some() { + Stdio::piped() + } else { + Stdio::null() + }); + let mut child = command + .spawn() + .with_context(|| format!("failed to run gpg ({label})"))?; + // The write and the drain run concurrently. Completing the stdin write + // first would deadlock the moment gpg emits more than a pipe buffer of + // status output before reading all of its input: gpg blocks writing + // stderr, we block writing stdin, and the signing job hangs until the + // job timeout with nothing to show for it. + let mut stdin = match stdin_data { + Some(_) => Some( + child + .stdin + .take() + .with_context(|| format!("no stdin on the gpg {label}"))?, + ), + None => None, + }; + let feed = async { + if let (Some(stdin), Some(data)) = (stdin.as_mut(), stdin_data) { + stdin.write_all(data).await?; + stdin.shutdown().await?; + } + drop(stdin.take()); + Ok::<_, std::io::Error>(()) + }; + let (fed, output) = tokio::join!(feed, child.wait_with_output()); + fed.with_context(|| format!("failed to feed gpg {label} on stdin"))?; + let output = output?; if !output.status.success() { bail!( - "gpg {} failed with {}: {}", - args.join(" "), + "gpg {label} failed with {}: {}", output.status, String::from_utf8_lossy(&output.stderr) ); @@ -91,6 +133,14 @@ async fn gpg(gnupg_home: &Path, args: &[&str]) -> anyhow::Result anyhow::Result { + gpg_with_stdin(gnupg_home, label, args, None).await +} + pub async fn run(options: &Options) -> anyhow::Result { // Job-scoped GNUPGHOME: created 0700, wiped when the TempDir drops. let gnupg_home = tempfile::TempDir::new().context("cannot create a temporary GNUPGHOME")?; @@ -99,28 +149,22 @@ pub async fn run(options: &Options) -> anyhow::Result { // Import the key via stdin so it never touches the filesystem outside // the GNUPGHOME. - let mut import = tokio::process::Command::new("gpg") - .args(["--batch", "--quiet", "--import"]) - .env("GNUPGHOME", home) - .stdin(Stdio::piped()) - .stderr(Stdio::piped()) - .stdout(Stdio::piped()) - .spawn() - .context("failed to spawn gpg for the key import")?; - let mut stdin = import.stdin.take().context("no stdin on the gpg import")?; - stdin.write_all(options.key.as_bytes()).await?; - stdin.write_all(b"\n").await?; - drop(stdin); - let import = import.wait_with_output().await?; - if !import.status.success() { - bail!( - "gpg --import failed with {}: {}", - import.status, - String::from_utf8_lossy(&import.stderr) - ); - } + let mut key = options.key.clone().into_bytes(); + key.push(b'\n'); + gpg_with_stdin( + home, + "--import", + &["--batch", "--quiet", "--import"], + Some(&key), + ) + .await?; - let listing = gpg(home, &["--list-secret-keys", "--with-colons"]).await?; + let listing = gpg( + home, + "--list-secret-keys", + &["--list-secret-keys", "--with-colons"], + ) + .await?; let (key_id, fingerprint) = parse_secret_key_listing(&String::from_utf8_lossy(&listing.stdout))?; @@ -146,15 +190,18 @@ pub async fn run(options: &Options) -> anyhow::Result { for artifact in &artifacts { let artifact_str = artifact.to_string_lossy().into_owned(); let signature = format!("{artifact_str}.asc"); - gpg( + // The passphrase goes over stdin via --passphrase-fd 0, never on the + // argv, where every process on the runner could read it out of /proc. + gpg_with_stdin( home, + "--detach-sign", &[ "--batch", "--yes", "--pinentry-mode", "loopback", - "--passphrase", - &options.passphrase, + "--passphrase-fd", + "0", "--local-user", &key_id, "--armor", @@ -163,11 +210,12 @@ pub async fn run(options: &Options) -> anyhow::Result { &signature, &artifact_str, ], + Some(options.passphrase.as_bytes()), ) .await .with_context(|| format!("signing {} failed", artifact.display()))?; // Self-verify before anything downstream trusts the .asc. - gpg(home, &["--verify", &signature, &artifact_str]) + gpg(home, "--verify", &["--verify", &signature, &artifact_str]) .await .with_context(|| format!("self-verification failed for {}", artifact.display()))?; signed.push( diff --git a/src/commands/release/store.rs b/src/commands/release/store.rs index 73e20c2c0..97f158c36 100644 --- a/src/commands/release/store.rs +++ b/src/commands/release/store.rs @@ -178,6 +178,12 @@ impl ReleaseStore { /// version forever. A version prefix with artifacts but no manifest is /// invisible, because the manifest is the commit point. pub async fn assert_monotonic(&self, app: &str, version: &str) -> anyhow::Result> { + // Parse the candidate BEFORE the comparison loop. Otherwise the very + // first publication of an app skips validation entirely (the loop body + // never runs), so an unparseable version commits once and then refuses + // every subsequent production publish forever, because `semver_gt` + // errors on the stored value. + Version::parse(version).with_context(|| format!("unparseable version: {version}"))?; let mut published = Vec::new(); let entries = self .op @@ -206,6 +212,28 @@ impl ReleaseStore { Ok(published) } + /// Attempt a write with a deliberately stale ETag. Returns `Ok(true)` when + /// the store REFUSED it, which is the behaviour publication depends on. + /// + /// Exists solely for `probe-store`: the positive CAS path cannot detect a + /// store that ignores `If-Match` on PUT, because ignoring the precondition + /// still produces a successful update. Without this, the probe reports + /// conditional writes as supported while lost-update protection on + /// index.json and channels.json is silently absent. + pub async fn refuses_stale_if_match( + &self, + key: &str, + stale_etag: &str, + data: Vec, + ) -> anyhow::Result { + match self.op.write_with(key, data).if_match(stale_etag).await { + Ok(_) => Ok(false), + Err(e) if e.kind() == ErrorKind::ConditionNotMatch => Ok(true), + Err(e) => Err(e) + .with_context(|| format!("stale-if-match probe on s3://{}/{key}", self.bucket)), + } + } + /// Re-read one object and compare its digest. Used between the artifact /// writes and the manifest write, before any manifest exists. pub async fn verify_key(&self, key: &str, want_sha256: &str) -> anyhow::Result<()> { @@ -220,7 +248,11 @@ impl ReleaseStore { /// Turn a manifest artifact URL back into this bucket's object key. pub fn key_from_url<'a>(&self, url: &'a str) -> anyhow::Result<&'a str> { let marker = format!("/{}/", self.bucket); - match url.find(&marker) { + // Last occurrence, not first: a public base URL that itself contains the + // bucket name yields a doubled marker, and taking the leftmost match + // returns a key with the bucket prefixed. That surfaces as a read-back + // failure at publish time, AFTER every artifact is already immutable. + match url.rfind(&marker) { Some(idx) => Ok(&url[idx + marker.len()..]), None => bail!( "artifact url {url} does not reference bucket {}", @@ -234,6 +266,38 @@ impl ReleaseStore { mod tests { use super::*; + fn store(bucket: &str) -> ReleaseStore { + // No I/O: key_from_url is pure string work over the bucket name. + ReleaseStore::new( + Operator::new(opendal::services::Memory::default()) + .unwrap() + .finish(), + bucket, + ) + } + + #[test] + fn key_from_url_takes_the_last_bucket_segment() { + let s = store("fsl-releases"); + assert_eq!( + s.key_from_url("https://api.s3.fsl.dev/fsl-releases/app/1.0.0/a.deb") + .unwrap(), + "app/1.0.0/a.deb" + ); + // A public base URL that itself contains the bucket name. Taking the + // FIRST match returned "fsl-releases/app/1.0.0/a.deb", which only + // surfaced as a read-back failure after every artifact was immutable. + assert_eq!( + s.key_from_url("https://api.s3.fsl.dev/fsl-releases/fsl-releases/app/1.0.0/a.deb") + .unwrap(), + "app/1.0.0/a.deb" + ); + assert!( + s.key_from_url("https://api.s3.fsl.dev/other/app/a.deb") + .is_err() + ); + } + #[test] fn semver_ordering_is_real_precedence() { assert!(semver_gt("1.10.0", "1.9.9").unwrap()); diff --git a/src/commands/release/types.rs b/src/commands/release/types.rs index 257e474a8..e17cada15 100644 --- a/src/commands/release/types.rs +++ b/src/commands/release/types.rs @@ -181,8 +181,10 @@ pub struct AppReleaseConfig { pub verbose_name: String, /// Target triples this application ships for. pub targets: Vec, - /// Path to the macOS EULA, relative to the package directory. - #[serde(default)] + /// Path to the macOS EULA, relative to the package directory. Omitted + /// entirely when absent: `jq -r` renders a JSON null as the literal string + /// "null", which the macOS leg would pass to create-dmg as `--eula /null`. + #[serde(default, skip_serializing_if = "Option::is_none")] pub license_macos: Option, } @@ -203,6 +205,31 @@ mod tests { assert_eq!(plain["kind"], "authenticode"); } + #[test] + fn absent_license_macos_is_omitted_not_null() { + // `jq -r .config.license_macos` renders a JSON null as the literal + // string "null", which the macOS leg passed to create-dmg as + // `--eula /null`. The key must not be present at all. + let config = AppReleaseConfig { + verbose_name: "App".into(), + targets: vec![TARGET_LINUX.into()], + license_macos: None, + }; + let v = serde_json::to_value(&config).unwrap(); + assert!( + v.get("license_macos").is_none(), + "serialized as {v}, which jq -r renders as the string \"null\"" + ); + let with = AppReleaseConfig { + license_macos: Some("eula.rtf".into()), + ..config + }; + assert_eq!( + serde_json::to_value(&with).unwrap()["license_macos"], + "eula.rtf" + ); + } + #[test] fn channels_roundtrip_preserves_unknown_free_shape() { let json = serde_json::json!({ diff --git a/src/commands/release/verify_production.rs b/src/commands/release/verify_production.rs index 8c2c68f26..a1e7a46ae 100644 --- a/src/commands/release/verify_production.rs +++ b/src/commands/release/verify_production.rs @@ -44,6 +44,20 @@ pub struct Options { /// Skip the check-run gate (only for environments with no API access). #[arg(long, default_value_t = false)] pub skip_check_runs: bool, + /// Check-run names that must have completed successfully on the tagged + /// commit. Required unless --skip-check-runs: see the gate below for why a + /// count of check runs cannot substitute for named ones. + #[arg(long, value_delimiter = ',')] + pub required_checks: Vec, +} + +/// Ordered worst-to-best so `max` lets a successful re-run supersede an earlier +/// failure of the same check name. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum CheckState { + Failed, + Pending, + Passed, } #[derive(Debug, Serialize, Clone)] @@ -73,6 +87,75 @@ impl PrettyPrintable for VerifyProductionResult { } } +/// Clear a commit for production against NAMED check runs. +/// +/// Only the named ones count. A count-based rule is satisfied by this command's +/// own check run, which is attached to the commit under test, and a commit on +/// main routinely carries failures and cancellations from unrelated workflows. +/// A required check whose `status` is not yet `completed` is not a pass: +/// `conclusion` is null until then. The same name can arrive more than once +/// (postsubmit and nightly both attach to main), so the best outcome per name +/// wins. +fn evaluate_check_runs(runs: &[serde_json::Value], required: &[String]) -> anyhow::Result<()> { + // Trimmed and emptied-out first: the value arrives from a repo variable, and + // `RELEASE_REQUIRED_CHECKS` written as "test, clippy" would otherwise look + // for a check named " clippy" and report it missing with an error visually + // identical to the name that did run. Normalising before the emptiness guard + // also stops a value of "," from passing the gate vacuously. + let required: Vec<&str> = required + .iter() + .map(|name| name.trim()) + .filter(|name| !name.is_empty()) + .collect(); + if required.is_empty() { + bail!( + "--required-checks must name at least one check run, or pass --skip-check-runs. \ + A bare count of check runs cannot gate anything: this command runs inside a job \ + whose own check run is attached to the same commit, so the count is never zero." + ); + } + // name -> best state seen. + let mut seen: std::collections::BTreeMap = + std::collections::BTreeMap::new(); + for run in runs { + let name = run["name"].as_str().unwrap_or("?").to_string(); + let status = run["status"].as_str().unwrap_or(""); + let conclusion = run["conclusion"].as_str().unwrap_or(""); + let state = match (status, conclusion) { + ("completed", "success") => CheckState::Passed, + // `neutral` and `skipped` are not success. A required check that + // skipped itself did not verify anything. + ("completed", _) => CheckState::Failed, + _ => CheckState::Pending, + }; + seen.entry(name) + .and_modify(|best| *best = (*best).max(state)) + .or_insert(state); + } + let mut unmet = Vec::new(); + for name in &required { + match seen.get(*name) { + Some(CheckState::Passed) => {} + Some(CheckState::Pending) => unmet.push(format!("{name} (still running)")), + Some(CheckState::Failed) => unmet.push(format!("{name} (did not succeed)")), + None => unmet.push(format!("{name} (never ran on this commit)")), + } + } + if !unmet.is_empty() { + bail!( + "does not satisfy the required check run(s): {}. \ + Production releases ship verified main. Check runs seen: {}", + unmet.join(", "), + if seen.is_empty() { + "none".to_string() + } else { + seen.keys().cloned().collect::>().join(", ") + } + ); + } + Ok(()) +} + fn git(repo_root: &Path, args: &[&str]) -> anyhow::Result { Command::new("git") .arg("-C") @@ -156,8 +239,13 @@ pub async fn run(options: &Options, repo_root: PathBuf) -> anyhow::Result, so `all_pages` does not apply. + let mut runs: Vec = Vec::new(); let mut page = 1u32; loop { let response: serde_json::Value = octocrab @@ -167,42 +255,19 @@ pub async fn run(options: &Options, repo_root: PathBuf) -> anyhow::Result anyhow::Result serde_json::Value { + serde_json::json!({"name": name, "status": status, "conclusion": conclusion}) + } + + fn required(names: &[&str]) -> Vec { + names.iter().map(|n| n.to_string()).collect() + } + + #[test] + fn an_in_progress_required_check_is_not_a_pass() { + // The regression this gate exists for: `conclusion` is null while a + // check is queued or running, and the previous rule ("not in the bad + // list") read that as green. + for status in ["queued", "in_progress", "waiting", "pending"] { + let runs = [run("bazel", status, serde_json::Value::Null)]; + let err = evaluate_check_runs(&runs, &required(&["bazel"])) + .unwrap_err() + .to_string(); + assert!(err.contains("still running"), "status {status}: {err}"); + } + } + + #[test] + fn the_gate_is_not_satisfied_by_the_job_it_runs_inside() { + // Exactly the shape seen in CI: this workflow's own check run is + // attached to the tagged commit and is in_progress, and the required + // check never ran. A count-based rule passes here; this must not. + let runs = [run("facts", "in_progress", serde_json::Value::Null)]; + let err = evaluate_check_runs(&runs, &required(&["bazel"])) + .unwrap_err() + .to_string(); + assert!(err.contains("never ran on this commit"), "{err}"); + assert!(err.contains("facts"), "should name what it did see: {err}"); + } + + #[test] + fn completed_success_passes_and_unrelated_runs_are_ignored() { + let runs = [ + run("bazel", "completed", "success".into()), + run("facts", "in_progress", serde_json::Value::Null), + run("some-optional-linter", "completed", "skipped".into()), + ]; + assert!(evaluate_check_runs(&runs, &required(&["bazel"])).is_ok()); + } + + #[test] + fn unrelated_failures_do_not_block_the_release() { + // Measured shape of fsl_libs main at 91075455: 20 check runs, including + // a failed docker_build and cancellations from concurrency. Gating on + // any bad run anywhere would have refused every release. + let runs = [ + run("test", "completed", "success".into()), + run( + "docker_build (prod, spatialdrive)", + "completed", + "failure".into(), + ), + run( + "docker_build (dev, spatialdrive-dev)", + "completed", + "cancelled".into(), + ), + run("report", "completed", "skipped".into()), + run("facts", "in_progress", serde_json::Value::Null), + ]; + assert!(evaluate_check_runs(&runs, &required(&["test"])).is_ok()); + } + + #[test] + fn a_required_check_that_did_not_succeed_blocks_the_release() { + for conclusion in ["failure", "timed_out", "cancelled", "action_required"] { + let runs = [run("test", "completed", conclusion.into())]; + let err = evaluate_check_runs(&runs, &required(&["test"])) + .unwrap_err() + .to_string(); + assert!(err.contains("did not succeed"), "{conclusion}: {err}"); + } + } + + #[test] + fn a_skipped_required_check_is_not_a_pass() { + // A required check that skipped itself verified nothing. + for conclusion in ["skipped", "neutral"] { + let runs = [run("test", "completed", conclusion.into())]; + let err = evaluate_check_runs(&runs, &required(&["test"])) + .unwrap_err() + .to_string(); + assert!(err.contains("did not succeed"), "{conclusion}: {err}"); + } + } + + #[test] + fn a_successful_rerun_supersedes_an_earlier_failure() { + // Both the postsubmit and the nightly publish a check named `test` + // against main's SHA, so the same name arrives twice with different + // outcomes. Ordering must not decide the verdict. + for runs in [ + vec![ + run("test", "completed", "failure".into()), + run("test", "completed", "success".into()), + ], + vec![ + run("test", "completed", "success".into()), + run("test", "completed", "failure".into()), + ], + ] { + assert!( + evaluate_check_runs(&runs, &required(&["test"])).is_ok(), + "a success for the required name must win regardless of order" + ); + } + } + + #[test] + fn no_required_checks_is_refused_rather_than_vacuously_true() { + let runs = [run("bazel", "completed", "success".into())]; + let err = evaluate_check_runs(&runs, &[]).unwrap_err().to_string(); + assert!(err.contains("--required-checks"), "{err}"); + } + + #[test] + fn zero_check_runs_fails_every_required_name() { + let err = evaluate_check_runs(&[], &required(&["bazel", "clippy"])) + .unwrap_err() + .to_string(); + assert!(err.contains("bazel"), "{err}"); + assert!(err.contains("clippy"), "{err}"); + assert!(err.contains("none"), "{err}"); + } +}