Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File resource permissions for windows #783

Closed
mhedgpeth opened this issue Jun 6, 2016 · 10 comments
Closed

File resource permissions for windows #783

mhedgpeth opened this issue Jun 6, 2016 · 10 comments
Assignees
Labels
Type: Bug Feature not working as expected
Milestone

Comments

@mhedgpeth
Copy link

Description

Right now permissions can't be tested with inspec on windows.

This won't work:

control 'chef-installation-should-be-readable' do
  impact 1
  title 'Chef Installation should be readable by vagrant user'
  desc 'To make sure we can run chef, the install should be readable'
  describe directory('C:/opscode/chef') do
    it { should be_readable.by('vagrant') }
  end
end

It gives the error:

 1) File C:/opscode/chef should be readable by vagrant
    Failure/Error: fail 'Checking file permissions is not supported on your os'

    RuntimeError:
      Checking file permissions is not supported on your os
    # ./test/integration/default/inspec/image.rb:27:in `block (3 levels) in load'

InSpec and Platform Version

0.24

Replication Case

Create a cookbook with inspec as the verifier with ncr/win2008r2 as the virtualization box for kitchen. Run kitchen verify.

Possible Solutions

It's simply not coded yet in the File resource. I assume this is because file permissions on the command line are difficult; it's probably easier on Powershell but I'm not sure how you do that.

@chris-rock chris-rock added the Type: Bug Feature not working as expected label Jun 8, 2016
@chris-rock
Copy link
Contributor

@mhedgpeth Thanks for reporting. We need to break this down into two cases:

  • it { should be_readable.by('owner') } will not be supported on Windows since this is requesting unix file permissions. I agree that this is hard to read. We added this to be compatible with Serverspec http://serverspec.org/resource_types.html#file
  • it { should be_owned_by 'vagrant' } should be supported on Windows. The implementation is not trivial, but should be part of InSpec.
control 'chef-installation-should-be-readable' do
  impact 1
  title 'Chef Installation should be readable by vagrant user'
  desc 'To make sure we can run chef, the install should be readable'
  describe directory('C:/opscode/chef') do
    # verifies specific users
    it { should be_owned_by 'vagrant' }
    it { should be_readable.by_user('vagrant') }
    it { should be_writable.by_user('vagrant') }

    # only applicable to unix group system
    # it { should be_readable.by('owner') }
    # it { should be_readable.by('group') }
    # it { should be_readable.by('others') }
  end
end

@chris-rock chris-rock added this to the 1.0.0 milestone Jul 26, 2016
@chris-rock chris-rock self-assigned this Aug 2, 2016
@chris-rock chris-rock modified the milestones: 0.29.0, 1.0.0, 0.30.0 Aug 2, 2016
@chris-rock chris-rock modified the milestones: 0.31.0, 0.30.0 Aug 12, 2016
@chris-rock chris-rock removed their assignment Aug 16, 2016
@chris-rock
Copy link
Contributor

The following powershell commands are useful to get this running:

# File permissions for user
  (Get-Acl .\test).Access | ConvertTo-Json
  [
    {
        "FileSystemRights":  2032127,
        "AccessControlType":  0,
        "IdentityReference":  {
                                  "Value":  "NT AUTHORITY\\SYSTEM"
                              },
        "IsInherited":  true,
        "InheritanceFlags":  3,
        "PropagationFlags":  0
    },
    {
        "FileSystemRights":  2032127,
        "AccessControlType":  0,
        "IdentityReference":  {
                                  "Value":  "BUILTIN\\Administrators"
                              },
        "IsInherited":  true,
        "InheritanceFlags":  3,
        "PropagationFlags":  0
    },
    {
        "FileSystemRights":  1179817,
        "AccessControlType":  0,
        "IdentityReference":  {
                                  "Value":  "BUILTIN\\Users"
                              },
        "IsInherited":  true,
        "InheritanceFlags":  3,
        "PropagationFlags":  0
    },
    {
        "FileSystemRights":  4,
        "AccessControlType":  0,
        "IdentityReference":  {
                                  "Value":  "BUILTIN\\Users"
                              },
        "IsInherited":  true,
        "InheritanceFlags":  1,
        "PropagationFlags":  0
    },
    {
        "FileSystemRights":  2,
        "AccessControlType":  0,
        "IdentityReference":  {
                                  "Value":  "BUILTIN\\Users"
                              },
        "IsInherited":  true,
        "InheritanceFlags":  1,
        "PropagationFlags":  0
    },
    {
        "FileSystemRights":  268435456,
        "AccessControlType":  0,
        "IdentityReference":  {
                                  "Value":  "CREATOR OWNER"
                              },
        "IsInherited":  true,
        "InheritanceFlags":  3,
        "PropagationFlags":  2
    }
]

Maybe we could also make our live easier and use CSV and parse the results, because this includes the name of the file permission rights

PS C:\> (Get-Acl .\chef).Access | ConvertTo-Csv
#TYPE System.Security.AccessControl.FileSystemAccessRule
"FileSystemRights","AccessControlType","IdentityReference","IsInherited","InheritanceFlags","PropagationFlags"
"FullControl","Allow","NT AUTHORITY\SYSTEM","True","ContainerInherit, ObjectInherit","None"
"FullControl","Allow","BUILTIN\Administrators","True","ContainerInherit, ObjectInherit","None"
"ReadAndExecute, Synchronize","Allow","BUILTIN\Users","True","ContainerInherit, ObjectInherit","None"
"AppendData","Allow","BUILTIN\Users","True","ContainerInherit","None"
"CreateFiles","Allow","BUILTIN\Users","True","ContainerInherit","None"
"268435456","Allow","CREATOR OWNER","True","ContainerInherit, ObjectInherit","InheritOnly"

In addition we need to read the groups of the user to see if he is allowed to access it:

# read all groups ids of user
PS C:\> [System.Security.Principal.WindowsIdentity]::GetCurrent().Groups | Select-Object -Property Value | ConvertTo-Jso
n

# Get name
[System.Security.Principal.WindowsIdentity]::GetCurrent().Name

@chris-rock
Copy link
Contributor

A potential integration test would look like:

  # verifies if a specific user has access to a file
  describe directory('C:/opscode/chef') do
    it { should be_readable.by_user('vagrant') }
    it { should be_writable.by_user('vagrant') }
  end

@chris-rock chris-rock assigned chris-rock and unassigned ksubrama and vjeffrey Aug 23, 2016
@ksubrama
Copy link

ksubrama commented Aug 25, 2016

Support for it { should be_owned_by 'username' } is partially available. The current ownership check is based on the complete (domain qualified) username - so you might need to use $env:COMPUTERNAME or equivalent to get the test to pass.

Supporting it { should be_readable.by_user('vagrant') } (and the equivalent for writable or executable) is harder to do correctly on Windows. File permissions are inherited in a fairly complicated way on windows. See https://web.archive.org/web/20090106125350/http://alt.pluralsight.com/wiki/default.aspx/Keith.GuideBook/WhatIsACLInheritance.html for a primer. The DACL on a securable object (such as a file or directory) is first considered. Negative ACEs in a DACL take precedence of positive ACEs. The user name or group name would have to be checked against each ACE to see one of them explicitly denies the requested permission. If one is found, the check fails. Then the positive ACEs are scanned to see if there is one that explicitly grants the requested permission. If one is found, the check succeeds immediately. If neither of these happens and permissions inheritance is enabled on the object, then the parent object's DACL is considered using the same rules as above in a recursive manner. If a root object is reached and its DACL is exhausted without any explicit grant or deny of the permission being checked, then the overall check fails.

This algorithm isn't too tricky to write by itself but the twist in the matter is the part where we check a user against a single ACE. An ACE that applies to a group that the user belongs to also applies to the user. So while checking the ACEs above, one also needs to know the group membership of the user in question. Powershell 5.1 has a LocalAccounts module that might make such a query a bit easier. Unfortunately, in the mean time, the way we can look up group membership of users (or arbitrary account names) is to use either ADSI (Active Directory Service Interface) or WMI. The Get-WmiObject Win32_GroupUser interface seems like the best option - see https://gallery.technet.microsoft.com/List-local-group-members-762b48c5 for an example. It only handles local groups though and I have not yet ascertained that it handles transitive group inclusions. The ADSI option is extremely slow to call into - it takes a few seconds per call and that can easily add up. See http://www.lazywinadmin.com/2012/12/get-localgroupmembership-using-adsiwinnt.html for an example.

One other possibility when it comes to implementing be_readable.by_user is to simply perform the action and see if we get a security exception or not. This is actually the recommended technique as far as Microsoft is concerned because a certain user might not even have the rights to list permissions on an object that they otherwise can read from. You might have write permissions on an object that you cannot read. While implementing such a behavior for the current user is easy, impersonating another user would require their credentials to manufacture a token on their behalf. Even the current user case has a few edge cases - trying to check if the current user has permission to write a file requires opening said file in write mode. If the file doesn't exist, a new file will be created. To avoid that, we'd need to check if a file exists at a location or not. The user might not have permissions to list files in a directory while still having permissions to write/create files in that directory. They may not even have the ability to traverse that directory but due to https://technet.microsoft.com/en-us/library/dn221950(v=ws.11).aspx, they may have the ability to ignore that flag to instead directly create the file instead. Hence, extreme caution must be taken before interpreting the validity of these various checks on Windows.

Given the complications involved, I recommend we not support be_Xable.by_user on windows until we have the time/expertise to sort through the above issues.

@chris-rock chris-rock removed this from the 0.32.0 milestone Aug 25, 2016
@chris-rock
Copy link
Contributor

Thanks @ksubrama for this update. I agree, that we need to have a clear path and idea how we do that properly without reinventing the wheel.

@mhedgpeth How do you believe we should go forward?

@arlimus
Copy link
Contributor

arlimus commented Aug 31, 2016

Also raised by a Chef customer here: https://getchef.zendesk.com/agent/tickets/10932

@arlimus arlimus added this to the 0.34.0 milestone Aug 31, 2016
@chris-rock chris-rock modified the milestones: 0.34.0, 0.35.0 Sep 9, 2016
@chris-rock chris-rock modified the milestones: 0.35.0, 0.34.0 Sep 9, 2016
@arlimus arlimus added the ready label Sep 12, 2016
@chris-rock chris-rock modified the milestones: 0.35.0, 0.36.0, 1.1.0 Sep 19, 2016
@jeremymv2
Copy link
Contributor

jeremymv2 commented Sep 21, 2016

@chris-rock @arlimus

Can we take some inspiration from this perhaps? https://github.com/mizzy/specinfra/search?utf8=✓&q=CheckFileAccessRules

@jeremymv2
Copy link
Contributor

jeremymv2 commented Sep 22, 2016

This is good news, here is a demo of the above working for an equivalent of be_readable.by_user('vagrant') - the other be_X.by_user() should presumably work as well. User vagrant-2012-r2\vagrant is an Admin so has all permissions for the file while user vagrant-2012-r2\johndoe had all permissions removed.

screen shot 2016-09-21 at 10 58 19 pm

@chris-rock
Copy link
Contributor

@mhedgpeth @jeremymv2 added file permission support for Windows. Do we need anything else?

@chris-rock
Copy link
Contributor

@mhedgpeth I am closing this issue. Please reopen if it continues to be an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

No branches or pull requests

6 participants