Re: [Maria-developers] Review of 5.2 pluggable-auth tree

25 Mar 2010

      Hi!

Review of ~maria-captains/maria/5.2-pluggable-auth
(3 day of work!)
...
=== modified file 'client/mysql.cc'
<cut>
...
+ /**
+   An example of mysql_authentication_dialog_ask callback.
+ 
+   The C function with the name "mysql_authentication_dialog_ask", if exists,
+   will be used by the "dialog" client authentication plugin when user
+   input is needed. This function should be of mysql_authentication_dialog_ask_t
+   type. If the function does not exists, a built-in implementation will be
+   used.
+ 
+   @param mysql          mysql
+   @param type           type of the input
+                         1 - normal string input
+                         2 - password string
+   @param prompt         prompt
+   @param buf            a buffer to store the use input
+   @param buf_len        the length of the buffer
+ 
+   @retval               a pointer to the user input string.
+                         It may be equal to 'buf' or to 'mysql->password'.
+                         In all other cases it is assumed to be an allocated
+                         string, and the "dialog" plugin will free() it.
+ */
+ extern "C" char *mysql_authentication_dialog_ask(MYSQL *mysql, int type,
I would prefer to have type as my_bool as there is only two options
for it and rename it to 'ask_for_password'.  This would make the code
more self_explanatory.
...
+                                                  const char *prompt,
+                                                  char *buf, int buf_len)
+ {
+   int ch;
+   char *s=buf, *end=buf+buf_len-1;
+ 
+   fputs("[mysql] ", stdout);
[mysql]  -> [mariadb]
...
+   fputs(prompt, stdout);
+   fputs(" ", stdout);
+ 
+   if (type == 2) /* password */
+   {
+     s= get_tty_password("");
+     strncpy(buf, s, buf_len);
Better to use strnmov() like we use in all other part of the code.
...
+     my_free(s, MYF(0));
+   }
+   else
+   {
+     for (ch= fgetc(stdin); s < end && ch != '\n' && ch != EOF; ch= fgetc(stdin))
+       *s++= ch;
hm, we should really support backspace too....

Why not simply use fgets() that gives you basic editing?

  fgets(buf, buf_len-1, stdin);
  if (buf[0] && (s= strend(buf))[-1] == '\n')
    *s= 0;
...
+     *s=0;
+   }
+ 
+   return buf;
+ }
...
=== modified file 'include/my_global.h'
*** include/my_global.h	2009-12-03 11:19:05 +0000
--- include/my_global.h	2010-02-19 08:18:09 +0000
*************** int	__void__;
*** 578,583 ****
--- 578,591 ----
  #define IF_VALGRIND(A,B) (B)
  #endif
+ #ifdef _WIN32
In the rest of my_global.h we use __WIN__
Wouldn't it be better to use the same constant everywhere?
...
+ #define SO_EXT ".dll"
+ #elif defined(__APPLE__)
+ #define SO_EXT ".dylib"
+ #else
+ #define SO_EXT ".so"
+ #endif
...
=== modified file 'include/mysql.h'
*** include/mysql.h	2010-02-01 06:14:12 +0000
--- include/mysql.h	2010-02-19 08:18:09 +0000
*************** enum mysql_option
*** 167,175 ****
    MYSQL_OPT_USE_REMOTE_CONNECTION, MYSQL_OPT_USE_EMBEDDED_CONNECTION,
    MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH,
    MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT,
!   MYSQL_OPT_SSL_VERIFY_SERVER_CERT
  };
struct st_mysql_options {
    unsigned int connect_timeout, read_timeout, write_timeout;
    unsigned int port, protocol;
--- 167,181 ----
    MYSQL_OPT_USE_REMOTE_CONNECTION, MYSQL_OPT_USE_EMBEDDED_CONNECTION,
    MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH,
    MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT,
!   MYSQL_OPT_SSL_VERIFY_SERVER_CERT, MYSQL_PLUGIN_DIR, MYSQL_DEFAULT_AUTH
  };
+ /**
+   @todo remove the "extension", move st_mysql_options completely
+   out of mysql.h
The reason to have it in mysql.h is to be able to put it into the
MYSQL structure to avoid mallocs (for most cases) if you have MYSQL on
the stack or as a static variable.  Don't see how we can easily remove
it, if we want to keep this property.
...
--- include/mysql/plugin_auth.h	2010-02-19 08:18:09 +0000
***************
*** 0 ****
--- 1,83 ----
+ #ifndef MYSQL_PLUGIN_AUTH_INCLUDED
+ /* Copyright (C) 2010 Monty Program Ab
Add your/original author name here too
...
=== added file 'include/mysql/plugin_auth_common.h'
*** include/mysql/plugin_auth_common.h	1970-01-01 00:00:00 +0000
--- include/mysql/plugin_auth_common.h	2010-02-19 08:18:09 +0000
***************
*** 0 ****
--- 1,105 ----
+ #ifndef MYSQL_PLUGIN_AUTH_COMMON_INCLUDED
+ /* Copyright (C) 2010 Monty Program Ab
Add your/original author name here too
...
+ /** the max allowed length for a user name */
+ #define MYSQL_USERNAME_LENGTH 48
Shouldn't this be the define USERNAME_LENGTH ?
Otherwise there will be a crash if someone redefines USERNAME_CHAR_LENGTH

<cut>
...
+ typedef struct st_plugin_vio_info
+ {
+   enum { MYSQL_VIO_INVALID, MYSQL_VIO_TCP, MYSQL_VIO_SOCKET,
+          MYSQL_VIO_PIPE, MYSQL_VIO_MEMORY } protocol;
+   int socket;     /**< it's set, if the protocol is SOCKET or TCP */
+ #ifdef _WIN32
_WIN32 -> __WIN__  ?
...
+   HANDLE handle;  /**< it's set, if the protocol is PIPE or MEMORY */
+ #endif
+ } MYSQL_PLUGIN_VIO_INFO;
+
...
=== modified file 'libmysqld/lib_sql.cc'
*** libmysqld/lib_sql.cc	2009-12-03 11:19:05 +0000
--- libmysqld/lib_sql.cc	2010-03-02 20:32:24 +0000
*************** static MYSQL_RES * emb_store_result(MYSQ
*** 413,423 ****
    return mysql_store_result(mysql);
  }
! int emb_read_change_user_result(MYSQL *mysql, 
! 				char *buff __attribute__((unused)),
! 				const char *passwd __attribute__((unused)))
  {
!   return mysql_errno(mysql);
  }
MYSQL_METHODS embedded_methods= 
--- 412,421 ----
    return mysql_store_result(mysql);
  }
! int emb_read_change_user_result(MYSQL *mysql)
  {
!   mysql->net.read_pos= (uchar*)""; // fake an OK packet
!   return mysql_errno(mysql) ? packet_error : 1;
  }
Add a comment that '1' is packet length.
...
*************** void init_embedded_mysql(MYSQL *mysql, i
*** 584,589 ****
--- 582,588 ----
    THD *thd = (THD *)mysql->thd;
    thd->mysql= mysql;
    mysql->server_version= server_version;
+   mysql->client_flag= client_flag;
    init_alloc_root(&mysql->field_alloc, 8192, 0);
  }
Was this a bug in the old code ?
(Just curious)
...
=== modified file 'mysql-test/suite/rpl/r/rpl_stm_000001.result'
*** mysql-test/suite/rpl/r/rpl_stm_000001.result	2009-11-18 14:50:31 +0000
--- mysql-test/suite/rpl/r/rpl_stm_000001.result	2010-02-19 08:18:09 +0000
*************** Warnings:
*** 66,71 ****
--- 66,72 ----
  Warning	1364	Field 'ssl_cipher' doesn't have a default value
  Warning	1364	Field 'x509_issuer' doesn't have a default value
  Warning	1364	Field 'x509_subject' doesn't have a default value
+ Warning	1364	Field 'auth_string' doesn't have a default value
Should auth_string have a default value (to not cause a problem for
anyone using --strict mode and is inserting things into the auth
tables?)

Note likely problem, but possible...

IRC: conclusion
- As auth_string is a blob and we don't know the length, it's best to
leave it as it is.

<cut>
...
*** plugin/auth/auth_socket.c	1970-01-01 00:00:00 +0000
--- plugin/auth/auth_socket.c	2010-02-19 08:18:09 +0000
***************
*** 0 ****
--- 1,93 ----
+ /* Copyright (C) 2010 Monty Program Ab
+
Add your/author's name here

<cut>
...
=== added file 'plugin/auth/dialog.c'
*** plugin/auth/dialog.c	1970-01-01 00:00:00 +0000
--- plugin/auth/dialog.c	2010-02-23 12:02:00 +0000
***************
*** 0 ****
--- 1,293 ----
+ /* Copyright (C) 2010 Monty Program Ab
Add your/author's name here

<cut>
...
+ static int two_questions(MYSQL_PLUGIN_VIO *vio, MYSQL_SERVER_AUTH_INFO *info)
+ {
+   unsigned char *pkt;
+   int pkt_len;
+ 
+   /* send a password question */
+   if (vio->write_packet(vio, PASSWORD_QUESTION "Password, please:", 18))
+     return CR_ERROR;
It's a bit ugly to use concat for sending parameters...
Nicer would be

if (vio->write_packet(vio, PASSWORD_QUESTION, "Password, please:", 18))

Where the command would be stored in a slot of the MYSQL_PLUGIN_VIO on the
server side.

We can look at fixing this if there would be a deamnd for it.

