[patch] 2.6.x patch

0001-2.6.x-Fixes-for-CVE-2012-1053-and-CVE-2012-1054.patch (text/plain), 41.72 KB, created by Matthew Marlowe (RETIRED) on 2012-02-15 21:52:59 UTC

From 781394b58bc71fb114426298b6aa2b345ae671cb Mon Sep 17 00:00:00 2001
From: Matthaus Litteken <[e-mail]>
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 <[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 0a09a64457ef37f97bbe493f2fd90aca5ca73746
Author: Nick Lewis <[e-mail]>
Date:   Mon Feb 13 13:03:52 2012 -0800

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

commit 2599d563ff290447653dca5e3927511b3a911422
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 ead36ff3a8b4f37f1f0b4d0afed5b6d7c0bd15b9
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 146953861e007a91545fdbd0ea59ab3509326e09
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 8461203aa7ebfb083bce605bd44c2f579342f5bb
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 0ad532a4b26ea54455d77791006324b684cf5b10
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 76d0749f0a9a496b70e7dc7e6d6d6ff692224e36
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 50909b93dc01b735c1aa104fd568d71b0760ccd9
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 d00c5cc45e3ccf8c80535d372027dca8452d1056
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/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 <<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 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