copy: do not fail if digest mismatches #1980

giuseppe · 2023-06-01T21:38:22Z

if the computed digest mismatches the expected one for a partial image. This is expected since partial images are stored by their TOC digest, not the diffID for the layer.

mtrmac

Unrestricted like this, the change would completely break the security of digest references, and signatures.

It’s reasonable to contemplate that in some cases, we need a non-digest layer identifier of some kind; currently, in some transports we need to compute a digest just to have a digest in a manifest and create a consumable ImageSource. But the assumption that layers are content-addressable and always have a digest is deeply and widely spread throughout the codebase, and we would need to both find a replacement for the current image integrity assumptions (or to decide that one isn’t necessary), and to carefully migrate code away from the existing content-addressability assumption.

giuseppe · 2023-06-05T10:36:18Z

Unrestricted like this, the change would completely break the security of digest references, and signatures.

It’s reasonable to contemplate that in some cases, we need a non-digest layer identifier of some kind; currently, in some transports we need to compute a digest just to have a digest in a manifest and create a consumable ImageSource. But the assumption that layers are content-addressable and always have a digest is deeply and widely spread throughout the codebase, and we would need to both find a replacement for the current image integrity assumptions (or to decide that one isn’t necessary), and to carefully migrate code away from the existing content-addressability assumption.

I've changed the implementation to use this behavior only for layers that were retrieved using the partial pull feature.

It needs this change in c/storage: containers/storage#1627 in particular the patch "store: store if the layer was created with a partial pull"

mtrmac

Most importantly, I’m much more worried about the pull path and layer deduplication. Let’s figure out how to do that first, and that will clarify the digests situation. Making the push path work should only come afterwards.

types/types.go

giuseppe · 2023-06-05T13:29:56Z

Most importantly, I’m much more worried about the pull path and layer deduplication. Let’s figure out how to do that first, and that will clarify the digests situation. Making the push path work should only come afterwards.

the pull should not be a problem (just a little bit more expensive) since we will end up requesting the TOC for the layer but then all files are already present locally, so it won't be a problem.

If you refer to having the zstd:chunked version and the gzip version being deduplicated, I think we should not worry about this case, the long term goal is to replace the gzip format, so even if for now we end up in the worse case with two layers, that is fine.

giuseppe · 2023-06-20T14:02:30Z

@mtrmac what do you think of this approach?

giuseppe · 2023-07-04T15:49:03Z

if there are no big objections can we move forward with this? We can rework it again if necessary, but without this fix a partial layer is pulled every time we do a pull

giuseppe · 2023-07-04T15:49:19Z

@mtrmac @rhatdan @vrothberg PTAL

vrothberg

Code LGTM but I want @mtrmac to chime in. He's partially unavailable this week.

copy/blob.go

giuseppe · 2023-07-28T13:54:45Z

rebased

vrothberg

LGTM
Nit: the last commit message still points to the warning that has been dropped

copy/single.go

storage/storage_dest.go

mtrmac

As partially a note to self, and to be clear there is progress:

Looking only at storage_src.go: overall approach and security LGTM.

storage/storage_src.go

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

add a new field to TryReusingBlobOptions to store the digest of the TOC for the layer. It will be used by later commits to enable looking up a partial layer. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

and generalize its naming to cover also the TOC digest, that is added by a later commit. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

this is a preparation for the next commit to clarify that the map can contain either the uncompressed digest for the layer (DiffID), or the digest for the TOC of the layer. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

when an image using partial layers is pulled, generate a different imageID that combines the digest of the TOC for the partial layers since the partial pull didn't validate the diffID. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

when copying a partial image, store the expected diffID so that it can be later used to validate the obtained layer stream. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

mtrmac · 2023-12-06T12:33:22Z

storage/storage_src.go

+				if err != nil {
+					return nil, fmt.Errorf("parsing expected diffID %q for layer %q: %w", expectedDigest, layerID, err)
+				}
+			} else {


Non-blocking nit: Returning early on this failure, and leaving the non-failure case less indented, would be a tiny bit more readable.

mtrmac · 2023-12-06T12:33:56Z

storage/storage_src.go

+				// if the layer is stored by its TOC, report the expected diffID as the layer Digest
+				// but store the TOC digest so we can later retrieve it from the storage.


This comment is no longer accurate now that we record digestToLayerID.

mtrmac · 2023-12-06T12:36:02Z

storage/storage_src.go

+	// If the digest was overriden by LayerInfosForCopy, then we need to use the TOC digest
+	// to retrieve it from the storage.


This comment needs updating.

storage/storage_src.go

mtrmac

Implementation of storage_src.go LGTM.

mtrmac

Another partial note, going to read storage_dest.go now.

copy/single.go

manifest/oci.go

copy/single.go

mtrmac

Thanks for containers/storage#1719 ; with that, I think there is a clean path for storageImageDestination to be unambiguous, see below about primarily keeping the state in a map[int]addedLayerInfo.

Admittedly it’s all a little vague/tentative at this point, do let me know if I’m missing something essential or if the attempt to sketch the idea is not clear enough to be understood.

storage/storage_dest.go

rhatdan · 2023-12-06T15:45:37Z

As I understand it during the meeting last week, we came to the agreement that merging this PR plus fixups from @giuseppe going forward, is the easiest path forward. I will merge, and then we can continue working on small fixups.

…1114) This PR contains the following updates: Package Change Age Adoption Passing Confidence github.com/containers/image/v5 v5.29.2 -> v5.30.0 WarningSome dependencies could not be looked up. Check the warning logs for more information. Release Notes containers/image (github.com/containers/image/v5) v5.30.0 Compare Source What's Changed A fair number of improvements when working with zstd and zstd:chunked-compressed images. Note that make install now installs policy.json and registries.d/default.yaml. Refuse compression to zstd when using schema1 by @mtrmac in containers/image#2196 Don't expose local account details in oci-archive tar files by @mtrmac in containers/image#2202 Trigger a conversion to OCI when compressing to Zstd by @mtrmac in containers/image#2204 Add buildtags to avoid fulcio and rekor dependencies by @siretart in containers/image#2180 copy: do not fail if digest mismatches by @giuseppe in containers/image#1980 Moving policy.json and default.yaml from containers/skopeo by @rahilarious in containers/image#2215 Embrace codespell: config, workflow (to alert when new typos added) and get typos fixed by @yarikoptic in containers/image#2214 Fix raspberry pi zero cpu variant recognition by @lstolcman in containers/image#2086 storage: validate images converted to zstd:chunked by @giuseppe in containers/image#2243 Make blob reuse choices manifest-format-sensitive, and allow conversions when writing to format-agnostic transports by @mtrmac in containers/image#2213 Edit the manifest when pushing uncompressed data from c/storage by @mtrmac in containers/image#2273 Random storage-related cleanups by @mtrmac in containers/image#2287 Improve storage transport documentation, primarily about locking by @mtrmac in containers/image#2291 Fix c/storage destination with partial pulls by @mtrmac in containers/image#2288 Fix manifest updates when we match a layer by TOC digest by @mtrmac in containers/image#2294 Cleanly fail when trying to obtain a DiffID of a non-OCI image by @mtrmac in containers/image#2295 Beautify TOC-related parts of storageImageSource by @mtrmac in containers/image#2296 storage: use the new ApplyStagedLayer interface by @giuseppe in containers/image#2301 Also annotate image instances using zstd:chunked as using zstd by @mtrmac in containers/image#2302 Support editing ArtifactType, preserve it in lists by @nalind in containers/image#2304 Provide data to correctly report throughput on partial pulls by @mtrmac in containers/image#2308 Add validation error to digesting reader by @saschagrunert in containers/image#2312 Fix handling of errors when fetching layers by URLs by @mtrmac in containers/image#2310 Improve handling of zstd vs. zstd:chunked matching by @mtrmac in containers/image#2317 New Contributors @rahilarious made their first contribution in containers/image#2215 @yarikoptic made their first contribution in containers/image#2214 @lstolcman made their first contribution in containers/image#2086 @bainsy88 made their first contribution in containers/image#2260 Full Changelog: containers/image@v5.29.2...v5.30.0 Configuration 📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied. ♻ Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 Ignore: Close this PR and you won't be reminded about this update again. If you want to rebase/retry this PR, check this box This PR has been generated by Mend Renovate. View repository job log here.

mtrmac requested changes Jun 2, 2023

View reviewed changes

giuseppe force-pushed the use-computed-digest branch from 6033259 to 8ea3ae3 Compare June 5, 2023 10:35

mtrmac reviewed Jun 5, 2023

View reviewed changes

types/types.go Outdated Show resolved Hide resolved

giuseppe force-pushed the use-computed-digest branch 2 times, most recently from 9a87d2e to 851f840 Compare June 6, 2023 13:57

giuseppe force-pushed the use-computed-digest branch 4 times, most recently from 87f1a5d to ee10af7 Compare June 20, 2023 10:57

giuseppe marked this pull request as ready for review June 20, 2023 14:02

giuseppe changed the title ~~[RFC] copy: do not fail if digest mismatches~~ copy: do not fail if digest mismatches Jun 20, 2023

giuseppe force-pushed the use-computed-digest branch 2 times, most recently from 3cd6552 to efc51d3 Compare July 4, 2023 15:43

vrothberg reviewed Jul 5, 2023

View reviewed changes

copy/blob.go Outdated Show resolved Hide resolved

giuseppe force-pushed the use-computed-digest branch 3 times, most recently from e503165 to d3e4c0b Compare July 24, 2023 10:08

giuseppe marked this pull request as draft July 25, 2023 10:49

giuseppe force-pushed the use-computed-digest branch from d3e4c0b to c498b8f Compare July 25, 2023 12:24

giuseppe marked this pull request as ready for review July 25, 2023 12:24

