(puppetlabs#8714) Don't follow symlinks in SELinux FS detection

When detecting the filesystem that a target file lives on, we used to follow
symbolic links.  This is actually a problem - the SELinux change applies to
the link, not the target of the link.

This rewrites the code to handle this more cleanly, following the logical tree
up to locate the record in the set of mountpoints.

Signed-off-by: Daniel Pittman <[email protected]>

lib/puppet/util/selinux.rb

@@ -186,34 +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['/']
+
+    # 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