(MODULES-5152) Fix ACE mutation on output #105

Iristyle · 2017-06-29T00:23:35Z

Existing code mutates an Ace instance in the methods used to emit
string versions of Ace objects - permissions_should_to_s and
permissions_to_s. In particular, the ALL APPLICATION PACKAGES
group is special as it must be specified without the leading
APPLICATION PACKAGE AUTHORITY to properly resolve to a SID
with the Windows API.

Previously, for the sake of outputting the name of the group
to users in the output of change messages / puppet resource,
a SID name would be resolved to a complete name like
APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES and
the original Ace instance would be mutated causing future
misbehavior as a result.
Previously permissions_to_s produced an Array of
Puppet::Type::Acl::Ace objects, but now produces an Array of
Ruby Hash objects, which can still be turned into log messages
just fine. Puppet 5 in particular changed how values are
logged to display to support arbitrarily nested structures.
Update documentation for how to use these accounts correctly.
Add integration tests for APPLICATION PACKAGES - though these
unfortunately do not prove that the accounts can be used in a
real manifest.

glennsarti · 2017-06-29T00:27:54Z

Looks like an id='' keyvalue pair has snuck into the tests...nvm you just push a commit 👍

Iristyle · 2017-06-29T00:33:22Z

Bummer.. output isn't quite right here as it now drops the original ACEs:

Notice: /Stage[main]/Main/Acl[c:\test]/permissions: permissions changed [] to [
 { identity => 'APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES', id => 'S-1-15-2-1', rights => ["full"] }]
Notice: /Stage[main]/Main/Acl[c:\test]/inherit_parent_permissions: inherit_parent_permissions changed true to 'false'

But at least it doesn't crash.

Iristyle · 2017-06-29T00:36:15Z

lib/puppet/provider/acl/windows.rb

+        perm_hash = perm.to_hash
+        perm_hash['identity'] = get_account_name(perm.identity)
+        # TODO: is it just as easy to perm.dup instead?
+        Puppet::Type::Acl::Ace.new(perm_hash, nil)


Look into just returning an array of strings / hashes here instead... or calling .to_s directly on the Ace object and supplanting the identity.

Note that the behavior on Puppet 4 / Puppet 5 will be different.

Iristyle · 2017-06-29T00:40:47Z

lib/puppet/type/acl/ace.rb

@@ -322,7 +323,7 @@ def inspect
        if key_value.is_a? Array
          "#{key} => #{key_value}"
        else
-          "#{key} => '#{key_value}'"
+          "#{key} => '#{key_value}'" unless (key == 'id' && key_value.nil?)


Gross... designed to not emit an empty SID (to match prior behavior).

Might have to scrap adding id to the returned hash.

Iristyle · 2017-06-30T14:41:23Z

lib/puppet/type/acl.rb

-        "'#{value}'"
-      end
-    end
+    # def self.format_value_for_display(value)


Commented this just to see if it's actually called anywhere... its unused in specs on OSX, but maybe it is called on Windows somehow?

Iristyle · 2017-07-06T00:41:02Z

Testing with manifest:

file { 'c:/test': ensure => 'directory' } ~>
acl { 'c:/test':
  purge => 'true',
  permissions => [{ identity => 'ALL APPLICATION PACKAGES', rights => ['full']},],
  inherit_parent_permissions => 'false',
}

Iristyle · 2017-07-25T20:49:33Z

@eputnam 6333f0b has the combined fixes for Puppet 5 / Ruby 2.4 / test changes / ubergem pinning.

- Although some of the changes in this PR should be independent, it was necessary to combine them to get a passing commit, given the timeline of some related releases. - rspec-puppet introduced some monkey patching in 2.6.0 which breaks Windows on Windows testing - introduced in rodjek/rspec-puppet#498 - AppVeyor won't demonstrate the problem given Puppet 4.2.3 has been removed from the matrix, but this PR also pins puppet-module-win-dev-r21 back to `0.0.7` for a few reasons. 0.0.8+ doesn't work properly in Jenkins due to the addition of rspec-puppet-facts which requires json and triggers compilation of a native extension in spec environments without compilers. The 0.0.9 / 0.0.10 releases include a dependency on metadata-json-list 2.0.0+ which requires Puppet 4.7+ - Finally, the behavior of Puppet format_value_for_display changed in Puppet 5, which produces different output for change notifications and `puppet resource` in Puppet 5. This module will likely need to be refactored later to simplify the use of Ruby types (to remove Ace deriving from Hash), but for now, changing just the test to accurately portray Puppet behavior is sufficient.

