libgnomesu [was Re: Proposed modules: my consensus so far]

[Mark McLoughlin <markmc redhat com>]

Hi,

On Tue, 2004-11-23 at 11:00 +0100, Murray Cumming wrote:

> libgnomesu:
>   Not until a Desktop modules uses it, or says that they want to use it.

	I've taken a look at this and come away feeling fairly queasy at the
thought of including this in GNOME and making widespread use of it. Some
detailed, but not exhaustive, comments below - I think this requires a
closer look even if all the comments are addressed.

	One thing that occurred to me when looking at libgnomesu was that
usermode is no more Red Hat specific than libgnomesu is e.g. JDS uses
usermode without any problems. If we find that GNOME has a need for this
kind of functionality, then perhaps it makes as much sense for usermode
to be included in GNOME as libgnomesu?

	Anyway, that's putting the horse in front of the cart a bit. What we
really need to think about the use cases for this run-as-root
functionality in GNOME and consider whether a libgnomesu-like
run-this-as-root API makes more sense than a usermode-like
allow-this-app-to-be-run-as-root interface.

	So, what are the GNOME use cases? Hongli, Carlos, Beno�

Cheers,
Mark.

Comments:

  - API problems:

      + It would make sense for the APIs to correspond much more 
        closely with the gspawn APIs - effectively, it should be
        the same as the gspawn APIs with an added "user" argument.
        See gdkspawn, for example.

      + There's no way for multi-screen apps to specify which screen
        the dialogs and the app itself should be displayed.

      + Should have error reporting via GError

      + The "async" APIs are a bit of a misnomer since they block
        until authentication has completed - why not make them fully
        asynchronous by putting an IO watch on the pipe?

      + Users of the async APIs need to call waitpid() - that's messy
        and undocumented. Why not use the intermediate-child technique
        gspawn uses to avoid this? Or even better, use gspawn?

  - No startup-notification integration

  - Ignoring the consolehelper backend for a minute, isn't the PAM
    backend sufficient? Why do we need the su backend? For platforms
    that don't use PAM? If so, which platforms are they?

  - sudo service should be removed if its broken

  - The BinReloc stuff looks very dodgy to have in a library - what
    happens if an app which links to libgnomesu also uses this stuff?
    Unless we thought that a hack like this was the way to for GNOME
    to address the relocatable package problem, then I think this
    should be removed.

  - Since we run the mainloop from some of the services, won't we be
    screwed by re-entrancy - i.e. if you click on a button that causes
    the an app to be launcher with libgnomesu, and while the password
    dialog is up, you click on the button again, what happens?

  - The thought of a Nautilus "Open as Superuser" component gives me
    the heebie-jeebies. I'm not sure exactly why. Its irrelevant now
    with Alex's recent changes to Nautilus, anyway :-)

  - We've a whole copy of "su" in here. Are we going to keep up to date 
    with upstream changes to the code? Copying and pasting code like 
    this worries me.

  - su-backend/common.c:modify_environment() is a copy of usermode and
    su code, both of which are licensed under the GPL, and you've 
    removed the copyright notice and re-licensed to LGPL.

  - Use of waitpid() without checking for EINTR will cause zombies

  - Every time you do find_service() each of the services allocate
    a struct for their function pointers. Seems gratuitous and easily
    avoided.

  - 2 leaks in consolehelper.c:detect()

  - Use GChildSource instead of while (waitpid ()) { sleep (); }

  - Before executing the PAM or su backends you should more carefully
    check that this is a "safe" directory - e.g. not writable by all -
    and more checking of the permissions on the backend so that you
    can be sure no-one has installed a trojan backend. Especially
    important with the BinReloc stuff.

  - No error checking with the xauth stuff in the PAM and su backends.
    But why not just use the same $XAUTHORITY?