Make mutate constructs immutable. (#1124)

I noticed that `cosign` was taking advantage of a bug in the mutate package
where the `Manifest()` returns a mutable copy of it's internal state, which
allowed them to permanently alter the result of `Manifest()`.  This mutability
violates the philosophy of our abstractions, and should not be allowed, so
this change ensures that each of the methods returning state in the `mutate`
package return a `.DeepCopy()` instead.  In `image` this included the
`configFile`, and `manifest`.  In `index` this included the `indexManifest`.
I have added tests that when the returned copies are mutated, a subsequent
invocation of the getter will return a pristine copy.

Since I'm not completely heartless, I am also adding a new
`mutate.ConfigMediaType` method to enable `cosign` to perform the alteration
they want, now that the trick they were using will no longer work.

Related: sigstore/cosign#718

pkg/legacy/tarball/write_test.go

@@ -382,9 +382,15 @@ func TestMultiWriteMismatchedHistory(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error getting image config: %v", err)
 	}
+
 	// Set the history such that number of history entries != layers. This
 	// should trigger an error during the image write.
 	cfg.History = make([]v1.History, 1)
+	img, err = mutate.ConfigFile(img, cfg)
+	if err != nil {
+		t.Fatalf("mutate.ConfigFile() = %v", err)
+	}
+
 	tag, err := name.NewTag("gcr.io/foo/bar:latest", name.StrictValidation)
 	if err != nil {
 		t.Fatalf("Error creating test tag: %v", err)

pkg/v1/mutate/image.go

@@ -30,13 +30,14 @@ type image struct {
 	base v1.Image
 	adds []Addendum
 
-	computed    bool
-	configFile  *v1.ConfigFile
-	manifest    *v1.Manifest
-	annotations map[string]string
-	mediaType   *types.MediaType
-	diffIDMap   map[v1.Hash]v1.Layer
-	digestMap   map[v1.Hash]v1.Layer
+	computed        bool
+	configFile      *v1.ConfigFile
+	manifest        *v1.Manifest
+	annotations     map[string]string
+	mediaType       *types.MediaType
+	configMediaType *types.MediaType
+	diffIDMap       map[v1.Hash]v1.Layer
+	digestMap       map[v1.Hash]v1.Layer
 }
 
 var _ v1.Image = (*image)(nil)
@@ -135,6 +136,11 @@ func (i *image) compute() error {
 		manifest.Config.Data = rcfg
 	}
 
+	// If the user wants to mutate the media type of the config
+	if i.configMediaType != nil {
+		manifest.Config.MediaType = *i.configMediaType
+	}
+
 	// With OCI media types, this should not be set, see discussion:
 	// https://github.com/opencontainers/image-spec/pull/795
 	if i.mediaType != nil {
@@ -210,7 +216,7 @@ func (i *image) ConfigFile() (*v1.ConfigFile, error) {
 	if err := i.compute(); err != nil {
 		return nil, err
 	}
-	return i.configFile, nil
+	return i.configFile.DeepCopy(), nil
 }
 
 // RawConfigFile returns the serialized bytes of ConfigFile()
@@ -242,7 +248,7 @@ func (i *image) Manifest() (*v1.Manifest, error) {
 	if err := i.compute(); err != nil {
 		return nil, err
 	}
-	return i.manifest, nil
+	return i.manifest.DeepCopy(), nil
 }
 
 // RawManifest returns the serialized bytes of Manifest()

pkg/v1/mutate/index.go

@@ -197,7 +197,7 @@ func (i *index) IndexManifest() (*v1.IndexManifest, error) {
 	if err := i.compute(); err != nil {
 		return nil, err
 	}
-	return i.manifest, nil
+	return i.manifest.DeepCopy(), nil
 }
 
 // RawManifest returns the serialized bytes of Manifest()

pkg/v1/mutate/index_test.go

@@ -19,6 +19,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
@@ -137,3 +138,45 @@ func TestAppendIndex(t *testing.T) {
 		t.Errorf("Validate() = %v", err)
 	}
 }
+
+func TestIndexImmutability(t *testing.T) {
+	base, err := random.Index(1024, 3, 3)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ii, err := random.Index(2048, 1, 2)
+	if err != nil {
+		t.Fatal(err)
+	}
+	i, err := random.Image(4096, 5)
+	if err != nil {
+		t.Fatal(err)
+	}
+	idx := mutate.AppendManifests(base, mutate.IndexAddendum{
+		Add: ii,
+		Descriptor: v1.Descriptor{
+			URLs: []string{"index.example.com"},
+		},
+	}, mutate.IndexAddendum{
+		Add: i,
+		Descriptor: v1.Descriptor{
+			URLs: []string{"image.example.com"},
+		},
+	})
+
+	t.Run("index manifest", func(t *testing.T) {
+		// Check that Manifest is immutable.
+		changed, err := idx.IndexManifest()
+		if err != nil {
+			t.Errorf("IndexManifest() = %v", err)
+		}
+		want := changed.DeepCopy() // Create a copy of original before mutating it.
+		changed.MediaType = types.DockerManifestList
+
+		if got, err := idx.IndexManifest(); err != nil {
+			t.Errorf("IndexManifest() = %v", err)
+		} else if !cmp.Equal(got, want) {
+			t.Errorf("IndexManifest changed! %s", cmp.Diff(got, want))
+		}
+	})
+}

pkg/v1/mutate/mutate.go

@@ -459,6 +459,14 @@ func MediaType(img v1.Image, mt types.MediaType) v1.Image {
 	}
 }
 
+// ConfigMediaType modifies the MediaType() of the given image's Config.
+func ConfigMediaType(img v1.Image, mt types.MediaType) v1.Image {
+	return &image{
+		base:            img,
+		configMediaType: &mt,
+	}
+}
+
 // IndexMediaType modifies the MediaType() of the given index.
 func IndexMediaType(idx v1.ImageIndex, mt types.MediaType) v1.ImageIndex {
 	return &index{

pkg/v1/mutate/mutate_test.go

@@ -344,7 +344,9 @@ func TestMutateTime(t *testing.T) {
 
 func TestMutateMediaType(t *testing.T) {
 	want := types.OCIManifestSchema1
+	wantCfg := types.OCIConfigJSON
 	img := mutate.MediaType(empty.Image, want)
+	img = mutate.ConfigMediaType(img, wantCfg)
 	got, err := img.MediaType()
 	if err != nil {
 		t.Fatal(err)
@@ -359,9 +361,14 @@ func TestMutateMediaType(t *testing.T) {
 	if manifest.MediaType != "" {
 		t.Errorf("MediaType should not be set for OCI media types: %v", manifest.MediaType)
 	}
+	if gotCfg := manifest.Config.MediaType; gotCfg != wantCfg {
+		t.Errorf("manifest.Config.MediaType = %v, wanted %v", gotCfg, wantCfg)
+	}
 
 	want = types.DockerManifestSchema2
+	wantCfg = types.DockerConfigJSON
 	img = mutate.MediaType(img, want)
+	img = mutate.ConfigMediaType(img, wantCfg)
 	got, err = img.MediaType()
 	if err != nil {
 		t.Fatal(err)
@@ -376,6 +383,9 @@ func TestMutateMediaType(t *testing.T) {
 	if manifest.MediaType != want {
 		t.Errorf("MediaType should be set for Docker media types: %v", manifest.MediaType)
 	}
+	if gotCfg := manifest.Config.MediaType; gotCfg != wantCfg {
+		t.Errorf("manifest.Config.MediaType = %v, wanted %v", gotCfg, wantCfg)
+	}
 
 	want = types.OCIImageIndex
 	idx := mutate.IndexMediaType(empty.Index, want)
@@ -559,6 +569,42 @@ func TestRemoveManifests(t *testing.T) {
 	}
 }
 
+func TestImageImmutability(t *testing.T) {
+	img := mutate.MediaType(empty.Image, types.OCIManifestSchema1)
+
+	t.Run("manifest", func(t *testing.T) {
+		// Check that Manifest is immutable.
+		changed, err := img.Manifest()
+		if err != nil {
+			t.Errorf("Manifest() = %v", err)
+		}
+		want := changed.DeepCopy() // Create a copy of original before mutating it.
+		changed.MediaType = types.DockerManifestList
+
+		if got, err := img.Manifest(); err != nil {
+			t.Errorf("Manifest() = %v", err)
+		} else if !cmp.Equal(got, want) {
+			t.Errorf("manifest changed! %s", cmp.Diff(got, want))
+		}
+	})
+
+	t.Run("config file", func(t *testing.T) {
+		// Check that ConfigFile is immutable.
+		changed, err := img.ConfigFile()
+		if err != nil {
+			t.Errorf("ConfigFile() = %v", err)
+		}
+		want := changed.DeepCopy() // Create a copy of original before mutating it.
+		changed.Author = "Jay Pegg"
+
+		if got, err := img.ConfigFile(); err != nil {
+			t.Errorf("ConfigFile() = %v", err)
+		} else if !cmp.Equal(got, want) {
+			t.Errorf("ConfigFile changed! %s", cmp.Diff(got, want))
+		}
+	})
+}
+
 func assertMTime(t *testing.T, layer v1.Layer, expectedTime time.Time) {
 	l, err := layer.Uncompressed()