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] 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) }