Merge pull request #2295 from mtrmac/diffid-non-oci-source

Cleanly fail when trying to obtain a DiffID of a non-OCI image

storage/storage_dest.go

@@ -54,14 +54,15 @@ type storageImageDestination struct {
 	stubs.ImplementsPutBlobPartial
 	stubs.AlwaysSupportsSignatures
 
-	imageRef       storageReference
-	directory      string                   // Temporary directory where we store blobs until Commit() time
-	nextTempFileID atomic.Int32             // A counter that we use for computing filenames to assign to blobs
-	manifest       []byte                   // Manifest contents, temporary
-	manifestDigest digest.Digest            // Valid if len(manifest) != 0
-	signatures     []byte                   // Signature contents, temporary
-	signatureses   map[digest.Digest][]byte // Instance signature contents, temporary
-	metadata       storageImageMetadata     // Metadata contents being built
+	imageRef              storageReference
+	directory             string                   // Temporary directory where we store blobs until Commit() time
+	nextTempFileID        atomic.Int32             // A counter that we use for computing filenames to assign to blobs
+	manifest              []byte                   // Manifest contents, temporary
+	manifestDigest        digest.Digest            // Valid if len(manifest) != 0
+	untrustedDiffIDValues []digest.Digest          // From config’s RootFS.DiffIDs, valid if not nil
+	signatures            []byte                   // Signature contents, temporary
+	signatureses          map[digest.Digest][]byte // Instance signature contents, temporary
+	metadata              storageImageMetadata     // Metadata contents being built
 
 	// Mapping from layer (by index) to the associated ID in the storage.
 	// It's protected *implicitly* since `commitLayer()`, at any given
@@ -758,30 +759,15 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D
 	if ok {
 		var untrustedUncompressedDigest digest.Digest
 		if diffOutput.UncompressedDigest == "" {
-			if s.manifest == nil {
-				logrus.Debugf("Skipping commit for layer %q, manifest not yet available", newLayerID)
-				return nil, nil
-			}
-
-			man, err := manifest.FromBlob(s.manifest, manifest.GuessMIMEType(s.manifest))
-			if err != nil {
-				return nil, fmt.Errorf("parsing manifest: %w", err)
-			}
-
-			cb, err := s.getConfigBlob(man.ConfigInfo())
+			d, err := s.untrustedLayerDiffID(index)
 			if err != nil {
 				return nil, err
 			}
-
-			// retrieve the expected uncompressed digest from the config blob.
-			configOCI := &imgspecv1.Image{}
-			if err := json.Unmarshal(cb, configOCI); err != nil {
-				return nil, err
-			}
-			if index >= len(configOCI.RootFS.DiffIDs) {
-				return nil, fmt.Errorf("index %d out of range for configOCI.RootFS.DiffIDs", index)
+			if d == "" {
+				logrus.Debugf("Skipping commit for layer %q, manifest not yet available", newLayerID)
+				return nil, nil
 			}
-			untrustedUncompressedDigest = configOCI.RootFS.DiffIDs[index]
+			untrustedUncompressedDigest = d
 		}
 
 		layer, err := s.imageRef.transport.store.CreateLayer(newLayerID, parentLayer, nil, "", false, nil)
@@ -936,6 +922,55 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D
 	return layer, nil
 }
 
+// untrustedLayerDiffID returns a DiffID value for layerIndex from the image’s config.
+// If the value is not yet available (but it can be available after s.manifets is set), it returns ("", nil).
+// WARNING: We don’t validate the DiffID value against the layer contents; it must not be used for any deduplication.
+func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.Digest, error) {
+	// At this point, we are either inside the multi-threaded scope of HasThreadSafePutBlob, and
+	// nothing is writing to s.manifest yet, or PutManifest has been called and s.manifest != nil.
+	// Either way this function does not need the protection of s.lock.
+	if s.manifest == nil {
+		logrus.Debugf("Skipping commit for layer %d, manifest not yet available", layerIndex)
+		return "", nil
+	}
+
+	if s.untrustedDiffIDValues == nil {
+		mt := manifest.GuessMIMEType(s.manifest)
+		if mt != imgspecv1.MediaTypeImageManifest {
+			// We could, in principle, build an ImageSource, support arbitrary image formats using image.FromUnparsedImage,
+			// and then use types.Image.OCIConfig so that we can parse the image.
+			//
+			// In practice, this should, right now, only matter for pulls of OCI images (this code path implies that a layer has annotation),
+			// while converting to a non-OCI formats, using a manual (skopeo copy) or something similar, not (podman pull).
+			// So it is not implemented yet.
+			return "", fmt.Errorf("determining DiffID for manifest type %q is not yet supported", mt)
+		}
+		man, err := manifest.FromBlob(s.manifest, mt)
+		if err != nil {
+			return "", fmt.Errorf("parsing manifest: %w", err)
+		}
+
+		cb, err := s.getConfigBlob(man.ConfigInfo())
+		if err != nil {
+			return "", err
+		}
+
+		// retrieve the expected uncompressed digest from the config blob.
+		configOCI := &imgspecv1.Image{}
+		if err := json.Unmarshal(cb, configOCI); err != nil {
+			return "", err
+		}
+		s.untrustedDiffIDValues = slices.Clone(configOCI.RootFS.DiffIDs)
+		if s.untrustedDiffIDValues == nil { // Unlikely but possible in theory…
+			s.untrustedDiffIDValues = []digest.Digest{}
+		}
+	}
+	if layerIndex >= len(s.untrustedDiffIDValues) {
+		return "", fmt.Errorf("image config has only %d DiffID values, but a layer with index %d exists", len(s.untrustedDiffIDValues), layerIndex)
+	}
+	return s.untrustedDiffIDValues[layerIndex], nil
+}
+
 // Commit marks the process of storing the image as successful and asks for the image to be persisted.
 // unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
 // if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the