Merge pull request #1930 from mtrmac/encryption-mime-types

Fix conversion determination when encrypting

copy/blob.go

@@ -43,7 +43,7 @@ func (ic *imageCopier) copyBlobFromStream(ctx context.Context, srcReader io.Read
  stream.reader = bar.ProxyReader(stream.reader)
 
  // === Decrypt the stream, if required.
- decryptionStep, err := ic.c.blobPipelineDecryptionStep(&stream, srcInfo)
+ decryptionStep, err := ic.blobPipelineDecryptionStep(&stream, srcInfo)
  if err != nil {
  return types.BlobInfo{}, err
  }
@@ -78,7 +78,7 @@ func (ic *imageCopier) copyBlobFromStream(ctx context.Context, srcReader io.Read
  // Before relaxing this, see the original pull request’s review if there are other reasons to reject this.
  return types.BlobInfo{}, errors.New("Unable to support both decryption and encryption in the same copy")
  }
- encryptionStep, err := ic.c.blobPipelineEncryptionStep(&stream, toEncrypt, srcInfo, decryptionStep)
+ encryptionStep, err := ic.blobPipelineEncryptionStep(&stream, toEncrypt, srcInfo, decryptionStep)
  if err != nil {
  return types.BlobInfo{}, err
  }

copy/encryption.go

@@ -33,28 +33,33 @@ type bpDecryptionStepData struct {
 // blobPipelineDecryptionStep updates *stream to decrypt if, it necessary.
 // srcInfo is only used for error messages.
 // Returns data for other steps; the caller should eventually use updateCryptoOperation.
-func (c *copier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) {
- if isOciEncrypted(stream.info.MediaType) && c.ociDecryptConfig != nil {
- desc := imgspecv1.Descriptor{
- Annotations: stream.info.Annotations,
- }
- reader, decryptedDigest, err := ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, desc, false)
- if err != nil {
- return nil, fmt.Errorf("decrypting layer %s: %w", srcInfo.Digest, err)
- }
-
- stream.reader = reader
- stream.info.Digest = decryptedDigest
- stream.info.Size = -1
- maps.DeleteFunc(stream.info.Annotations, func(k string, _ string) bool {
- return strings.HasPrefix(k, "org.opencontainers.image.enc")
- })
+func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) {
+ if !isOciEncrypted(stream.info.MediaType) || ic.c.ociDecryptConfig == nil {
  return &bpDecryptionStepData{
- decrypting: true,
+ decrypting: false,
  }, nil
  }
+
+ if ic.cannotModifyManifestReason != "" {
+ return nil, fmt.Errorf("layer %s should be decrypted, but we can’t modify the manifest: %s", srcInfo.Digest, ic.cannotModifyManifestReason)
+ }
+
+ desc := imgspecv1.Descriptor{
+ Annotations: stream.info.Annotations,
+ }
+ reader, decryptedDigest, err := ocicrypt.DecryptLayer(ic.c.ociDecryptConfig, stream.reader, desc, false)
+ if err != nil {
+ return nil, fmt.Errorf("decrypting layer %s: %w", srcInfo.Digest, err)
+ }
+
+ stream.reader = reader
+ stream.info.Digest = decryptedDigest
+ stream.info.Size = -1
+ maps.DeleteFunc(stream.info.Annotations, func(k string, _ string) bool {
+ return strings.HasPrefix(k, "org.opencontainers.image.enc")
+ })
  return &bpDecryptionStepData{
- decrypting: false,
+ decrypting: true,
  }, nil
 }
 
@@ -74,34 +79,39 @@ type bpEncryptionStepData struct {
 // blobPipelineEncryptionStep updates *stream to encrypt if, it required by toEncrypt.
 // srcInfo is primarily used for error messages.
 // Returns data for other steps; the caller should eventually call updateCryptoOperationAndAnnotations.
-func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool, srcInfo types.BlobInfo,
+func (ic *imageCopier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool, srcInfo types.BlobInfo,
  decryptionStep *bpDecryptionStepData) (*bpEncryptionStepData, error) {
- if toEncrypt && !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil {
- var annotations map[string]string
- if !decryptionStep.decrypting {
- annotations = srcInfo.Annotations
- }
- desc := imgspecv1.Descriptor{
- MediaType: srcInfo.MediaType,
- Digest: srcInfo.Digest,
- Size: srcInfo.Size,
- Annotations: annotations,
- }
- reader, finalizer, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc)
- if err != nil {
- return nil, fmt.Errorf("encrypting blob %s: %w", srcInfo.Digest, err)
- }
-
- stream.reader = reader
- stream.info.Digest = ""
- stream.info.Size = -1
+ if !toEncrypt || isOciEncrypted(srcInfo.MediaType) || ic.c.ociEncryptConfig == nil {
  return &bpEncryptionStepData{
- encrypting: true,
- finalizer: finalizer,
+ encrypting: false,
  }, nil
  }
+
+ if ic.cannotModifyManifestReason != "" {
+ return nil, fmt.Errorf("layer %s should be encrypted, but we can’t modify the manifest: %s", srcInfo.Digest, ic.cannotModifyManifestReason)
+ }
+
+ var annotations map[string]string
+ if !decryptionStep.decrypting {
+ annotations = srcInfo.Annotations
+ }
+ desc := imgspecv1.Descriptor{
+ MediaType: srcInfo.MediaType,
+ Digest: srcInfo.Digest,
+ Size: srcInfo.Size,
+ Annotations: annotations,
+ }
+ reader, finalizer, err := ocicrypt.EncryptLayer(ic.c.ociEncryptConfig, stream.reader, desc)
+ if err != nil {
+ return nil, fmt.Errorf("encrypting blob %s: %w", srcInfo.Digest, err)
+ }
+
+ stream.reader = reader
+ stream.info.Digest = ""
+ stream.info.Size = -1
  return &bpEncryptionStepData{
- encrypting: false,
+ encrypting: true,
+ finalizer: finalizer,
  }, nil
 }
 

copy/manifest.go

@@ -9,6 +9,7 @@ import (
  "github.com/containers/image/v5/internal/set"
  "github.com/containers/image/v5/manifest"
  "github.com/containers/image/v5/types"
+ v1 "github.com/opencontainers/image-spec/specs-go/v1"
  "github.com/sirupsen/logrus"
  "golang.org/x/exp/slices"
 )
@@ -18,6 +19,9 @@ import (
 // Include v2s1 signed but not v2s1 unsigned, because docker/distribution requires a signature even if the unsigned MIME type is used.
 var preferredManifestMIMETypes = []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1SignedMediaType}
 
+// ociEncryptionMIMETypes lists manifest MIME types that are known to support OCI encryption.
+var ociEncryptionMIMETypes = []string{v1.MediaTypeImageManifest}
+
 // orderedSet is a list of strings (MIME types or platform descriptors in our case), with each string appearing at most once.
 type orderedSet struct {
  list []string
@@ -76,18 +80,42 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
  destSupportedManifestMIMETypes = []string{in.forceManifestMIMEType}
  }
 
- if len(destSupportedManifestMIMETypes) == 0 && (!in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(srcType)) {
- return manifestConversionPlan{ // Anything goes; just use the original as is, do not try any conversions.
- preferredMIMEType: srcType,
- otherMIMETypeCandidates: []string{},
- }, nil
+ if len(destSupportedManifestMIMETypes) == 0 {
+ if !in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(srcType) {
+ return manifestConversionPlan{ // Anything goes; just use the original as is, do not try any conversions.
+ preferredMIMEType: srcType,
+ otherMIMETypeCandidates: []string{},
+ }, nil
+ }
+ destSupportedManifestMIMETypes = ociEncryptionMIMETypes
  }
  supportedByDest := set.New[string]()
  for _, t := range destSupportedManifestMIMETypes {
  if !in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(t) {
  supportedByDest.Add(t)
  }
  }
+ if supportedByDest.Empty() {
+ if len(destSupportedManifestMIMETypes) == 0 { // Coverage: This should never happen, empty values were replaced by ociEncryptionMIMETypes
+ return manifestConversionPlan{}, errors.New("internal error: destSupportedManifestMIMETypes is empty")
+ }
+ // We know, and have verified, that destSupportedManifestMIMETypes is not empty, so encryption must have been involved.
+ if !in.requiresOCIEncryption { // Coverage: This should never happen, destSupportedManifestMIMETypes was not empty, so we should have filtered for encryption.
+ return manifestConversionPlan{}, errors.New("internal error: supportedByDest is empty but destSupportedManifestMIMETypes is not, and not encrypting")
+ }
+ // destSupportedManifestMIMETypes has three possible origins:
+ if in.forceManifestMIMEType != "" { // 1. forceManifestType specified
+ return manifestConversionPlan{}, fmt.Errorf("encryption required together with format %s, which does not support encryption",
+ in.forceManifestMIMEType)
+ }
+ if len(in.destSupportedManifestMIMETypes) == 0 { // 2. destination accepts anything and we have chosen ociEncryptionMIMETypes
+ // Coverage: This should never happen, ociEncryptionMIMETypes all support encryption
+ return manifestConversionPlan{}, errors.New("internal error: in.destSupportedManifestMIMETypes is empty but supportedByDest is empty as well")
+ }
+ // 3. destination does not support encryption.
+ return manifestConversionPlan{}, fmt.Errorf("encryption required but the destination only supports MIME types [%s], none of which support encryption",
+ strings.Join(destSupportedManifestMIMETypes, ", "))
+ }
 
  // destSupportedManifestMIMETypes is a static guess; a particular registry may still only support a subset of the types.
  // So, build a list of types to try in order of decreasing preference.
@@ -122,11 +150,13 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
 
  // Finally, try anything else the destination supports.
  for _, t := range destSupportedManifestMIMETypes {
- prioritizedTypes.append(t)
+ if supportedByDest.Contains(t) {
+ prioritizedTypes.append(t)
+ }
  }
 
  logrus.Debugf("Manifest has MIME type %s, ordered candidate list [%s]", srcType, strings.Join(prioritizedTypes.list, ", "))
- if len(prioritizedTypes.list) == 0 { // Coverage: destSupportedManifestMIMETypes is not empty (or we would have exited in the “Anything goes” case above), so this should never happen.
+ if len(prioritizedTypes.list) == 0 { // Coverage: destSupportedManifestMIMETypes and supportedByDest, which is a subset, is not empty (or we would have exited above), so this should never happen.
  return manifestConversionPlan{}, errors.New("Internal error: no candidate MIME types")
  }
  res := manifestConversionPlan{

copy/manifest_test.go

@@ -215,6 +215,164 @@ func TestDetermineManifestConversion(t *testing.T) {
  otherMIMETypeCandidates: []string{},
  }, res, c.description)
  }
+
+ // When encryption is required:
+ for _, c := range []struct {
+ description string
+ in determineManifestConversionInputs // with requiresOCIEncryption implied
+ expected manifestConversionPlan // Or {} to expect a failure
+ }{
+ { // Destination accepts anything - no conversion necessary
+ "OCI→anything",
+ determineManifestConversionInputs{
+ srcMIMEType: v1.MediaTypeImageManifest,
+ destSupportedManifestMIMETypes: nil,
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: false,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ { // Destination accepts anything - need to convert for encryption
+ "s2→anything",
+ determineManifestConversionInputs{
+ srcMIMEType: manifest.DockerV2Schema2MediaType,
+ destSupportedManifestMIMETypes: nil,
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: true,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ // Destination accepts an encrypted format
+ {
+ "OCI→OCI",
+ determineManifestConversionInputs{
+ srcMIMEType: v1.MediaTypeImageManifest,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: false,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ {
+ "s2→OCI",
+ determineManifestConversionInputs{
+ srcMIMEType: manifest.DockerV2Schema2MediaType,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: true,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ // Destination does not accept an encrypted format
+ {
+ "OCI→s2",
+ determineManifestConversionInputs{
+ srcMIMEType: v1.MediaTypeImageManifest,
+ destSupportedManifestMIMETypes: supportS1S2,
+ },
+ manifestConversionPlan{},
+ },
+ {
+ "s2→s2",
+ determineManifestConversionInputs{
+ srcMIMEType: manifest.DockerV2Schema2MediaType,
+ destSupportedManifestMIMETypes: supportS1S2,
+ },
+ manifestConversionPlan{},
+ },
+ // Whatever the input is, with cannotModifyManifestReason we return "keep the original as is".
+ // Still, encryption is necessarily going to fail…
+ {
+ "OCI→OCI cannotModifyManifestReason",
+ determineManifestConversionInputs{
+ srcMIMEType: v1.MediaTypeImageManifest,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ cannotModifyManifestReason: "Preserving digests",
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: false,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ {
+ "s2→OCI cannotModifyManifestReason",
+ determineManifestConversionInputs{
+ srcMIMEType: manifest.DockerV2Schema2MediaType,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ cannotModifyManifestReason: "Preserving digests",
+ },
+ manifestConversionPlan{
+ preferredMIMEType: manifest.DockerV2Schema2MediaType,
+ preferredMIMETypeNeedsConversion: false,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ // forceManifestMIMEType to a type that supports encryption
+ {
+ "OCI→OCI forced",
+ determineManifestConversionInputs{
+ srcMIMEType: v1.MediaTypeImageManifest,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ forceManifestMIMEType: v1.MediaTypeImageManifest,
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: false,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ {
+ "s2→OCI forced",
+ determineManifestConversionInputs{
+ srcMIMEType: manifest.DockerV2Schema2MediaType,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ forceManifestMIMEType: v1.MediaTypeImageManifest,
+ },
+ manifestConversionPlan{
+ preferredMIMEType: v1.MediaTypeImageManifest,
+ preferredMIMETypeNeedsConversion: true,
+ otherMIMETypeCandidates: []string{},
+ },
+ },
+ // forceManifestMIMEType to a type that does not support encryption
+ {
+ "OCI→s2 forced",
+ determineManifestConversionInputs{
+ srcMIMEType: v1.MediaTypeImageManifest,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ forceManifestMIMEType: manifest.DockerV2Schema2MediaType,
+ },
+ manifestConversionPlan{},
+ },
+ {
+ "s2→s2 forced",
+ determineManifestConversionInputs{
+ srcMIMEType: manifest.DockerV2Schema2MediaType,
+ destSupportedManifestMIMETypes: supportS1S2OCI,
+ forceManifestMIMEType: manifest.DockerV2Schema2MediaType,
+ },
+ manifestConversionPlan{},
+ },
+ } {
+ in := c.in
+ in.requiresOCIEncryption = true
+ res, err := determineManifestConversion(in)
+ if c.expected.preferredMIMEType != "" {
+ require.NoError(t, err, c.description)
+ assert.Equal(t, c.expected, res, c.description)
+ } else {
+ assert.Error(t, err, c.description)
+ }
+ }
 }
 
 // fakeUnparsedImage is an implementation of types.UnparsedImage which only returns itself as a MIME type in Manifest,