Skip to content

Commit

Permalink
Merge pull request #3226 from MikaelSmith/task/master/PUP-2609-Window…
Browse files Browse the repository at this point in the history 
…s-error-on-use-source-permissions

(PUP-2609) Windows error on use source permissions
  • Loading branch information
@ferventcoder
ferventcoder committed Oct 27, 2014
2 parents 6163693 + 02bf973 commit a52294f
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 226 deletions.
43 changes: 3 additions & 40 deletions lib/puppet/file_serving/metadata.rb
View file
Expand Up @@ -27,7 +27,7 @@ class MetaStat

def initialize(stat, source_permissions = nil)
@stat = stat
@source_permissions_ignore = source_permissions == :ignore
@source_permissions_ignore = (!source_permissions || source_permissions == :ignore)
end

def owner
Expand All @@ -53,52 +53,15 @@ class WindowsStat < MetaStat
def initialize(stat, path, source_permissions = nil)
super(stat, source_permissions)
@path = path
raise(ArgumentError, "Unsupported Windows source permissions option #{source_permissions}") unless @source_permissions_ignore
end

{ :owner => 'S-1-5-32-544',
:group => 'S-1-0-0',
:mode => 0644
}.each do |method, default_value|
define_method method do
return default_value if @source_permissions_ignore

# this code remains for when source_permissions is not set to :ignore
begin
Puppet::Util::Windows::Security.send("get_#{method}", @path) || default_value
rescue Puppet::Util::Windows::Error => detail
# Very carefully catch only this specific error that result from
# trying to read permissions on a symlinked file that is on a volume
# that does not support ACLs.
#
# Unfortunately readlink method will not return the target path when
# the given path is not the symlink.
#
# For instance, consider:
# symlink c:\link points to c:\target
# FileSystem.readlink('c:/link') returns 'c:/target'
# FileSystem.readlink('c:/link/file') will NOT return 'c:/target/file'
#
# Since detecting this up front is costly, since the path in question
# needs to be recursively split and tested at each depth in the path,
# we catch the standard error that will result from trying to read a
# file that doesn't have a DACL - 1336 is ERROR_INVALID_DACL
#
# Note that this affects any manually created symlinks as well as
# paths like puppet:///modules
return default_value if detail.code == 1336

# Also handle a VirtualBox bug where ERROR_INVALID_FUNCTION is
# returned when following a symlink to a volume that is not NTFS.
# It appears that the VirtualBox file system is not propagating
# the standard Win32 error code above like it should.
#
# Apologies to all who enter this code path at a later date
if detail.code == 1 && Facter.value(:virtual) == 'virtualbox'
return default_value
end

raise
end
return default_value
end
end
end
Expand Down
7 changes: 3 additions & 4 deletions lib/puppet/type/file/source.rb
View file
Expand Up @@ -127,11 +127,10 @@ def copy_source_values
if [:use, :use_when_creating].include?(resource[:source_permissions]) &&
(resource[:owner] == nil || resource[:group] == nil || resource[:mode] == nil)

warning = "Copying %s from the source" <<
" file on Windows is deprecated;" <<
err_msg = "Copying %s from the source" <<
" file on Windows is not supported;" <<
" use source_permissions => ignore."
Puppet.deprecation_warning(warning % 'owner/mode/group')
resource.debug(warning % metadata_method.to_s)
self.fail Puppet::Error, err_msg % 'owner/mode/group'
end
# But never try to copy remote owner/group on Windows
next if [:owner, :group].include?(metadata_method) && !local?
Expand Down
132 changes: 24 additions & 108 deletions spec/integration/type/file_spec.rb
View file
Expand Up @@ -949,23 +949,25 @@ def build_path(dir)
describe "when sourcing" do
let(:source) { tmpfile_with_contents("source_default_values", "yay") }

it "should apply the source metadata values" do
set_mode(0770, source)
describe "on POSIX systems", :if => Puppet.features.posix? do
it "should apply the source metadata values" do
set_mode(0770, source)

file = described_class.new(
:path => path,
:ensure => :file,
:source => source,
:source_permissions => :use,
:backup => false
)
file = described_class.new(
:path => path,
:ensure => :file,
:source => source,
:source_permissions => :use,
:backup => false
)

catalog.add_resource file
catalog.apply
catalog.add_resource file
catalog.apply

get_owner(path).should == get_owner(source)
get_group(path).should == get_group(source)
(get_mode(path) & 07777).should == 0770
get_owner(path).should == get_owner(source)
get_group(path).should == get_group(source)
(get_mode(path) & 07777).should == 0770
end
end

it "should override the default metadata values" do
Expand All @@ -975,7 +977,6 @@ def build_path(dir)
:path => path,
:ensure => :file,
:source => source,
:source_permissions => :use,
:backup => false,
:mode => '0440'
)
Expand Down Expand Up @@ -1019,26 +1020,6 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path)
expects_at_least_one_inherited_ace_grants_full_access(path, @sids[:system])
end

it "should provide valid default values when ACLs are not supported" do
Puppet::Util::Windows::Security.stubs(:supports_acl?).returns(false)
Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false

file = described_class.new(
:path => path,
:ensure => :file,
:source => source,
:backup => false,
:source_permissions => :use
)

catalog.add_resource file
catalog.apply

get_owner(path).should =~ /^S\-1\-5\-.*$/
get_group(path).should =~ /^S\-1\-0\-0.*$/
get_mode(path).should == 0644
end

describe "when processing SYSTEM ACEs" do
before do
@sids = {
Expand All @@ -1063,56 +1044,26 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path)
catalog.add_resource @file
end

describe "when source permissions are ignored" do
it "preserves the inherited SYSTEM ACE" do
catalog.apply

expects_at_least_one_inherited_system_ace_grants_full_access(path)
end
end

describe "when permissions are insync?" do
before :each do
@file[:source_permissions] = :use
end

it "preserves the explicit SYSTEM ACE" do
FileUtils.touch(path)

sd = Puppet::Util::Windows::Security.get_security_descriptor(path)
sd.protect = true
sd.owner = @sids[:none]
sd.group = @sids[:none]
Puppet::Util::Windows::Security.set_security_descriptor(source, sd)
Puppet::Util::Windows::Security.set_security_descriptor(path, sd)

catalog.apply

expects_system_granted_full_access_explicitly(path)
end
end

describe "when permissions are not insync?" do
before :each do
@file[:owner] = 'None'
@file[:group] = 'None'
@file[:source_permissions] = :use
end

it "replaces inherited SYSTEM ACEs with an uninherited one for an existing file" do
it "preserves the inherited SYSTEM ACE for an existing file" do
FileUtils.touch(path)

expects_at_least_one_inherited_system_ace_grants_full_access(path)

catalog.apply

expects_system_granted_full_access_explicitly(path)
expects_at_least_one_inherited_system_ace_grants_full_access(path)
end

it "replaces inherited SYSTEM ACEs for a new file with an uninherited one" do
it "applies the inherited SYSTEM ACEs for a new file" do
catalog.apply

expects_system_granted_full_access_explicitly(path)
expects_at_least_one_inherited_system_ace_grants_full_access(path)
end
end

Expand All @@ -1121,7 +1072,6 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path)
@file[:owner] = @sids[:users]
@file[:group] = @sids[:system]
@file[:mode] = '0644'
@file[:source_permissions] = :use

catalog.apply
end
Expand Down Expand Up @@ -1182,60 +1132,26 @@ def grant_everyone_full_access(path)
grant_everyone_full_access(dir)
end

describe "when source permissions are ignored" do
it "preserves the inherited SYSTEM ACE" do
catalog.apply

expects_at_least_one_inherited_system_ace_grants_full_access(dir)
end
end

describe "when permissions are insync?" do
before :each do
@directory[:source_permissions] = :use
end

it "preserves the explicit SYSTEM ACE" do
Dir.mkdir(dir)

source_dir = tmpdir('source_dir')
@directory[:source] = source_dir

sd = Puppet::Util::Windows::Security.get_security_descriptor(source_dir)
sd.protect = true
sd.owner = @sids[:none]
sd.group = @sids[:none]
Puppet::Util::Windows::Security.set_security_descriptor(source_dir, sd)
Puppet::Util::Windows::Security.set_security_descriptor(dir, sd)

catalog.apply

expects_system_granted_full_access_explicitly(dir)
end
end

describe "when permissions are not insync?" do
before :each do
@directory[:owner] = 'None'
@directory[:group] = 'None'
@directory[:mode] = '0444'
@directory[:source_permissions] = :use
end

it "replaces inherited SYSTEM ACEs with an uninherited one for an existing directory" do
it "preserves the inherited SYSTEM ACEs for an existing directory" do
FileUtils.mkdir(dir)

expects_at_least_one_inherited_system_ace_grants_full_access(dir)

catalog.apply

expects_system_granted_full_access_explicitly(dir)
expects_at_least_one_inherited_system_ace_grants_full_access(dir)
end

it "replaces inherited SYSTEM ACEs with an uninherited one for an existing directory" do
it "applies the inherited SYSTEM ACEs for a new directory" do
catalog.apply

expects_system_granted_full_access_explicitly(dir)
expects_at_least_one_inherited_system_ace_grants_full_access(dir)
end

describe "created with SYSTEM as the group" do
Expand Down

0 comments on commit a52294f

Please sign in to comment.