Ubuntu
xinit package

ConsoleKit integration

Bug #199486 reported by Loïc Minier
24
Affects Status Importance Assigned to Milestone
xinit
Unknown
Medium
consolekit (Ubuntu)
Fix Released
Undecided
Michael Frey
ume-config-common (Ubuntu)
Invalid
Undecided
Unassigned
xinit (Debian)
Fix Released
Unknown
xinit (Ubuntu)
Invalid
Undecided
Unassigned
xorg (Ubuntu)
Invalid
Undecided
Unassigned

Bug Description

Hi,

xinit needs to gain ConsoleKit integration just like GDM has ConsoleKit integration as more and more parts of the GNOME/FreeDesktop desktops are relying on it to grant access to DBus services:
/etc/dbus-1/system.d/bluetooth.conf: <policy at_console="true">
/etc/dbus-1/system.d/newprinternotification.conf: <policy at_console="true">
/etc/dbus-1/system.d/nm-applet.conf: <policy at_console="true">
/etc/dbus-1/system.d/hal.conf: <policy at_console="true">
/etc/dbus-1/system.d/NetworkManager.conf: <policy at_console="true">

There are some upstream patches which don't seem to work in all cases.

This is critical for UME as it launches the hildon-desktop via startx.

Bye,

See original description
Revision history for this message
In , Zeuthen (zeuthen) wrote :

Created an attachment (id=11486)
proposed patch

Revision history for this message
In , Dbn-lists (dbn-lists) wrote :

Newer CK info here:

http://www.freedesktop.org/wiki/Software/ConsoleKit

Revision history for this message
In , Dbn-lists (dbn-lists) wrote :

Created an attachment (id=11489)
Updated patch against current git with some autotools polish

Revision history for this message
In , Julien Cristau (jcristau) wrote :

