trifectatechfoundation/sudo-rs

SETENV/NOSETENV option

mkg20001 opened this issue · 10 comments

mkg20001 commented

Describe the feature you'd like see implemented in sudo-rs
The SETENV/NOSETENV option currently is missing

What problem can be solved with this feature?
Currently the nixos sudo configuration uses setenv in some places, which we conditionally don't add when using sudo-rs

https://github.com/NixOS/nixpkgs/pull/253876/files#diff-5c91de272e9391a78a1d22be54a571b5dd3585be6e5968f0f62a7c9a790066b1R43

Having this feature would allow feature parity with regular sudo configurations on nixos

Describe alternatives you've considered
Currently we disable adding SETENV/NOSETENV when sudo-rs is in use

Additional context

mkg20001 commented

// cc @nbraud

  • ❤️1
nbraud commented

@mkg20001 There's also the env_keep option which is used in user-specific Defaults entries like so:

Defaults:root,%wheel env_keep+=TERMINFO

In the NixOS PR, I made this configurable and disabled-by-default when using sudo-rs, as it didn't seem to parse, but I'll revisit it once I get all the NixOS testsuite to pass with sudo-rs (disabling the tests using (NO)SETENV for sudo-rs)

PS: Got NixOS' testsuite to pass with sudo-rs, so support should be merged Soon™ 🎉

  • 🎉2
squell commented

Thanks for requesting.

  • "user-specific" Defaults was something we delayed until a later milestone (to see if there is a need for it; there are some corner-cases where we wanted to understand it's actual usage better before just blindly implementing it). I think the example you give for a more flexible env_keep is a compelling use case for it. (CC #61)

  • one question that would be useful for us as well: it's clear the you want SETENV since NixOS uses it, but why does NixOS use the SETENV flag in original sudo? I.e. what problems are solved by allowing a user to use sudo --preserve-env?

Back in may, we decided to postpone this feature because we were taking a stricter stance on env_reset (i.e. not allowing it to be disabled) out of a desire to build a simpler sudo; it would be useful to know what things this prohibits.

  • ❤️1
nbraud commented
  • "user-specific" Defaults was something we delayed until a later milestone (to see if there is a need for it; there are some corner-cases where we wanted to understand it's actual usage better before just blindly implementing it). I think the example you give for a more flexible env_keep is a compelling use case for it. (CC Specific Defaults parser+checking #61)

I'm glad we can provide you with useful feedback. I might get around to submitting a patch for #61, but don't wait up on me either: given my state of health, I can't commit to doing it in a timely manner.

  • one question that would be useful for us as well: it's clear the you want SETENV since NixOS uses it, but why does NixOS use the SETENV flag in original sudo? I.e. what problems are solved by allowing a user to use sudo --preserve-env?

Back in may, we decided to postpone this feature because we were taking a stricter stance on env_reset (i.e. not allowing it to be disabled) out of a desire to build a simpler sudo; it would be useful to know what things this prohibits.

That's a good question... I don't know 😓
It was added when @edolstra added the config module back in 2007, and the reason for SETENV is documented in neither the inline comments, nor the commit message.

To be clear, I am not a NixOS project member, I just send PRs... including the ones for sudo-rs support. 😅
In my personal opinion, NixOS users would be better served not having this option set (especially not by default) but changing this overnight is especially fraught when it would affect most users, and the reason for the status-quo is unknown.

IMO, the most sensible way forward for NixOS might well be:

  • make that default easily configurable;
  • make the transition to sudo-rs as easy as possible, and in particular automatically remove that default for them:
    the sudo-rs support PR is currently awaiting review, and once merged users could switch over with a single config line: security.sudo.package = sudo-rs;
  • see whether NixOS users transitioning to sudo-rs encounter any issues without SETENV ;
    I hope we will see many users switch early-on, as there already was some enthusiasm about sudo-rs support, even though it hadn't been advertised anywhere (as far as I know)
  • eventually see whether there is consensus to flip the default sudo implementation to sudo-rs.
pvdrz commented

I like the approach of "letting users tell us what they actually need"

squell commented

One easy step to take is of course to implement SETENV as a "dummy" tag (i.e. we accept it, but still -E/--preserve-env is unimplemented).

mkg20001 commented

It's a good idea, but it will lead to confusing problems later. When something that explicitly depends on SETENV starts failing after switching to sudo-rs is much more confusing to debug vs sudo-rs straight out rejecting the option.

Maybe add dummy and then give a specific error message about the missing SETENV feature would be an idea?

squell commented

Yes, we should emit a diagnostic for it and not just silently break things. We should probably do that for the other not-supported tags too (for our reference: cc #546 and #700).

yorickvP commented

I believe the reason NixOS uses SETENV is that it installs a lot of packages in the user environment that the user may want to run as root. An unsuccessful attempt to remove it was made a few years ago. There might be a better way to do this nowadays.

MayRedwood commented

If I may step in, I find that --preserve-env is very useful when I need to run graphical apps as root
This is, admittedly, not very common, but it's one of the few ways I've found to get gparted to run in a WM environment

  • ❤️2