fix: support to other privilege elevation programs.

[henriquekirchheck]

This pr adds support for both doas and pkexec, while keeping support for sudo.

For this to work, I decided that the cleanest way to implement this was by adding a extra parameter to the Command struct that specifies if the command should be ran as root or not. When the root parameter is set, it checks for the existence of doas, sudo, and pkexec, in that order, and uses it to run the command by prefixing the command invocation arguments with the respective program when it is constructed.

The specified order was chosen because I believe that if someone has doas installed in their system, it's more likely that they use it as their main privilege escalation program. ´pkexec´ was added more as a fallback in case someone doesn't have doas or sudo.

This was tested in my NixOS Unstable system with both sudo and doas, but I haven't tested pkexec personally.

[henriquekirchheck]

Forgot to mention that it will Fix #59, but it still uses sudo argv[..]

[Makesesama]

I also tested on NixOs Unstable with doas installed. Works fine for me.

edit: Ok i also tried to clean, which still doesnt work with doas and this pr. Cleaning calls self_elevate which still uses sudo.

[henriquekirchheck]

I'll look into the clean command and fix it

[henriquekirchheck]

Seems like the self elevate function is only using sudo but I should be able to just replace it with my get_elevation_program function.

@Makesesama If you could test it I would be very grateful.

[Makesesama]

Seems to work now

[henriquekirchheck]

Thank you!

[arthsmn]

You might also be interested in adding run0 support.

Related post: https://mastodon.social/@pid_eins/112353324518585654

[henriquekirchheck]

systemd is currently at version 255.4 in NixOS unstable so I'll refrain to add it until it is actually released, but I'll look into it when it is available

[boomshroom]

An alternative is to allow for some way for the user to specify what privilege escalator to use. This would be easier if nh had a config file though.