(In reply to comment #3)
> Created an attachment (id=11489) [details]
> Updated patch against current git with some autotools polish
>
 shouldn't you also check for dbus-1 and include <dbus/dbus.h>, since you're using dbus functions?

Revision history for this message
In , Loïc Minier (lool) wrote :

@jcristau: indeed, makes sense; it will work as is because ck-connector requires dbus in the .pc.

Using this patch, it seems that the session isn't made the active one; after discussion with other people, one thing which might help is that ck_connector_open_session_with_parameters should be used instead of ck_connector_open_session with some parameters from:
display-device
x11-display-device
x11-display

(probably session-type, is-local, and unix-user can be set easily as well)

Revision history for this message
In , Michael Frey (mfrey) wrote :

(In reply to comment #0)
> (not sure I've filed this against the right component, please advise)
>
> ConsoleKit [1] is used to track user desktop sessions so to work with
> ConsoleKit any process used to launch these needs to poke ConsoleKit so the
> session can be registered. Will attach a patch to xinit.c that does this.
>
> [1] :
> http://gitweb.freedesktop.org/?p=ConsoleKit.git
> http://people.freedesktop.org/~david/ConsoleKit.html (a bit outdated)
>
> Btw, this patch originated from this RH bug
> https://bugzilla.redhat.com/show_bug.cgi?id=233183
>

(In reply to comment #5)
> @jcristau: indeed, makes sense; it will work as is because ck-connector
> requires dbus in the .pc.
>
> Using this patch, it seems that the session isn't made the active one; after
> discussion with other people, one thing which might help is that
> ck_connector_open_session_with_parameters should be used instead of
> ck_connector_open_session with some parameters from:
> display-device
> x11-display-device
> x11-display
>
> (probably session-type, is-local, and unix-user can be set easily as well)
>

Having done more research on this and actually tried to update this patch to use the above mentioned call (open_session_with_parameters), this will not work.

Typically xinit is not run as a root, so the user does not have enough permissions to create a session with parameters.

I question weather or not we should try to move this call into X? X is setuid root.

Revision history for this message
Loïc Minier (lool) wrote :

Hi,

xinit needs to gain ConsoleKit integration just like GDM has ConsoleKit integration as more and more parts of the GNOME/FreeDesktop desktops are relying on it to grant access to DBus services:
/etc/dbus-1/system.d/bluetooth.conf: <policy at_console="true">
/etc/dbus-1/system.d/newprinternotification.conf: <policy at_console="true">
/etc/dbus-1/system.d/nm-applet.conf: <policy at_console="true">
/etc/dbus-1/system.d/hal.conf: <policy at_console="true">
/etc/dbus-1/system.d/NetworkManager.conf: <policy at_console="true">

There are some upstream patches which don't seem to work in all cases.

This is critical for UME as it launches the hildon-desktop via startx.

Bye,

Revision history for this message
Michael Frey (mfrey) wrote :

After some more investigation -- looks like using open_session_with_parameters is not an option. This call requires root privileges.

Need a different approach.

Revision history for this message
Loïc Minier (lool) wrote :

Might work in X instead of xinit as X is suid root.

Revision history for this message
Loïc Minier (lool) wrote :

18:32 < lool> mccann: Hey; I have a question on ConsoleKit xinit integration
              (freedesktop #Bug 12378); is this a good place/time to talk about
              it?
18:32 < mccann> sure
18:32 < lool> mccann: Basically it seems that consolekit isn't made aware of
              enough information and it requires root for this to be possible
18:33 < lool> xinit isn't usually running as root; should be attempting to do
              this in the X server instead?
18:33 < lool> Hmm Michael already summarized to the same level on the bug report
18:33 < lool> Basically, what to do now? :)
18:43 < mccann> so it is probably best to use /usr/bin/ck-launch-session instead
18:44 < lool> mccann: Hmm is this in recent consolekits?
18:44 < mccann> yes

Bug Watch Updater (bug-watch-updater)
Changed in xinit:
status: Unknown → Confirmed
Bug Watch Updater (bug-watch-updater)
Changed in xinit:
status: Unknown → Confirmed
Revision history for this message
In , Loïc Minier (lool) wrote :

Launching the session from xinit wont work and is actually harmful; instead we should start the ck session as an encapsulating client for the lifetime of the session; for example if you run your X11 client within ck-launch-session, CK should look at the DISPLAY of the calling DBus process and will provision the DISPLAY and the VT in the new CK session.

If however we were to merge this patch, then we wouldn't have any way to tell CK to update its info.

Revision history for this message
Colin Watson (cjwatson) wrote :

If it helps, here's the patch I applied to OpenSSH to implement ConsoleKit support. It does assume root access, though.

Revision history for this message
Loïc Minier (lool) wrote :

As starting a CK session from xinit is not only superfluous if we launch one later down it would actually be harmful if we want to honor running sessions in the Xsession.d snippe. Hence I'm marking this invalid for xinit. (I'm also adding the source packages which we will have to change: consolekit to provide the Xsession.d snippet and x11-common for the Xsession.options.)

Changed in xinit:
status: New → Invalid
Revision history for this message
Loïc Minier (lool) wrote :

Attaching an IRC log on #ubuntu-devel leading to these conclusions.

Revision history for this message
Loïc Minier (lool) wrote :

Michael confirmed that the general approach works; we're working on the exact integration now.

Revision history for this message
Michael Frey (mfrey) wrote :

As part of this fix, ck-launch-session needs to be back ported from 0.2.10 to the current Hardy version 0.2.3.
I have attached a debdiff patch for this.

The second part of this is to modify the ume-gui-start file to launch with ck-launch-session.

Revision history for this message
Loïc Minier (lool) wrote :

Thanks for the backport; as you might have seen, the current consolekit package has patches below debian/patches; could you please:

a) move the ck-launch-session backport to a separate patch

b) if the Makefile.in updates are the result of running automake, please move them to a separate automake patch, later in the patch stack

Otherwise, looks good!

(I'm assigning the consolekit changes to you! :-)

Changed in consolekit:
assignee: nobody → mfrey
Revision history for this message
Michael Frey (mfrey) wrote :

New patch attached that is broken out based on Loic's suggestion.

- ck-launch-session a new patch
- automake stuff a new patch
- debian changes in debdiff

Revision history for this message
Loïc Minier (lool) wrote :

@Michael: almost there:

1) I wasn't clear when I said to move things to an automake patch; I meant to please move the Makefile.in updates *only*; the Makefile.am changes do belong logically with the .c addition; the reasons I requested a separate patch for Makefile.in is to:
- keep generated data separate from source data
- this typically represents what you would send upstream, or also the "interesting" part to review for me; the Makefile.in parts aren't ever going to be submitted to an upstream nor am I interested in reviewing them if they are autogenerated
- it's very likely that conflicts happen with Makefile.in updates, having them separately means that it's easy to redo this part of the patch from scratch by simply running automake again
- it happens that multiple patches touch Makefile.am, it's easier to generate a single patch by running automake
- it helps to make sure that Makefile.in timestamps will be newer than Makefile.am timestamps once the two patches have been applied in the proper order; this is particulary important when you update configure as the files are usually ordered as configure first and configure.in/.ac second, and hence let autotools think that autoconf needs to be run (as configure.in is newer than configure) -- unless AM_MAINTAINER_MODE is used, which is the case here

2) the Makefile.in patch should be applied after the backport patch; usually patches are ordered by being named 10_foo.patch, 70_relibtoolize.patch etc., but here it's not the case :-/ You could try zzz_automake.patch.

