[patch] 2.7.x patch

0001-2.7.x-fixes-for-CVE-2012-1053-and-CVE-2012-1054.patch (text/plain), 44.68 KB, created by Matthew Marlowe (RETIRED) on 2012-02-15 21:54:00 UTC

From f9e9858999fa88d5fb74a2474cf814974c958f9c Mon Sep 17 00:00:00 2001
From: Matthaus Litteken <[e-mail]>
Date: Tue, 14 Feb 2012 23:27:48 -0800
Subject: [PATCH] 2.7.x fixes for CVE-2012-1053 and CVE-2012-1054

  * 5846da1 Disable replace_file on Windows
  * abb3f4b Remove unnecessary fallbacks in change_{user,group}
  * aedace0 Document uid/gid-related methods in Puppet::Util
  * 52a0c7e Copy owner/group in replace_file
  * d2bfa1f (#12463) eliminate `secure_open` in favour of `replace_file`
  * 2cf4f55 (#12460) use `replace_file` for the .k5login file
  * 101d845 (#12462) user_role_add: use `replace_file` for /etc/shadow
  * 247c35c (#12463) add secure `replace_file` to Puppet::Util
  * 617cbb2 (#12459) drop supplementary groups when permanently dropping UID
  * c06fe4d (#12458) default to users primary group, not root, in `asuser`
  * bfb7b8d (#12457) add users primary group, not Process.gid, in initgroups

commit 5846da10f8b7b9414a75f716f2f0604484c8a7ee
Author: Nick Lewis <[e-mail]>
Date:   Tue Feb 14 18:02:33 2012 -0800

    Disable replace_file on Windows

    Our current approach of setting owner/mode in the Puppet way risks
    losing any extra permissions beyond owner/group/everyone, which is
    significant enough that it is better to simply not support this
    operation until we have a better solution. replace_file isn't used in
    any code which actually runs on Windows at the moment, so this is
    a benign change.

commit abb3f4b592a65c24a68d64f7763703a3ba7b6c25
Author: Nick Lewis <[e-mail]>
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 aedace001457778af18a6a8cf7f400fa7ee79f70
Author: Nick Lewis <[e-mail]>
Date:   Mon Feb 13 13:03:52 2012 -0800

    Document uid/gid-related methods in Puppet::Util

commit 52a0c7e52565238fed7a686ea02f70df6c2c358b
Author: Nick Lewis <[e-mail]>
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 d2bfa1f4c0982f436e06b794d2a4163dac65d231
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

commit 2cf4f55bf963b008a143df099c6d9f28c1e36df8
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

commit 101d845af6cff563d3747dc311154d34b04b1a43
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

commit 247c35c63f7035ed975e68278d8d270ef601de0a
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

commit 617cbb2c2e2f113664cdf3d802ec93c6cf5ae835
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

    Signed-off-by: Daniel Pittman <[e-mail]>

commit c06fe4d51d3b2ade1be09dccd392c81374892cf8
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

    Contributed: Daniel Pittman <[e-mail]>
    Contributed: Deepak Giridharagopal <[e-mail]>
    Contributed: Nick Lewis <[e-mail]>

commit bfb7b8d8e0eed965afe2a8566e5ce82ae10b20d5
Author: Daniel Pittman <[e-mail]>
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 <[e-mail]>

    Signed-off-by: Daniel Pittman <[e-mail]>
---
 lib/puppet/daemon.rb                          |    4 +-
 lib/puppet/feature/base.rb                    |    1 +
 lib/puppet/network/server.rb                  |    2 +-
 lib/puppet/provider/user/user_role_add.rb     |   54 +++++----
 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 |   83 +++++++------
 spec/unit/type/k5login_spec.rb                |  115 +++++++++++++++++
 spec/unit/util/suidmanager_spec.rb            |  165 ++++++++++++-------------
 spec/unit/util_spec.rb                        |  126 +++++++++++++++++++
 13 files changed, 544 insertions(+), 213 deletions(-)
 create mode 100755 spec/unit/type/k5login_spec.rb

diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index a6945c8..5092bdf 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/feature/base.rb b/lib/puppet/feature/base.rb
index 8813197..222a56f 100644
--- a/lib/puppet/feature/base.rb
+++ b/lib/puppet/feature/base.rb
@@ -23,6 +23,7 @@ Puppet.features.add(:microsoft_windows) do
     require 'win32ole'
     require 'win32/api'
     require 'win32/taskscheduler'
+    require 'puppet/util/windows/security'
     true
   rescue LoadError => err
     warn "Cannot run on Microsoft Windows without the sys-admin, win32-process, win32-dir, win32-service and win32-taskscheduler gems: #{err}" unless Puppet.features.posix?
diff --git a/lib/puppet/network/server.rb b/lib/puppet/network/server.rb
index e83259c..b970c91 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 5bed1ee..799432a 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,33 +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(shadow_file, "r") do |shadow|
-        File.open("#{shadow_file}_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_arr[2] = Time.now().to_i / 86400
-              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("#{shadow_file}_tmp", shadow_file)
     rescue => detail
-      fail "Could not write temporary shadow file: #{detail}"
-    ensure
-      # Make sure this *always* gets deleted
-      File.unlink("#{shadow_file}_tmp") if File.exist?("#{shadow_file}_tmp")
+      fail "Could not write replace #{target_file_path}: #{detail}"
     end
   end
-
-  private
-
-  def shadow_file; '/etc/shadow'; 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 e7fbac0..b2fff27 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 5203fb0..ab9480a 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -2,12 +2,13 @@
 
 require 'English'
 require 'puppet/util/monkey_patches'
-require 'sync'
-require 'tempfile'
 require 'puppet/external/lock'
-require 'monitor'
 require 'puppet/util/execution_stub'
 require 'uri'
+require 'sync'
+require 'monitor'
+require 'tempfile'
+require 'pathname'
 
 module Puppet
   # A command failed to execute.
@@ -292,8 +293,7 @@ module Util
 
         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'
         Kernel.exec(*command)
@@ -459,27 +459,83 @@ 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?
+    raise Puppet::DevError, "replace_file is non-functional on Windows" if Puppet.features.microsoft_windows?
+
+    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 6003251..bb0ead7 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 c7bab23..f270c40 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
@@ -59,55 +60,76 @@ module Puppet::Util::SUIDManager
     Puppet::Util::Windows::User.admin?
   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)
@@ -132,10 +154,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
@@ -145,14 +170,4 @@ 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
index afcefa5..64f92a6 100755
--- a/spec/unit/provider/user/user_role_add_spec.rb
+++ b/spec/unit/provider/user/user_role_add_spec.rb
@@ -1,6 +1,6 @@
-#!/usr/bin/env rspec
 require 'spec_helper'
 require 'puppet_spec/files'
+require 'tempfile'
 
 provider_class = Puppet::Type.type(:user).provider(:user_role_add)
 
@@ -246,44 +246,51 @@ describe provider_class do
   end
 
   describe "when setting the password" do
-    before :each do
-      @shadow_file = tmpfile('shadow')
-      File.open(@shadow_file, 'w') do |f|
-        f.puts 'fakeval:password:0'
-      end
-      @provider.stubs(:shadow_file).returns(@shadow_file)
-    end
-
-    it 'opens #shadow_file for reading' do
-      File.expects(:open).with(@shadow_file, "r")
-      File.stubs(:rename)
-
-      @provider.password = "hashedpassword"
-    end
-
-    it 'writes to "#{shadow_file}_tmp"' do
-      File.stubs(:rename)
-      File.stubs(:unlink)
-      @provider.password = 'hashedpassword'
-
-      File.read("#{@shadow_file}_tmp").should =~ /hashedpassword/
-    end
-
-    it 'renames "#{shadow_file}_tmp" to shadow_file' do
-      File.stubs(:open)
-      File.expects(:rename).with("#{@shadow_file}_tmp", @shadow_file)
-
-      @provider.password = "hashedpassword"
-    end
+    let(:path) { tmpfile('etc-shadow') }
 
-    it 'updates the last changed field' do
-      Time.stubs(:now).returns(42 * 86400)
-
-      File.read(@shadow_file).should == "fakeval:password:0\n"
-
-      @provider.password = 'hashedpassword'
-
-      File.read(@shadow_file).should == "fakeval:hashedpassword:42"
+    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 <<FIXTURE
+fakeval:seriously:15315:0:99999:7:::
+FIXTURE
+      @provider.password = "totally"
+      File.read(path).should =~ /^fakeval:totally:/
+    end
+
+    it "should only update the target user" do
+      write_fixture <<FIXTURE
+before:seriously:15315:0:99999:7:::
+fakeval:seriously:15315:0:99999:7:::
+fakevalish:seriously:15315:0:99999:7:::
+after:seriously:15315:0:99999:7:::
+FIXTURE
+      @provider.password = "totally"
+      File.read(path).should == <<EOT
+before:seriously:15315:0:99999:7:::
+fakeval:totally:15315:0:99999:7:::
+fakevalish:seriously:15315:0:99999:7:::
+after:seriously:15315:0:99999:7:::
+EOT
+    end
+
+    # This preserves the current semantics, but is it right? --daniel 2012-02-05
+    it "should do nothing if the target user is missing" do
+      fixture = <<FIXTURE
+before:seriously:15315:0:99999:7:::
+fakevalish:seriously:15315:0:99999:7:::
+after:seriously:15315:0:99999:7:::
+FIXTURE
+
+      write_fixture fixture
+      @provider.password = "totally"
+      File.read(path).should == fixture
     end
   end
 
diff --git a/spec/unit/type/k5login_spec.rb b/spec/unit/type/k5login_spec.rb
new file mode 100755
index 0000000..be9aae2
--- /dev/null
+++ b/spec/unit/type/k5login_spec.rb
@@ -0,0 +1,115 @@
+#!/usr/bin/env ruby
+require 'spec_helper'
+require 'fileutils'
+require 'puppet/type'
+
+describe Puppet::Type.type(:k5login) do
+  include PuppetSpec::Files
+
+  context "the type class" do
+    subject { described_class }
+    it { should be_validattr :ensure }
+    it { should be_validattr :path }
+    it { should be_validattr :principals }
+    it { should be_validattr :mode }
+    # We have one, inline provider implemented.
+    it { should be_validattr :provider }
+  end
+
+  let(:path) { tmpfile('k5login') }
+
+  def resource(attrs = {})
+    attrs = {
+      :ensure     => 'present',
+      :path       => path,
+      :principals => '[e-mail]'
+    }.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 => "[e-mail]\n").retrieve }
+
+          it "should retrieve its principals correctly" do
+            subject[:principals].should == ["[e-mail]"]
+          end
+        end
+
+        context "with two principals" do
+          subject do
+            content = ["[e-mail]", "[e-mail]"].join("\n")
+            resource(:content => content).retrieve
+          end
+
+          it "should retrieve its principals correctly" do
+            subject[:principals].should == ["[e-mail]", "[e-mail]"]
+          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 => ["[e-mail]"]).property(:principals).sync
+        File.read(path).should == "[e-mail]\n"
+      end
+
+      it "should write multiple principals to the file" do
+        content = ["[e-mail]", "[e-mail]"]
+
+        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 7581e10..187754a 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,40 +39,88 @@ 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
       Puppet.features.stubs(:microsoft_windows?).returns(false)
+      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]) {}
 
-      yielded = false
-      Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) do
-        xids[:egid].should == user[:gid]
-        xids[:euid].should == user[:uid]
-        yielded = true
+      xids.should be_empty
+    end
+
+    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
 
     it "should not get or set euid/egid on Windows" do
@@ -78,7 +134,7 @@ describe Puppet::Util::SUIDManager do
 
   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
@@ -89,15 +145,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
@@ -112,21 +159,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)
@@ -138,6 +176,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
@@ -174,46 +213,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)
-        Puppet.features.stubs(:microsoft_windows?).returns(false)
-
-        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
-
-      it "should not get or set euid/egid on Windows" do
-        Puppet.features.stubs(:microsoft_windows?).returns true
-        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
index 35358d6..d0fcf53 100755
--- a/spec/unit/util_spec.rb
+++ b/spec/unit/util_spec.rb
@@ -5,6 +5,24 @@ require 'spec_helper'
 describe Puppet::Util do
   include PuppetSpec::Files
 
