Merge remote-tracking branch 'ethan/ticket/master/15559-dont-remove-windows-SYSTEM-permissions'

* ethan/ticket/master/15559-dont-remove-windows-SYSTEM-permissions:
  (#15559) Windows security test fails under SYSTEM

Author: joshcooper

lib/puppet/util/windows/security.rb

@@ -284,12 +284,15 @@ def get_mode(path)
               mode |= S_ISVTX
             end
           when well_known_system_sid
-            mode &= ~S_ISYSTEM_MISSING
           else
             #puts "Warning, unable to map SID into POSIX mode: #{ace[:sid]}"
             mode |= S_IEXTRA
           end
 
+          if ace[:sid] == well_known_system_sid
+            mode &= ~S_ISYSTEM_MISSING
+          end
+
           # if owner and group the same, then user and group modes are the OR of both
           if owner_sid == group_sid
             mode |= ((mode & S_IRWXG) << 3) | ((mode & S_IRWXU) >> 3)

spec/integration/type/file_spec.rb

@@ -1049,12 +1049,16 @@ def build_path(dir)
             # read the externally created file SYSTEM ACE(s)
             system_aces = get_aces_for_path_by_sid(path, @sids[:system])
             system_aces.should_not be_empty
-            system_aces.each do |ace|
-              ace[:mask].should == Windows::File::FILE_ALL_ACCESS
+            # when running under SYSTEM account, multiple ACEs come back
+            # so we only care that we have at least one of these
+            system_aces.any? do |ace|
               inherited = Windows::Security::INHERITED_ACE
-              (ace[:flags] & inherited).should == inherited
-            end
+              ace[:mask] == Windows::File::FILE_ALL_ACCESS &&
+              (ace[:flags] & inherited) == inherited
+            end.should be_true
 
+            # when running under SYSTEM user, must force the group to properly verify
+            @file[:group] = 'None'
             catalog.apply
 
             # should still have the same SYSTEM ACE(s), but with inheritance stripped
@@ -1115,18 +1119,32 @@ def build_path(dir)
             catalog.add_resource @directory
           end
 
-         it "that already exist (SYSTEM ACE remains unmodified)" do
+          it "that already exist by removing the inherited SYSTEM ACE but adding an uninherited one" do
             FileUtils.mkdir(dir)
 
             # read the externally created file SYSTEM ACE(s)
             system_aces = get_aces_for_path_by_sid(dir, @sids[:system])
             system_aces.should_not be_empty
-            system_aces.each { |ace| ace[:mask].should == Windows::File::FILE_ALL_ACCESS }
 
+            inherited = Windows::Security::INHERITED_ACE
+
+            # when running under SYSTEM account, multiple ACEs come back
+            # so we only care that we have at least one of these
+            system_aces.any? do |ace|
+              ace[:mask] == Windows::File::FILE_ALL_ACCESS &&
+              (ace[:flags] & inherited) == inherited
+            end.should be_true
+
+            # when running under SYSTEM user, must force the group to properly verify
+            @directory[:group] = 'Administrators'
             catalog.apply
 
-            # should still have the same SYSTEM ACE(s)
-            get_aces_for_path_by_sid(dir, @sids[:system]).should == system_aces
+            # should still have the same SYSTEM ACE(s), but with inheritance stripped
+            system_aces = get_aces_for_path_by_sid(dir, @sids[:system])
+            system_aces.each do |ace|
+              ace[:mask].should == Windows::File::FILE_ALL_ACCESS
+              (ace[:flags] & inherited).should_not == inherited
+            end
           end
 
           it "that are new where SYSTEM is not the given group or owner (SYSTEM ACE remains FULL)" do

spec/integration/util/windows/security_spec.rb

@@ -18,30 +18,51 @@ class WindowsSecurityTester
       :current_user => Puppet::Util::Windows::Security.name_to_sid(Sys::Admin.get_login),
       :system => Win32::Security::SID::LocalSystem,
       :admin => Puppet::Util::Windows::Security.name_to_sid("Administrator"),
+      :administrators => Win32::Security::SID::BuiltinAdministrators,
       :guest => Puppet::Util::Windows::Security.name_to_sid("Guest"),
       :users => Win32::Security::SID::BuiltinUsers,
       :power_users => Win32::Security::SID::PowerUsers,
+      :none => Win32::Security::SID::Nobody,
     }
   end
 
   let (:sids) { @sids }
   let (:winsec) { WindowsSecurityTester.new }
 
+  def set_group_depending_on_current_user(path)
+    if sids[:current_user] == sids[:system]
+      # if the current user is SYSTEM, by setting the group to
+      # guest, SYSTEM is automagically given full control, so instead
+      # override that behavior with SYSTEM as group and a specific mode
+      winsec.set_group(sids[:system], path)
+      mode = winsec.get_mode(path)
+      winsec.set_mode(mode & ~WindowsSecurityTester::S_IRWXG, path)
+    else
+      winsec.set_group(sids[:guest], path)
+    end
+  end
+
   shared_examples_for "only child owner" do
     it "should allow child owner" do
       check_child_owner
     end
 
     it "should deny parent owner" do
-      lambda { check_parent_owner }.should raise_error(Errno::EACCES)
+      pending("when running as SYSTEM the absence of a SYSTEM group/owner causes full access to be added for SYSTEM",
+        :if => sids[:current_user] == sids[:system]) do
+        lambda { check_parent_owner }.should raise_error(Errno::EACCES)
+      end
     end
 
     it "should deny group" do
       lambda { check_group }.should raise_error(Errno::EACCES)
     end
 
     it "should deny other" do
-      lambda { check_other }.should raise_error(Errno::EACCES)
+      pending("when running as SYSTEM the absence of a SYSTEM group/owner causes full access to be added for SYSTEM",
+        :if => sids[:current_user] == sids[:system]) do
+        lambda { check_other }.should raise_error(Errno::EACCES)
+      end
     end
   end
 
@@ -127,10 +148,15 @@ class WindowsSecurityTester
           # new file has SYSTEM
           system_aces = winsec.get_aces_for_path_by_sid(path, sids[:system])
           system_aces.should_not be_empty
-          system_aces.each { |ace| ace[:mask].should == WindowsSecurityTester::FILE_ALL_ACCESS }
+
+          # when running under SYSTEM account, multiple ACEs come back
+          # so we only care that we have at least one of these
+          system_aces.any? do |ace|
+            ace[:mask] == Windows::File::FILE_ALL_ACCESS
+          end.should be_true
 
           winsec.set_group(sids[:power_users], path)
-          winsec.set_owner(sids[:current_user], path)
+          winsec.set_owner(sids[:administrators], path)
 
           # and should still have a noninherited SYSTEM ACE granting full control
           system_aces = winsec.get_aces_for_path_by_sid(path, sids[:system])
@@ -175,8 +201,13 @@ class WindowsSecurityTester
             # new file has SYSTEM
             system_aces = winsec.get_aces_for_path_by_sid(path, sids[:system])
             system_aces.should_not be_empty
-            system_aces.each { |ace| ace[:mask].should == WindowsSecurityTester::FILE_ALL_ACCESS }
+            # when running under SYSTEM account, multiple ACEs come back
+            # so we only care that we have at least one of these
+            system_aces.any? do |ace|
+              ace[:mask] == WindowsSecurityTester::FILE_ALL_ACCESS
+            end.should be_true
 
+            winsec.set_group(sids[:none], path)
             winsec.set_mode(0600, path)
 
             # and should still have the same SYSTEM ACE(s)
@@ -204,6 +235,8 @@ class WindowsSecurityTester
 
           describe "for read-only objects" do
             before :each do
+              winsec.set_group(sids[:none], path)
+              winsec.set_mode(0600, path)
               winsec.add_attributes(path, WindowsSecurityTester::FILE_ATTRIBUTE_READONLY)
               (winsec.get_attributes(path) & WindowsSecurityTester::FILE_ATTRIBUTE_READONLY).should be_nonzero
             end
@@ -218,7 +251,12 @@ class WindowsSecurityTester
               (winsec.get_attributes(path) & WindowsSecurityTester::FILE_ATTRIBUTE_READONLY).should be_nonzero
 
               system_aces = winsec.get_aces_for_path_by_sid(path, sids[:system])
-              system_aces.each { |ace| ace[:mask].should == WindowsSecurityTester::FILE_ALL_ACCESS }
+
+              # when running under SYSTEM account, and set_group / set_owner hasn't been called
+              # SYSTEM full access will be restored
+              system_aces.any? do |ace|
+                ace[:mask] == Windows::File::FILE_ALL_ACCESS
+              end.should be_true
             end
           end
 
@@ -297,7 +335,7 @@ class WindowsSecurityTester
       describe "for an administrator", :if => Puppet.features.root? do
         before :each do
           winsec.set_mode(WindowsSecurityTester::S_IRWXU | WindowsSecurityTester::S_IRWXG, path)
-          winsec.set_group(sids[:guest], path)
+          set_group_depending_on_current_user(path)
           winsec.set_owner(sids[:guest], path)
           lambda { File.open(path, 'r') }.should raise_error(Errno::EACCES)
         end
@@ -474,7 +512,10 @@ def check_other
             end
 
             it "should deny other" do
-              lambda { check_other }.should raise_error(Errno::EACCES)
+              pending("when running as SYSTEM the absence of a SYSTEM group/owner causes full access to be added for SYSTEM",
+                :if => sids[:current_user] == sids[:system]) do
+                lambda { check_other }.should raise_error(Errno::EACCES)
+              end
             end
           end