passwordstore lookup plugin gopass compatibility

[stephan-devop]

Summary

The passwordstore lookup plugin uses the binary pass. The plugin used to work fine with the gopass drop-in replacement using a small wrapper script. But sadly, that does not work anymore.

The reason is that the passwordstore plugin checks the internal pass configuration. It checks for the ~/.password-store directory and the .gpg file at a location where pass would store it. If either fails the plugin throws an error.

For me, the main advantage of gopass over pass is the mounts feature which allows multiple password stores at arbitrary paths. They happen to be stored somewhere completely different than pass would store them but the interface to retrieve them is the same. I think that it's not a good idea if the passwordstore lookup plugin verifies the backend's configuration. It should rely on the pass interface instead.

I've tried to understand why the passwordstore plugin checks for the directory and the gpg file and I have two questions:

Why is there an else which kicks in if the passwordstore directory does not exist? Isn't this handled by pass?

Then there is a comment I do not understand:

# Only accept password as found, if there a .gpg file for it (might be a tree node otherwise)

What kind of tree node is this? This looks suspiciously like a workaround for some pass bug which should be fixed upstream.

For the time being, I've implemented a workaround for our (virtual) development environment by applying a simple patch which removes both checks. Of course, this is hardly a general solution, as those checks probably exist for a reason. Could someone please tell me for which reason?

Issue Type

Bug Report

Component Name

passwordstore lookup plugin

Ansible Version

