From 932ba3616067007fd5e449611a34e7e3837fc8ae Mon Sep 17 00:00:00 2001
From: Brian Coca <bcoca@users.noreply.github.com>
Date: Mon, 21 Dec 2020 11:20:52 -0500
Subject: [PATCH] ensure unsafe writes fallback (#70722)
* Ensure we actually fallback to unsafe_writes when set to true
add integration test
add fix for get_url not passing the parameter from args
---
lib/ansible/module_utils/basic.py | 19 ++++---
lib/ansible/modules/get_url.py | 2 +-
.../integration/targets/unsafe_writes/aliases | 6 +++
.../targets/unsafe_writes/basic.yml | 53 +++++++++++++++++++
.../targets/unsafe_writes/runme.sh | 5 ++
.../module_utils/basic/test_atomic_move.py | 1 +
6 files changed, 75 insertions(+), 11 deletions(-)
create mode 100644 test/integration/targets/unsafe_writes/aliases
create mode 100644 test/integration/targets/unsafe_writes/basic.yml
create mode 100755 test/integration/targets/unsafe_writes/runme.sh
diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index 6081574853b..f1cef2ff1d5 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -2369,8 +2369,7 @@ class AnsibleModule(object):
if e.errno not in [errno.EPERM, errno.EXDEV, errno.EACCES, errno.ETXTBSY, errno.EBUSY]:
# only try workarounds for errno 18 (cross device), 1 (not permitted), 13 (permission denied)
# and 26 (text file busy) which happens on vagrant synced folders and other 'exotic' non posix file systems
- self.fail_json(msg='Could not replace file: %s to %s: %s' % (src, dest, to_native(e)),
- exception=traceback.format_exc())
+ self.fail_json(msg='Could not replace file: %s to %s: %s' % (src, dest, to_native(e)), exception=traceback.format_exc())
else:
# Use bytes here. In the shippable CI, this fails with
# a UnicodeError with surrogateescape'd strings for an unknown
@@ -2380,14 +2379,13 @@ class AnsibleModule(object):
error_msg = None
tmp_dest_name = None
try:
- tmp_dest_fd, tmp_dest_name = tempfile.mkstemp(prefix=b'.ansible_tmp',
- dir=b_dest_dir, suffix=b_suffix)
+ tmp_dest_fd, tmp_dest_name = tempfile.mkstemp(prefix=b'.ansible_tmp', dir=b_dest_dir, suffix=b_suffix)
except (OSError, IOError) as e:
error_msg = 'The destination directory (%s) is not writable by the current user. Error was: %s' % (os.path.dirname(dest), to_native(e))
except TypeError:
# We expect that this is happening because python3.4.x and
- # below can't handle byte strings in mkstemp(). Traceback
- # would end in something like:
+ # below can't handle byte strings in mkstemp().
+ # Traceback would end in something like:
# file = _os.path.join(dir, pre + name + suf)
# TypeError: can't concat bytes to str
error_msg = ('Failed creating tmp file for atomic move. This usually happens when using Python3 less than Python3.5. '
@@ -2431,11 +2429,12 @@ class AnsibleModule(object):
self._unsafe_writes(b_tmp_dest_name, b_dest)
else:
self.fail_json(msg='Unable to make %s into to %s, failed final rename from %s: %s' %
- (src, dest, b_tmp_dest_name, to_native(e)),
- exception=traceback.format_exc())
+ (src, dest, b_tmp_dest_name, to_native(e)), exception=traceback.format_exc())
except (shutil.Error, OSError, IOError) as e:
- self.fail_json(msg='Failed to replace file: %s to %s: %s' % (src, dest, to_native(e)),
- exception=traceback.format_exc())
+ if unsafe_writes:
+ self._unsafe_writes(b_src, b_dest)
+ else:
+ self.fail_json(msg='Failed to replace file: %s to %s: %s' % (src, dest, to_native(e)), exception=traceback.format_exc())
finally:
self.cleanup(b_tmp_dest_name)
diff --git a/lib/ansible/modules/get_url.py b/lib/ansible/modules/get_url.py
index 6ac370c2ef1..c29d69e5c94 100644
--- a/lib/ansible/modules/get_url.py
+++ b/lib/ansible/modules/get_url.py
@@ -630,7 +630,7 @@ def main():
if backup:
if os.path.exists(dest):
backup_file = module.backup_local(dest)
- module.atomic_move(tmpsrc, dest)
+ module.atomic_move(tmpsrc, dest, unsafe_writes=module.params['unsafe_writes'])
except Exception as e:
if os.path.exists(tmpsrc):
os.remove(tmpsrc)
diff --git a/test/integration/targets/unsafe_writes/aliases b/test/integration/targets/unsafe_writes/aliases
new file mode 100644
index 00000000000..4fb7a116401
--- /dev/null
+++ b/test/integration/targets/unsafe_writes/aliases
@@ -0,0 +1,6 @@
+needs/root
+skip/freebsd
+skip/osx
+skip/macos
+skip/aix
+shippable/posix/group3
diff --git a/test/integration/targets/unsafe_writes/basic.yml b/test/integration/targets/unsafe_writes/basic.yml
new file mode 100644
index 00000000000..b173c7f8727
--- /dev/null
+++ b/test/integration/targets/unsafe_writes/basic.yml
@@ -0,0 +1,53 @@
+- hosts: testhost
+ gather_facts: false
+ vars:
+ testudir: '{{output_dir}}/unsafe_writes_test'
+ testufile: '{{testudir}}/unreplacablefile.txt'
+ tasks:
+ - name: test unsafe_writes on immutable dir (file cannot be atomically replaced)
+ block:
+ - name: create target dir
+ file: path={{testudir}} state=directory
+ - name: setup test file
+ copy: content=ORIGINAL dest={{testufile}}
+ - name: make target dir immutable (cannot write to file w/o unsafe_writes)
+ file: path={{testudir}} state=directory attributes="+i"
+ become: yes
+ ignore_errors: true
+ register: madeimmutable
+
+ - name: only run if immutable dir command worked, some of our test systems don't allow for it
+ when: madeimmutable is success
+ block:
+ - name: test this is actually immmutable working as we expect
+ file: path={{testufile}} state=absent
+ register: breakimmutable
+ ignore_errors: True
+
+ - name: only run if reallyh immutable dir
+ when: breakimmutable is failed
+ block:
+ - name: test overwriting file w/o unsafe
+ copy: content=NEW dest={{testufile}} unsafe_writes=False
+ ignore_errors: true
+ register: copy_without
+
+ - name: ensure we properly failed
+ assert:
+ that:
+ - copy_without is failed
+
+ - name: test overwriting file with unsafe
+ copy: content=NEW dest={{testufile}} unsafe_writes=True
+ register: copy_with
+
+ - name: ensure we properly changed
+ assert:
+ that:
+ - copy_with is changed
+
+ always:
+ - name: remove immutable flag from dir to prevent issues with cleanup
+ file: path={{testudir}} state=directory attributes="-i"
+ ignore_errors: true
+ become: yes
diff --git a/test/integration/targets/unsafe_writes/runme.sh b/test/integration/targets/unsafe_writes/runme.sh
new file mode 100755
index 00000000000..5c37f727eea
--- /dev/null
+++ b/test/integration/targets/unsafe_writes/runme.sh
@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+set -eux
+
+ansible-playbook basic.yml -i ../../inventory -e "output_dir=${OUTPUT_DIR}" "$@"
diff --git a/test/units/module_utils/basic/test_atomic_move.py b/test/units/module_utils/basic/test_atomic_move.py
index 7bd9496eddf..bbdb0519664 100644
--- a/test/units/module_utils/basic/test_atomic_move.py
+++ b/test/units/module_utils/basic/test_atomic_move.py
@@ -23,6 +23,7 @@ def atomic_am(am, mocker):
am.selinux_context = mocker.MagicMock()
am.selinux_default_context = mocker.MagicMock()
am.set_context_if_different = mocker.MagicMock()
+ am._unsafe_writes = mocker.MagicMock()
yield am