From 3d2caf3933b752828f59b69e854765bebb3a6e69 Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Mon, 21 Feb 2022 21:37:47 +0100 Subject: [PATCH] passwordstore: Add configurable locking (#4194) (#4243) * passwordstore: Add configurable locking Passwordstore cannot be accessed safely in parallel, which causes various issues: - When accessing the same path, multiple different secrets are returned when the secret didn't exist (missing=create). - When accessing the same _or different_ paths, multiple pinentry dialogs will be spawned by gpg-agent sequentially, having to enter the password for the same gpg key multiple times in a row. - Due to issues in gpg dependencies, accessing gpg-agent in parallel is not reliable, causing plays to fail (this can be fixed by adding `auto-expand-secmem` to _~/.gnupg/gpg-agent.conf_ though). These problems have been described in various github issues in the past, e.g., ansible/ansible#23816 and ansible/ansible#27277. This cannot be worked around in playbooks by users in a non-error-prone way. It is addressed by adding new configuration options: - lock: - readwrite: Lock all operations - write: Only lock write operations (default) - none: Disable locking - locktimeout: Time to wait for getting a lock (s/m/h suffix) (defaults to 15m) These options can also be set in ansible.cfg, e.g.: [passwordstore_lookup] lock=readwrite locktimeout=30s Also, add a note about modifying gpg-agent.conf. * Tidy up locking config There is no reason why lock configuration should be part of self.paramvals. Now locking and its configuration happen all in one place. * Change timeout description wording to the suggested value. * Rearrange plugin setup, apply PR feedback (cherry picked from commit 2416b81aa43164b2f47024fcea9dcb9585e006ba) Co-authored-by: grembo --- ...194-configurable-passwordstore-locking.yml | 2 + plugins/lookup/passwordstore.py | 166 ++++++++++++------ 2 files changed, 118 insertions(+), 50 deletions(-) create mode 100644 changelogs/fragments/4194-configurable-passwordstore-locking.yml diff --git a/changelogs/fragments/4194-configurable-passwordstore-locking.yml b/changelogs/fragments/4194-configurable-passwordstore-locking.yml new file mode 100644 index 0000000000..9268c2cf5a --- /dev/null +++ b/changelogs/fragments/4194-configurable-passwordstore-locking.yml @@ -0,0 +1,2 @@ +minor_changes: + - passwordstore lookup plugin - add configurable ``lock`` and ``locktimeout`` options to avoid race conditions in itself and in the ``pass`` utility it calls. By default, the plugin now locks on write operations (https://github.com/ansible-collections/community.general/pull/4194). diff --git a/plugins/lookup/passwordstore.py b/plugins/lookup/passwordstore.py index b3492745f0..a221e49625 100644 --- a/plugins/lookup/passwordstore.py +++ b/plugins/lookup/passwordstore.py @@ -14,6 +14,8 @@ DOCUMENTATION = ''' description: - Enables Ansible to retrieve, create or update passwords from the passwordstore.org pass utility. It also retrieves YAML style keys stored as multilines in the passwordfile. + - To avoid problems when accessing multiple secrets at once, add C(auto-expand-secmem) to + C(~/.gnupg/gpg-agent.conf). Where this is not possible, consider using I(lock=readwrite) instead. options: _terms: description: query key. @@ -77,54 +79,89 @@ DOCUMENTATION = ''' - warn - empty - create + lock: + description: + - How to synchronize operations. + - The default of C(write) only synchronizes write operations. + - C(readwrite) synchronizes all operations (including read). This makes sure that gpg-agent is never called in parallel. + - C(none) does not do any synchronization. + ini: + - section: passwordstore_lookup + key: lock + type: str + default: write + choices: + - readwrite + - write + - none + version_added: 4.5.0 + locktimeout: + description: + - Lock timeout applied when I(lock) is not C(none). + - Time with a unit suffix, C(s), C(m), C(h) for seconds, minutes, and hours, respectively. For example, C(900s) equals C(15m). + - Correlates with C(pinentry-timeout) in C(~/.gnupg/gpg-agent.conf), see C(man gpg-agent) for details. + ini: + - section: passwordstore_lookup + key: locktimeout + type: str + default: 15m + version_added: 4.5.0 ''' EXAMPLES = """ -# Debug is used for examples, BAD IDEA to show passwords on screen -- name: Basic lookup. Fails if example/test doesn't exist - ansible.builtin.debug: - msg: "{{ lookup('community.general.passwordstore', 'example/test')}}" +ansible.cfg: | + [passwordstore_lookup] + lock=readwrite + locktimeout=45s -- name: Basic lookup. Warns if example/test does not exist and returns empty string - ansible.builtin.debug: - msg: "{{ lookup('community.general.passwordstore', 'example/test missing=warn')}}" +playbook.yml: | + --- -- name: Create pass with random 16 character password. If password exists just give the password - ansible.builtin.debug: - var: mypassword - vars: - mypassword: "{{ lookup('community.general.passwordstore', 'example/test create=true')}}" + # Debug is used for examples, BAD IDEA to show passwords on screen + - name: Basic lookup. Fails if example/test does not exist + ansible.builtin.debug: + msg: "{{ lookup('community.general.passwordstore', 'example/test')}}" -- name: Create pass with random 16 character password. If password exists just give the password - ansible.builtin.debug: - var: mypassword - vars: - mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=create')}}" + - name: Basic lookup. Warns if example/test does not exist and returns empty string + ansible.builtin.debug: + msg: "{{ lookup('community.general.passwordstore', 'example/test missing=warn')}}" -- name: Prints 'abc' if example/test does not exist, just give the password otherwise - ansible.builtin.debug: - var: mypassword - vars: - mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=empty') | default('abc', true) }}" + - name: Create pass with random 16 character password. If password exists just give the password + ansible.builtin.debug: + var: mypassword + vars: + mypassword: "{{ lookup('community.general.passwordstore', 'example/test create=true')}}" -- name: Different size password - ansible.builtin.debug: - msg: "{{ lookup('community.general.passwordstore', 'example/test create=true length=42')}}" + - name: Create pass with random 16 character password. If password exists just give the password + ansible.builtin.debug: + var: mypassword + vars: + mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=create')}}" -- name: Create password and overwrite the password if it exists. As a bonus, this module includes the old password inside the pass file - ansible.builtin.debug: - msg: "{{ lookup('community.general.passwordstore', 'example/test create=true overwrite=true')}}" + - name: Prints 'abc' if example/test does not exist, just give the password otherwise + ansible.builtin.debug: + var: mypassword + vars: + mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=empty') | default('abc', true) }}" -- name: Create an alphanumeric password - ansible.builtin.debug: - msg: "{{ lookup('community.general.passwordstore', 'example/test create=true nosymbols=true') }}" + - name: Different size password + ansible.builtin.debug: + msg: "{{ lookup('community.general.passwordstore', 'example/test create=true length=42')}}" -- name: Return the value for user in the KV pair user, username - ansible.builtin.debug: - msg: "{{ lookup('community.general.passwordstore', 'example/test subkey=user')}}" + - name: Create password and overwrite the password if it exists. As a bonus, this module includes the old password inside the pass file + ansible.builtin.debug: + msg: "{{ lookup('community.general.passwordstore', 'example/test create=true overwrite=true')}}" -- name: Return the entire password file content - ansible.builtin.set_fact: - passfilecontent: "{{ lookup('community.general.passwordstore', 'example/test returnall=true')}}" + - name: Create an alphanumeric password + ansible.builtin.debug: + msg: "{{ lookup('community.general.passwordstore', 'example/test create=true nosymbols=true') }}" + + - name: Return the value for user in the KV pair user, username + ansible.builtin.debug: + msg: "{{ lookup('community.general.passwordstore', 'example/test subkey=user')}}" + + - name: Return the entire password file content + ansible.builtin.set_fact: + passfilecontent: "{{ lookup('community.general.passwordstore', 'example/test returnall=true')}}" """ RETURN = """ @@ -135,13 +172,15 @@ _raw: elements: str """ +from contextlib import contextmanager import os +import re import subprocess import time import yaml - from ansible.errors import AnsibleError, AnsibleAssertionError +from ansible.module_utils.common.file import FileLock from ansible.module_utils.common.text.converters import to_bytes, to_native, to_text from ansible.module_utils.parsing.convert_bool import boolean from ansible.utils.display import Display @@ -328,8 +367,25 @@ class LookupModule(LookupBase): else: return None - def run(self, terms, variables, **kwargs): - result = [] + @contextmanager + def opt_lock(self, type): + if self.get_option('lock') == type: + tmpdir = os.environ.get('TMPDIR', '/tmp') + lockfile = os.path.join(tmpdir, '.passwordstore.lock') + with FileLock().lock_file(lockfile, tmpdir, self.lock_timeout): + self.locked = type + yield + self.locked = None + else: + yield + + def setup(self, variables): + self.locked = None + timeout = self.get_option('locktimeout') + if not re.match('^[0-9]+[smh]$', timeout): + raise AnsibleError("{0} is not a correct value for locktimeout".format(timeout)) + unit_to_seconds = {"s": 1, "m": 60, "h": 3600} + self.lock_timeout = int(timeout[:-1]) * unit_to_seconds[timeout[-1]] self.paramvals = { 'subkey': 'password', 'directory': variables.get('passwordstore', os.environ.get( @@ -345,17 +401,27 @@ class LookupModule(LookupBase): 'missing': 'error', } + def run(self, terms, variables, **kwargs): + self.setup(variables) + result = [] + for term in terms: self.parse_params(term) # parse the input into paramvals - if self.check_pass(): # password exists - if self.paramvals['overwrite'] and self.paramvals['subkey'] == 'password': - result.append(self.update_password()) - else: - result.append(self.get_passresult()) - else: # password does not exist - if self.paramvals['missing'] == 'create': - result.append(self.generate_password()) - else: - result.append(None) + with self.opt_lock('readwrite'): + if self.check_pass(): # password exists + if self.paramvals['overwrite'] and self.paramvals['subkey'] == 'password': + with self.opt_lock('write'): + result.append(self.update_password()) + else: + result.append(self.get_passresult()) + else: # password does not exist + if self.paramvals['missing'] == 'create': + with self.opt_lock('write'): + if self.locked == 'write' and self.check_pass(): # lookup password again if under write lock + result.append(self.get_passresult()) + else: + result.append(self.generate_password()) + else: + result.append(None) return result