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

Try harder to become root and speed up by skipping unnecessary sudo's #608

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ngaro
Copy link

@ngaro ngaro commented Feb 27, 2021

All other scripts in Installer-Linux can be improved in the same way as below, but I'm not spending time on it if this doesn't get merged...

This PR:

  • Speeds up the script because you don't need to run sudo anymore if you are already root
  • Tries harder to run itself as root if this is not the case:
    • It checks where sudo is located instead of assuming it is in $PATH (it uses which instead of env for this because env is a binary that isn't necessarily available. which on the other hand is also a shell-buildin that even shells as tiny as busybox have)
    • It tries to use su if you don't have sudo and warns you that this is deprecated (also not assuming it's in $PATH)
    • It quits and warns you that you'll have to become root manually if both sudo and su aren't available
    • It gives the correct reason if something fails*
    • Doesn't depend on the name of root (see Accounts for a root-user with a different name #606 for more info)

*The 'real' part of the script hasn't been improved yet, that's for another PR. So if one these commands fail you will obviously not see the correct reason yet

@rusty-snake
Copy link
Contributor

  • Speeds up the script because you don't need to run sudo anymore if you are already root

I don't think performance matters for shell-scripts (they have no (JIT-)compiler and start new processes for everything they do). Anyway it allows to run the script on systems without sudo / su.

  • It checks where sudo is located instead of assuming it is in $PATH

To quote man 1 which:

Which takes one or more arguments. For each of its arguments it prints to stdout the full path of the executables that would have been executed when this argument had been entered at the shell prompt. It does this by searching for an executable or script in the directories listed in the environment variable PATH using the same algorithm as bash(1).

So it just searches for it in $PATH.

which on the other hand is also a shell-buildin that even shells as tiny as busybox have

It's not listed in man 1 builtins and LC_ALL=en_US.UTF8 builtin which shows bash: builtin: which: not a shell builtin. command on the other hand is a builtin (at least in bash).

  • It tries to use su if you don't have sudo and warns you that this is deprecated

It is deprecated? man 1 su does not say this, from where is this info?

  • It quits and warns you that you'll have to become root manually if both sudo and su aren't available

We could also check pkexec if we want.

@ngaro
Copy link
Author

ngaro commented Feb 28, 2021

  • Speeds up the script because you don't need to run sudo anymore if you are already root

I don't think performance matters for shell-scripts (they have no (JIT-)compiler and start new processes for everything they do). Anyway it allows to run the script on systems without sudo / su.

I agree with performance not being important. Still, low-importance improvements are also improvements. Although every call to a binary (e.g. foobar) starts at least 1 process, sudo foobar and su -c foobar start at least 2 processes.

  • It checks where sudo is located instead of assuming it is in $PATH

To quote man 1 which:

Which takes one or more arguments. For each of its arguments it prints to stdout the full path of the executables that would have been executed when this argument had been entered at the shell prompt. It does this by searching for an executable or script in the directories listed in the environment variable PATH using the same algorithm as bash(1).

So it just searches for it in $PATH.

which on the other hand is also a shell-buildin that even shells as tiny as busybox have

It's not listed in man 1 builtins and LC_ALL=en_US.UTF8 builtin which shows bash: builtin: which: not a shell builtin. command on the other hand is a builtin (at least in bash).

It seems that i was a bit to hasty with my conclusion. I tried my code on the tiniest "system" i had (a alpine docker container that barely has any binaries) and because it was present i incorrectly assumed it was a builtin.
Sorry about that. Your conclusions are correct.

While writing this I'm suddenly thinking about a better way to check if sudo (or su) is present that doesn't require which or env. I'll fix in my next commit

  • It tries to use su if you don't have sudo and warns you that this is deprecated

It is deprecated? man 1 su does not say this, from where is this info?

True, officially it's not deprecated.
But in practice it is. All recent tutorials, books, ... use sudo. Probably because it does everything su can do with the advantage of working correctly on systems without a root password (which are extremely common nowadays). I'll change the warning in the next commit.

  • It quits and warns you that you'll have to become root manually if both sudo and su aren't available

We could also check pkexec if we want.

The name vaguely rings a bell, but I've never used it. I'll take a look at it and use it in a 3rd commit if it it's a extra feature.

@rusty-snake
Copy link
Contributor

While writing this I'm suddenly thinking about a better way to check if sudo (or su) is present that doesn't require which or env.

If you want to only use builtins (is echo always a builtin?):

if ! command -v sudo >&-; then
	echo "no sudo"
fi

@ngaro
Copy link
Author

ngaro commented Feb 28, 2021

Changes after this commit:

  • Improved the warning about the more-or-less deprecated state of su
  • The presence of which is no longer necessary. Checking for the returncode 127 is also a method to check if the command is present. (This is also faster because the extra call to which is no longer needed.)

