Bug 1217178

I'll have a look at this.

It seems we never formally reviewed kauth, the last time somebody had a deeper
look was in 2017 via bug 1036244.

I compared the kf6-kauth against the kauth currently found in
openSUSE:Factory. The changes are small:

- global rename of identifiers towards kf6
- removal of deprecated parts of the API
- removal of Qt5 compatibility
- change of the buildsystem to build against Qt6

Otherwise there are practically no changes to the business logic of kauth.

Since kauth is at the heart of KDEs Polkit security and since we didn't review
it properly for a long time I will take a closer look at its key parts anyway.

The design of kauth really is weird in many spots. But the main authentication
path for Polkit/D-Bus should be fine, even if it causes headaches when trying
to follow it through.

I have a few pointers for upstream regarding the following though:

- a) this `s_watchers` hashmap is no longer used:
  https://invent.kde.org/frameworks/kauth/-/blob/master/src/executejob.cpp?ref_type=heads#L43

- b) the function DBusHelperProxy::callerUid() and related functions seem unused:
  https://invent.kde.org/frameworks/kauth/-/blob/master/src/backends/dbus/DBusHelperProxy.cpp?ref_type=heads#L352
  it's still called in HelperProxy::callerUid(), but this in turn isn't called
  by anybody. Except it would be called externally, but why should that
  happen?

- c) in DBusHelperProxy::performAction() this hack here:

  https://invent.kde.org/frameworks/kauth/-/blob/master/src/backends/dbus/DBusHelperProxy.cpp?ref_type=heads#L236

  is really awful. The process is basically multithreaded and the hack isn't
  thread safe, influences the Qt framework for the complete process. Wouldn't
  there be a better way to prevent the sketched problem?

  The logic was introduced in commit fc70fb01.

- d) This "ExtraCallerIDVerificationMethod" is really strange:

  https://invent.kde.org/frameworks/kauth/-/blob/master/src/backends/dbus/DBusHelperProxy.cpp?ref_type=heads#L200

  The `callerID` is an untrusted value supplied by the unprivileged client,
  when using Polkit. I can only imagine this very strange design is due to the
  need to also cover the MAC backend with the same interface. It makes
  absolutely no sense to pass-on this callerID when using Polkit/D-Bus on
  Linux, it is just a confusing burden.

Since this is a KDE6 major release it might be a good time to address some of
the problematic design aspects mentioned in c) and d), even if it breaks
interfaces.

What might be the best mechanism to approach upstream about the suggestions in comment 4? A bug on bugs.kde.org? Or are you in touch with upstream yourself?

So basically the review is complete.

Since it still takes a long time before KDE6 is actually released I'll switch
this bug into an AUDIT-WHITELIST bug, pending for whitelisting. When the
actual release is drawing near then we will have a look over any recent
changes before we submit the actual whitelisting.

The upstream Git revision I reviewed was 0cee240e50f07200740b99906e8a9b9a202296fc.

TODOs left:

- approach upstream with suggestions for improvement
- install source code change monitoring for kf6-kauth once it is in Factory.
- actually whitelist

https://invent.kde.org/frameworks/kauth/-/issues/3 was filed upstream

The current kf6-kauth package has reached version 5.249.0 - a bit confusing
that they still use a 5 major version for kf6.

The changes since the last review are 80 % noise, otherwise a change with
regards to Wayland support in GUI window handling. Of the items I reported in
comment 4 the topics a) and d) have been addressed.

The other item I raised only in the upstream issue mentioned in comment 7 is
that I suggested to support passing of file descriptors in D-Bus interfaces.
I did not receive any feedback about this.

The new kauth on its own is fine so far. I will start the whitelisting process
now.

The whitelisting is in Factory now. Closing as fixed.