From 0e0047393f869330979b5e0ca205f4667476379e Mon Sep 17 00:00:00 2001 From: nfagerlund Date: Wed, 10 Aug 2011 16:39:59 -0700 Subject: (#3553) Explain that cron resources require time attributes The cron resource docs previously read, "All fields except the command and the user are optional, although specifying no periodic fields would result in the command being executed every minute." This was factually incorrect; instead, specifying no periodic fields results in a failure and an unhelpful error on Puppet 2.6 and 2.7. Although the issue will remain open as a behavior bug, this commit corrects the documentation of which attributes are required. It also changes the @doc string to a heredoc to simplify quote escaping. --- lib/puppet/type/cron.rb | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb index 4f6ea733c..5367517b2 100755 --- a/lib/puppet/type/cron.rb +++ b/lib/puppet/type/cron.rb @@ -3,11 +3,12 @@ require 'facter' require 'puppet/util/filetype' Puppet::Type.newtype(:cron) do - @doc = "Installs and manages cron jobs. All fields except the command - and the user are optional, although specifying no periodic - fields would result in the command being executed every - minute. While the name of the cron job is not part of the actual - job, it is used by Puppet to store and retrieve it. + @doc = <<-EOT + Installs and manages cron jobs. Every cron resource requires a command + and user attribute, as well as at least one periodic attribute (hour, + minute, month, monthday, weekday, or special). While the name of the cron + job is not part of the actual job, it is used by Puppet to store and + retrieve it. If you specify a cron job that matches an existing job in every way except name, then the jobs will be considered equivalent and the @@ -18,30 +19,30 @@ Puppet::Type.newtype(:cron) do Example: cron { logrotate: - command => \"/usr/sbin/logrotate\", + command => "/usr/sbin/logrotate", user => root, hour => 2, minute => 0 } - Note that all cron values can be specified as an array of values: + Note that all periodic attributes can be specified as an array of values: cron { logrotate: - command => \"/usr/sbin/logrotate\", + command => "/usr/sbin/logrotate", user => root, hour => [2, 4] } - Or using ranges, or the step syntax `*/2` (although there's no guarantee that - your `cron` daemon supports it): + ...or using ranges or the step syntax `*/2` (although there's no guarantee + that your `cron` daemon supports these): cron { logrotate: - command => \"/usr/sbin/logrotate\", + command => "/usr/sbin/logrotate", user => root, hour => ['2-4'], minute => '*/10' } - " + EOT ensurable # A base class for all of the Cron parameters, since they all have -- cgit From d7c9c765dbf28df3631e709832c44c343569cb53 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 10 Aug 2011 16:20:00 -0700 Subject: (#8740) Do not enumerate files in the root directory. Previously the command 'puppet resource file' would enumerate all files in the root directory, and generate an exception if the file type was not a directory, file, or link. Worse, it would also do this when a file or directory was specified, e.g. 'puppet resource file /etc/hosts'. Ideally, the find method of the ral terminus should not need to call the type's instances class method, instead just creating an instance of the type with the specified name and parameters. However, some types, like package, depend on this behavior. The type walks all providers and all instances that they provide, checking to see if the provider provides an instance with that name, and also warning if another provider provides an instance with the same name. Also, ideally, puppet should not blow up when encountering an unsupported file type, e.g. Unix domain socket, but that would be too big of a change for 2.6.x. This commit changes 'puppet resource file' to return a message saying that the operation is not supported: Listing all file instances is not supported. Please specify a file or directory, e.g. puppet resource file /etc The change is bit of a hack, as ideally, the file type's instances method could raise an exception when called in a 'search' context, but return an empty array in a 'find' context. But that also would be too big of a change for 2.6.x. This commit also adds spec tests for the resource application and file type, as well as an acceptance test, which creates a Unix domain socket in the root directory, while running 'puppet resource file'. Paired-with: Nick Lewis Reviewed-by: Jacob Helwig --- ...ket_8740_should_not_enumerate_root_directory.rb | 32 ++++++++++++++++++++++ lib/puppet/application/resource.rb | 3 ++ lib/puppet/type/file.rb | 4 +-- spec/unit/application/resource_spec.rb | 25 +++++++++++++++++ spec/unit/type/file_spec.rb | 6 ++++ 5 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 acceptance/tests/resource/file/ticket_8740_should_not_enumerate_root_directory.rb diff --git a/acceptance/tests/resource/file/ticket_8740_should_not_enumerate_root_directory.rb b/acceptance/tests/resource/file/ticket_8740_should_not_enumerate_root_directory.rb new file mode 100644 index 000000000..5763a3c98 --- /dev/null +++ b/acceptance/tests/resource/file/ticket_8740_should_not_enumerate_root_directory.rb @@ -0,0 +1,32 @@ +test_name "#8740: should not enumerate root directory" + +target = "/test-socket-#{$$}" + +step "clean up the system before we begin" +on(agents, "rm -f #{target}") + +step "create UNIX domain socket" +on(agents, %Q{ruby -e "require 'socket'; UNIXServer::new('#{target}').close"}) + +step "query for all files, which should return nothing" +on(agents, puppet_resource('file'), :acceptable_exit_codes => [1]) do + assert_match(%r{Listing all file instances is not supported. Please specify a file or directory, e.g. puppet resource file /etc}, stderr) +end + +["/", "/etc"].each do |file| + step "query '#{file}' directory, which should return single entry" + on(agents, puppet_resource('file', file)) do + files = stdout.scan(/^file \{ '([^']+)'/).flatten + + assert_equal(1, files.size, "puppet returned multiple files: #{files.join(', ')}") + assert_match(file, files[0], "puppet did not return file") + end +end + +step "query file that does not exist, which should report the file is absent" +on(agents, puppet_resource('file', '/this/does/notexist')) do + assert_match(/ensure\s+=>\s+'absent'/, stdout) +end + +step "remove UNIX domain socket" +on(agents, "rm -f #{target}") diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb index f55caa58a..bc4faf5e4 100644 --- a/lib/puppet/application/resource.rb +++ b/lib/puppet/application/resource.rb @@ -81,6 +81,9 @@ class Puppet::Application::Resource < Puppet::Application [ Puppet::Resource.new( type, name, :parameters => params ).save( key ) ] end else + if type == "file" + raise "Listing all file instances is not supported. Please specify a file or directory, e.g. puppet resource file /etc" + end Puppet::Resource.search( key, {} ) end.map(&format).join("\n") diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb index 630ebe5de..e91929cf8 100644 --- a/lib/puppet/type/file.rb +++ b/lib/puppet/type/file.rb @@ -310,8 +310,8 @@ Puppet::Type.newtype(:file) do super(path.gsub(/\/+/, '/').sub(/\/$/, '')) end - def self.instances(base = '/') - return self.new(:name => base, :recurse => true, :recurselimit => 1, :audit => :all).recurse_local.values + def self.instances + return [] end @depthfirst = false diff --git a/spec/unit/application/resource_spec.rb b/spec/unit/application/resource_spec.rb index b6c52b11e..9059a1fc5 100755 --- a/spec/unit/application/resource_spec.rb +++ b/spec/unit/application/resource_spec.rb @@ -230,4 +230,29 @@ describe Puppet::Application::Resource do end end + + describe "when handling file type" do + before :each do + Facter.stubs(:loadfacts) + @resource.preinit + end + + it "should raise an exception if no file specified" do + @resource.command_line.stubs(:args).returns(['file']) + + lambda { @resource.main }.should raise_error(RuntimeError, /Listing all file instances is not supported/) + end + + it "should output a file resource when given a file path" do + res = Puppet::Type.type(:file).new(:path => "/etc").to_resource + Puppet::Resource.expects(:find).returns(res) + + @resource.command_line.stubs(:args).returns(['file', '/etc']) + @resource.expects(:puts).with do |args| + args.should =~ /file \{ '\/etc'/m + end + + @resource.main + end + end end diff --git a/spec/unit/type/file_spec.rb b/spec/unit/type/file_spec.rb index 90f3daf09..81ccee821 100755 --- a/spec/unit/type/file_spec.rb +++ b/spec/unit/type/file_spec.rb @@ -1190,4 +1190,10 @@ describe Puppet::Type.type(:file) do @file[:checksum].should be :md5lite end end + + describe ".instances" do + it 'should return an empty array' do + Puppet::Type::File.instances.should == [] + end + end end -- cgit From 00c4b25010068eeb57a15104fb178a72af733fda Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Thu, 11 Aug 2011 15:47:37 -0700 Subject: (#8662) Migrate suidmanager test case to rspec We're trying to move away from the legacy Test::Unit tests, and toward rspec specs, so rewrite this file as specs. Reviewed-By: Jacob Helwig --- spec/unit/util/suidmanager_spec.rb | 113 ++++++++++++++++++++++++++++++++++ test/puppet/tc_suidmanager.rb | 120 ------------------------------------- 2 files changed, 113 insertions(+), 120 deletions(-) create mode 100755 spec/unit/util/suidmanager_spec.rb delete mode 100755 test/puppet/tc_suidmanager.rb diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb new file mode 100755 index 000000000..19ddc2b4d --- /dev/null +++ b/spec/unit/util/suidmanager_spec.rb @@ -0,0 +1,113 @@ +#!/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(:euid).returns(50) + Process.stubs(:egid).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 == 50 + 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(:euid).returns(50) + Process.stubs(:egid).returns(50) + + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} + + xids.should be_empty + 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(:groups=) + Process.expects(:euid).returns(99997) + Process.expects(:egid).returns(99996) + + Process.expects(:euid=).with(uid) + Process.expects(:egid=).with(gid) + + Kernel.expects(:system).with('blah') + Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) + + xids[:egid].should == 99996 + xids[:euid].should == 99997 + 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 - -- cgit From 2a0de12af5305ed54dd99391ee17533e71e0d88e Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Tue, 9 Aug 2011 17:48:33 -0700 Subject: (#8770) Always fully drop privileges when changing user On Mac OS X, it is only possible to directly change the euid of a process, and not the uid. Thus, when a puppet master started as root on OS X would change to the service user (puppet), it would leave the uid of its process set to 0. This allowed any type of Ruby plugin executed on the master (a type, provider, function, etc.) to trivially regain root privileges (by setting the euid of its process back to 0) and potentially compromise the master. Now, when permanently changing user, we will first try Process::UID.change_privilege, before falling back to setting the euid/uid ourselves. change_privilege correctly sets the uid of the process to the desired new uid, preventing the process from later escalating itself back to root. Similar behavior is also used when changing group. This has no effect on the behavior when temporarily changing user/group (for instance, to execute a single command or create a file as a particular user). Reviewed-By: Jacob Helwig --- lib/puppet/util.rb | 56 +++++++--------------- lib/puppet/util/suidmanager.rb | 54 +++++++++++++++------ spec/unit/util/suidmanager_spec.rb | 97 +++++++++++++++++++++++++++++++++----- 3 files changed, 142 insertions(+), 65 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 index 19ddc2b4d..f58f6c708 100755 --- a/spec/unit/util/suidmanager_spec.rb +++ b/spec/unit/util/suidmanager_spec.rb @@ -34,8 +34,11 @@ describe Puppet::Util::SUIDManager do it "should set euid/egid when root" do Process.stubs(:uid).returns(0) + Process.stubs(:egid).returns(51) Process.stubs(:euid).returns(50) - Process.stubs(:egid).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 @@ -44,7 +47,7 @@ describe Puppet::Util::SUIDManager do yielded = true end - xids[:egid].should == 50 + xids[:egid].should == 51 xids[:euid].should == 50 # It's possible asuser could simply not yield, so the assertions in the @@ -55,8 +58,8 @@ describe Puppet::Util::SUIDManager do 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) - Process.stubs(:egid).returns(50) Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} @@ -64,6 +67,76 @@ describe Puppet::Util::SUIDManager do 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 @@ -73,18 +146,20 @@ describe Puppet::Util::SUIDManager do describe "with #system" do it "should set euid/egid when root" do Process.stubs(:uid).returns(0) - Process.stubs(:groups=) - Process.expects(:euid).returns(99997) - Process.expects(:egid).returns(99996) + 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) - Process.expects(:euid=).with(uid) - Process.expects(:egid=).with(gid) + 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]) - - xids[:egid].should == 99996 - xids[:euid].should == 99997 end it "should not get or set euid/egid when not root" do -- cgit From bb224dd1549817190b6471e677e43fa02bb766a3 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Fri, 12 Aug 2011 12:18:51 -0700 Subject: (#8770) Don't fail to set supplementary groups when changing user to root Previously, Puppet::Util::SUIDManager.change_user would always try to set supplementary groups (Process.initgroups) before changing its EUID. Process.initgroups requires the calling process to have EUID 0 in order to succeed. This worked fine in the case where the process was changing from root to a normal user, as it would set groups as root and then change EUID to 0. However, in the case where the process was changing back to root from a normal user, it would attempt to set groups as the normal user, and fail. Now, we check Process.euid before changing, and will set groups first if root, and will set euid first if not root. This ensures we can freely switch back and forth between root. This behavior is maintained inside of the change_user, rather than being broken into eg. raise_privilege and lower_privilege, because it is a relatively minor behavior difference, and the helper methods on their own would not have been generically useful. --- lib/puppet/util/suidmanager.rb | 12 ++++++++++-- spec/unit/util/suidmanager_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb index 2e12b220f..697bce111 100644 --- a/lib/puppet/util/suidmanager.rb +++ b/lib/puppet/util/suidmanager.rb @@ -82,13 +82,21 @@ module Puppet::Util::SUIDManager begin Process::UID.change_privilege(uid) rescue NotImplementedError + # If changing uid, we must be root. So initgroups first here. initgroups(uid) Process.euid = uid Process.uid = uid end else - initgroups(uid) - Process.euid = uid + # If we're already root, initgroups before changing euid. If we're not, + # change euid (to root) first. + if Process.euid == 0 + initgroups(uid) + Process.euid = uid + else + Process.euid = uid + initgroups(uid) + end end end module_function :change_user diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb index f58f6c708..fc70e1718 100755 --- a/spec/unit/util/suidmanager_spec.rb +++ b/spec/unit/util/suidmanager_spec.rb @@ -134,6 +134,28 @@ describe Puppet::Util::SUIDManager do xids[:euid].should == 42 xids[:uid].should == 0 end + + it "should set euid before groups if changing to root" do + Process.stubs(:euid).returns 50 + + when_not_root = sequence 'when_not_root' + + Process.expects(:euid=).in_sequence(when_not_root) + Puppet::Util::SUIDManager.expects(:initgroups).in_sequence(when_not_root) + + Puppet::Util::SUIDManager.change_user(0, false) + end + + it "should set groups before euid if changing from root" do + Process.stubs(:euid).returns 0 + + when_root = sequence 'when_root' + + Puppet::Util::SUIDManager.expects(:initgroups).in_sequence(when_root) + Process.expects(:euid=).in_sequence(when_root) + + Puppet::Util::SUIDManager.change_user(50, false) + end end end -- cgit From 2bf6721a35a001c5b1ce2c6b9707ef28058919e9 Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Tue, 16 Aug 2011 17:31:12 -0700 Subject: Reset indirector state after configurer tests. Because the indirector state persists across tests, we need to make sure that we clean up after ourselves whenever we explicitly set a non-default configuration. We now reset the terminus class after all the tests have run in the context with the modified configuration. --- spec/unit/configurer_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb index f96876b58..29225130f 100755 --- a/spec/unit/configurer_spec.rb +++ b/spec/unit/configurer_spec.rb @@ -90,6 +90,11 @@ describe Puppet::Configurer do Puppet::Util::Log.stubs(:close) end + after :all do + Puppet::Node::Facts.indirection.reset_terminus_class + Puppet::Resource::Catalog.indirection.reset_terminus_class + end + it "should prepare for the run" do @agent.expects(:prepare) -- cgit From 35c10060ed647ba221289d1edf90ce0c587b6c74 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Wed, 17 Aug 2011 08:02:10 +0100 Subject: (#9039) Update Augeas commands documentation Added documentation on commands added as part of #6494 and clarified existing commands documentation. --- lib/puppet/type/augeas.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/puppet/type/augeas.rb b/lib/puppet/type/augeas.rb index f4d3c43e1..05dd42625 100644 --- a/lib/puppet/type/augeas.rb +++ b/lib/puppet/type/augeas.rb @@ -95,13 +95,18 @@ Puppet::Type.newtype(:augeas) do Commands supported are: set [PATH] [VALUE] Sets the value VALUE at loction PATH + setm [PATH] [SUB] [VALUE] Sets multiple nodes matching SUB relative to PATH, to VALUE rm [PATH] Removes the node at location PATH remove [PATH] Synonym for rm - clear [PATH] Keeps the node at PATH, but removes the value. + clear [PATH] Sets the node at PATH to NULL, creating it if needed ins [LABEL] [WHERE] [PATH] Inserts an empty node LABEL either [WHERE={before|after}] PATH. insert [LABEL] [WHERE] [PATH] Synonym for ins + mv [PATH] [PATH] Moves a node at PATH to the new location PATH + move [PATH] [PATH] Synonym for mv + defvar [NAME] [PATH] Sets Augeas variable $NAME to PATH + defnode [NAME] [PATH] [VALUE] Sets Augeas variable $NAME to PATH, creating it with VALUE if needed - If the parameter 'context' is set that value is prepended to PATH" + If the parameter 'context' is set that value is prepended to a relative PATH" end -- cgit From 7ac10930b30f13b5fdb7aea094283c8c122a0918 Mon Sep 17 00:00:00 2001 From: nfagerlund Date: Tue, 9 Aug 2011 17:33:14 -0700 Subject: (#8037) Fix incorrect example in Augeas type reference The changes attribute for the Augeas type's second example was incorrect, as it had leading slashes that took the paths out of the context of /files. This commit fixes the bad example, and changes the doc string to a heredoc to eliminate some messy escaping. --- lib/puppet/type/augeas.rb | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/puppet/type/augeas.rb b/lib/puppet/type/augeas.rb index f4d3c43e1..c96986e7e 100644 --- a/lib/puppet/type/augeas.rb +++ b/lib/puppet/type/augeas.rb @@ -20,7 +20,8 @@ Puppet::Type.newtype(:augeas) do feature :need_to_run?, "If the command should run" feature :execute_changes, "Actually make the changes" - @doc = "Apply the changes (single or array of changes) to the filesystem + @doc = <<-EOT + Apply the changes (single or array of changes) to the filesystem via the augeas tool. Requires: @@ -30,24 +31,24 @@ Puppet::Type.newtype(:augeas) do Sample usage with a string: - augeas{\"test1\" : - context => \"/files/etc/sysconfig/firstboot\", - changes => \"set RUN_FIRSTBOOT YES\", - onlyif => \"match other_value size > 0\", + augeas{"test1" : + context => "/files/etc/sysconfig/firstboot", + changes => "set RUN_FIRSTBOOT YES", + onlyif => "match other_value size > 0", } Sample usage with an array and custom lenses: - augeas{\"jboss_conf\": - context => \"/files\", + augeas{"jboss_conf": + context => "/files", changes => [ - \"set /etc/jbossas/jbossas.conf/JBOSS_IP $ipaddress\", - \"set /etc/jbossas/jbossas.conf/JAVA_HOME /usr\" + "set etc/jbossas/jbossas.conf/JBOSS_IP $ipaddress", + "set etc/jbossas/jbossas.conf/JAVA_HOME /usr", ], - load_path => \"$/usr/share/jbossas/lenses\", + load_path => "$/usr/share/jbossas/lenses", } - " + EOT newparam (:name) do desc "The name of this task. Used for uniqueness" -- cgit From 5f22985fa68162a849f15e0ca8125b40529e5769 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 17 Aug 2011 14:03:48 -0700 Subject: maint: Fix order dependent test failure The spec tests failed when running spec/unit/face/node_spec.rb followed by spec/unit/ssl/certificate_request_spec.rb, because the clean action for the node face was leaving Puppet::SSL::Host.ca_location set to :local instead of its default :none state. This commit resets the ca_location back to :none in the top-level after :all block. --- spec/unit/face/node_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/unit/face/node_spec.rb b/spec/unit/face/node_spec.rb index 6f6edc6cc..4c6b80137 100755 --- a/spec/unit/face/node_spec.rb +++ b/spec/unit/face/node_spec.rb @@ -3,6 +3,10 @@ require 'spec_helper' require 'puppet/face' describe Puppet::Face[:node, '0.0.1'] do + after :all do + Puppet::SSL::Host.ca_location = :none + end + describe '#cleanup' do it "should clean everything" do { -- cgit