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

openssh: use ssh-keysign from PATH #63585

Merged
merged 1 commit into from Aug 11, 2019

Conversation

@edef1c
Copy link
Member

commented Jun 20, 2019

Motivation for this change

ssh-keysign is used for host-based authentication, and is designed to be used
as SUID-root program. OpenSSH defaults to referencing it from libexec, which
cannot be made SUID in Nix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from edolstra Jun 20, 2019

@edef1c edef1c changed the base branch from master to staging Jun 20, 2019

@edef1c edef1c force-pushed the edef1c:openssh-keysign branch from 0d34697 to d63d1a2 Jun 20, 2019

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@edolstra

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

I'd prefer not adding setuid binaries unless it's absolutely necessary.

@alyssais

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Does this configure where the ssh client expects to find this binary on a remote server? Or is it where a server expects to find this binary on itself?

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

@alyssais

Does this configure where the ssh client expects to find this binary on a remote server? Or is it where a server expects to find this binary on itself?

It's where the client expects to find ssh-keysign on the local machine.

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

@edolstra

I'd prefer not adding setuid binaries unless it's absolutely necessary.

I'm quite purposely not enabling the setuid wrapper by default, and unless we're committing to "host-based authentication doesn't work on NixOS", it is indeed necessary. If we're committing to making functionality unusable for the sake of decreasing the SUID binary count, I'd rather start with gnome-keyring-daemon than OpenSSH, and review all the others.

It's also worth noting that even with the wrapper enabled, it is entirely inactive without EnableSSHKeysign yes in global ssh_config.

@edef1c edef1c force-pushed the edef1c:openssh-keysign branch from d63d1a2 to 098ac09 Jun 26, 2019

@edef1c edef1c requested review from alyssais and matthewbauer Jul 10, 2019

pkgs/tools/networking/openssh/default.nix Outdated Show resolved Hide resolved
@grahamc

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Could we instead pass this value in as an SSH configuration option, in the /etc/ssh/ssh_config file?

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

@grahamc

Could we instead pass this value in as an SSH configuration option, in the /etc/ssh/ssh_config file?

That would cause every unpatched OpenSSH to crash:

[root@platypus:/etc/ssh]# echo Foo bar >> ssh_config
[root@platypus:/etc/ssh]# ssh anywhere
/etc/ssh/ssh_config: line 15: Bad configuration option: foo
/etc/ssh/ssh_config: terminating, 1 bad configuration options
@alyssais

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

You can use IgnoreUnknown Foo, and then non-patched versions will just ignore it. (Not that I think it’s a good idea.)

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

You can use IgnoreUnknown Foo, and then non-patched versions will just ignore it. (Not that I think it’s a good idea.)

Okay, so I guess that's an option. There doesn't seem to be a sensible distro-independent default, anyhow. It lives at /usr/lib/openssh/ssh-keysign on Debian, /usr/lib/ssh/ssh-keysign on Arch, /usr/libexec/openssh/ssh-keysign on Fedora, etc.

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

If anything, I'm tending towards either "just take it from PATH" or "just take it from SSH client config" to quiet the arguments. I was reluctant to do either because the normal mechanism hardcodes a root-controlled purpose-named location, but as far as I can tell ssh-keysign doesn't really receive anything sensitive, certainly no more so than whoever controls SSH_AUTH_SOCK.
This doesn't actually make anything work on non-NixOS without user intervention either, but at least rids us of the /run/wrappers boogeyman. We can even {default,fall back} to the Nix store path as we do now and remain compatible with the running-as-root case without additional configuration, if anyone really wants to push that case.

@alyssais

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

openssh: use ssh-keysign from PATH
ssh-keysign is used for host-based authentication, and is designed to be used
as SUID-root program. OpenSSH defaults to referencing it from libexec, which
cannot be made SUID in Nix.

@edef1c edef1c force-pushed the edef1c:openssh-keysign branch from 098ac09 to 9fe1028 Jul 31, 2019

@edef1c edef1c changed the title openssh: use SUID ssh-keysign path openssh: use ssh-keysign from PATH Jul 31, 2019

@edef1c edef1c requested review from adisbladis and alyssais Jul 31, 2019

@edef1c

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

I'm expecting using PATH to be fairly uncontroversial, and will default to merging in a week (2019-08-10) if nobody objects.

@edef1c edef1c merged commit 068f46f into NixOS:staging Aug 11, 2019

16 checks passed

Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
openssh on aarch64-linux Success
Details
openssh on x86_64-darwin Success
Details
openssh on x86_64-linux Success
Details

@edef1c edef1c deleted the edef1c:openssh-keysign branch Aug 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.