From 3d4dc206a1e7ede5aead9198dbd8633214194e2b Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Tue, 19 Jan 2016 11:59:43 +0000 Subject: [PATCH 1/2] Prevent race in key setup for accelerate daemon Ensure that initial setup in creating the key directory for ansible accelerate mode keys, and generation/storage of the key for a particular host are completed in a thread safe manner. Creating directories/files and then assigning permissions and contents to them means that paths may exist and satisfy the os.path.exists() method in python for other threads before they are usuable. Use a combination of locking around operations with use of unique named files and an OS file system move to ensure that the conditions of checking whether a file or directory exists, where it is potentially created by another thread, will only succeed when the file has both the correct contents and permissions. Fixes #13850 --- lib/ansible/utils/__init__.py | 46 +++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/lib/ansible/utils/__init__.py b/lib/ansible/utils/__init__.py index 8120fbc1398..1ad8dd8c394 100644 --- a/lib/ansible/utils/__init__.py +++ b/lib/ansible/utils/__init__.py @@ -50,6 +50,8 @@ import getpass import sys import subprocess import contextlib +import threading +import tempfile from vault import VaultLib @@ -63,6 +65,7 @@ LOOKUP_REGEX = re.compile(r'lookup\s*\(') PRINT_CODE_REGEX = re.compile(r'(?:{[{%]|[%}]})') CODE_REGEX = re.compile(r'(?:{%|%})') +_LOCK = threading.Lock() try: # simplejson can be much faster if it's available @@ -128,8 +131,15 @@ def key_for_hostname(hostname): key_path = os.path.expanduser(C.ACCELERATE_KEYS_DIR) if not os.path.exists(key_path): - os.makedirs(key_path, mode=0700) - os.chmod(key_path, int(C.ACCELERATE_KEYS_DIR_PERMS, 8)) + # avoid race with multiple forks trying to create paths on host + # but limit when locking is needed to creation only + with(_LOCK): + if not os.path.exists(key_path): + # use a temp directory and rename to ensure the directory + # searched for only appears after permissions applied. + tmp_dir = tempfile.mkdtemp(dir=os.path.dirname(key_path)) + os.chmod(tmp_dir, int(C.ACCELERATE_KEYS_DIR_PERMS, 8)) + os.rename(tmp_dir, key_path) elif not os.path.isdir(key_path): raise errors.AnsibleError('ACCELERATE_KEYS_DIR is not a directory.') @@ -140,19 +150,25 @@ def key_for_hostname(hostname): # use new AES keys every 2 hours, which means fireball must not allow running for longer either if not os.path.exists(key_path) or (time.time() - os.path.getmtime(key_path) > 60*60*2): - key = AesKey.Generate(size=256) - fd = os.open(key_path, os.O_WRONLY | os.O_CREAT, int(C.ACCELERATE_KEYS_FILE_PERMS, 8)) - fh = os.fdopen(fd, 'w') - fh.write(str(key)) - fh.close() - return key - else: - if stat.S_IMODE(os.stat(key_path).st_mode) != int(C.ACCELERATE_KEYS_FILE_PERMS, 8): - raise errors.AnsibleError('Incorrect permissions on the key file for this host. Use `chmod 0%o %s` to correct this issue.' % (int(C.ACCELERATE_KEYS_FILE_PERMS, 8), key_path)) - fh = open(key_path) - key = AesKey.Read(fh.read()) - fh.close() - return key + # avoid race with multiple forks trying to create key + # but limit when locking is needed to creation only + with(_LOCK): + if not os.path.exists(key_path) or (time.time() - os.path.getmtime(key_path) > 60*60*2): + key = AesKey.Generate() + # use temp file to ensure file only appears once it has + # desired contents and permissions + with tempfile.NamedTemporaryFile(mode='w', dir=os.path.dirname(key_path), delete=False) as fh: + tmp_key_path = fh.name + fh.write(str(key)) + os.chmod(tmp_key_path, int(C.ACCELERATE_KEYS_FILE_PERMS, 8)) + os.rename(tmp_key_path, key_path) + return key + + if stat.S_IMODE(os.stat(key_path).st_mode) != int(C.ACCELERATE_KEYS_FILE_PERMS, 8): + raise errors.AnsibleError('Incorrect permissions on the key file for this host. Use `chmod 0%o %s` to correct this issue.' % (int(C.ACCELERATE_KEYS_FILE_PERMS, 8), key_path)) + + with open(key_path) as fh: + return AesKey.Read(fh.read()) def encrypt(key, msg): return key.Encrypt(msg.encode('utf-8')) From 1200a7087972219e1adf8dc46b4c44c08d5a13f1 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Tue, 19 Jan 2016 12:06:46 +0000 Subject: [PATCH 2/2] Fix race in daemon initialize using delegate_to Ensure only one thread can start up an accelerate daemon on a target host where multiple hosts may be specified in the play, gather facts is disabled and the first task delegates to the same target host. This will slow down the initial connection to only allowing a single thread setup a connection at a time, however this should be of a negligible impact overall. --- .../runner/connection_plugins/accelerate.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/ansible/runner/connection_plugins/accelerate.py b/lib/ansible/runner/connection_plugins/accelerate.py index 0627267c16b..8b3d961854c 100644 --- a/lib/ansible/runner/connection_plugins/accelerate.py +++ b/lib/ansible/runner/connection_plugins/accelerate.py @@ -21,6 +21,7 @@ import base64 import socket import struct import time +import threading from ansible.callbacks import vvv, vvvv from ansible.errors import AnsibleError, AnsibleFileNotFound from ansible.runner.connection_plugins.ssh import Connection as SSHConnection @@ -35,6 +36,8 @@ from ansible import constants # multiple of the value to speed up file reads. CHUNK_SIZE=1044*20 +_LOCK = threading.Lock() + class Connection(object): ''' raw socket accelerated connection ''' @@ -111,6 +114,15 @@ class Connection(object): def connect(self, allow_ssh=True): ''' activates the connection object ''' + # ensure only one fork tries to setup the connection, in case the + # first task for multiple hosts is delegated to the same host. + if not self.is_connected: + with(_LOCK): + return self._connect(allow_ssh) + + return self + + def _connect(self, allow_ssh=True): try: if not self.is_connected: wrong_user = False @@ -150,7 +162,7 @@ class Connection(object): res = self._execute_accelerate_module() if not res.is_successful(): raise AnsibleError("Failed to launch the accelerated daemon on %s (reason: %s)" % (self.host,res.result.get('msg'))) - return self.connect(allow_ssh=False) + return self._connect(allow_ssh=False) else: raise AnsibleError("Failed to connect to %s:%s" % (self.host,self.accport)) self.is_connected = True