support sigstore attachments for v2s2 #2734
Conversation
fixes containers#2058 Signed-off-by: alex-berger <[email protected]>
There was a problem hiding this comment.
Thanks!
How did you test this? AFAICS this simply cannot work, when the schema2 definitions (as they exist in c/image) simply don’t have the “annotations” field which actually stores the signature.
Also, I think we’d need to build some amount of transport-agnostic manifest/image infrastructure to support this.
| 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) |
There was a problem hiding this comment.
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.
| // 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{ |
There was a problem hiding this comment.
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.
| // 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 { |
There was a problem hiding this comment.
Unmarshaling v2s2 into OCI is not semantically correct (loses data).
| // 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) { |
There was a problem hiding this comment.
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}).
| Size: layer.Size, | ||
| Annotations: layer.Annotations, | ||
| } | ||
| if layerMatchesSigstoreSignature(descriptor, mimeType, payloadBlob, annotations) { |
There was a problem hiding this comment.
This doesn’t work because schema2 manifests don’t have annotations at all.
| case *manifest.OCI1: | ||
| v.Layers = append(v.Layers, sigDesc) | ||
| case *manifest.Schema2: | ||
| v.LayersDescriptors = append(v.LayersDescriptors, manifest.Schema2Descriptor{ |
There was a problem hiding this comment.
This discards annotations (schema2 manifests don’t have annotations at all), and as a result, the signature is unusable.
| case *manifest.OCI1: | ||
| v.Config = configDesc | ||
| case *manifest.Schema2: | ||
| v.ConfigDescriptor = manifest.Schema2Descriptor{ |
There was a problem hiding this comment.
- 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.
Currently we support sigstore attachments for OCI. This PR adds support for sigstore attachments for v2s2 (Docker Registry HTTP API V2 Schema 2 Manifest).
fixes #2058