Append-only mode?

[Bouke]

Affected Puppet, Ruby, OS and module versions/distributions

• Puppet: 4.10
• Ruby: ?
• Distribution: CentOS 7.4
• Module version: 0.1.1

How to reproduce (e.g Puppet code you use)

    posix_acl { '/var/opt/rh/rh-haproxy18/lib/haproxy/stats':
        action     => set,
        permission => [
            "user:zabbix:rw-",
        ],
        require    => [Package['rh-haproxy18']],
    }

What are you seeing

Notice: /Stage[main]/Profile::Monitoring::Load_balancer/Posix_acl[/var/opt/rh/rh-haproxy18/lib/haproxy/stats]/permission: permission changed '["group::---", "other::---", "user::rw-"]' to '["user:zabbix:rw-"]'

[root@lb03 ~]# getfacl /var/opt/rh/rh-haproxy18/lib/haproxy/stats
getfacl: Removing leading '/' from absolute path names
# file: var/opt/rh/rh-haproxy18/lib/haproxy/stats
# owner: haproxy
# group: haproxy
user::rw-
user:zabbix:rw-			#effective:---
group::---
mask::---
other::---

What behaviour did you expect instead

[root@lb03 ~]# setfacl -m u:zabbix:rw /var/opt/rh/rh-haproxy18/lib/haproxy/stats
[root@lb03 ~]# getfacl /var/opt/rh/rh-haproxy18/lib/haproxy/stats
getfacl: Removing leading '/' from absolute path names
# file: var/opt/rh/rh-haproxy18/lib/haproxy/stats
# owner: haproxy
# group: haproxy
user::rw-
user:zabbix:rw-
group::---
mask::rw-
other::---

Any additional information you'd like to impart

The documentation says that the "set" operation should leave existing permissions alone. However it still replaces the user/group/other permissions.

[cdchase]

@Bouke As you are only setting user permission, the effective rights are dependent on the existing mask. Based on your output above, the mask is:

mask::---

If you want the user to get rw- then the mask needs to be set too.

permission => [
    "mask::rw-",
    "user:zabbix:rw-",
 ],

See: README

Note also mask must match base file modes. this was getting me... and how I happened to run across your issue.

[Bouke]

So I guess the point I'm trying to make is that I expected with action => 'set' I only had to specify what I wanted on top of already existing permissions, including the mask. However it doesn't work that way and I need to specify the mask property as well, making the behaviour more like action => 'exact'.

Similarly; I believe that when using the command line to append a user to the list, I don't have to specify the mask as the default is just fine.

[ghoneycutt]

Added needs-docs label. Not sure if this is an actual bug or just a miscommunication on how it works.

[matt-matt2]

So, I apply a sample documented "set" command and it wants to re-apply it every time I run puppet, for several possible reasons..

As per the original author's comment, "set" mode as documented suggests that it can behave much like "setfacl" on the command line, but it doesn't. It needs to pull the existing ACL and compare only the components specified in the "set" permissions block. Right now it compares the whole set of ACLs, which breaks idempotency as it's attempting to check base permission elements it does not explicitly manage (and this overlaps with core puppet file permissions).

A separate issue (perhaps this should be a separate bug actually?)
I found that I can specify ACLs using numerical UIDs, and it will then reapply them at every puppet run. (this applies to "set" or "exact" mode) it should be able to compare entries by numerical ID to remove any dependency on this aspect, or on capitalisation, or, I suppose, the documentation needs to explicitly state that it can't do this! But it could, just like the command line tools.

To avoid these issues I was forced to use "exact" mode and ensure the settings matched puppet's defined file permissions. It would be more useful if "set" behaved differently.

I think that the author's intention is that this should be treated as an RFE but I would say it's not working as expected in many documented use cases and is a bug.

[matt-matt2]

My example:

  posix_acl { "filename":
    action => set,
    permission => [
      'user:2001:r--',
      'user:2002:r--',
    ],
  }

Notice: /Stage[main]/Stunnel/Posix_acl/permission: current_value ["group::r--", "other::---", "user::rw-","user:phil:r--", "user:bob:r--"], should be ["user:2001:r--", "user:2002:r--"]

phil is user 2001, bob is user 2002. This re-applies every time the code is run. (I know, silly example, why would you do that? but it works if the check is cleverer and I have a use case for which this would be useful.)

The main point is that the permssion check will ALWAYS FAIL, even if the code becomes:

  posix_acl { "filename":
    action => set,
    permission => [
      'user:phil:r--',
      'user:bob:r--',
    ],
  }

because the check is comparing the whole value (including the base posix perms) against the "set" block, instead of only comparing the managed elements. Make this a new mode "add" if necessary? That's the concept I was looking for, tbh, as that's how I understood the documented behaviour of "set".

I suspect, however, that "set" won't work as advertised no matter what you do, unless it behaves the same as "exact" and you specify the full set of perms for the file/dir.

[matt-matt2]

And I should add we're using puppet 6.14 on RHEL 7.9 and version 2.0.0 of the module from puppetforge.
Cheers!

[matt-matt2]

I'm not sure I agree that this is a documentation issue .. the point being raised is that "append mode" can't just append, so it actually can't be used to "append", it has to be used like "exact" mode making it redundant.
My second issue is really biting me now (numerical UIDs) and I need to change tack. Perhaps I'll open a new separate issue for that.

[hoffie]

I was also bitten by this. I think it would be helpful if the posix_acl module supported automatic mask calculation as it is done by setfacl by default. I assume the existing code uses -n by default (and has the accompying README note), because it makes little sense to calculate the mask if you are managing everything explicitly (exact). However, it makes sense to support this with the set mode, as pointed out by other commentors.

I've been successfully using the following patch in my environment and I'm happy to submit a PR if it is deemed helpful.

diff --git a/posix_acl/lib/puppet/provider/posix_acl/posixacl.rb b/posix_acl/lib/puppet/provider/posix_acl/posixacl.rb
index 2dae274bb..221eb292a 100644
--- a/posix_acl/lib/puppet/provider/posix_acl/posixacl.rb
+++ b/posix_acl/lib/puppet/provider/posix_acl/posixacl.rb
@@ -18,20 +18,28 @@ def exists?
     permission
   end

+  def setfacl_mask
+    if @resource.value(:recalculatemask) == :true
+      return '--mask'
+    else
+      return '--no-mask'
+    end
+  end
+
   def unset_perm(perm, path)
     # Don't try to unset mode bits, it doesn't make sense!
     return if perm =~ %r{^(((u(ser)?)|(g(roup)?)|(m(ask)?)|(o(ther)?)):):}

     perm = perm.split(':')[0..-2].join(':')
     if check_recursive
-      setfacl('-R', '-n', '-x', perm, path)
+      setfacl('-R', setfacl_mask(), '-x', perm, path)
     else
-      setfacl('-n', '-x', perm, path)
+      setfacl(setfacl_mask(), '-x', perm, path)
     end
   end

   def set_perm(perm_set, path)
-    args_list = ['-n']
+    args_list = [setfacl_mask()]
     args_list << '-R' if check_recursive
     perm_set.each do |perm|
       args_list << '-m'
diff --git a/posix_acl/lib/puppet/type/posix_acl.rb b/posix_acl/lib/puppet/type/posix_acl.rb
index 2b63564fb..f60bbf098 100644
--- a/posix_acl/lib/puppet/type/posix_acl.rb
+++ b/posix_acl/lib/puppet/type/posix_acl.rb
@@ -256,6 +256,12 @@ def insync?(is)
     defaultto :false
   end

+  newparam(:recalculatemask) do
+    desc 'Recalculate the ACL mask field'
+    newvalues(:true, :false)
+    defaultto :false
+  end
+
   def self.pick_default_perms(acl)
     acl.reject { |a| a.split(':', -1).length == 4 }
   end