Tomb icon indicating copy to clipboard operation
Tomb copied to clipboard

Rewrites `pinentry` find code to be more resilient

Open ultimatespirit opened this issue 1 year ago • 2 comments

This change rewrites the custom pinentry search code to instead be a modified form of the standard /usr/bin/pinetry fallback script. The prior behaviour could not handle cases where a pinentry executable existed but was not actually usable. Now it checks using ldd for if an executable is functional or not. Additionally rewritten to be more clear and easier to extend with newer pinentry backends.

make test was ran, and it passed the main usability tests for password entry. As my testing system uses swap it failed during the KDF test from an error from swap being enabled; I did not attempt further tests with swap disabled.

This partially fixes Issue #542.

Note: This adds a new dependency on ldd. For systems without ldd installed (such as musl systems) they either need to create a compatibility ldd symlink / script, or we need to check for ld-musl-$ARCH.so and use it ourselves if ldd does not exist.

ultimatespirit avatar Sep 18 '24 22:09 ultimatespirit

It is documented in the official FAQ so users should be aware of such specialities and take care of it themselves? Magically doing stuff behind the scenes like tampering with $PATH and files therein is kinda not nice.

Narrat avatar Sep 29 '24 20:09 Narrat

Sorry, been busy with work and haven't had time to address this yet. Going to leave a quick proof sketch, and a follow up.

First some administrative points:

  • I do not have a musl system set up, so I cannot test this directly.
  • I do not have a non-x86_64 system set up, nor is it running musl, so I cannot test that directly either.
  • When I tried doing some further research into the practicalities of detecting musl usage, beyond just the FAQ about ldd in musl we all know, I saw repeated mentions of systems with musl having a ldd in their path anyway, presumably from distributions running musl knowing to implement that sanity work-around. So this may be superfluous in the expected cases anyway.
  • I'd appreciate testing by people who do have musl, or non-x86_64 architectures and musl, either of the proof sketch below or the final patch whenever I get to make it.
  • P.S. I keep mentioning "make a CLI argument to override this" for the implementation, in part the reason I haven't just done that is due to making this PR to fix my immediate breakage on archlinux, and not really wanting to read through the semi-complicated looking CLI parsing logic I saw in the tomb script in the bottom. The actual concept isn't too hard, just get a _pinentry_cmd variable defined by a cli arg, check if it exists, and if so skip all the checking logic. If not, do the logic implemented here for determining what pinentry to use.

Basic idea:

# `_ldd` must ALWAYS be used in a subshell, it potential runs `exec`.
local _ldd
local _potential_musl="/lib/ld-musl-$(uname -m).so.1"
if _is_found ldd; then
    # Nothing to do
    _ldd="ldd"
elif [ -f "$_potential_musl" ]; then
    # Musl will act like `ldd` if called as `ldd`. This requires an exec hack sadly.
    _ldd="exec -a ldd $_potential_musl"
else
    # Give up and revert to old behaviour of just blindly using the first thing we find
    # TODO: Make a CLI arg to specify pinentry executable and mention it in this warning
    _warning "System has no 'ldd' command nor does it appear to be running musl, falling back to imprecise pinentry detection mode"
    _ldd="true"
fi
unset _potential_musl
...
lddout=$($_ldd "$pinentry" 2>/dev/null) || continue
[[ "$lddout" == *'not found'* ]] && continue

The uname and true commands are specified by POSIX, if concerned could use : instead of true but I feel that's less clear to casual readers. Note that we don't actually need to use true here, since the detection is string reading based anyway, so long as the command used doesn't output the line "not found" it's a valid fall through case.

ultimatespirit avatar Oct 03 '24 11:10 ultimatespirit

The _pinentry_cmd variable defined by a cli arg to override detection is a nice idea. Ideally any automatic detection should have a way to be overridden.

jaromil avatar Dec 30 '24 23:12 jaromil

Time to merge this! very well done

jaromil avatar Jan 27 '25 10:01 jaromil