3) Please mention you added these as a patch in debian/changelog and mention the name of the patch: this allows to lookup in changelog when a patch was added / updated / removed.

Thanks!

Revision history for this message
Michael Frey (mfrey) wrote :

Third time's a charm.

New patch based on Loic's comments above.

Revision history for this message
Loïc Minier (lool) wrote :

@Michael: looks good; as archive was frozen yesterday for beta, I'm looking whether UME can get an exception; otherwise, I'll push it to PPA (or you're welcome to!) until we may upload again, next week.

Revision history for this message
Loïc Minier (lool) wrote :

Michael: Ah we also need to ship a Xsession.d snippet to use ck-launch-session!

e.g. /etc/X11/Xsession.d/92consolekit with something like:
if [ -z "$XDG_SESSION_COOKIE" ]; then
    STARTUP="ck-launch-session $STARTUP"
fi

Revision history for this message
Loïc Minier (lool) wrote :

There's a bug in the ume-config-common which would prevent the above snippet from working and should be fixed: it should set BASESTARTUP, not STARTUP.

Changed in consolekit:
status: New → In Progress
Revision history for this message
In , Loïc Minier (lool) wrote :

We've merged the ck-launch-session in a Xsession snippet approach in Ubuntu, and it works fine.

I think it's clean enough and doesn't add a dbus dependency to xinit; also, the current patch is insufficient (as it doesn't set x11-display etc.).

The Xsession snippet might make more sense in consolekit itself or distro packaging but I guess it should be shipped whereever the dbus-launch or ssh-agent snippets are shipped.

Revision history for this message
Michael Frey (mfrey) wrote :

New patch attached that adds an Xsession startup script. The script looks for the presence of XDG_SESSION_COOKIE as well as the binary ck-launch-session. if the conditions are met ck-launch-session is appended to the $STARTUP.

NOTE:
The /etc/event.d/session upstart script for UME might need to be changed to wait for rc2 to be finished. This is because DBUS must be running in order for ck-luanch-session to work.

Revision history for this message
Loïc Minier (lool) wrote :

Removing the xorg task as I don't intend to implement a Xsession.options entry for consolekit.

Changed in xorg:
status: New → Invalid
Revision history for this message
Loïc Minier (lool) wrote :

Uploaded with minor changelog tweak; pending review as we're in freeze.

Changed in consolekit:
status: In Progress → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package consolekit - 0.2.3-3ubuntu5

---------------
consolekit (0.2.3-3ubuntu5) hardy; urgency=low

  * Fix typo in bug id in 0.2.3-3ubuntu4.

consolekit (0.2.3-3ubuntu4) hardy; urgency=low

  * Add XSession ConsoleKit integration; LP: #199486.
    - New patch, debian/patches/ck-launch-session.patch, backport
      of the ck-launch-session binary from consolekit-0.2.10.
    - New patch, debian/patches/zzz_automake.patch, update autotools generated
      files patches by the previous patch.
    - Added debian/90-console-kit, an Xsession startup script, to prepend
      ck-launch-session to $STARTUP.

 -- Loic Minier <email address hidden> Fri, 21 Mar 2008 10:22:39 +0100

Changed in consolekit:
status: Fix Committed → Fix Released
Bug Watch Updater (bug-watch-updater)
Changed in xinit:
status: Confirmed → Fix Released
Michael Terry (mterry)
tags: added: oem-services
Bug Watch Updater (bug-watch-updater)
Changed in xinit:
importance: Unknown → Medium
Bug Watch Updater (bug-watch-updater)
Changed in xinit:
importance: Medium → Unknown
Bug Watch Updater (bug-watch-updater)
Changed in xinit:
importance: Unknown → Medium
Revision history for this message
In , Jeremy Sequoia (jeremyhu) wrote :

configure.ac needs a bit more work. For example, if I pass --with-consolekit
and I don't actually have it, configure should error out, but it just silently
continues without consolekit. Other than that, it looks good. Mind updating
the patch? Feel free to email it to xorg-devel for review, or attach here and
I'll review it again. Bonus points if you use 'git format-patch' with a commit
message that I can use rather than just 'git diff'

Bug Watch Updater (bug-watch-updater)
Changed in xinit:
status: Confirmed → In Progress
Revision history for this message
In , Julien Cristau (jcristau) wrote :

> --- Comment #9 from Jeremy Huddleston <email address hidden> 2011-10-03 00:31:49 PDT ---
> configure.ac needs a bit more work. For example, if I pass --with-consolekit
> and I don't actually have it, configure should error out, but it just silently
> continues without consolekit. Other than that, it looks good. Mind updating
> the patch? Feel free to email it to xorg-devel for review, or attach here and
> I'll review it again. Bonus points if you use 'git format-patch' with a commit
> message that I can use rather than just 'git diff'
>
Hrm. What about the comments from Loic?

Revision history for this message
In , Jeremy Sequoia (jeremyhu) wrote :

(In reply to comment #10)
> Hrm. What about the comments from Loic?

I don't think our comments conflict. Loic was concerned about adding dependencies, and I was suggesting making them soft rather than hard dependencies.

The only real concern that I see is Michael's:

> Typically xinit is not run as a root, so the user does not have enough
> permissions to create a session with parameters.
> I question weather or not we should try to move this call into X? X is setuid
> root.

I admit that I did not read that comment the first time through as it was collapsed in my view of the bug. On darwin, we trigger a LaunchDaemon in startx to do some privileged actions for xinit (like creating /tmp/.X11-unix). How is this currently handled on other OSs? Still as an init script?

Revision history for this message
In , Julien Cristau (jcristau) wrote :

> --- Comment #11 from Jeremy Huddleston <email address hidden> 2011-10-04 11:53:02 PDT ---
> (In reply to comment #10)
> > Hrm. What about the comments from Loic?
>
> I don't think our comments conflict. Loic was concerned about adding
> dependencies, and I was suggesting making them soft rather than hard
> dependencies.
>
Comment #7 and comment #9 sound conflicting to me, but ok.

> The only real concern that I see is Michael's:
>
> > Typically xinit is not run as a root, so the user does not have enough
> > permissions to create a session with parameters.
> > I question weather or not we should try to move this call into X? X is setuid
> > root.
>
> I admit that I did not read that comment the first time through as it was
> collapsed in my view of the bug. On darwin, we trigger a LaunchDaemon in
> startx to do some privileged actions for xinit (like creating /tmp/.X11-unix).
> How is this currently handled on other OSs? Still as an init script?
>
At least on Debian, yes, /tmp/.X11-unix is created on boot by an init
script.

Thomas Hotz (thotz-deactivatedaccount)
Changed in ume-config-common (Ubuntu):
status: New → Confirmed
Revision history for this message
Phillip Susi (psusi) wrote :

This package was removed.

Changed in ume-config-common (Ubuntu):
status: Confirmed → Invalid
Revision history for this message
In , Gitlab-migration (gitlab-migration) wrote :

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/app/xinit/issues/1.

Bug Watch Updater (bug-watch-updater)
Changed in xinit:
status: In Progress → Unknown
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.