Verify cookbook checksum before downloadpraj/FLOW-339/checksums

3 files changed, 86 insertions, 6 deletions

diff --git a/lib/chef/cookbook/synchronizer.rb b/lib/chef/cookbook/synchronizer.rb
index 1ee30bacc7..64e461890c 100644
--- a/lib/chef/cookbook/synchronizer.rb
+++ b/lib/chef/cookbook/synchronizer.rb
@@ -252,7 +252,7 @@ class Chef
       # (remote, per manifest), do the update. This will also execute if there
       # is no current checksum.
       if !cached_copy_up_to_date?(cache_filename, file.manifest_record["checksum"])
-        download_file(file.manifest_record["url"], cache_filename)
+        download_file(file, cache_filename)
         @events.updated_cookbook_file(file.cookbook.name, cache_filename)
       else
         Chef::Log.debug("Not storing #{cache_filename}, as the cache is up to date.")
@@ -278,11 +278,16 @@ class Chef
     # Unconditionally download the file from the given URL. File will be
     # downloaded to the path +destination+ which is relative to the Chef file
     # cache root.
-    def download_file(url, destination)
-      raw_file = server_api.streaming_request(url)
-
-      Chef::Log.info("Storing updated #{destination} in the cache.")
-      cache.move_to(raw_file.path, destination)
+    def download_file(file, destination)
+      raw_file = server_api.streaming_request(file.manifest_record["url"])
+      raw_file_md5 = Chef::Digester.generate_md5_checksum_for_file(raw_file.path)
+      Chef::Log.debug("The md5 hash of the cookbook file to be served: #{raw_file_md5}")
+      if raw_file_md5 == file.manifest_record["checksum"]
+        Chef::Log.debug("Storing updated #{destination} in the cache.")
+        cache.move_to(raw_file.path, destination)
+      else
+        raise Chef::Exceptions::FileIntegrityCompromise.new(raw_file.path, file.manifest_record["checksum"], raw_file_md5)
+      end
     end
 
     # Marks the given file as valid (non-stale).
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index 43759568a7..d80c0964ec 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -520,5 +520,11 @@ This error is most often caused by network issues (proxies, etc) outside of chef
         super "Found multiple matching resources. #{matches_info.join("\n")}"
       end
     end
+
+    class FileIntegrityCompromise < RuntimeError
+      def initialize(file, orig_chksum, current_chksum)
+        super "For the file: #{File.basename(file)} original checksum: (#{orig_chksum}) does not match existing content checksum: (#{current_chksum})"
+      end
+    end
   end
 end
diff --git a/spec/unit/cookbook/synchronizer_spec.rb b/spec/unit/cookbook/synchronizer_spec.rb
index 3f5624f3b0..e6012cf6c7 100644
--- a/spec/unit/cookbook/synchronizer_spec.rb
+++ b/spec/unit/cookbook/synchronizer_spec.rb
@@ -213,6 +213,36 @@ describe Chef::CookbookSynchronizer do
            :path => "/tmp/cookbook_a_template_default_tempfile")
   end
 
+  def setup_common_files_missing_expectations_for_downloads
+    # Files are not in the cache:
+    expect(file_cache).to receive(:has_key?).
+      with("cookbooks/cookbook_a/recipes/default.rb").
+      and_return(false)
+    expect(file_cache).to receive(:has_key?).
+      with("cookbooks/cookbook_a/attributes/default.rb").
+      and_return(false)
+
+    # Fetch and copy default.rb recipe
+    expect(server_api).to receive(:streaming_request).
+      with("http://chef.example.com/abc123").
+      and_return(cookbook_a_default_recipe_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_default_recipe_tempfile.path).
+      and_return("xyz123")
+    expect(file_cache).not_to receive(:move_to)
+    expect(file_cache).not_to receive(:load)
+
+    # Fetch and copy default.rb attribute file
+    expect(server_api).to receive(:streaming_request).
+      with("http://chef.example.com/abc456").
+      and_return(cookbook_a_default_attribute_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_default_attribute_tempfile.path).
+      and_return("xyz456")
+    expect(file_cache).not_to receive(:move_to)
+    expect(file_cache).not_to receive(:load)
+  end
+
   def setup_common_files_missing_expectations
     # Files are not in the cache:
     expect(file_cache).to receive(:has_key?).
