Skip to content

Conversation

@rien
Copy link

@rien rien commented Mar 12, 2021

Some linux distributions allow switching sudo with doas. This PR adds a flag -d --doas which allows users to use doas for privilege elevation.

@skuhl
Copy link
Contributor

skuhl commented Mar 15, 2021

Would it be better to just modify the logic here? https://github.com/sshuttle/sshuttle/blob/master/sshuttle/client.py#L213

Maybe we could use doas if that is in the path and otherwise use sudo. The logic to base the decision on platform name could then be removed.

@rien
Copy link
Author

rien commented Mar 16, 2021

Searching in the path feels a bit wrong. What if both sudo and doas are in the path? What if doas is at ~/.local/bin/doas? Would you want to trust any executable with your password if it just has the right name?

I would maybe even replace all this logic option --privilege-elevation= which should point to the absolute path of the desired binary. The default should be what is used on most systems (/usr/bin/sudo I think) and package maintainers will replace the default for their distribution if it is something else.

@skuhl
Copy link
Contributor

skuhl commented Mar 16, 2021

I don't have strong feelings about it, I just wanted to mention it as an option. If both were in the path, we'd have to default to one or the other. Considering the idea that a malicious executable might be in the path is a good point. Right now, sshuttle just looks for various commands in the path: sudo, nft, iptables, etc.

I agree that making a more general privilege-elevation option that points to the desired binary might be good. We'd have to look at the name in the option to determine the correct arguments to pass. I suppose we'd want to consider doing this kind of approach across the board for the other commands that sshuttle runs too?

@brianmay
Copy link
Member

I am a little bit sad about adding yet another argument to the main function. Which already has a huge number of arguments :-( Particularly one that is specific to an optional 3rd party package.

FirewallClient.__init__() looks like it could end up in the same situation too.

A generic solution that means we don't need to have arguments for each specific "grant root" would probably be ideal. If this is possible. Maybe something like:

--privilege-elevation=-p '[local sudo] Password: '
--privilege-elevation=doas

Where we append the command to run to the end of this.

For this to work, we would need to slip command into arguments correctly, and pass the quoted value as one argument. If we want to keep the -p argument that is.

Also looking at the existing code, the variable sudo_pythonpath name might be confusing, as I think it isn't specific to sudo.

skuhl added a commit to skuhl/sshuttle that referenced this pull request Dec 31, 2021
This is an alternative solution to pull request sshuttle#611.

Previously, sshuttle would use doas on OpenBSD and sudo on Linux.
However, some Linux distributions are opting to use doas.

This patch changes the logic so that there can be multiple attempts to
elevate privilages. If the first command fails to run, it moves on to
the next command. Part of the existing code looked like it might be
attempting to do this, but it didn't work.

It also looks for the presence of doas and sudo in the path. If we can
find doas (but cannot find sudo) or if the platform is OpenBSD, we try
doas first. Otherwise, we try sudo, then doas. We try all the options
until one succeeds (including running the command without sudo or
doas) regardless of what is in the path. I'm open to adjusting
the logic here based on feedback.

If systems have both sudo and doas, they might be configured to give
different users different permissions. For example, if a user wishes
to use doas on this system, sshuttle would try sudo first and the user
would need to enter invalid passwords to eventually cause sudo to fail
and cause sshuttle to then try doas. This might not be ideal, but it
avoids implement another sshuttle argument that the user would need to
specify. Perhaps machines actually using doas will not have sudo
installed?
@skuhl skuhl mentioned this pull request Dec 31, 2021
@brianmay
Copy link
Member

Closing as I merged #708.

@brianmay brianmay closed this Jan 16, 2022
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.

3 participants