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

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).

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
+ansible.cfg: |
-- name: Basic lookup. Fails if example/test doesn't exist
+  [passwordstore_lookup]
-  ansible.builtin.debug:
+  lock=readwrite
-    msg: "{{ lookup('community.general.passwordstore', 'example/test')}}"
+  locktimeout=45s
 
-- name: Basic lookup. Warns if example/test does not exist and returns empty string
+playbook.yml: |
-  ansible.builtin.debug:
+  ---
-    msg: "{{ lookup('community.general.passwordstore', 'example/test missing=warn')}}"
 
-- name: Create pass with random 16 character password. If password exists just give the password
+  # Debug is used for examples, BAD IDEA to show passwords on screen
-  ansible.builtin.debug:
+  - name: Basic lookup. Fails if example/test does not exist
-    var: mypassword
+    ansible.builtin.debug:
-  vars:
+      msg: "{{ lookup('community.general.passwordstore', 'example/test')}}"
-    mypassword: "{{ lookup('community.general.passwordstore', 'example/test create=true')}}"
 
-- name: Create pass with random 16 character password. If password exists just give the password
+  - name: Basic lookup. Warns if example/test does not exist and returns empty string
-  ansible.builtin.debug:
+    ansible.builtin.debug:
-    var: mypassword
+      msg: "{{ lookup('community.general.passwordstore', 'example/test missing=warn')}}"
-  vars:
-    mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=create')}}"
 
-- name: Prints 'abc' if example/test does not exist, just give the password otherwise
+  - name: Create pass with random 16 character password. If password exists just give the password
-  ansible.builtin.debug:
+    ansible.builtin.debug:
-    var: mypassword
+      var: mypassword
-  vars:
+    vars:
-    mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=empty') | default('abc', true) }}"
+      mypassword: "{{ lookup('community.general.passwordstore', 'example/test create=true')}}"
 
-- name: Different size password
+  - name: Create pass with random 16 character password. If password exists just give the password
-  ansible.builtin.debug:
+    ansible.builtin.debug:
-    msg: "{{ lookup('community.general.passwordstore', 'example/test create=true length=42')}}"
+      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
+  - name: Prints 'abc' if example/test does not exist, just give the password otherwise
-  ansible.builtin.debug:
+    ansible.builtin.debug:
-    msg: "{{ lookup('community.general.passwordstore', 'example/test create=true overwrite=true')}}"
+      var: mypassword
+    vars:
+      mypassword: "{{ lookup('community.general.passwordstore', 'example/test missing=empty') | default('abc', true) }}"
 
-- name: Create an alphanumeric password
+  - name: Different size password
-  ansible.builtin.debug:
+    ansible.builtin.debug:
-    msg: "{{ lookup('community.general.passwordstore', 'example/test create=true nosymbols=true') }}"
+      msg: "{{ lookup('community.general.passwordstore', 'example/test create=true length=42')}}"
 
-- name: Return the value for user in the KV pair user, username
+  - 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:
+    ansible.builtin.debug:
-    msg: "{{ lookup('community.general.passwordstore', 'example/test subkey=user')}}"
+      msg: "{{ lookup('community.general.passwordstore', 'example/test create=true overwrite=true')}}"
 
-- name: Return the entire password file content
+  - name: Create an alphanumeric password
-  ansible.builtin.set_fact:
+    ansible.builtin.debug:
-    passfilecontent: "{{ lookup('community.general.passwordstore', 'example/test returnall=true')}}"
+      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):
+    @contextmanager
-        result = []
+    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
+            with self.opt_lock('readwrite'):
-                if self.paramvals['overwrite'] and self.paramvals['subkey'] == 'password':
+                if self.check_pass():     # password exists
-                    result.append(self.update_password())
+                    if self.paramvals['overwrite'] and self.paramvals['subkey'] == 'password':
-                else:
+                        with self.opt_lock('write'):
-                    result.append(self.get_passresult())
+                            result.append(self.update_password())
-            else:                     # password does not exist
+                    else:
-                if self.paramvals['missing'] == 'create':
+                        result.append(self.get_passresult())
-                    result.append(self.generate_password())
+                else:                     # password does not exist
-                else:
+                    if self.paramvals['missing'] == 'create':
-                    result.append(None)
+                        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