From 3beea1e21e9bbefab97a69577d4ae827980e0e30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 13 Apr 2024 16:22:40 +0200 Subject: [PATCH 1/3] Only obtain the estargz TOC digest once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make it structually clear that the code is all using the same value, making it less likely for the verifier and other uses to get out of sync. Also avoids some redundant parsing and error paths. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/chunked/compression_linux.go | 9 ++------- pkg/chunked/storage_linux.go | 17 ++++++++--------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/pkg/chunked/compression_linux.go b/pkg/chunked/compression_linux.go index 112ca2c7c4..d5a478189d 100644 --- a/pkg/chunked/compression_linux.go +++ b/pkg/chunked/compression_linux.go @@ -7,7 +7,6 @@ import ( "io" "strconv" - "github.com/containerd/stargz-snapshotter/estargz" "github.com/containers/storage/pkg/chunked/internal" "github.com/klauspost/compress/zstd" "github.com/klauspost/pgzip" @@ -33,7 +32,7 @@ func typeToTarType(t string) (byte, error) { return r, nil } -func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, annotations map[string]string) ([]byte, int64, error) { +func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, tocDigest digest.Digest) ([]byte, int64, error) { // information on the format here https://github.com/containerd/stargz-snapshotter/blob/main/docs/stargz-estargz.md footerSize := int64(51) if blobSize <= footerSize { @@ -126,11 +125,7 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, return nil, 0, err } - d, err := digest.Parse(annotations[estargz.TOCJSONDigestAnnotation]) - if err != nil { - return nil, 0, err - } - if manifestDigester.Digest() != d { + if manifestDigester.Digest() != tocDigest { return nil, 0, errors.New("invalid manifest checksum") } diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index 7bbfd7bedd..05fb51f465 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -265,7 +265,7 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges } _, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey] - _, hasEstargzTOC := annotations[estargz.TOCJSONDigestAnnotation] + estargzTOCDigestString, hasEstargzTOC := annotations[estargz.TOCJSONDigestAnnotation] if hasZstdChunkedTOC && hasEstargzTOC { return nil, errors.New("both zstd:chunked and eStargz TOC found") @@ -275,7 +275,11 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges return makeZstdChunkedDiffer(ctx, store, blobSize, annotations, iss, &storeOpts) } if hasEstargzTOC { - return makeEstargzChunkedDiffer(ctx, store, blobSize, annotations, iss, &storeOpts) + estargzTOCDigest, err := digest.Parse(estargzTOCDigestString) + if err != nil { + return nil, fmt.Errorf("parsing estargz TOC digest %q: %w", estargzTOCDigestString, err) + } + return makeEstargzChunkedDiffer(ctx, store, blobSize, estargzTOCDigest, iss, &storeOpts) } return makeConvertFromRawDiffer(ctx, store, blobDigest, blobSize, annotations, iss, &storeOpts) @@ -333,8 +337,8 @@ func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize in }, nil } -func makeEstargzChunkedDiffer(ctx context.Context, store storage.Store, blobSize int64, annotations map[string]string, iss ImageSourceSeekable, storeOpts *types.StoreOptions) (*chunkedDiffer, error) { - manifest, tocOffset, err := readEstargzChunkedManifest(iss, blobSize, annotations) +func makeEstargzChunkedDiffer(ctx context.Context, store storage.Store, blobSize int64, tocDigest digest.Digest, iss ImageSourceSeekable, storeOpts *types.StoreOptions) (*chunkedDiffer, error) { + manifest, tocOffset, err := readEstargzChunkedManifest(iss, blobSize, tocDigest) if err != nil { return nil, fmt.Errorf("read zstd:chunked manifest: %w", err) } @@ -343,11 +347,6 @@ func makeEstargzChunkedDiffer(ctx context.Context, store storage.Store, blobSize return nil, err } - tocDigest, err := digest.Parse(annotations[estargz.TOCJSONDigestAnnotation]) - if err != nil { - return nil, fmt.Errorf("parse TOC digest %q: %w", annotations[estargz.TOCJSONDigestAnnotation], err) - } - return &chunkedDiffer{ fsVerityDigests: make(map[string]string), blobSize: blobSize, From 1f47b38c0949e1d0f4099e3ba5b6b7659a462220 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 13 Apr 2024 16:39:28 +0200 Subject: [PATCH 2/3] Only obtain the zstd:chunked TOC digest once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make it structually clear that the code is all using the same value, making it less likely for the verifier and other uses to get out of sync. Also avoids some redundant parsing and error paths. The conversion path looks longer, but that's just moving the parsing from the called function (which is redundant for other callers). Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/chunked/compression_linux.go | 4 ++-- pkg/chunked/internal/compression.go | 7 ++----- pkg/chunked/storage_linux.go | 27 +++++++++++++++++---------- pkg/chunked/zstdchunked_test.go | 7 ++++++- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/pkg/chunked/compression_linux.go b/pkg/chunked/compression_linux.go index d5a478189d..a191b8cb02 100644 --- a/pkg/chunked/compression_linux.go +++ b/pkg/chunked/compression_linux.go @@ -135,7 +135,7 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, // readZstdChunkedManifest reads the zstd:chunked manifest from the seekable stream blobStream. The blob total size must // be specified. // This function uses the io.github.containers.zstd-chunked. annotations when specified. -func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, annotations map[string]string) ([]byte, []byte, int64, error) { +func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, tocDigest digest.Digest, annotations map[string]string) ([]byte, []byte, int64, error) { footerSize := int64(internal.FooterSizeSupported) if blobSize <= footerSize { return nil, nil, 0, errors.New("blob too small") @@ -145,7 +145,7 @@ func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, ann if offsetMetadata := annotations[internal.ManifestInfoKey]; offsetMetadata != "" { var err error - footerData, err = internal.ReadFooterDataFromAnnotations(annotations) + footerData, err = internal.ReadFooterDataFromAnnotations(tocDigest, annotations) if err != nil { return nil, nil, 0, err } diff --git a/pkg/chunked/internal/compression.go b/pkg/chunked/internal/compression.go index 1136d67b80..bbbf6e3e77 100644 --- a/pkg/chunked/internal/compression.go +++ b/pkg/chunked/internal/compression.go @@ -231,13 +231,10 @@ func footerDataToBlob(footer ZstdChunkedFooterData) []byte { } // ReadFooterDataFromAnnotations reads the zstd:chunked footer data from the given annotations. -func ReadFooterDataFromAnnotations(annotations map[string]string) (ZstdChunkedFooterData, error) { +func ReadFooterDataFromAnnotations(tocDigest digest.Digest, annotations map[string]string) (ZstdChunkedFooterData, error) { var footerData ZstdChunkedFooterData - footerData.ChecksumAnnotation = annotations[ManifestChecksumKey] - if footerData.ChecksumAnnotation == "" { - return footerData, fmt.Errorf("manifest checksum annotation %q not found", ManifestChecksumKey) - } + footerData.ChecksumAnnotation = tocDigest.String() offsetMetadata := annotations[ManifestInfoKey] diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index 05fb51f465..2c49967c72 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -25,6 +25,7 @@ import ( "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chunked/compressor" "github.com/containers/storage/pkg/chunked/internal" + "github.com/containers/storage/pkg/chunked/toc" "github.com/containers/storage/pkg/fsverity" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/system" @@ -264,7 +265,7 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges return nil, errors.New("enable_partial_images not configured") } - _, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey] + zstdChunkedTOCDigestString, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey] estargzTOCDigestString, hasEstargzTOC := annotations[estargz.TOCJSONDigestAnnotation] if hasZstdChunkedTOC && hasEstargzTOC { @@ -272,7 +273,11 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges } if hasZstdChunkedTOC { - return makeZstdChunkedDiffer(ctx, store, blobSize, annotations, iss, &storeOpts) + zstdChunkedTOCDigest, err := digest.Parse(zstdChunkedTOCDigestString) + if err != nil { + return nil, fmt.Errorf("parsing zstd:chunked TOC digest %q: %w", zstdChunkedTOCDigestString, err) + } + return makeZstdChunkedDiffer(ctx, store, blobSize, zstdChunkedTOCDigest, annotations, iss, &storeOpts) } if hasEstargzTOC { estargzTOCDigest, err := digest.Parse(estargzTOCDigestString) @@ -307,8 +312,8 @@ func makeConvertFromRawDiffer(ctx context.Context, store storage.Store, blobDige }, nil } -func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize int64, annotations map[string]string, iss ImageSourceSeekable, storeOpts *types.StoreOptions) (*chunkedDiffer, error) { - manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, blobSize, annotations) +func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize int64, tocDigest digest.Digest, annotations map[string]string, iss ImageSourceSeekable, storeOpts *types.StoreOptions) (*chunkedDiffer, error) { + manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, blobSize, tocDigest, annotations) if err != nil { return nil, fmt.Errorf("read zstd:chunked manifest: %w", err) } @@ -317,11 +322,6 @@ func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize in return nil, err } - tocDigest, err := digest.Parse(annotations[internal.ManifestChecksumKey]) - if err != nil { - return nil, fmt.Errorf("parse TOC digest %q: %w", annotations[internal.ManifestChecksumKey], err) - } - return &chunkedDiffer{ fsVerityDigests: make(map[string]string), blobSize: blobSize, @@ -1691,7 +1691,14 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff blobFile.Close() blobFile = nil - manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(fileSource, c.blobSize, annotations) + tocDigest, err := toc.GetTOCDigest(annotations) + if err != nil { + return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("internal error: parsing just-created zstd:chunked TOC digest: %w", err) + } + if tocDigest == nil { + return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("internal error: just-created zstd:chunked missing TOC digest") + } + manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(fileSource, c.blobSize, *tocDigest, annotations) if err != nil { return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("read zstd:chunked manifest: %w", err) } diff --git a/pkg/chunked/zstdchunked_test.go b/pkg/chunked/zstdchunked_test.go index 8041db9a2e..ec1edffcf2 100644 --- a/pkg/chunked/zstdchunked_test.go +++ b/pkg/chunked/zstdchunked_test.go @@ -12,8 +12,10 @@ import ( "testing" "github.com/containers/storage/pkg/chunked/internal" + "github.com/containers/storage/pkg/chunked/toc" "github.com/klauspost/compress/zstd" "github.com/opencontainers/go-digest" + "github.com/stretchr/testify/require" ) type seekable struct { @@ -148,7 +150,10 @@ func TestGenerateAndParseManifest(t *testing.T) { t: t, } - manifest, _, _, err := readZstdChunkedManifest(s, 8192, annotations) + tocDigest, err := toc.GetTOCDigest(annotations) + require.NoError(t, err) + require.NotNil(t, tocDigest) + manifest, _, _, err := readZstdChunkedManifest(s, 8192, *tocDigest, annotations) if err != nil { t.Error(err) } From 053ac6105dfe7e11b2e7d7f6cc2a11f71d9c5c57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 13 Apr 2024 16:49:11 +0200 Subject: [PATCH 3/3] Remove ChecksumAnntation from ZstdChunkedFooterData MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manage the value directly to simplify. This happens to fix the ReadFooterDataFromBlob code path, which was not setting ChecksumAnntation at all. Signed-off-by: Miloslav Trmač --- pkg/chunked/compression_linux.go | 4 ++-- pkg/chunked/internal/compression.go | 6 +----- pkg/chunked/internal/compression_test.go | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/pkg/chunked/compression_linux.go b/pkg/chunked/compression_linux.go index a191b8cb02..38a892a6ef 100644 --- a/pkg/chunked/compression_linux.go +++ b/pkg/chunked/compression_linux.go @@ -145,7 +145,7 @@ func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, toc if offsetMetadata := annotations[internal.ManifestInfoKey]; offsetMetadata != "" { var err error - footerData, err = internal.ReadFooterDataFromAnnotations(tocDigest, annotations) + footerData, err = internal.ReadFooterDataFromAnnotations(annotations) if err != nil { return nil, nil, 0, err } @@ -233,7 +233,7 @@ func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, toc return nil, nil, 0, err } - decodedBlob, err := decodeAndValidateBlob(manifest, footerData.LengthUncompressed, footerData.ChecksumAnnotation) + decodedBlob, err := decodeAndValidateBlob(manifest, footerData.LengthUncompressed, tocDigest.String()) if err != nil { return nil, nil, 0, err } diff --git a/pkg/chunked/internal/compression.go b/pkg/chunked/internal/compression.go index bbbf6e3e77..f52a07a9f8 100644 --- a/pkg/chunked/internal/compression.go +++ b/pkg/chunked/internal/compression.go @@ -183,7 +183,6 @@ func WriteZstdChunkedManifest(dest io.Writer, outMetadata map[string]string, off Offset: manifestOffset, LengthCompressed: uint64(len(compressedManifest)), LengthUncompressed: uint64(len(manifest)), - ChecksumAnnotation: "", // unused OffsetTarSplit: uint64(tarSplitOffset), LengthCompressedTarSplit: uint64(len(tarSplitData.Data)), LengthUncompressedTarSplit: uint64(tarSplitData.UncompressedSize), @@ -207,7 +206,6 @@ type ZstdChunkedFooterData struct { Offset uint64 LengthCompressed uint64 LengthUncompressed uint64 - ChecksumAnnotation string // Only used when reading a layer, not when creating it OffsetTarSplit uint64 LengthCompressedTarSplit uint64 @@ -231,11 +229,9 @@ func footerDataToBlob(footer ZstdChunkedFooterData) []byte { } // ReadFooterDataFromAnnotations reads the zstd:chunked footer data from the given annotations. -func ReadFooterDataFromAnnotations(tocDigest digest.Digest, annotations map[string]string) (ZstdChunkedFooterData, error) { +func ReadFooterDataFromAnnotations(annotations map[string]string) (ZstdChunkedFooterData, error) { var footerData ZstdChunkedFooterData - footerData.ChecksumAnnotation = tocDigest.String() - offsetMetadata := annotations[ManifestInfoKey] if _, err := fmt.Sscanf(offsetMetadata, "%d:%d:%d:%d", &footerData.Offset, &footerData.LengthCompressed, &footerData.LengthUncompressed, &footerData.ManifestType); err != nil { diff --git a/pkg/chunked/internal/compression_test.go b/pkg/chunked/internal/compression_test.go index da660b89ba..9d4e60d47c 100644 --- a/pkg/chunked/internal/compression_test.go +++ b/pkg/chunked/internal/compression_test.go @@ -15,7 +15,6 @@ func TestGenerateAndReadFooter(t *testing.T) { Offset: 2, LengthCompressed: 3, LengthUncompressed: 4, - ChecksumAnnotation: "", // unused OffsetTarSplit: 5, LengthCompressedTarSplit: 6, LengthUncompressedTarSplit: 7,