Uploaded image for project: 'Modules'
  1. Modules
  2. MODULES-5051

ACL 2.0 doesn't work with corrective change

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Ready for Engineering
    • Priority: Normal
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: acl
    • Labels:
      None
    • Template:
      MODULES Bug Template
    • Epic Link:
    • Team:
      Windows
    • Sprint:
      Windows Hopper
    • Method Found:
      Needs Assessment
    • Zendesk Ticket IDs:
      34644
    • Zendesk Ticket Count:
      1
    • QA Risk Assessment:
      Manual
    • QA Risk Assessment Reason:
      UI manual test

      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

      /cc Ethan Brown, Glenn Sarti, Thomas Hallgren, Geoff Nichols

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              ethan Ethan Brown
              Reporter:
              josh Josh Cooper
              Votes:
              1 Vote for this issue
              Watchers:
              11 Start watching this issue

                Dates

                Created:
                Updated:

                  Zendesk Support