From e57c13f3cb1ecaf23cc0581bc10857ff2f6b7494 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 12 Aug 2015 19:37:43 -0700 Subject: [PATCH] Validate manifest parent-child relationships Since 1.8 may push bad manifests, we've added some validation to ensure that the parent-child relationships represented by image json are correct. If the relationship is not correct, we reject the push. Signed-off-by: Stephen J Day --- errors.go | 10 ++++ notifications/listener_test.go | 38 +++++++++++++ registry/handlers/api_test.go | 79 +++++++++++++++++++++++++- registry/handlers/images.go | 2 + registry/storage/manifeststore.go | 43 ++++++++++++++ registry/storage/manifeststore_test.go | 71 ++++++++++++++++++++++- 6 files changed, 238 insertions(+), 5 deletions(-) diff --git a/errors.go b/errors.go index 7883b9f7..d04a0a0c 100644 --- a/errors.go +++ b/errors.go @@ -74,6 +74,16 @@ func (ErrManifestUnverified) Error() string { return fmt.Sprintf("unverified manifest") } +// ErrManifestValidation is returned during manifest verification if a common +// validation error is encountered. +type ErrManifestValidation struct { + Reason string +} + +func (err ErrManifestValidation) Error() string { + return fmt.Sprintf("invalid manifest: %s", err.Reason) +} + // ErrManifestVerification provides a type to collect errors encountered // during manifest verification. Currently, it accepts errors of all types, // but it may be narrowed to those involving manifest verification. diff --git a/notifications/listener_test.go b/notifications/listener_test.go index 956279a2..ceb73a66 100644 --- a/notifications/listener_test.go +++ b/notifications/listener_test.go @@ -1,10 +1,12 @@ package notifications import ( + "encoding/json" "io" "reflect" "testing" + "code.google.com/p/go-uuid/uuid" "github.com/docker/distribution" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" @@ -132,6 +134,8 @@ func checkExerciseRepository(t *testing.T, repository distribution.Repository) { } } + m.History = generateHistory(t, len(m.FSLayers)) + pk, err := libtrust.GenerateECP256PrivateKey() if err != nil { t.Fatalf("unexpected error generating key: %v", err) @@ -176,3 +180,37 @@ func checkExerciseRepository(t *testing.T, repository distribution.Repository) { t.Fatalf("retrieved unexpected manifest: %v", err) } } + +// generateHistory creates a valid history entry of length n. +func generateHistory(t *testing.T, n int) []manifest.History { + var images []map[string]interface{} + + // first pass: create images entries. + for i := 0; i < n; i++ { + // simulate correct id -> parent links in v1Compatibility, using uuids. + image := map[string]interface{}{ + "id": uuid.New(), + } + + images = append(images, image) + } + + var history []manifest.History + + for i, image := range images { + if i+1 < len(images) { + image["parent"] = images[i+1]["id"] + } + + p, err := json.Marshal(image) + if err != nil { + t.Fatalf("error generating image json: %v", err) + } + + history = append(history, manifest.History{ + V1Compatibility: string(p), + }) + } + + return history +} diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index 1e31477f..7ec19020 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -16,6 +16,7 @@ import ( "strings" "testing" + "code.google.com/p/go-uuid/uuid" "github.com/docker/distribution/configuration" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" @@ -323,10 +324,12 @@ func TestManifestAPI(t *testing.T) { }, } + unsignedManifest.History = generateHistory(t, len(unsignedManifest.FSLayers), false) + 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, + _, p, counts := checkBodyHasErrorCodes(t, "putting unsigned manifest", resp, v2.ErrorCodeManifestUnverified, v2.ErrorCodeBlobUnknown, v2.ErrorCodeDigestInvalid) expectedCounts := map[v2.ErrorCode]int{ @@ -361,8 +364,10 @@ func TestManifestAPI(t *testing.T) { pushLayer(t, env.builder, imageName, dgst, uploadURLBase, rs) } - // ------------------- - // Push the signed manifest with all layers pushed. + // ----------------------- + // mostly valid, but we have an extra parent point in history. + unsignedManifest.History = generateHistory(t, len(unsignedManifest.FSLayers), true) + signedManifest, err := manifest.Sign(unsignedManifest, env.pk) if err != nil { t.Fatalf("unexpected error signing manifest: %v", err) @@ -377,6 +382,36 @@ func TestManifestAPI(t *testing.T) { manifestDigestURL, err := env.builder.BuildManifestURL(imageName, dgst.String()) checkErr(t, err, "building manifest url") + resp = putManifest(t, "putting signed manifest with bad parent", manifestURL, signedManifest) + checkResponse(t, "putting signed manifest with bad parent", resp, http.StatusBadRequest) + _, p, counts = checkBodyHasErrorCodes(t, "putting unsigned manifest with bad parent", resp, v2.ErrorCodeManifestInvalid) + + expectedCounts = map[v2.ErrorCode]int{ + 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)) + } + + // ------------------- + // Push the signed manifest with all layers pushed. + unsignedManifest.History = generateHistory(t, len(unsignedManifest.FSLayers), false) + + signedManifest, err = manifest.Sign(unsignedManifest, env.pk) + if err != nil { + t.Fatalf("unexpected error signing manifest: %v", err) + } + + payload, err = signedManifest.Payload() + checkErr(t, err, "getting manifest payload") + + dgst, err = digest.FromBytes(payload) + checkErr(t, err, "digesting manifest") + + manifestDigestURL, err = env.builder.BuildManifestURL(imageName, dgst.String()) + checkErr(t, err, "building manifest url") + resp = putManifest(t, "putting signed manifest", manifestURL, signedManifest) checkResponse(t, "putting signed manifest", resp, http.StatusAccepted) checkHeaders(t, resp, http.Header{ @@ -791,3 +826,41 @@ func checkErr(t *testing.T, err error, msg string) { t.Fatalf("unexpected error %s: %v", msg, err) } } + +// generateHistory creates a valid history entry of length n. +func generateHistory(t *testing.T, n int, extraParent bool) []manifest.History { + var images []map[string]interface{} + + // first pass: create images entries. + for i := 0; i < n; i++ { + // simulate correct id -> parent links in v1Compatibility, using uuids. + image := map[string]interface{}{ + "id": uuid.New(), + } + + images = append(images, image) + } + + var history []manifest.History + + for i, image := range images { + if i+1 < len(images) { + image["parent"] = images[i+1]["id"] + } + + if extraParent && i == len(images)-1 { + image["parent"] = uuid.New() + } + + p, err := json.Marshal(image) + if err != nil { + t.Fatalf("error generating image json: %v", err) + } + + history = append(history, manifest.History{ + V1Compatibility: string(p), + }) + } + + return history +} diff --git a/registry/handlers/images.go b/registry/handlers/images.go index 174bd3d9..a74fe512 100644 --- a/registry/handlers/images.go +++ b/registry/handlers/images.go @@ -140,6 +140,8 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http imh.Errors.Push(v2.ErrorCodeBlobUnknown, verificationError.FSLayer) case distribution.ErrManifestUnverified: imh.Errors.Push(v2.ErrorCodeManifestUnverified) + case distribution.ErrManifestValidation: + imh.Errors.Push(v2.ErrorCodeManifestInvalid, verificationError.Error()) default: if verificationError == digest.ErrDigestInvalidFormat { // TODO(stevvooe): We need to really need to move all diff --git a/registry/storage/manifeststore.go b/registry/storage/manifeststore.go index 4946785d..e1ae4a67 100644 --- a/registry/storage/manifeststore.go +++ b/registry/storage/manifeststore.go @@ -1,6 +1,7 @@ package storage import ( + "encoding/json" "fmt" "github.com/docker/distribution" @@ -102,6 +103,48 @@ func (ms *manifestStore) verifyManifest(mnfst *manifest.SignedManifest) error { } } + if len(mnfst.FSLayers) == 0 || len(mnfst.History) == 0 { + errs = append(errs, distribution.ErrManifestValidation{ + Reason: "no layers present"}) + } + + if len(mnfst.FSLayers) != len(mnfst.History) { + errs = append(errs, distribution.ErrManifestValidation{ + Reason: "mismatched layers and history"}) + } + + // image provides a local type for validating the image relationship. + type image struct { + ID string `json:"id"` + Parent string `json:"parent"` + } + + // Process the history portion to ensure that the parent links are + // correctly represented. We serialize the image json, then walk the + // entries, checking the parent link. + var images []image + for _, entry := range mnfst.History { + var im image + if err := json.Unmarshal([]byte(entry.V1Compatibility), &im); err != nil { + errs = append(errs, err) + } + + images = append(images, im) + } + + // go back through each image, checking the parent link and rank + var parentID string + for i := len(images) - 1; i >= 0; i-- { + // ensure that the parent id matches but only if there is a parent. + // There are cases where successive layers don't fill in the parents. + if images[i].Parent != parentID { + errs = append(errs, distribution.ErrManifestValidation{ + Reason: "parent not adjacent in manifest"}) + } + + parentID = images[i].ID + } + for _, fsLayer := range mnfst.FSLayers { exists, err := ms.repository.Layers().Exists(fsLayer.BlobSum) if err != nil { diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go index a70789d3..d2f7a2ca 100644 --- a/registry/storage/manifeststore_test.go +++ b/registry/storage/manifeststore_test.go @@ -2,15 +2,16 @@ package storage import ( "bytes" + "encoding/json" "io" "reflect" "testing" - "github.com/docker/distribution/registry/storage/cache" - + "code.google.com/p/go-uuid/uuid" "github.com/docker/distribution" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" + "github.com/docker/distribution/registry/storage/cache" "github.com/docker/distribution/registry/storage/driver" "github.com/docker/distribution/registry/storage/driver/inmemory" "github.com/docker/distribution/testutil" @@ -103,6 +104,38 @@ func TestManifestStorage(t *testing.T) { t.Fatalf("error signing manifest: %v", err) } + // try to put the manifest initially. this will fail since we have not + // included history or pushed any layers. + err = ms.Put(sm) + if err == nil { + t.Fatalf("expected errors putting manifest with full verification") + } + + switch err := err.(type) { + case distribution.ErrManifestVerification: + if len(err) != 4 { + t.Fatalf("expected 4 verification errors: %#v", err) + } + + for _, err := range err { + switch err := err.(type) { + case distribution.ErrUnknownLayer, distribution.ErrManifestValidation: + // noop: we expect these errors + default: + t.Fatalf("unexpected error type: %v", err) + } + } + default: + t.Fatalf("unexpected error verifying manifest: %v", err) + } + + m.History = generateHistory(t, len(m.FSLayers)) + sm, err = manifest.Sign(&m, pk) + if err != nil { + t.Fatalf("unexpected error signing manfiest with history: %v", err) + } + + // we've fixed the missing history, try the push and fail on layer checks. err = ms.Put(sm) if err == nil { t.Fatalf("expected errors putting manifest") @@ -288,3 +321,37 @@ func TestManifestStorage(t *testing.T) { t.Fatalf("unexpected an error deleting manifest by digest: %v", err) } } + +// generateHistory creates a valid history entry of length n. +func generateHistory(t *testing.T, n int) []manifest.History { + var images []map[string]interface{} + + // first pass: create images entries. + for i := 0; i < n; i++ { + // simulate correct id -> parent links in v1Compatibility, using uuids. + image := map[string]interface{}{ + "id": uuid.New(), + } + + images = append(images, image) + } + + var history []manifest.History + + for i, image := range images { + if i+1 < len(images) { + image["parent"] = images[i+1]["id"] + } + + p, err := json.Marshal(image) + if err != nil { + t.Fatalf("error generating image json: %v", err) + } + + history = append(history, manifest.History{ + V1Compatibility: string(p), + }) + } + + return history +}