Details
Description
Module Version: 2.0
Puppet Version: 4.6.1 and up
OS Name/Version: All Windows
If the puppet modifies an ACE that it has previously managed (so there's an entry in the transactionstore.yaml), then puppet cannot determine if the next change was corrective or not, and will log an exception at "Info" level like the following:
Info: Unknown failure using insync_values? on type: Acl[c:/temp/foo] / property: permissions to compare values [{"identity"=>"NT AUTHORITY\\SYSTEM", "rights"=>["read"], "affects"=>:self_only}] and [
|
{ identity => 'NT AUTHORITY\SYSTEM', rights => ["read"], affects => 'self_only' },
|
...
|
]
|
The problem was introduced with the fix for MODULES-4275, which ironically was to fix a related issue with the ACL module and corrective change on ruby 2.3.
The issue is that as of ACL 2.0, we correctly no longer emit class information in the transactionstore.yaml. So it looks like:
Acl[c:/temp/foo]:
|
parameters:
|
permissions:
|
system_value:
|
- identity: NT AUTHORITY\SYSTEM
|
rights:
|
- write
|
affects: :self_only
|
owner:
|
system_value:
|
- S-1-5-32-544
|
inherit_parent_permissions:
|
system_value:
|
- :true
|
However, when you run puppet the next time, it will load the above Ace as a Hash, and will call Puppet::Provider::Acl::Windows#permissions_insync?(current, should) where the current value is an Ace, but the should value is a Hash. The provider then tries to call Hash#affects which raises an exception. Note the exception is logged at Info level, which does not appear when running puppet apply, but does if you specify --verbose or if you run puppet agent -t because test implies verbose.
The root cause issue is that the provider's permissions method returns an Array of Ace objects, which end up in reports and the transactionstore. It should only return objects that are safe to serialize within the puppet runtime.
This issue is made worse in Puppet 5, because as of PUP-7382, puppet will emit a warning if the provider's current value is not serializable as Data, e.g. Hash, Array, String, etc it spews out a ton of warnings:
Notice: Applied catalog in 0.11 seconds
|
Warning: Event['previous_value'][0] contains a Puppet::Type::Acl::Ace value. It will be converted to the String '
|
{ identity => 'NT AUTHORITY\SYSTEM', rights => ["write"], affects => 'self_only' }'
|
Warning: Event['previous_value'][0]['previous_value'][1] contains a Puppet::Type::Acl::Ace value. It will be converted to the String '
|
{ identity => 'BUILTIN\Administrators', rights => ["full"], affects => 'self_only' }'
|
Warning: Event['previous_value'][0]['previous_value'][1]['previous_value'][2] contains a Puppet::Type::Acl::Ace value. It will be converted to the String '
|
{ identity => 'BUILTIN\Users', rights => ["full"], affects => 'self_only' }'
|
...
|
To reproduce:
C:\work>git clone puppet
|
C:\work>cd puppet
|
C:\work\puppet>git checkout 4.6.1
|
C:\work\puppet>bundle install --path .bundle --without development extra
|
C:\work\puppet>bundle exec puppet module install puppetlabs-acl
|
C:\work\puppet>cat manifest.pp
|
file { 'c:/temp':
|
ensure => directory,
|
}
|
file { 'c:/temp/foo':
|
ensure => file,
|
}
|
acl { 'c:/temp/foo':
|
owner => 'S-1-5-32-544',
|
permissions => [
|
{'identity' => 'NT AUTHORITY\SYSTEM', 'rights' => ['full']},
|
]
|
}
|
Apply the manifest once, it will succeed:
C:\work\puppet>bundle exec puppet apply manifest.pp --trace
|
Change the rights from full to read, and reapply the manifest, it will generate an Info message (Note in 4.6.1 it was logged at Error level, but that was changed to Info in 4.6.2 in PUP-6629):
C:\work\puppet>bundle exec puppet apply manifest.pp --trace
|
Notice: Compiled catalog for win-qp47voha2p4.solar.lan in environment production in 0.14 seconds
|
Info: Unknown failure comparing values [{"identity"=>"NT AUTHORITY\\SYSTEM", "rights"=>["write"], "affects"=>:self_only
|
}] and [
|
{ identity => 'NT AUTHORITY\SYSTEM', rights => ["write"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Administrators', rights => ["full"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Users', rights => ["full"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Administrators', rights => ["full"], affects => 'self_only', is_inherited => 'true' },
|
{ identity => 'NT AUTHORITY\SYSTEM', rights => ["full"], affects => 'self_only', is_inherited => 'true' },
|
{ identity => 'BUILTIN\Users', rights => ["read", "execute"], affects => 'self_only', is_inherited => 'true' }] using insync? on type: Acl[c:/temp/foo] property: permissions
|
C:/ProgramData/PuppetLabs/code/environments/production/modules/acl/lib/puppet/provider/acl/windows.rb:86:in `block in update_permissions_if_file'
|
C:/ProgramData/PuppetLabs/code/environments/production/modules/acl/lib/puppet/provider/acl/windows.rb:85:in `each'
|
C:/ProgramData/PuppetLabs/code/environments/production/modules/acl/lib/puppet/provider/acl/windows.rb:85:in `update_permissions_if_file'
|
C:/ProgramData/PuppetLabs/code/environments/production/modules/acl/lib/puppet/provider/acl/windows.rb:97:in `permissions_insync?'
|
C:/ProgramData/PuppetLabs/code/environments/production/modules/acl/lib/puppet/type/acl.rb:237:in `insync?'
|
C:/work/puppet/lib/puppet/property.rb:366:in `insync_values?'
|
C:/work/puppet/lib/puppet/transaction/event.rb:112:in `calculate_corrective_change'
|
C:/work/puppet/lib/puppet/transaction/resource_harness.rb:161:in `sync_if_needed'
|
...
|
Notice: /Stage[main]/Main/Acl[c:/temp/foo]/permissions: permissions changed [
|
{ identity => 'NT AUTHORITY\SYSTEM', rights => ["write"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Administrators', rights => ["full"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Users', rights => ["full"], affects => 'self_only' }
|
] to [
|
{ identity => 'NT AUTHORITY\SYSTEM', rights => ["full"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Administrators', rights => ["full"], affects => 'self_only' },
|
{ identity => 'BUILTIN\Users', rights => ["full"], affects => 'self_only' }
|
]
|
Notice: Applied catalog in 0.14 seconds
|
Desired Behavior
1. The transactionstore should continue to contain safe Data (so no !ruby/object tags).
2. The ACL module's permissions method should return an Array of Hashes, so that the Ace objects don't leak outside of the module
3. It should be possible to manage ACLs without warnings when verbose is enabled using ruby 2.1.9 and 2.4.1. This includes 3 scenarios which should all be represented correctly in the report:
- Managing the ACL the first time
- Updating the manifest, and ensuring puppet makes an intentional change
- Updating the manifest, and ensuring puppet makes a corrective change
Attachments
Issue Links
- relates to
-
PA-1374 Windows prop_file negative test failed "Expected ACL change event not detected!"
-
- Resolved
-
-
MODULES-4275 ACL module - Failed to apply catalog: undefined method `split' for :windows:Symbol
-
- Resolved
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
MODULES-3766 ACL - ace should appear as a hash so it gets serialized/deserialized properly
-
- Ready for Engineering
-
