rbw/gpg-agent: improve `pinentry` options by ambroisie · Pull Request #4895 · nix-community/home-manager

ambroisie · 2024-01-14T17:16:01Z

Description

This is mostly a follow-up to #4805.

Checklist

Change is backwards compatible.
Code formatted with ./format.
Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.
Test cases updated/added. See example.
Commit messages are formatted like
```
{component}: {description}

{long description}
```
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
- Added myself as module maintainer. See example.

Maintainer CC

@rycee

ambroisie · 2024-01-14T17:25:43Z

I tried running all tests, but got an FOD error in immutable-keyfiles.nix...

Details

building '/nix/store/0pka8pqyqk6k8i43112423y8hxsm0bd5-activation-script.drv'...
building '/nix/store/35wnsrj3m2npjdwvxf6dga0li3f916bc-home-manager-generation.drv'...
building '/nix/store/dpwjima15fr4k9kanlkn8sl82xa82gmd-nmt-report-gpg-agent-override-homedir.drv'...
building '/nix/store/y67jnqdl22qf4vji9yjw0r6q3i1f127c-home-manager-path.drv'...
building '/nix/store/kiddsys4690v680mhnvlx4zpgz50bb02-lookup?op=get-options=mr-search=0x36cacf52d098cc0e78fb0cb13573356c25c424d4.drv'...
building '/nix/store/28v6fwhzdaa4kc5nv5g92bh3nxk1fscm-pubkey.txt.drv'...
created 299 symlinks in user environment

trying https://keys.openpgp.org/pks/lookup?op=get&options=mr&search=0x36cacf52d098cc0e78fb0cb13573356c25c424d4
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

trying https://www.rsync.net/resources/pubkey.txt
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1855  100  1855    0     0   1440      0  0:00:01  0:00:01 --:--:--  1441
100 13209  100 13209    0     0   8088      0  0:00:01  0:00:01 --:--:--  8083
error: hash mismatch in fixed-output derivation '/nix/store/kiddsys4690v680mhnvlx4zpgz50bb02-lookup?op=get-options=mr-search=0x36cacf52d
098cc0e78fb0cb13573356c25c424d4.drv':
         specified: sha256-9Zjsb/TtOyiPzMO/Jg3CtJwSxuw7QmX0pcfZT2/1w5E=
            got:    sha256-Pvm0CpH13pvnD/c+IJAkRElR5/IhZt8pg7RjT/dZdfM=
error: 1 dependencies of derivation '/nix/store/07qfnkm1l1zz8519x3y824a5jibkjgbg-gpg-pubring.drv' failed to build
error: 1 dependencies of derivation '/nix/store/n7ghchmzg9k0hx6db9dfvgs1lbqj9qva-activation-script.drv' failed to build
error: 1 dependencies of derivation '/nix/store/4px1ar29q9dghisziqcncc1xw6as3nm4-home-manager-generation.drv' failed to build
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'nmt-run-all-tests'
         whose name attribute is located at /nix/store/lrmxkfadhlcrakza60w0mc8sc327a5qg-source/pkgs/stdenv/generic/make-derivation.nix:348:7

       … while evaluating attribute 'shellHook' of derivation 'nmt-run-all-tests'

         at /nix/store/3vax7vf054q4vvb1bj8sd9lvr8dcsfjb-source/default.nix:38:40:

           37|   runShellOnlyCommand = name: shellHook:
           38|     pkgs.runCommandLocal name { inherit shellHook; } ''
             |                                        ^
           39|       echo This derivation is only useful when run through nix-shell.

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: 1 dependencies of derivation '/nix/store/28rshsv810bbm5lr3nzjlvi9c7alcr92-nmt-report-gpg-immutable-keyfiles.drv' failed to build

modules/programs/rbw.nix

modules/services/gpg-agent.nix

ambroisie

The fixup commit should be rebases before merging.

EDIT: done.

fpletz · 2024-01-15T05:54:02Z

This PR makes the pinentry mess even more uglier. IMHO we should only support flavor strings as pinentryFlavor and pick the correct package via pkgs.pinentry-${pinentryFlavor} like in https://github.com/NixOS/nixpkgs/pull/277221/files#diff-9aace735eba8386316153e3fd2ba7c726a1f6ccfc37a981c0efcc173ed2481f3R90.

If we wanted to support pinentry packages a new option like pinentryPackage would make more sense instead of making this one option more complicated to use than it should be. We should rather move away from multiple outputs anyway.

