From 0ab6b1d9221f7a2a65c6565fed8f3d6f29fcec2d Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Tue, 17 Nov 2015 10:39:51 -0800 Subject: [PATCH] Correct parent chain in v2 push when v1Compatibility files on the disk are inconsistent This fixes an issue where two images with the same filesystem contents and configuration but different remote IDs could share a v1Compatibility file, resulting in corrupted manifests. Signed-off-by: Aaron Lehmann --- graph/push_v2.go | 91 +++++++++++++++++++++++++ integration-cli/docker_cli_push_test.go | 43 ++++++++++++ 2 files changed, 134 insertions(+) diff --git a/graph/push_v2.go b/graph/push_v2.go index 0f7c1103f6..1b9cb443f5 100644 --- a/graph/push_v2.go +++ b/graph/push_v2.go @@ -3,6 +3,8 @@ package graph import ( "bufio" "compress/gzip" + "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -205,6 +207,11 @@ func (p *v2Pusher) pushV2Tag(tag string) error { p.layersPushed[dgst] = true } + // Fix parent chain if necessary + if err = fixHistory(m); err != nil { + return err + } + logrus.Infof("Signed manifest for %s:%s using daemon's key: %s", p.repo.Name(), tag, p.trustKey.KeyID()) signed, err := schema1.Sign(m, p.trustKey) if err != nil { @@ -226,6 +233,90 @@ func (p *v2Pusher) pushV2Tag(tag string) error { return manSvc.Put(signed) } +// fixHistory makes sure that the manifest has parent IDs that are consistent +// with its image IDs. Because local image IDs are generated from the +// configuration and filesystem contents, but IDs in the manifest are preserved +// from the original pull, it's possible to have inconsistencies where parent +// IDs don't match up with the other IDs in the manifest. This happens in the +// case where an engine pulls images where are identical except the IDs from the +// manifest - the local ID will be the same, and one of the v1Compatibility +// files gets discarded. +func fixHistory(m *schema1.Manifest) error { + var lastID string + + for i := len(m.History) - 1; i >= 0; i-- { + var historyEntry map[string]*json.RawMessage + if err := json.Unmarshal([]byte(m.History[i].V1Compatibility), &historyEntry); err != nil { + return err + } + + idJSON, present := historyEntry["id"] + if !present || idJSON == nil { + return errors.New("missing id key in v1compatibility file") + } + var id string + if err := json.Unmarshal(*idJSON, &id); err != nil { + return err + } + + parentJSON, present := historyEntry["parent"] + + if i == len(m.History)-1 { + // The base layer must not reference a parent layer, + // otherwise the manifest is incomplete. There is an + // exception for Windows to handle base layers. + if !allowBaseParentImage && present && parentJSON != nil { + var parent string + if err := json.Unmarshal(*parentJSON, &parent); err != nil { + return err + } + if parent != "" { + logrus.Debugf("parent id mismatch detected; fixing. parent reference: %s", parent) + delete(historyEntry, "parent") + fixedHistory, err := json.Marshal(historyEntry) + if err != nil { + return err + } + m.History[i].V1Compatibility = string(fixedHistory) + } + } + } else { + // For all other layers, the parent ID should equal the + // ID of the next item in the history list. If it + // doesn't, fix it up (but preserve all other fields, + // possibly including fields that aren't known to this + // engine version). + if !present || parentJSON == nil { + return errors.New("missing parent key in v1compatibility file") + } + var parent string + if err := json.Unmarshal(*parentJSON, &parent); err != nil { + return err + } + if parent != lastID { + logrus.Debugf("parent id mismatch detected; fixing. parent reference: %s actual id: %s", parent, id) + historyEntry["parent"] = rawJSON(lastID) + fixedHistory, err := json.Marshal(historyEntry) + if err != nil { + return err + } + m.History[i].V1Compatibility = string(fixedHistory) + } + } + lastID = id + } + + return nil +} + +func rawJSON(value interface{}) *json.RawMessage { + jsonval, err := json.Marshal(value) + if err != nil { + return nil + } + return (*json.RawMessage)(&jsonval) +} + func (p *v2Pusher) pushV2Image(bs distribution.BlobService, img *image.Image) (digest.Digest, error) { out := p.config.OutStream diff --git a/integration-cli/docker_cli_push_test.go b/integration-cli/docker_cli_push_test.go index 71ebed7d84..7c4e5c9428 100644 --- a/integration-cli/docker_cli_push_test.go +++ b/integration-cli/docker_cli_push_test.go @@ -2,13 +2,16 @@ package main import ( "archive/tar" + "encoding/json" "fmt" "io/ioutil" "os" "os/exec" + "path/filepath" "strings" "time" + "github.com/docker/docker/image" "github.com/docker/docker/pkg/integration/checker" "github.com/go-check/check" ) @@ -83,6 +86,46 @@ func (s *DockerRegistrySuite) TestPushMultipleTags(c *check.C) { } } +// TestPushBadParentChain tries to push an image with a corrupted parent chain +// in the v1compatibility files, and makes sure the push process fixes it. +func (s *DockerRegistrySuite) TestPushBadParentChain(c *check.C) { + repoName := fmt.Sprintf("%v/dockercli/badparent", privateRegistryURL) + + id, err := buildImage(repoName, ` + FROM busybox + CMD echo "adding another layer" + `, true) + if err != nil { + c.Fatal(err) + } + + // Push to create v1compatibility file + dockerCmd(c, "push", repoName) + + // Corrupt the parent in the v1compatibility file from the top layer + filename := filepath.Join(dockerBasePath, "graph", id, "v1Compatibility") + + jsonBytes, err := ioutil.ReadFile(filename) + c.Assert(err, check.IsNil, check.Commentf("Could not read v1Compatibility file: %s", err)) + + var img image.Image + err = json.Unmarshal(jsonBytes, &img) + c.Assert(err, check.IsNil, check.Commentf("Could not unmarshal json: %s", err)) + + img.Parent = "1234123412341234123412341234123412341234123412341234123412341234" + + jsonBytes, err = json.Marshal(&img) + c.Assert(err, check.IsNil, check.Commentf("Could not marshal json: %s", err)) + + err = ioutil.WriteFile(filename, jsonBytes, 0600) + c.Assert(err, check.IsNil, check.Commentf("Could not write v1Compatibility file: %s", err)) + + dockerCmd(c, "push", repoName) + + // pull should succeed + dockerCmd(c, "pull", repoName) +} + func (s *DockerRegistrySuite) TestPushEmptyLayer(c *check.C) { repoName := fmt.Sprintf("%v/dockercli/emptylayer", privateRegistryURL) emptyTarball, err := ioutil.TempFile("", "empty_tarball")