From cdf6e3e4bf44fdab62c2e4ccd3f5fd67ea554548 Mon Sep 17 00:00:00 2001 From: Cambell Date: Fri, 22 Jan 2016 23:23:10 +0700 Subject: [PATCH 1/2] feature/copy-vault-dataloader: Add method get_real_file(file_path) to dataloader - get_real_file will decrypt vault encrypted files and return a path to a temporary file. - cleanup_real_file will remove a temporary file created previously with get_real_file --- lib/ansible/parsing/dataloader.py | 71 ++++++++++++++++++++++++++++++ lib/ansible/plugins/action/copy.py | 6 +++ test/integration/Makefile | 8 ++-- test/integration/test_vault.yml | 2 + test/integration/vault-secret.txt | 6 +++ 5 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 test/integration/vault-secret.txt diff --git a/lib/ansible/parsing/dataloader.py b/lib/ansible/parsing/dataloader.py index c0203d3cc3..bdc62d10b5 100644 --- a/lib/ansible/parsing/dataloader.py +++ b/lib/ansible/parsing/dataloader.py @@ -24,6 +24,10 @@ import os import json import subprocess from yaml import YAMLError +import tempfile + +from yaml import load, YAMLError +>>>>>>> feature/copy-vault-dataloader: Add method get_real_file(file_path) to dataloader from ansible.compat.six import text_type, string_types from ansible.errors import AnsibleFileNotFound, AnsibleParserError, AnsibleError @@ -58,10 +62,15 @@ class DataLoader(): def __init__(self): self._basedir = '.' self._FILE_CACHE = dict() + self._tempfiles = set() # initialize the vault stuff with an empty password self.set_vault_password(None) + def __del__(self): + for f in self._tempfiles: + os.unlink(f) + def set_vault_password(self, vault_password): self._vault_password = vault_password self._vault = VaultLib(password=vault_password) @@ -292,3 +301,65 @@ class DataLoader(): f.close() except (OSError, IOError) as e: raise AnsibleError("Could not read vault password file %s: %s" % (this_path, e)) + + def _create_content_tempfile(self, content): + ''' Create a tempfile containing defined content ''' + fd, content_tempfile = tempfile.mkstemp() + f = os.fdopen(fd, 'wb') + content = to_bytes(content) + try: + f.write(content) + except Exception as err: + os.remove(content_tempfile) + raise Exception(err) + finally: + f.close() + return content_tempfile + + def get_real_file(self, file_path): + """ + If the file is vault encrypted return a path to a temporary decrypted file + If the file is not encrypted then the path is returned + Temporary files are cleanup in the destructor + """ + + if not file_path or not isinstance(file_path, string_types): + raise AnsibleParserError("Invalid filename: '%s'" % str(file_path)) + + if not self.path_exists(file_path) or not self.is_file(file_path): + raise AnsibleFileNotFound("the file_name '%s' does not exist, or is not readable" % file_path) + + if not self._vault: + self._vault = VaultLib(password="") + + real_path = self.path_dwim(file_path) + + try: + with open(real_path, 'rb') as f: + data = f.read() + if self._vault.is_encrypted(data): + # if the file is encrypted and no password was specified, + # the decrypt call would throw an error, but we check first + # since the decrypt function doesn't know the file name + if not self._vault_password: + raise AnsibleParserError("A vault password must be specified to decrypt %s" % file_path) + + data = self._vault.decrypt(data) + # Make a temp file + real_path = self._create_content_tempfile(data) + self._tempfiles.add(real_path) + + return real_path + + except (IOError, OSError) as e: + raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (file_name, str(e))) + + def cleanup_real_file(self, file_path): + """ + Removes any temporary files created from a previous call to + get_real_file. file_path must be the path returned from a + previous call to get_real_file. + """ + if file_path in self._tempfiles: + os.unlink(file_path) + self._tempfiles.remove(file_path); diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index 9734a29444..7eb805aa5d 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/lib/ansible/plugins/action/copy.py @@ -152,6 +152,8 @@ class ActionModule(ActionBase): diffs = [] for source_full, source_rel in source_files: + source_full = self._loader.get_real_file(source_full) + # Generate a hash of the local file. local_checksum = checksum(source_full) @@ -219,6 +221,8 @@ class ActionModule(ActionBase): # We have copied the file remotely and no longer require our content_tempfile self._remove_tempfile_if_content_defined(content, content_tempfile) + self._loader.cleanup_real_file(source_full) + # fix file permissions when the copy is done as a different user self._fixup_perms(tmp, remote_user, recursive=True) @@ -247,6 +251,8 @@ class ActionModule(ActionBase): # the file module in case we want to change attributes self._remove_tempfile_if_content_defined(content, content_tempfile) + self._loader.cleanup_real_file(source_full) + if raw: # Continue to next iteration if raw is defined. self._remove_tmp_path(tmp) diff --git a/test/integration/Makefile b/test/integration/Makefile index db610c4641..a812ace890 100644 --- a/test/integration/Makefile +++ b/test/integration/Makefile @@ -144,10 +144,10 @@ test_var_precedence: setup ansible-playbook test_var_precedence.yml -i $(INVENTORY) $(CREDENTIALS_ARG) $(TEST_FLAGS) -v -e outputdir=$(TEST_DIR) -e 'extra_var=extra_var' -e 'extra_var_override=extra_var_override' test_vault: setup - ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) --list-tasks -e outputdir=$(TEST_DIR) - ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) --list-hosts -e outputdir=$(TEST_DIR) - ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) --syntax-check -e outputdir=$(TEST_DIR) - ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) -e outputdir=$(TEST_DIR) + ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) --list-tasks -e outputdir=$(TEST_DIR) -e @$(VARS_FILE) + ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) --list-hosts -e outputdir=$(TEST_DIR) -e @$(VARS_FILE) + ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) --syntax-check -e outputdir=$(TEST_DIR) -e @$(VARS_FILE) + ansible-playbook test_vault.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) --vault-password-file $(VAULT_PASSWORD_FILE) -e outputdir=$(TEST_DIR) -e @$(VARS_FILE) # test_delegate_to does not work unless we have permission to ssh to localhost. # Would take some more effort on our test systems to implement that -- probably diff --git a/test/integration/test_vault.yml b/test/integration/test_vault.yml index 3313f32d07..a3b2498766 100644 --- a/test/integration/test_vault.yml +++ b/test/integration/test_vault.yml @@ -8,4 +8,6 @@ - assert: that: - 'secret_var == "secret"' + + - copy: src=vault-secret.txt dest={{output_dir}}/secret.txt diff --git a/test/integration/vault-secret.txt b/test/integration/vault-secret.txt new file mode 100644 index 0000000000..b6bc9bfb17 --- /dev/null +++ b/test/integration/vault-secret.txt @@ -0,0 +1,6 @@ +$ANSIBLE_VAULT;1.1;AES256 +39303432393062643236616234306333383838333662386165616633303735336537613337396337 +6662666233356462326631653161663663363166323338320a653131656636666339633863346530 +32326238646631653133643936306666643065393038386234343736663239363665613963343661 +3230353633643361650a363034323631613864326438396665343237383566336339323837326464 +3930 From 5940d3d45b7361a92c13edd3f6379432e112a705 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Thu, 14 Apr 2016 10:31:39 -0400 Subject: [PATCH 2/2] fixes to vault/copy rm _del_ as it might leak memory renamed to tmp file cleanup added exception handling when traversing file list, even if one fails try rest added cleanup to finally to ensure removal in most cases --- lib/ansible/cli/adhoc.py | 2 ++ lib/ansible/cli/console.py | 2 ++ lib/ansible/executor/playbook_executor.py | 2 ++ lib/ansible/parsing/dataloader.py | 19 ++++++++++--------- lib/ansible/plugins/action/copy.py | 6 ++---- 5 files changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/ansible/cli/adhoc.py b/lib/ansible/cli/adhoc.py index c1a23f6c25..becb43e581 100644 --- a/lib/ansible/cli/adhoc.py +++ b/lib/ansible/cli/adhoc.py @@ -194,5 +194,7 @@ class AdHocCLI(CLI): finally: if self._tqm: self._tqm.cleanup() + if loader: + loader.cleanup_all_tmp_files() return result diff --git a/lib/ansible/cli/console.py b/lib/ansible/cli/console.py index c6b27f8f66..5c844e6094 100644 --- a/lib/ansible/cli/console.py +++ b/lib/ansible/cli/console.py @@ -215,6 +215,8 @@ class ConsoleCLI(CLI, cmd.Cmd): finally: if self._tqm: self._tqm.cleanup() + if self.loader: + self.loader.cleanup_all_tmp_files() if result is None: display.error("No hosts found") diff --git a/lib/ansible/executor/playbook_executor.py b/lib/ansible/executor/playbook_executor.py index 4827ebc8d4..7179692123 100644 --- a/lib/ansible/executor/playbook_executor.py +++ b/lib/ansible/executor/playbook_executor.py @@ -194,6 +194,8 @@ class PlaybookExecutor: finally: if self._tqm is not None: self._tqm.cleanup() + if self._loader: + self._loader.cleanup_all_tmp_files() if self._options.syntax: display.display("No issues encountered") diff --git a/lib/ansible/parsing/dataloader.py b/lib/ansible/parsing/dataloader.py index bdc62d10b5..5043d0e793 100644 --- a/lib/ansible/parsing/dataloader.py +++ b/lib/ansible/parsing/dataloader.py @@ -23,11 +23,9 @@ import copy import os import json import subprocess -from yaml import YAMLError import tempfile +from yaml import YAMLError -from yaml import load, YAMLError ->>>>>>> feature/copy-vault-dataloader: Add method get_real_file(file_path) to dataloader from ansible.compat.six import text_type, string_types from ansible.errors import AnsibleFileNotFound, AnsibleParserError, AnsibleError @@ -67,10 +65,6 @@ class DataLoader(): # initialize the vault stuff with an empty password self.set_vault_password(None) - def __del__(self): - for f in self._tempfiles: - os.unlink(f) - def set_vault_password(self, vault_password): self._vault_password = vault_password self._vault = VaultLib(password=vault_password) @@ -352,9 +346,9 @@ class DataLoader(): return real_path except (IOError, OSError) as e: - raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (file_name, str(e))) + raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (real_path, str(e))) - def cleanup_real_file(self, file_path): + def cleanup_tmp_file(self, file_path): """ Removes any temporary files created from a previous call to get_real_file. file_path must be the path returned from a @@ -363,3 +357,10 @@ class DataLoader(): if file_path in self._tempfiles: os.unlink(file_path) self._tempfiles.remove(file_path); + + def cleanup_all_tmp_files(self): + for f in self._tempfiles: + try: + self.cleanup_tmp_file(f) + except: + pass #TODO: this should at least warn diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index 7eb805aa5d..f798f8c4b8 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/lib/ansible/plugins/action/copy.py @@ -220,8 +220,7 @@ class ActionModule(ActionBase): # We have copied the file remotely and no longer require our content_tempfile self._remove_tempfile_if_content_defined(content, content_tempfile) - - self._loader.cleanup_real_file(source_full) + self._loader.cleanup_tmp_file(source_full) # fix file permissions when the copy is done as a different user self._fixup_perms(tmp, remote_user, recursive=True) @@ -250,8 +249,7 @@ class ActionModule(ActionBase): # no need to transfer the file, already correct hash, but still need to call # the file module in case we want to change attributes self._remove_tempfile_if_content_defined(content, content_tempfile) - - self._loader.cleanup_real_file(source_full) + self._loader.cleanup_tmp_file(source_full) if raw: # Continue to next iteration if raw is defined.