Fix copy module to reset filesystem acls (#51868)

The controller's fixup_perms2 uses filesystem acls to make the temporary
file for copy readable by an unprivileged become user. On Python3, the
acls are then copied to the destination filename so we have to remove
them from there.

We can't remove them prior to the copy because we may not have
permission to read the file if the acls are not present. We can't
remove them in atomic_move() because the move function shouldn't know
anything about controller features. We may want to generalize this into
a helper function, though.

Fixes #44412

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>

changelogs/fragments/44412-copy-fix-unwanted-acls.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Fix unwanted ACLs when using copy module (https://github.com/ansible/ansible/issues/44412)

lib/ansible/modules/files/copy.py

@@ -257,19 +257,26 @@ state:
     sample: file
 '''
 
+import errno
+import filecmp
+import grp
 import os
 import os.path
-import shutil
-import filecmp
+import platform
 import pwd
-import grp
+import shutil
 import stat
-import errno
 import tempfile
 import traceback
 
 from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.common.process import get_bin_path
 from ansible.module_utils._text import to_bytes, to_native
+from ansible.module_utils.six import PY3
+
+
+# The AnsibleModule object
+module = None
 
 
 class AnsibleModuleError(Exception):
@@ -277,6 +284,20 @@ class AnsibleModuleError(Exception):
         self.results = results
 
 
+# Once we get run_command moved into common, we can move this into a common/files module.  We can't
+# until then because of the module.run_command() method.  We may need to move it into
+# basic::AnsibleModule() until then but if so, make it a private function so that we don't have to
+# keep it for backwards compatibility later.
+def clear_facls(path):
+    setfacl = get_bin_path('setfacl', True)
+    # FIXME "setfacl -b" is available on Linux and FreeBSD. There is "setfacl -D e" on z/OS. Others?
+    acl_command = [setfacl, '-b', path]
+    b_acl_command = [to_bytes(x) for x in acl_command]
+    rc, out, err = module.run_command(b_acl_command, environ_update=dict(LANG='C', LC_ALL='C', LC_MESSAGES='C'))
+    if rc != 0:
+        raise RuntimeError('Error running "{0}": stdout: "{1}"; stderr: "{2}"'.format(' '.join(b_acl_command), out, err))
+
+
 def split_pre_existing_dir(dirname):
     '''
     Return the first pre-existing directory and a list of the new directories that will be created.
@@ -467,6 +488,8 @@ def copy_common_dirs(src, dest, module):
 
 def main():
 
+    global module
+
     module = AnsibleModule(
         # not checking because of daisy chain to file module
         argument_spec=dict(
@@ -631,7 +654,55 @@ def main():
                             module.warn("Unable to copy stats {0}".format(to_native(b_src)))
                         else:
                             raise
+
+                # might be needed below
+                if PY3 and hasattr(os, 'listxattr'):
+                    try:
+                        src_has_acls = 'system.posix_acl_access' in os.listxattr(src)
+                    except Exception as e:
+                        # assume unwanted ACLs by default
+                        src_has_acls = True
+
                 module.atomic_move(b_mysrc, dest, unsafe_writes=module.params['unsafe_writes'])
+
+                if PY3 and hasattr(os, 'listxattr') and platform.system() == 'Linux' and not remote_src:
+                    # atomic_move used above to copy src into dest might, in some cases,
+                    # use shutil.copy2 which in turn uses shutil.copystat.
+                    # Since Python 3.3, shutil.copystat copies file extended attributes:
+                    # https://docs.python.org/3/library/shutil.html#shutil.copystat
+                    # os.listxattr (along with others) was added to handle the operation.
+
+                    # This means that on Python 3 we are copying the extended attributes which includes
+                    # the ACLs on some systems - further limited to Linux as the documentation above claims
+                    # that the extended attributes are copied only on Linux. Also, os.listxattr is only
+                    # available on Linux.
+
+                    # If not remote_src, then the file was copied from the controller. In that
+                    # case, any filesystem ACLs are artifacts of the copy rather than preservation
+                    # of existing attributes. Get rid of them:
+
+                    if src_has_acls:
+                        # FIXME If dest has any default ACLs, there are not applied to src now because
+                        # they were overridden by copystat. Should/can we do anything about this?
+                        # 'system.posix_acl_default' in os.listxattr(os.path.dirname(b_dest))
+
+                        try:
+                            clear_facls(dest)
+                        except ValueError as e:
+                            if 'setfacl' in to_native(e):
+                                # No setfacl so we're okay.  The controller couldn't have set a facl
+                                # without the setfacl command
+                                pass
+                            else:
+                                raise
+                        except RuntimeError as e:
+                            # setfacl failed.
+                            if 'Operation not supported' in to_native(e):
+                                # The file system does not support ACLs.
+                                pass
+                            else:
+                                raise
+
             except (IOError, OSError):
                 module.fail_json(msg="failed to copy: %s to %s" % (src, dest), traceback=traceback.format_exc())
         changed = True

lib/ansible/plugins/action/__init__.py

@@ -407,6 +407,20 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                 self._connection._shell.tmpdir = None
 
     def _transfer_file(self, local_path, remote_path):
+        """
+        Copy a file from the controller to a remote path
+
+        :arg local_path: Path on controller to transfer
+        :arg remote_path: Path on the remote system to transfer into
+
+        .. warning::
+            * When you use this function you likely want to use use fixup_perms2() on the
+              remote_path to make sure that the remote file is readable when the user becomes
+              a non-privileged user.
+            * If you use fixup_perms2() on the file and copy or move the file into place, you will
+              need to then remove filesystem acls on the file once it has been copied into place by
+              the module.  See how the copy module implements this for help.
+        """
         self._connection.put_file(local_path, remote_path)
         return remote_path
 

lib/ansible/plugins/action/copy.py

@@ -305,6 +305,10 @@ class ActionModule(ActionBase):
             self._remove_tempfile_if_content_defined(content, content_tempfile)
             self._loader.cleanup_tmp_file(source_full)
 
+            # FIXME: I don't think this is needed when PIPELINING=0 because the source is created
+            # world readable.  Access to the directory itself is controlled via fixup_perms2() as
+            # part of executing the module. Check that umask with scp/sftp/piped doesn't cause
+            # a problem before acting on this idea. (This idea would save a round-trip)
             # fix file permissions when the copy is done as a different user
             if remote_path:
                 self._fixup_perms2((self._connection._shell.tmpdir, remote_path))

test/integration/targets/copy/tasks/acls.yml

@@ -0,0 +1,33 @@
+- block:
+  - block:
+    - name: Testing ACLs
+      copy:
+        content: "TEST"
+        mode: 0644
+        dest: "~/test.txt"
+
+    - shell: getfacl ~/test.txt
+      register: acls
+
+    become: yes
+    become_user: "{{ remote_unprivileged_user }}"
+
+  - name: Check that there are no ACLs leftovers
+    assert:
+      that:
+        - "'user:{{ remote_unprivileged_user }}:r-x\t#effective:r--' not in acls.stdout_lines"
+
+  - name: Check that permissions match with what was set in the mode param
+    assert:
+      that:
+        - "'user::rw-' in acls.stdout_lines"
+        - "'group::r--' in acls.stdout_lines"
+        - "'other::r--' in acls.stdout_lines"
+
+  always:
+    - name: Clean up
+      file:
+        path: "~/test.txt"
+        state: absent
+      become: yes
+      become_user: "{{ remote_unprivileged_user }}"

test/integration/targets/copy/tasks/main.yml

@@ -58,6 +58,9 @@
     - import_tasks: tests.yml
       remote_user: '{{ remote_unprivileged_user }}'
 
+    - import_tasks: acls.yml
+      when: ansible_system == 'Linux'
+
   always:
     - name: Cleaning
       file: