GNOME.org
 

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


Ludwig:

I think this patch looks okay, but I would appreciate it if you
could explain in more detail these changes.  Modifications to the
verify_pam code is a sensitive security area of the code, so I
want to make sure that your analysis of why changes are needed
are clear.  Please respond to the following questions before I
accept your patch.  Thanks.


I've been playing with gdm and my pam test module a bit yesterday.
Here are some changes I'd like to suggest:

gdmlogin:
- don't make the start again button insensitive as the prompt for
  the username is not necessarily the first.


This is reasonable.


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


- 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?  Also, does a similar fix need to be made in gdmgreeter?


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


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?  What problems happen with this code present?


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


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


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

Thanks, again.  I think when we work through these issues, we will
make GDM work much better with more dynamic PAM modules.

Brian



Index: gdm2/gui/gdmlogin.c
===================================================================
--- gdm2.orig/gui/gdmlogin.c
+++ gdm2/gui/gdmlogin.c
@@ -1558,7 +1558,8 @@ process_operation (guchar       op_code,
 					gdm_config_get_bool   (GDM_KEY_SOUND_ON_LOGIN));
 		gtk_label_set_text_with_mnemonic (GTK_LABEL (label), _("_Username:"));
 		greeter_probably_login_prompt = TRUE;
-		gtk_widget_set_sensitive (start_again_button, FALSE);
+		/* prompt for username is not necessarily the first */
+		gtk_widget_set_sensitive (start_again_button, TRUE);
 	} else {
 		gtk_widget_set_sensitive (start_again_button, TRUE);
 		if (tmp != NULL)
@@ -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
 	break;
case GDM_NOECHO: 
 	tmp = ve_locale_to_utf8 (args);
+	gtk_widget_set_sensitive (start_again_button, TRUE);
 	if (tmp != NULL && strcmp (tmp, _("Password:")) == 0) {
-		gtk_widget_set_sensitive (start_again_button, TRUE);
 		gtk_label_set_text_with_mnemonic (GTK_LABEL (label), _("_Password:"));
 	} else {
-		gtk_widget_set_sensitive (start_again_button, FALSE);
 		if (tmp != NULL)
 			gtk_label_set_text (GTK_LABEL (label), tmp);
 	}
@@ -2687,7 +2689,9 @@ gdm_login_gui_init (void)
     /* Put in error box here */
     err_box = gtk_label_new (NULL);
     gtk_widget_set_name (err_box, "Error box");
+#if 0 /* widget doesn't expand anymore after this call */
     gtk_widget_set_size_request (err_box, -1, 40);
+#endif
     g_signal_connect (G_OBJECT (err_box), "destroy",
 		      G_CALLBACK (gtk_widget_destroyed),
 		      &err_box);
@@ -2757,14 +2761,16 @@ gdm_login_gui_init (void)
 		      (GtkAttachOptions) (GTK_EXPAND | GTK_FILL),
 		      (GtkAttachOptions) (GTK_FILL), 0, 10);
- msg = gtk_label_new (_("Please enter your username")); 
+    msg = gtk_label_new (NULL);
     gtk_widget_set_name (msg, "Message");
     gtk_label_set_line_wrap (GTK_LABEL (msg), TRUE);
     gtk_label_set_justify (GTK_LABEL (msg), GTK_JUSTIFY_LEFT);
     gtk_table_attach (GTK_TABLE (stack), msg, 0, 1, 6, 7,
 		      (GtkAttachOptions) (GTK_EXPAND | GTK_FILL),
 		      (GtkAttachOptions) (GTK_FILL), 0, 10);
+#if 0 /* widget doesn't expand anymore after this call */
     gtk_widget_set_size_request (msg, -1, 30);
+#endif
gtk_widget_ref (msg); 
     g_object_set_data_full (G_OBJECT (login), "msg", msg,
Index: gdm2/daemon/verify-pam.c
===================================================================
--- gdm2.orig/daemon/verify-pam.c
+++ gdm2/daemon/verify-pam.c
@@ -52,9 +52,6 @@ static pam_handle_t *pamh = NULL;
static GdmDisplay *cur_gdm_disp = NULL; -/* this is a hack */ 
-static char *tmp_PAM_USER = NULL;
-
 /* this is another hack */
 static gboolean did_we_ask_for_password = FALSE;
@@ -489,7 +486,9 @@ gdm_verify_pam_conv (int num_msg, struct 
     /* Should never happen unless PAM is on crack and keeps asking questions
        after we told it to go away.  So tell it to go away again and
        maybe it will listen */
-    if ( ! gdm_slave_action_pending ())
+    /* well, it actually happens if there are multiple pam modules
+     * with conversations */
+    if ( ! gdm_slave_action_pending () || selected_user)
         return PAM_CONV_ERR;
reply = malloc (sizeof (struct pam_response) * num_msg); 
@@ -502,13 +501,8 @@ gdm_verify_pam_conv (int num_msg, struct
     /* Here we set the login if it wasn't already set,
      * this is kind of anal, but this way we guarantee that
      * the greeter always is up to date on the login */
-    if (pam_get_item (pamh, PAM_USER, (void **)&p) == PAM_SUCCESS ||
-	tmp_PAM_USER != NULL) {
+    if (pam_get_item (pamh, PAM_USER, (void **)&p) == PAM_SUCCESS) {
 	    login = (const char *)p;
-
-	    /* FIXME: this is a HACK HACK HACK, for some reason needed */
-	    if (tmp_PAM_USER != NULL)
-		    login = tmp_PAM_USER;
 	    gdm_slave_greeter_ctl_no_ret (GDM_SETLOGIN, login);
     }
@@ -517,7 +511,6 @@ gdm_verify_pam_conv (int num_msg, struct 
     openlog ("gdm", LOG_PID, LOG_DAEMON);
for (replies = 0; replies < num_msg; replies++) { 
-	gboolean islogin = FALSE;
 	const char *m = (*msg)[replies].msg;
 	m = perhaps_translate_message (m);
 	
@@ -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 {
+		    if ( ve_string_empty (selected_user)) {
 			    /* this is an evil hack, but really there is no way we'll
 			    know this is a username prompt.  However we SHOULD NOT
 			    rely on this working.  The pam modules can set their
@@ -542,7 +529,6 @@ gdm_verify_pam_conv (int num_msg, struct
 			    s = gdm_slave_greeter_ctl (GDM_PROMPT, m);
 			    /* this will clear the message */
 			    gdm_slave_greeter_ctl_no_ret (GDM_MSG, "");
-			    islogin = TRUE;
 		    }
 	    } else {
 		    s = gdm_slave_greeter_ctl (GDM_PROMPT, m);
@@ -557,29 +543,6 @@ gdm_verify_pam_conv (int num_msg, struct
 		    return PAM_CONV_ERR;
 	    }
- if (islogin) { 
-		    /* note that we should NOT rely on this for anything
-		       that is REALLY needed.  The reason this is here
-		       is for the face browser to work correctly.  The
-		       best way would really be to ask all questions at
-		       once, but that would 1) need some protocol rethinking,
-		       2) the gdmgreeter program is really not equipped
-		       for this, would need theme format change */
-		    /* Second problem is that pam will not actually set
-		       PAM_USER itself after it asks for it.  This is
-		       pretty evil, evil, evil, evil, but somewhat necessary
-		       then. */
-		    /* FIXME: this is a HACK HACK HACK */
-		    g_free (tmp_PAM_USER);
-		    tmp_PAM_USER = g_strdup (s);
-
-		    if (gdm_get_value_bool (GDM_KEY_DISPLAY_LAST_LOGIN)) {
-			    char *info = gdm_get_last_info (s);
-			    gdm_slave_greeter_ctl_no_ret (GDM_ERRBOX, info);
-			    g_free (info);
-		    }
-	    }
-
 	    reply[replies].resp_retcode = PAM_SUCCESS;
 	    reply[replies].resp = strdup (ve_sure_string (s));
 	    g_free (s);
@@ -874,10 +837,6 @@ verify_user_again:
authenticate_again: - /* hack */ 
-    g_free (tmp_PAM_USER);
-    tmp_PAM_USER = g_strdup (login);
-
     /*
      * Initialize a PAM session for the user...
      * Get value per-display so different displays can use different
@@ -894,6 +853,9 @@ authenticate_again:
     }
     g_free (pam_stack);
+ g_free (login); 
+    login = NULL; // have to unset it otherwise there is no chance to ever enter a different user
+
     pam_set_item (pamh, PAM_USER_PROMPT, _("Username:"));
#if 0 
@@ -940,7 +902,7 @@ authenticate_again:
/* FIXME: what about errors */ 
 		    /* really this has been a sucess, not a failure */
-		    pam_end (tmp_pamh, PAM_SUCCESS);
+		    pam_end (tmp_pamh, pamerr);
gdm_slave_greeter_ctl_no_ret (GDM_SETLOGIN, login); @@ -1119,9 +1081,6 @@ authenticate_again: cur_gdm_disp = NULL; - g_free (tmp_PAM_USER); 
-    tmp_PAM_USER = NULL;
-
 #ifdef  HAVE_LOGINDEVPERM
     if (d->attached)
 	(void) di_devperm_login ("/dev/console", pwent->pw_uid,
@@ -1206,9 +1165,6 @@ authenticate_again:
g_free (login); - g_free (tmp_PAM_USER); 
-    tmp_PAM_USER = NULL;
- cur_gdm_disp = NULL; return NULL;
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]