From 254ce15cd3eef19aa3bca4df24eb6e2c0cf62d64 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Tue, 17 Mar 2020 14:24:00 -0400 Subject: [PATCH 1/2] fix vault tmpe file handling * use local temp dir instead of system temp * ensure each worker clears dataloader temp files * added test for dangling temp files * added notes to data loader --- changelogs/fragments/vault_tmp_file.yml | 2 ++ lib/ansible/executor/process/worker.py | 13 +++++++ lib/ansible/parsing/dataloader.py | 14 +++++++- lib/ansible/parsing/vault/__init__.py | 2 +- .../vault/files/test_assemble/nonsecret.txt | 1 + .../vault/files/test_assemble/secret.vault | 7 ++++ test/integration/targets/vault/runme.sh | 4 +++ .../targets/vault/test_dangling_temp.yml | 34 +++++++++++++++++++ 8 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 changelogs/fragments/vault_tmp_file.yml create mode 100644 test/integration/targets/vault/files/test_assemble/nonsecret.txt create mode 100644 test/integration/targets/vault/files/test_assemble/secret.vault create mode 100644 test/integration/targets/vault/test_dangling_temp.yml diff --git a/changelogs/fragments/vault_tmp_file.yml b/changelogs/fragments/vault_tmp_file.yml new file mode 100644 index 00000000000000..1eaea6f3e7ff76 --- /dev/null +++ b/changelogs/fragments/vault_tmp_file.yml @@ -0,0 +1,2 @@ +bugfixes: + - Ensure DataLoader temp files are removed at appropriate times and that we observe the LOCAL_TMP setting. diff --git a/lib/ansible/executor/process/worker.py b/lib/ansible/executor/process/worker.py index d01ff7f5cc5f2b..d02a86589955c3 100644 --- a/lib/ansible/executor/process/worker.py +++ b/lib/ansible/executor/process/worker.py @@ -67,6 +67,10 @@ def __init__(self, final_q, task_vars, host, task, play_context, loader, variabl self._variable_manager = variable_manager self._shared_loader_obj = shared_loader_obj + # NOTE: this works due to fork, if switching to threads this should change to per thread storage of temp files + # clear var to ensure we only delete files for this child + self._loader._tempfiles = set() + def _save_stdin(self): self._new_stdin = os.devnull try: @@ -200,6 +204,10 @@ def _run(self): except Exception: display.debug(u"WORKER EXCEPTION: %s" % to_text(e)) display.debug(u"WORKER TRACEBACK: %s" % to_text(traceback.format_exc())) + finally: + self._clean_up() + finally: + self._clean_up() display.debug("WORKER PROCESS EXITING") @@ -210,3 +218,8 @@ def _run(self): # ps.print_stats() # with open('worker_%06d.stats' % os.getpid(), 'w') as f: # f.write(s.getvalue()) + + def _clean_up(self): + # NOTE: see note in init about forks + # ensure we cleanup all temp files for this worker + self._loader.cleanup_all_tmp_files() diff --git a/lib/ansible/parsing/dataloader.py b/lib/ansible/parsing/dataloader.py index 3e8bce709680ad..4109514734629e 100644 --- a/lib/ansible/parsing/dataloader.py +++ b/lib/ansible/parsing/dataloader.py @@ -51,8 +51,16 @@ class DataLoader: ''' def __init__(self): + self._basedir = '.' + + # NOTE: not effective with forks as the main copy does not get updated. + # avoids rereading files self._FILE_CACHE = dict() + + # NOTE: not thread safe, also issues with forks not returning data to main proc + # so they need to be cleaned independantly. See WorkerProcess for example. + # used to keep track of temp files for cleaning self._tempfiles = set() # initialize the vault stuff with an empty password @@ -322,7 +330,7 @@ def path_dwim_relative_stack(self, paths, dirname, source, is_role=False): def _create_content_tempfile(self, content): ''' Create a tempfile containing defined content ''' - fd, content_tempfile = tempfile.mkstemp() + fd, content_tempfile = tempfile.mkstemp(dir=C.DEFAULT_LOCAL_TMP) f = os.fdopen(fd, 'wb') content = to_bytes(content) try: @@ -385,6 +393,10 @@ def cleanup_tmp_file(self, file_path): self._tempfiles.remove(file_path) def cleanup_all_tmp_files(self): + """ + Removes all temporary files that DataLoader has created + NOTE: not thread safe, forks also need special handling see __init__ for details. + """ for f in self._tempfiles: try: self.cleanup_tmp_file(f) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index 566d6999758293..dde1fecb2365e0 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -850,7 +850,7 @@ def _edit_file_helper(self, filename, secret, # Create a tempfile root, ext = os.path.splitext(os.path.realpath(filename)) - fd, tmp_path = tempfile.mkstemp(suffix=ext) + fd, tmp_path = tempfile.mkstemp(suffix=ext, dir=C.DEFAULT_LOCAL_TMP) os.close(fd) cmd = self._editor_shell_command(tmp_path) diff --git a/test/integration/targets/vault/files/test_assemble/nonsecret.txt b/test/integration/targets/vault/files/test_assemble/nonsecret.txt new file mode 100644 index 00000000000000..320b6b4ca0e4aa --- /dev/null +++ b/test/integration/targets/vault/files/test_assemble/nonsecret.txt @@ -0,0 +1 @@ +THIS IS OK diff --git a/test/integration/targets/vault/files/test_assemble/secret.vault b/test/integration/targets/vault/files/test_assemble/secret.vault new file mode 100644 index 00000000000000..fd278564916130 --- /dev/null +++ b/test/integration/targets/vault/files/test_assemble/secret.vault @@ -0,0 +1,7 @@ +$ANSIBLE_VAULT;1.1;AES256 +37626439373465656332623633333336353334326531333666363766303339336134313136616165 +6561333963343739386334653636393363396366396338660a663537666561643862343233393265 +33336436633864323935356337623861663631316530336532633932623635346364363338363437 +3365313831366365350a613934313862313538626130653539303834656634353132343065633162 +34316135313837623735653932663139353164643834303534346238386435373832366564646236 +3461333465343434666639373432366139363566303564643066 diff --git a/test/integration/targets/vault/runme.sh b/test/integration/targets/vault/runme.sh index c4d17dbd26e20c..c8ecde9ba03ea2 100755 --- a/test/integration/targets/vault/runme.sh +++ b/test/integration/targets/vault/runme.sh @@ -517,3 +517,7 @@ ansible-playbook "$@" -i invalid_format/inventory --vault-id invalid_format/vaul # Run playbook with vault file with unicode in filename (https://github.com/ansible/ansible/issues/50316) ansible-playbook -i ../../inventory -v "$@" --vault-password-file vault-password test_utf8_value_in_filename.yml + +# Ensure we don't leave unencrypted temp files dangling +ansible-playbook -v "$@" --vault-password-file vault-password test_dangling_temp.yml + diff --git a/test/integration/targets/vault/test_dangling_temp.yml b/test/integration/targets/vault/test_dangling_temp.yml new file mode 100644 index 00000000000000..71a9d73aaf68df --- /dev/null +++ b/test/integration/targets/vault/test_dangling_temp.yml @@ -0,0 +1,34 @@ +- hosts: localhost + gather_facts: False + vars: + od: "{{output_dir|default('/tmp')}}/test_vault_assemble" + tasks: + - name: create target directory + file: + path: "{{od}}" + state: directory + + - name: assemble_file file with secret + assemble: + src: files/test_assemble + dest: "{{od}}/dest_file" + remote_src: no + mode: 0600 + + - name: remove assembled file with secret (so nothing should have unencrypted secret) + file: path="{{od}}/dest_file" state=absent + + - name: find temp files with secrets + find: + paths: '{{temp_paths}}' + contains: 'VAULT TEST IN WHICH BAD THING HAPPENED' + recurse: yes + register: badthings + vars: + temp_paths: "{{[lookup('env', 'TMP'), lookup('env', 'TEMP'), hardcoded]|flatten(1)|unique|list}}" + hardcoded: ['/tmp', '/var/tmp'] + + - name: ensure we failed to find any + assert: + that: + - badthings['matched'] == 0 From f6d7c306e347bbae7d350e1eeb91b9c2a49e1914 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Wed, 25 Mar 2020 13:35:38 -0400 Subject: [PATCH 2/2] remove 2nd cleanuP --- lib/ansible/executor/process/worker.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ansible/executor/process/worker.py b/lib/ansible/executor/process/worker.py index d02a86589955c3..0b18fc351f3d02 100644 --- a/lib/ansible/executor/process/worker.py +++ b/lib/ansible/executor/process/worker.py @@ -206,8 +206,6 @@ def _run(self): display.debug(u"WORKER TRACEBACK: %s" % to_text(traceback.format_exc())) finally: self._clean_up() - finally: - self._clean_up() display.debug("WORKER PROCESS EXITING")