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 --- spec/unit/application/resource_spec.rb | 25 +++++++++++++++++++++++++ spec/unit/type/file_spec.rb | 6 ++++++ 2 files changed, 31 insertions(+) (limited to 'spec') 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 +++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100755 spec/unit/util/suidmanager_spec.rb (limited to 'spec') 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 -- 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 --- spec/unit/util/suidmanager_spec.rb | 97 +++++++++++++++++++++++++++++++++----- 1 file changed, 86 insertions(+), 11 deletions(-) (limited to 'spec') 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. --- spec/unit/util/suidmanager_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'spec') 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(+) (limited to 'spec') 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 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(+) (limited to 'spec') 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