Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

~/.cache/gopass/gpg-binary.loc is storing an absolute path to gpg, which overrides the environment $PATH #1662

Closed
siriobalmelli opened this issue Dec 7, 2020 · 5 comments · Fixed by #1684
Labels
bug Defects gpg GPG related ux User experience / User Interface related
Milestone

Comments

@siriobalmelli
Copy link

Summary

~/.cache/gopass/gpg-binary.loc is storing an absolute path to the gpg executable, which overrides the environment $PATH.

Steps To Reproduce

Config file:

---
autoclip: true
autoimport: true
cliptimeout: 45
exportkeys: true
mime: true
nocolor: false
nopager: false
notifications: true
path: /Users/[my_user_name]/.password-store
safecontent: true
mounts: {}
export PATH=/unusual/path/to/gnupg/bin
gopass  my-password  # this works
mv /unusual/path/to/gnupg/bin /unusual/path2/to/gnupg/bin
export PATH=/unusual/path2/to/gnupg/bin
gopass  my-password  # breaks with 'Decryption failed: fork/exec /unusual/path/to/gnupg/bin/gpg'
rm ~/.cache/gopass/gpg-binary.loc
gopass  my-password  # this works again

Expected behavior

Any UNIX program should always respect the contents of $PATH

Environment

  • OS: Mac OS X Catalina
  • OS version: Darwin computer.home 19.6.0 Darwin Kernel Version 19.6.0: Thu Oct 29 22:56:45 PDT 2020; root:xnu-6153.141.2.2~1/RELEASE_X86_64 x86_64 i386 MacBookPro16,1 Darwin
  • gopass Version: gopass 1.10.1+af9c95d20b722dee331adcc3cc1ab1ce0108dc2b (af9c95d20b722dee331adcc3cc1ab1ce0108dc2b) go1.15.6 darwin amd64
  • Installation method: Nix, building the latest build from source
@siriobalmelli siriobalmelli changed the title ~/.cache/gopass/gpg-binary.loc is storing an absolute path to the gpg executable, which overrides the environment $PATH ~/.cache/gopass/gpg-binary.loc is storing an absolute path to gpg, which overrides the environment $PATH Dec 7, 2020
@siriobalmelli siriobalmelli changed the title ~/.cache/gopass/gpg-binary.loc is storing an absolute path to gpg, which overrides the environment $PATH ~/.cache/gopass/gpg-binary.loc is storing an absolute path to gpg, which overrides the environment $PATH Dec 7, 2020
@AnomalRoil AnomalRoil added bug Defects gpg GPG related ux User experience / User Interface related labels Dec 8, 2020
@AnomalRoil AnomalRoil added this to the 1.x.x milestone Dec 8, 2020
@dominikschulz
Copy link
Member

This file is used as an optimization for the awkwardly slow gpg operations we had before.
I guess a proper implementation wouldn't suffer from the limitations and wouldn't need this workaround, but we carry some cruft there. So for the time being this is a wont-fix, but ultimately you are right of course: we must respect $PATH.

siriobalmelli added a commit to siriobalmelli-foss/nixpkgs that referenced this issue Dec 14, 2020
gopass maintains a ~/.cache/gopass/gpg-binary.loc file
which stores an absolute path to the gpg executable,
overriding (and ignoring) the environment $PATH.

This creates a situation where gopass will work for a period of time
after install, but after gpg is upgraded and a 'nix-store --gc' is
performed, the cached gpg path will throw an error.

A gopass maintainer has declared this a wont-fix in
gopasspw/gopass#1662

As a workaround, add a --run clause in the wrapper script
that removes this cache file.

Once (if) upstream fixes this issue, this cruft can be removed.

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
@siriobalmelli
Copy link
Author

I understand the problem, and I see it's a difficult engineering tradeoff to make.

Unfortunately, Nix is very broken by this failure to respect $PATH, so I am forced to work around it by calling rm -f "${XDG_CONFIG_HOME:-~/.config}/gopass/gpg-binary.loc" before every invocation of gopass.

May I make a couple suggestions?

The Ideal Case

Ideally, there is a solution which can be found which will respect $PATH.

This is also a possible security issue because if a serious vulnerability is patched in gpg in the future and the patched gpg is installed in a different location, this program would continue using the old gpg until it was removed (which may never happen on a production system).

It is worth mentioning that even when I am deleting the cache file before each time gopass is run, I am still seeing fast response times:

$ time rm -f "${XDG_CONFIG_HOME:-~/.config}/gopass/gpg-binary.loc" && gopass show -C my_password
Entry 'my_password' not found. Starting search...
Found exact match in 'my_folder/my_password'
✔ Copied my_folder/my_password to clipboard. Will clear in 45 seconds.
login: user_name
password: ********


real	0m0.377s
user	0m0.169s
sys	0m0.263s

By not maintaining the cache file in the first place, this would actually be (slightly) faster.

Perhaps there is a clever solution which leverages the parallelism of Go to search all components of $PATH for the gpg binary at the same time?

The Patch Case

If, after evaluating the above, the decision is still wont-fix, would you at least consider a fall-back path in the code, where:

  • the cached binary is no longer found (error)
  • fall back to searching $PATH and, if finding a gpg binary, update the cache

This is not ideal because it still breaks a basic invariant on how programs should behave with $PATH (it took me a while to find this bug - I really was not expecting for $PATH to be ignored!), but at least it stops gopass from failing because of a "missing" gpg when there is a valid gpg in $PATH.

dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jan 4, 2021
Fixes gopasspw#1662

RELEASE_NOTES=[BUGFIX] Remove GPG location caching

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@dominikschulz
Copy link
Member

@siriobalmelli Thank you for detailing your reasoning. Please have a look at #1684 and let me know what you think.

dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jan 5, 2021
Fixes gopasspw#1662

RELEASE_NOTES=[BUGFIX] Remove GPG location caching

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@siriobalmelli
Copy link
Author

siriobalmelli commented Jan 6, 2021

This seems like the right way to go, I appreciate your work on this 😄

I'll keep this open until #1684 goes through and then I'll test on my end also.

dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jan 6, 2021
Fixes gopasspw#1662

RELEASE_NOTES=[BUGFIX] Remove GPG location caching

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jan 6, 2021
Fixes gopasspw#1662

RELEASE_NOTES=[BUGFIX] Remove GPG location caching

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit that referenced this issue Jan 6, 2021
Fixes #1662

RELEASE_NOTES=[BUGFIX] Remove GPG location caching

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
siriobalmelli added a commit to siriobalmelli-foss/nixpkgs that referenced this issue Jan 13, 2021
Remove workaround introduced in dda50e7:
gopasspw/gopass#1662
has now been fixed upstream.

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
@siriobalmelli
Copy link
Author

Looking good, thank you again @dominikschulz for your assistance getting this sorted.

kpitt pushed a commit to kpitt/gopass that referenced this issue Jul 21, 2022
Fixes gopasspw#1662

RELEASE_NOTES=[BUGFIX] Remove GPG location caching

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects gpg GPG related ux User experience / User Interface related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants