From 9e618c90c3544d38e722c5469bb774b548919f00 Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Thu, 15 Apr 2021 14:44:04 +0800 Subject: [PATCH] registry: verify digest and check blob presence when put manifest According to OCI image spec, the descriptor's digest field is required. For the normal config/layer blobs, the valivation should check the presence of the blob when put manifest. REF: https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md Signed-off-by: Arko Dasgupta Signed-off-by: Wei Fu --- registry/storage/ocimanifesthandler.go | 26 ++- registry/storage/ocimanifesthandler_test.go | 196 ++++++++++++++++++ registry/storage/schema2manifesthandler.go | 15 +- .../storage/schema2manifesthandler_test.go | 181 ++++++++++++++++ 4 files changed, 405 insertions(+), 13 deletions(-) diff --git a/registry/storage/ocimanifesthandler.go b/registry/storage/ocimanifesthandler.go index 242ae7587..2735a03ec 100644 --- a/registry/storage/ocimanifesthandler.go +++ b/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 { diff --git a/registry/storage/ocimanifesthandler_test.go b/registry/storage/ocimanifesthandler_test.go index d8a74b8a2..5eab9b9f6 100644 --- a/registry/storage/ocimanifesthandler_test.go +++ b/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) + } +} diff --git a/registry/storage/schema2manifesthandler.go b/registry/storage/schema2manifesthandler.go index fa4656232..023427c13 100644 --- a/registry/storage/schema2manifesthandler.go +++ b/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 { diff --git a/registry/storage/schema2manifesthandler_test.go b/registry/storage/schema2manifesthandler_test.go index ce4717ae4..908f8c137 100644 --- a/registry/storage/schema2manifesthandler_test.go +++ b/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) + } +}