Merge pull request puppetlabs#1099 from daniel-pittman/bug/2.7.x/8714…

…-better-symlink-handling-in-selinux-utils

Bug/2.7.x/8714 better symlink handling in selinux utils

lib/puppet/util/selinux.rb

@@ -136,6 +136,21 @@ def set_selinux_default_context(file)
     nil
   end
 
+  ########################################################################
+  # Internal helper methods from here on in, kids.  Don't fiddle.
+  private
+
+  # Check filesystem a path resides on for SELinux support against
+  # whitelist of known-good filesystems.
+  # Returns true if the filesystem can support SELinux labels and
+  # false if not.
+  def selinux_label_support?(file)
+    fstype = find_fs(file)
+    return false if fstype.nil?
+    filesystems = ['ext2', 'ext3', 'ext4', 'gfs', 'gfs2', 'xfs', 'jfs']
+    filesystems.include?(fstype)
+  end
+
   # Internal helper function to read and parse /proc/mounts
   def read_mounts
     mounts = ""
@@ -171,46 +186,26 @@ def read_mounts
     mntpoint
   end
 
-  def realpath(path)
-    path, rest = Pathname.new(path), []
-    path, rest = path.dirname, [path.basename] + rest while ! path.exist?
-    File.join( path.realpath, *rest )
-  end
-
-  def parent_directory(path)
-    Pathname.new(path).dirname.to_s
-  end
-
-  # Internal helper function to return which type of filesystem a
-  # given file path resides on
+  # Internal helper function to return which type of filesystem a given file
+  # path resides on
   def find_fs(path)
-    unless mnts = read_mounts
-      return nil
+    return nil unless mounts = read_mounts
+
+    # cleanpath eliminates useless parts of the path (like '.', or '..', or
+    # multiple slashes), without touching the filesystem, and without
+    # following symbolic links.  This gives the right (logical) tree to follow
+    # while we try and figure out what file-system the target lives on.
+    path = Pathname(path).cleanpath
+    unless path.absolute?
+      raise Puppet::DevError, "got a relative path in SELinux find_fs: #{path}"
     end
 
-    # For a given file:
-    # Check if the filename is in the data structure;
-    #   return the fstype if it is.
-    # Just in case: return something if you're down to "/" or ""
-    # Remove the last slash and everything after it,
-    #   and repeat with that as the file for the next loop through.
-    path = realpath(path)
-    while not path.empty?
-      return mnts[path] if mnts.has_key?(path)
-      path = parent_directory(path)
+    # Now, walk up the tree until we find a match for that path in the hash.
+    path.ascend do |segment|
+      return mounts[segment.to_s] if mounts.has_key?(segment.to_s)
     end
-    mnts['/']
-  end
 
-  # Check filesystem a path resides on for SELinux support against
-  # whitelist of known-good filesystems.
-  # Returns true if the filesystem can support SELinux labels and
-  # false if not.
-  def selinux_label_support?(file)
-    fstype = find_fs(file)
-    return false if fstype.nil?
-    filesystems = ['ext2', 'ext3', 'ext4', 'gfs', 'gfs2', 'xfs', 'jfs']
-    filesystems.include?(fstype)
+    # Should never be reached...
+    return mounts['/']
   end
-
 end

spec/unit/util/selinux_spec.rb

@@ -1,6 +1,7 @@
 #!/usr/bin/env rspec
 require 'spec_helper'
 
+require 'pathname'
 require 'puppet/util/selinux'
 include Puppet::Util::SELinux
 
@@ -33,7 +34,7 @@ def self.is_selinux_enabled
     end
   end
 
-  describe "filesystem detection" do
+  describe "read_mounts" do
     before :each do
       fh = stub 'fh', :close => nil
       File.stubs(:open).with("/proc/mounts").returns fh
@@ -48,6 +49,17 @@ def self.is_selinux_enabled
         '/proc' => 'proc',
         '/dev' => 'tmpfs' }
     end
+  end
+
+  describe "filesystem detection" do
+    before :each do
+      self.stubs(:read_mounts).returns({
+        '/'        => 'ext3',
+        '/sys'     => 'sysfs',
+        '/mnt/nfs' => 'nfs',
+        '/proc'    => 'proc',
+        '/dev'     => 'tmpfs' })
+    end
 
     it "should match a path on / to ext3" do
       find_fs('/etc/puppet/testfile').should == "ext3"
@@ -65,41 +77,21 @@ def self.is_selinux_enabled
       selinux_label_support?('/mnt/nfs/testfile').should be_false
     end
 
-    it "should follow symlinks when determining file systems" do
-      self.stubs(:realpath).with('/mnt/symlink/testfile').returns('/mnt/nfs/dest/testfile')
+    it "(#8714) don't follow symlinks when determining file systems" do
+      scratch = Pathname(PuppetSpec::Files.tmpdir('selinux'))
 
-      selinux_label_support?('/mnt/symlink/testfile').should be_false
-    end
+      self.stubs(:read_mounts).returns({
+          '/'             => 'ext3',
+          scratch + 'nfs' => 'nfs',
+        })
 
-  end
+      (scratch + 'foo').make_symlink('nfs/bar')
+      selinux_label_support?(scratch + 'foo').should be_true
+    end
 
-  describe "realpath" do
     it "should handle files that don't exist" do
-
-      # Since I'm stubbing Pathname.new for this test,
-      # I need to also stub the internal calls to Pathname.new,
-      # which happen in Pathname.dirname and Parthname.basename
-      # I want those to return real Pathname objects,
-      # so I'm creating them before the stub is in place.
-      realpaths = Hash.new {|hash, path| hash[path] = Pathname.new(path) }
-      paths = ['symlink', '/mnt']
-      paths.each { |path| realpaths[path] }
-
-      realpaths['/mnt/symlink'] = stubs "Pathname"
-      realpaths['/mnt/symlink'].stubs(:realpath).returns(realpaths['/mnt/nfs/dest'])
-      realpaths['/mnt/symlink'].stubs(:exist?).returns(true)
-
-      realpaths['/mnt/symlink/nonexistant'] = stubs "Pathname"
-      realpaths['/mnt/symlink/nonexistant'].stubs(:realpath).raises(Errno::ENOENT)
-      realpaths['/mnt/symlink/nonexistant'].stubs(:exist?).returns(false)
-      realpaths['/mnt/symlink/nonexistant'].stubs(:dirname).returns(realpaths['/mnt/symlink'])
-      realpaths['/mnt/symlink/nonexistant'].stubs(:basename).returns(realpaths['nonexistant'])
-
-      realpaths.each do |path, value|
-        Pathname.stubs(:new).with(path).returns(value)
-      end
-
-      realpath('/mnt/symlink/nonexistant').should == '/mnt/nfs/dest/nonexistant'
+      scratch = Pathname(PuppetSpec::Files.tmpdir('selinux'))
+      selinux_label_support?(scratch + 'nonesuch').should be_true
     end
   end