How is this different than the FLEXI_XNEST command, which also takes the uid as an argument?  Forgive me, but I use GDM on Solaris, which doesn't support virtual terminals so I can't quickly look at this.

Also, could you explain a use-case of how you would like this to be used?  I understand you probably want this for the FUSA, but am wondering how you see this working.

Assuming the use case is reasonable, I would certainly accept a patch to support such a new command.

Adding such functionality would be hard to do without it being a
nasty hack, because GDM depends on PAM.  Systems can configure PAM 
to use alternative mechanisms for username/password so you could set 
up a system to get the username from a SmartCard and the password by 
pressing your thumb against a fingerprint reader, etc.  So making the 
GDM daemon tell the GUI what username to enter may conflict with the 
PAM setup.  In other words, you really can't depend on the fact that
the username is entered corresponds to a string the user types, so 
you can't just tell the GUI to "enter the string and hit RETURN".

Let's think about this a bit, and I'll talk about this with our local
PAM expert and see if he has any ideas about how to correctly approach
this.

Also, consider looking at the gdmdynamic command.  It doesn't let you pass in the userid, but is a way to manage displays via the sockets protocol.

Brian, I believe having something like this is essential for reasonable fast user switching UI. And it is not very hard to do. In fact, gdm already contains code to do this kind of thing, for the root authentication when starting to configurator
and for automatic login. gdm_verify_user accepts an optional username parameter.
Passing a username that was received via a socked command in there should make everything work out as expected, I believe.

Created attachment 80620 [details] [review]
patch

Here is a prototype patch. It works by adding two new socket commands,
FLEXI_XSERVER_USER and FLEXI_XNEST_USER which are just variation of the
originals that also take a username. That username is than passed down
to the slave, which uses it instead of NULL for the username argument
when calling gdm_verify_user(). 

Seems to work nicely in my testing.

Forgot to mention, for testing this, I just patched gdmflexiserver to use the
_USER versions of the gdm commands, and hardcoded my name in the argument list...

Note, the pam apis take an optional username argument.  It's the responsibility of the PAM modules to check whether a username is already set (and for instance deny access to the user if the username doesn't match whats on the smart card).

(in fact, that username argument is required for reauthentication when unlocking a screensaver, for instance).

I have some issues with this patch.  It assumes that the PAM stack is going to request username/password, and will only work with PAM modules that work in this way.  

It might work better if it worked as follows:

1) Send GDM a request to authenticate the user.  Then GDM could display dialogs that talk with PAM and request the needed info.  Perhaps it could just launch gdmlogin so it pops up a login dialog on top of the users running session.  This request to do this could then also specify what to do after authentication which could include the following:

1) launch a program as root (e.g. a nice way to run gdmsetup or other programs
   as the root user, like an alternative to sudo)
2) start a new session.

This might be a little less nice from a usability perspective since launching a GUI like gdmlogin isn't quite so integrated into whatever program wants to
make the request, but it would work better with PAM.

Though we can discuss.  I'm not 100% opposed to a patch that only works with some PAM modules if there is a real need for it.  But the danger is that people will complain that it doesn't work with kerberos, SmartCard, etc. environments which use different PAM modules.  Then we'll maybe have to work on redoing this later to make it more generic for such cases, which would be annoying.

Also, a danger with passing in the username/password into the program is it makes it a bit more easy to possibly hack the system.  For example, if the GUI program that asks for the username/password crashes, the info could be kept around in a core file in the user's $HOME directory.  Better to let GDM handle this so that if there is a crash/corefile, it isn't readable by normal users.

Brian, I am not sure I see why this should not work with  e.g. smart cards. 

The documentation of pam_start says:

   The user argument can specify the name of the target user and will be
       stored as PAM_USER item. If the argument is NULL, the module has to ask
       for this item if necessary.

This does not say that passing in a username will be a problem for pam modules that don't need this information, it only says that pam modules which do need
the username don't have to ask for it, since it was already provided.

I guess we can just test that with smart cards, Ray ?

Yes, but isn't it confusing to ask for the user, if the username is
actually pulled from the SmartCard?  Also, what if the password is
provided via another mechanism (fingerprint reader, retina scanner,
etc.).  In the case of SmartCard for username and fingerprint reader
for password, passing both username/password is not meaningful.

My patch is trying to avoid asking for the username again...
And there is no passing of a password, so that is not an issue. 
All I am doing here is trying to handle the following situation better:

1) user picks a user name from the the fast-user-switch-applet
2) gdm login screen comes up and asks for a user name again
3) user is confused 

since the username is already selected in 1), and since pam_start has
a parameter to pass a preselected username in the pam conversation,
why not connect the dots and make this work as expected ?

Any PAM module that doesn't handle the PAM_USER item is broken.

This shouldn't break with smartcards or fingerprint readers or anything of the like because, ultimately all these devices have to map to a UID on the system (and corresponding username).

