From c0bc636d654c83b2c77b6738d24c41e293c12abe Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 23 Sep 2024 18:06:06 +0000 Subject: [PATCH] Fix digest usage In some places we started doing `.digest().digest()` which drops the algorithm. Some basic unit tests would have caught this - I added one. We also need an "upgrade from previous" test. Trying to fix this revealed a bunch more places where we had `String` and not `Digest`. Fixing those gets us type safety and points out the problem areas. Since we're breaking API as this will be the real first 0.15 release, - Drop unnecessary ManifestLayerState wrappering - Fix the manifest digest members of various structs to be a Digest --- lib/src/container/store.rs | 86 +++++++++++++++--------- lib/src/container/unencapsulate.rs | 4 +- lib/src/container/update_detachedmeta.rs | 2 +- lib/tests/it/main.rs | 19 +++--- 4 files changed, 64 insertions(+), 47 deletions(-) diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index 2828b361..6dc15713 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -21,7 +21,7 @@ use flate2::Compression; use fn_error_context::context; use futures_util::TryFutureExt; use oci_spec::image::{ - self as oci_image, Arch, Descriptor, History, ImageConfiguration, ImageManifest, + self as oci_image, Arch, Descriptor, Digest, History, ImageConfiguration, ImageManifest, }; use ostree::prelude::{Cast, FileEnumeratorExt, FileExt, ToVariant}; use ostree::{gio, glib}; @@ -68,7 +68,7 @@ fn ref_for_blob_digest(d: &str) -> Result { /// Convert e.g. sha256:12345... into `/ostree/container/blob/sha256_2B12345...`. fn ref_for_layer(l: &oci_image::Descriptor) -> Result { - ref_for_blob_digest(l.digest().digest()) + ref_for_blob_digest(&l.digest().to_string()) } /// Convert e.g. sha256:12345... into `/ostree/container/blob/sha256_2B12345...`. @@ -120,7 +120,7 @@ pub struct LayeredImageState { /// The merge commit unions all layers pub merge_commit: String, /// The digest of the original manifest - pub manifest_digest: String, + pub manifest_digest: Digest, /// The image manfiest pub manifest: ImageManifest, /// The image configuration @@ -151,7 +151,7 @@ pub struct CachedImageUpdate { /// The image configuration pub config: ImageConfiguration, /// The digest of the manifest - pub manifest_digest: String, + pub manifest_digest: Digest, } impl CachedImageUpdate { @@ -199,23 +199,11 @@ pub struct ManifestLayerState { pub commit: Option, } -impl ManifestLayerState { - /// The cryptographic checksum. - pub fn digest(&self) -> &str { - self.layer.digest().digest() - } - - /// The (possibly compressed) size. - pub fn size(&self) -> u64 { - self.layer.size() - } -} - /// Information about which layers need to be downloaded. #[derive(Debug)] pub struct PreparedImport { /// The manifest digest that was found - pub manifest_digest: String, + pub manifest_digest: Digest, /// The deserialized manifest. pub manifest: oci_image::ImageManifest, /// The deserialized configuration. @@ -223,7 +211,7 @@ pub struct PreparedImport { /// The previous manifest pub previous_state: Option>, /// The previously stored manifest digest. - pub previous_manifest_digest: Option, + pub previous_manifest_digest: Option, /// The previously stored image ID. pub previous_imageid: Option, /// The layers containing split objects @@ -286,7 +274,7 @@ impl PreparedImport { if v.commit.is_some() { (stored + 1, to_fetch, sz) } else { - (stored, to_fetch + 1, sz + v.size()) + (stored, to_fetch + 1, sz + v.layer.size()) } }); (to_fetch > 0).then(|| { @@ -313,10 +301,11 @@ pub(crate) fn query_layer( #[context("Reading manifest data from commit")] fn manifest_data_from_commitmeta( commit_meta: &glib::VariantDict, -) -> Result<(oci_image::ImageManifest, String)> { +) -> Result<(oci_image::ImageManifest, Digest)> { let digest = commit_meta - .lookup(META_MANIFEST_DIGEST)? + .lookup::(META_MANIFEST_DIGEST)? .ok_or_else(|| anyhow!("Missing {} metadata on merge commit", META_MANIFEST_DIGEST))?; + let digest = Digest::from_str(&digest)?; let manifest_bytes: String = commit_meta .lookup::(META_MANIFEST)? .ok_or_else(|| anyhow!("Failed to find {} metadata key", META_MANIFEST))?; @@ -344,7 +333,7 @@ fn image_config_from_commitmeta(commit_meta: &glib::VariantDict) -> Result Result { +pub fn manifest_digest_from_commit(commit: &glib::Variant) -> Result { let commit_meta = &commit.child_value(0); let commit_meta = &glib::VariantDict::new(Some(commit_meta)); Ok(manifest_data_from_commitmeta(commit_meta)?.1) @@ -548,12 +537,15 @@ impl ImageImporter { pub(crate) async fn cache_pending( &self, commit: &str, - manifest_digest: &str, + manifest_digest: &Digest, manifest: &ImageManifest, config: &ImageConfiguration, ) -> Result<()> { let commitmeta = glib::VariantDict::new(None); - commitmeta.insert(Self::CACHED_KEY_MANIFEST_DIGEST, manifest_digest); + commitmeta.insert( + Self::CACHED_KEY_MANIFEST_DIGEST, + manifest_digest.to_string(), + ); let cached_manifest = serde_json::to_string(manifest).context("Serializing manifest")?; commitmeta.insert(Self::CACHED_KEY_MANIFEST, cached_manifest); let cached_config = serde_json::to_string(config).context("Serializing config")?; @@ -573,7 +565,7 @@ impl ImageImporter { /// which e.g. includes a diff of the layers. fn create_prepared_import( &mut self, - manifest_digest: String, + manifest_digest: Digest, manifest: ImageManifest, config: ImageConfiguration, previous_state: Option>, @@ -638,7 +630,8 @@ impl ImageImporter { } let (manifest_digest, manifest) = self.proxy.fetch_manifest(&self.proxy_img).await?; - let new_imageid = manifest.config().digest().digest(); + let manifest_digest = Digest::from_str(&manifest_digest)?; + let new_imageid = manifest.config().digest(); // Query for previous stored state @@ -649,7 +642,7 @@ impl ImageImporter { return Ok(PrepareResult::AlreadyPresent(previous_state)); } // Failing that, if they have the same imageID, we're also done. - let previous_imageid = previous_state.manifest.config().digest().digest(); + let previous_imageid = previous_state.manifest.config().digest(); if previous_imageid == new_imageid { return Ok(PrepareResult::AlreadyPresent(previous_state)); } @@ -666,7 +659,7 @@ impl ImageImporter { if let Some(previous_state) = previous_state.as_ref() { self.cache_pending( previous_state.merge_commit.as_str(), - manifest_digest.as_str(), + &manifest_digest, &manifest, &config, ) @@ -741,7 +734,7 @@ impl ImageImporter { txn.commit(Some(cancellable))?; Ok::<_, anyhow::Error>(commit) }) - .map_err(|e| e.context(format!("Layer {}", layer.digest()))); + .map_err(|e| e.context(format!("Layer {}", layer.layer.digest()))); let commit = super::unencapsulate::join_fetch(import_task, driver).await?; layer.commit = commit; if let Some(p) = self.layer_progress.as_ref() { @@ -889,12 +882,12 @@ impl ImageImporter { crate::tar::write_tar(&self.repo, blob, layer.ostree_ref.as_str(), Some(opts)); let r = super::unencapsulate::join_fetch(r, driver) .await - .with_context(|| format!("Parsing layer blob {}", layer.digest()))?; + .with_context(|| format!("Parsing layer blob {}", layer.layer.digest()))?; layer_commits.push(r.commit); if !r.filtered.is_empty() { let filtered = HashMap::from_iter(r.filtered.into_iter()); tracing::debug!("Found {} filtered toplevels", filtered.len()); - layer_filtered_content.insert(layer.digest().to_string(), filtered); + layer_filtered_content.insert(layer.layer.digest().to_string(), filtered); } else { tracing::debug!("No filtered content"); } @@ -916,7 +909,10 @@ impl ImageImporter { let serialized_manifest = serde_json::to_string(&import.manifest)?; let serialized_config = serde_json::to_string(&import.config)?; let mut metadata = HashMap::new(); - metadata.insert(META_MANIFEST_DIGEST, import.manifest_digest.to_variant()); + metadata.insert( + META_MANIFEST_DIGEST, + import.manifest_digest.to_string().to_variant(), + ); metadata.insert(META_MANIFEST, serialized_manifest.to_variant()); metadata.insert(META_CONFIG, serialized_config.to_variant()); metadata.insert( @@ -1089,6 +1085,7 @@ fn parse_cached_update(meta: &glib::VariantDict) -> Result, diff --git a/lib/src/container/update_detachedmeta.rs b/lib/src/container/update_detachedmeta.rs index 503bc3b7..2803fab1 100644 --- a/lib/src/container/update_detachedmeta.rs +++ b/lib/src/container/update_detachedmeta.rs @@ -55,7 +55,7 @@ pub async fn update_detached_metadata( .read_json_blob(manifest_descriptor) .context("Reading manifest json blob")?; - anyhow::ensure!(manifest_descriptor.digest().digest() == pulled_digest.digest()); + anyhow::ensure!(manifest_descriptor.digest() == &pulled_digest); let platform = manifest_descriptor .platform() .as_ref() diff --git a/lib/tests/it/main.rs b/lib/tests/it/main.rs index 3a339a2b..d2bd27f6 100644 --- a/lib/tests/it/main.rs +++ b/lib/tests/it/main.rs @@ -5,7 +5,7 @@ use cap_std_ext::cap_std; use containers_image_proxy::oci_spec; use oci_image::ImageManifest; use oci_spec::image as oci_image; -use ocidir::oci_spec::image::Arch; +use ocidir::oci_spec::image::{Arch, DigestAlgorithm}; use once_cell::sync::Lazy; use ostree_ext::chunking::ObjectMetaSized; use ostree_ext::container::{store, ManifestDiff}; @@ -626,7 +626,7 @@ async fn test_export_as_container_nonderived() -> Result<()> { let desc = idx.manifests().first().unwrap(); let new_manifest: oci_image::ImageManifest = ocidir.read_json_blob(desc).unwrap(); - assert_eq!(desc.digest().digest(), exported.digest()); + assert_eq!(desc.digest().to_string(), exported.to_string()); assert_eq!(new_manifest.layers().len(), fixture::LAYERS_V0_LEN); // Reset the destrepo @@ -865,7 +865,7 @@ async fn test_container_chunked() -> Result<()> { for layer in prep.layers.iter() { assert!(layer.commit.is_none()); } - assert_eq!(digest, expected_digest.to_string()); + assert_eq!(digest, expected_digest); { let mut layer_history = prep.layers_with_history(); assert!(layer_history @@ -888,7 +888,7 @@ async fn test_container_chunked() -> Result<()> { ); } let import = imp.import(prep).await.context("Init pull derived").unwrap(); - assert_eq!(import.manifest_digest.as_str(), digest); + assert_eq!(import.manifest_digest, digest); assert_eq!(store::list_images(fixture.destrepo()).unwrap().len(), 1); @@ -914,7 +914,7 @@ r usr/bin/bash bash-v0 .context("Failed to update")?; let expected_digest = fixture.export_container().await.unwrap().1; - assert_ne!(digest, expected_digest.digest()); + assert_ne!(digest, expected_digest); let mut imp = store::ImageImporter::new(fixture.destrepo(), &imgref, Default::default()).await?; @@ -930,15 +930,12 @@ r usr/bin/bash bash-v0 assert_eq!(cached.version(), Some("42.0")); let cached_update = cached.cached_update.unwrap(); - assert_eq!( - cached_update.manifest_digest.as_str(), - prep.manifest_digest.as_str() - ); + assert_eq!(cached_update.manifest_digest, prep.manifest_digest); assert_eq!(cached_update.version(), Some("42.0")); } let to_fetch = prep.layers_to_fetch().collect::>>()?; assert_eq!(to_fetch.len(), 2); - assert_eq!(expected_digest.to_string(), prep.manifest_digest.as_str()); + assert_eq!(expected_digest, prep.manifest_digest); assert!(prep.ostree_commit_layer.commit.is_none()); assert_eq!(prep.ostree_layers.len(), nlayers); let (first, second) = (to_fetch[0], to_fetch[1]); @@ -1333,7 +1330,7 @@ async fn test_container_write_derive() -> Result<()> { .load_commit(import.merge_commit.as_str())? .0; let digest = store::manifest_digest_from_commit(imported_commit)?; - assert!(digest.starts_with("sha256:")); + assert_eq!(digest.algorithm(), &DigestAlgorithm::Sha256); assert_eq!(digest, expected_digest); let commit_meta = &imported_commit.child_value(0);