Correct unmarshal order for SignedManifest

To ensure that we only unmarshal the verified payload into the contained
manifest, we first copy the entire incoming buffer into Raw and then unmarshal
only the Payload portion of the incoming bytes. If the contents is later
verified, the caller can then be sure that the contents of the Manifest fields
can be trusted.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

manifest/schema1/manifest.go

@@ -62,15 +62,20 @@ type SignedManifest struct {
 
 // UnmarshalJSON populates a new ImageManifest struct from JSON data.
 func (sm *SignedManifest) UnmarshalJSON(b []byte) error {
+	sm.Raw = make([]byte, len(b), len(b))
+	copy(sm.Raw, b)
+
+	p, err := sm.Payload()
+	if err != nil {
+		return err
+	}
+
 	var manifest Manifest
-	if err := json.Unmarshal(b, &manifest); err != nil {
+	if err := json.Unmarshal(p, &manifest); err != nil {
 		return err
 	}
 
 	sm.Manifest = manifest
-	sm.Raw = make([]byte, len(b), len(b))
-	copy(sm.Raw, b)
-
 	return nil
 }
 

registry/client/repository_test.go

@@ -3,7 +3,6 @@ package client
 import (
 	"bytes"
 	"crypto/rand"
-	"encoding/json"
 	"fmt"
 	"io"
 	"log"
@@ -14,8 +13,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/docker/distribution/uuid"
-
 	"github.com/docker/distribution"
 	"github.com/docker/distribution/context"
 	"github.com/docker/distribution/digest"
@@ -23,6 +20,8 @@ import (
 	"github.com/docker/distribution/manifest/schema1"
 	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/docker/distribution/testutil"
+	"github.com/docker/distribution/uuid"
+	"github.com/docker/libtrust"
 )
 
 func testServer(rrm testutil.RequestResponseMap) (string, func()) {
@@ -420,7 +419,7 @@ func TestBlobUploadMonolithic(t *testing.T) {
 	}
 }
 
-func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.SignedManifest, digest.Digest) {
+func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.SignedManifest, digest.Digest, []byte) {
 	blobs := make([]schema1.FSLayer, blobCount)
 	history := make([]schema1.History, blobCount)
 
@@ -431,30 +430,38 @@ func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.Signed
 		history[i] = schema1.History{V1Compatibility: fmt.Sprintf("{\"Hex\": \"%x\"}", blob)}
 	}
 
-	m := &schema1.SignedManifest{
-		Manifest: schema1.Manifest{
-			Name:         name,
-			Tag:          tag,
-			Architecture: "x86",
-			FSLayers:     blobs,
-			History:      history,
-			Versioned: manifest.Versioned{
-				SchemaVersion: 1,
-			},
+	m := schema1.Manifest{
+		Name:         name,
+		Tag:          tag,
+		Architecture: "x86",
+		FSLayers:     blobs,
+		History:      history,
+		Versioned: manifest.Versioned{
+			SchemaVersion: 1,
 		},
 	}
-	manifestBytes, err := json.Marshal(m)
-	if err != nil {
-		panic(err)
-	}
-	dgst, err := digest.FromBytes(manifestBytes)
+
+	pk, err := libtrust.GenerateECP256PrivateKey()
 	if err != nil {
 		panic(err)
 	}
 
-	m.Raw = manifestBytes
+	sm, err := schema1.Sign(&m, pk)
+	if err != nil {
+		panic(err)
+	}
 
-	return m, dgst
+	p, err := sm.Payload()
+	if err != nil {
+		panic(err)
+	}
+
+	dgst, err := digest.FromBytes(p)
+	if err != nil {
+		panic(err)
+	}
+
+	return sm, dgst, p
 }
 
 func addTestManifestWithEtag(repo, reference string, content []byte, m *testutil.RequestResponseMap, dgst string) {
@@ -551,7 +558,7 @@ func checkEqualManifest(m1, m2 *schema1.SignedManifest) error {
 func TestManifestFetch(t *testing.T) {
 	ctx := context.Background()
 	repo := "test.example.com/repo"
-	m1, dgst := newRandomSchemaV1Manifest(repo, "latest", 6)
+	m1, dgst, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
 	addTestManifest(repo, dgst.String(), m1.Raw, &m)
 
@@ -586,9 +593,9 @@ func TestManifestFetch(t *testing.T) {
 
 func TestManifestFetchWithEtag(t *testing.T) {
 	repo := "test.example.com/repo/by/tag"
-	m1, d1 := newRandomSchemaV1Manifest(repo, "latest", 6)
+	_, d1, p1 := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
-	addTestManifestWithEtag(repo, "latest", m1.Raw, &m, d1.String())
+	addTestManifestWithEtag(repo, "latest", p1, &m, d1.String())
 
 	e, c := testServer(m)
 	defer c()
@@ -611,8 +618,8 @@ func TestManifestFetchWithEtag(t *testing.T) {
 
 func TestManifestDelete(t *testing.T) {
 	repo := "test.example.com/repo/delete"
-	_, dgst1 := newRandomSchemaV1Manifest(repo, "latest", 6)
-	_, dgst2 := newRandomSchemaV1Manifest(repo, "latest", 6)
+	_, dgst1, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
+	_, dgst2, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
 	m = append(m, testutil.RequestResponseMapping{
 		Request: testutil.Request{
@@ -651,7 +658,7 @@ func TestManifestDelete(t *testing.T) {
 
 func TestManifestPut(t *testing.T) {
 	repo := "test.example.com/repo/delete"
-	m1, dgst := newRandomSchemaV1Manifest(repo, "other", 6)
+	m1, dgst, _ := newRandomSchemaV1Manifest(repo, "other", 6)
 	var m testutil.RequestResponseMap
 	m = append(m, testutil.RequestResponseMapping{
 		Request: testutil.Request{
@@ -744,7 +751,7 @@ func TestManifestTags(t *testing.T) {
 
 func TestManifestUnauthorized(t *testing.T) {
 	repo := "test.example.com/repo"
-	_, dgst := newRandomSchemaV1Manifest(repo, "latest", 6)
+	_, dgst, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
 
 	m = append(m, testutil.RequestResponseMapping{

registry/handlers/api_test.go

@@ -760,14 +760,32 @@ func testManifestAPI(t *testing.T, env *testEnv, args manifestArgs) (*testEnv, m
 
 	resp = putManifest(t, "putting unsigned manifest", manifestURL, unsignedManifest)
 	defer resp.Body.Close()
-	checkResponse(t, "posting unsigned manifest", resp, http.StatusBadRequest)
-	_, p, counts := checkBodyHasErrorCodes(t, "getting unknown manifest tags", resp,
-		v2.ErrorCodeManifestUnverified, v2.ErrorCodeBlobUnknown, v2.ErrorCodeDigestInvalid)
+	checkResponse(t, "putting unsigned manifest", resp, http.StatusBadRequest)
+	_, p, counts := checkBodyHasErrorCodes(t, "getting unknown manifest tags", resp, v2.ErrorCodeManifestInvalid)
 
 	expectedCounts := map[errcode.ErrorCode]int{
-		v2.ErrorCodeManifestUnverified: 1,
-		v2.ErrorCodeBlobUnknown:        2,
-		v2.ErrorCodeDigestInvalid:      2,
+		v2.ErrorCodeManifestInvalid: 1,
+	}
+
+	if !reflect.DeepEqual(counts, expectedCounts) {
+		t.Fatalf("unexpected number of error codes encountered: %v\n!=\n%v\n---\n%s", counts, expectedCounts, string(p))
+	}
+
+	// sign the manifest and still get some interesting errors.
+	sm, err := schema1.Sign(unsignedManifest, env.pk)
+	if err != nil {
+		t.Fatalf("error signing manifest: %v", err)
+	}
+
+	resp = putManifest(t, "putting signed manifest with errors", manifestURL, sm)
+	defer resp.Body.Close()
+	checkResponse(t, "putting signed manifest with errors", resp, http.StatusBadRequest)
+	_, p, counts = checkBodyHasErrorCodes(t, "putting signed manifest with errors", resp,
+		v2.ErrorCodeManifestBlobUnknown, v2.ErrorCodeDigestInvalid)
+
+	expectedCounts = map[errcode.ErrorCode]int{
+		v2.ErrorCodeManifestBlobUnknown: 2,
+		v2.ErrorCodeDigestInvalid:       2,
 	}
 
 	if !reflect.DeepEqual(counts, expectedCounts) {
@@ -1426,7 +1444,7 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) {
 	}
 
 	// Manifest upload
-	unsignedManifest := &schema1.Manifest{
+	m := &schema1.Manifest{
 		Versioned: manifest.Versioned{
 			SchemaVersion: 1,
 		},
@@ -1434,7 +1452,13 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) {
 		Tag:      tag,
 		FSLayers: []schema1.FSLayer{},
 	}
-	resp := putManifest(t, "putting unsigned manifest", manifestURL, unsignedManifest)
+
+	sm, err := schema1.Sign(m, env.pk)
+	if err != nil {
+		t.Fatalf("error signing manifest: %v", err)
+	}
+
+	resp := putManifest(t, "putting unsigned manifest", manifestURL, sm)
 	checkResponse(t, "putting signed manifest to cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode)
 
 	// Manifest Delete

registry/handlers/images.go

@@ -163,7 +163,7 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http
 			for _, verificationError := range err {
 				switch verificationError := verificationError.(type) {
 				case distribution.ErrManifestBlobUnknown:
-					imh.Errors = append(imh.Errors, v2.ErrorCodeBlobUnknown.WithDetail(verificationError.Digest))
+					imh.Errors = append(imh.Errors, v2.ErrorCodeManifestBlobUnknown.WithDetail(verificationError.Digest))
 				case distribution.ErrManifestUnverified:
 					imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnverified)
 				default: