Skip to content

Comments

nixos/gnupg: add systemd configuration#231108

Merged
rnhmjoj merged 4 commits intoNixOS:masterfrom
corngood:gpg-agent
Jun 26, 2023
Merged

nixos/gnupg: add systemd configuration#231108
rnhmjoj merged 4 commits intoNixOS:masterfrom
corngood:gpg-agent

Conversation

@corngood
Copy link
Contributor

This depended on the systemd user configuration provided upstream in doc/examples. However, this was all removed in:

gpg/gnupg@eae28f1

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.05 Release Notes (or backporting 22.11 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.

@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 May 10, 2023
@corngood
Copy link
Contributor Author

The use of systemd is deprecated because of additional complexity and the race between systemd based autolaunching and the explicit gnupg based and lockfile protected autolaunching.

I'm not sure this is something we should be worried about, but without this change gpg-agent doesn't start under systemd, and doesn't have pinentry set.

@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 May 10, 2023
@SuperSandro2000 SuperSandro2000 requested a review from rnhmjoj May 10, 2023 17:57
@rnhmjoj
Copy link
Contributor

rnhmjoj commented May 10, 2023

Yes, there is a race and I think the solution is for gnupg to just stop trying to manage the agent itself and let systemd (or any init system, for that matter) do its job. Unfortunately upstream went the other way.

This PR is a reasonable solution, however they officially dropped systemd and already hinted that --pinentry-program should not be used and may be removed. So, I would save ourselves some future troubles by configuring pinentry-program with /etc/gnupg/gpg.conf and let gnupg start the agent.

@corngood
Copy link
Contributor Author

corngood commented May 11, 2023 via email

@corngood
Copy link
Contributor Author

corngood commented May 12, 2023

I added a commit to fix the existing reference to nixosTests.gnupg. This test actually caught the problem by getting stuck on pinentry, and now passes.

I also got it passing tests without systemd, but I'm worried the lack of socket activation will cause regressions.

For example, SSH_AUTH_SOCK should allow ssh to start gpg-agent with socket activation, but I'm not sure if this actually works in practice. The test mentions:

          # Note: apparently this must be run before using the OpenSSH agent
          # socket for the first time in a tty. It's not needed for `ssh`
          # because there's a hook that calls it automatically (only in NixOS).
          machine.send_chars("gpg-connect-agent updatestartuptty /bye\n")

@corngood corngood mentioned this pull request Jun 14, 2023
12 tasks
@marsam marsam mentioned this pull request Jun 14, 2023
@corngood
Copy link
Contributor Author

I think my recommendation would be to get this change in while we consider the consequences of dropping the systemd units. Then it'll be in the history in case people want to revert to it in the future, and we may even want to support both methods.

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.

As I already said, I'm ok with keeping the unit files provided pinentry-program is set from /etc/gnupg/gpg.conf, not the command line because that is too brittle.

@corngood
Copy link
Contributor Author

@rnhmjoj sorry, I misunderstood. I'll make that change.

@corngood
Copy link
Contributor Author

Updated with:

  • moved pinentry-program to /etc/gnupg/gpg-agent.conf
  • removed the doc/examples stuff from gnupg

The later is needed for gnupg < 2.4.1, and it's been reverted to 2.4.0 in master. Perhaps this should go in staging?

@ofborg ofborg bot requested review from fpletz and vrthra June 15, 2023 15:23
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. and removed 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 15, 2023
@corngood corngood requested a review from rnhmjoj June 16, 2023 01:45
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 16, 2023

Looks good. You can probably remove this line:

    systemd.packages = [ cfg.package ];

as we don't rely on (potential, at this point) upstream units anymore.

This depended on the systemd user configuration provided upstream in
doc/examples.  However, this was all removed in:

gpg/gnupg@eae28f1
@corngood
Copy link
Contributor Author

@rnhmjoj Good catch, now we can leave:

4bc7ddc

until we upgrade gpg again, which will reduce the number of rebuilds here drastically.

Updated with that change.

@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. and removed 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Jun 16, 2023
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.

I checked the definitions against the previously supplied units and it seems ok.
The test is passing, so let's not delay the update any longer and merge this now.

@rnhmjoj rnhmjoj merged commit 547cd96 into NixOS:master Jun 26, 2023
@corngood
Copy link
Contributor Author

I updated #231110 to include merging this into staging, and reverting the 2.4.1 rollback.

I'm not sure if that's the right approach to take, but if so, it's there.

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: 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