Merge pull request #475 from lsm5/digest-redux

image/internal: sha512 support for skopeo copy

Author: mtrmac

image/directory/directory_dest.go

@@ -20,8 +20,6 @@ import (
 	"go.podman.io/storage/pkg/fileutils"
 )
 
-const version = "Directory Transport Version: 1.1\n"
-
 // ErrNotContainerImageDir indicates that the directory doesn't match the expected contents of a directory created
 // using the 'dir' transport
 var ErrNotContainerImageDir = errors.New("not a containers image directory, don't want to overwrite important data")
@@ -33,7 +31,8 @@ type dirImageDestination struct {
 	stubs.NoPutBlobPartialInitialize
 	stubs.AlwaysSupportsSignatures
 
-	ref dirReference
+	ref                 dirReference
+	usesNonSHA256Digest bool
 }
 
 // newImageDestination returns an ImageDestination for writing to a directory.
@@ -76,9 +75,14 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (private.Im
 					return nil, err
 				}
 				// check if contents of version file is what we expect it to be
-				if string(contents) != version {
+				versionStr := string(contents)
+				parsedVersion, err := parseVersion(versionStr)
+				if err != nil {
 					return nil, ErrNotContainerImageDir
 				}
+				if parsedVersion.isGreaterThan(maxSupportedVersion) {
+					return nil, UnsupportedVersionError{Version: versionStr, Path: ref.resolvedPath}
+				}
 			} else {
 				return nil, ErrNotContainerImageDir
 			}
@@ -94,11 +98,6 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (private.Im
 			return nil, fmt.Errorf("unable to create directory %q: %w", ref.resolvedPath, err)
 		}
 	}
-	// create version file
-	err = os.WriteFile(ref.versionPath(), []byte(version), 0o644)
-	if err != nil {
-		return nil, fmt.Errorf("creating version file %q: %w", ref.versionPath(), err)
-	}
 
 	d := &dirImageDestination{
 		PropertyMethodsInitialize: impl.PropertyMethods(impl.Properties{
@@ -151,13 +150,17 @@ func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
 		}
 	}()
 
-	digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo)
+	digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo)
+
 	// TODO: This can take quite some time, and should ideally be cancellable using ctx.Done().
 	size, err := io.Copy(blobFile, stream)
 	if err != nil {
 		return private.UploadedBlob{}, err
 	}
 	blobDigest := digester.Digest()
+	if blobDigest.Algorithm() != digest.Canonical { // compare the special case in layerPath
+		d.usesNonSHA256Digest = true
+	}
 	if inputInfo.Size != -1 && size != inputInfo.Size {
 		return private.UploadedBlob{}, fmt.Errorf("Size mismatch when copying %s, expected %d, got %d", blobDigest, inputInfo.Size, size)
 	}
