Guard against the case of same sha7 but different sha256

heroku · Jul 25, 2024 · d7b6ed5 · d7b6ed5
1 parent 928adf1
commit d7b6ed5
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 8 deletions.
diff --git a/jruby_executable/src/bin/jruby_build.rs b/jruby_executable/src/bin/jruby_build.rs
@@ -6,9 +6,9 @@ use indoc::formatdoc;
 use inventory::artifact::Artifact;
 use jruby_executable::jruby_build_properties;
 use shared::{
-    append_filename_with, artifact_is_different, atomic_inventory_update, download_tar,
-    sha256_from_path, source_dir, tar_dir_to_file, untar_to_dir, ArtifactMetadata, BaseImage,
-    CpuArch, TarDownloadPath,
+    append_filename_with, artifact_is_different, artifact_same_url_different_checksum,
+    atomic_inventory_update, download_tar, sha256_from_path, source_dir, tar_dir_to_file,
+    untar_to_dir, ArtifactMetadata, BaseImage, CpuArch, TarDownloadPath,
 };
 use std::convert::From;
 use std::error::Error;
@@ -140,6 +140,21 @@ fn jruby_build(args: &Args) -> Result<(), Box<dyn Error>> {
                 },
             };
             atomic_inventory_update(&inventory, |inventory| {
+                for prior in &inventory.artifacts {
+                    if let Err(error) = artifact_same_url_different_checksum(prior, &artifact) {
+                        // TODO: Investigate bullet stream ownership
+                        println!(
+                            "{}",
+                            style::important(format!(
+                                "!!!!!!!!!! Error updating inventory: {error}"
+                            ))
+                        );
+
+                        fs_err::remove_file(&sha_seven_path)?;
+                        return Err(error);
+                    };
+                }
+
                 inventory
                     .artifacts
                     .retain(|a| artifact_is_different(a, &artifact));

diff --git a/ruby_executable/src/bin/ruby_build.rs b/ruby_executable/src/bin/ruby_build.rs
@@ -5,10 +5,12 @@ use fun_run::CommandWithName;
 use gem_version::GemVersion;
 use indoc::{formatdoc, indoc};
 use inventory::artifact::Artifact;
+use nom::Err;
 use shared::{
-    append_filename_with, artifact_is_different, atomic_inventory_update, download_tar,
-    output_tar_path, sha256_from_path, source_dir, validate_version_for_stack, ArtifactMetadata,
-    BaseImage, CpuArch, RubyDownloadVersion, TarDownloadPath,
+    append_filename_with, artifact_is_different, artifact_same_url_different_checksum,
+    atomic_inventory_update, download_tar, output_tar_path, sha256_from_path, source_dir,
+    validate_version_for_stack, ArtifactMetadata, BaseImage, CpuArch, RubyDownloadVersion,
+    TarDownloadPath,
 };
 use std::{
     io::Write,
@@ -185,10 +187,25 @@ fn ruby_build(args: &RubyArgs) -> Result<(), Box<dyn std::error::Error>> {
         };
 
         atomic_inventory_update(&inventory, |inventory| {
+            for prior in &inventory.artifacts {
+                if let Err(error) = artifact_same_url_different_checksum(prior, &artifact) {
+                    // TODO: Investigate bullet stream ownership
+                    println!(
+                        "{}",
+                        style::important(format!("!!!!!!!!!! Error updating inventory: {error}"))
+                    );
+
+                    fs_err::remove_file(&sha_seven_path)?;
+                    return Err(error);
+                };
+            }
+
             inventory
                 .artifacts
                 .retain(|a| artifact_is_different(a, &artifact));
+
             inventory.push(artifact);
+
             Ok(())
         })?;
 

diff --git a/ruby_inventory.toml b/ruby_inventory.toml
@@ -1 +1,10 @@
+[[artifacts]]
+version = "3.3.4"
+os = "linux"
+arch = "arm64"
+url = "https://heroku-buildpack-ruby.s3.us-east-1.amazonaws.com/heroku-24/arm64/ruby-3.3.4-7bebeee.tgz"
+checksum = "sha256:7bebeee1b9128bdbb290331b813fa01cf43e30cd0098286f7de011796cb8eee5"
 
+[artifacts.metadata]
+timestamp = "2024-07-25T21:13:38.723081Z"
+distro_version = "24.04"
diff --git a/shared/src/inventory_help.rs b/shared/src/inventory_help.rs
@@ -180,6 +180,28 @@ where
     Ok(digest.finalize_fixed())
 }
 
+/// Raises an error if the same URL has a different checksum
+///
+/// This protects against the (reasonably) unlikely event that the same version generates a checksum with the same first 7 characters but a net different checksum.
+/// While unlikely, it's still possible. If we didn't guard against this case, then it could break people's builds who are relying on the old checksum
+/// no not change.
+pub fn artifact_same_url_different_checksum(
+    a: &inventory::artifact::Artifact<GemVersion, Sha256, ArtifactMetadata>,
+    b: &inventory::artifact::Artifact<GemVersion, Sha256, ArtifactMetadata>,
+) -> Result<(), Box<dyn std::error::Error>> {
+    if a.url == b.url && a.checksum != b.checksum {
+        Err(format!(
+            "Duplicate url {url} has different checksums {a_checksum} != {b_checksum}",
+            url = a.url,
+            a_checksum = hex::encode(&a.checksum.value),
+            b_checksum = hex::encode(&b.checksum.value)
+        )
+        .into())
+    } else {
+        Ok(())
+    }
+}
+
 pub fn artifact_is_different(
     a: &inventory::artifact::Artifact<GemVersion, Sha256, ArtifactMetadata>,
     b: &inventory::artifact::Artifact<GemVersion, Sha256, ArtifactMetadata>,
@@ -198,6 +220,32 @@ mod test {
 
     use super::*;
 
+    #[test]
+    fn test_same_url_different_checksum_raises_error() {
+        let a = Artifact {
+            os: inventory::artifact::Os::Linux,
+            arch: inventory::artifact::Arch::Amd64,
+            version: GemVersion::from_str("1.0.0").unwrap(),
+            checksum: "sha256:dd073bda5665e758c3e6f861a6df435175c8e8faf5ec75bc2afaab1e3eebb2c7"
+                .parse()
+                .unwrap(),
+            metadata: ArtifactMetadata {
+                timestamp: Utc::now(),
+                distro_version: BaseImage::new("heroku-24").unwrap().distro_version(),
+            },
+            url: "https://example.com".to_string(),
+        };
+
+        let b = a.clone();
+        artifact_same_url_different_checksum(&a, &b).unwrap();
+
+        let mut b = a.clone();
+        b.checksum = "sha256:7bebeee1b9128bdbb290331b813fa01cf43e30cd0098286f7de011796cb8eee5"
+            .parse()
+            .unwrap();
+        assert!(artifact_same_url_different_checksum(&a, &b).is_err());
+    }
+
     #[test]
     fn test_is_not_version_match() {
         let a = Artifact {

diff --git a/shared/src/lib.rs b/shared/src/lib.rs
@@ -13,8 +13,8 @@ mod inventory_help;
 pub use base_image::{BaseImage, CpuArch, CpuArchError};
 pub use download_ruby_version::RubyDownloadVersion;
 pub use inventory_help::{
-    artifact_is_different, atomic_inventory_update, inventory_check, sha256_from_path,
-    ArtifactMetadata,
+    artifact_is_different, artifact_same_url_different_checksum, atomic_inventory_update,
+    inventory_check, sha256_from_path, ArtifactMetadata,
 };
 
 /// Appends the given string after the filename and before the `ends_with`