+  if Puppet.features.microsoft_windows?
+    def set_mode(mode, file)
+      Puppet::Util::Windows::Security.set_mode(mode, file)
+    end
+
+    def get_mode(file)
+      Puppet::Util::Windows::Security.get_mode(file) & 07777
+    end
+  else
+    def set_mode(mode, file)
+      File.chmod(mode, file)
+    end
+
+    def get_mode(file)
+      File.lstat(file).mode & 07777
+    end
+  end
+
   def process_status(exitstatus)
     return exitstatus if Puppet.features.microsoft_windows?
 
@@ -545,4 +563,112 @@ describe Puppet::Util do
       end
     end
   end
+
+  context "#replace_file" do
+    describe "on POSIX platforms", :if => Puppet.features.posix? do
+      subject { Puppet::Util }
+      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
+          set_mode(mode, target.path)
+
+          get_mode(target.path).should == mode
+
+          subject.replace_file(target.path, 0000) {|fh| fh.puts "bazam" }
+
+          get_mode(target.path).should == mode
+          File.read(target.path).should == "bazam\n"
+        end
+      end
+
+      it "should copy the permissions of the source file before yielding" do
+        set_mode(0555, target.path)
+        inode = File.stat(target.path).ino unless Puppet.features.microsoft_windows?
+
+        yielded = false
+        subject.replace_file(target.path, 0600) do |fh|
+          get_mode(fh.path).should == 0555
+          yielded = true
+        end
+        yielded.should be_true
+
+        # We can't check inode on Windows
+        File.stat(target.path).ino.should_not == inode unless Puppet.features.microsoft_windows?
+
+        get_mode(target.path).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" }
+          get_mode(new_target).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
+
+    describe "on Windows platforms" do
+      it "should fail and complain" do
+        Puppet.features.stubs(:microsoft_windows?).returns true
+
+        expect { Puppet::Util.replace_file("C:/foo", 0644) {} }.to raise_error(Puppet::DevError, "replace_file is non-functional on Windows")
+      end
+    end
+  end
 end
-- 
1.7.7.3