The way a smartcard PAM module should handle this is to check if PAM_USER is set and save it internally.  Then it should ask for a pin code, unlock the card, and  retrieve the username from the smartcard.  

Once it has the username from the smartcard if it doesn't match what PAM_USER was set to, then it should return an error code (like "insufficient credentials" because it doesn't have sufficient credentials to verify the user [namely the right smartcard corresponding to the user requested])

Of course, PAM_USER being set is optional.  If it isn't set, the pam module should read the username off the card and set PAM_USER for modules later in the stack.

Thanks, Ray, for your comments.  After looking over the patch again I am more comfortable with it.  However, a few comments.

1) I notice the string "mclasen" coded in the patch.  Why is this hardcoded
   string there?  Is this a typo?
2) You seem to be putting the "preset_user" in the display arary in the
   display structure, but you don't modify display.c.  I'd think it
   would be important to free this (if not NULL) in gdm_display_dispose.
   Also the display structure gets initialized in gdm_server_alloc, but
   I don't see you setting the initial value to NULL in this function.
   So you seem to be relying on it having a NULL value on initialize
   without actually initializing it, or am I missing something?
3) The comments in gdm.h should be fleshed out a bit more with an 
   example of the arguments, return codes, etc.  Also, you don't add
   any documentation to docs/C/gdm.xml.  Please do so.

I will accept this patch if the above issues are addressed.

Created attachment 80917 [details] [review]
updated patch

Here is an updated patch that addresses the issues. 
I note some issue with a patched gdm 2.17.5: The first login attempt
after a logout fails (I see a vt switch, then I end up back at the gdm login),
but the next attempt succeeds. 
I am not sure if that is a side effect of this patch.

The patch looks really good.  Do you see the problem with the failed login when you don't have the patch applied?

Note that we are past UI freeze for 2.17 so this patch will probably need to wait until 2.19 unless there is a pressing need to get it in for 2.18.  If you think this should go into 2.18, then we need to get approval from the release team.

I couldn't reproduce my re-login problem with an unpatched 2.17.6, so there may
indeed be some interaction with my patch. But I am seeing erroneous vt switching
at logout time, too, in rawhide. Maybe Ray can shed some light on this.

Ok, my crashes turn out to be due to me turning on the ConsoleKit support in
rawhide gdm. If I configure it --without-console-kit, my patch works fine.

Btw, the patch contains several references to 2.17.7 in documentation. Those will 
have to be adjusted if it goes in a later version.

Yup.  I've filed bug 400793.  Isn't related to this patch.

Matthias:  Thanks for tracking the problem down to the ConsoleKit.  It looks like William is looking into fixing that issue now.

Do you think this patch should go into 2.17?  If so, we should get release team approval.  Though since GDM isn't a part of the GNOME platform, we could probably
get away with putting it into 2.17 even without permission.  However, I'd only want to do this if there were a real need.  Are there plans for client programs (such as Fast Switching applet) to use this in the 2.18 timeframe?  If not, then lets put this into 2.19. 

Your thoughts?

We want to use something like this in FC7 to improve the user experience of user switching - that means making fast-user-switch-applet and gnome-screensaver use it. If you don't want to add this in 2.17, we'll probably just backport the patch that goes into 2.19 for our purposes.

That doesn't really answer my question.  My question is that if the maintainers of fast-switch-applet and gnome-screensaver are cool with changing the interfaces late in the 2.17 cycle, then I am agreeable to putting in this change late.  Otherwise lets wait until 2.19.

If you are agreeable to get a thumbs-up from the fast-switch-applet and gnome-screensaver maintainers then I'm willing to put this in now.  If they don't have time to get this in before 2.19, then lets wait.

Thumbs up from me (g-s maintainer).  BTW, there are a few indentation inconsistencies in the patch.

I have put an untested gnome-screensaver patch in bug 402183 

Thanks.  What about fast-switch-applet?  If we get one more thumbs-up, I'll put this in.  Assuming that all three maintainers are willing to put the update into 2.18.  If not, lets go for 2.19.

An untested fast-user-switch-applet patch is in bug 402475

Created attachment 81540 [details] [review]
updated patch

Previous patch didn't apply cleanly apparantly due to RH specific patches.  Updated to apply and also fix some indentation inconsistencies.

Works well for me so far.

Matthias, thanks for submitting the fast-user-switch-applet bug.  Do we have buy-in from the fast-user-switch applet maintainer?  I'd feel a little more comfortable if the fast-user-switch applet patch were tested.

So what do people think?  If we think this will all go into 2.18, then I'm happy to put this into GDM SVN head now.

I'd like to see this in 2.18 if we get buy in from the FUSA maintainer.

Brian, I don't think it is necessary to wait for FUSA to adopt this interface.  I really want to use this in gnome-screensaver and I'd like to commit that support ASAP (before the release on Monday).  Can we get this in now?

FWIW, I went ahead and put the gdm, gnome-screensaver and fast-user-switch-applet patches in Fedora rawhide earlier this week.

Okay, this patch is committed to SVN head.