From 57467104484ce744211231cc6ac749b4afeb439f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 25 Apr 2023 22:06:42 +0200 Subject: [PATCH 1/4] Fix conversion determination when encrypting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - When encrypting, actually make a choice even if the destination does not care about manifest formats - Don't try converting to formats that don't support encryption at all - Fail with a readable error messsage instead of "internal error" if we filter out all candidates. Signed-off-by: Miloslav Trmač --- copy/manifest.go | 44 ++++++++++-- copy/manifest_test.go | 158 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 7 deletions(-) diff --git a/copy/manifest.go b/copy/manifest.go index a35ea42205..6f01cf5cc3 100644 --- a/copy/manifest.go +++ b/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,11 +80,14 @@ 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 { @@ -88,6 +95,27 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest 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{ diff --git a/copy/manifest_test.go b/copy/manifest_test.go index 36a9a8a24d..0a7d6540af 100644 --- a/copy/manifest_test.go +++ b/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, From 0958a34ea54794e4b52ebd10ab8b5b70c7ada6f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 25 Apr 2023 22:14:36 +0200 Subject: [PATCH 2/4] Move blobPipelineDecryptionStep and blobPipelineEncryptionStep to imageCopier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will want to access imageCopier.cannotModifyManifestReason. All the ...Step functions are now methods of imageCopier, which is more consistent. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 4 ++-- copy/encryption.go | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index 96674ddbb8..f45b97f56c 100644 --- a/copy/blob.go +++ b/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 } diff --git a/copy/encryption.go b/copy/encryption.go index 54aca9e572..01d1ab79cb 100644 --- a/copy/encryption.go +++ b/copy/encryption.go @@ -33,12 +33,15 @@ 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 { +func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) { + if isOciEncrypted(stream.info.MediaType) && ic.c.ociDecryptConfig != 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(c.ociDecryptConfig, stream.reader, desc, false) + 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) } @@ -74,9 +77,13 @@ 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 { + if toEncrypt && !isOciEncrypted(srcInfo.MediaType) && ic.c.ociEncryptConfig != 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 @@ -87,7 +94,7 @@ func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool Size: srcInfo.Size, Annotations: annotations, } - reader, finalizer, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) + 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) } From 90085974185eb3a11ef4cd8ef62b213f10bc0337 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 25 Apr 2023 22:18:34 +0200 Subject: [PATCH 3/4] Invert the top-level control flow of encryption steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exit early if we are not changing anything, to make the "no change" case easier to follow, and most of the code less indented. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/encryption.go | 87 ++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 46 deletions(-) diff --git a/copy/encryption.go b/copy/encryption.go index 01d1ab79cb..555f25e33d 100644 --- a/copy/encryption.go +++ b/copy/encryption.go @@ -34,30 +34,28 @@ type bpDecryptionStepData struct { // srcInfo is only used for error messages. // Returns data for other steps; the caller should eventually use updateCryptoOperation. func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) { - if isOciEncrypted(stream.info.MediaType) && ic.c.ociDecryptConfig != 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") - }) + if !isOciEncrypted(stream.info.MediaType) || ic.c.ociDecryptConfig == nil { return &bpDecryptionStepData{ - decrypting: true, + decrypting: false, }, nil } + + 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 } @@ -79,36 +77,33 @@ type bpEncryptionStepData struct { // Returns data for other steps; the caller should eventually call updateCryptoOperationAndAnnotations. func (ic *imageCopier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool, srcInfo types.BlobInfo, decryptionStep *bpDecryptionStepData) (*bpEncryptionStepData, error) { - if toEncrypt && !isOciEncrypted(srcInfo.MediaType) && ic.c.ociEncryptConfig != 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 + if !toEncrypt || isOciEncrypted(srcInfo.MediaType) || ic.c.ociEncryptConfig == nil { return &bpEncryptionStepData{ - encrypting: true, - finalizer: finalizer, + encrypting: false, }, 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(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 } From cd5d28739ca9d114219274f6972cc1122399e52e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 25 Apr 2023 22:19:20 +0200 Subject: [PATCH 4/4] Explicitly fail encryption/decryption if we can't change the manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We would fail with an internal error anyway, this fails explicitly. Signed-off-by: Miloslav Trmač --- copy/encryption.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/copy/encryption.go b/copy/encryption.go index 555f25e33d..86fadff66e 100644 --- a/copy/encryption.go +++ b/copy/encryption.go @@ -40,6 +40,10 @@ func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo }, 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, } @@ -83,6 +87,10 @@ func (ic *imageCopier) blobPipelineEncryptionStep(stream *sourceStream, toEncryp }, 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