diff options
author | Jacob Helwig <jacob@puppetlabs.com> | 2011-08-11 17:16:15 -0700 |
---|---|---|
committer | Jacob Helwig <jacob@puppetlabs.com> | 2011-08-11 17:16:15 -0700 |
commit | 7de5ee899621e3a799ca87988ac1d2498b19d09a (patch) | |
tree | ef89595e13a1df0035dc9456f5fa5895c48c9bd3 | |
parent | 75786ada2187283f21241a5377908ff3a8b3d694 (diff) | |
parent | 2a0de12af5305ed54dd99391ee17533e71e0d88e (diff) | |
download | puppet-7de5ee899621e3a799ca87988ac1d2498b19d09a.tar.gz puppet-7de5ee899621e3a799ca87988ac1d2498b19d09a.tar.xz puppet-7de5ee899621e3a799ca87988ac1d2498b19d09a.zip |
Merge remote-tracking branch 'nicklewis/ticket/2.6.x/8770' into 2.6.x
* nicklewis/ticket/2.6.x/8770:
(#8770) Always fully drop privileges when changing user
(#8662) Migrate suidmanager test case to rspec
-rw-r--r-- | lib/puppet/util.rb | 56 | ||||
-rw-r--r-- | lib/puppet/util/suidmanager.rb | 54 | ||||
-rwxr-xr-x | spec/unit/util/suidmanager_spec.rb | 188 | ||||
-rwxr-xr-x | test/puppet/tc_suidmanager.rb | 120 |
4 files changed, 244 insertions, 174 deletions
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb index 34c6ec1ed..72c1f5f91 100644 --- a/lib/puppet/util.rb +++ b/lib/puppet/util.rb @@ -46,35 +46,24 @@ module Util # Change the process to a different user def self.chuser if group = Puppet[:group] - group = self.gid(group) - raise Puppet::Error, "No such group #{Puppet[:group]}" unless group - unless Puppet::Util::SUIDManager.gid == group - begin - Puppet::Util::SUIDManager.egid = group - Puppet::Util::SUIDManager.gid = group - rescue => detail - Puppet.warning "could not change to group #{group.inspect}: #{detail}" - $stderr.puts "could not change to group #{group.inspect}" - - # Don't exit on failed group changes, since it's - # not fatal - #exit(74) - end + begin + Puppet::Util::SUIDManager.change_group(group, true) + rescue => detail + Puppet.warning "could not change to group #{group.inspect}: #{detail}" + $stderr.puts "could not change to group #{group.inspect}" + + # Don't exit on failed group changes, since it's + # not fatal + #exit(74) end end if user = Puppet[:user] - user = self.uid(user) - raise Puppet::Error, "No such user #{Puppet[:user]}" unless user - unless Puppet::Util::SUIDManager.uid == user - begin - Puppet::Util::SUIDManager.initgroups(user) - Puppet::Util::SUIDManager.uid = user - Puppet::Util::SUIDManager.euid = user - rescue => detail - $stderr.puts "Could not change to user #{user}: #{detail}" - exit(74) - end + begin + Puppet::Util::SUIDManager.change_user(user, true) + rescue => detail + $stderr.puts "Could not change to user #{user}: #{detail}" + exit(74) end end end @@ -89,18 +78,14 @@ module Util if useself Puppet::Util::Log.create( - :level => level, :source => self, - :message => args ) else Puppet::Util::Log.create( - :level => level, - :message => args ) end @@ -261,9 +246,6 @@ module Util Puppet.debug "Executing '#{str}'" end - arguments[:uid] = Puppet::Util::SUIDManager.convert_xid(:uid, arguments[:uid]) if arguments[:uid] - arguments[:gid] = Puppet::Util::SUIDManager.convert_xid(:gid, arguments[:gid]) if arguments[:gid] - if execution_stub = Puppet::Util::ExecutionStub.current_value return execution_stub.call(command, arguments) end @@ -305,14 +287,8 @@ module Util $stderr.reopen(error_file) 3.upto(256){|fd| IO::new(fd).close rescue nil} - if arguments[:gid] - Process.egid = arguments[:gid] - Process.gid = arguments[:gid] unless @@os == "Darwin" - end - if arguments[:uid] - Process.euid = arguments[:uid] - Process.uid = arguments[:uid] unless @@os == "Darwin" - end + Puppet::Util::SUIDManager.change_group(arguments[:gid], true) if arguments[:gid] + Puppet::Util::SUIDManager.change_user(arguments[:uid], true) if arguments[:uid] ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C' if command.is_a?(Array) Kernel.exec(*command) diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb index 6633de002..2e12b220f 100644 --- a/lib/puppet/util/suidmanager.rb +++ b/lib/puppet/util/suidmanager.rb @@ -36,12 +36,6 @@ module Puppet::Util::SUIDManager end module_function :groups= - if Facter['kernel'].value == 'Darwin' - # Cannot change real UID on Darwin so we set euid - alias :uid :euid - alias :gid :egid - end - def self.root? Process.uid == 0 end @@ -50,23 +44,55 @@ module Puppet::Util::SUIDManager def asuser(new_uid=nil, new_gid=nil) return yield if Puppet.features.microsoft_windows? or !root? - # We set both because some programs like to drop privs, i.e. bash. - old_uid, old_gid = self.uid, self.gid old_euid, old_egid = self.euid, self.egid - old_groups = self.groups begin - self.egid = convert_xid :gid, new_gid if new_gid - self.initgroups(convert_xid(:uid, new_uid)) if new_uid - self.euid = convert_xid :uid, new_uid if new_uid + change_group(new_gid) if new_gid + change_user(new_uid) if new_uid yield ensure - self.euid, self.egid = old_euid, old_egid - self.groups = old_groups + change_group(old_egid) + change_user(old_euid) end end module_function :asuser + def change_group(group, permanently=false) + gid = convert_xid(:gid, group) + raise Puppet::Error, "No such group #{group}" unless gid + + if permanently + begin + Process::GID.change_privilege(gid) + rescue NotImplementedError + Process.egid = gid + Process.gid = gid + end + else + Process.egid = gid + end + end + module_function :change_group + + def change_user(user, permanently=false) + uid = convert_xid(:uid, user) + raise Puppet::Error, "No such user #{user}" unless uid + + if permanently + begin + Process::UID.change_privilege(uid) + rescue NotImplementedError + initgroups(uid) + Process.euid = uid + Process.uid = uid + end + else + initgroups(uid) + Process.euid = uid + end + end + module_function :change_user + # Make sure the passed argument is a number. def convert_xid(type, id) map = {:gid => :group, :uid => :user} diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb new file mode 100755 index 000000000..f58f6c708 --- /dev/null +++ b/spec/unit/util/suidmanager_spec.rb @@ -0,0 +1,188 @@ +#!/usr/bin/env rspec + +require 'spec_helper' + +describe Puppet::Util::SUIDManager do + let :user do + Puppet::Type.type(:user).new(:name => 'name', :uid => 42, :gid => 42) + end + + let :xids do + Hash.new {|h,k| 0} + end + + before :each do + Puppet::Util::SUIDManager.stubs(:convert_xid).returns(42) + Puppet::Util::SUIDManager.stubs(:initgroups) + + [:euid, :egid, :uid, :gid, :groups].each do |id| + Process.stubs("#{id}=").with {|value| xids[id] = value} + end + end + + describe "#uid" do + it "should allow setting euid/egid" do + Puppet::Util::SUIDManager.egid = user[:gid] + Puppet::Util::SUIDManager.euid = user[:uid] + + xids[:egid].should == user[:gid] + xids[:euid].should == user[:uid] + end + end + + describe "#asuser" do + it "should set euid/egid when root" do + Process.stubs(:uid).returns(0) + + Process.stubs(:egid).returns(51) + Process.stubs(:euid).returns(50) + + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:gid, 51).returns(51) + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:uid, 50).returns(50) + + yielded = false + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) do + xids[:egid].should == user[:gid] + xids[:euid].should == user[:uid] + yielded = true + end + + xids[:egid].should == 51 + xids[:euid].should == 50 + + # It's possible asuser could simply not yield, so the assertions in the + # block wouldn't fail. So verify those actually got checked. + yielded.should be_true + end + + it "should not get or set euid/egid when not root" do + Process.stubs(:uid).returns(1) + + Process.stubs(:egid).returns(51) + Process.stubs(:euid).returns(50) + + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} + + xids.should be_empty + end + end + + describe "#change_group" do + describe "when changing permanently" do + it "should try to change_privilege if it is supported" do + Process::GID.expects(:change_privilege).with do |gid| + Process.gid = gid + Process.egid = gid + end + + Puppet::Util::SUIDManager.change_group(42, true) + + xids[:egid].should == 42 + xids[:gid].should == 42 + end + + it "should change both egid and gid if change_privilege isn't supported" do + Process::GID.stubs(:change_privilege).raises(NotImplementedError) + + Puppet::Util::SUIDManager.change_group(42, true) + + xids[:egid].should == 42 + xids[:gid].should == 42 + end + end + + describe "when changing temporarily" do + it "should change only egid" do + Puppet::Util::SUIDManager.change_group(42, false) + + xids[:egid].should == 42 + xids[:gid].should == 0 + end + end + end + + describe "#change_user" do + describe "when changing permanently" do + it "should try to change_privilege if it is supported" do + Process::UID.expects(:change_privilege).with do |uid| + Process.uid = uid + Process.euid = uid + end + + Puppet::Util::SUIDManager.change_user(42, true) + + xids[:euid].should == 42 + xids[:uid].should == 42 + end + + it "should change euid and uid and groups if change_privilege isn't supported" do + Process::UID.stubs(:change_privilege).raises(NotImplementedError) + + Puppet::Util::SUIDManager.expects(:initgroups).with(42) + + Puppet::Util::SUIDManager.change_user(42, true) + + xids[:euid].should == 42 + xids[:uid].should == 42 + end + end + + describe "when changing temporarily" do + it "should change only euid and groups" do + Puppet::Util::SUIDManager.change_user(42, false) + + xids[:euid].should == 42 + xids[:uid].should == 0 + end + end + end + + describe "when running commands" do + before :each do + # We want to make sure $CHILD_STATUS is set + Kernel.system '' if $CHILD_STATUS.nil? + end + + describe "with #system" do + it "should set euid/egid when root" do + Process.stubs(:uid).returns(0) + Process.stubs(:egid).returns(51) + Process.stubs(:euid).returns(50) + + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:gid, 51).returns(51) + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:uid, 50).returns(50) + + Puppet::Util::SUIDManager.expects(:change_group).with(user[:uid]) + Puppet::Util::SUIDManager.expects(:change_user).with(user[:uid]) + + Puppet::Util::SUIDManager.expects(:change_group).with(51) + Puppet::Util::SUIDManager.expects(:change_user).with(50) + + Kernel.expects(:system).with('blah') + Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) + end + + it "should not get or set euid/egid when not root" do + Process.stubs(:uid).returns(1) + Kernel.expects(:system).with('blah') + + Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) + + xids.should be_empty + end + end + + describe "with #run_and_capture" do + it "should capture the output and return process status" do + Puppet::Util. + expects(:execute). + with('yay', :combine => true, :failonfail => false, :uid => user[:uid], :gid => user[:gid]). + returns('output') + output = Puppet::Util::SUIDManager.run_and_capture 'yay', user[:uid], user[:gid] + + output.first.should == 'output' + output.last.should be_a(Process::Status) + end + end + end +end diff --git a/test/puppet/tc_suidmanager.rb b/test/puppet/tc_suidmanager.rb deleted file mode 100755 index eeb56f2c9..000000000 --- a/test/puppet/tc_suidmanager.rb +++ /dev/null @@ -1,120 +0,0 @@ -#!/usr/bin/env ruby - -require File.dirname(__FILE__) + '/../lib/puppettest' - -require 'puppet' -require 'puppettest' -require 'test/unit' -require 'mocha' - -class TestSUIDManager < Test::Unit::TestCase - include PuppetTest - - def setup - the_id = 42 - Puppet::Util::SUIDManager.stubs(:convert_xid).returns(the_id) - Puppet::Util::SUIDManager.stubs(:initgroups) - @user = stub('user', :uid => the_id, :gid => the_id, :name => 'name') - super - end - - def test_metaprogramming_function_additions - # NOTE: the way that we are dynamically generating the methods in - # SUIDManager for the UID/GID calls was causing problems due to the - # modification of a closure. Should the bug rear itself again, this - # test will fail. - Process.expects(:uid).times(2) - - assert_nothing_raised do - Puppet::Util::SUIDManager.uid - Puppet::Util::SUIDManager.uid - end - end - - def test_id_set - Process.expects(:euid=).with(@user.uid) - Process.expects(:egid=).with(@user.gid) - - assert_nothing_raised do - Puppet::Util::SUIDManager.egid = @user.gid - Puppet::Util::SUIDManager.euid = @user.uid - end - end - - def test_utiluid - assert_not_equal(nil, Puppet::Util.uid(nonrootuser.name)) - end - - def test_asuser_as_root - Process.stubs(:uid).returns(0) - expects_id_set_and_revert @user.uid, @user.gid - Puppet::Util::SUIDManager.asuser @user.uid, @user.gid do end - rescue Errno::EPERM - end - - def test_asuser_as_nonroot - Process.stubs(:uid).returns(1) - expects_no_id_set - Puppet::Util::SUIDManager.asuser @user.uid, @user.gid do end - end - - - def test_system_as_root - Process.stubs(:uid).returns(0) - set_exit_status! - expects_id_set_and_revert @user.uid, @user.gid - Kernel.expects(:system).with('blah') - Puppet::Util::SUIDManager.system('blah', @user.uid, @user.gid) - end - - def test_system_as_nonroot - Process.stubs(:uid).returns(1) - set_exit_status! - expects_no_id_set - Kernel.expects(:system).with('blah') - Puppet::Util::SUIDManager.system('blah', @user.uid, @user.gid) - end - - def test_run_and_capture - if (RUBY_VERSION <=> "1.8.4") < 0 - warn "Cannot run this test on ruby < 1.8.4" - else - set_exit_status! - Puppet::Util. - expects(:execute). - with('yay',:combine => true, :failonfail => false, :uid => @user.uid, :gid => @user.gid). - returns('output') - output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid - - assert_equal 'output', output.first - assert_kind_of Process::Status, output.last - end - end - - private - - def expects_id_set_and_revert(uid, gid) - Process.stubs(:groups=) - Process.expects(:euid).returns(99997) - Process.expects(:egid).returns(99996) - - Process.expects(:euid=).with(uid) - Process.expects(:egid=).with(gid) - - Process.expects(:euid=).with(99997) - Process.expects(:egid=).with(99996) - end - - def expects_no_id_set - Process.expects(:egid).never - Process.expects(:euid).never - Process.expects(:egid=).never - Process.expects(:euid=).never - end - - def set_exit_status! - # We want to make sure $CHILD_STATUS is set, this is the only way I know how. - Kernel.system '' if $CHILD_STATUS.nil? - end -end - |