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

SETENV/NOSETENV option #760

Open
mkg20001 opened this issue Sep 8, 2023 · 10 comments
Open

SETENV/NOSETENV option #760

mkg20001 opened this issue Sep 8, 2023 · 10 comments
Labels
enhancement New feature or request
Milestone

Comments

@mkg20001
Copy link

mkg20001 commented Sep 8, 2023

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 mkg20001 added the enhancement New feature or request label Sep 8, 2023
@mkg20001
Copy link
Author

mkg20001 commented Sep 8, 2023

// cc @nbraud

@nbraud
Copy link

nbraud commented Sep 8, 2023

@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™ 🎉

nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 8, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 8, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 12, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 13, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 13, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
@squell squell added this to the Milestone 4 milestone Sep 14, 2023
@squell
Copy link
Collaborator

squell commented Sep 14, 2023

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 Specific Defaults parser+checking #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.

nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 14, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 14, 2023
Separated environment-related tests, and only run them with o.g. sudo,
as sudo-rs does not support `(NO)SETENV` yet; see memorysafety/sudo-rs#760
@nbraud
Copy link

nbraud commented Sep 14, 2023

  • "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.

nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 18, 2023
Duplicated sudo's testsuite for now, as its maintainer does not with
to collaborate on testing effors; see NixOS#253876.

Environment-related tests were removed, as sudo-rs does not support
`(NO)SETENV` yet; see memorysafety/sudo-rs#760
nbraud added a commit to nbraud/nixpkgs that referenced this issue Sep 18, 2023
Duplicated sudo's testsuite for now, as its maintainer does not with
to collaborate on testing effors; see NixOS#253876.

Environment-related tests were removed, as sudo-rs does not support
`(NO)SETENV` yet; see memorysafety/sudo-rs#760
@pvdrz
Copy link
Collaborator

pvdrz commented Sep 18, 2023

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

@squell
Copy link
Collaborator

squell commented Sep 19, 2023

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
Copy link
Author

mkg20001 commented Sep 19, 2023

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
Copy link
Collaborator

squell commented Sep 19, 2023

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
Copy link

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.

toastal pushed a commit to toastal/nixpkgs that referenced this issue Sep 25, 2023
Duplicated sudo's testsuite for now, as its maintainer does not with
to collaborate on testing effors; see NixOS#253876.

Environment-related tests were removed, as sudo-rs does not support
`(NO)SETENV` yet; see memorysafety/sudo-rs#760
@MayRedwood
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants