Merge pull request #1094 from aaronlehmann/correct-payload-serialization-order

Correct unmarshal order for SignedManifest

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{
+	m := schema1.Manifest{
-		Manifest: schema1.Manifest{
+		Name:         name,
-			Name:         name,
+		Tag:          tag,
-			Tag:          tag,
+		Architecture: "x86",
-			Architecture: "x86",
+		FSLayers:     blobs,
-			FSLayers:     blobs,
+		History:      history,
-			History:      history,
+		Versioned: manifest.Versioned{
-			Versioned: manifest.Versioned{
+			SchemaVersion: 1,
-				SchemaVersion: 1,
-			},
 		},
 	}
-	manifestBytes, err := json.Marshal(m)
+
-	if err != nil {
+	pk, err := libtrust.GenerateECP256PrivateKey()
-		panic(err)
-	}
-	dgst, err := digest.FromBytes(manifestBytes)
 	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)
+	_, dgst1, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
-	_, dgst2 := 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)
+	checkResponse(t, "putting unsigned manifest", resp, http.StatusBadRequest)
-	_, p, counts := checkBodyHasErrorCodes(t, "getting unknown manifest tags", resp,
+	_, p, counts := checkBodyHasErrorCodes(t, "getting unknown manifest tags", resp, v2.ErrorCodeManifestInvalid)
-		v2.ErrorCodeManifestUnverified, v2.ErrorCodeBlobUnknown, v2.ErrorCodeDigestInvalid)
 
 	expectedCounts := map[errcode.ErrorCode]int{
-		v2.ErrorCodeManifestUnverified: 1,
+		v2.ErrorCodeManifestInvalid: 1,
-		v2.ErrorCodeBlobUnknown:        2,
+	}
-		v2.ErrorCodeDigestInvalid:      2,
+
+	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: