From a8a318e588c8beb8f3dfd3c4817f5201ccd98010 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 3 Apr 2019 18:37:59 +0200 Subject: [PATCH] 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 (cherry picked from commit d15812fabf7113c89633ffe530140a3e52f203d3) --- .../44412-copy-fix-unwanted-acls.yaml | 2 + lib/ansible/modules/files/copy.py | 73 ++++++++++++++++++- lib/ansible/plugins/action/__init__.py | 14 ++++ lib/ansible/plugins/action/copy.py | 4 + test/integration/targets/copy/tasks/acls.yml | 33 +++++++++ test/integration/targets/copy/tasks/main.yml | 3 + 6 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 changelogs/fragments/44412-copy-fix-unwanted-acls.yaml create mode 100644 test/integration/targets/copy/tasks/acls.yml diff --git a/changelogs/fragments/44412-copy-fix-unwanted-acls.yaml b/changelogs/fragments/44412-copy-fix-unwanted-acls.yaml new file mode 100644 index 00000000000..1f6a88a0246 --- /dev/null +++ b/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) diff --git a/lib/ansible/modules/files/copy.py b/lib/ansible/modules/files/copy.py index 1cc7b139d8d..1cf2f04ed45 100644 --- a/lib/ansible/modules/files/copy.py +++ b/lib/ansible/modules/files/copy.py @@ -229,16 +229,23 @@ state: sample: file ''' +import errno import os import os.path +import platform 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): @@ -246,6 +253,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. @@ -279,6 +300,8 @@ def adjust_recursive_directory_permissions(pre_existing_dir, new_directory_list, def main(): + global module + module = AnsibleModule( # not checking because of daisy chain to file module argument_spec=dict( @@ -436,7 +459,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 diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index e1d1ea79194..66845424689 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -359,6 +359,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 diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index 198c55c2a5a..ef14dd37981 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/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)) diff --git a/test/integration/targets/copy/tasks/acls.yml b/test/integration/targets/copy/tasks/acls.yml new file mode 100644 index 00000000000..9a3be9b0640 --- /dev/null +++ b/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 }}" diff --git a/test/integration/targets/copy/tasks/main.yml b/test/integration/targets/copy/tasks/main.yml index e1ae5a46b6d..f1861ea7f8b 100644 --- a/test/integration/targets/copy/tasks/main.yml +++ b/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: