Repository: google/go-containerregistry
Pull request #1076 (2021-07-12 20:23:39)

Use Data field when fetching in remote

codecov-commenter: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) Report
> Merging [#1076](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) (b001afe) into [main](https://codecov.io/gh/google/go-containerregistry/commit/b448abac9a7021605f2142ed6e04daa61c769324?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) (b448aba) will **increase** coverage by `0.05%`.
> The diff coverage is `86.66%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/1076/graphs/tree.svg?width=650&height=150&src=pr&token=wG7iiDCdZ4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)

```diff
@@            Coverage Diff             @@
##             main    #1076      +/-   ##
==========================================
+ Coverage   75.50%   75.55%   +0.05%     
==========================================
  Files         107      107              
  Lines        5094     5106      +12     
==========================================
+ Hits         3846     3858      +12     
  Misses        703      703              
  Partials      545      545              
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) | Coverage Δ | |
|---|---|---|
| [pkg/v1/manifest.go](https://codecov.io/gh/google/go-containerregistry/pull/1076/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-cGtnL3YxL21hbmlmZXN0Lmdv) | `100.00% <ø> (ø)` | |
| [pkg/v1/remote/index.go](https://codecov.io/gh/google/go-containerregistry/pull/1076/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-cGtnL3YxL3JlbW90ZS9pbmRleC5nbw==) | `84.76% <80.00%> (+1.08%)` | :arrow_up: |
| [pkg/v1/remote/image.go](https://codecov.io/gh/google/go-containerregistry/pull/1076/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-cGtnL3YxL3JlbW90ZS9pbWFnZS5nbw==) | `81.01% <100.00%> (+1.28%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google). Last update [b448aba...b001afe](https://codecov.io/gh/google/go-containerregistry/pull/1076?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).

jonjohnsonjr: > The addition of a possibly-gigantic data elements in manifest contents makes me want a remote.WithMaxManifestSize option (and maybe some sane large default) to prevent resource exhaustion when fetching manifests.

I think we'd need to rewrite a ton of code if we want to start going down this path. I'd propose we punt on that for now and rethink how this would work in a v2 world.

Patch:
diff --git a/internal/verify/verify.go b/internal/verify/verify.go
index a26f9958..9d62214f 100644
--- a/internal/verify/verify.go
+++ b/internal/verify/verify.go
@@ -17,7 +17,9 @@
 package verify
 
 import (
+	"bytes"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"hash"
 	"io"
@@ -81,3 +83,25 @@ func ReadCloser(r io.ReadCloser, size int64, h v1.Hash) (io.ReadCloser, error) {
 		CloseFunc: r.Close,
 	}, nil
 }
+
+// Descriptor verifies that the embedded Data field matches the Size and Digest
+// fields of the given v1.Descriptor, returning an error if the Data field is
+// missing or if it contains incorrect data.
+func Descriptor(d v1.Descriptor) error {
+	if d.Data == nil {
+		return errors.New("error verifying descriptor; Data == nil")
+	}
+
+	h, sz, err := v1.SHA256(bytes.NewReader(d.Data))
+	if err != nil {
+		return err
+	}
+	if h != d.Digest {
+		return fmt.Errorf("error verifying Digest; got %q, want %q", h, d.Digest)
+	}
+	if sz != d.Size {
+		return fmt.Errorf("error verifying Size; got %d, want %d", sz, d.Size)
+	}
+
+	return nil
+}
diff --git a/pkg/v1/manifest.go b/pkg/v1/manifest.go
index 51a46704..8ded1ee5 100644
--- a/pkg/v1/manifest.go
+++ b/pkg/v1/manifest.go
@@ -43,6 +43,7 @@ type Descriptor struct {
 	MediaType   types.MediaType   `json:"mediaType"`
 	Size        int64             `json:"size"`
 	Digest      Hash              `json:"digest"`
+	Data        []byte            `json:"data,omitempty"`
 	URLs        []string          `json:"urls,omitempty"`
 	Annotations map[string]string `json:"annotations,omitempty"`
 	Platform    *Platform         `json:"platform,omitempty"`
diff --git a/pkg/v1/mutate/image.go b/pkg/v1/mutate/image.go
index bbdaf1c8..de284c36 100644
--- a/pkg/v1/mutate/image.go
+++ b/pkg/v1/mutate/image.go
@@ -130,6 +130,11 @@ func (i *image) compute() error {
 	manifest.Config.Digest = d
 	manifest.Config.Size = sz
 
+	// If Data was set in the base image, we need to update it in the mutated image.
+	if m.Config.Data != nil {
+		manifest.Config.Data = rcfg
+	}
+
 	// With OCI media types, this should not be set, see discussion:
 	// https://github.com/opencontainers/image-spec/pull/795
 	if i.mediaType != nil {
diff --git a/pkg/v1/mutate/index.go b/pkg/v1/mutate/index.go
index 767b1b74..e1f4937c 100644
--- a/pkg/v1/mutate/index.go
+++ b/pkg/v1/mutate/index.go
@@ -51,6 +51,9 @@ func computeDescriptor(ia IndexAddendum) (*v1.Descriptor, error) {
 	if len(ia.Descriptor.Annotations) != 0 {
 		desc.Annotations = ia.Descriptor.Annotations
 	}
+	if ia.Descriptor.Data != nil {
+		desc.Data = ia.Descriptor.Data
+	}
 
 	return desc, nil
 }
diff --git a/pkg/v1/remote/image.go b/pkg/v1/remote/image.go
index dbffeeb8..a36416d8 100644
--- a/pkg/v1/remote/image.go
+++ b/pkg/v1/remote/image.go
@@ -15,6 +15,7 @@
 package remote
 
 import (
+	"bytes"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -100,6 +101,14 @@ func (r *remoteImage) RawConfigFile() ([]byte, error) {
 		return nil, err
 	}
 
+	if m.Config.Data != nil {
+		if err := verify.Descriptor(m.Config); err != nil {
+			return nil, err
+		}
+		r.config = m.Config.Data
+		return r.config, nil
+	}
+
 	body, err := r.fetchBlob(r.context, m.Config.Size, m.Config.Digest)
 	if err != nil {
 		return nil, err
@@ -143,6 +152,10 @@ func (rl *remoteImageLayer) Compressed() (io.ReadCloser, error) {
 		return nil, err
 	}
 
+	if d.Data != nil {
+		return verify.ReadCloser(ioutil.NopCloser(bytes.NewReader(d.Data)), d.Size, d.Digest)
+	}
+
 	// We don't want to log binary layers -- this can break terminals.
 	ctx := redact.NewContext(rl.ri.context, "omitting binary blobs from logs")
 
diff --git a/pkg/v1/remote/index.go b/pkg/v1/remote/index.go
index c1393435..7a542b38 100644
--- a/pkg/v1/remote/index.go
+++ b/pkg/v1/remote/index.go
@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"sync"
 
+	"github.com/google/go-containerregistry/internal/verify"
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/partial"
@@ -198,9 +199,20 @@ func (r *remoteIndex) childByHash(h v1.Hash) (*Descriptor, error) {
 // Convert one of this index's child's v1.Descriptor into a remote.Descriptor, with the given platform option.
 func (r *remoteIndex) childDescriptor(child v1.Descriptor, platform v1.Platform) (*Descriptor, error) {
 	ref := r.Ref.Context().Digest(child.Digest.String())
-	manifest, _, err := r.fetchManifest(ref, []types.MediaType{child.MediaType})
-	if err != nil {
-		return nil, err
+	var (
+		manifest []byte
+		err      error
+	)
+	if child.Data != nil {
+		if err := verify.Descriptor(child); err != nil {
+			return nil, err
+		}
+		manifest = child.Data
+	} else {
+		manifest, _, err = r.fetchManifest(ref, []types.MediaType{child.MediaType})
+		if err != nil {
+			return nil, err
+		}
 	}
 	return &Descriptor{
 		fetcher: fetcher{
diff --git a/pkg/v1/zz_deepcopy_generated.go b/pkg/v1/zz_deepcopy_generated.go
index 3f92f091..b32b8b77 100644
--- a/pkg/v1/zz_deepcopy_generated.go
+++ b/pkg/v1/zz_deepcopy_generated.go
@@ -115,6 +115,11 @@ func (in *ConfigFile) DeepCopy() *ConfigFile {
 func (in *Descriptor) DeepCopyInto(out *Descriptor) {
 	*out = *in
 	out.Digest = in.Digest
+	if in.Data != nil {
+		in, out := &in.Data, &out.Data
+		*out = make([]byte, len(*in))
+		copy(*out, *in)
+	}
 	if in.URLs != nil {
 		in, out := &in.URLs, &out.URLs
 		*out = make([]string, len(*in))


Test patch:
diff --git a/internal/verify/verify_test.go b/internal/verify/verify_test.go
index f4087f83..9de625db 100644
--- a/internal/verify/verify_test.go
+++ b/internal/verify/verify_test.go
@@ -16,6 +16,7 @@ package verify
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"strings"
@@ -86,3 +87,47 @@ func TestBadSize(t *testing.T) {
 		})
 	}
 }
+
+func TestDescriptor(t *testing.T) {
+	for _, tc := range []struct {
+		err  error
+		desc v1.Descriptor
+	}{{
+		err: errors.New("error verifying descriptor; Data == nil"),
+	}, {
+		err: errors.New(`error verifying Digest; got "sha256:ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad", want ":"`),
+		desc: v1.Descriptor{
+			Data: []byte("abc"),
+		},
+	}, {
+		err: errors.New("error verifying Size; got 3, want 0"),
+		desc: v1.Descriptor{
+			Data: []byte("abc"),
+			Digest: v1.Hash{
+				Algorithm: "sha256",
+				Hex:       "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad",
+			},
+		},
+	}, {
+		desc: v1.Descriptor{
+			Data: []byte("abc"),
+			Size: 3,
+			Digest: v1.Hash{
+				Algorithm: "sha256",
+				Hex:       "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad",
+			},
+		},
+	}} {
+		got, want := Descriptor(tc.desc), tc.err
+
+		if got == nil {
+			if want != nil {
+				t.Errorf("Descriptor(): got nil, want %v", want)
+			}
+		} else if want == nil {
+			t.Errorf("Descriptor(): got %v, want nil", got)
+		} else if got, want := got.Error(), want.Error(); got != want {
+			t.Errorf("Descriptor(): got %q, want %q", got, want)
+		}
+	}
+}
diff --git a/pkg/v1/remote/image_test.go b/pkg/v1/remote/image_test.go
index 9e7de066..338eef4a 100644
--- a/pkg/v1/remote/image_test.go
+++ b/pkg/v1/remote/image_test.go
@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -403,6 +404,8 @@ func TestPullingManifestList(t *testing.T) {
 	manifest.Manifests[0].Platform = &fakePlatform
 	// Make sure the second manifest does.
 	manifest.Manifests[1].Platform = &defaultPlatform
+	// Do short-circuiting via Data.
+	manifest.Manifests[1].Data = mustRawManifest(t, child)
 	rawManifest, err := json.Marshal(manifest)
 	if err != nil {
 		t.Fatal(err)
@@ -674,3 +677,65 @@ func TestPullingForeignLayer(t *testing.T) {
 		t.Errorf("failed to Write: %v", err)
 	}
 }
+
+func TestData(t *testing.T) {
+	img := randomImage(t)
+	manifest, err := img.Manifest()
+	if err != nil {
+		t.Fatal(err)
+	}
+	layers, err := img.Layers()
+	if err != nil {
+		t.Fatal(err)
+	}
+	cb, err := img.RawConfigFile()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	manifest.Config.Data = cb
+	rc, err := layers[0].Compressed()
+	if err != nil {
+		t.Fatal(err)
+	}
+	lb, err := ioutil.ReadAll(rc)
+	if err != nil {
+		t.Fatal(err)
+	}
+	manifest.Layers[0].Data = lb
+	rawManifest, err := json.Marshal(manifest)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/v2/":
+			w.WriteHeader(http.StatusOK)
+		case "/v2/test/manifests/latest":
+			if r.Method != http.MethodGet {
+				t.Errorf("Method; got %v, want %v", r.Method, http.MethodGet)
+			}
+			w.Write(rawManifest)
+		default:
+			// explode if we try to read blob or config
+			t.Fatalf("Unexpected path: %v", r.URL.Path)
+		}
+	}))
+	defer server.Close()
+	u, err := url.Parse(server.URL)
+	if err != nil {
+		t.Fatalf("url.Parse(%v) = %v", server.URL, err)
+	}
+	ref, err := newReference(u.Host, "test", "latest")
+	if err != nil {
+		t.Fatal(err)
+	}
+	rmt, err := Image(ref)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := validate.Image(rmt); err != nil {
+		t.Fatal(err)
+	}
+}