Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support sigstore attachments for v2s2 #2734

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

support sigstore attachments for v2s2 #2734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions docker/docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1110,7 +1110,7 @@ func isManifestUnknownError(err error) bool {
// getSigstoreAttachmentManifest loads and parses the manifest for sigstore attachments for
// digest in ref.
// It returns (nil, nil) if the manifest does not exist.
func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref dockerReference, digest digest.Digest) (*manifest.OCI1, error) {
func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref dockerReference, digest digest.Digest) (manifest.Manifest, error) {
tag, err := sigstoreAttachmentTag(digest)
if err != nil {
return nil, err
Expand All @@ -1132,12 +1132,17 @@ func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref do
logrus.Debugf("Fetching sigstore attachment manifest failed: %v", err)
return nil, err
}
if mimeType != imgspecv1.MediaTypeImageManifest {
var res manifest.Manifest
switch mimeType {
case imgspecv1.MediaTypeImageManifest:
res, err = manifest.OCI1FromManifest(manifestBlob)
case manifest.DockerV2Schema2MediaType:
res, err = manifest.Schema2FromManifest(manifestBlob)
default:
// FIXME: Try anyway??
return nil, fmt.Errorf("unexpected MIME type for sigstore attachment manifest %s: %q",
sigstoreRef.String(), mimeType)
}
res, err := manifest.OCI1FromManifest(manifestBlob)
if err != nil {
return nil, fmt.Errorf("parsing manifest %s: %w", sigstoreRef.String(), err)
}
Expand Down
69 changes: 56 additions & 13 deletions docker/docker_image_dest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"net/url"
"os"
"path/filepath"
"reflect"
"slices"
"strings"

Expand Down Expand Up @@ -701,34 +702,49 @@ func (d *dockerImageDestination) putSignaturesToSigstoreAttachments(ctx context.
return errors.New("writing sigstore attachments is disabled by configuration")
}

ociManifest, err := d.c.getSigstoreAttachmentManifest(ctx, d.ref, manifestDigest)
genManifest, err := d.c.getSigstoreAttachmentManifest(ctx, d.ref, manifestDigest)
if err != nil {
return err
}
var ociConfig imgspecv1.Image // Most fields empty by default
if ociManifest == nil {
ociManifest = manifest.OCI1FromComponents(imgspecv1.Descriptor{
if genManifest == nil {
genManifest = manifest.OCI1FromComponents(imgspecv1.Descriptor{
MediaType: imgspecv1.MediaTypeImageConfig,
Digest: "", // We will fill this in later.
Size: 0,
}, nil)
ociConfig.RootFS.Type = "layers"
} else {
logrus.Debugf("Fetching sigstore attachment config %s", ociManifest.Config.Digest.String())
blobInfo := genManifest.ConfigInfo()
logrus.Debugf("Fetching sigstore attachment config %s", blobInfo.Digest.String())
descriptor := imgspecv1.Descriptor{
MediaType: blobInfo.MediaType,
Digest: blobInfo.Digest,
Size: blobInfo.Size,
Annotations: blobInfo.Annotations,
}
// We don’t benefit from a real BlobInfoCache here because we never try to reuse/mount configs.
configBlob, err := d.c.getOCIDescriptorContents(ctx, d.ref, ociManifest.Config, iolimits.MaxConfigBodySize,
configBlob, err := d.c.getOCIDescriptorContents(ctx, d.ref, descriptor, iolimits.MaxConfigBodySize,
none.NoCache)
if err != nil {
return err
}
if err := json.Unmarshal(configBlob, &ociConfig); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unmarshaling v2s2 into OCI is not semantically correct (loses data).

return fmt.Errorf("parsing sigstore attachment config %s in %s: %w", ociManifest.Config.Digest.String(),
return fmt.Errorf("parsing sigstore attachment config %s in %s: %w", blobInfo.Digest.String(),
d.ref.ref.Name(), err)
}
}

// To make sure we can safely append to the slices of ociManifest, without adding a remote dependency on the code that creates it.
ociManifest.Layers = slices.Clone(ociManifest.Layers)
// To make sure we can safely append to the slices of Manifest, without adding a remote dependency on the code that creates it.
switch v := genManifest.(type) {
Copy link
Collaborator

@mtrmac mtrmac Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No; hard-coding implementation types is an invisible dependency, and a trap for long-term maintenance.

At the very least, this package can build a private interface. More likely, we would need an (internal-only, because we can’t add methods to a public interface without breaking API) extension of the manifest.Manifest interface which adds an “add a layer” operation. (Compare internal/manifest.List{,Public}).

case *manifest.OCI1:
v.Layers = slices.Clone(v.Layers)
case *manifest.Schema2:
v.LayersDescriptors = slices.Clone(v.LayersDescriptors)
default:
return fmt.Errorf("sigstore attachment manifest has the wrong type %s", reflect.TypeOf(genManifest).Name())
}

// We don’t need to ^^^ for ociConfig.RootFS.DiffIDs because we have created it empty ourselves, and json.Unmarshal is documented to append() to
// the slice in the original object (or in a newly allocated object).
for _, sig := range signatures {
Expand All @@ -737,8 +753,14 @@ func (d *dockerImageDestination) putSignaturesToSigstoreAttachments(ctx context.
annotations := sig.UntrustedAnnotations()

alreadyOnRegistry := false
for _, layer := range ociManifest.Layers {
if layerMatchesSigstoreSignature(layer, mimeType, payloadBlob, annotations) {
for _, layer := range genManifest.LayerInfos() {
descriptor := imgspecv1.Descriptor{
MediaType: layer.MediaType,
Digest: layer.Digest,
Size: layer.Size,
Annotations: layer.Annotations,
}
if layerMatchesSigstoreSignature(descriptor, mimeType, payloadBlob, annotations) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn’t work because schema2 manifests don’t have annotations at all.

logrus.Debugf("Signature with digest %s already exists on the registry", layer.Digest.String())
alreadyOnRegistry = true
break
Expand All @@ -761,7 +783,17 @@ func (d *dockerImageDestination) putSignaturesToSigstoreAttachments(ctx context.
return err
}
sigDesc.Annotations = annotations
ociManifest.Layers = append(ociManifest.Layers, sigDesc)
switch v := genManifest.(type) {
case *manifest.OCI1:
v.Layers = append(v.Layers, sigDesc)
case *manifest.Schema2:
v.LayersDescriptors = append(v.LayersDescriptors, manifest.Schema2Descriptor{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This discards annotations (schema2 manifests don’t have annotations at all), and as a result, the signature is unusable.

MediaType: sigDesc.MediaType,
Size: sigDesc.Size,
Digest: sigDesc.Digest,
URLs: sigDesc.URLs,
})
}
ociConfig.RootFS.DiffIDs = append(ociConfig.RootFS.DiffIDs, sigDesc.Digest)
logrus.Debugf("Adding new signature, digest %s", sigDesc.Digest.String())
}
Expand All @@ -781,9 +813,20 @@ func (d *dockerImageDestination) putSignaturesToSigstoreAttachments(ctx context.
if err != nil {
return err
}
ociManifest.Config = configDesc

manifestBlob, err := ociManifest.Serialize()
switch v := genManifest.(type) {
case *manifest.OCI1:
v.Config = configDesc
case *manifest.Schema2:
v.ConfigDescriptor = manifest.Schema2Descriptor{
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • The config was uploaded above with a hard-coded MIME type
  • Structurally, assuming that OCI and schema2 configs are completely substitutable is not something the transport should assume; such a no-op conversion belongs into the image/ conversion code.

MediaType: configDesc.MediaType,
Size: configDesc.Size,
Digest: configDesc.Digest,
URLs: configDesc.URLs,
}
}

manifestBlob, err := genManifest.Serialize()
if err != nil {
return err
}
Expand Down
21 changes: 15 additions & 6 deletions docker/docker_image_src.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/containers/image/v5/types"
"github.com/containers/storage/pkg/regexp"
digest "github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -630,23 +631,31 @@ func (s *dockerImageSource) appendSignaturesFromSigstoreAttachments(ctx context.
return err
}

ociManifest, err := s.c.getSigstoreAttachmentManifest(ctx, s.physicalRef, manifestDigest)
genManifest, err := s.c.getSigstoreAttachmentManifest(ctx, s.physicalRef, manifestDigest)
if err != nil {
return err
}
if ociManifest == nil {
if genManifest == nil {
return nil
}

logrus.Debugf("Found a sigstore attachment manifest with %d layers", len(ociManifest.Layers))
for layerIndex, layer := range ociManifest.Layers {
layers := genManifest.LayerInfos()
numLayers := len(layers)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you look at the compiler outputs, adding a variable to avoid a len() does not speed things up — the slice itself is a local variable and this is a direct member access, quite likely to a value already in a register.

logrus.Debugf("Found a sigstore attachment manifest with %d layers", numLayers)
for layerIndex, layer := range layers {
// Note that this copies all kinds of attachments: attestations, and whatever else is there,
// not just signatures. We leave the signature consumers to decide based on the MIME type.
logrus.Debugf("Fetching sigstore attachment %d/%d: %s", layerIndex+1, len(ociManifest.Layers), layer.Digest.String())
logrus.Debugf("Fetching sigstore attachment %d/%d: %s", layerIndex+1, numLayers, layer.Digest.String())
// We don’t benefit from a real BlobInfoCache here because we never try to reuse/mount attachment payloads.
// That might eventually need to change if payloads grow to be not just signatures, but something
// significantly large.
payload, err := s.c.getOCIDescriptorContents(ctx, s.physicalRef, layer, iolimits.MaxSignatureBodySize,
descriptor := imgspecv1.Descriptor{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a generic types.BlobInfo; getOCIDescriptorContents used a raw OCI type just because that was convenient for this caller, but now that we need to make things generic, turning getOCIDescriptorContents into something generic would be simpler.

MediaType: layer.MediaType,
Digest: layer.Digest,
Size: layer.Size,
Annotations: layer.Annotations,
}
payload, err := s.c.getOCIDescriptorContents(ctx, s.physicalRef, descriptor, iolimits.MaxSignatureBodySize,
none.NoCache)
if err != nil {
return err
Expand Down