Merge pull request #1220 from opscode/CHEF-4639-updated

CHEF-4639: writing credentials files with `file` or `template` may leak credentials in diffs

4 files changed, 106 insertions, 50 deletions

diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb
index b2127d7c87..3ef7725173 100644
--- a/lib/chef/provider/file.rb
+++ b/lib/chef/provider/file.rb
@@ -352,16 +352,27 @@ class Chef
         if tempfile.path.nil? || !::File.exists?(tempfile.path)
           raise "chef-client is confused, trying to deploy a file that has no path or does not exist..."
         end
+
         # the file? on the next line suppresses the case in why-run when we have a not-file here that would have otherwise been removed
         if ::File.file?(@new_resource.path) && contents_changed?
-          diff.diff(@current_resource.path, tempfile.path)
-          @new_resource.diff( diff.for_reporting ) unless file_created?
-          description = [ "update content in file #{@new_resource.path} from #{short_cksum(@current_resource.checksum)} to #{short_cksum(checksum(tempfile.path))}" ]
-          description << diff.for_output
+          description = [ "update content in file #{@new_resource.path} from \
+#{short_cksum(@current_resource.checksum)} to #{short_cksum(checksum(tempfile.path))}" ]
+
+          # Hide the diff output if the resource is marked as a sensitive resource
+          if @new_resource.sensitive
+            @new_resource.diff("suppressed sensitive resource")
+            description << "suppressed sensitive resource"
+          else
+            diff.diff(@current_resource.path, tempfile.path)
+            @new_resource.diff( diff.for_reporting ) unless file_created?
+            description << diff.for_output
+          end
+
           converge_by(description) do
             update_file_contents
           end
         end
+
         # unlink necessary to clean up in why-run mode
         tempfile.unlink
       end
@@ -420,4 +431,3 @@ class Chef
     end
   end
 end
-
diff --git a/lib/chef/resource/file.rb b/lib/chef/resource/file.rb
index 676cbf200a..3db88dcda0 100644
--- a/lib/chef/resource/file.rb
+++ b/lib/chef/resource/file.rb
@@ -52,9 +52,9 @@ class Chef
         @force_unlink = false
         @manage_symlink_source = nil
         @diff = nil
+        @sensitive = false
       end
 
-
       def content(arg=nil)
         set_or_return(
           :content,
@@ -119,6 +119,13 @@ class Chef
         )
       end
 
+      def sensitive(arg=nil)
+        set_or_return(
+          :sensitive,
+          arg,
+          :kind_of => [ TrueClass, FalseClass ]
+        )
+      end
     end
   end
 end
diff --git a/spec/functional/resource/file_spec.rb b/spec/functional/resource/file_spec.rb
index f688bae434..d6f56db3e9 100644
--- a/spec/functional/resource/file_spec.rb
+++ b/spec/functional/resource/file_spec.rb
@@ -115,5 +115,4 @@ describe Chef::Resource::File do
       end
     end
   end
-
 end
diff --git a/spec/support/shared/functional/file_resource.rb b/spec/support/shared/functional/file_resource.rb
index 44048598c7..ba06235aa8 100644
--- a/spec/support/shared/functional/file_resource.rb
+++ b/spec/support/shared/functional/file_resource.rb
@@ -16,6 +16,8 @@
 # limitations under the License.
 #
 
+require 'pry'
+
 shared_context "deploying with move" do
   before do
     Chef::Config[:file_backup_path] = CHEF_SPEC_BACKUP_PATH
@@ -54,25 +56,62 @@ shared_examples_for "a file with the wrong content" do
     sha256_checksum(path).should == @expected_checksum
   end
 
-  include_context "diff disabled"
+  describe "when diff is disabled" do
 
-  context "when running action :create" do
-    context "with backups enabled" do
-      before do
-        resource.run_action(:create)
+    include_context "diff disabled"
+
+    context "when running action :create" do
+      context "with backups enabled" do
+        before do
+          resource.run_action(:create)
+        end
+
+        it "overwrites the file with the updated content when the :create action is run" do
+          File.stat(path).mtime.should > @expected_mtime
+          sha256_checksum(path).should_not == @expected_checksum
+        end
+
+        it "backs up the existing file" do
+          Dir.glob(backup_glob).size.should equal(1)
+        end
+
+        it "is marked as updated by last action" do
+          resource.should be_updated_by_last_action
+        end
+
+        it "should restore the security contexts on selinux", :selinux_only do
+          selinux_security_context_restored?(path).should be_true
+        end
       end
 
