From 2416b81aa43164b2f47024fcea9dcb9585e006ba Mon Sep 17 00:00:00 2001
From: grembo <freebsd@grem.de>
Date: Mon, 21 Feb 2022 21:14:17 +0100
Subject: [PATCH] passwordstore: Add configurable locking (#4194)
* 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
---
...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