<cut>
...
+ static int perform_dialog(MYSQL_PLUGIN_VIO *vio, MYSQL *mysql)
+ {
+   unsigned char *pkt, cmd= 0;
+   int pkt_len, res;
+   char reply_buf[1024], *reply;
+ 
+   do
+   {
+     /* read the prompt */
+     pkt_len= vio->read_packet(vio, &pkt);
+     if (pkt_len < 0)
+       return CR_ERROR;
+ 
+     if (pkt == 0)
+     {
+       /*
+         in mysql_change_user() the client sends the first packet, so
+         the first vio->read_packet() does nothing (pkt == 0).
+ 
+         We send the "password", assuming the client knows what its doing.
+         (in other words, the dialog plugin should be only set as a default
+         authentication plugin on the client if the first question
+         asks for a password - which will be sent in cleat text, by the way)
cleat -> clear
...
*** sql-common/client.c	2010-02-01 06:14:12 +0000
--- sql-common/client.c	2010-03-02 20:32:24 +0000
*************** my_bool	net_flush(NET *net);
*** 107,112 ****
--- 107,113 ----
#include "client_settings.h"
  #include <sql_common.h>
+ #include <mysql/client_plugin.h>
uint		mysql_port=0;
  char		*mysql_unix_port= 0;
*************** void net_clear_error(NET *net)
*** 332,338 ****
    @param ...       variable number of arguments
  */
! static void set_mysql_extended_error(MYSQL *mysql, int errcode,
                                       const char *sqlstate,
                                       const char *format, ...)
  {
--- 333,339 ----
    @param ...       variable number of arguments
  */
! void set_mysql_extended_error(MYSQL *mysql, int errcode,
                                       const char *sqlstate,
                                       const char *format, ...)
  {
*************** static const char *default_options[]=
*** 1002,1010 ****
    "replication-probe", "enable-reads-from-master", "repl-parse-query",
    "ssl-cipher", "max-allowed-packet", "protocol", "shared-memory-base-name",
    "multi-results", "multi-statements", "multi-queries", "secure-auth",
!   "report-data-truncation",
    NullS
  };
static TYPELIB option_types={array_elements(default_options)-1,
      	     "options",default_options, NULL};
--- 1003,1022 ----
    "replication-probe", "enable-reads-from-master", "repl-parse-query",
    "ssl-cipher", "max-allowed-packet", "protocol", "shared-memory-base-name",
    "multi-results", "multi-statements", "multi-queries", "secure-auth",
!   "report-data-truncation", "plugin-dir", "default-auth",
    NullS
  };
+ enum option_id {
+   OPT_port=1, OPT_socket, OPT_compress, OPT_password, OPT_pipe, OPT_timeout, OPT_user, 
+   OPT_init_command, OPT_host, OPT_database, OPT_debug, OPT_return_found_rows, 
+   OPT_ssl_key, OPT_ssl_cert, OPT_ssl_ca, OPT_ssl_capath, 
+   OPT_character_sets_dir, OPT_default_character_set, OPT_interactive_timeout, 
+   OPT_connect_timeout, OPT_local_infile, OPT_disable_local_infile, 
+   OPT_replication_probe, OPT_enable_reads_from_master, OPT_repl_parse_query,
+   OPT_ssl_cipher, OPT_max_allowed_packet, OPT_protocol, OPT_shared_memory_base_name, 
+   OPT_multi_results, OPT_multi_statements, OPT_multi_queries, OPT_secure_auth, 
+   OPT_report_data_truncation, OPT_plugin_dir, OPT_default_auth, 
+ };
If you add OPT_DUPLICATE= -1 and OPT_ERROR= 1 to the above, you can
cast (find_type(*option+2,&option_types,2)) to the enum and get more
readable code...
...
*** 1035,1040 ****
--- 1047,1060 ----
    return 0;
  }
+ #define extension_free(OPTS, X)                                  \
+     if ((OPTS)->extension)                                       \
+       my_free((OPTS)->extension->X, MYF(MY_ALLOW_ZERO_PTR));     \
+     else                                                         \
+       (OPTS)->extension= (struct st_mysql_options_extention *)   \
+         my_malloc(sizeof(struct st_mysql_options_extention),     \
+                   MYF(MY_WME | MY_ZEROFILL));
Free is a bad name as we actually create some structures here.

How about using 'extension_reset()' instead ?
...
+         case OPT_plugin_dir:
+           extension_free(options, plugin_dir);
+           options->extension->plugin_dir= my_strdup(opt_arg, MYF(MY_WME));
+           break;
+         case OPT_default_auth:
+           extension_free(options, default_auth);
+           options->extension->default_auth= my_strdup(opt_arg, MYF(MY_WME));
+           break;
Even better, would be to do:

extension_set_string(options, default_auth, opt_arg);

Which would make the usage compleately clear.

<cut>
...
*************** int mysql_init_character_set(MYSQL *mysq
*** 1856,1861 ****
--- 1889,2479 ----
  }
  C_MODE_END
+ /*********** client side authentication support **************************/
+ 
+ typedef struct st_mysql_client_plugin_AUTHENTICATION auth_plugin_t;
+ 
+ /* this is a "superset" of MYSQL_PLUGIN_VIO, in C++ I use inheritance */
+ typedef struct {
+   int (*read_packet)(struct st_plugin_vio *vio, uchar **buf);
+   int (*write_packet)(struct st_plugin_vio *vio, const uchar *pkt, int pkt_len);
+   void (*info)(struct st_plugin_vio *vio, struct st_plugin_vio_info *info);
+   /* -= end of MYSQL_PLUGIN_VIO =- */
+   MYSQL *mysql;
+   auth_plugin_t *plugin;            /**< what plugin we're under */
+   const char *db;
+   struct {
+     uchar *pkt;                     /**< pointer into NET::buff */
+     uint pkt_len;
+   } cached_server_reply;
+   int packets_read, packets_written; /**< counters for send/received packets */
+   int mysql_change_user;            /**< if it's mysql_change_user() */
+   int last_read_packet_len;         /**< the length of the last *read* packet */
+ } MCPVIO_EXT;
How about changing counters to uint and mysql_change_user to my_bool ?
...
+ #define native_password_plugin_name "mysql_native_password"
+ #define old_password_plugin_name    "mysql_old_password"
Please add defines to start of file (easier to find them)
...
+ /**
+   sends a client authentication packet (second packet in the 3-way handshake)
+ 
+   Packet format (when the server is 4.0 or earlier):
+    
+     Bytes       Content
+     -----       ----
+     2           client capabilities
+     3           max packet size
+     n           user name, \0-terminated
+     9           scramble_323, \0-terminated
+ 
+   Packet format (when the server is 4.1 or newer):
+    
+     Bytes       Content
+     -----       ----
+     4           client capabilities
+     4           max packet size
+     1           charset number
+     23          reserved (always 0)
+     n           user name, \0-terminated
+     n           plugin auth data (e.g. scramble), length (1 byte) coded
+     n           database name, \0-terminated
+                 (if CLIENT_CONNECT_WITH_DB is set in the capabilities)
+     n           client auth plugin name - \0-terminated string,
+                 (if CLIENT_PLUGIN_AUTH is set in the capabilities)
+ 
+   @retval 0 ok
+   @retval 1 error
+ */
+ static int send_client_reply_packet(MCPVIO_EXT *mpvio,
+                                     const uchar *data, int data_len)
+ {
+   MYSQL *mysql= mpvio->mysql;
+   NET *net= &mysql->net;
+   char *buff, *end;
+ 
+   /* see end= buff+32 below, fixed size of the packet is 32 bytes */
+   buff= my_alloca(33 + USERNAME_LENGTH + data_len + NAME_LEN + NAME_LEN);
+   
+   mysql->client_flag|= mysql->options.client_flag;
+   mysql->client_flag|= CLIENT_CAPABILITIES;
The code would be easier to read (and a bit faster/shorter) if you have
client_flag as a static variable and set mysql->client_flag from this
when all options are set.
...
+ 
+   if (mysql->client_flag & CLIENT_MULTI_STATEMENTS)
+     mysql->client_flag|= CLIENT_MULTI_RESULTS;
+ 
+ #if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
+   if (mysql->options.ssl_key || mysql->options.ssl_cert ||
+       mysql->options.ssl_ca || mysql->options.ssl_capath ||
+       mysql->options.ssl_cipher)
+     mysql->options.use_ssl= 1;
+   if (mysql->options.use_ssl)
+     mysql->client_flag|= CLIENT_SSL;
+ #endif /* HAVE_OPENSSL && !EMBEDDED_LIBRARY*/
Note that we from a client embedded with libmysqld, may also want to
connect to a remote MySQL server.  At some point we need to test and
fix this. (This is why some part of the old embedded code didn't have
defined(EMBEDDED_LIBRARY) in all possible places.

<cut>
...
+ #ifdef HAVE_OPENSSL
How come you don't have !defined(EMBEDDED_LIBRARY) here ?
...
+   }
+ #endif /* HAVE_OPENSSL && !EMBEDDED_LIBRARY */
Comment is wrong or ifdef...

<cut>
...
+   if (data_len)
Add a comment that data_len is always password length here.
...
+   {
+     if (mysql->server_capabilities & CLIENT_SECURE_CONNECTION)
+     {
+       *end++= data_len;
+       memcpy(end, data, data_len);
+       end+= data_len;
+     }
<cut>
...
+ static int client_mpvio_write_packet(struct st_plugin_vio*, const uchar*, int);
Please put static declarations first in the file.
...
+ /**
+   vio->read_packet() callback method for client authentication plugins
+ 
+   This function is called by a client authentication plugin, when it wants
+   to read data from the server.
+ */
Empty line here (makes things easier to read)
...
+ static int client_mpvio_read_packet(struct st_plugin_vio *mpv, uchar **buf)
+ {
+   MCPVIO_EXT *mpvio= (MCPVIO_EXT*)mpv;
+   MYSQL *mysql= mpvio->mysql;
+   ulong  pkt_len;
+ 
+   if (mpvio->packets_read == 0 && !mpvio->cached_server_reply.pkt)
+   {
+     /*
+       the server handshake packet came from the wrong plugin,
+       or it's mysql_change_user(). Either way, there is no data
+       for a plugin to read. send a dummy packet to the server
+       to initiate a dialog.
+     */
+     if (client_mpvio_write_packet(mpv, 0, 0))
+       return (int)packet_error;
+   }
+ 
+   /* there are cached data left, feed it to a plugin */
+   if (mpvio->cached_server_reply.pkt)
+   {
+     *buf= mpvio->cached_server_reply.pkt;
+     mpvio->cached_server_reply.pkt= 0;
+     mpvio->packets_read++;
+     return mpvio->cached_server_reply.pkt_len;
+   }
Move this test first as this allows you to simplify the current first if.
...
+ 
+   /* otherwise read the data */
+   pkt_len= (*mysql->methods->read_change_user_result)(mysql);
+   mpvio->last_read_packet_len= pkt_len;
+   *buf= mysql->net.read_pos;
+ 
+   /* was it a request to change plugins ? */
+   if (**buf == 254)
+     return (int)packet_error; /* if yes, this plugin shan't continue */
shan't -> can't
...
+   /*
+     the server sends \1\255 or \1\254 instead of just \255 or \254 -
+     for us to not confuse it with an error or "change plugin" packets.
+     We remove this escaping \1 here.
+   */
Please add a comment 'see server_mpvio_write_packet()'
...
+   if (pkt_len && **buf == 1)
+   {
+     (*buf)++;
+     pkt_len--;
+   }
+   mpvio->packets_read++;
+   return pkt_len;
+ }
Wouldn't be be better to always send a 1 prefix in all cases or at
least for all not native authorization handlers?

(It would make the protocol easier to understand and also remove some
'if' code) here and in server_mpvio_write_packet()'

However, I agree that it may give a problem for the current native auth
handlers.

If there is no easy solution, we may have to live with this for now...
...
+ /**
+   vio->write_packet() callback method for client authentication plugins
+ 
+   This function is called by a client authentication plugin, when it wants
+   to send data to the server.
+ 
+   It transparently wraps the data into a change user or authentication
+   handshake packet, if neccessary.
+ */
+ static int client_mpvio_write_packet(struct st_plugin_vio *mpv,
+                                      const uchar *pkt, int pkt_len)
+ {
+   int res;
+   MCPVIO_EXT *mpvio= (MCPVIO_EXT*)mpv;
+ 
+   if (mpvio->packets_written == 0)
+   {
+     if (mpvio->mysql_change_user)
+       res= send_change_user_packet(mpvio, pkt, pkt_len);
+     else
+       res= send_client_reply_packet(mpvio, pkt, pkt_len);
It would be nice to not have this if.

As a trick, it's possible to remove all of the above if's.

This would be to set write_packet directly to
send_change_user_packet & send_client_reply_packet

And then end both of these with
mpvio->write_packet=client_mpvio_write_packet;
mpvio->packets_written++;

In this case the packets_written may not even be neccessary to have.
...
+ 
+ /**
+   fills MYSQL_PLUGIN_VIO_INFO structure with the information about the
+   connection
+ */
+ void mpvio_info(Vio *vio, MYSQL_PLUGIN_VIO_INFO *info)
+ {
+   bzero(info, sizeof(*info));
+   switch (vio->type) {
+   case VIO_TYPE_TCPIP:
+     info->protocol= MYSQL_VIO_TCP;
+     info->socket= vio->sd;
+     return;
+   case VIO_TYPE_SOCKET:
+     info->protocol= MYSQL_VIO_SOCKET;
+     info->socket= vio->sd;
+     return;
+   case VIO_TYPE_SSL:
+     {
+       struct sockaddr addr;
+       socklen_t addrlen= sizeof(addr);
+       if (getsockname(vio->sd, &addr, &addrlen))
+         return;
+       info->protocol= addr.sa_family == AF_UNIX ?
+         MYSQL_VIO_SOCKET : MYSQL_VIO_TCP;
+       info->socket= vio->sd;
+       return;
+     }
+ #ifdef _WIN32
+   case VIO_TYPE_NAMEDPIPE:
+     info->protocol= MYSQL_VIO_PIPE;
+     info->handle= vio->hPipe;
+     return;
+   case VIO_TYPE_SHARED_MEMORY:
+     info->protocol= MYSQL_VIO_MEMORY;
+     info->handle= vio->handle_client_file_map; /* or what ? */
+     return;
+ #endif
+   default: DBUG_ASSERT(0);
+   }
+ }
As this is not used by MariaDBL by default, add a comment that for now
this is only used by the socket_peercred plugin.
...
+ int run_plugin_auth(MYSQL *mysql, char *data, uint data_len,
+                     char *data_plugin, const char *db)
+ {
+   const char    *auth_plugin_name;
+   auth_plugin_t *auth_plugin;
+   MCPVIO_EXT    mpvio;
+   ulong		pkt_length;
+   int           res;
+ 
+   /* determine the default/initial plugin to use */
+   if (mysql->options.extension && mysql->options.extension->default_auth &&
+       mysql->server_capabilities & CLIENT_PLUGIN_AUTH)
+     auth_plugin_name= mysql->options.extension->default_auth;
+   else
+     auth_plugin_name= mysql->server_capabilities & CLIENT_PROTOCOL_41 ?
+       native_password_plugin_name : old_password_plugin_name;
+
Better to set auth_plugin directly above.
(Yes, mysql_client_find_plugin() is fast, but setting things directly
is even faster and in this case not more code.
...
+   if (!(auth_plugin= (auth_plugin_t*) mysql_client_find_plugin(mysql,
+                          auth_plugin_name, MYSQL_CLIENT_AUTHENTICATION_PLUGIN)))
+     return 1; /* oops, not found */
+ 
+   mysql->net.last_errno= 0; /* just in case */
+ 
+   if (data_plugin && strcmp(data_plugin, auth_plugin_name))
but of course, here you need to use something like auth_plugin->name instead.

<cut>
...
+   res= auth_plugin->authenticate_user((struct st_plugin_vio *)&mpvio, mysql);
+ 
+   compile_time_assert(CR_OK == -1);
+   compile_time_assert(CR_ERROR == 0);
+   if (res > CR_OK && mysql->net.read_pos[0] != 254)
+   {
+     /*
+       the plugin returned an error. write it down in mysql,
+       unless the error code is CR_ERROR and mysql->net.last_errno
+       is already set (the plugin has done it)
+     */
+     if (res > CR_ERROR)
+       set_mysql_error(mysql, res, unknown_sqlstate);
+     else
+       if (!mysql->net.last_errno)
+         set_mysql_error(mysql, CR_UNKNOWN_ERROR, unknown_sqlstate);
+     return 1;
+   }
+ 
+   /* read the OK packet (or use the cached value in mysql->net.read_pos */
+   if (res == CR_OK)
+     pkt_length= (*mysql->methods->read_change_user_result)(mysql);
+   else /* res == CR_OK_HANDSHAKE_COMPLETE */
+     pkt_length= mpvio.last_read_packet_len;
Note that here res may be CR_ERROR as mysql->net.read_pos[] can
contain 254 as part of some old 'garbage' data, so the comment is
wrong.  I assume that in this case last_read_packet_len is packet_error.
...
+ 
+   if (pkt_length == packet_error)
+   {
+     if (mysql->net.last_errno == CR_SERVER_LOST)
+       set_mysql_extended_error(mysql, CR_SERVER_LOST, unknown_sqlstate,
+                                ER(CR_SERVER_LOST_EXTENDED),
+                                "reading authorization packet",
+                                errno);
+     return 1;
+   }
+ 
+   if (mysql->net.read_pos[0] == 254)
+   {
+     /* The server asked to use a different authentication plugin */
+     if (pkt_length == 1)
+     { /* old "use short scramble" packet */
Please move comment to it's own line. (Hard to read for me)
...
+       auth_plugin_name= old_password_plugin_name;
+       mpvio.cached_server_reply.pkt= (uchar*)mysql->scramble;
+       mpvio.cached_server_reply.pkt_len= SCRAMBLE_LENGTH + 1;
+     }
+     else
+     { /* new "use different plugin" packet */
Same here
...
+       uint len;
+       auth_plugin_name= (char*)mysql->net.read_pos + 1;
+       len= strlen(auth_plugin_name);
Here you may want to add a comment that this is safe becasue
my_net_read() guarantees that the packet ends with \0
...
MYSQL * STDCALL 
  CLI_MYSQL_REAL_CONNECT(MYSQL *mysql,const char *host, const char *user,
*************** CLI_MYSQL_REAL_CONNECT(MYSQL *mysql,cons
*** 1863,1870 ****
             uint port, const char *unix_socket,ulong client_flag)
  {
    char		buff[NAME_LEN+USERNAME_LENGTH+100];
!   char		error_string[1024];
!   char		*end,*host_info= NULL;
    my_socket	sock;
    in_addr_t	ip_addr;
    struct	sockaddr_in sock_addr;
--- 2481,2489 ----
             uint port, const char *unix_socket,ulong client_flag)
  {
    char		buff[NAME_LEN+USERNAME_LENGTH+100];
!   int           scramble_data_len, pkt_scramble_len;
!   char          *end,*host_info, *server_version_end, *pkt_end, *scramble_data;
!   char          *scramble_plugin;
    my_socket	sock;
    in_addr_t	ip_addr;
    struct	sockaddr_in sock_addr;
Will we not get a warning on some platform if we remove host_info= NULL ?
(I think this was the reason we added it)
...
--- 2812,2840 ----
                               PROTOCOL_VERSION);
      goto error;
    }
!   server_version_end= end= strend((char*) net->read_pos+1);
    mysql->thread_id=uint4korr(end+1);
    end+=5;
    /* 
!     Scramble is split into two parts because old clients do not understand
      long scrambles; here goes the first part.
    */
!   scramble_data= end;
!   scramble_data_len= SCRAMBLE_LENGTH_323 + 1;
!   scramble_plugin= old_password_plugin_name;
!   end+= scramble_data_len;
!   if (pkt_end >= end + 1)
      mysql->server_capabilities=uint2korr(end);
!   if (pkt_end >= end + 18)
    {
      /* New protocol with 16 bytes to describe server characteristics */
      mysql->server_language=end[2];
      mysql->server_status=uint2korr(end+3);
+     mysql->server_capabilities|= uint2korr(end+5) << 16;
+     pkt_scramble_len= end[7];
    }
    end+= 18;
Don't we need to check that end + pkt_scramble_len <= pkt_end ?

<cut>
...
*** 3185,3190 ****
--- 3650,3663 ----
      else
        mysql->options.client_flag&= ~CLIENT_SSL_VERIFY_SERVER_CERT;
      break;
+   case MYSQL_PLUGIN_DIR:
+     extension_free(&mysql->options, plugin_dir);
+     mysql->options.extension->plugin_dir= my_strdup(arg, MYF(MY_WME));
+     break;
+   case MYSQL_DEFAULT_AUTH:
+     extension_free(&mysql->options, default_auth);
+     mysql->options.extension->default_auth= my_strdup(arg, MYF(MY_WME));
+     break;
-> extension_set_string()
...
--- sql-common/client_plugin.c	2010-02-19 08:18:09 +0000
***************
*** 0 ****
--- 1,427 ----
+ /* Copyright (C) 2010 Monty Program Ab
-> Add author

<cut>
...
+ static void load_env_plugins(MYSQL *mysql)
+ {
+   char *plugs, *free_env, *s= getenv("LIBMYSQL_PLUGINS");
+ 
+   /* no plugins to load */
+   if(!s)
+     return;
+ 
+   free_env= plugs= my_strdup(s, MYF(MY_WME));
+   s= NULL;
Remove setting 's' to NULL.
...
+ 
+   do {
+     if ((s= strchr(plugs, ';')))
+       *s= '\0';
+     mysql_load_plugin(mysql, plugs, -1, 0);
+     if(s)
Remove if (not needed)
...
+       plugs= ++s;
Change to s+1
...
+   } while (s);
+ 
+   my_free(free_env, MYF(0));
+ }
add space after 'if'
...
+ 
+ /********** extern functions to be used by libmysql *********************/
+ 
+ /**
+   Initializes the client plugin layer.
+ 
+   This function must be called before any other client plugin function.
+ 
+   @retval 0    successful
+   @retval != 0 error occured
+ */
+ int mysql_client_plugin_init()
+ {
+   MYSQL mysql;
+   struct st_mysql_client_plugin **builtin;
+ 
+   if (initialized)
+     return 0;
+   bzero(&mysql, sizeof(mysql)); /* dummy mysql for set_mysql_extended_error */
+ 
+   pthread_mutex_init(&LOCK_load_client_plugin, MY_MUTEX_INIT_SLOW);
+   init_alloc_root(&mem_root, 128, 128);
+ 
+   bzero(&plugin_list, sizeof(plugin_list));
+ 
+   initialized= 1;
+ 
+   pthread_mutex_lock(&LOCK_load_client_plugin);
+ 
+   for (builtin= mysql_client_builtins; *builtin; builtin++)
+     add_plugin(&mysql, *builtin, 0, 0, 0);
+ 
+   pthread_mutex_unlock(&LOCK_load_client_plugin);
+ 
+   load_env_plugins(&mysql);
You don't need mutex_lock() here, as no other thread can access data
until this is setup.  You can make this safe by setting initialized to
1 at end of function.
...
+ 
+   return 0;
+ }
+ 
+ /**
+   Deinitializes the client plugin layer.
+ 
+   Unloades all client plugins and frees any associated resources.
+ */
+ void mysql_client_plugin_deinit()
+ {
+   int i;
+   struct st_client_plugin_int *p;
+ 
+   if (!initialized)
+     return;
Shouldn't this be a DBUG_ASSERT() to detect wrong usage?
...
--- sql/mysqld.cc	2010-02-19 08:18:09 +0000
*************** static int init_common_variables(const c
*** 3490,3495 ****
--- 3492,3498 ----
    if (init_errmessage())	/* Read error messages from file */
      return 1;
    init_client_errs();
+   mysql_library_init(un,us,ed); /* for replication */
Please add comment.  As this can never work, why have it here?
...
=== modified file 'sql/sql_acl.cc'
*** sql/sql_acl.cc	2010-02-01 06:14:12 +0000
<cut>
...
+ struct acl_host_and_ip
+ {
+   char *hostname;
+   long ip,ip_mask;                      // Used with masked ip:s
+ };
Just a note; For future, we should change all strings to use
LEX_STRING (to avoid calling strlen() and faster compare as we can
skip strings with different lengths).
...
+ class ACL_ACCESS {
+ public:
+   ulong sort;
+   ulong access;
+ };
+ 
+ /* ACL_HOST is used if no host is specified */
+ 
+ class ACL_HOST :public ACL_ACCESS
+ {
+ public:
+   acl_host_and_ip host;
+   char *db;
+ };
db is something I wanted to put as LEX_STRING a long time...
(Noting you can do about it just now...)
...
+ 
+ class ACL_USER :public ACL_ACCESS
+ {
+ public:
+   acl_host_and_ip host;
+   uint hostname_length;
+   USER_RESOURCES user_resource;
+   char *user;
+   uint8 salt[SCRAMBLE_LENGTH+1];       // scrambled password in binary form
+   uint8 salt_len;        // 0 - no password, 4 - 3.20, 8 - 4.0,  20 - 4.1.1 
+   enum SSL_type ssl_type;
+   const char *ssl_cipher, *x509_issuer, *x509_subject;
+   LEX_STRING plugin;
+   LEX_STRING auth_string;
+ 
+   ACL_USER *copy(MEM_ROOT *root)
+   {
+     ACL_USER *dst= (ACL_USER *)alloc_root(root, sizeof(ACL_USER));
+     if (!dst)
+       return 0;
+     *dst= *this;
+     dst->user= safe_strdup_root(root, user);
+     dst->ssl_cipher= safe_strdup_root(root, ssl_cipher);
+     dst->x509_issuer= safe_strdup_root(root, x509_issuer);
+     dst->x509_subject= safe_strdup_root(root, x509_subject);
+     if (plugin.str == native_password_plugin_name.str ||
+         plugin.str == old_password_plugin_name.str)
+       dst->plugin= plugin;
+     else
+       dst->plugin.str= strmake_root(root, plugin.str, plugin.length);
In theory, it would be better if we didn't have to test for names but
instead have a flag in the plugin if it's dynamic or not.
...
+     dst->auth_string.str = safe_strdup_root(root, auth_string.str);
+     dst->host.hostname= safe_strdup_root(root, host.hostname);
+     return dst;
+   }
+ };
...
*************** static my_bool acl_load(THD *thd, TABLE_
*** 433,459 ****
        continue;
      }
!     const char *password= get_field(thd->mem_root, table->field[2]);
      uint password_len= password ? strlen(password) : 0;
      set_user_salt(&user, password, password_len);
!     if (user.salt_len == 0 && password_len != 0)
!     {
        switch (password_len) {
        case 45: /* 4.1: to be removed */
!         sql_print_warning("Found 4.1 style password for user '%s@%s'. "
                            "Ignoring user. "
                            "You should change password for this user.",
                            user.user ? user.user : "",
                            user.host.hostname ? user.host.hostname : "");
!         break;
        default:
          sql_print_warning("Found invalid password for user: '%s@%s'; "
                            "Ignoring user", user.user ? user.user : "",
                             user.host.hostname ? user.host.hostname : "");
!         break;
!       }
      }
!     else                                        // password is correct
      {
        uint next_field;
        user.access= get_access(table,3,&next_field) & GLOBAL_ACLS;
--- 545,578 ----
        continue;
      }
!     char *password= get_field(thd->mem_root, table->field[2]);
      uint password_len= password ? strlen(password) : 0;
We should change 'get_field' to also return the length of the string.
I can do that as a separate patch if you don't want to do it.

<cut>
...
--- 649,681 ----
            ptr= get_field(thd->mem_root, table->field[next_field++]);
            user.user_resource.user_conn= ptr ? atoi(ptr) : 0;
          }
! 
!         if (table->s->fields >= 41)
!         {
!           /* We may have plugin & auth_String fields */
!           char *tmpstr= get_field(&mem, table->field[next_field++]);
!           if (tmpstr)
!           {
!             if (user.auth_string.length)
!             {
!               sql_print_warning("'user' entry '%s@%s' has both a password "
!                                 "and an authentication plugin specified. The "
!                                 "password will be ignored.",
!                                 user.user ? user.user : "",
!                                 user.host.hostname ? user.host.hostname : "");
!             }
!             user.plugin.str= tmpstr;
!             user.plugin.length= strlen(tmpstr);
Another strlen() to be removed if we fix get_field()...
...
!             user.auth_string.str= get_field(&mem, table->field[next_field++]);
!             if (!user.auth_string.str)
!               user.auth_string.str= const_cast<char*>("");
!             user.auth_string.length= strlen(user.auth_string.str);
and another one...
...
!           }
!         }
        }
        else
        {
          user.ssl_type=SSL_TYPE_NONE;
  #ifndef TO_BE_REMOVED
          if (table->s->fields <= 13)
          {						// Without grant
...
*************** static int acl_compare(ACL_ACCESS *a,ACL
*** 836,1081 ****
/*
!   Seek ACL entry for a user, check password, SSL cypher, and if
!   everything is OK, update THD user data and USER_RESOURCES struct.
! 
!   IMPLEMENTATION
!    This function does not check if the user has any sensible privileges:
!    only user's existence and  validity is checked.
!    Note, that entire operation is protected by acl_cache_lock.
SYNOPSIS
      acl_getroot()
-     thd         thread handle. If all checks are OK,
-                 thd->security_ctx->priv_user/master_access are updated.
-                 thd->security_ctx->host/ip/user are used for checks.
-     mqh         user resources; on success mqh is reset, else
-                 unchanged
-     passwd      scrambled & crypted password, received from client
-                 (to check): thd->scramble or thd->scramble_323 is
-                 used to decrypt passwd, so they must contain
-                 original random string,
-     passwd_len  length of passwd, must be one of 0, 8,
-                 SCRAMBLE_LENGTH_323, SCRAMBLE_LENGTH
-     'thd' and 'mqh' are updated on success; other params are IN.
-   
-   RETURN VALUE
-     0  success: thd->priv_user, thd->priv_host, thd->master_access, mqh are
-        updated
-     1  user not found or authentication failure
-     2  user found, has long (4.1.1) salt, but passwd is in old (3.23) format.
-    -1  user found, has short (3.23) salt, but passwd is in new (4.1.1) format.
- */
- 
- int acl_getroot(THD *thd, USER_RESOURCES  *mqh,
-                 const char *passwd, uint passwd_len)
- {
-   ulong user_access= NO_ACCESS;
-   int res= 1;
-   ACL_USER *acl_user= 0;
-   Security_context *sctx= thd->security_ctx;
-   DBUG_ENTER("acl_getroot");
- 
-   if (!initialized)
-   {
-     /* 
-       here if mysqld's been started with --skip-grant-tables option.
-     */
-     sctx->skip_grants();
-     bzero((char*) mqh, sizeof(*mqh));
-     DBUG_RETURN(0);
-   }
- 
-   VOID(pthread_mutex_lock(&acl_cache->lock));
- 
-   /*
-     Find acl entry in user database. Note, that find_acl_user is not the same,
-     because it doesn't take into account the case when user is not empty,
-     but acl_user->user is empty
-   */
- 
-   for (uint i=0 ; i < acl_users.elements ; i++)
-   {
-     ACL_USER *acl_user_tmp= dynamic_element(&acl_users,i,ACL_USER*);
-     if (!acl_user_tmp->user || !strcmp(sctx->user, acl_user_tmp->user))
-     {
-       if (compare_hostname(&acl_user_tmp->host, sctx->host, sctx->ip))
-       {
-         /* check password: it should be empty or valid */
-         if (passwd_len == acl_user_tmp->salt_len)
-         {
-           if (acl_user_tmp->salt_len == 0 ||
-               (acl_user_tmp->salt_len == SCRAMBLE_LENGTH ?
-               check_scramble(passwd, thd->scramble, acl_user_tmp->salt) :
-               check_scramble_323(passwd, thd->scramble,
-                                  (ulong *) acl_user_tmp->salt)) == 0)
-           {
-             acl_user= acl_user_tmp;
-             res= 0;
-           }
-         }
-         else if (passwd_len == SCRAMBLE_LENGTH &&
-                  acl_user_tmp->salt_len == SCRAMBLE_LENGTH_323)
-           res= -1;
-         else if (passwd_len == SCRAMBLE_LENGTH_323 &&
-                  acl_user_tmp->salt_len == SCRAMBLE_LENGTH)
-           res= 2;
-         /* linear search complete: */
-         break;
-       }
-     }
-   }
-   /*
-     This was moved to separate tree because of heavy HAVE_OPENSSL case.
-     If acl_user is not null, res is 0.
-   */
- 
-   if (acl_user)
-   {
-     /* OK. User found and password checked continue validation */
- #ifdef HAVE_OPENSSL
-     Vio *vio=thd->net.vio;
-     SSL *ssl= (SSL*) vio->ssl_arg;
-     X509 *cert;
- #endif
- 
-     /*
-       At this point we know that user is allowed to connect
-       from given host by given username/password pair. Now
-       we check if SSL is required, if user is using SSL and
-       if X509 certificate attributes are OK
-     */
-     switch (acl_user->ssl_type) {
-     case SSL_TYPE_NOT_SPECIFIED:		// Impossible
-     case SSL_TYPE_NONE:				// SSL is not required
-       user_access= acl_user->access;
-       break;
- #ifdef HAVE_OPENSSL
-     case SSL_TYPE_ANY:				// Any kind of SSL is ok
-       if (vio_type(vio) == VIO_TYPE_SSL)
- 	user_access= acl_user->access;
-       break;
-     case SSL_TYPE_X509: /* Client should have any valid certificate. */
-       /*
- 	Connections with non-valid certificates are dropped already
- 	in sslaccept() anyway, so we do not check validity here.
- 
- 	We need to check for absence of SSL because without SSL
- 	we should reject connection.
-       */
-       if (vio_type(vio) == VIO_TYPE_SSL &&
- 	  SSL_get_verify_result(ssl) == X509_V_OK &&
- 	  (cert= SSL_get_peer_certificate(ssl)))
-       {
- 	user_access= acl_user->access;
-         X509_free(cert);
-       }
-       break;
-     case SSL_TYPE_SPECIFIED: /* Client should have specified attrib */
-       /*
- 	We do not check for absence of SSL because without SSL it does
- 	not pass all checks here anyway.
- 	If cipher name is specified, we compare it to actual cipher in
- 	use.
-       */
-       if (vio_type(vio) != VIO_TYPE_SSL ||
- 	  SSL_get_verify_result(ssl) != X509_V_OK)
- 	break;
-       if (acl_user->ssl_cipher)
-       {
- 	DBUG_PRINT("info",("comparing ciphers: '%s' and '%s'",
- 			   acl_user->ssl_cipher,SSL_get_cipher(ssl)));
- 	if (!strcmp(acl_user->ssl_cipher,SSL_get_cipher(ssl)))
- 	  user_access= acl_user->access;
- 	else
- 	{
- 	  if (global_system_variables.log_warnings)
- 	    sql_print_information("X509 ciphers mismatch: should be '%s' but is '%s'",
- 			      acl_user->ssl_cipher,
- 			      SSL_get_cipher(ssl));
- 	  break;
- 	}
-       }
-       /* Prepare certificate (if exists) */
-       DBUG_PRINT("info",("checkpoint 1"));
-       if (!(cert= SSL_get_peer_certificate(ssl)))
-       {
- 	user_access=NO_ACCESS;
- 	break;
-       }
-       DBUG_PRINT("info",("checkpoint 2"));
-       /* If X509 issuer is specified, we check it... */
-       if (acl_user->x509_issuer)
-       {
-         DBUG_PRINT("info",("checkpoint 3"));
-         char *ptr = X509_NAME_oneline(X509_get_issuer_name(cert), 0, 0);
-         DBUG_PRINT("info",("comparing issuers: '%s' and '%s'",
- 			   acl_user->x509_issuer, ptr));
-         if (strcmp(acl_user->x509_issuer, ptr))
-         {
-           if (global_system_variables.log_warnings)
-             sql_print_information("X509 issuer mismatch: should be '%s' "
- 			      "but is '%s'", acl_user->x509_issuer, ptr);
-           free(ptr);
-           X509_free(cert);
-           user_access=NO_ACCESS;
-           break;
-         }
-         user_access= acl_user->access;
-         free(ptr);
-       }
-       DBUG_PRINT("info",("checkpoint 4"));
-       /* X509 subject is specified, we check it .. */
-       if (acl_user->x509_subject)
-       {
-         char *ptr= X509_NAME_oneline(X509_get_subject_name(cert), 0, 0);
-         DBUG_PRINT("info",("comparing subjects: '%s' and '%s'",
-                            acl_user->x509_subject, ptr));
-         if (strcmp(acl_user->x509_subject,ptr))
-         {
-           if (global_system_variables.log_warnings)
-             sql_print_information("X509 subject mismatch: should be '%s' but is '%s'",
-                             acl_user->x509_subject, ptr);
-           free(ptr);
-           X509_free(cert);
-           user_access=NO_ACCESS;
-           break;
-         }
-         user_access= acl_user->access;
-         free(ptr);
-       }
-       /* Deallocate the X509 certificate. */
-       X509_free(cert);
-       break;
- #else  /* HAVE_OPENSSL */
-     default:
-       /*
-         If we don't have SSL but SSL is required for this user the 
-         authentication should fail.
-       */
-       break;
- #endif /* HAVE_OPENSSL */
-     }
-     sctx->master_access= user_access;
-     sctx->priv_user= acl_user->user ? sctx->user : (char *) "";
-     *mqh= acl_user->user_resource;
- 
-     if (acl_user->host.hostname)
-       strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME);
-     else
-       *sctx->priv_host= 0;
-   }
-   VOID(pthread_mutex_unlock(&acl_cache->lock));
-   DBUG_RETURN(res);
- }
- 
- 
- /*
-   This is like acl_getroot() above, but it doesn't check password,
-   and we don't care about the user resources.
- 
-   SYNOPSIS
-     acl_getroot_no_password()
        sctx               Context which should be initialized
        user               user name
        host               host name
--- 977,986 ----
<cut>
...
--- 6238,6256 ----
        }
        else
        {
!         push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN, ER_PASSWD_LENGTH,
!                             ER(ER_PASSWD_LENGTH), SCRAMBLED_PASSWORD_CHAR_LENGTH);
          return TRUE;
        }
        combo->password.str= passwd_buff;
      }
! 
!     if (au->plugin.str != native_password_plugin_name.str &&
!         au->plugin.str != old_password_plugin_name.str)
Also here it would be nice with just:
 if (!au->plugin.native_plugin)

...
...
+ /****************************************************************************
+    AUTHENTICATION CODE
+    including initial connect handshake, invoking appropriate plugins,
+    client-server plugin negotiation, COM_CHANGE_USER, and native
+    MySQL authentication plugins.
+ ****************************************************************************/
+ 
+ /* few defines to have less ifdef's in the code below */
+ #ifdef EMBEDDED_LIBRARY
+ #ifdef NO_EMBEDDED_ACCESS_CHECKS
+ #define initialized 0
+ #define decrease_user_connections(X)        /* nothing */
+ #define check_for_max_user_connections(X,Y)   0
+ #endif
+ #undef HAVE_OPENSSL
+ #endif
Move the undef after the first #ifdef;  Makes the code easier to read
and one doesn't have to worry what each #endif stands for.
...
+ #ifndef HAVE_OPENSSL
+ #define ssl_acceptor_fd 0
+ #define sslaccept(A,B,C,D) 1
+ #endif
+ 
+ /**
+   The internal version of what plugins know as MYSQL_PLUGIN_VIO,
+   basically the context of the authentication session
+ */
+ struct MPVIO_EXT : public MYSQL_PLUGIN_VIO
+ {
+   MYSQL_SERVER_AUTH_INFO auth_info;
+   THD *thd;
+   ACL_USER *acl_user;       ///< a copy, independent from acl_users array
+   plugin_ref plugin;        ///< what plugin we're under
+   LEX_STRING db;            ///< db name from the handshake packet
+   /** when restarting a plugin this caches the last client reply */
+   struct {
+     char *plugin, *pkt;     ///< pointers into NET::buff
+     uint pkt_len;
+   } cached_client_reply;
+   /** this caches the first plugin packet for restart request on the client */
+   struct {
+     char *pkt;
+     uint pkt_len;
+   } cached_server_packet;
+   int packets_read, packets_written; ///< counters for send/received packets
+   uint connect_errors;      ///< if there were connect errors for this host
+   /** when plugin returns a failure this tells us what really happened */
+   enum { SUCCESS, FAILURE, RESTART } status;
+ };
Change comments from  ///< to //
(No reason for new style here)
Also change /** to /*
...
+ 
+ /**
+   a helper function to report an access denied error in all the proper places
+ */
+ static void login_failed_error(THD *thd, bool passwd_used)
+ {
+   my_error(ER_ACCESS_DENIED_ERROR, MYF(0),
+            thd->main_security_ctx.user,
+            thd->main_security_ctx.host_or_ip,
+            passwd_used ? ER(ER_YES) : ER(ER_NO));
+   general_log_print(thd, COM_CONNECT, ER(ER_ACCESS_DENIED_ERROR),
+                     thd->main_security_ctx.user,
+                     thd->main_security_ctx.host_or_ip,
+                     passwd_used ? ER(ER_YES) : ER(ER_NO));
+   status_var_increment(thd->status_var.access_denied_errors);
+   /* 
+     Log access denied messages to the error log when log-warnings = 2
+     so that the overhead of the general query log is not required to track 
+     failed connections.
+   */
+   if (global_system_variables.log_warnings > 1)
+   {
+     sql_print_warning(ER(ER_ACCESS_DENIED_ERROR),
+                       thd->main_security_ctx.user,
+                       thd->main_security_ctx.host_or_ip,
+                       passwd_used ? ER(ER_YES) : ER(ER_NO));      
+   }
+ }
Both general_log_print() and sql_print_warning() prints error to log
file.  Shouldn't we remove the first general_log_print() from above?
(I couldn't find the sql_print_warning() in the old code).
...
+ 
+ /**
+   sends a server handshake initialization packet, the very first packet
+   after the connection was established
+ 
+   Packet format:
+    
+     Bytes       Content
+     -----       ----
+     1           protocol version (always 10)
+     n           server version string, \0-terminated
+     4           thread id
+     8           first 8 bytes of the plugin provided data (scramble)
+     1           \0 byte, terminating the first part of a scramble
+     2           server capabilities (two lower bytes)
+     1           server character set
+     2           server status
+     2           server capabilities (two upper bytes)
+     1           length of the scramble
+     10          reserved, always 0
+     n           rest of the plugin provided data (at least 12 bytes)
+     1           \0 byte, terminating the second part of a scramble
+ 
+   @retval 0 ok
+   @retval 1 error
+ */
Shouldn't this function be in sql_connect.c ?
...
+ static bool send_server_handshake_packet(MPVIO_EXT *mpvio,
+                                          const char *data, uint data_len)
+ {
+   DBUG_ASSERT(mpvio->status == MPVIO_EXT::FAILURE);
Add also an assert if data_len > 255.
...
+ 
+   THD *thd= mpvio->thd;
+   char *buff= (char *)my_alloca(1 + SERVER_VERSION_LENGTH + data_len + 64);
+   char scramble_buf[SCRAMBLE_LENGTH];
+   char *end= buff;
+ 
+   *end++= protocol_version;
+ 
+   thd->client_capabilities= CLIENT_BASIC_FLAGS;
+ 
+   if (data_len)
+   {
+     mpvio->cached_server_packet.pkt= (char*)thd->memdup(data, data_len);
+     mpvio->cached_server_packet.pkt_len= data_len;
+   }
+ 
+   if (data_len < SCRAMBLE_LENGTH)
+   {
+     if (data_len)
+     { /*
+         the first packet *must* have at least 20 bytes of a scramble.
+         if a plugin provided less, we pad it to 20 with zeros
+       */
+       memcpy(scramble_buf, data, data_len);
+       bzero(scramble_buf+data_len, SCRAMBLE_LENGTH-data_len);
+       data= scramble_buf;
+     }
+     else
+     {
+       /*
+         if the default plugin does not provide the data for the scramble at
+         all, we generate a scramble internally anyway, just in case the
+         user account (that will be known only later) uses a
+         native_password_plugin (which needs a scramble). If we don't send a
+         scramble now - wasting 20 bytes in the packet -
+         native_password_plugin will have to send it in a separate packet,
+         adding one more round trip.
+       */
+       create_random_string(thd->scramble, SCRAMBLE_LENGTH, &thd->rand);
+       data= thd->scramble;
+     }
You could move the above to an else of the first if (data_len) and
move the first part inside the first if.
This saves one-two if's during execution and makes code shorter.
...
+     data_len= SCRAMBLE_LENGTH;
+   }
+ 
+   if (opt_using_transactions)
+     thd->client_capabilities|= CLIENT_TRANSACTIONS;
+ 
+   thd->client_capabilities|= CAN_CLIENT_COMPRESS;
+ 
+   if (ssl_acceptor_fd)
+   {
+     thd->client_capabilities |= CLIENT_SSL;
+     thd->client_capabilities |= CLIENT_SSL_VERIFY_SERVER_CERT;
+   }
+ 
+   end= strnmov(end, server_version, SERVER_VERSION_LENGTH) + 1;
+   int4store((uchar*) end, mpvio->thd->thread_id);
+   end+= 4;
+ 
+   /*
+     Old clients does not understand long scrambles, but can ignore packet
+     tail: that's why first part of the scramble is placed here, and second
+     part at the end of packet.
+   */
+   end= (char*)memcpy(end, data, SCRAMBLE_LENGTH_323);
+   end+= SCRAMBLE_LENGTH_323;
+   *end++= 0;
+  
+   int2store(end, thd->client_capabilities);
+   /* write server characteristics: up to 16 bytes allowed */
+   end[2]=(char) default_charset_info->number;
+   int2store(end+3, mpvio->thd->server_status);
+   int2store(end+5, thd->client_capabilities >> 16);
+   end[7]= data_len;
+   bzero(end+8, 10);
+   end+= 18;
+   /* write scramble tail */
+   end= (char*)memcpy(end, data + SCRAMBLE_LENGTH_323,
+                      data_len - SCRAMBLE_LENGTH_323);
Note that in old code, we had an end zero after scramble, that you
don't have here. I checked the client code and at least in the .c
client it's safe to nto have the end \0.  You should probable check
with other native drivers that it's safe with them too.
(Especially the php driver).
...
+   end+= data_len - SCRAMBLE_LENGTH_323;
+   end= strmake(end, plugin_name(mpvio->plugin)->str,
+                     plugin_name(mpvio->plugin)->length);
+ 
+   int res= my_net_write(&mpvio->thd->net, (uchar*) buff, (size_t) (end-buff)) ||
+            net_flush(&mpvio->thd->net);
Please move declaration of 'res' to function start.
...
+   my_afree(buff);
+   return res;
+ }
+ 
+ static uchar switch_plugin_request_buf[]= { 254 };
Move to file start.
...
+ 
+ /**
+   sends a "change plugin" packet, requesting a client to restart authentication
+   using a different authentication plugin
+ 
+   Packet format:
+    
+     Bytes       Content
+     -----       ----
+     1           byte with the value 254
+     n           client plugin to use, \0-terminated
+     n           plugin provided data
+ 
+   In a special case of switching from native_password_plugin to
+   old_password_plugin, the packet contains only one - the first - byte,
+   plugin name is omitted, plugin data aren't needed as the scramble was
+   already sent. This one-byte packet is identical to the "use the short
+   scramble" packet in the protocol before plugins were introduced.
+ 
+   @retval 0 ok
+   @retval 1 error
+ */
Maybe this also should be in sql_connect.cc ?
...
+ static bool send_plugin_request_packet(MPVIO_EXT *mpvio,
+                                        const uchar *data, uint data_len)
+ {
+   DBUG_ASSERT(mpvio->packets_written == 1);
+   DBUG_ASSERT(mpvio->packets_read == 1);
+   NET *net= &mpvio->thd->net;
+ 
+   mpvio->status= MPVIO_EXT::FAILURE; // the status is no longer RESTART
+ 
+   const char *client_auth_plugin=
+     ((st_mysql_auth *)(plugin_decl(mpvio->plugin)->info))->client_auth_plugin;
+ 
+   DBUG_ASSERT(client_auth_plugin);
Wouldn't it be better to make this a LEX_STRING *?
(You can then avoid the strlen() later on)
...
+ 
+   /*
+     we send an old "short 4.0 scramble request", if we need to request a
+     client to use 4.0 auth plugin (short scramble) and the scramble was
+     already sent to the client
+   */
+   bool switch_from_long_to_short_scramble=
Please move all variable declaration to start or in this case, just
change this to one if without a variable
...
+     my_strcasecmp(system_charset_info, native_password_plugin_name.str,
+                   mpvio->cached_client_reply.plugin) == 0 &&
+     client_auth_plugin == old_password_plugin_name.str;
+ 
+   if (switch_from_long_to_short_scramble)
+     return my_net_write(net, switch_plugin_request_buf, 1) ||
+            net_flush(net);
Please add to the above comment how 
mpvio->cached_client_reply.plugin and client_auth_plugin comes are set?
...
From IRC:
client_auth_plugin is set from the data in the users table.
mpvio->cached_client_reply.plugin contains what client tried to use.
...
+ 
+   /*
+     We never request a client to switch from a short to long scramble.
+     Plugin-aware clients can do that, but traditionally it meant to
+     ask an old 4.0 client to use the new 4.1 authentication protocol.
+   */
+   bool switch_from_short_to_long_scramble=
+     my_strcasecmp(system_charset_info, old_password_plugin_name.str,
+                   mpvio->cached_client_reply.plugin) == 0 &&
+     client_auth_plugin == native_password_plugin_name.str;
+ 
+   if (switch_from_short_to_long_scramble)
Remove variable.
...
+   {
+     my_error(ER_NOT_SUPPORTED_AUTH_MODE, MYF(0));
+     general_log_print(mpvio->thd, COM_CONNECT, ER(ER_NOT_SUPPORTED_AUTH_MODE));
+     return 1;
+   }
+ 
+   return net_write_command(net, switch_plugin_request_buf[0],
+                            (uchar*)client_auth_plugin,
+                            strlen(client_auth_plugin)+1,
+                            (uchar*)data, data_len);
+ }
+ 
+ #ifndef NO_EMBEDDED_ACCESS_CHECKS
+ /**
+    Finds acl entry in user database for authentication purposes.
+    
+    Finds a user and copies it into mpvio. Reports an authentication
+    failure if a user is not found.
+ 
+    @note find_acl_user is not the same, because it doesn't take into
+    account the case when user is not empty, but acl_user->user is empty
+ 
+    @retval 0    found
+    @retval 1    not found
+ */
Please add blank line for each /*^Jstatic expression.
...
+ static bool find_mpvio_user(MPVIO_EXT *mpvio, Security_context *sctx)
+ {
+   pthread_mutex_lock(&acl_cache->lock);
+   for (uint i=0 ; i < acl_users.elements ; i++)
+   {
+     ACL_USER *acl_user_tmp= dynamic_element(&acl_users,i,ACL_USER*);
+     if (!acl_user_tmp->user || (!strcmp(sctx->user, acl_user_tmp->user) &&
+         compare_hostname(&acl_user_tmp->host, sctx->host, sctx->ip)))
Fix indentation (move (!strcmp to next line))

Isn't the test wrong ?

Shouldn't it be:

if (((!acl_user_tmp->user ||
     (acl_user_tmp->user && strcmp(user, acl_user_tmp->user) == 0))) &&
    compare_hostname(&acl_user_tmp->host, sctx->host, sctx->ip)

As now compare_hostname() is not called if user is NULL
...
+         
+     {
+       mpvio->acl_user= acl_user_tmp->copy(mpvio->thd->mem_root);
+       break;
+     }
+   }
+   pthread_mutex_unlock(&acl_cache->lock);
+ 
+   if (!mpvio->acl_user)
+   {
+     login_failed_error(mpvio->thd, 0);
+     return 1;
+   }
Add an assert at function start that this is zero.
...
+ 
+   /* user account requires non-default plugin and the client is too old */
+   if (my_strcasecmp(system_charset_info, mpvio->acl_user->plugin.str,
+                                          native_password_plugin_name.str) &&
+       my_strcasecmp(system_charset_info, mpvio->acl_user->plugin.str,
+                                          old_password_plugin_name.str) &&
Haveing a 'native' plugin field, would make this test:

  if (!mpvio->acl_user->plugin.native_plugin)

I think we really should do this as it makes code shorter and we don't
have to do a lot of name comparisons everywhere.

<cut>
...
+   mpvio->auth_info.user_name= sctx->user;
+   mpvio->auth_info.auth_string= mpvio->acl_user->auth_string.str;
+   strmake(mpvio->auth_info.authenticated_as, mpvio->acl_user->user ?
+           mpvio->acl_user->user : "", USERNAME_LENGTH);
I plan to fix that acl_user->user is never a null pointer.  This will
remove a lot of if's in the code.
...
+ 
+   return 0;
+ }
+ #endif
+ 
+ /* the packet format is described in send_change_user_packet() */
+ static bool parse_com_change_user_packet(MPVIO_EXT *mpvio, uint packet_length)
+ {
+   THD *thd= mpvio->thd;
+   NET *net= &thd->net;
+   Security_context *sctx= thd->security_ctx;
+ 
+   char *user= (char*) net->read_pos;
+   char *end= user + packet_length;
+   /* Safe because there is always a trailing \0 at the end of the packet */
+   char *passwd= strend(user)+1;
+   uint user_len= passwd - user - 1;
+   char *db= passwd;
+   char db_buff[NAME_LEN + 1];                 // buffer to store db in utf8
+   char user_buff[USERNAME_LENGTH + 1];	      // buffer to store user in utf8
+   uint dummy_errors;
+ 
+   if (passwd >= end)
+   {
+     my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
+     return 1;
+   }
+ 
+   /*
+     Old clients send null-terminated string as password; new clients send
+     the size (1 byte) + string (not null-terminated). Hence in case of empty
+     password both send '\0'.
+ 
+     This strlen() can't be easily deleted without changing protocol.
+ 
+     Cast *passwd to an unsigned char, so that it doesn't extend the sign for
+     *passwd > 127 and become 2**32-127+ after casting to uint.
+   */
+   uint passwd_len= (thd->client_capabilities & CLIENT_SECURE_CONNECTION ?
+                     (uchar)(*passwd++) : strlen(passwd));
Move declaration first
...
+ 
+   db+= passwd_len + 1;
Better to do:

db= passwd + passwd_len + 1;

And remove initialization from start.
...
+   /*
+     Database name is always NUL-terminated, so in case of empty database
+     the packet must contain at least the trailing '\0'.
+   */
+   if (db >= end)
+   {
+     my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
+     return 1;
+   }
+ 
+   uint db_len= strlen(db);
+ 
+   char *ptr= db + db_len + 1;
Move declarations first.
...
+ 
+   /* Convert database and user names to utf8 */
+   db_buff[db_len= copy_and_convert(db_buff, sizeof(db_buff)-1,
+                                    system_charset_info, db, db_len,
+                                    thd->charset(), &dummy_errors)]= 0;
We don't need to set end \0 here as we use make__lex_string() on
db_buff.
...
+   db= db_buff;
+ 
+   user_buff[user_len= copy_and_convert(user_buff, sizeof(user_buff)-1,
+                                        system_charset_info, user, user_len,
+                                        thd->charset(), &dummy_errors)]= '\0';
We shouldn't set end \0 here either.
Better to use my_strndup() below.
(It's also faster than my_strdup())
...
+   user= user_buff;
+ 
+   if (ptr+1 < end)
+   {
+     uint cs_number= uint2korr(ptr);
+     thd_init_client_charset(thd, cs_number);
+     thd->update_charset();
Add ptr+= 2 here (Makes rest of code simpler).
...
+   }
We should first set the character set and then do copy_and_convert()

(The old code on sql_connect() did it that way)
...
+ 
+   if (!(sctx->user= my_strdup(user, MYF(MY_WME))))
+     return 1;
my_strdup -> my_strndup()
...
+ 
+   /* Clear variables that are allocated */
+   thd->user_connect= 0;
+   strmake(sctx->priv_user, sctx->user, USERNAME_LENGTH);
+ 
+   if (thd->make_lex_string(&mpvio->db, db, db_len, 0) == 0)
+     return 1; /* The error is set by make_lex_string(). */
+ 
+   /*
+     Clear thd->db as it points to something, that will be freed when
+     connection is closed. We don't want to accidentally free a wrong
+     pointer if connect failed.
+   */
+   thd->reset_db(NULL, 0);
+ 
+   if (!initialized)
+   {
+     // if mysqld's been started with --skip-grant-tables option
+     mpvio->status= MPVIO_EXT::SUCCESS;
+     return 0;
+   }
+ 
+ #ifndef NO_EMBEDDED_ACCESS_CHECKS
+   if (find_mpvio_user(mpvio, sctx))
+     return 1;
+ 
+   char *client_plugin;
+   if (thd->client_capabilities & CLIENT_PLUGIN_AUTH)
+   {
+     client_plugin= ptr + 2;
Use client_plugin= ptr;
...
+     if (client_plugin >= end)
+     {
+       my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
+       return 1;
+     }
+   }
+   else
Add brace here
...
+     if (thd->client_capabilities & CLIENT_SECURE_CONNECTION)
+       client_plugin= native_password_plugin_name.str;
+     else
+     {
+       client_plugin=  old_password_plugin_name.str;
+       /*
+         For a passwordless accounts we use native_password_plugin.
+         But when an old 4.0 client connects to it, we change it to
+         old_password_plugin, otherwise MySQL will think that server 
+         and client plugins don't match.
+       */
+       if (mpvio->acl_user->auth_string.length == 0)
+         mpvio->acl_user->plugin= old_password_plugin_name;
+     }
+   
+   /* remember the data part of the packet, to present it to plugin in read_packet() */
+   mpvio->cached_client_reply.pkt= passwd;
+   mpvio->cached_client_reply.pkt_len= passwd_len;
+   mpvio->cached_client_reply.plugin= client_plugin;
Consider adding here
   mpvio->cached_client_reply.native_plugin= 0 | 1;

(I know it requires a two strcasecmp() when the client packet contains
a plugin, but as this is not the normal case, it's ok.
...
+   mpvio->status= MPVIO_EXT::RESTART;
+ #endif
+ 
+   return 0;
+ }
+ 
+ /* the packet format is described in send_client_reply_packet() */
+ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio,
+                                            uchar **buff, ulong pkt_len)
+ {
+ #ifndef EMBEDDED_LIBRARY
+   THD *thd= mpvio->thd;
+   NET *net= &thd->net;
+   char *end;
+ 
+   DBUG_ASSERT(mpvio->status == MPVIO_EXT::FAILURE);
+ 
+   if (pkt_len < MIN_HANDSHAKE_SIZE)
+     return packet_error;
+ 
+   if (mpvio->connect_errors)
+     reset_host_errors(&net->vio->remote.sin_addr);
+ 
+   ulong client_capabilities= uint2korr(net->read_pos);
Move declaration to function start
...
+   if (client_capabilities & CLIENT_PROTOCOL_41)
+   {
+     client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) << 16;
+     thd->max_client_packet_length= uint4korr(net->read_pos+4);
+     DBUG_PRINT("info", ("client_character_set: %d", (uint) net->read_pos[8]));
+     thd_init_client_charset(thd, (uint) net->read_pos[8]);
+     thd->update_charset();
+     end= (char*) net->read_pos+32;
+   }
+   else
+   {
+     thd->max_client_packet_length= uint3korr(net->read_pos+2);
+     end= (char*) net->read_pos+5;
+   }
+ 
+   /* Disable those bits which are not supported by the client. */
Should be 'server' not 'client' as client_capabilities are initially
set by server.
...
+   thd->client_capabilities&= client_capabilities;
+ 
+   if (thd->client_capabilities & CLIENT_IGNORE_SPACE)
+     thd->variables.sql_mode|= MODE_IGNORE_SPACE;
+ 
+   DBUG_PRINT("info", ("client capabilities: %lu", thd->client_capabilities));
+   if (thd->client_capabilities & CLIENT_SSL)
+   {
+     char error_string[1024] __attribute__((unused));
+ 
+     /* Do the SSL layering. */
+     if (!ssl_acceptor_fd)
+       return packet_error;
+ 
+     DBUG_PRINT("info", ("IO layer change in progress..."));
+     if (sslaccept(ssl_acceptor_fd, net->vio, net->read_timeout, error_string))
+     {
+       DBUG_PRINT("error", ("Failed to accept new SSL connection"));
+       return packet_error;
+     }
+ 
+     DBUG_PRINT("info", ("Reading user information over SSL layer"));
+     pkt_len= my_net_read(net);
+     if (pkt_len == packet_error || pkt_len < NORMAL_HANDSHAKE_SIZE)
+     {
+       DBUG_PRINT("error", ("Failed to read user information (pkt_len= %lu)",
+ 			   pkt_len));
+       return packet_error;
+     }
+   }
+ 
+   if (end >= (char*) net->read_pos+ pkt_len +2)
+     return packet_error;
+ 
+   if (thd->client_capabilities & CLIENT_INTERACTIVE)
+     thd->variables.net_wait_timeout= thd->variables.net_interactive_timeout;
+   if ((thd->client_capabilities & CLIENT_TRANSACTIONS) &&
+       opt_using_transactions)
+     net->return_status= &thd->server_status;
+ 
+   char *user= end;
+   char *passwd= strend(user)+1;
+   uint user_len= passwd - user - 1;
+   char *db= passwd;
+   char db_buff[NAME_LEN + 1];           // buffer to store db in utf8
+   char user_buff[USERNAME_LENGTH + 1];	// buffer to store user in utf8
+   uint dummy_errors;
Please move variable declaration to function start.
...
+ 
+   /*
+     Old clients send null-terminated string as password; new clients send
+     the size (1 byte) + string (not null-terminated). Hence in case of empty
+     password both send '\0'.
+ 
+     This strlen() can't be easily deleted without changing protocol.
+ 
+     Cast *passwd to an unsigned char, so that it doesn't extend the sign for
+     *passwd > 127 and become 2**32-127+ after casting to uint.
+   */
+   uint passwd_len= thd->client_capabilities & CLIENT_SECURE_CONNECTION ?
+                    (uchar)(*passwd++) : strlen(passwd);
+   db= thd->client_capabilities & CLIENT_CONNECT_WITH_DB ?
+         db + passwd_len + 1 : 0;
+   /* strlen() can't be easily deleted without changing protocol */
+   uint db_len= db ? strlen(db) : 0;
Please clean up the above test to:
(Yes, I know it's old code, but better to fix it now as we are working
on it)

db_len= 0;
db= 0;
if (thd->client_capabilities & CLIENT_CONNECT_WITH_DB)
{
  db= passwd + passwd_len + 1;
  /* strlen() can't be easily deleted without changing protocol */
  db_len= strlen(db);
}
...
+ 
+   if (passwd + passwd_len + db_len > (char *)net->read_pos + pkt_len)
+     return packet_error;
+ 
+   char *client_plugin= passwd + passwd_len + (db ? db_len + 1 : 0);
client_plugin= passwd + passwd_len + db_len + test(db_len);
...
+ 
+   /* Since 4.1 all database names are stored in utf8 */
+   if (db)
+   {
+     db_buff[db_len= copy_and_convert(db_buff, sizeof(db_buff)-1,
+                                      system_charset_info,
+                                      db, db_len,
+                                      thd->charset(), &dummy_errors)]= 0;
No reason to set \0
...
+     db= db_buff;
+   }
+ 
+   user_buff[user_len= copy_and_convert(user_buff, sizeof(user_buff)-1,
+                                        system_charset_info, user, user_len,
+                                        thd->charset(), &dummy_errors)]= '\0';
No reason to set \0
...
+   user= user_buff;
+ 
+   /* If username starts and ends in "'", chop them off */
+   if (user_len > 1 && user[0] == '\'' && user[user_len - 1] == '\'')
+   {
+     user[user_len-1]= 0;
Remove setting of \0
...
+     user++;
+     user_len-= 2;
+   }
+ 
+   Security_context *sctx= thd->security_ctx;
+ 
+   if (thd->make_lex_string(&mpvio->db, db, db_len, 0) == 0)
+     return packet_error; /* The error is set by make_lex_string(). */
+   if (sctx->user)
+     x_free(sctx->user);
+   if (!(sctx->user= my_strdup(user, MYF(MY_WME))))
strdup-> strndup
...
+     return packet_error; /* The error is set by my_strdup(). */
+ 
+   /*
+     Clear thd->db as it points to something, that will be freed when
+     connection is closed. We don't want to accidentally free a wrong
+     pointer if connect failed.
+   */
+   thd->reset_db(NULL, 0);
+ 
+   if (!initialized)
+   {
+     // if mysqld's been started with --skip-grant-tables option
+     mpvio->status= MPVIO_EXT::SUCCESS;
+     return packet_error;
+   }
+ 
+   if (find_mpvio_user(mpvio, sctx))
+     return packet_error;
+ 
+   if (thd->client_capabilities & CLIENT_PLUGIN_AUTH)
+   {
+     if ((client_plugin + strlen(client_plugin)) > 
+           (char *)net->read_pos + pkt_len)
+       return packet_error;
+   }
+   else
Add brace
...
+     if (thd->client_capabilities & CLIENT_SECURE_CONNECTION)
+       client_plugin= native_password_plugin_name.str;
+     else
+     {
+       client_plugin=  old_password_plugin_name.str;
+       /*
+         For a passwordless accounts we use native_password_plugin.
+         But when an old 4.0 client connects to it, we change it to
+         old_password_plugin, otherwise MySQL will think that server 
+         and client plugins don't match.
+       */
+       if (mpvio->acl_user->auth_string.length == 0)
+         mpvio->acl_user->plugin= old_password_plugin_name;
+     }
+   
<cut>
...
+   /*
+     ok, we don't need to restart the authentication on the server.
+     but if the client used the wrong plugin, we need to restart
+     the authentication on the client. Do it here, the server plugin
+     doesn't need to know.
+   */
+   const char *client_auth_plugin=
+     ((st_mysql_auth *)(plugin_decl(mpvio->plugin)->info))->client_auth_plugin;
Would be better as a LEX_STRING *, so ether comments.
...
+ 
+   if (client_auth_plugin &&
+       my_strcasecmp(system_charset_info, client_plugin, client_auth_plugin))
+   {
+     mpvio->cached_client_reply.plugin= client_plugin;
+     if (send_plugin_request_packet(mpvio,
+                                    (uchar*)mpvio->cached_server_packet.pkt,
+                                    mpvio->cached_server_packet.pkt_len))
+       return packet_error;
+ 
+     passwd_len= my_net_read(&mpvio->thd->net);
+     passwd = (char*)mpvio->thd->net.read_pos;
+   }
+ 
+   *buff= (uchar*)passwd;
+   return passwd_len;
+ #else
+   return 0;
+ #endif
+ }
+
Add empty line
...
+ /**
+   vio->write_packet() callback method for server authentication plugins
+ 
+   This function is called by a server authentication plugin, when it wants
+   to send data to the client.
+ 
+   It transparently wraps the data into a handshake packet,
+   and handles plugin negotiation with the client. If necessary,
+   it escapes the plugin data, if it starts with a mysql protocol packet byte.
+ */
+ static int server_mpvio_write_packet(MYSQL_PLUGIN_VIO *param,
+                                    const uchar *packet, int packet_len)
+ {
+   MPVIO_EXT *mpvio= (MPVIO_EXT*)param;
+   int res;
Add empty line
...
+   /* reset cached_client_reply */
+   mpvio->cached_client_reply.pkt= 0;
<cut>
...
+ static int server_mpvio_read_packet(MYSQL_PLUGIN_VIO *param, uchar **buf)
+ {
+   MPVIO_EXT *mpvio= (MPVIO_EXT*)param;
+   ulong pkt_len;
+ 
+   if (mpvio->packets_written == 0)
+   {
+     /*
+       plugin wants to read the data without sending anything first.
+       send an empty packet, to start a handshake
+     */
Change 'empty packet' to 'server_handshake_packet'.
Here, the code would be much clearer if we would just use:

  mpvio->cached_client_reply.pkt= 0;
  mpvio->packets_written++;
  if (send_server_handshake_packet(mpvio, 0,0))
...
+     if (server_mpvio_write_packet(mpvio, 0, 0))
+       pkt_len= packet_error;
+     else
+       pkt_len= my_net_read(&mpvio->thd->net);
+   }
+   else
Move if to after else
...
+   if (mpvio->cached_client_reply.pkt)
+   {
+     DBUG_ASSERT(mpvio->status == MPVIO_EXT::RESTART);
+     DBUG_ASSERT(mpvio->packets_read > 0);
+     /*
+       if the have the data cached from the last server_mpvio_read_packet
+       (which can be the case if it's a restarted authentication)
+       and a client has used the correct plugin, then we can return the
+       cached data straight away and avoid one round trip.
+     */
+     const char *client_auth_plugin=
+       ((st_mysql_auth *)(plugin_decl(mpvio->plugin)->info))->client_auth_plugin;
+     if (client_auth_plugin == 0 ||
+         my_strcasecmp(system_charset_info, mpvio->cached_client_reply.plugin,
+                       client_auth_plugin) == 0)
+     {
+       mpvio->status= MPVIO_EXT::FAILURE;
+       *buf= (uchar*)mpvio->cached_client_reply.pkt;
+       mpvio->cached_client_reply.pkt= 0;
+       mpvio->packets_read++;
+       return (int)mpvio->cached_client_reply.pkt_len;
+     }
+     /*
+       But if the client has used the wrong plugin, the cached data are
+       useless. Furthermore, we have to send a "change plugin" request
+       to the client.
+     */
Here also, it would be clearer if we did:

   mpvio->cached_client_reply.pkt= 0;
   mpvio->packets_written++;
   if (send_plugin_request_packet(mpvio, 0, 0))

(Of course, we can do the packets_written in
send_plugin_request_packet and send_server_handshake_packet() to
remove the above increments.

<cut>
...
+ /**
+   fills MYSQL_PLUGIN_VIO_INFO structure with the information about the
+   connection
+ */
+ static void server_mpvio_info(MYSQL_PLUGIN_VIO *vio,
+                               MYSQL_PLUGIN_VIO_INFO *info)
+ {
+   MPVIO_EXT *mpvio= (MPVIO_EXT*)vio;
+   mpvio_info(mpvio->thd->net.vio, info);
+ }
+ 
+ static int acl_check_ssl(THD *thd, ACL_USER *acl_user)
int -> bool

Add function comment (at least regarding return values)

<cut>
...
+ /**
+   Perform the handshake, authorize the client and update thd sctx variables.
+ 
+   @param thd                     thread handle
+   @param connect_errors          number of previous failed connect attemps
+                                  from this host
+   @param com_change_user_pkt_len size of the COM_CHANGE_USER packet
+                                  (without the first, command, byte) or 0
+                                  if it's not a COM_CHANGE_USER (that is, if
+                                  it's a new connection)
+ 
+   @retval 0  success, thd is updated.
+   @retval 1  error
+ */
+ int acl_authenticate(THD *thd, uint connect_errors, uint com_change_user_pkt_len)
int -> bool
...
+ {
+   DBUG_ENTER("acl_authenticate");
Add DBUG_ENTER() after variable declarations.
...
+   int res, old_status;
+   plugin_ref plugin;
+   MPVIO_EXT mpvio;
+   st_mysql_auth *auth;
+   LEX_STRING *auth_plugin_name= default_auth_plugin_name;
+   bool unlock_plugin;
+   enum  enum_server_command command= com_change_user_pkt_len ? COM_CHANGE_USER
+                                                              : COM_CONNECT;
+ 
+   compile_time_assert(MYSQL_USERNAME_LENGTH == USERNAME_LENGTH);
+ 
+   bzero(&mpvio, sizeof(mpvio));
+   mpvio.read_packet= server_mpvio_read_packet;
+   mpvio.write_packet= server_mpvio_write_packet;
+   mpvio.info= server_mpvio_info;
+   mpvio.thd= thd;
+   mpvio.connect_errors= connect_errors;
+   mpvio.status= MPVIO_EXT::FAILURE;
+ 
+   if (com_change_user_pkt_len == 0)
+     thd->scramble[SCRAMBLE_LENGTH]= 1; // it means - there is no scramble yet
Wouldn't it be better to use a separate flag or state for this?
Using part of scramble, even if it's safe, looks strange and hard to
remember.  Having a state in MPVIO_EXT of what has taken place could
be a good addition.  This could be better than just incrementing
packets_written / packets_read (in the case we ever want to do more
round trips than 2)
...
+   else
+   {
+     mpvio.packets_written++; // pretend that a server handshake packet was sent
+     mpvio.packets_read++;    // take COM_CHANGE_USER packet into account
+ 
+     if (parse_com_change_user_packet(&mpvio, com_change_user_pkt_len))
+       DBUG_RETURN(1);
+ 
+     DBUG_ASSERT(mpvio.status == MPVIO_EXT::RESTART ||
+                 mpvio.status == MPVIO_EXT::SUCCESS);
+     /*
+       we skip the first step of the authentication -
+       the one that starts a default plugin, sends the handshake packet with the
+       scramble and reads the packet with the user name.
+       in COM_CHANGE_USER the client already has the scramble and we already
+       know the user name
+     */
+     goto skip_first;
Noe that 'res' and 'old_status' variables are not set here,
which may be a problem as they could be tested below.
...
+   }
+
The 'goto's here are not nice.
A way to fix it is to move the code between 'retry' and 'skip_first'
to a function and use:

if (!mpvio.plugin= get_plugin(auth_plugin_name))
  DBUG_RETURN(1);
In the above if and instead of 'goto retry'.
With this, you can remove both 'retry' and 'skip_first'
and replace this with a for (;;) loop.
...
+ retry:
+   unlock_plugin= false;
+   if (auth_plugin_name->str == native_password_plugin_name.str)
+     plugin= native_password_plugin;
+   else
+ #ifndef EMBEDDED_LIBRARY
+   if (auth_plugin_name->str == old_password_plugin_name.str)
+     plugin= old_password_plugin;
+   else
+   if ((plugin= my_plugin_lock_by_name(thd, auth_plugin_name,
+                                        MYSQL_AUTHENTICATION_PLUGIN)))
+     unlock_plugin= true;
+   else
+ #endif
+   {
+     /* Server cannot load required plugin. */
+     my_error(ER_PLUGIN_IS_NOT_LOADED, MYF(0), auth_plugin_name->str);
+     DBUG_RETURN(1);
+   }
+ 
+   mpvio.plugin= plugin;
+   auth= (st_mysql_auth*)plugin_decl(plugin)->info;
+ 
+   old_status= mpvio.status;
+   res= auth->authenticate_user(&mpvio, &mpvio.auth_info);
+ 
+   if (unlock_plugin)
+     plugin_unlock(thd, plugin);
+ 
+ skip_first:
<cut>
...
+ 
+   if (res > CR_OK && mpvio.status != MPVIO_EXT::SUCCESS)
How about res != CR_OK ? (More clear)
...
+   {
+     DBUG_ASSERT(mpvio.status == MPVIO_EXT::FAILURE);
+ 
+     if (!thd->is_error())
+       login_failed_error(thd, thd->password);
+     DBUG_RETURN(1);
+   }
<cut>
...
+   if (command == COM_CONNECT &&
+       !(thd->main_security_ctx.master_access & SUPER_ACL))
+   {
+     pthread_mutex_lock(&LOCK_connection_count);
+     bool count_ok= (*thd->scheduler->connection_count <=
+                *thd->scheduler->max_connections);
Indentation...
...
+     VOID(pthread_mutex_unlock(&LOCK_connection_count));
+     if (!count_ok)
+     {                                         // too many connections
+       my_error(ER_CON_COUNT_ERROR, MYF(0));
+       DBUG_RETURN(1);
+     }
+   }
+ 
+   /*
+     This is the default access rights for the current database.  It's
+     set to 0 here because we don't have an active database yet (and we
+     may not have an active database to set.
+   */
+   sctx->db_access=0;
+ 
+   /* Change a database if necessary */
+   if (mpvio.db.length)
+   {
+     if (mysql_change_db(thd, &mpvio.db, FALSE))
+     {
+       /* mysql_change_db() has pushed the error message. */
+       if (thd->user_connect)
+       {
+         decrease_user_connections(thd->user_connect);
+         status_var_increment(thd->status_var.access_denied_errors);
+         thd->user_connect= 0;
Move this just after the test (easier to read)
...
+       }
+       DBUG_RETURN(1);
+     }
+   }
...
+ static int old_password_authenticate(MYSQL_PLUGIN_VIO *vio, 
+                                            MYSQL_SERVER_AUTH_INFO *info)
+ {
+   uchar *pkt;
+   int pkt_len;
+   MPVIO_EXT *mpvio=(MPVIO_EXT*)vio;
+   THD *thd=mpvio->thd;
+ 
+   if (thd->scramble[SCRAMBLE_LENGTH])
+   {
+     /* no scramble was sent to the client yet, do it now */
+     create_random_string(thd->scramble,
+                          pkt_len= SCRAMBLE_LENGTH, &thd->rand);
+     pkt= (uchar*)thd->scramble;
+     if (mpvio->write_packet(mpvio, pkt, pkt_len))
+       return CR_ERROR;
+   }
+ 
+   /* ok, the client has got the scramble. read the reply and authenticate */
+   if ((pkt_len= mpvio->read_packet(mpvio, &pkt)) < 0)
+     return CR_ERROR;
+ 
+ #ifdef NO_EMBEDDED_ACCESS_CHECKS
+   return CR_OK;
+ #endif
+ 
+   /*
+     legacy: if switch_from_long_to_short_scramble,
+     the password is sent \0-terminated, the pkt_len is always 9 bytes.
+     We need to figure out the correct scramble length here.
+   */
+   if (pkt_len == SCRAMBLE_LENGTH_323+1)
+     pkt_len= strnlen((char*)pkt, pkt_len);
+   if (pkt_len == 0) /* no password */
+     return info->auth_string[0] ? CR_ERROR : CR_OK;
If we want to do things like in the old code, we should here check the
packet length.

if (pkt_len != SCRAMBLE_LENGTH_323)
{
  inc_host_errors(&mpvio->thd->net.vio->remote.sin_addr);
  my_error(ER_HANDSHAKE_ERROR, MYF(0), thd->main_security_ctx.host_or_ip);
  return CR_ERROR;
}
...
+     If the server is running in secure auth mode, short scrambles are 
+     forbidden. Extra juggling to report the same error as the old code.
+   */
+   if (opt_secure_auth)
+   {
+     if (thd->client_capabilities & CLIENT_PROTOCOL_41)
+     {
+       my_error(ER_SERVER_IS_IN_SECURE_AUTH_MODE, MYF(0),
+                thd->security_ctx->user,
+                thd->security_ctx->host_or_ip);
+       general_log_print(thd, COM_CONNECT, ER(ER_SERVER_IS_IN_SECURE_AUTH_MODE),
+                         thd->security_ctx->user,
+                         thd->security_ctx->host_or_ip);
+     }
I think the above error should be tested for and given when we ask for
old scramble.  This error is to be given when we get an new format
scramble at start when database has old.

Now we are likely to have a problem that if client sends new password
and server has old, we will request for old one, even if
opt_secure_auth is set, which is not how things worked before.
...
+     else
+     {
+       my_error(ER_NOT_SUPPORTED_AUTH_MODE, MYF(0));
+       general_log_print(thd, COM_CONNECT, ER(ER_NOT_SUPPORTED_AUTH_MODE));
+     }
+     return CR_ERROR;
+   }
The old code (sql_connect.cc:check_user()) tested first for:

  if (opt_secure_auth_local && passwd_len == SCRAMBLE_LENGTH_323)

And the other error condition next.

Here things are reversed so we may get other errors than from MySQL.
...
+ 
+   info->password_used = 1;
+ 
+   if (pkt_len == SCRAMBLE_LENGTH_323)
+     return check_scramble_323(pkt, thd->scramble,
+                              (ulong *)mpvio->acl_user->salt) ? CR_ERROR : CR_OK;
+ 
+   inc_host_errors(&mpvio->thd->net.vio->remote.sin_addr);
+   my_error(ER_HANDSHAKE_ERROR, MYF(0), thd->main_security_ctx.host_or_ip);
+   return CR_ERROR;
If we do the early check, we should of course remove the above if and
the error reporting
...
+ }
<cut>
...
*** sql/sql_insert.cc	2010-02-01 06:14:12 +0000
--- sql/sql_insert.cc	2010-02-19 08:18:09 +0000
*************** public:
*** 1769,1776 ****
       table(0),tables_in_use(0),stacked_inserts(0), status(0), dead(0),
       group_count(0)
    {
!     thd.security_ctx->user=thd.security_ctx->priv_user=(char*) delayed_user;
      thd.security_ctx->host=(char*) my_localhost;
      thd.current_tablenr=0;
      thd.version=refresh_version;
      thd.command=COM_DELAYED_INSERT;
--- 1769,1778 ----
       table(0),tables_in_use(0),stacked_inserts(0), status(0), dead(0),
       group_count(0)
    {
!     thd.security_ctx->user=(char*) delayed_user;
      thd.security_ctx->host=(char*) my_localhost;
+     strmake(thd.security_ctx->priv_user, thd.security_ctx->user,
+             USERNAME_LENGTH);
A little better would be to use delayed_user above, but not that important.
...
=== modified file 'sql/sql_parse.cc'
*** sql/sql_parse.cc	2010-02-01 06:14:12 +0000
    case COM_CHANGE_USER:
    {
      status_var_increment(thd->status_var.com_other);
thd->change_user();
      thd->clear_error();                         // if errors from rollback
!     /* acl_authenticate() takes the data from net->read_pos */
!     net->read_pos= (uchar*)packet;
!     uint save_db_length= thd->db_length;
!     char *save_db= thd->db;
!     USER_CONN *save_user_connect= thd->user_connect;
      Security_context save_security_ctx= *thd->security_ctx;
!     CHARSET_INFO *save_character_set_client=
!       thd->variables.character_set_client;
!     CHARSET_INFO *save_collation_connection=
!       thd->variables.collation_connection;
!     CHARSET_INFO *save_character_set_results=
!       thd->variables.character_set_results;
Please move declarations to start of case (should make the above one liners)
...
!     if (acl_authenticate(thd, 0, packet_length))
      {
        x_free(thd->security_ctx->user);
        *thd->security_ctx= save_security_ctx;
        thd->user_connect= save_user_connect;
        thd->db= save_db;
        thd->db_length= save_db_length;
Better to use:
       thd->reset_db(save_db, save_db_length);
...
+       thd->variables.character_set_client= save_character_set_client;
+       thd->variables.collation_connection= save_collation_connection;
+       thd->variables.character_set_results= save_character_set_results;
+       thd->update_charset();
      }
      else
      {
*** 4348,4355 ****
          if (sp_grant_privileges(thd, lex->sphead->m_db.str, name,
                                  lex->sql_command == SQLCOM_CREATE_PROCEDURE))
            push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
!                        ER_PROC_AUTO_GRANT_FAIL,
!                        ER(ER_PROC_AUTO_GRANT_FAIL));
        }
/*
--- 4280,4287 ----
          if (sp_grant_privileges(thd, lex->sphead->m_db.str, name,
                                  lex->sql_command == SQLCOM_CREATE_PROCEDURE))
            push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
!                        ER_PROC_AUTO_GRANT_FAIL, ER(ER_PROC_AUTO_GRANT_FAIL));
!         thd->clear_error();
Don't understand why this is needed now, but I assume this isn't
critical to understand (except maybe it would be nice to ensure that
this is correct if sp_grant_privileges did get a out-of-memory error)

<cut>

Other things.

You need to add creation of auth_string to:
scripts/mysql_fix_privilege_tables.sql

In old_password_auth_client(), if we get an error from write_packet,

it would be good to do:
  set_mysql_extended_error(mysql, CR_SERVER_LOST, unknown_sqlstate,
                           ER(CR_SERVER_LOST_EXTENDED),
                           "sending password information",
                           errno);

See if you can combine common code from parse_com_change_user_packet()
and parse_client_handshake_packet().

It would also be nice to avoid comparison of strings and even string
pointers and instead compare objects (like pointer to plugin) and
values in plugin object (like if it's native or not).

Other than the above suggestions, it looked good and definitely
something we need!

Regards,
Monty

Re: [Maria-developers] Review of 5.2 pluggable-auth tree

Michael Widenius