Allow +X in ACL's

[timwsuqld]

Related to #31270 Wanting to create an ACL that applies the execute permission to directories, but not files. Using chmod and setfacl you can use X instead of x, which means it'll apply the execute permission to directories, or files if they already have that permission. From the chmod man page, execute/search only if the file is a directory or already has execute permission for some user (X)

An example SLS file would look like. Note the Capital X, not lower case x

developers_acl:
  acl.present:
    - name: /srv/www
    - acl_type: default:group
    - acl_name: developers
    - perms: rwX
    - recurse: True

This unfortunately bombs with the following error (Same as #31270)

          ID: developers_acl
    Function: acl.present
        Name: /srv/www
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python2.7/site-packages/salt/state.py", line 1703, in call
                  **cdata['kwargs'])
                File "/usr/lib/python2.7/site-packages/salt/loader.py", line 1649, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/states/linux_acl.py", line 91, in present
                  if user[_search_name]['octal'] == sum([_octal.get(i, i) for i in perms]):
              TypeError: unsupported operand type(s) for +: 'int' and 'str'
     Started: 11:56:24.019937
    Duration: 8.121 ms
     Changes:   

Looking at the code, I'm not sure the easiest way to handle this. We take the easy way of comparing current permissions to intended permissions by getting the octal value of the current permissions, and calulating the new octal value. To support X we'd need to handle the execute bit on a case by case basis, as we shouldn't be removing the execute bit if it's present, but we should only be adding it to directories if it's absent.

Versions Report

$ salt --versions-report
Salt Version:
           Salt: 2015.8.10

Dependency Versions:
         Jinja2: 2.7.3
       M2Crypto: Not Installed
           Mako: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.7.0
         Python: 2.7.5 (default, Nov 20 2015, 02:00:19)
           RAET: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5
           cffi: 0.8.6
       cherrypy: 3.2.2
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
        libgit2: 0.21.0
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.7
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
         pygit2: 0.21.4
   python-gnupg: Not Installed
          smmap: Not Installed
        timelib: Not Installed

System Versions:
           dist: centos 7.2.1511 Core
        machine: x86_64
        release: 3.10.0-327.18.2.el7.x86_64
         system: CentOS Linux 7.2.1511 Core

[Ch3LL]

@timwsuqld I am able to replicate this error with a smaller test case as follows:

/tmp/acl1/:
  acl.present:
    - acl_type: user
    - acl_name: root
    - perms: rwX

Looks like we need to add the ability to use this X argument. Does X have a value attributed to it that would possible be an approach if it does. Then it could possibly be added to _octal = {'r': 4, 'w': 2, 'x': 1, '-': 0}

[timwsuqld]

@Ch3LL Unfortunately X doesn't have a numeric value assigned to it. It's special because it's a 1, but only if it's directory or file that already has an execute bit. If it did have an octal value, I would have easily added it to the dict and submitted a patch.

Unfortunately I think it'll need some more logic added to the code to handle it

[rsuarezsoto]

Hello,

has there been any advance on this?

Thanks!

[Ch3LL]

No one is currently working on this due to other higher priority issues. Please feel free to take a stab at a PR if you would like.

[doubletwist13]

I would very much like a fix for this as well, but I'm far from knowledgeable enough to do it myself.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[rhoths]

Still an issue.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[knine]

I ran into this one again today. Just keeping it on the radar.

[sagetherage]

The Core team won't be able to get to this in Aluminium, moving it back into planning for another release.

[doubletwist13]

Just a reminder that this is still a massive shortcoming in the linux_acl state.

[leifliddy]

I ran into this problem today and was in the midst of creating an issue when I saw that this was already an outstanding issue for this. So this logic works....I literally just wrote this....could probably be tidied up a bit

--- /usr/lib/python3.11/site-packages/salt/states/linux_acl.py.orig 2022-10-09 22:45:53.798161441 +0200
+++ /usr/lib/python3.11/site-packages/salt/states/linux_acl.py  2022-10-10 20:15:16.757902616 +0200
@@ -102,8 +102,8 @@ def present(name, acl_type, acl_name="",
     """
     ret = {"name": name, "result": True, "changes": {}, "comment": ""}

-    _octal = {"r": 4, "w": 2, "x": 1, "-": 0}
-    _octal_lookup = {0: "-", 1: "r", 2: "w", 4: "x"}
+    _octal = {"r": 4, "w": 2, "x": 1, "X": 1, "-": 0}
+    _octal_lookup = {4: "r", 2: "w", 1: "x", 0: "-"}    

     if not os.path.exists(name):
         ret["comment"] = "{} does not exist".format(name)
@@ -145,7 +145,12 @@ def present(name, acl_type, acl_name="",
             user = None

         if user:
-            octal_sum = sum(_octal.get(i, i) for i in perms)
+            if perms.endswith('X'):
+                conditional_x = True
+            else:
+                conditional_x = False              
+
+            octal_new = sum(_octal.get(i, i) for i in perms)
             need_refresh = False
             # If recursive check all paths retrieved via acl.getfacl
             if recurse:
@@ -159,11 +164,18 @@ def present(name, acl_type, acl_name="",
                     else:
                         _current_perms_path = __current_perms[path]
                     for user_acl in _current_perms_path.get(_acl_type, []):
-                        if (
-                            _search_name in user_acl
-                            and user_acl[_search_name]["octal"] == octal_sum
-                        ):
-                            acl_found = True
+                        if _search_name in user_acl:
+                            octal_current = user_acl[_search_name]["octal"]
+                            executable = bool(octal_current % 2 == 1)       
+                            if (
+                                octal_current == octal_new
+                                or 
+                                (conditional_x and not executable and octal_current == (octal_new - 1))
+                            ):
+                                acl_found = True
                     if not acl_found:
                         need_refresh = True
                         break
@@ -179,26 +191,27 @@ def present(name, acl_type, acl_name="",
                 ret["comment"] = "Permissions are in the desired state"
             else:
                 _num = user[_search_name]["octal"]
-                new_perms = "{}{}{}".format(
-                    _octal_lookup[_num & 1],
-                    _octal_lookup[_num & 2],
+                old_perms = "{}{}{}".format(
                     _octal_lookup[_num & 4],
+                    _octal_lookup[_num & 2],
+                    _octal_lookup[_num & 1],
                 )
                 changes = {
                     "new": {"acl_name": acl_name, "acl_type": acl_type, "perms": perms},
-                    "old": {
-                        "acl_name": acl_name,
-                        "acl_type": acl_type,
-                        "perms": new_perms,
+                    "old": {"acl_name": acl_name, "acl_type": acl_type, "perms": old_perms,
                     },
                 }
                 if __opts__["test"]:
                     ret.update(
                         {
                             "comment": (
                                 "Updated permissions will be applied for "
-                                "{}: {} -> {}".format(acl_name, new_perms, perms)
+                                "{}: {} -> {}".format(acl_name, old_perms, perms)

I run it through various tests (targeted files/directories, recurse=True/False....etc) and it seems to work fine. @OrangeDog If you have time, could you please review this? I'll submit a PR if you think it looks good...

[OrangeDog]

I'm not familiar with linux acls. Make the PR and anyone can review it.

[leifliddy]

Whaaat!? Ok, so if you just perform this one line change in the list_present function

diff -up linux_acl.py.orig  linux_acl.py
--- linux_acl.py.orig   2022-10-10 18:14:13.309437026 +0200
+++ linux_acl.py    2022-10-10 19:48:53.704767782 +0200
@@ -401,7 +401,7 @@ def list_present(name, acl_type, acl_nam
         acl_names = []
     ret = {"name": name, "result": True, "changes": {}, "comment": ""}

-    _octal = {"r": 4, "w": 2, "x": 1, "-": 0}
+    _octal = {"r": 4, "w": 2, "x": 1, "X": 1, "-": 0}
     _octal_perms = sum(_octal.get(i, i) for i in perms)
     if not os.path.exists(name):
         ret["comment"] = "{} does not exist".format(name)

You can just run this

acl_test:
  acl.list_present:
    - name: /tmp/testdir
    - acl_type: group
    - acl_names:
      - wheel
    - perms: rwX
    - recurse: True

And it'll work. Which begs the questions why are present and list_present two distinct functions?
It seems like present should just call list_present

The only thing that doesn't seem to work with list_present is if you run the above state and then run the following state

acl_test:
  acl.list_present:
    - name: /tmp/testdir
    - acl_type: group
    - acl_names:
      - wheel
    - perms: rwx
    - recurse: True

**trying the change the permissions from rwX to rwx recursively doesn't work as it's only checking the directory permissions.
I'll see if I can sort that out. Would be much easier to just do a PR for this function....

[jlrcontegix]

Any forward movement on this? Looks like it should have been merged by now, but we're still seeing the original behavior described here.