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,