Re: [gdm-list] [PATCH] some pam related improvements

[Ludwig Nussel <ludwig nussel suse de>]

Brian Cameron wrote:
> Can you resend the patch updated based on our discussion so far?  I'd
> like to get this upstream but would like to see a patch that takes
> into account our discussion so far.

Yes but might take a while as I need to isolate the changes and
rediff my patch stack.

> > It's this hunk:
> > @@ -1584,17 +1585,18 @@ process_operation (guchar       op_code,
> >  
> >  	login_window_resize (FALSE /* force */);
> >  
> > +#if 0 /* sends interruption to backend which would cause it to redo auth */
> >          if (greeter_probably_login_prompt == TRUE && selected_user != NULL)
> >  		face_browser_select_user (selected_user);
> > +#endif
> 
> What this code does is fix GDM so that if a user is selected, and the
> user mistypes the password, the user doesn't get unselected.  So the
> same user stays selected and you can just try to enter the password
> again.   You can click the "Start Again" button to clear the Face
> Browser list to try and enter a username.
> 
> I'm not opposed to breaking this feature if it breaks PAM assumption,
> but perhaps this could be coded in a different way?  If a user is
> selected in the face browser, then you shouldn't have to reselect
> the same user just because you failed uathentication.  Perhaps this
> can be coded in a smarter way?

Hmm, ok. IMO there should be at least some counter that really
restarts from scratch without user name after e.g. three failed
logins (login uses LOGIN_RETRIES from /etc/login.defs).

> >> Also, does a similar fix need to be made in gdmgreeter?
> > 
> > I've seen that at least the error message gets cut at the border of
> > the screen. So yes, some improvements to make sure a message is
> > displayed completely would be necessary.
> > 
> >>> - don't preset message with "please enter your username" as the
> >>>   first prompt might be a different one.
> >> This is related to bug #333200:
> >>
> >>    http://bugzilla.gnome.org/show_bug.cgi?id=333200
> >>
> >> Please update the bug with your thoughts about this.  I'm not opposed
> >> to removing the labels, but if we decide to do this then be aware that
> >> this might break l10n for systems where PAM isn't localized.  Perhaps
> >> we might want a more elegant fix where this "extra" message can be
> >> turned on or off, perhaps off by default.  Systems with broken pam
> >> could turn it on maybe?  Ideas?
> > 
> > My patch just removes the text in the gui code where the label is
> > constructed. The message 'please enter your username' is still
> > displayed if the backend (verify-pam.c) sees a username prompt.
> > Although that code should probably be moved into the gui at the
> > proper place in order to avoid faking a pam message. I wasn't
> > suggesting to remove the message compeletly.
> 
> Would you be agreeable to fixing this up properly?  It sounds like
> your previous patch was a half-fix?

The patch is ok for the current single prompt implementation. For
multiple prompts I really had to move the message into the GUI.

> >>> - don't try to be smart about the username prompt. If pam asks for a
> >>>   username then also ask the user about it. The GUI could be made to
> >>>   preset the text entry with the username instead.
> >> Please explain more clearly what you mean when you say GDM is trying
> >> to be "smart" about the username prompt, and why we don't want to try
> >> and be smart.  :)  What problems happen with this code present?
> > 
> > If verify-pam sees a username prompt although a user was selected
> > with the face browser it just fills in the username in the reply and
> > doesn't even bother to pass the prompt to the GUI. One wouldn't
> > expect pam to prompt for a username when it already has one but if
> > it does it problably has a reason for doing it. I don't have a
> > particular case where the current code breaks things but I don't see
> > a case where the current behavior makes sense either. So just remove
> > that code for the sake of getting rid of weird hacks.
> 
> Could you explain where in the patch this change is made?

@@ -526,13 +519,7 @@ gdm_verify_pam_conv (int num_msg, struct
        /* PAM requested textual input with echo on */
        case PAM_PROMPT_ECHO_ON:
            if (strcmp (m, _("Username:")) == 0) {
-                   if ( ! ve_string_empty (selected_user)) {
-                           /* Sometimes we are just completely on crack,
-                              and pam asks for the username even if we
-                              already gave it.  PAM is on better crack,
-                              then I can afford. */
-                           s = g_strdup (selected_user);
-                   } else {

> >>> - remove the lastlogin hack. pam_lastlog can do that *after*
> >>>   authentication instead.
> >> Again, please explain what the lastlogin hack does and why it is a
> >> good idea to get rid of it.  What problems happen with this code
> >> present?
> > 
> > Well, I didn't actually try the config option. From looking at the
> > code one can see that gdm displays when a user logged in last time
> > as soon as one enters the user name. Ie you can find out whether and
> > when someone else was on the system without authenticating yourself.
> > You just have to enter the username of someone else. If you think
> > the lastlogin feature should be kept in gdm (maybe other systems
> > don't have pam_lastlog?) I'd move it further down and call it only
> > after succesful authentication.
> 
> I suspect we shouldn't remove a configuration option that some users
> might be using, unless there is a good reason.

I'll move the code after pam_sm_authenticate() then.

cu
Ludwig

-- 
 (o_   Ludwig Nussel
 //\   SUSE LINUX Products GmbH, Development
 V_/_  http://www.suse.de/