GNOME Bugzilla – Bug 724543
Buildtime option to restrict location into a particular user only
Last modified: 2014-10-15 08:25:15 UTC
To make it possible for distros to restrict location info to geoclue only, we'd need a configure option to restrict location-related API to a particular user only. Distros can specify the same user as they configure geoclue to run as. You can look at geoclue code to see how this can be implemented: * http://cgit.freedesktop.org/geoclue/tree/src/gclue-client-info.c#n237 (to find the userid) * http://cgit.freedesktop.org/geoclue/tree/src/gclue-service-client.c#n419 and * http://cgit.freedesktop.org/geoclue/tree/src/gclue-service-client.c#n454 for restricting access to methods and properties.
Oh and about signals, gio provides api to only send signal to a particular peer: http://cgit.freedesktop.org/geoclue/tree/src/gclue-service-client.c#n104
So, in order to know the user id/name of the remote client ModemManager would need to perform an async query via DBus, if I'm not mistaken. As we want to give access also when reading properties or emitting signals, we shouldn't be doing the async query each time, and that leaves us with the only option of needing to track clients. Now, there are 2 types of clients using ModemManager: a) the always-connected ones (e.g. NetworkManager or geoclue), for which we can ask once the user id/name and then track their availability in the bus. b) one-action clients, like the mmcli command line interface. For each action performed by this client, it will connect to the bus, do the action, disconnect from the bus. For the first kind I already thought on how we could this. For example, we would only allow reading location to clients who have already done a Location.Enable() themselves (and so we could start tracking them during that step). But how can I track the second type of clients? I'm not sure we can properly track these. One thing we could do is to allow property reading or signal reception only to tracked clients (i.e. a) case). While non-tracked clients (i.e. b) case) should instead use the GetLocation() method, which is already protected via polkit rules. What do you think?
(In reply to comment #2) > So, in order to know the user id/name of the remote client ModemManager would > need to perform an async query via DBus, if I'm not mistaken. As we want to > give access also when reading properties or emitting signals, we shouldn't be > doing the async query each time, and that leaves us with the only option of > needing to track clients. > > Now, there are 2 types of clients using ModemManager: > a) the always-connected ones (e.g. NetworkManager or geoclue), for which we > can ask once the user id/name and then track their availability in the bus. > b) one-action clients, like the mmcli command line interface. For each action > performed by this client, it will connect to the bus, do the action, disconnect > from the bus. > > For the first kind I already thought on how we could this. For example, we > would only allow reading location to clients who have already done a > Location.Enable() themselves (and so we could start tracking them during that > step). But how can I track the second type of clients? I'm not sure we can > properly track these. > > One thing we could do is to allow property reading or signal reception only to > tracked clients (i.e. a) case). While non-tracked clients (i.e. b) case) should > instead use the GetLocation() method, which is already protected via polkit > rules. What do you think? polkit rules -> dbus policy? Yeah, I don't see any other way either.
(In reply to comment #0) > To make it possible for distros to restrict location info to geoclue only, we'd > need a configure option to restrict location-related API to a particular user > only. Distros can specify the same user as they configure geoclue to run as. > > You can look at geoclue code to see how this can be implemented: > > * http://cgit.freedesktop.org/geoclue/tree/src/gclue-client-info.c#n237 (to > find the userid) > * http://cgit.freedesktop.org/geoclue/tree/src/gclue-service-client.c#n419 and > * http://cgit.freedesktop.org/geoclue/tree/src/gclue-service-client.c#n454 for > restricting access to methods and properties. Wait... how is that supposed to work w.r.t the DBus.Properties interface and the PropertiesChanged signal? From the looks of that code (handle_get_property) you're just limiting access when the properties are directly read. But whenever the property is updated in the skeleton, the gdbus-codegen generated code will internally emit PropertiesChanged for all readable properties. So a client could hook up to that signal and still get updated property values, even if you restrict it in handle_get_property... Or am I wrong? So far, the only way I'm seeing to handle this properly up to the PropertiesChanged level is to fully reimplement the skeleton, making sure the PropertiesChanged signal is only emitted to the allowed peers. Or, update GIO so that the gdbus-codegen generated code allows subclassing the PropertiesChanged emission code...
And another related issue... in ModemManager we also use the ObjectManager interface, which gdbus-codegen also supports. Now, gdbus-codegen assumes that all readable properties will be readable (regardless of any subclassed _handle_get_property method used to control access). Now, the gdbus-codegen generated code for the ObjectManager uses the get_properties() method in each interface to load all properties; and each interface implementation will use handle_get_property() to load each property *but* passing a NULL sender. This means that applying the same logic (e.g. subclassing handle_get_property) will break the initial loading of managed objects in the ObjectManager interface... From my POV, subclassing handle_get_property() to restrict access per sender is broken, because that method was not meant to handle that. If we want to have a general read access control of properties per peer, we really need to rework gdbus-codegen to avoid all those assumptions, and provide signals or subclass-able methods to provide the access control.
Now, the good news is that restricting location info to a single user is doable without many changes in MM. Geoclue doesn't use the info provided by the PropertiesChanged signal; it just uses that signal to get notified about when a new location info is available, and then uses the GetLocation() method. We can update the MM interface to provide a new signal "LocationUpdated", without parameters, which will just notify peers that a new location is retrievable, and for that GetLocation() needs to be used. So, when configuring the MM build to limit location info to a single user (which distros will be able to choose), we would also always forbid setting "signals-location" to TRUE during Location.Setup(). If that is never set to TRUE, the Location property will never get updated (and therefore no PropertiesChanged emitted to all peers). The Location property, along with PropertiesChanged, would then only be available if ModemManager wasn't compiled limiting the location info to a single user. How does that sound?
(In reply to comment #6) > Now, the good news is that restricting location info to a single user is doable > without many changes in MM. > > Geoclue doesn't use the info provided by the PropertiesChanged signal; it just > uses that signal to get notified about when a new location info is available, > and then uses the GetLocation() method. We can update the MM interface to > provide a new signal "LocationUpdated", without parameters, which will just > notify peers that a new location is retrievable, and for that GetLocation() > needs to be used. > > So, when configuring the MM build to limit location info to a single user > (which distros will be able to choose), we would also always forbid setting > "signals-location" to TRUE during Location.Setup(). If that is never set to > TRUE, the Location property will never get updated (and therefore no > PropertiesChanged emitted to all peers). If its not allowed to set that parameter to FALSE, why keep it? To deprecate it? > The Location property, along with PropertiesChanged, would then only be > available if ModemManager wasn't compiled limiting the location info to a > single user. Sure but I think you should only keep it for backwards compatibility and mark it as deprecated. At some point in future when we are sure that nobody will use it against older version of geoclue (the only user of this API AFAIK), you can just drop it.
(In reply to comment #7) > (In reply to comment #6) > > Now, the good news is that restricting location info to a single user is doable > > without many changes in MM. > > > > Geoclue doesn't use the info provided by the PropertiesChanged signal; it just > > uses that signal to get notified about when a new location info is available, > > and then uses the GetLocation() method. We can update the MM interface to > > provide a new signal "LocationUpdated", without parameters, which will just > > notify peers that a new location is retrievable, and for that GetLocation() > > needs to be used. > > > > So, when configuring the MM build to limit location info to a single user > > (which distros will be able to choose), we would also always forbid setting > > "signals-location" to TRUE during Location.Setup(). If that is never set to > > TRUE, the Location property will never get updated (and therefore no > > PropertiesChanged emitted to all peers). > > If its not allowed to set that parameter to FALSE, why keep it? To deprecate > it? > > > The Location property, along with PropertiesChanged, would then only be > > available if ModemManager wasn't compiled limiting the location info to a > > single user. > > Sure but I think you should only keep it for backwards compatibility and mark > it as deprecated. At some point in future when we are sure that nobody will use > it against older version of geoclue (the only user of this API AFAIK), you can > just drop it. There may be distros or more likely custom systems which don't use geoclue and therefore they may not need the single-user limitation and can benefit from direct location property updates...
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Now, the good news is that restricting location info to a single user is doable > > > without many changes in MM. > > > > > > Geoclue doesn't use the info provided by the PropertiesChanged signal; it just > > > uses that signal to get notified about when a new location info is available, > > > and then uses the GetLocation() method. We can update the MM interface to > > > provide a new signal "LocationUpdated", without parameters, which will just > > > notify peers that a new location is retrievable, and for that GetLocation() > > > needs to be used. > > > > > > So, when configuring the MM build to limit location info to a single user > > > (which distros will be able to choose), we would also always forbid setting > > > "signals-location" to TRUE during Location.Setup(). If that is never set to > > > TRUE, the Location property will never get updated (and therefore no > > > PropertiesChanged emitted to all peers). > > > > If its not allowed to set that parameter to FALSE, why keep it? To deprecate > > it? > > > > > The Location property, along with PropertiesChanged, would then only be > > > available if ModemManager wasn't compiled limiting the location info to a > > > single user. > > > > Sure but I think you should only keep it for backwards compatibility and mark > > it as deprecated. At some point in future when we are sure that nobody will use > > it against older version of geoclue (the only user of this API AFAIK), you can > > just drop it. > > There may be distros or more likely custom systems which don't use geoclue and > therefore they may not need the single-user limitation and can benefit from > direct location property updates... Which software other than geoclue on those systems uses those? If there is (going to be) any, it can just use the new API and MM on those distros just wont authorize the access. I don't see any need to have two different API for authorizing and non-authorizing access.
I have a branch that introduces a new --with-limited-location-user=[USERNAME] build option. When this option is used, the Location property in the location interface will be fully unused. The only way to get the location info is through GetLocation(). Also, processes running as [USERNAME] will be allowed to run the Setup() and GetLocation() methods without further polkit checks. I wouldn't mind some review: "aleksander/limited-location-user" Note: this branch won't go into the next stable 1.4 release, which should happen one of these days... As soon as 1.4 is released we'll start merging all this new stuff.
(In reply to comment #10) > I have a branch that introduces a new --with-limited-location-user=[USERNAME] > build option. When this option is used, the Location property in the location > interface will be fully unused. The only way to get the location info is > through GetLocation(). Also, processes running as [USERNAME] will be allowed to > run the Setup() and GetLocation() methods without further polkit checks. Cool. > I wouldn't mind some review: > "aleksander/limited-location-user" Put patches here if you want reviews. :) git-bz is your friend!
Moving bugreport to the new ModemManager bugzilla in fd.o; summarized the issue there as well. Please subscribe to the new bugreport to get new updates. https://bugs.freedesktop.org/show_bug.cgi?id=85036 Closing this report as NOTGNOME.