@@ -257,6 +260,14 @@ func (d *dirImageDestination) PutSignaturesWithFormat(ctx context.Context, signa
 // - Uploaded data MAY be visible to others before CommitWithOptions() is called
 // - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
 func (d *dirImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
+	versionToWrite := version1_1
+	if d.usesNonSHA256Digest {
+		versionToWrite = version1_2
+	}
+	err := os.WriteFile(d.ref.versionPath(), []byte(versionToWrite.String()), 0o644)
+	if err != nil {
+		return fmt.Errorf("writing version file %q: %w", d.ref.versionPath(), err)
+	}
 	return nil
 }
 

image/directory/directory_dest_test.go

@@ -0,0 +1,67 @@
+package directory
+
+import (
+	"bytes"
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/opencontainers/go-digest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.podman.io/image/v5/pkg/blobinfocache/memory"
+	"go.podman.io/image/v5/types"
+)
+
+func TestVersionAssignment(t *testing.T) {
+	for _, c := range []struct {
+		name            string
+		algorithms      []digest.Algorithm
+		expectedVersion version
+	}{
+		{
+			name:            "SHA256 only gets version 1.1",
+			algorithms:      []digest.Algorithm{digest.SHA256},
+			expectedVersion: version1_1,
+		},
+		{
+			name:            "SHA512 only gets version 1.2",
+			algorithms:      []digest.Algorithm{digest.SHA512},
+			expectedVersion: version1_2,
+		},
+		{
+			name:            "Mixed SHA256 and SHA512 gets version 1.2",
+			algorithms:      []digest.Algorithm{digest.SHA256, digest.SHA512},
+			expectedVersion: version1_2,
+		},
+	} {
+		t.Run(c.name, func(t *testing.T) {
+			ref, tmpDir := refToTempDir(t)
+			cache := memory.New()
+
+			dest, err := ref.NewImageDestination(context.Background(), nil)
+			require.NoError(t, err)
+			defer dest.Close()
+
+			for i, algo := range c.algorithms {
+				blobData := []byte("test-blob-" + algo.String() + "-" + string(rune(i)))
+				var blobDigest digest.Digest
+				if algo == digest.SHA256 {
+					blobDigest = ""
+				} else {
+					blobDigest = algo.FromBytes(blobData)
+				}
+				_, err = dest.PutBlob(context.Background(), bytes.NewReader(blobData), types.BlobInfo{Digest: blobDigest, Size: int64(len(blobData))}, cache, false)
+				require.NoError(t, err)
+			}
+
+			err = dest.Commit(context.Background(), nil)
+			require.NoError(t, err)
+
+			versionBytes, err := os.ReadFile(filepath.Join(tmpDir, "version"))
+			require.NoError(t, err)
+			assert.Equal(t, c.expectedVersion.String(), string(versionBytes))
+		})
+	}
+}

image/directory/directory_src.go

@@ -26,7 +26,24 @@ type dirImageSource struct {
 
 // newImageSource returns an ImageSource reading from an existing directory.
 // The caller must call .Close() on the returned ImageSource.
-func newImageSource(ref dirReference) private.ImageSource {
+func newImageSource(ref dirReference) (private.ImageSource, error) {
+	versionPath := ref.versionPath()
+	contents, err := os.ReadFile(versionPath)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			return nil, fmt.Errorf("reading version file %q: %w", versionPath, err)
+		}
+	} else {
+		versionStr := string(contents)
+		parsedVersion, err := parseVersion(versionStr)
+		if err != nil {
+			return nil, fmt.Errorf("invalid version file content: %q", versionStr)
+		}
+		if parsedVersion.isGreaterThan(maxSupportedVersion) {
+			return nil, UnsupportedVersionError{Version: versionStr, Path: ref.resolvedPath}
+		}
+	}
+
 	s := &dirImageSource{
 		PropertyMethodsInitialize: impl.PropertyMethods(impl.Properties{
 			HasThreadSafeGetBlob: false,
@@ -36,7 +53,7 @@ func newImageSource(ref dirReference) private.ImageSource {
 		ref: ref,
 	}
 	s.Compat = impl.AddCompat(s)
-	return s
+	return s, nil
 }
 
 // Reference returns the reference used to set up this source, _as specified by the user_

image/directory/directory_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"io"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/opencontainers/go-digest"
@@ -203,6 +204,8 @@ func TestGetPutSignatures(t *testing.T) {
 
 func TestSourceReference(t *testing.T) {
 	ref, tmpDir := refToTempDir(t)
+	err := os.WriteFile(filepath.Join(tmpDir, "version"), []byte("Directory Transport Version: 1.1\n"), 0o644)
+	require.NoError(t, err)
 
 	src, err := ref.NewImageSource(context.Background(), nil)
 	require.NoError(t, err)

image/directory/directory_transport.go

@@ -146,7 +146,7 @@ func (ref dirReference) NewImage(ctx context.Context, sys *types.SystemContext)
 // NewImageSource returns a types.ImageSource for this reference.
 // The caller must call .Close() on the returned ImageSource.
 func (ref dirReference) NewImageSource(ctx context.Context, sys *types.SystemContext) (types.ImageSource, error) {
-	return newImageSource(ref), nil
+	return newImageSource(ref)
 }
 
 // NewImageDestination returns a types.ImageDestination for this reference.
@@ -172,12 +172,19 @@ func (ref dirReference) manifestPath(instanceDigest *digest.Digest) (string, err
 }
 
 // layerPath returns a path for a layer tarball within a directory using our conventions.
-func (ref dirReference) layerPath(digest digest.Digest) (string, error) {
-	if err := digest.Validate(); err != nil { // digest.Digest.Encoded() panics on failure, and could possibly result in a path with ../, so validate explicitly.
+func (ref dirReference) layerPath(d digest.Digest) (string, error) {
+	if err := d.Validate(); err != nil { // digest.Digest.Encoded() panics on failure, and could possibly result in a path with ../, so validate explicitly.
 		return "", err
 	}
-	// FIXME: Should we keep the digest identification?
-	return filepath.Join(ref.path, digest.Encoded()), nil
+
+	var filename string
+	if d.Algorithm() == digest.Canonical {
+		filename = d.Encoded()
+	} else {
+		filename = d.Algorithm().String() + "-" + d.Encoded()
+	}
+
+	return filepath.Join(ref.path, filename), nil
 }
 
 // signaturePath returns a path for a signature within a directory using our conventions.

image/directory/directory_transport_test.go

@@ -172,7 +172,9 @@ func TestReferenceNewImageNoValidManifest(t *testing.T) {
 }
 
 func TestReferenceNewImageSource(t *testing.T) {
-	ref, _ := refToTempDir(t)
+	ref, tmpDir := refToTempDir(t)
+	err := os.WriteFile(filepath.Join(tmpDir, "version"), []byte("Directory Transport Version: 1.1\n"), 0o644)
+	require.NoError(t, err)
 	src, err := ref.NewImageSource(context.Background(), nil)
 	assert.NoError(t, err)
 	defer src.Close()
@@ -209,14 +211,21 @@ func TestReferenceManifestPath(t *testing.T) {
 }
 
 func TestReferenceLayerPath(t *testing.T) {
-	const hex = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+	const hex256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+	const hex512 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
 
 	ref, tmpDir := refToTempDir(t)
 	dirRef, ok := ref.(dirReference)
 	require.True(t, ok)
-	res, err := dirRef.layerPath("sha256:" + hex)
+
+	res, err := dirRef.layerPath("sha256:" + hex256)
 	require.NoError(t, err)
-	assert.Equal(t, tmpDir+"/"+hex, res)
+	assert.Equal(t, tmpDir+"/"+hex256, res)
+
+	res, err = dirRef.layerPath("sha512:" + hex512)
+	require.NoError(t, err)
+	assert.Equal(t, tmpDir+"/sha512-"+hex512, res)
+
 	_, err = dirRef.layerPath(digest.Digest("sha256:../hello"))
 	assert.Error(t, err)
 }

image/directory/version.go

@@ -0,0 +1,62 @@
+package directory
+
+import (
+	"fmt"
+)
+
+const (
+	versionPrefix = "Directory Transport Version: "
+)
+
+// version represents a parsed directory transport version
+type version struct {
+	major int
+	minor int
+}
+
+// Supported versions
+// Write version file based on digest algorithm used.
+// 1.1 for sha256-only images, 1.2 otherwise.
+var (
+	version1_1          = version{major: 1, minor: 1}
+	version1_2          = version{major: 1, minor: 2}
+	maxSupportedVersion = version1_2
+)
+
+// String formats a version as a string suitable for writing to the version file
+func (v version) String() string {
+	return fmt.Sprintf("%s%d.%d\n", versionPrefix, v.major, v.minor)
+}
+
+// parseVersion parses a version string into major and minor components.
+// Returns an error if the format is invalid.
+func parseVersion(versionStr string) (version, error) {
+	var v version
+	expectedFormat := versionPrefix + "%d.%d\n"
+	// Sscanf parsing is a bit loose (treats spaces specially), but a strict check immediately follows
+	n, err := fmt.Sscanf(versionStr, expectedFormat, &v.major, &v.minor)
+	if err != nil || n != 2 || versionStr != v.String() {
+		return version{}, fmt.Errorf("invalid version format")
+	}
+	return v, nil
+}
+
+// TODO: Potential refactor for better interoperability with `cmp`
+// https://github.com/containers/container-libs/pull/475#discussion_r2571131267
+// isGreaterThan returns true if v is greater than other
+func (v version) isGreaterThan(other version) bool {
+	if v.major != other.major {
+		return v.major > other.major
+	}
+	return v.minor > other.minor
+}
+
+// UnsupportedVersionError indicates that the directory uses a version newer than we support
+type UnsupportedVersionError struct {
+	Version string // The unsupported version string found
+	Path    string // The path to the directory
+}
+
+func (e UnsupportedVersionError) Error() string {
+	return fmt.Sprintf("unsupported directory transport version %q at %s", e.Version, e.Path)
+}

image/directory/version_test.go

@@ -0,0 +1,55 @@
+package directory
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseVersion(t *testing.T) {
+	for _, c := range []struct {
+		input       string
+		expected    version
+		shouldError bool
+	}{
+		{
+			input:    "Directory Transport Version: 1.1\n",
+			expected: version{major: 1, minor: 1},
+		},
+		{
+			input:    "Directory Transport Version: 1.2\n",
+			expected: version{major: 1, minor: 2},
+		},
+		{
+			input:       "Invalid prefix 1.1\n",
+			shouldError: true,
+		},
+		{
+			input:       "Directory Transport Version: 1.1",
+			shouldError: true,
+		},
+		{
+			input:       "Directory Transport Version: abc\n",
+			shouldError: true,
+		},
+	} {
+		t.Run(c.input, func(t *testing.T) {
+			v, err := parseVersion(c.input)
+			if c.shouldError {
+				assert.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, c.expected, v)
+			}
+		})
+	}
+}
+
+func TestVersionComparison(t *testing.T) {
+	assert.False(t, version1_1.isGreaterThan(version1_1))
+	assert.False(t, version1_1.isGreaterThan(version1_2))
+	assert.True(t, version1_2.isGreaterThan(version1_1))
+	assert.True(t, version{major: 2, minor: 0}.isGreaterThan(version1_2))
+	assert.True(t, version{major: 1, minor: 3}.isGreaterThan(version1_2))
+}