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

Add recursemode parameter to chose to apply ACL's recursivel #12

Closed
wants to merge 1 commit into from

Conversation

roidelapluie
Copy link
Member

This commit adds a Recursemode parameter to generate additional
resources for children files when recurse is set to true.

Depends on #11.

@@ -0,0 +1,20 @@
---

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roidelapluie Shouldn't this file be in a separate PR? This is part of the Travis support for test running, not part of the recursive ACL changes, right?

@dobbymoodge
Copy link

@roidelapluie Sorry for being so slow to merge these. This looks good, except all the test support stuff - nodesets, .travis.yml, etc. - need to be put in a separate pull request unless they relate directly to the ACL recursion changes. That is, it makes sense for spec/unit/puppet/type/acl_spec.rb to be in this pull request, but a lot of the other stuff should be in a separate PR so the history is a bit clearer.

I should have reviewed this before I merged in #11, since that would have been the right place for most of these files. Oh well. :)

@dobbymoodge
Copy link

Also, maybe update README.org to document the new options (:lazy, :deep)

@dobbymoodge
Copy link

But what the heck, this project is so low-traffic, having the PR do double-duty isn't such a big deal. :)

EDIT: Actually, I found this problem:

recursemode => deep erroneously tries to apply default ACLs to files.

The deep recursion code should cache permissions lists with and
without the default: ACLs. For each filespec returned in generate,
it should test if the filespec is a directory or a file, and apply the
appropriate cached permission list accordingly.

Default ACLs are an important tool for policy enforcement in some
environments, this use case must be supported for the pull request to
be accepted.

A test should be written to validate this use case.

Here's how I was able to reproduce the error. I used a Vagrant
environment, thus the use of the vagrant user.

# cat <<EOF > ~/test.pp
file { "/tmp/testfile":
    ensure => directory,
    owner => vagrant,
    group => vagrant,
}

file { [ "/tmp/testfile/a", "/tmp/testfile/b" ]:
    ensure => file,
    owner => vagrant,
    group => vagrant,
}

acl { "/tmp/testfile/b":
    action => set,
    permission => [
                   "user::rw-",
                   "group::rw-",
                   "mask::r-x",
                   "other::---",
                   "group:vagrant:rw-",
                   ],
    provider   => posixacl,
}

acl { "/tmp/testfile":
    action     => set,
    permission => [
                   "user::rwx",
                   "group::---",
                   "mask::r-x",
                   "other::---",
                   "group:vagrant:r-x",
                   "default:user::rw-",
                   "default:group::---",
                   "default:mask::rwx",
                   "default:other::---",
                   "default:group:vagrant:rw-",
                   ],
    provider   => posixacl,
    recursive  => true,
    recursemode => deep,
}
EOF

# sed -i -e 's/vagrant/YOUR_USER/g' test.pp
# puppet apply --verbose --debug --modulepath=. ./test.pp

This is the error captured from the output:

...
Error: Execution of '/usr/bin/setfacl -n -m default:user::rw- /tmp/testfile/a' returned 1: setfacl: /tmp/testfile/a: Only directories can have default ACLs
Error: /Stage[main]/Main/Acl[/tmp/testfile/a]/permission: change from group::---,group:vagrant:r-x,mask::r-x,other::---,user::rwx to user::rwx,group::---,mask::r-x,other::---,group:vagrant:r-x,default:user::rw-,default:group::---,default:mask::rwx,default:other::---,default:group:vagrant:rw- failed: Execution of '/usr/bin/setfacl -n -m default:user::rw- /tmp/testfile/a' returned 1: setfacl: /tmp/testfile/a: Only directories can have default ACLs

@roidelapluie
Copy link
Member Author

rebased and noted your comment. Should we silently disable default ACL's for files?

@dobbymoodge
Copy link

That's what setfacl -R does, so I think we should do the same.

This commit adds a Recursemode parameter to generate additional
resources for children files when recurse is set to true.

Depends on voxpupuli#11.
@dobbymoodge
Copy link

Changes from this PR pulled in with additional fixes by #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants