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

Rewrites pinentry find code to be more resilient #543

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ultimatespirit
Copy link

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.

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 dyne#542.
@Narrat Narrat requested a review from jaromil September 29, 2024 20:07
Copy link
Member

@jaromil jaromil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a neat improvement, good to ldd check. But we need to build the musl compatible alternative in it to avoid breaking people's workflow.

@Narrat
Copy link
Collaborator

Narrat commented Sep 29, 2024

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.

@ultimatespirit
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants