ssh-agent: set $SSH_AUTH_SOCK in non-interactive shells #8183
Conversation
|
@rhododendrox, @meenzen, @slotThe would you mind testing whether this PR fixes your issues? That would be appreciated because I do not have any issues myself! |
|
Just tested this, it works and kinda fixes my problem, but that's fine since I discovered the issues I had where partially caused by my config. Nevertheless I think this is a good change since it makes much more sense to export SSH_AUTH_SOCK in .profile instead of .bashrc PS: I only tested bash and zsh because those are the shells I use. |
Tested it with fish, and everything seems to work as before after I shuffled some bits: I start X11 from Thanks! |
bmrips
force-pushed
the
ssh-agent-fix
branch
from
c04213c to
a469b11
Compare
That's a good point! I added an |
|
Since I just also noticed the Problem, I have tried your fix (on fish). |
|
|
||
| enableBashIntegration = lib.hm.shell.mkBashIntegrationOption { inherit config; }; | ||
|
|
||
| enableZshIntegration = lib.hm.shell.mkZshIntegrationOption { inherit config; }; | ||
|
|
||
| enableFishIntegration = lib.hm.shell.mkFishIntegrationOption { inherit config; }; | ||
|
|
||
| enableNushellIntegration = lib.hm.shell.mkNushellIntegrationOption { inherit config; }; |
There was a problem hiding this comment.
Meh, IMO the options can stay, I can imagine that someone might want to disable it for some reason.
At the very least, this should make use of mkRemoveOptionModule if you do remove them.
There was a problem hiding this comment.
That's true. With the newest change, I left them intact.
modules/services/ssh-agent.nix
Outdated
| in | ||
| { | ||
| bash.initExtra = lib.mkIf cfg.enableBashIntegration bashIntegration; | ||
| bash.profileExtra = lib.mkBefore bashIntegration; |
There was a problem hiding this comment.
Why mkBefore?
Ah:
Tested it with fish, and everything seems to work as before after I shuffled some bits: I start X11 from
.profile, and by default the export ofSSH_AUTH_SOCKwas inserted after the call toxinit. Inserting alib.mkAfterin my own config fixed this, but this should probably be documented as a possible problem when upgrading.That's a good point! I added an
lib.mkBeforeto put the initialization code before other code such that these kind of issues should be fixed.
More of a downstream user issue than module issue IMO, though it is a simple change.
There was a problem hiding this comment.
Wouldn't say this is a downstream user issue, if the behaviour of defined options differ due to an upstream change. There is also no other option for code you want to always run. An alternative to mkBefore would be a new option which is like profileExtra + mkBefore.
There was a problem hiding this comment.
Why
mkBefore?
To preserve the behavior of the module as it was before. I also just changed the priority from 500 to 900 such that lib.mkBefore on the user side gets priority as you would expect.
Since PR nix-community#8099, the module sets `$SSH_AUTH_SOCK` through shells' options for interactive shell initialization instead of `home.sessionVariablesExtra`. The replacement was not faithful, however, since `home.sessionVariablesExtra` is sourced also in non-interactive shells. With this commit, the shells' profile options (where `home.sessionVariablesExtra` is sourced) are used to set `$SSH_AUTH_SOCK`. Fixes nix-community#8129.
5 participants
Description
This PR is a follow-up on #8099.
Since ssh-agent: add shell integrations #8099, the module sets
$SSH_AUTH_SOCKthrough shells' options for interactive shell initialization instead ofhome.sessionVariablesExtra. The replacement was not faithful, however, sincehome.sessionVariablesExtrais sourced also in non-interactive shells. With this PR, the shells' profile options (wherehome.sessionVariablesExtrais sourced) are used to set$SSH_AUTH_SOCK.ssh-agent: add shell integrations #8099 introduced shell integration options that guard the code for setting$SSH_AUTH_SOCK. To my understanding it is crucial for the ssh-agent that this env var is set, hence I do not see a reason to have these options. Consequently, this PR removes them.Note that this change is not backwards-compatible!Fixes #8129.
Checklist
Change is backwards compatible.
Code formatted with
nix fmtornix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.Code tested through
nix run .#tests -- test-allornix-shell --pure tests -A run.all.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.