Add PutSignaturesWithFormat/GetSignaturesWithFormat to OCI layout #312
Conversation
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
bitoku
force-pushed
the
layout-signature
branch
from
4c51aa2 to
f0391e0
Compare
|
Packit jobs failed. @containers/packit-build please check. |
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
|
The The “Skopeo test” failures here look relevant, I’m not immediately sure (I’m afraid I didn’t yet read the updated version of this PR) whether they are bugs to be fixed here, or whether the tests will need to be adjusted. |
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
bitoku
force-pushed
the
layout-signature
branch
from
4a93cdd to
ffcc126
Compare
|
This is the commit ffcc126 to fix the CI |
|
@mtrmac PTAL when you have a moment. thanks! |
| if !strings.HasSuffix(tag, ".sig") { | ||
| return false | ||
| } | ||
| digestPart := strings.TrimSuffix(tag, ".sig") |
There was a problem hiding this comment.
(Non-blocking: This can be strings.CutSuffix)
| } | ||
| digestPart := strings.TrimSuffix(tag, ".sig") | ||
| digestPart = strings.Replace(digestPart, "-", ":", 1) | ||
| return digest.Digest(digestPart).Validate() == nil |
There was a problem hiding this comment.
I’d prefer _, err := digest.Parse(…); err == nil here. It admittedly does exactly the same thing, but it is clearer that it uses the external API, without breaking encapsulation and assuming a specific representation of the digest.Digest value.
| } | ||
| fi, err := r.Stat() | ||
| if err != nil { | ||
| return nil, 0, err |
There was a problem hiding this comment.
Pre-existing, but this leaks r
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read blob: %w", err) | ||
| } | ||
| mimeType := manifest.GuessMIMEType(signBlob) |
There was a problem hiding this comment.
We have an authoritative MIME type in signDesc, using that would be more accurate.
| @@ -5,6 +5,7 @@ import ( | |||
| "encoding/json" | |||
There was a problem hiding this comment.
I’m afraid this is not documented anywhere, but the *_transport.go files primarily deal with image references and naming; and because image identity is critical for preserving the users’ intent when verifying signatures, we want as good unit test coverage as possible for *_transport.go.
That’s, in principle, an option, but it might be easier to move some of the helper functions to oci_src.go or oci_dest.go, where the unit test coverage requirements are much more pragmatic. E.g. getOCIDescriptorContents seems to have a single caller outside of this file, so it can be moved closer to the caller; probably similarly for getBlob and others.
Also, getManifestDescriptor should have unit test coverage for the new code paths.
| @@ -312,6 +351,26 @@ func (d *ociImageDestination) CommitWithOptions(ctx context.Context, options pri | |||
| if err != nil { | |||
| return err | |||
| } | |||
| // Delete unreferenced blobs (e.g. old signature manifest and its config) | |||
| if !d.blobsToDelete.Empty() { | |||
| count := make(map[digest.Digest]int) | |||
There was a problem hiding this comment.
| count := make(map[digest.Digest]int) | |
| blobsUsedInRootIndex := make(map[digest.Digest]int) |
to be explicit about intent
| if err != nil { | ||
| return err | ||
| } | ||
| // If it overwrote an existing signature manifest, delete blobs referenced by the old manifest. |
There was a problem hiding this comment.
We didn’t remove any entries from signManifest.Layers; so the only other digests that might perhaps be removed, and need counting, are signManifest.Config (which we already added) and the digest of signManifest itself.
So I think it would suffice for the signManifest != nil branch to manually count the manifest+config, and this can be removed entirely.
(Alternatively, to be clearly safe and to rely on the existing deletion logic without duplicating anything, do this countBlobsForDescriptor computation in the signManifest != nil branch, and don’t manually add signManifest.Config.)
| sig := signature.SigstoreFromComponents("application/vnd.dev.cosign.simplesigning.v1+json", testPayload, testAnnotations) | ||
|
|
||
| // Test that PutSignaturesWithFormat fails when instanceDigest is nil | ||
| err = ociDest.PutSignaturesWithFormat(context.Background(), []signature.Signature{sig}, nil) |
There was a problem hiding this comment.
(I’m not sure we urgently need a specific test for this — this is a caller error. Testing the success case of PutManifest+PutSignaturesWithFormat(…, nil) would be a bit more relevant.
But now that the test exists, it’s perfectly fine to keep it.)
| require.NoError(t, err) | ||
| defer dest.Close() | ||
|
|
||
| // Cast to ociImageDestination to access PutSignaturesWithFormat |
There was a problem hiding this comment.
Using imagedestination.FromPublic would be more similar to actual callers, and thus more representative. (Also elsewhere.)
| @@ -216,3 +217,136 @@ func TestPutblobFromLocalFile(t *testing.T) { | |||
| err = ociDest.CommitWithOptions(context.Background(), private.CommitOptions{}) | |||
| require.NoError(t, err) | |||
| } | |||
|
|
|||
| // TestPutSignaturesWithFormat tests that sigstore signatures are properly stored in OCI layout | |||
| func TestPutSignaturesWithFormat(t *testing.T) { | |||
There was a problem hiding this comment.
(Absolutely non-blocking: consider consolidating the tests into a table-driven one. This is perfectly fine as is.)
This PR is copied from containers/image#2934 .
This PR adds PutSignaturesWithFormat/GetSignaturesWithFormat support to OCI layout.
The general idea is tag-based discovery, same as sigstore signature discovery.
It stores the signature with
annotation org.opencontainers.image.ref.name: "sha256-<hash>.sig", which can be used as a tag. https://specs.opencontainers.org/image-spec/image-layout/#IMAGE-SPEC-IMAGE-LAYOUT-19Fixes #186