diff --git a/image/directory/directory_dest.go b/image/directory/directory_dest.go index fc82c969f1..38fcd46a65 100644 --- a/image/directory/directory_dest.go +++ b/image/directory/directory_dest.go @@ -20,12 +20,64 @@ import ( "go.podman.io/storage/pkg/fileutils" ) -const version = "Directory Transport Version: 1.1\n" +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" + n, err := fmt.Sscanf(versionStr, expectedFormat, &v.major, &v.minor) + if err != nil || n != 2 || versionStr != fmt.Sprintf(expectedFormat, v.major, v.minor) { + return version{}, fmt.Errorf("invalid version format") + } + return v, nil +} + +// 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 +} // 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") +// 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) +} + type dirImageDestination struct { impl.Compat impl.PropertyMethodsInitialize @@ -33,7 +85,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 +129,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 +152,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 +204,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 +314,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 } diff --git a/image/directory/directory_dest_test.go b/image/directory/directory_dest_test.go new file mode 100644 index 0000000000..2f4c1d027c --- /dev/null +++ b/image/directory/directory_dest_test.go @@ -0,0 +1,114 @@ +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 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)) +} + +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)) + }) + } +} diff --git a/image/directory/directory_src.go b/image/directory/directory_src.go index b7fb3855c5..e2953b4233 100644 --- a/image/directory/directory_src.go +++ b/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_ diff --git a/image/directory/directory_test.go b/image/directory/directory_test.go index 1d5fb96947..4a23d497fb 100644 --- a/image/directory/directory_test.go +++ b/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) diff --git a/image/directory/directory_transport.go b/image/directory/directory_transport.go index 77c0e7be57..165159e588 100644 --- a/image/directory/directory_transport.go +++ b/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. diff --git a/image/directory/directory_transport_test.go b/image/directory/directory_transport_test.go index bcafff5352..b9e6ce581c 100644 --- a/image/directory/directory_transport_test.go +++ b/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) }