jjk-jacky / statusnotifier

Library to use KDE's StatusNotifierItem via GObject
Other
25 stars 8 forks source link

Add ability to set custom bus name #18

Open TingPing opened 6 years ago

TingPing commented 6 years ago

Hardcoding org.kde.StatusNotifierItem-XXX is bad for sandboxing. The ability to set a custom string at the very least would be nice.

jjk-jacky commented 6 years ago

I'm not sure what you mean by that (bad for sandboxing); But also, those names are actually part of the specifications:

" Each application can register an arbitrary number of Status Notifier Items by registering on the session bus the service org.freedesktop.StatusNotifierItem-PID-ID, where PID is the process id of the application and ID is an arbitrary numeric unique identifier between different instances registered by the same application. " Source: https://freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/

So what's done here is only following the specs, and setting a custom name doesn't really make sense, since it would break things.

TingPing commented 6 years ago

I'm not sure what you mean by that (bad for sandboxing)

Flatpak grants access to applications based on bus names so by default applications cannot own random bus names. Granting access to org.kde.* is a very broad permission with potential security implications.

So what's done here is only following the specs, and setting a custom name doesn't really make sense, since it would break things.

Hmm do you happen to know how AppIndicator works then?

jjk-jacky commented 6 years ago

I'm not sure what you mean by that (bad for sandboxing)

Flatpak grants access to applications based on bus names so by default applications cannot own random bus names. Granting access to org.kde.* is a very broad permission with potential security implications.

Ah, I see. Well it could be org.kde.StatusNotifierItem-- but I get the point, yes.

So what's done here is only following the specs, and setting a custom
name doesn't really make sense, since it would break things.

Hmm do you happen to know how AppIndicator works then?

Nope, no idea, sorry.

TingPing commented 6 years ago

Just as an update, since you pass the bus name at registration time it can be anything in practice:

https://freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierWatcher/

Register a StatusNotifierItem into the StatusNotifierWatcher, in the form of its full name on the session bus, for instance org.freedesktop.StatusNotifierItem-4077-1.

jjk-jacky commented 6 years ago

Just as an update, since you pass the bus name at registration time it can be anything in practice:

https://freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierWatcher/

Register a StatusNotifierItem into the StatusNotifierWatcher, in the form of its full name on the session bus, for instance org.freedesktop.StatusNotifierItem-4077-1.

No, it cannot (be anything). Here you're looking at/quoting the part about the StatusNotifierWatcher, and the method used to register a StatusNotifierItem with it. And yes, an example is given under "for instance", but that naming isn't random, nor can it be anything.

Let's have a look back at those same specs, and how StatusNotifierItem must behave shall we:

https://freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/

Which (still) states: " Each application can register an arbitrary number of Status Notifier Items by registering on the session bus the service org.freedesktop.StatusNotifierItem-PID-ID, where PID is the process id of the application and ID is an arbitrary numeric unique identifier between different instances registered by the same application.

As soon as a new instance of a StatusNotifierItem is created, the application must register the unique instance name to the StatusNotifierWatcher as described in the Section called StatusNotifierWatcher "

TingPing commented 6 years ago

In practice it seems StatusNotifierWatcher implementations don't care what the bus name is (why should they) and it means it can work in sandboxes.

Honestly its a bad spec, PIDs aren't even unique with the invention of PID namespaces. So you have to violate the naming rules to even function as expected.

jjk-jacky commented 6 years ago

Well, I'm sure today's implementations you know of might not, but what about tomorrow, or future implementations?

Specs are there to define things, tell what to do & what not to do, what to expect from others, and the problem here is that we're not talking about not following a recommendation or general convention, but breaking things by going against what is supposed to be done, which would make statusnotifier a non-conforming implementation.

One does it, another one does another thing, and we end up in a mess of an ecosystem where everyone does its own thing based on its own beliefs/needs/whatever, and nothing works as expected anywhere... ah, if only there had been some kind of document written, establishing rules to follow... :p

My point is, maybe that part of the specs isn't ideal, maybe it could/should be changed, but that's likely another discussion for another place then. As far as statusnotifier goes, I feel it's best to try and remain conforming to the specs.

TingPing commented 6 years ago

Well the end result is my software just uses libappindicator and I just recommend libappindciator. Not because its good, not because it follows specs, but because it functionally works where this library does not.

jjk-jacky commented 6 years ago

hmm, so does libappindicator allow to set custom bus names instead of the ones from spec?

TingPing commented 6 years ago

It doesn't expose that as part of their API. Looking at the source it seems to actually call RegisterStatusNotifierItem() with a path not a bus name:

I'm not sure I actually see where it owns any bus name.

(Side note I am linking a fork just to browse on Github rather than Launchpad)

jjk-jacky commented 6 years ago

hmm... so yeah it looks like it is not spec-conforming at all, in fact it looks like a case of "stupid" as enlightenment calls them[1] That's a case of a watcher that already had to implement a workaround for non-conforming apps, such as any using that lib (e.g. steam) it seems.

I'm also wondering if such app would work at all with another (spec conforming only) host such as KDE Plasma? Or do they also had to implement such crap?

Anyhow, this is bad. For sure. Exactly what I'd rather avoid creating...

But, how does that mess in appindicator help wrt your issue and sandboxing?

[1] https://phab.enlightenment.org/rE6ff98d8a397a496bdfbdc9cf61533b82de3f206e

TingPing commented 6 years ago

libappindicator is very widely used, very likely the most widely used "implementation" of statusnotifiers at all. KDE and the GNOME-Shell extension do support it.

I agree it is a bad library and thats why I want a library like this to be used.

But, how does that mess in appindicator help wrt your issue and sandboxing?

The key to the sandboxing problem is bus name ownership. An application doesn't have permission to own any name other than its own app-id, so trying to own org.kde.* fails and allowing it opens up the possibility of pretending to be a core kde service which is a huge risk.

My ideal solution would be that this library simply calls g_application_get_application_id() and appends .StatusNotifierItem-$instance to the end and uses that. It will always work in a sandbox and logically it just makes more sense.

jjk-jacky commented 6 years ago

Okay, so I did some more research on this, and it looks like the situation is a goram mess for sure... Also, apparently even some KDE apps do not follow the spec; You'd imagine they would, since they created the interface/wrote the spec, but hey.

So, one way some KDE apps deal with this (and for sandboxing issues as well) it to simply not register any name on the bus at all. Instead, what they'll use to register the object and what they send to the StatusNotifierWatcher is the unique name of their existing dbus connection, so in the form :x.xxxx -- apparently that solves the issue with sandboxing, and though it is not conforming to the spec, what is sent to the watcher remains a valid dbus name where the StatusNotifierItem object can be found, so it all works fine in the end.

(One could note that such app will be treated as conforming per KDE Plasma's watcher, but not by enlightenment's one, since they use different methods for "identifying" appindicator's brokenness. However, I believe such behavior should work fine with enlightenment's watcher as well.)

I've just pushed a branch no-reg on github with a very crude way to bypass the whole registration bit and simply use the current connection, merely for testing purposes, much like those KDE apps seem to do. I've only run a simple test to ensure that under KDE the whole thing still works (using the provided example) and it seemed to be the case; I'd appreciate if you could try & make sure that solves your issue wrt to sandboxing as well.

If that's the case, and even though I don't really like it, I guess I'll be willing to accept a new property, FALSE by default of course, but that if enabled (prior to registering of course, afterwards it becomes read-only) would mean to not register any name on the bus, and instead just as described use the existing connection's unique name. Hopefully this should resolve your issue (even though you still can't define a custom bus name) with only "minor" breakage for the spec.

Having said that, I'm not sure when I'll find the time to work on this, so this would be a patches welcome kinda situation :)

Cheers,

TingPing commented 6 years ago

I'd appreciate if you could try & make sure that solves your issue wrt to sandboxing as well.

I can confirm that it works in a sandbox and against GNOME-Shell's extension.

Having said that, I'm not sure when I'll find the time to work on this, so this would be a patches welcome kinda situation :)

Just adding a property for this? Sure I can look into this weekend.

jjk-jacky commented 6 years ago

I can confirm that it works in a sandbox and against GNOME-Shell's extension.

Great, thank you.

Just adding a property for this? Sure I can look into this weekend.

That'd be awesome! It shouldn't be too complicated, I think the main thing now will be to name it... not sure what it should be, or even if it should refer to sandboxing or lack of bus registration, but I'll let you work this out I guess ;)