sudo-rs: init at 0.2.0

[nbraud]

Description of changes

• packaged sudo-rs, a pure-Rust, mostly-compatible implementation of sudo
• didn't work (in this PRs) on the NixOS modules to allow users to switch to it

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
• Tested, as applicable:
  • and/or package tests
• Tested basic functionality of all binary files (usually in ./result/bin/)
• Fits CONTRIBUTING.md.

[nbraud]

I had a quick look, and it looks like sudo-rs won't work with NixOS sudo
module as it is now, with the fairly-vanilla configuration on my laptop:

$ ./sudo id
/etc/sudoers:8:38: expecting '=' but found '
'
root        ALL=(ALL:ALL) SETENV: ALL
                                     ^
/etc/sudoers:11:33: expecting '=' but found '
'
%wheel	ALL=(ALL:ALL)	SETENV: ALL
      	             	           ^
/etc/sudoers:15:9: expected parameter
Defaults:root,%wheel env_keep+=TERMINFO_DIRS
        ^
/etc/sudoers:16:9: expected parameter
Defaults:root,%wheel env_keep+=TERMINFO
        ^
sudo-rs: authentication failed: I'm sorry nicoo. I'm afraid I can't do that

I expect this is mostly minor mismatches in acceptable syntax,
but I am absolutely not looking at it tonight. ;3

[nyabinary]

Damn

[colemickens]

this partially addresses: #252193 (just to link these1)

[r-vdp]

@nbraud: you'll need a patch like this to make it work with our security wrappers. I'll be submitting this upstream but I need some time to do a proper write-up and finish the testing.

You'll also need to create an additional PAM service, called sudo-i for login shells using sudo, see here.

And we need to disable a bunch of stuff in the sudoers file that is not supported yet, things like SETENV and the user-specific Defaults that are included by the terminfo module.

This will need some testing as well. I think it would be nice to either extend or semi-duplicate the existing sudo nixos test.

[nyabinary]

Options like this should be added too right?

[r-vdp]

Options like this should be added too right?

Not necessarily, as sudo-rs aims to be a drop-in replacement for sudo. So we could just set security.sudo.package = pkgs.sudo-rs.
This is not currently possible yet though, since many things are not supported yet by sudo-rs.

[nbraud]

@nbraud: you'll need a patch like this to make it work with our security wrappers. I'll be submitting this upstream but I need some time to do a proper write-up and finish the testing.

Thanks a bunch. I was looking as doing something similar once I started working on a NixOS module, but I'm just as glad you beat me to it. ^^

You'll also need to create an additional PAM service, called sudo-i for login shells using sudo, see here.

And we need to disable a bunch of stuff in the sudoers file that is not supported yet, things like SETENV and the user-specific Defaults that are included by the terminfo module.

This will need some testing as well. I think it would be nice to either extend or semi-duplicate the existing sudo nixos test.

That was pretty-much my plan, but I wanted to defer that to a follow-on PR, splitting the generic nixpkgs work and the NixOS-specific one.

Testing-wise, for now I focused on getting sudo-rs' Rust tests to pass, but my goal in the long run is to have both the test(s) for NixOS' sudo module, as well as sudo-rs' own compliance tests... if I can find a reasonable way to make them run under NixOS (they make all the path assumptions)

For the former, it looks like sudo's “strict” testcase should work “as is” once the necessary module work is in place, but I'll need to check whether sudo-rs can handle the machine testcase's rules & config. Either way, I'll also need to test that the module asserts properly when given config sudo-rs cannot handle, rather than silently produce a configuration where users cannot use sudo anymore.

[nbraud]

Options like [security.doas] should be added too right?

You mentioned as much before, and my answer is the same (and in this PR's description too) : this will be in a followup PR.

PS: and as @r-vdp mentioned, we will most-likely adapt the current sudo module to make this more of a drop-in replacement.

[nbraud]

@r-vdp I included your patch for the moment, though hopefully it will soon make its way upstream. <3

[happysalada]

Would you mind squashing the 3 commits into 1 ? (I don't see a strong reason to have 3 commits here).

[nyabinary]

Any update on this?

[nbraud]

Any update on this?

@nyabinary Your enthusiasm is noted, but it's rather exhausting to be repeatedly asked the same questions, or asked whether there's been change only 2 days after receiving a review... when I may do other things with my weekend.

[happysalada]

I'm waiting for ofborg and will squash the 3 commits into 1.

[nbraud]

I'm waiting for ofborg and will squash the 3 commits into 1.

Why? 😕

The commit messages contain useful information & context.

[happysalada]

the 3 commits are just part of adding this package.
the sudo module's change is just a warning, IMO, this is just part of initializing the package.

[mkg20001]

this is sudo-rs. maybe future sudo. history and package file will be under intense scrutiny and people will want to know why what was done and will ask git blame. this is one of the packages where i'd just argue for no just because.

[nbraud]

the 3 commits are just part of adding this package. the sudo module's change is just a warning, IMO, this is just part of initializing the package.

By that logic, every PR would be squashed. Not only does that remove (potentially useful) information from history, but it makes tooling strictly worse:

• anything smaller than a PR cannot be reverted except by hand ;
• git cannot tell which branches and/or individual commits were actually “merged” ;
• this breaks any development branch that's based on the squashed branch: for instance, I have a branch where I'm working on adapting NixOS' sudo module, which would need a manual rebase if you squash-merge.

[happysalada]

alright let's get a second opinion then.
@SuperSandro2000 sorry to bother, but just in case you have time, what do you think about having this PR in 1 commit or 3 commits ?

[nbraud]

@happysalada Your choice of advocate is really not helping your case, I feel...
Moreover, I have that person blocked on Github for a reason, thanks.

PS: Wasn't Sandro's maintainership removed anyways?

[happysalada]

I was trying to find someone that might have time to help on this.
I am not going to take this issue to the grave though, I'll leave it 3 commits. It's definitely not worth arguing over.
I'll wait until ofborg finishes.

[nbraud]

Thanks for the review and merge.

[nyabinary]

Hmm, what are the options for this package? Well, how to use it over sudo?

[mkg20001]

It's planned that security.sudo.package = pkgs.sudo-rs will work

Currently sudo-rs seems to be missing a few features for that to work

@nbraud is currently working on fixing sudo-rs support

[nyabinary]

@mkg20001 Any PR for the progress so far?

[mkg20001]

master...nbraud:nixpkgs:nixos/sudo-rs well there's this but no pr yet.

regardless of that, what could be done is: clone nixpkgs, remove the assertion for sudo-rs, start a vm with security.sudo.package = pkgs.sudo-rs and report the missing features it complains about to the sudo-rs repo

[nyabinary]

@mkg20001 Isn't that just what got merged?

[mkg20001]

@nyabinary its that plus more commits

[mkg20001]

btw lets move further discussion to the tracking issue: #253465

[r-vdp]

@nyabinary feel free to help us out and propose changes that will help move this forward.

However, simply asking about the progress every other day is frankly a bit annoying and doesn't add anything.

So either be patient, or put in the time and effort yourself.