This would allow for forcing pkexec even if sudo is present, or using run0 (or its predecessor systemd-run) without needing to update nh to support it. And of course it would be ideal in such a scenario to be able to include extra arguments in case the user had particular needs (would be necessary for systemd-run since it doesn't pass through stdio without either -t or -P).

[deviantsemicolon]

Now that systemd v256 is out and currently being reviewed for eventual merging into nixpkgs, will this be updated to add support for run0

[henriquekirchheck]

I'll update everything with the suggestions, thank you!

[henriquekirchheck]

I guess I could add run0 to the list of automatically checked programs, because even if it isn't released yet in nixpkgs unstable it should be fine for people that don't have it because it will just be ignored by the checks.

Now I'm not sure where in that list it should be included, considering that it will be installed in every systemd based system, I think it should be low in the list, maybe above pkexec. I'm open for second opinions.

Also, it should probably check for a environment variable to determine what to do for convenience, just like nh already does with the FLAKE environment variable, but I don't know of any standard environment variable names that specify that.

[henriquekirchheck]

@boomshroom

And of course it would be ideal in such a scenario to be able to include extra arguments in case the user had particular needs (would be necessary for systemd-run since it doesn't pass through stdio without either -t or -P).

Any ideas on how that would look like in the command line? I added a way to specify a custom privilege elevation binary to use, but I cannot think of a way to add the extra command line arguments. Maybe another flag would work, but that seems messy to me.

A config file would be optimal, both for the FLAKE environment variable and the custom privilege elevation program, but I'd probably wait for @viperML to comment on what he wants to do.

[deviantsemicolon]

Systemd 256 was merged into staging-next and should be in master and unstable very soon. I would appreciate run0 support

[henriquekirchheck]

@deviantsemicolon run0 support was already added! Testing would be appreciated.

[deviantsemicolon]

@deviantsemicolon run0 support was already added! Testing would be appreciated.

If the nixpkgs maintainers ever get around to merging the next staging-next thing, where the systemd 256 PR is, into master, then I might be able to see what I can do

[deviantsemicolon]

@henriquekirchheck i disabled sudo and ran nix run to test your code. I got an error regarding setting a locale and it asked me to reauthenticate a few times, but that could be a nix run issue. not sure though. Other than that, it did use run0 as expected, so LGTM

the error basically said it couldn't set a locale and fell back to a standard one called "C"

[henriquekirchheck]

@deviantsemicolon this issue is normally related to localization, and considering my code is basically just calling a different program it doesn't feel related. I'm not currently able to test it myself, but I'll try to do it asap.

[calebstewart]

I'm also getting the localization warnings mentioned above. My best guess is it's related to the environment differences w/ run0 which exposes little to nothing of the calling environment to the subprocess. To be a bit more clear, I'll add the exact warnings I'm getting below. They are warnings from Perl, but they don't appear to be causing any actual problems. The build and install appears to have worked properly.

The only real issue I see is that w/ run0, there is no cached authentication, so you are prompted 3 times during a run of nh os switch with run0. :/

❯ nh os switch -e run0
> Building NixOS configuration
warning: Git tree '/home/caleb/git/nix' is dirty
warning: input 'nixos-hardware' has an override for a non-existent input 'nixpkgs'
Finished at 12:37:43 after 0s
> Comparing changes
<<< /run/current-system
>>> /tmp/nh-os-nuyqlh/result
No version or selection state changes.
Closure size: 2610 -> 2610 (0 paths added, 0 paths removed, delta +0, disk usage +0B).
> Activating configuration
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
activating the configuration...
setting up /etc...
reloading user units for caleb...
restarting sysinit-reactivation.target
> Adding configuration to bootloader
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").

On the positive side, the automatic coloring of output from the elevated subprocesses from run0 is pretty nifty. It's really easy to see what logs are from elevated sections.

Update

Yeah, run0 does not pass LOCALE_ARCHIVE through to the elevated process by default, but does pass LANG. Not strictly an nh problem necessarily, but could be solved by adding a way to pass arguments to the run0 command (e.g. adding --setenv=LOCALE_ARCHIVE).

I've verified this by writing a quick script:

#!/bin/sh

exec run0 --setenv=LOCALE_ARCHIVE $@

And then using that as the elevation provider:

❯ nh os switch -e "$(pwd)/test.sh"
> Building NixOS configuration
warning: Git tree '/home/caleb/git/nix' is dirty
warning: input 'nixos-hardware' has an override for a non-existent input 'nixpkgs'
Finished at 13:05:07 after 0s
> Comparing changes
<<< /run/current-system
>>> /tmp/nh-os-zOvtIc/result
No version or selection state changes.
Closure size: 2610 -> 2610 (0 paths added, 0 paths removed, delta +0, disk usage +0B).
> Activating configuration
activating the configuration...
setting up /etc...
reloading user units for caleb...
restarting sysinit-reactivation.target
the following new units were started: libvirtd.service
> Adding configuration to bootloader

Update 2

Installing the following in Home Manager home.packages will take precedence over the system-wide run0 binary in the path, and allow you to globally add arguments to run0. So, this works for me since LOCALE_ARCHIVE seems like a sane thing to add to the environment.

run0Wrapper = pkgs.writeShellScriptBin "run0" ''
  exec ${pkgs.systemd}/bin/run0 --setenv=LOCALE_ARCHIVE $@
'';

[viperML]

You can use any elevation program with run0 nh os switch -R, and a cleaner abstraction of --use-remote-sudo will be implemented in #107

[deviantsemicolon]

You can use any elevation program with run0 nh os switch -R, and a cleaner abstraction of --use-remote-sudo will be implemented in #107

I would like the ability to set a default in settings so I don't need to type run0 and -R everytime.

It also does not work on my system, and outputs an error saying it can't find the NixOS configuration, even when specifying the directory my flake is in.

[viperML]

Please send the actual log with --verbose

[deviantsemicolon]

Please send the actual log with --verbose

TRACE nh::logging:86: Logging OK
DEBUG nh:22: args=NHParser { verbose: true, command: Os(OsArgs { action: Switch(OsRebuildArgs { common: CommonRebuildArgs { dry: false, ask: false, flakeref: FlakeRef("./flake.nix"), update: false, no_nom: false, diff_provider: "nvd diff", out_link: None }, hostname: None, specialisation: None, no_specialisation: false, extra_args: [], bypass_root_check: true }) }) }
DEBUG nh:23: NH_VERSION=3.5.26
! Bypassing root check, now running nix as root @ src/nixos.rs:32
DEBUG nh::nixos:58: out_path=("/tmp/nh-osBMrumc/result", TempDir { path: "/tmp/nh-osBMrumc" })
> Building NixOS configuration
DEBUG nh::commands:136: cmd=Pipeline { nix build './flake.nix#nixosConfigurations.""nsa-laptop"".config.system.build.toplevel' --log-format internal-json --verbose --out-link /tmp/nh-osBMrumc/result | nom --json }
Error:
   0: Building NixOS configuration
   1: No such file or directory (os error 2)
   2: No such file or directory (os error 2)

Location:
   src/commands.rs:149

[deviantsemicolon]

this worked with the PR perfectly fine.

[henriquekirchheck]

You can use any elevation program with run0 nh os switch -R, and a cleaner abstraction of --use-remote-sudo will be implemented in #107

I think that --use-remote-sudo is a better way to do this, so I don`t have any problems

[henriquekirchheck]

The main difficulty I see with supporting other programs is the lack of support for --preserve-env, which of the four I pretend to support, only run0 and sudo supports it.

The way that nixos-rebuild-ng seems to be doing it is just passing all args via sudo env {ARG=VALUE...} [COMMAND] {ARGS...} when running via ssh, but it also doesn't inherit from the current env so it's functionally equivalent to the current implementation.

But in general, it seems like they don't need any extra env that sudo doesn't automatically preserve, but I haven't studied this code base enough to know why these extra envs are needed.

Now, I think the way to progress in a compatible way is to just read the current value of the variable that is meant to be preserved and just add it to the env args, which would be pretty straightforward and reduce the logic needed to support extra programs in the future if they come to exist or even to make sure that a custom program that is passed in can be used instead of the officially supported options.

I will be progressing with this idea, however I don't know if it could cause any problems because of the use of env::set_var throughout the code base. But I assume it wont considering I couldn't find any use of threads in the affected paths.

[hqnna]

The main difficulty I see with supporting other programs is the lack of support for --preserve-env, which of the four I pretend to support, only run0 and sudo supports it.

This is not correct. You can configure doas to support keeping env similarly via the config:
https://man.openbsd.org/doas.conf#keepenv

To do it you could just assign a custom rule to the group you want:

security.doas.extraRules = [{
  groups = [ "wheel" ];
  keepenv = true;
}];

It is via a config option rather than a flag, but it does support preserving env.

[henriquekirchheck]

This is not correct. You can configure doas to support keeping env similarly via the config: https://man.openbsd.org/doas.conf#keepenv

I'm aware of this option, however it is not dynamic and doesn't seem to have a option to filter some specific variables. I still think the {elevation} env VAR=VAL {command} is better as it should be generic for any privilege elevation programs, however I'll look into it in the case I can't make it work.

[NotAShelf]

Sorry for the constant changes to relevant modules. There were some things I wanted to get out of my way for the 4.2.0 release. Should be around the last time I've created merge conflicts, maybe one or two more times with the SSH changes I'm planning.

Once the master changes are done, I'd like to review and potentially merge this PR. Perhaps even in time for 4.2.0.

[henriquekirchheck]

This should be working as expected, it creates a nh::commands::ElevationStrategy based on the command line arguments and propagates that to the commands that need it.

This was tested on nixos-unstable with doas, "sudo" (an alias to doas), run0, and pkexec. I don't have a Darwin system, so I can't test for problems there, but it should work.

Any feedback appreciated.

[henriquekirchheck]

Re-implemented NH_SUDO_PRESERVE_ENV as NH_PRESERVE_ENV. It should work the same, but now it just omits the environment variables from the env arguments instead of not passing it to sudo

[henriquekirchheck]

Just as a heads up, unless there is any change needed in the code or some bug I couldn't catch, I think this should be complete and ready for merging. Please let me know if any changes are desired, be it something in the choice/environment logic or tests or whatever else.

[NotAShelf]

Sorry for the radio silence, I've been a bit too swamped at work and what little time I've got has been going into personal projects & additions that I will forget otherwise. I'll provide you a review later today, if you get a chance to deal with the merge conflicts by then but I'll try to fix those myself if I end up merging this today. The implementation looks good from a first glance anyway, so I wouldn't expect any substantial changes.

[midischwarz12]

I'll have quite a bit of time to test this tomorrow. Feel free to ping me if I forget.

[NotAShelf]

Few changes for the sake of clean code and cleaner error handling. I've been working on getting rid of all unwraps and panic-based error handling for the past few weeks.

[NotAShelf]

Seeing I've had the opportunity to nit a littlee bit, this is probably my last review comment. This is now suitable for a merge, which I will go ahead with after my latest comments are addressed. Good work!

P.S. if you could squash your commits that would be great so that I can avoid a squash merge.

[henriquekirchheck]

@NotAShelf Everything should be done now, squashed as requested!

[NotAShelf]

LGTM. This was technically the last item required for 4.2.0. I'll give it a few days in the beta phase, and then prepare the final work for 4.2 release.

Thank you for your patience and hard work :)