Add hash verification to the PluginVmImage Manager

After DownloadService successfully finishes the download of PluginVm
image archive download should be verified using hash specified by
PluginVmImage cloud user policy. If hash provided by the policy doesn't
match hash provided by DownloadService download fails.

Test: unit_tests --gtest_filter="PluginVmImageManager*"
Bug: 928816
Change-Id: I5571b8c65638a31113e6c407e62737006809c3fd
Reviewed-on: https://chromium-review.googlesource.com/c/1477225
Commit-Queue: Olya Kalitova <okalitova@chromium.org>
Reviewed-by: Igor <igorcov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635530}

chrome/browser/chromeos/plugin_vm/plugin_vm_image_download_client.cc

@@ -98,9 +98,7 @@ void PluginVmImageDownloadClient::OnDownloadSucceeded(
     const download::CompletionInfo& completion_info) {
   VLOG(1) << __func__ << " called";
   VLOG(1) << "Downloaded file is in " << completion_info.path.value();
-  // TODO(https://crbug.com/904851): Verify download using hash specified by
-  // PluginVmImage user policy. If hashes don't match remove downloaded file.
-  GetManager()->OnDownloadCompleted(completion_info.path);
+  GetManager()->OnDownloadCompleted(completion_info);
 }
 
 bool PluginVmImageDownloadClient::CanServiceRemoveDownloadedFile(

chrome/browser/chromeos/plugin_vm/plugin_vm_image_manager.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_pref_names.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/download/public/background_service/download_metadata.h"
 #include "components/download/public/background_service/download_service.h"
 #include "components/prefs/pref_service.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
@@ -88,11 +89,18 @@ void PluginVmImageManager::OnProgressUpdated(
     observer_->OnProgressUpdated(fraction_complete);
 }
 
-void PluginVmImageManager::OnDownloadCompleted(base::FilePath file_path) {
-  // TODO(https://crbug.com/928816): Call OnDownloadFailed() in case download
-  // verification using hashes fails.
-  downloaded_plugin_vm_image_archive_ = file_path;
+void PluginVmImageManager::OnDownloadCompleted(
+    const download::CompletionInfo& info) {
+  downloaded_plugin_vm_image_archive_ = info.path;
   current_download_guid_.clear();
+
+  if (!VerifyDownload(info.hash256)) {
+    LOG(ERROR) << "Downloaded PluginVm image archive hash doesn't match "
+               << "hash specified by the PluginVmImage policy";
+    OnDownloadFailed();
+    return;
+  }
+
   if (observer_)
     observer_->OnDownloadCompleted();
 }
@@ -119,7 +127,7 @@ void PluginVmImageManager::SetDownloadServiceForTesting(
 }
 
 void PluginVmImageManager::SetDownloadedPluginVmImageArchiveForTesting(
-    base::FilePath downloaded_plugin_vm_image_archive) {
+    const base::FilePath& downloaded_plugin_vm_image_archive) {
   downloaded_plugin_vm_image_archive_ = downloaded_plugin_vm_image_archive;
 }
 
@@ -188,7 +196,19 @@ bool PluginVmImageManager::IsDownloading() {
   return !current_download_guid_.empty();
 }
 
-bool PluginVmImageManager::VerifyDownload(std::string downloaded_archive_hash) {
+bool PluginVmImageManager::VerifyDownload(
+    const std::string& downloaded_archive_hash) {
+  // Hash should be there in the common case. However, there are situations,
+  // when hash could not be available, for example, when download is parallel
+  // or the completion of download is reported after restart. Therefore, hash
+  // not being specified should not resolve in download being considered
+  // unsuccessful, but should be logged.
+  // TODO(okalitova): Consider download as unsuccessful once hash should always
+  // be in place.
+  if (downloaded_archive_hash.empty()) {
+    LOG(WARNING) << "No hash for downloaded PluginVm image archive";
+    return true;
+  }
   const base::Value* plugin_vm_image_hash_ptr =
       profile_->GetPrefs()
           ->GetDictionary(plugin_vm::prefs::kPluginVmImage)

chrome/browser/chromeos/plugin_vm/plugin_vm_image_manager.h

@@ -21,6 +21,7 @@ class Optional;
 
 namespace download {
 class DownloadService;
+struct CompletionInfo;
 }  // namespace download
 
 class Profile;
@@ -82,7 +83,7 @@ class PluginVmImageManager : public KeyedService {
   // Called by PluginVmImageDownloadClient.
   void OnDownloadStarted();
   void OnProgressUpdated(base::Optional<double> fraction_complete);
-  void OnDownloadCompleted(base::FilePath file_path);
+  void OnDownloadCompleted(const download::CompletionInfo& info);
   void OnDownloadCancelled();
   void OnDownloadFailed();
 
@@ -90,10 +91,14 @@ class PluginVmImageManager : public KeyedService {
   // image archive. In case |success| is false also deletes PluginVm image.
   void OnUnzipped(bool success);
 
+  // Returns true in case downloaded PluginVm image archive passes verification
+  // and false otherwise.
+  bool VerifyDownload(const std::string& downloaded_archive_hash);
+
   void SetDownloadServiceForTesting(
       download::DownloadService* download_service);
   void SetDownloadedPluginVmImageArchiveForTesting(
-      base::FilePath downloaded_plugin_vm_image_archive);
+      const base::FilePath& downloaded_plugin_vm_image_archive);
   std::string GetCurrentDownloadGuidForTesting();
 
  private:
@@ -134,9 +139,6 @@ class PluginVmImageManager : public KeyedService {
   void OnStartDownload(const std::string& download_guid,
                        download::DownloadParams::StartResult start_result);
   bool IsDownloading();
-  // Returns true in case downloaded PluginVm image archive passes verification
-  // and false otherwise.
-  bool VerifyDownload(std::string downloaded_archive_hash);
 
   bool UnzipDownloadedPluginVmImageArchive();
   bool IsUnzippingCancelled();

chrome/browser/chromeos/plugin_vm/plugin_vm_image_manager_unittest.cc

@@ -32,6 +32,10 @@ const char kProfileName[] = "p1";
 const char kUrl[] = "http://example.com";
 const char kHash[] =
     "842841a4c75a55ad050d686f4ea5f77e83ae059877fe9b6946aa63d3d057ed32";
+const char kHashUppercase[] =
+    "842841A4C75A55AD050D686F4EA5F77E83AE059877FE9B6946AA63D3D057ED32";
+const char kHash2[] =
+    "02f06421ae27144aacdc598aebcd345a5e2e634405e8578300173628fe1574bd";
 const char kPluginVmImageUnzipped[] = "plugin_vm_image_unzipped";
 const char kPluginVmImageFile1[] = "plugin_vm_image_file_1";
 const char kContent1[] = "This is content #1.";
@@ -158,8 +162,7 @@ TEST_F(PluginVmImageManagerTest, DownloadPluginVmImageParamsTest) {
 
   // Finishing image processing.
   test_browser_thread_bundle_.RunUntilIdle();
-  // Faking downloaded file for testing. Creates one additional call to
-  // OnDownloadCompleted().
+  // Faking downloaded file for testing.
   manager_->SetDownloadedPluginVmImageArchiveForTesting(
       fake_downloaded_plugin_vm_image_archive_);
   manager_->StartUnzipping();
@@ -302,4 +305,10 @@ TEST_F(PluginVmImageManagerTest, EmptyPluginVmImageUrlTest) {
   test_browser_thread_bundle_.RunUntilIdle();
 }
 
+TEST_F(PluginVmImageManagerTest, VerifyDownloadTest) {
+  EXPECT_FALSE(manager_->VerifyDownload(kHash2));
+  EXPECT_TRUE(manager_->VerifyDownload(kHashUppercase));
+  EXPECT_TRUE(manager_->VerifyDownload(kHash));
+}
+
 }  // namespace plugin_vm

chrome/browser/ui/views/plugin_vm/plugin_vm_launcher_view_browsertest.cc

@@ -7,6 +7,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/download/public/background_service/download_metadata.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/views/window/dialog_client_view.h"
@@ -80,7 +81,7 @@ IN_PROC_BROWSER_TEST_F(PluginVmLauncherViewBrowserTest, SetupCompleted) {
   CheckSetupIsInProgress();
 
   view_->GetPluginVmImageManagerForTesting()->OnDownloadCompleted(
-      base::FilePath());
+      download::CompletionInfo());
 
   CheckSetupIsInProgress();
 
@@ -118,7 +119,7 @@ IN_PROC_BROWSER_TEST_F(PluginVmLauncherViewBrowserTest,
   CheckSetupIsInProgress();
 
   view_->GetPluginVmImageManagerForTesting()->OnDownloadCompleted(
-      base::FilePath());
+      download::CompletionInfo());
 
   CheckSetupIsInProgress();