@@ -226,6 +256,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/abc123").
       and_return(cookbook_a_default_recipe_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_default_recipe_tempfile.path).
+      and_return("abc123")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_recipes_default_rb", "cookbooks/cookbook_a/recipes/default.rb")
     expect(file_cache).to receive(:load).
@@ -236,6 +269,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/abc456").
       and_return(cookbook_a_default_attribute_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_default_attribute_tempfile.path).
+      and_return("abc456")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_attributes_default_rb", "cookbooks/cookbook_a/attributes/default.rb")
     expect(file_cache).to receive(:load).
@@ -254,6 +290,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/megaman.conf").
       and_return(cookbook_a_file_default_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_file_default_tempfile.path).
+      and_return("abc124")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_file_default_tempfile", "cookbooks/cookbook_a/files/default/megaman.conf")
     expect(file_cache).to receive(:load).
@@ -263,6 +302,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/ffffff").
       and_return(cookbook_a_template_default_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_template_default_tempfile.path).
+      and_return("abc125")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_template_default_tempfile", "cookbooks/cookbook_a/templates/default/apache2.conf.erb")
     expect(file_cache).to receive(:load).
@@ -283,6 +325,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/abc123").
       and_return(cookbook_a_default_recipe_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_default_recipe_tempfile.path).
+      and_return("abc123")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_recipes_default_rb", "cookbooks/cookbook_a/recipes/default.rb")
     expect(file_cache).to receive(:load).
@@ -299,6 +344,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/abc456").
       and_return(cookbook_a_default_attribute_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_default_attribute_tempfile.path).
+      and_return("abc456")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_attributes_default_rb", "cookbooks/cookbook_a/attributes/default.rb")
     expect(file_cache).to receive(:load).
@@ -325,6 +373,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/megaman.conf").
       and_return(cookbook_a_file_default_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_file_default_tempfile.path).
+      and_return("abc124")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_file_default_tempfile", "cookbooks/cookbook_a/files/default/megaman.conf")
     expect(file_cache).to receive(:load).
@@ -336,6 +387,9 @@ describe Chef::CookbookSynchronizer do
     expect(server_api).to receive(:streaming_request).
       with("http://chef.example.com/ffffff").
       and_return(cookbook_a_template_default_tempfile)
+    expect(Chef::Digester).to receive(:generate_md5_checksum_for_file).
+      with(cookbook_a_template_default_tempfile.path).
+      and_return("abc125")
     expect(file_cache).to receive(:move_to).
       with("/tmp/cookbook_a_template_default_tempfile", "cookbooks/cookbook_a/templates/default/apache2.conf.erb")
     expect(file_cache).to receive(:load).
@@ -427,6 +481,17 @@ describe Chef::CookbookSynchronizer do
       allow(synchronizer).to receive(:cache).and_return(file_cache)
     end
 
+    context "when the cookbook checksum does not match the original checksum" do
+      before do
+        setup_common_files_missing_expectations_for_downloads
+      end
+      let(:no_lazy_load) { false }
+
+      it "does not download cookbook and raise error" do
+        expect { synchronizer.sync_cookbooks }.to raise_error(Chef::Exceptions::FileIntegrityCompromise)
+      end
+    end
+
     context "when the cache does not contain the desired files" do
       before do
         setup_common_files_missing_expectations
@@ -439,6 +504,10 @@ describe Chef::CookbookSynchronizer do
           synchronizer.sync_cookbooks
         end
 
+        it "downloads cookbooks if checksum matches" do
+          expect { synchronizer.sync_cookbooks }.not_to raise_error
+        end
+
         it "does not fetch templates or cookbook files" do
           # Implicitly tested in previous test; this test is just for behavior specification.
           expect(server_api).not_to receive(:streaming_request).