From 781394b58bc71fb114426298b6aa2b345ae671cb Mon Sep 17 00:00:00 2001 From: Matthaus Litteken Date: Tue, 14 Feb 2012 23:30:13 -0800 Subject: [PATCH] 2.6.x Fixes for CVE-2012-1053 and CVE-2012-1054 * ade5965 Remove unnecessary fallbacks in change_{user,group} * 0a09a64 Document uid/gid-related methods in Puppet::Util * 2599d56 Copy owner/group in replace_file * ead36ff (#12463) eliminate `secure_open` in favour of `replace_file` * 1469538 (#12460) use `replace_file` for the .k5login file * 8461203 (#12462) user_role_add: use `replace_file` for /etc/shadow * 0ad532a (#12463) add secure `replace_file` to Puppet::Util * 76d0749 (#12459) drop supplementary groups when permanently dropping UID * 50909b9 (#12458) default to users primary group, not root, in `asuser` * d00c5cc (#12457) add users primary group, not Process.gid, in initgroups commit ade5965e4e4d31e7b554372a96d599804b3e6ee7 Author: Nick Lewis Date: Mon Feb 13 15:36:33 2012 -0800 Remove unnecessary fallbacks in change_{user,group} These methods were attempting to call change_privilege, and rescuing NotImplementedError, to use our own implementation of similar behavior. As it turns out, the only system on which change_privilege is not implemented is Windows, which these methods already don't support. So there's no reason to have a fallback that would only ever fail or not be reached. commit 0a09a64457ef37f97bbe493f2fd90aca5ca73746 Author: Nick Lewis Date: Mon Feb 13 13:03:52 2012 -0800 Document uid/gid-related methods in Puppet::Util commit 2599d563ff290447653dca5e3927511b3a911422 Author: Nick Lewis Date: Mon Feb 13 13:02:30 2012 -0800 Copy owner/group in replace_file If the file doesn't exist, this will still use the current user/group as the uid/gid of the file. commit ead36ff3a8b4f37f1f0b4d0afed5b6d7c0bd15b9 Author: Daniel Pittman Date: Mon Feb 6 14:37:06 2012 -0800 (#12463) eliminate `secure_open` in favour of `replace_file` None of the users of `secure_open` in the codebase actually want to achieve a result different from `replace_file`, so we might as well eliminate the older API in favour of one that is easier to use and clearer in intent. Signed-off-by: Daniel Pittman commit 146953861e007a91545fdbd0ea59ab3509326e09 Author: Daniel Pittman Date: Mon Feb 6 11:03:16 2012 -0800 (#12460) use `replace_file` for the .k5login file Previously this hand-coded replacing the .k5login file; Replacing that with the standard API ensures consistent, safe, and atomic updates of the target file. It also reduces the cognitive burden of the provider, since the semantics of the standard method are clearer than the semantics of the type. Finally, this commit adds some tests for the type, and the inline provider, to help ensure that the type continues to work into the future. Signed-off-by: Daniel Pittman commit 8461203aa7ebfb083bce605bd44c2f579342f5bb Author: Daniel Pittman Date: Sun Feb 5 21:16:29 2012 -0800 (#12462) user_role_add: use `replace_file` for /etc/shadow The user_role_add provider for user management previously open-coded the safe, atomic replacement of /etc/shadow after in modified it. Mostly even safely, except that it didn't enforce permissions on the temporary file, so might have spilled data to third parties, if tempfile isn't sufficiently secure. (Ruby makes no promises about tempfile mode.) We replace that with the standard, central replace_file API, which ensures that is done in a safe, correct, and standard fashion. On the way through this removes the window where this could previously have lost the content of /etc/shadow due to an unfortunately timed crash, by ensuring we fsync the content of the temporary file. It also documents, but doesn't eliminate, the lack of locking and other races around this update to the shadow file. Finally, some minor code and test updates to make it easier to test this on fake data, rather than like on your own /etc/shadow file... Signed-off-by: Daniel Pittman commit 0ad532a4b26ea54455d77791006324b684cf5b10 Author: Daniel Pittman Date: Sun Feb 5 20:45:36 2012 -0800 (#12463) add secure `replace_file` to Puppet::Util There is some complex logic around replacing an existing file safely, and it used to be scattered all over the tree - making the whole system much harder to use nicely and pleasantly. It is much better that we provide a central, and easy to use, method for achieving that specific and common goal in a consistent, secure, and easy to audit fashion. Based on code submitted by me on 2010-04-08. Signed-off-by: Daniel Pittman commit 76d0749f0a9a496b70e7dc7e6d6d6ff692224e36 Author: Daniel Pittman Date: Sun Feb 5 15:58:24 2012 -0800 (#12459) drop supplementary groups when permanently dropping UID On certain platforms, Process::UID.change_privilege doesn't handle changing supplementary groups. In that case, we need to always explicitly initgroups, regardless of whether we're using change_privilege, or change_group/change_user. Analysis by Dominic Cleal Signed-off-by: Daniel Pittman commit 50909b93dc01b735c1aa104fd568d71b0760ccd9 Author: Daniel Pittman Date: Sun Feb 5 15:46:31 2012 -0800 (#12458) default to users primary group, not root, in `asuser` When only a user (no group) was given to the asuser method, the egid wouldn't be changed and would remain the privileged gid. This has been changed to the primary gid of the user. Fix and analysis by Dominic Cleal Contributed: Daniel Pittman Contributed: Deepak Giridharagopal Contributed: Nick Lewis commit d00c5cc45e3ccf8c80535d372027dca8452d1056 Author: Daniel Pittman Date: Sun Feb 5 12:09:37 2012 -0800 (#12457) add users primary group, not Process.gid, in initgroups The Process.gid (real gid) was always included when initialising supplementary groups in the initgroups method (called when changing the euid via the change_user method). This has been replaced by the primary gid of the user. This led to a privilege leak, as well as a potential surprise when it came to file ownership, in both the agent and the master. Fix and analysis by Dominic Cleal Signed-off-by: Daniel Pittman --- lib/puppet/daemon.rb | 4 +- lib/puppet/network/server.rb | 2 +- lib/puppet/provider/user/user_role_add.rb | 49 +++++--- lib/puppet/rails/benchmark.rb | 2 +- lib/puppet/type/k5login.rb | 5 +- lib/puppet/util.rb | 100 ++++++++++++---- lib/puppet/util/reference.rb | 15 ++- lib/puppet/util/suidmanager.rb | 85 ++++++++------ spec/unit/provider/user/user_role_add_spec.rb | 59 ++++++++-- spec/unit/type/k5login_spec.rb | 115 ++++++++++++++++++ spec/unit/util/suidmanager_spec.rb | 154 +++++++++++++------------ spec/unit/util_spec.rb | 104 +++++++++++++++++ 12 files changed, 523 insertions(+), 171 deletions(-) mode change 100644 => 100755 spec/unit/provider/user/user_role_add_spec.rb create mode 100755 spec/unit/type/k5login_spec.rb create mode 100755 spec/unit/util_spec.rb diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb index 22630ff..2632bc2 100755 --- a/lib/puppet/daemon.rb +++ b/lib/puppet/daemon.rb @@ -33,9 +33,9 @@ class Puppet::Daemon Puppet::Util::Log.reopen rescue => detail Puppet.err "Could not start #{Puppet[:name]}: #{detail}" - Puppet::Util::secure_open("/tmp/daemonout", "w") { |f| + Puppet::Util::replace_file("/tmp/daemonout", 0644) do |f| f.puts "Could not start #{Puppet[:name]}: #{detail}" - } + end exit(12) end end diff --git a/lib/puppet/network/server.rb b/lib/puppet/network/server.rb index e4de07d..80987e4 100644 --- a/lib/puppet/network/server.rb +++ b/lib/puppet/network/server.rb @@ -22,7 +22,7 @@ class Puppet::Network::Server $stderr.reopen $stdout Puppet::Util::Log.reopen rescue => detail - Puppet::Util.secure_open("/tmp/daemonout", "w") { |f| + Puppet::Util.replace_file("/tmp/daemonout", 0644) { |f| f.puts "Could not start #{Puppet[:name]}: #{detail}" } raise "Could not start #{Puppet[:name]}: #{detail}" diff --git a/lib/puppet/provider/user/user_role_add.rb b/lib/puppet/provider/user/user_role_add.rb index 2377f9e..e562d68 100644 --- a/lib/puppet/provider/user/user_role_add.rb +++ b/lib/puppet/provider/user/user_role_add.rb @@ -1,3 +1,4 @@ +require 'puppet/util' require 'puppet/util/user_attr' Puppet::Type.type(:user).provide :user_role_add, :parent => :useradd, :source => :useradd do @@ -145,11 +146,22 @@ Puppet::Type.type(:user).provide :user_role_add, :parent => :useradd, :source => run([command(:modify)] + build_keys_cmd(keys_hash) << @resource[:name], "modify attribute key pairs") end + + # This helper makes it possible to test this on stub data without having to + # do too many crazy things! + def target_file_path + "/etc/shadow" + end + private :target_file_path + #Read in /etc/shadow, find the line for this user (skipping comments, because who knows) and return it #No abstraction, all esoteric knowledge of file formats, yay def shadow_entry return @shadow_entry if defined? @shadow_entry - @shadow_entry = File.readlines("/etc/shadow").reject { |r| r =~ /^[^\w]/ }.collect { |l| l.chomp.split(':') }.find { |user, _| user == @resource[:name] } + @shadow_entry = File.readlines(target_file_path). + reject { |r| r =~ /^[^\w]/ }. + collect { |l| l.chomp.split(':') }. + find { |user, _| user == @resource[:name] } end def password @@ -164,28 +176,31 @@ Puppet::Type.type(:user).provide :user_role_add, :parent => :useradd, :source => shadow_entry ? shadow_entry[4] : :absent end - #Read in /etc/shadow, find the line for our used and rewrite it with the new pw - #Smooth like 80 grit + # Read in /etc/shadow, find the line for our used and rewrite it with the + # new pw. Smooth like 80 grit sandpaper. + # + # Now uses the `replace_file` mechanism to minimize the chance that we lose + # data, but it is still terrible. We still skip platform locking, so a + # concurrent `vipw -s` session will have no idea we risk data loss. def password=(cryptopw) begin - File.open("/etc/shadow", "r") do |shadow| - File.open("/etc/shadow_tmp", "w", 0600) do |shadow_tmp| - while line = shadow.gets - line_arr = line.split(':') - if line_arr[0] == @resource[:name] - line_arr[1] = cryptopw - line = line_arr.join(':') - end - shadow_tmp.print line + shadow = File.read(target_file_path) + + # Go Mifune loves the race here where we can lose data because + # /etc/shadow changed between reading it and writing it. + # --daniel 2012-02-05 + Puppet::Util.replace_file(target_file_path, 0640) do |fh| + shadow.each_line do |line| + line_arr = line.split(':') + if line_arr[0] == @resource[:name] + line_arr[1] = cryptopw + line = line_arr.join(':') end + fh.print line end end - File.rename("/etc/shadow_tmp", "/etc/shadow") rescue => detail - fail "Could not write temporary shadow file: #{detail}" - ensure - # Make sure this *always* gets deleted - File.unlink("/etc/shadow_tmp") if File.exist?("/etc/shadow_tmp") + fail "Could not write replace #{target_file_path}: #{detail}" end end end diff --git a/lib/puppet/rails/benchmark.rb b/lib/puppet/rails/benchmark.rb index 375674e..e1e92bb 100644 --- a/lib/puppet/rails/benchmark.rb +++ b/lib/puppet/rails/benchmark.rb @@ -58,6 +58,6 @@ module Puppet::Rails::Benchmark data = {} end data[branch] = $benchmarks - Puppet::Util.secure_open(file, "w") { |f| f.print YAML.dump(data) } + Puppet::Util.replace_file(file, 0644) { |f| f.print YAML.dump(data) } end end diff --git a/lib/puppet/type/k5login.rb b/lib/puppet/type/k5login.rb index 2e87ca9..4bc35df 100644 --- a/lib/puppet/type/k5login.rb +++ b/lib/puppet/type/k5login.rb @@ -1,4 +1,5 @@ # Plug-in type for handling k5login files +require 'puppet/util' Puppet::Type.newtype(:k5login) do @doc = "Manage the `.k5login` file for a user. Specify the full path to @@ -79,8 +80,8 @@ Puppet::Type.newtype(:k5login) do private def write(value) - Puppet::Util.secure_open(@resource[:name], "w") do |f| - f.puts value.join("\n") + Puppet::Util.replace_file(@resource[:name], 0644) do |f| + f.puts value end end end diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb index 72c1f5f..58cdc17 100644 --- a/lib/puppet/util.rb +++ b/lib/puppet/util.rb @@ -1,10 +1,11 @@ # A module to collect utility functions. - require 'puppet/util/monkey_patches' -require 'sync' require 'puppet/external/lock' -require 'monitor' require 'puppet/util/execution_stub' +require 'sync' +require 'monitor' +require 'tempfile' +require 'pathname' module Puppet # A command failed to execute. @@ -261,7 +262,6 @@ module Util output_file="/dev/null" error_file="/dev/null" if ! arguments[:squelch] - require "tempfile" output_file = Tempfile.new("puppet") error_file=output_file if arguments[:combine] end @@ -287,8 +287,7 @@ module Util $stderr.reopen(error_file) 3.upto(256){|fd| IO::new(fd).close rescue nil} - Puppet::Util::SUIDManager.change_group(arguments[:gid], true) if arguments[:gid] - Puppet::Util::SUIDManager.change_user(arguments[:uid], true) if arguments[:uid] + Puppet::Util::SUIDManager.change_privileges(arguments[:uid], arguments[:gid], true) ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C' if command.is_a?(Array) Kernel.exec(*command) @@ -409,27 +408,82 @@ module Util module_function :memory, :thinmark - def secure_open(file,must_be_w,&block) - raise Puppet::DevError,"secure_open only works with mode 'w'" unless must_be_w == 'w' - raise Puppet::DevError,"secure_open only requires a block" unless block_given? - Puppet.warning "#{file} was a symlink to #{File.readlink(file)}" if File.symlink?(file) - if File.exists?(file) or File.symlink?(file) - wait = File.symlink?(file) ? 5.0 : 0.1 - File.delete(file) - sleep wait # give it a chance to reappear, just in case someone is actively trying something. + # Replace a file, securely. This takes a block, and passes it the file + # handle of a file open for writing. Write the replacement content inside + # the block and it will safely replace the target file. + # + # This method will make no changes to the target file until the content is + # successfully written and the block returns without raising an error. + # + # As far as possible the state of the existing file, such as mode, is + # preserved. This works hard to avoid loss of any metadata, but will result + # in an inode change for the file. + # + # Arguments: `filename`, `default_mode` + # + # The filename is the file we are going to replace. + # + # The default_mode is the mode to use when the target file doesn't already + # exist; if the file is present we copy the existing mode/owner/group values + # across. + def replace_file(file, default_mode, &block) + raise Puppet::DevError, "replace_file requires a block" unless block_given? + + file = Pathname(file) + tempfile = Tempfile.new(file.basename.to_s, file.dirname.to_s) + + file_exists = file.exist? + + # If the file exists, use its current mode/owner/group. If it doesn't, use + # the supplied mode, and default to current user/group. + if file_exists + stat = file.lstat + + # We only care about the four lowest-order octets. Higher octets are + # filesystem-specific. + mode = stat.mode & 07777 + uid = stat.uid + gid = stat.gid + else + mode = default_mode + uid = Process.euid + gid = Process.egid end + + # Set properties of the temporary file before we write the content, because + # Tempfile doesn't promise to be safe from reading by other people, just + # that it avoids races around creating the file. + tempfile.chmod(mode) + tempfile.chown(uid, gid) + + # OK, now allow the caller to write the content of the file. + yield tempfile + + # Now, make sure the data (which includes the mode) is safe on disk. + tempfile.flush begin - File.open(file,File::CREAT|File::EXCL|File::TRUNC|File::WRONLY,&block) - rescue Errno::EEXIST - desc = File.symlink?(file) ? "symlink to #{File.readlink(file)}" : File.stat(file).ftype - puts "Warning: #{file} was apparently created by another process (as" - puts "a #{desc}) as soon as it was deleted by this process. Someone may be trying" - puts "to do something objectionable (such as tricking you into overwriting system" - puts "files if you are running as root)." - raise + tempfile.fsync + rescue NotImplementedError + # fsync may not be implemented by Ruby on all platforms, but + # there is absolutely no recovery path if we detect that. So, we just + # ignore the return code. + # + # However, don't be fooled: that is accepting that we are running in + # an unsafe fashion. If you are porting to a new platform don't stub + # that out. end + + tempfile.close + + File.rename(tempfile.path, file) + + # Ideally, we would now fsync the directory as well, but Ruby doesn't + # have support for that, and it doesn't matter /that/ much... + + # Return something true, and possibly useful. + file end - module_function :secure_open + module_function :replace_file end end diff --git a/lib/puppet/util/reference.rb b/lib/puppet/util/reference.rb index a4921ed..ab74b3c 100644 --- a/lib/puppet/util/reference.rb +++ b/lib/puppet/util/reference.rb @@ -36,14 +36,15 @@ class Puppet::Util::Reference def self.pdf(text) puts "creating pdf" - Puppet::Util.secure_open("/tmp/puppetdoc.txt", "w") do |f| - f.puts text - end - rst2latex = which('rst2latex') || which('rst2latex.py') || raise("Could not find rst2latex") + rst2latex = which('rst2latex') || which('rst2latex.py') || + raise("Could not find rst2latex") + cmd = %{#{rst2latex} /tmp/puppetdoc.txt > /tmp/puppetdoc.tex} - Puppet::Util.secure_open("/tmp/puppetdoc.tex","w") do |f| - # If we get here without an error, /tmp/puppetdoc.tex isn't a tricky cracker's symlink - end + Puppet::Util.replace_file("/tmp/puppetdoc.txt") {|f| f.puts text } + # There used to be an attempt to use secure_open / replace_file to secure + # the target, too, but that did nothing: the race was still here. We can + # get exactly the same benefit from running this effort: + File.unlink('/tmp/puppetdoc.tex') rescue nil output = %x{#{cmd}} unless $CHILD_STATUS == 0 $stderr.puts "rst2latex failed" diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb index 697bce1..d939155 100644 --- a/lib/puppet/util/suidmanager.rb +++ b/lib/puppet/util/suidmanager.rb @@ -1,5 +1,6 @@ require 'puppet/util/warnings' require 'forwardable' +require 'etc' module Puppet::Util::SUIDManager include Puppet::Util::Warnings @@ -40,55 +41,76 @@ module Puppet::Util::SUIDManager Process.uid == 0 end - # Runs block setting uid and gid if provided then restoring original ids + # Methods to handle changing uid/gid of the running process. In general, + # these will noop or fail on Windows, and require root to change to anything + # but the current uid/gid (which is a noop). + + # Runs block setting euid and egid if provided then restoring original ids. + # If running on Windows or without root, the block will be run with the + # current euid/egid. def asuser(new_uid=nil, new_gid=nil) - return yield if Puppet.features.microsoft_windows? or !root? + return yield if Puppet.features.microsoft_windows? + return yield unless root? + return yield unless new_uid or new_gid old_euid, old_egid = self.euid, self.egid begin - change_group(new_gid) if new_gid - change_user(new_uid) if new_uid + change_privileges(new_uid, new_gid, false) yield ensure - change_group(old_egid) - change_user(old_euid) + change_privileges(new_uid ? old_euid : nil, old_egid, false) end end module_function :asuser + # If `permanently` is set, will permanently change the uid/gid of the + # process. If not, it will only set the euid/egid. If only uid is supplied, + # the primary group of the supplied gid will be used. If only gid is + # supplied, only gid will be changed. This method will fail if used on + # Windows. + def change_privileges(uid=nil, gid=nil, permanently=false) + return unless uid or gid + + unless gid + uid = convert_xid(:uid, uid) + gid = Etc.getpwuid(uid).gid + end + + change_group(gid, permanently) + change_user(uid, permanently) if uid + end + module_function :change_privileges + + # Changes the egid of the process if `permanently` is not set, otherwise + # changes gid. This method will fail if used on Windows, or attempting to + # change to a different gid without root. 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 + Process::GID.change_privilege(gid) else Process.egid = gid end end module_function :change_group + # As change_group, but operates on uids. If changing user permanently, + # supplementary groups will be set the to default groups for the new uid. 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 - # If changing uid, we must be root. So initgroups first here. - initgroups(uid) - Process.euid = uid - Process.uid = uid - end + # If changing uid, we must be root. So initgroups first here. + initgroups(uid) + + Process::UID.change_privilege(uid) else - # If we're already root, initgroups before changing euid. If we're not, + # We must be root to initgroups, so initgroups before dropping euid if + # we're root, otherwise elevate euid before initgroups. # change euid (to root) first. if Process.euid == 0 initgroups(uid) @@ -113,10 +135,13 @@ module Puppet::Util::SUIDManager end module_function :convert_xid - # Initialize supplementary groups - def initgroups(user) - require 'etc' - Process.initgroups(Etc.getpwuid(user).name, Process.gid) + # Initialize primary and supplemental groups to those of the target user. We + # take the UID and manually look up their details in the system database, + # including username and primary group. This method will fail on Windows, or + # if used without root to initgroups of another user. + def initgroups(uid) + pwent = Etc.getpwuid(uid) + Process.initgroups(pwent.name, pwent.gid) end module_function :initgroups @@ -126,15 +151,5 @@ module Puppet::Util::SUIDManager [output, $CHILD_STATUS.dup] end module_function :run_and_capture - - def system(command, new_uid=nil, new_gid=nil) - status = nil - asuser(new_uid, new_gid) do - Kernel.system(command) - status = $CHILD_STATUS.dup - end - status - end - module_function :system end diff --git a/spec/unit/provider/user/user_role_add_spec.rb b/spec/unit/provider/user/user_role_add_spec.rb old mode 100644 new mode 100755 index f739423..f374ced --- a/spec/unit/provider/user/user_role_add_spec.rb +++ b/spec/unit/provider/user/user_role_add_spec.rb @@ -1,10 +1,13 @@ #!/usr/bin/env ruby require File.dirname(__FILE__) + '/../../../spec_helper' +require 'tempfile' provider_class = Puppet::Type.type(:user).provider(:user_role_add) describe provider_class do + include PuppetSpec::Files + before do @resource = stub("resource", :name => "myuser", :managehome? => nil) @resource.stubs(:should).returns "fakeval" @@ -244,17 +247,51 @@ describe provider_class do end describe "when setting the password" do - #how can you mock these blocks up? - it "should open /etc/shadow for reading and /etc/shadow_tmp for writing" do - File.expects(:open).with("/etc/shadow", "r") - File.stubs(:rename) - @provider.password=("hashedpassword") - end - - it "should rename the /etc/shadow_tmp to /etc/shadow" do - File.stubs(:open).with("/etc/shadow", "r") - File.expects(:rename).with("/etc/shadow_tmp", "/etc/shadow") - @provider.password=("hashedpassword") + let(:path) { tmpfile('etc-shadow') } + + before :each do + @provider.stubs(:target_file_path).returns(path) + end + + def write_fixture(content) + File.open(path, 'w') { |f| f.print(content) } + end + + it "should update the target user" do + write_fixture < 'present', + :path => path, + :principals => 'fred@EXAMPLE.COM' + }.merge(attrs) + + if content = attrs.delete(:content) + File.open(path, 'w') { |f| f.print(content) } + end + + resource = described_class.new(attrs) + resource + end + + before :each do + FileUtils.touch(path) + end + + context "the provider" do + context "when the file is missing" do + it "should initially be absent" do + File.delete(path) + resource.retrieve[:ensure].must == :absent + end + + it "should create the file when synced" do + resource(:ensure => 'present').parameter(:ensure).sync + File.should be_exist path + end + end + + context "when the file is present" do + context "retrieved initial state" do + subject { resource.retrieve } + + it "should retrieve its properties correctly with zero principals" do + subject[:ensure].should == :present + subject[:principals].should == [] + # We don't really care what the mode is, just that it got it + subject[:mode].should_not be_nil + end + + context "with one principal" do + subject { resource(:content => "daniel@EXAMPLE.COM\n").retrieve } + + it "should retrieve its principals correctly" do + subject[:principals].should == ["daniel@EXAMPLE.COM"] + end + end + + context "with two principals" do + subject do + content = ["daniel@EXAMPLE.COM", "george@EXAMPLE.COM"].join("\n") + resource(:content => content).retrieve + end + + it "should retrieve its principals correctly" do + subject[:principals].should == ["daniel@EXAMPLE.COM", "george@EXAMPLE.COM"] + end + end + end + + it "should remove the file ensure is absent" do + resource(:ensure => 'absent').property(:ensure).sync + File.should_not be_exist path + end + + it "should write one principal to the file" do + File.read(path).should == "" + resource(:principals => ["daniel@EXAMPLE.COM"]).property(:principals).sync + File.read(path).should == "daniel@EXAMPLE.COM\n" + end + + it "should write multiple principals to the file" do + content = ["daniel@EXAMPLE.COM", "george@EXAMPLE.COM"] + + File.read(path).should == "" + resource(:principals => content).property(:principals).sync + File.read(path).should == content.join("\n") + "\n" + end + + describe "when setting the mode", :unless => Puppet.features.microsoft_windows? do + # The defined input type is "mode, as an octal string" + ["400", "600", "700", "644", "664"].each do |mode| + it "should update the mode to #{mode}" do + resource(:mode => mode).property(:mode).sync + + (File.stat(path).mode & 07777).to_s(8).should == mode + end + end + end + end + end +end diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb index fc70e17..62ee9bc 100755 --- a/spec/unit/util/suidmanager_spec.rb +++ b/spec/unit/util/suidmanager_spec.rb @@ -13,10 +13,18 @@ describe Puppet::Util::SUIDManager do before :each do Puppet::Util::SUIDManager.stubs(:convert_xid).returns(42) - Puppet::Util::SUIDManager.stubs(:initgroups) + pwent = stub('pwent', :name => 'fred', :uid => 42, :gid => 42) + Etc.stubs(:getpwuid).with(42).returns(pwent) [:euid, :egid, :uid, :gid, :groups].each do |id| - Process.stubs("#{id}=").with {|value| xids[id] = value} + Process.stubs("#{id}=").with {|value| xids[id] = value } + end + end + + describe "#initgroups" do + it "should use the primary group of the user as the 'basegid'" do + Process.expects(:initgroups).with('fred', 42) + described_class.initgroups(42) end end @@ -31,45 +39,93 @@ describe Puppet::Util::SUIDManager do end describe "#asuser" do - it "should set euid/egid when root" do - Process.stubs(:uid).returns(0) + 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.stubs(:convert_xid).with(:gid, 51).returns(51) - Puppet::Util::SUIDManager.stubs(:convert_xid).with(:uid, 50).returns(50) + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} + + xids.should be_empty + end - yielded = false - Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) do - xids[:egid].should == user[:gid] - xids[:euid].should == user[:uid] - yielded = true + context "when root and not windows" do + before :each do + Process.stubs(:uid).returns(0) + Puppet.features.stubs(:microsoft_windows?).returns(false) end - xids[:egid].should == 51 - xids[:euid].should == 50 + it "should set euid/egid when root" do + Process.stubs(:uid).returns(0) - # 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 + Process.stubs(:egid).returns(51) + Process.stubs(:euid).returns(50) - it "should not get or set euid/egid when not root" do - Process.stubs(:uid).returns(1) + 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.stubs(:initgroups).returns([]) - Process.stubs(:egid).returns(51) - Process.stubs(:euid).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 - Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} + xids[:egid].should == 51 + xids[:euid].should == 50 - xids.should be_empty + # 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 just yield if user and group are nil" do + yielded = false + Puppet::Util::SUIDManager.asuser(nil, nil) { yielded = true } + yielded.should be_true + xids.should == {} + end + + it "should just change group if only group is given" do + yielded = false + Puppet::Util::SUIDManager.asuser(nil, 42) { yielded = true } + yielded.should be_true + xids.should == { :egid => 42 } + end + + it "should change gid to the primary group of uid by default" do + Process.stubs(:initgroups) + + yielded = false + Puppet::Util::SUIDManager.asuser(42) { yielded = true } + yielded.should be_true + xids.should == { :euid => 42, :egid => 42 } + end + + it "should change both uid and gid if given" do + # I don't like the sequence, but it is the only way to assert on the + # internal behaviour in a reliable fashion, given we need multiple + # sequenced calls to the same methods. --daniel 2012-02-05 + horror = sequence('of user and group changes') + Puppet::Util::SUIDManager.expects(:change_group).with(43, false).in_sequence(horror) + Puppet::Util::SUIDManager.expects(:change_user).with(42, false).in_sequence(horror) + Puppet::Util::SUIDManager.expects(:change_group). + with(Puppet::Util::SUIDManager.egid, false).in_sequence(horror) + Puppet::Util::SUIDManager.expects(:change_user). + with(Puppet::Util::SUIDManager.euid, false).in_sequence(horror) + + yielded = false + Puppet::Util::SUIDManager.asuser(42, 43) { yielded = true } + yielded.should be_true + end end end describe "#change_group" do describe "when changing permanently" do - it "should try to change_privilege if it is supported" do + it "should change_privilege" do Process::GID.expects(:change_privilege).with do |gid| Process.gid = gid Process.egid = gid @@ -80,15 +136,6 @@ describe Puppet::Util::SUIDManager do 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 @@ -103,21 +150,12 @@ describe Puppet::Util::SUIDManager do describe "#change_user" do describe "when changing permanently" do - it "should try to change_privilege if it is supported" do + it "should change_privilege" 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) @@ -129,6 +167,7 @@ describe Puppet::Util::SUIDManager do describe "when changing temporarily" do it "should change only euid and groups" do + Puppet::Util::SUIDManager.stubs(:initgroups).returns([]) Puppet::Util::SUIDManager.change_user(42, false) xids[:euid].should == 42 @@ -165,35 +204,6 @@ describe Puppet::Util::SUIDManager do 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. diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb new file mode 100755 index 0000000..c076c8d --- /dev/null +++ b/spec/unit/util_spec.rb @@ -0,0 +1,104 @@ +#!/usr/bin/env rspec +require 'spec_helper' + +describe Puppet::Util do + subject { Puppet::Util } + include PuppetSpec::Files + + context "#replace_file" do + it { should respond_to :replace_file } + + let :target do + target = Tempfile.new("puppet-util-replace-file") + target.puts("hello, world") + target.flush # make sure content is on disk. + target.fsync rescue nil + target.close + target + end + + it "should fail if no block is given" do + expect { subject.replace_file(target.path, 0600) }.to raise_error /block/ + end + + it "should replace a file when invoked" do + # Check that our file has the expected content. + File.read(target.path).should == "hello, world\n" + + # Replace the file. + subject.replace_file(target.path, 0600) do |fh| + fh.puts "I am the passenger..." + end + + # ...and check the replacement was complete. + File.read(target.path).should == "I am the passenger...\n" + end + + [0555, 0600, 0660, 0700, 0770].each do |mode| + it "should copy 0#{mode.to_s(8)} permissions from the target file by default" do + File.chmod(mode, target.path) + + (File.stat(target.path).mode & 07777).should == mode + + subject.replace_file(target.path, 0000) {|fh| fh.puts "bazam" } + + (File.stat(target.path).mode & 07777).should == mode + File.read(target.path).should == "bazam\n" + end + end + + it "should copy the permissions of the source file before yielding" do + File.chmod(0555, target.path) + inode = File.stat(target.path).ino + + yielded = false + subject.replace_file(target.path, 0600) do |fh| + (File.stat(fh.path).mode & 07777).should == 0555 + yielded = true + end + yielded.should be_true + + # We can't check inode on Windows + File.stat(target.path).ino.should_not == inode + + (File.stat(target.path).mode & 07777).should == 0555 + end + + it "should use the default permissions if the source file doesn't exist" do + new_target = target.path + '.foo' + File.should_not be_exist(new_target) + + begin + subject.replace_file(new_target, 0555) {|fh| fh.puts "foo" } + (File.stat(new_target).mode & 07777).should == 0555 + ensure + File.unlink(new_target) if File.exists?(new_target) + end + end + + it "should not replace the file if an exception is thrown in the block" do + yielded = false + threw = false + + begin + subject.replace_file(target.path, 0600) do |fh| + yielded = true + fh.puts "different content written, then..." + raise "...throw some random failure" + end + rescue Exception => e + if e.to_s =~ /some random failure/ + threw = true + else + raise + end + end + + yielded.should be_true + threw.should be_true + + # ...and check the replacement was complete. + File.read(target.path).should == "hello, world\n" + end + end +end -- 1.7.7.3