ambroisie · 2024-01-15T09:29:16Z

I think it's essential to support packages, not just flavors -- for one, I would rather use pinentry-rofi than any of the specific flavors made available by the package. It also sounds like a more modular way of specifying the option.

Rather than have two competing pinentry* options, I think we could deprecate pinentryFlavor (in gpg-agent) and introduce a pinentry option which is a mkPackageOption { null = true; }.
For rbw, we could add a warning if the user's option is using a string "flavor" to introduce the deprecation.

ambroisie · 2024-01-23T14:58:11Z

Switching this to a draft until NixOS/nixpkgs#133542 is merged.

ambroisie · 2024-03-10T10:31:06Z

@fpletz now that the upstream changes have been merged, I updated this PR.

modules/services/gpg-agent.nix

modules/misc/news.nix

modules/programs/rbw.nix

modules/misc/news.nix

rycee · 2024-03-12T19:31:35Z

Thanks! I just added a few minor final comments. You will also have to run the ./format script in the project root to make the CI happy.

Perhaps this PR could be marked as ready to review?

ambroisie · 2024-03-12T22:31:11Z

Urgh, so much back and forth, sorry about that @rycee.

Should be good now though.

ambroisie · 2024-03-13T10:18:37Z

modules/programs/rbw.nix

          email = "name@example.com";
          lock_timeout = 300;
-          pinentry = "gnome3";
+          pinentry = pkgs.pinentry-gnome;


Yes but the current version of nixpkgs used by Home-manager uses pinentry-gnome...

This is just an example...

This follows upstream's module change [1], which allows setting any package as a pinentry program. [1]: NixOS/nixpkgs#133542

Following some upstream changes [1], it's now possible to use a simplified package type for the option. [1]: NixOS/nixpkgs#133542

rycee · 2024-03-14T08:17:45Z

Thanks! Merged to master now 🙂 Note, I switched back to pinentry-gnome3 since it seems to be what is in Nixpkgs right now. I'm a bit confused, though, since I'm pretty sure I saw pinentry-gnome for a while.

yshui · 2024-03-14T12:02:19Z

modules/services/gpg-agent.nix

-          ++ optional (cfg.pinentryFlavor != null)
-          "pinentry-program ${pkgs.pinentry.${cfg.pinentryFlavor}}/bin/pinentry"
+          ++ optional (cfg.pinentryPackage != null)
+          "pinentry-program ${lib.getExe pinentryPackage}"


shouldn't this be cfg.pinentryPackage?

musjj · 2024-03-14T12:03:11Z

modules/services/gpg-agent.nix

-          ++ optional (cfg.pinentryFlavor != null)
-          "pinentry-program ${pkgs.pinentry.${cfg.pinentryFlavor}}/bin/pinentry"
+          ++ optional (cfg.pinentryPackage != null)
+          "pinentry-program ${lib.getExe pinentryPackage}"


I'm late, but this should be:

- "pinentry-program ${lib.getExe pinentryPackage}" + - "pinentry-program ${lib.getExe cfg.pinentryPackage}"

Getting this error:

┃ error: undefined variable 'pinentryPackage' ┃ ┃ at /nix/store/kww2vp5mhnvb0mczv2xll0m5qsrjrw9q-source/modules/services/gpg-agent.nix:251:42: ┃ ┃ 250| ++ optional (cfg.pinentryPackage != null) ┃ 251| "pinentry-program ${lib.getExe pinentryPackage}" ┃ | ^ ┃ 252| ++ [ cfg.extraConfig ]);

Woops, review sniped :)

colemickens · 2024-03-14T16:32:43Z

I don't understand the choice to specify package here rather than binary. My pinentry of choice does not provide it's binary as the default exe, nor is it named pinentry.

ambroisie · 2024-03-14T17:02:08Z

