Create Task
Maniphest T14862

make kauth reentrant/concurrent
Open, LowPublic
Actions

Description

https://bugs.kde.org/show_bug.cgi?id=379215

This is fairly depressing. KAuth's polkit/dbus backend has no notion of running the same action more than once at any given point in time. Neither from the same client nor multiple clients. This means the same helper practically cannot be used by the multiple clients (the linked bug), and also cannot run multiple different action-ids at the same time (plasma-disks shoots off multiple SMART queries for all drives).

Fixing it is super involved though. What I would do is:

Helper

Instead of exposing the actions as functions we should scope them into objects. i.e. the clients call org.kde.kf5auth.createAction and get back a dbus object path /Action/123 which then encapsulates that invocation context. This gives us a 1:1 mapping between a client Action and a helper Action which solves most of the reentrancy problem by allowing us to easily scope all the context. Obviously this would need to be opt-in because there is no way of telling if existing code is in fact reentrant :(. The helper-side context object then also allows us to authenticate the request at creation time and from then on out scope all dbus traffic to the authenticated remote (as opposed to right now where we broadcast all signals).

Client

Client-side API would not change since Actions are already jobs there. Internally we would need to rejigger some things inside the Action but for the most part I think we only need to give out a dbus interface wrapper from the proxyhelper and then connect to that instead of the helper. Thus embracing the 1:1 mapping with the helper action.

Concurrency

On its own the dbus changes don't really help much though. If multiple actions are being run at the same time that either means we need to remove all timeouts (which may be a reasonable thing to do in the interest of simplicity) and have all actions run sequentially, but neatly separated.

To achieve concurrency I don't think we'll be able to avoid threading, unless we want to fork off each Action into its own anonymous instance. I'm not sure that is necessarily simpler though.

Fork away

createAction would authenticate the request and then fork itself to create a specific instance for the authenticated request. Instead of giving out an object path we'd give out the new serviceName (e.g. :1.234). The client would then connect to that helper instance and that helper will only ever talk to that client. The main helper would also have to keep track of the forked off instances somehow (service watcher?) to detect when idle and auto-terminate.

Threading

Runnable/Function/Future

First option for threading would be to simply have the helpers opt into threading and when that is the case we assume all functions are thread-safe. We can then simply run them in the threadpool. Since the main helper is still in the driver seat, detecting idleness for auto-termination should be easy.

Objectivy

The alternative would be to go full on OOP on the problem and have the helper be an ActionFactory that creates Action objects and we then moveTo the Actions in an own thread each. This may be a bit overblown since I expect most helper actions are fairly straight forward functions and if not one could still objectivy stuff by blocking the action function with an eventloop.

sitter created this task.Sep 9 2021, 2:45 PM
sitter triaged this task as Low priority.
sitter added a subscriber: davidedmundson.

@davidedmundson thoughts on this would be very welcome

davidedmundson added a comment.Sep 9 2021, 2:57 PM

KAuth is a high level API round doing simple tasks. In this case you don't want to do a simple task.

I think your proposals is going the wrong way, we don't need to add more complexity on top, but rather expose the underlying base components that KAuth is built on so that you can customise things.

From a client POV, you can just be using QDBus. QDBus should have everything you want.
It doesn't need to care that the other end needs polkit. Only difference is a call might take longer.

From the helper POV you want to call Polkit1Backend::isCallerAuthorized on all your dbus endpoints before doing anything.
This isn't exposed in KAuth directly, but could be.

AFAIK we did this for the KDE partition manager using polkit-qt directly and I've not heard of any issues since.

sitter added a comment.Sep 9 2021, 3:16 PM
In T14862#263187, @davidedmundson wrote:

In this case you don't want to do a simple task.

But we do. The KIO case is a glorified QFile::write, the plasma-disks case is not having to care that the system has >1 disk that need data. What I'm proposing isn't really adding complexity on the consumer side. It's adding complexity in our abstraction in between such that the consumer side no longer allows us to shoot ourselves in the foot ;)

Not that I disagree with anything you've said besides that, but surely if we leak polkit/dbus specifics we may as well do away with the platform abstraction and instead build a bespoke convenience framework for polkit?

ahiemstra added a subscriber: ahiemstra.Sep 10 2021, 1:37 PM

I rather like the "fork a new process" option personally, since that would make it harder to exploit security problems in the actions. That said, I don't know if that's a concern in the first place.

Would it be worth stepping back and reconsidering the purpose and design of KAuth in general?

sitter added a comment.Sep 14 2021, 9:52 AM

Would it be worth stepping back and reconsidering the purpose and design of KAuth in general?

Perhaps. I, for one, did struggle with coming up with a better "thing" unfortunately. On paper this all makes perfect sense. We do certainly want a way to easily authenticate trivial actions - like when a KCM needs to tell timedated to change the timezone. The way elevation works on OSX is certainly similar enough that you could abstract from polkit and osx with the same concepts (you launch a daemon via launchd, it IPCs with the client, requests get authorized via a framework on the daemon-end -- https://developer.apple.com/library/archive/documentation/Security/Conceptual/SecureCodingGuide/Articles/AccessControl.html#//apple_ref/doc/uid/TP40002589-SW2).

The way I see it the biggest hurdles are on the inside. Notably the design of the HelperProxy which is

  • shared by client and helper, making the class needlessly complex to read because it mixes both sides of the coin
  • used as a singleton given out by the actual platform backend which is indeed the main reason concurrency is broken, if the backend gave out per-request proxy objects (proxying the specific request remote) then the design would naturally lead towards more concurrency. hence my proposal

All that being said though. Considering I can't think of any app that targets OSX and wants elevation, perhaps the way to look at all of this is that kauth should be deprecated until we have a use case and instead we should make polkitqt have facilities for easy use? Being liberated of the abstraction might help see what's needed. And if/when we need an abstraction we can then heavily inform its design by what we've done with polkitqt. Perhaps that ship has sailed because I sure don't fancy porting all the stuff to another thing 😢