GNOME Bugzilla – Bug 707983
[review] dcbw/kill-at-console: remove usage of at_console and clean up session management
Last modified: 2014-05-05 06:11:45 UTC
Current issues: 1) dbus at_console permissions aren't granular enough; it prevents ssh-ed users from even checking network state 2) dbus at_console permissions are almost always confusing to users; they dont' know why a session is not at_console 3) we already have session management inside NM that covers everything dbus at_console does 4) not all distros used at_console anyway 5) we haven't kept src/org.freedesktop.NetworkManager.conf updated WRT new dbus interfaces that we've added for new device types anyway; best to not require that updating anyway So what we should do here is to consolidate all the dbus permissions for !root and get rid of at_console, and rely on ConsoleKit and/or systemd for session handling. We must be very careful with this, however, since some things that are currently blocked by dbus at_console may not be protected by PolicyKit/systemd/ConsoleKit authorization. First, we need to audit any D-Bus method calls and writable properties to ensure that they are adequately protected by session management and/or PolicyKit. This should take into account whether the property/method should be accessible to non-root users (eg, Sleep/Wake should *not* be accessible to non-root users) and whether access to it is correctly protected by a PolicyKit permission. Second, once we are satisfied that everything is protected, we can consolidate the at_console and normal sections of src/org.freedesktop.NetworkManager.conf and allow non-root users to call any interfaces that should not be root-only, instead of the current split where at_console can talk to everything but !at_console has a more restricted set. Third, we should fix up the systemd session code to correctly return errors, instead of the current bits that don't: ret = sd_uid_get_sessions (uid, TRUE, NULL) > 0; if (ret < 0) { and we should also fix up the TRUE/FALSE arguments to sd_uid_get_sessions() since that was changed to 'int' upstream; TRUE should be changed to '1' and FALSE should be changed to '0' to be 100% clear what NM wants. This is not a behavior change, just a code clarity change.
Created attachment 265867 [details] perms.txt Analysis of all D-Bus interfaces, their methods, and properties. Since we are removing the at_console permissions, we need to verify that all methods and properties are correctly protected by other mechanisms, or need no protection.
Changes pushed upstream to dcbw/kill-at-console
We'll also want to implement a PolicyKit agent in 'nmcli' and possible 'nmtui' as well; details on that: (12:43:54 PM) dcbw: davidz: while you're around, for PK, if you SSH into a machine and try to do something that requires authorization, what happens? 12:50 (12:53:46 PM) davidz: dcbw: most likely nothing since there will likely not be an Authentication Agent around to ask you to authenticate (12:54:07 PM) davidz: dcbw: (it's possible to write one that works with ssh - e.g. by using /dev/console - but I don't think anyone ever did this.) (12:54:44 PM) davidz: dcbw: however, some polkit apps including pkexec(1) are smart about this and then registers their own polkit authentication agent and thereby prompts you using a textual interface (12:54:55 PM) dcbw: davidz: ok, so that would be the suggested route? 12:55 (12:55:12 PM) davidz: if you're writing a polkit app, yeah (12:55:19 PM) davidz: it's actually very easy to do so (12:55:20 PM) dcbw: davidz: if we know that a command-line utility will trigger operations that require PK authorization, that utility should register a pk agent to handle the requests? (12:55:26 PM) davidz: just use pkttyagent(1) (12:55:27 PM) davidz: http://www.freedesktop.org/software/polkit/docs/latest/pkttyagent.1.html (12:55:38 PM) davidz: IIRC systemd uses that (12:56:03 PM) dcbw: davidz: did udisks ever do anything like that? just looking for samples (12:56:09 PM) davidz: I *think* you can even get away with just launching it at startup... if there's an exiting GUI authentication, polkitd will prefer that one to your (12:56:38 PM) davidz: dcbw: yeah, see http://cgit.freedesktop.org/udisks/tree/tools/udisksctl.c (12:57:13 PM) dcbw: was grepping udisks sources but didn't know what to search for (12:57:16 PM) dcbw: thanks (12:57:21 PM) davidz: dcbw: that's probably the best way to do it since it doesn't waste a process (12:58:34 PM) davidz: dcbw: so, yeah either just copy setup_local_polkit_agent() or spawn pkttyagent(1) yourself (but be careful to wait on the notification fd for when setup has completed) (12:58:45 PM) dcbw: davidz: looks like the agent gets registered *after* the method call is made? is that to ensure that it only handles its own requests? (12:58:54 PM) davidz: also remember to use --fallback (12:59:27 PM) davidz: dcbw: yes, the udisks D-Bus service has a way to convey that "authorization was denied but if there was an authentication agent available maybe it would have worked"
http://cgit.freedesktop.org/udisks/tree/tools/udisksctl.c is probably the best example to work from here; it registers a text agent that runs in a separate thread and shouldn't be that hard to integrate into nmcli. nmtui on the other hand might want to implement a PK agent that fits into the pseudo-windowed mode better...
(In reply to comment #2) > Changes pushed upstream to dcbw/kill-at-console ACK, pushed one !fixup
> sessions: fix return value handling for sd_uid_get_sessions() (bgo #707983) >- return ret > 0 ? TRUE : FALSE; >+ return !!ret; I don't think that's an improvement, clarity-wise. (If you want to get rid of the explicit TRUE/FALSE-ing, I think "ret > 0" or "ret != 0" would be better than "!!ret".) And how about renaming "ret" to "num_sessions"? > dbus: kill at_console usage in permissions (bgo #707983) (rh #979416) >+ <!-- Devices (read only properties, no methods) --> "read-only" should be hyphenated (in several places). >+ <!-- Root-only functions; should be denied-by-default but >+ add explicit denials to be paranoid --> It seems wrong to be paranoid about these when we aren't (and can't be) paranoid about the Device and Device.Wireless methods. (Meaning, I'd just drop this part.) > policy: allow inactive (remote/SSH) sessions to perform some actions I would have expected modify.system to be "<allow_any>@NM_MODIFY_SYSTEM_POLICY@</allow_any>" now...
Fixed up and repushed. I took Thomas' fixup, and all of DanW's fixups, except for removal of the root-only permissions. New patch "core: enforce permissions for SetLogging" too. My original comment in the dbus permissions file was wrong there. We cannot remove the explicit denials, because we are protecting specific method calls on interfaces that normally need to be accessible, so there's already an explicit allow send_destination/send_interface call above to allow the interface that SetLogging and Sleep are on. Same goes for LoadConnections and ReloadConnections. Technically yes, we can protect all of these with UID checks internally, except for the Sleep() method (due to legacy issues with pm-utils). I adjusted the comment and I think it's worthwhile to keep them there just to err on the side of caution.
OK. Reading the commit messages, I'm confused about exactly what the effect of "kill at_console usage" is, if "no permissions are being relaxed" and it doesn't yet "allow inactive sessions to perform some actions"... Is is just that at that point in the series of commits, at_console has become fully redundant with the other checks?
(In reply to comment #8) > OK. > > Reading the commit messages, I'm confused about exactly what the effect of > "kill at_console usage" is, if "no permissions are being relaxed" and it > doesn't yet "allow inactive sessions to perform some actions"... Is is just > that at that point in the series of commits, at_console has become fully > redundant with the other checks? Correct. at_console was a dbus-ism from long long ago that uses pam_console for determining whether some user is "local" or not, which is not quite the same as what PolicyKit does. In any case, at_console vs. not-at_console is already covered 100% by the more flexible PolicyKit allow_active/allow_inactive permissions, assuming that we authorize all necessary D-Bus calls (which this branch ensures).
Branch merged to master after clarifying commit message per danw's request.
*** Bug 729550 has been marked as a duplicate of this bug. ***