@colemickens you can just overrideAttrs (_: { meta = meta // { mainProgram = "..."; }; }) IIRC for that.

danjl1100 · 2024-06-01T19:06:37Z

I didn't see the removal of pinentryFlavor mentioned in the 24.05 release notes. This is mentioned in news.nix for the PR... is there some easy way to find that when upgrading home-manager? (e.g. 23.11 -> 24.05)

Is the required change in home-manager configs as simple as this?

 # example for `pinentry-curses`
-services.gpg-agent.pinentryFlavor = "curses";
+services.gpg-agent.pinentryPackage = pkgs.pinentry-curses;

ambroisie marked this pull request as ready for review January 14, 2024 17:25

ambroisie mentioned this pull request Jan 14, 2024

pinentry-bemenu/pinentry-rofi/pinentry_mac: add 'meta.mainProgram' NixOS/nixpkgs#280982

Merged

13 tasks

ambroisie commented Jan 14, 2024

View reviewed changes

modules/programs/rbw.nix Outdated Show resolved Hide resolved

modules/services/gpg-agent.nix Outdated Show resolved Hide resolved

ambroisie commented Jan 14, 2024

View reviewed changes

ambroisie force-pushed the improve-pinentry-options branch from 7f4cc3b to fd8153f Compare January 14, 2024 17:53

ambroisie marked this pull request as draft January 23, 2024 14:58

ambroisie force-pushed the improve-pinentry-options branch from fd8153f to 7c9b4f4 Compare March 10, 2024 10:30

ambroisie force-pushed the improve-pinentry-options branch 2 times, most recently from 5513c26 to 4000687 Compare March 10, 2024 14:05

ambroisie commented Mar 11, 2024

View reviewed changes

modules/services/gpg-agent.nix Outdated Show resolved Hide resolved

ambroisie commented Mar 11, 2024

View reviewed changes

modules/services/gpg-agent.nix Outdated Show resolved Hide resolved

rycee reviewed Mar 11, 2024

View reviewed changes

modules/misc/news.nix Outdated Show resolved Hide resolved

rycee reviewed Mar 11, 2024

View reviewed changes

modules/programs/rbw.nix Show resolved Hide resolved

ambroisie force-pushed the improve-pinentry-options branch 2 times, most recently from 422648c to d04ae8c Compare March 12, 2024 17:07

rycee reviewed Mar 12, 2024

View reviewed changes

modules/programs/rbw.nix Outdated Show resolved Hide resolved

rycee reviewed Mar 12, 2024

View reviewed changes

modules/misc/news.nix Show resolved Hide resolved

rycee reviewed Mar 12, 2024

View reviewed changes

modules/misc/news.nix Show resolved Hide resolved

ambroisie force-pushed the improve-pinentry-options branch from d04ae8c to 788fcc3 Compare March 12, 2024 22:28

ambroisie marked this pull request as ready for review March 12, 2024 22:28

ambroisie force-pushed the improve-pinentry-options branch from 788fcc3 to 1d24e19 Compare March 12, 2024 22:30

Stzx reviewed Mar 13, 2024

View reviewed changes

getchoo mentioned this pull request Mar 14, 2024

flake: update nixpkgs getchoo/borealis#318

Merged

ambroisie added 2 commits March 14, 2024 08:22

gpg-agent: migrate to 'pinentryPackage'

01e4a51

This follows upstream's module change [1], which allows setting any package as a pinentry program. [1]: NixOS/nixpkgs#133542

rbw: simplify 'pinentry' type

1ab3cec

Following some upstream changes [1], it's now possible to use a simplified package type for the option. [1]: NixOS/nixpkgs#133542

rycee force-pushed the improve-pinentry-options branch from 1d24e19 to 1ab3cec Compare March 14, 2024 07:30

rycee merged commit 1ab3cec into nix-community:master Mar 14, 2024

rycee mentioned this pull request Mar 14, 2024

bug: pinentry-related modules broken #5135

Closed

2 tasks

yshui reviewed Mar 14, 2024

View reviewed changes

musjj reviewed Mar 14, 2024

View reviewed changes

TomaSajt mentioned this pull request Mar 21, 2024

bug: gpg-agent using removed "gtk2" pinentry flavor as default #4804

Closed

2 tasks

Uh oh!

Conversation

ambroisie commented Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Maintainer CC

Uh oh!

ambroisie commented Jan 14, 2024

Uh oh!

Uh oh!

Uh oh!

ambroisie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpletz commented Jan 15, 2024

Uh oh!

ambroisie commented Jan 15, 2024

Uh oh!

ambroisie commented Jan 23, 2024

Uh oh!

ambroisie commented Mar 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rycee commented Mar 12, 2024

Uh oh!

ambroisie commented Mar 12, 2024

Uh oh!

This comment was marked as resolved.

Uh oh!

ambroisie Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

ambroisie Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

rycee commented Mar 14, 2024

Uh oh!

yshui Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

musjj Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

musjj Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

colemickens commented Mar 14, 2024

Uh oh!

ambroisie commented Mar 14, 2024

Uh oh!

danjl1100 commented Jun 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ambroisie commented Jan 14, 2024 •

edited

Loading

ambroisie left a comment •

edited

Loading