Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssh: Pass the user-specified ssh options last #8303

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thufschmitt
Copy link
Member

Motivation

When NIX_SSHOPTS is set, append it to the generate it ssh command-line rather than adding it in the middle. This allows doing nasty things like prepending anything to the command than Nix runs, and provides a workaround for #1078.

Context

Fix #8292

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Théophane Hufschmitt added 2 commits May 8, 2023 09:42
When `NIX_SSHOPTS` is set, append it to the generate it `ssh`
command-line rather than adding it in the middle. This allows doing
nasty things like prepending anything to the command than Nix runs, and
provides a workaround for NixOS#1078
@thufschmitt thufschmitt requested a review from edolstra as a code owner May 8, 2023 07:57
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 8, 2023
@edolstra
Copy link
Member

This seems like a really nasty hack. If I came across this, I would assume it was a (shell injection) bug.

Wouldn't it be better to add a setting for prepending stuff to the remote command?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-48/28102/1

@thufschmitt
Copy link
Member Author

Wouldn't it be better to add a setting for prepending stuff to the remote command?

Wouldn't that just add one more setting for something that's not much less of a hack?

@quentinmit
Copy link

Wouldn't it be better to add a setting for prepending stuff to the remote command?

Wouldn't that just add one more setting for something that's not much less of a hack?

A better argument is that this applies to ssh options, too, since later options override earlier options. Without fixing the order of the arguments, users can't override any of the options nix itself is trying to set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting PATH in NIX_SSHOPTS no longer works!
4 participants