diff --git a/CHANGELOG.md b/CHANGELOG.md index ff40ba4..3c5d53e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,13 @@ ### Added ### Fixed +## 0.4.4 - 2025-07-01 + +### Fixed +- Make GPG config be consistently ordered in lockfile. +- Remove debug print statement. +- Handle case where RPMs have a different checksum algorithm than expected when they're being downloaded, while still verifying the checksum. + ## 0.4.3 - 2025-05-28 ### Added @@ -32,7 +39,7 @@ ### Fixed - Set size in hardlink headers correctly. - - Fixes integrity failures during this `docker push` + - Fixes integrity failures during this `docker push` ## 0.3.1 - 2024-07-24 diff --git a/Cargo.lock b/Cargo.lock index 1f4204b..ba92b43 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1960,7 +1960,7 @@ dependencies = [ [[package]] name = "rpmoci" -version = "0.4.3" +version = "0.4.4" dependencies = [ "anyhow", "chrono", diff --git a/Cargo.toml b/Cargo.toml index e353c3a..ea53194 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rpmoci" -version = "0.4.3" +version = "0.4.4" edition = "2024" description = "Build container images from RPMs" # rpmoci uses DNF (via pyo3) which is GPLV2+ licensed, diff --git a/src/cli.rs b/src/cli.rs index d4916ac..54905aa 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -38,7 +38,7 @@ pub struct Cli { fn label_parser(s: &str) -> Result<(String, String), String> { match s.split_once('=') { Some((key, value)) => Ok((key.to_string(), value.to_string())), - None => Err(format!("`{}` should be of the form KEY=VALUE.", s)), + None => Err(format!("`{s}` should be of the form KEY=VALUE.")), } } diff --git a/src/config.rs b/src/config.rs index 5d70513..86238ce 100644 --- a/src/config.rs +++ b/src/config.rs @@ -176,7 +176,7 @@ impl ImageConfig { .entrypoint(entrypoint.clone()) .env( envs.iter() - .map(|(k, v)| format!("{}={}", k, v)) + .map(|(k, v)| format!("{k}={v}")) .collect::>(), ) .exposed_ports(exposed_ports.clone()) diff --git a/src/lib.rs b/src/lib.rs index b9386e7..09eff88 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -130,13 +130,12 @@ pub fn main(command: Command) -> anyhow::Result<()> { write::error( "Warning", format!( - "failed to parse existing lock file. Generating a new one. Error: {}", - err + "failed to parse existing lock file. Generating a new one. Error: {err}" ), )?; err.chain() .skip(1) - .for_each(|cause| eprintln!("caused by: {}", cause)); + .for_each(|cause| eprintln!("caused by: {cause}")); changed = true; Lockfile::resolve_from_config(&cfg)? } diff --git a/src/lockfile/build.rs b/src/lockfile/build.rs index 2328abf..abc09e8 100644 --- a/src/lockfile/build.rs +++ b/src/lockfile/build.rs @@ -142,7 +142,7 @@ impl Lockfile { .context(format!("Failed to read file: {}", path.display()))?; let checksum = format!("{:x}", hasher.finalize()); - eprintln!("Checksum: {}", checksum); + eprintln!("Checksum: {checksum}"); if checksum != p.checksum.checksum { bail!( @@ -217,7 +217,7 @@ impl Lockfile { } } write::ok("Installing", "packages")?; - log::debug!("Running `{:?}`", dnf_install); + log::debug!("Running `{dnf_install:?}`"); let status = dnf_install.status().context("Failed to run dnf")?; if !status.success() { bail!("failed to dnf install"); @@ -239,7 +239,7 @@ impl Lockfile { rpm_erase.arg(pkg); } write::ok("Removing", "excluded packages")?; - log::debug!("Running `{:?}`", rpm_erase); + log::debug!("Running `{rpm_erase:?}`"); let status = rpm_erase.status().context("Failed to run rpm")?; if !status.success() { bail!("failed to rpm erase excluded packages"); @@ -272,7 +272,7 @@ fn creation_time() -> Result, anyhow::Error> { let creation_time = if let Ok(sde) = std::env::var("SOURCE_DATE_EPOCH") { let timestamp = sde .parse::() - .with_context(|| format!("Failed to parse SOURCE_DATE_EPOCH `{}`", sde))?; + .with_context(|| format!("Failed to parse SOURCE_DATE_EPOCH `{sde}`"))?; DateTime::from_timestamp(timestamp, 0) .ok_or_else(|| anyhow::anyhow!("SOURCE_DATE_EPOCH out of range: `{}`", sde))? } else { diff --git a/src/lockfile/download.py b/src/lockfile/download.py index ee0347f..8784874 100644 --- a/src/lockfile/download.py +++ b/src/lockfile/download.py @@ -19,32 +19,114 @@ from dnf.i18n import _ import dnf from dnf.cli.progress import MultiFileProgressMeter +import hashlib +import hawkey + + +class Package: + """Store information about RPM packages.""" + + def __init__(self, name, evr, algo, checksum, arch) -> None: + self.name = name + self.evr = evr + self.algo = algo + self.checksum = checksum + self.arch = arch + self.package = None # dnf.package.Package + self.checked = False def download(base, packages, directory): """Downloads packages. Parameters: - base needs to be a dnf.Base() object that has had repos configured and fill_sack called. - packages is an array of requested package specifications - - packages is a list of [name, evr, checksum, arch] lists. - - directory, where to copy the RPMs to + - packages is a list of [name, evr, checksum algorithm, checksum, arch] lists, of requested package specifications. + - directory, where to copy the RPMs to. """ - pkgs = [get_package(base, p[0], p[1], p[2], p[3]) for p in packages] - base.download_packages(pkgs, MultiFileProgressMeter(fo=sys.stdout)) - for pkg in pkgs: - shutil.copy(pkg.localPkg(), directory) + # Convert input to our own Package type for convenience. + pkgs = [Package(p[0], p[1], p[2], p[3], p[4]) for p in packages] + + # Fill in DNF package info, and take a note of whichever checksums already match. + for p in pkgs: + get_package(base, p) + + # Download the RPMs. + base.download_packages( + (p.package for p in pkgs), MultiFileProgressMeter(fo=sys.stdout) + ) + + # Download each RPM. If we haven't been able to verify the checksum yet because we got a different checksum + # algorithm than we had when we originally resolved the RPM, then verify it now by hashing the file. + for p in pkgs: + if not p.checked: + # Checksum types must not have matched, hash the RPM now. + hasher = hashlib.new(p.algo) + with open(p.package.localPkg(), "rb") as f: + while True: + data = f.read(65536) + if not data: + break + hasher.update(data) + + checksum = hasher.hexdigest() + if p.checksum != checksum: + msg = ( + "Package checksum didn't match: " + f"Name: '{p.name}', evr: '{p.evr}', algo: '{p.algo}', expected: '{p.checksum}', found: '{checksum}'" + ) + raise dnf.exceptions.DepsolveError(msg) + + shutil.copy(p.package.localPkg(), directory) -def get_package(base, name, evr, checksum, arch): +def hawkey_chksum_to_name(id): + if id == hawkey.CHKSUM_MD5: # Devskim: ignore DS126858 + return "md5" # Devskim: ignore DS126858 + elif id == hawkey.CHKSUM_SHA1: # Devskim: ignore DS126858 + return "sha1" # Devskim: ignore DS126858 + elif id == hawkey.CHKSUM_SHA256: + return "sha256" + elif id == hawkey.CHKSUM_SHA384: + return "sha384" + elif id == hawkey.CHKSUM_SHA512: + return "sha512" + raise dnf.exceptions.Error("Unknown checksum value %d" % id) + + +def raise_no_package_error(pkg): + msg = f"Package could no longer be found in repositories. Name: '{pkg.name}', evr: '{pkg.evr}'" + raise dnf.exceptions.DepsolveError(msg) + + +def get_package(base, pkg): """Find packages matching given spec.""" - if arch: - pkgs = base.sack.query().filter(name=name, evr=evr, arch=arch).run() + if pkg.arch: + pkgs = base.sack.query().filter(name=pkg.name, evr=pkg.evr, arch=pkg.arch).run() else: - pkgs = base.sack.query().filter(name=name, evr=evr).run() - # Filter by checksum manually as hawkey does not support it - pkgs = [p for p in pkgs if p.chksum and p.chksum[1].hex() == checksum] + pkgs = base.sack.query().filter(name=pkg.name, evr=pkg.evr).run() if not pkgs: - msg = f"Package could no longer be found in repositories. Name: '{name}', evr: '{evr}'" - raise dnf.exceptions.DepsolveError(msg) - return pkgs[0] + raise_no_package_error(pkg) + + # Filter by checksum manually as hawkey does not support it. + # A package may be presented with a different checksum algorithm here than when it was originally resolved. + # Therefore, only bail out here if we find the right type of checksum but no matches, otherwise we'll verify the + # checksum after downloading the package. + found_correct_checksum_type = False + for p in pkgs: + if p.chksum: + if p.chksum[1].hex() == pkg.checksum: + # Found a match, indicate that the checksum is correct and return it. + pkg.package = p + pkg.checked = True + return + + if hawkey_chksum_to_name(p.chksum[0]) == pkg.algo: + found_correct_checksum_type = True + + if found_correct_checksum_type: + # Should have found a matching checksum because we found at least one of the same type, but none did. + raise_no_package_error(pkg) + + # Fallback, just pick the first package and verify the checksum after downloading it. + pkg.package = pkgs[0] diff --git a/src/lockfile/download.rs b/src/lockfile/download.rs index 177d6f7..0af30ca 100644 --- a/src/lockfile/download.rs +++ b/src/lockfile/download.rs @@ -50,6 +50,7 @@ impl Lockfile { ( p.name.clone(), p.evr.clone(), + p.checksum.algorithm.to_string(), p.checksum.checksum.clone(), p.arch.clone().unwrap_or_default(), ) @@ -84,7 +85,7 @@ impl Lockfile { for (repoid, repo_key_info) in &self.repo_gpg_config { if repo_key_info.gpgcheck { for (i, key) in repo_key_info.keys.iter().enumerate() { - load_key(&tmp_dir, &format!("{}-{}", repoid, i), key)?; + load_key(&tmp_dir, &format!("{repoid}-{i}"), key)?; } } } diff --git a/src/lockfile/mod.rs b/src/lockfile/mod.rs index db5ef92..c4d4f35 100644 --- a/src/lockfile/mod.rs +++ b/src/lockfile/mod.rs @@ -14,7 +14,7 @@ //! //! You should have received a copy of the GNU General Public License //! along with this program. If not, see . -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeMap, BTreeSet}; use std::fmt; use std::io::Write; use std::path::Path; @@ -36,8 +36,8 @@ pub struct Lockfile { packages: BTreeSet, #[serde(default, skip_serializing_if = "BTreeSet::is_empty")] local_packages: BTreeSet, - #[serde(default, skip_serializing_if = "HashMap::is_empty")] - repo_gpg_config: HashMap, + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + repo_gpg_config: BTreeMap, #[serde(default, skip_serializing_if = "Vec::is_empty")] global_key_specs: Vec, } @@ -65,7 +65,7 @@ struct DnfOutput { /// Local packages local_packages: Vec, /// Repository GPG configuration - repo_gpg_config: HashMap, + repo_gpg_config: BTreeMap, } /// GPG key configuration for a specified repository @@ -194,14 +194,14 @@ impl Lockfile { for (name, evr) in old { if let Some(new_evr) = new.remove(name) { if new_evr != evr { - write::ok("Updating", format!("{} {} -> {}", name, evr, new_evr))?; + write::ok("Updating", format!("{name} {evr} -> {new_evr}"))?; } } else { - write::ok("Removing", format!("{} {}", name, evr))?; + write::ok("Removing", format!("{name} {evr}"))?; } } for (name, evr) in new { - write::ok("Adding", format!("{} {}", name, evr))?; + write::ok("Adding", format!("{name} {evr}"))?; } Ok(()) diff --git a/src/lockfile/resolve.rs b/src/lockfile/resolve.rs index ba975b9..43db26b 100644 --- a/src/lockfile/resolve.rs +++ b/src/lockfile/resolve.rs @@ -56,7 +56,6 @@ impl Lockfile { } else { pkg_specs.clone() }; - eprintln!("3"); let args = PyTuple::new(py, [base.as_any(), &specs.into_pyobject(py)?])?; // Run the resolve function, returning a json string, which we shall deserialize. diff --git a/src/main.rs b/src/main.rs index de1a68e..f59aad8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,7 +22,7 @@ fn main() { write::error("Error", err.to_string()).unwrap(); err.chain() .skip(1) - .for_each(|cause| eprintln!("caused by: {}", cause)); + .for_each(|cause| eprintln!("caused by: {cause}")); std::process::exit(1); } } diff --git a/src/write.rs b/src/write.rs index b5a75c2..732a5c8 100644 --- a/src/write.rs +++ b/src/write.rs @@ -21,9 +21,9 @@ use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; fn msg(label: &str, message: impl Display, color: &ColorSpec) -> io::Result<()> { let mut stderr = StandardStream::stderr(ColorChoice::Auto); stderr.set_color(color)?; - write!(&mut stderr, "{:>20} ", label)?; + write!(&mut stderr, "{label:>20} ")?; stderr.set_color(ColorSpec::new().set_fg(None))?; - writeln!(&mut stderr, "{}", message)?; + writeln!(&mut stderr, "{message}")?; Ok(()) } diff --git a/tests/it.rs b/tests/it.rs index 4944290..ac0d1df 100644 --- a/tests/it.rs +++ b/tests/it.rs @@ -67,7 +67,7 @@ fn test_incompatible_lockfile() { .unwrap(); assert!(!output.status.success()); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(stderr.contains("needs to be updated but --locked was passed to prevent this")); // Updating should succeed @@ -86,7 +86,7 @@ fn test_updatable_lockfile() { .unwrap(); assert!(output.status.success()); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(stderr.contains("Updating etcd 3.5.1-1.cm2 -> ")); assert!(stderr.contains("Updating filesystem 1.1-9.cm2 ->")); assert!(stderr.contains("Updating glibc 2.35-1.cm2 -> ")); @@ -106,7 +106,7 @@ fn test_unparseable_lockfile() { .unwrap(); let stderr = std::str::from_utf8(&output.stderr).unwrap(); assert!(!output.status.success()); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(stderr.contains("failed to parse existing lock file")); // but we should be able to update it @@ -117,7 +117,7 @@ fn test_unparseable_lockfile() { .output() .unwrap(); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(output.status.success()); assert!(stderr.contains("Adding tini-static ")); } @@ -135,7 +135,7 @@ fn test_no_lockfile() { .unwrap(); assert!(!output.status.success()); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!( stderr.contains("is missing and needs to be generated but --locked was passed to prevent ") ); @@ -152,7 +152,7 @@ fn test_update_from_lockfile() { .output() .unwrap(); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(output.status.success()); assert!(stderr.contains("Updating dnf 4.8.0-1.cm2 -> ")); } @@ -173,7 +173,7 @@ fn test_simple_build() { .output() .unwrap(); let stderr = std::str::from_utf8(&output1.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(output1.status.success()); // Open the lockfile and verify /etc/os-release was included as a dependency @@ -187,7 +187,7 @@ fn test_simple_build() { ); let stderr = std::str::from_utf8(&output1.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(output1.status.success()); // Repeat the build, to ensure reproducing the same image works @@ -202,7 +202,7 @@ fn test_simple_build() { .output() .unwrap(); let stderr = std::str::from_utf8(&output2.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(output2.status.success()); let index = ImageIndex::from_file(root.join("foo").join("index.json")).unwrap(); @@ -230,7 +230,7 @@ fn test_vendor() { .output() .unwrap(); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!(output.status.success()); let status = rpmoci() @@ -340,7 +340,7 @@ fn test_base_arch() { fn test_capabilities() { let output = build_and_run("capabilities", true); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert!( std::str::from_utf8(&output.stdout) .unwrap() @@ -355,7 +355,7 @@ fn test_hardlinks() { // This test checks that /usr/bin/ld has a hardlink, i.e that rpmoci hasn't copied the file let output = build_and_run("hardlinks", true); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); assert_eq!(std::str::from_utf8(&output.stdout).unwrap().trim(), "2"); // Test we can push the image to a registry @@ -399,11 +399,11 @@ fn build_and_run(image: &str, should_succeed: bool) -> std::process::Output { copy_to_docker(image, &root); let output = Command::new("docker") .arg("run") - .arg(format!("{}:test", image)) + .arg(format!("{image}:test")) .output() .expect("failed to run container"); let stderr = std::str::from_utf8(&output.stderr).unwrap(); - eprintln!("stderr: {}", stderr); + eprintln!("stderr: {stderr}"); if should_succeed { assert!(output.status.success()); } else { @@ -415,8 +415,8 @@ fn build_and_run(image: &str, should_succeed: bool) -> std::process::Output { fn copy_to_docker(image: &str, root: impl AsRef) { let status = Command::new("skopeo") .arg("copy") - .arg(format!("oci:{}:test", image)) - .arg(format!("docker-daemon:{}:test", image)) + .arg(format!("oci:{image}:test")) + .arg(format!("docker-daemon:{image}:test")) .current_dir(root.as_ref()) .status() .expect("failed to run skopeo");