From 0971a342d85b77bc0aa54c73fd0f8cccc0d6b1f0 Mon Sep 17 00:00:00 2001 From: Zhikang Zhang Date: Wed, 15 Aug 2018 15:10:52 -0400 Subject: [PATCH] Solve race condition in password lookup (#42529) NOTE: 1. use os.open() with os.O_CREAT|os.O_EXCL to check existence and create a lock file if not exists, it's an atomic operation 2. the fastest process will create the lock file and others will wait until the lock file is removed 3. after the writer finished writing to the password file, all the reading operations use built-in open so processes can read the file parallel --- lib/ansible/plugins/lookup/password.py | 46 +++++++++++++++++++++- test/units/plugins/lookup/test_password.py | 41 ++++++++++++++++++- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/lib/ansible/plugins/lookup/password.py b/lib/ansible/plugins/lookup/password.py index 746e7d003ed..2d41ba92629 100644 --- a/lib/ansible/plugins/lookup/password.py +++ b/lib/ansible/plugins/lookup/password.py @@ -92,6 +92,9 @@ _raw: import os import string +import time +import shutil +import hashlib from ansible.errors import AnsibleError, AnsibleAssertionError from ansible.module_utils._text import to_bytes, to_native, to_text @@ -267,6 +270,40 @@ def _write_password_file(b_path, content): f.write(b_content) +def _get_lock(b_path): + """Get the lock for writing password file.""" + first_process = False + b_pathdir = os.path.dirname(b_path) + lockfile_name = to_bytes("%s.ansible_lockfile" % hashlib.md5(b_path).hexdigest()) + lockfile = os.path.join(b_pathdir, lockfile_name) + if not os.path.exists(lockfile) and b_path != to_bytes('/dev/null'): + try: + makedirs_safe(b_pathdir, mode=0o700) + fd = os.open(lockfile, os.O_CREAT | os.O_EXCL) + os.close(fd) + first_process = True + except OSError as e: + if e.strerror != 'File exists': + raise + + counter = 0 + # if the lock is got by other process, wait until it's released + while os.path.exists(lockfile) and not first_process: + time.sleep(2 ** counter) + if counter >= 2: + raise AnsibleError("Password lookup cannot get the lock in 7 seconds, abort..." + "This may caused by un-removed lockfile" + "you can manually remove it from controller machine at %s and try again" % lockfile) + counter += 1 + return first_process, lockfile + + +def _release_lock(lockfile): + """Release the lock so other processes can read the password file.""" + if os.path.exists(lockfile): + os.remove(lockfile) + + class LookupModule(LookupBase): def run(self, terms, variables, **kwargs): ret = [] @@ -277,7 +314,10 @@ class LookupModule(LookupBase): b_path = to_bytes(path, errors='surrogate_or_strict') chars = _gen_candidate_chars(params['chars']) - changed = False + changed = None + # make sure only one process finishes all the job first + first_process, lockfile = _get_lock(b_path) + content = _read_password_file(b_path) if content is None or b_path == to_bytes('/dev/null'): @@ -295,6 +335,10 @@ class LookupModule(LookupBase): content = _format_content(plaintext_password, salt, encrypt=params['encrypt']) _write_password_file(b_path, content) + if first_process: + # let other processes continue + _release_lock(lockfile) + if params['encrypt']: password = do_encrypt(plaintext_password, params['encrypt'], salt=salt) ret.append(password) diff --git a/test/units/plugins/lookup/test_password.py b/test/units/plugins/lookup/test_password.py index 17da1373baa..9dcd783ee2e 100644 --- a/test/units/plugins/lookup/test_password.py +++ b/test/units/plugins/lookup/test_password.py @@ -29,6 +29,7 @@ from ansible.compat.tests.mock import mock_open, patch from ansible.errors import AnsibleError from ansible.module_utils.six import text_type from ansible.module_utils.six.moves import builtins +from ansible.module_utils._text import to_bytes from ansible.plugins.loader import PluginLoader from ansible.plugins.lookup import password from ansible.utils import encrypt @@ -372,6 +373,14 @@ class TestLookupModule(unittest.TestCase): self.fake_loader = DictDataLoader({'/path/to/somewhere': 'sdfsdf'}) self.password_lookup = password.LookupModule(loader=self.fake_loader) self.os_path_exists = password.os.path.exists + self.os_open = password.os.open + password.os.open = lambda path, flag: None + self.os_close = password.os.close + password.os.close = lambda fd: None + self.os_remove = password.os.remove + password.os.remove = lambda path: None + self.makedirs_safe = password.makedirs_safe + password.makedirs_safe = lambda path, mode: None # Different releases of passlib default to a different number of rounds self.sha256 = passlib.registry.get_crypt_handler('pbkdf2_sha256') @@ -380,6 +389,10 @@ class TestLookupModule(unittest.TestCase): def tearDown(self): password.os.path.exists = self.os_path_exists + password.os.open = self.os_open + password.os.close = self.os_close + password.os.remove = self.os_remove + password.makedirs_safe = self.makedirs_safe passlib.registry.register_crypt_handler(self.sha256, force=True) @patch.object(PluginLoader, '_get_paths') @@ -425,7 +438,7 @@ class TestLookupModule(unittest.TestCase): @patch('ansible.plugins.lookup.password._write_password_file') def test_password_already_created_encrypt(self, mock_get_paths, mock_write_file): mock_get_paths.return_value = ['/path/one', '/path/two', '/path/three'] - password.os.path.exists = lambda x: True + password.os.path.exists = lambda x: x == to_bytes('/path/to/somewhere') with patch.object(builtins, 'open', mock_open(read_data=b'hunter42 salt=87654321\n')) as m: results = self.password_lookup.run([u'/path/to/somewhere chars=anything encrypt=pbkdf2_sha256'], None) @@ -436,7 +449,7 @@ class TestLookupModule(unittest.TestCase): @patch('ansible.plugins.lookup.password._write_password_file') def test_password_already_created_no_encrypt(self, mock_get_paths, mock_write_file): mock_get_paths.return_value = ['/path/one', '/path/two', '/path/three'] - password.os.path.exists = lambda x: True + password.os.path.exists = lambda x: x == to_bytes('/path/to/somewhere') with patch.object(builtins, 'open', mock_open(read_data=b'hunter42 salt=87654321\n')) as m: results = self.password_lookup.run([u'/path/to/somewhere chars=anything'], None) @@ -452,3 +465,27 @@ class TestLookupModule(unittest.TestCase): results = self.password_lookup.run([u'/path/to/somewhere chars=a'], None) for result in results: self.assertEquals(result, u'a' * password.DEFAULT_LENGTH) + + def test_lock_been_held(self): + # pretend the lock file is here + password.os.path.exists = lambda x: True + try: + with patch.object(builtins, 'open', mock_open(read_data=b'hunter42 salt=87654321\n')) as m: + # should timeout here + results = self.password_lookup.run([u'/path/to/somewhere chars=anything'], None) + self.fail("Lookup didn't timeout when lock already been held") + except AnsibleError: + pass + + def test_lock_not_been_held(self): + # pretend now there is password file but no lock + password.os.path.exists = lambda x: x == to_bytes('/path/to/somewhere') + try: + with patch.object(builtins, 'open', mock_open(read_data=b'hunter42 salt=87654321\n')) as m: + # should not timeout here + results = self.password_lookup.run([u'/path/to/somewhere chars=anything'], None) + except AnsibleError: + self.fail('Lookup timeouts when lock is free') + + for result in results: + self.assertEqual(result, u'hunter42')