Merge pull request #805 from stevvooe/ng-manifest-bugs

NG: Various Manifest Oriented bugfixes

api_test.go

@@ -250,6 +250,10 @@ func TestManifestAPI(t *testing.T) {
 		t.Fatalf("should have received two invalid digest errors: %v", respErrs)
 	}
 
+	// TODO(stevvooe): Add a test case where we take a mostly valid registry,
+	// tamper with the content and ensure that we get a unverified manifest
+	// error.
+
 	// Push 2 random layers
 	expectedLayers := make(map[digest.Digest]io.ReadSeeker)
 
@@ -277,7 +281,7 @@ func TestManifestAPI(t *testing.T) {
 
 	resp = putManifest(t, "putting signed manifest", manifestURL, signedManifest)
 
-	checkResponse(t, "putting manifest", resp, http.StatusOK)
+	checkResponse(t, "putting signed manifest", resp, http.StatusOK)
 
 	resp, err = http.Get(manifestURL)
 	if err != nil {
@@ -299,10 +303,16 @@ func TestManifestAPI(t *testing.T) {
 }
 
 func putManifest(t *testing.T, msg, url string, v interface{}) *http.Response {
-	body, err := json.Marshal(v)
+	var body []byte
+	if sm, ok := v.(*storage.SignedManifest); ok {
+		body = sm.Raw
+	} else {
+		var err error
+		body, err = json.MarshalIndent(v, "", "   ")
 		if err != nil {
 			t.Fatalf("unexpected error marshaling %v: %v", v, err)
 		}
+	}
 
 	req, err := http.NewRequest("PUT", url, bytes.NewReader(body))
 	if err != nil {

storage/manifest.go

@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/Sirupsen/logrus"
+
 	"github.com/docker/libtrust"
 
 	"github.com/docker/docker-registry/digest"
@@ -78,7 +80,7 @@ type Manifest struct {
 // SignedManifest. This typically won't be used within the registry, except
 // for testing.
 func (m *Manifest) Sign(pk libtrust.PrivateKey) (*SignedManifest, error) {
-	p, err := json.Marshal(m)
+	p, err := json.MarshalIndent(m, "", "   ")
 	if err != nil {
 		return nil, err
 	}
@@ -107,7 +109,7 @@ func (m *Manifest) Sign(pk libtrust.PrivateKey) (*SignedManifest, error) {
 // The public key of the first element in the chain must be the public key
 // corresponding with the sign key.
 func (m *Manifest) SignWithChain(key libtrust.PrivateKey, chain []*x509.Certificate) (*SignedManifest, error) {
-	p, err := json.Marshal(m)
+	p, err := json.MarshalIndent(m, "", "   ")
 	if err != nil {
 		return nil, err
 	}
@@ -138,8 +140,9 @@ type SignedManifest struct {
 	Manifest
 
 	// Raw is the byte representation of the ImageManifest, used for signature
-	// verification. The manifest byte representation cannot change or it will
-	// have to be re-signed.
+	// verification. The value of Raw must be used directly during
+	// serialization, or the signature check will fail. The manifest byte
+	// representation cannot change or it will have to be re-signed.
 	Raw []byte `json:"-"`
 }
 
@@ -148,6 +151,7 @@ type SignedManifest struct {
 func (sm *SignedManifest) Verify() ([]libtrust.PublicKey, error) {
 	js, err := libtrust.ParsePrettySignature(sm.Raw, "signatures")
 	if err != nil {
+		logrus.WithField("err", err).Debugf("(*SignedManifest).Verify")
 		return nil, err
 	}
 
@@ -174,13 +178,16 @@ func (sm *SignedManifest) UnmarshalJSON(b []byte) error {
 	}
 
 	sm.Manifest = manifest
-	sm.Raw = b
+	sm.Raw = make([]byte, len(b), len(b))
+	copy(sm.Raw, b)
 
 	return nil
 }
 
 // MarshalJSON returns the contents of raw. If Raw is nil, marshals the inner
-// contents.
+// contents. Applications requiring a marshaled signed manifest should simply
+// use Raw directly, since the the content produced by json.Marshal will
+// compacted and will fail signature checks.
 func (sm *SignedManifest) MarshalJSON() ([]byte, error) {
 	if len(sm.Raw) > 0 {
 		return sm.Raw, nil

storage/manifeststore.go

@@ -111,11 +111,13 @@ func (ms *manifestStore) verifyManifest(name, tag string, manifest *SignedManife
 
 	var errs ErrManifestVerification
 	if manifest.Name != name {
-		return fmt.Errorf("name does not match manifest name")
+		// TODO(stevvooe): This needs to be an exported error
+		errs = append(errs, fmt.Errorf("name does not match manifest name"))
 	}
 
 	if manifest.Tag != tag {
-		return fmt.Errorf("tag does not match manifest tag")
+		// TODO(stevvooe): This needs to be an exported error.
+		errs = append(errs, fmt.Errorf("tag does not match manifest tag"))
 	}
 
 	// TODO(stevvooe): These pubkeys need to be checked with either Verify or
@@ -127,9 +129,13 @@ func (ms *manifestStore) verifyManifest(name, tag string, manifest *SignedManife
 		case libtrust.ErrMissingSignatureKey, libtrust.ErrInvalidJSONContent, libtrust.ErrMissingSignatureKey:
 			errs = append(errs, ErrManifestUnverified{})
 		default:
+			if err.Error() == "invalid signature" { // TODO(stevvooe): This should be exported by libtrust
+				errs = append(errs, ErrManifestUnverified{})
+			} else {
 				errs = append(errs, err)
 			}
 		}
+	}
 
 	for _, fsLayer := range manifest.FSLayers {
 		exists, err := ms.layerService.Exists(name, fsLayer.BlobSum)