Merge pull request #3395 from fuweid/verify-layer-config-descriptor

OCI: verify digest and check blob presence when put manifest

registry/storage/ocimanifesthandler.go

@@ -81,7 +81,11 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc
 	blobsService := ms.repository.Blobs(ctx)
 
 	for _, descriptor := range mnfst.References() {
-		var err error
+		err := descriptor.Digest.Validate()
+		if err != nil {
+			errs = append(errs, err, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest})
+			continue
+		}
 
 		switch descriptor.MediaType {
 		case v1.MediaTypeImageLayer, v1.MediaTypeImageLayerGzip, v1.MediaTypeImageLayerNonDistributable, v1.MediaTypeImageLayerNonDistributableGzip:
@@ -95,9 +99,14 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc
 					break
 				}
 			}
-			if err == nil && len(descriptor.URLs) == 0 {
-				// If no URLs, require that the blob exists
-				_, err = blobsService.Stat(ctx, descriptor.Digest)
+			if err == nil {
+				// check the presence if it is normal layer or
+				// there is no urls for non-distributable
+				if len(descriptor.URLs) == 0 ||
+					(descriptor.MediaType == v1.MediaTypeImageLayer || descriptor.MediaType == v1.MediaTypeImageLayerGzip) {
+
+					_, err = blobsService.Stat(ctx, descriptor.Digest)
+				}
 			}
 
 		case v1.MediaTypeImageManifest:
@@ -107,12 +116,13 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc
 				err = distribution.ErrBlobUnknown // just coerce to unknown.
 			}
 
+			if err != nil {
+				dcontext.GetLogger(ms.ctx).WithError(err).Debugf("failed to ensure exists of %v in manifest service", descriptor.Digest)
+			}
 			fallthrough // double check the blob store.
 		default:
-			// forward all else to blob storage
-			if len(descriptor.URLs) == 0 {
-				_, err = blobsService.Stat(ctx, descriptor.Digest)
-			}
+			// check the presence
+			_, err = blobsService.Stat(ctx, descriptor.Digest)
 		}
 
 		if err != nil {

registry/storage/ocimanifesthandler_test.go

@@ -3,12 +3,14 @@ package storage
 import (
 	"context"
 	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/distribution/distribution/v3"
 	"github.com/distribution/distribution/v3/manifest"
 	"github.com/distribution/distribution/v3/manifest/ocischema"
 	"github.com/distribution/distribution/v3/registry/storage/driver/inmemory"
+	"github.com/opencontainers/go-digest"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
@@ -37,6 +39,15 @@ func TestVerifyOCIManifestNonDistributableLayer(t *testing.T) {
 		MediaType: v1.MediaTypeImageLayerNonDistributableGzip,
 	}
 
+	emptyLayer := distribution.Descriptor{
+		Digest: "",
+	}
+
+	emptyGzipLayer := distribution.Descriptor{
+		Digest:    "",
+		MediaType: v1.MediaTypeImageLayerGzip,
+	}
+
 	template := ocischema.Manifest{
 		Versioned: manifest.Versioned{
 			SchemaVersion: 2,
@@ -107,6 +118,26 @@ func TestVerifyOCIManifestNonDistributableLayer(t *testing.T) {
 			[]string{"https://foo/bar"},
 			nil,
 		},
+		{
+			emptyLayer,
+			[]string{"https://foo/empty"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			emptyLayer,
+			[]string{},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			emptyGzipLayer,
+			[]string{"https://foo/empty"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			emptyGzipLayer,
+			[]string{},
+			digest.ErrDigestInvalidFormat,
+		},
 	}
 
 	for _, c := range cases {
@@ -136,3 +167,168 @@ func TestVerifyOCIManifestNonDistributableLayer(t *testing.T) {
 		}
 	}
 }
+
+func TestVerifyOCIManifestBlobLayerAndConfig(t *testing.T) {
+	ctx := context.Background()
+	inmemoryDriver := inmemory.New()
+	registry := createRegistry(t, inmemoryDriver,
+		ManifestURLsAllowRegexp(regexp.MustCompile("^https?://foo")),
+		ManifestURLsDenyRegexp(regexp.MustCompile("^https?://foo/nope")))
+
+	repo := makeRepository(t, registry, strings.ToLower(t.Name()))
+	manifestService := makeManifestService(t, repo)
+
+	config, err := repo.Blobs(ctx).Put(ctx, v1.MediaTypeImageConfig, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	layer, err := repo.Blobs(ctx).Put(ctx, v1.MediaTypeImageLayerGzip, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	template := ocischema.Manifest{
+		Versioned: manifest.Versioned{
+			SchemaVersion: 2,
+			MediaType:     v1.MediaTypeImageManifest,
+		},
+	}
+
+	checkFn := func(m ocischema.Manifest, rerr error) {
+		dm, err := ocischema.FromStruct(m)
+		if err != nil {
+			t.Error(err)
+			return
+		}
+
+		_, err = manifestService.Put(ctx, dm)
+		if verr, ok := err.(distribution.ErrManifestVerification); ok {
+			// Extract the first error
+			if len(verr) == 2 {
+				if _, ok = verr[1].(distribution.ErrManifestBlobUnknown); ok {
+					err = verr[0]
+				}
+			} else if len(verr) == 1 {
+				err = verr[0]
+			}
+		}
+		if err != rerr {
+			t.Errorf("%#v: expected %v, got %v", m, rerr, err)
+		}
+	}
+
+	type testcase struct {
+		Desc distribution.Descriptor
+		URLs []string
+		Err  error
+	}
+
+	layercases := []testcase{
+		// empty media type
+		{
+			distribution.Descriptor{},
+			[]string{"http://foo/bar"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+		// unknown media type, but blob is present
+		{
+			distribution.Descriptor{
+				Digest: layer.Digest,
+			},
+			nil,
+			nil,
+		},
+		{
+			distribution.Descriptor{
+				Digest: layer.Digest,
+			},
+			[]string{"http://foo/bar"},
+			nil,
+		},
+		// gzip layer, but invalid digest
+		{
+			distribution.Descriptor{
+				MediaType: v1.MediaTypeImageLayerGzip,
+			},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{
+				MediaType: v1.MediaTypeImageLayerGzip,
+			},
+			[]string{"https://foo/bar"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{
+				MediaType: v1.MediaTypeImageLayerGzip,
+				Digest:    digest.Digest("invalid"),
+			},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+		// normal uploaded gzip layer
+		{
+			layer,
+			nil,
+			nil,
+		},
+		{
+			layer,
+			[]string{"https://foo/bar"},
+			nil,
+		},
+	}
+
+	for _, c := range layercases {
+		m := template
+		m.Config = config
+
+		l := c.Desc
+		l.URLs = c.URLs
+
+		m.Layers = []distribution.Descriptor{l}
+
+		checkFn(m, c.Err)
+	}
+
+	configcases := []testcase{
+		// valid config
+		{
+			config,
+			nil,
+			nil,
+		},
+		// invalid digest
+		{
+			distribution.Descriptor{
+				MediaType: v1.MediaTypeImageConfig,
+			},
+			[]string{"https://foo/bar"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{
+				MediaType: v1.MediaTypeImageConfig,
+				Digest:    digest.Digest("invalid"),
+			},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+	}
+
+	for _, c := range configcases {
+		m := template
+		m.Config = c.Desc
+		m.Config.URLs = c.URLs
+
+		checkFn(m, c.Err)
+	}
+}

registry/storage/schema2manifesthandler.go

@@ -87,7 +87,11 @@ func (ms *schema2ManifestHandler) verifyManifest(ctx context.Context, mnfst sche
 	blobsService := ms.repository.Blobs(ctx)
 
 	for _, descriptor := range mnfst.References() {
-		var err error
+		err := descriptor.Digest.Validate()
+		if err != nil {
+			errs = append(errs, err, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest})
+			continue
+		}
 
 		switch descriptor.MediaType {
 		case schema2.MediaTypeForeignLayer:
@@ -113,12 +117,13 @@ func (ms *schema2ManifestHandler) verifyManifest(ctx context.Context, mnfst sche
 				err = distribution.ErrBlobUnknown // just coerce to unknown.
 			}
 
+			if err != nil {
+				dcontext.GetLogger(ms.ctx).WithError(err).Debugf("failed to ensure exists of %v in manifest service", descriptor.Digest)
+			}
 			fallthrough // double check the blob store.
 		default:
-			// forward all else to blob storage
-			if len(descriptor.URLs) == 0 {
-				_, err = blobsService.Stat(ctx, descriptor.Digest)
-			}
+			// check its presence
+			_, err = blobsService.Stat(ctx, descriptor.Digest)
 		}
 
 		if err != nil {

registry/storage/schema2manifesthandler_test.go

@@ -2,6 +2,7 @@ package storage
 
 import (
 	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/distribution/distribution/v3"
@@ -9,6 +10,7 @@ import (
 	"github.com/distribution/distribution/v3/manifest"
 	"github.com/distribution/distribution/v3/manifest/schema2"
 	"github.com/distribution/distribution/v3/registry/storage/driver/inmemory"
+	"github.com/opencontainers/go-digest"
 )
 
 func TestVerifyManifestForeignLayer(t *testing.T) {
@@ -36,6 +38,10 @@ func TestVerifyManifestForeignLayer(t *testing.T) {
 		MediaType: schema2.MediaTypeForeignLayer,
 	}
 
+	emptyLayer := distribution.Descriptor{
+		Digest: "",
+	}
+
 	template := schema2.Manifest{
 		Versioned: manifest.Versioned{
 			SchemaVersion: 2,
@@ -107,6 +113,16 @@ func TestVerifyManifestForeignLayer(t *testing.T) {
 			[]string{"https://foo/bar"},
 			nil,
 		},
+		{
+			emptyLayer,
+			[]string{"https://foo/empty"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			emptyLayer,
+			[]string{},
+			digest.ErrDigestInvalidFormat,
+		},
 	}
 
 	for _, c := range cases {
@@ -134,3 +150,168 @@ func TestVerifyManifestForeignLayer(t *testing.T) {
 		}
 	}
 }
+
+func TestVerifyManifestBlobLayerAndConfig(t *testing.T) {
+	ctx := context.Background()
+	inmemoryDriver := inmemory.New()
+	registry := createRegistry(t, inmemoryDriver,
+		ManifestURLsAllowRegexp(regexp.MustCompile("^https?://foo")),
+		ManifestURLsDenyRegexp(regexp.MustCompile("^https?://foo/nope")))
+
+	repo := makeRepository(t, registry, strings.ToLower(t.Name()))
+	manifestService := makeManifestService(t, repo)
+
+	config, err := repo.Blobs(ctx).Put(ctx, schema2.MediaTypeImageConfig, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	layer, err := repo.Blobs(ctx).Put(ctx, schema2.MediaTypeLayer, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	template := schema2.Manifest{
+		Versioned: manifest.Versioned{
+			SchemaVersion: 2,
+			MediaType:     schema2.MediaTypeManifest,
+		},
+	}
+
+	checkFn := func(m schema2.Manifest, rerr error) {
+		dm, err := schema2.FromStruct(m)
+		if err != nil {
+			t.Error(err)
+			return
+		}
+
+		_, err = manifestService.Put(ctx, dm)
+		if verr, ok := err.(distribution.ErrManifestVerification); ok {
+			// Extract the first error
+			if len(verr) == 2 {
+				if _, ok = verr[1].(distribution.ErrManifestBlobUnknown); ok {
+					err = verr[0]
+				}
+			} else if len(verr) == 1 {
+				err = verr[0]
+			}
+		}
+		if err != rerr {
+			t.Errorf("%#v: expected %v, got %v", m, rerr, err)
+		}
+	}
+
+	type testcase struct {
+		Desc distribution.Descriptor
+		URLs []string
+		Err  error
+	}
+
+	layercases := []testcase{
+		// empty media type
+		{
+			distribution.Descriptor{},
+			[]string{"http://foo/bar"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+		// unknown media type, but blob is present
+		{
+			distribution.Descriptor{
+				Digest: layer.Digest,
+			},
+			nil,
+			nil,
+		},
+		{
+			distribution.Descriptor{
+				Digest: layer.Digest,
+			},
+			[]string{"http://foo/bar"},
+			nil,
+		},
+		// gzip layer, but invalid digest
+		{
+			distribution.Descriptor{
+				MediaType: schema2.MediaTypeLayer,
+			},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{
+				MediaType: schema2.MediaTypeLayer,
+			},
+			[]string{"https://foo/bar"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{
+				MediaType: schema2.MediaTypeLayer,
+				Digest:    digest.Digest("invalid"),
+			},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+		// normal uploaded gzip layer
+		{
+			layer,
+			nil,
+			nil,
+		},
+		{
+			layer,
+			[]string{"https://foo/bar"},
+			nil,
+		},
+	}
+
+	for _, c := range layercases {
+		m := template
+		m.Config = config
+
+		l := c.Desc
+		l.URLs = c.URLs
+
+		m.Layers = []distribution.Descriptor{l}
+
+		checkFn(m, c.Err)
+	}
+
+	configcases := []testcase{
+		// valid config
+		{
+			config,
+			nil,
+			nil,
+		},
+		// invalid digest
+		{
+			distribution.Descriptor{
+				MediaType: schema2.MediaTypeImageConfig,
+			},
+			[]string{"https://foo/bar"},
+			digest.ErrDigestInvalidFormat,
+		},
+		{
+			distribution.Descriptor{
+				MediaType: schema2.MediaTypeImageConfig,
+				Digest:    digest.Digest("invalid"),
+			},
+			nil,
+			digest.ErrDigestInvalidFormat,
+		},
+	}
+
+	for _, c := range configcases {
+		m := template
+		m.Config = c.Desc
+		m.Config.URLs = c.URLs
+
+		checkFn(m, c.Err)
+	}
+}