From e845cb0828ddaad2c7ea58e099d76a02765c567d Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Wed, 12 Aug 2026 07:50:36 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20v0.15.1=20=E2=80=94=20self-update=20conv?= =?UTF-8?q?erges=20on=20artifact=20identity=20(varve#38)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit varve#38: the v0.14.0 binary reported 0.13.1, so `self-update --check` compared self-reported version strings (is_newer("v0.14.0","0.13.1") = true forever) and looped — every run re-installing identical bytes. REQ-UPDATE-002 — self-update now decides on ARTIFACT IDENTITY, not version strings. A new core `resolve_update` fetches and VERIFIES the candidate against the trust root, then compares it to what is on disk: identical bytes -> AlreadyCurrent (a no-op — the loop terminates), differing bytes -> Available, not-newer -> UpToDate. The CLI handler is a thin wrapper over it. So a mis-reported version string degrades to a no-op instead of an infinite loop. `perform` is refactored into `fetch_verified_binary` + `install_binary`, keeping verify-before-write intact. REQ-RELVER-001 strengthened — the release now also runs the freshly built binary and asserts `--version` == the tag (native targets), the exact artifact-level oracle varve#38 suggested, on top of the pre-build version-guard. Independently reviewed twice (VER-REVIEW-v0.15.1): the first review passed all refutations but DISSENTED that the handler behaviour was tested only by a pure helper; resolved by adding behavioural tests through the real release-API mock server (signed archive, real ed25519 verify) — loop-break and impostor-refused-before-offer both asserted end-to-end — and a second independent review confirmed the dissent closed. Residual handler-glue coverage tracked as a follow-up. 128 lib + 6 self-update-double + 34 CLI green; clippy/fmt clean; --locked synced; rivet validate + verification-evidence (106 steps) + claim-check green. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019TNtfRjLNhEz82G2ggeeNu --- .github/workflows/release.yml | 19 ++++ Cargo.lock | 4 +- Cargo.toml | 2 +- artifacts/requirements.yaml | 23 ++++ artifacts/verification.yaml | 54 +++++++++ crates/varve-core/src/update.rs | 111 +++++++++++++++++-- crates/varve-core/tests/selfupdate_double.rs | 52 ++++++++- crates/varve/src/main.rs | 70 +++++++++--- 8 files changed, 303 insertions(+), 32 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 78101a1..d0b8f9f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -102,6 +102,25 @@ jobs: if: matrix.cross run: cross build --release --locked --target ${{ matrix.target }} -p varve + - name: Assert built binary reports the tag version (REQ-RELVER-001) + # The artifact-level twin of the version-guard job: run the freshly + # built binary and confirm `--version` == the tag. This is the exact + # oracle varve#38 asked for — a binary that mis-reports its own version + # (as v0.14.0 did) fails the release here. Native targets only; a + # cross-built binary cannot be executed on the runner. + if: ${{ !matrix.cross }} + env: + INPUT_TAG: ${{ inputs.tag }} + run: | + set -euo pipefail + VERSION="${INPUT_TAG:-${GITHUB_REF#refs/tags/}}" + REPORTED=$("target/${{ matrix.target }}/release/varve" --version | awk '{print $2}') + echo "tag=${VERSION#v} binary --version=$REPORTED" + if [ "${VERSION#v}" != "$REPORTED" ]; then + echo "::error::built binary reports '$REPORTED' but the release tag is $VERSION" + exit 1 + fi + - name: Strip binary if: ${{ !matrix.cross }} run: strip "target/${{ matrix.target }}/release/varve" 2>/dev/null || true diff --git a/Cargo.lock b/Cargo.lock index e8f848b..00e369b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1787,7 +1787,7 @@ checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" [[package]] name = "varve" -version = "0.15.0" +version = "0.15.1" dependencies = [ "anyhow", "assert_cmd", @@ -1803,7 +1803,7 @@ dependencies = [ [[package]] name = "varve-core" -version = "0.15.0" +version = "0.15.1" dependencies = [ "flate2", "hex", diff --git a/Cargo.toml b/Cargo.toml index fca7fef..fb0d21e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ resolver = "2" members = ["crates/varve", "crates/varve-core"] [workspace.package] -version = "0.15.0" +version = "0.15.1" edition = "2024" license = "Apache-2.0" repository = "https://github.com/pulseengine/varve" diff --git a/artifacts/requirements.yaml b/artifacts/requirements.yaml index b2a9999..c549b92 100644 --- a/artifacts/requirements.yaml +++ b/artifacts/requirements.yaml @@ -1456,3 +1456,26 @@ artifacts: fields: priority: should category: functional + + - id: REQ-UPDATE-002 + type: requirement + title: Self-update converges on artifact identity, not self-reported version + status: verified + release: v0.15.1 + description: > + `varve self-update` (and `--check`) shall decide whether an update is + needed by comparing the VERIFIED latest binary's bytes against what is + already on disk, not by self-reported version strings alone. A binary + that mis-reports its own version (as v0.14.0 did — reporting 0.13.1) must + NOT loop forever: version-string comparison stays "newer" and every run + re-installs identical bytes (varve#38). Deciding on digest identity makes + a stale version string degrade to a no-op. `--check` fetches and verifies + the candidate (needing the trust root) so it reports a genuinely-verified + update, not a phantom one. + tags: [core, integrity] + links: + - type: traces-to + target: REQ-UPDATE-001 + fields: + priority: must + category: functional diff --git a/artifacts/verification.yaml b/artifacts/verification.yaml index ec4f442..30063df 100644 --- a/artifacts/verification.yaml +++ b/artifacts/verification.yaml @@ -543,6 +543,8 @@ artifacts: steps: # The guard job exists and asserts the agreement before any build. - run: grep -q "does not match Cargo.toml version" .github/workflows/release.yml + # Artifact-level twin (varve#38): the built binary's --version is asserted == tag. + - run: grep -q "built binary reports" .github/workflows/release.yml links: - type: verifies target: REQ-RELVER-001 @@ -580,3 +582,55 @@ artifacts: target: REQ-CRATE-001 - type: verifies target: REQ-RELVER-001 + + - id: VER-UPDATE-002 + type: verification + title: A wrong version string degrades to a no-op instead of an update loop + status: verified + release: v0.15.1 + fields: + method: automated-test + steps: + # The pure identity anchor. + - run: cargo test -p varve-core a_wrong_version_string_does_not_force_an_update_when_the_bytes_match + # The behavioral claim, end-to-end through a release-API double: a + # mis-reported version resolves to AlreadyCurrent (no loop), differing + # bytes to Available, and an impostor is refused before being offered. + - run: cargo test -p varve-core --test selfupdate_double a_mis_reported_version_converges_on_artifact_identity_not_a_loop + - run: cargo test -p varve-core --test selfupdate_double resolve_update_verifies_before_it_offers_an_impostor + links: + - type: verifies + target: REQ-UPDATE-002 + + - id: VER-REVIEW-v0.15.1 + type: verification + title: v0.15.1 self-update fix independently reviewed — dissent raised, resolved, re-confirmed + status: verified + release: v0.15.1 + tags: [independence, review] + fields: + method: review + baseline: > + Two independent clean-room reviews (fresh context, refute-framed). The + FIRST found the implementation sound (all five refutations failed — the + loop breaks, trust is preserved, digests are sound, --check's root + requirement is disclosed, the release version-assertion is correct) but + DISSENTED on one substantive ground: REQ-UPDATE-002 is a handler-level + requirement, yet its only evidence was a pure unit test of the + `already_current` helper — the handler behaviour (loop-break, + verify-before-compare, --check no-op) had zero coverage. Resolved before + release: the decision was lifted into a core `resolve_update` + (UpToDate/AlreadyCurrent/Available), the handler reduced to a thin + wrapper, and behavioural tests added through the real release-API mock + server (signed archive, real ed25519 verify) — a mis-reported version + with identical on-disk bytes resolves to AlreadyCurrent (loop + terminates), and an impostor is refused before ever being offered. A + SECOND independent review re-ran the suites (6 self-update, 128 lib, 34 + CLI) and confirmed VERDICT pass, DISSENT CLOSED — the identity decision + is genuinely exercised end-to-end. Residual, non-blocking: the handler's + printed-message glue and --check install-gate have no test at any level + (cli.rs carries no self-update case); the identity decision they wrap is + covered. Tracked as a follow-up hardening, not a release blocker. + links: + - type: verifies + target: REQ-UPDATE-002 diff --git a/crates/varve-core/src/update.rs b/crates/varve-core/src/update.rs index 06f721b..9f84794 100644 --- a/crates/varve-core/src/update.rs +++ b/crates/varve-core/src/update.rs @@ -64,6 +64,15 @@ pub fn is_newer(candidate: &str, current: &str) -> bool { } } +/// Whether the running binary is already the latest release's binary, decided +/// on ARTIFACT IDENTITY rather than self-reported version strings (varve#38). +/// A binary that mis-reports its own version (as v0.14.0 did) would otherwise +/// loop forever: `is_newer` stays true, every check re-installs the same bytes. +/// Comparing digests makes a stale version string degrade to a no-op. +pub fn already_current(running_binary: &[u8], latest_binary: &[u8]) -> bool { + crate::store::manifest_digest(running_binary) == crate::store::manifest_digest(latest_binary) +} + /// Ask the release API for the latest tag and locate this platform's assets. /// `api_latest_url` is the GitHub "latest release" endpoint (or a mirror / /// test double — the URL changes availability, never acceptance). @@ -144,14 +153,14 @@ pub fn extract_tool_from_targz(bytes: &[u8], tool: &str) -> Result, Upda Err(UpdateError::NoBinaryInArchive) } -/// Execute an update plan: download, VERIFY with the running binary's trust -/// root, extract, and atomically install at `dest`. Returns the verified -/// archive digest. -pub fn perform( +/// Download and verify the successor binary WITHOUT installing it — the +/// running varve verifies its successor against the trust root. Returns the +/// verified binary bytes and the archive digest. Splitting this from the write +/// lets the caller decide on artifact identity before touching disk (varve#38). +pub fn fetch_verified_binary( plan: &UpdatePlan, root_public_key: &[u8], - dest: &std::path::Path, -) -> Result { +) -> Result<(Vec, String), UpdateError> { let agent = ureq::Agent::new_with_defaults(); let fetch = |url: &str| -> Result, UpdateError> { agent @@ -167,18 +176,20 @@ pub fn perform( }; let envelope = fetch(&plan.envelope_url)?; let archive = fetch(&plan.archive_url)?; - - // The trust decision: the running varve verifies its successor. let digest = verify_release_file(&plan.archive_name, &archive, &envelope, root_public_key)?; - let binary = extract_tool_from_targz(&archive, "varve")?; + Ok((binary, digest)) +} + +/// Atomically install already-verified successor bytes at `dest`. +pub fn install_binary(binary: &[u8], dest: &std::path::Path) -> Result<(), UpdateError> { let io = |path: &std::path::Path, source: std::io::Error| UpdateError::Io { path: path.display().to_string(), source, }; // Atomic on the same filesystem: write beside dest, then rename over it. let tmp = dest.with_extension("varve-update-tmp"); - std::fs::write(&tmp, &binary).map_err(|e| io(&tmp, e))?; + std::fs::write(&tmp, binary).map_err(|e| io(&tmp, e))?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -186,6 +197,66 @@ pub fn perform( .map_err(|e| io(&tmp, e))?; } std::fs::rename(&tmp, dest).map_err(|e| io(dest, e))?; + Ok(()) +} + +/// The self-update decision, resolved on ARTIFACT IDENTITY (varve#38). +#[derive(Debug)] +pub enum UpdateDecision { + /// The API's latest is not newer by version — nothing fetched. + UpToDate, + /// The version string says newer, but the verified latest binary is + /// byte-identical to what is on disk. A no-op — this is what breaks the + /// mis-reported-version loop. + AlreadyCurrent { latest: String }, + /// A genuine, verified update is available: the plan, the verified binary + /// bytes (ready to install), and the archive digest. + Available { + plan: UpdatePlan, + binary: Vec, + digest: String, + }, +} + +/// Resolve whether an update is needed, deciding on artifact identity rather +/// than self-reported version strings (varve#38). `on_disk` is the current +/// binary's bytes (None if the destination does not yet exist). Fetches and +/// VERIFIES the candidate against the trust root before comparing or offering +/// it, so a reported "available" is always a genuinely-verified update. +pub fn resolve_update( + api_latest_url: &str, + current_version: &str, + platform: &str, + on_disk: Option<&[u8]>, + root_public_key: &[u8], +) -> Result { + let Some(plan) = check_latest(api_latest_url, current_version, platform)? else { + return Ok(UpdateDecision::UpToDate); + }; + let (binary, digest) = fetch_verified_binary(&plan, root_public_key)?; + if let Some(current) = on_disk + && already_current(current, &binary) + { + return Ok(UpdateDecision::AlreadyCurrent { + latest: plan.latest, + }); + } + Ok(UpdateDecision::Available { + plan, + binary, + digest, + }) +} + +/// Download, verify against the trust root, extract, and atomically install at +/// `dest`. Returns the verified archive digest. +pub fn perform( + plan: &UpdatePlan, + root_public_key: &[u8], + dest: &std::path::Path, +) -> Result { + let (binary, digest) = fetch_verified_binary(plan, root_public_key)?; + install_binary(&binary, dest)?; Ok(digest) } @@ -206,6 +277,26 @@ mod tests { assert!(!is_newer("0.8.0.1", "0.7.0")); } + // rivet: verifies REQ-UPDATE-002 + #[test] + fn a_wrong_version_string_does_not_force_an_update_when_the_bytes_match() { + // The varve#38 loop: a binary reporting "0.13.1" that is actually the + // latest release. Version strings alone say "update forever"; artifact + // identity says "already current" and the loop terminates. + let running = b"the-genuine-latest-binary"; + let latest = b"the-genuine-latest-binary"; + assert!( + is_newer("v0.14.0", "0.13.1"), + "version strings alone would loop" + ); + assert!( + already_current(running, latest), + "identical verified bytes must read as already-current regardless of version" + ); + // A genuine update has different bytes. + assert!(!already_current(running, b"a-newer-binary")); + } + // rivet: verifies REQ-UPDATE-001 #[test] fn the_binary_is_extracted_from_a_release_shaped_tarball() { diff --git a/crates/varve-core/tests/selfupdate_double.rs b/crates/varve-core/tests/selfupdate_double.rs index a2d2597..252ac38 100644 --- a/crates/varve-core/tests/selfupdate_double.rs +++ b/crates/varve-core/tests/selfupdate_double.rs @@ -7,7 +7,7 @@ use std::io::{BufRead, BufReader, Write}; use std::net::TcpListener; use std::sync::Arc; -use varve_core::update::{check_latest, perform}; +use varve_core::update::{UpdateDecision, check_latest, perform, resolve_update}; /// Serve a fake "latest release" API + asset downloads. fn serve(tag: &str, assets: BTreeMap>) -> String { @@ -116,6 +116,56 @@ fn the_running_binary_verifies_and_installs_its_successor() { assert_eq!(std::fs::read(&dest).unwrap(), b"new-varve-bytes"); } +// rivet: verifies REQ-UPDATE-002 +#[test] +fn a_mis_reported_version_converges_on_artifact_identity_not_a_loop() { + // The varve#38 loop: the running binary reports "0.13.1" but IS the latest + // release bytes. Version strings alone say "update forever"; resolving on + // artifact identity says AlreadyCurrent — a no-op, so the loop terminates. + let release_binary = b"the-genuine-v99-binary"; + let (api, pk, _) = release_fixture("v99.0.0", release_binary, true); + let platform = varve_core::host_platform(); + + // On-disk bytes are byte-identical to the (verified) latest release. + let decision = resolve_update(&api, "0.13.1", &platform, Some(release_binary), &pk).unwrap(); + assert!( + matches!(decision, UpdateDecision::AlreadyCurrent { .. }), + "identical verified bytes must resolve to AlreadyCurrent, got {decision:?}" + ); + + // Different on-disk bytes → a genuine, verified update is available. + let decision = resolve_update(&api, "0.13.1", &platform, Some(b"stale-bytes"), &pk).unwrap(); + assert!( + matches!(decision, UpdateDecision::Available { .. }), + "differing bytes must resolve to Available, got {decision:?}" + ); + + // A version that is not newer never fetches — UpToDate, root untouched path. + let decision = resolve_update(&api, "100.0.0", &platform, Some(b"x"), &pk).unwrap(); + assert!(matches!(decision, UpdateDecision::UpToDate)); +} + +// rivet: verifies REQ-UPDATE-002 +#[test] +fn resolve_update_verifies_before_it_offers_an_impostor() { + // A release signed by an impostor must not surface as Available even though + // its version is newer — resolve_update verifies before comparing/offering. + let (api, _real_pk, _) = release_fixture("v99.0.0", b"evil-bytes", true); + let (_, other_pk) = varve_core::generate_root_keypair(); + let err = resolve_update( + &api, + "0.13.1", + &varve_core::host_platform(), + Some(b"current"), + &other_pk, + ) + .unwrap_err(); + assert!( + err.to_string().to_lowercase().contains("signature"), + "{err}" + ); +} + // rivet: verifies REQ-UPDATE-001 #[test] fn an_older_or_equal_release_is_a_no_op() { diff --git a/crates/varve/src/main.rs b/crates/varve/src/main.rs index cb60370..5c903f4 100644 --- a/crates/varve/src/main.rs +++ b/crates/varve/src/main.rs @@ -492,28 +492,62 @@ fn self_update(check: bool, to: Option<&std::path::Path>) -> anyhow::Result<()> "https://api.github.com/repos/pulseengine/varve/releases/latest".to_string() }); let platform = varve_core::host_platform(); - let Some(plan) = varve_core::update::check_latest(&api, current, &platform)? else { - println!("varve {current} is current"); - return Ok(()); - }; - if check { - println!( - "varve {current} installed; {} available — run `varve self-update` to install (verified)", - plan.latest - ); - return Ok(()); - } - let root_pk = trust_root_bytes()?; let dest = match to { Some(path) => path.to_path_buf(), None => std::env::current_exe().context("cannot locate the running varve binary")?, }; - let digest = varve_core::update::perform(&plan, &root_pk, &dest)?; - println!( - "varve {current} -> {} installed at {} ({digest})", - plan.latest, - dest.display() - ); + // Decide on ARTIFACT IDENTITY, not the self-reported version string: a + // binary that mis-reports its own version (varve#38) must not loop forever + // re-installing identical bytes. resolve_update fetches and verifies the + // candidate against the trust root, then compares it to what is on disk. + let on_disk = std::fs::read(&dest).ok(); + // A first cheap check: if the API isn't even newer by version, skip the + // trust-root requirement entirely so an up-to-date `--check` stays root-free. + if varve_core::update::check_latest(&api, current, &platform)?.is_none() { + println!("varve {current} is current"); + return Ok(()); + } + let root_pk = trust_root_bytes().context( + "self-update needs the trust root to confirm a verified update — set VARVE_TRUST_ROOT \ + or pin a realm", + )?; + match varve_core::update::resolve_update( + &api, + current, + &platform, + on_disk.as_deref(), + &root_pk, + )? { + varve_core::update::UpdateDecision::UpToDate => { + println!("varve {current} is current"); + } + varve_core::update::UpdateDecision::AlreadyCurrent { latest } => { + println!( + "varve is already the latest release bytes ({latest}); the reported version \ + {current} is stale but the binary is current — nothing to do" + ); + } + varve_core::update::UpdateDecision::Available { + plan, + binary, + digest, + } => { + if check { + println!( + "varve {current} installed; {} available — run `varve self-update` to install \ + (verified)", + plan.latest + ); + } else { + varve_core::update::install_binary(&binary, &dest)?; + println!( + "varve {current} -> {} installed at {} ({digest})", + plan.latest, + dest.display() + ); + } + } + } Ok(()) }