Ultimate.Hosts.Blacklist icon indicating copy to clipboard operation
Ultimate.Hosts.Blacklist copied to clipboard

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

Open ngaro opened this issue 3 years ago • 7 comments

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

  • This PR conflicts with #606 . Use that one if you want something easy and this one if you want something good.
  • This PR has been tested with multiple shells, including tiny ones, so it can be combined with #605

ngaro avatar Feb 27 '21 19:02 ngaro

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

rusty-snake avatar Feb 28 '21 10:02 rusty-snake

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

ngaro avatar Feb 28 '21 13:02 ngaro

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

rusty-snake avatar Feb 28 '21 13:02 rusty-snake

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.

ngaro avatar Feb 28 '21 14:02 ngaro

  • 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 :unamused: 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.

ngaro avatar Feb 28 '21 16:02 ngaro

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.

funilrys avatar May 02 '21 07:05 funilrys

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.

funilrys avatar May 02 '21 09:05 funilrys