image/internal: sha512 support for skopeo copy #475
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6507 |
|
Packit jobs failed. @containers/packit-build please check. |
|
/packit rebuild-failed |
|
@mtrmac PTAL |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
ACK to the image part; the dir: part needs a bit more.
| }() | ||
|
|
||
| digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo) | ||
| digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo) |
There was a problem hiding this comment.
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?!)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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.
`validateBlobAgainstDigest` verifies that the provided blob matches the exepcted digest. If expected digest itself is invalid or unusable, it rejects the blob. Callers don't need to pre-validate the expected digest. This enables `skopeo copy` to work with sha512-digested images. Co-Authored-By: Claude Code <[email protected]> Signed-off-by: Lokesh Mandvekar <[email protected]>
When storing blobs with non-canonical digest algorithms (e.g., sha512): - Store the blob under the provided digest algorithm (e.g., "sha512-abc") - Also compute and store a hard link under the canonical digest (sha256) Co-Authored-By: Claude Code <[email protected]> Signed-off-by: Lokesh Mandvekar <[email protected]>
|
@mtrmac PTA(nother)L . I think I have addressed all comments so far. Used Claude too, but hopefully no AI slop this time 🤞 . LMK. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
The internal/image part LGTM.
| return filepath.Join(ref.path, digest.Encoded()), nil | ||
|
|
||
| var filename string | ||
| if digest.Algorithm().String() == "sha256" { |
There was a problem hiding this comment.
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.)
| }() | ||
|
|
||
| digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo) | ||
| digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo) |
There was a problem hiding this comment.
- 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.
3 participants
See individual commits.
Vendored by containers/skopeo#2747