(puppetlabs/puppet) (#14283) Fix suntab filetype when run as normal user

[weareth...@puppetlabs.com]

Please review pull request #830: (#14283) Fix suntab filetype when run as normal user opened by (stschulte)

Description:

Running puppet as an unpriviledged user to manage its own crontab caused
two problems under solaris:

The uid argument was passed to the execute method to set the correct uid
before executing the crontab command. The execute method has a default
argumenthash with the key :failonfail set to true. Because the suntab
filetype passed it's own hash, failonfail was nil so if crontab
completes with a non-zero exit code no error will be raised.

This is bad because running execute with :uid not only sets the correct
userid in the child process but also tries to run Process.initgroups
to set the correct supplementary groups of that user. This call will most
likely fail as a non priviledged user causing the whole child process to fail.

The fix now makes sure that failonfail is passed to the execute method
so failures can be handled correctly and uid is only passed if necessary
(which is not the case when the target user in the crontab resource is
the same user that is running puppet).

• Opened: Fri Jun 01 21:01:06 UTC 2012
• Based on: puppetlabs:2.7.x (401aa7fac2d3d55ab9270ab5cfb6e720fff95b28)
• Requested merge: stschulte:ticket/2.7.x/14283 (446e78091c6cdaf1daf8df2ae7c6f3e26fe2fc11)

Diff follows:

diff --git a/lib/puppet/util/filetype.rb b/lib/puppet/util/filetype.rb
index fdc6ccd..1c8a893 100755
--- a/lib/puppet/util/filetype.rb
+++ b/lib/puppet/util/filetype.rb
@@ -202,9 +202,10 @@ def cmdbase
   # SunOS has completely different cron commands; this class implements
   # its versions.
   newfiletype(:suntab) do
+
     # Read a specific @path's cron tab.
     def read
-        output = Puppet::Util.execute(%w{crontab -l}, :uid => @path)
+        output = Puppet::Util.execute(%w{crontab -l}, cronargs)
         return "" if output.include?("can't open your crontab")
         raise Puppet::Error, "User #{@path} not authorized to use cron" if output.include?("you are not authorized to use cron")
         return output
@@ -214,7 +215,7 @@ def read
 
     # Remove a specific @path's cron tab.
     def remove
-        Puppet::Util.execute(%w{crontab -r}, :uid => @path)
+        Puppet::Util.execute(%w{crontab -r}, cronargs)
     rescue => detail
         raise Puppet::Error, "Could not remove crontab for #{@path}: #{detail}"
     end
@@ -224,20 +225,32 @@ def remove
     def write(text)
       puts text
       require "tempfile"
-      output_file = Tempfile.new("puppet")
-      fh = output_file.open
-      fh.print text
-      fh.close
-
-      # We have to chown the stupid file to the user.
-      File.chown(Puppet::Util.uid(@path), nil, output_file.path)
+      output_file = Tempfile.new("puppet_suntab")
 
       begin
-        Puppet::Util.execute(["crontab", output_file.path], :uid => @path)
+        output_file.print text
+        output_file.close
+        # We have to chown the stupid file to the user.
+        File.chown(Puppet::Util.uid(@path), nil, output_file.path)
+        Puppet::Util.execute(["crontab", output_file.path], cronargs)
       rescue => detail
         raise Puppet::Error, "Could not write crontab for #{@path}: #{detail}"
+      ensure
+        output_file.close!
+      end
+    end
+
+    private
+
+    # Arguments that will be passed to the execute method. Will set the uid
+    # to the target user if the target user and the current user are not
+    # the same
+    def cronargs
+      if uid = Puppet::Util.uid(@path) and uid == Puppet::Util::SUIDManager.uid
+        {:failonfail => true, :combine => true}
+      else
+        {:failonfail => true, :combine => true, :uid => @path}
       end
-      output_file.delete
     end
   end
 
@@ -263,20 +276,18 @@ def remove
     # and the text with which to create the cron tab.
     def write(text)
       require "tempfile"
-      output_file = Tempfile.new("puppet")
-      fh = output_file.open
-      fh.print text
-      fh.close
-
-      # We have to chown the stupid file to the user.
-      File.chown(Puppet::Util.uid(@path), nil, output_file.path)
+      output_file = Tempfile.new("puppet_aixtab")
 
       begin
+        output_file.print text
+        output_file.close
+        # We have to chown the stupid file to the user.
+        File.chown(Puppet::Util.uid(@path), nil, output_file.path)
         Puppet::Util.execute(["crontab", output_file.path], :uid => @path)
       rescue => detail
         raise Puppet::Error, "Could not write crontab for #{@path}: #{detail}"
       ensure
-        output_file.delete
+        output_file.close!
       end
     end
   end
diff --git a/spec/fixtures/unit/util/filetype/suntab_output b/spec/fixtures/unit/util/filetype/suntab_output
new file mode 100644
index 0000000..e6ca376
--- /dev/null
+++ b/spec/fixtures/unit/util/filetype/suntab_output
@@ -0,0 +1,9 @@
+#ident  "@(#)root       1.19    98/07/06 SMI"   /* SVr4.0 1.1.3.1       */
+#
+# The root crontab should be used to perform accounting data collection.
+#
+#
+10 3 * * * /usr/sbin/logadm
+15 3 * * 0 /usr/lib/fs/nfs/nfsfind
+30 3 * * * [ -x /usr/lib/gss/gsscred_clean ] && /usr/lib/gss/gsscred_clean
+#10 3 * * * /usr/lib/krb5/kprop_script ___slave_kdcs___
diff --git a/spec/unit/util/filetype_spec.rb b/spec/unit/util/filetype_spec.rb
index a2c0da6..b3de3e7 100755
--- a/spec/unit/util/filetype_spec.rb
+++ b/spec/unit/util/filetype_spec.rb
@@ -36,6 +36,90 @@
     end
   end
 
+  describe "the suntab filetype" do
+    before :each do
+      @type = Puppet::Util::FileType.filetype(:suntab)
+      @cron = @type.new('no_such_user')
+    end
+
+    let :suntab do
+      File.read(my_fixture('suntab_output'))
+    end
+
+    it "should exist" do
+      @type.should_not be_nil
+    end
+
+    describe "#read" do
+      it "should run crontab -l as the target user" do
+        Puppet::Util.expects(:execute).with(['crontab', '-l'], :failonfail => true, :combine => true, :uid => 'no_such_user').returns suntab
+        @cron.read.should == suntab
+      end
+
+      it "should not switch user if current user is the target user" do
+        Puppet::Util.expects(:uid).with('no_such_user').returns 9000
+        Puppet::Util::SUIDManager.expects(:uid).returns 9000
+        Puppet::Util.expects(:execute).with(['crontab', '-l'], :failonfail => true, :combine => true).returns suntab
+        @cron.read.should == suntab
+      end
+
+      # possible crontab output was taken from here:
+      # http://docs.oracle.com/cd/E19082-01/819-2380/sysrescron-60/index.html
+      it "should treat an absent crontab as empty" do
+        Puppet::Util.expects(:execute).with(['crontab', '-l'], :failonfail => true, :combine => true, :uid => 'no_such_user').returns 'crontab: can\'t open your crontab file'
+        @cron.read.should == ''
+      end
+
+      it "should raise an error if the user is not authorized to use cron" do
+        Puppet::Util.expects(:execute).with(['crontab', '-l'], :failonfail => true, :combine => true, :uid => 'no_such_user').returns 'crontab: you are not authorized to use cron. Sorry.'
+        expect { @cron.read }.to raise_error Puppet::Error, /User no_such_user not authorized to use cron/
+      end
+    end
+
+    describe "#remove" do
+      it "should run crontab -r as the target user" do
+        Puppet::Util.expects(:execute).with(['crontab', '-r'], :failonfail => true, :combine => true, :uid => 'no_such_user')
+        @cron.remove
+      end
+
+      it "should not switch user if current user is the target user" do
+        Puppet::Util.expects(:uid).with('no_such_user').returns 9000
+        Puppet::Util::SUIDManager.expects(:uid).returns 9000
+        Puppet::Util.expects(:execute).with(['crontab','-r'], :failonfail => true, :combine => true)
+        @cron.remove
+      end
+    end
+
+    describe "#write" do
+      before :each do
+        @tmp_cron = Tempfile.new("puppet_suntab_spec")
+        @tmp_cron_path = @tmp_cron.path
+        Puppet::Util.stubs(:uid).with('no_such_user').twice.returns 9000
+        Tempfile.expects(:new).with("puppet_suntab").returns @tmp_cron
+      end
+
+      after :each do
+        File.should_not be_exist @tmp_cron_path
+      end
+
+      it "should run crontab as the target user on a temporary file" do
+        File.expects(:chown).with(9000, nil, @tmp_cron_path)
+        Puppet::Util.expects(:execute).with(["crontab", @tmp_cron_path], :failonfail => true, :combine => true, :uid => 'no_such_user')
+
+        @tmp_cron.expects(:print).with("foo\n")
+        @cron.write "foo\n"
+      end
+
+      it "should not switch user if current user is the target user" do
+        Puppet::Util::SUIDManager.expects(:uid).returns 9000
+        File.expects(:chown).with(9000, nil, @tmp_cron_path)
+        Puppet::Util.expects(:execute).with(["crontab", @tmp_cron_path], :failonfail => true, :combine => true)
+        @tmp_cron.expects(:print).with("foo\n")
+        @cron.write "foo\n"
+      end
+    end
+  end
+
   describe "the flat filetype" do
     before do
       @type = Puppet::Util::FileType.filetype(:flat)