From 98f5f30e75338dcb4cd1c789d864469962da7aa3 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 1 Dec 2014 15:57:05 -0800 Subject: [PATCH 1/5] Create copy of buffer for SignedManifest.Raw Without this copy, the buffer may be re-used in the json package, causing missing or corrupted content for the long-lived SignedManifest object. By creating a new buffer, owned by the SignedManifest object, the content remains stable. --- storage/manifest.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/storage/manifest.go b/storage/manifest.go index 8b288625..895a112e 100644 --- a/storage/manifest.go +++ b/storage/manifest.go @@ -174,7 +174,8 @@ 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 } From b73a6c19980a2bdbcfcbac8d8af957dd7be6c764 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 1 Dec 2014 16:11:27 -0800 Subject: [PATCH 2/5] Use json.MashalIndent for raw manifest json This provides compatibility with what is in docker core, ensuring that image manifests generated here have the same formatting. We'll need to automate this some how. --- storage/manifest.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/storage/manifest.go b/storage/manifest.go index 895a112e..000bc617 100644 --- a/storage/manifest.go +++ b/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 } @@ -148,6 +150,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 } From 8c7bec72b1e4b3f724e862b538e3632e1fad0af9 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 1 Dec 2014 16:13:01 -0800 Subject: [PATCH 3/5] Cleanup image verification error handling This diff removes a few early outs that caused errors to be unreported and catches a missed error case for signature verification from libtrust. More work needs to be done around ensuring consistent error handling but this is enough to make the API work correctly. --- storage/manifeststore.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/storage/manifeststore.go b/storage/manifeststore.go index 707311b8..e1760dd8 100644 --- a/storage/manifeststore.go +++ b/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,7 +129,11 @@ func (ms *manifestStore) verifyManifest(name, tag string, manifest *SignedManife case libtrust.ErrMissingSignatureKey, libtrust.ErrInvalidJSONContent, libtrust.ErrMissingSignatureKey: errs = append(errs, ErrManifestUnverified{}) default: - errs = append(errs, err) + if err.Error() == "invalid signature" { // TODO(stevvooe): This should be exported by libtrust + errs = append(errs, ErrManifestUnverified{}) + } else { + errs = append(errs, err) + } } } From e6e021906584398848ac73025e0d456577f98437 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 1 Dec 2014 17:10:33 -0800 Subject: [PATCH 4/5] Avoid manifest verification errors by using Raw Because json.Marshal does compaction on returned results, applications must directly use SignedManifest.Raw when the marshaled value is required. Otherwise, the returned manifest will fail signature checks. --- api_test.go | 14 ++++++++++---- storage/manifest.go | 9 ++++++--- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/api_test.go b/api_test.go index cc27e5b0..cce4a251 100644 --- a/api_test.go +++ b/api_test.go @@ -277,7 +277,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,9 +299,15 @@ func TestManifestAPI(t *testing.T) { } func putManifest(t *testing.T, msg, url string, v interface{}) *http.Response { - body, err := json.Marshal(v) - if err != nil { - t.Fatalf("unexpected error marshaling %v: %v", v, err) + 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)) diff --git a/storage/manifest.go b/storage/manifest.go index 000bc617..daeaa39b 100644 --- a/storage/manifest.go +++ b/storage/manifest.go @@ -140,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:"-"` } @@ -184,7 +185,9 @@ func (sm *SignedManifest) UnmarshalJSON(b []byte) error { } // 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 From 17b32e0aa0f944d4a4e62ed8697859f8c3a7be6f Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 1 Dec 2014 17:40:14 -0800 Subject: [PATCH 5/5] Add TODO about manifest tampering test --- api_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api_test.go b/api_test.go index cce4a251..41f3de69 100644 --- a/api_test.go +++ b/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)