Increase windows ACL testing coverage. Fix children inheritance of windows ACLs.

7 files changed, 132 insertions, 71 deletions

diff --git a/lib/chef/config.rb b/lib/chef/config.rb
index 4f06aa9a63..a010d4874d 100644
--- a/lib/chef/config.rb
+++ b/lib/chef/config.rb
@@ -331,10 +331,9 @@ class Chef
     # for file resources, deploy files with either :move or :copy
     file_deploy_with :move
 
-    # do we create /tmp or %TEMP% files, or do we create temp files in the destination directory of the file?
-    #  - on windows this avoids issues with permission inheritance with the %TEMP% directory (do not set this to false)
-    #  - on unix this creates temp files like /etc/.sudoers.X-Y-Z and may create noise and make for itchy neckbeards
-    #  - with selinux and other ACLs approaches it may still be useful or to avoid copying across filesystems
-    file_deployment_uses_destdir ( RUBY_PLATFORM =~ /mswin|mingw|windows/ )
+    # If false file deployment is will be done via tempfiles that are
+    # created under ENV['TMP'] otherwise tempfiles will be created in
+    # the directory that files are going to reside.
+    file_deployment_uses_destdir false
   end
 end
diff --git a/lib/chef/file_access_control/windows.rb b/lib/chef/file_access_control/windows.rb
index be0ece291f..35a16337ab 100644
--- a/lib/chef/file_access_control/windows.rb
+++ b/lib/chef/file_access_control/windows.rb
@@ -218,17 +218,24 @@ class Chef
       def calculate_flags(rights)
         # Handle inheritance flags
         flags = 0
-        case rights[:applies_to_children]
-        when :containers_only
-          flags |= CONTAINER_INHERIT_ACE
-        when :objects_only
-          flags |= OBJECT_INHERIT_ACE
-        when true
-          flags |= CONTAINER_INHERIT_ACE
-          flags |= OBJECT_INHERIT_ACE
-        when nil
-          flags |= CONTAINER_INHERIT_ACE
-          flags |= OBJECT_INHERIT_ACE
+
+        #
+        # Configure child inheritence only if the the resource is some
+        # type of a directory.
+        #
+        if resource.is_a? Chef::Resource::Directory
+          case rights[:applies_to_children]
+          when :containers_only
+            flags |= CONTAINER_INHERIT_ACE
+          when :objects_only
+            flags |= OBJECT_INHERIT_ACE
+          when true
+            flags |= CONTAINER_INHERIT_ACE
+            flags |= OBJECT_INHERIT_ACE
+          when nil
+            flags |= CONTAINER_INHERIT_ACE
+            flags |= OBJECT_INHERIT_ACE
+          end
         end
 
         if rights[:applies_to_self] == false
diff --git a/lib/chef/file_content_management/deploy/mv_windows.rb b/lib/chef/file_content_management/deploy/mv_windows.rb
index 4e4103593d..9449b43832 100644
--- a/lib/chef/file_content_management/deploy/mv_windows.rb
+++ b/lib/chef/file_content_management/deploy/mv_windows.rb
@@ -37,36 +37,53 @@ class Chef
         end
 
         def deploy(src, dst)
-          dst_so = Security::SecurableObject.new(dst)
+          #
+          # At the time of deploy ACLs are correctly configured on the
+          # dst. This would be a simple atomic move operations in
+          # windows was not converting inherited ACLs of src to
+          # non-inherited ACLs in certain cases.See:
+          # http://blogs.msdn.com/b/oldnewthing/archive/2006/08/24/717181.aspx
+          #
+
+          #
+          # First cache the ACLs of dst file
+          #
 
-          # FIXME: catch exception when we can't elevate privs?
-          dst_sd = dst_so.security_descriptor(true)  # get the sd with the SACL
+          dst_so = Security::SecurableObject.new(dst)
+          begin
+            # get the sd with the SACL
+            dst_sd = dst_so.security_descriptor(true)
+          rescue Chef::Exceptions::Win32APIError
+            # Catch and raise if the user is not elevated enough.
+            # At this point we can't configure the file as expected so
+            # we're failing action on the resource.
+            raise Chef::Exceptions::WindowsNotAdmin
+          end
 
           if dst_sd.dacl_present?
             apply_dacl = ACL.create(dst_sd.dacl.select { |ace| !ace.inherited? })
           end
+
           if dst_sd.sacl_present?
             apply_sacl = ACL.create(dst_sd.sacl.select { |ace| !ace.inherited? })
           end
 
-          Chef::Log.debug("applying owner #{dst_sd.owner} to staged file")
-          Chef::Log.debug("applying group #{dst_sd.group} to staged file")
-          Chef::Log.debug("applying dacl #{dst_sd.dacl} to staged file") if dst_sd.dacl_present?
-          Chef::Log.debug("applying dacl inheritance to staged file") if dst_sd.dacl_inherits?
-          Chef::Log.debug("applying sacl #{dst_sd.sacl} to staged file") if dst_sd.sacl_present?
-          Chef::Log.debug("applying sacl inheritance to staged file") if dst_sd.sacl_inherits?
-
-          so = Security::SecurableObject.new(src)
+          #
+          # Then deploy the file
+          #
 
-          so.set_dacl(apply_dacl, dst_sd.dacl_inherits?) if dst_sd.dacl_present?
-
-          so.group = dst_sd.group
+          FileUtils.mv(src, dst)
 
-          so.owner = dst_sd.owner
+          #
+          # Then apply the cached files to the new dst file
+          #
 
-          so.set_sacl(apply_sacl, dst_sd.sacl_inherits?) if dst_sd.sacl_present?
+          dst_so = Security::SecurableObject.new(dst)
+          dst_so.group = dst_sd.group
+          dst_so.owner = dst_sd.owner
+          dst_so.set_dacl(apply_dacl, dst_sd.dacl_inherits?) if dst_sd.dacl_present?
+          dst_so.set_sacl(apply_sacl, dst_sd.sacl_inherits?) if dst_sd.sacl_present?
 
-          FileUtils.mv(src, dst)
         end
       end
     end
diff --git a/spec/support/shared/functional/directory_resource.rb b/spec/support/shared/functional/directory_resource.rb
index c345585690..ee9085424e 100644
--- a/spec/support/shared/functional/directory_resource.rb
+++ b/spec/support/shared/functional/directory_resource.rb
@@ -63,6 +63,31 @@ shared_examples_for "a directory resource" do
         end
       end
     end
+
+    # Set up the context for security tests
+    def allowed_acl(sid, expected_perms)
+      [
+       ACE.access_allowed(sid, expected_perms[:specific]),
+       ACE.access_allowed(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE))
+      ]
+    end
+
+    def denied_acl(sid, expected_perms)
+      [
+       ACE.access_denied(sid, expected_perms[:specific]),
+       ACE.access_denied(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE))
+      ]
+    end
+
+    def parent_inheritable_acls
+      dummy_directory_path = File.join(test_file_dir, "dummy_directory")
+      dummy_directory = FileUtils.mkdir_p(dummy_directory_path)
+      dummy_desc = get_security_descriptor(dummy_directory_path)
+      FileUtils.rm_rf(dummy_directory_path)
+      dummy_desc
+    end
+
+    it_behaves_like "a securable resource without existing target"
   end
 
   context "when the target directory exists" do
@@ -120,27 +145,29 @@ shared_examples_for "a directory resource" do
     end
   end
 
-  # Set up the context for security tests
-  def allowed_acl(sid, expected_perms)
-    [
-      ACE.access_allowed(sid, expected_perms[:specific]),
-      ACE.access_allowed(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE))
-    ]
+end
+
+shared_context Chef::Resource::Directory do
+  # We create the  directory than tmp to exercise different file
+  # deployment strategies more completely.
+  let(:test_file_dir) do
+    if windows?
+      File.join(ENV['systemdrive'], "test-dir")
+    else
+      File.join(CHEF_SPEC_DATA, "test-dir")
+    end
   end
 
-  def denied_acl(sid, expected_perms)
-    [
-      ACE.access_denied(sid, expected_perms[:specific]),
-      ACE.access_denied(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE))
-    ]
+  let(:path) do
+    File.join(test_file_dir, make_tmpname(directory_base))
   end
 
-  it_behaves_like "a securable resource without existing target"
-end
+  before do
+    FileUtils::mkdir_p(test_file_dir)
+  end
 
-shared_context Chef::Resource::Directory do
-  let(:path) do
-    File.join(Dir.tmpdir, make_tmpname(directory_base))
+  after do
+    FileUtils::rm_rf(test_file_dir)
   end
 
   after(:each) do
diff --git a/spec/support/shared/functional/file_resource.rb b/spec/support/shared/functional/file_resource.rb
index 31da5c8fbb..ea69aa7032 100644
--- a/spec/support/shared/functional/file_resource.rb
+++ b/spec/support/shared/functional/file_resource.rb
@@ -320,6 +320,14 @@ shared_examples_for "a configured file resource" do
     [ ACE.access_denied(sid, expected_perms[:specific]) ]
   end
 
+  def parent_inheritable_acls
+    dummy_file_path = File.join(test_file_dir, "dummy_file")
+    dummy_file = FileUtils.touch(dummy_file_path)
+    dummy_desc = get_security_descriptor(dummy_file_path)
+    FileUtils.rm_rf(dummy_file_path)
+    dummy_desc
+  end
+
   it_behaves_like "a securable resource without existing target"
 
   context "when the target file has the wrong content" do
diff --git a/spec/support/shared/functional/securable_resource.rb b/spec/support/shared/functional/securable_resource.rb
index d144ad25ab..e68335ec5f 100644
--- a/spec/support/shared/functional/securable_resource.rb
+++ b/spec/support/shared/functional/securable_resource.rb
@@ -21,6 +21,9 @@
 require 'etc'
 
 shared_context "setup correct permissions" do
+  if windows?
+    include_context "use Windows permissions"
+  end
 
   # I could not get this to work with :requires_unprivileged_user for whatever
   # reason. The setup when running as root is the same as non-root, except we
@@ -37,17 +40,19 @@ shared_context "setup correct permissions" do
   end
 
   before :each, :windows_only do
-    Security = Chef::ReservedNames::Win32::Security
-    so = Security::SecurableObject.new(path)
+    so = SecurableObject.new(path)
     so.owner = SID.Administrator
     so.group = SID.Administrators
-    dacl = Security::ACL.create(denied_acl(SID.Guest, expected_full_control_perms) +
-                                allowed_acl(SID.Guest, expected_write_perms))
-    so.set_dacl(dacl, true)
+    dacl = ACL.create(denied_acl(SID.Guest, expected_modify_perms) +
+                      allowed_acl(SID.Guest, expected_read_perms))
+    so.dacl = dacl
   end
 end
 
 shared_context "setup broken permissions" do
+  if windows?
+    include_context "use Windows permissions"
+  end
 
   before :each, :unix_only do
     File.chmod(0644, path)
@@ -58,12 +63,10 @@ shared_context "setup broken permissions" do
   end
 
   before :each, :windows_only do
-    Security = Chef::ReservedNames::Win32::Security
-    so = Security::SecurableObject.new(path)
+    so = SecurableObject.new(path)
     so.owner = SID.Guest
     so.group = SID.Everyone
-    dacl = Security::ACL.create(allowed_acl(SID.Administrator, expected_write_perms) +
-                                denied_acl(SID.Administrator, expected_full_control_perms))
+    dacl = ACL.create(allowed_acl(SID.Guest, expected_modify_perms))
     so.set_dacl(dacl, true)
   end
 end
@@ -72,6 +75,8 @@ shared_context "use Windows permissions", :windows_only do
   if windows?
     SID ||= Chef::ReservedNames::Win32::Security::SID
     ACE ||= Chef::ReservedNames::Win32::Security::ACE
+    ACL ||= Chef::ReservedNames::Win32::Security::ACL
+    SecurableObject ||= Chef::ReservedNames::Win32::Security::SecurableObject
   end
 
   def get_security_descriptor(path)
@@ -222,6 +227,8 @@ shared_examples_for "a securable resource with existing target" do
   end
 
   context "on Windows", :windows_only do
+    include_context "use Windows permissions"
+
     describe "when setting owner" do
       before do
         resource.owner('Administrator')
@@ -254,13 +261,13 @@ shared_examples_for "a securable resource with existing target" do
 
     describe "when setting rights and deny_rights" do
       before do
-        resource.rights(:write, 'Guest')
-        resource.deny_rights(:full_control, 'Guest')
+        resource.deny_rights(:modify, 'Guest')
+        resource.rights(:read, 'Guest')
         resource.run_action(:create)
       end
 
       it "should set the rights and deny_rights" do
-        explicit_aces.should == denied_acl(SID.Guest, expected_full_control_perms) + allowed_acl(SID.Guest, expected_write_perms)
+        explicit_aces.should == denied_acl(SID.Guest, expected_modify_perms) + allowed_acl(SID.Guest, expected_read_perms)
       end
 
       it "is marked as updated only if changes are made" do
@@ -495,6 +502,9 @@ shared_examples_for "a securable resource without existing target" do
     end
 
     it "does not inherit aces if inherits is set to false" do
+      # We need at least one ACE if we're creating a securable without
+      # inheritance
+      resource.rights(:full_control, 'Administrators')
       resource.inherits(false)
       resource.run_action(:create)
 
@@ -506,15 +516,13 @@ shared_examples_for "a securable resource without existing target" do
     it "has the inheritable acls of parent directory if no acl is specified" do
       File.exist?(path).should == false
 
-      resource.run_action(:create)
+      parent_acls = parent_inheritable_acls
 
-      dummy_file_path = File.join(test_file_dir, "dummy_file")
-      dummy_file = FileUtils.touch(dummy_file_path)
-      dummy_desc = get_security_descriptor(dummy_file_path)
+      resource.run_action(:create)
 
       descriptor.dacl.each_with_index do |ace, index|
         ace.inherited?.should == true
-        ace.should == dummy_desc.dacl[index]
+        ace.should == parent_acls.dacl[index]
       end
     end
 
diff --git a/spec/unit/resource/file_spec.rb b/spec/unit/resource/file_spec.rb
index 58a7bd68b3..f937d0035b 100644
--- a/spec/unit/resource/file_spec.rb
+++ b/spec/unit/resource/file_spec.rb
@@ -95,11 +95,6 @@ describe Chef::Resource::File do
       end
     end
 
-    context "on windows", :windows_only do
-      # according to Chef::Resource::File, windows state attributes are rights + deny_rights
-      pending "it describes its state"
-    end
-
     it "returns the file path as its identity" do
       @resource.identity.should == "/tmp/foo.txt"
     end