Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion image/directory/directory_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,15 @@ func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
}
}()

digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo)
digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the destination-transport-side digest computation must be a more complex logic, see in the earlier PR about the interaction with cannotModifyManifestReason.

… and dirReference.layerPath discards the algorithm name; that does not generalize for other algorithms, we need to move towards agility where adding an extra algorithm is a ~parameter change and does not require any more changes to the “code proper”; i.e. discarding algorithm names is no longer much of an option.

(We need to keep the existing file names for sha256, to retain compatibility. And… do we define a new value for versionPath?!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PutBlob to store blob under provided digest algorithm with the algorithm name prepended (except for Canonical) along with a canonical digest hardlink.

$ /usr/bin/ls
dc518581817f4e75a7dcfd35383e67c3ef85438250c17e10090b5a31ab8f68d4  manifest.json  sha512-2ee373e378345b35e7966a106c5c0a40a005a13bfc87695d89c5bb217f969c351e73810cf78d3d841237098731cf76878f05af8f8d28176c316681f9422ff688  version

$ diff dc518581817f4e75a7dcfd35383e67c3ef85438250c17e10090b5a31ab8f68d4 sha512-2ee373e378345b35e7966a106c5c0a40a005a13bfc87695d89c5bb217f969c351e73810cf78d3d841237098731cf76878f05af8f8d28176c316681f9422ff688
$

do we define a new value for versionPath?!)

Doesn't break existing behaviour but there's new stuff, so maybe we should? I'll defer to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Hard links are not supported by all file systems (FAT)
  • Symbolic links are not supported by all file systems either, and generally restricted to admin users on Windows

And, anyway, readers of dir: can only start with the manifest, and the values provided in the manifest. So if PutBlob returns sha512, and the manifest is written to include sha512, readers will not know the sha256 value and have no way to use it.

So I don’t think we need to compute both digests at all; just the layerPath changes to the path computation, + some (as-yet-undefined) logic for PutBlob to use “the algorithm the user wanted”, should be sufficient.

do we define a new value for versionPath?!)

I think with the changes to layerPath, we need to. Previously it was, hypothetically, possible to read a complete sha512 image from dir:, and those images will now break. And we will need to update both dir…Dest… and dir…Src…: destinations should refuse to work on future versions, and still assign the existing 1.1 version for sha256 images for maximum compatibility. sources should detect+refuse future versions.

Consider making the dir changes a separate PR.


var canonicalDigester digest.Digester
computeCanonical := inputInfo.Digest != "" && inputInfo.Digest.Algorithm() != digest.Canonical
if computeCanonical {
canonicalDigester = digest.Canonical.Digester()
stream = io.TeeReader(stream, canonicalDigester.Hash())
}

// TODO: This can take quite some time, and should ideally be cancellable using ctx.Done().
size, err := io.Copy(blobFile, stream)
if err != nil {
Expand Down Expand Up @@ -185,6 +193,18 @@ func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
if err := os.Rename(blobFile.Name(), blobPath); err != nil {
return private.UploadedBlob{}, err
}

if computeCanonical {
canonicalDigest := canonicalDigester.Digest()
canonicalPath, err := d.ref.layerPath(canonicalDigest)
if err != nil {
return private.UploadedBlob{}, err
}
if err := os.Link(blobPath, canonicalPath); err != nil && !os.IsExist(err) {
return private.UploadedBlob{}, fmt.Errorf("creating canonical digest link: %w", err)
}
}

succeeded = true
return private.UploadedBlob{Digest: blobDigest, Size: size}, nil
}
Expand Down
11 changes: 9 additions & 2 deletions image/directory/directory_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,15 @@ 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.
return "", err
}
// FIXME: Should we keep the digest identification?
return filepath.Join(ref.path, digest.Encoded()), nil

var filename string
if digest.Algorithm().String() == "sha256" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

digest.Algorithm == digest.Canonical? Pedantically I’m not sure equality of Algorithm is documented to work, but pragmatically it does. At the very least, please don’t use a string constant here.

(I don’t have much of a preference for digest.Canonical vs. digest.SHA256. I assume Canonical will never change. Canonical sort of better expresses the intent.)

filename = digest.Encoded()
} else {
filename = digest.Algorithm().String() + "-" + digest.Encoded()
}

return filepath.Join(ref.path, filename), nil
}

// signaturePath returns a path for a signature within a directory using our conventions.
Expand Down
13 changes: 10 additions & 3 deletions image/directory/directory_transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,14 +209,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+"/"+hex256, res)

res, err = dirRef.layerPath("sha512:" + hex512)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/"+hex, res)
assert.Equal(t, tmpDir+"/sha512-"+hex512, res)

