From e27e5332da3a812ca6a54ee72335b9a54213d369 Mon Sep 17 00:00:00 2001 From: Kai Lueke Date: Wed, 3 Jan 2024 15:36:04 +0100 Subject: [PATCH] Verify checksum after download, with retry The self.verify_checksum(...) call's return value wasn't checked in the package download call. Even if we do it there we should rather move it into the retry loop and make it explicit whether we expect certain checksums or not. Check the checksum after the download, and also retry when it mismatches. --- examples/download_test.rs | 2 +- examples/full_test.rs | 2 +- src/bin/download_sysext.rs | 13 +++++++--- src/download.rs | 52 +++++++++++++++++++++++++++++++++----- 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/examples/download_test.rs b/examples/download_test.rs index c8dabd1..367808a 100644 --- a/examples/download_test.rs +++ b/examples/download_test.rs @@ -13,7 +13,7 @@ fn main() -> Result<(), Box> { let tempdir = tempfile::tempdir()?; let path = tempdir.path().join("tmpfile"); - let res = download_and_hash(&client, url, &path, false)?; + let res = download_and_hash(&client, url, &path, None, None, false)?; tempdir.close()?; println!("hash: {}", res.hash_sha256); diff --git a/examples/full_test.rs b/examples/full_test.rs index 24920f4..319ae4a 100644 --- a/examples/full_test.rs +++ b/examples/full_test.rs @@ -80,7 +80,7 @@ fn main() -> Result<(), Box> { let tempdir = tempfile::tempdir()?; let path = tempdir.path().join("tmpfile"); - let res = ue_rs::download_and_hash(&client, url.clone(), &path, false).context(format!("download_and_hash({url:?}) failed"))?; + let res = ue_rs::download_and_hash(&client, url.clone(), &path, Some(expected_sha256.clone()), None, false).context(format!("download_and_hash({url:?}) failed"))?; tempdir.close()?; println!("\texpected sha256: {}", expected_sha256); diff --git a/src/bin/download_sysext.rs b/src/bin/download_sysext.rs index 55d480c..6bb6727 100644 --- a/src/bin/download_sysext.rs +++ b/src/bin/download_sysext.rs @@ -109,7 +109,14 @@ impl<'a> Package<'a> { info!("downloading {}...", self.url); let path = into_dir.join(&*self.name); - let res = match ue_rs::download_and_hash(client, self.url.clone(), &path, print_progress) { + match ue_rs::download_and_hash( + client, + self.url.clone(), + &path, + self.hash_sha256.clone(), + self.hash_sha1.clone(), + print_progress, + ) { Ok(ok) => ok, Err(err) => { error!("Downloading failed with error {}", err); @@ -118,7 +125,7 @@ impl<'a> Package<'a> { } }; - self.verify_checksum(res.hash_sha256, res.hash_sha1); + self.status = PackageStatus::Unverified; Ok(()) } @@ -249,7 +256,7 @@ where U: reqwest::IntoUrl + From + std::clone::Clone + std::fmt::Debug, Url: From, { - let r = ue_rs::download_and_hash(client, input_url.clone(), path, print_progress).context(format!("unable to download data(url {:?})", input_url))?; + let r = ue_rs::download_and_hash(client, input_url.clone(), path, None, None, print_progress).context(format!("unable to download data(url {:?})", input_url))?; Ok(Package { name: Cow::Borrowed(path.file_name().unwrap_or(OsStr::new("fakepackage")).to_str().unwrap_or("fakepackage")), diff --git a/src/download.rs b/src/download.rs index 14b6cb8..7b2fdac 100644 --- a/src/download.rs +++ b/src/download.rs @@ -2,7 +2,7 @@ use anyhow::{Context, Result, bail}; use std::io::{BufReader, Read}; use std::fs::File; use std::path::Path; -use log::info; +use log::{info, debug}; use url::Url; use reqwest::StatusCode; @@ -59,7 +59,14 @@ pub fn hash_on_disk(path: &Path, maxlen: Option) -> R Ok(omaha::Hash::from_bytes(Box::new(hasher).finalize())) } -fn do_download_and_hash(client: &Client, url: U, path: &Path, print_progress: bool) -> Result +fn do_download_and_hash( + client: &Client, + url: U, + path: &Path, + expected_sha256: Option>, + expected_sha1: Option>, + print_progress: bool, +) -> Result where U: reqwest::IntoUrl + Clone, Url: From, @@ -95,20 +102,53 @@ where let mut file = File::create(path).context(format!("failed to create path ({:?})", path.display()))?; res.copy_to(&mut file)?; + let calculated_sha256 = hash_on_disk::(path, None)?; + let calculated_sha1 = hash_on_disk::(path, None)?; + + debug!(" expected sha256: {:?}", expected_sha256); + debug!(" calculated sha256: {}", calculated_sha256); + debug!(" sha256 match? {}", expected_sha256 == Some(calculated_sha256.clone())); + debug!(" expected sha1: {:?}", expected_sha1); + debug!(" calculated sha1: {}", calculated_sha1); + debug!(" sha1 match? {}", expected_sha1 == Some(calculated_sha1.clone())); + + if expected_sha256.is_some() && expected_sha256 != Some(calculated_sha256.clone()) { + bail!("Checksum mismatch for sha256"); + } + if expected_sha1.is_some() && expected_sha1 != Some(calculated_sha1.clone()) { + bail!("Checksum mismatch for sha1"); + } + Ok(DownloadResult { - hash_sha256: hash_on_disk::(path, None)?, - hash_sha1: hash_on_disk::(path, None)?, + hash_sha256: calculated_sha256, + hash_sha1: calculated_sha1, data: file, }) } -pub fn download_and_hash(client: &Client, url: U, path: &Path, print_progress: bool) -> Result +pub fn download_and_hash( + client: &Client, + url: U, + path: &Path, + expected_sha256: Option>, + expected_sha1: Option>, + print_progress: bool, +) -> Result where U: reqwest::IntoUrl + Clone, Url: From, { crate::retry_loop( - || do_download_and_hash(client, url.clone(), path, print_progress), + || { + do_download_and_hash( + client, + url.clone(), + path, + expected_sha256.clone(), + expected_sha1.clone(), + print_progress, + ) + }, MAX_DOWNLOAD_RETRY, ) }