giuseppe force-pushed the use-computed-digest branch from c498b8f to d9b3d48 Compare July 28, 2023 13:53

flouthoc mentioned this pull request Aug 2, 2023

manifest, push: implement --add-compression to push with compressed variants. containers/buildah#4912

Merged

vrothberg approved these changes Aug 7, 2023

View reviewed changes

mtrmac requested changes Aug 7, 2023

View reviewed changes

copy/single.go Outdated Show resolved Hide resolved

copy/single.go Outdated Show resolved Hide resolved

copy/single.go Outdated Show resolved Hide resolved

storage/storage_dest.go Outdated Show resolved Hide resolved

storage/storage_dest.go Outdated Show resolved Hide resolved

giuseppe force-pushed the use-computed-digest branch 3 times, most recently from e600ada to cbea9be Compare December 1, 2023 21:11

mtrmac reviewed Dec 5, 2023

View reviewed changes

giuseppe added 7 commits December 5, 2023 22:08

go.mod: update c/storage

d975c48

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

storage: accept layers without UncompressedDigest

10ee154

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

copy: specify the TOC digest from layer annotations

372f14f

add a new field to TryReusingBlobOptions to store the digest of the TOC for the layer. It will be used by later commits to enable looking up a partial layer. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

storage: add helper function to get diffID

49f6483

and generalize its naming to cover also the TOC digest, that is added by a later commit. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

storage: rename blobDiffIDs to uncompressedOrTocDigest

f240c49

this is a preparation for the next commit to clarify that the map can contain either the uncompressed digest for the layer (DiffID), or the digest for the TOC of the layer. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

storage: use the TOC digest for partial layers

919d43a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the use-computed-digest branch 3 times, most recently from 0bd2e0c to 1032469 Compare December 5, 2023 21:23

storage: store the diffID and use for validation

b15a850

when copying a partial image, store the expected diffID so that it can be later used to validate the obtained layer stream. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the use-computed-digest branch from 1032469 to b15a850 Compare December 6, 2023 08:35

giuseppe marked this pull request as ready for review December 6, 2023 08:36

mtrmac reviewed Dec 6, 2023

View reviewed changes

copy/single.go Show resolved Hide resolved

manifest/oci.go Show resolved Hide resolved

mtrmac reviewed Dec 6, 2023

View reviewed changes

copy/single.go Show resolved Hide resolved

mtrmac reviewed Dec 6, 2023

View reviewed changes

rhatdan merged commit 28a299f into containers:main Dec 6, 2023
9 checks passed

giuseppe mentioned this pull request Dec 6, 2023

podman push --compression-format=zstd:chunked generates invalid OCI images containers/storage#1771

Closed

mtrmac mentioned this pull request Jan 5, 2024

storage, dest: clarify when TOCDigest is used #2218

Closed

mtrmac mentioned this pull request Jan 16, 2024

Image ID not consistent between Podman and Docker in image history containers/podman#21198

Open

mtrmac mentioned this pull request Jan 25, 2024

Fix image filters parsing containers/podman#21260

Merged

mtrmac mentioned this pull request Feb 15, 2024

Beautify TOC-related parts of storageImageSource #2296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copy: do not fail if digest mismatches #1980

copy: do not fail if digest mismatches #1980

giuseppe commented Jun 1, 2023 •

edited

Loading

mtrmac left a comment

giuseppe commented Jun 5, 2023

mtrmac left a comment

giuseppe commented Jun 5, 2023

giuseppe commented Jun 20, 2023

giuseppe commented Jul 4, 2023

giuseppe commented Jul 4, 2023

vrothberg left a comment

giuseppe commented Jul 28, 2023

vrothberg left a comment

mtrmac left a comment

mtrmac Dec 6, 2023

mtrmac Dec 6, 2023

mtrmac Dec 6, 2023

mtrmac left a comment

mtrmac left a comment

mtrmac left a comment

rhatdan commented Dec 6, 2023

		// if the layer is stored by its TOC, report the expected diffID as the layer Digest
		// but store the TOC digest so we can later retrieve it from the storage.

		// If the digest was overriden by LayerInfosForCopy, then we need to use the TOC digest
		// to retrieve it from the storage.

copy: do not fail if digest mismatches #1980

copy: do not fail if digest mismatches #1980

Conversation

giuseppe commented Jun 1, 2023 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

giuseppe commented Jun 5, 2023

mtrmac left a comment

Choose a reason for hiding this comment

giuseppe commented Jun 5, 2023

giuseppe commented Jun 20, 2023

giuseppe commented Jul 4, 2023

giuseppe commented Jul 4, 2023

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe commented Jul 28, 2023

vrothberg left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Dec 6, 2023

Choose a reason for hiding this comment

mtrmac Dec 6, 2023

Choose a reason for hiding this comment

mtrmac Dec 6, 2023

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

rhatdan commented Dec 6, 2023

giuseppe commented Jun 1, 2023 •

edited

Loading