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: