Merge pull request #1083 from opscode/lcg/CHEF-4662-11-stable

CHEF-4662: fix insecure knife tempfiles

5 files changed, 179 insertions, 63 deletions

diff --git a/lib/chef/knife/core/node_editor.rb b/lib/chef/knife/core/node_editor.rb
index 7707743ce5..073492197c 100644
--- a/lib/chef/knife/core/node_editor.rb
+++ b/lib/chef/knife/core/node_editor.rb
@@ -18,6 +18,7 @@
 
 require 'chef/json_compat'
 require 'chef/node'
+require 'tempfile'
 
 class Chef
   class Knife
@@ -35,11 +36,24 @@ class Chef
         abort "You specified the --disable_editing option, nothing to edit" if config[:disable_editing]
         assert_editor_set!
 
-        updated_node_data = edit_data(view)
+        updated_node_data = @ui.edit_data(view)
         apply_updates(updated_node_data)
         @updated_node
       end
 
+      def updated?
+        pristine_copy = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(node), :create_additions => false)
+        updated_copy  = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(@updated_node), :create_additions => false)
+        unless pristine_copy == updated_copy
+          updated_properties = %w{name normal chef_environment run_list default override automatic}.reject do |key|
+             pristine_copy[key] == updated_copy[key]
+          end
+        end
+        ( pristine_copy != updated_copy ) && updated_properties
+      end
+
+      private
+
       def view
         result = {}
         result["name"] = node.name
@@ -52,12 +66,7 @@ class Chef
           result["override"]  = node.override_attrs
           result["automatic"] = node.automatic_attrs
         end
-        Chef::JSONCompat.to_json_pretty(result)
-      end
-
-      def edit_data(text)
-        edited_data = tempfile_for(text) {|filename| system("#{config[:editor]} #{filename}")}
-        Chef::JSONCompat.from_json(edited_data)
+        result
       end
 
       def apply_updates(updated_data)
@@ -84,19 +93,6 @@ class Chef
         end
       end
 
-      def updated?
-        pristine_copy = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(node), :create_additions => false)
-        updated_copy  = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(@updated_node), :create_additions => false)
-        unless pristine_copy == updated_copy
-          updated_properties = %w{name normal chef_environment run_list default override automatic}.reject do |key|
-             pristine_copy[key] == updated_copy[key]
-          end
-        end
-        ( pristine_copy != updated_copy ) && updated_properties
-      end
-
-      private
-
       def abort(message)
         ui.error(message)
         exit 1
@@ -108,22 +104,6 @@ class Chef
         end
       end
 
-      def tempfile_for(data)
-        # TODO: include useful info like the node name in the temp file
-        # name
-        basename = "knife-edit-" << rand(1_000_000_000_000_000).to_s.rjust(15, '0') << '.json'
-        filename = File.join(Dir.tmpdir, basename)
-        File.open(filename, "w+") do |f|
-          f.sync = true
-          f.puts data
-        end
-
-        yield filename
-
-        IO.read(filename)
-      ensure
-        File.unlink(filename)
-      end
     end
   end
 end
diff --git a/lib/chef/knife/core/ui.rb b/lib/chef/knife/core/ui.rb
index d0bdaa7ac0..dfa8c11644 100644
--- a/lib/chef/knife/core/ui.rb
+++ b/lib/chef/knife/core/ui.rb
@@ -21,6 +21,7 @@
 require 'forwardable'
 require 'chef/platform/query_helpers'
 require 'chef/knife/core/generic_presenter'
+require 'tempfile'
 
 class Chef
   class Knife
@@ -165,19 +166,14 @@ class Chef
         output = Chef::JSONCompat.to_json_pretty(data)
 
         if (!config[:disable_editing])
-          filename = "knife-edit-"
-          0.upto(20) { filename += rand(9).to_s }
-          filename << ".json"
-          filename = File.join(Dir.tmpdir, filename)
-          tf = File.open(filename, "w")
-          tf.sync = true
-          tf.puts output
-          tf.close
-          raise "Please set EDITOR environment variable" unless system("#{config[:editor]} #{tf.path}")
-          tf = File.open(filename, "r")
-          output = tf.gets(nil)
-          tf.close
-          File.unlink(filename)
+          Tempfile.open([ 'knife-edit-', '.json' ]) do |tf|
+            tf.sync = true
+            tf.puts output
+            tf.close
+            raise "Please set EDITOR environment variable" unless system("#{config[:editor]} #{tf.path}")
+
+            output = IO.read(tf.path)
+          end
         end
 
         parse_output ? Chef::JSONCompat.from_json(output) : output
