Merge pull request #1901 from mtrmac/wo-binary-footer

Make ZstdChunkedFooterData write-only

pkg/chunked/compression_linux.go

@@ -132,77 +132,44 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64,
 	return manifestUncompressed, tocOffset, nil
 }
 
-// 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, 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")
+// readZstdChunkedManifest reads the zstd:chunked manifest from the seekable stream blobStream.
+func readZstdChunkedManifest(blobStream ImageSourceSeekable, tocDigest digest.Digest, annotations map[string]string) ([]byte, []byte, int64, error) {
+	offsetMetadata := annotations[internal.ManifestInfoKey]
+	if offsetMetadata == "" {
+		return nil, nil, 0, fmt.Errorf("%q annotation missing", internal.ManifestInfoKey)
+	}
+	var manifestChunk ImageSourceChunk
+	var manifestLengthUncompressed, manifestType uint64
+	if _, err := fmt.Sscanf(offsetMetadata, "%d:%d:%d:%d", &manifestChunk.Offset, &manifestChunk.Length, &manifestLengthUncompressed, &manifestType); err != nil {
+		return nil, nil, 0, err
 	}
-
-	var footerData internal.ZstdChunkedFooterData
-
-	if offsetMetadata := annotations[internal.ManifestInfoKey]; offsetMetadata != "" {
-		var err error
-		footerData, err = internal.ReadFooterDataFromAnnotations(annotations)
-		if err != nil {
-			return nil, nil, 0, err
-		}
-	} else {
-		chunk := ImageSourceChunk{
-			Offset: uint64(blobSize - footerSize),
-			Length: uint64(footerSize),
-		}
-		parts, errs, err := blobStream.GetBlobAt([]ImageSourceChunk{chunk})
-		if err != nil {
-			return nil, nil, 0, err
-		}
-		var reader io.ReadCloser
-		select {
-		case r := <-parts:
-			reader = r
-		case err := <-errs:
-			return nil, nil, 0, err
-		}
-		footer := make([]byte, footerSize)
-		if _, err := io.ReadFull(reader, footer); err != nil {
-			return nil, nil, 0, err
-		}
-
-		footerData, err = internal.ReadFooterDataFromBlob(footer)
-		if err != nil {
+	// The tarSplit… values are valid if tarSplitChunk.Offset > 0
+	var tarSplitChunk ImageSourceChunk
+	var tarSplitLengthUncompressed uint64
+	var tarSplitChecksum string
+	if tarSplitInfoKeyAnnotation, found := annotations[internal.TarSplitInfoKey]; found {
+		if _, err := fmt.Sscanf(tarSplitInfoKeyAnnotation, "%d:%d:%d", &tarSplitChunk.Offset, &tarSplitChunk.Length, &tarSplitLengthUncompressed); err != nil {
 			return nil, nil, 0, err
 		}
+		tarSplitChecksum = annotations[internal.TarSplitChecksumKey]
 	}
 
-	if footerData.ManifestType != internal.ManifestTypeCRFS {
+	if manifestType != internal.ManifestTypeCRFS {
 		return nil, nil, 0, errors.New("invalid manifest type")
 	}
 
 	// set a reasonable limit
-	if footerData.LengthCompressed > (1<<20)*50 {
+	if manifestChunk.Length > (1<<20)*50 {
 		return nil, nil, 0, errors.New("manifest too big")
 	}
-	if footerData.LengthUncompressed > (1<<20)*50 {
+	if manifestLengthUncompressed > (1<<20)*50 {
 		return nil, nil, 0, errors.New("manifest too big")
 	}
 
-	chunk := ImageSourceChunk{
-		Offset: footerData.Offset,
-		Length: footerData.LengthCompressed,
+	chunks := []ImageSourceChunk{manifestChunk}
+	if tarSplitChunk.Offset > 0 {
+		chunks = append(chunks, tarSplitChunk)
 	}
-
-	chunks := []ImageSourceChunk{chunk}
-
-	if footerData.OffsetTarSplit > 0 {
-		chunkTarSplit := ImageSourceChunk{
-			Offset: footerData.OffsetTarSplit,
-			Length: footerData.LengthCompressedTarSplit,
-		}
-		chunks = append(chunks, chunkTarSplit)
-	}
-
 	parts, errs, err := blobStream.GetBlobAt(chunks)
 	if err != nil {
 		return nil, nil, 0, err
@@ -228,28 +195,28 @@ func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, toc
 		return blob, nil
 	}
 
-	manifest, err := readBlob(footerData.LengthCompressed)
+	manifest, err := readBlob(manifestChunk.Length)
 	if err != nil {
 		return nil, nil, 0, err
 	}
 
-	decodedBlob, err := decodeAndValidateBlob(manifest, footerData.LengthUncompressed, tocDigest.String())
+	decodedBlob, err := decodeAndValidateBlob(manifest, manifestLengthUncompressed, tocDigest.String())
 	if err != nil {
 		return nil, nil, 0, err
 	}
 	decodedTarSplit := []byte{}
-	if footerData.OffsetTarSplit > 0 {
-		tarSplit, err := readBlob(footerData.LengthCompressedTarSplit)
+	if tarSplitChunk.Offset > 0 {
+		tarSplit, err := readBlob(tarSplitChunk.Length)
 		if err != nil {
 			return nil, nil, 0, err
 		}
 
-		decodedTarSplit, err = decodeAndValidateBlob(tarSplit, footerData.LengthUncompressedTarSplit, footerData.ChecksumAnnotationTarSplit)
+		decodedTarSplit, err = decodeAndValidateBlob(tarSplit, tarSplitLengthUncompressed, tarSplitChecksum)
 		if err != nil {
 			return nil, nil, 0, err
 		}
 	}
-	return decodedBlob, decodedTarSplit, int64(footerData.Offset), err
+	return decodedBlob, decodedTarSplit, int64(manifestChunk.Offset), err
 }
 
 func decodeAndValidateBlob(blob []byte, lengthUncompressed uint64, expectedCompressedChecksum string) ([]byte, error) {

pkg/chunked/internal/compression.go

@@ -8,7 +8,6 @@ import (
 	"archive/tar"
 	"bytes"
 	"encoding/binary"
-	"errors"
 	"fmt"
 	"io"
 	"time"
@@ -186,7 +185,6 @@ func WriteZstdChunkedManifest(dest io.Writer, outMetadata map[string]string, off
 		OffsetTarSplit:             uint64(tarSplitOffset),
 		LengthCompressedTarSplit:   uint64(len(tarSplitData.Data)),
 		LengthUncompressedTarSplit: uint64(tarSplitData.UncompressedSize),
-		ChecksumAnnotationTarSplit: "", // unused
 	}
 
 	manifestDataLE := footerDataToBlob(footer)
@@ -200,6 +198,11 @@ func ZstdWriterWithLevel(dest io.Writer, level int) (*zstd.Encoder, error) {
 }
 
 // ZstdChunkedFooterData contains all the data stored in the zstd:chunked footer.
+// This footer exists to make the blobs self-describing, our implementation
+// never reads it:
+// Partial pull security hinges on the TOC digest, and that exists as a layer annotation;
+// so we are relying on the layer annotations anyway, and doing so means we can avoid
+// a round-trip to fetch this binary footer.
 type ZstdChunkedFooterData struct {
 	ManifestType uint64
 
@@ -210,7 +213,7 @@ type ZstdChunkedFooterData struct {
 	OffsetTarSplit             uint64
 	LengthCompressedTarSplit   uint64
 	LengthUncompressedTarSplit uint64
-	ChecksumAnnotationTarSplit string // Only used when reading a layer, not when creating it
+	ChecksumAnnotationTarSplit string // Deprecated: This field is not a part of the footer and not used for any purpose.
 }
 
 func footerDataToBlob(footer ZstdChunkedFooterData) []byte {
@@ -227,44 +230,3 @@ func footerDataToBlob(footer ZstdChunkedFooterData) []byte {
 
 	return manifestDataLE
 }
-
-// ReadFooterDataFromAnnotations reads the zstd:chunked footer data from the given annotations.
-func ReadFooterDataFromAnnotations(annotations map[string]string) (ZstdChunkedFooterData, error) {
-	var footerData ZstdChunkedFooterData
-
-	offsetMetadata := annotations[ManifestInfoKey]
-
-	if _, err := fmt.Sscanf(offsetMetadata, "%d:%d:%d:%d", &footerData.Offset, &footerData.LengthCompressed, &footerData.LengthUncompressed, &footerData.ManifestType); err != nil {
-		return footerData, err
-	}
-
-	if tarSplitInfoKeyAnnotation, found := annotations[TarSplitInfoKey]; found {
-		if _, err := fmt.Sscanf(tarSplitInfoKeyAnnotation, "%d:%d:%d", &footerData.OffsetTarSplit, &footerData.LengthCompressedTarSplit, &footerData.LengthUncompressedTarSplit); err != nil {
-			return footerData, err
-		}
-		footerData.ChecksumAnnotationTarSplit = annotations[TarSplitChecksumKey]
-	}
-	return footerData, nil
-}
-
-// ReadFooterDataFromBlob reads the zstd:chunked footer from the binary buffer.
-func ReadFooterDataFromBlob(footer []byte) (ZstdChunkedFooterData, error) {
-	var footerData ZstdChunkedFooterData
-
-	if len(footer) < FooterSizeSupported {
-		return footerData, errors.New("blob too small")
-	}
-	footerData.Offset = binary.LittleEndian.Uint64(footer[0:8])
-	footerData.LengthCompressed = binary.LittleEndian.Uint64(footer[8:16])
-	footerData.LengthUncompressed = binary.LittleEndian.Uint64(footer[16:24])
-	footerData.ManifestType = binary.LittleEndian.Uint64(footer[24:32])
-	footerData.OffsetTarSplit = binary.LittleEndian.Uint64(footer[32:40])
-	footerData.LengthCompressedTarSplit = binary.LittleEndian.Uint64(footer[40:48])
-	footerData.LengthUncompressedTarSplit = binary.LittleEndian.Uint64(footer[48:56])
-
-	// the magic number is stored in the last 8 bytes
-	if !bytes.Equal(ZstdChunkedFrameMagic, footer[len(footer)-len(ZstdChunkedFrameMagic):]) {
-		return footerData, errors.New("invalid magic number")
-	}
-	return footerData, nil
-}

pkg/chunked/internal/compression_test.go

@@ -4,6 +4,9 @@
 package internal
 
 import (
+	"bytes"
+	"encoding/binary"
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -23,10 +26,32 @@ func TestGenerateAndReadFooter(t *testing.T) {
 	b := footerDataToBlob(footer)
 	assert.Len(t, b, FooterSizeSupported)
 
-	footer2, err := ReadFooterDataFromBlob(b)
+	footer2, err := readFooterDataFromBlob(b)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	assert.Equal(t, footer, footer2)
 }
+
+// readFooterDataFromBlob reads the zstd:chunked footer from the binary buffer.
+func readFooterDataFromBlob(footer []byte) (ZstdChunkedFooterData, error) {
+	var footerData ZstdChunkedFooterData
+
+	if len(footer) < FooterSizeSupported {
+		return footerData, errors.New("blob too small")
+	}
+	footerData.Offset = binary.LittleEndian.Uint64(footer[0:8])
+	footerData.LengthCompressed = binary.LittleEndian.Uint64(footer[8:16])
+	footerData.LengthUncompressed = binary.LittleEndian.Uint64(footer[16:24])
+	footerData.ManifestType = binary.LittleEndian.Uint64(footer[24:32])
+	footerData.OffsetTarSplit = binary.LittleEndian.Uint64(footer[32:40])
+	footerData.LengthCompressedTarSplit = binary.LittleEndian.Uint64(footer[40:48])
+	footerData.LengthUncompressedTarSplit = binary.LittleEndian.Uint64(footer[48:56])
+
+	// the magic number is stored in the last 8 bytes
+	if !bytes.Equal(ZstdChunkedFrameMagic, footer[len(footer)-len(ZstdChunkedFrameMagic):]) {
+		return footerData, errors.New("invalid magic number")
+	}
+	return footerData, nil
+}

pkg/chunked/storage_linux.go

@@ -314,7 +314,7 @@ func makeConvertFromRawDiffer(ctx context.Context, store storage.Store, blobDige
 }
 
 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)
+	manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, tocDigest, annotations)
 	if err != nil {
 		return nil, fmt.Errorf("read zstd:chunked manifest: %w", err)
 	}
@@ -1701,7 +1701,7 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
 		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)
+		manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(fileSource, *tocDigest, annotations)
 		if err != nil {
 			return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("read zstd:chunked manifest: %w", err)
 		}

pkg/chunked/zstdchunked_test.go

@@ -153,7 +153,7 @@ func TestGenerateAndParseManifest(t *testing.T) {
 	tocDigest, err := toc.GetTOCDigest(annotations)
 	require.NoError(t, err)
 	require.NotNil(t, tocDigest)
-	manifest, _, _, err := readZstdChunkedManifest(s, 8192, *tocDigest, annotations)
+	manifest, _, _, err := readZstdChunkedManifest(s, *tocDigest, annotations)
 	if err != nil {
 		t.Error(err)
 	}