pinentry-qt: Add support for external password manager with libsecret
AcceptedPublic
Actions

Authored by jasonc on Nov 8 2023, 11:21 PM.

Details

Reviewers

aheinecke
ikloecker
TobiasFella

Maniphest Tasks

T6801: Pinentry-qt: Add support for external password manager with libsecret

Summary

pinentry-qt: Add support for external password manager with libsecret

* qt/main.cpp: Pass pinentry info to PinEntryDialog constructor. Set
  save passphrase checkbox text from pinentry_t->default_pwmngr.
* qt/pinentrydialog.cpp, qt/pinentrydialog.h: Dialog now accepts
  pinentry info in the constructor and removed unneeded setter for
  pinentry info. Add save passphrase checkbox.

This patch adds functionality to save key passphrases with pinentry-qt that already exists in pinentry-gtk-2.

A "save passphrase" checkbox is shown when libsecret is available, the external password cache is enabled, and there is valid data in pinentry_t->keyinfo. When checked, the pinentry info is updated to allow the underlying implementation in pinentry/pinentry.c and pinentry/password-cache.c to cache the password using libsecret.

GnuPG-bug-id: T6801
Signed-off-by: Jason Carrete <jasoncarrete5@gmail.com>

Test Plan

Example sequence of commands to test this patch:

install -m700 -d /tmp/gnupg
export GNUPGHOME=/tmp/gnupg
gpg --gen-key
gpg --clearsign

During the clearsign operation, pinentry should provide an option to save the passphrase when prompting the user. Repeated attempts to sign a message should now be able to use libsecret to retrieve the passphrase instead of prompting the user with pinentry. Note that when generating a key or changing the password of a key through gpg, there is no "save passphrase" checkbox. It looks like this is because gpg-agent sends SETKEYINFO --clear to pinentry and so there is no keyinfo to determine which passphrase to use.

This patch can also be tested directly with pinentry as long as OPTION allow-external-password-cache and SETKEYINFO <mykeyinfo> are sent before prompting.

Diff Detail

Repository

rP Pinentry

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

jasonc requested review of this revision.Nov 8 2023, 11:21 PM

jasonc created this revision.

jasonc added a project: pinentry.Nov 9 2023, 2:11 AM

In general, the changes look good.

qt/main.cpp
216	Use `QStringLiteral` instead of `QLatin1String`. I saw that other code also uses `QLatin1String` but that's suboptimal. Additionally, I'd write something like "Save passphrase in password manager". "using libsecret" may be more correct on a technical level but most users won't understand what that means.
qt/pinentrydialog.cpp
311	Usually we listed to the signal `QCheckBox::toggled(bool checked)` (see mVisiCB). `stateChanged` makes sense if you need the tristate of the check box.
530	This is error prone if for some reason the checked state and the value of the flag get out of sync. Use `QCheckBox::isChecked()` to determine whether the user enabled saving or not. Or, even better, add `bool checked` as function parameter. Then you will receive the checked state directly from the signal and don't have to ask the check box yourself.

jasonc updated this revision to Diff 1544.Nov 10 2023, 5:00 AM

jasonc marked 3 inline comments as done.Nov 10 2023, 7:42 AM

Thanks for the update.

I just saw this in the API documentation:

If @code{may_cache_password} and @code{keyinfo} are set and the user
consents, then the @pinentry{} may cache the password with an external
manager. Note: getting the user's consent is essential, because
password managers often provide a different level of security. If the
above condition is true and @code{tried_password_cache} is false, then
a check box with the specified string should be displayed. The check
box must default to off.

Your code (and the code of the other pinentry variants) doesn't handle tried_password_cache. I have to admit that I don't understand why the checkbox should be hidden if tried_password_cache is true. As far as I can see, if tried_password_cache is true and the pinentry is displayed, then no password was stored in the password manager (or the password store didn't work, but then allow_external_password_cache is set to false which is handled by the pinentry apps). If no password was stored in the password manager then the user will surely want to be able to enable storing in the password manager. *shrug*
Unless somebody else (Werner? Niibe-san?) can explain why tried_password_cache should be handled as described in the API documentation I'm fine with your patch.