diff options
-rw-r--r-- | lib/chef/config.rb | 9 | ||||
-rw-r--r-- | lib/chef/file_access_control/windows.rb | 29 | ||||
-rw-r--r-- | lib/chef/file_content_management/deploy/mv_windows.rb | 51 | ||||
-rw-r--r-- | spec/support/shared/functional/directory_resource.rb | 59 | ||||
-rw-r--r-- | spec/support/shared/functional/file_resource.rb | 8 | ||||
-rw-r--r-- | spec/support/shared/functional/securable_resource.rb | 42 | ||||
-rw-r--r-- | spec/unit/resource/file_spec.rb | 5 |
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 |