Merge pull request #761 from Mirantis/ivan4th/image-digest-check

Check image digests

pkg/image/fake/store.go

@@ -65,13 +65,13 @@ func (s *FakeStore) ListImages(filter string) ([]*image.Image, error) {
 
 // ImageStatus implements ImageStatus method of Store interface.
 func (s *FakeStore) ImageStatus(name string) (*image.Image, error) {
-	name = image.StripTags(name)
+	name, _ = image.SplitImageName(name)
 	return s.images[name], nil
 }
 
 // PullImage implements PullImage method of Store interface.
 func (s *FakeStore) PullImage(ctx context.Context, name string, translator image.Translator) (string, error) {
-	name = image.StripTags(name)
+	name, _ = image.SplitImageName(name)
 	ep := translator(ctx, name)
 	d := digest.FromString(name)
 	named, err := reference.WithName(name)

pkg/image/image.go

@@ -159,7 +159,7 @@ func (s *FileStore) dataFileName(hexDigest string) string {
 }
 
 func (s *FileStore) linkFileName(imageName string) string {
-	imageName = StripTags(imageName)
+	imageName, _ = SplitImageName(imageName)
 	return filepath.Join(s.linkDir(), strings.Replace(imageName, "/", "%", -1))
 }
 
@@ -322,6 +322,8 @@ func (s *FileStore) imageInfo(fi os.FileInfo) (*Image, error) {
 }
 
 func (s *FileStore) listImagesUnlocked(filter string) ([]*Image, error) {
+	filter, digestSpec := SplitImageName(filter)
+
 	if linkDirExists, err := s.linkDirExists(); err != nil {
 		return nil, err
 	} else if !linkDirExists {
@@ -339,13 +341,16 @@ func (s *FileStore) listImagesUnlocked(filter string) ([]*Image, error) {
 			continue
 		}
 		image, err := s.imageInfo(fi)
-		if err != nil {
+		switch {
+		case err != nil:
 			glog.Warningf("listing images: skipping image link %q: %v", fi.Name(), err)
 			continue
+		case filter != "" && image.Name != filter:
+			continue
+		case digestSpec != "" && digest.Digest(image.Digest) != digestSpec:
+			continue
 		}
-		if filter == "" || image.Name == filter {
-			r = append(r, image)
-		}
+		r = append(r, image)
 	}
 
 	return r, nil
@@ -363,7 +368,15 @@ func (s *FileStore) imageStatusUnlocked(name string) (*Image, error) {
 	// get info about the link itself, not its target
 	switch fi, err := os.Lstat(linkFileName); {
 	case err == nil:
-		return s.imageInfo(fi)
+		info, err := s.imageInfo(fi)
+		if err != nil {
+			return nil, err
+		}
+		_, digestSpec := SplitImageName(name)
+		if digestSpec != "" && digest.Digest(info.Digest) != digestSpec {
+			return nil, fmt.Errorf("image digest mismatch: %s instead of %s", info.Digest, digestSpec)
+		}
+		return info, nil
 	case os.IsNotExist(err):
 		return nil, nil
 	default:
@@ -380,7 +393,7 @@ func (s *FileStore) ImageStatus(name string) (*Image, error) {
 
 // PullImage implements PullImage method of Store interface.
 func (s *FileStore) PullImage(ctx context.Context, name string, translator Translator) (string, error) {
-	name = StripTags(name)
+	name, specDigest := SplitImageName(name)
 	ep := translator(ctx, name)
 	glog.V(1).Infof("Image translation: %q -> %q", name, ep.URL)
 	if err := os.MkdirAll(s.dataDir(), 0777); err != nil {
@@ -390,6 +403,11 @@ func (s *FileStore) PullImage(ctx context.Context, name string, translator Trans
 	if err != nil {
 		return "", fmt.Errorf("failed to create a temporary file: %v", err)
 	}
+	defer func() {
+		if tempFile != nil {
+			tempFile.Close()
+		}
+	}()
 	if err := s.downloader.DownloadFile(ctx, ep, tempFile); err != nil {
 		tempFile.Close()
 		if err := os.Remove(tempFile.Name()); err != nil {
@@ -409,7 +427,12 @@ func (s *FileStore) PullImage(ctx context.Context, name string, translator Trans
 	if err := tempFile.Close(); err != nil {
 		return "", fmt.Errorf("closing %q: %v", tempFile.Name(), err)
 	}
-	if err := s.placeImage(tempFile.Name(), d.Hex(), name); err != nil {
+	fileName := tempFile.Name()
+	tempFile = nil
+	if specDigest != "" && d != specDigest {
+		return "", fmt.Errorf("image digest mismatch: %s instead of %s", d, specDigest)
+	}
+	if err := s.placeImage(fileName, d.Hex(), name); err != nil {
 		return "", err
 	}
 	named, err := reference.WithName(name)
@@ -524,17 +547,25 @@ func (s *FileStore) SetRefGetter(imageRefGetter RefGetter) {
 	s.refGetter = imageRefGetter
 }
 
-// StripTags removes tags from an image name.
-func StripTags(imageName string) string {
+// SplitImageName parses image nmae and returns the name sans tag and
+// the digest, if any.
+func SplitImageName(imageName string) (string, digest.Digest) {
 	ref, err := reference.Parse(imageName)
 	if err != nil {
 		glog.Warningf("StripTags: failed to parse image name as ref: %q: %v", imageName, err)
-		return imageName
+		return imageName, ""
 	}
-	if namedTagged, ok := ref.(reference.NamedTagged); ok {
-		return namedTagged.Name()
+
+	named, ok := ref.(reference.Named)
+	if !ok {
+		return imageName, ""
 	}
-	return imageName
+
+	if digested, ok := ref.(reference.Digested); ok {
+		return named.Name(), digested.Digest()
+	}
+
+	return named.Name(), ""
 }
 
 // GetHexDigest returns the hex digest contained in imageSpec, if any,

pkg/image/image_test.go

@@ -295,6 +295,8 @@ func TestPullListStatus(t *testing.T) {
 	tst.pullImage(tst.images[2].Name, tst.refs[2])
 	tst.verifyListImages("", tst.images[1], tst.images[0], tst.images[2]) // alphabetically sorted by name
 	tst.verifySubpathContents("links/foobar", "###baz")
+
+	tst.verifyListImages(tst.refs[1], tst.images[1])
 }
 
 func TestReplaceImage(t *testing.T) {
@@ -476,3 +478,35 @@ func TestCancelPullImage(t *testing.T) {
 		t.Errorf("the downloader isn't marked as canelled")
 	}
 }
+
+func TestVerifyImageChecksum(t *testing.T) {
+	tst := newIfsTester(t)
+	defer tst.teardown()
+
+	// Use image ref instead of the name.
+	// The ref contains sha256 sum
+	tst.pullImage(tst.refs[0], tst.refs[0])
+	tst.verifyListImages("foobar")
+
+	refWithBadDigest := tst.images[0].Name + "@sha256:0000000000000000000000000000000000000000000000000000000000000000"
+	_, err := tst.store.PullImage(
+		context.Background(),
+		refWithBadDigest,
+		tst.translateImageName)
+	switch {
+	case err == nil:
+		tst.t.Errorf("PullImage() din't return any error for an image with mismatching digest")
+	case !strings.Contains(err.Error(), "image digest mismatch"):
+		t.Errorf("PullImage() is expected to return invalid checksum error but returned %q", err)
+	}
+
+	switch _, err := tst.store.ImageStatus(refWithBadDigest); {
+	case err == nil:
+		tst.t.Errorf("ImageStatus() din't return any error for an image with mismatching digest")
+	case !strings.Contains(err.Error(), "image digest mismatch"):
+		t.Errorf("ImageStatus() is expected to return invalid checksum error but returned %q", err)
+	}
+
+	// the bad digest should not match any images while listing
+	tst.verifyListImages(refWithBadDigest)
+}

tests/e2e/basic_test.go

@@ -38,7 +38,7 @@ var _ = Describe("Virtlet [Basic cirros tests]", func() {
 
 	BeforeAll(func() {
 		vm = controller.VM("cirros-vm")
-		Expect(vm.Create(VMOptions{}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
+		Expect(vm.CreateAndWait(VMOptions{}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		var err error
 		vmPod, err = vm.Pod()
 		Expect(err).NotTo(HaveOccurred())
@@ -182,7 +182,7 @@ var _ = Describe("Virtlet [Disruptive]", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		vm = controller.VM("cirros-vm")
-		Expect(vm.Create(VMOptions{}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
+		Expect(vm.CreateAndWait(VMOptions{}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 	})
 })
 
@@ -228,3 +228,35 @@ func itShouldHaveNetworkConnectivity(podIface func() *framework.PodInterface, ss
 		}, 60*5)
 	})
 }
+
+var _ = Describe("Virtlet [Image verification]", func() {
+	var (
+		vm *framework.VMInterface
+	)
+
+	BeforeAll(func() {
+		vm = controller.VM("cirros-vm")
+	})
+
+	AfterAll(func() {
+		Expect(vm.Delete(time.Minute * 2)).To(Succeed())
+	})
+
+	It("Should fail for images with mismatching digest", func() {
+		opts := VMOptions{}.ApplyDefaults()
+		p := strings.Index(opts.Image, "@")
+		if p >= 0 {
+			opts.Image = opts.Image[:p]
+		}
+		opts.Image += "@sha256:0000000000000000000000000000000000000000000000000000000000000000"
+		Expect(vm.Create(opts, nil)).To(Succeed())
+
+		pod := vm.PodWithoutChecks()
+		Expect(pod.WaitForPodStatus([]string{
+			"ErrImagePull", "ImagePullBackOff", "ImageInspectError",
+		}, time.Minute*5)).To(Succeed())
+		events, err := pod.LoadEvents()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(events).To(ContainElement(ContainSubstring("image digest mismatch")))
+	})
+})

tests/e2e/ceph_test.go

@@ -65,7 +65,7 @@ var _ = Describe("Ceph volumes tests", func() {
 				})
 			}
 
-			Expect(vm.Create(VMOptions{}.ApplyDefaults(), time.Minute*5, podCustomization)).To(Succeed())
+			Expect(vm.CreateAndWait(VMOptions{}.ApplyDefaults(), time.Minute*5, podCustomization)).To(Succeed())
 			var err error
 			_, err = vm.Pod()
 			Expect(err).NotTo(HaveOccurred())
@@ -141,7 +141,7 @@ var _ = Describe("Ceph volumes tests", func() {
 				})
 			}
 
-			Expect(vm.Create(VMOptions{}.ApplyDefaults(), time.Minute*5, podCustomization)).To(Succeed())
+			Expect(vm.CreateAndWait(VMOptions{}.ApplyDefaults(), time.Minute*5, podCustomization)).To(Succeed())
 			_ = do(vm.Pod()).(*framework.PodInterface)
 		})
 

tests/e2e/cloudinit_test.go

@@ -44,7 +44,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("ssh-from-cm-impl")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				SSHKeySource: "configmap/cm-ssh-key-impl",
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		})
@@ -75,7 +75,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("ssh-from-cm-expl")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				SSHKeySource: "configmap/cm-ssh-key-expl/myKey",
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		})
@@ -106,7 +106,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("ssh-from-secret-impl")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				SSHKeySource: "secret/secret-ssh-key-impl",
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		})
@@ -137,7 +137,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("ssh-from-secret-expl")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				SSHKeySource: "secret/secret-ssh-key-expl/myKey",
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		})
@@ -171,7 +171,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("userdata-cm")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				UserDataSource: "configmap/cm-userdata",
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		})
@@ -206,7 +206,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("userdata-secret")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				UserDataSource: "secret/secret-userdata",
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())
 		})
@@ -242,7 +242,7 @@ var _ = Describe("Cloud-init related tests", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			vm = controller.VM("userdata-cm-merge")
-			Expect(vm.Create(VMOptions{
+			Expect(vm.CreateAndWait(VMOptions{
 				UserDataSource: "configmap/cm-userdata",
 				UserData:       userData,
 			}.ApplyDefaults(), time.Minute*5, nil)).To(Succeed())

tests/e2e/cni_test.go

@@ -35,7 +35,7 @@ var _ = Describe("Virtlet CNI", func() {
 		// if network namespace was deleted
 		It("Should delete network namespace when VM is deleted", func() {
 			vm = controller.VM("cirros-vm")
-			err := vm.Create(VMOptions{}.ApplyDefaults(), time.Minute*5, nil)
+			err := vm.CreateAndWait(VMOptions{}.ApplyDefaults(), time.Minute*5, nil)
 			Expect(err).NotTo(HaveOccurred())
 
 			virtletPod, err := vm.VirtletPod()

tests/e2e/constants.go

@@ -17,7 +17,7 @@ limitations under the License.
 package e2e
 
 const (
-	defaultVMImageLocation = "download.cirros-cloud.net/0.4.0/cirros-0.4.0-x86_64-disk.img"
+	defaultVMImageLocation = "download.cirros-cloud.net/0.4.0/cirros-0.4.0-x86_64-disk.img@sha256:a8dd75ecffd4cdd96072d60c2237b448e0c8b2bc94d57f10fdbc8c481d9005b8"
 	DefaultSSHUser         = "cirros"
 
 	SshPublicKey = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCaJEcFDXEK2ZbX0ZLS1EIYFZRbDAcRfuVjpstSc0De8+sV1aiu+deP" +