diff --git a/graph/pull_v2.go b/graph/pull_v2.go index 6c2fa20653..63cac1dbbc 100644 --- a/graph/pull_v2.go +++ b/graph/pull_v2.go @@ -102,13 +102,12 @@ func (p *v2Puller) pullV2Repository(tag string) (err error) { // downloadInfo is used to pass information from download to extractor type downloadInfo struct { - img *image.Image - tmpFile *os.File - digest digest.Digest - layer distribution.ReadSeekCloser - size int64 - err chan error - verified bool + img *image.Image + tmpFile *os.File + digest digest.Digest + layer distribution.ReadSeekCloser + size int64 + err chan error } type errVerification struct{} @@ -176,9 +175,11 @@ func (p *v2Puller) download(di *downloadInfo) { out.Write(p.sf.FormatProgress(stringid.TruncateID(di.img.ID), "Verifying Checksum", nil)) - di.verified = verifier.Verified() - if !di.verified { - logrus.Infof("Image verification failed for layer %s", di.digest) + if !verifier.Verified() { + err = fmt.Errorf("filesystem layer verification failed for digest %s", di.digest) + logrus.Error(err) + di.err <- err + return } out.Write(p.sf.FormatProgress(stringid.TruncateID(di.img.ID), "Download complete", nil)) @@ -252,7 +253,6 @@ func (p *v2Puller) pullV2Tag(tag, taggedName string) (bool, error) { return false, err } } - verified = verified && d.verified if d.layer != nil { // if tmpFile is empty assume download and extracted elsewhere defer os.Remove(d.tmpFile.Name()) @@ -368,6 +368,28 @@ func (p *v2Puller) verifyTrustedKeys(namespace string, keys []libtrust.PublicKey } func (p *v2Puller) validateManifest(m *manifest.SignedManifest, tag string) (verified bool, err error) { + // If pull by digest, then verify the manifest digest. NOTE: It is + // important to do this first, before any other content validation. If the + // digest cannot be verified, don't even bother with those other things. + if manifestDigest, err := digest.ParseDigest(tag); err == nil { + verifier, err := digest.NewDigestVerifier(manifestDigest) + if err != nil { + return false, err + } + payload, err := m.Payload() + if err != nil { + return false, err + } + if _, err := verifier.Write(payload); err != nil { + return false, err + } + if !verifier.Verified() { + err := fmt.Errorf("image verification failed for digest %s", manifestDigest) + logrus.Error(err) + return false, err + } + } + // TODO(tiborvass): what's the usecase for having manifest == nil and err == nil ? Shouldn't be the error be "DoesNotExist" ? if m == nil { return false, fmt.Errorf("image manifest does not exist for tag %q", tag) @@ -389,21 +411,5 @@ func (p *v2Puller) validateManifest(m *manifest.SignedManifest, tag string) (ver if err != nil { return false, fmt.Errorf("error verifying manifest keys: %v", err) } - localDigest, err := digest.ParseDigest(tag) - // if pull by digest, then verify - if err == nil { - verifier, err := digest.NewDigestVerifier(localDigest) - if err != nil { - return false, err - } - payload, err := m.Payload() - if err != nil { - return false, err - } - if _, err := verifier.Write(payload); err != nil { - return false, err - } - verified = verified && verifier.Verified() - } return verified, nil } diff --git a/integration-cli/docker_cli_by_digest_test.go b/integration-cli/docker_cli_by_digest_test.go index 46167634cb..ef04869667 100644 --- a/integration-cli/docker_cli_by_digest_test.go +++ b/integration-cli/docker_cli_by_digest_test.go @@ -1,25 +1,29 @@ package main import ( + "encoding/json" "fmt" "regexp" "strings" + "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" "github.com/docker/docker/utils" "github.com/go-check/check" ) var ( - repoName = fmt.Sprintf("%v/dockercli/busybox-by-dgst", privateRegistryURL) + remoteRepoName = "dockercli/busybox-by-dgst" + repoName = fmt.Sprintf("%v/%s", privateRegistryURL, remoteRepoName) pushDigestRegex = regexp.MustCompile("[\\S]+: digest: ([\\S]+) size: [0-9]+") digestRegex = regexp.MustCompile("Digest: ([\\S]+)") ) -func setupImage(c *check.C) (string, error) { +func setupImage(c *check.C) (digest.Digest, error) { return setupImageWithTag(c, "latest") } -func setupImageWithTag(c *check.C, tag string) (string, error) { +func setupImageWithTag(c *check.C, tag string) (digest.Digest, error) { containerName := "busyboxbydigest" dockerCmd(c, "run", "-d", "-e", "digest=1", "--name", containerName, "busybox") @@ -52,7 +56,7 @@ func setupImageWithTag(c *check.C, tag string) (string, error) { } pushDigest := matches[1] - return pushDigest, nil + return digest.Digest(pushDigest), nil } func (s *DockerRegistrySuite) TestPullByTagDisplaysDigest(c *check.C) { @@ -72,7 +76,7 @@ func (s *DockerRegistrySuite) TestPullByTagDisplaysDigest(c *check.C) { pullDigest := matches[1] // make sure the pushed and pull digests match - if pushDigest != pullDigest { + if pushDigest.String() != pullDigest { c.Fatalf("push digest %q didn't match pull digest %q", pushDigest, pullDigest) } } @@ -95,7 +99,7 @@ func (s *DockerRegistrySuite) TestPullByDigest(c *check.C) { pullDigest := matches[1] // make sure the pushed and pull digests match - if pushDigest != pullDigest { + if pushDigest.String() != pullDigest { c.Fatalf("push digest %q didn't match pull digest %q", pushDigest, pullDigest) } } @@ -291,7 +295,7 @@ func (s *DockerRegistrySuite) TestListImagesWithDigests(c *check.C) { out, _ := dockerCmd(c, "images", "--digests") // make sure repo shown, tag=, digest = $digest1 - re1 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest1 + `\s`) + re1 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest1.String() + `\s`) if !re1.MatchString(out) { c.Fatalf("expected %q: %s", re1.String(), out) } @@ -319,7 +323,7 @@ func (s *DockerRegistrySuite) TestListImagesWithDigests(c *check.C) { } // make sure repo shown, tag=, digest = $digest2 - re2 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest2 + `\s`) + re2 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest2.String() + `\s`) if !re2.MatchString(out) { c.Fatalf("expected %q: %s", re2.String(), out) } @@ -332,7 +336,7 @@ func (s *DockerRegistrySuite) TestListImagesWithDigests(c *check.C) { // make sure image 1 has repo, tag, AND repo, , digest reWithTag1 := regexp.MustCompile(`\s*` + repoName + `\s*tag1\s*\s`) - reWithDigest1 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest1 + `\s`) + reWithDigest1 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest1.String() + `\s`) if !reWithTag1.MatchString(out) { c.Fatalf("expected %q: %s", reWithTag1.String(), out) } @@ -357,7 +361,7 @@ func (s *DockerRegistrySuite) TestListImagesWithDigests(c *check.C) { // make sure image 2 has repo, tag, digest reWithTag2 := regexp.MustCompile(`\s*` + repoName + `\s*tag2\s*\s`) - reWithDigest2 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest2 + `\s`) + reWithDigest2 := regexp.MustCompile(`\s*` + repoName + `\s*\s*` + digest2.String() + `\s`) if !reWithTag2.MatchString(out) { c.Fatalf("expected %q: %s", reWithTag2.String(), out) } @@ -401,3 +405,95 @@ func (s *DockerRegistrySuite) TestDeleteImageByIDOnlyPulledByDigest(c *check.C) dockerCmd(c, "rmi", imageID) } + +// TestPullFailsWithAlteredManifest tests that a `docker pull` fails when +// we have modified a manifest blob and its digest cannot be verified. +func (s *DockerRegistrySuite) TestPullFailsWithAlteredManifest(c *check.C) { + manifestDigest, err := setupImage(c) + if err != nil { + c.Fatalf("error setting up image: %v", err) + } + + // Load the target manifest blob. + manifestBlob := s.reg.readBlobContents(c, manifestDigest) + + var imgManifest manifest.Manifest + if err := json.Unmarshal(manifestBlob, &imgManifest); err != nil { + c.Fatalf("unable to decode image manifest from blob: %s", err) + } + + // Add a malicious layer digest to the list of layers in the manifest. + imgManifest.FSLayers = append(imgManifest.FSLayers, manifest.FSLayer{ + BlobSum: digest.Digest("sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"), + }) + + // Move the existing data file aside, so that we can replace it with a + // malicious blob of data. NOTE: we defer the returned undo func. + undo := s.reg.tempMoveBlobData(c, manifestDigest) + defer undo() + + alteredManifestBlob, err := json.Marshal(imgManifest) + if err != nil { + c.Fatalf("unable to encode altered image manifest to JSON: %s", err) + } + + s.reg.writeBlobContents(c, manifestDigest, alteredManifestBlob) + + // Now try pulling that image by digest. We should get an error about + // digest verification for the manifest digest. + + // Pull from the registry using the @ reference. + imageReference := fmt.Sprintf("%s@%s", repoName, manifestDigest) + out, exitStatus, _ := dockerCmdWithError("pull", imageReference) + if exitStatus == 0 { + c.Fatalf("expected a zero exit status but got %d: %s", exitStatus, out) + } + + expectedErrorMsg := fmt.Sprintf("image verification failed for digest %s", manifestDigest) + if !strings.Contains(out, expectedErrorMsg) { + c.Fatalf("expected error message %q in output: %s", expectedErrorMsg, out) + } +} + +// TestPullFailsWithAlteredLayer tests that a `docker pull` fails when +// we have modified a layer blob and its digest cannot be verified. +func (s *DockerRegistrySuite) TestPullFailsWithAlteredLayer(c *check.C) { + manifestDigest, err := setupImage(c) + if err != nil { + c.Fatalf("error setting up image: %v", err) + } + + // Load the target manifest blob. + manifestBlob := s.reg.readBlobContents(c, manifestDigest) + + var imgManifest manifest.Manifest + if err := json.Unmarshal(manifestBlob, &imgManifest); err != nil { + c.Fatalf("unable to decode image manifest from blob: %s", err) + } + + // Next, get the digest of one of the layers from the manifest. + targetLayerDigest := imgManifest.FSLayers[0].BlobSum + + // Move the existing data file aside, so that we can replace it with a + // malicious blob of data. NOTE: we defer the returned undo func. + undo := s.reg.tempMoveBlobData(c, targetLayerDigest) + defer undo() + + // Now make a fake data blob in this directory. + s.reg.writeBlobContents(c, targetLayerDigest, []byte("This is not the data you are looking for.")) + + // Now try pulling that image by digest. We should get an error about + // digest verification for the target layer digest. + + // Pull from the registry using the @ reference. + imageReference := fmt.Sprintf("%s@%s", repoName, manifestDigest) + out, exitStatus, _ := dockerCmdWithError("pull", imageReference) + if exitStatus == 0 { + c.Fatalf("expected a zero exit status but got: %d", exitStatus) + } + + expectedErrorMsg := fmt.Sprintf("filesystem layer verification failed for digest %s", targetLayerDigest) + if !strings.Contains(out, expectedErrorMsg) { + c.Fatalf("expected error message %q in output: %s", expectedErrorMsg, out) + } +} diff --git a/integration-cli/registry.go b/integration-cli/registry.go index ab44f05250..35e1b4eb9f 100644 --- a/integration-cli/registry.go +++ b/integration-cli/registry.go @@ -8,6 +8,7 @@ import ( "os/exec" "path/filepath" + "github.com/docker/distribution/digest" "github.com/go-check/check" ) @@ -70,3 +71,50 @@ func (t *testRegistryV2) Close() { t.cmd.Process.Kill() os.RemoveAll(t.dir) } + +func (t *testRegistryV2) getBlobFilename(blobDigest digest.Digest) string { + // Split the digest into it's algorithm and hex components. + dgstAlg, dgstHex := blobDigest.Algorithm(), blobDigest.Hex() + + // The path to the target blob data looks something like: + // baseDir + "docker/registry/v2/blobs/sha256/a3/a3ed...46d4/data" + return fmt.Sprintf("%s/docker/registry/v2/blobs/%s/%s/%s/data", t.dir, dgstAlg, dgstHex[:2], dgstHex) +} + +func (t *testRegistryV2) readBlobContents(c *check.C, blobDigest digest.Digest) []byte { + // Load the target manifest blob. + manifestBlob, err := ioutil.ReadFile(t.getBlobFilename(blobDigest)) + if err != nil { + c.Fatalf("unable to read blob: %s", err) + } + + return manifestBlob +} + +func (t *testRegistryV2) writeBlobContents(c *check.C, blobDigest digest.Digest, data []byte) { + if err := ioutil.WriteFile(t.getBlobFilename(blobDigest), data, os.FileMode(0644)); err != nil { + c.Fatalf("unable to write malicious data blob: %s", err) + } +} + +func (t *testRegistryV2) tempMoveBlobData(c *check.C, blobDigest digest.Digest) (undo func()) { + tempFile, err := ioutil.TempFile("", "registry-temp-blob-") + if err != nil { + c.Fatalf("unable to get temporary blob file: %s", err) + } + tempFile.Close() + + blobFilename := t.getBlobFilename(blobDigest) + + // Move the existing data file aside, so that we can replace it with a + // another blob of data. + if err := os.Rename(blobFilename, tempFile.Name()); err != nil { + os.Remove(tempFile.Name()) + c.Fatalf("unable to move data blob: %s", err) + } + + return func() { + os.Rename(tempFile.Name(), blobFilename) + os.Remove(tempFile.Name()) + } +}