Replace gnome-keyring ssh-agent by gcr and implement NixOS module #379731
Conversation
This builds `gcr_4` with ssh-agent, in order to move away from `gnome-keyring-daemon`, which was [deprecated in 1.46.0](https://gitlab.gnome.org/GNOME/gnome-keyring/-/commit/25c5a1982467802fa12c6852b03c57924553ba73). Co-authored-by: lilyinstarlight <[email protected]>
github-actions
bot
added
6.topic: nixos
nezia1
force-pushed
the
replace-gnome-keyring-with-gcr
branch
from
120fd55 to
46bbbac
Compare
nix-owners
bot
requested review from
hedning,
jtojnar,
dasj19 and
bobby285271
|
| systemd.packages = [ pkgs.gcr_4 ]; | ||
|
|
||
| environment.extraInit = '' | ||
| if [ -z "$SSH_AUTH_SOCK" -a -n "$XDG_RUNTIME_DIR" ]; then |
There was a problem hiding this comment.
I would expect this to be handled by gcr-ssh-agent.socket.
There was a problem hiding this comment.
I have been told that ExecStartPost=-/usr/bin/systemctl --user set-environment SSH_AUTH_SOCK=%t/gcr/ssh might not set it for the rest of the session in most non-gnome setups, but I might be wrong. This is why I did it like this. I'd be welcome to remove if it does though
There was a problem hiding this comment.
I would expect it to work in other DEs as well, as long as the default.target (which gcr-ssh-agent.service is WantedBy) is executed before the session.
Or does #81138 apply? (I do not recall, it might have only been an issue for system services.)
Either way, the reason including examples of affected DEs should be documented in a comment.
There was a problem hiding this comment.
I can test this on my machine and see what happens if I remove the extraInit part. I'll let you know how it goes. I'm also on Hyprland so this can additionally be tested with a non-GNOME environment
There was a problem hiding this comment.
Nevermind. I did test my current PR with simply commenting the extraInit part , and the variable does get set correctly in Hyprland on login
There was a problem hiding this comment.
However, after tests with another other DE/session manager (Cosmic), it did not seem to work because Cosmic's session manager does not pull environment variables from systemd. So we do need that extraInit line.
That sounds to me like a bug in Cosmic packaging then.
There was a problem hiding this comment.
Well, desktop environment/session managers don't have to source these, so I think it would make sense if we at least added a condition for it. To be fair, they don't have to source /etc/profile either, so that's why I think having both makes sense here.
It would allow for full coverage, even if the display manager or DE is not implementing one or the other.
EDIT: Unless this is also a freedesktop I'm unfamiliar with (having to pull systemd user variables), in which case it would make sense to only keep the "ExecStartPost".
There was a problem hiding this comment.
There should be no need for any sourcing – systemd should start default.target, which should start this service and then the DE, the latter should, as a child process, automatically inherit the environment.
If some DE needs this hack, it should go to its module. But preferably, we should find out what is wrong in its activation sequence and fix that.
There was a problem hiding this comment.
That sounds to me like a bug in Cosmic packaging then.
This does not have to do with COSMIC or COSMIC packaging. COSMIC actually does import systemd user manager env specifically to solve issues like this (iirc similarly to KDE Plasma) and using the standard COSMIC launcher does not exhibit any issue when SSH_AUTH_SOCK is set in the systemd user manager but not in the wider user session
systemd should start default.target, which should start this service and then the DE, the latter should, as a child process, automatically inherit the environment
Most compositors/DEs do not run as systemd units (though some launch applications, for example a terminal emulator, via systemd units -- that is not universal though)
we should find out what is wrong in its activation sequence and fix that
As far as I can tell, there is nothing wrong with the activation sequence. The environment.extraInit is just applying the variable to the entire user session instead of just to spawned processes of systemd user manager
Is ensuring the user session as a whole has the variable set (in parallel to the .socket unit setting it in the user manager) causing issues?
There was a problem hiding this comment.
I would also like to point out that, having tested running Hyprland without the use of a systemd wrapper (and having my session variables set in environment.d), all of the variables set there were empty in my terminal emulator (spawned with the compositor itself).
I don't think we can ensure that this will be set on every DE, and even less more barebones compositors, considering most of them don't spawn applications using systemd. This is where the /etc/profile variable sourcing would come in handy, and allow for full coverage of environments that do not import user environment.
This removes ssh-agent from `gnome-keyring`, which was [deprecated in 1.46.0](https://gitlab.gnome.org/GNOME/gnome-keyring/-/commit/25c5a1982467802fa12c6852b03c57924553ba73) Co-authored-by: lilyinstarlight <[email protected]>
nezia1
force-pushed
the
replace-gnome-keyring-with-gcr
branch
3 times, most recently
from
c215371 to
89abb80
Compare
|
|
||
| config = lib.mkIf cfg.enable { | ||
| systemd.packages = [ pkgs.gcr_4 ]; | ||
| systemd.user.sockets.gcr-ssh-agent.wantedBy = [ "sockets.target" ]; |
There was a problem hiding this comment.
Also, systemd.user.sockets.gcr-ssh-agent.wantedBy = [ "sockets.target" ] was missing in the original service definition. I'll add it too
Why is that needed? Reading https://www.freedesktop.org/software/systemd/man/latest/systemd.special.html#sockets.target, I do not think it applies:
- It cannot start after boot since it is a user service.
- It cannot rely on socket activation because by the time something could do it, we would already need to have a session the something could run in, and then it would be too late for the environment to be updated. That’s why the service is
WantedBy=default.target, I believe.
There was a problem hiding this comment.
That does make sense. I'll remove it, I have just seen it on other socket services, but I did not read the freedesktop documentation for it.
There was a problem hiding this comment.
It cannot start after boot since it is a user service.
This code is setting the socket to be enabled for the user manager by default, it has nothing to do with boot
It cannot rely on socket activation because by the time something could do it, we would already need to have a session the something could run in, and then it would be too late for the environment to be updated.
It relies on socket activation just fine. If you only enable the service and not the socket, then you don't get environment updating at all since that is only done as an ExecStartPost on the .socket unit (though I have seen noise upstream about stopping setting it from systemd units by default and use some other mechanism)
Co-authored-by: lilyinstarlight <[email protected]>
nezia1
force-pushed
the
replace-gnome-keyring-with-gcr
branch
from
89abb80 to
da3186a
Compare
This PR effectively deprecates
gnome-keyring, in favor ofgcr, that has been deprecated since version 1.46.0. A new option has been added underservices.gnome.gcr-ssh-agent.enablein order to set it up. Fixes #140824gcr_4has been updated to build with the ssh agentgnome-keyringhas been updated to completely remove the now deprecated ssh componentservices.gnome.gcr-ssh-agent.enable, which effectively sets up systemd services andSSH_AUTH_SOCKSince this PR adds a GNOME module, I also want to make sure the GNOME team approves of it.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.