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

[Ludwig Nussel <ludwig nussel suse de>]

[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

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

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

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

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

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

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

> 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 :-)

cu
Ludwig

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