GNOME Bugzilla – Bug 365375
support for ConsoleKit
Last modified: 2007-02-12 04:19:59 UTC
In thinking about bug #311623 it seems that instead of adding that kind of interface to GDM it would be better make it separate and somewhat lower level. Some of the reasons for this: * So it may be used by system level daemons such as HAL and PolicyKit * So that sessions are defined at a more desktop agnostic/neutral level (ie. can be used by KDM etc) * So it can be used even when a display manager isn't present (on a server) * So not to add (possibly) unnecessary complexity and instability to GDM One way to think about this kind of interface is a more sophisticated utmp. So, we've made this thing called ConsoleKit: http://gitweb.freedesktop.org/?p=ConsoleKit.git;a=summary Since the interface to this toolkit is through D-Bus adding support for it to GDM will not add a hard dependency - unless we want it to. I'll attach a patch that adds basic support for this.
Created attachment 75463 [details] [review] patch Please let me know what you think. Thanks.
Okay, it looks like you are using D-Bus to make visible the following information: session open: + add_param_int (parameters, "user", pwent->pw_uid); + add_param_string (parameters, "x11-display", d->name); + add_param_string (parameters, "host-name", d->hostname); + add_param_boolean (parameters, "is-local", d->attached); uid, display name, hostname and whether they are logged into the console. session close: Here you just pass the cookie? Wouldn't you want information about which session is closed? + res = dbus_g_proxy_call (proxy, + "CloseSession", + &error, + G_TYPE_STRING, + &cookie, + G_TYPE_INVALID, + G_TYPE_INVALID); session unlock: Looks like the UID and something from &sessions. Why do you need to pass the &sessions info along? + res = dbus_g_proxy_call (proxy, + "GetSessionsForUser", + &error, + G_TYPE_UINT, + pwent->pw_uid, + G_TYPE_INVALID, + dbus_g_type_get_collection ("GPtrArray", DBUS_TYPE_G_OBJECT_PATH), + &sessions, + G_TYPE_INVALID); Also, might be useful to make the various gdmflexiserver --command data available via D-Bus. Like a list of active sessions, GDM Version, Server list/details, get_config, update_config, greeterpids, query_logout_action, set_logout_action, VT management, etc. Also, you didn't respond to my questions in bug #311623. I'd like to hear your ananalysis on how this impacts GDM security. How does adding D-Bus as a dependency affect GDM's security model? Are new dependant interfaces appropriate for use in a program like GDM, where any exploit could perhaps allow a user to access a system without username/password? Do the maintainers of these modules think that usage in programs like GDM is appropriate? If so, do they document their awareness/approval of such usages? A few comments. 1) I'd prefer to see the ConsoleKit in a separate file rather than clutter up the slave.c code with logic that is specific to these new interfaces. Perhaps put it in daemon/gdmconsolekit.c. 2) Don't you think this should be configurable, so you can --enable or --disable with configure since not all systems may be using this interface. Would be nice if the new dependencies (D-BUS) are only required if this feature is enabled. Not sure if it should be enabled or disabled by default. Then we would only compile the gdmconsolekit.c file if enabled, and use #ifdef's in the slave.c code so that the functions only get called if enabled. 3) I think that this feature should be documented in docs/C/gdm.xml. The D-Bus/ConsoleKit interfaces that GDM depends upon should be clearly documented, how this works, and how users are expected to use it. Probably worthy of a new <sect1>...</sect1> in the gdm.xml file? I won't accept a patch like this without docs. Does this make sense? If so, feel free to commit when your patch is done. Commit to CVS head.
Thanks for the review Brian. (In reply to comment #2) > session close: > > Here you just pass the cookie? Wouldn't you want information about > which session is closed? > > + res = dbus_g_proxy_call (proxy, > + "CloseSession", > + &error, > + G_TYPE_STRING, > + &cookie, > + G_TYPE_INVALID, > + G_TYPE_INVALID); The cookie is unique and can be used to identify the session. > session unlock: > > Looks like the UID and something from &sessions. Why do you need to > pass the &sessions info along? > > + res = dbus_g_proxy_call (proxy, > + "GetSessionsForUser", > + &error, > + G_TYPE_UINT, > + pwent->pw_uid, > + G_TYPE_INVALID, > + dbus_g_type_get_collection ("GPtrArray", > DBUS_TYPE_G_OBJECT_PATH), > + &sessions, > + G_TYPE_INVALID); sessions is the "out" parameter. It is the output of the GetSessionsForUser method call. > Also, might be useful to make the various gdmflexiserver --command > data available via D-Bus. Like a list of active sessions, GDM Version, Server > list/details, get_config, update_config, greeterpids, query_logout_action, > set_logout_action, VT management, etc. ConsoleKit includes the ability to list active sessions and perform some VT management. For these other things, if they are really needed, I think it is best to add a GDM specific D-Bus interface on the system message bus. > Also, you didn't respond to my questions in bug #311623. I'd like to hear your > ananalysis on how this impacts GDM security. How does adding D-Bus as a > dependency affect GDM's security model? Are new dependant interfaces > appropriate for use in a program like GDM, where any exploit could perhaps > allow a user to access a system without username/password? Do the maintainers > of these modules think that usage in programs like GDM is appropriate? If so, > do they document their awareness/approval of such usages? D-Bus has been designed with security in mind and has been used in daemons running as root for a couple of years now. A 1.0 release is about to happen and it will be officially announced as frozen. The system message bus has the ability to work with OS security frameworks like SELinux. So, I don't think adding it has any effect at all on GDM security. I think that using the GDM socket interface is much more of a risk. There is no risk of login without a password because the authentication code and child process creation is unchanged. None of the information that is used in the communication with ConsoleKit originates from user input. I am certain that the D-Bus hackers think that usage in GDM is appropriate. It is already used by very low level daemons like HAL, NetworkManager, etc and is planned for inclusion in Xorg. > A few comments. > > 1) I'd prefer to see the ConsoleKit in a separate file rather than > clutter up the slave.c code with logic that is specific to these > new interfaces. Perhaps put it in daemon/gdmconsolekit.c. > > 2) Don't you think this should be configurable, so you can --enable > or --disable with configure since not all systems may be using > this interface. Would be nice if the new dependencies (D-BUS) > are only required if this feature is enabled. Not sure if it > should be enabled or disabled by default. Then we would only > compile the gdmconsolekit.c file if enabled, and use #ifdef's > in the slave.c code so that the functions only get called if > enabled. > > 3) I think that this feature should be documented in docs/C/gdm.xml. > The D-Bus/ConsoleKit interfaces that GDM depends upon should > be clearly documented, how this works, and how users are expected > to use it. Probably worthy of a new <sect1>...</sect1> in the > gdm.xml file? I won't accept a patch like this without docs. > > Does this make sense? If so, feel free to commit when your patch > is done. Commit to CVS head. OK. Will attach new patch.
Created attachment 75736 [details] [review] updated patch I didn't make the D-Bus dependency optional since I think we want to use it for stuff even when ConsoleKit isn't used. Does this look OK?
Looks very good. Please commit. Though, please update the docs to mention that ConsoleKit can be configured via the configure option.
Sweet. I've updated the docs as requested and changed the Makefile.am so that the gdmconsolekit.{c,h} aren't built if not needed and committed it. However, perhaps these files should go in EXTRA_gdm_binary_SOURCES to make sure they are included when dist'ed?
Yes, please update the Makefile's so that "make distcheck" works. Always good to do this when adding a new file to the build.
I guess I didn't test that because make distcheck was already broken due to gdmsetup so I've committed a patch that hopefully fixes both. Thanks Brian!
*** Bug 311623 has been marked as a duplicate of this bug. ***
Why does your daemon/Makefile.am change refer to the verify-pam code instead of gdmsetup as you say, which is in the gui directory? I assume this is a typo, but I'd appreciate it if when you commit a change to CVS head that you also attach the patch to the bugreport so it can be reviewed more easily. Could you update this bugreport with the additional changes you made to fix distcheck? Thanks.
I made these changes: 1. http://cvs.gnome.org/viewcvs/gdm2/daemon/Makefile.am?r1=1.38&r2=1.39 2. http://cvs.gnome.org/viewcvs/gdm2/Makefile.am?r1=1.45&r2=1.46 Sorry, I figured that when you said 'please update the Makefile's so that "make distcheck" works' you just wanted me to commit the changes. Part 1. didn't change anything with verify-pam. CVS just chose to represent the patch in a way that looks like it I guess. Part 2. is necessary because distcheck complains that distclean leaves that file behind. Actually looks like a bug in the naming of the generated file. Makefile.am has this: gdmsetup: $(srcdir)/gdmsetup-security.in sed -e 's,[@]sbindir[@],$(sbindir),g' <$(srcdir)/gdmsetup-security.in >gdmsetup It originates here: http://cvs.gnome.org/viewcvs/gdm2/Makefile.am?r1=1.44&r2=1.45 I'm not sure what you're trying to do there...
John: Thanks for explaining. I got confused when you refered to gdmsetup since I think of gui/gdmsetup when you said that, but the patch didn't seem to modify the gui/Makefile.am. That's why I asked you for more information. Very good catch. Looks like this weirdness was introduced with this patch by Julio Vadel. See attachment here: http://bugzilla.gnome.org/show_bug.cgi?id=336364 This patch was an effort to clean up the Makefiles so that we didn't do all sorts of hackish expanding of the installation directories in the configure script and propegate the values into the Makefiles in a more sane way. Overall, I think this patch is good, but I think this part is coded incorrectly. It looks like gdmsetup-security.in is an incomplete copy of gui/gdmsetup.desktop.in.in. It looks like aside from running sed to create the gdmsetup output file, we don't actually install this file. Note that we do install gdmsetup-pam into $(DESTDIR)$(PAM_PREFIX)/pam.d/gdmsetup, but this is different. I think we should probably remove gdmsetup-security.in from the module and remove the references to it in the Makefile.am. Seems like some extra cruft that got into the code. Do you want to take care of this and commit? And attach the patch to the bug as you close it.
William - I am reopening this bug because I notice that GDM fails to run properly if consolekit is not installed. It seems that configure builds with it turned on by default, but if you don't actually have ConsoleKit on your machine, this enabled code causes GDM to always fail to authenticate any user typed into the login screen. So you can't log in. If you rebuild with the configure option set to no, so it doesn't build with support, then it works okay. However, I think either configure should be smarter to turn it off if ConsoleKit isn't on the system, or at runtime it shouldn't fail to authenticate users if ConsoleKit is not present. It should fail more gracefully. What do you think?
Can you please provide some details on how it fails to authenticate any user? I think the logic in configure is fine.
At least on Solaris, when you type in username/password, it always fails to authenticate. So even if you type a valid username/password, it acts like you typed in an invalid name/password. If you build/install GDM on a system with no ConsoleKit on the system, you don't see this problem? Why would the configure test pass and say "yes" on Solaris when we don't have ConsoleKit on the system? Looking at configure.ac, it just always assumes yes unless you turn it off.
No problem here. Here's a test case that is equivalent to not having console kit: 1. SSH into the system 2. sudo /sbin/service ConsoleKit stop 3. sudo gdm-restart 4. Enter username/password It works for me. There is no need to do a test for console kit at build-time since one of the properties of dbus is that existance of a service can be detected at run-time. I did notice that we are still setting XDG_SESSION_COOKIE when console kit is not available - I'll create a new bug for that. We should probably address the problem you are seeing in a new bug too.
Thanks for verifying that this is a Solaris specific problem. Perhaps I'll have to work with our HAL guru here at Sun to see if the problem is to be fixed there. Also we're using an older version of D-Bus (0.62) so maybe that's the problem too.
I just verified that now compiling on Solaris works okay even when console-kit isn't specified to be "no" when configuring. So closing this bug now. Thanks, I think the fixes you did for bugs #383866 and #400793 corrected this issue.