$ ansible --version
ansible [core 2.12.6]
  config file = None
  configured module search path = ['/Users/tovenaar/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.9/dist-packages/ansible
  ansible collection location = /Users/tovenaar/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/local/bin/ansible
  python version = 3.9.2 (default, Feb 28 2021, 17:03:44) [GCC 10.2.1 20210110]
  jinja version = 3.1.2
  libyaml = True

Community.general Version

$ ansible-galaxy collection list community.general
Collection        Version
----------------- -------
community.general 4.8.1

Configuration

$ ansible-config dump --only-changed

OS / Environment

Debian GNU/Linux 11 (bullseye), cf. takelage-dev

Steps to Reproduce

Use gopass as backend instead of pass.

Expected Results

gopass should work as a passwordstore lookup plugin backend as its interface is the same as the pass interface.

Actual Results

The passwordstore lookup plugin complains about the missing pass configuration directory and missing gpg files.

Code of Conduct

• I agree to follow the Ansible Code of Conduct

[ansibullbot]

Files identified in the description:

• lib/ansible/plugins/lookup

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[felixfontein]

This was introduced in #4192, for the rationale see that PR. Maybe @grembo, the PR's author, can also comment on this.

[grembo]

This was introduced in #4192, for the rationale see that PR. Maybe @grembo, the PR's author, can also comment on this.

@stephan-devop Please see #4185 for a detailed description of the problem this fixes, short version: pass doesn't distinguish between a password stored or a tree node of the same name. It has been like that forever. This isn't a problem for interactive use (which is what pass was designed to do), as a human would of course notice the problem.

One is of course free to open an issue there and request a change in behavior, but this will take a while to trickle down into the field reliably (I would assume for it to take years), also because it should somehow be backwards compatible, so probably adding a command line switch, which could then be added to the plugin combined with a version check.

Now, nowhere does the plugin state that it supports anything besides "real" passwordstore, that's why I optimized the behavior for it to stop horrible things to happen by accident (e.g., using the tree node name as a cryptographic key is terribly insecure) . Therefore this wasn't foreseen, but having a compatibility shim is not something outlandish either, so I can feel your pain.

That said, I see multiple practical solutions:

1. Add a gopass plugin to ansible, so it becomes a first class citizen (some work and forces gopass on all users - so if you happen to also have users using pass, probably not a good solution).
2. Alter the passwordstore plugin to have a way to disable the check automatically (e.g. based on the output of pass --version)
3. Alter the passwordstore plugin to add a conditional variable that tells it to skip these checks (something like pass_shim=true/false). I would assume gopass doesn't need those checks.

The last option is probably the most realistic short term solution to your specific problem.

[felixfontein]

There have been fixes in the past (I quickly found #1493, #1589) to make the lookup compatible with gopass, but we do not explicitly declare that it is compatible (for that we should have tests for gopass as well). 2 and 3 sound like good solutions to me, but someone would have to implement them, preferably with tests.

[stephan-devop]

@stephan-devop Please see #4185 for a detailed description of the problem this fixes, short version: pass doesn't distinguish between a password stored or a tree node of the same name.

Why don't we check if the password is equal to the passname and if so then reject it?

[grembo]

Because

1. This is not what is returned in the first place
2. What is returned exactly can also differ based on the version of tree installed (and potentially the character set installed)
3. It is not precise - what we do now is exactly the same check that pass does, so it’s a really very high chance it’s doing the right thing vs a hack like comparing the results of the operation
4. It prevents storing ‘passname’ as a password (well, that’s not a terrible limitation to have, but still, kind of unpredicted behavior, imagine storing “test” in “test” and then the plug-in telling you “password not found”)

Adding what I proposed above is a few lines of code changed plus another five lines of documentation (which then should also mention gopass and the wrapper script).

Is your gopass wrapper available somewhere (if yes, it should be linked to from documentation so it’s actually a thing).

[stephan-devop]

I agree with your reasoning and it's fine for me. I've opted for 2 as I think it is more user-friendly. This is my first pull request for this project so please point out any code style violations I might have committed.

I've created a draft pull request introducing a variable self.backend which is set to the string pass by default. Only if the output of pass --version contains the string gopass then self.backend is set to gopass. Both exceptions for pass are applied unless a different backend has been detected.

Adding what I proposed above is a few lines of code changed plus another five lines of documentation (which then should also mention gopass and the wrapper script).

I've linked the wrapper script above. But the logical next step would be to incorporate the script into the lookup plugin by adding "exceptions" for gopass, right?

Then I had a look at the integration tests. How do I run them locally? Is there some kind of docker environment available?

[stephan-devop]

The wrapper script does two things which I've added to the plugin:

1. It calls gopass version to get rid of the "You haven't checked for updates in a while." message which confuses the parser because it expects a password. This part is already handled by the setup_backend() method which calls pass --version (which is nearly the same and should be ok, too, I guess).
2. It adds --password to gopass show. Someone in the gopass team apparently thought that it's a smart idea to make the verbose output the default. So we have to add the flag --password to fall back to good old pass behaviour: show shows the password and nothing but the password.

[grembo]

Hi @stephan-devop,

I also created a patch (should have probably coordinated with you).

I also went for option 2, but in a lighter way:

1. Only check the version of the pass utility if there is a problem (therefore it's zero cost for pass users)
2. Don't be gopass specific
3. Add integration tests (this was the nasty bit)

Now, I don't want to take away your steam/spirit, but I think this is an easier implementation (also to review). It lacks the extra "--password" flag of course, this would still have to be done in the wrapper.

I'll open a draft review, so we can compare.

[grembo]

I added #4780, which has minimal logic changes to it (and works with wrappers beyond gopass).

@stephan-devop Would that work for you, or do you think it needs to be specific?
@felixfontein Can you live with the integration tests (moving pass around is a bit hacky, but I couldn't think of another low-effort way of doing it - it's a destructive test anyway, so it might be ok)

Again, my bad for not coordinating our efforts, one way or another we spent time working on the same thing.

[stephan-devop]

I have a suggestion how we could get rid of the wrapper script and achieve gopass compatibility without the penalty of a system call for pass users:

If the password contains "You haven't checked for updates in a while." or "Parsing is enabled. Use -n to disable." then run pass show --password and try again. It's ugly and very gopass specific but it should work. And it adds real value to the plugin. ;-)

[stephan-devop]

But anyway, even if I still have to use a wrapper script, I won't have to patch this plugin in the future. So your work helps a lot, thank you!

[grembo]

But anyway, even if I still have to use a wrapper script, I won't have to patch this plugin in the future. So your work helps a lot, thank you!

Let's see what Felix says, but I think for now this is a relatively low profile solution (the change was quite quick, but figuring out integration tests took forever).

There is still room for a change to fully support multiple pass-like managers in the future through explicit configuration or by simply wrapping the existing plugin into another one - but that would be quite a lot of work to yield clean results.

[stephan-devop]

I can understand your point and I appreciate your work. Actually, I found it quite interesting to get a glimpse of the ansible code which I use so often.

[felixfontein]

@felixfontein Can you live with the integration tests (moving pass around is a bit hacky, but I couldn't think of another low-effort way of doing it - it's a destructive test anyway, so it might be ok)

Looks OK to me.

[felixfontein]

How about combining both approaches? I.e. have a backend property that is set to pass if the expected string for the real pass is detected, set to shim if that one is not detected, and set to gopass if gopass is detected?

[grembo]

How about combining both approaches? I.e. have a backend property that is set to pass if the expected string for the real pass is detected, set to shim if that one is not detected, and set to gopass if gopass is detected?

The current approach has the advantage that it’s basically zero cost for users of pass and that it will work with all kinds of shims.

If we add real gopass support, a lot of additional things would need to happen (as I don’t think that it supports all features of pass). We should also ship the required shim in that case (so users don’t hack their own and report their own bugs) and maintain it of course. And we will need unit tests for all of that. In that case it might also make sense to make the binary to call instead of pass configurable, which would bring us close to having a universal password storage plug-in.

As all of this seems like a lot of work, maybe it makes more sense for someone to develop a pass/gopass shim that is smart enough to understand the arguments passed in and mimic pass’s behavior as a separate effort outside of ansible. This would also have the advantage that it could be used in other projects too (think: pkg/apt-get install gopass-shim).

[grembo]

@stephan-devop See below for a cleaner version of your wrapper:

1. Works on all POSIX shells (not just bash).
2. Only one place to call gopass, use exec to make sure the script's return code is always gopass's return code and signal propagation works in case the script's PID is killed
3. Never remind the user of checking for updates (the previous solution of calling gopass version on every invocation created a lot of overhead)
4. Disable desktop notifications (probably a matter of taste)

#!/bin/sh

export GOPASS_NO_REMINDER=yes
export GOPASS_NO_NOTIFY=yes

cmd="$1"
shift
cmdopt=""

# if command is "show", add option to
# return bare password
if [ "$1" = "show" ]; then
    cmdopt="--password"
fi

exec gopass "$cmd" $cmdopt "$@"

[stephan-devop]

@stephan-devop See below for a cleaner version of your wrapper

Thanks a lot! But shouldn’t we initialize $cmdopt with an empty string before using it?

[felixfontein]

The current approach has the advantage that it’s basically zero cost for users of pass and that it will work with all kinds of shims.

My suggestion has zero cost for users of pass ann with all kinds of shims (as your solution), and it has zero costs for users of gopass since they don't need to create a wrapper script.

[grembo]

@stephan-devop See below for a cleaner version of your wrapper

Thanks a lot! But shouldn’t we initialize $cmdopt with an empty string before using it?

Assuming it could be part of the environment, true. I'll edit the comment.

[grembo]

The current approach has the advantage that it’s basically zero cost for users of pass and that it will work with all kinds of shims.

My suggestion has zero cost for users of pass ann with all kinds of shims (as your solution), and it has zero costs for users of gopass since they don't need to create a wrapper script.

How would this be zero cost, if we need to call pass --version unconditionally?

Also, users would still need to install gopass as pass (through a wrapper or as a symlink/copy etc.). So if we support that, I would go the whole nine yards and have a configuration option in ansible.cfg to enable gopass support explicitly (but also leave the shim detection from #4780 in place for generic wrappers to other tools).

This would also allow users to have both packages - gopass and pass - installed on the same host.

[grembo]

@felixfontein Could be as simple as:

diff --git a/plugins/lookup/passwordstore.py b/plugins/lookup/passwordstore.py
index 359190c3..559403d7 100644
--- a/plugins/lookup/passwordstore.py
+++ b/plugins/lookup/passwordstore.py
@@ -106,6 +106,20 @@ DOCUMENTATION = '''
         type: str
         default: 15m
         version_added: 4.5.0
+      backend:
+        description:
+          - Specify which backend to use.
+          - Defaults to C(pass), passwordstore.org's original pass utility.
+          - C(gopass) support is incomplete.
+        ini:
+          - section: passwordstore_lookup
+            key: backend
+        type: str
+        default: pass
+        choices:
+          - pass
+          - gopass
+        version_added: 4.5.0
 '''
 EXAMPLES = """
 ansible.cfg: |
@@ -240,7 +254,7 @@ class LookupModule(LookupBase):
         if self.realpass is None:
             try:
                 self.passoutput = to_text(
-                    check_output2(["pass", "--version"], env=self.env),
+                    check_output2([self.pass_cmd, "--version"], env=self.env),
                     errors='surrogate_or_strict'
                 )
                 self.realpass = 'pass: the standard unix password manager' in self.passoutput
@@ -288,6 +302,9 @@ class LookupModule(LookupBase):
             self.env = os.environ.copy()
             self.env['LANGUAGE'] = 'C'  # make sure to get errors in English as required by check_output2
 
+            if self.get_option('backend') == 'gopass':
+                self.env['GOPASS_NO_REMINDER'] = "YES"
+
             # Set PASSWORD_STORE_DIR
             if os.path.isdir(self.paramvals['directory']):
                 self.env['PASSWORD_STORE_DIR'] = self.paramvals['directory']
@@ -306,7 +323,9 @@ class LookupModule(LookupBase):
     def check_pass(self):
         try:
             self.passoutput = to_text(
-                check_output2(["pass", "show", self.passname], env=self.env),
+                check_output2([self.pass_cmd, 'show'] +
+                              (['--password'] if self.get_option('backend') == 'gopass' else []) +
+                              [self.passname], env=self.env),
                 errors='surrogate_or_strict'
             ).splitlines()
             self.password = self.passoutput[0]
@@ -357,7 +376,7 @@ class LookupModule(LookupBase):
         if self.paramvals['backup']:
             msg += "lookup_pass: old password was {0} (Updated on {1})\n".format(self.password, datetime)
         try:
-            check_output2(['pass', 'insert', '-f', '-m', self.passname], input=msg, env=self.env)
+            check_output2([self.pass_cmd, 'insert', '-f', '-m', self.passname], input=msg, env=self.env)
         except (subprocess.CalledProcessError) as e:
             raise AnsibleError(e)
         return newpass
@@ -369,7 +388,7 @@ class LookupModule(LookupBase):
         datetime = time.strftime("%d/%m/%Y %H:%M:%S")
         msg = newpass + '\n' + "lookup_pass: First generated by ansible on {0}\n".format(datetime)
         try:
-            check_output2(['pass', 'insert', '-f', '-m', self.passname], input=msg, env=self.env)
+            check_output2([self.pass_cmd, 'insert', '-f', '-m', self.passname], input=msg, env=self.env)
         except (subprocess.CalledProcessError) as e:
             raise AnsibleError(e)
         return newpass
@@ -398,6 +417,7 @@ class LookupModule(LookupBase):
             yield
 
     def setup(self, variables):
+        self.pass_cmd = self.get_option('backend')  # pass and gopass are commands as well
         self.locked = None
         timeout = self.get_option('locktimeout')
         if not re.match('^[0-9]+[smh]$', timeout):

[felixfontein]

My suggestion has zero cost for users of pass ann with all kinds of shims (as your solution), and it has zero costs for users of gopass since they don't need to create a wrapper script.

How would this be zero cost, if we need to call pass --version unconditionally?

Ah, I forgot that your solution only calls it in certain cases. In that case your solution isn't zero cost as well ;-) But in the 'happy path' it is...

Also, users would still need to install gopass as pass (through a wrapper or as a symlink/copy etc.).

So far I was assuming that it automatically does that.

So if we support that, I would go the whole nine yards and have a configuration option in ansible.cfg to enable gopass support explicitly (but also leave the shim detection from #4780 in place for generic wrappers to other tools).

This would also allow users to have both packages - gopass and pass - installed on the same host.

That would be even better. Your diff looks good. (I would also allow to configure it by inventory though, i.e. vars, next to ini.)

[grembo]

@felixfontein I integrated this with the previous change and updated #4780.