Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Additional Layer Store] Use TOCDigest as ID of each layer #1673

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

ktock
Copy link
Member

@ktock ktock commented May 14, 2024

Needs: containers/storage#1924 and containers/image#2416

This commit changes stargz-store to use TOCDigest as the layer specifier.
For example, the extracted view of a layer diff contents is provided in the following path:

 <mountpoint>/base64(imageref)/<TOCDigest>/diff

When an access happens to a layer, stargz store searches the layer that has the TOC matching to the specified TOCDigest.
If it fails to get the layer, stargz-store returns EIO.

Copy link

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I’m afraid I know nothing about this codebase; I could be completely off-base.)

store/manager.go Outdated
if l.Digest.String() != target.Digest.String() {
continue // This is not the target layer; nop
r.layerDigestToTOCDigestMu.Lock()
layerTOCDigest, ok := r.layerDigestToTOCDigest[refspec.String()][l.Digest.String()]
Copy link

@mtrmac mtrmac May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the manifest contains multiple layers with the same compressed digest but different TOC digests?

Note that such a situation can “legitimately” happen, where all digests validate, if an attacker decides to create such a multi-TOC layer; and the TOCs can differ.

Whether the compressedDigest is validated or not, c/image image signing relies the ALS backed enforcing exactly the TOC digest provided over the FUSE API. Looking at how resolveLayer uses the layerDigestToTOCDigest map, it seems to me that the data flow goes (TOC digest -> compressed digest (?) -> possibly-different TOC digest) and potentially uses the wrong TOC digest for some operations.

Here, getCachedLayer is called with the layerTOCDigest and not the original tocDigest. Luckily, it is followed by a Info().TOCDigest == tocDigest check, but still, the opportunity to replace the TOC digest worries me.


In principle, ((repo, compressed digest) <-> TOC digest) is an N to M mapping. It’s reasonable to assume that the same compressed digest implies the same TOC digest, except when under attack; the same TOC digest can legitimately map to different compressed digest values (in the same repo, or in different repos).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed layerDigestToTOCDigest.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock force-pushed the store-tocdigest-id branch 2 times, most recently from 81beb1b to 8d185bf Compare May 22, 2024 01:00
@ktock ktock marked this pull request as ready for review May 22, 2024 02:04
@ktock ktock marked this pull request as draft May 22, 2024 02:06
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock marked this pull request as ready for review June 19, 2024 04:06
@ktock ktock requested a review from AkihiroSuda June 19, 2024 04:06
@AkihiroSuda AkihiroSuda merged commit 7e8fc21 into containerd:main Jun 19, 2024
26 checks passed
@ktock ktock deleted the store-tocdigest-id branch June 19, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants