[BUG] Setting file acl crashes on capital X perm, fails octal conversion

[jinnatar]

Description
Using acl.present to set an ACL of X crashes in attempting to interpret it as an octal value. A capital X is a valid value, quoting the man page (3rd line being relevant):

The letters rwxXst select file mode bits for the affected users:
read (r), write (w), execute (or search for directories) (x),
execute/search only if the file is a directory or already has execute permission for some user (X),
set user or group ID on execution (s), restricted deletion flag or sticky bit (t).

My intent in using it is to only grant execute recursively to directories, not files.

Setup

/var/spool/postfix:
  acl.present:
    - acl_type: user
    - acl_name: telegraf
    - perms: rX
    - recurse: True

Steps to Reproduce the behavior

1. Run state.apply to push ACL change out
2. Run state.apply again.

Expected behavior
Change applied on first run, no changes or errors on the second run.

On the first run the changes seems to get applied successfully but any subsequent runs produce an error:

----------
          ID: /var/spool/postfix
    Function: acl.present
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 2154, in call
                  *cdata["args"], **cdata["kwargs"]
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2106, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/linux_acl.py", line 153, in present
                  octal_sum = sum([_octal.get(i, i) for i in perms])
              TypeError: unsupported operand type(s) for +: 'int' and 'str'
     Started: 13:35:12.611978
    Duration: 40.803 ms
     Changes:   

Versions Report

salt --versions-report

Identical versions between minion & master, same base image.

Salt Version:
          Salt: 3002.2

Dependency Versions:
          cffi: 1.11.5
      cherrypy: Not Installed
      dateutil: 2.6.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.10
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 0.5.6
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.19
      pycrypto: 2.6.1
  pycryptodome: 3.4.7
        pygit2: Not Installed
        Python: 3.6.9 (default, Oct  8 2020, 12:12:24)
  python-gnupg: 0.4.1
        PyYAML: 3.12
         PyZMQ: 17.1.2
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.2.5

System Versions:
          dist: ubuntu 18.04 Bionic Beaver
        locale: utf-8
       machine: x86_64
       release: 5.4.0-52-generic
        system: Linux
       version: Ubuntu 18.04 Bionic Beaver

Additional context
Inspecting the folder manually shows the permissions seem to have been applied as expected, and it's only the subsequent runs that fail.

A recursed directory gets x set for user:telegraf:

%> getfacl /var/spool/postfix/lib
getfacl: Removing leading '/' from absolute path names
# file: var/spool/postfix/lib
# owner: root
# group: root
user::rwx          
user:telegraf:r-x
group::r-x
mask::r-x
other::r-x

But a file does not:

%> getfacl /var/spool/postfix/lib/x86_64-linux-gnu/libgcc_s.so.1
getfacl: Removing leading '/' from absolute path names
# file: var/spool/postfix/lib/x86_64-linux-gnu/libgcc_s.so.1
# owner: root
# group: root
user::rw-
user:telegraf:r--
group::r--
mask::r--
other::r--

[jinnatar]

Since the code is comparing against acl.getfacl here's the relevant output. Since getfacl won't know about the applied capital X mode the effects are not visible here.

%> sudo salt-call acl.getfacl /var/spool/postfix
local:
    ----------
    /var/spool/postfix:
        ----------
        comment:
            ----------
            file:
                /var/spool/postfix
            group:
                root
            owner:
                root
        group:
            |_
              ----------
              root:
                  ----------
                  octal:
                      5
                  permissions:
                      ----------
                      execute:
                          True
                      read:
                          True
                      write:
                          False
        mask:
            |_
              ----------
              :
                  ----------
                  octal:
                      5
                  permissions:
                      ----------
                     execute:                                                                                                                                                                                                  [7/136]
                          True
                      read:
                          True
                      write:
                          False
        other:
            |_
              ----------
              :
                  ----------
                  octal:
                      5
                  permissions:
                      ----------
                      execute:
                          True
                      read:
                          True
                      write:
                          False
        user:
            |_
              ----------
              root:
                  ----------
                  octal:
                      7
                  permissions:
                      ----------
                      execute:
                          True
                      read:
                          True
                      write:
                          True
            |_
              ----------
              telegraf:
                  ----------
                  octal:
                      5
                  permissions:
                      ----------
                      execute:
                          True
                      read:
                          True
                      write:
                          False

[jinnatar]

Did a stupid simple test of amending the octal map to also map X to 1:

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

This however is only a partial workaround since now the change is triggered every time:

----------
          ID: /var/spool/postfix
    Function: acl.present
      Result: True
     Comment: Updated permissions for telegraf
     Started: 18:23:59.714015
    Duration: 50.346 ms
     Changes:
              ----------
              new:
                  ----------
                  acl_name:
                      telegraf
                  acl_type:
                      user
                  perms:
                      rX
              old:
                  ----------
                  acl_name:
                      telegraf
                  acl_type:
                      user
                  perms:
                      r-x

[krionbsd]

Thanks for reporting it, please note X for perms is not implemented yet.

[shalkie]

Hi!

I have also encountered this same "feature" and wonder if it is possible to expand this patch. We use FACLs a fair amount and the use of X is rather necessary for us.

The classic example for us is when we want the user to be able to inherit read only access on files and read execute on subdirectories, allowing them to traverse the directory structure. The X in FACL world is a nice conditional that applies x only on directories and not files. Unfortunately there is no octal equivalent for X like there is for r,w,x,s,or t.

Changing the map to make X = 1 would fix the break resulting in a traceback, but it doesn't fix the use of X or other permissions (s or t). In fact from what I can tell it leaves it broken and even worse it throws out a really useful feature and actually results in the acl perms being set to r-x (giving execute on files and directories!!).

From what I see it looks like there is logic in both states/linux_acl.py and modules/linux_acl.py that converts from the symbolic to octal and uses the summed value. I will also note that this logic doesn't account for the use of s,t, or X, only r,w,and x.

Instead the logic should probably be a direct comparison of the FACL record entries in symbolic form. That would fix this issue and allow the use of X, s, and t.

[doubletwist13]

This is also discussed in #33921