- Use manifests to prove that the ACL mdoule can properly handle the identities in APPLICATION PACKAGE AUTHORITY - namely S-1-15-2-1 / ALL APPLICATION PACKAGES and S-1-15-2-2 / ALL RESTRICTED APPLICATION PACKAGES. Note that accounts in APPLICATION PACKAGE AUTHORITY are handled strangely in the Windows API, and are not resolvable when fully qualified with the LookupAccountName like other accounts are. Therefore there are no tests showing that the fully qualified name actually works, and the README for this module will be updated accordingly.

- Existing code mutates an Ace instance in the methods used to emit string versions of Ace objects - permissions_should_to_s and permissions_to_s. In particular, the `ALL APPLICATION PACKAGES` group is special as it must be specified without the leading `APPLICATION PACKAGE AUTHORITY` to properly resolve to a SID with the Windows API. Previously, for the sake of outputting the name of the group to users in the output of change messages / `puppet resource`, a SID name would be resolved to a complete name like `APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES` *and* the original Ace instance would be mutated causing future misbehavior as a result. - Previously permissions_to_s produced an Array of Puppet::Type::Acl::Ace objects, but now produces an Array of Ruby Hash objects, which can still be turned into log messages just fine. Puppet 5 in particular changed how values are logged to display to support arbitrarily nested structures. - Update documentation for how to use these accounts correctly. - Add integration tests for APPLICATION PACKAGES - though these unfortunately do not prove that the accounts can be used in a real manifest.

eputnam

looks good from a non-windows perspective

glennsarti

Apart from a typo, ready for merge!

glennsarti · 2017-07-26T03:19:49Z

README.md