-      it "overwrites the file with the updated content when the :create action is run" do
-        File.stat(path).mtime.should > @expected_mtime
-        sha256_checksum(path).should_not == @expected_checksum
+      context "with backups disabled" do
+        before do
+          resource.backup(0)
+          resource.run_action(:create)
+        end
+
+        it "should not attempt to backup the existing file if :backup == 0" do
+          Dir.glob(backup_glob).size.should equal(0)
+        end
+
+        it "should restore the security contexts on selinux", :selinux_only do
+          selinux_security_context_restored?(path).should be_true
+        end
       end
+    end
 
-      it "backs up the existing file" do
-        Dir.glob(backup_glob).size.should equal(1)
+    describe "when running action :create_if_missing" do
+      before do
+        resource.run_action(:create_if_missing)
       end
 
-      it "is marked as updated by last action" do
-        resource.should be_updated_by_last_action
+      it "doesn't overwrite the file when the :create_if_missing action is run" do
+        File.stat(path).mtime.should == @expected_mtime
+        sha256_checksum(path).should == @expected_checksum
+      end
+
+      it "is not marked as updated" do
+        resource.should_not be_updated_by_last_action
       end
 
       it "should restore the security contexts on selinux", :selinux_only do
@@ -80,52 +119,53 @@ shared_examples_for "a file with the wrong content" do
       end
     end
 
-    context "with backups disabled" do
+    describe "when running action :delete" do
       before do
-        resource.backup(0)
-        resource.run_action(:create)
+        resource.run_action(:delete)
       end
 
-      it "should not attempt to backup the existing file if :backup == 0" do
-        Dir.glob(backup_glob).size.should equal(0)
+      it "deletes the file" do
+        File.should_not exist(path)
       end
 
-      it "should restore the security contexts on selinux", :selinux_only do
-        selinux_security_context_restored?(path).should be_true
+      it "is marked as updated by last action" do
+        resource.should be_updated_by_last_action
       end
     end
+
   end
 
-  describe "when running action :create_if_missing" do
-    before do
-      resource.run_action(:create_if_missing)
-    end
+  context "when diff is enabled" do
+    describe 'sensitive attribute' do
+      context "should be insensitive by default" do
+        it { expect(resource.sensitive).to(be_false) }
+      end
 
-    it "doesn't overwrite the file when the :create_if_missing action is run" do
-      File.stat(path).mtime.should == @expected_mtime
-      sha256_checksum(path).should == @expected_checksum
-    end
+      context "when set" do
+        before { resource.sensitive(true) }
 
-    it "is not marked as updated" do
-      resource.should_not be_updated_by_last_action
-    end
+        it "should be set on the resource" do
+          expect(resource.sensitive).to(be_true)
+        end
 
-    it "should restore the security contexts on selinux", :selinux_only do
-      selinux_security_context_restored?(path).should be_true
-    end
-  end
+        context "when running :create action" do
+          let(:provider) { resource.provider_for_action(:create) }
+          let(:reporter_messages) { provider.instance_variable_get("@converge_actions").actions[0][0] }
 
-  describe "when running action :delete" do
-    before do
-      resource.run_action(:delete)
-    end
+          before do
+            provider.run_action
+          end
 
-    it "deletes the file" do
-      File.should_not exist(path)
-    end
+          it "should suppress the diff" do
+            expect(resource.diff).to(include('suppressed sensitive resource'))
+            expect(reporter_messages[1]).to eq("suppressed sensitive resource")
+          end
 
-    it "is marked as updated by last action" do
-      resource.should be_updated_by_last_action
+          it "should still include the updated checksums" do
+            expect(reporter_messages[0]).to include("update content in file")
+          end
+        end
+      end
     end
   end
 end