diff --git a/lib/chef/knife/edit.rb b/lib/chef/knife/edit.rb
index 830da84a12..7408179ed0 100644
--- a/lib/chef/knife/edit.rb
+++ b/lib/chef/knife/edit.rb
@@ -51,10 +51,8 @@ class Chef
 
       def edit_text(text, extension)
         if (!config[:disable_editing])
-          file = Tempfile.new([ 'knife-edit-', extension ])
-          begin
+          Tempfile.open([ 'knife-edit-', extension ]) do |file|
             # Write the text to a temporary file
-            file.open
             file.write(text)
             file.close
 
@@ -63,12 +61,9 @@ class Chef
               raise "Please set EDITOR environment variable"
             end
 
-            file.open
-            result_text = file.read
-            return result_text if result_text != text
+            result_text = IO.read(file.path)
 
-          ensure
-            file.close!
+            return result_text if result_text != text
           end
         end
       end
diff --git a/spec/unit/knife/core/ui_spec.rb b/spec/unit/knife/core/ui_spec.rb
index 0b6978596a..c56ac9eab6 100644
--- a/spec/unit/knife/core/ui_spec.rb
+++ b/spec/unit/knife/core/ui_spec.rb
@@ -28,6 +28,124 @@ describe Chef::Knife::UI do
     @ui = Chef::Knife::UI.new(@out, @err, @in, @config)
   end
 
+  describe "edit" do
+    ruby_for_json = { 'foo' => 'bar' }
+    json_from_ruby = "{\n  \"foo\": \"bar\"\n}"
+    json_from_editor = "{\n  \"bar\": \"foo\"\n}"
+    ruby_from_editor = { 'bar' => 'foo' }
+    my_editor = "veeeye"
+    temp_path = "/tmp/bar/baz"
+
+    let(:subject) { @ui.edit_data(ruby_for_json, parse_output) }
+    let(:parse_output) { false }
+
+    context "when editing is disabled" do
+      before do
+        @ui.config[:disable_editing] = true
+        stub_const("Tempfile", double)  # Tempfiles should never be invoked
+      end
+      context "when parse_output is false" do
+        it "returns pretty json string" do
+          expect(subject).to eql(json_from_ruby)
+        end
+      end
+      context "when parse_output is true" do
+        let(:parse_output) { true }
+        it "returns a ruby object" do
+          expect(subject).to eql(ruby_for_json)
+        end
+      end
+
+    end
+
+    context "when editing is enabled" do
+      before do
+        @ui.config[:disable_editing] = false
+        @ui.config[:editor] = my_editor
+        @mock = mock('Tempfile')
+        @mock.should_receive(:sync=).with(true)
+        @mock.should_receive(:puts).with(json_from_ruby)
+        @mock.should_receive(:close)
+        @mock.should_receive(:path).at_least(:once).and_return(temp_path)
+        Tempfile.should_receive(:open).with([ 'knife-edit-', '.json' ]).and_yield(@mock)
+      end
+      context "and the editor works" do
+        before do
+          @ui.should_receive(:system).with("#{my_editor} #{temp_path}").and_return(true)
+          IO.should_receive(:read).with(temp_path).and_return(json_from_editor)
+        end
+
+        context "when parse_output is false" do
+          it "returns an edited pretty json string" do
+            expect(subject).to eql(json_from_editor)
+          end
+        end
+        context "when parse_output is true" do
+          let(:parse_output) { true }
+          it "returns an edited ruby object" do
+            expect(subject).to eql(ruby_from_editor)
+          end
+        end
+      end
+      context "when running the editor fails with nil" do
+        before do
+          @ui.should_receive(:system).with("#{my_editor} #{temp_path}").and_return(nil)
+          IO.should_not_receive(:read)
+        end
+        it "throws an exception" do
+          expect{ subject }.to raise_error(RuntimeError)
+        end
+      end
+      context "when running the editor fails with false" do
+        before do
+          @ui.should_receive(:system).with("#{my_editor} #{temp_path}").and_return(false)
+          IO.should_not_receive(:read)
+        end
+        it "throws an exception" do
+          expect{ subject }.to raise_error(RuntimeError)
+        end
+      end
+    end
+    context "when editing and not stubbing Tempfile (semi-functional test)" do
+      before do
+        @ui.config[:disable_editing] = false
+        @ui.config[:editor] = my_editor
+        @tempfile = Tempfile.new([ 'knife-edit-', '.json' ])
+        Tempfile.should_receive(:open).with([ 'knife-edit-', '.json' ]).and_yield(@tempfile)
+      end
+
+      context "and the editor works" do
+        before do
+          @ui.should_receive(:system).with("#{my_editor} #{@tempfile.path}").and_return(true)
+          IO.should_receive(:read).with(@tempfile.path).and_return(json_from_editor)
+        end
+
+        context "when parse_output is false" do
+          it "returns an edited pretty json string" do
+            expect(subject).to eql(json_from_editor)
+          end
+          it "the tempfile should have mode 0600" do
+            # XXX: this looks odd because we're really testing Tempfile.new here
+            expect(File.stat(@tempfile.path).mode & 0777).to eql(0600)
+            expect(subject).to eql(json_from_editor)
+          end
+        end
+
+        context "when parse_output is true" do
+          let(:parse_output) { true }
+          it "returns an edited ruby object" do
+            expect(subject).to eql(ruby_from_editor)
+          end
+          it "the tempfile should have mode 0600" do
+            # XXX: this looks odd because we're really testing Tempfile.new here
+            expect(File.stat(@tempfile.path).mode & 0777).to eql(0600)
+            expect(subject).to eql(ruby_from_editor)
+          end
+        end
+      end
+    end
+  end
+
   describe "format_list_for_display" do
     it "should print the full hash if --with-uri is true" do
       @ui.config[:with_uri] = true
diff --git a/spec/unit/knife/node_edit_spec.rb b/spec/unit/knife/node_edit_spec.rb
index e8f6937a31..7fbd8192d1 100644
--- a/spec/unit/knife/node_edit_spec.rb
+++ b/spec/unit/knife/node_edit_spec.rb
@@ -20,6 +20,12 @@ require 'spec_helper'
 Chef::Knife::NodeEdit.load_deps
 
 describe Chef::Knife::NodeEdit do
+
+  # helper to convert the view from Chef objects into Ruby objects representing JSON
+  def deserialized_json_view
+    actual = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json_pretty(@knife.node_editor.send(:view)))
+  end
+
   before(:each) do
     Chef::Config[:node_name]  = "webmonkey.example.com"
     @knife = Chef::Knife::NodeEdit.new
@@ -49,7 +55,7 @@ describe Chef::Knife::NodeEdit do
     end
 
     it "creates a view of the node without attributes from roles or ohai" do
-      actual = Chef::JSONCompat.from_json(@knife.node_editor.view)
+      actual = deserialized_json_view
       actual.should_not have_key("automatic")
       actual.should_not have_key("override")
       actual.should_not have_key("default")
@@ -61,7 +67,7 @@ describe Chef::Knife::NodeEdit do
     it "shows the extra attributes when given the --all option" do
       @knife.config[:all_attributes] = true
 
-      actual = Chef::JSONCompat.from_json(@knife.node_editor.view)
+      actual = deserialized_json_view
       actual["automatic"].should == {"go" => "away"}
       actual["override"].should == {"dont" => "show"}
       actual["default"].should == {"hide" => "me"}
@@ -71,18 +77,39 @@ describe Chef::Knife::NodeEdit do
     end
 
     it "does not consider unedited data updated" do
-      view = Chef::JSONCompat.from_json( @knife.node_editor.view )
-      @knife.node_editor.apply_updates(view)
+      view = deserialized_json_view
+      @knife.node_editor.send(:apply_updates, view)
       @knife.node_editor.should_not be_updated
     end
 
     it "considers edited data updated" do
-      view = Chef::JSONCompat.from_json( @knife.node_editor.view )
+      view = deserialized_json_view
       view["run_list"] << "role[fuuu]"
-      @knife.node_editor.apply_updates(view)
+      @knife.node_editor.send(:apply_updates, view)
       @knife.node_editor.should be_updated
     end
 
   end
+
+  describe "edit_node" do
+
+    before do
+      @knife.stub!(:node).and_return(@node)
+    end
+
+    let(:subject) { @knife.node_editor.edit_node }
+
+    it "raises an exception when editing is disabled" do
+      @knife.config[:disable_editing] = true
+      expect{ subject }.to raise_error(SystemExit)
+    end
+
+    it "raises an exception when the editor is not set" do
+      @knife.config[:editor] = nil
+      expect{ subject }.to raise_error(SystemExit)
+    end
+
+  end
+
 end