@@ -443,6 +443,11 @@ To ensure that a specific set of permissions are absent from the ACL, set `purge

 * When using SIDs for identities, autorequire tries to match to users with fully qualified names (e.g., User[BUILTIN\Administrators]) in addition to SIDs (User[S-1-5-32-544]). However, it can't match against 'User[Administrators]', because that could cause issues if domain accounts and local accounts share the same name e.g., 'Domain\Bob' and 'LOCAL\Bob'.

+ * When referring to accounts in the `APPLICATION PACKAGE AUTHORITY`, use either their SID values or their unqualified names. The Windows API has well documented bugs preventing the fully qualifed account names from being used.


Think it's missing a word "accounts in the APPLICATION PACKAGE AUTHORITY" should be "accounts in the APPLICATION PACKAGE AUTHORITY domain" ?

In Windows terms, I believe they're actually referred to as authorities, even though they look like domains.

Other authorities (including SECURITY_APP_PACKAGE_AUTHORITY ({0,0,0,0,0,15} AKA S-1-15)):
https://msdn.microsoft.com/en-us/library/dd302645.aspx

The new defs in WinNT.h (added for Windows 8) - https://recxltd.blogspot.com/2012/03/windows-8-app-container-security-notes.html

// // Application Package Authority. // #define SECURITY_APP_PACKAGE_AUTHORITY {0,0,0,0,0,15} #define SECURITY_APP_PACKAGE_BASE_RID (0x00000002L) #define SECURITY_BUILTIN_APP_PACKAGE_RID_COUNT (2L) #define SECURITY_APP_PACKAGE_RID_COUNT (8L) #define SECURITY_CAPABILITY_BASE_RID (0x00000003L) #define SECURITY_BUILTIN_CAPABILITY_RID_COUNT (2L) #define SECURITY_CAPABILITY_RID_COUNT (5L)

AllocateAndInitializeSid

Ahh ok so APPLICATION PACKAGE AUTHORITY is an authority and it would look odd to have " ... in the APPLICATION PACKAGE AUTHORITY authority. PR is good for merge.

Iristyle changed the title ~~(MODULES-2152) Fix ACE ctor / unwanted mutation~~ (MODULES-5152) Fix ACE ctor / unwanted mutation Jun 29, 2017

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch from 2bc8493 to 88fab9a Compare June 29, 2017 00:27

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch from 88fab9a to 709cd72 Compare June 29, 2017 00:28

Iristyle commented Jun 29, 2017

View reviewed changes

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch from 709cd72 to 480984e Compare June 29, 2017 00:39

Iristyle commented Jun 29, 2017

View reviewed changes

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch 2 times, most recently from dd4a78b to 18b147f Compare June 29, 2017 04:19

Iristyle commented Jun 30, 2017

View reviewed changes

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch 7 times, most recently from aa5d1fd to c7b2ea2 Compare June 30, 2017 23:20

eputnam added bugfix needs-tests labels Jul 18, 2017

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch 8 times, most recently from d5976c7 to d2390e1 Compare July 25, 2017 18:58

Iristyle changed the title ~~(MODULES-5152) Fix ACE ctor / unwanted mutation~~ (MODULES-5152) Fix ACE mutation on output Jul 25, 2017

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch from be02ad1 to 94ff5e7 Compare July 25, 2017 20:55

Iristyle force-pushed the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch from ab96e40 to 6333f0b Compare July 25, 2017 21:31

Iristyle added 3 commits July 25, 2017 14:31

(maint) Fix invalid Markdown / typos

7ebdcaa

eputnam removed the needs-tests label Jul 25, 2017

eputnam mentioned this pull request Jul 25, 2017

(MODULES-5187) mysnc puppet 5 and ruby 2.4 #106

Closed

eputnam approved these changes Jul 25, 2017

View reviewed changes

glennsarti reviewed Jul 26, 2017

View reviewed changes

glennsarti merged commit 6eac937 into puppetlabs:master Jul 27, 2017

Iristyle deleted the ticket/MODULES-5152-error-with-ALL-APPLICATION-PACKAGES branch July 27, 2017 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-5152) Fix ACE mutation on output #105

(MODULES-5152) Fix ACE mutation on output #105

Iristyle commented Jun 29, 2017 •

edited

Loading

glennsarti commented Jun 29, 2017

Iristyle commented Jun 29, 2017

Iristyle Jun 29, 2017

Iristyle Jun 29, 2017

Iristyle Jun 30, 2017

Iristyle commented Jul 6, 2017

Iristyle commented Jul 25, 2017 •

edited

Loading

eputnam left a comment

glennsarti left a comment

glennsarti Jul 26, 2017

Iristyle Jul 26, 2017 •

edited

Loading

Iristyle Jul 26, 2017

glennsarti Jul 27, 2017

		@@ -443,6 +443,11 @@ To ensure that a specific set of permissions are absent from the ACL, set `purge

		* When using SIDs for identities, autorequire tries to match to users with fully qualified names (e.g., User[BUILTIN\Administrators]) in addition to SIDs (User[S-1-5-32-544]). However, it can't match against 'User[Administrators]', because that could cause issues if domain accounts and local accounts share the same name e.g., 'Domain\Bob' and 'LOCAL\Bob'.

		* When referring to accounts in the `APPLICATION PACKAGE AUTHORITY`, use either their SID values or their unqualified names. The Windows API has well documented bugs preventing the fully qualifed account names from being used.

(MODULES-5152) Fix ACE mutation on output #105

(MODULES-5152) Fix ACE mutation on output #105

Conversation

Iristyle commented Jun 29, 2017 • edited Loading

glennsarti commented Jun 29, 2017

Iristyle commented Jun 29, 2017

Iristyle Jun 29, 2017

Choose a reason for hiding this comment

Iristyle Jun 29, 2017

Choose a reason for hiding this comment

Iristyle Jun 30, 2017

Choose a reason for hiding this comment

Iristyle commented Jul 6, 2017

Iristyle commented Jul 25, 2017 • edited Loading

eputnam left a comment

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

glennsarti Jul 26, 2017

Choose a reason for hiding this comment

Iristyle Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Iristyle Jul 26, 2017

Choose a reason for hiding this comment

glennsarti Jul 27, 2017

Choose a reason for hiding this comment

Iristyle commented Jun 29, 2017 •

edited

Loading

Iristyle commented Jul 25, 2017 •

edited

Loading

Iristyle Jul 26, 2017 •

edited

Loading