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

[Brian Cameron <Brian Cameron Sun COM>]

Ludwig:

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.

> [hopefully I didn't miss a question]
> Brian Cameron wrote:
> > > - don't tell backend that a user was selected in the face browser if
> > >   the backend itself caused that to avoid aborting an ongoing
> > >   authentication.
> > Could you explain this, and what code in your patch is actually
> > avoiding the daemon notification?
>
> If gdm_verify_pam_conv() gets called a second time because a chained
> pam module wants to converse as well gdm notifies the gui of
> the PAM_USER. The gui now highlights the username in the face
> browser. That action now in turn triggers a 'username selected in
> face browser' interruption in the backend, gdm aborts the
> conversation function and  starts from scratch assuming that the
> user indeed used the face browser.
>
> 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?

> > > - allow the message boxes to expand
> > Could you explain how this affects the behavior of gdmlogin?  When
> > messages expand does it cause the login window to grow and shrink
> > in size?
>
> Yes. You can hardly avoid that though. Without allowing it to change
> size messages are not displayed completely. The German translation
> of some gdm help texts (like the hint that upper/lower case matters)
> already doesn't fit in the current dialog.

Okay, I'll accept a patch to allow the message boxes to expand.

> > 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?

> > > verify-pam:
> > > - remove the stange tmp_PAM_USER hack
> > Could you please explain what the tmp_PAM_USER is used for, and why
> > it is okay to remove it?
>
> Good question :-) The code says:
>
> * this is a hack */
> static char *tmp_PAM_USER = NULL;
>
> /* FIXME: this is a HACK HACK HACK, for some reason needed */
> if (tmp_PAM_USER != NULL)
>         login = tmp_PAM_USER;
>
> Obviously the original author of that code didn't understand what
> exactly it's useful for either.
>
> > What problems happen with this code present?
>
> I think it's a reason for an endless authentication loop with the same user. Ie
> once you select a username in the face browser you can only get to a different
> one if you use the face browser again.

Yes, I'm comfortable accepting a patch to remove this.

> > > - cancel all conversations in a pam_authenticate run if a user was
> > >   selected in the face browser.
> > Why would we want to avoid calling PAM if the user is selected in the
> > face browser.  Sorry, I just don't understand why this is a good idea.
> > Perhaps you could explain.
>
> If you have two pam modules A and B the control flow goes like this:
>
> 1. GDM    pam_authenticate()
> 2. A         pam_sm_authenticate()
> 3. GDM          gdm_verify_pam_conv()
> 4. B         pam_sm_authenticate()
> 5. GDM          gdm_verify_pam_conv()
>
> My observation was that PAM calls chained modules even if one module
> in the chain returned PAM_CONV_ERR. So if GDM aborts 3 because a
> different user was selected it also needs to abort in 5. Otherwise
> the user gets prompted by module B before GDM restarts with
> pam_authenticate().

Okay this makes sense.  I'd accept a patch with this change.

> > > - 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?

> > > - 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.

> > Thanks, again.  I think when we work through these issues, we will
> > make GDM work much better with more dynamic PAM modules.
>
> That's my intention :-)

Brian