From 2bb09bfd12e42e7be6f39ab0e45a992512c240f9 Mon Sep 17 00:00:00 2001 From: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Date: Mon, 18 Mar 2024 10:58:00 -0400 Subject: [PATCH] atomic_move - fix preserving extended acls (#82818) * use copystat to copy as many attributes as possible before os.rename update unit test mocks for updated method of attribute preservation add integration test for lineinfile case remove erroneous `- meta: end_play` from lineinfile test suite * add keep_dest_attrs parameter to control whether src attributes are copied initially, and for existing destinations, whether the src is updated using the dest before being renamed consolidate with copy unsetting extended attrs ci_complete --- .../atomic-move-fix-extended-attrs.yml | 2 + lib/ansible/module_utils/basic.py | 28 +++----- lib/ansible/modules/copy.py | 72 +------------------ test/integration/targets/lineinfile/aliases | 1 + .../targets/lineinfile/tasks/acls.yml | 54 ++++++++++++++ .../targets/lineinfile/tasks/main.yml | 7 +- .../module_utils/basic/test_atomic_move.py | 18 ++--- 7 files changed, 80 insertions(+), 102 deletions(-) create mode 100644 changelogs/fragments/atomic-move-fix-extended-attrs.yml create mode 100644 test/integration/targets/lineinfile/tasks/acls.yml diff --git a/changelogs/fragments/atomic-move-fix-extended-attrs.yml b/changelogs/fragments/atomic-move-fix-extended-attrs.yml new file mode 100644 index 00000000000..78041c01761 --- /dev/null +++ b/changelogs/fragments/atomic-move-fix-extended-attrs.yml @@ -0,0 +1,2 @@ +bugfixes: + - AnsibleModule.atomic_move - fix preserving extended ACLs of the destination when it exists (https://github.com/ansible/ansible/issues/72929). diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index dbef12b684d..7308bb7e8ba 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1585,7 +1585,7 @@ class AnsibleModule(object): current_attribs = current_attribs.get('attr_flags', '') self.set_attributes_if_different(dest, current_attribs, True) - def atomic_move(self, src, dest, unsafe_writes=False): + def atomic_move(self, src, dest, unsafe_writes=False, keep_dest_attrs=True): '''atomically move src to dest, copying attributes from dest, returns true on success it uses os.rename to ensure this as it is an atomic operation, rest of the function is to work around limitations, corner cases and ensure selinux context is saved if possible''' @@ -1593,24 +1593,11 @@ class AnsibleModule(object): dest_stat = None b_src = to_bytes(src, errors='surrogate_or_strict') b_dest = to_bytes(dest, errors='surrogate_or_strict') - if os.path.exists(b_dest): + if os.path.exists(b_dest) and keep_dest_attrs: try: dest_stat = os.stat(b_dest) - - # copy mode and ownership - os.chmod(b_src, dest_stat.st_mode & PERM_BITS) os.chown(b_src, dest_stat.st_uid, dest_stat.st_gid) - - # try to copy flags if possible - if hasattr(os, 'chflags') and hasattr(dest_stat, 'st_flags'): - try: - os.chflags(b_src, dest_stat.st_flags) - except OSError as e: - for err in 'EOPNOTSUPP', 'ENOTSUP': - if hasattr(errno, err) and e.errno == getattr(errno, err): - break - else: - raise + shutil.copystat(b_dest, b_src) except OSError as e: if e.errno != errno.EPERM: raise @@ -1658,18 +1645,21 @@ class AnsibleModule(object): os.close(tmp_dest_fd) # leaves tmp file behind when sudo and not root try: - shutil.move(b_src, b_tmp_dest_name) + shutil.move(b_src, b_tmp_dest_name, copy_function=shutil.copy if keep_dest_attrs else shutil.copy2) except OSError: # cleanup will happen by 'rm' of tmpdir # copy2 will preserve some metadata - shutil.copy2(b_src, b_tmp_dest_name) + if keep_dest_attrs: + shutil.copy(b_src, b_tmp_dest_name) + else: + shutil.copy2(b_src, b_tmp_dest_name) if self.selinux_enabled(): self.set_context_if_different( b_tmp_dest_name, context, False) try: tmp_stat = os.stat(b_tmp_dest_name) - if dest_stat and (tmp_stat.st_uid != dest_stat.st_uid or tmp_stat.st_gid != dest_stat.st_gid): + if keep_dest_attrs and dest_stat and (tmp_stat.st_uid != dest_stat.st_uid or tmp_stat.st_gid != dest_stat.st_gid): os.chown(b_tmp_dest_name, dest_stat.st_uid, dest_stat.st_gid) except OSError as e: if e.errno != errno.EPERM: diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index 67558f076b6..cb2ccf9c8f8 100644 --- a/lib/ansible/modules/copy.py +++ b/lib/ansible/modules/copy.py @@ -290,7 +290,6 @@ import filecmp import grp import os import os.path -import platform import pwd import shutil import stat @@ -299,12 +298,6 @@ import traceback from ansible.module_utils.common.text.converters import to_bytes, to_native from ansible.module_utils.basic import AnsibleModule -from ansible.module_utils.common.process import get_bin_path -from ansible.module_utils.common.locale import get_best_parsable_locale - - -# The AnsibleModule object -module = None class AnsibleModuleError(Exception): @@ -312,21 +305,6 @@ 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') - # 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] - locale = get_best_parsable_locale(module) - rc, out, err = module.run_command(b_acl_command, environ_update=dict(LANG=locale, LC_ALL=locale, LC_MESSAGES=locale)) - 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. @@ -527,8 +505,6 @@ 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( @@ -703,54 +679,8 @@ def main(): else: raise - # might be needed below - if 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 - # at this point we should always have tmp file - module.atomic_move(b_mysrc, dest, unsafe_writes=module.params['unsafe_writes']) - - if 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 + module.atomic_move(b_mysrc, dest, unsafe_writes=module.params['unsafe_writes'], keep_dest_attrs=not remote_src) except (IOError, OSError): module.fail_json(msg="failed to copy: %s to %s" % (src, dest), traceback=traceback.format_exc()) diff --git a/test/integration/targets/lineinfile/aliases b/test/integration/targets/lineinfile/aliases index 765b70da796..ca7c9128514 100644 --- a/test/integration/targets/lineinfile/aliases +++ b/test/integration/targets/lineinfile/aliases @@ -1 +1,2 @@ shippable/posix/group2 +destructive diff --git a/test/integration/targets/lineinfile/tasks/acls.yml b/test/integration/targets/lineinfile/tasks/acls.yml new file mode 100644 index 00000000000..1be6ecb59ec --- /dev/null +++ b/test/integration/targets/lineinfile/tasks/acls.yml @@ -0,0 +1,54 @@ +- block: + - name: Install the acl package on Ubuntu + apt: + name: acl + when: ansible_distribution in ('Ubuntu') + register: setup_acl + + - name: Create file + copy: + content: "TEST" + mode: 0644 + dest: "~/test.txt" + + - shell: setfacl -m nobody:rwx ~/test.txt + + - shell: getfacl ~/test.txt + register: acls + + - name: Check that permissions match with the copy mode and setfacl command + assert: + that: + - "'user::rw-' in acls.stdout_lines" + - "'user:nobody:rwx' in acls.stdout_lines" + - "'group::r--' in acls.stdout_lines" + - "'other::r--' in acls.stdout_lines" + + - name: test atomic_move via lineinfile doesn't delete extended acls + lineinfile: + path: "~/test.txt" + regexp: "TEST" + line: "UPDATE" + + - shell: getfacl ~/test.txt + register: acls + + - name: Validate the acls are unmodified + assert: + that: + - "'user::rw-' in acls.stdout_lines" + - "'user:nobody:rwx' in acls.stdout_lines" + - "'group::r--' in acls.stdout_lines" + - "'other::r--' in acls.stdout_lines" + + always: + - name: Remove the acl package on Ubuntu + apt: + name: acl + state: absent + when: setup_acl is changed + + - name: Clean up + file: + path: "~/test.txt" + state: absent diff --git a/test/integration/targets/lineinfile/tasks/main.yml b/test/integration/targets/lineinfile/tasks/main.yml index ae34c757593..1914920ab7a 100644 --- a/test/integration/targets/lineinfile/tasks/main.yml +++ b/test/integration/targets/lineinfile/tasks/main.yml @@ -965,9 +965,6 @@ The search string is an empty string, which will match every line in the file. This may have unintended consequences, such as replacing the last line in the file rather than appending. -- name: meta - meta: end_play - ################################################################### ## Issue #58923 ## Using firstmatch with insertafter and ensure multiple lines are not inserted @@ -1445,3 +1442,7 @@ that: - result1 is changed - result2.stat.exists + +- name: Integration test for issue 72929 + import_tasks: acls.yml + when: ansible_system == 'Linux' diff --git a/test/units/module_utils/basic/test_atomic_move.py b/test/units/module_utils/basic/test_atomic_move.py index 248115c29c9..927ed3ee032 100644 --- a/test/units/module_utils/basic/test_atomic_move.py +++ b/test/units/module_utils/basic/test_atomic_move.py @@ -32,6 +32,7 @@ def atomic_am(am, mocker): def atomic_mocks(mocker, monkeypatch): environ = dict() mocks = { + 'copystat': mocker.patch('shutil.copystat'), 'chmod': mocker.patch('os.chmod'), 'chown': mocker.patch('os.chown'), 'close': mocker.patch('os.close'), @@ -102,7 +103,7 @@ def test_existing_file(atomic_am, atomic_mocks, fake_stat, mocker, selinux): atomic_am.atomic_move('/path/to/src', '/path/to/dest') atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') - assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', basic.DEFAULT_PERM & ~18)] + assert atomic_mocks['copystat'].call_args_list == [mocker.call(b'/path/to/dest', b'/path/to/src')] if selinux: assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)] @@ -125,7 +126,8 @@ def test_no_tty_fallback(atomic_am, atomic_mocks, fake_stat, mocker): atomic_am.atomic_move('/path/to/src', '/path/to/dest') atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') - assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', basic.DEFAULT_PERM & ~18)] + assert atomic_mocks['copystat'].call_args_list == [mocker.call(b'/path/to/dest', b'/path/to/src')] + assert atomic_mocks['chmod'].call_args_list == [] assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)] assert atomic_am.selinux_context.call_args_list == [mocker.call('/path/to/dest')] @@ -134,19 +136,19 @@ def test_no_tty_fallback(atomic_am, atomic_mocks, fake_stat, mocker): @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) def test_existing_file_stat_failure(atomic_am, atomic_mocks, mocker): """Failure to stat an existing file in order to copy permissions propogates the error (unless EPERM)""" - atomic_mocks['stat'].side_effect = OSError() + atomic_mocks['copystat'].side_effect = FileNotFoundError('testing os copystat with non EPERM error') atomic_mocks['path_exists'].return_value = True - with pytest.raises(OSError): + with pytest.raises(FileNotFoundError, match='testing os copystat with non EPERM error'): atomic_am.atomic_move('/path/to/src', '/path/to/dest') @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) def test_existing_file_stat_perms_failure(atomic_am, atomic_mocks, mocker): """Failure to stat an existing file to copy the permissions due to permissions passes fine""" - # and now have os.stat return EPERM, which should not fail + # and now have os.copystat return EPERM, which should not fail mock_context = atomic_am.selinux_context.return_value - atomic_mocks['stat'].side_effect = OSError(errno.EPERM, 'testing os stat with EPERM') + atomic_mocks['copystat'].side_effect = OSError(errno.EPERM, 'testing os copystat with EPERM') atomic_mocks['path_exists'].return_value = True atomic_am.selinux_enabled.return_value = True @@ -155,7 +157,7 @@ def test_existing_file_stat_perms_failure(atomic_am, atomic_mocks, mocker): atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') # FIXME: Should atomic_move() set a default permission value when it cannot retrieve the # existing file's permissions? (Right now it's up to the calling code. - # assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', basic.DEFAULT_PERM & ~18)] + assert atomic_mocks['copystat'].call_args_list == [mocker.call(b'/path/to/dest', b'/path/to/src')] assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)] assert atomic_am.selinux_context.call_args_list == [mocker.call('/path/to/dest')] @@ -203,8 +205,6 @@ def test_rename_perms_fail_temp_succeeds(atomic_am, atomic_mocks, fake_stat, moc mock_context = atomic_am.selinux_default_context.return_value atomic_mocks['path_exists'].return_value = False atomic_mocks['rename'].side_effect = [OSError(errno.EPERM, 'failing with EPERM'), None] - atomic_mocks['stat'].return_value = fake_stat - atomic_mocks['stat'].side_effect = None atomic_mocks['mkstemp'].return_value = (None, '/path/to/tempfile') atomic_mocks['mkstemp'].side_effect = None atomic_am.selinux_enabled.return_value = selinux