Merge pull request #666 from cgwalters/fix-digest-traps

Fix digest usage

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<String> {
 
 /// Convert e.g. sha256:12345... into `/ostree/container/blob/sha256_2B12345...`.
 fn ref_for_layer(l: &oci_image::Descriptor) -> Result<String> {
-    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,31 +199,19 @@ pub struct ManifestLayerState {
     pub commit: Option<String>,
 }
 
-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.
     pub config: oci_image::ImageConfiguration,
     /// The previous manifest
     pub previous_state: Option<Box<LayeredImageState>>,
     /// The previously stored manifest digest.
-    pub previous_manifest_digest: Option<String>,
+    pub previous_manifest_digest: Option<Digest>,
     /// The previously stored image ID.
     pub previous_imageid: Option<String>,
     /// 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::<String>(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::<String>(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<Image
 ///
 /// This can be used to uniquely identify the image.  For example, it can be used
 /// in a "digested pull spec" like `quay.io/someuser/exampleos@sha256:...`.
-pub fn manifest_digest_from_commit(commit: &glib::Variant) -> Result<String> {
+pub fn manifest_digest_from_commit(commit: &glib::Variant) -> Result<Digest> {
     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<Box<LayeredImageState>>,
@@ -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<Option<CachedImageUpd
             // our key; gracefully handle that.
             return Ok(None);
         };
+    let manifest_digest = Digest::from_str(&manifest_digest)?;
     // If we found the cached manifest digest key, then we must have the manifest and config;
     // otherwise that's an error.
     let manifest = meta.lookup_value(ImageImporter::CACHED_KEY_MANIFEST, None);
@@ -1476,7 +1473,7 @@ fn gc_image_layers_impl(
     let mut referenced_layers = BTreeSet::new();
     for m in all_manifests.iter() {
         for layer in m.layers() {
-            referenced_layers.insert(layer.digest().digest());
+            referenced_layers.insert(layer.digest().to_string());
         }
     }
     tracing::debug!("Referenced layers: {}", referenced_layers.len());
@@ -1808,3 +1805,26 @@ pub(crate) fn verify_container_image(
 
     Ok(true)
 }
+
+#[cfg(test)]
+mod tests {
+    use oci_image::{DescriptorBuilder, MediaType, Sha256Digest};
+
+    use super::*;
+
+    #[test]
+    fn test_ref_for_descriptor() {
+        let d = DescriptorBuilder::default()
+            .size(42u64)
+            .media_type(MediaType::ImageManifest)
+            .digest(
+                Sha256Digest::from_str(
+                    "2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae",
+                )
+                .unwrap(),
+            )
+            .build()
+            .unwrap();
+        assert_eq!(ref_for_layer(&d).unwrap(), "ostree/container/blob/sha256_3A_2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae");
+    }
+}

lib/src/container/unencapsulate.rs

@@ -37,7 +37,7 @@ use super::*;
 use containers_image_proxy::{ImageProxy, OpenedImage};
 use fn_error_context::context;
 use futures_util::{Future, FutureExt, TryFutureExt as _};
-use oci_spec::image as oci_image;
+use oci_spec::image::{self as oci_image, Digest};
 use std::sync::{Arc, Mutex};
 use tokio::{
     io::{AsyncBufRead, AsyncRead},
@@ -140,7 +140,7 @@ pub struct Import {
     /// The ostree commit that was imported
     pub ostree_commit: String,
     /// The image digest retrieved
-    pub image_digest: String,
+    pub image_digest: Digest,
 
     /// Any deprecation warning
     pub deprecated_warning: Option<String>,

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()

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::<Result<Vec<_>>>()?;
     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);