Version: KDE 3.91.0 (using KDE Devel) Installed from: Compiled sources Compiler: gcc-c++-4.1.2-12 Fedora 7 OS: Linux Recent GNU/Linux distributions, such as Fedora 7, are using a tool called ConsoleKit to keep track of logged-in users. When built with ConsoleKit support, HAL grants or rejects device access privileges based on the presence or absence of a valid ConsoleKit cookie for a local session. Therefore, KDM needs to register itself with ConsoleKit for things like USB automounting to work in current distributions. See also: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228111 Fedora currently ships a patch I did against KDE 3.5.6/3.5.7 which adds this support. (Thanks also go to S.Çağlar Onur who fixed a segfault in my original patch.) I now ported the patch to 3.91.0 (I'll rediff against the trunk if it doesn't apply cleanly) and am seeking for upstream inclusion into 4.0. What the patch contains: * functions to communicate with ConsoleKit using the low-level D-Bus interface lifted from GDM (under the GPL, by William Jon McCann) and ported to work with KDM, * added CMake logic to detect the low-level D-Bus interface (KDE currently only checks for QtDBus) - note that the patch only needs dbus-devel to build, not ConsoleKit-devel or hal-devel, * integration with KDM code, conditionalized under #ifdef WITH_CONSOLE_KIT. What is currently missing is a way to turn this feature on or off with a CMake option, currently the #define WITH_CONSOLE_KIT is hardcoded in dm.h. The KDE 3 version of the patch is being used successfully in production in Fedora 7. The KDE 4 version isn't much different and has been verified to compile, but some testing won't hurt.
Created attachment 21120 [details] The current patch (KDE 4 version) And here's the patch I mentioned. I'm attaching only the KDE 4 version because KDE 3 is now in bugfix-only mode as I understand it.
Could we have some feedback on: * whether the feature is considered desirable for KDE in the first place and * the current implementation? Should I send this to a mailing list for discussion? kde-devel? kde-core-devel? Somewhere else?
the integration into the existing structure sucks. ;) anyway, the first question that comes to mind is: why not a PAM module? if you need some information that is not available in the currently provided context, we can try to work this out.
The ConsoleKit folks say a PAM module is the wrong approach for a graphical login manager. See for example this thread: http://lists.freedesktop.org/archives/hal/2007-February/007363.html In particular the post from David Zeuthen: [snip] > Plus, you really want your login manager to have native ConsolKit > support (rather than a PAM module) so you can feed in more hints.. this > PAM module is more of a stop-gap solution because the RH util-linux > maintainer wanted it that way for /bin/login. Sure, this PAM module > might be useful too for wdm, kdm but I think the position of the > ConsoleKit team (or me at least; can't speak for Jon) is that you want > to call into ConsoleKit from your app... heck, you already have to patch > e.g. wdm to set/unset these environment variables.
i'm sort of with the util-linux guy. it's right that some env vars need to be set (well, actually, nothing except xauthority is missing, afaict - the vt can be queried from the x server), but that might have wider use anyway. oh, right - it is said in that thread, too. the "PAM sucks too much" argumentation doesn't make sense - get in touch with the novell guys who are the (only?) maintainers and fix it. so far for stop-gap solutions ...
But what do we have to gain by setting more environment variables and writing a PAM module or patching the existing one to handle the extra variables over just calling ConsoleKit directly using D-Bus? The code to do the latter is already written, it's how GDM does it, it's how KDM in Fedora 7 does it, and I don't see what the extra indirection through a PAM module would bring us.
you don't really grok the concepts of abstraction and modularity, huh? ;) and argumentum ad populum does not work for me.
William Jon McCann has explained the rationale for the ConsoleKit design at GUADEC, here are his slides: http://people.freedesktop.org/~hughsient/temp/guadec-multiuser.pdf
Regardless, a solution is needed, and it looks like more discussion is warranted... so taking this to (something like) kde-core-devel is probably a good idea at this point.
in this context, deep integration makes a lot of sense, indeed. your patch isn't anything like that, though. it's just a poor hack that should be better done via PAM. ;) this wish depends on bug #89023, bug #59353, bug #103046 ... sort of all the same.
The goal of my patch wasn't to implement GDM-style fast user switching in KDM, but to make KDM with its existing behavior "play nice" with the ConsoleKit infrastructure. I did, however, port all of the ConsoleKit integration code from GDM, so the code needed to communicate with ConsoleKit for fast user switching if/when it gets implemented is already in consolekit.c (see unlock_ck_session), just not used. Note that fast user switching probably also needs support from KScreensaver, not just KDM. (gnome-screensaver communicates with ConsoleKit to support the session locking and unlocking for fast user switching.)
and this mere "playing nice" is better served via PAM. period. if you want this patch to be a basis for anything, you have to a) fix the license and b) fix the coding style. but don't expect integration any time soon anyway.
The GPL is compatible with the X11 license and the KDM frontend is already under the GPL, so what's the problem with GPL code in the backend?
the backend is supposed to stay x11 for free exchange with xdm (even though this hasn't happened for some years now).
fyi, http://fedoraproject.org/wiki/Releases/FeatureRemovePAMConsole
It shall be noted that this is the next step (which is going to be taken for Fedora 8 if everything goes as planned), which probably isn't of direct relevance to KDM (it's just a move from a mix of the old pam_console setup and the new ConsoleKit+HAL setup to only the new one). What went into Fedora 7 already (and what we're patching KDM for) is this: http://fedoraproject.org/wiki/Releases/FeatureFastUserSwitching (As already discussed above, we don't have GDM-style fast user switching really working in KDM yet, but we did implement the parts needed to keep HAL working in KDM sessions.)
Created attachment 22250 [details] Updated consolekit-kdm patch This is an updated version of the patch: * applies against kdebase-workspace 3.96.1 * includes Mandriva's fixes for xdmcp not working, see: https://bugzilla.redhat.com/show_bug.cgi?id=243560 http://qa.mandriva.com/show_bug.cgi?id=35502 http://www.mandriva.com/security/advisories?name=MDKA-2007:110
Also http://qa.mandriva.com/show_bug.cgi?id=34786 (35502 is just the update tracker, not the actual bug report).
Created attachment 23030 [details] kdm consolekit support patch for 3.x series
Also http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=450603 -------- the pkg-utopia team is going to upload a hal version to experimental, which has compiled in support for ConsoleKit/PolicyKit and we plan to upload this version to unstable later if nothing goes awfully wrong. This means, that hal clients in the desktop session have to be active, in order to call hal methods ( at least for stuff like Suspend()/Hibernate()/Mount() ). For this to work, the login manager has to register the session on login with ConsoleKit. Then ConsoleKit allows to track, if the desktop session is active or not. This allows to fix many longstanding issues with regard to fast user switching. gdm already has support for ConsoleKit (upstream) and the attached patch also adds ConsoleKit support to kdm. It only adds a dependency on D-Bus. Running kdm without ConsoleKit being installed is still possible, so it's perfectly safe to include this patch now, even if the current hal version in unstable doesn't require it yet. This is a heads up, to give you enough time to upload an updated kdm version, before the CK/PK enabled hal version is uploaded to unstable. So please consider to add this patch to your next upload. -------- We have included the patch to our Debian packages. It could be nice to have it included upstream too.
Created attachment 23031 [details] kdm consolekit support patch for 3.5.8 (with Mandriva fixes) WARNING: The patch you uploaded is outdated, in particular you're missing the fix by Nicolas Lécureuil from Mandriva to make XDMCP work. I'm attaching the latest version I know about, which is the current version in Fedora CVS and includes the Mandriva fixes.
Created attachment 23032 [details] kdm consolekit support patch for 4.0.0 (with Mandriva fixes) And here's a version which applies against 4.0.0.
Another update related to http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=457487 --- a/kdm/backend/consolekit.c +++ b/kdm/backend/consolekit.c @@ -461,7 +461,7 @@ add_param_string (&iter_struct, "x11-display", d->name); add_param_boolean (&iter_struct, "is-local", ((d->displayType & d_location) == dLocal)); #ifdef XDMCP - if ((d->displayType & d_location) != dLocal) { + if ((d->displayType & d_location) != dLocal && d->remoteHost) { add_param_string (&iter_struct, "remote-host-name", d->remoteHost); } #endif
That change is redundant if the fixes from Mandriva have been applied.
To be more explicit, the fix from Mandriva is the proper fix, as it actually sets d->remoteHost as it's supposed to, see the xdmcp.c hunk: + d->remoteHost = networkAddressToHostname (pdpy->connectionType, + &pdpy->connectionAddress);
Please read Debian bug report. Debian was already using "kdm consolekit support patch for 3.5.8 (with Mandriva fixes)" and d->remoteHost could still be NULL under certain circumstances (see bug report).
we applied the patch with mandriva fixes, it seems not sufficient and resulted in the updated patch. from the bug report: [...] When logging in on a remote xterminal via XDMCP kdm segfaults, causing the session to die and the greeter to be redisplayed. A backtrace of the crash (below) shows that we crash trying to create a bogus DBUS entry. Further investigation indicates the d->remoteHost is NULL. It looks like there are two different types of nonlocal session: Localy manged X servers that connect to a remote XDMCP server, and remote X terminals logging into this host via XDMCP. remoteHost is only set in the former case. The patch below fixes the crash by only adding the dbus attribute if remoteHost is known. [...]
srsly, about every major linux distro is including some variant of this patch, and that's just plain silly. Are we still blocking on just license and coding style (Re comment #12) ?
I applied a version of this patch in foresight's not yet released KDE version. I updated it for 3.5.9.
Please merge this into trunk. KDE Developers in Fedora will have serious problems because ConsoleKit is defacto now (it uses PAM) and thus No sound
s/it uses PAM/PulseAudio needs it to get access to sound devices and everything which outputs sound uses PulseAudio/
(Display managers are still expected to talk to ConsoleKit directly, not through PAM. And PAM is completely irrelevant for the sound issue, maybe you mixed up PAM with PA (PulseAudio)?)
There's also a version of the patch by Patrice Dumas which uses libck-connector instead of the consolekit.c I adapted from GDM: https://bugzilla.redhat.com/show_bug.cgi?id=430388 This is probably cleaner, and easier to properly autodetect (use ConsoleKit if and only if libck-connector is there), the drawback is that the extra dependency is used instead of just D-Bus, but that's probably not a real issue. The real issue is that that version of the patch hasn't had much testing yet.
One advantage of using libck-connector is that there's no longer the GPLed consolekit.c, and libck-connector is licensed under the MIT X11 license, so Ossi's licensing concerns should be dealt with by that solution.
Created attachment 26151 [details] KDM ConsoleKit support patch using libck-connector (against 4.0.98) I have cleaned up the version of the patch using libck-connector, and Rex Dieter has tested it. I also applied the fix by the Debian folks (from comment #23). This patch: * automatically detects whether to enable ConsoleKit support: it is enabled if and only if libck-connector is detected (it can be overridden with -DWITH_CkConnector=off, as I'm using macro_optional_find_package). * uses macro_log_feature to report on the above. * contains no GPL code, it's all X11-licensed. * doesn't duplicate ConsoleKit interfacing code, reuses the (X11-licensed) ck-connector library instead. Therefore, I think this patch should be acceptable for upstream merging. Please take into account that most, if not all, major distributions are using some variant of this patch (or the older revision which used the low-level D-Bus interface only and had the ConsoleKit interfacing code copied from GDM) and that vanilla KDE no longer works properly on all those distributions, because ConsoleKit registration is required for several core mechanisms (e.g. HAL, PulseAudio, ...) to work.
(The patch is against 4.0.98, it should apply against both 4.1 branch and 4.2 trunk, probably 4.0 too. But by the freeze policies, I think only 4.2 can be considered for upstream merging.)
And I almost forgot: Thanks to Patrice Dumas for the original revision of the libck-connector-based version of the patch!
my very first sentence still applies. ;) look at the way pam is integrated; anything else is just wrong. the coding style does not match. i will fix that myself if you insist.
Created attachment 26177 [details] KDM ConsoleKit support patch using libck-connector (against 4.0.98, actually tested) Oops, we accidentally tested an old version. (Sorry, we really thought we had tested the new one successfully, but it turns out we didn't, oops.) This version is actually tested (I swear!), I fixed 2 more bugs (a segfault and an incorrect parameter name which caused ConsoleKit not to give out the device permissions), now it's working fine. > the coding style does not match. i will fix that myself if you insist That would be very helpful indeed, because I'm not sure I can find all the coding style mismatches. :-( And yes, I insist that the patch is needed. :-)
> And yes, I insist that the patch is needed. :-) > i don't doubt that. ;) you're not listening properly, though. the bottom line is, if you are touching session.c, you are doing something wrong. btw, the WIN32 test in FindCKConnector seems pretty pointless ...
Created attachment 28377 [details] KDM ConsoleKit support patch using libck-connector (against trunk or 4.1.2) So here's a version of the patch which does not touch session.c at all, but only client.c (and a one-line fix to xdmcp.c), as requested. I also made some minor adjustments to match the KDM backend's coding style. The behavior should be strictly equivalent to the previous version of the patch. The diff was made against 4.1.2, but I verified that it also applies cleanly (no rejects, no fuzz, even no offsets) against the current trunk. I have tested it in a Fedora kdebase-workspace-4.1.2 RPM (I replaced the previous ConsoleKit patch by this one), it works (builds and runs) fine (as confirmed by ConsoleKit's ck-list-sessions tool).
Created attachment 28378 [details] KDM ConsoleKit support patch using libck-connector (against trunk or 4.1.2) Same as above, but without the -Wwrite-strings warning (just a s/char \*remote_host_name/const char *remote_host_name/). ;-) The patch now introduces no warnings.
now we are getting somewhere ... ;) - the dbus check should use pkgconfig, too - extending the #ifdef in hunk 1207+ is wrong. it must be complementary to code in verify() - the if(p) in hunk 1216+ makes no sense. basically for the same reason as above. - !is_local && !td->remoteHost is most likely insatisfiable, too. pls check. - afaik, the generic error messages should not mention dbus-induced failures, as those are guaranteed to return a proper error code. btw, doesn't the connector provide some function to stringify its own error messages? can there be non-dbus errors at all? - camelCase is preferred to underscore_madness ;) there are some other minor stylistic issues which are harder to explain than to fix, so i'll do that myself prior to checkin.
> - the dbus check should use pkgconfig, too IIRC, I didn't use it because the 2 include dirs (DBUS_INCLUDE_DIR and DBUS_ARCH_INCLUDE_DIR) made the default cmake pkgconfig macros fail and so I followed the path of least resistance, but there must be a way to use pkgconfig anyway, I'm rechecking, I'll change it if I can. > - extending the #ifdef in hunk 1207+ is wrong. it must be complementary to code in verify() Uh, I don't understand, the pwent is needed by both the PAM and AIX code and the ConsoleKit code so why should I be computing the exact same pwent twice? I can easily change it, but I don't understand what's wrong with the way I implemented it. > - the if(p) in hunk 1216+ makes no sense. basically for the same reason as above. See above. > - !is_local && !td->remoteHost is most likely insatisfiable, too. pls check. That's what I thought too, but the Debian folks assured me otherwise, see comment #23 and comment #27. > - afaik, the generic error messages should not mention dbus-induced failures, > as those are guaranteed to return a proper error code. So should I just change it to a generic "Cannot close CK session\n"? > btw, doesn't the connector provide some function to stringify its own error > messages? Unfortunately no, it doesn't. > can there be non-dbus errors at all? I don't know, so I have to assume the worst. > - camelCase is preferred to underscore_madness ;) Uh, sorry, I tried to match the surrounding code, which uses underscores in a lot of places. :-) But I'm going to change my variable names to camelCase.
Oops, I understand now what you mean by those: > - extending the #ifdef in hunk 1207+ is wrong. it must be complementary to code > in verify() > - the if(p) in hunk 1216+ makes no sense. basically for the same reason as > above. (If not PAM nor AIX, p is already computed by verify, so I'm actually computing it twice, and if (p) is always satisfied, right?) /me needs more sleep, sorry. :-) I'll fix this.
> (If not PAM nor AIX, p is already computed by verify, so I'm actually computing > it twice, and if (p) is always satisfied, right?) > yup :) > the Debian folks assured me otherwise, see comment #23 and comment #27. > hmm. i wonder whether any of these cases is actually legitimate. "foreign" and "local feedback" (Xnest -query localhost) displays should be investigated, and the findings documented or the other code fixed. > So should I just change it to a generic "Cannot close CK session\n"? > sounds kind of reasonable. > I don't know, so I have to assume the worst. > i'd seek some definite clarification on that. > I tried to match the surrounding code, which uses underscores in a > lot of places. :-) > yeah, the naming is a mess. i'm trying to standardize incrementally.
Created attachment 28418 [details] KDM ConsoleKit support patch using libck-connector (against trunk/4.1.2, updated) Here's a version of the patch addressing your comments: * FindDBus.cmake now uses pkg-config, through the new FindPkgConfig. This also solves the problem with D-Bus's 2 include directories. * I also changed FindCkConnector.cmake to use FindPkgConfig instead of the old UsePkgConfig (also avoids having to hardcode the "/ConsoleKit/ck-connector" part of the includedir). * I removed the #ifdef extension (which was computing p a second time when verify() already did it) and the useless if (p) check. * I added a comment for the seemingly-redundant !is_local && td->remoteHost check pointing to the relevant Debian bug report. * The fallback error messages for when dbus_error_is_set is false for some reason are now less verbose. * I renamed all the variable names I added to camelCase. (Of course I can't change the names used by D-Bus and libck-connector nor pwent's pw_uid.) Tested, builds, runs, registers CK session successfully.
Ping? The 4.2 hard feature freeze is approaching, we need this merged before. (And yes, it's in the 4.2 feature plan, Rex Dieter added it.)
Created attachment 28549 [details] fixed partial patch i knew what i'm getting into, so i procrastinated. ;) i knew that the remoteHost stuff is fishy, just not how much. :} this patch compiles, but is otherwise untested. have fun with it. the case that would legitimately use remoteHost (the local chooser mode, "remote login" in the greeter) needs to be handled completely separately (yes, outside client.c this time ;).
So can we please commit it now (with your changes)? I really don't want to miss the freeze and get told it now has to wait until 4.3! And it would be helpful if you explained what exactly was wrong with the way remoteHost was handled. Because "fishy" is a bit vague. ;-)
stop panicking. this is clearly a bugfix, right? ;) everything is wrong with the remoteHost usage. grep for it, get a grip of what it is meant to do (you'll need that to implement the second part anyway).
But can we merge the current code now? The fact that this: > the case that would legitimately use remoteHost (the local chooser mode, > "remote login" in the greeter) needs to be handled completely separately (yes, > outside client.c this time ;). is missing isn't really a blocker bug, is it?
Oh, and for sessions where KDM is local and the session is remote (is this what remoteHost is used for? Looking at the code and your comment #49, I think it is, but I'm not 100% sure I understand), ConsoleKit probably doesn't really need to know about that session anyway. ConsoleKit is used to enumerate sessions on the local machine to give out permissions to those sessions (e.g. device permissions and PolicyKit-controlled application permissions), so as far as I know sessions which are actually on the remote machine aren't really interesting to ConsoleKit anyway.
> ConsoleKit probably doesn't really need to know about that session anyway. > maybe. it could be useful for fast user switching ... if there was a way to switch back to a local session. maybe some day. if you want me to commit, you have to say whether my patch works. ;)
Hmmm, to really test this, we'll need some XDMCP setup. I can have a try later today or tomorrow with my 2 machines (I have stuff for the university to do right now), hopefully that'll allow some testing. But I won't complain if somebody with more experience with XDMCP helps test this. ;-) (And I'm worried that the release team will kill us if we sneak this into 4.2 now, we're already late. ;-( )
SVN commit 886022 by ossi: add rudimentary support for ConsoleKit. this is quite a crutch - it is so poorly integrated that one could just as well do it with PAM (if the respective module wasn't that braindead). BUG: 147790 A cmake/modules/FindCkConnector.cmake A cmake/modules/FindDBus.cmake M +11 -0 kdm/CMakeLists.txt M +4 -0 kdm/backend/CMakeLists.txt M +96 -0 kdm/backend/client.c M +3 -0 kdm/config-kdm.h.cmake WebSVN link: http://websvn.kde.org/?view=rev&revision=886022