I've tested the returncode 127-rule in bash, zsh and busybox so it's safe to assume that it works everywhere.
This doesn't stop developers of choosing to exit their code with 127 themselves but this isn't the case for sudo and su. (And it's probably never the case in well-known programs because it's just 'evil')

Note:
Returncode 127 only checks for shell builtins and binaries in $PATH so it could be that the binaries are at a unexpected place and we are not using them although we could have been.
But checking this would be extremely slow because commands like locate or find would be necessary. Also, a sudo (or su) at a 'strange' place could just as well be a completely different command that just happens to have the same name.

Copy link
Contributor

@rusty-snake rusty-snake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

sudo $0 && exit 0
if [ $? -eq 127 ] ; then
echo "I can't find 'sudo', I'll try to use 'su' to become root."
echo "(Remember that 'su' is considerd deprecated (it needs a root password), I strongly recommend installing 'sudo'.)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab -> 8 spaces

Suggested change
echo "(Remember that 'su' is considerd deprecated (it needs a root password), I strongly recommend installing 'sudo'.)"
echo "(Remember that 'su' is considerd deprecated (it needs a root password), I strongly recommend installing 'sudo'.)"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that i wrote considerd instead of considered . I'll fix both things in the next commit (after taking a look at pkexec and including it if we can use it)

(Also fixes typo 'considerEd' typo and incorrect indenting
@ngaro
Copy link
Author

ngaro commented Feb 28, 2021

  • indenting corrected
  • typo fixed
  • pkexec support
  • warnings/errors to STDERR because that's the standard policy . This is a separate commit because I only thought about it after the previous commit

According to it's man-page pkexec can return 127 which breaks using 127 to check for a non-existent pkexec 😒
So in case of a problem I'm just checking for 126 (authorization-related problems) or any other non-zero code (all other problems)

We could extend the code even more by:

  • Using the 127 trick to check the existence of which
  • Falling back to env in case of non-existence of which and also doing a 127 check of env
  • Mentioning we don't know the cause of pkexec's 127 if env is also unavailable
  • Using which (or env if we couldn't find which) to check the existence of pkexec
  • Use this info to report the reason of the pkexec's 127

But according to me that's overkill, certainly because unless you have a really strange configuration, you will see a 'command-not-found'-type of error anyway and it will cause running which or env which will be a slowdown.

@funilrys funilrys requested a review from rusty-snake May 2, 2021 07:43
@funilrys
Copy link
Member

funilrys commented May 2, 2021

Hi everyone, this looks marvelous. About your "We could extend the code [...]" part, I agree that it's overkill. People who manage "strange configuration" should be able to handle such thing.

Anything to add @rusty-snake ?

cc @mitchellkrogza.

Copy link
Contributor

@rusty-snake rusty-snake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything to add @rusty-snake ?

Exit-codes don't work like exceptions. The don't tell you the reason why some thing failed. The don't even tell you if something failed. IMHO it would be better to use command (or which) to check if sudo/su/pkexec are present.

Furthermore this has duplicated and nested code. Here are two ideas to improve this.

become_root() {
        if command -v "$1" >/dev/null; then
                "$@" && exit 0
                echo "Something went wrong with trying to use '$1' to run this script" > /dev/stderr
        else
                return 1
        fi
}       

echo "This script needs to be ran as root, let's switch to root:" > /dev/stderr
become_root sudo "$0" && exit 0
echo "I can't find 'sudo', I'll try to use 'su' to become root." > /dev/stderr
echo "(Remember that 'su' is considered deprecated (it needs a root password), I strongly recommend installing 'sudo'.)" > /dev/stderr
become_root su -c "$0" && exit 0
echo "I also can't find 'su', I'll try to use 'pkexec' to run this as root..." > /dev/stderr
become_root pkexec "$0" && exit 0
echo "I can't give you authorization to run this as root, you'll need to find another way to become root..." > /dev/stderr
exit 1
echo "This script needs to be ran as root, let's switch to root:" > /dev/stderr
if command -v sudo >/dev/null; then
        sudo "$0" && exit 0
        echo "Something went wrong with trying to use 'sudo' to run this script" > /dev/stderr
        exit 1
elif command -v su >/dev/null; then
        su -c "$0"
elif command -v pkexec >/dev/null; then
        pkexec "$0" && exit 0
        echo "Something went wrong with trying to use 'pkexec' to run this script." > /dev/stderr
        exit 1
else    
        echo "I can't give you authorization to run this as root, you'll need to find another way to become root..." > /dev/stderr
        exit 1
fi

@funilrys funilrys marked this pull request as draft May 2, 2021 09:46
@funilrys
Copy link
Member

funilrys commented May 2, 2021

Great. Thanks for your input and time @rusty-snake. This PR will stay a draft until the recommendation of @rusty-snake are applied.

Stay safe and healthy.

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

Successfully merging this pull request may close these issues.

None yet

3 participants