Skip to content

Comments

nixos/gnupg: fix gpg-agent when pinentryFlavor is null#240708

Merged
rnhmjoj merged 2 commits intoNixOS:masterfrom
corngood:gpg-pinentry-fix
Jun 30, 2023
Merged

nixos/gnupg: fix gpg-agent when pinentryFlavor is null#240708
rnhmjoj merged 2 commits intoNixOS:masterfrom
corngood:gpg-pinentry-fix

Conversation

@corngood
Copy link
Contributor

@corngood corngood commented Jun 30, 2023

Problem reported here: doronbehar@8ea6449#r120230629

Introduced by #231108

8ea6449 moved the configuration outside the pinentryFlavor check, causing evaluation to fail when it was set to null.

960a514 removed the upstream systemd units, causing gpg-agent.service to be conditional on pinentryFlavor.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

8ea6449 moved the configuration outside
the pinentryFlavor check, causing evaluation to fail when it was set to
null.

960a514 removed the upstream systemd
units, causing gpg-agent.service to be conditional on pinentryFlavor.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 30, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 30, 2023
@corngood corngood assigned rnhmjoj and unassigned rnhmjoj Jun 30, 2023
@corngood corngood requested a review from rnhmjoj June 30, 2023 11:41
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

LGTM, though I haven't tested.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 30, 2023

@GrahamcOfBorg test gnupg

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 30, 2023

I'm not sure what is the point of pinentryFlavor = null. The option default is already picking an appropriate package depending on the DE. Can't we just make the option non-nullable?

@doronbehar
Copy link
Contributor

I'm not sure what is the point of pinentryFlavor = null. The option default is already picking an appropriate package depending on the DE. Can't we just make the option non-nullable?

I set pinentry-program in my user's gnupg.conf to a script in my ~/.bin/ that I manage non decoratively. Hence I don't want /etc/gnupg/gnupg.conf to set it to anything, though I still want to use the systemd services NixOS provides.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 30, 2023

Hence I don't want /etc/gnupg/gnupg.conf to set it to anything, though I still want to use the systemd services NixOS provides.

But it shouldn't be a problem: your local gnupg.conf will override the global configuration.

@corngood
Copy link
Contributor Author

I added a commit that fixes the documentation for pinentryFlavor.

The old docs said:

If not null, the path to the pinentry binary will be passed to gpg-agent via commandline and thus overrides the pinentry option in gpg-agent.conf in the user's home directory.

You don't need to set it to null any more for your user config to take precedence.

However, I think it's still reasonable to be able to remove the config entry by passing null.

@doronbehar
Copy link
Contributor

Hence I don't want /etc/gnupg/gnupg.conf to set it to anything, though I still want to use the systemd services NixOS provides.

But it shouldn't be a problem: your local gnupg.conf will override the global configuration.

Still, that will create a reference to a pinentry-program that I don't necessarily want to have in my configuration.

@corngood
Copy link
Contributor Author

This does point out that there's been a behaviour change. I think it's a good one (i.e. user config taking precedence), but I should probably put something in the release notes.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 30, 2023

via commandline and thus overrides the pinentry option in gpg-agent.conf in the user's home directory.

Oh, I see.

You don't need to set it to null any more for your user config to take precedence.
However, I think it's still reasonable to be able to remove the config entry by passing null.

Personally, I would simplify the implementation by removing null. A pinentry program must be set anyway and the option wasn't super clear about this. If you don't care about what the default is, it's fine, if you care because you want to provide your one manually, just do so in your home and it's still fine.

Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Choose a reason for hiding this comment

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

Ok, let's leave it as is.

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes labels Jun 30, 2023
@corngood
Copy link
Contributor Author

Updated with release note.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 30, 2023

@GrahamcOfBorg test gnupg

@rnhmjoj rnhmjoj merged commit 6e1462f into NixOS:master Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants