Pinentry fallback mechanism is poorly constructed

[ultimatespirit]

Checked the issue history and saw several pinentry issues that basically went over this exact same issue and never really got handled in any longterm manner.

Issue

As noted here on line 491 tomb rolls its own fallback mechanism for /usr/bin/pinentry, and does so poorly.

Current algorithm:

• Check if a DISPLAY is set
• If so, check if a specific command happens to exist, and if it does use it unconditionally
• Else, try more commands
• Else, use pinentry-curses else pinentry-tty (one of those virtually guaranteed to succeed if they exist)
• Else, fail

Algorithm used by /usr/bin/pinentry wrapping script

Note, this is from my Arch Linux installation with pinentry 1.3.1-5.

# Guess preferred backend based on environment.
backends=(curses tty)
if [[ -n "$DISPLAY" || -n "$WAYLAND_DISPLAY" ]]; then
	case "$XDG_CURRENT_DESKTOP" in
	KDE|LXQT|LXQt)
		backends=(qt qt5 gnome3 gtk curses tty)
		;;
	*)
		backends=(gnome3 gtk qt qt5 curses tty)
		;;
	esac
fi

for backend in "${backends[@]}"
do
	lddout=$(ldd "/usr/bin/pinentry-$backend" 2>/dev/null) || continue
#       ^~~~ most important part here, emphasis mine
	[[ "$lddout" == *'not found'* ]] && continue
	exec "/usr/bin/pinentry-$backend" "$@"
done

Which importantly checks that the found executable would even run before deciding on it.

Proposed solution

Several fold solution:

1. Just adopt that wrapper script's implementation. Checking an array of potential executables is clearly more maintainable than the current hand-unrolled implementation, and ldd is guaranteed to be on any functional system, or at least any sane one. On the off chance it's a musl-only system and they didn't make a ldd symlink, consider having support for ld-musl-$ARCH.so.
2. Provide a user-facing option to just specify the pinentry they want to use at command invocation time
3. Also, add pinentry-gtk to the list of potential applications, with gtk2 gone Arch Linux at least has removed pinentry-gtk2 and made pinentry-gtk3 -> pinentry-gtk

The implementation

# Guess preferred backend based on environment.
backends=(curses tty)
if [[ -n "$DISPLAY" || -n "$WAYLAND_DISPLAY" ]]; then
    _verbose "Graphical display system detected"
	case "$XDG_CURRENT_DESKTOP" in
	KDE|LXQT|LXQt)
		backends=(qt qt5 gnome3 gtk gtk2 gtk3 curses tty)
		;;
	*)
		backends=(gnome3 gtk gtk2 gtk3 qt qt5 curses tty)
		;;
	esac
    _verbose "Checking backends '${backends[@]}'"
fi

for backend in "${backends[@]}"
do
	lddout=$(ldd "/usr/bin/pinentry-$backend" 2>/dev/null) || continue
	[[ "$lddout" == *'not found'* ]] && continue
    _verbose "using $backend"
    output=$(pinentry_assuan_getpass | /usr/bin/pinentry-$backend)
    break
done
# Add a check that we asked for a password and then '_failure "Cannot find any viable pinentry command"'

Added in gtk{2,3} fallback, the logging statements tomb has, and changed the logic to match what tomb needs. Importantly, I did not address the "musl system that doesn't have ldd" case, as I can't test that, nor do I know if it's even a case distros can have. Additionally it does not handle alternative paths, I didn't check if tomb takes a which dependency, considering it uses zsh though it should have a which builtin. So can just augment the backend checking code to do lddout=$(ldd $(which $backend) 2>/dev/null) || continue

It's over all a trivial enough fix that I didn't bother making a pull for it. Though, that's also mostly because it doesn't address item 2, which I sincerely believe should be added regardless of this script change, and I'm not sure the exact testing across distributions required. The only real caveats I can see are which (which should be handled by the hard zsh dependency) and ldd (which you have the musl alternative, not sure there's anything else even remotely viable enough to need to be worried about here).

Conclusion

Finally, thanks y'all for making this tool. It's great, warts and all, let's make it greater.

[Narrat]

Thank you for this summary. Indeed a place which could see some improvements. And great that you still opened the PR, even if you were intially hesistant :)

[jaromil]

And thanks for fixing it yourself so nicely @ultimatespirit ! welcome to Tomb's contributors

[linnabraham]

Tomb errors out with - No valid password supplied for me. If I set the GPG_TTY while using tomb open however it works. Is this how it is supposed to be? In contrast, on the same terminal (ssh through termius on phone) the pass show command works perfectly and gives me a tty pinentry

[Narrat]

What version are you using? 2.12 contains a regression that got fixed with 2.13 See https://github.com/dyne/tomb/issues/575 for more information

[linnabraham]

Oh now there seems to be another strange issue. I am on Arch, so the package version shown in paru -Ss is 2.12 but tomb --version reports as 2.12. I even tried the tomb-git version and same. I am not sure if I need to change something in the tomb-git PKGBUILD though.

[Narrat]

Oh now there seems to be another strange issue. I am on Arch, so the package version shown in paru -Ss is 2.12 but tomb --version reports as 2.12.

Huh? Should the version from the paru command have been 2.13? But in general: Yes, 2.13 still reports as 2.12... it seems this bit was forgotten with https://github.com/dyne/tomb/commit/ebefec7e6f97555959b52e2340e83226a5ae04c5

I even tried the tomb-git version and same. I am not sure if I need to change something in the tomb-git PKGBUILD though.

No need for a change in the PKGBUILD for the -git one.

Could you please post a debug output? (CLI option being -D) And tell something about the general setup? What WM/DE, which pinentry you would expect to appear? You also could test locally with additional output added to the script. See https://github.com/dyne/tomb/issues/575#issuecomment-3122178922 for examples where to place those. Not necessary to change the one installed via the package manager. Copy the script somewhere else and use this for debugging purposes.

(And best being done in a new issue)