Page MenuHome GnuPG

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

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

Details

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 created this revision.

In general, the changes look good.

qt/main.cpp
217

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
312

Usually we listed to the signal QCheckBox::toggled(bool checked) (see mVisiCB). stateChanged makes sense if you need the tristate of the check box.

531

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.

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.

I think that tried_password_cache in the documentation is wrong. The text:

and @code{tried_password_cache} is false

should be removed.

So, go ahead with the patch.

ikloecker mentioned this in Unknown Object (Event).Nov 13 2023, 9:10 AM
TobiasFella added a subscriber: TobiasFella.

Thanks for the contribution, sorry this took us a while to get to. I've merged your changes to the pinentry repo now.

This revision is now accepted and ready to land.Jan 10 2024, 12:16 PM
TobiasFella mentioned this in Unknown Object (Event).Jan 15 2024, 8:59 AM