_, err = dirRef.layerPath(digest.Digest("sha256:../hello"))
assert.Error(t, err)
}
Expand Down
26 changes: 26 additions & 0 deletions image/internal/image/digest_validation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package image

import (
"fmt"

"github.com/opencontainers/go-digest"
)

func validateBlobAgainstDigest(blob []byte, expectedDigest digest.Digest) error {
if expectedDigest == "" {
return fmt.Errorf("expected digest is empty")
}
err := expectedDigest.Validate()
if err != nil {
return fmt.Errorf("invalid digest format %q: %w", expectedDigest, err)
}
digestAlgorithm := expectedDigest.Algorithm()
if !digestAlgorithm.Available() {
return fmt.Errorf("unsupported digest algorithm: %s", digestAlgorithm)
}
computedDigest := digestAlgorithm.FromBytes(blob)
if computedDigest != expectedDigest {
return fmt.Errorf("blob digest %s does not match expected %s", computedDigest, expectedDigest)
}
return nil
}
101 changes: 101 additions & 0 deletions image/internal/image/digest_validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package image

import (
"testing"

"github.com/opencontainers/go-digest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestValidateBlobAgainstDigest(t *testing.T) {
testBlob := []byte("test data")

tests := []struct {
name string
blob []byte
expectedDigest digest.Digest
expectError bool
errorContains string
}{
{
name: "empty digest",
blob: testBlob,
expectedDigest: "",
expectError: true,
errorContains: "expected digest is empty",
},
{
name: "invalid digest format - no algorithm",
blob: testBlob,
expectedDigest: "invalidsyntax",
expectError: true,
errorContains: "invalid digest format",
},
{
name: "invalid digest format - sha256 prefix w/ malformed hex",
blob: testBlob,
expectedDigest: "sha256:notahexstring!@#",
expectError: true,
errorContains: "invalid digest format",
},
{
name: "invalid digest format - sha512 prefix w/ malformed hex",
blob: testBlob,
expectedDigest: "sha512:notahexstring!@#",
expectError: true,
errorContains: "invalid digest format",
},
{
name: "invalid digest format - unknown algorithm",
blob: testBlob,
expectedDigest: "unknown-algo:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
expectError: true,
errorContains: "invalid digest format",
},
{
name: "digest mismatch",
blob: testBlob,
expectedDigest: digest.SHA256.FromBytes([]byte("different data")),
expectError: true,
errorContains: "blob digest",
},
{
name: "empty blob with matching digest",
blob: []byte{},
expectedDigest: digest.SHA256.FromBytes([]byte{}),
expectError: false,
},
{
name: "unavailable algorithm - blake2b",
blob: testBlob,
expectedDigest: "blake2b:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
expectError: true,
errorContains: "invalid digest format",
},
{
name: "sha256 digest success",
blob: testBlob,
expectedDigest: digest.SHA256.FromBytes(testBlob),
expectError: false,
},
{
name: "sha512 digest success",
blob: testBlob,
expectedDigest: digest.SHA512.FromBytes(testBlob),
expectError: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateBlobAgainstDigest(tt.blob, tt.expectedDigest)
if tt.expectError {
require.Error(t, err)
assert.Contains(t, err.Error(), tt.errorContains)
} else {
assert.NoError(t, err)
}
})
}
}
5 changes: 2 additions & 3 deletions image/internal/image/docker_schema2.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,8 @@ func (m *manifestSchema2) ConfigBlob(ctx context.Context) ([]byte, error) {
if err != nil {
return nil, err
}
computedDigest := digest.FromBytes(blob)
if computedDigest != m.m.ConfigDescriptor.Digest {
return nil, fmt.Errorf("Download config.json digest %s does not match expected %s", computedDigest, m.m.ConfigDescriptor.Digest)
if err := validateBlobAgainstDigest(blob, m.m.ConfigDescriptor.Digest); err != nil {
return nil, fmt.Errorf("config validation failed: %w", err)
}
m.configBlob = blob
}
Expand Down
6 changes: 2 additions & 4 deletions image/internal/image/oci.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"slices"

ociencspec "github.com/containers/ocicrypt/spec"
"github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"go.podman.io/image/v5/docker/reference"
"go.podman.io/image/v5/internal/iolimits"
Expand Down Expand Up @@ -74,9 +73,8 @@ func (m *manifestOCI1) ConfigBlob(ctx context.Context) ([]byte, error) {
if err != nil {
return nil, err
}
computedDigest := digest.FromBytes(blob)
if computedDigest != m.m.Config.Digest {
return nil, fmt.Errorf("Download config.json digest %s does not match expected %s", computedDigest, m.m.Config.Digest)
if err := validateBlobAgainstDigest(blob, m.m.Config.Digest); err != nil {
return nil, fmt.Errorf("config validation failed: %w", err)
}
m.configBlob = blob
}
Expand Down
Loading