linutil
linutil copied to clipboard
ChrisTitusTech
Add root check
Type of Change
- [ ] New feature
- [x] Bug fix
- [ ] Documentation update
- [ ] Refactoring
- [ ] Hotfix
- [x] Security patch
- [ ] UI/UX improvement
Description
- Thanks @cartercanedy and @adamperkowski for helping out !!
- Add root check and warn users about running as root
- Uses
nixcrate
Testing
- Undergoing. Most of the scripts are tested with root, should be okay !!
Issues / other PRs related
- Resolves #816
Checklist
- [x] My code adheres to the coding and style guidelines of the project.
- [x] I have performed a self-review of my own code.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [ ] I have made corresponding changes to the documentation.
- [x] My changes generate no errors/warnings/merge conflicts.
just look for
.dockerenvin /
Not only docker env runs as root. Normal user may invoke linutil as root
just look for
.dockerenvin /Not only docker env runs as root. Normal user may invoke linutil as root
this allows the user run the script to run it in docker/podman/ andy other container system. this is because users do not usally create users in containers, and insted default to the root user pre-created. we just need to tweak the scripts to allow the scripts to skip elevation checks if the user is root
just look for
.dockerenvin /Not only docker env runs as root. Normal user may invoke linutil as root
this allows the user run the script to run it in docker/podman/ andy other container system. this is because users do not usally create users in containers, and insted default to the root user pre-created. we just need to tweak the scripts to allow the scripts to skip elevation checks if the user is root
Running scripts as root may break many things. For example configuration files should not be in root permission. An application or non-root user may not be able to rw the configuration file due to insufficient permissions. And why would someone try to run linutil in a docker container ??
@jeevithakannan2 instead of precluding the use, it might be worth implementing a root warning and doing a refactor to just leave ESCALATION_TOOL unset when run as root. Configuration files should written relative to $HOME, which would be inaccessible to other users anyways, and software is almost exclusively, if not completely, handled by package managers, so there shouldn't be any permission issues there. If there are any game-breaking conflicts, I'll eat my words, but I don't think that we need to put that tight of a restriction.
just look for
.dockerenvin /
I don't like this. Running a ubuntu docker container should generally the same as running as root in a bare-metal ubuntu install, unless you're running a crazy stripped-down image. There are better solutions to make this workable for this kind of use-case
@jeevithakannan2 instead of precluding the use, it might be worth implementing a root warning and doing a refactor to just leave
ESCALATION_TOOLunset when run as root. Configuration files should written relative to $HOME, which would be inaccessible to other users anyways, and software is almost exclusively, if not completely, handled by package managers, so there shouldn't be any permission issues there. If there are any game-breaking conflicts, I'll eat my words, but I don't think that we need to put that tight of a restriction.
Ok will go through this one more time. If root is disabled arch server setup cannot be run. We could add a param to allow running as root like compatible mode linutil -t compatible likewise linutil --allow-root
Also, I made a PR (jeevithakannan2/linutil#1) on your dev branch that implements a warning prompt notifying users that they are running as root. Since we're not really testing these scripts with standard user perms, it's probably best to notify users that they are outside the bounds of the current user story
I also would remove the cli flag since most users are probably going to be using the network runner
I also would remove the cli flag since most users are probably going to be using the network runner
Yeah removed it thanks for the PR
This will only slow linutil down
?
This PR is adding another rust dependency, more checks and code that has to be maintained. I just think this can be easily avoided with a more efficient approach.
Getting the inlined superuser check functional would need even more code to maintain, and it would be hard to beat 8 lines of bash except by just unquoting all references toESCALATION_TOOL and not bothering with an additional function
Getting the inlined superuser check functional would need even more code to maintain, and it would be hard to beat 8 lines of bash except by just unquoting all references to
ESCALATION_TOOLand not bothering with an additional function
I'm talking about Rust code.
To be 100% honest, I love this idea
BUT
depending on a crate that hasn't been updated in 3-4 years for privilege-escalation related stuff is just not a good idea.
So I undo the changes and not use a crate. If yes I have solution for that too !!
depending on a crate that hasn't been updated in 3-4 years for privilege-escalation related stuff is just not a good idea.
Fair point
We can check the USER env right for running as root ? @cartercanedy What do you think should I just check the USER env for root and redo the scripts and remove the quotes ??
@jeevithakannan2 You can use Rust's libc to check current UID. I think it has to be unsafe though. Let me check.
libc
Yeah thought of that. It needs unsafe that's why I leaned toward crate
Yeah we currently 0% of Linutil's codebase is unsafe. Would be sad to loose that.
We could check for an env variable $USER or $UID and if that fails, use std::process::Command with id -u. Wouldn't be very efficient.
What about the scripts ??
scripts are fine
We can just unquote the ESCALATION_TOOL instead of creating a new function