From da49c0968d6056e7591a324ffab210ce85f9dde4 Mon Sep 17 00:00:00 2001
From: grembo <freebsd@grem.de>
Date: Thu, 17 Feb 2022 20:58:36 +0100
Subject: [PATCH] passwordstore: Prevent using path as password (#4192)

Given a password stored in _path/to/secret_, requesting the password
_path/to_ will literally return `path/to`. This can lead to using
weak passwords by accident/mess up logic in code, based on the
state of the password store.

This is worked around by applying the same logic `pass` uses:
If a password was returned, check if there is a .gpg file it could
have come from. If not, treat it as missing.

Fixes ansible-collections/community.general#4185
---
 ...4192-improve-passwordstore-consistency.yml |  2 +
 plugins/lookup/passwordstore.py               | 46 ++++++++++---------
 2 files changed, 27 insertions(+), 21 deletions(-)
 create mode 100644 changelogs/fragments/4192-improve-passwordstore-consistency.yml

diff --git a/changelogs/fragments/4192-improve-passwordstore-consistency.yml b/changelogs/fragments/4192-improve-passwordstore-consistency.yml
new file mode 100644
index 0000000000..bf50669c34
--- /dev/null
+++ b/changelogs/fragments/4192-improve-passwordstore-consistency.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - passwordstore lookup plugin - prevent returning path names as passwords by accident (https://github.com/ansible-collections/community.general/issues/4185, https://github.com/ansible-collections/community.general/pull/4192).
diff --git a/plugins/lookup/passwordstore.py b/plugins/lookup/passwordstore.py
index 9abb90e950..d5ebf8b6d8 100644
--- a/plugins/lookup/passwordstore.py
+++ b/plugins/lookup/passwordstore.py
@@ -154,6 +154,7 @@ display = Display()
 
 # backhacked check_output with input for python 2.7
 # http://stackoverflow.com/questions/10103551/passing-data-to-subprocess-check-output
+# note: contains special logic for calling 'pass', so not a drop-in replacement for check_output
 def check_output2(*popenargs, **kwargs):
     if 'stdout' in kwargs:
         raise ValueError('stdout argument not allowed, it will be overridden.')
@@ -175,9 +176,10 @@ def check_output2(*popenargs, **kwargs):
         process.wait()
         raise
     retcode = process.poll()
-    if retcode != 0 or \
-            b'encryption failed: Unusable public key' in b_out or \
-            b'encryption failed: Unusable public key' in b_err:
+    if retcode == 0 and (b'encryption failed: Unusable public key' in b_out or
+                         b'encryption failed: Unusable public key' in b_err):
+        retcode = 78  # os.EX_CONFIG
+    if retcode != 0:
         cmd = kwargs.get("args")
         if cmd is None:
             cmd = popenargs[0]
@@ -228,12 +230,11 @@ class LookupModule(LookupBase):
             # Collect pass environment variables from the plugin's parameters.
             self.env = os.environ.copy()
 
-            # Set PASSWORD_STORE_DIR if directory is set
-            if self.paramvals['directory']:
-                if os.path.isdir(self.paramvals['directory']):
-                    self.env['PASSWORD_STORE_DIR'] = self.paramvals['directory']
-                else:
-                    raise AnsibleError('Passwordstore directory \'{0}\' does not exist'.format(self.paramvals['directory']))
+            # Set PASSWORD_STORE_DIR
+            if os.path.isdir(self.paramvals['directory']):
+                self.env['PASSWORD_STORE_DIR'] = self.paramvals['directory']
+            else:
+                raise AnsibleError('Passwordstore directory \'{0}\' does not exist'.format(self.paramvals['directory']))
 
             # Set PASSWORD_STORE_UMASK if umask is set
             if 'umask' in self.paramvals:
@@ -261,19 +262,20 @@ class LookupModule(LookupBase):
                     if ':' in line:
                         name, value = line.split(':', 1)
                         self.passdict[name.strip()] = value.strip()
+            if os.path.isfile(os.path.join(self.paramvals['directory'], self.passname + ".gpg")):
+                # Only accept password as found, if there a .gpg file for it (might be a tree node otherwise)
+                return True
         except (subprocess.CalledProcessError) as e:
-            if e.returncode != 0 and 'not in the password store' in e.output:
-                # if pass returns 1 and return string contains 'is not in the password store.'
-                # We need to determine if this is valid or Error.
-                if self.paramvals['missing'] == 'error':
-                    raise AnsibleError('passwordstore: passname {0} not found and missing=error is set'.format(self.passname))
-                else:
-                    if self.paramvals['missing'] == 'warn':
-                        display.warning('passwordstore: passname {0} not found'.format(self.passname))
-                    return False
-            else:
+            # 'not in password store' is the expected error if a password wasn't found
+            if 'not in the password store' not in e.output:
                 raise AnsibleError(e)
-        return True
+
+        if self.paramvals['missing'] == 'error':
+            raise AnsibleError('passwordstore: passname {0} not found and missing=error is set'.format(self.passname))
+        elif self.paramvals['missing'] == 'warn':
+            display.warning('passwordstore: passname {0} not found'.format(self.passname))
+
+        return False
 
     def get_newpass(self):
         if self.paramvals['nosymbols']:
@@ -329,7 +331,9 @@ class LookupModule(LookupBase):
         result = []
         self.paramvals = {
             'subkey': 'password',
-            'directory': variables.get('passwordstore'),
+            'directory': variables.get('passwordstore', os.environ.get(
+                                       'PASSWORD_STORE_DIR',
+                                       os.path.expanduser('~/.password-store'))),
             'create': False,
             'returnall': False,
             'overwrite': False,