Why is SASL authentication have to be so difficult? Round 2

[Derrick J Brashear]

On Mon, 6 Dec 2004, Robert Lubbers wrote:

> * OK cyrus.domain.com Cyrus IMAP4 v2.2.9 server ready
> . login cyrususer  secret
> . NO Login failed: can't request info until later in exchange
> . logout
> * BYE LOGOUT received
> . OK Completed

Ken pointed something out to me about this yesterday, notably, that imap 
has code e.g.
         r = sasl_getprop(imapd_saslconn, SASL_USERNAME,
                          (const void **) &canon_user);
while pop does not. And I don't know the history. This is one reason I'd 
rather have a real database of bugs, but making bugzilla manageable for 
this is somewhat hard.

The commit log on 1.398.2.81 is "don't canonify a userid twice"

So we switch from (effectively) calling auth_canonifyid from 
canonify_userid before doing sasl stuff, to this sasl_getprop after.

Reversing that change would be as follows, you'll almost certainly need to 
apply it by hand.
hand.
Index: imapd.c
===================================================================
RCS file: /afs/andrew.cmu.edu/system/cvs/src/cyrus/imap/imapd.c,v
retrieving revision 1.398.2.81
retrieving revision 1.398.2.80
diff -u -r1.398.2.81 -r1.398.2.80
--- imapd.c	29 May 2003 20:18:58 -0000	1.398.2.81
+++ imapd.c	29 May 2003 14:50:45 -0000	1.398.2.80
@@ -38,7 +38,7 @@
   * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
   */

-/* $Id: imapd.c,v 1.398.2.81 2003/05/29 20:18:58 rjs3 Exp $ */
+/* $Id: imapd.c,v 1.398.2.80 2003/05/29 14:50:45 ken3 Exp $ */

  #include <config.h>

@@ -1561,7 +1561,8 @@
      char c;
      struct buf passwdbuf;
      char *passwd;
-    const char *reply = NULL;
+    char *canon_user;
+    const char *reply = 0;
      int plaintextloginpause;
      int r;

@@ -1571,10 +1572,20 @@
  	return;
      }

+    canon_user = canonify_userid(user, NULL, NULL);
+
+    if (!canon_user) {
+	syslog(LOG_NOTICE, "badlogin: %s plaintext %s invalid user",
+	       imapd_clienthost, beautify_string(user));
+	prot_printf(imapd_out, "%s NO %s\r\n", tag, 
+		    error_message(IMAP_INVALID_USER));
+	return;
+    }
+
      /* possibly disallow login */
      if ((imapd_starttls_done == 0) &&
  	(config_getswitch(IMAPOPT_ALLOWPLAINTEXT) == 0) &&
-	!is_userid_anonymous(user)) {
+	strcmp(canon_user, "anonymous") != 0) {
  	eatline(imapd_in, ' ');
  	prot_printf(imapd_out, "%s NO Login only available under a layer\r\n",
  		    tag);
@@ -1596,7 +1607,7 @@

      passwd = passwdbuf.s;

-    if (is_userid_anonymous(user)) {
+    if (!strcmp(canon_user, "anonymous")) {
  	if (config_getswitch(IMAPOPT_ALLOWANONYMOUSLOGIN)) {
  	    passwd = beautify_string(passwd);
  	    if (strlen(passwd) > 500) passwd[500] = '\0';
@@ -1615,21 +1626,22 @@
  	}
      }
      else if ((r = sasl_checkpass(imapd_saslconn,
-				 user,
-				 strlen(user),
+				 canon_user,
+				 strlen(canon_user),
  				 passwd,
  				 strlen(passwd))) != SASL_OK) {
  	syslog(LOG_NOTICE, "badlogin: %s plaintext %s %s",
-	       imapd_clienthost, user, sasl_errdetail(imapd_saslconn));
+	       imapd_clienthost, canon_user, sasl_errdetail(imapd_saslconn));

  	sleep(3);

-	if ((reply = sasl_errstring(r, NULL, NULL)) != NULL) {
+	if (reply) {
+	    prot_printf(imapd_out, "%s NO Login failed: %s\r\n", tag, reply);
+	} else if ((reply = sasl_errstring(r, NULL, NULL)) != NULL) {
  	    prot_printf(imapd_out, "%s NO Login failed: %s\r\n", tag, reply);
  	} else {
  	    prot_printf(imapd_out, "%s NO Login failed: %d\r\n", tag, r);
  	}
-
  	snmp_increment_args(AUTHENTICATION_NO, 1,
  			    VARIABLE_AUTH, 0 /* hash_simple("LOGIN") */,
  			    VARIABLE_LISTEND);
@@ -1637,26 +1649,6 @@
  	return;
      }
      else {
-	const char *canon_user;
- 
-	r = sasl_getprop(imapd_saslconn, SASL_USERNAME,
-			 (const void **) &canon_user);
-
-	if(r != SASL_OK) {
-	    if ((reply = sasl_errstring(r, NULL, NULL)) != NULL) {
-		prot_printf(imapd_out, "%s NO Login failed: %s\r\n",
-			    tag, reply);
-	    } else {
-		prot_printf(imapd_out, "%s NO Login failed: %d\r\n", tag, r);
-	    }
-
-	    snmp_increment_args(AUTHENTICATION_NO, 1,
-				VARIABLE_AUTH, 0 /* hash_simple("LOGIN") */,
-				VARIABLE_LISTEND);
-	    freebuf(&passwdbuf);
-	    return;
-	}
-
  	imapd_userid = xstrdup(canon_user);
  	snmp_increment_args(AUTHENTICATION_YES, 1,
  			    VARIABLE_AUTH, 0 /*hash_simple("LOGIN") */, 
@@ -1761,6 +1753,7 @@
       */
      sasl_result = sasl_getprop(imapd_saslconn, SASL_USERNAME,
  			       (const void **) &canon_user);
+    imapd_userid = xstrdup(canon_user);
      if (sasl_result != SASL_OK) {
  	prot_printf(imapd_out, "%s NO weird SASL error %d SASL_USERNAME\r\n",
  		    tag, sasl_result);
@@ -1769,7 +1762,6 @@
  	reset_saslconn(&imapd_saslconn);
  	return;
      }
-    imapd_userid = xstrdup(canon_user);

      proc_register("imapd", imapd_clienthost, imapd_userid, (char *)0);

---
Cyrus Home Page: http://asg.web.cmu.edu/cyrus
Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu
List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html