diff --git a/registry/storage/manifeststore.go b/registry/storage/manifeststore.go index 3a897ab197b..7c04c238608 100644 --- a/registry/storage/manifeststore.go +++ b/registry/storage/manifeststore.go @@ -92,7 +92,11 @@ func (ms *manifestStore) Get(ctx context.Context, dgst digest.Digest, options .. var versioned manifest.Versioned if err = json.Unmarshal(content, &versioned); err != nil { - return nil, err + dcontext.GetLogger(ctx).Warnf("manifest %s: content is not valid JSON: %v", dgst, err) + return nil, distribution.ErrManifestUnknownRevision{ + Name: ms.repository.Named().Name(), + Revision: dgst, + } } switch versioned.SchemaVersion { @@ -114,19 +118,50 @@ func (ms *manifestStore) Get(ctx context.Context, dgst digest.Digest, options .. // First see if it looks like an image index res, err := ms.ocischemaIndexHandler.Unmarshal(ctx, dgst, content) - resIndex := res.(*ocischema.DeserializedImageIndex) - if err == nil && resIndex.Manifests != nil { - return resIndex, nil + if err == nil { + if resIndex, ok := res.(*ocischema.DeserializedImageIndex); ok && resIndex.Manifests != nil { + return resIndex, nil + } } // Otherwise, assume it must be an image manifest - return ms.ocischemaHandler.Unmarshal(ctx, dgst, content) + res, err = ms.ocischemaHandler.Unmarshal(ctx, dgst, content) + if err != nil { + dcontext.GetLogger(ctx).Warnf("manifest %s: content without a media type is not a valid image manifest: %v", dgst, err) + return nil, distribution.ErrManifestUnknownRevision{ + Name: ms.repository.Named().Name(), + Revision: dgst, + } + } + + // Unmarshalling an image manifest only rejects a conflicting media + // type, so content that is not a manifest at all still deserializes + // into an empty one. Every image manifest references a config blob; + // without one the content describes nothing and is not servable. + resManifest, ok := res.(*ocischema.DeserializedManifest) + if !ok || resManifest.Config.Digest == "" { + dcontext.GetLogger(ctx).Warnf("manifest %s: content without a media type has no config descriptor", dgst) + return nil, distribution.ErrManifestUnknownRevision{ + Name: ms.repository.Named().Name(), + Revision: dgst, + } + } + + return resManifest, nil default: - return nil, distribution.ErrManifestVerification{fmt.Errorf("unrecognized manifest content type %s", versioned.MediaType)} + dcontext.GetLogger(ctx).Warnf("manifest %s: unrecognized manifest content type %s", dgst, versioned.MediaType) + return nil, distribution.ErrManifestUnknownRevision{ + Name: ms.repository.Named().Name(), + Revision: dgst, + } } } - return nil, fmt.Errorf("unrecognized manifest schema version %d", versioned.SchemaVersion) + dcontext.GetLogger(ctx).Warnf("manifest %s: unrecognized manifest schema version %d", dgst, versioned.SchemaVersion) + return nil, distribution.ErrManifestUnknownRevision{ + Name: ms.repository.Named().Name(), + Revision: dgst, + } } func (ms *manifestStore) Put(ctx context.Context, manifest distribution.Manifest, options ...distribution.ManifestServiceOption) (digest.Digest, error) { diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go index 04793194094..aace8b7f321 100644 --- a/registry/storage/manifeststore_test.go +++ b/registry/storage/manifeststore_test.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "io" - "reflect" "testing" "github.com/docker/distribution" @@ -328,7 +327,7 @@ func testManifestStorage(t *testing.T, schema1Enabled bool, options ...RegistryO case distribution.ErrManifestUnknownRevision: break default: - t.Errorf("Unexpected error getting deleted manifest: %s", reflect.ValueOf(err).Type()) + t.Errorf("Unexpected error getting deleted manifest: %T", err) } if deletedManifest != nil { @@ -534,6 +533,92 @@ func testOCIManifestStorage(t *testing.T, testname string, includeMediaTypes boo } } +// TestManifestGetNonManifestContent ensures content that is readable as a blob +// but is not a recognizable manifest reports ErrManifestUnknownRevision, which +// the API maps to 404, rather than an untyped error that maps to 500. +func TestManifestGetNonManifestContent(t *testing.T) { + for _, testcase := range []struct { + name string + content []byte + }{ + { + name: "image config blob carries no schema version", + content: []byte(`{"architecture":"amd64","os":"linux","rootfs":{"type":"layers","diff_ids":[]}}`), + }, + { + name: "schema version 2 with an unregistered media type", + content: []byte(`{"schemaVersion":2,"mediaType":"application/vnd.example.not-a-manifest.v1+json"}`), + }, + { + name: "content is not JSON at all", + content: []byte("this is not a manifest"), + }, + { + name: "no media type and no config descriptor", + content: []byte(`{"schemaVersion":2}`), + }, + } { + t.Run(testcase.name, func(t *testing.T) { + repoName, _ := reference.WithName("foo/bar") + env := newManifestStoreTestEnv(t, repoName, "thetag") + + ctx := context.Background() + manifestService, err := env.repository.Manifests(ctx) + if err != nil { + t.Fatal(err) + } + + descriptor, err := env.repository.Blobs(ctx).Put(ctx, "", testcase.content) + if err != nil { + t.Fatalf("unexpected error putting content as a blob: %v", err) + } + + _, err = manifestService.Get(ctx, descriptor.Digest) + if err == nil { + t.Fatal("expected an error fetching non-manifest content as a manifest") + } + + if _, ok := err.(distribution.ErrManifestUnknownRevision); !ok { + t.Fatalf("expected ErrManifestUnknownRevision, got %T: %v", err, err) + } + }) + } +} + +// TestManifestGetMalformedIndexWithoutMediaType covers content that reaches the +// empty-media-type branch and fails image index unmarshalling. The index +// handler returns a nil manifest alongside its error, so Get must check that +// error before type-asserting the result. The image manifest fallback then +// deserializes the same content into a manifest with no config descriptor, +// which must be reported as unknown rather than served. +func TestManifestGetMalformedIndexWithoutMediaType(t *testing.T) { + repoName, _ := reference.WithName("foo/bar") + env := newManifestStoreTestEnv(t, repoName, "thetag") + + ctx := context.Background() + manifestService, err := env.repository.Manifests(ctx) + if err != nil { + t.Fatal(err) + } + + content := []byte(`{"schemaVersion":2,"manifests":"not-a-list"}`) + + descriptor, err := env.repository.Blobs(ctx).Put(ctx, "", content) + if err != nil { + t.Fatalf("unexpected error putting content as a blob: %v", err) + } + + // This call panics against the pre-fix code. + fetched, err := manifestService.Get(ctx, descriptor.Digest) + if err == nil { + t.Fatalf("expected an error fetching malformed content as a manifest, got %T", fetched) + } + + if _, ok := err.(distribution.ErrManifestUnknownRevision); !ok { + t.Fatalf("expected ErrManifestUnknownRevision, got %T: %v", err, err) + } +} + // TestLinkPathFuncs ensures that the link path functions behavior are locked // down and implemented as expected. func TestLinkPathFuncs(t *testing.T) {