Add root check

[jeevithakannan2]

Only merge this at last after merging all the script PRs

Type of Change

• New feature
• Bug fix
• Documentation update
• Refactoring
• Hotfix
• Security patch
• UI/UX improvement

Description

Caution

This is going to cause many merge conflicts. Merge other PRs first and I will update this PR later.

• Thanks @cartercanedy @adamperkowski @lj3954 for helping out !!
• Add root check and warn users about running as root
• Uses nix crate

Testing

• Tested with no issues !!

Issues / other PRs related

• Resolves remove "cant find escaltion tool if user is already root #816

Checklist

• My code adheres to the coding and style guidelines of the project.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no errors/warnings/merge conflicts.

[solomoncyj]

just look for .dockerenv in /

[jeevithakannan2]

just look for .dockerenv in /

Not only docker env runs as root. Normal user may invoke linutil as root

[solomoncyj]

just look for .dockerenv in /

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

[jeevithakannan2]

just look for .dockerenv in /

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 ??

[solomoncyj]

I need to set up the bash scripts as I can't stand the default one

[cartercanedy]

@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.

[cartercanedy]

just look for .dockerenv in /

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]

@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.

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

[cartercanedy]

Also, I made a PR (jeevithakannan2#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

[cartercanedy]

I also would remove the cli flag since most users are probably going to be using the network runner

[jeevithakannan2]

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

[adamperkowski]

I think this implementation is wrong and overengineered. We can just check the UID or even file permissions. This will only slow linutil down.

[cartercanedy]

This will only slow linutil down

?

[adamperkowski]

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.

[cartercanedy]

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

[cartercanedy]

Tbh, I think unquoting would be the simplest @jeevithakannan2

[jeevithakannan2]

Mmm 🤔

[adamperkowski]

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

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.

[jeevithakannan2]

So I undo the changes and not use a crate. If yes I have solution for that too !!

[cartercanedy]

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

[jeevithakannan2]

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 ??

[adamperkowski]

@jeevithakannan2 You can use Rust's libc to check current UID. I think it has to be unsafe though. Let me check.

[lj3954]

I'm not sure what all this discussion is, it's already been decided that we're going to adapt the scripts to work on root (as as been done in the recent commits here) rather than prohibiting linutil from launching as root. Please drop c997a3d.

By the way, @adamperkowski, both crates mentioned here, nix and this unmaintained sudo crate use libc::getuid() anyways. I think it's a poor idea to religiously avoid writing a line of unsafe code, just to pick up another dependency which calls quite literally the exact same function. That does the opposite of improving security.

[adamperkowski]

By the way, @adamperkowski, both crates mentioned here, nix and this unmaintained sudo crate use libc::getuid() anyways. I think it's a poor idea to religiously avoid writing a line of unsafe code, just to pick up another dependency which calls quite literally the exact same function. That does the opposite of improving security.

I am aware that they do use libc::getuid but by not including unsafe code blocks in our code directly we at least know that future PRs / other stuff like that will be relatively safe and have an overall lower chance of causing something like memory leaks, overflows, etc. It is 100% possible that (for example) libc gets updated in the future and using getuid directly could cause issues. Using nix::unistd solves this issue. (assuming that nix would be regularly updated)

[jeevithakannan2]

I'm not sure what all this discussion is, it's already been decided that we're going to adapt the scripts to work on root (as as been done in the recent commits here) rather than prohibiting linutil from launching as root. Please drop c997a3d.

I think you should check the files changed before suggesting. That commit was reverted

[cartercanedy]

We should use a #![forbid(unsafe_code)] top-level directive to enforce safety then

[jeevithakannan2]

@ChrisTitusTech All the scripts have shellcheck warning and formatting issues that need to be resolved in a separate PR. This PR has no issues.

[adamperkowski]

?

[cartercanedy]

?

I was just about to say...