RFC: support doas (and maybe su)

madblobfish commented 1 year ago

Hello polygamma,

I'm still using aurman and am very grateful for you to continue signing your commits (and releases)! aurman is still the best AUR helper to me, thanks a ton for continuing!

Now to the actual changes: aurman is one of the few programs without support for doas on my systems. This is a quick shot at getting supporting doas in aurman, I can clean up if there's interest.

Notes

A new member sudo_method was put into the SudoLoop class in utilities.py to store the chosen method to get root. This might be a bit ugly to you, if not there were would you like it?
There is now a bit of code duplication in utilities.py's acquire_sudo, this can be merged, but might be less readable.
I needed to move search_and_print from utilities.py to aur_utilities.py to prevent a dependencyloop (loop was: aur_utilities => wrappers => utilities => aur_utilities) This was put into a separate commit to make it more easy to read. When I was fixing the other imports I also removed some unused ones.
Basic support for also supporting su is implemented, but completely untested! I could throw that out, guess there probably is not a lot of interest.
I clarified some error messages, which were raised after testing for being root and then misinformed users about requiring sudo for that.
Not all operations have been tested, I can ofc thoroughly test.

polygamma commented 1 year ago

Hey :)

Thanks a lot for your merge request! Cool to see, that there are people (besides me) who still use aurman.

I will definitely come back to this MR and give feedback and I don't see a reason why doas should not be supported, so I'll probably also accept the MR.

But currently I'm a bit busy, so it may take a few days (weeks?).

madblobfish commented 1 year ago

That's great to hear, no need to hurry (even a low number of months is okay for me).

I figured out we could also implement this using configuration options instead (like in makepkg) to support any authentication method: So add a string and two arrays into the config file.

String: is the command to execute for root escalation
Array: parameters for interactive escalation (pre sudoloop usage). This could be skipped if we just run /bin/true and append to the string from 1. Alternatively we could run id -u to check for uid=0
Array: parameters for non interactive escalation (e.g. sudoloop usage), prolonging timestamp and also checking this

This would have the disadvantage of requiring users to configure it. I'll daily drive the current pr then to give it some testing :)

polygamma commented 1 year ago

Hey :)

I did not forget about this... :D

Are you still interested in this Pull request?

madblobfish commented 1 year ago

Hi,

yes I am, I also did not forget ;)

polygamma commented 1 year ago

So, first things first: Supporting doas is fine, but supporting su sounds like trouble, so let's not do it ;D

I like the idea of supporting any authentication method, but I don't know enough about that, in order to be really sure, that what we are doing is sensible. So let's stick to sudo and doas (at least for now).

Important is one thing: Whatever we do now, the default, if the user changes nothing, has to remain at sudo usage.

I guess we only have to support doas via the aurman config, a commandline argument seems useless. See e. g. the following commit, which implements a new option for both aurman config + commandline: https://github.com/polygamma/aurman/commit/a13021cef6903b12484abfec8e960d2405f1c164

Regarding testing all of this: I'm currently working on a new project, which is private on GitHub for now (but will be publicly available in a few days I guess), and that new project has a nice CI and is actually using aurman quite extensively. It boils down to: The CI of the new project kind of tests aurman as well. So we can use that as well.

Since it's your idea to implement doas support, do you want to finish this PR? If so, let's talk about things like This might be a bit ugly to you, if not there were would you like it? after doas support is implemented, tested and fully working?

madblobfish commented 1 year ago

I like the idea of supporting any authentication method, but I don't know enough about that. [...]

What exactly do you mean by "about that", are you concerned about the security implications of users plugging in some random privilege escalation method? I think the user should know what they do if they adjust this. Otherwise I think you may mean how this will be implemented? I think setting a two strings or arrays is sufficent (one to test if the sudo loop works without password, one to prefix commands for privilege escalation).

[...] the default, if the user changes nothing, has to remain at sudo usage.

what if the system does not have sudo installed? (thats what I have) would you be okay detecting this?

Regarding testing all of this: [...]

Works for me.

[...], do you want to finish this PR?

Sure, if you don't want to take over. The current implementation works for me already as written before. The code automatically detects in get_sudo_method() what authentication methods are available.

I'll get around to implement the configuration option after clarifying if we shouldn't just automatically detect doas. By the way I think sudo should be preferred if both are installed, a configuration option works for me as well, no strong feelings here (but I think its more slick if it just works in both cases without configuration. Giving the users the option to plug in arbitrary stuff would also future proof aurman, no need to change aurman to implement escalation methods then).

polygamma commented 1 year ago

What exactly do you mean by "about that" [...]

E. g. I don't know why a string and two arrays are needed in the aurman config. Does that really cover EVERYTHING? If so, how do you know? I just don't feel comfortable implementing something in aurman that I don't understand 100%.

what if the system does not have sudo installed? (thats what I have) would you be okay detecting this?

Why would we want to detect that? Don't you simply get an error message telling you that sudo isn't installed, if you try to execute aurman and sudo isn't installed?

Sure, if you don't want to take over.

In theory I'd like to take over, but I don't have the time for it, and since I use sudo, aurman already gives me everything I need. For me there is no need to support anything else.

I'll get around to implement the configuration option after clarifying if we shouldn't just automatically detect doas. By the way I think sudo should be preferred if both are installed

If sudo and doas are installed, and the user wants to use doas, we still need the config, don't we? For me it seems like we need a config option anyway.

polygamma commented 1 year ago

One more thing: https://wiki.archlinux.org/title/Arch_User_Repository#Getting_started Still, base-devel (https://archlinux.org/packages/core/any/base-devel/) is a requirement, and that has sudo as dependency.

So I'd argue: Detecting whether sudo is installed, is not needed because sudo is a requirement to use the AUR. If people want to do something else (e. g. using doas), they can do it because it's Arch, but I don't think aurman should "step in" in such cases. If people don't know what they are doing, when they are explicitly not installing sudo, that's kinda their problem...

I'm so hesitant because aurman really just works for me... I'm using it in a productive environment for years and it's working perfectly. It would just really suck if I'd break it now ;D

Overall I'm a huge fan of abstract solutions, covering all possible cases. As I said, I like the idea of supporting any authentication method, but I'm no expert on that topic. That's why I don't know if a string and two arrays in the config file are really sufficient to support all possible authentication methods or not. If you are able to give me literature, references etc. so that I can be really sure, that this approach really covers everything, I'll be glad to support that. If you can't do that, I would rather not want to support it.

polygamma commented 1 year ago

I just remembered that there are quite a lot of people, who know a lot more about the AUR, makepkg, pacman etc. than myself. @eli-schwartz @Morganamilo @AladW (just to mention a few...)

It's been a few years, but maybe one of them is interested in this discussion and willing to help? :)

Let me summarize:

We want to support other authentication methods besides sudo, especially doas

It was mentioned to maybe also support su, but I guess that's not possible and/or not sensible? See e. g.:

[jonny@jonny-arch-pc ~]$ sudo su root makepkg
==> ERROR: Running makepkg as root is not allowed as it can cause permanent,
catastrophic damage to your system.

Another idea is to support "any authentication method", see: https://github.com/polygamma/aurman/pull/273#issuecomment-1362262395 However I've only used sudo my whole life, so I have no idea whether that's sensible to do or not. Since makepkg was mentioned by @madblobfish in this context, maybe @eli-schwartz can tell us something about this?

It boils down to: I don't know anything about the whole "authentication topic", that's why I'm hesitant to decide anything.

madblobfish commented 1 year ago

Does that really cover EVERYTHING? If so, how do you know? I just don't feel comfortable implementing something in aurman that I don't understand 100%.

I believe it will cover everything. And agree that you should not merge something you do not understand.

Why would we want to detect that? Don't you simply get an error message telling you that sudo isn't installed, if you try to execute aurman and sudo isn't installed?

Yeah thats kinda what happens without my patch. But its annoying to reconfigure all tools to use another method. I do agree its non standard and that it is a setup not supported by archlinux (base-devel being required and all, I also removed some other packages I deemed not required like systemd-sysvcompat, my /tmp is mounted noexec and so on. I like that I'm able to configure things to my personal tastes, not trying to impose anything on you or other users here).

For me there is no need to support anything else.

Not going on is for me okay as well, as long as upstream development stays slow its not a hassle to update the changes without merging. Your decision.

If sudo and doas are installed, and the user wants to use doas, we still need the config, don't we?

Yeah, I wanted to say (like above) that having automatic fallback might be nice (it is for me).

If people want to do something else (e. g. using doas), they can do it because it's Arch, but I don't think aurman should "step in" in such cases.

Works for me, a config flag suffices for me too.

I'm so hesitant because aurman really just works for me...

Not trying to break it. And I'm willing to help you out especially when it affects something I touched. But of course I can not provide a SLA.

Overall I'm a huge fan of abstract solutions, covering all possible cases.

And I like designing solutions that get not too ugly and cover all cases required :)

[...] I don't know if a string and two arrays in the config file are really sufficient [...]

We need 2 due to:

needing to test if noninteractive authentication is possible (sudo - or faked for doas using doas -n true (not actually equivalent because opendoas does not implement a true test without executing)). This is kinda required for the sudoloop/doasloop, we could also just skip this when not using sudo.
actually escalating privileges

If you are able to give me literature, references etc. [...]

Search for pacman_auth in man makepkg.conf. As far as I understand (did not read the source code for this) its a single string prefixed to commands (or interpolated, when using %c).

It was mentioned to maybe also support su, but I guess that's not possible and/or not sensible?

Well makepkg should not be run as root. but it still needs root privileges to install and remove packages (freshly built or when installing dependencies). You are supposed to have it only escalate privileges when doing that and not expose all of the code to root user privileges.

As far as I know su is in no way a program you should 100% avoid in all cases.

su elevates privileges and checks that you know the password of the user you're changing to. then it runs a shell or program (using -c) after clearing the environment and dropping privileges if you did not escalate to root
sudo asks you for your own password and checks the rules from sudoers if you got permission to do what you want. It provides some more protections and way more configurability. But note that it also leaves out some protections when run in default configuration, e.g. https://github.com/sudo-project/sudo/issues/258.
doas is a port from BSD. in the arch case it would usually be opendoas. you can think of it as a smaller sudo implementation with less features (and yes it also misses some small security gotchas, e.g. recently this).
there may exist more

All in all we need some way to escalate privileges to install packages or do other actions only root should be able to.

madblobfish commented 1 year ago

Hope this clears most of your questions and is not a too long read

AladW commented 1 year ago

I didn't check the entire discussion, but I'd just do like makepkg and introduce a PACMAN_AUTH variable (array) that defines a command-line for elevation. For example

pacman_auth = ['sudo']

pacman_auth = ['doas']

pacman_auth = ['pkexec' '--keep-cwd']

For su, makepkg has some special code to make it work. I wouldn't implement it for aurman, just leave it to the user, if they break it they can keep the pieces.

Things get more complicated when you want to keep the "sudo loop" support (sudo -v). I've never used doas, but I suppose doas /bin/true should work.

An alternative solution - which also doesn't leave all commands in a PKGBUILD with free root access, due to sudo/doas persistence - is to run aurman as root and drop privileges where required. yay/aurutils explored this approach, see e.g. https://github.com/AladW/aurutils/blob/master/examples/sync-asroot

polygamma commented 1 year ago

[...] and is not a too long read

Don't worry about that, I like to read and I like to write, there is no such thing as "too long to read" ;D

Now that I've read everything and thought about it, I have no problem supporting "any authentication method".

How about the following: Implementing the two arrays via the aurman config, and if it's not explicitly configured, aurman uses sudo, just like it is now. That will be documented in the README, together with an example, how to fill out the arrays to use doas.

Unless I missed something, that should be all we need. Or do you still see the need to detect if sudo is installed?

polygamma commented 1 year ago

Hey :)

Just wanted to ask, how it's going. And maybe there is a misunderstanding, I'm not sure ;D

If you want to finish the PR, and simply didn't find the time until now - everything's fine If you think that I'll finish the PR, and thus didn't push new code, there is the small misunderstanding :D I wrote

In theory I'd like to take over, but I don't have the time for it [...]

but I guess since I also wrote

Now that I've read everything and thought about it, I have no problem supporting "any authentication method".

one could think, that I changed my mind ;D

In a few weeks I'll come back to

Regarding testing all of this: I'm currently working on a new project, which is private on GitHub for now, and that new project has a nice CI and is actually using aurman quite extensively. It boils down to: The CI of the new project kind of tests aurman as well. So we can use that as well.

I should have the time to implement the PR by myself, then. So the question is: Who implements the PR? :)

And if you have any more ideas regarding "all of this", now is the time for it ;D

madblobfish commented 1 year ago

Hi @polygamma,

I'm just busy and did not come around to it yet, did not forget ;) If you like to: go ahead and implement it. If I find the time I'll look here for an update from you and maybe still implement it then, but you don't need to wait for me.

Feel free to ask me questions and I could also try to review your changes.

polygamma commented 1 year ago

Just a small update...

Regarding testing all of this: I'm currently working on a new project, which is private on GitHub for now (but will be publicly available in a few days I guess), and that new project has a nice CI and is actually using aurman quite extensively.

The project is public now: https://github.com/polygamma/auv

madblobfish commented 1 year ago

Hi @polygamma,

thanks for informing me, looks interesting (but due to me no longer using XOrg its not for me right now).

Here's some more good news. I finally found some time and am implementing it right now. :) There are two ways I envision the configuration to go:

either the user defines an escalation command (su, sudo, doas, whatever), then parameters for noninteraction and then parameters to test for an active session
or the user defines four command lines (more like prefixes) for all cases (probably repetitive and uselessly so).

Here's how option one could look like:

[privilege_escalation]
# sudo
escalation_command=['sudo']
noninterative_params=['--non-interactive']
test_params=['-v']

# doas
escalation_command=['doas']
noninterative_params=['-n']
test_params=['/bin/test']

# su
escalation_command=['su']
# noninterative_params and test_params are not supported therefore sudo_loop is not

Option 2

[privilege_escalation]
# sudo
interactive=['sudo']
noninterative=['sudo', '--non-interactive']
test_interative=['sudo', '-v']
test_noninterative=['sudo', '-v',  '--non-interactive']

# doas
interactive=['doas']
noninterative=['doas', '-n']
test_interative=['doas', '/bin/test']
test_noninterative=['doas', '-n', '/bin/test']

# su
interactive=['su']
# noninterative, test_interative and test_noninterative are not supported by su therefore sudo_loop is not

Design decisions:

I define the options as arrays because otherwise we run into some shellsplitting madness
I'm going for option one unless you want the more explicit syntax (its slightly more brittle and magic this way)
I'll try to keep everything as backwards compatible as possible (this means the code will check for the old sudo_loop and set the new variables accordingly). I also will make it so it disables the old sudo_loop logic if one of the new variables is set.

polygamma commented 1 year ago

Fantastic :) I like Option 1 better, I guess you do, too?

madblobfish commented 1 year ago

Yes I do.

it has the slight downside of not being as flexible. But I do not know if you would ever need that.

The implementation for option 1 will mix the arrays together to generate the 4 arrays internally (I think that's reasonable)

Quickly copypasted example of that

SudoLoop.interactive_command = AurmanConfig.aurman_config['privilege_escalation']['escalation_command']
noninterative_params = AurmanConfig.aurman_config['privilege_escalation']['noninterative_params']
test_params = AurmanConfig.aurman_config['privilege_escalation']['test_params']
SudoLoop.noninteractive_command = [*SudoLoop.interactive_command, *noninterative_params]
SudoLoop.test_interative = [*SudoLoop.interactive_command, *test_params]
SudoLoop.test_noninterative = [*SudoLoop.interactive_command, *noninterative_params, *test_params]

polygamma commented 1 year ago

I think that's reasonable

So do I :)

madblobfish commented 1 year ago

So I got a basic implementation down and working for me:

if escalation_command is not set it will default to the old sudo style
if only escalation_command is set in the config it will disable sudo_looping (basically ignoring no_sudo_loop)
if noninterative_params and test_params are set it will enable sudo_looping (unless no_sudo_loop is set, we could deactivate this but it seems needless to deprecate the old way completely)
It will complain if one one of them is set, the logic for that is not the prettiest though (but I did not want to go too fancy with it)
I also learned that the ConfigParser does not have an easy way to parse arrays, there are the following options:
- just implement splitting on something like , (thats what i did)
- separate by newline (ini supports multiline fields, I think that's a bit ugly)
- parse it with something like json.parse() (seems needlessly complicated)
- parse it with something else like python's ast.literal_eval (looks a bit dangerous to me)

polygamma commented 1 year ago

Very nice :)

I will do the following: Over the next couple of days, when I have the time, I'll create a new branch in the new project of mine. That branch will contain a nice "test suite" for aurman, including sudo usage and doas usage. Should make it easy to see whether or not it works :)

madblobfish commented 1 year ago

The most recent push just removed a PKGBUILD change that was for local testing, sorry.

Todo:

agree that the behavior is good (also agree on the config parsing point)
write some more documentation for the users :)

madblobfish commented 1 year ago

Also: no rush. Thanks for the patience and again for aurman :)

polygamma commented 1 year ago

agree that the behavior is good (also agree on the config parsing point)

Everything seems good.

polygamma / aurman

RFC: support doas (and maybe su) #273