Merge pull request #33 from dotconfig404/fix/chown-in-rootless-containers

fix: skip chown in rootless containers via ensure_ownership helper

Author: bastelfreak

lib/puppetserver/ca/utils/file_system.rb

@@ -15,17 +15,26 @@ class FileSystem
           :signeddir => 0755
         }
 
-        def self.instance
-          @instance ||= new
-        end
-
-        def self.write_file(*args)
-          instance.write_file(*args)
+        def self.write_file(path, one_or_more_objects, mode)
+          File.open(path, 'w', mode) do |f|
+            Array(one_or_more_objects).each do |object|
+              f.puts object.to_s
+            end
+          end
+          ensure_ownership(path)
         end
 
         def self.ensure_dirs(one_or_more_dirs)
           Array(one_or_more_dirs).each do |directory|
-            instance.ensure_dir(directory)
+            ensure_dir(directory)
+          end
+        end
+
+        # Warning: directory mode should be specified in DIR_MODES above
+        def self.ensure_dir(directory)
+          if !File.exist?(directory)
+            FileUtils.mkdir_p(directory, mode: DIR_MODES[directory])
+            ensure_ownership(directory)
           end
         end
 
@@ -53,56 +62,30 @@ def self.check_for_existing_files(one_or_more_paths)
         def self.forcibly_symlink(source, link_target)
           FileUtils.remove_dir(link_target, true)
           FileUtils.symlink(source, link_target)
-          # Ensure the symlink has the same ownership as the source.
-          # This requires using `FileUtils.chown` rather than `File.chown`, as
-          # the latter will update the ownership of the source rather than the
-          # link itself.
-          # Symlink permissions are ignored in favor of the source's permissions,
-          # so we don't have to change those.
-          source_info = File.stat(source)
-          FileUtils.chown(source_info.uid, source_info.gid, link_target)
-        end
-
-        def initialize
-          @user, @group = find_user_and_group
+          ensure_ownership(link_target)
         end
 
-        def find_user_and_group
-          if !running_as_root?
-            return Process.euid, Process.egid
-          else
-            if pe_puppet_exists?
-              return 'pe-puppet', 'pe-puppet'
-            else
-              return 'puppet', 'puppet'
-            end
-          end
+        # Chown the path to the puppet user when running as root.
+        # Skipped otherwise: a non-root process can only have created the path
+        # as itself, so ownership is already correct, and chowning to any other
+        # user would require CAP_CHOWN (unavailable in rootless containers).
+        #
+        # Uses `FileUtils.chown` rather than `File.chown` so that when `path`
+        # is a symlink it operates on the link itself rather than its target.
+        def self.ensure_ownership(path)
+          return unless running_as_root?
+          user = pe_puppet_exists? ? 'pe-puppet' : 'puppet'
+          group = pe_puppet_exists? ? 'pe-puppet' : 'puppet'
+          FileUtils.chown(user, group, path)
         end
 
-        def running_as_root?
+        def self.running_as_root?
           !Gem.win_platform? && Process.euid == 0
         end
 
-        def pe_puppet_exists?
+        def self.pe_puppet_exists?
           !!(Etc.getpwnam('pe-puppet') rescue nil)
         end
-
-        def write_file(path, one_or_more_objects, mode)
-          File.open(path, 'w', mode) do |f|
-            Array(one_or_more_objects).each do |object|
-              f.puts object.to_s
-            end
-          end
-          FileUtils.chown(@user, @group, path)
-        end
-
-        # Warning: directory mode should be specified in DIR_MODES above
-        def ensure_dir(directory)
-          if !File.exist?(directory)
-            FileUtils.mkdir_p(directory, mode: DIR_MODES[directory])
-            FileUtils.chown(@user, @group, directory)
-          end
-        end
       end
     end
   end

spec/puppetserver/ca/utils/file_system_spec.rb

@@ -0,0 +1,39 @@
+require 'spec_helper'
+require 'puppetserver/ca/utils/file_system'
+
+RSpec.describe Puppetserver::Ca::Utils::FileSystem do
+  describe '.ensure_ownership' do
+    let(:path) { '/some/path' }
+
+    context 'when not running as root' do
+      before do
+        allow(described_class).to receive(:running_as_root?).and_return(false)
+      end
+
+      it 'does not change ownership' do
+        expect(FileUtils).not_to receive(:chown)
+        described_class.ensure_ownership(path)
+      end
+    end
+
+    context 'when running as root' do
+      before do
+        allow(described_class).to receive(:running_as_root?).and_return(true)
+      end
+
+      it 'chowns to puppet when pe-puppet does not exist' do
+        allow(described_class).to receive(:pe_puppet_exists?).and_return(false)
+
+        expect(FileUtils).to receive(:chown).with('puppet', 'puppet', path)
+        described_class.ensure_ownership(path)
+      end
+
+      it 'chowns to pe-puppet when pe-puppet exists' do
+        allow(described_class).to receive(:pe_puppet_exists?).and_return(true)
+
+        expect(FileUtils).to receive(:chown).with('pe-puppet', 'pe-puppet', path)
+        described_class.ensure_ownership(path)
+      end
+    end
+  end
+end