Skip to content

Commit

Permalink
Fix conversion determination when encrypting
Browse files Browse the repository at this point in the history 
- 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č <[email protected]>
  • Loading branch information
@mtrmac
mtrmac committed Apr 25, 2023
1 parent f5e31f1 commit 5746710
Show file tree
Hide file tree
Showing 2 changed files with 195 additions and 7 deletions.
44 changes: 37 additions & 7 deletions copy/manifest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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{
Expand Down
158 changes: 158 additions & 0 deletions copy/manifest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 5746710

Please sign in to comment.